Attention is currently required from: laforge, osmith. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/35124?usp=email ) Change subject: Add GSMTAP encapsulation of RLP frames in CSD NT mode ...................................................................... Patch Set 6: Code-Review-1 (5 comments) File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/35124/comment/596bb9af_6b338676 PS6, Line 1854: if (!inst || !trx->bts->gsmtap.rlp) We also need to check `if (lchan->csd_mode == LCHAN_CSD_M_NT)`, otherwise we would be emitting garbage RLP frames during transparent calls. I saw this check in an early revision of this patch, but somehow it's gone? https://gerrit.osmocom.org/c/osmo-bts/+/35124/comment/2382735b_2be5884e PS6, Line 1866: 2x Actually it's `4x36bit` for TCH/H2.4. Similarly to TCH/H4.8 carrying more data (per-block) than TCH/F4.8. * TCH/F2.4: `2 * 36` bits every 20 ms, so `2 * 36 * (1000 / 20) = 3600` => 3.6 kbps * TCH/H2.4: `4 * 36` bits every 40 ms, so `4 * 36 * (1000 / 40) = 3600` => 3.6 kbps https://gerrit.osmocom.org/c/osmo-bts/+/35124/comment/e05035e2_2a196cef PS6, Line 1867: 2x 290 bit IIRC, 290 is actually `8 * 36 + 2`. https://gerrit.osmocom.org/c/osmo-bts/+/35124/comment/ae1a3861_11e2db86 PS6, Line 1867: TCH/H 14.4 TCH/F, not half-rate. https://gerrit.osmocom.org/c/osmo-bts/+/35124/comment/a31d677b_cf7187f9 PS6, Line 1879: } else { I think it's fine to have it enabled in general. pylint is also enforcing the same rule for Python by default. The benefit of following this rule is reduced nesting. But I wish there was a way to say 'wontfix' for specific comments, rather than having to remove Jenkins and do CR+1 manually, or having to disable checks in the linter. -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/35124?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: I6a258458822bcb3fe7290a9b9b3d104beecda219 Gerrit-Change-Number: 35124 Gerrit-PatchSet: 6 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: osmith <osmith(a)sysmocom.de> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Thu, 30 Nov 2023 10:08:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: osmith <osmith(a)sysmocom.de> Gerrit-MessageType: comment