Attention is currently required from: fixeria.
Hello Jenkins Builder, laforge,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bts/+/32240
to look at the new patch set (#2).
Change subject: Move GPRS NSE under BTS SiteMgr
......................................................................
Move GPRS NSE under BTS SiteMgr
As per ipaccess expectancies and following TS 12.21.
Change-Id: If44d8f256cab7b2660900cedfb0ed9fe67eb3420
---
M include/osmo-bts/bts.h
M include/osmo-bts/bts_sm.h
M src/common/bts.c
M src/common/bts_sm.c
M src/common/nm_bts_sm_fsm.c
M src/common/nm_gprs_cell_fsm.c
M src/common/nm_gprs_nse_fsm.c
M src/common/nm_gprs_nsvc_fsm.c
M src/common/oml.c
M src/common/pcu_sock.c
M src/common/vty.c
11 files changed, 88 insertions(+), 65 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/40/32240/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32240
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: If44d8f256cab7b2660900cedfb0ed9fe67eb3420
Gerrit-Change-Number: 32240
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: fixeria.
Hello Jenkins Builder, laforge,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bts/+/32236
to look at the new patch set (#5).
Change subject: Merge gsm_network into gsm_bts_sm and place gsm_bts under it
......................................................................
Merge gsm_network into gsm_bts_sm and place gsm_bts under it
This way the data model in TS 12.21 (Figure 1) is followed, where
there's a BTS Site Manager containing one or more BTS. In our case we
only support 1 BTS (cell) so far.
Change-Id: Ideb0d458ec631008223f861cf8b46d09524a1a21
Related: OS#5994
---
M include/osmo-bts/Makefile.am
M include/osmo-bts/bts.h
A include/osmo-bts/bts_sm.h
M include/osmo-bts/gsm_data.h
M include/osmo-bts/pcu_if.h
M include/osmo-bts/vty.h
M src/common/Makefile.am
M src/common/abis_osmo.c
M src/common/bts.c
M src/common/bts_shutdown_fsm.c
A src/common/bts_sm.c
M src/common/main.c
M src/common/nm_bts_sm_fsm.c
M src/common/oml.c
M src/common/pcu_sock.c
M src/common/vty.c
M src/osmo-bts-oc2g/oc2gbts_vty.c
M src/osmo-bts-omldummy/main.c
M tests/agch/agch_test.c
M tests/cipher/cipher_test.c
M tests/handover/handover_test.c
M tests/meas/meas_test.c
M tests/misc/misc_test.c
M tests/paging/paging_test.c
M tests/tx_power/tx_power_test.c
25 files changed, 272 insertions(+), 161 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/36/32236/5
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32236
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ideb0d458ec631008223f861cf8b46d09524a1a21
Gerrit-Change-Number: 32236
Gerrit-PatchSet: 5
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32236 )
Change subject: Merge gsm_network into gsm_bts_sm and place gsm_bts under it
......................................................................
Patch Set 4:
(1 comment)
File src/common/bts.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32236/comment/cde1c099_71285387
PS4, Line 406: g_bts_sm->num_bts++;
> Ack
This change would not be part of this patch however, I'll fix it in a followup patch.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32236
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ideb0d458ec631008223f861cf8b46d09524a1a21
Gerrit-Change-Number: 32236
Gerrit-PatchSet: 4
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 11 Apr 2023 09:24:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32240 )
Change subject: Move GPRS NSE under BTS SiteMgr
......................................................................
Patch Set 1:
(2 comments)
File include/osmo-bts/bts_sm.h:
https://gerrit.osmocom.org/c/osmo-bts/+/32240/comment/7d832391_069306d3
PS1, Line 43: gprs
> `pcu_state` is also related to GPRS, but not part of this struct. […]
I liked having the .gprs suffix in the previous place, so I'm keeping here. We can move pcu_state to .gprs later on in another commit.
File src/common/bts_sm.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32240/comment/bbd58577_7b9eb955
PS1, Line 77: %d", 0
> just have `0` in the format string and use `osmo_fsm_inst_update_id()`?
Ack
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32240
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: If44d8f256cab7b2660900cedfb0ed9fe67eb3420
Gerrit-Change-Number: 32240
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 11 Apr 2023 09:01:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32236 )
Change subject: Merge gsm_network into gsm_bts_sm and place gsm_bts under it
......................................................................
Patch Set 4:
(5 comments)
File src/common/bts.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32236/comment/fee8f64d_5be68596
PS4, Line 406: g_bts_sm->num_bts++;
> I find this weird that a BTS is added to `&g_bts_sm->bts_list` in `gsm_bts_alloc()`, but `g_bts_sm-> […]
Ack
File src/common/bts_sm.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32236/comment/4b300e48_d802b7d0
PS4, Line 37: talloc_free
> talloc does free() all its children anyway, do we really need to do this manually?
I prefer doing it this way in order to control the order of stuff being freed.
File src/common/oml.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32236/comment/914ddc11_1d905977
PS4, Line 154: if (!bts) {
> `OSMO_UNLIKELY`?
Ack
File src/common/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32236/comment/ab0b3e3a_a507e439
PS4, Line 897: struct gsm_bts_sm *bts_sm
> Can we remove this argument since `g_bts_sm` is available globally?
Yes we could, I'm not sure if it makes sense doing it in this commit or not, I simply changed the minimum required stuff in this file.
https://gerrit.osmocom.org/c/osmo-bts/+/32236/comment/0cddd3b8_ec40428b
PS4, Line 957: struct gsm_bts_sm *bts_sm
> Can we remove this argument since `g_bts_sm` is available globally?
Yes we could, I'm not sure if it makes sense doing it in this commit or not, I simply changed the minimum required stuff in this file.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32236
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ideb0d458ec631008223f861cf8b46d09524a1a21
Gerrit-Change-Number: 32236
Gerrit-PatchSet: 4
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 11 Apr 2023 09:00:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32251 )
Change subject: si2quater: check return value of osmo_earfcn_del()
......................................................................
Patch Set 2:
(1 comment)
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/32251/comment/bd34aa3c_8768fbbe
PS2, Line 2082: if (osmo_earfcn_del(e, arfcn) != 0)
> IIUC if you arrived here is because you failed adding the earfcn to the list, so why would you want […]
Not really. We end up here if generating si2quater fails after adding an EARFCN, so we need to remove it. See `si2q_num()` in `src/osmo-bsc/system_information.c`.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32251
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I227dad57737721c40a508f67616d9f5003bb1a3e
Gerrit-Change-Number: 32251
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 11 Apr 2023 08:59:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32251 )
Change subject: si2quater: check return value of osmo_earfcn_del()
......................................................................
Patch Set 2:
(1 comment)
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/32251/comment/9f57dc27_6c61eb5a
PS2, Line 2082: if (osmo_earfcn_del(e, arfcn) != 0)
IIUC if you arrived here is because you failed adding the earfcn to the list, so why would you want to remove it?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32251
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I227dad57737721c40a508f67616d9f5003bb1a3e
Gerrit-Change-Number: 32251
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 11 Apr 2023 08:56:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: neels, laforge, fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32043 )
Change subject: logging: add 'logging timezone (localtime|utc)'
......................................................................
Patch Set 2:
(1 comment)
File src/core/logging.c:
https://gerrit.osmocom.org/c/libosmocore/+/32043/comment/d9ff978e_25f73f0f
PS1, Line 874: target->timezone = timezone;
> But we do not *need* to care whether a given timezone function is available. If the system cannot generate a timestamp the way it is configured, then we just don't print one in the logs.
I'm sorry but this behavior makes no sense to me. Why they hell if I set a specific tiemstamp format I end up with having no timestamp at all? It really makes no sense.
If I try to set a timestamp and the system/app is not supporting it, it should just tell me when I try to configure it that way.
The only reason you argue for not doing that you are presenting are:
- because it makes code more complex: A few more lines of code checking errors?
- because it adds non-conforming API: aka having a function return a return code while others do. I really see no problem here. Specially in a lib public API, it makes totally sense to have it return an error code.
- make it harder to launch an osmo program...: Not really. If you attempt to configure it in a way the system doesn't support it then it fails with the related error message, then you remove/change the config option which makes no sense in that system and then get an expected behavior.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32043
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I7f868b47bf8f8dfcf85e735f490ae69b18111af4
Gerrit-Change-Number: 32043
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(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, 11 Apr 2023 08:50:33 +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>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment