<p style="white-space: pre-wrap; word-wrap: break-word;">(ensure draft replies are sent)</p><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918">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-bts/+/15918/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/+/15918/1/src/common/l1sap.c@67">Patch Set #1, Line 67:</a> <code style="font-family:monospace,monospace"> * indication for itsself that is passed up in parallel to the payload data</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">itself</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@74">Patch Set #1, Line 74:</a> <code style="font-family:monospace,monospace">static bool tch_data_meas_present = true;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ACK.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@646">Patch Set #1, Line 646:</a> <code style="font-family:monospace,monospace">        struct info_meas_ind_param *info_meas_ind = &l1sap->u.info.u.meas_ind;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Let's rather move these assignments under each switch case.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@695">Patch Set #1, Line 695:</a> <code style="font-family:monospace,monospace">                "%s MPH_INFO meas ind, ta_offs_256bits=%d, ber10k=%d, inv_rssi=%u\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">MPH_INFO only on some case right? maybe add a string pointing to the name in the switch case.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/common/l1sap.c@740">Patch Set #1, Line 740:</a> <code style="font-family:monospace,monospace">              rc = 0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">rc is already zero here, it can be dropped.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/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/+/15918/1/src/common/scheduler.c@760">Patch Set #1, Line 760:</a> <code style="font-family:monospace,monospace">l1sap->u.tch.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How about 'is_sub'? (still not aware of its meaning)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/osmo-bts-trx/scheduler_trx.c">File src/osmo-bts-trx/scheduler_trx.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/+/15918/1/src/osmo-bts-trx/scheduler_trx.c@a205">Patch Set #1, Line 205:</a> <code style="font-family:monospace,monospace">/* FIXME: use actual values for BER etc */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Not sure why you're removing this comment...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/osmo-bts-trx/scheduler_trx.c@a991">Patch Set #1, Line 991:</a> <code style="font-family:monospace,monospace">/* Send uplink measurement information to L2 */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This comment does not make sense anymore.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15918/1/src/osmo-bts-trx/scheduler_trx.c@a1105">Patch Set #1, Line 1105:</a> <code style="font-family:monospace,monospace">/* Send uplink measurement information to L2 */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same.</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/+/15918">change 15918</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/+/15918"/><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: I710d0b7cf193afa8515807836ee69b8b7db84a84 </div>
<div style="display:none"> Gerrit-Change-Number: 15918 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </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-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 11 Nov 2019 13:08:22 +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"> Gerrit-MessageType: comment </div>