fix(midi): correct VFO binary encoder direction and step size source (#2993)#2994
Conversation
Two bugs in the rx.tuneKnob MIDI path, both confirmed on FLEX-8600 hardware and root-caused by comparison with Thetis Midi2CatCommands.cs (Principle VIII). Bug 1 — binary 0/127 encoder (Thetis convention: 127 = CW, 0 = CCW): relativeCcDelta(127) returns -1 (two's-complement CCW) so CW turns tune the VFO backwards; relativeCcDelta(0) returns 0 so CCW turns produce no movement at all. Fix: in the relative dispatch path, override the two's-complement decode for values 0 and 127 on the VFO knob to use the binary convention (+1/-1). Extend the backward-compat tier with a center-64 fallback for controllers such as Behringer/Arturia/MidiController Continuous mode (value - 64 = signed step count). Bug 2 — step size UI control does not affect MIDI tuning: The relativeAction handler read s->stepHz(), which only updates after the radio acknowledges the slice set step=N round-trip command. Keyboard and HID encoder tuning both use spectrum()->stepSize(), which updates immediately on UI change. Fix: use spectrum()->stepSize() (with s->stepHz() fallback) to be consistent. Fixes aethersdr#2993. Principle VIII. Principle XI.
There was a problem hiding this comment.
Thanks for the patch, @M8WLO — the diagnosis is clear and the layered tier-1/tier-2 decoding for rx.tuneKnob reads well. The spectrum()->stepSize() swap in MainWindow.cpp:3707 also brings the MIDI path in line with the HID encoder path right next door (MainWindow.cpp:3802), which is the right consistency call.
A couple of things worth a look:
1. Direction flip for two's-complement encoders explicitly marked relative (MidiControlManager.cpp:378)
The new branch only triggers for rx.tuneKnob + data2 ∈ {0, 127}, but 127 is also the legit two's-complement encoding for −1. So a user who already has a two's-complement encoder bound to the VFO knob in relative mode will see CCW pulses (127) flipped to CW after this change. They can recover with the inverted flag, but it's a silent behavior change for an existing working config. Two options if you want to be more conservative:
- Keep the binary remap restricted to
!binding.relative(i.e., only apply binary semantics in the legacy/backward-compat path where users can't opt out via the relative flag), and require the explicit-relative path to stay two's-complement. - Or add an explicit
EncoderConvention::Binarysetting on the binding so users can pick — the current code essentially makes binary the default for VFO and there's no way back.
Worth at least calling out in the PR description so a Thetis-coming user who reads it knows what changed.
2. Tier-2 center-64 catches everything non-relative (MidiControlManager.cpp:417)
data2 != 64 consumes any CC value 2–63 or 65–126 on a VFO-knob binding that wasn't marked relative. That's fine for genuine center-64 encoders, but if anyone had a fader/absolute control bound to rx.tuneKnob (admittedly unusual), each intermediate value now becomes a large data2 - 64 jump. Probably acceptable since absolute control of the tune knob is nonsensical, but a one-line note in the comment that this is intentionally "swallowing" absolute mode for VFO would help the next reader.
3. MainWindow.cpp:3707 — minor
spectrum() is checked but s->stepHz() is used as the fallback regardless — that's correct, just noting the ternary reads slightly oddly. No change needed.
Nothing blocking from my side — the fix solves a real bug and the structure is clean. Just want to make sure the two's-complement-relative case is a conscious trade-off rather than an oversight.
🤖 aethersdr-agent · cost: $7.7063 · model: claude-opus-4-7
Andy's original fix put the binary {0,127} override inside the
explicit-relative branch of onMidiMessage, which silently flipped CCW
pulses (data2=127, two's-complement -1) to CW for existing users with
two's-complement encoders bound to the VFO knob in relative mode.
This is the common case for two's-complement encoders — a single CCW
detent sends 127, so the regression would hit them on the very first
turn after upgrade.
Move the binary semantics down to the existing Tier 1 backward-compat
path (already restricted to !binding.relative VFO bindings). The
explicit-relative branch stays pure two's-complement, which is what
the relative flag has always meant. Binary-mode encoder users keep
their bindings on relative=false where Tier 1 handles {0, 1, 127} as
unit pulses.
Also clarifies the Tier 2 comment: routing non-64 values to step
counts intentionally swallows absolute-mode handling for VFO bindings,
because absolute control of an endless dial is nonsensical.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Merged as d5cf7f2 — thanks @M8WLO. The Thetis cross-reference in the root-cause analysis ( Pushed one regression-avoidance refinement on the binary-encoder side before merge: moved the End shape after the refinement is two tiers under the same
Explicit- Verifying on your FLEX-8600 against the bug repro before declaring closed would still be the gold standard if you have a moment — and the unchecked checkboxes in your test plan would be the natural place to land that confirmation. 73, |
…ethersdr#2993) (aethersdr#2994) ## Summary - **Bug 1:** MIDI VFO binary 0/127 encoder (Thetis "standard" convention) tuned in the wrong direction for CW (value 127 decoded as two's-complement −1) and produced no movement for CCW (value 0 decoded as 0). Fixed by overriding the decode for values 0 and 127 on the VFO knob in both the relative dispatch path and the backward-compat tier. - **Bug 2:** The `relativeAction` MIDI handler used `s->stepHz()` (updates only after radio round-trip echo) rather than `spectrum()->stepSize()` (updates immediately on UI change), making the step-size control in AetherSDR's UI have no effect on MIDI tuning. Fixed to match keyboard and HID encoder behaviour. - Backward-compat tier extended with a center-64 fallback for Behringer/Arturia-style controllers (value − 64 = signed step count). ## Root Cause Analysis Root cause confirmed by comparing AetherSDR's `relativeCcDelta` decode path against Thetis `Midi2CatCommands.cs::ProcessStdMIDIWheelAsVFO`, which treats `direction > 125` as CW (+1) and `direction <= 125` (including 0) as CCW (−1). ## Test Plan - [ ] Bind a binary 0/127 relative encoder to `rx.tuneKnob` via MIDI Learn - [ ] CW turn (sends 127) → VFO tunes CW by one configured step - [ ] CCW turn (sends 0) → VFO tunes CCW by one configured step - [ ] Change step size in AetherSDR UI → MIDI step changes immediately (no reconnect required) - [ ] Center-64 encoder (e.g. MidiController Continuous mode, Behringer): verify CW/CCW and multi-step remain correct - [ ] Existing two's-complement relative binding (marked `relative=true`, values 1/127): verify no regression Fixes aethersdr#2993. Principle VIII. Principle XI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Jeremy Fielder <kk7gwy@aethersdr.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
relativeActionMIDI handler useds->stepHz()(updates only after radio round-trip echo) rather thanspectrum()->stepSize()(updates immediately on UI change), making the step-size control in AetherSDR's UI have no effect on MIDI tuning. Fixed to match keyboard and HID encoder behaviour.Root Cause Analysis
Root cause confirmed by comparing AetherSDR's
relativeCcDeltadecode path against ThetisMidi2CatCommands.cs::ProcessStdMIDIWheelAsVFO, which treatsdirection > 125as CW (+1) anddirection <= 125(including 0) as CCW (−1).Test Plan
rx.tuneKnobvia MIDI Learnrelative=true, values 1/127): verify no regressionFixes #2993. Principle VIII. Principle XI.
🤖 Generated with Claude Code