Patch Set 1: (6 comments)
Assuming these tables are correctly copy and pasted. What is the minimal dependency to get this in? Which patch removes the <= 32 assumption? tests/alloc/AllocTest.cpp requires updates to use/test the new classes as well. The commit message needs some work. You are not "adding missing" classes but you extend from one version of the specification to the other. Also indicate which devices use the classes and if you had a change to verify it.. Also how you intend to address the 35-45 TA issue. The comment that old code had it too might be misleading as the requirement didn't exist for the other classes or if they did we have more work to do on the tables. https://gerrit.osmocom.org/#/c/4072/1//COMMIT_MSG Commit Message: Line 10: Emacs macros into C struct to avoid typos. We don't do reformat and extend in one go. We rarely do. As a reviewer I still have to see if things change. :( Line 13: yet (this was the case with the old code too). Good to describe it. https://gerrit.osmocom.org/#/c/4072/1/src/gprs_rlcmac_ts_alloc.cpp File src/gprs_rlcmac_ts_alloc.cpp: Line 41: /* FIXME: use actual TA offset for computation - make sure to adjust "1 + MS_TO" accordingly */ Good to communicate it. Line 52: /* | Rx Tx Sum | Tta Ttb Tra Trb | */ this header is nice Line 54: /* 1 */ { 1, 1, 2, 3, 2, 4, 2, 1 }, To be more interesting we can even do [0] = {}, [1].. but that is not a call to action just to illustrate that there are always things to do better but the cost vs. value vs. danger might not be big enough to warrant making a change. Line 83: /* 30 */ { 5, 1, 6, 2, 1, 1, 1, 1 }, Any idea why we padded the structs before? Were they not assigned? Why did we extend it? -- To view, visit https://gerrit.osmocom.org/4072 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ef2eb99c517f25e7d1e71b985a3e0eb3879eb2c Gerrit-PatchSet: 1 Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Holger Freyther <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
