Attention is currently required from: arehbein.

View Change

13 comments:

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

  • Patch Set #9, Line 535: if (!gsm_bts_check_ny1(bts, LOGL_ERROR, "Value of Ny1 too low.")) {

    This string can probably go inside the gsm_bts_check_ny1 function.

  • Patch Set #9, Line 1725: /* Return 'true' iff ny1 satisfies network requirements, otherwise return a suggestion for ny1.

    "Ny1" (see initial cap).
    BTW I think this comment needs to be updated, I don't see it returning any "suggestion".

  • Patch Set #9, Line 1727: bool gsm_bts_check_ny1(struct gsm_bts *bts, unsigned int log_level, char const *msg)

    const always first. Also, "msg" name is usually used for struct msgb, better call it "str".

  • Patch Set #9, Line 1729: struct gsm_lchan *gl = gsm_bts_get_cbch(bts);

    we usually use "lchan" as variable name for a struct gsm_lchan.
    Why are you getting a CBCH lchan here btw?

  • Patch Set #9, Line 1738: unsigned long T3105 = osmo_tdef_get_entry(bts->network->T_defs, 3105)->val;

    You can use osmo_tdef_get() here. Passing -1 to "val_if_not_present" param akes care of proper exiting if the timer is not found (shouldn't happen).

  • Patch Set #9, Line 1740: unsigned long ny1 = osmo_tdef_get_entry(bts->network->T_defs, -3105)->val;

    Same, osmo_tdef_get()

  • Patch Set #9, Line 1742: bool ny1_satisfies_requirements = T3105 * ny1 > T3124 + GSM_NY1_REQ_DELTA;

    We usually declare variables at the start of the function/block.

  • Patch Set #9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1;

    Why this +1? can you explain?

  • Patch Set #9, Line 1748: return ny1_satisfies_requirements;

    You can simplify all this code with early return:

    if (T3105 * ny1 < T3124 + GSM_NY1_REQ_DELTA) {
        unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1;
        LOGP(DNM, log_level, "%s Is: %lu Lowest recommendation: %lu\n", msg,
    		     ny1, ny1_recommended);
        return false;
    }
    return true;

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

  • Patch Set #9, Line 96: (void) gsm_bts_check_ny1(bts, LOGL_NOTICE, "Value of Ny1 should be higher.");

    In one function is "should be lower" and here "should be higher"? This probably makes no sense.

• File tests/nanobts_omlattr/nanobts_omlattr_test.c:

  • Patch Set #9, Line 127: static unsigned long const T3105_TESTVAL = 13;

    Move const after static - use 'static const unsigned long'

    Please fix. const always at the start (well, after "static" in this case as the linter says).

  • Patch Set #9, Line 130: static unsigned long const NY1_TESTVAL = (GSM_T3124_MAX + GSM_NY1_REQ_DELTA)/T3105_TESTVAL + 1;

    Move const after static - use 'static const unsigned long'

    Please fix.

  • Patch Set #9, Line 133: { .T = -3105, .default_val = GSM_NY1_DEFAULT, .val = NY1_TESTVAL, .unit = OSMO_TDEF_CUSTOM,

    My understanding from the comments above is that you want to set Ny1 to have it applied in the attr_bts_expected[] below. That's fine.
    If T3105 is not needed in the test, then simply don't set it and drop the comment.

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