Attention is currently required from: wbokslag. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/29387 )

Change subject: added support for fill bits and basic link defragmentation (also thanks to SQ5BPF) ......................................................................

Patch Set 3: Code-Review-1

(3 comments)

File src/tetra_upper_mac.h:

https://gerrit.osmocom.org/c/osmo-tetra/+/29387/comment/48383659_ca5a1eda PS3, Line 13: in sounds like 'active' could be of type bool? would be nice to have some comments explaining the contents for the non-obvious ones such as 'age' and 'encrypytion'.

The age comment should not just explain the units, but also explain the rationale of having a signed type for the age.

For 'encryption' it also would be good to explain what it is for. Is it just a flag? or an indication of a specific algorithm?... In the code I also see stuff like 'encryption = encryption_mode' where the latter is an unsigned type. So once again the question arises why a signed type is used to represent something that's unsigned.

File src/tetra_upper_mac.c:

https://gerrit.osmocom.org/c/osmo-tetra/+/29387/comment/33b82f9f_99caaf4d PS3, Line 44: s this introduces a global, static variable. Normally all state should be contained witin the tetra_mac_state or whatever other related 'context' structure to ensure the code works also if there are many instances in parallel, without any clashes over global variables storing state that logically belongs to an instance.

https://gerrit.osmocom.org/c/osmo-tetra/+/29387/comment/a2b9dee1_9d5a3db1 PS3, Line 50: fragslots[i].msgb = msgb_alloc(FRAGSLOT_MSGB_SIZE, "fragslot"); is there a specific reason to pre-allocate lots of message buffers here?