Open hyperbolist opened 2 years ago
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.
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 ?
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
Cherry-picked 2ae9d9e
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
If this code is deleted, we need to find a why for the transports to switch off Thru
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.
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
.
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.
Exporting the message type is definitely a clean, path-of-least-surprise solution.
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.
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.