Neels Hofmeyr 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: (13 comments) https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/gsm_data.h@518 PS6, Line 518: uint8_t error_cause; > might be good to mention that this refers to RSL cause values? (would be nice to have an RTP error enum instead of #defines) 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? inter-MO means we are handover-ing an lchan to another BSC. In my current wording, a HO always has an MO and an MT side; MO is where the MS is, and MT is where the MS is supposed to carry on after the HO. In intra-BSC, one cell is the MO and another is the MT cell. In inter-BSC HO, one BSC is the MO and another is the MT BSC. MO: the MS sent a measurement report, so the handover reason is mobile-originated. MT: we are asking an MS to respond on a prepared lchan, kind of like how paging is mobile-terminated. (it does make consistent sense to me, but different terminology pending...) 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 […] The current master branch code does not ever perform a Chan Mode Modify, so neither does this refactored code. It is relatively trivial to add it, but I decided to not add more to this refactoring. For example: we gave an MS a TCH/F lchan for initial Channel Request. Now a BSSMAP Assignment Command tells us to assign a TCH/F to the MS. We don't need to find a new unused lchan, we can just use the assigned one. All that's needed is to send a Chan Mode Modify to the BTS and go through the FSM states that set up the RTP stream. But we don't do that yet; so far, instead we look for a completely new lchan. (If the lchan mode already matches, we do use the current lchan. See assignment_fsm_start() in assignment_fsm.c ) 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? […] Adding a comment to explain. Thanks for spotting the cause, it got lost when I added rsl_cause_name(), also in other places; will fix. 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. […] The drawback there is that if an assertion hits, we will get the line number of the one common function, and to find out which of the N handling functions had the problem, we need to go and do a traceback first. It's a common pattern I'm using in FSM implementations. But can change that if you prefer. 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. in principle i agree; but the resulting code would be bloaty and easy to forget. With this though no fsm definition is registered until an fi first turns up; is that a good thing or a bad thing? (thinking ctrl interface) (I think I copied this pattern from somewhere else, actually) https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c@1727 PS6, Line 1727: DEFUN(handover_ext_cgi, > we should probably have a FIXME here actually i think i should keep this bit shelved on a private branch yet... https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c File src/osmo-bsc/gsm_data.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1536 PS6, Line 1536: static enum gsm0808_permitted_speech audio_support_to_gsm88(const struct gsm_audio_support *audio) > static helper functions here and below also don't seem gsm_data. […] used in osmo_bsc_bssap.c and handover_fsm.c. The point is that handover_fsm.c is used in handover_test.c, which does explicitly want to avoid linking osmo_bsc_bssap.c with its dependencies on sigtran. Since they are little more than converting enum values based on libosmocore API, I decided to just drop it here. I could create a new file just for gsm0808_permitted_speech() and audio_support_to_gsm88() ... yes? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1715 PS6, Line 1715: osmo_ntohs > we normally only use osmo_ntohs() in situations where it's not IP/socket > related and hence htons() m […] I merely copied this code from the current-master assignment code paths... so, what, this should use ntohs() instead? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c File src/osmo-bsc/handover_decision.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c@203 PS6, Line 203: { > style-wise, I would generally prefer the variable declaration at the top of > the function, rather tha […] I want to highlight that the local struct exists merely to pass parameters to the function call, and then be gone. Style wise it's the same reason as when creating a variable in an if-/for-/while-body --- just without the if/for/while... If you don't stop me I will use this every now and then, so if you still disagree after my reasoning I can bite my lip and drop it. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c File src/osmo-bsc/handover_fsm.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@644 PS6, Line 644: return result_counter_INTER_BSC_HO_MT(result); > two return statements in one case? very interesting indeed. copy paste artifact... https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c File src/osmo-bsc/lchan_select.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c@1 PS6, Line 1: /* Select a suitable lchan from a given cell. > I guess this is code we traditionally would have had in chan_alloc. […] This is not allocating an lchan, it is looking for an unused lchan. IMO chan_alloc.c has always been a misnomer... lchan_alloc() used to lead more or less directly into an RSL Chan Activ. Now I see it further removed as a separate preliminary step; the {assignment,handover,...} FSM using the lchan might still decide to not use the lchan after all, e.g. if the conn FSM is in the wrong state and denies {assignment,handover,...}; and the point where we start really "allocating" the lchan (which I understand as Chan Activ and establishing RLL) is wayy further down the road. I wanted to get rid of that misunderstanding. This really and only just selects an unused lchan that might get allocated -- or not. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c File src/osmo-bsc/osmo_bsc_api.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c@189 PS6, Line 189: no-one is using this > this is odd? any service we don't know / support should be rejected. […] i _think_ we just forward the service to the MSC regardless?? ... but beats me. If it should change, then probably in a separate patch. -- 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: Tue, 26 Jun 2018 02:55:08 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
