<p>pespin <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/18323">View Change</a></p><div style="white-space:pre-wrap">Approvals:
laforge: Looks good to me, but someone else must approve
daniel: Looks good to me, approved
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">tbf_ul: Fix UL ACK not sent to MS if intermediate UL block is lost<br><br>In normal conditions ACKing of UL blocks is only sent every<br>SEND_ACK_AFTER_FRAMES (20) frames. Which means if CV=0 is received (and<br>hence no more packets are received) less than 20 frames before a lost,<br>the PCU won't ask for a retransmission and wait there until some timer<br>destroys the TBF.<br><br>This issue is shown by TTCN3 test PCU_Tests.ttcn<br>TC_ul_intermediate_retrans.<br><br>Unit tests triggering this condition are adapted. Some similar tests are<br>not triggering it because BSN/CV relation being used is totally wrong<br>(like CV=0 being sent on a BSN with previous value than others).<br><br>Change-Id: I9b4ef7b7277efa645bdb5becf2e9f6b32c99a9b1<br>---<br>M src/tbf_ul.cpp<br>M src/tbf_ul.h<br>M tests/tbf/TbfTest.cpp<br>M tests/tbf/TbfTest.err<br>4 files changed, 25 insertions(+), 15 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/tbf_ul.cpp b/src/tbf_ul.cpp</span><br><span>index 9899580..009dfcb 100644</span><br><span>--- a/src/tbf_ul.cpp</span><br><span>+++ b/src/tbf_ul.cpp</span><br><span>@@ -188,6 +188,9 @@</span><br><span> const struct gprs_rlc_data_info *rlc,</span><br><span> uint8_t *data, struct pcu_l1_meas *meas)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ const struct gprs_rlc_data_block_info *rdbi;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlc_data *block;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> int8_t rssi = meas->have_rssi ? meas->rssi : 0;</span><br><span> </span><br><span> const uint16_t ws = m_window.ws();</span><br><span>@@ -218,10 +221,8 @@</span><br><span> for (block_idx = 0; block_idx < rlc->num_data_blocks; block_idx++) {</span><br><span> int num_chunks;</span><br><span> uint8_t *rlc_data;</span><br><span style="color: hsl(0, 100%, 40%);">- const struct gprs_rlc_data_block_info *rdbi =</span><br><span style="color: hsl(0, 100%, 40%);">- &rlc->block_info[block_idx];</span><br><span style="color: hsl(120, 100%, 40%);">+ rdbi = &rlc->block_info[block_idx];</span><br><span> bool need_rlc_data = false;</span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_rlc_data *block;</span><br><span> </span><br><span> LOGPTBFUL(this, LOGL_DEBUG,</span><br><span> "Got %s RLC data block: CV=%d, BSN=%d, SPB=%d, PI=%d, E=%d, TI=%d, bitoffs=%d\n",</span><br><span>@@ -330,14 +331,13 @@</span><br><span> assemble_forward_llc(m_rlc.block(index));</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* Check CV of last frame in buffer */</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Last frame in buffer: */</span><br><span style="color: hsl(120, 100%, 40%);">+ block = m_rlc.block(m_window.mod_sns(m_window.v_r() - 1));</span><br><span style="color: hsl(120, 100%, 40%);">+ rdbi = &block->block_info;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Check if we already received all data TBF had to send: */</span><br><span> if (this->state_is(GPRS_RLCMAC_FLOW) /* still in flow state */</span><br><span> && this->m_window.v_q() == this->m_window.v_r()) { /* if complete */</span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_rlc_data *block =</span><br><span style="color: hsl(0, 100%, 40%);">- m_rlc.block(m_window.mod_sns(m_window.v_r() - 1));</span><br><span style="color: hsl(0, 100%, 40%);">- const struct gprs_rlc_data_block_info *rdbi =</span><br><span style="color: hsl(0, 100%, 40%);">- &block->block_info;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> LOGPTBFUL(this, LOGL_DEBUG,</span><br><span> "No gaps in received block, last block: BSN=%d CV=%d\n",</span><br><span> rdbi->bsn, rdbi->cv);</span><br><span>@@ -351,13 +351,13 @@</span><br><span> </span><br><span> /* If TLLI is included or if we received half of the window, we send</span><br><span> * an ack/nack */</span><br><span style="color: hsl(0, 100%, 40%);">- maybe_schedule_uplink_acknack(rlc);</span><br><span style="color: hsl(120, 100%, 40%);">+ maybe_schedule_uplink_acknack(rlc, rdbi->cv == 0);</span><br><span> </span><br><span> return 0;</span><br><span> }</span><br><span> </span><br><span> void gprs_rlcmac_ul_tbf::maybe_schedule_uplink_acknack(</span><br><span style="color: hsl(0, 100%, 40%);">- const gprs_rlc_data_info *rlc)</span><br><span style="color: hsl(120, 100%, 40%);">+ const gprs_rlc_data_info *rlc, bool countdown_finished)</span><br><span> {</span><br><span> bool require_ack = false;</span><br><span> bool have_ti = rlc->block_info[0].ti ||</span><br><span>@@ -373,10 +373,14 @@</span><br><span> LOGPTBFUL(this, LOGL_DEBUG,</span><br><span> "Scheduling Ack/Nack, because TLLI is included.\n");</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- if (state_is(GPRS_RLCMAC_FINISHED)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (countdown_finished) {</span><br><span> require_ack = true;</span><br><span style="color: hsl(0, 100%, 40%);">- LOGPTBFUL(this, LOGL_DEBUG,</span><br><span style="color: hsl(0, 100%, 40%);">- "Scheduling final Ack/Nack, because all data was received and last block has CV==0.\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ if (state_is(GPRS_RLCMAC_FLOW))</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPTBFUL(this, LOGL_DEBUG,</span><br><span style="color: hsl(120, 100%, 40%);">+ "Scheduling Ack/Nack, because some data is missing and last block has CV==0.\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ else if (state_is(GPRS_RLCMAC_FINISHED))</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPTBFUL(this, LOGL_DEBUG,</span><br><span style="color: hsl(120, 100%, 40%);">+ "Scheduling final Ack/Nack, because all data was received and last block has CV==0.\n");</span><br><span> }</span><br><span> if ((m_rx_counter % SEND_ACK_AFTER_FRAMES) == 0) {</span><br><span> require_ack = true;</span><br><span>diff --git a/src/tbf_ul.h b/src/tbf_ul.h</span><br><span>index 85da4f6..c6df0cd 100644</span><br><span>--- a/src/tbf_ul.h</span><br><span>+++ b/src/tbf_ul.h</span><br><span>@@ -21,6 +21,8 @@</span><br><span> </span><br><span> #ifdef __cplusplus</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#include <stdbool.h></span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> #include "tbf.h"</span><br><span> /*</span><br><span> * TBF instance</span><br><span>@@ -97,7 +99,7 @@</span><br><span> struct rate_ctr_group *m_ul_egprs_ctrs;</span><br><span> </span><br><span> protected:</span><br><span style="color: hsl(0, 100%, 40%);">- void maybe_schedule_uplink_acknack(const gprs_rlc_data_info *rlc);</span><br><span style="color: hsl(120, 100%, 40%);">+ void maybe_schedule_uplink_acknack(const gprs_rlc_data_info *rlc, bool countdown_finished);</span><br><span> </span><br><span> /* Please note that all variables below will be reset when changing</span><br><span> * from WAIT RELEASE back to FLOW state (re-use of TBF).</span><br><span>diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp</span><br><span>index 36f9ced..3f2925a 100644</span><br><span>--- a/tests/tbf/TbfTest.cpp</span><br><span>+++ b/tests/tbf/TbfTest.cpp</span><br><span>@@ -1459,6 +1459,7 @@</span><br><span> </span><br><span> pdch = &the_bts->bts_data()->trx[trx_no].pdch[ts_no];</span><br><span> pdch->rcv_block(&data[0], sizeof(data), *fn, &meas);</span><br><span style="color: hsl(120, 100%, 40%);">+ ul_tbf->create_ul_ack(*fn, ts_no);</span><br><span> </span><br><span> request_dl_rlc_block(ul_tbf, fn);</span><br><span> </span><br><span>diff --git a/tests/tbf/TbfTest.err b/tests/tbf/TbfTest.err</span><br><span>index a1530c1..57787c5 100644</span><br><span>--- a/tests/tbf/TbfTest.err</span><br><span>+++ b/tests/tbf/TbfTest.err</span><br><span>@@ -7193,6 +7193,9 @@</span><br><span> TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW EGPRS) Got MCS-4 RLC data block: CV=0, BSN=64, SPB=0, PI=0, E=1, TI=0, bitoffs=33</span><br><span> TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW EGPRS) BSN 64 storing in window (1..192)</span><br><span> TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW EGPRS) data_length=44, data=80 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 95 </span><br><span style="color: hsl(120, 100%, 40%);">+TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW EGPRS) Scheduling Ack/Nack, because some data is missing and last block has CV==0.</span><br><span style="color: hsl(120, 100%, 40%);">+TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW EGPRS) changes UL ACK state from GPRS_RLCMAC_UL_ACK_NONE to GPRS_RLCMAC_UL_ACK_SEND_ACK</span><br><span style="color: hsl(120, 100%, 40%);">+TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=FLOW EGPRS) changes UL ACK state from GPRS_RLCMAC_UL_ACK_SEND_ACK to GPRS_RLCMAC_UL_ACK_NONE</span><br><span> Received RTS for PDCH: TRX=0 TS=7 FN=2654279 block_nr=10 scheduling USF=0 for required uplink resource of UL TFI=0</span><br><span> TBF(TFI=0 TLLI=0xf1223344 DIR=DL STATE=ASSIGN EGPRS) start Packet Downlink Assignment (PACCH)</span><br><span> +++++++++++++++++++++++++ TX : Packet Downlink Assignment +++++++++++++++++++++++++</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/18323">change 18323</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/+/18323"/><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: I9b4ef7b7277efa645bdb5becf2e9f6b32c99a9b1 </div>
<div style="display:none"> Gerrit-Change-Number: 18323 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </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: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>