<p>Patch set 5:<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>6 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/5/doc/manuals/chapters/power_control.adoc">File doc/manuals/chapters/power_control.adoc:</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/5/doc/manuals/chapters/power_control.adoc@211">Patch Set #5, Line 211:</a> <code style="font-family:monospace,monospace">The general idea of power control is to maintain the signal level (RxLev) and</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like an unrelated/unneeded reformatting change to me. I don't see words added or removed, things just moved around.</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/5/doc/manuals/chapters/power_control.adoc@257">Patch Set #5, Line 257:</a> <code style="font-family:monospace,monospace">   rxlev-thresh lower 32 upper 38 <2></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">These changes also look unrelated to me. I am not against them, just don't see how could they related to the C/I parameters you're adding. I would CR+2 a separate change, thanks!</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/5/doc/manuals/chapters/power_control.adoc@368">Patch Set #5, Line 368:</a> <code style="font-family:monospace,monospace">   ci-avg amr-fr algo osmo-ewma beta 50 <4></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like you wanted to use <6> and <7> 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@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;">So far, I'm just adding it for completeness when doing all the monkey-type work, since we got values […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would still avoid making this configurable in the VTY unless it's actually implemented.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/5/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/5/src/osmo-bsc/bts_vty.c@3227">Patch Set #5, 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;">Not fixed: upper_cmp_p, upper_cmp_n.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25298/5/src/osmo-bsc/gsm_data.c">File src/osmo-bsc/gsm_data.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/5/src/osmo-bsc/gsm_data.c@1201">Patch Set #5, Line 1201:</a> <code style="font-family:monospace,monospace">C/I averaging is not yet implemented</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">As far as I can see, I52eb0558fd7a215a6ee0b2aced189ae4a37d8a22 implements it in osmo-bts. So I guess it makes sense to enable EWMA with some alpha by default? Or at least remove this comment?</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: 5 </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-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 03 Sep 2021 16:16:17 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>