Attention is currently required from: daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/37662?usp=email )
Change subject: osmo_bsc_sigtran: Don't try to decode ipaccess_head a second time
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
IIUC this is not really a fix of a bug, but simply a code cleanup/improvement right?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/37662?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: I300bbab3c0215d52bc5f6232ace5c37a6613feb7
Gerrit-Change-Number: 37662
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 30 Jul 2024 08:54:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/37661?usp=email )
Change subject: ipa: Ensure osmo_ipa_segmentation_cb sets msg->l2h to the payload data
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
Sounds like this deserves a patch release?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/37661?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Id91cddf9948ca49abf092850fae3a8f17e445b45
Gerrit-Change-Number: 37661
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 30 Jul 2024 08:52:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
daniel has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/37662?usp=email )
Change subject: osmo_bsc_sigtran: Don't try to decode ipaccess_head a second time
......................................................................
osmo_bsc_sigtran: Don't try to decode ipaccess_head a second time
osmo_io segmentation will already parse the ipaccess_header and set
msg->data to the payload of the control command.
Depends: Id91cddf9948ca49abf092850fae3a8f17e445b45 (libosmo-netif.git)
Related: OS#6422
Change-Id: I300bbab3c0215d52bc5f6232ace5c37a6613feb7
---
M src/osmo-bsc/osmo_bsc_sigtran.c
1 file changed, 16 insertions(+), 12 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/62/37662/1
diff --git a/src/osmo-bsc/osmo_bsc_sigtran.c b/src/osmo-bsc/osmo_bsc_sigtran.c
index 224e2c6..cdc5785 100644
--- a/src/osmo-bsc/osmo_bsc_sigtran.c
+++ b/src/osmo-bsc/osmo_bsc_sigtran.c
@@ -38,6 +38,7 @@
#include <osmocom/bsc/paging.h>
#include <osmocom/bsc/bssmap_reset.h>
#include <osmocom/mgcp_client/mgcp_common.h>
+#include <osmocom/netif/ipa.h>
/* A pointer to a list with all involved MSCs
* (a copy of the pointer location submitted with osmo_bsc_sigtran_init() */
@@ -733,9 +734,6 @@
* libosmo-sigtran doesn't know about, such as piggy-backed CTRL and/or MGCP */
static int asp_rx_unknown(struct osmo_ss7_asp *asp, int ppid_mux, struct msgb *msg)
{
- struct ipaccess_head *iph;
- struct ipaccess_head_ext *iph_ext;
-
if (osmo_ss7_asp_get_proto(asp) != OSMO_SS7_ASP_PROT_IPA) {
msgb_free(msg);
return 0;
@@ -743,15 +741,7 @@
switch (ppid_mux) {
case IPAC_PROTO_OSMO:
- if (msg->len < sizeof(*iph) + sizeof(*iph_ext)) {
- LOGP(DMSC, LOGL_ERROR, "The message is too short.\n");
- msgb_free(msg);
- return -EINVAL;
- }
- iph = (struct ipaccess_head *) msg->data;
- iph_ext = (struct ipaccess_head_ext *) iph->data;
- msg->l2h = iph_ext->data;
- switch (iph_ext->proto) {
+ switch (osmo_ipa_msgb_cb_proto_ext(msg)) {
case IPAC_PROTO_EXT_CTRL:
return bsc_sccplite_rx_ctrl(asp, msg);
case IPAC_PROTO_EXT_MGCP:
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/37662?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: I300bbab3c0215d52bc5f6232ace5c37a6613feb7
Gerrit-Change-Number: 37662
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: newchange
daniel has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-netif/+/37661?usp=email )
Change subject: ipa: Ensure osmo_ipa_segmentation_cb sets msg->l2h to the payload data
......................................................................
ipa: Ensure osmo_ipa_segmentation_cb sets msg->l2h to the payload data
Previously if ih->proto was IPAC_PROTO_OSMO msg->l2h would still point
to the extension header byte which breaks assumptions in e.g. osmo-bsc.
Change-Id: Id91cddf9948ca49abf092850fae3a8f17e445b45
Related: OS#6422
---
M src/ipa.c
1 file changed, 16 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/61/37661/1
diff --git a/src/ipa.c b/src/ipa.c
index 8720427..d43df00 100644
--- a/src/ipa.c
+++ b/src/ipa.c
@@ -375,7 +375,8 @@
#define MSG_CB_IPA_INFO_OFFSET 0
/* Check and remove headers (in case of p == IPAC_PROTO_OSMO, also the IPA extension header).
- * Returns a negative number on error, otherwise the number of octets removed */
+ * Returns a negative number on error, otherwise the number of octets removed.
+ * Both msg->data and msg->l2h point to the user data after the (extended) IPA header if this function is successful. */
static inline int ipa_check_pull_headers(struct msgb *msg)
{
int ret;
@@ -397,6 +398,7 @@
osmo_ipa_msgb_cb_proto_ext(msg) = msg->data[0];
msgb_pull(msg, sizeof(struct ipa_head_ext));
octets_removed += sizeof(struct ipa_head_ext);
+ msg->l2h = msg->data;
return octets_removed;
}
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/37661?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Id91cddf9948ca49abf092850fae3a8f17e445b45
Gerrit-Change-Number: 37661
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: falconia, laforge, osmith, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email )
Change subject: codec: add osmo_hr_sid_classify()
......................................................................
Patch Set 3: Code-Review+1
(2 comments)
File src/codec/hr_sid_class.c:
https://gerrit.osmocom.org/c/libosmocore/+/37558/comment/f2b95218_091039e1
PS2, Line 31: static unsigned ones_in_byte(uint8_t byte)
> The least powerful BTS CPU we need to support is ARM926EJS on sysmoBTS. […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/37558/comment/ba3920ce_de6eb663
PS2, Line 35: for (; byte; byte >>= 1) {
> Thanks for the recommendation - I will implement it in the next revision of this patch.
Done
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5f4eb65379646125b966cf182775b6e9348900bd
Gerrit-Change-Number: 37558
Gerrit-PatchSet: 3
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: osmith <osmith(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 29 Jul 2024 20:58:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: falconia <falcon(a)freecalypso.org>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: falconia, laforge, osmith, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email )
Change subject: codec: add osmo_hr_sid_classify()
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> The Jenkins build failure is cryptic, seems to have something to do with the build CI rather than th […]
@osmith@sysmocom.de looks like a regression of your recent patches.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5f4eb65379646125b966cf182775b6e9348900bd
Gerrit-Change-Number: 37558
Gerrit-PatchSet: 3
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: osmith <osmith(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 29 Jul 2024 20:56:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: falconia <falcon(a)freecalypso.org>
Gerrit-MessageType: comment
Attention is currently required from: fixeria, laforge, osmith, pespin.
falconia has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email )
Change subject: codec: add osmo_hr_sid_classify()
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
The Jenkins build failure is cryptic, seems to have something to do with the build CI rather than the patch under consideration.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5f4eb65379646125b966cf182775b6e9348900bd
Gerrit-Change-Number: 37558
Gerrit-PatchSet: 3
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: osmith <osmith(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 29 Jul 2024 20:27:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: fixeria, laforge, pespin.
Hello Jenkins Builder, laforge,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Code-Review+1 by laforge, Verified+1 by Jenkins Builder
Change subject: codec: add osmo_hr_sid_classify()
......................................................................
codec: add osmo_hr_sid_classify()
In order to support DTX, each frame coming out of the channel decoder
for TCH/FS, TCH/HS and TCH/EFS needs to be classified as valid SID,
invalid SID or non-SID speech as specified in GSM 06.31, 06.41 and
06.81, respectively. However, the case of TCH/HS (GSM 06.41) is
more difficult than FR & EFR: ETSI provided an example implementation
instead of a stipulation, and because they failed to document the BCI
error flag they relied upon, ETSI's reference implementation of HR SID
classification defied understanding for a long time. But now this
mystery has been cracked:
https://osmocom.org/projects/retro-gsm/wiki/HRv1_error_flags
Add a function to libosmocodec that implements the same logic as
ETSI's swSidDetection(), including support for BCI flag. This function
is intended to be used in OsmoBTS (it can post-process the output of
sysmoBTS PHY) when TW-TS-002 output is requested, and it can also be
used in implementations of SDR-based GSM MS when feeding TCH/HS Rx to
a proper speech decoder and Rx DTX handler for HRv1 codec.
Related: OS#6036
Change-Id: I5f4eb65379646125b966cf182775b6e9348900bd
---
M include/osmocom/codec/codec.h
M src/codec/Makefile.am
A src/codec/hr_sid_class.c
3 files changed, 209 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/58/37558/3
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5f4eb65379646125b966cf182775b6e9348900bd
Gerrit-Change-Number: 37558
Gerrit-PatchSet: 3
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: fixeria, pespin.
falconia has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email )
Change subject: codec: add osmo_hr_sid_classify()
......................................................................
Patch Set 2:
(1 comment)
File src/codec/hr_sid_class.c:
https://gerrit.osmocom.org/c/libosmocore/+/37558/comment/3e70872f_cf0295cc
PS2, Line 31: static unsigned ones_in_byte(uint8_t byte)
> Without testing, we don't know if this built-in function is actually faster than the look-up table a […]
The least powerful BTS CPU we need to support is ARM926EJS on sysmoBTS. That CPU does not have any special instructions that can implement `__builtin_popcount()` with native efficiency, hence I prefer the table approach recommended by Vadim - it will probably be more efficient than whatever gcc will generate.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/37558?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5f4eb65379646125b966cf182775b6e9348900bd
Gerrit-Change-Number: 37558
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 29 Jul 2024 19:42:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
falconia has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/37647?usp=email )
Change subject: sysmo: generate empty TCH/H payload on FACCH/H Rx
......................................................................
sysmo: generate empty TCH/H payload on FACCH/H Rx
When sysmoBTS PHY decodes TCH UL Rx as FACCH, it sends only one
PH-DATA.ind for the FACCH, but none for TCH. This case was handled
correctly for TCH/F in 2023-03, but FACCH/H has not been handled
correctly until now.
Change-Id: Id6d966348fb2be084485279e75480f98f46e464b
---
M src/osmo-bts-sysmo/l1_if.c
1 file changed, 35 insertions(+), 16 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
diff --git a/src/osmo-bts-sysmo/l1_if.c b/src/osmo-bts-sysmo/l1_if.c
index 9ca1c32..aa4a739 100644
--- a/src/osmo-bts-sysmo/l1_if.c
+++ b/src/osmo-bts-sysmo/l1_if.c
@@ -987,23 +987,28 @@
return rc;
}
- /* If we got FACCH, the RTP clock needs to account for it,
- * and if we have rtp continuous-streaming enabled,
- * an actual BFI packet will be emitted.
+ /* When TCH UL receiver in sysmoBTS PHY detects FACCH,
+ * it behaves as follows (observed):
*
- * Only the case of TCH/F is currently handled; the task of
- * supporting TCH/H is left as a FIXME for other/later
- * developers. The difficulty with supporting FACCH/H is that
- * only one GsmL1_Sapi_FacchH message will be received,
- * covering 40 ms of time, but two RTP "tick" output calls
- * will need to be made, with appropriately adjusted frame
- * numbers. As a further consideration for rtp continuous-streaming
- * mode, having the two RTP BFI packets sent directly back to back,
- * as opposed to proper 20 ms timing, may be so undesirable
- * that some deployments may rather disable TCH/H (use only TCH/F)
- * than deal with the extra pain, or use TCH/H only on SDR-based
- * BTS with osmo-bts-trx where correct timing is easily achieved. */
- if (data_ind->sapi == GsmL1_Sapi_FacchF)
+ * - In each 20 ms window (both TCH/F and TCH/H), a single
+ * PH-DATA.ind arrives.
+ * - In the case of TCH/F, this PH-DATA.ind carries GsmL1_Sapi_FacchF
+ * instead of GsmL1_Sapi_TchF.
+ * - In the case of TCH/H, the PH-DATA.ind for block 0 carries
+ * GsmL1_Sapi_FacchH instead of GsmL1_Sapi_TchH. However,
+ * in the following 20 ms window (block 1 of FACCH/H)
+ * a PH-DATA.ind with GsmL1_Sapi_TchH arrives as normal.
+ * Typically the latter PHY message carries a BFI (0 length payload),
+ * but nonzero payloads have also been observed under some conditions.
+ * (The latter case remains to be investigated further.)
+ *
+ * In those 20 ms windows where we receive PH-DATA.ind for FACCH
+ * instead of the usual TCH kind, we need to pass an empty payload
+ * to the upper layers so we can produce the correct effect in
+ * the outgoing RTP stream depending on configuration.
+ */
+ if (data_ind->sapi == GsmL1_Sapi_FacchF ||
+ data_ind->sapi == GsmL1_Sapi_FacchH)
l1if_tch_rx_facch(trx, chan_nr, l1p_msg);
/* fill L1SAP header */
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/37647?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Id6d966348fb2be084485279e75480f98f46e464b
Gerrit-Change-Number: 37647
Gerrit-PatchSet: 1
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged