osmo-pcu[master]: Split TS allocation into digestible pieces

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/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Sat Sep 2 17:35:03 UTC 2017


Patch Set 2: Code-Review-1

(9 comments)

A general problem is that the split of Algorithm B is super complex to review, yet this patch places more cosmetic changes on top -- tldr syndrome.

I accept that there is complex logic merely spread across functions to clarify, and now that I read these functions on their own I spot various little "API cracks"; i.e. that I am stricter on the code now that it is split into a smaller function, and would not have dared to touch the complexity otherwise.

That's why I'm stopping the review halfway through: I fear that I'm annoying more than helping and using our time for no benefit.

It would be easier to review this in smaller chunks :)

https://gerrit.osmocom.org/#/c/3760/2/src/bts.h
File src/bts.h:

Line 210
it would be nice to have a brief explanation in the commit log why this cust thing can be dropped.


https://gerrit.osmocom.org/#/c/3760/2/src/gprs_rlcmac_ts_alloc.cpp
File src/gprs_rlcmac_ts_alloc.cpp:

Line 390: 	if (trx) {
Does this function make sense with trx == NULL?  If yes, it needs to be documented above. If not, drop the check.


Line 848: /*! Assign fiven UL timeslots to UL TBF
"five"? "given"? :)


Line 872: /*! Assign fiven DL timeslots to DL TBF
same


Line 892: /*! Update timeslot counters
"Count used bits in slots and reserved_slots bitmasks"?
It doesn't really update anything, rather returns a count.


Line 912:  *  \returns UL or DL timeslot number
and document what is returned in *ts


Line 914: static uint8_t get_slots_single(gprs_rlcmac_trx *trx, const gprs_rlcmac_tbf *tbf, uint8_t dl_slots, uint8_t ul_slots,
the naming of "get_slots" + "single" cracks my head. Get several slots or a single one?


Line 923: 		return pcu_lsb(ret);
should *ts be assigned a value when we return here? If not, this special case needs documenting.


Line 925: 	return ret & (1 << (*ts));
It seems that we quite possibly return no TS number: if the dl_slots & ul_slots mask results in a bitmask that doesn't match 1 << *ts. Is that expected? Document it?


-- 
To view, visit https://gerrit.osmocom.org/3760
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I02da2b8ba8c9c8815dae0e39e1fed277ca0df171
Gerrit-PatchSet: 2
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list