Attention is currently required from: fixeria, laforge, neels, pespin.
arehbein 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 17:
(20 comments)
Patchset:
PS12:
> Hmm, the task this patch has is actually quite difficult, for these reasons:
> […]
There also seems to be some other ambiguity I think concerning some of the
T1-T5 timers that comes from the specs (I recall seeing different descriptions
for some of the Ti for i between 1 and 5..., so some of those timer numbers may
be context related. Don't know anymore where I saw it though).
Initial thoughts towards a solution:
We could switch all the remaining BTS timers that are still global to per-BTS
as a first step (make the per-BTS timer then override the global one?).
Then maybe it would be good to somehow add syntax recognition for counters
('Nnnn') in the tdef API.
Introduce a counter_id field and give all counters a tdef_id of INT_MIN (?)
Might be a bit involved to implement these things without breaking older
code/binaries
File include/osmocom/bsc/bts.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/c1343fe6_251dab13
PS12, Line 323: GSM_BTS_TDEF_ID_COUNTDOWN_VALUE
> These are not standard 3GPP specified timer numbers, so they should not be
> positive. […]
as alluded to above, I was told it'd be good to use positive values for
anything coming from the specs and that negative values were reserved for extra
stuff from Osmocom only.
I switched back to negative ones for now, but it appears to be wrong either way
for now.
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/d212f932_a13c5b9e
PS16, Line 720: enum gprs_rlc_par {
> I bet this is not needed anymore and can be dropped?
Done
File src/osmo-bsc/bsc_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/b776ff34_c10d350f
PS16, Line 51: extern void bts_gprs_timer_groups_init(struct gsm_bts *bts);
> why don't you add this to a header file?
it was only needed by `src/osmo-bsc/bsc_init.c` - apart from the file
`tests/nanobts_omlattr/nanobts_omlattr_test.c`, which is for testing only, and
so far my impression has been that we don't usually add stuff to header files
in such a situation.
I have now moved it to `gsm_bts_alloc()`, so it only appears in `bts.c` and
`bts_init.c` and isn't needed in the test file anymore
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/840d6c93_21da7d9b
PS16, Line 240: bts_gprs_timer_groups_init(bts);
> why is this put here and not in gsm_bts_alloc_register() ?
(as mentioned above:) I have moved it to `gsm_bts_alloc()`; the function
description seemed like a better fit than `gsm_bts_alloc_register()`, which
inits more general information than timers/counters
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/7721eed5_5234f8f6
PS12, Line 353: bts_write_group_timers
> So once we have found the matching group, don't we want to `break` the loop?
the loop works the other way round, it continues if there is no match
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/a037ff6a_44fc6994
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, a […]
I looked at what current code does and no or most gprs command didn't work
unless we're in gprs mode - especially existing timer commands. So I extended
what the bsc already did and assumed it was implemented that way for a reason
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/28181ff3_fbebee6c
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 […]
hm yeah looks like nonsense. I've updated this
File src/osmo-bsc/bts_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/e654143f_8d0ae7ea
PS12, Line 41: _templates
> (I think the term "template" is fine and accurate, i assume no vty edits
> these)
@nhofm...@sysmocom.de yes, the vty doesn't edit any of it.
@vyanits...@sysmocom.de the timer entries here are used like a template, i.e.
being copy-pasted to every BTS. They're not used as reference for resetting
information.
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/23628941_27a1c276
PS12, Line 56: Extended uplink TBF
> This does not really explain what this timer is for. Same for DL below.
Done
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/a4c7b767_02307d0b
PS12, Line 60: 3101
> I actually find this problematic. […]
(this reply is older, hadn't yet published it...)
@vyanits...@sysmocom.de
Using the tdef API for counters: I'm aware that there is a difference. Pau told
me back when I introduced Ny1 configuration, that it should be added via the
tdef API (see
https://gerrit.osmocom.org/c/osmo-bsc/+/30539/comment/a3ffa35b_1a41b537/ ...),
so I did the same here, because I didn't see any reason to use the tdef API for
one counter, but not for others.
Concerning this patch, I was messaging with Pau on Xabber and was told that he
would go for positive values for 'Nxyz' - I originally (until patchset 2) had
intended to use negative values for anything that wasn't specifically mentioned
as 'Tnnn' in the specs with basically the same reasoning as you mentioned...)
unless there was any specific problem. Didn't see T3101, but I guess it
qualifies as such a problem.
Not sure what to do with conflicting directions like these.
(new part of reply):
I have for now finished the patch series with these timers with Nnnn now being
given a negative TID '-nnn'
File src/osmo-bsc/bts_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/d33352f3_d33e79c1
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_ […]
that looks wrong to me, I have replaced the line marked with
`memcpy(bts->timer_groups, bts_gprs_timer_template_groups,
sizeof(bts_gprs_timer_template_groups));`
File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/d1a20102_88900584
PS12, Line 253: struct abis_nm_ipacc_att_rlc_cfg
> cosmetic: it's not necessary to specify the struct type here.
this wasn't my commit
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/9f75f14f_6e50a199
PS3, Line 1706: /* TODO (BSSGP timer patch): If argument is one of
BSSGP Timers strings, then translate to
> So if this patch is for RLC; why are you touching BSSGP lines here?
what lines are you referring to? This is just a TODO comment for the BSSGP
timer patch
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/7250a80d_2d32c81b
PS3, Line 2068:
> In general, the tdefs API was made modular with flexible re-use in mind. […]
alright, then I'll be working on a libosmocore patch for that then.
I was tempted to adapt the existing code to remove/adapt the check about
whether or not the global tdef group has been set previously
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/df070208_c6e80cd2
PS12, Line 56: limits.h
> Ack
Done
File tests/nanobts_omlattr/nanobts_omlattr_test.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/b1188505_100e78e3
PS2, Line 228: 0x02, 0x00, 0x02, 0xa3, 0x00, 0x09,
> I have aligned all columns vertically
Done
File tests/nanobts_omlattr/nanobts_omlattr_test.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/03e843c4_1d8f0b8a
PS16, Line 37: extern void bts_gprs_timer_groups_init(struct gsm_bts *bts);
> don't we have headers for this?
see answer above concerning this function and its declarations
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/09a66cc8_d20523a3
PS16, Line 38: extern void bts_grprs_tdef_groups_init(void);
> typo: grprs.
Done
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/80802c28_fc78f23c
PS16, Line 158: bts_grprs_tdef_groups_init();
> typo: grprs.
Done
--
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: 17
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: laforge <lafo...@osmocom.org>
Gerrit-Attention: pespin <pes...@sysmocom.de>
Gerrit-Attention: fixeria <vyanits...@sysmocom.de>
Gerrit-Comment-Date: Tue, 05 Dec 2023 08:57:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofm...@sysmocom.de>
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
