<p style="white-space: pre-wrap; word-wrap: break-word;">Nice work!!<br>Looking forward to get this merged.</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/include/osmocom/bsc/lchan_fsm.h">File include/osmocom/bsc/lchan_fsm.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/include/osmocom/bsc/lchan_fsm.h@60">Patch Set #1, Line 60:</a> <code style="font-family:monospace,monospace">void lchan_modfy(struct gsm_lchan *lchan, struct lchan_activate_info *info);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"lchan_mode_modify" with 'mode_' and 'i' please</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c">File src/osmo-bsc/assignment_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c@442">Patch Set #1, Line 442:</a> <code style="font-family:monospace,monospace"> * however, this will be not the common case. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(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)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c@483">Patch Set #1, Line 483:</a> <code style="font-family:monospace,monospace"> * still the old lchan. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(use width of 120?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/assignment_fsm.c@486">Patch Set #1, Line 486:</a> <code style="font-family:monospace,monospace"> /* Also we need to spare the RR assignment, so we jump forward</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/spare/skip</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c">File src/osmo-bsc/lchan_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@539">Patch Set #1, Line 539:</a> <code style="font-family:monospace,monospace"> goto abort;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">to match the way I usually arranged these FSMs, you would:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Dispatch a new event like</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> LCHAN_EV_REQUEST_MODE_MODIFY</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and pass the activate.info pointer as event argument pointer.<br>(If the state change returns error, go into lchan_fail())</p><p style="white-space: pre-wrap; word-wrap: break-word;">Then the FSM definition can decide whether the event is permitted or not, i.e. this event will only be allowed on state ESTABLISHED.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In the ESTABLISHED action I would just state-change into LCHAN_ST_WAIT_CHAN_MODE_MODIF_ACK,<br>and the rest of the code of this function would go into that state's onenter function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@570">Patch Set #1, Line 570:</a> <code style="font-family:monospace,monospace"> gsm48_lchan_modify(lchan, info->chan_mode);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@894">Patch Set #1, Line 894:</a> <code style="font-family:monospace,monospace"> lchan_fsm_state_chg(LCHAN_ST_WAIT_RSL_MT_MODE_MODIFY_ACK);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same thing, rather place the sending action in the onenter function.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(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)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@922">Patch Set #1, Line 922:</a> <code style="font-family:monospace,monospace"> lchan_fsm_state_chg(LCHAN_ST_WAIT_RLL_RTP_ESTABLISH);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">onenter</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792/1/src/osmo-bsc/lchan_fsm.c@923">Patch Set #1, Line 923:</a> <code style="font-family:monospace,monospace"> lchan_rtp_fsm_start(lchan);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">have you looked at the case where an lchan may already be an active voice call when the mode changes?<br>In that case the RTP FSM would need to tear down and/or modify existing MGW endpoints.<br>I guess that case is uncommon, so we could also fail the mode-modify early if RTP is already active?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792">change 19792</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I2c5a283b1ee33745cc1fcfcc09a0f9382224e2eb </div>
<div style="display:none"> Gerrit-Change-Number: 19792 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 24 Aug 2020 11:18:35 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>