qlcplus icon indicating copy to clipboard operation
qlcplus copied to clipboard

Bug in midiToInput & 1500+ unit tests

Open m-dhooge opened this issue 6 years ago • 4 comments
trafficstars

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 avatar Dec 17 '18 10:12 m-dhooge

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

m-dhooge avatar Dec 17 '18 10:12 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.

m-dhooge avatar Dec 17 '18 12:12 m-dhooge

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.

mcallegari avatar Dec 17 '18 12:12 mcallegari

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.

m-dhooge avatar Dec 17 '18 13:12 m-dhooge