Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/9671 )
Change subject: large refactoring: use FSMs for lchans; add inter-BSC HO ...................................................................... Patch Set 6: (6 comments) https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/handover_fsm.h File include/osmocom/bsc/handover_fsm.h: https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/handover_fsm.h@42 PS6, Line 42: inter-MO FSM what's inter-MO ? two mobile originated (outbound) hand-overs? https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/lchan_fsm.h File include/osmocom/bsc/lchan_fsm.h: https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/lchan_fsm.h@53 PS6, Line 53: /* FIXME: not yet implemented: Chan Mode Modify */ does this mean that channel mode modify still bypasses the FSM or that this functionality is missing/broken? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/abis_rsl.c File src/osmo-bsc/abis_rsl.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/abis_rsl.c@885 PS6, Line 885: if (!lchan->conn) { when do we expect this? Is it a normal event? should we log it? The "cause" variable is initialized once to the static value of "0" above and hence could be removed. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c File src/osmo-bsc/assignment_fsm.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c@44 PS6, Line 44: #define GET_CONN() \ I'm sorry, but I don't like the idea of hiding a variable declaration in a macro. I don't think we have any precedent for that. I think This could well become: struct gsm_subscriber_connection *conn = ass_fi_get_conn(fi); at the caller site, with the ass_fi_get_conn() then doing the dereference and ASSERTs and returning the pointer. The ass_fi_geet_conn() name is just an example, not set in stone. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c@48 PS6, Line 48: struct state_timeout assignment_fsm_timeouts[32] = { is either of tose possible here: const? static? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c@294 PS6, Line 294: static bool g_initialized = false; : if (!g_initialized) { : OSMO_ASSERT(osmo_fsm_register(&assignment_fsm) == 0); : g_initialized = true; : } I'm much more in favor of an explicit registration somewhere. But not critical. -- To view, visit https://gerrit.osmocom.org/9671 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82e3f918295daa83274a4cf803f046979f284366 Gerrit-Change-Number: 9671 Gerrit-PatchSet: 6 Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-CC: Harald Welte <[email protected]> Gerrit-Comment-Date: Mon, 25 Jun 2018 16:18:19 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
