Attention is currently required from: arehbein, fixeria, laforge, neels. pespin 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 16: Code-Review-1 (16 comments) File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/805a2cc6_e0ac3ba7 PS16, Line 720: enum gprs_rlc_par { I bet this is not needed anymore and can be dropped? File src/osmo-bsc/bsc_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/709588ee_c5374837 PS16, Line 51: extern void bts_gprs_timer_groups_init(struct gsm_bts *bts); why don't you add this to a header file? https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/1ea08409_2909a72c PS16, Line 240: bts_gprs_timer_groups_init(bts); why is this put here and not in gsm_bts_alloc_register() ? File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/d8c3ec12_bf937d5a PS16, Line 235: You could submit this a a preparation patch. https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/5059a9c4_d631760d PS16, Line 336: int rc = CMD_WARNING, bts_nr = atoi(argv[0]); please use separate lines. https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/200c39ee_c7676da8 PS16, Line 346: if (bts->gprs.mode == BTS_GPRS_NONE) { why can't I configure gprs timers if gprs is not currently enabled? I don't see a problem with it, and several usability problems, like having to comment all timer lines if I decide to start the app with "gprs mode none". https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/07070b25_bd8b2072 PS16, Line 351: ++group; we tend to use group++ unless there's a good reason to use preinc. https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/ffb743b5_be8f4972 PS16, Line 354: if (bts_write_group_timers(vty, "", bts_nr, group, T_arg, false) == CMD_WARNING) I don't really understand what are you doing here comparing against == CMD_WARNING and then doing rc = CMD_SUCCESS. File src/osmo-bsc/bts_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/fb231078_729120e6 PS12, Line 60: 3101 > I actually find this problematic. […] Simply use 3101 for T3101, and -3101 (X3101) for N3101. File src/osmo-bsc/bts_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/dde08777_91ad0b5f PS16, Line 88: for (gbtg = 0; gbtg < ARRAY_SIZE(bts_gprs_timer_template_groups); gbtg++) memcpy(&bts->timer_groups[0], bts_gprs_timer_template_groups[0], ARRAY_SIZE(bts_gprs_timer_template_groups)); File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/882fe6f8_b5ac08bf PS12, Line 253: struct abis_nm_ipacc_att_rlc_cfg > cosmetic: it's not necessary to specify the struct type here. Ack File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/c071f5c0_bce88d2d PS3, Line 1706: /* TODO (BSSGP timer patch): If argument is one of BSSGP Timers strings, then translate to > My understanding so far has been, that we have three timer groups that relate > to OS#5335 (e.g. […] So if this patch is for RLC; why are you touching BSSGP lines here? File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/28ee33b2_d47d1a37 PS12, Line 56: limits.h > why including thise header? I cannot see `_MIN` or `_MAX` in new code... Ack File tests/nanobts_omlattr/nanobts_omlattr_test.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/ad2e2f37_e3dc11a2 PS16, Line 37: extern void bts_gprs_timer_groups_init(struct gsm_bts *bts); don't we have headers for this? https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/165508af_c16add51 PS16, Line 38: extern void bts_grprs_tdef_groups_init(void); typo: grprs. https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/b11f60da_c8b22194 PS16, Line 158: bts_grprs_tdef_groups_init(); typo: grprs. -- 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: 16 Gerrit-Owner: arehbein <arehb...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanits...@sysmocom.de> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Attention: neels <nhofm...@sysmocom.de> Gerrit-Attention: arehbein <arehb...@sysmocom.de> Gerrit-Attention: laforge <lafo...@osmocom.org> Gerrit-Attention: fixeria <vyanits...@sysmocom.de> Gerrit-Comment-Date: Tue, 28 Nov 2023 13:04:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: arehbein <arehb...@sysmocom.de> Comment-In-Reply-To: fixeria <vyanits...@sysmocom.de> Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
