FortySevenEffects / arduino_midi_library

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

I can set defaults for a custom instance, but how do I define the message type? #153

Closed eagereyes closed 4 years ago

eagereyes commented 4 years ago

This is a great library! I'm building a USB-to-Serial interface using an AMD M0 board. The goal is to be able to transmit DX7-style SysEx instrument configurations that are 4K in size.

I'm modifying the MIDI-DIN2USB example from the USB-MIDI library.

struct LargeSysEx : public midi::DefaultSettings {
    static const unsigned SysExMaxSize = 8192;
    static const unsigned BaudRate = 31250;
};

MIDI_CREATE_CUSTOM_INSTANCE(HardwareSerial, Serial1, MIDICoreSerial, LargeSysEx);

This works fine for normal MIDI messages (I can play notes through it). Now the problem comes with the MidiMessage type definition. The default

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

obviously won't cut it, but when I modify it to

typedef Message<8192> MidiMessage;

I get compiler errors:

.../Documents/Arduino/MIDI-DIN2USB/MIDI-DIN2USB.ino: In function 'void setup()':
MIDI-DIN2USB:23:44: error: invalid conversion from 'void (*)(const MidiMessage&) {aka void (*)(const midi::Message<8192>&)}' to 'void (*)(const MidiMessage&) {aka void (*)(const midi::Message<128>&)}' [-fpermissive]
   MIDICoreUSB.setHandleMessage(onUsbMessage);

How do I change the MidiMessage type to use the larger size?

franky47 commented 4 years ago

Do you have the latest version (5.0.2) ? From what I can see in the code, it should pick up the size defined in the settings.

eagereyes commented 4 years ago

Yes, it's the latest version (I only just started messing with this tonight).

So if it picks it up, what do I put into the type definition though? MIDI_NAMESPACE::DefaultSettings::SysExMaxSize doesn't change (I just found a trick to have the compiler print it). Also, it appears that I'm getting truncated SysExes (I get small changes in the instruments just like without the custom instance).

franky47 commented 4 years ago

I don't have any mention of typedef Message<MIDI_NAMESPACE::DefaultSettings::SysExMaxSize> MidiMessage; in the source code for 5.0.2 (I just downloaded a fresh update in the Arduino Library Manager), what's your trick to have the compiler print it ?

franky47 commented 4 years ago

@lathoub, could this be related to your fixes in #151 ?

eagereyes commented 4 years ago

That typedef is from the MIDI_DIN2USB example in the USB-MIDI library I'm modifying. It's used in the onUsbMessage and onSerialMessage function definitions. Unless there's another place to get that definition from?

The trick is to define an array using the number you want to see as the size and then initializing it to 1. The compiler then prints out the size of the array. I found that here.

char (*__kaboom)[MIDI_NAMESPACE::DefaultSettings::SysExMaxSize] = 1;
lathoub commented 4 years ago

I'll look at the example in USB-MIDI that pre-dates the issue #151.

@eagereyes Why did you report the issue here, and not in USB-MIDI? @franky47 We need to add a note before folks add an issue, I've seen multiple transport related issues ending up here, not in the transport repo.

franky47 commented 4 years ago

In this case I think it makes sense to report it here, since the settings are defined in this library and using a core macro. But adding an issue template would be a good idea indeed !

lathoub commented 4 years ago

In this case I think it makes sense to report it here, since the settings are defined in this library and using a core macro.

Not sure, as the example is from USB-MIDI.

True though that the issue relates to the use of MidiMessage in combination with the setHandleMessage callback. Because MidiMessage is a typedef that makes it hard(er) to expose when the Settings are overwritten.

lathoub commented 4 years ago

I'll rework the MIDI_DIN2USB example

lathoub commented 4 years ago

@lathoub, could this be related to your fixes in #151 ?

The Settings part of #151 needs to be come to the Master branch.

ActiveSensing needs more thinking. @franky47 How do you want to precede?

franky47 commented 4 years ago

Can you cherry-pick the settings change onto another PR ?

lathoub commented 4 years ago

@eagereyes when using the DIN2USB, both transport protocols need to have the same SysEx message length

Here is a reworked example of DIN2USB in USB-MIDI branch feat/v1.1.3, but it depends on branch v5.0.3 of this library.

Can you test it? (I'll tackle the SysEx length after you confirmed that the example works)

lathoub commented 4 years ago

Can you cherry-pick the settings change onto another PR ?

I don't know, I was hoping you would :-)

lathoub commented 4 years ago

Can you cherry-pick the settings change onto another PR ?

I don't know, I was hoping you would :-)

I can add the modif in another Branch and PR - OK?

lathoub commented 4 years ago

Can you cherry-pick the settings change onto another PR ?

I don't know, I was hoping you would :-)

I can add the modif in another Branch and PR - OK?

new Branch (v5.0.3a) and PR

lathoub commented 4 years ago

@eagereyes when using the DIN2USB, both transport protocols need to have the same SysEx message length

Here is a reworked example of DIN2USB in USB-MIDI branch feat/v1.1.3, but it depends on branch v5.0.3 of this library.

Can you test it? (I'll tackle the SysEx length after you confirmed that the example works)

Add DIN2USB example with a modified SysEx buffer size: https://github.com/lathoub/Arduino-USBMIDI/blob/feat/v1.1.3/examples/MIDI_DIN2USB_SysEx/MIDI_DIN2USB_SysEx.ino

eagereyes commented 4 years ago

That works! Thank you both so much!

As for why I asked here: The parameter override is described in midi_Settings.h, which is part of this library. This also made sense to me, the message size shouldn't depend on the transport… Also, I had looked through the old issues here and had seen people have similar questions, though their solutions didn't help in my case.

But yes, this works well now! I can send a complete voice bank to my Korg Volca FM. Thanks again for your quick response, I really appreciate it!

franky47 commented 4 years ago

Glad it all worked out, enjoy your Volca, those little synths are 🔥 (I have a Beats and a Keys)