Fix bug in extraction of E and TI header fields in parsing of uplink header type 1 RLC data block. Test suite for the same is also updated. --- src/decoding.cpp | 15 ++++++++------- tests/edge/EdgeTest.cpp | 16 +++++++--------- 2 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/src/decoding.cpp b/src/decoding.cpp index 6844856..b8e453e 100644 --- a/src/decoding.cpp +++ b/src/decoding.cpp @@ -513,9 +513,9 @@ int Decoding::rlc_parse_ul_data_header_egprs_type_1( offs = rlc->data_offs_bits[0] / 8; OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 0);
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7; - rlc->block_info[0].e = !!(e_ti_header & 0x01); - rlc->block_info[0].ti = !!(e_ti_header & 0x02); + e_ti_header = (data[offs - 1] & (0xc0)) >> 6; + rlc->block_info[0].e = (e_ti_header & 0x01); + rlc->block_info[0].ti = (e_ti_header & 0x02); cur_bit += 2;
rlc->block_info[1].cv = egprs1->cv; @@ -524,20 +524,21 @@ int Decoding::rlc_parse_ul_data_header_egprs_type_1( ((egprs1->bsn2_a << 0) | (egprs1->bsn2_b << 2)); rlc->block_info[1].bsn = rlc->block_info[1].bsn & (RLC_EGPRS_SNS - 1);
- cur_bit += rlc->data_offs_bits[1] - 2; + cur_bit = rlc->data_offs_bits[1] - 2;
offs = rlc->data_offs_bits[1] / 8; OSMO_ASSERT(rlc->data_offs_bits[1] % 8 == 2);
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7; - rlc->block_info[1].e = !!(e_ti_header & 0x01); - rlc->block_info[1].ti = !!(e_ti_header & 0x02); + e_ti_header = (data[offs] & (0x03)); + rlc->block_info[1].e = (e_ti_header & 0x01); + rlc->block_info[1].ti = (e_ti_header & 0x02); cur_bit += 2; /* skip data area */ cur_bit += cs.maxDataBlockBytes() * 8;
return cur_bit; } + /** * \brief Copy LSB bitstream RLC data block to byte aligned buffer. * diff --git a/tests/edge/EdgeTest.cpp b/tests/edge/EdgeTest.cpp index ff080f9..9b4a1a5 100644 --- a/tests/edge/EdgeTest.cpp +++ b/tests/edge/EdgeTest.cpp @@ -1376,18 +1376,16 @@ static gprs_rlcmac_ul_tbf *uplink_header_parsing_test(BTS *the_bts, egprs1->r = 1; egprs1->cv = 7; egprs1->tfi_a = tfi & (~((~0) << 2)); - egprs1->tfi_b = tfi & (~((~0) << 3)) << 2; - egprs1->bsn1_a = 10; - egprs1->bsn1_b = 17; - egprs1->bsn2_a = 0; - egprs1->bsn2_b = 25; + egprs1->tfi_b = (tfi & (~((~0) << 3)) << 2) >> 2; + egprs1->bsn1_a = 0; + egprs1->bsn1_b = 0; + egprs1->bsn2_a = 1; + egprs1->bsn2_b = 0; egprs1->cps = 15; egprs1->rsb = 0; egprs1->pi = 0; - data[6] = 1; - data[6 + 68] = 1; - data[75] = 1; - data[75 + 68] = 1; + data[5] = 0x40; + data[5 + 69] = 1; pdch = &the_bts->bts_data()->trx[trx_no].pdch[ts_no]; pdch->rcv_block(&data[0], 143, *fn, &meas); }
A test case is added to validate the uplink header type 2 extraction functionality. Bug identified by unit test in the header type 2 parsing is fixed. --- src/decoding.cpp | 8 ++-- tests/edge/EdgeTest.cpp | 114 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/edge/EdgeTest.ok | 2 + 3 files changed, 120 insertions(+), 4 deletions(-)
diff --git a/src/decoding.cpp b/src/decoding.cpp index b8e453e..fb0bd76 100644 --- a/src/decoding.cpp +++ b/src/decoding.cpp @@ -471,11 +471,11 @@ int Decoding::rlc_parse_ul_data_header_egprs_type_2( cur_bit += rlc->data_offs_bits[0] - 2;
offs = rlc->data_offs_bits[0] / 8; - OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 1); + OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 7);
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7; - rlc->block_info[0].e = !!(e_ti_header & 0x01); - rlc->block_info[0].ti = !!(e_ti_header & 0x02); + e_ti_header = (data[offs] & 0x60) >> 5; + rlc->block_info[0].e = (e_ti_header & 0x01); + rlc->block_info[0].ti = (e_ti_header & 0x02); cur_bit += 2;
/* skip data area */ diff --git a/tests/edge/EdgeTest.cpp b/tests/edge/EdgeTest.cpp index 9b4a1a5..c7184e2 100644 --- a/tests/edge/EdgeTest.cpp +++ b/tests/edge/EdgeTest.cpp @@ -1409,6 +1409,119 @@ void uplink_header_type1_test(void) printf("=== end %s ===\n", __func__); }
+static gprs_rlcmac_ul_tbf *uplink_header_type_2_parsing_test(BTS *the_bts, + uint8_t ts_no, uint32_t tlli, uint32_t *fn, uint16_t qta, + uint8_t ms_class) +{ + GprsMs *ms; + struct pcu_l1_meas meas; + uint32_t rach_fn = *fn - 51; + uint32_t sba_fn = *fn + 52; + uint8_t trx_no = 0; + int tfi = 0; + gprs_rlcmac_ul_tbf *ul_tbf; + struct gprs_rlcmac_pdch *pdch; + gprs_rlcmac_bts *bts; + RlcMacUplink_t ulreq = {0}; + uint8_t data[79] = {0}; + struct gprs_rlc_ul_header_egprs_2 *egprs2 = NULL; + + meas.set_rssi(31); + egprs2 = (struct gprs_rlc_ul_header_egprs_2 *) data; + bts = the_bts->bts_data(); + + /* needed to set last_rts_fn in the PDCH object */ + request_dl_rlc_block(bts, trx_no, ts_no, 0, fn); + + /* simulate RACH,this sends a Immediate Assignment Uplink on the AGCH */ + the_bts->rcv_rach(0x73, rach_fn, qta); + + /* get next free TFI */ + tfi = the_bts->tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); + + /* fake a resource request */ + ulreq.u.MESSAGE_TYPE = MT_PACKET_RESOURCE_REQUEST; + ulreq.u.Packet_Resource_Request.PayloadType = GPRS_RLCMAC_CONTROL_BLOCK; + ulreq.u.Packet_Resource_Request.ID.UnionType = 1; /* != 0 */ + ulreq.u.Packet_Resource_Request.ID.u.TLLI = tlli; + ulreq.u.Packet_Resource_Request.Exist_MS_Radio_Access_capability = 1; + ulreq.u.Packet_Resource_Request.MS_Radio_Access_capability. + Count_MS_RA_capability_value = 1; + ulreq.u.Packet_Resource_Request.MS_Radio_Access_capability. + MS_RA_capability_value[0].u.Content. + Exist_Multislot_capability = 1; + ulreq.u.Packet_Resource_Request.MS_Radio_Access_capability. + MS_RA_capability_value[0].u.Content.Multislot_capability. + Exist_GPRS_multislot_class = 1; + ulreq.u.Packet_Resource_Request.MS_Radio_Access_capability. + MS_RA_capability_value[0].u.Content.Multislot_capability. + GPRS_multislot_class = ms_class; + ulreq.u.Packet_Resource_Request.MS_Radio_Access_capability. + MS_RA_capability_value[0].u.Content.Multislot_capability. + Exist_EGPRS_multislot_class = 1; + ulreq.u.Packet_Resource_Request.MS_Radio_Access_capability. + MS_RA_capability_value[0].u.Content.Multislot_capability. + EGPRS_multislot_class = ms_class; + + send_ul_mac_block(the_bts, trx_no, ts_no, &ulreq, sba_fn); + + /* check the TBF */ + ul_tbf = the_bts->ul_tbf_by_tfi(tfi, trx_no, ts_no); + OSMO_ASSERT(ul_tbf != NULL); + OSMO_ASSERT(ul_tbf->ta() == qta / 4); + + /* header parsing */ + egprs2->r = 1; + egprs2->si = 1; + egprs2->cv = 7; + egprs2->tfi_a = tfi & (~((~0) << 2)); + egprs2->tfi_b = (tfi & ((~((~0) << 3)) << 2)) >> 2; + egprs2->bsn1_a = 0; + egprs2->bsn1_b = 0; + egprs2->cps_a = 3; + egprs2->cps_b = 0; + egprs2->rsb = 0; + egprs2->pi = 0; + data[10] = 0x20; /* Setting E field */ + pdch = &the_bts->bts_data()->trx[trx_no].pdch[ts_no]; + pdch->rcv_block(&data[0], 79, *fn, &meas); + + egprs2->r = 1; + egprs2->si = 1; + egprs2->cv = 7; + egprs2->tfi_a = tfi & (~((~0) << 2)); + egprs2->tfi_b = tfi & ((~((~0) << 3)) << 2); + egprs2->bsn1_a = 1; + egprs2->bsn1_b = 0; + egprs2->cps_a = 2; + egprs2->cps_b = 0; + egprs2->rsb = 0; + egprs2->pi = 0; + data[10] = 0x20; /*Setting E field */ + pdch = &the_bts->bts_data()->trx[trx_no].pdch[ts_no]; + pdch->rcv_block(&data[0], 79, *fn, &meas); +} + +void uplink_header_type2_test(void) +{ + BTS the_bts; + int ts_no = 7; + uint32_t fn = 2654218; + uint16_t qta = 31; + uint32_t tlli = 0xf1223344; + const char *imsi = "0011223344"; + uint8_t ms_class = 1; + gprs_rlcmac_ul_tbf *ul_tbf; + GprsMs *ms; + + printf("=== start %s ===\n", __func__); + setup_bts(&the_bts, ts_no, 10); + + ul_tbf = uplink_header_type_2_parsing_test(&the_bts, ts_no, + tlli, &fn, qta, ms_class); + printf("=== end %s ===\n", __func__); +} + int main(int argc, char **argv) { struct vty_app_info pcu_vty_info = {0}; @@ -1431,6 +1544,7 @@ int main(int argc, char **argv) test_rlc_unaligned_copy(); test_rlc_unit_encoder(); uplink_header_type1_test(); + uplink_header_type2_test(); if (getenv("TALLOC_REPORT_FULL")) talloc_report_full(tall_pcu_ctx, stderr); return EXIT_SUCCESS; diff --git a/tests/edge/EdgeTest.ok b/tests/edge/EdgeTest.ok index 353f55d..1277d70 100644 --- a/tests/edge/EdgeTest.ok +++ b/tests/edge/EdgeTest.ok @@ -8,3 +8,5 @@ === end test_rlc_unit_encoder === === start uplink_header_type1_test === === end uplink_header_type1_test === +=== start uplink_header_type2_test === +=== end uplink_header_type2_test ===
On 07 Apr 2016, at 13:57, Bhargava Abhyankar Bhargava.Abhyankar@radisys.com wrote:
A test case is added to validate the uplink header type 2 extraction functionality. Bug identified by unit test in the header type 2 parsing is fixed.
offs = rlc->data_offs_bits[0] / 8;
- OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 1);
- OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 7);
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
- rlc->block_info[0].e = !!(e_ti_header & 0x01);
- rlc->block_info[0].ti = !!(e_ti_header & 0x02);
- e_ti_header = (data[offs] & 0x60) >> 5;
- rlc->block_info[0].e = (e_ti_header & 0x01);
- rlc->block_info[0].ti = (e_ti_header & 0x02); cur_bit += 2;
you probably want this to be in another commit.
- pdch = &the_bts->bts_data()->trx[trx_no].pdch[ts_no];
- pdch->rcv_block(&data[0], 79, *fn, &meas);
like in my other mails, please have more strong post conditions. What do you actually expect in terms of side effects here? Test them specifically.
kind regards holger
Hello Holger,
On 07 Apr 2016, at 03:59 AM, Holger Freyther < holger@freyther.de> wrote:
- pdch = &the_bts->bts_data()->trx[trx_no].pdch[ts_no];
- pdch->rcv_block(&data[0], 79, *fn, &meas);
like in my other mails, please have more strong post conditions. What do you actually expect in terms of side effects here? >> Test them specifically.
Will find the relevant conditions to be tested and update it in the patch.
kind regards Bhargava Abhyankar
On 07 Apr 2016, at 13:57, Bhargava Abhyankar Bhargava.Abhyankar@radisys.com wrote:
Fix bug in extraction of E and TI header fields in parsing of uplink header type 1 RLC data block. Test suite for the same is also updated.
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
- e_ti_header = (data[offs - 1] & (0xc0)) >> 6;
Could you please elaborate about this hunk? What is the issue, how was it found, how are we certain that this is the fix?
- rlc->block_info[0].e = !!(e_ti_header & 0x01);
- rlc->block_info[0].ti = !!(e_ti_header & 0x02);
- rlc->block_info[0].e = (e_ti_header & 0x01);
- rlc->block_info[0].ti = (e_ti_header & 0x02);
what is the issue with this? The intention of !! is to normalize the value to 0 and 1. Now .ti can have 0x02 and 0x00 as value. Why is that needed?
cur_bit += 2;
rlc->block_info[1].cv = egprs1->cv; @@ -524,20 +524,21 @@ int Decoding::rlc_parse_ul_data_header_egprs_type_1( ((egprs1->bsn2_a << 0) | (egprs1->bsn2_b << 2)); rlc->block_info[1].bsn = rlc->block_info[1].bsn & (RLC_EGPRS_SNS - 1);
- cur_bit += rlc->data_offs_bits[1] - 2;
- cur_bit = rlc->data_offs_bits[1] - 2;
Why don't we need to advance this cursor anymore?
offs = rlc->data_offs_bits[1] / 8;
OSMO_ASSERT(rlc->data_offs_bits[1] % 8 == 2);
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
- e_ti_header = (data[offs] & (0x03));
What is the misunderstanding here. How did we think we needed to take bits from two different octets?
- rlc->block_info[1].e = !!(e_ti_header & 0x01);
- rlc->block_info[1].ti = !!(e_ti_header & 0x02);
- rlc->block_info[1].e = (e_ti_header & 0x01);
- rlc->block_info[1].ti = (e_ti_header & 0x02);
Again, why the change from 0 or 1 to .. 0 or 1 .. and 0 or 2?
diff --git a/tests/edge/EdgeTest.cpp b/tests/edge/EdgeTest.cpp index ff080f9..9b4a1a5 100644 --- a/tests/edge/EdgeTest.cpp +++ b/tests/edge/EdgeTest.cpp @@ -1376,18 +1376,16 @@ static gprs_rlcmac_ul_tbf *uplink_header_parsing_test(BTS *the_bts, egprs1->r = 1; egprs1->cv = 7; egprs1->tfi_a = tfi & (~((~0) << 2));
- egprs1->tfi_b = tfi & (~((~0) << 3)) << 2;
- egprs1->bsn1_a = 10;
- egprs1->bsn1_b = 17;
- egprs1->bsn2_a = 0;
- egprs1->bsn2_b = 25;
- egprs1->tfi_b = (tfi & (~((~0) << 3)) << 2) >> 2;
- egprs1->bsn1_a = 0;
- egprs1->bsn1_b = 0;
- egprs1->bsn2_a = 1;
- egprs1->bsn2_b = 0; egprs1->cps = 15; egprs1->rsb = 0; egprs1->pi = 0;
- data[6] = 1;
- data[6 + 68] = 1;
- data[75] = 1;
- data[75 + 68] = 1;
- data[5] = 0x40;
- data[5 + 69] = 1;
Please explain the intention here. What is the issue with tfi_b? Why do you change it. Why do you pick the new bsn1_a and bsn1_b values?
holger
On 08 Apr 2016, at 00:19, Holger Freyther holger@freyther.de wrote:
On 07 Apr 2016, at 13:57, Bhargava Abhyankar Bhargava.Abhyankar@radisys.com wrote:
Fix bug in extraction of E and TI header fields in parsing of uplink header type 1 RLC data block. Test suite for the same is also updated.
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
- e_ti_header = (data[offs - 1] & (0xc0)) >> 6;
Could you please elaborate about this hunk? What is the issue, how was it found, how are we certain that this is the fix?
And you can confirm that this is an issue in code we have not merged yet? In that case it should be fixed before including a wrong version.
thank you holger
Hello Holger,
On 08 Apr 2016, at 03:49 AM, Holger Freyther holger@freyther.de wrote:
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
- e_ti_header = (data[offs - 1] & (0xc0)) >> 6;
Could you please elaborate about this hunk? What is the issue, how was it found, how are we certain that this is the fix?
According to Figure 10.3a.4.3.1 of TS 44.060, E and TI bits are spread across two bytes for header type 3. In earlier patch sent for header type 1 same was incorrectly used. Later for header type 1, E and TI bits extraction was corrected as given in the spec 10.3a.4.1.1 of TS 44.060 for first RLC data block. According to the fix, The E and TI field are present in last byte of RLC header.
- rlc->block_info[0].e = !!(e_ti_header & 0x01);
- rlc->block_info[0].ti = !!(e_ti_header & 0x02);
- rlc->block_info[0].e = (e_ti_header & 0x01);
- rlc->block_info[0].ti = (e_ti_header & 0x02);
what is the issue with this? The intention of !! is to normalize the value to 0 and 1. Now .ti can have 0x02 and 0x00 as value. Why is that needed?
Due to oversight normalization is skipped, Normalizing is required, and will be adapted in correction patch.
- cur_bit += rlc->data_offs_bits[1] - 2;
- cur_bit = rlc->data_offs_bits[1] - 2;
Why don't we need to advance this cursor anymore?
In case of header type 1, we have two data blocks and one header. rlc->data_offs_bits[1] - 2 will have number of bits of header + first data block, then we calculate length of second data block and add it which makes it to be number of bits of header + 2 data blocks.
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
- e_ti_header = (data[offs] & (0x03));
What is the misunderstanding here. How did we think we needed to take bits from two different octets?
According to Figure 10.3a.4.3.1 of TS 44.060, E and TI bits are spread across two bytes for header type 3. In earlier patch sent for header type 1 same was incorrectly used. Later for header type 1, E and TI bits extraction was corrected as given in the spec 10.3a.4.1.1 of TS 44.060 for second RLC data block. According to the fix, The E and TI field are present first 2 LSBs after first RLC data block.
- rlc->block_info[1].e = !!(e_ti_header & 0x01);
- rlc->block_info[1].ti = !!(e_ti_header & 0x02);
- rlc->block_info[1].e = (e_ti_header & 0x01);
- rlc->block_info[1].ti = (e_ti_header & 0x02);
Again, why the change from 0 or 1 to .. 0 or 1 .. and 0 or 2?
Due to oversight normalization is skipped, Normalizing is required, and will be adapted in correction patch.
egprs1->r = 1; egprs1->cv = 7; egprs1->tfi_a = tfi & (~((~0) << 2));
- egprs1->tfi_b = tfi & (~((~0) << 3)) << 2;
- egprs1->bsn1_a = 10;
- egprs1->bsn1_b = 17;
- egprs1->bsn2_a = 0;
- egprs1->bsn2_b = 25;
- egprs1->tfi_b = (tfi & (~((~0) << 3)) << 2) >> 2;
- egprs1->bsn1_a = 0;
- egprs1->bsn1_b = 0;
- egprs1->bsn2_a = 1;
- egprs1->bsn2_b = 0; egprs1->cps = 15; egprs1->rsb = 0; egprs1->pi = 0;
- data[6] = 1;
- data[6 + 68] = 1;
- data[75] = 1;
- data[75 + 68] = 1;
- data[5] = 0x40;
- data[5 + 69] = 1;
Please explain the intention here. What is the issue with tfi_b? Why do you change it. Why do you pick the new bsn1_a and bsn1_b values
Tfi_b was filled inappropriately. According to 10.3a.4.1 in 44.060. bsn1_a and bsn1_b are changed to starting values of the sequence number which is 0 and 1.
Regards Bhargava Abhyankar
osmocom-net-gprs@lists.osmocom.org