fix(midi): MIDI tx.mox toggle reads isTransmitting() not isMox()#2859
Conversation
There was a problem hiding this comment.
Thanks @M8WLO — the MOX fix itself is well-reasoned and correct. The root-cause analysis matches the codebase: the FlexControl path at src/gui/MainWindow.cpp:3546 already uses isTransmitting() for the same toggle, so this brings the MIDI path into line. Nice catch on the SmartSDR protocol quirk (radio never sending mox=).
A few observations:
1. Scope creep — QSO Recorder registration is undisclosed
The PR title and body describe a one-line MOX fix, but the diff also adds 46 lines registering global.qsoRecord and global.qsoPlay MIDI params (the +47/-1 line count was the giveaway). The added code itself is reasonable — it correctly mirrors the dual client-side/radio-side routing from the VFO recordToggled/playToggled handlers at MainWindow.cpp:11413-11443, which is an improvement over a naive client-only registration.
That said, please either:
- Split the QSO Recorder addition into its own PR, or
- Update the title/body to reflect both changes.
A surprise feature in a one-line bugfix is hard to review and hard to bisect later.
2. Overlap with your own PR #2845
Your open PR #2845 already registers qso.record and qso.play as MIDI-learnable params (client-only path). The additions here use different IDs (global.qsoRecord / global.qsoPlay) and add the radio-side SliceModel::setRecordOn/setPlayOn routing that #2845 lacks. If both land, users get two entries in the MIDI Mapping dialog that do nearly-the-same thing, with the older one being functionally inferior. Worth reconciling — likely by closing #2845 in favor of this branch, or rebasing one onto the other.
3. Minor: activeSlice() may be null at registration time
The QSO Recorder lambdas correctly null-check activeSlice() before dereferencing — good. No change needed, just calling it out as verified.
The actual tx.mox one-liner is good to merge as-is. Holding the COMMENT review until the scope/#2845 question is sorted.
— AetherClaude
jensenpat
left a comment
There was a problem hiding this comment.
Please submit MIDI fix without your QSO recorder as that has already been merged. In the future, please use multiple work trees and send separate PRs.
|
The recorder and play are already there , they are just incorrectly tagged and don’t work , it’s one change who ever did the original didn’t do it properly
Sent from Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: jensenpat ***@***.***>
Sent: Friday, May 22, 2026 1:31:05 AM
To: aethersdr/AetherSDR ***@***.***>
Cc: Andrew Hughes ***@***.***>; Mention ***@***.***>
Subject: Re: [aethersdr/AetherSDR] fix(midi): MIDI tx.mox toggle reads isTransmitting() not isMox() (PR #2859)
@jensenpat requested changes on this pull request.
Please submit MIDI fix without your QSO recorder as that has already been merged. In the future, please use multiple work trees and send separate PRs.
—
Reply to this email directly, view it on GitHub<#2859 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AMBMATVMUI7BEEJFFWCXQPL436NUTAVCNFSM6AAAAACZDIETSSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGNBRGYZDONZRGY>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
isMox() reads m_mox which is only populated if the radio echoes mox= in a transmit status message. The radio never sends that key (per protocol notes in CLAUDE.md), so m_mox stays false permanently. The MIDI toggle sentinel reads the getter to decide which direction to flip: with isMox() always returning false, cur was always 0, so every press computed 0 → flip to 1 → setTransmit(true). The toggle could only ever go TX-on, never TX-off. Fix: use isTransmitting() which is updated optimistically by setTransmit() the moment the user presses the button — the same source the FlexControl ToggleMox path (MainWindow.cpp:3545) already uses. Reported by user: MIDI-mapped MOX button on Raspberry Pi would key TX on first press but could not unkey via the same button; only the on- screen MOX click could release TX.
f912ed2 to
9759808
Compare
|
Rebased onto upstream/main. The QSO Recorder commit (50d603c) was already merged as part of #2845 (now at 78af7cd on main) — git rebase detected this and skipped it. The PR is now a clean 1-insertion/1-deletion against current main. Addressing the three review points: 1. Scope creep resolved — The rebase eliminates the QSO Recorder lines from this diff entirely. They are already on main. 2. Overlap with #2845 resolved — #2845 merged the dual-path QSO MIDI registration (both 3. activeSlice() null-check confirmed — as noted in the review, no change needed. The branch now contains only the original intent: fix MIDI MOX toggle to use |
|
Claude here — merged. Thanks @M8WLO, this is exactly the kind of One-character change, but the diagnosis is the substantive part: For Linux/Pi operators who'd previously been stuck reaching for the 73, |
…hersdr#2859) Fixes aethersdr#2858 ## Root Cause The MIDI toggle sentinel reads the getter, flips it, then calls the setter. The `tx.mox` getter used `transmitModel().isMox()` which reads `m_mox`. Per the SmartSDR protocol spec (v1.4.0.0), **the radio never sends `mox=` in transmit status messages**, so `m_mox` is always `false`. The getter therefore always returns 0, the toggle always computes 1, and `setTransmit(true)` is called on every press — stuck in TX on Linux/Pi. ## Fix One-line change: `tx.mox` MIDI getter changed from `isMox()` to `isTransmitting()`. `isTransmitting()` is set optimistically by `setTransmit()` the moment the command is sent, so it correctly reflects toggle state immediately. The FlexControl path at MainWindow.cpp:3545 already used `isTransmitting()` correctly — this brings the MIDI path into line. ## Test Plan - [ ] MIDI button mapped to MOX: press once → TX on, press again → TX off - [ ] Verified on Linux/Pi where `isMox()` was always `false` - [ ] No regression on Windows FlexControl toggle path
Fixes #2858
Root Cause
The MIDI toggle sentinel reads the getter, flips it, then calls the setter. The
tx.moxgetter usedtransmitModel().isMox()which readsm_mox. Per the SmartSDR protocol spec (v1.4.0.0), the radio never sendsmox=in transmit status messages, som_moxis alwaysfalse. The getter therefore always returns 0, the toggle always computes 1, andsetTransmit(true)is called on every press — stuck in TX on Linux/Pi.Fix
One-line change:
tx.moxMIDI getter changed fromisMox()toisTransmitting().isTransmitting()is set optimistically bysetTransmit()the moment the command is sent, so it correctly reflects toggle state immediately.The FlexControl path at MainWindow.cpp:3545 already used
isTransmitting()correctly — this brings the MIDI path into line.Test Plan
isMox()was alwaysfalse