Attention is currently required from: laforge, neels, pespin.
lynxis lazus has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/osmo-msc/+/38490?usp=email )
Change subject: vlr: add PS support ......................................................................
Patch Set 5:
(7 comments)
File src/libvlr/vlr.c:
https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/af3cff48_9c583ff3?usp=... : PS5, Line 1490: return osmo_fsm_inst_dispatch(vsub->lu_fsm,
can go in same line.
Done
File src/libvlr/vlr_lu_fsm.c:
https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/3759abb5_720d31c8?usp=... : PS5, Line 357: *!
about the doxygen format comment: […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/0fab90b5_12deb888?usp=... : PS5, Line 589: LOGPFSML(fi, LOGL_ERROR, "LU Compl timeout %d / %d\n", fi->T, lcvp->N);
let's have "T%d / N%d"
Done
https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/c79d807a_bbd82098?usp=... : PS5, Line 598: break;
(we often have a default: OSMO_ASSERT(false) in these to ensure we notice enum bugs)
in timeouts?
https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/c3348b61_547ac77a?usp=... : PS5, Line 771: /*! count times timer T timed out */
.
I would like to replace the words "count" and "out" with a word starting with t. Any ideas?
Not sure if this comment can be more cleared, because the spec depends on this knowledge anyways.
https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/e6dfe964_6b4c48b9?usp=... : PS5, Line 1597: mian
typo
Done
https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/85d503e9_20c3c779?usp=... : PS5, Line 1611: fi->T
technically, middle arg should be 0 or -1 or so, because vlr_is_cs() == false from above. […]
In this case, you could also pass -1 to it. We could use `osmo_tdef_get(vlr_tdefs, ps_timer, OSMO_TDEF_S, 0);`, not sure if it makes it clearer the code instead of using vlr_timer_secs() as we did in all other places. vlr_timer_secs() is usually used by fsm_state_chg() and you pass the CS and PS timer values of the state into it, because CS has different Ts than PS.
-
lynxis lazus