Attention is currently required from: lynxis lazus.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/35216?usp=email )
The change is no longer submittable: Code-Review is unsatisfied now.
Change subject: dia: 3gpp_ts29_273_s6b: add missing *[ AVP ] to AAR
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Patchset:
PS1:
This is wrong. Youy are mentioning AAR in commit description, but modifying AAA.
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/35216?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: erlang/osmo-epdg
Gerrit-Branch: master
Gerrit-Change-Id: I934c87e912882ddef1cbac1466dec66c72b9c77c
Gerrit-Change-Number: 35216
Gerrit-PatchSet: 1
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Tue, 05 Dec 2023 12:11:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31882?usp=email )
Change subject: Make BSSGP timing data configurable
......................................................................
Patch Set 8:
(5 comments)
File include/osmocom/bsc/vty.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/31882/comment/c7ac3904_99c10ccd
PS8, Line 106: /* Add additional group strings for vty command generation here */
IMHOyou should in general avoid filling your commits in a patchset with this type of comments which only make them more complex to review. This kind of stuff is for you keep private during dev imho.
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31882/comment/59055467_74eb3aaf
PS8, Line 323: /* TODO: Add other options for group argument bssgp '[(rlc|bssgp|ns)]' when adding additional timer groups.
Same with this one.
File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31882/comment/6814fd72_fbcff715
PS8, Line 212: .t1_s = osmo_tdef_get(g->tdefs, 1, OSMO_TDEF_S, -1),
so you added defines for timers 1..4 but you are not using them?
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31882/comment/00deb420_fbbce117
PS8, Line 1704: struct tdef_data gprs_bssgp_timer_tdef_data[11] = {
I still find this is most probably not needed.
You can probably change the indices in gprs_bssgp_cfg_strs to directly be the GSM_BTS_TDEF_ID_BSSGP_* values?
https://gerrit.osmocom.org/c/osmo-bsc/+/31882/comment/6f2ae417_4db4b14d
PS8, Line 1741: int idx = get_string_value(gprs_bssgp_cfg_strs, argv[0]), val = atoi(argv[1]);
definetly not in a single line.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31882?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: Id4779f033b5eb1742462d4efc28a0398645acfe6
Gerrit-Change-Number: 31882
Gerrit-PatchSet: 8
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 05 Dec 2023 12:10:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31880?usp=email )
Change subject: Make NSE timing data configurable
......................................................................
Patch Set 7:
(5 comments)
File include/osmocom/bsc/bts.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/e1361416_f0442e57
PS7, Line 347: extern const size_t bts_gprs_rlc_timer_templates_bytes;
why is this being removed here? it probably should be removed in the previous commit?
File src/osmo-bsc/bts_init.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/73d560f5_01f82f49
PS7, Line 111: /* Init per-BTS NS timers */
it will probably help adding an extra blank line before this one.
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/49d596a6_1cb99518
PS7, Line 1662: static const struct tdef_data gprs_ns_timer_tdef_data[7] = {
I'm still totally not understanding why do you need this index lookup here. If you want to keep the older indices to keep backward compatibilities, why not using those same timer names in the tdef structure?
File src/osmo-bsc/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/3ada7b05_d99d0ee3
PS7, Line 225: g = &bts->timer_groups[OSMO_BSC_BTS_TDEF_GROUPS_RLC];
you could fix your proper commit and already use this name so you don't need to change it in the followup commit (and save my time reviewing it).
File tests/gprs_params.vty:
https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/f5b24ba8_fc25acc3
PS7, Line 36: gprs nsei 0
why did the timers dissappear here? because they are the default values? then ACK.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31880?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: Ie46ec5cb7095bc1dfe3effd0e76d6ccfd6bd2f3f
Gerrit-Change-Number: 31880
Gerrit-PatchSet: 7
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 05 Dec 2023 12:03:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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 <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 05 Dec 2023 11:48:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
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 <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 05 Dec 2023 11:45:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment