Attention is currently required from: arehbein, neels, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email ) Change subject: Introduce per-BTS timers, RLC timer commands ...................................................................... Patch Set 12: Code-Review-1 (10 comments) File include/osmocom/bsc/bts.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/2d7bae3a_8d5e8eaf PS12, Line 323: GSM_BTS_TDEF_ID_COUNTDOWN_VALUE These are not standard 3GPP specified timer numbers, so they should not be positive. The usual practice in Osmocom is to use negative numbers for custom timer numbers, so that they show up as X1234 instead of T1234. File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/5afd0fe2_569b3318 PS12, Line 353: bts_write_group_timers So once we have found the matching group, don't we want to `break` the loop? File src/osmo-bsc/bts_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/f0e57fd2_65445e6e PS12, Line 41: _templates I am wondering why `_templates`? `_def` or `_defaults` maybe? https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/9ed2643f_a959f2a3 PS12, Line 56: Extended uplink TBF This does not really explain what this timer is for. Same for DL below. https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/7c26fbb8_009132ce PS12, Line 60: 3101 I actually find this problematic. * 3GPP TS 44.018 defines T3101 in section 11.1.2 "Timers on the network side"; * 3GPP TS 44.060 defines N3101 in section 13.4 "Counters on the Network side". The timer and counter are not the same thing. In this patch you're making a counter value N3101 configurable via `timer` related VTY commands. I find this extremely confusing, especially given that `T3101` in the VTY would actually reference a counter! Same applies to N3103 and N3105. I don't know what would be a good way to solve this though. Maybe @nhofmeyr@sysmocom.de could suggest something, since he worked with timers a lot and AFAIR he wrote the tdef API. File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/3e561314_e2bea10b PS12, Line 253: struct abis_nm_ipacc_att_rlc_cfg cosmetic: it's not necessary to specify the struct type here. File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/7333c7aa_29321e2f PS3, Line 1896: static int parse_timer_arg_fmt(struct vty *vty, const char *timer_str, int *tid) I understand your motivation, this is indeed a problem. But maybe we should invest time into solving the fundamental problem in libosmocore, rather than replicating various stuff from there? Maybe also something @nhofmeyr@sysmocom.de can comment on. File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/c54dc09a_d87c74e9 PS12, Line 39: osmocom/bsc/bts.h this header is already included below... https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/76c3bb2c_351f2192 PS12, Line 56: limits.h why including thise header? I cannot see `_MIN` or `_MAX` in new code... File tests/nanobts_omlattr/nanobts_omlattr_test.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/b1b4b444_65dd4b6e PS12, Line 138: 3142 Why do these new timers need to be in `gsm_network_T_defs[]`? They are per-BTS timers, while this is a global array for the whole network. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675 Gerrit-Change-Number: 31878 Gerrit-PatchSet: 12 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 15 Nov 2023 07:05:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment