Today I looked at enum egprs_puncturing_values by coincidence:
/* * Valid puncturing scheme values * TS 44.060 10.4.8a.3.1, 10.4.8a.2.1, 10.4.8a.1.1 */ enum egprs_puncturing_values { EGPRS_PS_1, EGPRS_PS_2, EGPRS_PS_3, EGPRS_PS_INVALID, };
I notice that TS 44.060 10.4.8a.3.1 defines further items besides PS_1, 2 and 3, and that our EGPRS_PS_INVALID occupies the value the spec defines as "MCS-3/P1". I believe that this is rather unfortunate.
[[[ Table 10.4.8a.3.1: Coding and Puncturing Scheme indicator field for Header type 3
bits 4321 First block CPS ---------------------------- 0000 MCS-4/P1 0001 MCS-4/P2 0010 MCS-4/P3 0011 MCS-3/P1 0100 MCS-3/P2 0101 MCS-3/P3 0110 MCS-3/P1 with padding (MCS-8 retransmission) 0111 MCS-3/P2 with padding (MCS-8 retransmission) 1000 MCS-3/P3 with padding (MCS-8 retransmission) 1001 MCS-2/P1 1010 MCS-2/P2 1011 MCS-1/P1 1100 MCS-1/P2
NOTE: All the other values are reserved for future use The bit numbering is relative to the field position. ]]]
I would prefer EGPRS_PS_INVALID=-1 (i.e. outside the spec's value range) and the other enum values named appropriately, like EGPRS_MSC4_P1, so that our enum actually reflects the spec as advertised. Is there something I'm missing?
These enum values were added in: commit 7a05b039c835868eff34308d861edfeb28d1763b Author: Aravind Sirsikar arvind.sirsikar@radisys.com Date: Wed Mar 23 18:29:45 2016 +0530
Thanks,
~N
Hi Neels,
AFAICS you are mixing up "puncturing scheme" and "Coding and Puncturing Scheme", there are still only three puncturing schemes, where up to 3 of them may be used with a certain MCS. The P scheme has to be changed (incremented IIRC) after each use.
Nevertheless choosing a safe value for INVALID is probably not bad if there might be another P in some future spec. But I'd be careful with negative values in enums.
(I didn't check that against the current version of the code, so I might be completely out of sync).
HTH Jacob
On 12/15/2016 12:45 PM, Neels Hofmeyr wrote:
Today I looked at enum egprs_puncturing_values by coincidence:
/*
- Valid puncturing scheme values
- TS 44.060 10.4.8a.3.1, 10.4.8a.2.1, 10.4.8a.1.1
*/ enum egprs_puncturing_values { EGPRS_PS_1, EGPRS_PS_2, EGPRS_PS_3, EGPRS_PS_INVALID, };
...
I would prefer EGPRS_PS_INVALID=-1 (i.e. outside the spec's value range) and the other enum values named appropriately, like EGPRS_MSC4_P1, so that our enum actually reflects the spec as advertised. Is there something I'm missing?
These enum values were added in: commit 7a05b039c835868eff34308d861edfeb28d1763b Author: Aravind Sirsikar arvind.sirsikar@radisys.com Date: Wed Mar 23 18:29:45 2016 +0530
Thanks,
~N
On Thu, Dec 15, 2016 at 02:36:23PM +0100, Jacob wrote:
Hi Neels,
Hey Jacob!
AFAICS you are mixing up "puncturing scheme" and "Coding and Puncturing Scheme", there are still only three puncturing schemes, where up to 3 of them may be used with a certain MCS. The P scheme has to be changed (incremented IIRC) after each use.
Well, if it brings you back into Osmocom dev, I might want to start mixing up things a lot more from now on ;)
Thanks for your remarks, I have since noticed that I let myself be mislead by the first spec reference in the comment. I find it confusing, but I've posted a separate mail on that...
Nevertheless choosing a safe value for INVALID is probably not bad if there might be another P in some future spec. But I'd be careful with negative values in enums.
I thought negative values in enums are fine? What could be a problem here? e.g.
enum my_vals { MY_VAL_INVALID = -1, MY_VAL_A = 0, MY_VAL_B, MY_VAL_C, };
~N
Hi Neels,
On 12/16/2016 12:57 PM, Neels Hofmeyr wrote:
On Thu, Dec 15, 2016 at 02:36:23PM +0100, Jacob wrote:
I thought negative values in enums are fine? What could be a problem here? e.g.
enum my_vals { MY_VAL_INVALID = -1, MY_VAL_A = 0, MY_VAL_B, MY_VAL_C, };
I checked that with C99 and you are right. According to §6.7.2.2 (4) the compiler must use a signed type for the enum if at least one constant is a valid negative int. Nevertheless I had some issues with assigning -1 to an enum some time ago, but it might well be that there was no negative constant involved and I went into some implicit signed/unsigned conversion hell with differing enum bit sizes or something like that.
Sorry for the noise.
Jacob
~N
osmocom-net-gprs@lists.osmocom.org
-
Jacob -
Neels Hofmeyr