<p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309">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-pcu/+/22309/3/src/gprs_bssgp_pcu.c">File src/gprs_bssgp_pcu.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-pcu/+/22309/3/src/gprs_bssgp_pcu.c@213">Patch Set #3, Line 213:</a> <code style="font-family:monospace,monospace">       /* FIXME: look if MS is attached a specific BTS and then only page on that one? */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Sounds like looping over the attached BTS and their MS would solve it. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes it's out of the scope of this patch. This patch is not aiming at properly supporting multibts at runtime, simply adapting the code architecture for allowing it in the future. In the only event contemplated here (1 BTS), looping does 1 iteration so no change in behavior.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This patch is already too big to try to do more stuff :)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/gprs_bssgp_pcu.c@823">Patch Set #3, Line 823:</a> <code style="font-family:monospace,monospace">      bts = llist_first_entry_or_null(&the_pcu->bts_list, struct gprs_rlcmac_bts, list);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why not add bts as parameter to gprs_bssgp_pcu_rx_ptp()?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">because caller of this function, bvc_timeout(), is called in lots of places with NULL param where specific BTS object is not available.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In any case, I put this here because this function is already super fucked up and should be completely rewritten independently of this patch (see FIXME below). I also remember myself seeing the calculations were wrong for other reasons in the past.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So, to keep old behavior, the easiest here is to take the first BTS in list.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/osmo-bts-litecell15/lc15_l1_if.c">File src/osmo-bts-litecell15/lc15_l1_if.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-pcu/+/22309/3/src/osmo-bts-litecell15/lc15_l1_if.c@160">Patch Set #3, Line 160:</a> <code style="font-family:monospace,monospace">  bts = llist_first_entry_or_null(&the_pcu->bts_list, struct gprs_rlcmac_bts, list);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">add bts parameter instead, or at least a FIXME comment like above? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's not a current limitation. If we are using the direct_phy backend, it means we are attached to the BTS directly, so there's only 1 BTS announced in PCUIF, and hence taking the first one is fine (because it's the only one to be ever available).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Same applies for file sysmo_l1_if.c, oc2g_l1_if.c.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/osmobts_sock.c">File src/osmobts_sock.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-pcu/+/22309/4/src/osmobts_sock.c@64">Patch Set #4, Line 64:</a> <code style="font-family:monospace,monospace">   bool retry = !llist_empty(&the_pcu->bts_list);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think the ! is wrong, shouldn't retry be true if the list is empty?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indeed, thanks!<br>In general is not a big issue because pcu_tx_txt_ind() is sent during successful port open in pcu_l1if_open(), but indeed you are right.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/osmobts_sock.c@114">Patch Set #4, Line 114:</a> <code style="font-family:monospace,monospace">        llist_for_each_entry(bts, &the_pcu->bts_list, list) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">My understanding is, that if one bts closes the socket to the pcu, the pcu will give up completely,  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">There's only 1 PCUIF unix socket, which can be connected to a BTS or a BSC. In the later, BSC sends several info_ind, one for each BTS. But in this patch, when the unix socket is closed, we want to drop all BTS.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_l1_if.h">File src/pcu_l1_if.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-pcu/+/22309/4/src/pcu_l1_if.h@159">Patch Set #4, Line 159:</a> <code style="font-family:monospace,monospace">struct gprs_rlcmac_bts;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">its declared above already in the "ifdef __cplusplus" section. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, because I need it on top of the pcu_l1if_tx_* functions, but if a C file is including this header, then I also need to put the struct gprs_rlcmac_bts here since the block above will not be seen by it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I agree some headers may look a bit messy but I expect them to become cleaner as more and more helper classes are moved to C over time.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_l1_if.cpp">File src/pcu_l1_if.cpp:</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-pcu/+/22309/4/src/pcu_l1_if.cpp@845">Patch Set #4, Line 845:</a> <code style="font-family:monospace,monospace">       bts = gprs_pcu_get_bts_by_nr(the_pcu, pcu_prim->bts_nr);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this is called twice: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_vty.c">File src/pcu_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/osmo-pcu/+/22309/4/src/pcu_vty.c@1073">Patch Set #4, Line 1073:</a> <code style="font-family:monospace,monospace">              pcu_vty_show_tbf_all(vty, bts, flags);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Print out the BTS number too? Otherwise it will just be "UL TBFs", "DL TBFs", "UL TBFs", ...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Which is fine since I don't want to change current behavior in this code. This is printing TBFs, not BTS and its associated TBFs. If at all, one should decide whether it makes sense to print the BTS number inside each TBF block in pcu_vty_show_tbf_all().</p><p style="white-space: pre-wrap; word-wrap: break-word;">But in any case, I'm not willing to change that here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_vty.c@1085">Patch Set #4, Line 1085:</a> <code style="font-family:monospace,monospace">            pcu_vty_show_ms_all(vty, bts);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How about extending show_ms() to mention the BTS number?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same reason as above,this lists MS, I don't plan to change behavior here.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22309">change 22309</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-pcu/+/22309"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I6b10913f46c19d438c4e250a436a7446694b725a </div>
<div style="display:none"> Gerrit-Change-Number: 22309 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 19 Jan 2021 16:59:45 +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: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>