osmo-bsc[master]: Add support for Access Control Class ramping.

Harald Welte gerrit-no-reply at lists.osmocom.org
Mon Feb 26 13:27:50 UTC 2018


Patch Set 7:

(6 comments)

looks great, aside from some cosmetic issues. The uint16_t is an optional idea (isn't it simpler?) as is enabled-in-struct-acc_ramp, but all other comments I would like to see adressed. thanks.

https://gerrit.osmocom.org/#/c/6324/7//COMMIT_MSG
Commit Message:

Line 17:   access-control-class-ramping-step-interval (<30,600>|dynamic)
30-600 not 30,600? same below?


Line 29: ramping stepm with a 'dyanmic' step interval. This means the
dynamic


https://gerrit.osmocom.org/#/c/6324/7/include/osmocom/bsc/acc_ramp.h
File include/osmocom/bsc/acc_ramp.h:

PS7, Line 55: 8_t barred_t2;
            : 	uint
I would have used one uint16_t bit mask in order to make it simpler to set the bit for one given ACC? Maybe I'm thinking too complicated ;)


Line 77:  * Initialize an acc_ramp data structure.
We're not using doxygen inside applications at this point, but as you're adding extensive API docs it makes sense to go for doxygen syntax, even if it's not generating any docs yet.


https://gerrit.osmocom.org/#/c/6324/7/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:

Line 950: 	bool acc_ramping_enabled;
why not make the 'enabled' flage a member of struct acc_ramp?


https://gerrit.osmocom.org/#/c/6324/7/src/libbsc/acc_ramp.c
File src/libbsc/acc_ramp.c:

Line 23: #include <errno.h>
sttbool as you're using  bool?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a5ac3a08f992f326435944f17e0a9171911afb0
Gerrit-PatchSet: 7
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-HasComments: Yes


More information about the gerrit-log mailing list