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; 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; 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. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 9 Gerrit-Owner: arehbein &lt;arehbein(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: arehbein &lt;arehbein(a)sysmocom.de&gt; Gerrit-Comment-Date: Mon, 19 Dec 2022 10:51:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment