- gerrit-log - lists.osmocom.org

[M] Change in ...osmo-epdg[master]: epdg_gtpc_s2b: add TLV Serving Network
by lynxis lazus 05 Dec '23

05 Dec '23

lynxis lazus has uploaded this change for review. ( https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/35218?usp=email ) Change subject: epdg_gtpc_s2b: add TLV Serving Network ...................................................................... epdg_gtpc_s2b: add TLV Serving Network The open5gs requires Serving Network TLV. Change-Id: I2a9459859fc660e6433cd8178ab9d1f92ae74fc0 --- M src/epdg_gtpc_s2b.erl A src/gtp_utils.erl 2 files changed, 63 insertions(+), 3 deletions(-) git pull ssh://gerrit.osmocom.org:29418/erlang/osmo-epdg refs/changes/18/35218/1 diff --git a/src/epdg_gtpc_s2b.erl b/src/epdg_gtpc_s2b.erl index a5658f1..a71e71d 100644 --- a/src/epdg_gtpc_s2b.erl +++ b/src/epdg_gtpc_s2b.erl @@ -59,6 +59,9 @@ %% TODO: make APN configurable? get it from HSS? -define(APN, <<"internet">>). +-define(MCC, 901). +-define(MNC, 42). +-define(MNC_SIZE, 42). -record(gtp_state, { socket, @@ -330,7 +333,10 @@ #v2_access_point_name{instance = 0, apn = [Apn]}, #v2_selection_mode{mode = 0}, #v2_pdn_address_allocation{type = ipv4, address = <<0,0,0,0>>}, - #v2_bearer_context{group = BearersIE} + #v2_bearer_context{group = BearersIE}, + #v2_serving_network{ + plmn_id = gtp_utils:plmn_to_bin(?MCC, ?MNC, ?MNC_SIZE) + } ], #gtp{version = v2, type = create_session_request, tei = 0, seq_no = SeqNo, ie = IEs}. @@ -346,5 +352,3 @@ tei = RemoteCtlTEI, seq_no = Req#gtp.seq_no, ie = IEs}. - - diff --git a/src/gtp_utils.erl b/src/gtp_utils.erl new file mode 100644 index 0000000..b899530 --- /dev/null +++ b/src/gtp_utils.erl @@ -0,0 +1,45 @@ +% GTP utilities +% +% (C) 2023 by sysmocom - s.f.m.c. GmbH <info(a)sysmocom.de> +% Author: Alexander Couzens <lynxis(a)fe80.eu> +% +% All Rights Reserved +% +% This program is free software; you can redistribute it and/or modify +% it under the terms of the GNU Affero General Public License as +% published by the Free Software Foundation; either version 3 of the +% License, or (at your option) any later version. +% +% This program is distributed in the hope that it will be useful, +% but WITHOUT ANY WARRANTY; without even the implied warranty of +% MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +% GNU General Public License for more details. +% +% You should have received a copy of the GNU Affero General Public License +% along with this program. If not, see <http://www.gnu.org/licenses/>. +% +% Additional Permission under GNU AGPL version 3 section 7: +% +% If you modify this Program, or any covered work, by linking or +% combining it with runtime libraries of Erlang/OTP as released by +% Ericsson on http://www.erlang.org (or a modified version of these +% libraries), containing parts covered by the terms of the Erlang Public +% License (http://www.erlang.org/EPLICENSE), the licensors of this +% Program grant you additional permission to convey the resulting work +% without the need to license the runtime libraries of Erlang/OTP under +% the GNU Affero General Public License. Corresponding Source for a +% non-source form of such a combination shall include the source code +% for the parts of the runtime libraries of Erlang/OTP used as well as +% that of the covered work. + +-module(gtp_utils). +-author('Alexander Couzens <lynxis(a)fe80.eu>'). + +-export([plmn_to_bin/3]). + +% ergw/apps/ergw/test/*.erl +% under GPLv2+ +plmn_to_bin(CC, NC, NCSize) -> + MCC = iolist_to_binary(io_lib:format("~3..0b", [CC])), + MNC = iolist_to_binary(io_lib:format("~*..0b", [NCSize, NC])), + {MCC, MNC}. -- To view, visit https://gerrit.osmocom.org/c/erlang/osmo-epdg/+/35218?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: I2a9459859fc660e6433cd8178ab9d1f92ae74fc0 Gerrit-Change-Number: 35218 Gerrit-PatchSet: 1 Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu> Gerrit-MessageType: newchange

1 0

[S] Change in osmo-mgw[master]: IuUP: Allow Initialization with set rem IP address and unset rem port
by pespin 05 Dec '23

05 Dec '23

Attention is currently required from: neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35176?usp=email ) Change subject: IuUP: Allow Initialization with set rem IP address and unset rem port ...................................................................... Patch Set 4: (4 comments) File src/libosmo-mgcp/mgcp_network.c: https://gerrit.osmocom.org/c/osmo-mgw/+/35176/comment/ada45e97_26192709 PS4, Line 838: osmo_sockaddr_port(&conn->end.addr.u.sa) == 0) { > the most concise restriction would be: […] We need to keep it this way I submitted, at least for address, since the configured address at this point may be wrong (the hnb may actually be using another IP address, what osmo-mgw was submitted was just a guess, and it cannot be considered confirmed until IuUP Initialization happens). So if IP address is set but port is not, we cannot validate IP address of received packet against the configured one, because it is "a guess", we must accept it anyway, at least to carry on with IuUP initialization. https://gerrit.osmocom.org/c/osmo-mgw/+/35176/comment/01d4c9a3_be3f7c7e PS4, Line 841: announce > "not all hNodeB", but some are happily sending RAB Assignment success before IuUP Initialization. […] that's why I didn't write "not all hNodeb", to leave it undetailed. I can add "some", but since we didn't play with many others I preferred leaving it this way. I think even in the diagrams in specs it shows up this way (IuUP Initialization happening before RAB-Ass-Resp). Do you know of any HNB not doing it this way? https://gerrit.osmocom.org/c/osmo-mgw/+/35176/comment/d6994204_0c6123e7 PS4, Line 842: ASs > (RAB Assignment Response at HNBGW) Ack https://gerrit.osmocom.org/c/osmo-mgw/+/35176/comment/238815b4_a883cf2a PS4, Line 843: MDCX > Writing "MDCX" is too specific: it is up to the client to do CRCX with SDP all-in-one or adjust with […] Ack -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35176?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Idd833997abce46886e9664505b2776fa5dadc8db Gerrit-Change-Number: 35176 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Tue, 05 Dec 2023 10:37:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in osmo-mgw[master]: IuUP: allow Initialization from any address if not yet set
by pespin 05 Dec '23

05 Dec '23

Attention is currently required from: laforge, neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35205?usp=email ) Change subject: IuUP: allow Initialization from any address if not yet set ...................................................................... Patch Set 2: (3 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/35205/comment/158c6533_50887307 PS1, Line 28: Decided for now that it's not worth the extra effort to make this more : restrictive > ok, I understand. […] "we do allow any source address to send MGCP to the MGW and actually". This is easily constraint by selecting a proper IP address like a localhost address when configuring osmo-mgw local MGCP address, or binding to an IP address only available on a given interface. The problem with "security" from the RTP ports comes from the fact that the remote IP address is selected by a 3rd entity/node connected to us, and osmo-mgw uses ip probing to find out how to connect. File src/libosmo-mgcp/mgcp_network.c: https://gerrit.osmocom.org/c/osmo-mgw/+/35205/comment/d09ff72c_2e8481ca PS1, Line 838: if (mgcp_conn_rtp_is_iuup(conn) && !conn->iuup.configured) { > (There are two places to indicate codecs, one in the MGCP header which we basically ignore, and the […] With current master (at least from yesterday) yes, you are right. However, as I mentioned I merged a patch in osmo-hnbgw to announce a "hnb IuUP address" to osmo-mgw right from the first RAN-side MGCP CRCX, by using the Iuh remote IP address at the hnbgw, so that osmo-mgw can guess correctly its binding IuUP address in the assumed general case where Iuh IP address = IuUP IP address in HNB. See https://gitea.osmocom.org/cellular-infrastructure/osmo-hnbgw/commit/656d1d2… Besides that, libosmo-mgcp-client needs to be adapted in order to allow submitting an IP address on the wire (CRCX) even if the port is yet not known (port=0). This is what this osmo-mgw.git patch is accomplishing: https://gerrit.osmocom.org/c/osmo-mgw/+/35152 As you see, with this patch the generated CRCX now contains an SDP, and hence it doesn't go through the add_lco() path, but through the add_sdp() (because it's the only way to provide an IP address to osmo-mgw so it does the ip probing). Now that you processed all the above, read again my previous comment to understand the modifications (going back more or less to version 1 of the patch to send the codec in the CRCX even if the port=0). File src/libosmo-mgcp/mgcp_network.c: https://gerrit.osmocom.org/c/osmo-mgw/+/35205/comment/6a20a4bb_df864304 PS2, Line 837: != 0) > (i find this really hard to read. […] It's not a bool, it's a tristate 1, 0, -1. It's a bool + error. So in here we are handling error as "consider it as not set". -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35205?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I6c365559a7bd197349f0ea99f7a13b56a4bb580b Gerrit-Change-Number: 35205 Gerrit-PatchSet: 2 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Tue, 05 Dec 2023 10:30:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in osmocom-bb[master]: Correctly assemble measurement result into MEASUREMENT REPORT
by fixeria 05 Dec '23

05 Dec '23

Attention is currently required from: jolly. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/35214?usp=email ) Change subject: Correctly assemble measurement result into MEASUREMENT REPORT ...................................................................... Patch Set 1: Code-Review+1 (1 comment) File src/host/layer23/src/mobile/gsm48_rr.c: https://gerrit.osmocom.org/c/osmocom-bb/+/35214/comment/3d16b8df_1307504a PS1, Line 3710: strongest_i > This is not required. […] Ok then. I was concerned about potentially uninitialized memory access. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/35214?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Iaeeaf978da31611c47a20af41790bfa6640dcffd Gerrit-Change-Number: 35214 Gerrit-PatchSet: 1 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Comment-Date: Tue, 05 Dec 2023 09:41:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: jolly <andreas(a)eversberg.eu> Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in osmocom-bb[master]: Correctly assemble measurement result into MEASUREMENT REPORT
by jolly 05 Dec '23

05 Dec '23

Attention is currently required from: fixeria. jolly has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/35214?usp=email ) Change subject: Correctly assemble measurement result into MEASUREMENT REPORT ...................................................................... Patch Set 1: (1 comment) File src/host/layer23/src/mobile/gsm48_rr.c: https://gerrit.osmocom.org/c/osmocom-bb/+/35214/comment/5977cae4_4b039f5b PS1, Line 3710: strongest_i > Initialize to 0? This is not required. The initial "strongest" value is always above any measurement value, so "strongest_i" does not matter. After getting the cell from measurement, "strongest_i" is set and used only when there is another cell with the same level. "strongest_i" is used to prevent reporting a measurement multiple times. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/35214?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Iaeeaf978da31611c47a20af41790bfa6640dcffd Gerrit-Change-Number: 35214 Gerrit-PatchSet: 1 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Tue, 05 Dec 2023 09:14:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: Make BSSGP timing data configurable
by arehbein 05 Dec '23

