Attention is currently required from: pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33224 )
Change subject: stream: Drop dot at the end of log line
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Patchset:
PS2:
thanks. I always found those quite useless ;)
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33224
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I59b0e56088c60e3de5f3a2f38f158e16074a65c9
Gerrit-Change-Number: 33224
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 10:45:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33222 )
Change subject: l1ctl_proto: add 'start_fn' field to UL/DL TBF CFG.req messages
......................................................................
Patch Set 1:
(1 comment)
File src/host/layer23/src/modem/rlcmac.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/33222/comment/c7fec2be_0803ebb8
PS1, Line 166: 0xffffffff /* TODO: start Fn */);
iiuc this 0xffffff is a specific meaning which means "use next available FN"? Can we add a define for it?
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33222
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Ibb6a05165fe1c81268fb0e3674adae4065e78171
Gerrit-Change-Number: 33222
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 09:28:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32734 )
Change subject: paging: do not confirm PAGING COMMAND messages
......................................................................
Patch Set 14:
(1 comment)
Patchset:
PS14:
This patch caused a regression:
https://jenkins.osmocom.org/jenkins/view/TTCN3/job/ttcn3-bts-test/2050/
The following two testcases started to fail:
* BTS_Tests.TC_pcu_data_req_imm_ass_pch
* BTS_Tests:hopping.TC_pcu_data_req_imm_ass_pch
```
Timeout waiting for PCU DATA.cnf
BTS_Tests.ttcn:9033 BTS_Tests control part
BTS_Tests.ttcn:5838 TC_pcu_data_req_imm_ass_pch testcase
```
Please modify `TC_pcu_data_req_imm_ass_pch` to not expect the confirmation, function `f_PCUIF_tx_imm_ass_pch` has an optional parameter `wait_for_cnf` for that.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32734
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I8b8264d28b1b1deb08774cdba58dd4c6dafe115d
Gerrit-Change-Number: 32734
Gerrit-PatchSet: 14
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 09:26:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33178 )
Change subject: cfg: add 'hnbgw' / 'plmn MCC MNC'
......................................................................
Patch Set 5:
(1 comment)
File src/osmo-hnbgw/hnbgw_l3.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33178/comment/d3fe4054_a35f4456
PS3, Line 281: osmo_plmn_from_bcd(ies->lai.pLMNidentity.buf, &local_plmn);
> a log line here would say something like […]
I didn't write the details because I expected you to know them better 😄
So, IIUC the idea here is that we prefer from now on that the user configures the PLMN in the config file, and taking the PLMN on-the-fly from sent/received messages is more like a hack for backward compatibility (may I have understood it wrong?).
So, the point here is to notify through logging to the user that we are setting the PLMN here dynamically, but that the preferred way would be for them to configure it in the cfg file. This also allows the user to see if what we set dynamically matches their expectations.
Regarding your examples, I think only the first one is useful to log for the reasons mentioned by me above. Maybe add text to hint the user to update the config file.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33178
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: If404c3859acdfba274c719c7cb7d16f97d831a2a
Gerrit-Change-Number: 33178
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 09:24:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33178 )
Change subject: cfg: add 'hnbgw' / 'plmn MCC MNC'
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS4:
> (btw, in case it is interesting to know why this occurs / to explain that this is not meant to ignor […]
No worries, I was not thinking you were explicitly ignoring the comments.
I write "not yet addressed" because it has several benefits:
- Remark it in case you didn't see it / forgot about it
- Remind me of the last review state after I re-review it, and for others to see that there's still stuff not done.
In any case, I'd welcome if you could as much as possible fix all the comments before pushing a new version, since that means I don't need to do several rounds of review.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33178
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: If404c3859acdfba274c719c7cb7d16f97d831a2a
Gerrit-Change-Number: 33178
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 09:19:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33173 )
Change subject: cnpool: add context_map_cnlink_lost() handling
......................................................................
Patch Set 5:
(1 comment)
File include/osmocom/hnbgw/context_map.h:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33173/comment/1aa5ef49_b58d4380
PS3, Line 70: MAP_SCCP_EV_CN_LINK_LOST,
> Scenario: […]
IMHO LINK_LOST seems to indicate that lower layer connection (eg. SCTP) or even lower layer link (eg. ethernet) was lost.
If you want to have a single event for all those, which goes from SCTP lost to RANAP RESET because they are to be acted the same way, then fine, but don't only mention "The CN link has a RESET" case.
Otherwise, if you plan to have those as separate event, I'd rather call this EV_RX_RANAP_RESET or alike.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33173
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Ic0a6fcfb747dc093528ca2bd12a269ad390d465c
Gerrit-Change-Number: 33173
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
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: Thu, 08 Jun 2023 09:16:54 +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
Attention is currently required from: neels, pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169 )
Change subject: fix umts_cell_id_name(): show CID
......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169/comment/3b94c0a4_2fa9aa83
PS3, Line 12: Use OTC_SELECT
> I personally suggest using stack-allocated buffers for those cases where we don't have/want static buffers.
Agreed.
> But I'm really surprised to hear any praise for static buffers at all.
I thought that the consensus and aim is to get away from static buffers
I'm not aware of such a notion/consensus. The heap allocated variants to me only make sense in cases where they don't work for some specific reason.
Also, it is a big difference whether some function is called once (like to generate a fsm name, or some other name of an object that has a certain lifetime) in a while, or a function is called potentially thousands of times per second.
> IIRC there is a log line that prints two distinct cell IDs
If you know that, then I would suggest to special-case that one statement.
> If you mean to imply that this is inefficient and expensive related to the rest of osmo-hnbgw
I don't want to make such decisions specific to each and every program. The fact that there may be some other horribly inefficient parts in osmo-hnbgw shouldn't be seen as an excuse to introduce more *unless* there's a clear advantage.
Also, given that the user triggering this development is very keen on efficiency, and we are currently spending an awful lot of time on osmo-bsc optimizations. Sooner or later I expect the same will happen with osmo-hnbgw, as it gets loaded.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Id903b8579ded8b908e644808e440d373bcca8da4
Gerrit-Change-Number: 33169
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 08:36:41 +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