Attention is currently required from: Thorkell Thorkelsson, fixeria, pespin.
falconia has posted comments on this change by Thorkell Thorkelsson. ( https://gerrit.osmocom.org/c/osmo-bts/+/42244?usp=email ) Change subject: tch: fix RTP clock not ticking for unhandled payload types ...................................................................... Patch Set 2: Code-Review-1 (1 comment) File src/osmo-bts-sysmo/tch.c: https://gerrit.osmocom.org/c/osmo-bts/+/42244/comment/aec6ad15_7b86e380?usp=email : PS2, Line 601: FIXME: what about GsmL1_TchPlType_Amr_SidBad? not well documented > The patch catches Amr_SidBad in the generic fallback case. […] First of all, when a BTS receives a corrupted SID update (one that fails CRC) on TCH/AFS or TCH/AHS in the middle of a DTX pause, an empty RTP tick is **not** the optimal handling. It is certainly better than getting RTP time scale out of sync, but it is not ideal. Instead the correct handling is to emit RFC 4867 representation of a SID_BAD frame: FT=8, Q=0, STI=1. But because we have the additional complication of sysmoBTS PHY being a black box (with insufficient documentation, according to the comment which you shouldn't be deleting), what we really need to do is experiment with different TCH UL conditions for AMR (including a special GSM MS that can transmit intentionally corrupted SID update blocks) and observe what sysmoBTS PHY puts out in each condition. It won't be easy - hence I currently avoid AMR and run my sysmoBTS with EFR instead... But back to your proposed patch, there are two separate issues going on: 1. In cases when, for whatever reason, we are not able to handle the Rx frame from the sysmoBTS PHY in a truly proper manner, we should emit an empty RTP tick instead of just dropping that 20 ms quantum of time. In this regard your patch is fully proper and welcome. 2. Applying truly correct handling to AMR corner cases is a separate problem that will remain unsolved for now, until someone who cares about the combination of sysmoBTS hw with AMR invests the time and effort needed for a proper investigation. However, I agree very strongly with Vadim that the FIXME comment should **not** be removed. Therefore, I have to CR-1 your patch for now. If you restore that FIXME comment in the next patch iteration, I will give CR+1 as the rest of your patch is fine. -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/42244?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: I202522ea6f83d77872a2f84d9a2622b87e829a0c Gerrit-Change-Number: 42244 Gerrit-PatchSet: 2 Gerrit-Owner: Thorkell Thorkelsson <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: falconia <[email protected]> Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: Thorkell Thorkelsson <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Sun, 01 Mar 2026 21:08:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Thorkell Thorkelsson <[email protected]> Comment-In-Reply-To: fixeria <[email protected]>
