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

Outgoing MIDI debugging messages #1004

Merged
merged 10 commits into from
Dec 21, 2016
Merged

Outgoing MIDI debugging messages #1004

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8fcf96c
don't require length parameter for Controller::send()
Be-ing Sep 8, 2016
6aa4607
print debugging messages for outgoing MIDI from scripts
Be-ing Sep 8, 2016
258556e
MIDI output code cleanup
Be-ing Sep 9, 2016
653136a
don't do any operations for debugging MIDI when not debugging
Be-ing Sep 9, 2016
047de0b
document purpose of seemingly useless parameter
Be-ing Sep 10, 2016
f698d84
fix MIDI debugging string
Be-ing Sep 10, 2016
6033525
fix MIDI controller test builds
Be-ing Sep 11, 2016
6d9f433
merge upstream/master
Be-ing Dec 20, 2016
6e8b4d0
move formatMidiMessage() and formatSysexMessage() from MidiController…
Be-ing Dec 20, 2016
4d708cd
Merge remote-tracking branch 'upstream/master' into outgoing_midi_debug
Be-ing Dec 21, 2016
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
1 change: 1 addition & 0 deletions src/controllers/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ void Controller::send(QList<int> data, unsigned int length) {
// If you change this implementation, also change it in HidController (That
// function is required due to HID devices having report IDs)

length = data.size();
Copy link
Member

Choose a reason for hiding this comment

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

length is not used before, I think you can remove the parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is Q_INVOKABLE. The length parameter has been required for scripts, so I'm leaving it for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment to clarify that.

Copy link
Member

Choose a reason for hiding this comment

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

aaaa. wait. or you could use the minimum of both, to keep the old functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the length parameter should ever be different from data.size(). It would be an easy mistake pass the wrong length and confuse PortMidi or another backend.

QByteArray msg(length, 0);
for (unsigned int i = 0; i < length; ++i) {
msg[i] = data.at(i);
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class Controller : public QObject, ConstControllerPresetVisitor {
void stopLearning();

protected:
Q_INVOKABLE void send(QList<int> data, unsigned int length);
Q_INVOKABLE void send(QList<int> data, unsigned int length = 0);

// To be called in sub-class' open() functions after opening the device but
// before starting any input polling/processing.
Expand Down
16 changes: 10 additions & 6 deletions src/controllers/midi/hss1394controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* @brief HSS1394-based MIDI backend
*/

#include "controllers/midi/midiutils.h"
#include "controllers/midi/hss1394controller.h"
#include "controllers/controllerdebug.h"
#include "util/time.h"
Expand Down Expand Up @@ -168,18 +169,20 @@ int Hss1394Controller::close() {
return 0;
}

void Hss1394Controller::sendWord(unsigned int word) {
void Hss1394Controller::sendShortMsg(unsigned char status, unsigned char byte1,
unsigned char byte2) {
unsigned char channel = MidiUtils::channelFromStatus(status);
unsigned char opCode = MidiUtils::opCodeFromStatus(status);
unsigned int word = (((unsigned int)byte2) << 16) |
(((unsigned int)byte1) << 8) | status;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to get rid of this word intermediate, but I don't have any HSS1394 devices nor a Firewire port to test a change.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have it as well. Let's do pair programming:

void Hss1394Controller::sendShortMsg(
        unsigned char status, unsigned char byte1, unsigned char byte2) {
    unsigned char channel = MidiUtils::channelFromStatus(status);
    unsigned char opCode = MidiUtils::opCodeFromStatus(status);

    unsigned char data[3];
    data[0] = status;
    data[1] = byte1;
    data[2] = byte2;

    int bytesSent = m_pChannel->SendChannelBytes(data, 3);
    controllerDebug(formatMidiMessage(getName(), status, byte1, byte2,
                    channel, opCode));

    //if (bytesSent != 3) {
    //    qDebug()<<"ERROR: Sent" << bytesSent << "of 3 bytes:" << message;
    //    //m_pChannel->Flush();
    //}
}

Copy link
Member

Choose a reason for hiding this comment

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

I think we should move the formating behind the ControllerDebug::enabled() flag.
In a way, that the code runs at max speed when there is no debugging enabled.

unsigned char data[3];
data[0] = word & 0xFF;
data[1] = (word >> 8) & 0xFF;
data[2] = (word >> 16) & 0xFF;

QString message = QString("%1 %2 %3").arg(
QString("%1").arg(data[0], 2, 16, QChar('0')),
QString("%1").arg(data[1], 2, 16, QChar('0')),
QString("%1").arg(data[2], 2, 16, QChar('0')));

int bytesSent = m_pChannel->SendChannelBytes(data, 3);
controllerDebug(formatMidiMessage(getName(), status, byte1, byte2,
channel, opCode));

//if (bytesSent != 3) {
// qDebug()<<"ERROR: Sent" << bytesSent << "of 3 bytes:" << message;
Expand All @@ -191,6 +194,7 @@ void Hss1394Controller::send(QByteArray data) {
int bytesSent = m_pChannel->SendChannelBytes(
(unsigned char*)data.constData(), data.size());

controllerDebug(formatSysexMessage(getName(), data));
Copy link
Member

Choose a reason for hiding this comment

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

The same here. There should be no formating if controller debug is disabled.

//if (bytesSent != length) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pegasus-RPG why is this commented out?

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember. Am I listed as the last person to touch that line? If so, maybe the HSS1394 spec said that it wasn't implemented and always returns 0 or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that line hasn't been modified since you created the file. If the hss1394 library actually does indicate an error somehow, it would be good to handle that.

// qDebug()<<"ERROR: Sent" << bytesSent << "of" << length << "bytes (SysEx)";
// //m_pChannel->Flush();
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/midi/hss1394controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ class Hss1394Controller : public MidiController {
virtual int close();

private:
void sendWord(unsigned int word);
void sendShortMsg(unsigned char status, unsigned char byte1,
unsigned char byte2);

// The sysex data must already contain the start byte 0xf0 and the end byte
// 0xf7.
Expand Down
80 changes: 45 additions & 35 deletions src/controllers/midi/midicontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,53 +160,78 @@ void MidiController::destroyOutputHandlers() {
}
}

QString formatMidiMessage(const QString& controllerName,
unsigned char status, unsigned char control,
unsigned char value, unsigned char channel,
unsigned char opCode, mixxx::Duration timestamp) {
QString MidiController::formatMidiMessage(const QString& controllerName,
Copy link
Member

@Pegasus-RPG Pegasus-RPG Sep 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm curious why you've added the class name to the definitions. Isn't it implied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it isn't implied. That was actually confusing me for a bit before I got help in ##c++-basic on Freenode. I was getting a linker error when I added it to the class as a protected member function but didn't add the class name to the definition.

Copy link
Member

Choose a reason for hiding this comment

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

How were you able to build before then?

Copy link
Contributor Author

@Be-ing Be-ing Sep 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It wasn't in the header. It was only defined in the .cpp file, but I had to make it a protected member of the class to reuse it in PortMidiController::sendShortMsg() and Hss1394Controller::sendShortMsg().

Copy link
Member

Choose a reason for hiding this comment

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

Nit: since formatMidiMessage/formatSysexMessage don't use any member variables/methods of MidiController, they could move to MidiUtils (src/controllers/midi/midiutils.h). Would be cleaner that way.

unsigned char status, unsigned char control,
unsigned char value, unsigned char channel,
unsigned char opCode, mixxx::Duration timestamp) {
QString t;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions for a better variable name here?

Copy link
Member

Choose a reason for hiding this comment

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

Not really. .. "msg2" or something that is not "time" related .

if (timestamp == mixxx::Duration::fromMillis(0)) {
t = "outgoing:";
} else {
t = QString("t:%1").arg(timestamp.formatMillisWithUnit());
}
switch (opCode) {
case MIDI_PITCH_BEND:
return QString("%1: t:%2 status 0x%3: pitch bend ch %4, value 0x%5")
.arg(controllerName, timestamp.formatMillisWithUnit(),
QString::number(status, 16).toUpper(),
return QString("%1: %2 status 0x%3: pitch bend ch %4, value 0x%5")
.arg(controllerName, t,
QString::number(status, 16).toUpper(), t,
QString::number(channel+1, 10),
QString::number((value << 7) | control, 16).toUpper().rightJustified(4,'0'));
case MIDI_SONG_POS:
return QString("%1: t:%5 status 0x%3: song position 0x%4")
Copy link
Member

Choose a reason for hiding this comment

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

Remove the "t:" here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the %5 a typo? There are only 4 arguments. It seems it should be changed to %2.

.arg(controllerName, timestamp.formatMillisWithUnit(),
.arg(controllerName, t,
QString::number(status, 16).toUpper(),
QString::number((value << 7) | control, 16).toUpper().rightJustified(4,'0'));
case MIDI_PROGRAM_CH:
case MIDI_CH_AFTERTOUCH:
return QString("%1: t:%2 status 0x%3 (ch %4, opcode 0x%5), value 0x%6")
.arg(controllerName, timestamp.formatMillisWithUnit(),
return QString("%1: %2 status 0x%3 (ch %4, opcode 0x%5), value 0x%6")
.arg(controllerName, t,
QString::number(status, 16).toUpper(),
QString::number(channel+1, 10),
QString::number((status & 255)>>4, 16).toUpper(),
QString::number(control, 16).toUpper().rightJustified(2,'0'));
case MIDI_SONG:
return QString("%1: t:%2 status 0x%3: select song #%4")
.arg(controllerName, timestamp.formatMillisWithUnit(),
return QString("%1: %2 status 0x%3: select song #%4")
.arg(controllerName, t,
QString::number(status, 16).toUpper(),
QString::number(control+1, 10));
case MIDI_NOTE_OFF:
case MIDI_NOTE_ON:
case MIDI_AFTERTOUCH:
case MIDI_CC:
return QString("%1: t:%2 status 0x%3 (ch %4, opcode 0x%5), ctrl 0x%6, val 0x%7")
.arg(controllerName, timestamp.formatMillisWithUnit(),
return QString("%1: %2 status 0x%3 (ch %4, opcode 0x%5), ctrl 0x%6, val 0x%7")
.arg(controllerName, t,
QString::number(status, 16).toUpper(),
QString::number(channel+1, 10),
QString::number((status & 255)>>4, 16).toUpper(),
QString::number(control, 16).toUpper().rightJustified(2,'0'),
QString::number(value, 16).toUpper().rightJustified(2,'0'));
default:
return QString("%1: t:%2 status 0x%3")
.arg(controllerName, timestamp.formatMillisWithUnit(),
return QString("%1: %2 status 0x%3")
.arg(controllerName, t,
QString::number(status, 16).toUpper());
}
}

QString MidiController::formatSysexMessage(const QString& controllerName, const QByteArray& data,
mixxx::Duration timestamp) {
QString t;
if (timestamp == mixxx::Duration::fromMillis(0)) {
t = "outgoing:";
} else {
t = QString("t:%1").arg(timestamp.formatMillisWithUnit());
}
QString message = QString("%1: %2 %3 byte sysex: [")
.arg(controllerName).arg(t)
.arg(data.size());
for (int i = 0; i < data.size(); ++i) {
message += QString("%1%2").arg(
QString("%1").arg((unsigned char)(data.at(i)), 2, 16, QChar('0')).toUpper(),
QString("%1").arg((i < (data.size()-1)) ? ' ' : ']'));
}
return message;
}

void MidiController::learnTemporaryInputMappings(const MidiInputMappings& mappings) {
foreach (const MidiInputMapping& mapping, mappings) {
m_temporaryInputMappings.insert(mapping.key.key, mapping);
Expand Down Expand Up @@ -497,19 +522,6 @@ double MidiController::computeValue(MidiOptions options, double _prevmidivalue,
return _newmidivalue;
}

QString formatSysexMessage(const QString& controllerName, const QByteArray& data,
mixxx::Duration timestamp) {
QString message = QString("%1: t:%2 %3 bytes: [")
.arg(controllerName).arg(timestamp.formatMillisWithUnit())
.arg(data.size());
for (int i = 0; i < data.size(); ++i) {
message += QString("%1%2").arg(
QString("%1").arg((unsigned char)(data.at(i)), 2, 16, QChar('0')).toUpper(),
QString("%1").arg((i < (data.size()-1)) ? ' ' : ']'));
}
return message;
}

void MidiController::receive(QByteArray data, mixxx::Duration timestamp) {
controllerDebug(formatSysexMessage(getName(), data, timestamp));
Copy link
Member

Choose a reason for hiding this comment

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

Uhh, Here is a real CPU burner. Please hide it behind the ControllerDebug::enabled() flag

Copy link
Contributor Author

@Be-ing Be-ing Sep 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't the controllerDebug() macro already do this?

#define controllerDebug(stream)       \
{                                     \
    if (ControllerDebug::enabled()) { \
        QDebug(QtDebugMsg) << stream; \
    }                                 \
}                                     \

#endif // CONTROLLERDEBUG_H

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right -- controllerDebug(...) is transformed so it only costs a potential branch miss when disabled.


Expand Down Expand Up @@ -558,9 +570,7 @@ void MidiController::processInputMapping(const MidiInputMapping& mapping,
<< formatSysexMessage(getName(), data, timestamp);
}

void MidiController::sendShortMsg(unsigned char status, unsigned char byte1,
unsigned char byte2) {
unsigned int word = (((unsigned int)byte2) << 16) |
(((unsigned int)byte1) << 8) | status;
sendWord(word);
}
// void MidiController::sendShortMsg(unsigned char status, unsigned char byte1,
// unsigned char byte2) {
// sendWord(status, byte1, byte2);
// }
19 changes: 15 additions & 4 deletions src/controllers/midi/midicontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,24 @@ class MidiController : public Controller {
unsigned char value);

protected:
Q_INVOKABLE void sendShortMsg(unsigned char status, unsigned char byte1, unsigned char byte2);
Q_INVOKABLE virtual void sendShortMsg(unsigned char status,
unsigned char byte1, unsigned char byte2) = 0;
// Alias for send()
Q_INVOKABLE inline void sendSysexMsg(QList<int> data, unsigned int length) {
send(data, length);
Q_INVOKABLE inline void sendSysexMsg(QList<int> data, unsigned int length = 0) {
Q_UNUSED(length);
Copy link
Member

Choose a reason for hiding this comment

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

The same here, all the length and int to char looks scary.
I think we should either keep the Q_INVOKABLE interface untouched or clean it up entirely.
Why using a QList if we actually send a byte array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering that as well, but I am being careful not to break the QtScript interface. I could try making sendSysexMsg accept a QByteArray and test if it works.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's because QtScript passes the data as a QList. It was quite difficult getting this translation to work properly.

Copy link
Member

Choose a reason for hiding this comment

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

@daschuer QtScript can't pass us anything other than a list of ints, so that's why. The length argument was always unnecessary (and error prone) because the real length is already stored in the list itself.

I can't remember if QtScript will drop extra arguments when calling C++ objects. Javascript certainly will if you're calling a normally JS function with more arguments than it expects. @Be-ing did you test that?

We have a more efficient ByteArray class available to scripts that we use for passing bytearrays into the script engine:
https://github.com/mixxxdj/mixxx/blob/master/src/controllers/controllerengine.cpp#L211
but scripts can't use it to pass us a bytearray at the moment. That would allow us to get rid of the int -> char conversion for every send. (but would be a breaking change to the script API, so we would have to introduce a separate function like sendByteArray/sendSysexByteArray).

send(data);
}

QString formatMidiMessage(const QString& controllerName,
unsigned char status, unsigned char control,
unsigned char value, unsigned char channel,
unsigned char opCode,
mixxx::Duration timestamp = mixxx::Duration::fromMillis(0));
QString formatSysexMessage(const QString& controllerName,
const QByteArray& data,
mixxx::Duration timestamp = mixxx::Duration::fromMillis(0));


protected slots:
virtual void receive(unsigned char status, unsigned char control,
unsigned char value, mixxx::Duration timestamp);
Expand All @@ -88,7 +100,6 @@ class MidiController : public Controller {
const QByteArray& data,
mixxx::Duration timestamp);

virtual void sendWord(unsigned int word) = 0;
double computeValue(MidiOptions options, double _prevmidivalue, double _newmidivalue);
void createOutputHandlers();
void updateAllOutputs();
Expand Down
23 changes: 19 additions & 4 deletions src/controllers/midi/portmidicontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*
*/

#include "controllers/midi/midiutils.h"
#include "controllers/midi/portmidicontroller.h"
#include "controllers/controllerdebug.h"

Expand Down Expand Up @@ -203,14 +204,26 @@ bool PortMidiController::poll() {
return numEvents > 0;
}

void PortMidiController::sendWord(unsigned int word) {
void PortMidiController::sendShortMsg(unsigned char status, unsigned char byte1,
unsigned char byte2) {
if (m_pOutputDevice.isNull() || !m_pOutputDevice->isOpen()) {
return;
}

unsigned int word = (((unsigned int)byte2) << 16) |
(((unsigned int)byte1) << 8) | status;
unsigned char channel = MidiUtils::channelFromStatus(status);
unsigned char opCode = MidiUtils::opCodeFromStatus(status);

PmError err = m_pOutputDevice->writeShort(word);
if (err != pmNoError) {
qWarning() << "PortMidi sendShortMsg error:" << Pm_GetErrorText(err);
if (err == pmNoError) {
controllerDebug(formatMidiMessage(getName(), status, byte1, byte2,
channel, opCode));
} else {
qWarning() << "Error sending short message"
<< formatMidiMessage(getName(), status, byte1, byte2,
channel, opCode)
<< "\nPortMidi error:" << Pm_GetErrorText(err);
Copy link
Member

Choose a reason for hiding this comment

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

On Windows it's \r\n. Instead I would either keep them on the same line or make a separate qWarning if you want it on a separate line (I don't think QtDebug supports std::endl).

}
}

Expand All @@ -229,7 +242,9 @@ void PortMidiController::send(QByteArray data) {
}

PmError err = m_pOutputDevice->writeSysEx((unsigned char*)data.constData());
if (err != pmNoError) {
if (err == pmNoError) {
controllerDebug(formatSysexMessage(getName(), data));
} else {
qWarning() << "PortMidi sendSysexMsg error:"
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to print the formatted message on error here too?

<< Pm_GetErrorText(err);
}
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/midi/portmidicontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ class PortMidiController : public MidiController {
virtual bool poll();

private:
void sendWord(unsigned int word);
void sendShortMsg(unsigned char status, unsigned char byte1,
unsigned char byte2);
// The sysex data must already contain the start byte 0xf0 and the end byte
// 0xf7.
void send(QByteArray data);
Expand Down