Change in osmo-bsc[master]: lchan_fsm: make rsl mode-modify working again

[dexter]

dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/19792 )

Change subject: lchan_fsm: make rsl mode-modify working again
......................................................................


Patch Set 2:

(10 comments)

This change is ready for review.

https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/include/osmocom/bsc/lchan_fsm.h 
File include/osmocom/bsc/lchan_fsm.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/include/osmocom/bsc/lchan_fsm.h@21 
PS1, Line 21: LCHAN_ST_WAIT_CHAN_MODE_MODIF_ACK,
            : 	LCHAN_ST_WAIT_RSL_MT_MODE_MODIFY_ACK
> the first one is about RR  to the UE, the second one is about RSL to the BTS. […]
The names are derived from what I have found in abis_rsl_rx_dchan() in abis_rsl.c but the naming is indeed wired. I think I have found better names now.


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/include/osmocom/bsc/lchan_fsm.h@60 
PS1, Line 60: void lchan_modfy(struct gsm_lchan *lchan, struct lchan_activate_info *info);
> "lchan_mode_modify" with 'mode_' and 'i' please
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c 
File src/osmo-bsc/assignment_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c@442 
PS1, Line 442: 		 * however, this will be not the common case. */
> (maybe rather drop the "however... […]
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c@483 
PS1, Line 483: 		 * still the old lchan. */
> (use width of 120?)
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c@486 
PS1, Line 486: 		/* Also we need to spare the RR assignment, so we jump forward
> s/spare/skip
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c 
File src/osmo-bsc/lchan_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@539 
PS1, Line 539: 		goto abort;
> to match the way I usually arranged these FSMs, you would: […]
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@570 
PS1, Line 570: 	gsm48_lchan_modify(lchan, info->chan_mode);
> doing things after a state change is dangerous business, FSMs have often bitten me like that because […]
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@894 
PS1, Line 894: 		lchan_fsm_state_chg(LCHAN_ST_WAIT_RSL_MT_MODE_MODIFY_ACK);
> same thing, rather place the sending action in the onenter function. […]
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@922 
PS1, Line 922: 			lchan_fsm_state_chg(LCHAN_ST_WAIT_RLL_RTP_ESTABLISH);
> onenter
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@923 
PS1, Line 923: 			lchan_rtp_fsm_start(lchan);
> have you looked at the case where an lchan may already be an active voice call when the mode changes […]
I thought of this yes, but I think this is not a real life scenario. A voice call usually runs until it is terminated, I do not know a scenario where a voice call is modified back to signalling.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/19792
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I2c5a283b1ee33745cc1fcfcc09a0f9382224e2eb
Gerrit-Change-Number: 19792
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Comment-Date: Tue, 01 Sep 2020 14:51:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy at sysmocom.de>
Comment-In-Reply-To: neels <nhofmeyr at sysmocom.de>
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200901/fec12bc5/attachment.htm>