Attention is currently required from: laforge. wbokslag 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:

(3 comments)

File src/tetra_upper_mac.h:

https://gerrit.osmocom.org/c/osmo-tetra/+/29387/comment/fa659b74_5992edd1 PS3, Line 13: in

sounds like 'active' could be of type bool? […]

You're right, it's kind of underdocumented. I'll make sure to annotate the fields. Booleans are used nowhere in the osmo-tetra project, so I sticked with what seems to be convention. tetra_mac_state for instance uses a signed int for the is_traffic field. If you want, I can start using the bool type or make the active field and similar fields a uint8_t.

File src/tetra_upper_mac.c:

https://gerrit.osmocom.org/c/osmo-tetra/+/29387/comment/6894bd91_c3ed915d PS3, Line 44: s

this introduces a global, static variable. […]

The lower mac allocates a global context structure tetra_cell_data with global pointer tcd. I'd figure that it doesn't belong in that context. We could, however, add an upper mac context (upper_mac_context umc?), which, for now, would only hold the fragslots. Would that be preferable?

Note that passing the context by parameter will not work without changing a lot of unrelated prototypes, as that would imply the lower mac (and burst detector, etcetera) also need to have it. It would not be a bad idea to create a context in tetra-rx that contains the respective states for the burst detector, lower mac, upper mac and possibly layers above, but that would be a significant rework.

https://gerrit.osmocom.org/c/osmo-tetra/+/29387/comment/32214bbc_8fb3be83 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?

You're right, I could allocate them on demand, whenever a fragmented resource is encountered on a slot. I'll do that in the next patch set