This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/osmocom-net-gprs@lists.osmocom.org/.
Holger Freyther holger at freyther.deHi, I am a bit lost with the review of the current osmo-pcu patchset. Going through the patches I have difficulties to see the red thread and what is better after we integrate the changes. I guess the goal is to add the allocation of multiple uplink timeslots but from my point of view this is buried in a lot of subjective sideway development. My wish would be: For multiple slot UL allocation: Make the most direct patchset to implement this feature. I understand that to avoid code duplication some methods can be re-used but instead of having 10 patches that change int->bool, change signatures of functions, maybe make it minimal to see how it works first? After it works send a cover mail that explains what and why you took the approach, what the _speed_ improvement for browsing is or when it is being used. What you see the danger in terms of common ts and allocation. For the clean-ups: I don't say we shouldn't do them but we should do them in a way they don't take much time to review and still provide a net gain to the contributor. A "cosmetic" patch that breaks the build is very suspicious and forces a reviewer to look extra carefully into these "clean-ups". int->bool: I think int->bool and 1->true adds little value for an application but if it is important then please do it by semantic patching so they can be easily reviewed. bts->trx: I think there is little value to change a function signature from getting a BTS to a TRX when the implementation starts to do: - bts->list + trx->bts->list "API" documentation: Comments can be helpful and it is good to explain on a high level what a function will do and what the side-effects will be. But in an application I think writing: \param[in,out] bts Pointer to BTS struct adds not enough value. First it is wrong as the type is not "BTS" but it is the "C" gprs_rlcmac_bts. And second doxygen will perfectly tell you the type of of the parameter anyway. Documenting that -1 of a parameter has a specific semantic can have some advantage but the risk of code/doc not agreeing is there as well. libosmocore: We do have a desire for reuse but everytime you make osmo-pcu depend on a unreleased version of libosmocore rebuilding osmo-pcu get's more difficult and the compile error is not obvious. It is not clear where the macro is coming from. So please keep this in mind and try to find a balance. thank you for your consideration holger