Hi,

in the wake of https://gerrit.osmocom.org/1411 I have some more comments on the code that seem to large for the gerrit edit field.

I said in that patch comment that gprs_rlc_mcs_cps() must check that it isn't using INVALID enum values in its bitfield calculations. Furthermore:

The bit fields are obtained by modulo calculation on enum values. This seems unnecessarily complex and error prone, it is hard to figure out for a reader. The relation is quite direct though, e.g. for EGPRS_MAX_PS_NUM_2 it means that PS_1 and PS_3 mean + 0, PS_2 means + 1. I wish that were easier to see. It would be good to prepare that above the switch() and use it below instead of duplicating the same calculation numerous times.

Thirdly, for enum egprs_puncturing_values, the comment refers to TS 44.060 10.4.8a.3.1, 10.4.8a.2.1, 10.4.8a.1.1. When reading the first time, I saw table 10.4.8a.3.1 reflecting the names found in the table: PS1, PS2 and PS3. And it appeared that there are values missing. Typically, when we place a spec reference on an enum in Osmocom code, it means that this enum *exactly* represents the given table. This enum, however, is "merely" an internal indicator, later translated into the table's bit values. The enum definitions should have a comment sort of like "Puncturing scheme values. See gprs_rlc_mcs_cps()" without TS table references, and TS refs should be with that function.

It's also not helpful to place references to two more tables in the comment: the reader is left guessing what the exact relation is. If more than one TS table is involved, the comment should explain the relation.

Cosmetic: the switch() statement in that function has line breaking that doesn't match Osmocom coding style, making it even harder to read.

These are things we missed when accepting these changes. It would be nice to get them fixed at some point.

Thanks! ~N