Attention is currently required from: neels, msuraev. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/31286 )
Change subject: rlcmac: Introduce DL TBF creation through PCH ImmAss ......................................................................
Patch Set 5:
(3 comments)
File include/osmocom/gprs/rlcmac/tbf_dl.h:
https://gerrit.osmocom.org/c/libosmo-gprs/+/31286/comment/889c0970_09976e8d PS5, Line 9: //#include <osmocom/gprs/rlcmac/tbf_dl_fsm.h>
What's the point of commented include? Just drop it if it's unused.
It will be added soon in a follow up commit. Having it here it's just saving me time adding it later everywhere. I don't see a problem with having this here since anyway this is not fully usable for regular end users yet.
File src/rlcmac/rlcmac.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/31286/comment/85f0ecf9_393b37c0 PS5, Line 291: LOGRLCMAC(LOGL_NOTICE, "TODO: handle decoded dl ctrl block!\n");
hmm, a TODO in the logs? maybe a #warning pragma instead?
no, TODO is fine because we are actively developing this and we want to clearly see if some part of the code is missing at runtime when doing tests.
File src/rlcmac/tbf_dl.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/31286/comment/398ddb6d_9f24f648 PS5, Line 39: // goto err_tbf_destruct;
i can understand that some code will come in only later, but you do add an awful lot of these tempor […]
All of them comments are related to the dl_tbf_fsm, they relate to the same part of the code. If this really annoys you I can spend time dropping it here and adding it in one of next patches I have to work on. It will simply make me or anyone else taking over this lose time finding out what needs to be added where. This is all the boilerplate code to alloc+initialize+free the state FSM of a dl_tbf, similar to what we already have partially implemented for ul_tbf.
-
pespin