Patch Set 2: Code-Review-1 (11 comments)
https://gerrit.osmocom.org/#/c/6324/2/include/osmocom/bsc/acc_ramp.h File include/osmocom/bsc/acc_ramp.h: Line 1: /* (C) 2018 Stefan Sperling <ssperl...@sysmocom.de> (C) sysmocom / Author: Stefan Sperling, please look at other examples, thanks. Line 41: struct gsm_bts *bts; /* backpointer to BTS using this ACC ramp */ one could avoid the back-pointer if we used offsetof() to go back from acc_ramp to gsm_bts, assuming that the acc_ramp is always embedded there. Or one could even modify the API to directly operate on gsm_bts rather than acc_ramp. But these are just random thoughts, we can keep it as is. It's just a bit unusual for Osmocom to have a static, embedded structure which then has a back-pointer. https://gerrit.osmocom.org/#/c/6324/2/include/osmocom/bsc/gsm_data_shared.h File include/osmocom/bsc/gsm_data_shared.h: Line 792: struct acc_ramp acc_ramp; continuing form my previous comment: If the struct acc_ramp was dynamically allocated, then one would need (and expect) a back pointer. And here one could remove the acc_ramping_enabled member and simply check for if (bts->acc_ramp) ? Again just ideas, no requirement to change. https://gerrit.osmocom.org/#/c/6324/2/src/libbsc/acc_ramp.c File src/libbsc/acc_ramp.c: Line 68: static void allow_all_allowed_accs(struct acc_ramp *acc_ramp) the naming is a bit ... odd. PS2, Line 93: DRLL this is not related to the Radio Link Layer (which is a sub-layer of RSL). We need to find a more matching log subsystem. Line 98: static void send_bts_system_info(struct gsm_bts *bts) this should become a public function that's part of the general bts/trx/sysinfo code, rather than a private/static function here. Line 107: static void do_ramping_step(void *data) 'acc' in the name might be useful in backtraces/debugging/... to indicate which of the various ramps this function adrresses Line 169: LOGP(DRLL, LOGL_DEBUG, "(bts=%d) ACC RAMP: ramping step size set to %u\n", acc_ramp->bts->nr, step_size); see above, RLL is not applicable in all the log statements in this file. https://gerrit.osmocom.org/#/c/6324/2/src/libbsc/bsc_init.c File src/libbsc/bsc_init.c: Line 335: acc_ramp_start(&trx->bts->acc_ramp); your first ramp step is executed before the call to gsm_bts_trx_set_system_infos(). So you will send system information messages to the BTS which might contain lots of uninitialized fields, as far as I can tell. Maybe we need to split the "set the systeminfo buffers from vty config data" and the "send system infos to BTS" parts into separate functions? https://gerrit.osmocom.org/#/c/6324/2/src/libbsc/bsc_vty.c File src/libbsc/bsc_vty.c: PS2, Line 3135: enabled (0|1) while we have some commands like this dating back to the early days, it is discouraged these days. "(enabled|disabled)" would be an alternative. And the true "Switch/Router style" would be to have "access-control-class-ramping" and "no access-control-class-ramping". https://gerrit.osmocom.org/#/c/6324/2/src/libbsc/system_information.c File src/libbsc/system_information.c: Line 662: static void apply_acc_ramp(struct gsm48_rach_control *rach_control, struct acc_ramp *acc_ramp) I think that should be part of the acc related code, not here? At that point, for exported/non-static functions, we prefer to have the object name first, i.e. acc_ramp_apply(). -- 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: 2 Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Owner: Stefan Sperling <ssperl...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Stefan Sperling <ssperl...@sysmocom.de> Gerrit-Reviewer: Vadim Yanitskiy <axilira...@gmail.com> Gerrit-HasComments: Yes
