fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/15807 )
Change subject: osmo-bts-trx/scheduler: refactor Uplink measurement processing ...................................................................... Patch Set 3: Hi Alexander, > Could you explain why the result is no correct? because in the current code we merely pass measurements of the last burst to L1SAP, see: https://git.osmocom.org/osmo-bts/tree/src/osmo-bts-trx/scheduler_trx.c#n1228 https://git.osmocom.org/osmo-bts/tree/src/osmo-bts-trx/scheduler_trx.c#n1256 https://git.osmocom.org/osmo-bts/tree/src/osmo-bts-trx/scheduler_trx.c#n1439 https://git.osmocom.org/osmo-bts/tree/src/osmo-bts-trx/scheduler_trx.c#n1468 > If that's because of each burst affecting two frames [...] Exactly! A TCH/F or FACCH/F frame is interleaved over 8 consecutive bursts (228 even numbered bits of the first 4 bursts and 228 odd numbered bits of the last 4 bursts), a TCH/H (speech) frame is interleaved over 4 consecutive bursts (114 even numbered bits of the first 2 bursts and 114 odd numbered bits of the last 2 bursts), and a FACCH/H frame is interleaved over 6 consecutive bursts (114 even numbered bits of the first 2 bursts, all bits of the middle 2 bursts, and 114 odd numbered bits of the last 2 bursts). > why not just have an avg counter per frame and add values to them accordingly? Because a) we allocate a frame (actually, a L1SAP primitive) _after_ decoding of bursts, not before; b) before decoding, we don't know whether this is a FACCH or a TCH frame. > If there is something I missed why the above can't be done and we do need to > store values [...] Averaging on the fly _could_ be implemented for TCH/F and FACCH/F by having two separate measurement buffers, but definitely not for TCH/H and FACCH/H because they have different interleaving periods. > why not use an array? We know that we need 8 values max, so why not allocate > an array of 8 elements to avoid linked list operations? We basically need a LIFO stack, so we push new measurements on top of it and remove old ones from the tail every time we decode a new frame. It should be possible to implement this using an array, so doing a lookup (i.e. just referencing by index) will be much faster, but then pushing a new set of measurements will be slower as we would need to shift the existing measurements right (copy arr[i] to arr[i + 1]) every time we receive a burst. Given that we're pushing new measurements much more often than we average them, I would not go for that. Alternatively, we could use a FIFO stack and always push to the end. Then we still would need to shift the measurements left (by copying arr[i] to arr[i - 1]) as soon as we decode a new frame. So I still think that traversing through a linked list is not that bad. And having the measurement history in general gives us a bonus: we can easily lookup TDMA frame number of the first burst instead of using ugly formulas or the block mapping tables from 3GPP 45.002. Also, I've got a few optimization ideas: 1) we can combine averaging and squashing into a single function, so that would be done in a single iteration; 2) TDMA frame number lookup can also be combined into the averaging function; 3) we can use a talloc pool, so malloc() will not be a problem anymore. -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/15807 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I6bc511223069f66b49109d3267bee7bd89585713 Gerrit-Change-Number: 15807 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria <axilira...@gmail.com> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <axilira...@gmail.com> Gerrit-Reviewer: ipse <alexander.cheme...@gmail.com> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Sun, 20 Oct 2019 17:52:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
