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 Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/be16332f_8dbbaec5 PS5, Line 842: if (gl) { Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/0cb1d49b_a2d42c7f PS5, Line 855: talloc_strdup_append Done File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/c5ee481a_36fe55a8 PS1, Line 535: unsigned long ny1; 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) 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.")) { 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. 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) Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/ebf5a96d_e97a1565 PS9, Line 1729: struct gsm_lchan *gl = gsm_bts_get_cbch(bts); 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; 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; 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; 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; 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; 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; 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."); 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; 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; 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, Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/f03bc13c_0b4053c7 PS9, Line 161: trx = talloc_zero(ctx, struct gsm_bts_trx); 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 <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 02 Jan 2023 22:55:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment