Change in osmo-bsc[master]: large refactoring: use FSMs for lchans; add inter-BSC HO

historical

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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-Comment-Date: Tue, 26 Jun 2018 02:55:08 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180626/79c6db66/attachment.htm>