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.

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

Bug in midiToInput & 1500+ unit tests #1168

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bug in midiToInput & 1500+ unit tests #1168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da2360a
test: MIDI plugin: Created 1268 tests for function midiToInput()
m-dhooge Dec 13, 2018
562ce11
test: MIDI plugin: Created 275 tests for function feedbackToMidi()
m-dhooge Dec 17, 2018
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
9 changes: 4 additions & 5 deletions plugins/midi/src/common/midiprotocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ bool QLCMIDIProtocol::midiToInput(uchar cmd, uchar data1, uchar data2,
uchar midi_ch = MIDI_CH(cmd);

/* Check that the command came on the correct MIDI channel */
if (midiChannel <= 0xF && midi_ch != midiChannel)
if (midiChannel < MAX_MIDI_CHANNELS && midi_ch != midiChannel)
return false;

switch(MIDI_CMD(cmd))
Expand All @@ -67,8 +67,8 @@ bool QLCMIDIProtocol::midiToInput(uchar cmd, uchar data1, uchar data2,
break;

case MIDI_PROGRAM_CHANGE:
*channel = CHANNEL_OFFSET_PROGRAM_CHANGE + quint32(data1);
*value = MIDI2DMX(data2);
*channel = CHANNEL_OFFSET_PROGRAM_CHANGE;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is plain wrong. You probably haven't invested enough time to understand how the MIDI plugin works.
See https://www.qlcplus.org/docs/html_en_EN/midiplugin.html - QLC+ Channels map

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did… by using what I believe IS the MIDI reference!
https://www.midi.org/specifications-old/item/table-1-summary-of-midi-message

cmd = 1100nnnn
data1 = 0ppppppp
data2 = NONE
Program Change. This message sent when the patch number changes. (ppppppp) is the new program number.

And furthermore, if you look at feedbackToMidi(), it only uses data1.

M.

Copy link
Owner

@mcallegari mcallegari Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be:

*channel = CHANNEL_OFFSET_PROGRAM_CHANGE + quint32(data1);
*value = 0xFF;

If you remove data1 from the channel number, then all program change will be sent on the same QLC+ channel and cannot be distinguished anymore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be an option… but this isn't what you chose for feedbackToMidi ;-)
And as I wrote in another comment, it seems this isn't used by DAWs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked at the InputChannelEditor UI. And your above proposal, with value = 0xFF makes sense. Otherwise, there is a huge gap in the QLC channel numbers… as with the Midi Beat (between 513 & 529).
I can come back with a new PR, with feedbackToMidi in sync with your above comment. And all the unit tests adapted.
M.

*value = MIDI2DMX(data1);
break;

case MIDI_CHANNEL_AFTERTOUCH:
Expand Down Expand Up @@ -157,8 +157,7 @@ bool QLCMIDIProtocol::feedbackToMidi(quint32 channel, uchar value,
*data1 = static_cast <uchar> (channel - CHANNEL_OFFSET_NOTE_AFTERTOUCH);
*data2 = DMX2MIDI(value);
}
else if (channel >= CHANNEL_OFFSET_PROGRAM_CHANGE &&
channel <= CHANNEL_OFFSET_PROGRAM_CHANGE_MAX)
else if (channel == CHANNEL_OFFSET_PROGRAM_CHANGE)
{
*cmd = MIDI_PROGRAM_CHANGE | midiChannel;
*data1 = DMX2MIDI(value);
Expand Down
4 changes: 2 additions & 2 deletions plugins/midi/src/common/midiprotocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ namespace QLCMIDIProtocol

/****************************************************************************
* MIDI commands with a MIDI channel (0-16)
*
* See https://www.midi.org/specifications-old/item/table-1-summary-of-midi-message
****************************************************************************/
#define MIDI_NOTE_OFF 0x80
#define MIDI_NOTE_ON 0x90
Expand Down Expand Up @@ -137,8 +139,6 @@ namespace QLCMIDIProtocol
#define CHANNEL_OFFSET_NOTE_AFTERTOUCH_MAX 383

#define CHANNEL_OFFSET_PROGRAM_CHANGE 384
#define CHANNEL_OFFSET_PROGRAM_CHANGE_MAX 511

#define CHANNEL_OFFSET_CHANNEL_AFTERTOUCH 512
#define CHANNEL_OFFSET_PITCH_WHEEL 513

Expand Down
279 changes: 267 additions & 12 deletions plugins/midi/test/midi_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,285 @@

#include <QTest>

#define private public
#include "midi_test.h"
#include "midiprotocol.h"

#undef private

/****************************************************************************
* MIDI tests
****************************************************************************/

void Midi_Test::midiToInput_data()
{
QTest::addColumn<uchar>("cmd");
QTest::addColumn<uchar>("data1");
QTest::addColumn<uchar>("data2");
QTest::addColumn<quint32>("channel_expected");
QTest::addColumn<uchar>("value_expected");

uchar cmd;
quint32 channel;

// Note OFF 0x80
// Channel = data1
// Value = 0, whatever is the velocity (data2)
cmd = MIDI_NOTE_OFF;
channel = CHANNEL_OFFSET_NOTE;
for( uchar i = 0; i < 128; ++i )
QTest::addRow("Note OFF key=%d vel=%d", i, i) << cmd << i << i << channel + i << (uchar) 0;

// Note ON 0x90
// Channel = data1
// Value = data2 * 2 (and 255 for data2=127)
cmd = MIDI_NOTE_ON;
channel = CHANNEL_OFFSET_NOTE;
for( uchar i = 0; i < 128; ++i ) {
uchar tmp2 = 127 - i;
QTest::addRow("Note ON key=%d vel=%d", i, tmp2) << cmd << i << tmp2 << channel + i << midi7b_to_dmx(tmp2);
}

// Note Aftertouch 0xA0
// Channel = data1
// Value = data2 * 2 (and 255 for data2=127)
cmd = MIDI_NOTE_AFTERTOUCH;
channel = CHANNEL_OFFSET_NOTE_AFTERTOUCH;
for( uchar i = 0; i < 128; ++i )
QTest::addRow("Note Aftertouch key=%d vel=%d", i, i) << cmd << i << i << channel + i << midi7b_to_dmx(i);

// Control Change 0xB0
// Channel = data1
// Value = data2 * 2 (and 255 for data2=127)
cmd = MIDI_CONTROL_CHANGE;
channel = CHANNEL_OFFSET_CONTROL_CHANGE;
for( uchar i = 0; i < 128; ++i ) {
uchar tmp2 = 127 - i;
QTest::addRow("CC ctrl=%d val=%d", i, tmp2) << cmd << i << tmp2 << channel + i << midi7b_to_dmx(tmp2);
}

// Program Change 0xC0
// value = data1 * 2 (and 255 for data2=127)
cmd = MIDI_PROGRAM_CHANGE;
channel = CHANNEL_OFFSET_PROGRAM_CHANGE;
for( uchar i = 0; i < 128; ++i ) {
uchar tmp2 = 127 - i;
QTest::addRow("Program Change prg=%d", i) << cmd << i << tmp2 << channel << midi7b_to_dmx(i);
}

// Channel Aftertouch 0xD0
// value = data1 * 2 (and 255 for data2=127)
cmd = MIDI_CHANNEL_AFTERTOUCH;
channel = CHANNEL_OFFSET_CHANNEL_AFTERTOUCH;
for( uchar i = 0; i < 128; ++i ) {
uchar tmp2 = 127 - i;
QTest::addRow("Channel Aftertouch val=%d", i) << cmd << i << tmp2 << channel << midi7b_to_dmx(i);
}

// Pitch Bend 0xE0
// value = data1 for LSB & data2 for MSB
cmd = MIDI_PITCH_WHEEL;
channel = CHANNEL_OFFSET_PITCH_WHEEL;
for( int i = 0; i < 0x4000; i+=100 ) {
uchar data1 = i & 0x7F;
uchar data2 = i >> 7;
QTest::addRow("Pitch Wheel val=%d", i) << cmd << data1 << data2 << channel << midi14b_to_dmx(i);
}

}

void Midi_Test::midiToInput()
{
quint32 channel = 0;
uchar value = 0;
quint32 channel_result;
uchar value_result;

QFETCH(uchar, cmd);
QFETCH(uchar, data1);
QFETCH(uchar, data2);
QFETCH(quint32, channel_expected);
QFETCH(uchar, value_expected);

QVERIFY2(
QLCMIDIProtocol::midiToInput(
cmd, data1, data2, 0 /* midiChannel */, &channel_result, &value_result),
"Expecting valid answer from midiToInput");

QCOMPARE(channel_result, channel_expected);
QCOMPARE(value_result, value_expected);
}



void Midi_Test::feedbackToMidi_data()
{
QTest::addColumn<quint32>("qlcChannel");
QTest::addColumn<uchar>("qlcValue");
QTest::addColumn<uchar>("midiChannel");
QTest::addColumn<bool>("sendNoteOff");
QTest::addColumn<uchar>("cmd_expected");
QTest::addColumn<uchar>("data1_expected");
QTest::addColumn<uchar>("data2_expected"); // 255 => data2 byte not used by MIDI command

for(quint32 qlcChannel = 0; qlcChannel < MAX_MIDI_CHANNELS; qlcChannel +=6)
{
for(uchar midiChannel = 0; midiChannel <= MAX_MIDI_CHANNELS; midiChannel += 4)
{
uchar qlcValue = 7 + 3 * qlcChannel + 3 * midiChannel;

for(uchar key = qlcChannel * 3 + midiChannel * 5; key < 128; key += 27)
{
QTest::addRow("Note OFF key=%d qlc=%d midi=%d", key, qlcChannel, midiChannel)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_NOTE + key << uchar(0) << midiChannel << true
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_NOTE_OFF)
<< uchar(key) << uchar(0);

QTest::addRow("Note ON key=%d qlc=%d midi=%d val=0", key, qlcChannel, midiChannel)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_NOTE + key << uchar(0) << midiChannel << false
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_NOTE_ON)
<< uchar(key) << uchar(0);

QTest::addRow("Note ON key=%d qlc=%d midi=%d val=%d", key, qlcChannel, midiChannel, qlcValue)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_NOTE + key << qlcValue << midiChannel << true
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_NOTE_ON)
<< uchar(key) << dmx_to_midi7b(qlcValue);

QTest::addRow("Note Aftertouch key=%d qlc=%d midi=%d val=%d", key, qlcChannel, midiChannel, qlcValue)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_NOTE_AFTERTOUCH + key << qlcValue << midiChannel << false
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_NOTE_AFTERTOUCH)
<< uchar(key) << dmx_to_midi7b(qlcValue);

QTest::addRow("Control Change ctrl=%d qlc=%d midi=%d val=%d", key, qlcChannel, midiChannel, qlcValue)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_CONTROL_CHANGE + key << qlcValue << midiChannel << true
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_CONTROL_CHANGE)
<< uchar(key) << dmx_to_midi7b(qlcValue);
}

QTest::addRow("Program Change prg=%d qlc=%d midi=%d", qlcValue, qlcChannel, midiChannel)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_PROGRAM_CHANGE << qlcValue << midiChannel << false
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_PROGRAM_CHANGE)
<< dmx_to_midi7b(qlcValue) << uchar(255);

QTest::addRow("Channel Aftertouch val=%d qlc=%d midi=%d", qlcValue, qlcChannel, midiChannel)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_CHANNEL_AFTERTOUCH << qlcValue << midiChannel << true
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_CHANNEL_AFTERTOUCH)
<< dmx_to_midi7b(qlcValue) << uchar(255);

QTest::addRow("Pitch Bend val=%d qlc=%d midi=%d", qlcValue, qlcChannel, midiChannel)
<< qlcChannel * OmniChannelOffset + CHANNEL_OFFSET_PITCH_WHEEL << qlcValue << midiChannel << false
<< qlc_to_midiCmd(qlcChannel, midiChannel, MIDI_PITCH_WHEEL)
<< uchar((qlcValue & 0x01) << 6) << uchar(qlcValue >> 1);

++qlcValue;
}
}

}

void Midi_Test::feedbackToMidi()
{
uchar cmd_result, data1_result, data2_result;

QFETCH(quint32, qlcChannel);
QFETCH(uchar, qlcValue);
QFETCH(uchar, midiChannel);
QFETCH(bool, sendNoteOff);
QFETCH(uchar, cmd_expected);
QFETCH(uchar, data1_expected);
QFETCH(uchar, data2_expected);
if(data2_expected == 255) // data2 byte not used by MIDI command
data2_result = data2_expected;

QVERIFY2(
QLCMIDIProtocol::feedbackToMidi(
qlcChannel, qlcValue, midiChannel, sendNoteOff,
&cmd_result, &data1_result, &data2_result),
"Expecting valid answer from feedbackToMidi");

QCOMPARE(cmd_result, cmd_expected);
QCOMPARE(data1_result, data1_expected);
QCOMPARE(data2_result, data2_expected);
}



