solidsnake745 / MIDI_Device_Controller

An Arduino based library for controlling various devices via the MIDI interface
GNU General Public License v3.0
6 stars 0 forks source link

Add logic and setting to shift pitchbend values #32

Open solidsnake745 opened 4 years ago

solidsnake745 commented 4 years ago

Recently discovered pitch bending was broken while working on intro course.

Issue occurring was all pitch bend commands would sound wrong - resulting note after bending was far too high. For the notes tested with, the bend values should have resulted in lower tones - not higher; suggesting the bend value used was larger than expected.

After researching further, looks like there were two separate paths of failure that can be fixed with one change.

To summarize:

  1. Pitch bending failed when using serial MIDI through Hairless MIDI
  2. Pitch bending also failed when using Teensy's built in usbMIDI
  3. Built pitch bending test sketch with Teensy to debug input from both frameworks
  4. Found Teensy has updated pitch bend values from 0 to 16383 (expected range) to -8192 to 8192
  5. Found Hairless MIDI is sending the expected range, but the logic used to parse the serial is still imperfect and causing misinterpretation of the MIDI events and their data

For now, addressing only the need to shift the new values into the expected range to fix existing functionality for my own instruments. Need to address the issue with Hairless MIDI in #30.

solidsnake745 commented 4 years ago

Closing as unnecessary. Fix was to simply subtract 8192 from the incoming pitchbend value when calling the method. I.E. MDC.pitchBend([device index], [bend value] - 8192)

solidsnake745 commented 5 months ago

Just a little more info on this: Per the MIDI spec, the actual range of the data is from 0 to 16383 for pitch bend events, but the formula to apply pitch bending expects a range of -8192 to 8192. So depending on the framework parsing/sending MIDI data, one of those two ranges are possible and valid for pitchbend events.

Will just have to accommodate accordingly by either subtracting 8192 or not.

solidsnake745 commented 5 months ago

On second thought, it might be worth including that logic in a centralized function. In recent changes, there are two places where we calculate the pitch factor to do bending with. It's the same logic in two places, but separately called. Also the code and calculation was updated to subtract 8192 from the incoming value so the new fix would instead be MDC.bendDeviceNote([device index], [bend value] - 8192) when using Haireless MIDI instead of for Teensy usbMIDI

Current pitch bend methods are:

Would be useful to have a single function that does this calculation in a common place that can be used by both. Could then add an optional parameter for doing the shifting from one range to the other to each of the above functions. Maybe call it 'shiftToNegative' or maybe 'shiftRange' and default it to true? That implies the expected input is the original MIDI value of 0 to 16383 and the shifting will be done in the method. This might make it a little more simple for users? Maybe, maybe not. At the very least though, the logic will be part of the library and not reliant on a user's knowledge of the MIDI standard and frameworks that send MIDI data. Will have example sketches with the proper usage that they can just copy anyway.

So usage would look like this: Current Updated
Teensy usbMIDI bendNote(value); bendNote(value, false);
Hairless MIDI bendNote(value - 8192); bendNote(value);
Moppy Serial bendNote(value); bendNote(value, false);
MD_MIDIFile bendNote(value - 8192); bendNote(value);
Also I just noticed a bit of an issue with the calculation I'm doing for the pitch factor that needs to be corrected. Current Updated
Formula 2^([value]/8192) 2^(([semitones]/[semitones per octave]) * ([value]/8192))
solidsnake745 commented 5 months ago

After adding the common function and using it, felt better to default shiftRange parameter to false. Made more sense to pass in the expected range for the calculation and have that be the default behavior rather than expect the original value from MIDI data. Just a feeling/opinion.

Updated usage looks like this now: Current Updated
Teensy usbMIDI bendNote(value); bendNote(value);
Hairless MIDI bendNote(value - 8192); bendNote(value, true);
Moppy Serial bendNote(value); bendNote(value);
MD_MIDIFile bendNote(value - 8192); bendNote(value, true);