<p><a href="https://gerrit.osmocom.org/9671">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.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/9671/6/include/osmocom/bsc/gsm_data.h@518">Patch Set #6, Line 518:</a> <code style="font-family:monospace,monospace"> uint8_t error_cause;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">might be good to mention that this refers to RSL cause values?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(would be nice to have an RTP error enum instead of #defines)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/handover_fsm.h">File include/osmocom/bsc/handover_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/9671/6/include/osmocom/bsc/handover_fsm.h@42">Patch Set #6, Line 42:</a> <code style="font-family:monospace,monospace">inter-MO FSM </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what's inter-MO ? two mobile originated (outbound) hand-overs?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">inter-MO means we are handover-ing an lchan to another BSC.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In my current wording, a HO always has an MO and an MT side; MO is where the MS is, and MT is where the MS is supposed to carry on after the HO. In intra-BSC, one cell is the MO and another is the MT cell. In inter-BSC HO, one BSC is the MO and another is the MT BSC.</p><p style="white-space: pre-wrap; word-wrap: break-word;">MO: the MS sent a measurement report, so the handover reason is mobile-originated.</p><p style="white-space: pre-wrap; word-wrap: break-word;">MT: we are asking an MS to respond on a prepared lchan, kind of like how paging is mobile-terminated.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(it does make consistent sense to me, but different terminology pending...)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/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/9671/6/include/osmocom/bsc/lchan_fsm.h@53">Patch Set #6, Line 53:</a> <code style="font-family:monospace,monospace">/* FIXME: not yet implemented: Chan Mode Modify */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">does this mean that channel mode modify still bypasses the FSM or that this functionality is missing […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The current master branch code does not ever perform a Chan Mode Modify, so neither does this refactored code. It is relatively trivial to add it, but I decided to not add more to this refactoring.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For example: we gave an MS a TCH/F lchan for initial Channel Request. Now a BSSMAP Assignment Command tells us to assign a TCH/F to the MS. We don't need to find a new unused lchan, we can just use the assigned one. All that's needed is to send a Chan Mode Modify to the BTS and go through the FSM states that set up the RTP stream. But we don't do that yet; so far, instead we look for a completely new lchan. (If the lchan mode already matches, we do use the current lchan. See assignment_fsm_start() in assignment_fsm.c )</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/abis_rsl.c">File src/osmo-bsc/abis_rsl.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/9671/6/src/osmo-bsc/abis_rsl.c@885">Patch Set #6, Line 885:</a> <code style="font-family:monospace,monospace"> if (!lchan->conn) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">when do we expect this? Is it a normal event? should we log it? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Adding a comment to explain.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for spotting the cause, it got lost when I added rsl_cause_name(), also in other places; will fix.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/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/9671/6/src/osmo-bsc/assignment_fsm.c@44">Patch Set #6, Line 44:</a> <code style="font-family:monospace,monospace">#define GET_CONN() \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm sorry, but I don't like the idea of hiding a variable declaration in a macro. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The drawback there is that if an assertion hits, we will get the line number of the one common function, and to find out which of the N handling functions had the problem, we need to go and do a traceback first. It's a common pattern I'm using in FSM implementations. But can change that if you prefer.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c@294">Patch Set #6, Line 294:</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;"> static bool g_initialized = false;<br> if (!g_initialized) {<br> OSMO_ASSERT(osmo_fsm_register(&assignment_fsm) == 0);<br> g_initialized = true;<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm much more in favor of an explicit registration somewhere. But not critical.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">in principle i agree; but the resulting code would be bloaty and easy to forget. With this though no fsm definition is registered until an fi first turns up; is that a good thing or a bad thing? (thinking ctrl interface)</p><p style="white-space: pre-wrap; word-wrap: break-word;">(I think I copied this pattern from somewhere else, actually)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c">File src/osmo-bsc/bsc_vty.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/9671/6/src/osmo-bsc/bsc_vty.c@1727">Patch Set #6, Line 1727:</a> <code style="font-family:monospace,monospace">DEFUN(handover_ext_cgi,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we should probably have a FIXME here</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">actually i think i should keep this bit shelved on a private branch yet...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c">File src/osmo-bsc/gsm_data.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/9671/6/src/osmo-bsc/gsm_data.c@1536">Patch Set #6, Line 1536:</a> <code style="font-family:monospace,monospace">static enum gsm0808_permitted_speech audio_support_to_gsm88(const struct gsm_audio_support *audio)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">static helper functions here and below also don't seem gsm_data. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">used in osmo_bsc_bssap.c and handover_fsm.c. The point is that handover_fsm.c is used in handover_test.c, which does explicitly want to avoid linking osmo_bsc_bssap.c with its dependencies on sigtran. Since they are little more than converting enum values based on libosmocore API, I decided to just drop it here. I could create a new file just for gsm0808_permitted_speech() and audio_support_to_gsm88() ... yes?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1715">Patch Set #6, Line 1715:</a> <code style="font-family:monospace,monospace">osmo_ntohs</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we normally only use osmo_ntohs() in situations where it's not IP/socket related and hence htons() m […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I merely copied this code from the current-master assignment code paths... so, what, this should use ntohs() instead?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c">File src/osmo-bsc/handover_decision.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/9671/6/src/osmo-bsc/handover_decision.c@203">Patch Set #6, Line 203:</a> <code style="font-family:monospace,monospace">{</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">style-wise, I would generally prefer the variable declaration at the top of the function, rather tha […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I want to highlight that the local struct exists merely to pass parameters to the function call, and then be gone. Style wise it's the same reason as when creating a variable in an if-/for-/while-body --- just without the if/for/while... If you don't stop me I will use this every now and then, so if you still disagree after my reasoning I can bite my lip and drop it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c">File src/osmo-bsc/handover_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/9671/6/src/osmo-bsc/handover_fsm.c@644">Patch Set #6, Line 644:</a> <code style="font-family:monospace,monospace"> return result_counter_INTER_BSC_HO_MT(result);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">two return statements in one case?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">very interesting indeed. copy paste artifact...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c">File src/osmo-bsc/lchan_select.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/9671/6/src/osmo-bsc/lchan_select.c@1">Patch Set #6, Line 1:</a> <code style="font-family:monospace,monospace">/* Select a suitable lchan from a given cell.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I guess this is code we traditionally would have had in chan_alloc. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not allocating an lchan, it is looking for an unused lchan. IMO chan_alloc.c has always been a misnomer... lchan_alloc() used to lead more or less directly into an RSL Chan Activ. Now I see it further removed as a separate preliminary step; the {assignment,handover,...} FSM using the lchan might still decide to not use the lchan after all, e.g. if the conn FSM is in the wrong state and denies {assignment,handover,...}; and the point where we start really "allocating" the lchan (which I understand as Chan Activ and establishing RLL) is wayy further down the road. I wanted to get rid of that misunderstanding. This really and only just selects an unused lchan that might get allocated -- or not.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c">File src/osmo-bsc/osmo_bsc_api.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/9671/6/src/osmo-bsc/osmo_bsc_api.c@189">Patch Set #6, Line 189:</a> <code style="font-family:monospace,monospace">no-one is using this</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this is odd? any service we don't know / support should be rejected. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i _think_ we just forward the service to the MSC regardless?? ... but beats me. If it should change, then probably in a separate patch.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/9671">change 9671</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/9671"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I82e3f918295daa83274a4cf803f046979f284366 </div>
<div style="display:none"> Gerrit-Change-Number: 9671 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 26 Jun 2018 02:55:08 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>