[cw] drift-correct local CWX sidetone keyer#3202
Open
jensenpat wants to merge 1 commit into
Open
Conversation
…ethersdr#2980). Principle I. CwxLocalKeyer schedules each element by calling m_timer.start(durationMs) relative to "now" after every tick. On macOS the Cocoa run loop coalesces GUI-thread sources (panadapter paint, VITA-49 burst handling, status processing) and defers QTimer dispatch even with Qt::PreciseTimer set. Each deferral pushes the next edge later; the slip accumulates over the rest of the macro. At 20 WPM a dit is 60 ms, so 10–15 ms of jitter per tick rots the dit/dah cadence within seconds — audible as stutter and structurally wrong letter/word spacing. JS69135 confirmed via WebSDR that the radio's on-air signal sounds perfect: the air-side keyer (FlexLib's domain — Principle I, the radio is authoritative for protocol behavior) is fine, and the symptom is entirely AetherSDR's local sidetone. QSK-on exposes it because the operator's ear has the radio's fast-keyed monitor as a reference; QSK-off masks it because semi-break-in holds T/R continuously and small offsets become inaudible. Switch to drift-corrected absolute scheduling: track an QElapsedTimer from the start of a run, accumulate target edge times in m_nextEdgeMs, and set each m_timer.start() to the difference between target and elapsed. A late tick is followed by a correspondingly shorter wait, so the next edge lands on its absolute target instead of drifting later. When the run drains (both queue and elements empty) or stop() is called, m_elapsed is invalidated so the next start() begins a fresh epoch. Public API and signal shape unchanged — only private state added. Blast radius: risk_score=0.139, 1 high-risk affected (MainWindow::MainWindow constructs CwxLocalKeyer; recompile-only, no behavioral coupling — public surface and keyStateChanged signature are unchanged). Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
Reviewed the diff against main. The change is well-scoped to the two CwxLocalKeyer files and the implementation matches the description.
A few things I checked and was happy with:
- Reset paths are symmetric. Both
stop()and the final-drain branch inscheduleNext()invalidatem_elapsedand zerom_nextEdgeMs, so the next run starts from a fresh epoch. The partial-drain path (nextPendingalready queued) intentionally keeps the epoch alive — that's correct, since CWX text segments arriving back-to-back should stay drift-corrected against the same clock, and it also plays nicely with them_currentlyDownCharGap injection just above it. - Catch-up after a large stall. When a timeout is delivered very late, the next
waitclamps to 1 ms and elements fire back-to-back until the absolute timeline is caught up. That's the right behaviour for sidetone (the radio is the authority on what's on air) and matches the "long contest macro" scenario in the PR body. - Overflow.
m_nextEdgeMsisqint64,waitis clamped[1, m_nextEdgeMs]before theintcast — safe for any realistic CW run length. - No
AppSettings/ RAII / lifetime concerns for this change.
Nothing else from me — clean drift-correction patch and the inline comment at CwxLocalKeyer.cpp explaining why the absolute clock is in use is exactly the kind of context that's worth keeping in the source.
Thanks for the careful write-up of the manual-test methodology, and for noting the title-bar TX cancel-path follow-up rather than scope-creeping it into this PR.
73 and thanks @jensenpat 👍
🤖 aethersdr-agent · cost: $5.2470 · model: claude-opus-4-7
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.
Summary
This is a clean replacement PR for the CWX local sidetone timing fix from #3109, rebased onto current
upstream/main.QTimer::start(durationMs)scheduling inCwxLocalKeyerwith absolute, drift-corrected edge scheduling.QElapsedTimerepoch plus accumulatedm_nextEdgeMstarget so late GUI-thread timer dispatch is recovered on the following wait instead of accumulating.keyStateChanged(bool)signal shape unchanged.Root Cause
CWX local sidetone is driven from
CwxLocalKeyer, which runs on the GUI thread and schedules each Morse element withm_timer.start(durationMs)relative to the time the previous timeout handler finally ran.On macOS, panadapter/waterfall paint, VITA-49 burst handling, status processing, and general Cocoa run-loop coalescing can delay
QTimerdelivery even withQt::PreciseTimer. With relative scheduling, every delayed timeout pushes every later edge farther behind. Over a long CWX macro that slip becomes audible as local sidetone stutter and can stretch letter/word spacing.The radio's own CWX keyer remains authoritative for the on-air signal. This change only corrects AetherSDR's local sidetone cadence so it stays aligned with the intended CW timing under GUI load.
Implementation Notes
The scheduler now works like this:
m_nextEdgeMs.target - elapsed, clamped to at least 1 ms.stop()cancels it.Validation
Built and tested from current
upstream/mainplus this patch on macOS.Automated checks run:
cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_PREFIX_PATH=/opt/homebrew/opt/qt -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdkcmake --build build --parallel 22build/cw_sidetone_testpassedbuild/iambic_keyer_testpassedQT_QPA_PLATFORM=offscreen build/cwx_panel_testpassedctest --test-dir build --output-on-failure -j 22reached 21/22 passing;theme_manager_testis failing on current latest-main local preference/theme-import state and is unrelated to the two-fileCwxLocalKeyerchange.Manual radio test performed by @jensenpat:
Scharacters.The corrected build was also deployed to the remote macOS test machine at
patj@mac.jensencloud.net:/Users/patj/Desktop/AetherSDR.appand quarantine was cleared.Reviewer Test Plan
CWorCWL.Scharacters, or a long contest macro.Escwhile CWX is active; expected result: CWX sendscwx clearand local sidetone stops.Follow-Up Note
The title-bar
TXcancel path forces MOX/CW/PTT down but does not currently clear the CWX local sidetone queue.Escis the correct CWX abort path today. That is a separate UX/cancel-path follow-up from this timing drift fix.👨🏼💻 Generated with OpenAI Codex (GPT-5.5 Pro 4/23) and tested by @jensenpat