Patch Set 1:

(5 comments)

https://gerrit.osmocom.org/#/c/7723/1/include/osmocom/bsc/acc_ramp.h
File include/osmocom/bsc/acc_ramp.h:

Line 145: void acc_ramp_disable(struct acc_ramp *acc_ramp);
> I think it makes more sense to have it as one function, no need for two:
Agreed. See next patch set.


https://gerrit.osmocom.org/#/c/7723/1/src/libbsc/acc_ramp.c
File src/libbsc/acc_ramp.c:

Line 34: static bool acc_is_enabled(struct gsm_bts *bts, unsigned int acc)
> Not directly related to this patch but:
I will address this in a separate patch.


Line 120:       if (acc_ramp->step_size == ACC_RAMP_STEP_SIZE_MAX) {
> I think we can drop this if and its contents completely in a follow up patc
Sure.


https://gerrit.osmocom.org/#/c/7723/1/src/libbsc/bsc_vty.c
File src/libbsc/bsc_vty.c:

Line 1918:              acc_ramp_init(&bts->acc_ramp, bts);
> I think it really makes sense to move _init inside gsm_bts_alloc_register n
I agree.

In the past moving acc_ramp related calls outside of libbsc was impossible 
because it created a chicken-and-egg depency loop between libcommon and libbsc.
I just found out that libcommon has been merged into libbsc so this is no 
longer an issue.

However, moving this acc_ramp_init() call today still creates build failures at 
link time because it ends up pulling additional symbols (e.g. from libvty) into 
the various programs under the tests/ directory. I know how to fix this but the 
necessary Makefile.am changes would at least double the size of this patch, so 
I'd rather leave it for later.


Line 3278:      acc_ramp_enable(&bts->acc_ramp);
> You want to dothe same in here (abort + set_system_infos) as you do in no_a
I don't understand what this would achieve. The 'enable' flag does not change 
any state related to an on-going ramping process. And setting system infos 
depends on the RSL link being up which is not the case while the config is 
being loaded. The effect of enabling this feature redundantly, via config file 
lines or via the VTY, should be a no-op because the feature is already enabled.

So I think this is fine as it is.
For clarity we could enable the feature only if ramping is not yet enabled, 
even though technically it already behaves as a no-op in this case (setting the 
flag to 'true' when it was already 'true'). See next patch set.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia25bff85d9e5c277da76bffa11d31972e9fdc323
Gerrit-PatchSet: 1
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: Pau Espin Pedrol <pes...@sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperl...@sysmocom.de>
Gerrit-HasComments: Yes

Reply via email to