Skip to content

[cw] Add CwxLocalKeyer drift-correction regression test#3271

Merged
ten9876 merged 1 commit into
aethersdr:mainfrom
jensenpat:aether/issue-3219-cwx-drift-tests
May 29, 2026
Merged

[cw] Add CwxLocalKeyer drift-correction regression test#3271
ten9876 merged 1 commit into
aethersdr:mainfrom
jensenpat:aether/issue-3219-cwx-drift-tests

Conversation

@jensenpat

Copy link
Copy Markdown
Collaborator

Summary

  • Add a focused cwx_local_keyer_drift_test that drives CwxLocalKeyer::onTick() synchronously with a simulated elapsed clock.
  • Cover cumulative absolute edge targets, late-tick wait shortening, sustained-overload 1 ms catch-up clamping, queued macro continuity, and both stop() and natural-drain epoch reset paths.
  • Add a small protected seam around elapsed-clock reads and timer arming so production behavior stays the same while tests can avoid Qt event-loop timing races.

Why

Issue #3219 tracks the missing automated coverage for the drift-corrected scheduling introduced for the macOS CWX sidetone stutter fix. Before this change, the keyer's m_elapsed / m_nextEdgeMs behavior was manually verified but not directly asserted by unit tests, leaving subtle absolute-vs-relative scheduling regressions easy to miss.

Closes #3219.

Validation

cmake -S . -B build-issue-3219 -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk
cmake --build build-issue-3219 --target cwx_local_keyer_drift_test --parallel 22
ctest --test-dir build-issue-3219 -R cwx_local_keyer_drift_test --output-on-failure
git diff --check

Note: this was a focused unit-test build; no full AetherSDR.app binary was produced.

👨🏼‍💻 Generated with OpenAI Codex (GPT-5.5 Pro 4/23) and tested by @jensenpat

Co-authored-by: Codex <noreply@openai.com>
@jensenpat jensenpat changed the title Add CwxLocalKeyer drift-correction regression test [cw] Add CwxLocalKeyer drift-correction regression test May 29, 2026
@jensenpat jensenpat marked this pull request as ready for review May 29, 2026 18:16
@jensenpat jensenpat requested review from a team as code owners May 29, 2026 18:16

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filling in the #2980 drift-correction coverage gap, @jensenpat — this is exactly the kind of focused regression test the keyer needed.

Production refactor is a no-op. The four extracted helpers (resetElapsed, startElapsed, elapsedMs, armTimer) preserve the existing semantics:

  • elapsedMs()'s isValid ? elapsed() : 0 fallback is dead-but-harmless in production since scheduleNext() always calls startElapsed() before reading.
  • onTick() moving from private to protected doesn't affect the in-class QObject::connect(&m_timer, &QTimer::timeout, this, &CwxLocalKeyer::onTick) (access check happens at the connect site, not the slot).
  • No other production code subclasses CwxLocalKeyer, so adding the virtual to the two seams has no observable effect outside the test.

Test design is solid. Driving onTick() synchronously while a fake armTimer records waits and a controllable elapsedMs substitutes the clock is the right call — much better than trying to chase Qt event-loop timing. The six scenarios (exact cadence, late-tick slip, sustained overload clamp, queued macro continuity, stop() reset, natural drain reset) cover the absolute-vs-relative scheduling concern well, and asserting the cumulative nextEdgeMs targets (60/120/180/240/300, then 480/540 across the queued boundary) is the key thing — it's exactly what would regress if someone "simplified" back to relative durations.

Nothing blocking from me. Build wiring in CMakeLists.txt matches the pattern of the neighboring cw_sidetone_test target. Nice work.


🤖 aethersdr-agent · cost: $4.3797 · model: claude-opus-4-7

@ten9876 ten9876 merged commit e7b9d50 into aethersdr:main May 29, 2026
5 checks passed
ten9876 pushed a commit that referenced this pull request Jun 19, 2026
…3644)

## Summary
Fixes #3623 — the CWX local sidetone drops/clips individual CW elements
under panadapter/VITA-49 load on Windows.

