thru filter overhaul

[hyperbolist]

• resolves #40 with franky47's proposed thru filter overhaul
• removes thru filter modes
• adds thru filter callback
• adds thru map callback
• old thru filter unit tests have been replicated with filter callbacks
• does not yet include documentation changes

I believe this implements the latest proposal for #40 and exercises everything necessary in the unit tests, including the immutability of mMessage after a thru map callback has modified the outgoing message.

The thru filter callbacks in the unit tests are not suitable for copying and pasting by end-users due to the difference in the MIDI namespace when setup via the unit tests vs via MIDI_CREATE_DEFAULT_INSTANCE().

If the changes here are deemed suitable, I'll work on documentation.

[hyperbolist]

It just occurred to me that the other transports expect thru to be off by default, and I haven't tested any transport other than the baked-in serial.

[franky47]

I've added a commit on master that adds a copy constructor for the Message object, which will be needed when copying SysEx data (otherwise just the pointers would be copied). Can you cherry-pick it or rebase your commits onto master ?

[franky47]

It just occurred to me that the other transports expect thru to be off by default, and I haven't tested any transport other than the baked-in serial.

Good point, Thru makes sense only for serial transports. That being said, it was initialized as active by default. Maybe we could have transports indicate whether they support Thru, and initialize the callbacks based on a static templated property.

cc @lathoub

[hyperbolist]

Cherry-picked 2ae9d9e

[lathoub]

It just occurred to me that the other transports expect thru to be off by default, and I haven't tested any transport other than the baked-in serial.

Good point, Thru makes sense only for serial transports. That being said, it was initialized as active by default. Maybe we could have transports indicate whether they support Thru, and initialize the callbacks based on a static templated property.

cc @lathoub

I was lurking on the thread; indeed: thru filter does not make sense in point to point communication (AppleMIDI, ipMIDI, BLEMIDI, USBMIDI) and is disabled by these transports in code:

// Override default thruActivated. Must be false for all packet based messages
static const bool thruActivated = false;

and taken up here

https://github.com/FortySevenEffects/arduino_midi_library/blob/9f5317f299af196277a05ed051882e047ed29d09/src/MIDI.hpp#L97

If this code is deleted, we need to find a why for the transports to switch off Thru

[franky47]

Since each transport already defines their Thru preference as a static const bool, there's no need to copy it in the MidiInterface instance, it can be referenced directly as Transport::thruActivated, so both begin and processThru could use this to return early for non-Thru transports.

[hyperbolist]

begin and processThru now refer to Transport::thruActivated. begin sets the initial filter callback based on its value, and processThru will exit early (prior to calling the filter callback) when false.

[franky47]

One thing that came clear when doing the example in https://github.com/FortySevenEffects/arduino_midi_library/pull/232/commits/2fec41ecfe56160589b475c9be27b028364daa54 is that it's hard to reference the midi::Message type, because of its template dependency over the SysEx array size, which is defined in the Settings:

using Message = midi::Message<midi::DefaultSettings::SysExSize>;

// Without the typedef above, this gets overly verbose:
bool filter(const Message& message)
{
    return true;
}

MIDI.setThruFilter(filter);

There is some strong coupling between the Settings traits, the MidiInterface and its underlying Message type, because of this centralisation of the static SysEx array size.

There are other open issues and discussion on how to reference types somewhere else in the sketch code, rather than just using the instance object, and I think it would be interesting to extend the macros to add type definitions.

I'm not keen on having a default value for the Message type template argument (pointing to the definition above), I believe it would cause more problems than it tries to solve with mismatching SysEx lengths when using custom settings.

[hyperbolist]

Exporting the message type is definitely a clean, path-of-least-surprise solution.

[franky47]

The only one issue I could see is a name clash with already user-defined types with the same name, although it's very unlikely.