Skip to content

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

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.

Sign up for GitHub

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

Closed
arximboldi wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/controllers/midi/midicontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ void MidiController::processInputMapping(const MidiInputMapping& mapping,
newValue = math_min(newValue, 127.0);
} else {
double currControlValue = pCO->getMidiParameter();
newValue = computeValue(mapping.options, currControlValue, value);
newValue = computeValue(mapping.options, opCode, currControlValue, value);
}

// ControlPushButton ControlObjects only accept NOTE_ON, so if the midi
Expand All @@ -400,7 +400,7 @@ void MidiController::processInputMapping(const MidiInputMapping& mapping,
pCO->setValueFromMidi(static_cast<MidiOpCode>(opCode), newValue);
}

double MidiController::computeValue(MidiOptions options, double _prevmidivalue, double _newmidivalue) {
double MidiController::computeValue(MidiOptions options, unsigned char opcode, double _prevmidivalue, double _newmidivalue) {
double tempval = 0.;
double diff = 0.;

Expand Down Expand Up @@ -456,7 +456,7 @@ double MidiController::computeValue(MidiOptions options, double _prevmidivalue,
}

if (options.button) {
_newmidivalue = _newmidivalue != 0;
_newmidivalue = opcode != MIDI_NOTE_OFF && _newmidivalue != 0;
}

if (options.sw) {
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/midi/midicontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class MidiController : public Controller {
mixxx::Duration timestamp);

virtual void sendWord(unsigned int word) = 0;
double computeValue(MidiOptions options, double _prevmidivalue, double _newmidivalue);
double computeValue(MidiOptions options, unsigned char opcode, double _prevmidivalue, double _newmidivalue);
void createOutputHandlers();
void updateAllOutputs();
void destroyOutputHandlers();
Expand Down
189 changes: 188 additions & 1 deletion src/test/midicontrollertest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,34 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_PushOnOff) {
EXPECT_DOUBLE_EQ(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_PushOnOffNonZero) {
// Some MIDI controller send push-buttons as (NOTE_ON, 0x7F) for press and
// with non-zero velocy e.g. (NOTE_OFF, 0x42) for release.
ConfigKey key("[Channel1]", "hotcue_1_activate");
ControlPushButton cpb(key);

unsigned char channel = 0x01;
unsigned char control = 0x10;

addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_ON | channel, control),
MidiOptions(), key));
addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_OFF | channel, control),
MidiOptions(), key));
loadPreset(m_preset);

// Receive an on/off, sets the control on/off with each press.
receive(MIDI_NOTE_ON | channel, control, 0x7F);
EXPECT_LT(0.0, cpb.get());
receive(MIDI_NOTE_OFF | channel, control, 0x42);
EXPECT_DOUBLE_EQ(0.0, cpb.get());

// Receive an on/off, sets the control on/off with each press.
receive(MIDI_NOTE_ON | channel, control, 0x7F);
EXPECT_LT(0.0, cpb.get());
receive(MIDI_NOTE_OFF | channel, control, 0x42);
EXPECT_DOUBLE_EQ(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_PushOnOn) {
// Some MIDI controllers send push-buttons as (NOTE_ON, 0x7f) for press and
// (NOTE_ON, 0x00) for release.
Expand Down Expand Up @@ -134,6 +162,37 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_ToggleOnOff_ButtonMidiOpt
EXPECT_DOUBLE_EQ(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_ToggleOnOffNonZero_ButtonMidiOption) {
// Using the button MIDI option allows you to use a MIDI toggle button as a
// push button.
ConfigKey key("[Channel1]", "hotcue_1_activate");
ControlPushButton cpb(key);

unsigned char channel = 0x01;
unsigned char control = 0x10;

MidiOptions options;
options.button = true;

addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_ON | channel, control),
options, key));
addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_OFF | channel, control),
options, key));
loadPreset(m_preset);

// NOTE(rryan): This behavior is broken!

// Toggle the switch on, sets the push button on.
receive(MIDI_NOTE_ON | channel, control, 0x7F);
EXPECT_LT(0.0, cpb.get());

// The push button is stuck down here!

// Toggle the switch off, sets the push button off.
receive(MIDI_NOTE_OFF | channel, control, 0x42);
EXPECT_DOUBLE_EQ(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_ToggleOnOff_SwitchMidiOption) {
// Using the switch MIDI option interprets a MIDI toggle button as a toggle
// button rather than a momentary push button.
Expand Down Expand Up @@ -183,6 +242,55 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_ToggleOnOff_SwitchMidiOpt
// this.
}

TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_ToggleOnOffNonZero_SwitchMidiOption) {
// Using the switch MIDI option interprets a MIDI toggle button as a toggle
// button rather than a momentary push button.
ConfigKey key("[Channel1]", "hotcue_1_activate");
ControlPushButton cpb(key);

unsigned char channel = 0x01;
unsigned char control = 0x10;

MidiOptions options;
options.sw = true;

addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_ON | channel, control),
options, key));
addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_OFF | channel, control),
options, key));
loadPreset(m_preset);

// NOTE(rryan): This behavior is broken!

// Toggle the switch on, sets the push button on.
receive(MIDI_NOTE_ON | channel, control, 0x7F);
EXPECT_LT(0.0, cpb.get());

