Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

morisil
Copy link
Contributor

@morisil morisil commented Jul 9, 2024

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)

… 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
Copy link
Member

hamoid commented Jul 18, 2024

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
Copy link
Contributor Author

morisil commented Jul 26, 2024

  • 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
Copy link
Contributor Author

morisil commented Jul 26, 2024

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.

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.

2 participants