<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298">View Change</a></p><p>10 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 style="white-space: pre-wrap; word-wrap: break-word;">I think using an array with pre-defined indexes would simplify the code:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  enum {<br>    GSM_PWR_CTRL_MP_CI_FR,<br>    GSM_PWR_CTRL_MP_CI_HR,<br>    _GSM_PWR_CTRL_MP_NUM,<br>  };</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  ....</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  struct gsm_power_ctrl_meas_params ci[_GSM_PWR_CTRL_MP_NUM];</pre><p style="white-space: pre-wrap; word-wrap: break-word;">So you can iterate over 'ci' using a loop:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  for (i = 0; i < ARRAY_SIZE(ci); i++)<br>    ...</pre><p style="white-space: pre-wrap; word-wrap: break-word;">rather than using huge macros in the generating/parsing code. And for the VTY, you would be able to use value_string instead of custom mapping functions.</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 style="white-space: pre-wrap; word-wrap: break-word;">Why do we add osmo-bts specific code in bts_ipaccess_nanobts.c? I believe it should be in bts_osmobts.c. This would require to make some API non-static in this file, so you can re-use it from bts_osmobts.c, which complicates the task a bit. But logically it should be there, not here.</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 style="white-space: pre-wrap; word-wrap: break-word;">FR/EFR is not really a 'Channel type', but rather 'Codec type'.</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 style="white-space: pre-wrap; word-wrap: break-word;">Can you please explain how is this related to GPRS? Currently all parameters in 'config-{ms,bs}-power-ctrl' nodes are for CS specific power control only. As far as I know, GPRS specific power control is completely different from CS domain. How is this going to be used and where? Are you planning to forward it to osmo-pcu somehow?</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 style="white-space: pre-wrap; word-wrap: break-word;">Hmm, maybe worth adding a comment like: , /* nope */, because two comas look odd.</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 style="white-space: pre-wrap; word-wrap: break-word;">Ha! I also copy-pasted it here :P</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 style="white-space: pre-wrap; word-wrap: break-word;">Lower?</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 style="white-space: pre-wrap; word-wrap: break-word;">Copy-pasted, should be upper (also see comment above).</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 style="white-space: pre-wrap; word-wrap: break-word;">-to-</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 style="white-space: pre-wrap; word-wrap: break-word;">Why do we print C/I parameters for *BS* power control if they only apply to MS power control?</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 08:47:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>