18 Oct
2022
18 Oct
'22
11:55 a.m.
Attention is currently required from: lynxis lazus.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/29788
)
Change subject: Add BTS ramping to prevent mass configuration of BTS at the same time
......................................................................
Patch Set 1: Code-Review-1
(16 comments)
File include/osmocom/bsc/nm_bts_ramp.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/83cf1130_b414ce9b
PS1, Line 36: bool bts_ramp_active(struct gsm_network *net);
bts_ramp ("BTS ramp" concept I already saw written in the commit decription) is
too generic.
What about bts_bringup_ramp or bts_setup_ramp?
I'd also drop the "nm" part in the file name, and use same bts_bringup_ramp
or similar as in the APIs.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/1add034f_d401edfb
PS1, Line 40: /*!
All this documentation we always put it in the .c file.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/13b189a2_b159ce0d
PS1, Line 43: * \param max_bts allow how many bts to provision
how many concurrent right? so max_concurrent_bts.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/fc7b6b68_876c67d6
PS1, Line 65: * \brief bts_ramp_unblock_bts
IMprove documentation on what unblock means here.
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/56175227_38cc6403
PS1, Line 1284: "allow-bts-configuration <0-65535>",
bts-unblock-bringup-ramping? The "allow-bts-configuration" thing is not really
descriptivie, it's too generic.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/8bef54db_7d66d863
PS1, Line 2659: vty_out(vty, " bts ramp limit %d within %d seconds%s",
description all way to generic. One really needs to look at the code to understand what
the limit means (units).
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/ec8d391f_3eab7192
PS1, Line 3087: "bts ramp limit <0-999> within <0-999>
seconds",
We already have "access-control-class-ramping" set of commands in osmo-bsc and
"power-ramp" set of commands in osmo-bts.
Both use same terminology and format, so let's better use the same here.
That'd probably be:
no bts-bringup-ramping
bts-bringup-ramping
bts-bringup-ramping-step-interval <0-999>
bts-bringup-ramping-step-size <0-999>
BTW I see no reason to limit the time interval to 0-999.
File src/osmo-bsc/nm_bb_transc_fsm.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/3cf29b4d_bb22ee50
PS1, Line 345: case NM_EV_RAMP_GO:
Since this only makes sense when in DISABLED_DEPENDENCY or DISABLED_OFFLINE, better move
it (duplicated with different boolean passed) to each of those 2 states.
This makes all the FSMs far easier to follow.
Do it for all the FSM files you modify.
File src/osmo-bsc/nm_bts_ramp.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/1992775a_d7a98f97
PS1, Line 42: /* inform all MOs */
It may make sense to have this moved into a function in nm_common_fsm.c (with generic
event and data pointers).
nm_fsm_dispatch_all(int action, void *data);
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/fa2de165_7c1c2f12
PS1, Line 63: if (llist_entry_empty(&bts->bts_ramp.list))
did you think about initing the list during constructor time? then you wouldn't need
this kind of stuff afaiu.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/d6e9134e_e67bc558
PS1, Line 86: net->bts_ramp.count >= net->bts_ramp.max_bts)
wrong indentation
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/6545f276_ce0244f3
PS1, Line 113: void bts_ramp_deactivate(struct gsm_network *net)
bts_ramp_disable.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/d33aad6c_e04504ce
PS1, Line 121: bool bts_ramp_active(struct gsm_network *net)
"active" would be count < max_bts.
What you mean here is probably "bts_ramp_enabled".
Active sounds more like there's BTS in the list still waiting.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/384a5d40_1f95815a
PS1, Line 143: OSMO_ASSERT(llist_entry_empty(&bts->bts_ramp.list));
if you remove the BTS previously using bts_ramp_remove() then this will most probably
assert.
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/62cfd077_87e2e672
PS1, Line 172: if (!llist_entry_empty(&bts->bts_ramp.list))
does this API check if the list is all zeroed?
File src/osmo-bsc/nm_bts_sm_fsm.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/846ba00b_ff6eb759
PS1, Line 162: case NM_EV_RAMP_GO:
You should call configure_loop() here instead of copying the code again.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/29788
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Id56dde6d58f3d0d20352f6c306598d2cccc6345d
Gerrit-Change-Number: 29788
Gerrit-PatchSet: 1
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Tue, 18 Oct 2022 09:55:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment