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? -- To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/29387 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-tetra Gerrit-Branch: master Gerrit-Change-Id: I41c9438b0b12c2fac9dff1b226eec5b33f30fbb4 Gerrit-Change-Number: 29387 Gerrit-PatchSet: 3 Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl> Gerrit-Comment-Date: Tue, 20 Sep 2022 12:17:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment