FortySevenEffects / arduino_midi_library

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

Running MIDI_DIN2USB.ino example results in missing data transmitted over USB #159

Open IsThisNameGoodEnough opened 4 years ago

IsThisNameGoodEnough commented 4 years ago

Hi,

I hope this is the right place to post, as there doesn't seem to be any issue tickets in the USB-MIDI repository.

I'm running the MIDI_DIN2USB.ino example script shown here on an Arduino Leonardo board with no modifications to the software. When I generate an external MIDI event on the serial RX pin I receive a mirror response on the serial TX (so the Thru function is working), but the response I see on USB is stripped of event information. An example is below:

When I generate a CC message and monitor the serial ports, I see two identical messages (one on the RX and the resulting message on the TX): image

But when I generate the same CC message on the serial RX pin and monitor the USB output, the CC number and value are missing: image

Both MIDI libraries are up to date on my system, so I'm kinda at a loss. Any help getting the example code to run properly would be greatly appreciated!

IsThisNameGoodEnough commented 4 years ago

As an update, I also tried modifying the MIDI_DIN2USB.ino example so that it uses the approach used in the DualMerger.ino example, in where the merging is done in an if statement in the main loop rather than using a message callback (my modified code is below). This fixes the problem of the missing data, but it seems that only a fraction of messages are actually passed through to the USB output (from testing it seems that less than 20% of messages are passed through). I'm guessing this is because the buffer is wiped when the MIDICoreSerial.read(); function is called by itself, so there's nothing to be read when it's checked immediately afterwards? From my experience this doesn't seem like a viable solution.

Modified code:

#include <USB-MIDI.h>
USING_NAMESPACE_MIDI;

typedef USBMIDI_NAMESPACE::usbMidiTransport __umt;
typedef MIDI_NAMESPACE::MidiInterface<__umt> __ss;
__umt usbMIDI(0); // cableNr
__ss MIDICoreUSB((__umt&)usbMIDI);

typedef Message<MIDI_NAMESPACE::DefaultSettings::SysExMaxSize> MidiMessage;

MIDI_CREATE_INSTANCE(HardwareSerial, Serial1, MIDICoreSerial);

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
void setup()
{
  MIDICoreUSB.begin(MIDI_CHANNEL_OMNI);
  MIDICoreSerial.begin(MIDI_CHANNEL_OMNI);
}

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
void loop()
{
  MIDICoreUSB.read();
  if(MIDICoreUSB.read()){
    MIDICoreSerial.send(MIDICoreUSB.getType(), MIDICoreUSB.getData1(), MIDICoreUSB.getData2(), MIDICoreUSB.getChannel());
  }

  MIDICoreSerial.read();
  if(MIDICoreSerial.read()){
    MIDICoreUSB.send(MIDICoreSerial.getType(), MIDICoreSerial.getData1(), MIDICoreSerial.getData2(), MIDICoreSerial.getChannel());
  }
}
franky47 commented 4 years ago

In your last example, you call the read methods twice, which would cause some messages to be dropped.

Let's say you have 2 messages in the RX buffer of MIDICoreUSB, the first read will parse and handle this message, but since nothing is done with it, it will be dropped when the next read is executed and handles the second message.

This should help:

void loop()
{
  if(MIDICoreUSB.read()){
    MIDICoreSerial.send(MIDICoreUSB.getType(), MIDICoreUSB.getData1(), MIDICoreUSB.getData2(), MIDICoreUSB.getChannel());
  }

  if(MIDICoreSerial.read()){
    MIDICoreUSB.send(MIDICoreSerial.getType(), MIDICoreSerial.getData1(), MIDICoreSerial.getData2(), MIDICoreSerial.getChannel());
  }
}
IsThisNameGoodEnough commented 4 years ago

Doh! I'm sorry, I thought the DualMerger.ino example included the function calls before the if statements. My second post works as intended with the extra function calls removed.

But the second approach doesn't handle SysEx messages, which I also need to forward. I've tried to add callbacks for SysEx messages, but they aren't called when using the approach in the DualMerger.ino example. In the updated code below, SysEx messages are only forwarded if I uncomment the read functions, which results in the issue that I mentioned in my second post.

#include <USB-MIDI.h>
USING_NAMESPACE_MIDI;

typedef USBMIDI_NAMESPACE::usbMidiTransport __umt;
typedef MIDI_NAMESPACE::MidiInterface<__umt> __ss;
__umt usbMIDI(0); // cableNr
__ss MIDICoreUSB((__umt&)usbMIDI);

typedef Message<MIDI_NAMESPACE::DefaultSettings::SysExMaxSize> MidiMessage;

MIDI_CREATE_INSTANCE(HardwareSerial, Serial1, MIDICoreSerial);

void setup()
{
  MIDICoreUSB.begin(MIDI_CHANNEL_OMNI);
  MIDICoreSerial.begin(MIDI_CHANNEL_OMNI);

  MIDICoreUSB.setHandleSystemExclusive(OnUSBSysEx);
  MIDICoreSerial.setHandleSystemExclusive(OnSerialSysEx);
}

void loop()
{
//  MIDICoreUSB.read();
  if(MIDICoreUSB.read()){
    MIDICoreSerial.send(MIDICoreUSB.getType(), MIDICoreUSB.getData1(), MIDICoreUSB.getData2(), MIDICoreUSB.getChannel());
  }

//  MIDICoreSerial.read();
  if(MIDICoreSerial.read()){
    MIDICoreUSB.send(MIDICoreSerial.getType(), MIDICoreSerial.getData1(), MIDICoreSerial.getData2(), MIDICoreSerial.getChannel());
  }
}

void OnUSBSysEx(byte* data, unsigned length) {
  MIDICoreSerial.sendSysEx(length, data, true);
}

void OnSerialSysEx(byte* data, unsigned length) {
  MIDICoreUSB.sendSysEx(length, data, true);
}

