Skip to content

Fix/midi transceiver fails on short/system messages - #347

Merged
edwinRNDR merged 6 commits into
openrndr:masterfrom
morisil:fix/midiTransceiverFailsOnVeryShortMessages
Sep 1, 2025
Merged

Fix/midi transceiver fails on short/system messages#347
edwinRNDR merged 6 commits into
openrndr:masterfrom
morisil:fix/midiTransceiverFailsOnVeryShortMessages

Conversation

@morisil

@morisil morisil commented Jul 9, 2024

Copy link
Copy Markdown
Contributor

I had a problem with orx-midi. My Yamaha keyboard is sending system MIDI events without data payload and channel information, which was crashing the program with ArrayIndexOutOfBounds. This PR is addressing all the issues while adding some features.

  • fix: default MidiDeviceReceiver will not crash on received messages without data byes (they won't be handled, but logged on trace level).
  • fix: NOTE_OFF MIDI events / messages fixed to carry also velocity required by MIDI standard for NOTE_OFF MIDI message
  • default MidiDeviceReceiver refactored to "trigger" function to avoid code cascading
  • MIDI message sending functions refactored to share one "send" function with a message factory
  • MidiEventType.CONTROL_CHANGED renamed to MidiEventType.CONTROL_CHANGE for consistency with Java MIDI naming
  • MidiEventType extended with all the types supported by MIDI
  • tests including mocks of Java MIDI devices (mockk added to dependencies)

morisil added 4 commits July 9, 2024 19:32
… without data byes (they won't be handled, but logged on trace level).

* fix: NOTE_OFF trigger carries velocity information
* default MidiDeviceReceiver refactored to "trigger" function to avoid code cascading
* MIDI message sending functions refactored to share one "send" function with a message factory
* MidiEventType.CONTROL_CHANGED renamed to MidiEventType.CONTROL_CHANGED for consistency
* MidiEventType extended with all the types supported by MIDI
* NOTE_OFF MIDI event fixed to carry also velocity required by MIDI standard for NOTE_OFF MIDI message
* tests including mocks of Java MIDI devices
@hamoid

hamoid commented Jul 18, 2024

Copy link
Copy Markdown
Member

Hi :)

  • I was wondering if mockk is required (as we haven't used it so far).
  • Maybe the volume in the note off event could have a default value of 0? I guess in most cases it will be 0, so the user doesn't need to specify it.

@morisil

morisil commented Jul 26, 2024

Copy link
Copy Markdown
Contributor Author
  • I was wondering if mockk is required (as we haven't used it so far).

We are not using any mocking framework, because there is almost no unit tests. :) And maybe the difficulty of writing tests without mocking framework is one of the reasons behind it. For example orx-midi is using a very tiny dependency on the Program, but providing test implementation of the Program instance would be a challenge, which mocking library is fixing. Purely functional unit tests do not require mocks. However in my practice I cannot imagine writing any bigger unit tests without mocks for the code which has injected dependencies. Unless what is injected is just a single method functional interface.

  • Maybe the volume in the note off event could have a default value of 0? I guess in most cases it will be 0, so the user doesn't need to specify it.

Interestingly enough 0 does not seem to be a default in this case, but 64, this is the reason I skipped it to avoid even more confusion:

The second data byte is the velocity, a value from 0 to 127. This indicates how quickly the note should be released (where 127 is the fastest). It's up to a MIDI device how it uses velocity information. Often velocity will be used to tailor the VCA release time. MIDI devices that can generate Note Off messages, but don't implement velocity features, will transmit Note Off messages with a preset velocity of 64.

http://midi.teragonaudio.com/tech/midispec.htm

@morisil

morisil commented Jul 26, 2024

Copy link
Copy Markdown
Contributor Author

Oops, I just notice that should receive NOTE_OFF event on receiving NOTE_ON message with velocity 0 is not testing what it is describing, I will fix it.

@edwinRNDR
edwinRNDR merged commit df2a596 into openrndr:master Sep 1, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants