Attention is currently required from: falconia, pespin.
Hello Jenkins Builder, fixeria, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bts/+/38556?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
The change is no longer submittable: Verified is unsatisfied now.
Change subject: csd_v110_rtp_decode: preserve E2 & E3 bits for RLP alignment
......................................................................
csd_v110_rtp_decode: preserve E2 & E3 bits for RLP alignment
Modify CSD RTP input path code so incoming bits E2 & E3 from V.110
frames reach the TCH-RTS.ind handler in l1sap. These bits will be
used in the next patch to ensure proper alignment of DL RLP frames
in non-transparent CSD modes.
Related: OS#6579
Change-Id: I43b97caa6030b9401779998ca5dddc4cfe636e2f
---
M include/osmo-bts/csd_v110.h
M include/osmo-bts/msg_utils.h
M src/common/csd_v110.c
M src/common/l1sap.c
M tests/csd/csd_test.c
5 files changed, 17 insertions(+), 4 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/56/38556/3
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38556?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I43b97caa6030b9401779998ca5dddc4cfe636e2f
Gerrit-Change-Number: 38556
Gerrit-PatchSet: 3
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Attention is currently required from: pespin.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bts/+/38557?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
Change subject: CSD NT modes: transmit properly aligned RLP frames on DL
......................................................................
CSD NT modes: transmit properly aligned RLP frames on DL
There are two levels of alignment inside clearmode RTP packets
carrying CSData per TS 48.103 section 5.6:
1) Alignment of 2 or 4 V.110 (T) or pseudo-V.110 (NT) frames within
one RTP packet of 160 octets of an imaginary ISDN B channel;
2) For NT modes only, alignment of 4 pseudo-V.110 frames to form
a single 240-bit RLP frame.
Per previous patch, alignment 1 is to be treated as mandatory for
RTP transport inside an Osmocom network. Alignment 2 _could_ be
made mandatory for TCH/F9.6 NT, but the same is not possible for
TCH/[FH]4.8 NT: in the best case of half-alignment, alternating
RTP packets will carry alternating halves of RLP frames.
Implemented solution: allow arbitrary state of alignment 2
(aligned or misaligned) in the incoming RTP stream for all CSD NT
modes, and perform the necessary alignment internally.
This approach is consistent with the world of E1 BTS: a TRAU in
data mode is responsible for alignment 1 (with 20 ms TRAU frames
taking the place of our clearmode RTP packets), but only the BTS can
perform alignment 2, as the TRAU is agnostic to T vs NT distinction.
Related: OS#6579
Change-Id: Idaebfce6da13b23ba265a197502712d83991873e
---
M include/osmo-bts/csd_rlp.h
M include/osmo-bts/lchan.h
M src/common/csd_rlp.c
M src/common/l1sap.c
4 files changed, 301 insertions(+), 29 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/57/38557/3
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38557?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Idaebfce6da13b23ba265a197502712d83991873e
Gerrit-Change-Number: 38557
Gerrit-PatchSet: 3
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Attention is currently required from: fixeria, pespin.
falconia has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-bts/+/38553?usp=email )
Change subject: CSD: implement half-rate modes correctly
......................................................................
Patch Set 2:
(4 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/9cc638f0_5f775bca?usp… :
PS2, Line 26: by emitting two RTP packets
: directly back-to-back for UL, and pulling two RTP packets at a time
: from the Rx jitter buffer at the needed times for DL.
> I understand your idea now. […]
You are correct: if we were implementing a new E1 BTS, there would be no other way to handle the present issue except by buffering the bits and emitting them at the TDM-controlled rate. However, E1 BTS do similar buffering at all times, not just in this one corner case. Consider what happens with all other TCH types besides CSD-HR, i.e., consider CSD-FR and all speech modes. For those "traditional" TCH, osmo-bts (all models, not just trx) emits an RTP packet statistically every 20 ms - but it is not perfect 20 ms, it has TDMA jitter, with the actual time deltas between emitted RTP packets being TDMA*4 or TDMA*5 per multiframe structure. E1 BTS are different in this regard: their internal buffering fully conceals TDMA jitter from the external TDM connection, and the timing of TRAU frames on Abis is perfect 20 ms for each frame.
This aspect of osmo-bts (the fact that we normally expose our TDMA jitter in RTP output) is the main reason why I felt it would make no sense to try to replicate the exact behavior of E1 BTS here. Suppose we wanted to artificially delay the second half of CSD-HR frames by 20 ms to make our RTP output more E1-like - would it be a delay of exactly 20 ms per Linux system clock? Or would be 4 TDMA frames? Or 5 TDMA frames? Exactly which FNs would produce an artificial delay of 4 vs 5 frames? There won't be an answer in the specs as we talking about purely artificial delay here. So why delay at all, why not simply emit both packets right away...
File src/common/l1sap.c:
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/f0babc20_9d4822b7?usp… :
PS2, Line 1539: 2
> `ARRAY_SIZE(input_msg)` here and below, please.
Will do.
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/938ed2ec_7f9c834d?usp… :
PS2, Line 1556: else
> I would put a comment that it's an IDLE frame (all bits set to 1).
Will do.
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/3e3f65cd_38cd9886?usp… :
PS2, Line 2098: switch (lchan->tch_mode) {
> As a further improvement, I suggest sharing `csd_v110_lchan_desc[]`, so that it can be used here (`d […]
Not sure if I agree - let me handle all other feedback first, then I'll get back to this one.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38553?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ib35e910df263743cd243f51fb0bd6551ddfcf4c5
Gerrit-Change-Number: 38553
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: 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: Sun, 27 Oct 2024 22:06:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Attention is currently required from: falconia, pespin.
fixeria has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-bts/+/38555?usp=email )
Change subject: CSD RTP: verify alignment of V.110 frames
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
File src/common/csd_v110.c:
https://gerrit.osmocom.org/c/osmo-bts/+/38555/comment/7be057c4_ad8d113b?usp… :
PS2, Line 158: for (i = 0; i < 8; i++) {
We can avoid if-statement in loops to improve performance (in theory):
```
ubit_t bit0 = 0;
ubit_t bit1 = 1;
for (i = 0; i < 8; i++)
bit0 |= ra_bits[i];
for (i = 1; i < 10; i++)
bit1 &= ra_bits[i * 8];
return (bit0 == 0) && (bit1 == 1);
```
https://gerrit.osmocom.org/c/osmo-bts/+/38555/comment/3e949862_619f828a?usp… :
PS2, Line 204: return -EINVAL;
> We do have a rate_ctr: when `csd_v110_rtp_decode()` returns negative, the code in `l1sap_rtp_rx_cb() […]
Acknowledged
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38555?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Icd704dc7fa02e60074efc8a29ad7e42ebdf63783
Gerrit-Change-Number: 38555
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(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: Sun, 27 Oct 2024 21:34:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: falconia <falcon(a)freecalypso.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: falconia, pespin.
fixeria has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-bts/+/38554?usp=email )
Change subject: csd_v110: set E2 bit correctly for TCH/[FH]4.8 NT
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
File src/common/l1sap.c:
https://gerrit.osmocom.org/c/osmo-bts/+/38554/comment/6e559ef9_2a6b1630?usp… :
PS2, Line 2066: static const uint8_t tchf48_nt_e2_map[26] = {
> again where does this 26 come from
Obviously from 3GPP specs. mentioned above in comment...
It's further explained in the first patch of this patchset.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38554?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I485af5e01ea87c1721a298a486cd344a17884200
Gerrit-Change-Number: 38554
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 27 Oct 2024 21:26:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: falconia, pespin.
fixeria has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-bts/+/38553?usp=email )
Change subject: CSD: implement half-rate modes correctly
......................................................................
Patch Set 2: Code-Review+1
(5 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/f55d153e_392cfca0?usp… :
PS2, Line 26: by emitting two RTP packets
: directly back-to-back for UL, and pulling two RTP packets at a time
: from the Rx jitter buffer at the needed times for DL.
I understand your idea now. Just curious, how would you do that if osmo-bts were an E1 BTS (not Abis-over-IP)? I guess the only option would be to buffer V.110 frames, since you cannot do the same trick for a TDM interface.
File src/common/l1sap.c:
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/7e6234ce_4292240f?usp… :
PS2, Line 1493: /* TDMA frame number of burst 'a' % 26 is the table index.
> This bit of code is not newly written with this patch - I simply moved it from osmo-bts-trx to the c […]
I agree with Mychaela here. It's a well known constant that is used quite often here and there.
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/5aa500ad_532f6eb5?usp… :
PS2, Line 1539: 2
`ARRAY_SIZE(input_msg)` here and below, please.
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/d0770a5a_b08f40ee?usp… :
PS2, Line 1556: else
I would put a comment that it's an IDLE frame (all bits set to 1).
https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/8879c982_8934bcf2?usp… :
PS2, Line 2098: switch (lchan->tch_mode) {
As a further improvement, I suggest sharing `csd_v110_lchan_desc[]`, so that it can be used here (`desc->num_blocks * desc->num_bits`). I have a WIP patch moving that lookup table to libosmocore: https://gerrit.osmocom.org/c/libosmocore/+/35914.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38553?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ib35e910df263743cd243f51fb0bd6551ddfcf4c5
Gerrit-Change-Number: 38553
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 27 Oct 2024 21:22:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: falconia <falcon(a)freecalypso.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: pespin.
falconia has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-bts/+/38557?usp=email )
Change subject: CSD NT modes: transmit properly aligned RLP frames on DL
......................................................................
Patch Set 2:
(1 comment)
File src/common/l1sap.c:
https://gerrit.osmocom.org/c/osmo-bts/+/38557/comment/6f3e27fc_42bbebdd?usp… :
PS2, Line 1520: bool good_rlp;
> you can probably move this to a more specific scope below, since this function is quite large and th […]
Noted, will do.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38557?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Idaebfce6da13b23ba265a197502712d83991873e
Gerrit-Change-Number: 38557
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 27 Oct 2024 20:47:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: pespin.
falconia has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/osmo-bts/+/38556?usp=email )
Change subject: csd_v110_rtp_decode: preserve E2 & E3 bits for RLP alignment
......................................................................
Patch Set 2:
(1 comment)
File src/common/csd_v110.c:
https://gerrit.osmocom.org/c/osmo-bts/+/38556/comment/e8674549_fbb46b1a?usp… :
PS2, Line 213: align_accum <<= 2;
> IIUC desc->num_blocks can never be >4 then right?
Correct. 4 is the greatest possible number of V.110 frames per 20 ms clearmode packet of TS 48.103 section 5.6, occurring only in TCH/F9.6 mode.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/38556?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I43b97caa6030b9401779998ca5dddc4cfe636e2f
Gerrit-Change-Number: 38556
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 27 Oct 2024 20:43:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>