void Midi_Test::midiToInput_midiChannels_data()
{
QTest::addColumn<uchar>("cmd");
QTest::addColumn<uchar>("data1");
QTest::addColumn<uchar>("data2");
QTest::addColumn<uchar>("midiChannel");
QTest::addColumn<quint32>("qlc_channel_expected");
QTest::addColumn<int>("qlc_value_expected");

for(uchar midiChannel = 0; midiChannel < 16; ++midiChannel)
{
uchar data1 = midiChannel * 7;
uchar data2 = midiChannel * 3;

for(uchar qlcMidiChannel = 2; qlcMidiChannel <= 16; qlcMidiChannel+=7)
{
QTest::addRow("Note OFF midi=%d qlc=%d", midiChannel, qlcMidiChannel)
<< uchar(MIDI_NOTE_OFF + midiChannel) << data1 << data2 << qlcMidiChannel
<< midiChannel_to_qlcChannel( midiChannel, qlcMidiChannel, CHANNEL_OFFSET_NOTE + data1++ )
<< midiChannel_to_qlcValue( midiChannel, qlcMidiChannel, 0 );

QTest::addRow("Note ON midi=%d qlc=%d", midiChannel, qlcMidiChannel)
<< uchar(MIDI_NOTE_ON + midiChannel) << data1 << data2 << qlcMidiChannel
<< midiChannel_to_qlcChannel( midiChannel, qlcMidiChannel, CHANNEL_OFFSET_NOTE + data1++ )
<< midiChannel_to_qlcValue( midiChannel, qlcMidiChannel, midi7b_to_dmx(data2++) );

QTest::addRow("Note Aftertouch midi=%d qlc=%d", midiChannel, qlcMidiChannel)
<< uchar(MIDI_NOTE_AFTERTOUCH + midiChannel) << data1 << data2 << qlcMidiChannel
<< midiChannel_to_qlcChannel( midiChannel, qlcMidiChannel, CHANNEL_OFFSET_NOTE_AFTERTOUCH + data1++ )
<< midiChannel_to_qlcValue( midiChannel, qlcMidiChannel, midi7b_to_dmx(data2++) );

QTest::addRow("CC midi=%d qlc=%d", midiChannel, qlcMidiChannel)
<< uchar(MIDI_CONTROL_CHANGE + midiChannel) << data1 << data2 << qlcMidiChannel
<< midiChannel_to_qlcChannel( midiChannel, qlcMidiChannel, CHANNEL_OFFSET_CONTROL_CHANGE + data1++ )
<< midiChannel_to_qlcValue( midiChannel, qlcMidiChannel, midi7b_to_dmx(data2++) );

QTest::addRow("Program Change midi=%d qlc=%d", midiChannel, qlcMidiChannel)
<< uchar(MIDI_PROGRAM_CHANGE + midiChannel) << data1 << data2++ << qlcMidiChannel
<< midiChannel_to_qlcChannel( midiChannel, qlcMidiChannel, CHANNEL_OFFSET_PROGRAM_CHANGE )
<< midiChannel_to_qlcValue( midiChannel, qlcMidiChannel, midi7b_to_dmx(data1++) );

QTest::addRow("Channel Aftertouch midi=%d qlc=%d", midiChannel, qlcMidiChannel)
<< uchar(MIDI_CHANNEL_AFTERTOUCH + midiChannel) << data1 << data2++ << qlcMidiChannel
<< midiChannel_to_qlcChannel( midiChannel, qlcMidiChannel, CHANNEL_OFFSET_CHANNEL_AFTERTOUCH )
<< midiChannel_to_qlcValue( midiChannel, qlcMidiChannel, midi7b_to_dmx(data1++) );

int val = data2 << 7 | data1;
QTest::addRow("Pitch Wheel midi=%d qlc=%d", midiChannel, qlcMidiChannel)
<< uchar(MIDI_PITCH_WHEEL + midiChannel) << data1 << data2 << qlcMidiChannel
<< midiChannel_to_qlcChannel( midiChannel, qlcMidiChannel, CHANNEL_OFFSET_PITCH_WHEEL )
<< midiChannel_to_qlcValue( midiChannel, qlcMidiChannel, midi14b_to_dmx(val) );
}
}
}

