comments on enum egprs_puncturing_values and gprs_rlc_mcs_cps()

Neels Hofmeyr nhofmeyr at sysmocom.de
Fri Dec 16 11:39:32 UTC 2016


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


-- 
- Neels Hofmeyr <nhofmeyr at sysmocom.de>          http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschäftsführer / Managing Directors: Harald Welte
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.osmocom.org/pipermail/osmocom-net-gprs/attachments/20161216/b4f658d1/attachment.bin>


More information about the osmocom-net-gprs mailing list