## Root cause
`CwxLocalKeyer` scheduled every element edge with a `QTimer` parented to
`MainWindow`, so the timer ran on the GUI event loop and
`keyStateChanged` was delivered through a GUI-thread slot. Under
panadapter paint + VITA-49 burst handling the loop coalesces both, the
key gate lands a block or two late, and an element gets gapped/clipped.
#3202's drift-correction fixes *cumulative* slip (the message stays in
sync) but not this *per-edge* jitter, and reasons specifically about
macOS — this is the residual Windows case.

## Fix
Re-architect `CwxLocalKeyer` to mirror **`IambicKeyer`**, which already
abandoned `QTimer` for exactly this reason (*"QTimer's jitter is too
high for CW"*):

- Run the element schedule on a dedicated worker thread.
- Time each edge to an absolute `std::chrono::steady_clock` deadline
waited on an interruptible `condition_variable` — no QTimer, no event
loop.
- Drive the sidetone gate via a lock-free `onKeyDownChange` callback;
`CwSidetoneGenerator::setKeyDown` is `std::atomic`, the same gate path
the iambic keyer uses.

`encode()` and the drift-correction logic are unchanged, so message
timing and the live-mode CharGap behaviour (#2473/#2754) are preserved.
`MainWindow` now owns the keyer as a `unique_ptr` and stops it (joining
the worker) before `AudioEngine` is torn down, since the gate callback
touches `m_audio->cwSidetone()`.

This is the approach recommended in the issue triage.

## Testing
- **`cwx_local_keyer_drift_test` (#3271)** retargeted to the new clock
seam (worker-disabled synchronous subclass + callback) — **passes**; all
drift-correction assertions (absolute edge targets, slip-shortened
waits, sustained-overload clamping, queued-macro epoch continuity,
stop/drain resets) still hold, so the scheduling math is unchanged.
- Clean build (Windows MSVC, Qt 6.10.3).
- Ran the app against a synthetic FlexRadio source under a heavy
spectrum load and sent long CWX macros: **CW sends cleanly at both 20
and 40 wpm — the element drops/stutter are gone**, including at 40 wpm
where the tighter element timing is the more demanding case. Clean
startup / connect / shutdown (worker joins without hanging).

## Known residual
Verified clean at **both 20 and 40 wpm** under heavy synthetic load. A
single faint, low-level click heard at 20 wpm did not recur and isn't
speed-correlated (40 wpm — tighter timing — is completely clean),
consistent with an incidental audio under-run under the deliberately
brutal synthetic load rather than the keyer. `CwSidetoneGenerator` is
unchanged by this PR and its raised-cosine envelope ramps cleanly across
block-quantized edges. The "fullest fix" floated in the issue (advancing
the gate by sample count inside `CwSidetoneGenerator::process()`)
remains available as a follow-up if sub-block edge precision is ever
wanted.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
G6PWY-Chris pushed a commit to G6PWY-Chris/AetherSDR that referenced this pull request Jun 22, 2026
## Summary

- Add a focused `cwx_local_keyer_drift_test` that drives
`CwxLocalKeyer::onTick()` synchronously with a simulated elapsed clock.
- Cover cumulative absolute edge targets, late-tick wait shortening,
sustained-overload 1 ms catch-up clamping, queued macro continuity, and
both `stop()` and natural-drain epoch reset paths.
- Add a small protected seam around elapsed-clock reads and timer arming
so production behavior stays the same while tests can avoid Qt
event-loop timing races.

## Why

Issue aethersdr#3219 tracks the missing automated coverage for the
drift-corrected scheduling introduced for the macOS CWX sidetone stutter
fix. Before this change, the keyer's `m_elapsed` / `m_nextEdgeMs`
behavior was manually verified but not directly asserted by unit tests,
leaving subtle absolute-vs-relative scheduling regressions easy to miss.

Closes aethersdr#3219.

## Validation

```bash
cmake -S . -B build-issue-3219 -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk
cmake --build build-issue-3219 --target cwx_local_keyer_drift_test --parallel 22
ctest --test-dir build-issue-3219 -R cwx_local_keyer_drift_test --output-on-failure
git diff --check
```

Note: this was a focused unit-test build; no full `AetherSDR.app` binary
was produced.

👨🏼‍💻 Generated with OpenAI Codex (GPT-5.5 Pro 4/23) and tested by
@jensenpat

Co-authored-by: Codex <noreply@openai.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit test for CwxLocalKeyer drift-correction logic

2 participants