Outgoing MIDI debugging messages #1004

Be-ing · 2016-09-08T16:55:54Z

implements https://bugs.launchpad.net/mixxx/+bug/1473695

Be-ing · 2016-09-08T18:34:43Z

src/controllers/midi/midicontroller.cpp

+                                          unsigned char status, unsigned char control,
+                                          unsigned char value, unsigned char channel,
+                                          unsigned char opCode, mixxx::Duration timestamp) {
+    QString t;


Any suggestions for a better variable name here?

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

for efficiency

and fix some spacing

Pegasus-RPG · 2016-09-10T05:34:53Z

For reference, the HSS1394 library code is available here: https://launchpad.net/hss1394
Code browsing: http://bazaar.launchpad.net/~mixxxdevelopers/hss1394/trunk/files

Pegasus-RPG · 2016-09-10T05:50:33Z

src/controllers/midi/hss1394controller.h

@@ -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);


What gain is there in eliminating the ability to send data words?

The way it was before, with sendShortMsg() passing the computed word to sendWord(), the sendWord() function that actually did the sending would have to undo the operations of sendShortMsg() to print the three separate MIDI bytes.
https://github.com/mixxxdj/mixxx/pull/1004/files#diff-0212d99488d889be2b8194be8f10eed0R178

That may be the case in HSS1394Controller, but not in PortMidiController. https://github.com/mixxxdj/mixxx/pull/1004/files#diff-ab47417f917c4a5cea304eded0d68381R213
I guess it doesn't much matter since there's no engine.sendWord() function for scripts.

and minor code formatting

Pegasus-RPG · 2016-09-10T15:40:09Z

src/controllers/midi/midicontroller.cpp

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


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

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.

How were you able to build before then?

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().

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.

Be-ing · 2016-09-10T16:16:34Z

@Pegasus-RPG can you test that this hasn't broken anything for HSS1394 controllers?

Pegasus-RPG · 2016-09-10T16:51:48Z

I can, but it may be a few days. In the meantime, can you create test fixtures that create virtual HSS1394 controllers and I'll populate them with MIDI data and expected results? (I'm still new to creating tests.)

Be-ing · 2016-09-10T19:01:15Z

I'll look into it, but a lot of the PortMidiController tests are segfaulting on my system (on this branch and master), so I'm not sure I'll figure out how to set it up right. I did notice Travis is having trouble compiling tests on this branch, so I'll look into that as well.

rryan

This will be a big help for script authors, thanks for working on it.

RE: HSS1394, this looks like a safe change to me.

rryan · 2016-12-18T18:38:01Z

src/controllers/midi/portmidicontroller.cpp

+                                           status, byte1, byte2,
+                                           MidiUtils::channelFromStatus(status),
+                                           MidiUtils::opCodeFromStatus(status))
+                      << "\nPortMidi error:" << Pm_GetErrorText(err);


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).

rryan · 2016-12-18T18:48:07Z

src/controllers/midi/portmidicontroller.cpp

