[midi] Fix center-64 MIDI VFO tuning jumps (CTR2-Midi)#4271
Conversation
There was a problem hiding this comment.
Solid, well-scoped fix. The root cause is diagnosed correctly: MidiControlManager.cpp:328 force-sets binding.relative = true for learned VFO knobs, so the explicit-relative path always decoded two's-complement and the Tier-2 center-64 fallback (line 416) was unreachable — matching the reporter's 63-step jump signature. Routing VFO relative CC through the new auto-detecting decoder with per-binding encoding lock is the right shape, and the extracted header + focused unit test is a nice touch. Encoding state is correctly cleared in rebuildIndex()/clearBindings(), so re-learns reset without stale locks, and the automation injection uses BlockingQueuedConnection correctly for the cross-thread hop to the MIDI manager. All CI is green and the commit is signed.
No blocking issues.
Non-blocking notes
- Encoding auto-detection keys off the first directional value being exactly
63/65; a fast initial turn (or a stray value) can lock the wrong encoding for the session. Inline comment with detail — worth a doc line at least. - The automation
midi ccpath decodes under its own{ch0,CC,0}encoding slot that persists Center64 once detected within a session, so a proof that sends65then later1would decode1as center-64, not two's-complement. Fine for the documented 65/63 use, just a testing gotcha to keep in mind.
🤖 aethersdr-agent · cost: $4.8266 · model: claude-opus-4-8
| if (value == 63 || value == 65) { | ||
| encoding = MidiRelativeCcEncoding::Center64; | ||
| } else if (value != 64) { |
There was a problem hiding this comment.
The encoding is locked from the first directional value, and only 63/65 select center-64 — any other non-64 first value falls to two's-complement and stays there. Two realistic mis-locks:
- A center-64 encoder whose first movement is a fast/multi-step detent (e.g.
66or62) is classified two's-complement, then66decodes as66-128 = -62(big backward jump) and stays wrong. - A two's-complement encoder whose first value happens to be
63/65(a 63-step spin) locks to center-64.
Both are unlikely as a first event and the common single-detent case is handled, so this is fine to ship — but please note the heuristic limitation in a comment or the bridge doc so it's not surprising later. No code change required.
There was a problem hiding this comment.
Clean, well-targeted fix for the right root cause. Learned VFO CC bindings are forced relative=true (MidiControlManager.cpp:328), so #2994's center-64 tier — guarded by !binding.relative — was dead code for exactly these bindings; routing the relative VFO path through the new auto-detecting decoder is the correct place to fix it. Per-binding encoding lock keeps existing two's-complement encoders from having their 1/127 pulses reinterpreted, the BlockingQueuedConnection from the main thread to m_midiControl (which lives on m_extCtrlThread) is the correct cross-thread pattern with no deadlock, and the decoder is nicely isolated behind a header-only pure function with focused tests. All 7 checks green, commit signed.
Non-blocking notes
MidiRelativeCcDecoder.h: the center-64 signature only matches a ±1 first detent (63/65). A center-64 controller whose first movement is ≥2 steps (e.g.66/62) locksTwosComplementfor the binding's lifetime and decodes as a large wrong-direction jump. Fine for CTR2-MIDI (sends ±1 per detent), but see the inline suggestion for a more robust detection band.
Nice work — thanks for the isolated decoder + regression test rather than inlining the logic.
🤖 aethersdr-agent · cost: $3.3624 · model: claude-opus-4-8
| if (value == 63 || value == 65) { | ||
| encoding = MidiRelativeCcEncoding::Center64; | ||
| } else if (value != 64) { |
There was a problem hiding this comment.
The auto-detect only recognizes the ±1 center-64 signature. If a center-64 controller's first directional event is ≥2 steps (value 66/62/67/61…), this falls into the else if (value != 64) branch and permanently locks TwosComplement — then 66 decodes as 66-128 = -62, a large jump in the wrong direction, for the rest of the binding's life.
Since two's-complement small moves cluster near 0/128 and center-64 small moves cluster near 64, you can disambiguate on a band around center without ambiguity (a genuine two's-complement value in 60..68 would be a ~60-step first flick — implausible as an initial event):
| if (value == 63 || value == 65) { | |
| encoding = MidiRelativeCcEncoding::Center64; | |
| } else if (value != 64) { | |
| if (value >= 60 && value <= 68 && value != 64) { | |
| encoding = MidiRelativeCcEncoding::Center64; | |
| } else if (value != 64) { |
Judgment call on the exact band width — not required for the CTR2-MIDI case this PR targets, but it makes first-movement detection robust to a fast initial detent.
There was a problem hiding this comment.
@jensenpat Saw the help wanted - not sure if you are looking for hardware testing? I was already in the process of testing my CTR2-MIDI this week, when I came across this PR. I've got a CTR2-MIDI on the bench and ran it through your midi cc bridge path: the fix works — slow first click locks center-64, and both slow and fast detents tune the right direction and amount (65→+100 Hz, 72→+800 Hz, and back).
I confirmed the auto-detect edge case the bot flagged is real (a fast first value like 72 does lock two's-complement), but on this controller it's hard to hit in practice — the firmware's speed-tuning always sends ±1 first from rest, so a real first touch lands on 63/65 every time.
One extra thing to note: if I change the knob's MIDI Type (WheelA→WheelB) without re-Learning, it tunes in an unexpected way — the binding stays locked to the encoding detected at the previous Learn, so a re-Learn is needed after any MIDI Type change for it to decode correctly. As I understand it, this PR decodes the two relative wheel encodings — WheelA (center-64) and WheelB (two's-complement) — so the user needs the knob set to WheelA or WheelB for VFO tuning. The CTR2 offers other MIDI Types too (sliders, a button, WheelB-r) that I haven't tested.
Happy to share the capture logs and/or test further, if that's the help you are looking for?
— authored by agent (Claude Code) on behalf of @skerker
There was a problem hiding this comment.
@skerker Thank you for testing and confirmation. Would you mind opening a separate issue on the re-learn behavior (and/or PR) since you have the wheel and can test please? Please feel free to pick that PR up if you have cycles.
NF0T
left a comment
There was a problem hiding this comment.
Correct root-cause diagnosis and the right general shape — auto-detecting center-64 vs. two's-complement from the first directional value, with a per-binding lock, closes the reported CTR2-MIDI bug cleanly, and the isolated decoder header + focused test is nice work. CI is green, commit is signed, and the live bridge proof plus @skerker's independent hardware test on real CTR2-MIDI both confirm the fix works for the reported case.
Two issues found beyond what's already been discussed here:
1. The automation injection path and a real controller can corrupt each other's encoding lock. MidiBinding::key() is (channel<<16)|(msgType<<8)|number — it doesn't include paramId. injectVfoCcForAutomation() builds a synthetic binding with channel=0, CC, number=0 (key=0) to read/write m_relativeCcEncodings. If a real user's VFO-tune-knob binding is also learned on MIDI channel 1 (internal 0) / CC 0 — a fully reachable combination — its key is the same. Running the midi cc automation verb with a value outside {63,64,65} in the same process lifetime as that real controller can permanently mis-lock its encoding, producing a large wrong-direction jump on every subsequent real detent. Suggest either a reserved sentinel key the real key() function can never produce, or looking up the actual rx.tuneKnob binding's real key instead of hardcoding channel/CC 0/0.
2. Editing any unrelated MIDI binding silently wipes the VFO knob's already-detected encoding. rebuildIndex()/clearBindings() now unconditionally clear all of m_relativeCcEncodings. MidiMappingDialog.cpp (untouched by this PR) calls rebuildIndex() from three existing paths that have nothing to do with the VFO binding: toggling Invert on any binding, toggling Relative on any binding, and deleting any single binding. So a user with a correctly-locked, working CTR2-MIDI VFO knob who opens the Mapping dialog to adjust something unrelated (e.g. an AF-Gain slider's Invert flag) gets their VFO encoding silently reset to Undetermined, with no visible indication — reopening the already-discussed "wrong first sample" window far more often than once at device connect. This changes the risk profile of the fast-first-detent edge case both bot reviews characterized as low-practical-risk, since it can now recur on any unrelated Mapping-dialog interaction, not just once per session. Suggest scoping the clear to just the affected binding's key rather than the whole map (or skip the clear entirely for edits that don't touch the VFO binding's own key).
Non-blocking: the header comment describes only the happy path — worth a line noting the "locks from first sample, no re-detection" limitation now that finding #2 shows it's reachable more than once per session.
Happy to re-review quickly once these are addressed — the core approach is right, these are both reachability/robustness gaps rather than problems with the detection heuristic itself.
Summary
65and63produce one clockwise/counter-clockwise step.1/127behavior for existing relative encoders by locking the detected encoding per binding.midi ccautomation verb for repeatable controller-path proof.Root cause
MIDI Learn marks every VFO Control Change binding as relative. The explicit-relative path then unconditionally decoded values as two's complement, so CTR2-MIDI's center-64 detents (
65/63) became-63/+63. The center-64 fallback added in #2994 was guarded by!binding.relative, making it unreachable for learned VFO bindings.The reporter's follow-up log shows the exact signature: 14.300 MHz moved to 14.363 MHz and then 14.237 MHz in 63-step increments.
Proof
Built the macOS app and drove the same learned VFO relative decoder through the agent automation bridge on a connected FLEX-8400M (firmware 4.2.18.41174), with the configured 100 Hz tuning step:
midi cc 65midi cc 63Both detents moved exactly one 100 Hz step in opposite directions, the starting frequency was restored, and the final radio snapshot reported
transmitting:false.Tests
cmake --build build --target AetherSDR -j8midi_relative_cc_decoder_testmidi_settings_testbridge_docs_checktools/check_engine_boundary.py --strict(zero blocking findings)tools/check_a11y.py(existing warning-only findings)Fixes #4096
👨🏼💻 Generated with OpenAI Codex (GPT-5.6 Sol) and tested by @jensenpat