void Midi_Test::midiToInput_midiChannels()
{
quint32 channel_result;
uchar value_result;

uchar midiChannel = 7;
uchar cmd = MIDI_NOTE_ON | midiChannel;
uchar data1 = 10;
uchar data2 = 127;
QFETCH(uchar, cmd);
QFETCH(uchar, data1);
QFETCH(uchar, data2);
QFETCH(uchar, midiChannel);
QFETCH(quint32, qlc_channel_expected);
QFETCH(int, qlc_value_expected);

QLCMIDIProtocol::midiToInput(cmd, data1, data2, midiChannel, &channel, &value);
bool result = QLCMIDIProtocol::midiToInput(
cmd, data1, data2, midiChannel,
&channel_result, &value_result);

QCOMPARE(channel, 138U);
QCOMPARE(value, uchar(255U));
if(qlc_value_expected >= 0)
{
QVERIFY2(result, "Expecting valid answer from midiToInput");
QCOMPARE(channel_result, qlc_channel_expected);
QCOMPARE(value_result, qlc_value_expected);
}
else
{
QVERIFY2(!result, "Expecting midiToInput to ignore the call");
}
}

QTEST_MAIN(Midi_Test)
45 changes: 43 additions & 2 deletions plugins/midi/test/midi_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,51 @@

class Midi_Test : public QObject
{
Q_OBJECT
Q_OBJECT

public:

static inline
uchar midi7b_to_dmx( int in )
{ return in >= 127 ? 255 : in * 2; }
static inline
uchar midi14b_to_dmx( int in )
{ return in >> 6; }

static inline
uchar dmx_to_midi7b( int in )
{ return in / 2; }


static const quint32 OmniChannelOffset = 1 << 12;

inline static
quint32 midiChannel_to_qlcChannel(uchar midi, uchar qlc, quint32 offset)
{ return offset + (qlc == 16 ? midi * OmniChannelOffset : 0); }

inline static
int midiChannel_to_qlcValue(uchar midi, uchar qlc, uchar value)
{ return (qlc == 16 || qlc == midi) ? value : -1; }

inline static
uchar qlc_to_midiCmd(quint32 qlcChannel, uchar midiChannel, uchar cmd)
{ return cmd + (midiChannel == 16 ? qlcChannel : midiChannel); }

private slots:
void midiToInput();

// Tests of data bytes conversion
void midiToInput_data();
void midiToInput();

void feedbackToMidi_data();
void feedbackToMidi();

// Tests of MIDI Channel filtering
void midiToInput_midiChannels_data();
void midiToInput_midiChannels();

// TODO Tests of System Common Messages

};

#endif