Hi.

Thank you for your feedback and efforts in patch review.

On 21.09.2017 05:22, Holger Freyther wrote:

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.

It might make sense to review the ticket mentioned in commit message when reviewing commits. It'll remove the need in guessing most of the time.

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.

It is rather big development which is also interspersed with other unrelated tasks I work on. That's why instead of submitting huge patch bomb I'm sending partially completed work the moment it reaches some milestone: the set of patches which I think is ready for review. That way we increase the chance of more thorough review and it allows to incorporate feedback early on: it's easier to change 2 recent patches than to apply changes to patch #2 in a series of 22 patches for example.

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?

I wholeheartedly agree but unfortunately I have troubles understanding and following undocumented and unformatted code with 5 nested for(); in it. That's why the first step was to unwind this code into smaller chunks, understand and document them, and to make further changes on top of that.

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.

I'll certainly do that but I still think that it's worth it to get the feedback early on, even if the code is not ready for browser testing yet. Hence the current patch submission.

What you see the danger in terms of common ts and allocation.

Could you please elaborate? I'm not following.

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.

Could you please point me to an example of such patch in one of the Osmocom projects? Would be nice to have it as a guideline/blueprint of how it should be done. Also, do we have a wiki page which explains it? It would be helpful to know how it could make my life as a reviewer easier too.

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

That's not part of the patchset in question (if I guessed correctly and you mean TS alloc related work). The list of related changes could be found in "Related Changes" menu in gerrit web UI.

"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.

It would be more helpful if this sort of feedback would be provided via gerrit because there it's easy to point to particular line of code. That way we can be sure that such oversights are quickly corrected. Thank you.

And second doxygen will perfectly tell you the type of of the parameter anyway.

Could you please provide a better doxygen line as an example? How would you document it to make it more useful?

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.

Well, yes. There's always a risk that code will disagree with the docs. What exactly do you propose to do to mitigate it?

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.

I think this is generic issue unrelated to OsmoPCU or libosmocore. There are some attempts to rectify it in https://osmocom.org/projects/cellular-infrastructure/wiki/Make_a_new_release as well as in this ML. Don't hesitate to contribute your ideas on what the right balance should be and how the concrete procedures should look like to the wiki.

Thank you again for taking time to make a review and for sharing it.