// The push button is stuck down here!

// Toggle the switch off, sets the push button on again.
receive(MIDI_NOTE_OFF | channel, control, 0x42);
EXPECT_LT(0.0, cpb.get());

// NOTE(rryan): What is supposed to happen in this case? It's an open
// question I think. I think if you want to connect a switch MIDI control to
// a push button CO then the switch should directly set the CO. After all,
// the preset author asked for the switch to be interpreted as a switch. If
// they want the switch to act like a push button, they should use the
// button MIDI option.
//
// Most of our push buttons trigger behavior on press and do nothing on
// release, and most don't care about being "stuck down" except for hotcue
// and cue controls that have preview behavior.

// "reverse" is an example of a push button that is a push button because we
// want the default behavior to be momentary press and not toggle. If I
// mapped a switch to it, I would expect the switch to enable it (set it to
// 1) when the switch was enabled and set it to 0 when the switch was
// disabled. So I think we should change the switch option to behave like
// this.
}

TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_PushCC) {
// Some MIDI controllers (e.g. Korg nanoKONTROL) send momentary push-buttons
// as (CC, 0x7f) for press and (CC, 0x00) for release.
Expand Down Expand Up @@ -238,6 +346,35 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_PushOnOff) {
EXPECT_DOUBLE_EQ(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_PushOnOffNonZero) {
// Some MIDI controller send push-buttons as (NOTE_ON, 0x7F) for press and
// e.g. (NOTE_OFF, 0x42) for release.
ConfigKey key("[Channel1]", "keylock");
ControlPushButton cpb(key);
cpb.setButtonMode(ControlPushButton::TOGGLE);

unsigned char channel = 0x01;
unsigned char control = 0x10;

addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_ON | channel, control),
MidiOptions(), key));
addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_OFF | channel, control),
MidiOptions(), key));
loadPreset(m_preset);

// Receive an on/off, toggles the control.
receive(MIDI_NOTE_ON | channel, control, 0x7F);
receive(MIDI_NOTE_OFF | channel, control, 0x42);

EXPECT_LT(0.0, cpb.get());

// Receive an on/off, toggles the control.
receive(MIDI_NOTE_ON | channel, control, 0x7F);
receive(MIDI_NOTE_OFF | channel, control, 0x42);

EXPECT_DOUBLE_EQ(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_PushOnOn) {
// Some MIDI controllers send push-buttons as (NOTE_ON, 0x7f) for press and
// (NOTE_ON, 0x00) for release.
Expand Down Expand Up @@ -295,7 +432,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_ToggleOnOff_ButtonMidiOption)

// Toggle the switch off, since it is interpreted as a button release it
// does nothing to the toggle button.
receive(MIDI_NOTE_OFF | channel, control, 0x00);
receive(MIDI_NOTE_OFF | channel, control, 0x42);
EXPECT_LT(0.0, cpb.get());
}

Expand Down Expand Up @@ -349,6 +486,56 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_ToggleOnOff_SwitchMidiOption)
EXPECT_LT(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_ToggleOnOffNonZero_SwitchMidiOption) {
// Using the switch MIDI option interprets a MIDI toggle button as a toggle
// button rather than a momentary push button.
ConfigKey key("[Channel1]", "keylock");
ControlPushButton cpb(key);
cpb.setButtonMode(ControlPushButton::TOGGLE);

unsigned char channel = 0x01;
unsigned char control = 0x10;

MidiOptions options;
options.sw = true;

addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_ON | channel, control),
options, key));
addMapping(MidiInputMapping(MidiKey(MIDI_NOTE_OFF | channel, control),
options, key));
loadPreset(m_preset);

// NOTE(rryan): If the intended behavior of switch MIDI option is to make a
// toggle MIDI button act like a toggle button then this isn't working. The
// toggle on presses the CO and the toggle off presses the CO. This toggles
// the control but allows it to get out of sync.

// Toggle the switch on, since it is interpreted as a button press it
// toggles the control on.
receive(MIDI_NOTE_ON | channel, control, 0x7F);
EXPECT_LT(0.0, cpb.get());

// Toggle the switch off, since it is interpreted as a button press it
// toggles the control off.
receive(MIDI_NOTE_OFF | channel, control, 0x42);
EXPECT_DOUBLE_EQ(0.0, cpb.get());

// Meanwhile, the GUI toggles the control on again.
// NOTE(rryan): Now the MIDI toggle button is out of sync with the toggle
// CO.
cpb.set(1.0);

// Toggle the switch on, since it is interpreted as a button press it
// toggles the control off (since it was on).
receive(MIDI_NOTE_ON | channel, control, 0x7F);
EXPECT_DOUBLE_EQ(0.0, cpb.get());

// Toggle the switch off, since it is interpreted as a button press it
// toggles the control on (since it was off).
receive(MIDI_NOTE_OFF | channel, control, 0x42);
EXPECT_LT(0.0, cpb.get());
}

TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_PushCC) {
// Some MIDI controllers (e.g. Korg nanoKONTROL) send momentary push-buttons
// as (CC, 0x7f) for press and (CC, 0x00) for release.
Expand Down