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 <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pma...@sysmocom.de> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-CC: fixeria <vyanits...@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 <vyanits...@sysmocom.de> Comment-In-Reply-To: neels <nhofm...@sysmocom.de> Comment-In-Reply-To: laforge <lafo...@osmocom.org> Gerrit-MessageType: comment