It seems that using the setHandleMessage() function would be the right approach (which is what's used in the MIDI_DIN2USB.ino example I tried at first), but that results in the lost data I mentioned in my first post. The code I tried using to test the setHandleMessage() is below. The only other option I see is setting up separate callbacks for every type of message, but that seems a little crazy since my understanding is that the setHandleMessage() is supposed to work as a catch-all. Any idea on what I'm missing? Appreciate the help!

#include <USB-MIDI.h>
USING_NAMESPACE_MIDI;

typedef USBMIDI_NAMESPACE::usbMidiTransport __umt;
typedef MIDI_NAMESPACE::MidiInterface<__umt> __ss;
__umt usbMIDI(0); // cableNr
__ss MIDICoreUSB((__umt&)usbMIDI);

typedef Message<MIDI_NAMESPACE::DefaultSettings::SysExMaxSize> MidiMessage;

MIDI_CREATE_INSTANCE(HardwareSerial, Serial1, MIDICoreSerial);

void setup()
{
  MIDICoreUSB.setHandleMessage(onUsbMessage);
  MIDICoreSerial.setHandleMessage(onSerialMessage);

  MIDICoreUSB.begin(MIDI_CHANNEL_OMNI);
  MIDICoreSerial.begin(MIDI_CHANNEL_OMNI);
}

void loop()
{
  MIDICoreUSB.read();
  MIDICoreSerial.read();
}

void onUsbMessage(const MidiMessage& message)
{
  MIDICoreSerial.send(message);
}

void onSerialMessage(const MidiMessage& message)
{
  MIDICoreUSB.send(message);
}
franky47 commented 4 years ago

Could it be that when you actually receive a SysEx, the callback runs first, effectively forwarding the SysEx correctly, but then MIDI.read returns true, which calls the manual forwarding again but on a subset of the data, which corrupts the stream somehow ?

You'd have to check after MIDI.read returns true what message type you've received, and only forward it manually if it's not SysEx.

CC @lathoub for the loss of data in setHandleMessage

IsThisNameGoodEnough commented 4 years ago

Could it be that when you actually receive a SysEx, the callback runs first, effectively forwarding the SysEx correctly, but then MIDI.read returns true, which calls the manual forwarding again but on a subset of the data, which corrupts the stream somehow ?

You'd have to check after MIDI.read returns true what message type you've received, and only forward it manually if it's not SysEx.

CC @lathoub for the loss of data in setHandleMessage

Oh, I think you're on to something! I added some LEDs to the SysEx callbacks and can confirm that they light up. I also tried commenting out the send commands in the if statements and in that case the SysEx messages are sent properly. So it looks like the second manual forward is causing the issue!

I tried checking the message type being received after MIDI.read returns true, but no matter what I try it stills seems to be called. I originally tried if(MIDI.getType() != SystemExclusive), but when that didn't work I also tried to check against SystemExclusiveStart and SystemExclusiveEnd as well. When I run the code below (which includes LED flashes when the manual forward is performed), I can confirm that the manual forward is still triggered when I send a SysEx message. I'm using the program MIDI-OX to send SysEx messages and it's been working well so far, so I'm pretty sure I'm generating the external messages properly. Am I missing something when it comes to filtering the message types?

#include <USB-MIDI.h>
USING_NAMESPACE_MIDI;

#define LED_USB 16
#define LED_Serial 14

typedef USBMIDI_NAMESPACE::usbMidiTransport __umt;
typedef MIDI_NAMESPACE::MidiInterface<__umt> __ss;
__umt usbMIDI(0); // cableNr
__ss MIDICoreUSB((__umt&)usbMIDI);

typedef Message<MIDI_NAMESPACE::DefaultSettings::SysExMaxSize> MidiMessage;

MIDI_CREATE_INSTANCE(HardwareSerial, Serial1, MIDICoreSerial);

void setup()
{ 
  pinMode(LED_USB, OUTPUT);
  pinMode(LED_Serial, OUTPUT);

  MIDICoreUSB.begin(MIDI_CHANNEL_OMNI);
  MIDICoreSerial.begin(MIDI_CHANNEL_OMNI);

  MIDICoreUSB.setHandleSystemExclusive(OnUSBSysEx);
  MIDICoreSerial.setHandleSystemExclusive(OnSerialSysEx);
}

void loop()
{
  long currentTime;

  if(MIDICoreUSB.read()){
    if((MIDICoreUSB.getType() != SystemExclusive) && (MIDICoreUSB.getType() != SystemExclusiveStart) && (MIDICoreUSB.getType() != SystemExclusiveEnd)) {
      MIDICoreSerial.send(MIDICoreUSB.getType(), MIDICoreUSB.getData1(), MIDICoreUSB.getData2(), MIDICoreUSB.getChannel());

      digitalWrite(LED_USB, HIGH);
      currentTime = millis();
      while(millis() - currentTime < 250) {}
      digitalWrite(LED_USB, LOW);
    }
  }

  if(MIDICoreSerial.read()){
    if((MIDICoreSerial.getType() != SystemExclusive) && (MIDICoreSerial.getType() != SystemExclusiveStart) && (MIDICoreSerial.getType() != SystemExclusiveEnd)) {
      MIDICoreUSB.send(MIDICoreSerial.getType(), MIDICoreSerial.getData1(), MIDICoreSerial.getData2(), MIDICoreSerial.getChannel());

      digitalWrite(LED_Serial, HIGH);
      currentTime = millis();
      while(millis() - currentTime < 250) {}
      digitalWrite(LED_Serial, LOW);
    }
  }
}

void OnUSBSysEx(byte* data, unsigned length) {
  MIDICoreSerial.sendSysEx(length, data, true);
}

void OnSerialSysEx(byte* data, unsigned length) {
  MIDICoreUSB.sendSysEx(length, data, true);
}
IsThisNameGoodEnough commented 4 years ago

Oh, just noticed one interesting thing. The manual forward in the code above is only triggered when a SysEx message is externally generated on the serial RX pin and not when it's externally generated on the USB. Kind of at a loss on why the two are acting differently.

franky47 commented 4 years ago

@lathoub might be able to shed a light on the difference, considering that SysEx is always a special cases (spanning multiple "packets").

copych commented 3 years ago

Hello! Actually I face the same problem. Generally MIDI_DIN2USB works as expected, except for the SysEx, only part of which is being forwarded between 2 ports. Searching thru the web I've found several thoughts that might be useful, but I definitely got not enough skills to test them all. What seemed to be the truth is that the size of RX buffer on the USB side is larger than TX one's of the COM port. So when we normally receive SysEx thru the USB we truncate it pushing thru the COM port (People say 64bytes). Maybe anyone could write some code for splitting/sending larger packets? If so, it would be nice if this issue'd be fixed in the library, not just in this example.

paul-emile-element commented 2 years ago

I have seen similar issues with this example. I have been using both Leonardo and promicro boards with a standard UART midi interface.

I have had problems with sysex and real time messages (active sensing in my case).

It appears that the send(midiMessage&) function does not handle the message as it should.

I have changed the code in my local repo to handle the sysex first. The problems here are 1- the code sends a status byte for the sysex message (this should only be sent for channel messages) 2- the code also assumes that the message does not include the start and end tags (which it does) and so it adds extra tags to the message, which ends up corrupting the data.

That resolves the sysex handling, but the code still can't handle real time messages. In my case, I have a piece of equipment sending active sensing. The code processes everything that is not sysex as a channel message, and that results in corruption of the real time messages.

I can submit a partial fix (sysex only) now and/or a more complete fix later (including real time messages)

paul-emile-element commented 2 years ago

I submitted a pull request (#250) Any comments / feedback would be appreciated

Petrus125 commented 1 year ago

I'm also currently working over this DIN2USB sketch. In fact, I would like to get the oposite, USB2Din, and seems should work, but does not. I tested: -DIN2 USB: Works fine ( for notes, cc etc) -Din2Din: works fine. -USB2Din not working -USB2USB not working

Tested with 32U4 bds. Leonardo, ProMicro.

Wish that USB2Din also work, but could not get. Any one tested or know how to ?

copych commented 1 year ago

some time ago i've stumbled over this with u4, and my lame solution was as following:

#include <USB-MIDI.h>
USING_NAMESPACE_MIDI;

typedef USBMIDI_NAMESPACE::usbMidiTransport __umt;
typedef MIDI_NAMESPACE::MidiInterface<__umt> __ss;
__umt usbMIDI(0); // cableNr
__ss MIDICoreUSB((__umt&)usbMIDI);

typedef Message<MIDI_NAMESPACE::DefaultSettings::SysExMaxSize> MidiMessage;

MIDI_CREATE_INSTANCE(HardwareSerial, Serial1, MIDICoreSerial);

void setup()
{ 

  MIDICoreUSB.begin(MIDI_CHANNEL_OMNI);
  MIDICoreSerial.begin(MIDI_CHANNEL_OMNI);

  MIDICoreUSB.setHandleSystemExclusive(OnUSBSysEx);
}

void loop()
{
  if(MIDICoreUSB.read()){
    if((MIDICoreUSB.getType() != SystemExclusive) && (MIDICoreUSB.getType() != SystemExclusiveStart) && (MIDICoreUSB.getType() != SystemExclusiveEnd)) {
      MIDICoreSerial.send(MIDICoreUSB.getType(), MIDICoreUSB.getData1(), MIDICoreUSB.getData2(), MIDICoreUSB.getChannel());
    }
  }

}

void OnUSBSysEx(byte* data, unsigned length) {
  MIDICoreSerial.sendSysEx(length, data, true);
}