<p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.h:</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-bsc/+/25298/2/include/osmocom/bsc/gsm_data.h@1346">Patch Set #2, Line 1346:</a> <code style="font-family:monospace,monospace">      /* Measurement averaging parameters for C/I: */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think using an array with pre-defined indexes would simplify the code: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I also though about it, but I don't think it really helps, because anyway the RSL messages are not using arrays, so you still need to copy to/from specific offsets, which means you cannot easily use loops. And I want to avoid using arrays with enums in the RSL protocol because that would also require speechifying enums for the protocol, adding even more non-standard stuff.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I agree some code can look a bit verbose, but it's manily due to having lots of parameters added. I can move some big macros to a helper function if it really makes it less hard to read.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_ipaccess_nanobts.c">File src/osmo-bsc/bts_ipaccess_nanobts.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-bsc/+/25298/2/src/osmo-bsc/bts_ipaccess_nanobts.c@870">Patch Set #2, Line 870:</a> <code style="font-family:monospace,monospace">enc_osmo_meas_proc_params</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why do we add osmo-bts specific code in bts_ipaccess_nanobts. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ok I can move it there.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_ipaccess_nanobts.c@889">Patch Set #2, Line 889:</a> <code style="font-family:monospace,monospace">#define ENC_PROC(PARAMS, TO, TYPE) do { \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I could move this to a helper function, but not sure if we'd really win a lot, opinions/preferences welcome.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_ipaccess_nanobts.c@994">Patch Set #2, Line 994:</a> <code style="font-family:monospace,monospace">           #define ENC_THRESH_CI(TYPE) \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it's fine keeping this one and the one below as macros.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c">File src/osmo-bsc/bts_vty.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-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3063">Patch Set #2, Line 3063:</a> <code style="font-family:monospace,monospace">Channel Type</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">FR/EFR is not really a 'Channel type', but rather 'Codec type'.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm just following naming used in "GSM/EDGE: Evolution and Performance" Table 10.3.<br>It's a channel type in the generic sense, not in "GSM logical channel type". I don't care about changing it to Codec Type if you prefer though.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3068">Patch Set #2, Line 3068:</a> <code style="font-family:monospace,monospace">Channel Type (E)GPRS</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Can you please explain how is this related to GPRS? Currently all parameters in 'config-{ms,bs}-powe […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">So far, I'm just adding it for completeness when doing all the monkey-type work, since we got values for it too.<br>It's still not clear to me how are we going to use it, I never had a look at MS power control on PDCH but I guess there may be some way to tell the MS to reduce the power from the PCU?<br>So yes, we'd pass that to the PCU at some point from the BTS.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3137">Patch Set #2, Line 3137:</a> <code style="font-family:monospace,monospace">,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Hmm, maybe worth adding a comment like: , /* nope */, because two comas look odd.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ACK good idea.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3190">Patch Set #2, Line 3190:</a> <code style="font-family:monospace,monospace">lower_cmp_p, lower_cmp_n</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ha! I also copy-pasted it here :P</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ACK I'll send a separate patch.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3209">Patch Set #2, Line 3209:</a> <code style="font-family:monospace,monospace">Upper</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Lower?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3227">Patch Set #2, Line 3227:</a> <code style="font-family:monospace,monospace">lower_cmp_p, lower_cmp_n</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Copy-pasted, should be upper (also see comment above).</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3354">Patch Set #2, Line 3354:</a> <code style="font-family:monospace,monospace">-toI</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">-to-</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/tests/power_ctrl.vty">File tests/power_ctrl.vty:</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-bsc/+/25298/2/tests/power_ctrl.vty@78">Patch Set #2, Line 78:</a> <code style="font-family:monospace,monospace">   ci-thresh fr lower 13 upper 17</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why do we print C/I parameters for *BS* power control if they only apply to MS power control?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indeed my fault, thanks for noticing.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298">change 25298</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-bsc/+/25298"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I7e76ec47b323d469f777624b74b08752d1f5584f </div>
<div style="display:none"> Gerrit-Change-Number: 25298 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 01 Sep 2021 11:28:40 +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: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>