<p>laforge <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/14697">View Change</a></p><div style="white-space:pre-wrap">Approvals:
laforge: Looks good to me, approved
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Encoding: ACK/NACK: always encode with length field present<br><br>In most cases the length field was present and this field takes 7<br>bits of the maximum available 110 rest bits.<br>The length field was only removed when encoding huge bitmaps usually<br>only happen on lossy connections with packet lost.<br>However the cases without length field were encoded incorrect,<br>because all remaining bits must be used by the uncompressed bitmaps,<br>but the PCU violates this by encoding always the "release 5" bit.<br>Rather than fixing the encoding without length field, simply remove it<br>and always encode with length field. This also reduces the code<br>complexity.<br><br>Change-Id: I7bc2e18d647b72b8f17ba7a5c9c5e421d88275fb<br>---<br>M src/encoding.cpp<br>1 file changed, 39 insertions(+), 43 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/encoding.cpp b/src/encoding.cpp</span><br><span>index 48f3829..ca9e906 100644</span><br><span>--- a/src/encoding.cpp</span><br><span>+++ b/src/encoding.cpp</span><br><span>@@ -828,6 +828,17 @@</span><br><span> bitvec_write_field(dest, &wp, 0, 1); // 0: don't have REL 5</span><br><span> };</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/* Encode the Ack/Nack for EGPRS. 44.060</span><br><span style="color: hsl(120, 100%, 40%);">+ * The PCU encodes to receive block bitmap to the following rules:</span><br><span style="color: hsl(120, 100%, 40%);">+ * - always encode the lenght field</span><br><span style="color: hsl(120, 100%, 40%);">+ * - use compressed receive block bitmap if it's smaller than uncompressed</span><br><span style="color: hsl(120, 100%, 40%);">+ * receive block bitmap</span><br><span style="color: hsl(120, 100%, 40%);">+ * - use the remaining bits for an uncompressed receive block bitmap if needed</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Note: The spec also defines an Ack/Nack without length field, but the PCU's</span><br><span style="color: hsl(120, 100%, 40%);">+ * doesn't support this in UL. It would require a lot more code complexity</span><br><span style="color: hsl(120, 100%, 40%);">+ * and only saves 7 bit in lossy sitations.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span> static void write_packet_ack_nack_desc_egprs(</span><br><span> bitvec * dest, unsigned& wp,</span><br><span> gprs_rlc_ul_window *window, bool is_final, unsigned rest_bits)</span><br><span>@@ -849,12 +860,12 @@</span><br><span> bitvec ucmp_vec;</span><br><span> bitvec crbb_vec;</span><br><span> uint8_t uclen_crbb = 0;</span><br><span style="color: hsl(0, 100%, 40%);">- bool len_coded = true;</span><br><span> uint8_t crbb_start_clr_code;</span><br><span> uint8_t i;</span><br><span> </span><br><span> /* static size of 16 bits</span><br><span style="color: hsl(0, 100%, 40%);">- ..0. .... = ACKNACK: (Union)</span><br><span style="color: hsl(120, 100%, 40%);">+ ..1. .... = ACKNACK: (Union)</span><br><span style="color: hsl(120, 100%, 40%);">+ 0 0000 000 Length</span><br><span> Desc</span><br><span> </span><br><span> ...0 .... = FINAL_ACK_INDICATION: False</span><br><span>@@ -865,8 +876,9 @@</span><br><span> </span><br><span> .... ..10 0101 0001 1... .... = STARTING_SEQUENCE_NUMBER: 1187</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- .0.. .... = CRBB Exist: 0 */</span><br><span style="color: hsl(0, 100%, 40%);">- rest_bits -= 16;</span><br><span style="color: hsl(120, 100%, 40%);">+ .0.. .... = CRBB Exist: 0</span><br><span style="color: hsl(120, 100%, 40%);">+ minimal size is 24 rest_bits */</span><br><span style="color: hsl(120, 100%, 40%);">+ rest_bits -= 24;</span><br><span> </span><br><span> if (num_blocks > 0)</span><br><span> /* V(Q) is NACK and omitted -> SSN = V(Q) + 1 */</span><br><span>@@ -903,47 +915,31 @@</span><br><span> crbb_len = crbb_vec.cur_bit;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (is_compressed == 0) {</span><br><span style="color: hsl(0, 100%, 40%);">- /* length field takes 8 bits*/</span><br><span style="color: hsl(0, 100%, 40%);">- if (num_blocks > rest_bits - 8) {</span><br><span style="color: hsl(0, 100%, 40%);">- eow = false;</span><br><span style="color: hsl(0, 100%, 40%);">- urbb_len = rest_bits;</span><br><span style="color: hsl(0, 100%, 40%);">- len_coded = false;</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (num_blocks == rest_bits) {</span><br><span style="color: hsl(0, 100%, 40%);">- urbb_len = rest_bits;</span><br><span style="color: hsl(0, 100%, 40%);">- len_coded = false;</span><br><span style="color: hsl(0, 100%, 40%);">- } else</span><br><span style="color: hsl(0, 100%, 40%);">- urbb_len = num_blocks;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- len = urbb_len + 15;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (is_compressed) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* 8 = 7 (CRBBlength) + 1 (CRBB starting color code) */</span><br><span style="color: hsl(120, 100%, 40%);">+ rest_bits -= 8;</span><br><span> } else {</span><br><span style="color: hsl(0, 100%, 40%);">- if (num_blocks > uclen_crbb) {</span><br><span style="color: hsl(0, 100%, 40%);">- eow = false;</span><br><span style="color: hsl(0, 100%, 40%);">- urbb_len = num_blocks - uclen_crbb;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">- /* Union bit takes 1 bit */</span><br><span style="color: hsl(0, 100%, 40%);">- /* Other fields in descr of compresed bitmap takes 23 bits</span><br><span style="color: hsl(0, 100%, 40%);">- * -8 = CRBB_STARTING_COLOR_CODE + CRBB_LENGTH */</span><br><span style="color: hsl(0, 100%, 40%);">- if (urbb_len > (rest_bits - crbb_len - 8)) {</span><br><span style="color: hsl(0, 100%, 40%);">- eow = false;</span><br><span style="color: hsl(0, 100%, 40%);">- len_coded = false;</span><br><span style="color: hsl(0, 100%, 40%);">- urbb_len = rest_bits - crbb_len - 8;</span><br><span style="color: hsl(0, 100%, 40%);">- /* -16 = ACKNACK Dissector length + CRBB_STARTING_COLOR_CODE + CRBB_LENGTH */</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (urbb_len > (rest_bits - crbb_len - 16)) {</span><br><span style="color: hsl(0, 100%, 40%);">- eow = false;</span><br><span style="color: hsl(0, 100%, 40%);">- len_coded = false;</span><br><span style="color: hsl(0, 100%, 40%);">- urbb_len = rest_bits - crbb_len - 16;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(120, 100%, 40%);">+ uclen_crbb = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ crbb_len = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (num_blocks > uclen_crbb + rest_bits) {</span><br><span style="color: hsl(120, 100%, 40%);">+ eow = false;</span><br><span style="color: hsl(120, 100%, 40%);">+ urbb_len = rest_bits - crbb_len;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else</span><br><span style="color: hsl(120, 100%, 40%);">+ urbb_len = num_blocks - uclen_crbb;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (is_compressed)</span><br><span> len = urbb_len + crbb_len + 23;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(120, 100%, 40%);">+ else</span><br><span style="color: hsl(120, 100%, 40%);">+ len = urbb_len + 15;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* EGPRS Ack/Nack Description IE */</span><br><span style="color: hsl(0, 100%, 40%);">- if (len_coded == false) {</span><br><span style="color: hsl(0, 100%, 40%);">- bitvec_write_field(dest, &wp, 0, 1); // 0: don't have length</span><br><span style="color: hsl(0, 100%, 40%);">- } else {</span><br><span style="color: hsl(0, 100%, 40%);">- bitvec_write_field(dest, &wp, 1, 1); // 1: have length</span><br><span style="color: hsl(0, 100%, 40%);">- bitvec_write_field(dest, &wp, len, 8); // length</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* EGPRS Ack/Nack Description IE</span><br><span style="color: hsl(120, 100%, 40%);">+ * do not support Ack/Nack without length */</span><br><span style="color: hsl(120, 100%, 40%);">+ bitvec_write_field(dest, &wp, 1, 1); // 1: have length</span><br><span style="color: hsl(120, 100%, 40%);">+ bitvec_write_field(dest, &wp, len, 8); // length</span><br><span> </span><br><span> bitvec_write_field(dest, &wp, is_final, 1); // FINAL_ACK_INDICATION</span><br><span> bitvec_write_field(dest, &wp, bow, 1); // BEGINNING_OF_WINDOW</span><br><span>@@ -973,9 +969,9 @@</span><br><span> }</span><br><span> LOGP(DRLCMACUL, LOGL_DEBUG,</span><br><span> "EGPRS URBB, urbb len = %d, SSN = %u, ESN_CRBB = %u, "</span><br><span style="color: hsl(0, 100%, 40%);">- "len present = %s,desc len = %d, "</span><br><span style="color: hsl(120, 100%, 40%);">+ "desc len = %d, "</span><br><span> "SNS = %d, WS = %d, V(Q) = %d, V(R) = %d%s%s\n",</span><br><span style="color: hsl(0, 100%, 40%);">- urbb_len, ssn, esn_crbb, len_coded ? "yes" : "No" , len,</span><br><span style="color: hsl(120, 100%, 40%);">+ urbb_len, ssn, esn_crbb, len,</span><br><span> window->sns(), window->ws(), window->v_q(), window->v_r(),</span><br><span> bow ? ", BOW" : "", eow ? ", EOW" : "");</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/14697">change 14697</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-pcu/+/14697"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I7bc2e18d647b72b8f17ba7a5c9c5e421d88275fb </div>
<div style="display:none"> Gerrit-Change-Number: 14697 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>