osmo-pcu[master]: Fix slot allocation based on direction configured

[Neels Hofmeyr]

Patch Set 1: Code-Review-1

(15 comments)

https://gerrit.osmocom.org/#/c/819/1//COMMIT_MSG
Commit Message:

Line 9: Earlier number of TS allocation for second  DL  TBF was less compared
(some extra spaces around 'DL')


Line 10: Compared to first TBF. As the allocation was considering combined capacity
'compared Compared'

and your sentence is weird: "Earlier allocation for second was less than first" ?? Keep it simple :)


Line 11: for TS allocation which was causing inconsistencies.
I don't understand the "As the allocation ... was causing 
inconsistencies" sentence at all. What inconsistency in
detail?


Line 12: This patch controls the TS allocation based on direction configured.
"controls the allocation"? Again rather name it in detail
what the patch changes.


https://gerrit.osmocom.org/#/c/819/1/src/bts.cpp
File src/bts.cpp:

Line 629: 	if (bts->capacity_type == 1)
looks like we'd typically use a switch() here, instead.


https://gerrit.osmocom.org/#/c/819/1/src/bts.h
File src/bts.h:

Line 203: 	 */
Do you intentionally define the values 1, 2, 3 to map to the
enum values 0, 1, 2? If capacity_type == 0 is not a valid
indicator for empty, I'd rather define values 0, 1, 2.

If == 0 is a valid indicator, you'd also need a kind of
DL_NONE enum value to return in get_ts_allocation_type().


Line 294: 	/* get TS allocation type */
The comment is worthless as it reflects the function name 1:1.


https://gerrit.osmocom.org/#/c/819/1/src/gprs_rlcmac_ts_alloc.cpp
File src/gprs_rlcmac_ts_alloc.cpp:

Line 780: 		if (capacity_type == UL_ONLY) {
naming the value gotten from get_ts_allocation_type() again
'capacity_type' like the uint8_t in struct gprs_rlcmac_bts made
me think that this is a mapping mistake between the enum and the uint8_t.

What is it, a capacity_type or a ts_allocation_type?
To make things worse, the enum is called allocation_capacity.
Better to stick with one naming consistently.


https://gerrit.osmocom.org/#/c/819/1/src/pcu_main.cpp
File src/pcu_main.cpp:

Line 220: 	bts->capacity_type = 3;
I dislike that you're using the numbers 1, 2, 3 in various
places to mean things instead of defining as names in an enum.
If enum allocation_capacity matched the uint8_t values, you could use those names.


https://gerrit.osmocom.org/#/c/819/1/src/pcu_vty.c
File src/pcu_vty.c:

Line 504:       "enable DL capacity based allocation support\n"
So to summarize, "the capacity type is a TS allocation that is configured
based on capacity; I can enable support for DL or UL". I find this rather
hard to understand, I think you can do better than this.

It looks like you need to decide for one proper name and
description and use that consistently everywhere, and try
to drop meaningless buzzwords where possible.

Maybe you could first describe how this works in detail in the commit
log, and then we can come back to a crisp and concise summary here?


Line 513: 	else
it is impossible to enter this 'else', since the VTY will
only allow exactly either 'dl' or 'ul' to be passed.
Do you mean to have '(dl|ul|both)' above?


https://gerrit.osmocom.org/#/c/819/1/tests/alloc/AllocTest.cpp
File tests/alloc/AllocTest.cpp:

Line 829: 	printf("TBF1: numTs1(%d)\n", numTs1);
I don't see why you'd add a "1" to "numTs", it says TBF1 already.
If you need to, this should already have been so in the previous patch.


Line 841: 	OSMO_ASSERT(numTs2 == numTs1);
(obsolete assertion Ts2 == Ts1)


Line 865: 
unrelated whitespace ... you should know by now ;)
Please read your own patches?


https://gerrit.osmocom.org/#/c/819/1/tests/alloc/AllocTest.ok
File tests/alloc/AllocTest.ok:

Line 10798: TBF2: numTs2(4)
see, now we have an added 1 and 2 instead of just the
second TBF being fixed to be 4.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4b4a99194ccae68bb3417bce538d16e944d5ec71
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: arvind.sirsikar <arvind.sirsikar at radisys.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes