Skip to content

fix(cwx): insert CharGap between live-mode Pending entries in CwxLocalKeyer#2754

Merged
ten9876 merged 1 commit into
aethersdr:mainfrom
NF0T:fix/cwx-live-sidetone-char-gap
May 16, 2026
Merged

fix(cwx): insert CharGap between live-mode Pending entries in CwxLocalKeyer#2754
ten9876 merged 1 commit into
aethersdr:mainfrom
NF0T:fix/cwx-live-sidetone-char-gap

Conversation

@NF0T

@NF0T NF0T commented May 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes garbled local sidetone when typing characters quickly in CWX live mode. Closes #2473.

Confirmed via second receiver: over-the-air CW is correct. The radio's CWX buffer queues and times characters properly. The bug is client-side only — the local sidetone keyer was merging adjacent characters into undecodable noise.

Root cause

In live mode, each keystroke calls CwxModel::sendChar()CwxLocalKeyer::start(ch, wpm), enqueuing a separate Pending entry per character. When scheduleNext() exhausts the current character's elements and dequeues the next Pending, it calls encode() with firstChar = true — so no CharGap is prepended for the transition.

The last element of any encoded character is always Dit or Dah, leaving m_currentlyDown = true at the boundary. The guard if (keyDownNext != m_currentlyDown) never fires, so the key stays continuously down and adjacent characters merge. Example — K7:

Element Duration
K final Dah 3 units
CharGapmissing 3 units
7 first Dah 3 units (merged — 6u continuous key-down)

Typing slowly is unaffected because the keyer fully drains between characters (m_running = false at the next start() call, no boundary transition occurs).

Fix

In scheduleNext(), before dequeuing the next Pending entry, enqueue a CharGap if m_currentlyDown is true:

if (m_currentlyDown) {
    m_elements.enqueue(Element::CharGap);
}

This keys the tone down, waits the standard 3-unit inter-character silence, then begins the next character's elements normally.

Buffer mode is unaffected: a full-string transmission is a single Pending entry — the queue boundary is never crossed and encode() handles all internal CharGaps via its own firstChar logic.

Test plan

  • Live mode — type a callsign quickly: sidetone should be clean, correctly spaced Morse
  • Live mode — type slowly (one char at a time, spaced apart): still correct, no spurious leading gap on first character
  • Buffer mode (Enter / Send): no change in behavior
  • ESC / clearBuffer during live typing: stop fires immediately, no leftover gap elements

🤖 Generated with Claude Code

…lKeyer

In live mode each keystroke calls CwxModel::sendChar() which enqueues a
separate Pending entry via CwxLocalKeyer::start(). encode() always begins
with firstChar=true, so no CharGap is prepended for the transition between
entries. The last element of any encoded character is always Dit or Dah,
leaving m_currentlyDown=true at the boundary; without an explicit gap the
key stays continuously down and adjacent characters merge into
undecodable noise in the local sidetone (aethersdr#2473).

Fix: in scheduleNext(), when m_elements is exhausted and the next Pending
entry is about to be dequeued, enqueue a CharGap first if m_currentlyDown
is true. This keys the tone down, observes the standard 3-unit
inter-character silence, then begins the next character's elements.

No effect on buffer mode: a full-string transmission is a single Pending
entry, so the queue boundary is never crossed and encode() handles all
internal CharGaps via its own firstChar logic.

Closes aethersdr#2473.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@NF0T NF0T requested review from jensenpat and ten9876 as code owners May 16, 2026 20: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.

Nice fix — the diagnosis is spot-on and the code change is exactly the minimal correction needed. Thanks @NF0T.

A few things I traced and confirmed:

Ordering is correct. The CharGap is enqueued before encode() appends, so when execution falls through to the m_elements.dequeue() below, the gap plays first (3u of silence with keyStateChanged(false)), then the new character's first Dit/Dah fires keyStateChanged(true) cleanly. The keyDownNext != m_currentlyDown guard correctly fires on both transitions.

No double-gap risk. encode() always starts with firstChar=true and emits no leading CharGap, so the externally-inserted one isn't duplicated.

Slow-typing path unchanged. When the keyer fully drains before the next start(), the no-pending-no-elements branch at the top of scheduleNext() resets m_currentlyDown=false, so the new check is correctly skipped on the next entry.

Space-as-its-own-Pending still totals 7u. Traced K, , 7 typed as three separate live entries: K drains → boundary inserts CharGap (3u) → encode(" ") appends WordGap (4u) → 7 arrives with m_currentlyDown=false so no extra gap → 7 starts. Total inter-character = 3u + 4u = 7u, matches standard CW word spacing. Buffer mode unaffected as you noted.

stop() cleanup is fine — clears m_elements (so any pending CharGap is dropped) and resets m_currentlyDown, so ESC/clearBuffer mid-typing won't leave a stale gap.

Minor observation, not blocking: if a Pending entry contains only unknown characters, encode() adds nothing, and the externally-inserted CharGap will play as a lone 3u silence. Edge case, inaudible, not worth fixing on this PR.

LGTM modulo manual testing of the four cases in your test plan.

@ten9876 ten9876 merged commit 9b2ddd9 into aethersdr:main May 16, 2026
5 checks passed
@ten9876

ten9876 commented May 16, 2026

Copy link
Copy Markdown
Collaborator

Claude here — landed at 22:36 UTC. Fourth contribution today, Ryan! 🎉

The diagnostic correction is the part that elevates this PR — second-receiver verification that over-the-air CW was already correct, and that the issue lived entirely client-side in CwxLocalKeyer, would have saved hours of barking up the wrong tree (the radio's command stream). The Pending-boundary analysis with the K→7 worked example is exactly the kind of writeup that makes future maintenance trivial.

Surgical 9-line fix at the actual root cause — if (m_currentlyDown) enqueue(CharGap) before the Pending dequeue. Buffer mode untouched because that path never crosses the Pending boundary. Clean separation.

Four PRs today (#2745, #2747, #2752, #2754), four different code paths (RADE UI → RADE model → CWX UI → CWX core), four clean diffs. Genuinely outstanding day of contributions.

73, Jeremy KK7GWY & Claude (AI dev partner)

@NF0T NF0T deleted the fix/cwx-live-sidetone-char-gap branch May 17, 2026 00:07
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>
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.

CWX Live mode force-sends new characters before previous character finishes

2 participants