falconia submitted this change.

View Change


Approvals: laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, but someone else must approve falconia: Looks good to me, approved Jenkins Builder: Verified
ECU in UL path: make it optional per vty config

Current osmo-bts-trx includes a provision for invoking ECUs from
libosmocodec in the UL path from the channel decoder to the RTP
output. This pre-existing implementation is counter to the spirit
of 3GPP specs (a BTS should merely mark BFI conditions in its UL
output, as opposed to actively modifying the frame stream with an ECU),
inconsistent between different osmo-bts models (only in -trx and no
others), and inconsistent even within osmo-bts-trx itself, where
the link quality check in l1sap will sometimes suppress the output
of the ECU - a quirk which the designers of the current mechanism
most certainly did not intend.

The solution decided upon in OsmoDevCall on 2023-06-21 is to make
this ECU optional per vty config, and move it from the trx model
to the common layer to resolve the inconsistencies. Implement the
first part: make the ECU application optional per vty config.

For backward compatibility with existing deployments, the new
"rtp internal-uplink-ecu" setting is enabled by default on osmo-bts-trx
but not on any other models.

Related: OS#6040
Change-Id: I0acca9c6d7da966a623287563e0789db9e0fae8e
---
M include/osmo-bts/bts.h
M src/common/vty.c
M src/osmo-bts-trx/l1_if.c
M src/osmo-bts-trx/main.c
M tests/osmo-bts.vty
5 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/include/osmo-bts/bts.h b/include/osmo-bts/bts.h
index 7ca3224..62017d7 100644
--- a/include/osmo-bts/bts.h
+++ b/include/osmo-bts/bts.h
@@ -300,6 +300,7 @@
int rtp_priority;

bool rtp_nogaps_mode; /* emit RTP stream without any gaps */
+ bool use_ul_ecu; /* "rtp internal-uplink-ecu" option */
bool emit_hr_rfc5993;

