[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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 18: Code-Review-1 (2 comments) Patchset: PS18: Some of my previous comments haven't been addressed. File tests/bts_features.vty: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/2662ad2e_c1d4bc20 PS18, Line 66: OsmoBSC(config-net-bts)# gprs show timer this is unexpected from user point of view. Users expect all commands showing state of stuff to start with "show", then press TAB to see the possibilities. -- 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: 18 Gerrit-Owner: arehbein Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Attention: arehbein Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Comment-Date: Thu, 14 Dec 2023 12:32:25 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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 18: (4 comments) File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/53580373_9da62382 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. […] functionality has been moved to libosmocore via https://gerrit.osmocom.org/c/libosmocore/+/35355/9 https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/3421d774_7ed97e36 PS3, Line 2068: > alright, then I'll be working on a libosmocore patch for that then. […] https://gerrit.osmocom.org/c/libosmocore/+/35355/9 File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/1959fe8a_d26a2edd PS12, Line 39: osmocom/bsc/bts.h > this header is already included below... Done File tests/nanobts_omlattr/nanobts_omlattr_test.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/84cfe61b_286e6ebc PS12, Line 138: 3142 > Why do these new timers need to be in `gsm_network_T_defs[]`? […] 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: 18 Gerrit-Owner: arehbein Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-Comment-Date: Thu, 14 Dec 2023 00:38:45 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein Comment-In-Reply-To: neels Comment-In-Reply-To: fixeria Comment-In-Reply-To: pespin Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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 18:
(1 comment)
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/78218b76_b9a0e2af
PS16, Line 346: if (bts->gprs.mode == BTS_GPRS_NONE) {
> I think for setting timers is totally fine being able to configure them,
> otherwise config files are […]
It used to be exactly the same for the existing gprs timer commands. I have
removed the check.
--
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: 18
Gerrit-Owner: arehbein
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: neels
Gerrit-Attention: laforge
Gerrit-Attention: pespin
Gerrit-Attention: fixeria
Gerrit-Comment-Date: Thu, 14 Dec 2023 00:25:29 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, fixeria, laforge, neels.
Hello Jenkins Builder, fixeria, laforge, neels, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
to look at the new patch set (#18).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
Change subject: Introduce per-BTS timers, RLC timer commands
..
Introduce per-BTS timers, RLC timer commands
- Add per-BTS timer groups ('rlc' only for now, others to follow)
- Add vty commands & tests for those timers
Requires: libosmocore.git Ib3b22640a11eae152d66d62c0f47b953d80de945
Related: OS#5335
Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
---
M include/osmocom/bsc/bts.h
M include/osmocom/bsc/gsm_data.h
M include/osmocom/bsc/vty.h
M src/osmo-bsc/bsc_init.c
M src/osmo-bsc/bsc_vty.c
M src/osmo-bsc/bts.c
M src/osmo-bsc/bts_init.c
M src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c
M src/osmo-bsc/bts_vty.c
M src/osmo-bsc/pcu_sock.c
M tests/bts_features.vty
M tests/nanobts_omlattr/nanobts_omlattr_test.c
12 files changed, 253 insertions(+), 65 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/78/31878/18
--
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: 18
Gerrit-Owner: arehbein
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: arehbein
Gerrit-Attention: neels
Gerrit-Attention: laforge
Gerrit-Attention: fixeria
Gerrit-MessageType: newpatchset
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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 17:
(3 comments)
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/4c9b74ec_01aa9c47
PS16, Line 235:
> You could submit this a a preparation patch.
This has been ignored.
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/22fb9289_e4403213
PS16, Line 346: if (bts->gprs.mode == BTS_GPRS_NONE) {
> I looked at what current code does and no or most gprs command didn't work
> unless we're in gprs mode […]
I think for setting timers is totally fine being able to configure them,
otherwise config files are going to be failing if someone disables gprs
temporarily.
Please remove this check, I see no good use for it.
https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/5a8d06ab_2271abb0
PS16, Line 351: ++group;
> we tend to use group++ unless there's a good reason to use preinc.
this comment was ignored.
--
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
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: arehbein
Gerrit-Attention: neels
Gerrit-Attention: laforge
Gerrit-Attention: fixeria
Gerrit-Comment-Date: Tue, 05 Dec 2023 11:48:33 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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 17: (7 comments) File include/osmocom/bsc/bts.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/410d863c_6d55ea01 PS12, Line 323: GSM_BTS_TDEF_ID_COUNTDOWN_VALUE > as alluded to above, I was told it'd be good to use positive values for > anything coming from the spe […] Not "anything coming from the specs", but counters/timers explicitly specified with a given T123 or N123 naming. Hence, since Countdown value is not aexplicitly defined as a T... or N... it must be done as negative (X...). File src/osmo-bsc/bsc_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/7fb0226f_ec45c056 PS16, Line 240: bts_gprs_timer_groups_init(bts); > (as mentioned above:) I have moved it to `gsm_bts_alloc()`; the function > description seemed like a b […] Done File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/cb270503_135b1952 PS17, Line 466: bts_gprs_timer_groups_init(bts); did you make sure all counters set up here are not used beforehand in this function when initializing stuff? I bet some counter may be, so this should be far above. File src/osmo-bsc/bts_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/987e30c1_d72af677 PS12, Line 60: 3101 > (this reply is older, hadn't yet published it...) […] Done File src/osmo-bsc/bts_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/7e893e33_fe82bedb PS17, Line 57:.desc = "\"In the extended uplink TBF mode, the uplink TBF may be maintained during temporary inactive periods, " "Keep uplink TBF alive during inactive periods where the mobile station has no RLC information to transmit (3GPP TS 44.060 Version 6.14.0)" https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/d3a42a61_ee58d91c PS17, Line 61:.desc = "A delayed release of the downlink TBF is when the release of the downlink TBF is delayed following the transmission of a final data block, " "Delay release of downlink TBF after all available data has been transmitted" File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/3e9bb2c4_44fcf5d0 PS3, Line 1706: /* TODO (BSSGP timer patch): If argument is one of BSSGP Timers strings, then translate to > what lines are you referring to? This is just a TODO comment for the BSSGP > timer patch aha, I'll rephrase: so if it's a a patch about RLC, why are you adding a TODO about BSSGP timers here? You can simplify this patch since you are anyway adding them in a follow up patch, it's not something left over "for later". -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Attention: arehbein Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Comment-Date: Tue, 05 Dec 2023 11:45:26 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein Comment-In-Reply-To: fixeria Comment-In-Reply-To: pespin Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, fixeria, laforge, neels, pespin.
Hello Jenkins Builder, fixeria, laforge, neels, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
to look at the new patch set (#17).
The following approvals got outdated and were removed:
Code-Review-1 by pespin, Verified+1 by Jenkins Builder
Change subject: Introduce per-BTS timers, RLC timer commands
..
Introduce per-BTS timers, RLC timer commands
- Add per-BTS timer groups ('rlc' only for now, others to follow)
- Add vty commands & tests for those timers
Related: OS#5335
Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
---
M include/osmocom/bsc/bts.h
M include/osmocom/bsc/gsm_data.h
M include/osmocom/bsc/vty.h
M src/osmo-bsc/bsc_init.c
M src/osmo-bsc/bsc_vty.c
M src/osmo-bsc/bts.c
M src/osmo-bsc/bts_init.c
M src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c
M src/osmo-bsc/bts_vty.c
M src/osmo-bsc/pcu_sock.c
M tests/bts_features.vty
M tests/nanobts_omlattr/nanobts_omlattr_test.c
12 files changed, 477 insertions(+), 65 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/78/31878/17
--
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
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: neels
Gerrit-Attention: arehbein
Gerrit-Attention: laforge
Gerrit-Attention: pespin
Gerrit-Attention: fixeria
Gerrit-MessageType: newpatchset
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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(>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
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: neels
Gerrit-Attention: arehbein
Gerrit-Attention: laforge
Gerrit-Attention: fixeria
Gerrit-Comment-Date: Tue, 28 Nov 2023 13:04:06 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: arehbein
Comment-In-Reply-To: fixeria
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, fixeria, laforge, neels, pespin.
Hello Jenkins Builder, fixeria, laforge, neels, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
to look at the new patch set (#16).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
Change subject: Introduce per-BTS timers, RLC timer commands
..
Introduce per-BTS timers, RLC timer commands
- Add per-BTS timer groups ('rlc' only for now, others to follow)
- Add vty commands & tests for those timers
Related: OS#5335
Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
---
M include/osmocom/bsc/bts.h
M include/osmocom/bsc/gsm_data.h
M include/osmocom/bsc/vty.h
M src/osmo-bsc/bsc_init.c
M src/osmo-bsc/bsc_vty.c
M src/osmo-bsc/bts.c
M src/osmo-bsc/bts_init.c
M src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c
M src/osmo-bsc/bts_vty.c
M src/osmo-bsc/pcu_sock.c
M tests/bts_features.vty
M tests/nanobts_omlattr/nanobts_omlattr_test.c
12 files changed, 473 insertions(+), 50 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/78/31878/16
--
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
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: neels
Gerrit-Attention: arehbein
Gerrit-Attention: laforge
Gerrit-Attention: fixeria
Gerrit-Attention: pespin
Gerrit-MessageType: newpatchset
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, fixeria, laforge, neels, pespin.
Hello Jenkins Builder, fixeria, laforge, neels, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
to look at the new patch set (#15).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
Change subject: Introduce per-BTS timers, RLC timer commands
..
Introduce per-BTS timers, RLC timer commands
- Add per-BTS timer groups ('rlc' only for now, others to follow)
- Add vty commands & tests for those timers
Related: OS#5335
Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
---
M include/osmocom/bsc/bts.h
M include/osmocom/bsc/gsm_data.h
M include/osmocom/bsc/vty.h
M src/osmo-bsc/bsc_init.c
M src/osmo-bsc/bsc_vty.c
M src/osmo-bsc/bts.c
M src/osmo-bsc/bts_init.c
M src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c
M src/osmo-bsc/bts_vty.c
M src/osmo-bsc/pcu_sock.c
M tests/bts_features.vty
M tests/nanobts_omlattr/nanobts_omlattr_test.c
12 files changed, 474 insertions(+), 50 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/78/31878/15
--
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: 15
Gerrit-Owner: arehbein
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: neels
Gerrit-Attention: arehbein
Gerrit-Attention: laforge
Gerrit-Attention: fixeria
Gerrit-Attention: pespin
Gerrit-MessageType: newpatchset
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, fixeria, laforge, neels, pespin.
Hello Jenkins Builder, fixeria, laforge, neels, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/31878?usp=email
to look at the new patch set (#13).
The following approvals got outdated and were removed:
Code-Review+1 by laforge, Code-Review-1 by fixeria, Code-Review-1 by neels,
Verified+1 by Jenkins Builder
Change subject: Introduce per-BTS timers, RLC timer commands
..
Introduce per-BTS timers, RLC timer commands
- Add per-BTS timer groups ('rlc' only for now, others to follow)
- Add vty commands & tests for those timers
Related: OS#5335
Change-Id: I2c24110d8c977d6cc74c3c8e77bcc709ad9d2675
---
M include/osmocom/bsc/bts.h
M include/osmocom/bsc/gsm_data.h
M include/osmocom/bsc/vty.h
M src/osmo-bsc/bsc_init.c
M src/osmo-bsc/bsc_vty.c
M src/osmo-bsc/bts.c
M src/osmo-bsc/bts_init.c
M src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c
M src/osmo-bsc/bts_vty.c
M src/osmo-bsc/pcu_sock.c
M tests/bts_features.vty
M tests/nanobts_omlattr/nanobts_omlattr_test.c
12 files changed, 475 insertions(+), 50 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/78/31878/13
--
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: 13
Gerrit-Owner: arehbein
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: neels
Gerrit-Attention: arehbein
Gerrit-Attention: laforge
Gerrit-Attention: fixeria
Gerrit-Attention: pespin
Gerrit-MessageType: newpatchset
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, fixeria, pespin. neels 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: (2 comments) File src/osmo-bsc/bts_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/06a4a40c_5c500598 PS12, Line 41: _templates > I am wondering why `_templates`? `_def` or `_defaults` maybe? (I think the term "template" is fine and accurate, i assume no vty edits these) File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/f216e673_fd0b3aa1 PS3, Line 2068: > (explanation in comment above https://gerrit.osmocom. […] In general, the tdefs API was made modular with flexible re-use in mind. I havent gone into the smallest details of reading this -- I might find a simplification here or there but in general, I think that this is the way to go: plug tdefs API functions to a custom vty "frontend", tailored to per-BTS timers, which is not provided by the tdefs API in itself. -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Attention: arehbein Gerrit-Attention: fixeria Gerrit-Attention: pespin Gerrit-Comment-Date: Wed, 15 Nov 2023 19:43:05 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein Comment-In-Reply-To: fixeria Comment-In-Reply-To: pespin Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, pespin.
neels 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
(1 comment)
Patchset:
PS12:
Hmm, the task this patch has is actually quite difficult, for these reasons:
- the osmo_tdefs API was implemented with T1234 and X1234 in mind, but not
N1234. Looks like we won't get around adding another namespace for N, in the
libosmocore tdef API itself. Mainly because T3101 and N3101 collide. (If it's
only this one maybe we want an ugly workaround instead? T23101 is N3101?).
Adding the 'N' category is a separate discussion, Vadim and I discussed it a
bit in PM and let's say there's potential for diverse opinions.
- the osmo_tdefs API was implemented with a single global timer config in mind,
not individual per-object timers. The tdefs API is modular enough to be helpful
in implementing this, so no problem here, just many choices to be made.
- confusion: we already have a global timer 'net' / 'T3101'; now we are also
adding a per-BTS T3101 as well as N3101. So per-BTS for GPRS, but only global T
for CS. And these seemingly identical T3101 items do not interact...? ("are you
sure they are separate?" / "did you really use the correct T3101?"...)
I'll not dive into per-line comments now, but I'd like to already say that we
need to still work on this patch (and it's not arehbein's fault)
--
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
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Attention: arehbein
Gerrit-Attention: pespin
Gerrit-Comment-Date: Wed, 15 Nov 2023 19:27:18 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
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 @nhofm...@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) > 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 @nhofm...@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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Attention: neels Gerrit-Attention: arehbein Gerrit-Attention: pespin Gerrit-Comment-Date: Wed, 15 Nov 2023 07:05:20 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: arehbein Comment-In-Reply-To: pespin Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: arehbein, pespin. laforge 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 -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Attention: arehbein Gerrit-Attention: pespin Gerrit-Comment-Date: Tue, 14 Nov 2023 20:41:33 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
Attention is currently required from: 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 12: (2 comments) This change is ready for review. File src/osmo-bsc/bsc_init.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/f585d0ed_a267887c PS3, Line 241: bts->timer_groups[OSMO_BSC_BTS_TDEF_GROUPS_RLC].tdefs = bts->gprs.cell.rlc_cfg.timers; > Hm yeah didn't think of that, I'll remove the timers from the `rlc_cfg` > struct. Done File tests/nanobts_omlattr/nanobts_omlattr_test.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31878/comment/37f64bbc_70226341 PS2, Line 228: 0x02, 0x00, 0x02, 0xa3, 0x00, 0x09, > to me they already are aligned (the timer names and the respective values > below). […] I have aligned all columns vertically -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-Attention: pespin Gerrit-Comment-Date: Sat, 11 Nov 2023 18:13:09 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein Comment-In-Reply-To: pespin Gerrit-MessageType: comment
