FortySevenEffects / arduino_midi_library

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

Incoming message length in callback is 0? #333

Open kisse66 opened 10 months ago

kisse66 commented 10 months ago

Perhaps I have missed something, but it looks like there is an issue with using message callback (midi::Message)?

I'm trying to make a midi router (patchbay) on a Teensy 4 (7 serial ports) and use setHandleMessage() to get incoming messages and decide what to do. Assumption is the incoming message could be forwarded as is. Wrong?

It almost works as expected, but if I just pass the incoming midi::Message& to .send() nothing happens (no valid message sent). This seems to be because incoming msg.length is 0: "MIDI MSG: ch 1 type 192 data1 54 data2 0 valid 1 len 0" (debug print).

if I pass that to send() it does not work. If instead of send(msg) using something like

    //g_ports[port]->send(msg);   NOT working length 0 ??
    g_ports[port]->send(msg.type, msg.data1, msg.data2, msg.channel);

does work (except for sysex). Looks like send() is using the .length and thus fails. But why the incoming length is 0 on a valid message? Other fields look valid.

Teensy 4 (seems to be same for STM32 at least), latest platformio, MIDI 5.0.2 (latest as fetched by platformio).

g_ports is an array of pointers to midi instances like midi::MidiInterface<midi::SerialMIDI<HardwareSerial>, MyMidiSettings> *g_ports[NUM_PORTS] = { &midi1, &midi2, &midi3, &midi4, &midi5, &midi6, &midi7 };

Instances created via e.g.

midi::SerialMIDI<HardwareSerial> serialmidi1(Serial1);
midi::MidiInterface<midi::SerialMIDI<HardwareSerial>, MyMidiSettings> midi1(serialmidi1);

custom settings used to increase sysex size mainly (SYSEX_BUF_SIZE 768).

struct MyMidiSettings : public midi::DefaultSettings
{
    static const unsigned SysExMaxSize = MIDIROUTER::SYSEX_BUF_SIZE; // the default is too small
    static const bool UseRunningStatus = false;
    static const bool Use1ByteParsing = false;
};
franky47 commented 10 months ago

While working on refactoring the SysEx code, I found that this field seems to only be used for SysEx, which doubles up with the length being encoded in data1 and data2. So there is indeed a bug here.

For your use-case, the upcoming filter and map Thru feature would be a perfect fit, I'm hoping to get it shipped by the end of the month.

kisse66 commented 10 months ago

Thanks for looking at this and pointing out the new feature. Was looking through all issues, but that title didn't trigger my eye. Great, will test as available. Could help my case also yes.

rbarreiros commented 8 months ago

I'm having the same issue, working on a AppleMIDI/SerialMIDI bridge, and it could be quite easy with setHandleMessage, but after a couple of days debugging I found that length is wrong, and send() checks for length to send data1/data2. I haven't tested SysEX yet.

Anyone managed to find the culprit?

rbarreiros commented 8 months ago

Oh, now I understand franky47 comment, length isn't updated at all during parse(). I managed to do that, might make a PR later or I can just paste a diff, might be quicker as it's just a couple of lines, unless @franky47 has already fixed the issue somewhere!

franky47 commented 8 months ago

Go ahead for a PR, but please base it off feat/v5.1.0. Thanks!