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

[Neels Hofmeyr]

Patch Set 15: Code-Review-1

(5 comments)

Thank you for your patience ... I still found some issues.

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

Line 489:       "dl arq-type (0|1)",
I notice that 0 is for EGPRS_ARQ1 and 1 is for EGPRS_ARQ2.
This is confusing; as holger likes to say: the vty is for humans.

So IMHO you should have (1|2) and explicitly translate those
to EGPRS_ARQ1 and EGPRS_ARQ2 in the arg parsing, preferably
even with a switch() or if() statement, to decouple.

Even better could be to use string arguments like
(spb|arq2) instead of the numbers. Make it most convenient for humans ;)


Line 490:       EGPRS_STR "DL ARQ options\n"
ah, missed this one before: EGPRS_STR is only for "egprs", not for "dl".

i.e. EGPRS_STR should be dropped, and the doc string for "arq-type" is missing.


Line 491:       "enable DL SPB support\n"
is SPB == ARQ1? I'm not familiar with the topic, but if
it is, the doc string could mention both?


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

Line 632: 	unsigned int spb_status = get_egprs_dl_spb_status(index);
nice.


Line 1215: 					uint8_t **block_data)
this now fits on one line.


-- 
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: 15
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