Attention is currently required from: fixeria, laforge.
dexter has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/pysim/+/39863?usp=email )
The change is no longer submittable: Code-Review and Verified are unsatisfied now.
Change subject: saip-tool: add commandline option to edit mandatory services list
......................................................................
Patch Set 2:
(1 comment)
File pySim/esim/saip/__init__.py:
https://gerrit.osmocom.org/c/pysim/+/39863/comment/4001c70f_734520cc?usp=em… :
PS1, Line 1356: if service_name in self.decoded['eUICC-Mandatory-services'].keys():
> Maybe it's worth letting the user know that they're trying to remove something that is not present i […]
Done
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/39863?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I120b98d4b0942c26674bc1365c5711101ec95235
Gerrit-Change-Number: 39863
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 31 Mar 2025 15:00:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Attention is currently required from: fixeria.
laforge has posted comments on this change by fixeria. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/39871?usp=email )
Change subject: msc: use f_expect_paging() in SS/USSD TCs
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/39871?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: Ia75420c738ef412af6ae602566fab7c997b48335
Gerrit-Change-Number: 39871
Gerrit-PatchSet: 1
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: Mon, 31 Mar 2025 14:45:18 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: falconia, neels, pespin.
laforge has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-mgw/+/39869?usp=email )
Change subject: mgw: rtp-patch rfc5993hr: convert to each end's respective format
......................................................................
Patch Set 2:
(3 comments)
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/5bf24b1f_1f1c6782?usp… :
PS2, Line 646: exist 3 different RTP payload formats
> There exist maybe lots of formats for HR, but there exist TWO widely deployed formats, plus we have […]
I don't really see why we would need to declare which of those formats are more widely deployed than others.
This comment is not an API doc but just a comment. It may be long but I'd rather have it than not have it.
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/6c81cf96_e0bbe4c9?usp… :
PS2, Line 691: * consider the full RTP chain of a single call:
> (my idea so far, can be in a future patch, is to not send any HR payload to a peer until we have received its first packet;
And what if both sides of the connection follow that policy? It will fail...
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/dd028f70_de311297?usp… :
PS2, Line 698: will always be RFC 5993
> Ideally, according to 3GPP yes, but in practice, do we always convert to RFC5993? […]
On AoIP, there is no discussion. It is always RFC5993 as that is what (likely, quoting from memory) 3GPP TS 48.103 mandates. On other interfaces it's a different story.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/39869?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6b446ad83c540fb8b7e1aae24b78c27010212d64
Gerrit-Change-Number: 39869
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 31 Mar 2025 14:43:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Attention is currently required from: falconia, pespin.
neels has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-mgw/+/39869?usp=email )
Change subject: mgw: rtp-patch rfc5993hr: convert to each end's respective format
......................................................................
Patch Set 2:
(10 comments)
Patchset:
PS2:
> Idea: It may make sense at some point to get rid of such VTY comamnd and somehow be able to specify […]
yes, there has been discussion.
The autodetection came up because there is no widely accepted way to indicate varying HR formats in SDP.
The proper way would be to indicate the HR format in fmtp from each MGCP client, but for that we have to invent a new fmtp, and also the BSC has to then be aware of the HR formats in use. That would actually be correct, too. That is also how dexter started out until i started interfering, see https://gerrit.osmocom.org/c/osmo-mgw/+/31214
So this here might all be one huge show of extra effort introduced by me for dubious reasons.
But because HR formats are so trivially detectable, I thought why not keep that out of our 3GPP components and just convert it implicitly in the MGW. It would save us from many changes and added complexity in our MGCP clients, always having to indicate HR formats in fmtp as well...
I do think this point is still valid, I am not 100% sure whether my idea of autodetection is in the end going to bite us for some reason... Maybe it would be better to invent a fmtp instead.
So far, I still would like to try the autodetection, because it seems the far simpler way, avoiding TS101318 from cascading into our 3GPP components.
File include/osmocom/mgcp/mgcp_rtp_end.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/54a61fd4_fe289753?usp… :
PS2, Line 40: bool hr_ts101318_detected;
I would prefer better layer separation for these:
- Here should be an indicator whether and to which fmt to convert, i see an enum here. Who or what detects this or even if any detection happened at all should not be relevant here.
- the new field should reflect the target format, so we do not need to go back to SDP details.
My humble suggestion, replace both the current rfc5993_convert flag and the hr_tsnnnn_detected flag with sth like
~~~
enum hr_format {
HR_FMT_UNSET,
HR_FMT_RFC5993,
HR_FMT_TS101318,
HR_FMT_TWTS002,
};
struct mgcp_end {
...
enum hr_format convert_hr_to;
...
};
...
static int rfc5993_hr_convert(struct msgb *msg, struct mgcp_conn_rtp *dst)
{
switch (hr_conversion) {
case HR_FMT_UNSET:
/* never convert */
break;
case HR_FMT_RFC5993:
/* always emit RFC5993 */
if (rtp_plen == GSM_HR_BYTES_RTP_TS101318) {
/* TS 101318 encoding => RFC 5993 encoding */
...
}
break;
case HR_FMT_TS101318:
/* always emit TS 101.318 */
if (rtp_plen != GSM_HR_BYTES_RTP_TS101318) {
/* RFC5993 => TS 101318 encoding */
...
}
break;
...
~~~
(while at it I would also put the actual conversion in separate functions, one per switch case.)
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/4669fe82_c24682cc?usp… :
PS2, Line 646: exist 3 different RTP payload formats
There exist maybe lots of formats for HR, but there exist TWO widely deployed formats, plus we have another Osmocom-specific extension, right? TWTS is taking too much mindshare here.
There should be an easy way to TL;DR for a busy hacker:
This API doc should efficiently and shortly describe what the function does. When that is clear and clean, there can be another *separate* paragraph about the larger context, and often that larger context should rather be described in the manual instead, so users can find out without having to read the src.
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/89a1ec56_e3e1e759?usp… :
PS2, Line 691: * consider the full RTP chain of a single call:
(my idea so far, can be in a future patch, is to not send any HR payload to a peer until we have received its first packet; this should be configurable via maybe two new vty options sort of like "[no] hr-auto-detect" to switch auto-detection, and another one maybe "[no] hr-drop-undetected" for dropping HR packets until we have detected something...)
(As in my above comment: this function here should only and simply convert according to that enum value, and let's describe detection complexity and decision quirks elsewhere.)
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/e6e89f34_a49df8a2?usp… :
PS2, Line 698: will always be RFC 5993
Ideally, according to 3GPP yes, but in practice, do we always convert to RFC5993?
But again i have the problem, the feature *is* almost a layer violation: we are in the MGW -- ideally, the BSC,MSC,etc should make these decisions, and the MGW should just adhere to instructions given. So this autodetection of HR formats should be as flat and simple as possible, and this doc should try to avoid tying too much into AoIP specs, really.
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/d4ffc6d6_6725be1b?usp… :
PS2, Line 715: static int rfc5993_hr_convert(struct msgb *msg, struct mgcp_conn_rtp *dst)
(kind of weird to swap the msgb argument position for no reason)
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/06d1d3bf_051fdcbe?usp… :
PS2, Line 1577: Th
s/This internal function examines/Examine/
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/6386809e_87758d28?usp… :
PS2, Line 1658: Autodetect
s/between // ?
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/f43d96a8_66d08c4a?usp… :
PS2, Line 1661: int rc = examine_rtp_rx_gsmhr(msg, conn_src);
Is this called always on GSM-HR-08 or only once, until we successfully detected the HR format? ... It is a minor tweak but it would be needed only once at the start of the HR stream.
https://gerrit.osmocom.org/c/osmo-mgw/+/39869/comment/3c2e3eb1_6b844c34?usp… :
PS2, Line 1666: mgcp_conn_watchdog_kick(conn_src->conn);
> It probably makes sense to call this even if output is wrong, but not a hard opinion.
(i'm not sure, but if yes then this kick should go below "out_free:" ? -- i see this change in a separate patch anyway, so let's resolve the comment for the sake of *this* patch)
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/39869?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6b446ad83c540fb8b7e1aae24b78c27010212d64
Gerrit-Change-Number: 39869
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 31 Mar 2025 13:32:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/39877?usp=email )
Change subject: bts: simplify f_tc_rsl_ms_pwr_ctrl()
......................................................................
bts: simplify f_tc_rsl_ms_pwr_ctrl()
Change-Id: I67968a0398f9957dc4cc9c7fdffaf2b0ac40b88a
---
M bts/BTS_Tests.ttcn
1 file changed, 2 insertions(+), 11 deletions(-)
Approvals:
pespin: Looks good to me, approved
Jenkins Builder: Verified
laforge: Looks good to me, but someone else must approve
diff --git a/bts/BTS_Tests.ttcn b/bts/BTS_Tests.ttcn
index 8c05bb3..b49cb28 100644
--- a/bts/BTS_Tests.ttcn
+++ b/bts/BTS_Tests.ttcn
@@ -3831,8 +3831,6 @@
* the BTS is forwarding those values to the MS via the SACCH L1 header. */
private function f_tc_rsl_ms_pwr_ctrl(charstring id) runs on ConnHdlr {
var SacchL1Header l1h;
- var RSL_IE_MS_Power ms_power;
- var RSL_Message rsl;
var uint5_t power_level := 0;
f_l1_tune(L1CTL);
@@ -3840,16 +3838,11 @@
f_est_dchan();
- ms_power.reserved := 0;
- ms_power.fpc_epc := false;
-
/* Send the first power control command. This will disable any BTS/TRX
* internal power control and switch the MS (which is not in scope of
* this test) to a constant power level. We start with a power level
* of 0 */
- ms_power.power_level := power_level;
- rsl := valueof(ts_RSL_MS_PWR_CTRL(g_chan_nr, ms_power));
- RSL.send(rsl);
+ RSL.send(ts_RSL_MS_PWR_CTRL(g_chan_nr, ts_RSL_IE_MS_Power(power_level)));
alt {
[] as_l1_sacch_l1h(l1h, do_apply := false) {
@@ -3862,9 +3855,7 @@
/* Signal a new power level via RSL for the next turn. */
if (power_level < 31) {
power_level := power_level + 1;
- ms_power.power_level := power_level;
- rsl := valueof(ts_RSL_MS_PWR_CTRL(g_chan_nr, ms_power));
- RSL.send(rsl);
+ RSL.send(ts_RSL_MS_PWR_CTRL(g_chan_nr, ts_RSL_IE_MS_Power(power_level)));
repeat;
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/39877?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I67968a0398f9957dc4cc9c7fdffaf2b0ac40b88a
Gerrit-Change-Number: 39877
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>