October 2024 - gerrit-log - lists.osmocom.org

[S] Change in osmo-bts[master]: CSD RTP: verify alignment of V.110 frames
by falconia 27 Oct '24

27 Oct '24

Attention is currently required from: pespin. falconia 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: (1 comment) File src/common/csd_v110.c: https://gerrit.osmocom.org/c/osmo-bts/+/38555/comment/0735cc52_0579239d?usp… : PS2, Line 204: return -EINVAL; > sounds like we may want to log this, or/and have a rate_ctr fr this event? We do have a rate_ctr: when `csd_v110_rtp_decode()` returns negative, the code in `l1sap_rtp_rx_cb()` does this: ``` rate_ctr_inc2(bts->ctrs, BTS_CTR_RTP_RX_DROP_V110_DEC); ``` -- 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: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Sun, 27 Oct 2024 20:41:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>

1 0

[L] Change in osmo-bts[master]: CSD: implement half-rate modes correctly
by falconia 27 Oct '24

27 Oct '24

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) File include/osmo-bts/csd_v110.h: https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/b37caa59_22d35635?usp… : PS2, Line 11: uint8_t ra2_ir; > I can't figure out what this field is about by looking at it, maybe a short comment here helps. It is the intermediate rate (8 or 16 kbit/s) to be used in the RA2 packing/unpacking step. File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/2ffbf778_eeccf58d?usp… : PS2, Line 1493: /* TDMA frame number of burst 'a' % 26 is the table index. > where does this 26 come from? do we have a define for it? Can we add one? This bit of code is not newly written with this patch - I simply moved it from osmo-bts-trx to the common layer. 26 is the number of TDMA frames per TCH multiframe, or the number of TDMA frames after which we get a perfectly repeating pattern of which frame numbers we need to trigger on for this or that function. The original TCH scheduling code in `src/osmo-bts-trx/sched_lchan_tch[fh].c` makes very heavy use of directly-written number 26. I disagree with the obsessive compulsion to add a define for this 26. While I wrote "TCH multiframe" above, GSM hackers will most often refer to it as a "26-multiframe" in casual speech, and I argue that the meaning will be obvious to anyone who knows GSM. I argue we can make a reasonable assumption that anyone hacking on osmo-bts code can be expected to know the GSM multiframe structure of 05.02, and will know exactly what 26 refers to when they hack on or even look at code paths that deal with TCH. https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/6e256b5c_cf8bfab4?usp… : PS2, Line 1523: if (!sched_tchh_dl_csd_map[fn % 26]) > define for 26 to be used here to. Again, I don't just have the number 26 randomly floating around in the code. Since the actual construct is FN modulo 26, anyone who knows GSM and is qualified to touch TCH handling code will immediately know what is going on here. Existing code in `src/osmo-bts-trx/sched_lchan_tch[fh].c` is exactly the same in this regard. https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/6f6d21b9_7f65168a?usp… : PS2, Line 2045: * back-to-back every 40 ms on the same frame number, */ > why are we sending with same frame number if they have different data? this looks weird. If you read my comment carefully, you'll see that I don't say anything about sending packets "with" the same frame number - instead I point out that we send two RTP packets **on** the same frame number. There is no GSM frame number field in RTP packets, hence sending "with" the same FN would be meaningless - but because we send two RTP packets (with different content, carrying first and second chunks of "virtual" 20-ms-worth) at the same instant in physical time, at the same point in our event-driven code execution, we are "on" the same GSM FN in terms of osmo-bts-trx view of GSM Um. The timestamp field in these two successive RTP packets does increment correctly - I verified it by examining pcap files from TTCN3 tests performed by @vyanitskiy@sysmocom.de, attached in OS#6579 ticket. As for the weirdness of sending two RTP packets directly back to back in the first place, I concur - but what is the alternative? How else would you satisfy the requirements of TS 48.103 section 5.6 for CSD-HR, how would you reconcile the requirement for 20 ms RTP packets with the physical reality of the channel coder running only every 40 ms statistically? Perhaps artificially delay the second half by buffering it and then sending it out 20 ms later? But why add artificial delays, if we have the data "in hand", why not send it out right away, even though it will be 20 ms early relative to when an E1 BTS would begin serially transmitting the equivalent TRAU frame... -- 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: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Sun, 27 Oct 2024 20:35:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>

1 0

[L] Change in osmo-bts[master]: CSD NT modes: transmit properly aligned RLP frames on DL
by pespin 27 Oct '24

27 Oct '24

Attention is currently required from: falconia. pespin 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: (2 comments) Patchset: PS2: You could perhaps split this commint into 2: First one moving stuff to new file, second one doing the changes. File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/38557/comment/87c258d8_89a4c876?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 then it's difficult to figure out where is this used. -- 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: falconia <falcon(a)freecalypso.org> Gerrit-Comment-Date: Sun, 27 Oct 2024 19:50:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in osmo-bts[master]: csd_v110_rtp_decode: preserve E2 & E3 bits for RLP alignment
by pespin 27 Oct '24

27 Oct '24

Attention is currently required from: falconia. pespin 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: Code-Review+1 (1 comment) File src/common/csd_v110.c: https://gerrit.osmocom.org/c/osmo-bts/+/38556/comment/54242181_62f1aabd?usp… : PS2, Line 213: align_accum <<= 2; IIUC desc->num_blocks can never be >4 then right? -- 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: falconia <falcon(a)freecalypso.org> Gerrit-Comment-Date: Sun, 27 Oct 2024 19:43:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[S] Change in osmo-bts[master]: CSD RTP: verify alignment of V.110 frames
by pespin 27 Oct '24

27 Oct '24

Attention is currently required from: falconia. pespin 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 (1 comment) File src/common/csd_v110.c: https://gerrit.osmocom.org/c/osmo-bts/+/38555/comment/a8247c00_ecff9d93?usp… : PS2, Line 204: return -EINVAL; sounds like we may want to log this, or/and have a rate_ctr fr this event? -- 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: pespin <pespin(a)sysmocom.de> Gerrit-Attention: falconia <falcon(a)freecalypso.org> Gerrit-Comment-Date: Sun, 27 Oct 2024 19:39:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[S] Change in osmo-bts[master]: csd_v110: set E2 bit correctly for TCH/[FH]4.8 NT
by pespin 27 Oct '24

27 Oct '24

Attention is currently required from: falconia. pespin 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: (2 comments) File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/38554/comment/68d67a83_14c4c114?usp… : PS2, Line 2066: static const uint8_t tchf48_nt_e2_map[26] = { again where does this 26 come from https://gerrit.osmocom.org/c/osmo-bts/+/38554/comment/06ce145f_d98a79e8?usp… : PS2, Line 2076: uint8_t tchf48_half = tchf48_nt_e2_map[tch_ind->fn % 26]; where dos 26 come from. -- 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-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: falconia <falcon(a)freecalypso.org> Gerrit-Comment-Date: Sun, 27 Oct 2024 19:35:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[L] Change in osmo-bts[master]: CSD: implement half-rate modes correctly
by pespin 27 Oct '24

27 Oct '24

Attention is currently required from: falconia. pespin 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) File include/osmo-bts/csd_v110.h: https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/5c5c8947_3e4f419f?usp… : PS2, Line 11: uint8_t ra2_ir; I can't figure out what this field is about by looking at it, maybe a short comment here helps. File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/45fd32a6_fad3fd4a?usp… : PS2, Line 1493: /* TDMA frame number of burst 'a' % 26 is the table index. where does this 26 come from? do we have a define for it? Can we add one? https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/75134c9e_1afbf897?usp… : PS2, Line 1523: if (!sched_tchh_dl_csd_map[fn % 26]) define for 26 to be used here to. https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/7ceea5fd_295a5839?usp… : PS2, Line 2045: * back-to-back every 40 ms on the same frame number, */ why are we sending with same frame number if they have different data? this looks weird. -- 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-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: falconia <falcon(a)freecalypso.org> Gerrit-Comment-Date: Sun, 27 Oct 2024 19:31:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[L] Change in osmo-bts[master]: CSD: implement half-rate modes correctly
by falconia 27 Oct '24

27 Oct '24

Attention is currently required from: falconia. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bts/+/38553?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: CSD: implement half-rate modes correctly ...................................................................... CSD: implement half-rate modes correctly TCH/H4.8 and TCH/H2.4 modes are different from all other TCH in that the channel coder runs (statistically) every 40 ms instead of the usual 20 ms. However, the standard RTP interface of TS 48.103 still requires 20 ms packets for all CSD modes; furtherfore, this requirement is not just a matter of 3GPP spec being obstinent, but is highly useful for interoperability, both between FR and HR and between OsmoBTS and E1 BTS. The handling of CSD HR was totally broken in this regard: wrong RA2 packing mode, RTP clock model was violated, and even an internal mismatch with l1sap sending TCH.req prims at twice the rate at which the PHY expects them. With this patch CSD HR handling becomes correct for TCH/H2.4 and for TCH/H4.8 transparent; fully correct handling for TCH/H4.8 NT will appear in a follow-up patch. The packet/frame rate mismatch (40 ms Rx/Tx times while each RTP packet carries 20 ms worth of data) is reconciled 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. Note: due to bug OS#6604, TTCN3 tests for CSD half-rate transparent modes will start failing once this patch is merged; to make them pass again, TTCN3 test code will need to be reworked to fix that bug. Related: OS#6577 Change-Id: Ib35e910df263743cd243f51fb0bd6551ddfcf4c5 --- M include/osmo-bts/csd_v110.h M src/common/csd_v110.c M src/common/l1sap.c M src/osmo-bts-trx/sched_lchan_tchh.c M tests/csd/csd_test.c M tests/csd/csd_test.err 6 files changed, 258 insertions(+), 112 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/53/38553/2 -- 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: newpatchset 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-Attention: falconia <falcon(a)freecalypso.org>

1 0

[L] Change in osmo-bts[master]: CSD: implement half-rate modes correctly
by Jenkins Builder 27 Oct '24

27 Oct '24

Jenkins Builder 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 1: (6 comments) File src/common/l1sap.c: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-19465): https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/3e93bb42_b18a51dc?usp… : PS1, Line 2064: const uint8_t *data, uint16_t data_len) code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-gerrit-lint-19465): https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/1510371f_47b52466?usp… : PS1, Line 2064: const uint8_t *data, uint16_t data_len) please, no space before tabs Robot Comment from checkpatch (run ID jenkins-gerrit-lint-19465): https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/9de6dd21_398f2fe8?usp… : PS1, Line 2092: const uint8_t *data, uint16_t data_len) code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-gerrit-lint-19465): https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/5de453ae_a4b829da?usp… : PS1, Line 2092: const uint8_t *data, uint16_t data_len) please, no space before tabs Robot Comment from checkpatch (run ID jenkins-gerrit-lint-19465): https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/47c1f6de_38b2229d?usp… : PS1, Line 2126: const uint8_t *data, uint16_t data_len) code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-gerrit-lint-19465): https://gerrit.osmocom.org/c/osmo-bts/+/38553/comment/60fe7884_d97e4f6a?usp… : PS1, Line 2126: const uint8_t *data, uint16_t data_len) please, no space before tabs -- 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: 1 Gerrit-Owner: falconia <falcon(a)freecalypso.org> Gerrit-CC: Jenkins Builder Gerrit-Comment-Date: Sun, 27 Oct 2024 17:48:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in osmo-bts[master]: CSD RTP: verify alignment of V.110 frames
by falconia 27 Oct '24

27 Oct '24

falconia has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bts/+/38555?usp=email ) Change subject: CSD RTP: verify alignment of V.110 frames ...................................................................... CSD RTP: verify alignment of V.110 frames Since the beginning of CSD implementation, OsmoBTS has operated with an implicit (unstated) policy that V.110 frames must be perfectly aligned within received clearmode RTP packets - a requirement which is NOT set anywhere in TS 48.103 or any of the other specs it references. This design policy is sensible from the standpoint of implementation complexity (both OsmoBTS and osmo_trau2rtp emit such perfectly aligned packets; if someone is building a gateway between an Osmocom GSM network and ISDN-style external networks, that gateway can act as the aligner), but it should be explicit rather than implicit. Check V.110 frame alignment in received clearmode RTP packets, and reject packets that fail this alignment check. Change-Id: Icd704dc7fa02e60074efc8a29ad7e42ebdf63783 --- M src/common/csd_v110.c 1 file changed, 21 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/55/38555/1 diff --git a/src/common/csd_v110.c b/src/common/csd_v110.c index cbbb83b..edd051e 100644 --- a/src/common/csd_v110.c +++ b/src/common/csd_v110.c @@ -20,6 +20,7 @@ */ #include <stdint.h> +#include <stdbool.h> #include <errno.h> #include <osmocom/core/bits.h> @@ -150,6 +151,21 @@ return RFC4040_RTP_PLEN; } +static bool check_v110_align(const ubit_t *ra_bits) +{ + int i; + + for (i = 0; i < 8; i++) { + if (ra_bits[i]) + return false; + } + for (i = 1; i < 10; i++) { + if (!ra_bits[i * 8]) + return false; + } + return true; +} + int csd_v110_rtp_decode(const struct gsm_lchan *lchan, uint8_t *data, const uint8_t *rtp, size_t rtp_len) { @@ -181,6 +197,11 @@ for (unsigned int i = 0; i < desc->num_blocks; i++) { struct osmo_v110_decoded_frame df; + /* We require our RTP input to consist of aligned V.110 + * frames. If we get misaligned input, let's catch it + * explicitly, rather than send garbage downstream. */ + if (!check_v110_align(&ra_bits[i * 80])) + return -EINVAL; /* convert a V.110 80-bit frame to a V.110 36-/60-bit frame */ osmo_v110_decode_frame(&df, &ra_bits[i * 80], 80); if (desc->num_bits == 60) -- 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: newchange Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Icd704dc7fa02e60074efc8a29ad7e42ebdf63783 Gerrit-Change-Number: 38555 Gerrit-PatchSet: 1 Gerrit-Owner: falconia <falcon(a)freecalypso.org>

1 0