<p style="white-space: pre-wrap; word-wrap: break-word;">This change is ready for review.</p><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19792">View Change</a></p><p>10 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@21">Patch Set #1, Line 21:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">LCHAN_ST_WAIT_CHAN_MODE_MODIF_ACK,<br> LCHAN_ST_WAIT_RSL_MT_MODE_MODIFY_ACK<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">the first one is about RR to the UE, the second one is about RSL to the BTS. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"lchan_mode_modify" with 'mode_' and 'i' please</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(maybe rather drop the "however... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(use width of 120?)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/spare/skip</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">to match the way I usually arranged these FSMs, you would: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">doing things after a state change is dangerous business, FSMs have often bitten me like that because […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">same thing, rather place the sending action in the onenter function. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">onenter</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">have you looked at the case where an lchan may already be an active voice call when the mode changes […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</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: 2 </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: dexter <pmaier@sysmocom.de> </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: Tue, 01 Sep 2020 14:51:38 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>