FortySevenEffects / arduino_midi_library

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

SysEx message issue with 5.0.2 #188

Open 0ba-pia opened 3 years ago

0ba-pia commented 3 years ago

I had to revert to 4.3.1 release since using v5.0.2 I had Sysex messages corruption. Difficult to debug but basically my setup is device (32U4 based) receives sysex "heartbeat" message every 2s from hardware serial (note that MIDIUSB v1.0.4 is also used by this device). When using MIDI v4.3.1 it works perfectly but compiling with 5.0.2 I receive sysex messages with wrong size (2 bytes instead of 3) after some time.

franky47 commented 3 years ago

Thanks, can you post your code here so we can see what could be the problem ?

lathoub commented 3 years ago

Thanks @0ba-pia for reporting. Can you provide the byte array(s) of the heartbeat SysEx

0ba-pia commented 3 years ago

Sorry .. code on sender side (uses MIDI v4.3.1):

keepAliveBuf[0] = SYSEX_KEEPALIVE; //0x54
keepAliveBuf[1] = 1;
keepAliveBuf[2] = 24;
MIDI.sendSysEx(3, keepAliveBuf, false);

And on receiver side (uses MIDI v5.0.2):

void handleSystemExclusive(byte *buf, unsigned bufSize) {
  if (bufSize > 2) {
    switch (buf[1]) {
      case SYSEX_KEEPALIVE :
        handleKeepAlive(buf[2],buf[3]);
        break;
      case ...
    }
  } else {
    logging = String();
    logging = logging + "Invalid SysEx size:"+bufSize;
    Serial.println(logging);
  }
}
lathoub commented 3 years ago

I'm unable to reproduce the error. I have 1 Leonardos sending 3 bytes SysEx every second (0xF0, 0x54, 1, 24, 0xF7), another one receiving the SysEx.

I noticed in your code that you dynamically allocate memory (logging = String();) which could lead to heap fragmentation (or memory leaks). Could it be that your keepAliveBuf gets corrupted?

Here is the code for both Leonardos:

#include <MIDI.h>

unsigned long t1 = millis();
byte sysex0bapia [] = { 0xF0, 0x54, 1, 24, 0xF7 };

MIDI_CREATE_DEFAULT_INSTANCE();

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
void setup()
{
  Serial.begin(115200);
  while (!Serial);

  MIDI.begin();
  MIDI.setHandleSystemExclusive(OnMidiSysEx);

  Serial.println(F("Send SysEx every second"));
}

// -----------------------------------------------------------------------------
//
// -----------------------------------------------------------------------------
void loop()
{
  MIDI.read();

  if ((millis() - t1) > 1000)
  {
    MIDI.sendSysEx(sizeof(sysex0bapia), sysex0bapia, true);
    t1 = millis();
  }
}

void OnMidiSysEx(byte* data, unsigned length) {
  Serial.print(F("SYSEX: ("));
  Serial.print(getSysExStatus(data, length));
  Serial.print(F(", "));
  Serial.print(length);
  Serial.print(F(" bytes) "));
  for (uint16_t i = 0; i < length; i++)
  {
    Serial.print(data[i], HEX);
    Serial.print(" ");
  }
  Serial.println();
}

char getSysExStatus(const byte* data, uint16_t length)
{
  if (data[0] == 0xF0 && data[length - 1] == 0xF7)
    return 'F'; // Full SysEx Command
  else if (data[0] == 0xF0 && data[length - 1] != 0xF7)
    return 'S'; // Start of SysEx-Segment
  else if (data[0] != 0xF0 && data[length - 1] != 0xF7)
    return 'M'; // Middle of SysEx-Segment
  else
    return 'E'; // End of SysEx-Segment
}
0ba-pia commented 3 years ago

Can you try hot unplugging and replugging the UART connection between those 2 Leonardos? Trying this several times, with v5.0.2 you may get the wrong packed size issue. And not with 4.3.1

franky47 commented 3 years ago

This might be your issue: establishing the connection mid-stream will cause false readings no matter what firmware is used.

Now if all subsequent messages received have the wrong size, this is indeed a problem. But it is expected that connecting in the middle of a message will have it be incomplete.

lathoub commented 3 years ago

I tried the scenario of disconnecting the connection a then re-establishing so. First I send (using MIDI-OX) a complete SysEx (F0 54 01 18 F7), then a partial SysEx (F0 53 01), then a complete again (F0 54 01 18 F7) - using version 4.3.1 and 5.0.2 (using the Library Manager - keeping the COM* window open).

Here is the log, showing the same output for both versions:

21:23:15.209 -> 40300
21:23:15.209 -> Receive SysEx
21:23:23.827 -> SYSEX: (F, 5 bytes) F0 54 1 18 F7 
21:23:32.429 -> SYSEX: (F, 4 bytes) F0 54 1 F7 
21:23:32.429 -> SYSEX: (F, 5 bytes) F0 54 1 18 F7 
21:24:20.108 -> 50000
21:24:20.108 -> Receive SysEx
21:24:27.581 -> SYSEX: (F, 5 bytes) F0 54 1 18 F7 
21:24:35.300 -> SYSEX: (F, 4 bytes) F0 54 1 F7 
21:24:35.300 -> SYSEX: (F, 5 bytes) F0 54 1 18 F7 

@franky47 MIDI_LIBRARY_VERSION & MIDI_LIBRARY_VERSION_PATCH need to be updated (for next release)

Here is the code:

#include <MIDI.h>

MIDI_CREATE_DEFAULT_INSTANCE();

void setup()
{
  Serial.begin(115200);
  while (!Serial);

  MIDI.begin();
  MIDI.setHandleSystemExclusive(OnMidiSysEx);

  Serial.println(MIDI_LIBRARY_VERSION, HEX);
  Serial.println(F("Receive SysEx"));
}

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

void OnMidiSysEx(byte* data, unsigned length) {
  Serial.print(F("SYSEX: ("));
  Serial.print(getSysExStatus(data, length));
  Serial.print(F(", "));
  Serial.print(length);
  Serial.print(F(" bytes) "));
  for (uint16_t i = 0; i < length; i++)
  {
    Serial.print(data[i], HEX);
    Serial.print(" ");
  }
  Serial.println();
}

char getSysExStatus(const byte* data, uint16_t length)
{
  if (data[0] == 0xF0 && data[length - 1] == 0xF7)
    return 'F'; // Full SysEx Command
  else if (data[0] == 0xF0 && data[length - 1] != 0xF7)
    return 'S'; // Start of SysEx-Segment
  else if (data[0] != 0xF0 && data[length - 1] != 0xF7)
    return 'M'; // Middle of SysEx-Segment
  else
    return 'E'; // End of SysEx-Segment
}
lathoub commented 3 years ago

@0ba-pia Have you looked at ActiveSensing to see if the connection is alive?

lathoub commented 3 years ago

@0ba-pia Was the above useful?

See also franky47's note: https://github.com/FortySevenEffects/arduino_midi_library/issues/188#issuecomment-748511841