FortySevenEffects / arduino_midi_library

MIDI for Arduino
MIT License
1.59k stars 255 forks source link

How to get a type for a pointer or reference to a MIDI instance #162

Open sehrgut opened 4 years ago

sehrgut commented 4 years ago

There is no documentation for this, and no way to use the macros to generate an appropriate typedef for pointers to the MIDI interface. The only documentation for this is in the closed issue #33, but it's quite obsolete.

That guidance still only provided a way to hardcode a specific type. This restricts portability of code, since code written against one board won't necessarily run on another.

A macro which creates a typedef with the same type that would be created by the CREATE_INSTANCE macros would solve this issue, since it could be used in class headers that needed to receive references to the existing MIDI instance.

sehrgut commented 4 years ago

For anyone who reads this after me, the typedef I had to do for an Arduino Leonardo rev. 3 board is:

typedef midi::MidiInterface<midi::SerialMIDI<HardwareSerial>> MidiInterface;

in contrast to the typedef from the old issue report:

typedef midi::MidiInterface<HardwareSerial> MidiInterface;

kisse66 commented 3 years ago

...and if you use custom settings you need something like

MidiInterface<SerialMIDI<HardwareSerial, MyMidiSettings>>

lathoub commented 3 years ago

typedef'ing the Transport layers and MIDI protocol would create name collisions when combining multiple Transports in 1 sketch - and - would also create multiple typedef's for the optional Settings and Platforms.

The idea was to hide as much of the Transport layer as possible, so that only the MIDI protocol has access to it. What is the use-case for accessing the underlying Transport layer?

sehrgut commented 3 years ago

I'm trying to pass it into another class as a dependency, so that the class can have a pointer to the MIDI interface to send MIDI messages even if it wasn't the class that set it up. I have multiple modules which can be switched in and out that generate the MIDI messages. I need a way to have a pointer to the MIDI interface so that I don't have to have each class that needs to use MIDI tear down and set up MIDI each time:

https://github.com/sehrgut/ardeublement/blob/master/ardeublement/PerformModule.hpp

lathoub commented 3 years ago

The design decision to use templates, rather than inheritance is dictated by Arduino target: avoid dynamic memory allocation at run-time, try to keep the runtime as small & fast as possible, and stable. (see discussion here) and here (Avoid using dynamic memory allocation). Dynamic polymorphism adds considerable overhead (vtable with pointers to virtual functions, ...) and so we opted for static polymorphism (less overhead, compile-time checks, ...) that is friendlier to memory constraint devices like the Arduino. The downside is that making the MIDIInterface portable, like in your use case above, more difficult.

I hope the above explanation helped on some fundamental design decisions

franky47 commented 3 years ago

What you could do is use a const reference (eg: const MyTranport& transport), which kind of acts like a pointer (no ownership setup/teardown), but have to be defined/resolved at assignation, so you can pass them as arguments in constructors, and they won't change. Or a simple reference if you need to mutate anything in there.

You can also use the typedef of the resolved type for the reference so you're sure to require the right API.

sehrgut commented 3 years ago

@lathoub Would it be possible to template some form of reference that could be passed around? Really all I'm trying to do is set up a MidiInterface once and pass it to code that emits MIDI messages to it. I'm not trying to avoid templates and static allocation — it's one of the things I like about this library! — I just don't want to be forced into a corner where all MIDI messages have to go through the one bit of code that happened to set up the interface instance without hardcoding a derived type.

@franky47 I don't think I follow. Are you suggesting that passing the underlying transport and constructing a new MidiInterface instance within each caller, or am I missing something?

kisse66 commented 3 years ago

Not sure we're talking about the same thing, but I also needed a reference to an initialized midi interface and just came up with

midi::MidiInterface<midi::SerialMIDI<HardwareSerial>, MyMidiSettings> & port;

I'm using custom sysex size hence the MyMidiSettings. This seems to work fine now. I use that type in an array for all my (several) ports initialized from the instances created elsewhere. Then it's easy to just loop the array and handle each port like ports[i].port.read()

Couldn't such type be used to pass as an argument also, just looks complex? My C++ is a bit rusty... Didn't try since now I just pass the index of the midi ports array for e.g. sending message like ports[p].port.send(...).

One problem I have now is that my system consists of two boards each with 4 MIDI ports. It would be so nice to create instances with HardwareSerial to 4 first ports and then something like RemoteSerial for the rest (physically at the other board). This class would look like serial port, but could transfer data to and from the remote board. This I could do. Maybe even add some SoftwareSerial MIDI on top. But now the template creates different types and can't simply handle all at once by index, right?

But yeah I understand the template approach reasoning.

sehrgut commented 3 years ago

@kisse66 Yeah, the template strategy isn't the issue, because all it does is ensure those configuration variants become compile-time types. If we could get type references at compile time, what both you and I are trying to do would work.

lathoub commented 3 years ago

a single typedef for all the cases (Hardware/Software - Settings, SerialSettings) is (AFAIK) not possible. What you need (?) is a virtual interface that exposes the MIDI commands

