This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/osmocom-net-gprs@lists.osmocom.org/.
Holger Freyther holger at freyther.de> On 07 Apr 2016, at 13:57, Bhargava Abhyankar <Bhargava.Abhyankar at 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