Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Advanced Thru #40

Open
mink99 opened this issue Apr 7, 2016 · 11 comments · May be fixed by #232
Open

[RFC] Advanced Thru #40

mink99 opened this issue Apr 7, 2016 · 11 comments · May be fixed by #232
Milestone

Comments

@mink99
Copy link

mink99 commented Apr 7, 2016

Hi,

I am currently testing enhanced features in the midi-thru parts. The concept is, that you can add a callback to the midi auto-thru logic.

It has the signature bool fn(message.type, &message.data &message.data2, &channel) maybe will change ..

The behaviour is : if the function returns false, the message will not be sent , otherwise it will be sent with potentially modified parameters data1, data2 and channel (if available).

This is the first stage of the required features of a hardware midi patchbay, but is also useful on other occasions.

Currently I am using it for a box, that provides complex-preset based program changes to external modules. It will be connected by its midi-in from the masterkeyboard, on its midi out to a multitimbral instrument. First I am suppressing all active sensing from the keyboard, then I trap all program changes from the keyboard, remap them to the internal presets and send those new program changes from the box. This is just one example, there could be more.

I am convinced that it is not only me who could use this feature, so I am raising this issue as a starting point for a discussion.

Thank you

Kris

@mink99
Copy link
Author

mink99 commented Apr 7, 2016

I could post my code, but it is not really conforming to the coding style, and maybe would interfere with other planned changes ? Should I ?

@franky47
Copy link
Member

franky47 commented Apr 8, 2016

Hi Kris,

It seems like a good idea ! The original intent of the thru filter was to provide a way to choose which messages were thru'd, based only on the input channel, but I have seen users having the need to remap messages and have less permissive filters, so this could be useful.

I'd be glad to review your code and we could work on it on a separate branch.

@mink99
Copy link
Author

mink99 commented Apr 9, 2016

ok here is my code :
MIDI.zip
diffs (against stable)

diff.zip

i did some testing, hopefully it works as expected

sample Code from my application :

bool channelMessageThruFilter(midi::MidiType inType,  midi::DataByte &inData1,  midi::DataByte &inData2,  midi::Channel &inChannel)
{
  if (inType == midi::ActiveSensing )  return false;

  if (inType == midi::ProgramChange )
  {
    currentPatchID = inData1; // --> this is internal code of the app. capture program changes from a master Keyboard
    updateScreen(VK_CHANNEL); // this is what you should never do , expensive calls within a callback
    return false; // do not forward
  }
}

@mink99 mink99 closed this as completed Apr 9, 2016
@mink99
Copy link
Author

mink99 commented Apr 9, 2016

Sample code for #41 would be

bool channelMessageThruFilter(midi::MidiType inType,  midi::DataByte &inData1,  midi::DataByte &inData2,  midi::Channel &inChannel)
{
  if (inType == midi:: PitchBend   )
  {
     int bend = (int)((inData1 & 0x7f) | ((inData2 & 0x7f) << 7)) + MIDI_PITCHBEND_MIN);
     CCPitch = 1+ 127* float(
            (bend - PITCHBENDMIN)/
                float(PITCHBENDMAX-PITCHBENDMIN));

    MIDI.sendControlChange(80, CCPitch, channel);
    return false; // do not forward
  }
}

this is not tested. and shows one of the challenges of this approach : would it make sense to be able to modify message types ? I am on the strong opinion no, it would be more feasible to be able to explicitly send messages within the callback. but that would require some macro support for transforming existing parameters into new ones...

@mink99 mink99 reopened this Apr 9, 2016
@mink99
Copy link
Author

mink99 commented Apr 10, 2016

The above example should be simplified and split into two methods:

In the callback filter suppress the pitch bend messages only. In the callback create the modified message.
It seems important, that, if we are receiving in running-status mode, and suppress messages through the filter, we will have to recreate the status bytes for the next, non-suppressed message, because on a channel message, which is directed to another channel than the message before, if we suppress this message, the following message will be sent to the channel of the previous message, not to the intended.... Maybe this is also the problem on # # #41

@mink99 mink99 closed this as completed Apr 10, 2016
@mink99 mink99 reopened this Apr 10, 2016
@eclab
Copy link

eclab commented Oct 6, 2016

I like this idea in general but it propagates a weakness in the code with regard to MIDI thru. So far as I can tell -- and I may be wrong -- a message must be completely parsed before it is sent. So if you have (say) a three-byte message, you'll have to wait perhaps 1 ms before you can resend it. So you have a 1ms lag. That doesn't sound like much but really is.

Ideally a thru filter mechanism would query the filter as soon as enough information about the message has been parsed to determine useful information about it. For example, as soon as we determine that the message is (say), a CC message of interest to us, we should query the filter, then if it says it's okay, we pass the message in real time as the rest of it comes in, thus minimizing possible lag.

