Attention is currently required from: lynxis lazus.

Patch set 1:Code-Review -1

View Change

16 comments:

• File include/osmocom/bsc/nm_bts_ramp.h:

  • Patch Set #1, 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.

  • Patch Set #1, Line 40: /*!

    All this documentation we always put it in the .c file.

  • Patch Set #1, Line 43: * \param max_bts allow how many bts to provision

    how many concurrent right? so max_concurrent_bts.

  • Patch Set #1, Line 65: * \brief bts_ramp_unblock_bts

    IMprove documentation on what unblock means here.

• File src/osmo-bsc/bsc_vty.c:

  • Patch Set #1, Line 1284: "allow-bts-configuration <0-65535>",

    bts-unblock-bringup-ramping? The "allow-bts-configuration" thing is not really descriptivie, it's too generic.

  • Patch Set #1, 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).

  • Patch Set #1, 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:

  • Patch Set #1, 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:

  • Patch Set #1, 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);

  • Patch Set #1, 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.

  • Patch Set #1, Line 86: net->bts_ramp.count >= net->bts_ramp.max_bts)

    wrong indentation

  • Patch Set #1, Line 113: void bts_ramp_deactivate(struct gsm_network *net)

    bts_ramp_disable.

  • Patch Set #1, 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.

  • Patch Set #1, 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.

  • Patch Set #1, 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:

  • Patch Set #1, Line 162: case NM_EV_RAMP_GO:

    You should call configure_loop() here instead of copying the code again.

To view, visit change 29788. To unsubscribe, or for help writing mail filters, visit settings.