Attention is currently required from: arehbein. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 )

Change subject: WIP: vty: Add check against sensible default value for Ny1 ......................................................................

Patch Set 9:

(13 comments)

File src/osmo-bsc/bts.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/594c7528_17c128f5 PS9, 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.

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/7d9f4a2b_889360b2 PS9, 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".

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/8dbb7c58_755159a9 PS9, 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".

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/06324143_a7020bb8 PS9, 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?

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/7f753c77_4c7e325d PS9, 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).

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/0c071ad3_7c42de9e PS9, Line 1740: unsigned long ny1 = osmo_tdef_get_entry(bts->network->T_defs, -3105)->val; Same, osmo_tdef_get()

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/a5045235_1cecaf44 PS9, Line 1742: bool ny1_satisfies_requirements = T3105 * ny1 > T3124 + GSM_NY1_REQ_DELTA; We usually declare variables at the start of the function/block.

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/856f773a_855077d4 PS9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1; Why this +1? can you explain?

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/11c8a152_b2b01c89 PS9, 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:

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/7767e628_93eaad1c PS9, 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:

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/642a3ec1_9fa75aec PS9, 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).

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/16fd2c9f_9bb0546c PS9, 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.

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/591c2218_e53633da PS9, 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.