This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
neels gerrit-no-reply at lists.osmocom.orgneels 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 1: Code-Review-1 (9 comments) Nice work!! Looking forward to get this merged. 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@60 PS1, Line 60: void lchan_modfy(struct gsm_lchan *lchan, struct lchan_activate_info *info); "lchan_mode_modify" with 'mode_' and 'i' please 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..." line, not sure how commonly this will be used, could also play a role in handover-related channel re-assignment) 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?) 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 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: Dispatch a new event like LCHAN_EV_REQUEST_MODE_MODIFY and pass the activate.info pointer as event argument pointer. (If the state change returns error, go into lchan_fail()) Then the FSM definition can decide whether the event is permitted or not, i.e. this event will only be allowed on state ESTABLISHED. In the ESTABLISHED action I would just state-change into LCHAN_ST_WAIT_CHAN_MODE_MODIF_ACK, and the rest of the code of this function would go into that state's onenter function. 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 a state change might be doing some error handling and discarding the object. Rather use the state's onenter function. The second theoretical reason for using onenter is that a state change may be caused from multiple places, and the onenter makes sure that each separate caller for that state change has the same actions without needing code dup. Even if there is only one invocation, it would be nice to stay in the same pattern. 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. (the theoretical reason underneath is that a state change may be caused from multiple places, and the onenter makes sure that each separate caller for that state change has the same actions without needing code dup. Even if there is only one invocation, it would be nice to stay in the same pattern) 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 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? In that case the RTP FSM would need to tear down and/or modify existing MGW endpoints. I guess that case is uncommon, so we could also fail the mode-modify early if RTP is already active? -- 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: 1 Gerrit-Owner: dexter <pmaier at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge at osmocom.org> Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de> Gerrit-CC: fixeria <vyanitskiy at sysmocom.de> Gerrit-Comment-Date: Mon, 24 Aug 2020 11:18:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200824/0ffae656/attachment.htm>