05 Dec '23

Attention is currently required from: arehbein, pespin. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bsc/+/31882?usp=email to look at the new patch set (#8). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: Make BSSGP timing data configurable ...................................................................... Make BSSGP timing data configurable Also: Deprecate/hide old respective VTY command, while preserving backwards compatibility for BSSGP timing data configuration. Related: OS#5335 Change-Id: Id4779f033b5eb1742462d4efc28a0398645acfe6 --- M include/osmocom/bsc/bts.h M include/osmocom/bsc/vty.h M src/osmo-bsc/bsc_vty.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 8 files changed, 174 insertions(+), 25 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/82/31882/8 -- 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-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-bsc[master]: Make BSSGP timing data configurable
by arehbein 05 Dec '23

05 Dec '23

Attention is currently required from: pespin. arehbein 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 7: (1 comment) Patchset: PS2: > Again, the timers should be per-BTS configurable and probably on a separate tdef group of it own. Done -- 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: 7 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 05 Dec 2023 08:57:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: Make BSSGP timing data configurable
by Jenkins Builder 05 Dec '23

05 Dec '23

Attention is currently required from: arehbein. Jenkins Builder 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 7: (1 comment) File src/osmo-bsc/bts_vty.c: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-12814): https://gerrit.osmocom.org/c/osmo-bsc/+/31882/comment/8ee42d42_0fc43b4c PS7, Line 1754: if (osmo_tdef_set(bts_bssgp->tdefs, tdef_params->tdef_id, (val * tdef_params->factor)? : 1, tdef_params->unit) < 0) spaces required around that '?' (ctx:VxW) -- 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: 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 08:57:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: Make NSE timing data configurable
by arehbein 05 Dec '23

05 Dec '23

Attention is currently required from: pespin. arehbein 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: (4 comments) File include/osmocom/bsc/bts.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/79739441_23ab5969 PS2, Line 326: #define GSM_BTS_TDEF_ID_TNS_BLOCK_NEG29 (-29) > Better leave some space in the number series so that we can later on add similar timers next to the […] Done File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/39ebfef1_022fa19a PS2, Line 187: struct osmo_tdef *tdefs = bts->network->T_defs; > Again, this needs to be per BTS. Done File src/osmo-bsc/bts_vty.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/2a683e66_6e7a8c89 PS2, Line 76: static const int gprs_ns_timer_tdef_data[][2] = { > AFAICT this is only used in cfg_bts_gprs_ns_timer_cmd, so it can be moved there. I originally put it below `gprs_ns_timer_strs` so that the index correspondence can be checked easily. I've moved it now. https://gerrit.osmocom.org/c/osmo-bsc/+/31880/comment/5f9337fe_17162c2b PS2, Line 1627: int idx = get_string_value(gprs_ns_timer_strs, argv[0]); > so you are converting to an index here once, and in line 1637 again. This looks ptoentially wrong. […] Not sure what you mean to say. I'm only converting a string to an index once. That same index is then used to get the corresponding data for setting the tdef for the BTS. -- 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: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 05 Dec 2023 08:57:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[L] Change in osmo-bsc[master]: Introduce per-BTS timers, RLC timer commands
by arehbein 05 Dec '23

05 Dec '23

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) @nhofmeyr@sysmocom.de yes, the vty doesn't edit any of it. @vyanitskiy@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...) @vyanitskiy@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 <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: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)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 <nhofmeyr(a)sysmocom.de> 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

1 0

2026

2025

2024

2023

2022

gerrit-log