openrndr / orx

A growing library of assorted data structures, algorithms and utilities for OPENRNDR
https://openrndr.org
BSD 2-Clause "Simplified" License
120 stars 36 forks source link

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

Open morisil opened 1 month ago

morisil commented 1 month ago

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.

hamoid commented 1 month ago

Hi :)

morisil commented 1 month ago
  • 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 commented 1 month ago

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.