bugfix: midi thru from udp to usb triggers an assert()#998
Merged
probonopd merged 3 commits intoprobonopd:mainfrom Dec 4, 2025
Merged
bugfix: midi thru from udp to usb triggers an assert()#998probonopd merged 3 commits intoprobonopd:mainfrom
probonopd merged 3 commits intoprobonopd:mainfrom
Conversation
usbmidi supports 16 virtual cables (nCable in the code) any midi message received from udp sets this to 24 which is invalid. This replaces 24 with 0
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes an assertion failure when forwarding MIDI messages received over UDP to USB by normalizing an out-of-range virtual cable number to a valid USB MIDI cable index. Sequence diagram for UDP to USB MIDI forwarding with cable normalizationsequenceDiagram
participant UdpSource
participant MidiRouter
participant CMIDIKeyboard
participant UsbMidiDriver
UdpSource->>MidiRouter: MIDI message (nCable 24)
MidiRouter->>CMIDIKeyboard: Send(pMessage, nLength, nCable 24)
CMIDIKeyboard->>CMIDIKeyboard: Check nCable
alt nCable == 24
CMIDIKeyboard->>CMIDIKeyboard: Set Entry.nCable = 0
else nCable != 24
CMIDIKeyboard->>CMIDIKeyboard: Set Entry.nCable = nCable
end
CMIDIKeyboard->>UsbMidiDriver: Enqueue USB MIDI message (Entry.nCable <= 15)
UsbMidiDriver->>UsbMidiDriver: Assert Cable <= 15 (no failure)
UsbMidiDriver-->>UdpSource: MIDI successfully forwarded via USB
Class diagram for CMIDIKeyboard Send logic with cable normalizationclassDiagram
class CMIDIKeyboard {
+void Send(u8* pMessage, size_t nLength, unsigned nCable)
}
class TSendQueueEntry {
+u8* pMessage
+size_t nLength
+unsigned nCable
}
CMIDIKeyboard "1" --> "many" TSendQueueEntry : enqueues
class SendImplementation {
+void Send(u8* pMessage, size_t nLength, unsigned nCable)
+void NormalizeCable(unsigned nCable)
}
CMIDIKeyboard ..> SendImplementation : implements logic
class NormalizeCableLogic {
+unsigned NormalizeCable(unsigned nCable)
}
SendImplementation ..> NormalizeCableLogic : uses
%% Pseudocode representation of the updated logic
class NormalizeCableLogicDetails {
+unsigned NormalizeCable(unsigned nCable)
unsigned nCableParam
unsigned resultCable
}
NormalizeCableLogicDetails : NormalizeCable(unsigned nCable)
NormalizeCableLogicDetails : if nCable == 24 then resultCable = 0
NormalizeCableLogicDetails : else resultCable = nCable
NormalizeCableLogic <|-- NormalizeCableLogicDetails
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of hard-coding 24, consider defining a named constant or reusing an existing enum/value for the UDP virtual cable so the intent is clearer and less error-prone if it changes elsewhere.
- You might want to clamp or validate nCable more generally (e.g., if nCable > 15, map to 0 or a default) rather than only handling the special case of 24, to avoid similar asserts from other invalid values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of hard-coding 24, consider defining a named constant or reusing an existing enum/value for the UDP virtual cable so the intent is clearer and less error-prone if it changes elsewhere.
- You might want to clamp or validate nCable more generally (e.g., if nCable > 15, map to 0 or a default) rather than only handling the special case of 24, to avoid similar asserts from other invalid values.
## Individual Comments
### Comment 1
<location> `src/midikeyboard.cpp:79-91` </location>
<code_context>
Entry.pMessage = new u8[nLength];
Entry.nLength = nLength;
- Entry.nCable = nCable;
+ // If the midi message originated from udp, nCable is VIRTUALCABLE (24)
+ // This violates usb midi as its > 15 and will trigger an assert
+ // maybe make this configurable or use the 1st usb rx message nCable
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding the magic value 24; rely on a named constant instead.
Since the code and comment both rely on the value 24, it would be clearer and safer to use the named constant (e.g., VIRTUALCABLE) instead of the literal. That way, if the underlying value changes, this comparison stays correct and the intent remains obvious.
```suggestion
Entry.pMessage = new u8[nLength];
Entry.nLength = nLength;
// If the midi message originated from udp, nCable is VIRTUALCABLE
// This violates usb midi as its > 15 and will trigger an assert
// maybe make this configurable or use the 1st usb rx message nCable
if (nCable == VIRTUALCABLE)
{
Entry.nCable = 0;
}
else
{
Entry.nCable = nCable;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/midikeyboard.cpp
Outdated
Comment on lines
+79
to
+91
| Entry.pMessage = new u8[nLength]; | ||
| Entry.nLength = nLength; | ||
| Entry.nCable = nCable; | ||
| // If the midi message originated from udp, nCable is VIRTUALCABLE (24) | ||
| // This violates usb midi as its > 15 and will trigger an assert | ||
| // maybe make this configurable or use the 1st usb rx message nCable | ||
| if (nCable == 24) | ||
| { | ||
| Entry.nCable = 0; | ||
| } | ||
| else | ||
| { | ||
| Entry.nCable = nCable; | ||
| } |
There was a problem hiding this comment.
suggestion: Avoid hardcoding the magic value 24; rely on a named constant instead.
Since the code and comment both rely on the value 24, it would be clearer and safer to use the named constant (e.g., VIRTUALCABLE) instead of the literal. That way, if the underlying value changes, this comparison stays correct and the intent remains obvious.
Suggested change
| Entry.pMessage = new u8[nLength]; | |
| Entry.nLength = nLength; | |
| Entry.nCable = nCable; | |
| // If the midi message originated from udp, nCable is VIRTUALCABLE (24) | |
| // This violates usb midi as its > 15 and will trigger an assert | |
| // maybe make this configurable or use the 1st usb rx message nCable | |
| if (nCable == 24) | |
| { | |
| Entry.nCable = 0; | |
| } | |
| else | |
| { | |
| Entry.nCable = nCable; | |
| } | |
| Entry.pMessage = new u8[nLength]; | |
| Entry.nLength = nLength; | |
| // If the midi message originated from udp, nCable is VIRTUALCABLE | |
| // This violates usb midi as its > 15 and will trigger an assert | |
| // maybe make this configurable or use the 1st usb rx message nCable | |
| if (nCable == VIRTUALCABLE) | |
| { | |
| Entry.nCable = 0; | |
| } | |
| else | |
| { | |
| Entry.nCable = nCable; | |
| } |
|
Build for testing: |
Contributor
|
I think you should simply change the VIRTUALCABLE define to 0 in udpmididevice.cpp. |
This reverts commit d300d2e. For a much simpler solution...
Contributor
Author
much simpler :) |
usbmidi supports 16 virtual cables (nCable in the code)
any midi message received from udp sets this to 24 which is invalid.
This replaces 24 with 0
|
Build for testing: |
Owner
|
@penfold42 thank you very much for this bugfix. Does the latest build work for you as intended? Do you think it is ready for being merged? |
Contributor
Author
The build from my dev environment does - do you think I should also test the github actions build? |
Owner
|
Yes, that'd be the their purpose. Thanks! |
Contributor
Author
Tested that build on my pi3. VMPK->rtpmidid->minidexed->Arduino USB midi cable->homebrew WiFi midi box->fluidsynth |
Owner
|
Thanks @penfold42 |
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
3 participants
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.
usbmidi.cpp(91): assertion failed: Cable <= 15
usbmidi supports 16 virtual cables (nCable in the code).
Midi message received from udp sets this to 24 which is invalid.
This replaces 24 with 0 but perhaps something configurable or dynamic could be done.
Summary by Sourcery
Bug Fixes: