dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/15918 )
Change subject: l1sap: merge MEAS IND into PRIM PH DATA / PRIM TCH ...................................................................... Patch Set 9: (5 comments) https://gerrit.osmocom.org/c/osmo-bts/+/15918/7/src/common/l1sap.c File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/15918/7/src/common/l1sap.c@735 PS7, Line 735: if (!gsm_bts_has_feature(trx->bts, BTS_FEAT_MEAS_PAYLOAD_COMB)) : process_l1sap_meas_data(trx, l1sap, PRIM_MPH_INFO); > we sould probably print an error message here (once? rate-limited)? Or even > have an OSMO_ASSERT()? […] Done https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/scheduler.c File src/common/scheduler.c: https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/scheduler.c@760 PS1, Line 760: l1sap->u.tch. > I believe I explained this elsewhere here in gerrit or in redmine. […] The is_sub flag is, as far as I know only set by the higher layers. From what I can see the higher layers are not setting it, at least not in l1sap.c. However when we the data ind and the measurement report are one indication the decision would be easy to make in l1sap.c https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/osmo-bts-trx/scheduler_trx.c File src/osmo-bts-trx/scheduler_trx.c: https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/osmo-bts-trx/scheduler_trx.c@a205 PS1, Line 205: /* FIXME: use actual values for BER etc */ > the best would be if those values simply wouldn't count. […] We could use rssi_sum rssi_num from the chan_state struct to do that. I see it in other places too so I use this method now. However, I am still not sure if this is right. The value is now averaged so when the signal fades it should be realistic, but what if there is a sudden signal drop then the lost SACCH frames we compensate here would reflect better signal conditions as they actually are. All in all it might not matter much as the measurement results are averaged anyway in the higher layers. https://gerrit.osmocom.org/c/osmo-bts/+/15918/8/src/osmo-bts-trx/scheduler_trx.c File src/osmo-bts-trx/scheduler_trx.c: https://gerrit.osmocom.org/c/osmo-bts/+/15918/8/src/osmo-bts-trx/scheduler_trx.c@198 PS8, Line 198: *rssi_sum / *rssi_num > This is a TX burst handler, so there can be no RSSI measurements. […] Yes, in TX the division by zero case is very likely. However, I see the division with rssi and toa in many other places. Do you think it makes sense to protect those as well? https://gerrit.osmocom.org/c/osmo-bts/+/15918/9/src/osmo-bts-trx/scheduler_trx.c File src/osmo-bts-trx/scheduler_trx.c: https://gerrit.osmocom.org/c/osmo-bts/+/15918/9/src/osmo-bts-trx/scheduler_trx.c@401 PS9, Line 401: toa_num > Same here. Its probably the wrong approach to calculate those values from the chan_state. Instead I suggest to remember the last good toa256 and the last good rssi and use those instead. However I still do not understand why there aren't any toa256_sum values in the chan state. During the channel establishment phase, before the MS transmitting I see that there can not be any values but what happens when a TCH gets lost while the channel is up? -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/15918 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I710d0b7cf193afa8515807836ee69b8b7db84a84 Gerrit-Change-Number: 15918 Gerrit-PatchSet: 9 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: Thu, 09 Jan 2020 14:54:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: dexter <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Comment-In-Reply-To: laforge <[email protected]> Gerrit-MessageType: comment
