osmo-pcu[master]: Modify EGPRS DL TBF flow to support SPB

[Neels Hofmeyr]

Patch Set 13:

(9 comments)

I have only cosmetic comments, but their number has some weight.
I wouldn't block the commit on these, but would be nice-to-have.
Some are definitely "wrong" while others are just my opinion.

https://gerrit.osmocom.org/#/c/655/13/src/pcu_vty.c
File src/pcu_vty.c:

Line 488: 	cfg_pcu_dl_arq_cmd,
the other DEFUN() in this file indent the arguments to line up with the first argument, i.e. 6 spaces instead of a tab.


https://gerrit.osmocom.org/#/c/655/13/src/rlc.h
File src/rlc.h:

Line 113: enum egprs_rlcmac_dl_spb_values {
an enum always contains "values", so this name could be shortened by dropping the "_values" part.


https://gerrit.osmocom.org/#/c/655/13/src/tbf.h
File src/tbf.h:

Line 424: 	enum egprs_rlc_dl_reseg_bsn_state egprs_dl_get_data_buffer
data is always in a buffer, maybe drop the "_buffer"


https://gerrit.osmocom.org/#/c/655/13/src/tbf_dl.cpp
File src/tbf_dl.cpp:

Line 383: 				bts->bts_data()->dl_arq_type);
I would add another tab indent for the arguments of get_retx_mcs()


Line 392: 			bts->bts_data()->dl_arq_type, bsn);
I would add another 5-spaces indent for the arguments of LOGP(). If the 80 characters width is too tight in this nested if(), it would be good to move this code block to a new function.


Line 660: 			EGPRS_RESEG_SECOND_SEG_SENT))
I find the indenting of the if condition confusing.
IMHO this would improve readability:

if (get_egprs_dl_spb_status(index) == EGPRS_RESEG_DL_DEFAULT ||
    get_egprs_dl_spb_status(index) == EGPRS_RESEG_SECOND_SEG_SENT)

The rest of this file does not use braces around '==' conditionals.


Line 1292: 						(const int bsn)
if you also drop the "_value" from "get_egprs_dl_spb_value", this will fit on one line.


https://gerrit.osmocom.org/#/c/655/13/tests/tbf/TbfTest.cpp
File tests/tbf/TbfTest.cpp:

Line 1839: 	if ((mcs == 6) && (demanded_mcs == 3)) {
basically the entire function is in an if {} block. Remove the indent by doing

  if (!(mcs == 6 && demanded_mcs == 3))
          return;

here ... unless you expect more else if() branches to be added later?


Line 1937: 		((mcs == 4) && (demanded_mcs == 1))) {
again, remove indent by if (![...]) return;


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I242afdd8ae7622dec8593b26382ad66bad5b9516
Gerrit-PatchSet: 13
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-Reviewer: arvind.sirsikar <arvind.sirsikar at radisys.com>
Gerrit-HasComments: Yes