<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-bts/+/16170">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/16170/1/src/common/l1sap.c">File src/common/l1sap.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-bts/+/16170/1/src/common/l1sap.c@1252">Patch Set #1, Line 1252:</a> <code style="font-family:monospace,monospace">                return -EINVAL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">That means in this case lchan_ms_pwr_ctrl() is not called and the MS power loop is missing informati […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is indeed a problem but it is unrelated to this patch. The bug exists for longer. I have created a ticket now: https://osmocom.org/issues/4281</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/16170/3/src/common/l1sap.c">File src/common/l1sap.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-bts/+/16170/3/src/common/l1sap.c@1246">Patch Set #3, Line 1246:</a> <code style="font-family:monospace,monospace">                            le = &lchan->lapdm_ch.lapdm_acch;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why restrict this to tch? sdcch do have a sacch, too. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have changed it now but I do not know if this is now correct. see comment in scheduler.c</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/16170/4/src/common/rsl.c">File src/common/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/osmo-bts/+/16170/4/src/common/rsl.c@2905">Patch Set #4, Line 2905:</a> <code style="font-family:monospace,monospace">if (l3 && l3_len > 0)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How RSL_IE_MS_TIMING_OFFSET is related to the presence of l3? Am I missing something?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am not entirely sure, but when I include it and the L3 Info is missing then wireshark complains about malformed packets. I have looked at the hexdump, it looks ok.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In 3GPP TS 08.58 version 8.6.0: I can read: "MS Timing Offset can be optionally included to increase the accuracy of possible distance measurements."</p><p style="white-space: pre-wrap; word-wrap: break-word;">From the spec I can not see if it is related to L3 Info but since the IE is optional I think we can just skip it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/16170/1/src/common/scheduler.c">File src/common/scheduler.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-bts/+/16170/1/src/common/scheduler.c@194">Patch Set #1, Line 194:</a> <code style="font-family:monospace,monospace">rx_tchf_fn</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How about TCH/H? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We do not need TCH/H and TCH/F. The reason for this is that only the SACCH is crucial to keep the measurement processing going. Missing TCH measurements are compensated by the code that calculates the result.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I did not know that the buffer in the NOPE.ind is uninitalized. For rx_data_fn I now check on bi->burst_len to see if there is data or not. If there is no data I make sure that the buffer is initalized. This is for sure not perfect. Actually we should indeed skip all decoding steps if one of the bursts (or two, i do not know when the redundancy is exhausted) is bad, but I do not think that the CPU time we would safe would be all that much as it is only the SACCH that is affected here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/16170/3/src/common/scheduler.c">File src/common/scheduler.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-bts/+/16170/3/src/common/scheduler.c@368">Patch Set #3, Line 368:</a> <code style="font-family:monospace,monospace">.nope_fn = rx_data_fn,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Again, why only SACCH/TF? We also have SACCH on TCH/H and on SDCCH...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">First of all for TRXC_SACCHTH_0 and TRXC_SACCHTH_1 I indeed forgot to put the .nope_fn.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In the beginning I thought that limiting the mechanism to TCH channels only would be a good idea as I was experiencing problems with unrelated NOPE indications that where uncontrollably streaming in (I don't know how this happend). I have now extended everything to the other SACCH channels and it seems to work fine now. I don't know what went wrong last time. However I am still unsure if it is fully correct now. I do not know if the SACCH in uplink always carries measurement reports. On TCH/F and TCH/H i know each SACCH interval produces one measurement report but how about the other SACCH channels? Do we need some additional logic in l1sap.c to determine if the incoming bad SACCH did carry a measurement report?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/16170/4/src/common/scheduler.c">File src/common/scheduler.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-bts/+/16170/4/src/common/scheduler.c@430">Patch Set #4, Line 430:</a> <code style="font-family:monospace,monospace">TRXC_SACCH4_3</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is also SACCH, please assign .nope_fn here too.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bts/+/16170">change 16170</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-bts/+/16170"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bts </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Idfa8ef94e8cf131ff234dac8f93f337051663ae2 </div>
<div style="display:none"> Gerrit-Change-Number: 16170 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </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: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 18 Dec 2019 10:10:01 +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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>