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.
-
pespin