struct {
diff --git a/src/common/vty.c b/src/common/vty.c
index 9f3c675..0fc9007 100644
--- a/src/common/vty.c
+++ b/src/common/vty.c
@@ -418,6 +418,8 @@
vty_out(vty, " rtp socket-priority %d%s", bts->rtp_priority, VTY_NEWLINE);
if (bts->rtp_nogaps_mode)
vty_out(vty, " rtp continuous-streaming%s", VTY_NEWLINE);
+ vty_out(vty, " %srtp internal-uplink-ecu%s",
+ bts->use_ul_ecu ? "" : "no ", VTY_NEWLINE);
vty_out(vty, " rtp hr-format %s%s",
bts->emit_hr_rfc5993 ? "rfc5993" : "ts101318", VTY_NEWLINE);
vty_out(vty, " paging queue-size %u%s", paging_get_queue_max(bts->paging_state),
@@ -799,6 +801,28 @@
return CMD_SUCCESS;
}

+DEFUN(cfg_bts_rtp_int_ul_ecu,
+ cfg_bts_rtp_int_ul_ecu_cmd,
+ "rtp internal-uplink-ecu",
+ RTP_STR "Apply a BTS-internal ECU to the uplink traffic frame stream\n")
+{
+ struct gsm_bts *bts = vty->index;
+
+ bts->use_ul_ecu = true;
+ return CMD_SUCCESS;
+}
+
+DEFUN(cfg_bts_no_rtp_int_ul_ecu,
+ cfg_bts_no_rtp_int_ul_ecu_cmd,
+ "no rtp internal-uplink-ecu",
+ NO_STR RTP_STR "Apply a BTS-internal ECU to the uplink traffic frame stream\n")
+{
+ struct gsm_bts *bts = vty->index;
+
+ bts->use_ul_ecu = false;
+ return CMD_SUCCESS;
+}
+
DEFUN_ATTR(cfg_bts_rtp_hr_format,
cfg_bts_rtp_hr_format_cmd,
"rtp hr-format (rfc5993|ts101318)",
@@ -2700,6 +2724,8 @@
install_element(BTS_NODE, &cfg_bts_rtp_priority_cmd);
install_element(BTS_NODE, &cfg_bts_rtp_cont_stream_cmd);
install_element(BTS_NODE, &cfg_bts_no_rtp_cont_stream_cmd);
+ install_element(BTS_NODE, &cfg_bts_rtp_int_ul_ecu_cmd);
+ install_element(BTS_NODE, &cfg_bts_no_rtp_int_ul_ecu_cmd);
install_element(BTS_NODE, &cfg_bts_rtp_hr_format_cmd);
install_element(BTS_NODE, &cfg_bts_band_cmd);
install_element(BTS_NODE, &cfg_description_cmd);
diff --git a/src/osmo-bts-trx/l1_if.c b/src/osmo-bts-trx/l1_if.c
index b39c51f..69ee117 100644
--- a/src/osmo-bts-trx/l1_if.c
+++ b/src/osmo-bts-trx/l1_if.c
@@ -439,8 +439,13 @@
break;
}

- /* attempt to allocate an Error Concealment Unit instance, if available */
- lchan->ecu_state = osmo_ecu_init(trx, lchan2ecu_codec(lchan));
+ /* Attempt to allocate an Error Concealment Unit
+ * instance, if available, unless it is disabled
+ * by the vty config. */
+ if (trx->bts->use_ul_ecu)
+ lchan->ecu_state = osmo_ecu_init(trx, lchan2ecu_codec(lchan));
+ else
+ lchan->ecu_state = NULL;

/* activate dedicated channel */
trx_sched_set_lchan(lchan, chan_nr, LID_DEDIC, true);
@@ -473,7 +478,10 @@
/* ECU for possibly new codec */
if (lchan->ecu_state)
osmo_ecu_destroy(lchan->ecu_state);
- lchan->ecu_state = osmo_ecu_init(trx, lchan2ecu_codec(lchan));
+ if (trx->bts->use_ul_ecu)
+ lchan->ecu_state = osmo_ecu_init(trx, lchan2ecu_codec(lchan));
+ else
+ lchan->ecu_state = NULL;
/* change mode */
trx_sched_set_mode(lchan->ts, chan_nr,
lchan->rsl_cmode, lchan->tch_mode,
diff --git a/src/osmo-bts-trx/main.c b/src/osmo-bts-trx/main.c
index f3b8513..ff71b3a 100644
--- a/src/osmo-bts-trx/main.c
+++ b/src/osmo-bts-trx/main.c
@@ -164,6 +164,10 @@
* a surprise functional change upon software update. */
bts->emit_hr_rfc5993 = true;

+ /* For the same reason as the above, rtp internal-uplink-ecu
+ * needs to be enabled by default on osmo-bts-trx model only. */
+ bts->use_ul_ecu = true;
+
return 0;
}

diff --git a/tests/osmo-bts.vty b/tests/osmo-bts.vty
index 356f2aa..c473234 100644
--- a/tests/osmo-bts.vty
+++ b/tests/osmo-bts.vty
@@ -235,6 +235,8 @@
rtp socket-priority <0-255>
rtp continuous-streaming
no rtp continuous-streaming
+ rtp internal-uplink-ecu
+ no rtp internal-uplink-ecu
rtp hr-format (rfc5993|ts101318)
band (450|GSM450|480|GSM480|750|GSM750|810|GSM810|850|GSM850|900|GSM900|1800|DCS1800|1900|PCS1900)
description .TEXT

To view, visit change 32110. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I0acca9c6d7da966a623287563e0789db9e0fae8e
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 7
Gerrit-Owner: falconia <falcon@freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: falconia <falcon@freecalypso.org>
Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de>
Gerrit-Reviewer: laforge <laforge@osmocom.org>
Gerrit-Reviewer: pespin <pespin@sysmocom.de>
Gerrit-MessageType: merged