Current osmo-pcu patch set

[Max]

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.

-- 
Max Suraev <msuraev at sysmocom.de> http://www.sysmocom.de/
======================================================================= 
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93 
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B 
* Geschaeftsfuehrer / Managing Director: Harald Welte