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 all 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
3 changes: 3 additions & 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,9 @@ 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)

// The length parameter is here for backwards compatibility for when scripts
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 also mention this in the header definition of send() and belongs in the header at the definition of Controller::send and MidiController::sendSysexMessage.

// were required to specify it.
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
4 changes: 3 additions & 1 deletion src/controllers/controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ class Controller : public QObject, ConstControllerPresetVisitor {
void stopLearning();

protected:
Q_INVOKABLE void send(QList<int> data, unsigned int length);
// The length parameter is here for backwards compatibility for when scripts
// were required to specify it.
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
19 changes: 9 additions & 10 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,15 @@ int Hss1394Controller::close() {
return 0;
}

void Hss1394Controller::sendWord(unsigned int word) {
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')));
void Hss1394Controller::sendShortMsg(unsigned char status, unsigned char byte1,
unsigned char byte2) {
unsigned char data[3] = { status, byte1, byte2 };

int bytesSent = m_pChannel->SendChannelBytes(data, 3);
controllerDebug(MidiUtils::formatMidiMessage(getName(),
status, byte1, byte2,
MidiUtils::channelFromStatus(status),
MidiUtils::opCodeFromStatus(status)));

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

controllerDebug(MidiUtils::formatSysexMessage(getName(), data));
//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
6 changes: 4 additions & 2 deletions src/controllers/midi/hss1394controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,11 @@ class Hss1394Controller : public MidiController {
int open() override;
int close() override;

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

private:
// The sysex data must already contain the start byte 0xf0 and the end byte
// 0xf7.
void send(QByteArray data) override;
Expand Down
75 changes: 4 additions & 71 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,6 @@ 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) {
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(),
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")
.arg(controllerName, timestamp.formatMillisWithUnit(),
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(),
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(),
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(),
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(),
QString::number(status, 16).toUpper());
}
}

void MidiController::learnTemporaryInputMappings(const MidiInputMappings& mappings) {
foreach (const MidiInputMapping& mapping, mappings) {
m_temporaryInputMappings.insert(mapping.key.key, mapping);
Expand Down Expand Up @@ -249,8 +202,8 @@ void MidiController::receive(unsigned char status, unsigned char control,
unsigned char channel = MidiUtils::channelFromStatus(status);
unsigned char opCode = MidiUtils::opCodeFromStatus(status);

controllerDebug(formatMidiMessage(getName(), status, control, value,
channel, opCode, timestamp));
controllerDebug(MidiUtils::formatMidiMessage(getName(), status, control, value,
channel, opCode, timestamp));
MidiKey mappingKey(status, control);

if (isLearning()) {
Expand Down Expand Up @@ -497,21 +450,8 @@ 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));
controllerDebug(MidiUtils::formatSysexMessage(getName(), data, timestamp));

MidiKey mappingKey(data.at(0), 0xFF);

Expand Down Expand Up @@ -555,12 +495,5 @@ void MidiController::processInputMapping(const MidiInputMapping& mapping,
return;
}
qWarning() << "MidiController: No script function specified for"
<< 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);
<< MidiUtils::formatSysexMessage(getName(), data, timestamp);
}
14 changes: 8 additions & 6 deletions src/controllers/midi/midicontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include "controllers/midi/midimessage.h"
#include "controllers/midi/midioutputhandler.h"
#include "controllers/softtakeover.h"
#include "util/duration.h"

class MidiController : public Controller {
Q_OBJECT
Expand Down Expand Up @@ -57,11 +56,15 @@ 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);
// The length parameter is here for backwards compatibility for when scripts
// were required to specify it.
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);
}

protected slots:
Expand Down Expand Up @@ -89,7 +92,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
74 changes: 74 additions & 0 deletions src/controllers/midi/midiutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,77 @@ QString MidiUtils::midiOptionToTranslatedString(MidiOption option) {
.arg(option, 4, 16, QLatin1Char('0'));
}
}

