16 Mar
2016
16 Mar
'16
3:16 p.m.
On 10 Mar 2016, at 06:45, Aravind Sirsikar
<Arvind.Sirsikar(a)radisys.com> wrote:
Hi,
the main issue is already in the subject of the patch. "Do this ... and ...
that" is better done as two separate patches.
Let me just give you one example
-int Decoding::rlc_parse_ul_data_header(struct
gprs_rlc_ul_header_generic *rlc,
- const uint8_t *data, GprsCodingScheme cs)
+int Decoding::decode_ul_data_fanr_type1(struct gprs_rlc_ul_header_generic *rlc,
+ const uint8_t *data, GprsCodingScheme* cs)
{
- const struct rlc_ul_header *gprs;
unsigned int data_len = 0;
+ data_len = cs->maxDataBlockBytes();
+
+ const struct gprs_rlc_ul_header_egprs_fanr_type1 *egprs1;
+ egprs1 = static_cast<struct gprs_rlc_ul_header_egprs_fanr_type1*>
+ ((void *)data);
+ rlc->r = egprs1->r;
+ rlc->si = egprs1->si;
+ rlc->tfi = (egprs1->tfi_a << 0) | (egprs1->tfi_b << 2);
+ rlc->cps = egprs1->cps;
+ rlc->rsb = egprs1->rsb;
+ rlc->cv = egprs1->cv;
+ rlc->pani = egprs1->pani;
+
+ rlc->num_data_blocks = 2;
+ rlc->block_info[0].cv = egprs1->cv;
+ rlc->block_info[0].pi = egprs1->pi;
+ rlc->block_info[0].bsn =
+ (egprs1->bsn1_a << 0) | (egprs1->bsn1_b << 5);
+
+ rlc->block_info[0].spb = 0;
+
+ rlc->block_info[1].cv = egprs1->cv;
+ rlc->block_info[1].pi = egprs1->pi;
+
+ rlc->block_info[1].bsn = rlc->block_info[0].bsn + ((egprs1->bsn2_a << 0)
| (egprs1->bsn2_b << 2));
+ rlc->block_info[1].bsn = (rlc->block_info[1].bsn + 2048) % 2047;
+
+ rlc->block_info[0].e = (data[6] & 0x01);
+ rlc->block_info[0].ti = (data[6] & 0x02) >> 1;
+ rlc->block_info[1].e = (data[6 + data_len + 1] & 0x01);
+ rlc->block_info[1].ti = (data[6 + data_len + 1] & 0x02) >> 1;
+ rlc->block_info[1].spb = 0;
+
+ rlc->block_info[0].data_len = data_len;
+ rlc->block_info[1].data_len = data_len;
+ rlc->data_offs_bytes[0] = 7;
+ rlc->data_offs_bytes[1] = 7 + data_len + 1;
+ return 0;
+}
+
+int Decoding::rlc_parse_ul_data_header(struct
gprs_rlc_ul_header_generic *rlc,
+ const uint8_t *data, GprsCodingScheme cs)
- rlc->pani= 0;
+ rlc->pani = 0;
please don't do drive-by changes.
- case GprsCodingScheme::HEADER_EGPRS_DATA_TYPE_3:
- const struct gprs_rlc_ul_header_egprs_3 *egprs3;
- egprs3 = static_cast<struct gprs_rlc_ul_header_egprs_3 *>
- ((void *)data);
- rlc->r = egprs3->r;
- rlc->si = egprs3->si;
- rlc->tfi = (egprs3->tfi_a << 0) | (egprs3->tfi_b
<< 2);
- rlc->cps = (egprs3->cps_a << 0) | (egprs3->cps_b
<< 2);
- rlc->rsb = egprs3->rsb;
- rlc->cv = egprs3->cv;
- /* pani 0 for fanr inactive */
- rlc->pani = 0;
-
- rlc->num_data_blocks = 1;
- rlc->block_info[0].cv = egprs3->cv;
- rlc->block_info[0].pi = egprs3->pi;
- rlc->block_info[0].spb = egprs3->spb;
- rlc->block_info[0].bsn =
- (egprs3->bsn1_a << 0) | (egprs3->bsn1_b <<
5);
-
- rlc->block_info[0].e = (data[4] & 0x01);
- rlc->block_info[0].ti = (data[4] & 0x02) >> 1;
-
- rlc->block_info[0].data_len = data_len;
- rlc->data_offs_bytes[0] = 5;
-
- if(rlc->block_info[0].spb == 2){
- if(cs == GprsCodingScheme::MCS3){
- if(rlc->cps == 6 || rlc->cps == 7 || rlc->cps == 8){
- rlc->block_info[0].data_len -= pad_bytes;
- rlc->data_offs_bytes[0] += pad_bytes;
- rlc->block_info[0].e = (data[4 + pad_bytes] & 0x01);
- rlc->block_info[0].ti = (data[4 + pad_bytes] & 0x02)
>> 1;
- }
- }
- }
- break;
Okay, it is not the same method that was moved but as a rule of thumb. if you move code,
first copy the code in a commit and then modify it. This makes the actual change visible.
With a patch series you tell a story and need to leave the code in a better state than it
was before and additional features. To do this we have small commits.
holger