diff --git a/src/controllers/midi/midicontroller.cpp b/src/controllers/midi/midicontroller.cpp index 83ab750089ed..efad5519c579 100644 --- a/src/controllers/midi/midicontroller.cpp +++ b/src/controllers/midi/midicontroller.cpp @@ -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 @@ -400,7 +400,7 @@ void MidiController::processInputMapping(const MidiInputMapping& mapping, pCO->setValueFromMidi(static_cast(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.; @@ -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) { diff --git a/src/controllers/midi/midicontroller.h b/src/controllers/midi/midicontroller.h index 4f1ef75c6ccb..8b174b206dd4 100644 --- a/src/controllers/midi/midicontroller.h +++ b/src/controllers/midi/midicontroller.h @@ -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(); diff --git a/src/test/midicontrollertest.cpp b/src/test/midicontrollertest.cpp index 3e628c572e85..20d68037e16a 100644 --- a/src/test/midicontrollertest.cpp +++ b/src/test/midicontrollertest.cpp @@ -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. @@ -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. @@ -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. @@ -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. @@ -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()); } @@ -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.