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 4: (9 comments) (ensure draft replies are sent) https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@67 PS1, Line 67: * indication for itsself that is passed up in parallel to the payload data > itself Done https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@74 PS1, Line 74: static bool tch_data_meas_present = true; > ACK. I wonder if there is a failsafe way to do this. We could have a function in l1sap.c that is called by the bts model and then does the switching. A per-bts-model API is usually a very difficult thing as in theory it would require a test with each model and I do not even have the hardware for each and every BTS, so its always blind flying. https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@646 PS1, Line 646: struct info_meas_ind_param *info_meas_ind = &l1sap->u.info.u.meas_ind; > Let's rather move these assignments under each switch case. Done https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@695 PS1, Line 695: "%s MPH_INFO meas ind, ta_offs_256bits=%d, ber10k=%d, inv_rssi=%u\n", > MPH_INFO only on some case right? maybe add a string pointing to the name in > the switch case. Done https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@740 PS1, Line 740: rc = 0; > rc is already zero here, it can be dropped. 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. > How about 'is_sub'? (still not aware of its meaning) I am also a bit confused about the is_sub struct member. As far as I know it is used for DTX to mark special frames which must not be removed by DTX. I think this is to distinguish between real bad frames and those what are just not sent. My guess is that osmo-bts-trx seems not to support this for some reason, but phy based BTS might support it and set the flag then? I am not sure. 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 */ > Not sure why you're removing this comment... The problem here is that we can not use actual values for BER. This part of the code generates an artificial frame to satisfy the higher layers. We can only use artificial values. A BER value of 10000 is basically 100% Error, which matches the reality here. The rssi of -110 is fake, but realistic. I wonder about the ta_offs_256bits, maybe we should try to find some better approximation than 0 here... https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/osmo-bts-trx/scheduler_trx.c@a991 PS1, Line 991: /* Send uplink measurement information to L2 */ > This comment does not make sense anymore. Done https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/osmo-bts-trx/scheduler_trx.c@a1105 PS1, Line 1105: /* Send uplink measurement information to L2 */ > Same. Done -- 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: 4 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <[email protected]> Gerrit-Reviewer: fixeria <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Comment-Date: Mon, 11 Nov 2019 13:08:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Gerrit-MessageType: comment
