Attention is currently required from: pespin, fixeria. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 )
Change subject: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 13: (20 comments) File include/osmocom/bsc/bts.h: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/4f43fbc7_33887c4a PS5, Line 839: struct gsm_lchan *gl > > it's the initials of `gsm_lchan`. […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/be16332f_8dbbaec5 PS5, Line 842: if (gl) { > I would actually prefer it as is, at least I find there is a gain in > readability that makes up for a […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/0cb1d49b_a2d42c7f PS5, Line 855: talloc_strdup_append > Ah yeah thanks! I don't know what I was doing there Done File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/c5ee481a_36fe55a8 PS1, Line 535: unsigned long ny1; > It's totally fine to include logging in bts. […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/c8103c5e_c976ecd1 PS1, Line 654: unsigned long gsm_bts_check_ny1(struct gsm_bts *bts, unsigned long *ny1_out) > Since this is only needed in this file, set the function to static (move it > above where it is used). Done (considering your approval of having the check called during runtime) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/5e69a9f2_e409af94 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. Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/71d7596e_b5df306b PS9, Line 1725: /* Return 'true' iff ny1 satisfies network requirements, otherwise return a suggestion for ny1. > "Ny1" (see initial cap). […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/dfeaacec_e0ac0b9f 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". Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/ebf5a96d_e97a1565 PS9, Line 1729: struct gsm_lchan *gl = gsm_bts_get_cbch(bts); > we usually use "lchan" as variable name for a struct gsm_lchan. […] According to the specs, T3124 is set depending on the channel type (I assume 'lchan' stands for 'link channel' or something like that (?)). So I'm using that information to create the lower bound for Ny1. https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/fc980505_41fbc2cc PS9, Line 1738: unsigned long T3105 = osmo_tdef_get_entry(bts->network->T_defs, 3105)->val; > You can use osmo_tdef_get() here. […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/4de4ec1e_c5b781dd PS9, Line 1740: unsigned long ny1 = osmo_tdef_get_entry(bts->network->T_defs, -3105)->val; > Same, osmo_tdef_get() Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/93138ff7_6012304c 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. Is this a hard rule of ours or a preference? If there is any leeway, I'd rather leave it as is. https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/564a86e5_f09ca4c9 PS9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1; > Why this +1? can you explain? Without the '+ 1', due to integer arithmetic in C, we would be setting ny1 to the largest integer smaller or equal to the rational number (T3124 + delta)/T3105. With it we are setting it to the smallest integer larger than that number which from my understanding is what we would want (assuming 'delta' is 'good'). https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/b721b2b8_9547a352 PS9, Line 1748: return ny1_satisfies_requirements; > You can simplify all this code with early return: […] I considered this. I put '!(T3105 * ny1 > T3124 + GSM_NY1_REQ_DELTA)' as the condition so as to preserve the original inequality mentioned everywhere else. File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/eb3067ba_4f6aac89 PS1, Line 96: unsigned long ny1; > ACK, feel free to leave it there then. Done File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/aef3b8e9_6a164c62 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. It was 'Value of Ny1 too low' and here it is 'should be higher'. But I got rid of the message string itself as a parameter, so it doesn't matter anymore (only the log level is a parameter now). File tests/nanobts_omlattr/nanobts_omlattr_test.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/94cc6064_f8100cd0 PS9, Line 127: static unsigned long const T3105_TESTVAL = 13; > > Move const after static - use 'static const unsigned long' […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/c7d11e80_4d36ce84 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' […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/d714a30b_faddbdec PS9, Line 133: { .T = -3105, .default_val = GSM_NY1_DEFAULT, .val = NY1_TESTVAL, .unit = OSMO_TDEF_CUSTOM, > Ah I see now that you actually this this T3015 here because it's used when > test calls the check_ny1 […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/f03bc13c_0b4053c7 PS9, Line 161: trx = talloc_zero(ctx, struct gsm_bts_trx); > I think this test is missing a calling to osmo_tdefs_reset(net->T_defs); I tried it out but that wasn't actually missing (the test is setting the necessary values) -- 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: 13 Gerrit-Owner: arehbein <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Mon, 02 Jan 2023 22:55:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