class IMidiInterface
{
   // MIDI command
   void sendClock() = 0;
}
template<class Transport, class _Settings = DefaultSettings, class _Platform = DefaultPlatform>
class MidiInterface : IMidiInterface
{
   void sendClock() {...}

Then use IMidiInterface

As stated above, we did not opt for this, as we wanted to keep overhead low (Arduino platform)

sehrgut commented 3 years ago

I suppose I could do that, but it does extend the overhead and code size. I guess I was hoping there would be some way to define a macro that would evaluate to a typedef aligning with the types created by the existing MIDI_CREATE_INSTANCE, MIDI_CREATE_DEFAULT_INSTANCE, and MIDI_CREATE_CUSTOM_INSTANCE macros. I'm perfectly comfortable writing:

MIDI_CREATE_DEFAULT_INSTANCE();
typedef MIDI_DEFAULT_INSTANCE_TYPE() MidiInterface;

I'm not looking for a single typedef that matches every possible concrete type, just a macro that will expand to a type name matching the instance created by one of the create-instance macros called with the same parameters. If that were possible, it should be zero-overhead, and within the design philosophy of the library.

I'm just trying to avoid having to write special code for each board, since I'd like my code to be able to run without modifications on any board supported by the library.

sehrgut commented 3 years ago

I'm wondering if something like this would work:

#define MIDI_TYPEOF_INSTANCE(Type)           \
    MIDI_NAMESPACE::MidiInterface<MIDI_NAMESPACE::SerialMIDI<Type>>

#define MIDI_TYPEOF_DEFAULT_INSTANCE()           \
    MIDI_TYPEOF_INSTANCE(HardwareSerial)

This seems way too simple to work in the general case, so I feel like I'm missing something; but it looks like it would generate the typename I use for Leonardo boards:

typedef midi::MidiInterface<midi::SerialMIDI<HardwareSerial>> MidiInterface;

which might be able then to be replaced by:

typedef MIDI_TYPEOF_DEFAULT_INSTANCE() MidiInterface;

My concern is that things like Settings might have ways of altering the final type in ways I'm not forseeing.

lathoub commented 3 years ago

this? (in SerialMIDI.h) Then use midiInterface_t

Downside: when creating 2 MIDI instances, the typdef is recreated and creates an error

/*! \brief Create an instance of the library attached to a serial port.
 You can use HardwareSerial or SoftwareSerial for the serial port.
 Example: MIDI_CREATE_INSTANCE(HardwareSerial, Serial2, midi2);
 Then call midi2.begin(), midi2.read() etc..
 */
#define MIDI_CREATE_INSTANCE(Type, SerialPort, Name)  \
    typedef MIDI_NAMESPACE::SerialMIDI<Type> transport_t; \
    transport_t serial##Name(SerialPort);\
    typedef MIDI_NAMESPACE::MidiInterface<transport_t> midiInterface_t; \
    midiInterface_t Name((transport_t&)serial##Name);

#if defined(ARDUINO_SAM_DUE) || defined(USBCON) || defined(__MK20DX128__) || defined(__MK20DX256__) || defined(__MKL26Z64__)
    // Leonardo, Due and other USB boards use Serial1 by default.
    #define MIDI_CREATE_DEFAULT_INSTANCE()                                      \
        MIDI_CREATE_INSTANCE(HardwareSerial, Serial1, MIDI);
#else
    /*! \brief Create an instance of the library with default name, serial port
    and settings, for compatibility with sketches written with pre-v4.2 MIDI Lib,
    or if you don't bother using custom names, serial port or settings.
    */
    #define MIDI_CREATE_DEFAULT_INSTANCE() \
        MIDI_CREATE_INSTANCE(HardwareSerial, Serial,  MIDI);
#endif

/*! \brief Create an instance of the library attached to a serial port with
 custom settings.
 @see DefaultSettings
 @see MIDI_CREATE_INSTANCE
 */
#define MIDI_CREATE_CUSTOM_INSTANCE(Type, SerialPort, Name, Settings) \
    typedef MIDI_NAMESPACE::SerialMIDI<Type> transport_t; \
    transport_t serial##Name(SerialPort); \
    typedef MIDI_NAMESPACE::MidiInterface<transport_t, Settings> midiInterface_t; \
    midiInterface_t Name((transport_t&)serial##Name);

/*! \brief Create an instance of the library attached to a serial port with
 custom settings nd customer serial settings.
 @see DefaultSettings
 @see MIDI_CREATE_INSTANCE
 */
#define MIDI_CREATE_VERYCUSTOM_INSTANCE(Type, SerialPort, Name, Settings, SerialSettings) \
    typedef MIDI_NAMESPACE::SerialMIDI<Type, SerialSettings> transport_t; \
    transport_t serial##Name(SerialPort);\
    typedef MIDI_NAMESPACE::MidiInterface<transport_t, Settings> midiInterface_t; \
    midiInterface_t Name((transport_t&)serial##Name);
bptremblay commented 1 year ago

Looks like trying to get a pointer to the MIDI object is an anti pattern, then. In my application, I only need to use a small set of methods, so if I create a wrapper for those methods, it would work.