-    if (err != pmNoError) {
+    if (err == pmNoError) {
+        controllerDebug(formatSysexMessage(getName(), data));
+    } else {
        qWarning() << "PortMidi sendSysexMsg error:"


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

rryan · 2016-12-18T18:51:53Z

src/controllers/controller.cpp

@@ -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


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.

Be-ing · 2016-12-18T19:31:03Z

Thanks for reviewing. I'll get back to work on this in a bit after the new effects interface and Deere redesign.

… to MidiUtils plus miscellaneous small fixes

Be-ing · 2016-12-21T00:20:12Z

Merge conflicts resolved.

rryan · 2016-12-21T03:23:15Z

Thanks, LGTM

``--debugAssertBreak`` ``--logLevel`` ``--controllerDebug`` mixxxdj/mixxx#1265 mixxxdj/mixxx/mixxxdj#118 mixxxdj/mixxx#1004

Be-ing added 2 commits September 8, 2016 11:52

don't require length parameter for Controller::send()

8fcf96c

print debugging messages for outgoing MIDI from scripts

6aa4607

Be-ing reviewed Sep 8, 2016
View reviewed changes

Be-ing added 2 commits September 9, 2016 16:55

MIDI output code cleanup

258556e

don't do any operations for debugging MIDI when not debugging

653136a

for efficiency

Be-ing force-pushed the outgoing_midi_debug branch 2 times, most recently from bc410d9 to 6490153 Compare September 10, 2016 05:04

document purpose of seemingly useless parameter

047de0b

and fix some spacing

Be-ing force-pushed the outgoing_midi_debug branch from 6490153 to 047de0b Compare September 10, 2016 05:06

Pegasus-RPG reviewed Sep 10, 2016
View reviewed changes

fix MIDI debugging string

f698d84

and minor code formatting

Pegasus-RPG reviewed Sep 10, 2016
View reviewed changes

fix MIDI controller test builds

6033525

rryan requested changes Dec 18, 2016

View reviewed changes

Be-ing added 2 commits December 20, 2016 15:38

merge upstream/master

6d9f433

move formatMidiMessage() and formatSysexMessage() from MidiController…

6e8b4d0

… to MidiUtils plus miscellaneous small fixes

Be-ing force-pushed the outgoing_midi_debug branch from 336b978 to 6e8b4d0 Compare December 20, 2016 22:18

Merge remote-tracking branch 'upstream/master' into outgoing_midi_debug

4d708cd

Be-ing force-pushed the outgoing_midi_debug branch from 463f34d to 4d708cd Compare December 21, 2016 00:21

rryan approved these changes Dec 21, 2016

View reviewed changes

rryan merged commit 17cd046 into mixxxdj:master Dec 21, 2016

Be-ing deleted the outgoing_midi_debug branch February 1, 2017 01:22

Be-ing mentioned this pull request Feb 14, 2017

Update Mixtrack (Pro) 3 mappings for Mixxx 2.1 #1180

Merged

4 tasks

esbrandt added a commit to esbrandt/manual that referenced this pull request Jun 24, 2017

Adds latest additions and changes to command line options.

431b5ad

``--debugAssertBreak`` ``--logLevel`` ``--controllerDebug`` mixxxdj/mixxx#1265 mixxxdj/mixxx/mixxxdj#118 mixxxdj/mixxx#1004

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

mixxxbot mentioned this pull request Aug 22, 2022

command line option to print outgoing MIDI messages #8146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Outgoing MIDI debugging messages #1004

Outgoing MIDI debugging messages #1004

Be-ing commented Sep 8, 2016

Be-ing Sep 8, 2016

daschuer Sep 9, 2016

Pegasus-RPG commented Sep 10, 2016

Pegasus-RPG Sep 10, 2016

Be-ing Sep 10, 2016

Pegasus-RPG Sep 10, 2016

Pegasus-RPG Sep 10, 2016 •

edited

Loading

Be-ing Sep 10, 2016

Pegasus-RPG Sep 10, 2016

Be-ing Sep 10, 2016 •

edited

Loading

rryan Dec 18, 2016

Be-ing commented Sep 10, 2016

Pegasus-RPG commented Sep 10, 2016

Be-ing commented Sep 10, 2016 •

edited

Loading

rryan left a comment

rryan Dec 18, 2016

rryan Dec 18, 2016

rryan Dec 18, 2016

Be-ing commented Dec 18, 2016

Be-ing commented Dec 21, 2016

rryan commented Dec 21, 2016

Outgoing MIDI debugging messages #1004

Outgoing MIDI debugging messages #1004

Conversation

Be-ing commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pegasus-RPG commented Sep 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pegasus-RPG Sep 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Sep 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Sep 10, 2016

Pegasus-RPG commented Sep 10, 2016

Be-ing commented Sep 10, 2016 • edited Loading

rryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Dec 18, 2016

Be-ing commented Dec 21, 2016

rryan commented Dec 21, 2016

Pegasus-RPG Sep 10, 2016 •

edited

Loading

Be-ing Sep 10, 2016 •

edited

Loading

Be-ing commented Sep 10, 2016 •

edited

Loading