dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/16170 )
Change subject: rsl: ensure measurement reports are sent ...................................................................... Patch Set 6: (6 comments) This change is ready for review. https://gerrit.osmocom.org/c/osmo-bts/+/16170/1/src/common/l1sap.c File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/16170/1/src/common/l1sap.c@1252 PS1, Line 1252: return -EINVAL; > That means in this case lchan_ms_pwr_ctrl() is not called and the MS power > loop is missing informati […] 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 https://gerrit.osmocom.org/c/osmo-bts/+/16170/3/src/common/l1sap.c File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/16170/3/src/common/l1sap.c@1246 PS3, Line 1246: le = &lchan->lapdm_ch.lapdm_acch; > Why restrict this to tch? sdcch do have a sacch, too. […] I have changed it now but I do not know if this is now correct. see comment in scheduler.c https://gerrit.osmocom.org/c/osmo-bts/+/16170/4/src/common/rsl.c File src/common/rsl.c: https://gerrit.osmocom.org/c/osmo-bts/+/16170/4/src/common/rsl.c@2905 PS4, Line 2905: if (l3 && l3_len > 0) > How RSL_IE_MS_TIMING_OFFSET is related to the presence of l3? Am I missing > something? 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. 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." >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. https://gerrit.osmocom.org/c/osmo-bts/+/16170/1/src/common/scheduler.c File src/common/scheduler.c: https://gerrit.osmocom.org/c/osmo-bts/+/16170/1/src/common/scheduler.c@194 PS1, Line 194: rx_tchf_fn > How about TCH/H? […] 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. 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. https://gerrit.osmocom.org/c/osmo-bts/+/16170/3/src/common/scheduler.c File src/common/scheduler.c: https://gerrit.osmocom.org/c/osmo-bts/+/16170/3/src/common/scheduler.c@368 PS3, Line 368: .nope_fn = rx_data_fn, > Again, why only SACCH/TF? We also have SACCH on TCH/H and on SDCCH... First of all for TRXC_SACCHTH_0 and TRXC_SACCHTH_1 I indeed forgot to put the .nope_fn. 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? https://gerrit.osmocom.org/c/osmo-bts/+/16170/4/src/common/scheduler.c File src/common/scheduler.c: https://gerrit.osmocom.org/c/osmo-bts/+/16170/4/src/common/scheduler.c@430 PS4, Line 430: TRXC_SACCH4_3 > This is also SACCH, please assign .nope_fn here too. Done -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/16170 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Idfa8ef94e8cf131ff234dac8f93f337051663ae2 Gerrit-Change-Number: 16170 Gerrit-PatchSet: 6 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <[email protected]> Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Wed, 18 Dec 2019 10:10:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Comment-In-Reply-To: laforge <[email protected]> Gerrit-MessageType: comment
