<p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24777">View Change</a></p><p>15 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/+/24777/3//COMMIT_MSG">Commit Message:</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/+/24777/3//COMMIT_MSG@14">Patch Set #3, Line 14:</a> <code style="font-family:monospace,monospace">However, in the recent 3GPP TS 45.008 there is a feature called</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(could be nice to mention the spec version nr)</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/+/24777/3//COMMIT_MSG@30">Patch Set #3, Line 30:</a> <code style="font-family:monospace,monospace">A value greater than zero would make osmo-bts reduce the power</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what do you mean, "would"?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/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/+/24777/3/doc/manuals/chapters/power_control.adoc@334">Patch Set #3, Line 334:</a> <code style="font-family:monospace,monospace">a BTS shall maintain discontinuous Downlink transmission at full power in order to</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(are you sure that you don't mean "continuous")</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/+/24777/3/doc/manuals/chapters/power_control.adoc@337">Patch Set #3, Line 337:</a> <code style="font-family:monospace,monospace">version 13.0.0 (2015-11) - "BCCH carrier power reduction operation".</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"in version"</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/+/24777/3/doc/manuals/chapters/power_control.adoc@348">Patch Set #3, Line 348:</a> <code style="font-family:monospace,monospace">and the simulation results can be found in 3GPP TR 45.926.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">("TS"?)</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/+/24777/3/doc/manuals/chapters/power_control.adoc@352">Patch Set #3, Line 352:</a> <code style="font-family:monospace,monospace">At the moment of writing this manual, the only BTS model that can instructed to</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(the usual expression is "at the time of writing")</p><p style="white-space: pre-wrap; word-wrap: break-word;">"that can be instructed"</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/+/24777/3/doc/manuals/chapters/power_control.adoc@354">Patch Set #3, Line 354:</a> <code style="font-family:monospace,monospace">can be added on request</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">sounds like it doesn't belong in this manual?</p><p style="white-space: pre-wrap; word-wrap: break-word;">if you need to say this, maybe rather say "may be added in the future", otherwise it sounds like osmocom is offering a free service of implementing things on request</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/+/24777/3/doc/manuals/chapters/power_control.adoc@358">Patch Set #3, Line 358:</a> <code style="font-family:monospace,monospace">`show bts` command in the VTY interface.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe "in OsmoBSC's VTY interface"</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/+/24777/3/doc/manuals/chapters/power_control.adoc@363">Patch Set #3, Line 363:</a> <code style="font-family:monospace,monospace">There is currently no logic in osmo-bsc for automatic activation and deactivation</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(technically "osmo-bsc" is the binary and here it should be OsmoBSC ...)</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/+/24777/3/doc/manuals/chapters/power_control.adoc@407">Patch Set #3, Line 407:</a> <code style="font-family:monospace,monospace">3652121201381481804</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe rather make it "1" ? otherwise it looks like a meaningful important number</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bsc_ctrl_commands.c">File src/osmo-bsc/bsc_ctrl_commands.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/+/24777/3/src/osmo-bsc/bsc_ctrl_commands.c@555">Patch Set #3, Line 555:</a> <code style="font-family:monospace,monospace"> const int red = atoi(value);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">atoi() is not sufficient to verify proper input. The value could be *anything*, supplied by an external user. atoi() would return some int value on error, e.g. if the user supplies value = "potatoe" or "1.234".</p><p style="white-space: pre-wrap; word-wrap: break-word;">A proper input value checking is found at e.g.<br>https://git.osmocom.org/libosmocore/tree/src/gsm/gsm23003.c?id=d9825c0a2cfeab682f29167b11aaa154b8a36bae#n498</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> errno = 0;<br> _mnc = strtol(mnc_str, &endptr, 10);<br> if (errno)<br> return -errno;<br> else if (*endptr)<br> return -EINVAL;<br> if (_mnc < 0 || _mnc > 999)<br> return -ERANGE;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(slightly tweaked the example here to return -errno)</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/+/24777/3/src/osmo-bsc/bsc_ctrl_commands.c@584">Patch Set #3, Line 584:</a> <code style="font-family:monospace,monospace"> const int red = atoi(cmd->value);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(here atoi() is fine because the verify function should have verified the value already)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/src/osmo-bsc/bts_osmobts.c">File src/osmo-bsc/bts_osmobts.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/+/24777/3/src/osmo-bsc/bts_osmobts.c@61">Patch Set #3, Line 61:</a> <code style="font-family:monospace,monospace"> dh = (struct abis_rsl_dchan_hdr *) msgb_put(msg, sizeof(*dh));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(usually no spaces in type cast)</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/+/24777/3/src/osmo-bsc/bts_osmobts.c@65">Patch Set #3, Line 65:</a> <code style="font-family:monospace,monospace"> dh->chan_nr = RSL_CHAN_BCCH;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">in the commit log you're saying this is abusing the message?<br>maybe some comment here would be nice to indicate that it is an osmocom specific coding?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24777/3/tests/osmo-bsc.vty">File tests/osmo-bsc.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/+/24777/3/tests/osmo-bsc.vty@49">Patch Set #3, Line 49:</a> <code style="font-family:monospace,monospace"> c0-power-reduction BCCH carrier power reduction operation</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">i would recommend also adding tests for the parameters in the way done below, e.g. 'bts 0 c0-power-reduction ?' to verify that the argument doc strings are matched up correctly</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24777">change 24777</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/+/24777"/><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: I047fce33d4d3e4c569dd006ba17858467a2f4783 </div>
<div style="display:none"> Gerrit-Change-Number: 24777 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 02 Jul 2021 15:52:07 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>