FortySevenEffects / arduino_midi_library

MIDI for Arduino
MIT License
1.56k stars 253 forks source link

Version 5.0.0 "reflects" MIDI messages over USB #147

Closed SteveCooling closed 4 years ago

SteveCooling commented 4 years ago

Just tried to move a synth project over to versjon 5.0.0.

I have Arduino Leonardo (clone.. a Pro micro) connected via USB. It is supposed to take MIDI messages and convert them to CV/Gate ++ for an analog synth.

Compiling using Arduino 1.8.12 on macOS 10.15.4

Two failures occur on version 5.0.0 (confirmed not a problem on version 4.3.1)

  1. NoteOn and NoteOff messages are "reflected" back via USB, but with incorrect note and velocity.
  2. The NoteOn/NoteOff handler callbacks in the firmware are being fred the same bad data.

See attached .ino example and screenshot of MIDI messages passed.

#include <USB-MIDI.h>

USBMIDI_CREATE_DEFAULT_INSTANCE();

unsigned long t0 = millis();

#define PIN_CV 3

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
void setup()
{
  pinMode(LED_BUILTIN, OUTPUT);

  // Listen for MIDI messages on channel 1
  MIDI.begin(1);

  MIDI.setHandleNoteOn(OnMidiNoteOn);
  MIDI.setHandleNoteOff(OnMidiNoteOff);

  pinMode(PIN_CV, OUTPUT);
}

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
void loop()
{
  // Listen to incoming notes
  MIDI.read();

}

static int noteToPWM(int note) {
  return note * 2;
}

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
static void OnMidiNoteOn(byte channel, byte note, byte velocity) {
  analogWrite(PIN_CV, noteToPWM(note));
}

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
static void OnMidiNoteOff(byte channel, byte note, byte velocity) {
  //analogWrite(PIN_CV, 0);
}
Skjermbilde 2020-04-22 19 29 37
franky47 commented 4 years ago

Thanks for the report, @lathoub can you have a look at the incorrect data being passed to the callbacks ?

As for the reflection, it might be due to SoftThru being enabled by default, while this makes sense for hardware MIDI, it does not for USB and probably other transports.

lathoub commented 4 years ago

Hi, I tried to reproduce the issue, but can't.. I have used the updated NoteOnOffEverySec.ino demo.

Here is my setup: Leonardo with Sparkfun MIDI shield, the shield is connected to an M-Audio MIDI USB device.

Not sure what is happening

lathoub commented 4 years ago

@SteveCooling have you cloned the repo or used the IDE to download the USBMIDI library?

lathoub commented 4 years ago

I have issued a new release of the USB-MIDI library (v1.1.0)

SteveCooling commented 4 years ago

I used the Library Manager in the IDE22. apr. 2020 21:15 skrev lathoub notifications@github.com: @SteveCooling have you cloned the repo or used the IDE to download the USBMIDI library?

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

lathoub commented 4 years ago

The Arduino has picked up on the new release (v1.1.0) and should be fine now (I forgot to make a Release version of USB-MIDI when 5.0.0 of the MIDI Lib came out 🙈). @SteveCooling please test again

SteveCooling commented 4 years ago

Tested and works fine. Version 1.1.0 of USB-MIDI got rid of the corrupted data. MIDI.turnThruOff(); removed the unexpected "reflection". 👍

franky47 commented 4 years ago

Thanks, we might need to disable Thru by default for non-serial transports then.

SteveCooling commented 4 years ago

Yes it was very unexpected behaviour, and created a loop when I routed MIDI traffic from Ableton to it 😄

lathoub commented 4 years ago

How about adding a method in the Transport layer:

...
bool thruActivated()
{
  return true;
}
...

in MIDI.hpp

template<class Transport, class Settings, class Platform>
void MidiInterface<Transport, Settings, Platform>::begin(Channel inChannel)
{
...
    mMessage.data2   = 0;
    mMessage.length  = 0;

    mThruFilterMode = Thru::Full;
    mThruActivated  = mTransport.thruActivated();
}
franky47 commented 4 years ago

Good idea, it could be a static bool member rather than a function though.

lathoub commented 4 years ago

Ah, good!

static const bool thruActivated = false;

...
    mThruFilterMode = Thru::Full;
    mThruActivated  = mTransport.thruActivated;
}
lathoub commented 4 years ago

pr #148