Fix MIDI pickup crossing detection never firing#3835
Open
mcfnord wants to merge 1 commit into
Open
Conversation
Pickup mode has a second acceptance path, next to the tolerance test, for the case where a fast fader move makes consecutive MIDI values skip over the software value. Two separate defects meant it never ran. While waiting for pickup, midiPickupTryApply returned early without recording the value, so the history deque stayed empty and the size() >= 2 guard in midiPickupShouldApply was never satisfied. And prevMidi read back(), which is the value just appended, so the bracket test reduced to midiValue == currentValue, already covered by the tolerance test above it. Either defect alone disables the path; both are fixed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.
MIDI pickup mode has a second acceptance path beside the tolerance test, for when a fast fader move makes consecutive MIDI values skip straight over the software value:
It has never run. There are two independent defects, and either one alone is enough to disable it.
1. The history is never recorded while waiting.
midiPickupTryApplybuilds a temporary copy, tests it, and on failure returns early — jumping over the// Update the pickup bufferblock at the end of the function. So whilewaitingForPickupis true, which is exactly the state the history exists to serve,pickupBufferstays empty. The largest deque ever passed tomidiPickupShouldApplyis 1, sosize() >= 2is never satisfied.2.
prevMidiis not the previous value.recentMidiValues.back()is the elementmidiValuewas justpush_back'd as, soprevMidi == midiValueand the bracket test reduces tomidiValue == currentValue— already covered by the tolerance test three lines above. Sweeping(prev, current, new)over 0..100 outside tolerance, the current code detects 0 of 323400 genuine crossings.The
size() >= 2guard is itself the evidence of intent: you only need two elements if you mean to look past the last one.What it costs a user
CSoundBase::ParseMIDIMessagemaps CC 0..127 onto fader 0..AUD_MIX_FADER_MAX(100), so one CC step is 0.79 fader units against aMIDI_PICKUP_TOLERANCEof 2. Slow moves are picked up by the tolerance test and the bug stays invisible. Fast moves are where it shows: sweeping a hardware fader past a software fader sitting at 50, the fader stays dead at several speeds until the user slows down and lands within ±2 units.The two middle columns are the reason this PR changes two things rather than one: fixing either defect on its own changes nothing measurable.
Checks
Harness compiles the two patched functions verbatim out of
audiomixerboard.cpprather than a re-typed copy, so what is measured is what is shipped.clang-format14.0.6 reports no change; compiles clean under-Wall -Wextra.What I did not do
I have not run a patched Jamulus with a physical MIDI controller — the behaviour above is from the extracted functions, not from hardware. If someone with a controller can confirm the fast-sweep case, that would close the loop.
One related thing I noticed and deliberately left alone:
g_midiPickupWaitingForPickupis a single flag per channel shared by the fader and the pan, while the history deques and timestamps beside it inMidiPickupStateare correctly per-control (recentFader/recentPan,lastMidiTimeFader/lastMidiTimePan). That means picking up the pan also clears the fader's waiting state, and either control's inactivity timeout re-arms both. It needs two flags rather than one, which is a different change from this one, so I have not folded it in. Happy to open it separately if you would like.CHANGELOG: Bugfix: MIDI pickup mode now correctly detects a fader crossing the software value during fast moves.