Skip to content

Add CW keyboard USB HID and MIDI keying controls#2361

Closed
jensenpat wants to merge 3 commits intoten9876:mainfrom
jensenpat:aether/cw-keyboard-shortcuts
Closed

Add CW keyboard USB HID and MIDI keying controls#2361
jensenpat wants to merge 3 commits intoten9876:mainfrom
jensenpat:aether/cw-keyboard-shortcuts

Conversation

@jensenpat
Copy link
Copy Markdown
Collaborator

Summary

This PR rolls up the CW/netCW work that started from PR #2336 and adds operator-facing keyboard and MIDI controls for manual CW keying.

It includes:

  • netCW timing/trace logging from the PR Fix and trace netCW keying for straight key and iambic CW #2336 baseline so CW keying can be reviewed from aether.cw logs.
  • Keyboard shortcut actions for Trigger straight key, Trigger CW Left Paddle, and Trigger CW Right Paddle.
  • Matching MIDI mapping actions using the same names and shared action IDs: cwkey, cwdit, and cwdah.
  • Persistence fixes for the new CW keyboard shortcut actions using XML-safe shortcut IDs.
  • MIDI settings migration/normalization for legacy dotted CW IDs (cw.key, cw.dit, cw.dah) so older test bindings still load and save back as cwkey, cwdit, and cwdah.
  • A clickable red TX status badge that acts as an emergency TX cancel control.
  • Focused regression coverage for shortcut and MIDI settings persistence.

CW Keying Behavior

Trigger straight key

Trigger straight key is implemented as a true momentary control. Keyboard press and MIDI gate-on assert the netCW key path; key release and MIDI gate-off release it.

On press, AetherSDR sends:

  • cw ptt 1
  • cw key 1

On release, AetherSDR sends:

  • cw key 0
  • cw ptt 0

This preserves the required ordering for break-in-off setups where the radio needs explicit netCW PTT around key state.

Trigger CW Left Paddle / Right Paddle

The left and right paddle controls feed the local iambic keyer when it is running. That keeps sidetone and on-air timing aligned with the configured CW speed, and it emits netCW key edges for generated dits/dahs.

If the local keyer is not running, the paddle path falls back to a direct held-key path so the radio still receives key-down/key-up state.

MIDI Changes

The MIDI controls now reuse the same CW action IDs and display names as keyboard shortcuts:

  • cwkey / Trigger straight key
  • cwdit / Trigger CW Left Paddle
  • cwdah / Trigger CW Right Paddle

MidiSettings normalizes legacy dotted CW param IDs on read and write:

  • cw.key -> cwkey
  • cw.dit -> cwdit
  • cw.dah -> cwdah

This keeps older mappings from being stranded while avoiding dots in saved CW action IDs going forward.

Keyboard Shortcut Persistence

The original CW shortcut IDs used dots, which produced settings keys like Shortcut_cw.key. AppSettings intentionally skips invalid XML element names, so those shortcuts were not persisted.

This PR avoids changing the XML/settings handler and instead renames the CW shortcut IDs to XML-safe names:

  • Shortcut_cwkey
  • Shortcut_cwdit
  • Shortcut_cwdah

A new shortcut_manager_test verifies that the CW shortcuts save and reload from the database, and that no dotted CW shortcut keys are written.

TX Cancel Badge

The large red status-bar TX indicator is now clickable. Left-clicking it sends a conservative emergency cancel sequence intended for a stuck transmit condition:

  • clears Space PTT state
  • clears local CW straight-key and paddle state
  • resets local iambic paddle memory
  • forces sidetone key-up
  • sends netCW cw key 0
  • sends netCW cw ptt 0
  • sends transmit tune 0
  • sends xmit 0

The handler checks Multi-Flex ownership before acting. If another station owns TX and this client is not locally transmitting, it reports that TX is owned elsewhere instead of pretending this client can cancel another station's transmit.

netCW Diagnostics

The CW/netCW log path records each stage of MIDI/keyboard input and netCW scheduling, including trace IDs, source labels, timing deltas, VITA stream/index details, UDP copy timing, and TCP fallback/backstop information.

During local on-air testing, the reviewed netCW traces showed:

  • straight-key press order: cw ptt 1 -> cw key 1
  • straight-key release order: cw key 0 -> cw ptt 0
  • paddle events feeding the local iambic keyer
  • WPM changes reflected in generated dit/dah timing
  • press-to-netCW scheduling typically at 0-1 ms

Validation

Built with the project-required 22 parallel jobs:

env -u CPLUS_INCLUDE_PATH cmake --build build-ninja --parallel 22

Ran focused tests:

ctest --test-dir build-ninja --output-on-failure -R "(shortcut_manager_test|midi_settings_test|radio_status_ownership_test)"

Result: 3/3 tests passed.

Also validated manually by @jensenpat:

  • keyboard straight key works over the air
  • keyboard left/right paddle controls work over the air
  • MIDI straight key works over the air
  • MIDI left/right paddle controls work over the air
  • WPM changes affect generated dits/dahs over the air
  • holding straight key keeps the tone active over the air

Reviewer Notes

The actual netCW UDP datagrams are still sent on the PanadapterStream network thread, while timing/scheduling is initiated from RadioModel. This PR does not move netCW scheduling to a dedicated high-priority timer thread; it focuses on functional CW controls, persistence, diagnostics, and a practical stuck-TX escape hatch.

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

jensenpat and others added 3 commits May 4, 2026 12:16
Add shared CW straight-key, dit, and dah actions for keyboard shortcuts and MIDI mappings; persist the XML-safe shortcut IDs; normalize legacy dotted CW MIDI IDs; and make the TX status badge an emergency transmit cancel control. Add focused shortcut and MIDI settings regression tests.

Co-authored-by: Codex <[email protected]>
@jensenpat jensenpat marked this pull request as ready for review May 4, 2026 22:26
@jensenpat jensenpat changed the title Add CW keyboard and MIDI keying controls Add CW keyboard USB HID and MIDI keying controls May 4, 2026
Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

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

Thanks @jensenpat — this is a thorough, well-tested rollup. Persisting CW shortcuts via XML-safe IDs (cwkey/cwdit/cwdah) plus the bidirectional MIDI migration is the right call (avoids touching AppSettings's element-name validator), and pulling the initial syncLocalKeyerToRadio() after the iambic callbacks are installed fixes a real ordering bug. The new shortcut_manager_test and the expanded midi_settings_test (plus the missing add_test(NAME midi_settings_test ...) registration) close the regression window neatly.

A few notes:

1. TCP backstop is re-enabled — please double-check on a connected radio. RadioModel::sendNetCwCommand now ends with sendCmd(fullCmd) after the four UDP copies. The previous comment that this code is replacing said the opposite — that fw 4.1.5 rejected every CW command over TCP with 0x50001000 ("command syntax error") and the TCP send was producing log noise. The new comment says the 16-bit timestamp format makes it dedupable. Worth confirming aethersdr.log doesn't show a 0x50001000 per keying event; if it does, this will spam logs even if keying still works via UDP. If you have a fresh on-radio capture confirming clean TCP responses with the new timestamp format, calling that out in the PR description would be helpful for the next person to revisit this.

2. m_currentMidiTrace stashing pattern. In MainWindow.cpp the lambda for paramActionTrace writes m_currentMidiTrace = {paramId, traceId, midiCallbackMs, midiDispatchMs}, calls the setter (which reads it via setCwStraightKeyState etc.), and then clears it. This works because the call is synchronous on the main thread, but it's brittle — if any future MIDI setter ever defers via QMetaObject::invokeMethod or a QTimer, the trace will be empty by the time it lands. A small comment on the MidiActionTrace struct in MainWindow.h noting "valid only during synchronous setter dispatch" would help.

3. shortcutSequenceFromKeyEvent Qt6 deprecation. QKeySequence(static_cast<int>(modifiers) | ev->key()) works but is deprecated in Qt6 in favor of QKeyCombination. Not a blocker — most of the codebase still uses the int form — but if you're touching it anyway, QKeySequence(QKeyCombination(modifiers, static_cast<Qt::Key>(ev->key()))) is the modern form.

4. m_lastCwPaddleTraceId semantics. The atomic holds the most recent paddle-input trace, which iambic-generated dit/dah edges then attribute to. That's fine for tracing but means a single squeeze produces N edges all tagged with the original press's trace ID — readers of aether.cw should know that's expected. Minor.

Scope review: all 15 files map to the stated PR scope (CW keyboard/MIDI, persistence, TX cancel badge, netCW diagnostics rolled up from #2336). Nothing outside scope. Conventions look good — AppSettings not QSettings, atomics for cross-thread state, RAII throughout.

Use event COMMENT only — leaving final approval to maintainers since the TCP backstop change has practical impact worth a maintainer's eyes.

@ten9876
Copy link
Copy Markdown
Owner

ten9876 commented May 5, 2026

Claude here on Jeremy's behalf — really nice work, and the persistence-bug catch (dotted shortcut IDs being silently dropped because AppSettings won't write XML-invalid element names) is a great find. Hardware validation on both keyboard + MIDI paths is exactly the rigor we want. Two requests before merge:

1. Rebase onto current main. Your base (89bdcfa7) predates the merges of #2336 (netCW protocol fix, now fa3c5aa8) and #2370 (small follow-up that dropped the dead paramAction signal). The diff currently includes #2336's content as-if-new, which makes it hard to read. Most of the rebase will auto-resolve cleanly — git's 3-way merge will recognize the byte-identical overlap from #2336.

2. Resolve the atomic-naming overlap. This is the one piece that needs explicit attention:

You have two clean options on rebase:

  • Drop the new names, use m_lastCwMidiTraceId/SourceMs everywhere your new code currently uses m_lastCwPaddleTraceId/SourceMs. Smaller diff, no churn.
  • Keep the new _Paddle* names, also update the serial-paddle handler to use them, and drop the old _Midi* declarations. More accurate naming (these atomics are specifically for the paddle bridge) at the cost of touching the serial path.

I lean toward option 2 since the new names describe their actual role better, but either is fine.

The other small overlap (paramAction signal in MidiControlManager + stale comment in MainWindow.h:327) should auto-resolve cleanly because main deleted those lines and your branch didn't touch them — standard 3-way merge drops them.

Once rebased + atomic-naming resolved, this is a clean admin-merge.

73, Jeremy KK7GWY & Claude (AI dev partner)

ten9876 added a commit that referenced this pull request May 6, 2026
- Add CW keyboard and MIDI controls (#2361)
- KeyboardMapWidget: fix J tile mapping to Qt::Key_I
- MainWindow: don't block CW/Space PTT shortcuts during slider lease
- Honor cw break_in setting for keyboard CW keying
@ten9876 ten9876 closed this in #2391 May 6, 2026
@ten9876
Copy link
Copy Markdown
Owner

ten9876 commented May 6, 2026

Claude here on Jeremy's behalf — thanks @jensenpat, this was solid work and exactly the kind of operator-facing CW polish AetherSDR needed. Merged into main as b4283a9 via #2391. You're preserved as Co-Author on the squashed commit, alongside Codex.

A few things changed on the way in:

  • Skipped your first two commits (netCW timing fix + trace logging) since Fix and trace netCW keying for straight key and iambic CW #2336 already landed an equivalent on main before this PR. Just the third commit ("Add CW keyboard and MIDI controls") was cherry-picked onto current main.
  • Atomic-naming overlap auto-resolved in your favor — the 3-way merge adopted your m_lastCwPaddle* names everywhere. The serial-paddle handler was updated to match. No leftover _Midi* references.
  • Three follow-up fixes from hardware testing, bundled in the same squash:
    • Pre-existing KeyboardMapWidget bug where the J tile was bound to Qt::Key_I (one-character typo unrelated to your work — caught while testing the new CW shortcuts in the editor).
    • Slider shortcut lease was blocking momentary CW + Space PTT for 2+ seconds when a slider had focus — split the helper so the lease only affects QShortcut dispatch, not momentary keying.
    • RadioModel::sendCwKey and your iambic keyer's onPaddleEvent both auto-PTT'd around every key/squeeze, which made break_in=0 a no-op and killed break_in_delay hang time with break_in=1. Stripped the auto-PTT in both paths so the radio's break-in setting is fully respected.

Hardware-validated against FLEX-8600 fw 4.1.5 on the way through. Closing this PR in favor of #2391. Really nice contribution — looking forward to more.

73, Jeremy KK7GWY & Claude (AI dev partner)

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.

2 participants