// static
QString MidiUtils::formatMidiMessage(const QString& controllerName,
unsigned char status, unsigned char control,
unsigned char value, unsigned char channel,
unsigned char opCode, mixxx::Duration timestamp) {
QString msg2;
if (timestamp == mixxx::Duration::fromMillis(0)) {
msg2 = "outgoing:";
} else {
msg2 = QString("t:%1").arg(timestamp.formatMillisWithUnit());
}
switch (opCode) {
case MIDI_PITCH_BEND:
return QString("%1: %2 status 0x%3: pitch bend ch %4, value 0x%5")
.arg(controllerName, msg2,
QString::number(status, 16).toUpper(),
QString::number(channel+1, 10),
QString::number((value << 7) | control, 16).toUpper().rightJustified(4,'0'));
case MIDI_SONG_POS:
return QString("%1: %2 status 0x%3: song position 0x%4")
.arg(controllerName, msg2,
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: %2 status 0x%3 (ch %4, opcode 0x%5), value 0x%6")
.arg(controllerName, msg2,
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: %2 status 0x%3: select song #%4")
.arg(controllerName, msg2,
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: %2 status 0x%3 (ch %4, opcode 0x%5), ctrl 0x%6, val 0x%7")
.arg(controllerName, msg2,
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: %2 status 0x%3")
.arg(controllerName, msg2,
QString::number(status, 16).toUpper());
}
}

// static
QString MidiUtils::formatSysexMessage(const QString& controllerName, const QByteArray& data,
mixxx::Duration timestamp) {
QString msg2;
if (timestamp == mixxx::Duration::fromMillis(0)) {
msg2 = "outgoing:";
} else {
msg2 = QString("t:%1").arg(timestamp.formatMillisWithUnit());
}
QString message = QString("%1: %2 %3 byte sysex: [")
.arg(controllerName).arg(msg2)
.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;
}
9 changes: 9 additions & 0 deletions src/controllers/midi/midiutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define MIDIUTILS_H

#include "controllers/midi/midimessage.h"
#include "util/duration.h"

class MidiUtils {
public:
Expand Down Expand Up @@ -38,6 +39,14 @@ class MidiUtils {
static QString opCodeToTranslatedString(MidiOpCode code);
static QString formatByteAsHex(unsigned char value);
static QString midiOptionToTranslatedString(MidiOption option);
static 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));
static QString formatSysexMessage(const QString& controllerName,
const QByteArray& data,
mixxx::Duration timestamp = mixxx::Duration::fromMillis(0));
};


Expand Down
32 changes: 26 additions & 6 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,29 @@ 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;

PmError err = m_pOutputDevice->writeShort(word);
if (err != pmNoError) {
qWarning() << "PortMidi sendShortMsg error:" << Pm_GetErrorText(err);
if (err == pmNoError) {
controllerDebug(MidiUtils::formatMidiMessage(getName(),
status, byte1, byte2,
MidiUtils::channelFromStatus(status),
MidiUtils::opCodeFromStatus(status)));
} else {
// Use two qWarnings() to ensure line break works on all operating systems
qWarning() << "Error sending short message"
<< MidiUtils::formatMidiMessage(getName(),
status, byte1, byte2,
MidiUtils::channelFromStatus(status),
MidiUtils::opCodeFromStatus(status));
qWarning() << "PortMidi error:" << Pm_GetErrorText(err);
}
}

Expand All @@ -229,8 +245,12 @@ void PortMidiController::send(QByteArray data) {
}

PmError err = m_pOutputDevice->writeSysEx((unsigned char*)data.constData());
if (err != pmNoError) {
qWarning() << "PortMidi sendSysexMsg error:"
<< Pm_GetErrorText(err);
if (err == pmNoError) {
controllerDebug(MidiUtils::formatSysexMessage(getName(), data));
} else {
// Use two qWarnings() to ensure line break works on all operating systems
qWarning() << "Error sending SysEx message:"
<< MidiUtils::formatSysexMessage(getName(), data);
qWarning() << "PortMidi error:" << Pm_GetErrorText(err);
}
}
6 changes: 5 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 @@ -70,8 +70,12 @@ class PortMidiController : public MidiController {
int close() override;
bool poll() override;

protected:
// MockPortMidiController needs this to not be private.
void sendShortMsg(unsigned char status, unsigned char byte1,
unsigned char byte2) override;

private:
void sendWord(unsigned int word) override;
// The sysex data must already contain the start byte 0xf0 and the end byte
// 0xf7.
void send(QByteArray data) override;
Expand Down
4 changes: 3 additions & 1 deletion src/test/midicontrollertest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ class MockMidiController : public MidiController {

MOCK_METHOD0(open, int());
MOCK_METHOD0(close, int());
MOCK_METHOD1(sendWord, void(unsigned int work));
MOCK_METHOD3(sendShortMsg, void(unsigned char status,
unsigned char byte1,
unsigned char byte2));
MOCK_METHOD1(send, void(QByteArray data));
MOCK_CONST_METHOD0(isPolling, bool());
};
Expand Down