This is of course a more complex behavior.

@franky47
Copy link
Member

franky47 commented Oct 6, 2016

@eclab, that would minimise the latency, indeed. One issue though is that some messages can be intertwined with others (ie: Clock messages can happen at any time, even in between other messages bytes or within a SysEx frame).

@eclab
Copy link

eclab commented Oct 6, 2016

Ugh, yes, that's an ugly issue. :-(

@franky47
Copy link
Member

franky47 commented Oct 6, 2016

Another reason why we need to fully parse a message before thruing it is to avoid collisions. Say you have an input stream that gave you a status byte (the data byte(s) have not arrived yet), you can't send anything out until the input has finished thruing the complete message.

One potential way of solving this particular issue would be to use a send queue, where output messages would be pushed by the send methods, and popped to the UART TX when there are no more incomplete incoming messages.

@franky47 franky47 modified the milestones: v4.3, v5.0 Oct 10, 2016
@franky47 franky47 changed the title Enhanced filtering [RFC] Advanced Thru Jun 20, 2020
@franky47
Copy link
Member

I'd like to re-open discussion on this, as it's been way too long, and this was actually a good idea.

At the moment, the Software Thru feature is limited to 4 modes:

  • Fully off: nothing is forwarded from the input to the output
  • Fully on: everything is forwarded from the input to the output
  • Same Channel: Forward only Channel messages that correspond to the input channel. (non-channel messages are forwarded)
  • Different Channel: Forward only Channel messages that were not intended for this device (with a channel different than the input channel).

The Different Channel modes was intended to reduce MIDI traffic for other devices in a chain (consume what you need, feed the rest back to the chain). The Same Channel mode seems like YAGNI gone wrong, it was easy to make (by negating the Different Channel mode), but in practice I don't see much use to it.

Instead, to follow up on what was proposed by @mink99, we can have two new functions:

  • filter: choose whether an incoming message should be forwarded to the output
  • map: transform an incoming message before forwarding it.

Filter

Filter would be a callback that takes the incoming message as an argument (a const ref to avoid modification),
and returns a boolean: true to forward the message, false to consume it.

bool thruFilter(const MIDI::MidiMessage& inMessage)
{
  if (inMessage.type == midi::Clock)
  {
    // Consume (don't forward) Clock messages
    return false;
  }
  return true;
}

MIDI.setThruFilter(thruFilter);

Map

Map would also be a callback that takes the incoming message as argument, but returns another message. The message returned will be forwarded to the output.

MIDI::MidiMessage thruMap(const MIDI::MidiMessage& inMessage)
{
  if (inMessage.channel == 3)
  {
    // Remap all messages from channel 3 to channel 4:
    MIDI::MidiMessage modified = inMessage;
    modified.channel = 4;
    return modified;
  }
  return inMessage;
}

MIDI.setThruMap(thruMap);

Caveats & Limitations

  • Callbacks will always fire before Thru filter or map is called, and therefore will be passed the original received message.
    The modified message is only destined for Thru.
  • Similarly, the message that can be accessed after MIDI.read returns via MIDI.getMessage(), MIDI.getType() and other accessors will be the originally received one.

Complete flow diagram:
image

Note: this follows the behaviour of map and filter functions found in various other programming languages, like JavaScript or Python.

Migration from previous API

It's possible to have all four previous modes using only the filter callback:

bool thruOn(const MIDI::MidiMessage& inMessage)
{
  return true; // Forward everything
}

bool thruOff(const MIDI::MidiMessage& inMessage)
{
  return false; // Forward nothing
}

bool thruSameChannel(const MIDI::MidiMessage& inMessage)
{
  if (MIDI::isChannelMessage(inMessage.type) == false)
  {
    return true; // Always forward non-channel messages
  }
  return inMessage.channel == MIDI.getInputChannel();
}

bool thruDifferentChannel(const MIDI::MidiMessage& inMessage)
{
  if (MIDI::isChannelMessage(inMessage.type) == false)
  {
    return true; // Always forward non-channel messages
  }
  return inMessage.channel != MIDI.getInputChannel();
}

@hyperbolist
Copy link

I love this latest iteration on the idea. I've just started a new project where a thru filter is its primary function. I was going to naively use handlers for every incoming message type and conditionally echo them to the midi out until I stumbled upon the baked-in thru functionality and this discussion here.

Having a single thruFilter function to write would simplify my project considerably.

Looking forward to it!

hyperbolist added a commit to hyperbolist/arduino_midi_library that referenced this issue Aug 6, 2021
* resolves FortySevenEffects#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 FortySevenEffects#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 hyperbolist linked a pull request Aug 6, 2021 that will close this issue
franky47 pushed a commit that referenced this issue Oct 8, 2022
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants