Attention is currently required from: arehbein, neels, pespin.
Patch set 12:Code-Review -1
10 comments:
File include/osmocom/bsc/bts.h:
Patch Set #12, 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:
Patch Set #12, 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:
Patch Set #12, Line 41: _templates
I am wondering why `_templates`? `_def` or `_defaults` maybe?
Patch Set #12, Line 56: Extended uplink TBF
This does not really explain what this timer is for. Same for DL below.
I actually find this problematic.
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:
Patch Set #12, 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:
Patch Set #3, Line 1896: static int parse_timer_arg_fmt(struct vty *vty, const char *timer_str, int *tid)
Just as a general comment on why a lot of code has been replicated from libosmocore with slight alte […]
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:
Patch Set #12, Line 39: osmocom/bsc/bts.h
this header is already included below...
Patch Set #12, 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:
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 change 31878. To unsubscribe, or for help writing mail filters, visit settings.