<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -2</span></p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21789">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21789/1/src/gsm/gsm48_rest_octets.c">File src/gsm/gsm48_rest_octets.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/libosmocore/+/21789/1/src/gsm/gsm48_rest_octets.c@613">Patch Set #1, Line 613:</a> <code style="font-family:monospace,monospace">early_cm_restrict_3g</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we need to agree on what do we expect from 'early_cm_restrict_3g = true' and 'early_cm_restrict_3g = false' first. IMHO, the field name itself is confusing, because it reads/sounds like 'to restrict'. In reality, 3GPP TS 44.018 defines this bit as "3G Early Classmark Sending Restriction", and H does not mean 'restrict'.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your patch makes the encoding function behave asymmetrically from osmo_gsm48_rest_octets_si3_encode():</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if (bitvec_get_bit_high(&bv) == H)<br>    si3->early_cm_restrict_3g = 1;<br>  else<br>    si3->early_cm_restrict_3g = 0;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">which by the way means that we don't have sufficient unit test coverage :/</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think the fix that I merged back then is correct, and we should rather fix osmo-bsc:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  src/osmo-bsc/system_information.c:968:<br>    si_info.early_cm_restrict_3g = !bts->early_classmark_allowed_3g;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">i.e. there is need to inverse the value/meaning of 'early_classmark_allowed_3g'.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/21789">change 21789</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/libosmocore/+/21789"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I0ee48d3240c62c4d2e15063b26da7a2a617f383e </div>
<div style="display:none"> Gerrit-Change-Number: 21789 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Hoernchen <ewild@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: Sat, 19 Dec 2020 20:29:06 +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>