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