Fix midi note-off should be always considered a button release

[arximboldi]

Some controllers, like the M-Audio Xponent or keyboards with
after-touch, that send a non-zero note off velocity. A note off should
always be considered a button release, regardless of its velocity value.

We duplicated every test involving a note-off to make sure we test both
for the velocity == 0 and velocity != 0 case -- given the subtle
structure of the code, it is easy to break either case. Alternative for
conciseness we could just test the later case.

[daschuer]

Thank you for the pull request.

Isn't the real issue here:
https://github.com/arximboldi/mixxx/blob/5e9b7e0e6892066def0cb0647e562f0fb6301e76/src/controllers/midi/midicontroller.cpp#L390
where MIDI_NOTE_OFF becomes MIDI_NOTE_ON?

The button itself should already accept MIDI_NOTE_OFF

mixxx/src/control/controlbehavior.cpp

Line 282 in 43434f9

if (o == MIDI_NOTE_OFF || dParam == 0) {

---

in oder to distribute you changes with Mixxx, we need your permission.
please sign:
https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
and comment here when done.

[arximboldi]

@daschuer I considered that fix but I was afraid such change would cause unexpected problems. Would you prefer that instead? I can try it out and submit it if you want.

[arximboldi]

Ok, I just tried what you suggested and it worked (makes tests pass, and makes my scripts work). It is unclear to me what the best option is, let me know if you'd prefer me to submit that version.

Btw, I also signed the CLA.

[arximboldi]

Oh no! Your suggested fix actually breaks toggle buttons. So I'd rather leave the change as is.

[daschuer]

Strange, why it does brake the toggle button? It should act on the same "pressed" value as the other buttons inside ControlPushButtonBehavior::setValueFromMidiParameter()

After investigates this a bit more, it looks like the <options><Button/></options> and <options><Switch/></options> are obsolete, and could be removed.
The surrounding code does not makes sense, since we have now the behavior classes.

The documentation is also hard to understand:
http://www.mixxx.org/wiki/doku.php/midi_controller_mapping_file_format

Does your mapping work without them?

Since setValueFromMidiParameter() takes the opcode and a value I would prefer to pass unchanged data to it and fix possible button errors there. All other controller types should benefit from it.
Ironing the input data like "opCode = MIDI_NOTE_ON" looks somehow ugly.

Setting <options> should only be required for fixing Hardware issues, like a button that does not emit a message on release.

@Pegasus-RPG @Be-ing What do you think?

[arximboldi]

Aha!

Yes, indeed removing the <button> option did the trick. I know understand a bit further the changes that when on with the new ControlBehavior stuff. I guess then the documentation should be updated to indicated how these flags are deprecated and giving a bit more clarity on what the intended use is.

[Be-ing]

It's a wiki, so please feel free to edit the documentation yourself:
http://mixxx.org/wiki/doku.php/midi_controller_mapping_file_format#input_options

[arximboldi]

It's a wiki, so please feel free to edit the documentation yourself: http://mixxx.org/wiki/doku.php/midi_controller_mapping_file_format#input_options

I'll try once I understand better what did actually change with 2.0. I am currently working on upgrading Mixco [1] to work properly with 2.0 and I feel that I need to dig deeper on this concept of behaviors... [1] https://github.com/arximboldi/mixco

[daschuer]

@Be-ing: From my point of view, we can remove the "Button" option, completely. I cannot see a benefit from it. Do you agree?

The same might be true for the "Switch" option. I would say it is broken, however the option is uses in some of our mappings, so it cannot be that broken. Do you have clue how it works, or if it is actually broken but no one has noticed it?

[Pegasus-RPG]

The intent of those options is to be able to match desired CO behavior with what the physical controls do without scripting. (They existed even before we had scripting.) Consider all of the possible cases: buttons that send note-on on press, note-off on release; note-on velocity != 0 on press, note-on velocity == 0 on release; and switches or physical toggle buttons that could send any of the above at any time, appearing like a button that was held down indefinitely. All of these need to be mappable to all types of (on/off) COs. These options allow that. The tests should cover each of these cases. (I don't have time to review them myself right now.)

[arximboldi]

That is the way I understood it. It seems though that some changes have made button in particular mostly obsolete, since the PushControlBehavior takes into account all sensible ways a button could behave. In fact, as this PR shows, button is now broken for previously working scripts.

So for now I just stopped using it. I managed to update my scripts without it, see #1016.

It is still unclear to me whether this pull request should be closed, or a workaround for button so old scripts are not broken should still be included. Feel free to close it if you feel like.

Thanks for insights!

[daschuer]

The "Button" parameter now means: "Ignore opcode, use Value > 0 as pressed"

I have updated:
http://mixxx.org/wiki/doku.php/midi_controller_mapping_file_format?&#input_options

Since it should work without this parameter for all outlined cases by @Pegasus-RPG, we could just remove it.

Any objections?

If we notice later, we need this "feature", we should reintroduce it using a better name.

So if you like, you can do this In this branch, ... or just close it and ignore the issue.

[Be-ing]

Let's not remove it to maintain backwards compatibility. Better to keep it in a working state and mark it as deprecated on the wiki and in the source code.

[Be-ing]

Oh, are you saying that old mappings using <button /> will work now without any code to do anything special for the option?

[daschuer]

Oh, are you saying that old mappings using will work now without any code to do anything special for the option?

Yes, <button/> can be removed from the mappings. The hypothetical case where it is still required, is a button that always sends MIDI_NOTE_OFF along with the value. I consider such a hardware as broken.

[JosepMaJAZ]

The way I understand those MIDI mappings is as follows:

A button is something for what the controller will report when it is pressed (buttondown) and when it is released (buttonup).
A Switch is something that only reports the value, without reporting a "release". Usually, the controller itself maintains the state, and only reports the change to the host. (A shift button, a "mic on" button, etc.)

Letting the host know how the controller will report the values allows it to know if it has to ignore the release. I am unsure if a controller would opt to not send the release in case of a button.

How those differ from the "normal" setting, i don't know for sure.

As for the value, I think I had read somewhere that some controllers do not report note off, but a note on with velocity 0, and that was the reason why only the velocity was taken into consideration.
I guess you need a way to differentiate between a button press and a button release, and then simply translate all the note off messages to release, as well as the note on with velocity 0.

[daschuer]

The "Switch" and "Button" options inside the mapping do not effect the messages the controller sends.
They "normally" send at least a message on press, release.

Mixxx is able to map those controls without any extra options (Switch/Button), because it knows from the type of connected ControlObject how it is intended to behave.

These options are only required to map buttons that do not behave as expected. They act as filter for the incoming messages like this:

• Button: Ignore opcode, use value > 0 as “pressed”
• Switch: Ignore opcode and value, all messages are used as “pressed”

The real issue here, that the naming of the options leads to the wrong assumption, that any switch and button mapping needs to have this value set.

Renaming them to:

• Button -> ButtonValueOnly
• Switch -> AllMessagesAsPressed

would help, but this will break existing mappings, if this feature is really required.

IMHO nothing of this should be really required.
Because of the leak of testing devices, we can only verify this, by removing the option and hope no one is complaining about a broken mapping.

[rryan]

Sounds like this PR is no longer active, closing.