Bug in midiToInput & 1500+ unit tests

[m-dhooge]

Hello,

There is a bug in QLCMIDIProtocol::midiToInput(): It expects 2 data bytes but according to the MIDI spec only 1 byte is provided. Note that the reverse function (feedbackToMidi) correctly uses only the first byte.

I also produced some unit tests...

M.

[m-dhooge]

...for the MIDI_PROGRAM_CHANGE command. Other commands are valid.

[mcallegari]

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

[m-dhooge]

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.

[mcallegari]

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.

[m-dhooge]

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.

[m-dhooge]

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.

[m-dhooge]

I didn't try to recompile the whole project… and CHANNEL_OFFSET_PROGRAM_CHANGE_MAX is used by InputChannelEditor::numberToMidi :-(
I'll update my code with something that wholly compiles.

[mcallegari]

Ok, let's take a step back then.
What is the reason why this patch is needed ? Cause last time I checked, program changes were working.
Please indicate step by step what you did, and the equipment you used. Thanks.

[m-dhooge]

I have a problem with a Behringer X-Touch Compact -- but this isn't directly related to this patch.
However, I wanted to learn a bit the code before diving into my problem; so I started writing some unit tests for the midiprotocol.cpp functions. And this is how I discovered there is an inconsistency between both functions (midiToInput & feedbackToMidi).

Note that I just grep-ed through all the profile files in resources/inputprofiles for any value between 256 and 513 as the channel number (i.e. Note aftertouch, Program Change, Channel aftertouch & Pitch Wheel). Only Pitch Wheel is used, by Behringer-BCF2000inMackieControlMode.qxi & Zoom-R16.qxi.

So I believe no manufacturers use PC within their DAW… So there is no real reason -- so far -- to apply this patch. But there is a bug in midiToInput because data2 isn't defined in the case of PC. Or this is feedbackToMidi that is wrong (because it uses data1) -- but I think midi.org is accurate.

My 2 cents.