Attention is currently required from: neels, fixeria, pespin. 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 16: (8 comments) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/02b3622f_27b56219 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. Done

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/1d37c437_221f9e75 PS9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1;

the standard idiom for integer division rounding up is: […] I'm not sure I understand... if 'a', 'b' are wlog nonnegative integers and b is nonzero, then 'a/b = rounddown(a/b) + r' with 'r in [0,1)', which goes along with my earlier reply.

Since we are only using unsigned types this should be alright. I can still change it if it's nonetheless important File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/05e83da7_2b401911 PS14, Line 537: }

(no need for braces when the body is a single line) Done

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/059d9674_7d8f6fa0 PS14, Line 1725: otherwise return a suggestion for Ny1

this is still not true Done

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/d79887c1_16a8e4b6 PS14, Line 1726: the message to print

this also needs to be updated, I guess (I see no msg argument) Done

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/298a4763_7b98d011 PS14, Line 1729: const struct gsm_lchan *gl = gsm_bts_get_cbch(bts);

(re earlier question, 'lchan' stands for 'logical channel', a sub-division of a BTS timeslot) thanks :)

https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/00ced4a7_f056287f PS14, Line 1738: unsigned long T3105 = osmo_tdef_get(bts->network->T_defs, 3105, OSMO_TDEF_MS, -1);

in osmocom style we are required to declare all variables at the top of the scope that was my 'reasoning' as well, didn't see it in our guidelines. I've changed it though

File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/35711518_f3d8897a PS16, Line 95: (void)

drop the cast to void? Done

-- 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: 16 Gerrit-Owner: arehbein <arehbein@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-CC: neels <nhofmeyr@sysmocom.de> Gerrit-CC: pespin <pespin@sysmocom.de> Gerrit-Attention: neels <nhofmeyr@sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy@sysmocom.de> Gerrit-Attention: pespin <pespin@sysmocom.de> Gerrit-Comment-Date: Tue, 03 Jan 2023 00:19:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein@sysmocom.de> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> Comment-In-Reply-To: pespin <pespin@sysmocom.de> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> Gerrit-MessageType: comment