Repository navigation
test(hl2): pin the TXA transmit passband and onset. Principle VIII. - #6347
Conversation
Refs aethersdr#5911. hl2_txdsp_test asserts sideband handedness and suppression ratios, never the wanted bin's amplitude, the band edges or the onset, so most changes to what operators transmit are invisible to it. hl2_tx_passband_onset_test drives Hl2TxDsp (ALC off, real-time paced) and reads the IQ it emits: - passband: a 24-tone multitone, 100..3500 Hz on a 50 Hz grid, read on whole 960-sample periods so every bin is orthogonal; each wanted bin against the 1 kHz one, for USB, LSB (300..2700) and DIGU (150..3000) as defaultTxPassbandForModeName() gives them. The edges are written out in the test, so a narrower shipped passband fails. - onset: a 1 kHz tone as the over's first audio (key-up), and after a 200 ms burst and 300 ms of silence (mid-over). Measured at 4e54734 (after aethersdr#6245; every figure as at 25de97c), on the IQ stream's sample clock: flat within 0.009 dB from edge+100 to edge-100 Hz, -6.02 dB at each edge, -0.64 dB 50 Hz inside, -22.97 dB 50 Hz outside, -59.38 dB 100 Hz outside, 142 dB and more beyond. Key-up: 50 % point 95.54 ms after the first audio, 10-90 % rise 14.77 ms. Mid-over: 50 % point 72.92 ms, rise 0.73 ms, -60 dB reached 8.84 ms before the 50 % point. The key-up shape is WdspChannel's mute envelope, not the bandpass: WDSP holds the first 10 ms at zero and then slews over 25 ms (raised cosine) from the over's first non-zero input sample. The rise matches that ramp to 0.01 ms. Two probes at 25de97c, not re-taken: it is unchanged with TXA's bp0 at 8192 taps or the TXA ALC off; only the 50 % point moves (+64.0 ms, -6.0 ms). Nothing in the chain changes: the tap count and the default passband are a ruling (aethersdr#5911 ask 1). Separate target, about 7 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed against the test's own header comment and issue #5911 — the PR body was not available to me (the harness handed me title, file list and diff only), so if it discloses local run results or tolerance provenance, some questions below may already be answered there.
1. Issue fit
Partially — and the unaddressed half is a maintainer question, not a code gap.
#5911 makes two asks. Ask 2 is delivered well: "a transient and multi-tone leg in the TX test, so the next change of this class is visible before a release rather than after one." This is exactly that — a 24-tone stimulus through the real Hl2TxDsp chain with wanted-bin amplitude assertions (the thing #5911 identified as structurally missing: hl2_txdsp_test asserts only opposite-sideband ratios, so "a tone being filtered away entirely fails nothing"), plus edge probes at 300/2700/150/3000 and an onset/rise/pre-ring leg. I checked the anti-tautology claim in the header comment and it holds: cfg takes the passband from defaultTxPassbandForModeName(), but checkShape() is handed literal 300, 2700 / 150, 3000 (tests/hl2_tx_passband_onset_test.cpp:323-328), so a shipped mapping that narrowed to e.g. 400..2600 would put the 300 Hz and 2700 Hz edge tones ~50 dB down and fail edgesOk. It is not reading the implementation back to itself.
Ask 1 — "is the 300–2700 box the intended shipped voice bandwidth for HL2 SSB?" — is pre-empted, not answered. That is reasonable for a test PR, but worth naming for the maintainer: this test now pins the status quo in both directions (−6 ±1 dB at the edge, ≤ −50 dB at edge+100), so a later ruling to widen the voice default will require editing main()'s literals. The issue carries maintainer-review and that question is still open.
#5911's third, smaller item — Hl2Backend::setTxFilter's comment still justifying eSSB by "the 255-tap Blackman prototype keeps a usable skirt", describing a non-default modulator — is untouched. Out of scope for a test-only PR; it stays open.
2. Scope
| File | What it changes | Claimed by the title? | Verdict |
|---|---|---|---|
tests/hl2_tx_passband_onset_test.cpp |
New socket-free CTest: 24-tone passband response per mode (USB/LSB/DIGU) + key-up and mid-over onset measurement | Yes — "pin the TXA transmit passband and onset" | In scope |
tests/tests.cmake:5927-5934 |
Registers the new target, SKIP_RETURN_CODE 77 |
Yes | In scope |
Everything in the diff is explained by the issue. No production code, no new public surface, no capability, no settings key, no CHANGELOG.md entry (correct). No deleted guards — this is a pure addition. The added target is genuinely registered (add_executable + add_test), not a dead addition. tests/hl2_twotone_probe.cpp — the orphan instrument #5911 names as the source of the stimulus generator — is already gone from both trees, so there is nothing left to retire here.
Socket-test surface: none. I read the added test source directly (not the working tree): no QTcpServer/QTcpSocket/QUdpSocket/QLocalServer/QWebSocketServer, no bind/listen/connectToHost, no peer process, no Fake* radio class. The only Qt plumbing is QCoreApplication::processEvents and a signal/slot connection to an in-process Hl2TxDsp. It is socket-free, which is the right layer: AGENTS.md routes DSP to a socket-free CTest.
3. Blockers
None.
4. Nits (non-blocking)
(a) tests/hl2_tx_passband_onset_test.cpp:90,101-116,336-343 — a real defect can exit 77 (SKIP) instead of 1. This is the one I would most like fixed before merge.
Three paths converge on it:
-
runOnce()reads only the aggregate counter:c.faults = tx.modulatorFaultBlocks();
But
src/core/backends/hl2/Hl2TxDsp.h:103-107draws exactly the distinction this collapses: "The part ofmodulatorFaultBlocks()dropped unexchanged because the worker had not produced the previous block's output within a block period… A drop means the worker was starved; any other fault means the exchange gate failed." An exchange-gate failure is the #5910 class — a defect, not load. The sibling test reads both (tests/hl2_txdsp_capture_burst_test.cpp:103-105) andtests/tests.cmake:5923states the convention outright: "An underrun still fails." -
The exit-code order is inverted relative to that sibling.
hl2_txdsp_capture_burst_test.cpp:349-356checks failures first:if (g_failures) { … return 1; } if (g_inconclusive) { … return 77; }
The new test does the opposite (
:336-343):g_starved→ 77 is tested beforeg_failures→ 1. So a run where three passband checks genuinely failed and one onset capture also starved reports SKIP. -
The
configure()failure path takes the same exit.:79-84does++g_failuresand thenc.faults = ~0ull, whichrun()reads as starvation, retries three times, and converts tog_starved→ 77. A hard modulator configure failure is reported as inconclusive.
Drop-in for (1) and (3) — add the counter to Capture, read it, and fail hard on the non-drop remainder:
struct Capture {
std::vector<std::complex<float>> iq;
unsigned long long faults = 0;
unsigned long long stallDrops = 0;
}; c.faults = tx.modulatorFaultBlocks();
c.stallDrops = tx.modulatorStallDrops();
return c;Capture run(const char* what, const std::vector<float>& audio, WdspChannel::Mode mode,
std::pair<int, int> passband)
{
Capture c;
for (int attempt = 0; attempt < 3; ++attempt) {
c = runOnce(audio, mode, passband);
if (c.faults == 0) {
return c;
}
if (c.faults > c.stallDrops) {
// Not load: the exchange gate failed, which is the defect #5910 names.
check(false, (std::string(what) + ": every block reached the wire").c_str());
return c;
}
std::fprintf(stderr, "starved in %s: %llu blocks missed the wire (attempt %d of 3)\n",
what, c.faults, attempt + 1);
}
g_starved = true;
return c;
}and for (2), swap the two blocks in main() so g_failures is tested first, matching the sibling.
(b) tests/hl2_tx_passband_onset_test.cpp:148-157 — DIGU's low edge has no stopband probe, and nothing notices. checkShape matches f == lo - 100 and f <= lo - 150. For DIGU lo = 150, that is 50 Hz and ≤ 0 Hz, and kTones starts at 100 — so for the DIGU row, "100 Hz outside an edge is 50 dB down" is evaluated against the 3100 Hz tone alone, and the low-side 150+ Hz check never fires. The edges == 2 guard (:164) catches this class for the edge tones but there is no counterpart for the stopband points, so the gap is silent. Either add 50 to kTones (still a multiple of 50, so it stays orthogonal in the 960-sample window) or count the stop100 hits and assert the count the way edges is asserted.
(c) tests/hl2_tx_passband_onset_test.cpp:139,152-153 — kFlatDb = 0.1 is applied at edge ∓ 100 Hz, where I can't find supporting data. WdspChannel.h's own figure, quoted in #5911, is a "200 Hz [transition] at 2048 taps", and #5911's measured flatness figure is "passband ripple, 600–2400 Hz: 0.00 dB". The tones this test requires flat within 0.1 dB include 400, 2500 and 2600 (USB/LSB) and 250, 2650, 2700, 2750, 2800, 2900 (DIGU) — several of them inside one transition width of an edge, i.e. on the skirt knee rather than the flat top. If those were measured at 0.1 dB in your runs, saying so in the body would settle it; if the figures came from the 600–2400 region, this is the tolerance most likely to go red on correct code.
(d) tests/hl2_tx_passband_onset_test.cpp:68-75 — declaration order of tx and c. Hl2TxDsp tx; is declared before Capture c;, and the iqReady lambda captures c by reference, so reverse-order destruction tears down c while tx is still alive. I checked and there is no live path today: both emit sites (Hl2TxDsp.cpp:333,658) are synchronous inside processAudioBlock, which this test calls on the main thread, and ~Hl2TxDsp is = default. It is one line of hygiene against a future teardown-flush, not a bug — move Capture c; above Hl2TxDsp tx;.
(e) Coverage reality, worth one line in the body. PR CI never runs this test: every ctest in ci.yml is -R-filtered to a named handful (lines 819-820, 832, 1174, 1189) and this target is in none of them. The five green checks prove it compiles on all three lanes — which is real value, and the file defining its own kPi rather than M_PI is why check-windows is green. First actual execution is post-merge in full-suite.yml. On the sanitizer lanes the real-time pacing will almost certainly starve the worker and exit 77 every run, so it guards nothing there (the same trade the burst test makes by design). On the normal lane, a load spike during the full -j suite also lands on 77 — which is the reason nit (a) matters more than it looks. Was it run concurrently with the rest of the suite, rather than alone?
5. What I tried to break
- CodeGuard's two CRITICALs — both refuted.
CS-VLN-CMDI-BACKTICKat:50and:66are backtick-quoted parameter names inside//comments (`ms`,`audio`). The file contains nosystem/popen/exec*/QProcessand no shell string anywhere, and everyfprintfuses a literal format string withwhat/err.c_str()as arguments, so there is no format-string sink either. Credit to security-audit (CodeGuard) for the sweep. Heads-up::225and:227carry the identical pattern (`startIn`,`warm`) and will refire on a rerun. - Tried to make the test pass against the unfixed behaviour. It can't:
checkShape's edges are literals, not read back fromdefaultTxPassbandForModeName(), and the wanted-bin magnitudes are compared to the 1 kHz reference rather than to a suppression floor — which is precisely the blindness #5911 documents inhl2_txdsp_test. - Index bounds in every raw read.
bin()reads[kSettle, kSettle+count)withcount = ((size-kSettle)/kPeriod)*kPeriod, guarded by thesize < kSettle + 8*kPeriodearly return;onset()guardssize < settledFrom + 4*kPeriod;envDb's index is bounded above byhalfAtand below by the explicitat < t0test.halfAtis always in range becausesteadyis the mean of|iq[n]|over a non-empty tail, so a sample at ≥ half the mean necessarily exists. No out-of-range read found. - DFT orthogonality and capture-length margins. All 24 tones are multiples of 50 Hz and the window is whole 960-sample periods at 48 kHz, so the bins and their images are orthogonal as claimed. Passband capture: 0.75 s × 24 kHz → ~36000 output samples vs the 19968 the guard needs. Warm onset: 0.9 s → 43200 vs 16032 needed. Both have slack.
- The mid-over premise. The claim that "WDSP starts [the mute envelope] at the over's first non-zero input sample" and does not re-arm across 300 ms of silence held up:
WdspChannel::Config'smuteDelayUpSec = 0.010/muteSlewUpSec = 0.025(src/core/dsp/WdspChannel.h:130-131, un-overridden byHl2TxDsp) are open-time config driven by the running state, and I found no input-silence detector that would re-arm them mid-over. That also reconcileskMinKeyUpRiseMs/kMaxKeyUpRiseMs: the 10–90 % portion of a 25 ms raised cosine is ≈ 15 ms, comfortably inside [10, 20]. AndkMaxKeyUpHalfMs = 106sits about half a 1024-sample block above #5911's measured +96 ms, as the comment claims. - "mic gain unity" (
:66-67) — confirmed:Hl2TxDsp.h:172defaultsm_micGain = 1.0and the test never callssetMicGain(). - Couldn't check: the shipped figures themselves — 0.1 dB in-band, −6 ±1 dB at the edges, 50 dB at edge±100, 100 dB beyond. I'm headless with no build and no run, so every finding above is reasoned from code rather than reproduced; nits (a) and (b) are source-level and don't depend on a run, nit (c) explicitly does.
6. Recommendation
Approve with nits. This is careful, well-motivated work that closes the real gap #5911 identified, and the anti-tautology design (literal edges, amplitude assertions, retry-then-inconclusive) shows the author thought about how the test could lie. No blockers: nothing here breaks users or violates canon.
Concrete next step: fix (a) — split stall drops from gate faults and put g_failures ahead of g_starved in main(), matching hl2_txdsp_capture_burst_test's precedent — since a test whose failure mode is SKIP is the one way this instrument could quietly stop guarding. Then (b), a one-tone addition. (c) is a question rather than a change: confirm the edge∓100 Hz flatness figures came from a real run, because that tolerance is where a correct build is most likely to go red once this starts executing in full-suite.yml. Thanks for picking this up — the sideband-ratio blindness in the old test was a genuinely non-obvious find and this is the right shape of fix.
🤖 aethersdr-agent · cost: $9.3513 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (2 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $9.7111 · model: claude-opus-5 |
Review of aethersdr#6347. (a) A defect exits 1, not 77. Capture reads modulatorStallDrops() beside modulatorFaultBlocks(): only a stall drop is retried and can end inconclusive; any other fault is a failed check at once. A configure() failure is a failed check and is not retried. main() tests g_failures before g_starved, as hl2_txdsp_capture_burst_test does. (b) DIGU's low edge has a stopband probe: kTones gains 50 Hz, and each row asserts that both edges are probed 100 Hz outside, as it does for the edge tones. 50 Hz reads -59.38 dB under DIGU 150..3000. The 25-tone sum peaks at 0.164 of full scale (0.163 with 24). No bound is changed. (d) Capture is declared before Hl2TxDsp, so the iqReady slot's target outlives the emitter. Refs aethersdr#5911 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Fixed in (a) A defect now exits 1, not 77. All three paths, as you described them:
Forced on the old head and on the new one: (b) (c) The 0.1 dB figure is measured at the tones you named, not taken from the 600–2400 Hz region. In dB relative to 1 kHz, five runs with byte-identical output:
The worst flat-band tone is 0.009 dB down, in every mode: the one 100 Hz inside an edge. 50 Hz inside an edge reads −0.64 dB and is not in the flat check. (d) (e) Yes, it ran inside the full Not done: #5911's ask 1 and the 73, ON8ST |
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed 9c715b470911696fe586f4443a9685b751d67c01.
Issue fit: matches #5911's remaining ask. The withdrawn measurement table stays withdrawn; this test is the re-take, on the TXA chain, with the edges written as literals. It does not decide whether 300..2700 is the intended voice passband (ask 1). Refs #5911 is the right link. No production code changes, so the RadioModel seam is untouched.
Verdict: Approve with nits. No blockers.
| Path | What changed |
|---|---|
tests/hl2_tx_passband_onset_test.cpp |
Socket-free TXA passband and onset measurement. Configures Hl2TxDsp from defaultTxPassbandForModeName(), asserts literal edges. |
tests/tests.cmake |
Registers the target next to the other HL2 TX DSP tests. Exit 77 is the phasing build or a capture that stayed starved. |
The second commit addresses the prior review: a non-stall fault fails instead of skipping, g_failures is tested before g_starved, DIGU's low edge has a 50 Hz stopband probe (stop100 == 2), and Capture is declared before Hl2TxDsp.
Verified on this SHA, TXA, Qt 6.12, QT_QPA_PLATFORM=offscreen. One run, exit 0. USB and LSB: worst in-band 0.009 dB, 100 Hz outside −59.38 dB, 150 Hz and beyond −142 dB. DIGU: 50 Hz and 3100 Hz at −59.38 dB, edges −6.02 dB, 250..2900 Hz within 0.009 dB. Key-up 50% at 95.54 ms, 10–90% rise 14.77 ms. Mid-over 50% at 72.92 ms, −60 dB lead 8.84 ms. Repeated legs matched. Narrowing defaultTxPassbandForModeName() to 300..2400 in a scratch tree made USB and LSB flatness and edge checks fail (exit 1); DIGU stayed green. Restored, rebuilt, exit 0 again.
CI on this SHA: build, check-windows, check-macos, Static checks, and Sanitizer option configures are green. Those jobs compile this target. They do not run it; ci.yml's per-PR gate is frozen, and the body says so. First upstream execution is full-suite.yml.
Nits (not merge-blocking): the non-stall failure string is the opposite of the condition it guards, and envDb indexes the capture with no upper bound when the 50% point is never found.
Non-blocking follow-up: the phasing modulator still exits 77, so #5911's phasing column is not re-taken here. A sanitizer or heavily loaded full-suite run can exit 77 when the worker is starved; that run is inconclusive, not a pass. DIGU has no deep-stop tone below 0 Hz; the low edge is covered at 50 Hz (−59.4 dB) and the deep stop is the 3500 Hz tone.
Refs #6347 Answers the two non-blocking notes left on the approving review of #6347. `tests/hl2_tx_passband_onset_test.cpp` only. ## Limits - Test only. No product code, no tolerance and no bound changes; the measured figures are the same before and after. - Neither path occurs on a healthy run. Both were reached by scratch variants of the test that are not in this PR. - AddressSanitizer instrumented the test's own translation unit only, not the library it links. The out-of-range read it reports is a BUS on a wild address, not a redzone hit. - Built and run on one Mac (macOS, Qt 6.12.0), TXA build. ## Change 1. `run()`: when the modulator reports a fault that is not a stall drop, the check failed with ": no fault but a stall drop". The FAIL line now states the expectation that broke: "every modulator fault is a stall drop", as the comment above `run()` describes it. 2. `onset()`: `firstAbove()` returns 1e9 when a level is never reached. `halfAt` is then far past the capture, and `envDb` indexed `c.iq` out of range before `checkKeyUp` or `checkMidOver` could fail. `envDb` now returns the -400 dB floor for any index outside `[t0, c.iq.size())`. 3. `onset()`, from this PR's own review: when the 50 % point is never reached, `leadMs` is the 1e9 sentinel too, not `1e9 - 1e9 = 0`, and the short-capture return sets it as well. So the mid-over lead check fails with the others, where it passed for the wrong reason. ## Evidence Four scratch variants, the test file compiled with `-fsanitize=address` and linked against the tree's library at `5a8a6eb3`: | variant | result | |---|---| | before this PR, 50 % level made unreachable | `AddressSanitizer: BUS on unknown address`, in `envDb` called from `onset()`; no check ran | | this PR, 50 % level made unreachable | no sanitizer report; envelope printed as -400.0 dB; 6 checks failed cleanly, exit 1 | | this PR, one non-stall fault forced | `FAIL: USB: every modulator fault is a stall drop` (and per leg), exit 1 | | this PR, unmodified | all checks passed, no sanitizer report; key-up 50 % at 95.54 ms, rise 14.77 ms; mid-over 50 % at 72.92 ms | Full ctest at `e726c404`, `-j8`: 755 of 758 passed, 3 failed, all three also on the serial rerun. Besides the two below, `vkamp_connection_test` failed with only one of its two known checks (#5254). - `peripheral_auth_dialog_interaction_test`: fails on clean main on this Mac (#6008); this diff touches no dialog file. - `unified_title_bar_test`: fails on clean main on this Mac (#6224); only its two known checks failed; this diff touches no title-bar file. `hl2_tx_passband_onset_test` passed in 6.69 s. `check_test_registration.py --strict`, `check_wdsp_test_links.py --strict` and `gen_touchpoint_manifest.py --check` pass. 73, ON8ST 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Refs #5911
Correction to #5911 first. The issue put its onset figures (a 14 ms rise, about 12 ms of pre-ringing) down to the 2048-tap bandpass. That is wrong. At current main the key-up onset comes from
WdspChannel's mute envelope. WDSP holds the first 10 ms of an over's first non-zero audio at zero, then fades it in over 25 ms (raised cosine,muteDelayUpSec/muteSlewUpSec). This test measures a 14.77 ms 10-90 % rise; that ramp predicts 14.76 ms. The shape does not change with TXA'sbp0at 8192 taps or with the TXA ALC switched off. Only the delay moves. The bandpass's own onset, measured once the envelope has run, is a 0.73 ms rise.Limits
Hl2TxDspemits, and nothing else. No radio, no simulator, no socket, no keying.-j8load) gave the same figures to 0.01.What it checks
hl2_tx_passband_onset_testis a separate target and takes about 7 s;hl2_txdsp_testalready takes about 180 s.defaultTxPassbandForModeName(). The expected edges are written out in the test, so a narrower shipped passband fails. The checks: flat to 0.1 dB from edge+100 to edge-100 Hz, -6 ± 1 dB at each edge, at least 50 dB down 100 Hz outside, at least 100 dB down from 150 Hz outside.Measured at
4e547347Every figure below is the same as at
25de97c3, before #6245 moved the HL2 ALC intoHl2TxAlc.h. The test runs with that ALC off.Passband (the same for USB and LSB; DIGU has the same shape at its own edges):
The -59.38 dB at 200 and 2800 Hz matches the figure the withdrawn table in #5911 gave for TXA.
Onset:
The two columns differ by 22.6 ms. The mute envelope accounts for that: 10 ms held at zero plus half of the 25 ms slew. Two probes, never pushed, taken at
25de97c3and not re-taken since, account for part of the 72.92 ms. With the TXA ALC off, the 50 % point comes 6.0 ms earlier (its 6-τ look-ahead at 1 ms). Withbp0at 8192 taps it comes 64.0 ms later, which is (8192 - 2048) / 2 samples at 48 kHz, and the mid-over -60 dB lead grows from 8.84 to 25.04 ms. #5911's "+96 ms" observation therefore matches the key-up 50 % point here (95.54 ms).Red on mutation (scratch commits on
25de97c3, never pushed, not re-taken; the full suite each time)hl2_txdsp_testdefaultTxPassbandForModeNameHl2TxDsponly (mapping unchanged)bp0at 8192 tapshl2_dsp_readback_testalso fails on the second mutation, because it reads back the edges the channel was given. A narrowing inside WDSP itself would get past it. At9c715b47, 755 of 757 tests pass on this Mac, and this test ran inside that-j8suite (6.79 s, not skipped). PR CI only compiles this test: it is in noctest -Rlist inci.yml, so its first run upstream isfull-suite.ymlafter merge. The two that fail are known failures here, and this commit touches neither:peripheral_auth_dialog_interaction_test(the #6008 clipped-page check, in the per-case test #6156 introduced) andunified_title_bar_test(its two #6224 checks).What this does not change
The chain stays as it is. The tap count, the 300..2700 default and the mute envelope are untouched. Whether 300..2700 is the intended shipped voice passband is still #5911's ask 1, and it is yours to decide. If the tap count comes into it: TX's 2048 taps are
create_txa's ownmax(2048, dsp_size).WdspChannel::Config::filterTapsreaches receive channels only (the "Receive only" clause inWdspChannel.cpp), so setting it on the TX config would change nothing. If you would rather the 10 ms hold at the start of every over were shorter, that is a separate decision: it drops the first 10 ms of the first syllable, and the test pins only the ramp's 10..20 ms rise, not the hold.73, ON8ST
🤖 Generated with Claude Code