Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/32056 )
Change subject: trxcon: add GSMTAP logging target if '-g' is given
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
I think we should start thinking about adding a VTY there, in order to avoid adding this kind of stuff one after another.
Furthermore, we could then also be able to use the cpusched stuff already available in libosmovty, etc.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/32056
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I2c5e630dd508dff12d0116bdc0a4cc1276cac5ed
Gerrit-Change-Number: 32056
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 25 Mar 2023 01:05:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: laforge, pespin.
Hello Jenkins Builder, laforge,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/32034
to look at the new patch set (#2).
Change subject: codec: add osmo_efr_check_sid() function
......................................................................
codec: add osmo_efr_check_sid() function
Previously existing code provides osmo_fr_check_sid() and
osmo_hr_check_sid() functions for FR1 and HR1 codecs; these functions
are used by various RTP-touching programs in the Osmocom CNI suite
when they need to differentiate between speech and SID frames.
However, there was no corresponding function of this form for EFR
codec, with the result being that the same programs that handle
speech vs SID distinction correctly for FR1 and HR1 fail to do so
for EFR.
The present change adds an osmo_efr_check_sid() function to libosmocodec
that fully mirrors previously existing osmo_fr_check_sid() and
osmo_hr_check_sid(), providing the first step toward more correct
EFR handling in programs where a SID check may be needed.
Change-Id: Iab9fb60028f4135375287bc42f5da7ca7838b5f0
---
M include/osmocom/codec/codec.h
M src/codec/gsm660.c
2 files changed, 68 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/34/32034/2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32034
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Iab9fb60028f4135375287bc42f5da7ca7838b5f0
Gerrit-Change-Number: 32034
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: laforge, pespin.
falconia has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32034 )
Change subject: codec: add osmo_efr_check_sid() function
......................................................................
Patch Set 1:
(3 comments)
File src/codec/gsm660.c:
https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/68640fc0_ddd57eeb
PS1, Line 271: static con
> If you asked me my bet would be that a local non-static variable would always be allocated in the st […]
@laforge: I checked the code produced by gcc (5.3.0, came with Slackware 14.2) with and without the static, and if I drop the static, the compiler generates silly code: the actual table is still in .rodata, but it isn't entered into the symbol table as a sid_code_word_bits local symbol, instead there is a stack allocation (the actual "object") and the compiler emits code (basically an inline memcpy equivalent) that copies the table from .rodata to the stack. Putting the static back in results in sane generated code: just the table in .rodata, with the sid_code_word_bits local symbol on it, no extra stack allocation or copying. Hence I argue for keeping the static.
@pespin: you correctly interpreted my intent here.
https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/68ce2a90_b31f85ec
PS1, Line 272: /* bit numbers relative to "pure" EFR frame beginning,
: * not counting the signature bits. */
: 45, 46, 48, 49, 50, 51, 52, 53, 54, 55,
: 56, 57, 58, 59, 60, 61, 62, 63, 64, 65,
: 66, 67, 68, 94, 95, 96, 98, 99, 100, 101,
: 102, 103, 104, 105, 106, 107, 108, 109, 110, 111,
: 112, 113, 114, 115, 116, 117, 118, 148, 149, 150,
: 151, 152, 153, 154, 155, 156, 157, 158, 159, 160,
: 161, 162, 163, 164, 165, 166, 167, 168, 169, 170,
: 171, 196, 197, 198, 199, 200, 201, 202, 203, 204,
: 205, 206, 207, 208, 209, 212, 213, 214, 215, 216,
: 217, 218, 219, 220, 221 };
> Ack
I was going for the same code structure and style as in osmo_fr_check_sid() in gsm610.c, but I agree that the code looks better and more stylistically correct with one less tab of indentation. I'll be spinning the next patch version with the reduced indent.
https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/e684efdf_ddf89716
PS1, Line 293: for (i = 0; i < ARRAY_SIZE(sid_code_word_bits); i++)
> In general we always use {} in code blocks with multiple lines in it.
Once again I was copying the code structure and style from osmo_fr_check_sid() and osmo_hr_check_sid(), where the same construct appears without the braces. But following the principle that newly added code should be styled "right", I am changing the construct in question to the braced style in the next patch version.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32034
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Iab9fb60028f4135375287bc42f5da7ca7838b5f0
Gerrit-Change-Number: 32034
Gerrit-PatchSet: 1
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 24 Mar 2023 23:07:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmocom-bb/+/32056 )
Change subject: trxcon: add GSMTAP logging target if '-g' is given
......................................................................
trxcon: add GSMTAP logging target if '-g' is given
Unlike the other more mature Osmocom projects, trxcon does not have its
own VTY interface and thus does not support the config file parsing,
so currently it's impossible to configure additional logging targets.
There is a command line option '-g', which enables GSMTAP Um logging.
Let's also add a GSMTAP logging target if it's given. This is a quick
hack, but good enough for occasional debugging.
Change-Id: I2c5e630dd508dff12d0116bdc0a4cc1276cac5ed
Related: OS#5500
---
M src/host/trxcon/src/trxcon_main.c
1 file changed, 31 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/56/32056/1
diff --git a/src/host/trxcon/src/trxcon_main.c b/src/host/trxcon/src/trxcon_main.c
index 26a5742..d30c98e 100644
--- a/src/host/trxcon/src/trxcon_main.c
+++ b/src/host/trxcon/src/trxcon_main.c
@@ -348,11 +348,23 @@
/* Optional GSMTAP */
if (app_data.gsmtap_ip != NULL) {
+ struct log_target *lt;
+
app_data.gsmtap = gsmtap_source_init(app_data.gsmtap_ip, GSMTAP_UDP_PORT, 1);
if (!app_data.gsmtap) {
- LOGP(DAPP, LOGL_ERROR, "Failed to init GSMTAP\n");
+ LOGP(DAPP, LOGL_ERROR, "Failed to init GSMTAP Um logging\n");
goto exit;
}
+
+ lt = log_target_create_gsmtap(app_data.gsmtap_ip, GSMTAP_UDP_PORT,
+ "trxcon", false, false);
+ if (lt == NULL) {
+ LOGP(DAPP, LOGL_ERROR, "Failed to init GSMTAP logging target\n");
+ goto exit;
+ } else {
+ log_add_target(lt);
+ }
+
/* Suppress ICMP "destination unreachable" errors */
gsmtap_source_add_sink(app_data.gsmtap);
}
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/32056
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I2c5e630dd508dff12d0116bdc0a4cc1276cac5ed
Gerrit-Change-Number: 32056
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmocom-bb/+/32058 )
Change subject: trxcon: do not call l1sched_prim_dequeue() at ul_bid != 0
......................................................................
trxcon: do not call l1sched_prim_dequeue() at ul_bid != 0
It may happen that the Tx queue is empty at TDMA Fn corresponding to
ul_bid == 0, and then shortly after something appears at ul_bid != 0.
Change-Id: Ic0bbe2ab6c0ccd96c1f8af8aa17ac88adf7f88ed
Related: OS#5500
---
M src/host/trxcon/src/sched_trx.c
1 file changed, 22 insertions(+), 7 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/58/32058/1
diff --git a/src/host/trxcon/src/sched_trx.c b/src/host/trxcon/src/sched_trx.c
index ac55f92..23c8f3f 100644
--- a/src/host/trxcon/src/sched_trx.c
+++ b/src/host/trxcon/src/sched_trx.c
@@ -120,14 +120,16 @@
return;
/* If no primitive is being processed, try obtaining one from Tx queue */
- if (lchan->prim == NULL)
- lchan->prim = l1sched_prim_dequeue(&ts->tx_prims, br->fn, lchan);
- if (lchan->prim == NULL) {
- /* If CBTX (Continuous Burst Transmission) is required */
- if (l1sched_lchan_desc[chan].flags & L1SCHED_CH_FLAG_CBTX)
- l1sched_prim_dummy(lchan);
+ if (frame->ul_bid == 0) {
if (lchan->prim == NULL)
- return;
+ lchan->prim = l1sched_prim_dequeue(&ts->tx_prims, br->fn, lchan);
+ if (lchan->prim == NULL) {
+ /* If CBTX (Continuous Burst Transmission) is required */
+ if (l1sched_lchan_desc[chan].flags & L1SCHED_CH_FLAG_CBTX)
+ l1sched_prim_dummy(lchan);
+ if (lchan->prim == NULL)
+ return;
+ }
}
/* TODO: report TX buffers health to the higher layers */
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/32058
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Ic0bbe2ab6c0ccd96c1f8af8aa17ac88adf7f88ed
Gerrit-Change-Number: 32058
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange