Desktop: clamp TX output at unity gain (resample overshoot overdrives the rig)#580
Open
patrickrb wants to merge 1 commit into
Open
Desktop: clamp TX output at unity gain (resample overshoot overdrives the rig)#580patrickrb wants to merge 1 commit into
patrickrb wants to merge 1 commit into
Conversation
…rdriving the rig
The desktop TX output stage folded the full-scale clamp into the gain step and
guarded it out whenever `gain == 1.0`:
if gain != 1.0 {
for s in data.iter_mut() { *s = (*s * gain).clamp(-1.0, 1.0); }
}
The guard is inverted relative to need. When `gain < 1.0` the product is already
in range so the clamp is a no-op; at `gain == 1.0` (TX level 100%) the clamp is
the only thing taming post-resample overshoot — and that is exactly the branch
that was skipped.
FFT resampling (rubato `FftFixedIn`) of the full-scale GFSK waveform, which peaks
at exactly ±1.0, interpolates a few samples slightly past full scale — measured
≈1.049 for 12 kHz→48 kHz (85 of 49152 samples over 1.0). The f32 output path
hands samples to the driver verbatim (`f32::from_sample` is identity) with no
saturation net, unlike the i16 cast which saturates in Rust. So at unity gain
those >1.0 samples reach the soundcard unclamped and overdrive the SSB rig's
input past 0 dBFS → ALC overshoot / distortion / spurious emissions. Desktop
almost always resamples on TX (hardware rarely offers 12 kHz), and the default
`DEFAULT_TX_GAIN = 0.9` clips (masking the bug) — only a 100% TX level exposes
it.
Root cause: the clamp is safety-critical and must not depend on gain. Fix mirrors
Android, which clamps unconditionally on the output cast
(`FT8TransmitSignal.float2Short` / `floatToInt16NoPad`), kept separate from its
`applyVolume` gain step. Extracted `apply_gain_and_clip()` and call it
unconditionally; for in-range samples the result is byte-identical to the old
`gain < 1.0` path.
Tests: 4 pure `#[cfg(test)]` cases in output.rs — unity-gain overshoot is clamped,
gain scales-then-clamps, in-range unity gain is identity, and an end-to-end case
that resamples a full-scale tone (asserting the resampler really overshoots) then
verifies the output stage clamps it at gain 1.0. The two unity-gain cases fail on
the pre-fix guarded code, verified by reverting only the guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #580 +/- ##
============================================
+ Coverage 29.36% 29.59% +0.23%
Complexity 197 197
============================================
Files 179 179
Lines 24068 24099 +31
Branches 3263 3263
============================================
+ Hits 7067 7133 +66
+ Misses 16780 16745 -35
Partials 221 221
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a safety-critical bug in the desktop (Tauri) TX audio output path where full-scale clipping was accidentally skipped at gain == 1.0, allowing FFT-resampler overshoot to reach the audio driver unclamped and potentially overdrive the rig.
Changes:
- Extracts an
apply_gain_and_clip()helper and applies it unconditionally after resampling (including at unity gain). - Removes the previous
if gain != 1.0guard that prevented clamping at 100% TX level. - Adds unit tests covering clamping behavior at unity gain and basic end-to-end resample → clamp behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+268
to
+273
| // Guard the test itself: the resampler really does overshoot, so the | ||
| // clamp is doing real work (not silently passing on a no-overshoot input). | ||
| assert!( | ||
| data.iter().any(|&x| x.abs() > 1.0), | ||
| "expected resample overshoot past full scale" | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
The desktop TX output stage (
audio/output.rs::prepare) folded the mandatory full-scale clamp into the gain step and guarded it out atgain == 1.0:The guard is inverted relative to need:
gain < 1.0→ the product(*s * gain)is already in range, so the clamp is a no-op.gain == 1.0(TX level 100%) → the clamp is the only thing taming post-resample overshoot — and that is exactly the branch that was skipped.Why it's a real, reachable bug
FFT resampling (rubato
FftFixedIn) of the full-scale GFSK waveform — which peaks at exactly ±1.0 (synth_gfsk) — interpolates a few samples slightly past full scale. Measured empirically with the realresample_buffer:The f32 output path hands samples to the driver verbatim (
f32::from_sampleis identity) with no saturation net — unlike the i16 cast, which saturates in Rust. So at unity gain those >1.0 samples reach the soundcard unclamped and overdrive the SSB rig's audio input past 0 dBFS → ALC overshoot / distortion / spurious emissions on the air.Reachability: desktop almost always resamples on TX (hardware rarely offers 12 kHz natively), f32 output devices are the common case (WASAPI/CoreAudio/PulseAudio), and
clamp_tx_gainallows exactly1.0. The defaultDEFAULT_TX_GAIN = 0.9clips (the product stays in range), which masks the bug — only a 100% TX-level setting exposes it.Fix
The clamp is safety-critical and must not depend on gain. Extracted
apply_gain_and_clip()and call it unconditionally. This mirrors the Android reference, which clamps unconditionally on the output cast (FT8TransmitSignal.float2Short/floatToInt16NoPad), kept deliberately separate from itsapplyVolumegain step. For in-range samples the result is byte-identical to the oldgain < 1.0path, so the common case is unchanged.Testing
4 pure
#[cfg(test)]cases added tooutput.rs:gain = 0.5)gain = 1.0The two unity-gain cases fail on the pre-fix guarded code and pass after — verified by reverting only the guard. Ran locally against the real
resample.rsvia a#[path]-include harness (zig-cc linker; the full Tauri crate can't link webkit2gtk/dbus in a headless sandbox). The#[cfg(test)]tests run in desktop CI undercargo llvm-cov.Risk
Minimal. One-line behavioral change (clamp now always runs); byte-identical output for every in-range sample at every gain, including the default 0.9. No DSP/decoder/interop impact — this only tightens the TX output to full scale, matching Android and the encoder's own ±1.0 range. No new dependencies.
Platforms
Desktop (Tauri) only. Android/iOS already clamp unconditionally.
🤖 Generated with Claude Code