This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Stefan Sperling gerrit-no-reply at lists.osmocom.org
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 <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-HasComments: Yes