Merge pull request #790 from daschuer/controllerpoll

removed while loop in poll slot, Bug #1520619

7 files changed, 101 insertions, 33 deletions

diff --git a/src/controllers/controllermanager.cpp b/src/controllers/controllermanager.cpp
index 43056a324d..7d81f1e8fc 100644
--- a/src/controllers/controllermanager.cpp
+++ b/src/controllers/controllermanager.cpp
@@ -12,6 +12,7 @@
 #include "controllers/defs_controllers.h"
 #include "controllers/controllerlearningeventfilter.h"
 #include "util/cmdlineargs.h"
+#include "util/time.h"
 
 #include "controllers/midi/portmidienumerator.h"
 #ifdef __HSS1394__
@@ -26,6 +27,7 @@
 #include "controllers/bulk/bulkenumerator.h"
 #endif
 
+namespace {
 // http://developer.qt.nokia.com/wiki/Threads_Events_QObjects
 
 // Poll every 1ms (where possible) for good controller response
@@ -37,6 +39,8 @@ const int kPollIntervalMillis = 5;
 const int kPollIntervalMillis = 1;
 #endif
 
+} // anonymous namespace
+
 QString firstAvailableFilename(QSet<QString>& filenames,
                                const QString originalFilename) {
     QString filename = originalFilename;
@@ -60,7 +64,8 @@ ControllerManager::ControllerManager(ConfigObject<ConfigValue>* pConfig)
           // ControllerManager because the CM is moved to its own thread and runs
           // its own event loop.
           m_pControllerLearningEventFilter(new ControllerLearningEventFilter()),
-          m_pollTimer(this) {
+          m_pollTimer(this),
+          m_skipPoll(false) {
     qRegisterMetaType<ControllerPresetPointer>("ControllerPresetPointer");
 
     // Create controller mapping paths in the user's home directory.
@@ -282,17 +287,45 @@ void ControllerManager::stopPolling() {
 }
 
 void ControllerManager::pollDevices() {
-    Trace tracer("ControllerManager::pollDevices");
-    bool eventsProcessed(false);
-    // Continue to poll while any device returned data.
-    do {
-        eventsProcessed = false;
-        foreach (Controller* pDevice, m_controllers) {
-            if (pDevice->isOpen() && pDevice->isPolling()) {
-                eventsProcessed = pDevice->poll() || eventsProcessed;
-            }
+    // Note: this function is called from a high priority thread which
+    // may stall the GUI or may reduce the available CPU time for other
+    // High Priority threads like caching reader or broadcasting more
+    // then desired, if it is called endless loop like.
+    //
+    // This especially happens if a controller like the 3x Speed
+    // Stanton SCS.1D emits more massages than Mixxx is able to handle
+    // or a controller like Hercules RMX2 goes wild. In such a case the
+    // receive buffer is stacked up every call to insane values > 500 messages.
+    //
+    // To avoid this we pick here a strategies similar like the audio
+    // thread. In case pollDevice() takes longer than a call cycle
+    // we are cooperative a skip the next cycle to free at least some
+    // CPU time
+    //
+    // Some random test data form a i5-3317U CPU @ 1.70GHz Running
+    // Ubuntu Trusty:
+    // * Idle poll: ~5 µs.
+    // * 5 messages burst (full midi bandwidth): ~872 µs.
+
+    if (m_skipPoll) {
+        // skip poll in overload situation
+        m_skipPoll = false;
+        //qDebug() << "ControllerManager::pollDevices() skip";
+        return;
+    }
+
+    qint64 start = Time::elapsed();
+    foreach (Controller* pDevice, m_controllers) {
+        if (pDevice->isOpen() && pDevice->isPolling()) {
+            pDevice->poll();
         }
-    } while (eventsProcessed);
+    }
+
+    qint64 duration = Time::elapsed() - start;
+    if (duration > kPollIntervalMillis * 1000000) {
+        m_skipPoll = true;
+    }
+    //qDebug() << "ControllerManager::pollDevices()" << duration << start;
 }
 
 void ControllerManager::openController(Controller* pController) {
diff --git a/src/controllers/controllermanager.h b/src/controllers/controllermanager.h
index 79cb8c8fec..b4503965af 100644
--- a/src/controllers/controllermanager.h
+++ b/src/controllers/controllermanager.h
@@ -90,6 +90,7 @@ class ControllerManager : public QObject {
     QList<Controller*> m_controllers;
     QThread* m_pThread;
     PresetInfoEnumerator* m_pMainThreadPresetEnumerator;
+    bool m_skipPoll;
 };
 
 #endif  // CONTROLLERMANAGER_H
diff --git a/src/controllers/midi/midicontroller.cpp b/src/controllers/midi/midicontroller.cpp
index d2472d9296..c889437f26 100644
--- a/src/controllers/midi/midicontroller.cpp
+++ b/src/controllers/midi/midicontroller.cpp
@@ -161,40 +161,44 @@ void MidiController::destroyOutputHandlers() {
 }
 
 QString formatMidiMessage(unsigned char status, unsigned char control, unsigned char value,
-                          unsigned char channel, unsigned char opCode) {
+                          unsigned char channel, unsigned char opCode, int32_t timestamp) {
+
+    QString prefix = QString("MIDI t:%2 ms ").arg(QString::number(timestamp));
+    //QString prefix = QString("MIDI ");
+
     switch (opCode) {
         case MIDI_PITCH_BEND:
-            return QString("MIDI status 0x%1: pitch bend ch %2, value 0x%3")
+            return prefix + QString("status 0x%1: pitch bend ch %2, value 0x%3")
                     .arg(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("MIDI status 0x%1: song position 0x%2")
+            return prefix + QString("status 0x%1: song position 0x%2")
                     .arg(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("MIDI status 0x%1 (ch %2, opcode 0x%3), value 0x%4")
+            return prefix + QString("status 0x%1 (ch %2, opcode 0x%3), value 0x%4")
                     .arg(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("MIDI status 0x%1: select song #%2")
+            return prefix + QString("status 0x%1: select song #%2")
                     .arg(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("MIDI status 0x%1 (ch %2, opcode 0x%3), ctrl 0x%4, val 0x%5")
+            return prefix + QString("status 0x%1 (ch %2, opcode 0x%3), ctrl 0x%4, val 0x%5")
                     .arg(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("MIDI status 0x%1")
+            return prefix + QString("status 0x%1")
                     .arg(QString::number(status, 16).toUpper());
     }
 }
@@ -237,11 +241,11 @@ void MidiController::commitTemporaryInputMappings() {
 }
 
 void MidiController::receive(unsigned char status, unsigned char control,
-                             unsigned char value) {
+                             unsigned char value, int32_t timestamp) {
     unsigned char channel = MidiUtils::channelFromStatus(status);
     unsigned char opCode = MidiUtils::opCodeFromStatus(status);
 
-    controllerDebug(formatMidiMessage(status, control, value, channel, opCode));
+    controllerDebug(formatMidiMessage(status, control, value, channel, opCode, timestamp));
 
     MidiKey mappingKey(status, control);
 
diff --git a/src/controllers/midi/midicontroller.h b/src/controllers/midi/midicontroller.h
index d45dc4da7d..1df27f47f8 100644
--- a/src/controllers/midi/midicontroller.h
+++ b/src/controllers/midi/midicontroller.h
@@ -62,9 +62,9 @@ class MidiController : public Controller {
         send(data, length);
     }
 
-  protected slots:
-    void receive(unsigned char status, unsigned char control = 0,
-                 unsigned char value = 0);
+    void receive(unsigned char status, unsigned char control,
+                 unsigned char value, int32_t timestamp);
+
     // For receiving System Exclusive messages
     void receive(const QByteArray data);
     virtual int close();
diff --git a/src/controllers/midi/portmidicontroller.cpp b/src/controllers/midi/portmidicontroller.cpp
index 5a486e6c4f..e82edc4091 100644
--- a/src/controllers/midi/portmidicontroller.cpp
+++ b/src/controllers/midi/portmidicontroller.cpp
@@ -150,11 +150,14 @@ int PortMidiController::close() {
 
 bool PortMidiController::poll() {
     // Poll the controller for new data if it's an input device
-    if (!m_pInputStream)
+    if (!m_pInputStream) {
+        qDebug() << "PortMidiController::poll() no Input Stream";
         return false;
+    }
 
     PmError gotEvents = Pm_Poll(m_pInputStream);
     if (gotEvents == FALSE) {
+        //qDebug() << "PortMidiController::poll() no events";
         return false;
     }
     if (gotEvents < 0) {
@@ -164,6 +167,8 @@ bool PortMidiController::poll() {
 
     int numEvents = Pm_Read(m_pInputStream, m_midiBuffer, MIXXX_PORTMIDI_BUFFER_LEN);
 
+    //qDebug() << "PortMidiController::poll()" << numEvents;
+
     if (numEvents < 0) {
         qWarning() << "PortMidi error:" << Pm_GetErrorText((PmError)numEvents);
         return false;
@@ -174,7 +179,7 @@ bool PortMidiController::poll() {
 
         if ((status & 0xF8) == 0xF8) {
             // Handle real-time MIDI messages at any time
-            receive(status, 0, 0);
+            receive(status, 0, 0, m_midiBuffer[i].timestamp);
             continue;
         }
 
@@ -188,7 +193,7 @@ bool PortMidiController::poll() {
                 //unsigned char channel = status & 0x0F;
                 unsigned char note = Pm_MessageData1(m_midiBuffer[i].message);
                 unsigned char velocity = Pm_MessageData2(m_midiBuffer[i].message);
-                receive(status, note, velocity);
+                receive(status, note, velocity, m_midiBuffer[i].timestamp);
             }
         }
 
diff --git a/src/controllers/midi/portmidicontroller.h b/src/controllers/midi/portmidicontroller.h
index 3e89792833..213b1b3e5d 100644
--- a/src/controllers/midi/portmidicontroller.h
+++ b/src/controllers/midi/portmidicontroller.h
@@ -20,13 +20,38 @@
 #include <portmidi.h>
 #include "controllers/midi/midicontroller.h"
 
-// Mixxx completely stops responding to the controller if more than this number of messages queue up.
-//  Don't lower this (much.) The SCS.1d accumulated 500 messages in a single poll during stress-testing.
-#define MIXXX_PORTMIDI_BUFFER_LEN 1024 /**Number of MIDI messages to buffer*/
-#define MIXXX_SYSEX_BUFFER_LEN 1024 /**Length of SysEx buffer*/
-#define MIXXX_PORTMIDI_NO_DEVICE_STRING "None" /**String to display for no MIDI devices present */
+// Note:
+// A standard Midi device runs at 31.25 kbps, with 10 bits / byte
+// 1 byte / 320 microseconds
+// a usual Midi message has 3 byte which results to
+// 1042.6 messages per second
+//
+// The MIDI over IEEE-1394:
+// http://www.midi.org/techspecs/rp27v10spec%281394%29.pdf
+// which is also used for USB defines 3 speeds:
+// 1 byte / 320 microseconds
+// 2 bytes / 320 microseconds
+// 3 bytes / 320 microseconds
+// which results in up to 3125 messages per second
+// if we assume normal 3 Byte messages.
+//
+// For instants the SCS.1d, uses the 3 x speed
+//
+// Due to a bug Mixxx completely stops responding to the controller
+// if more than this number of messages queue up. Don't lower this (much.)
+// The SCS.1d a 3x Speed device
+// accumulated 500 messages in a single poll during stress-testing.
+// A midi message contains 1 .. 4 bytes.
+// a 1024 messages buffer will buffer ~327 ms Midi-Stream
+#define MIXXX_PORTMIDI_BUFFER_LEN 1024
 
-/** A PortMidi-based implementation of MidiController */
+// Length of SysEx buffer in byte
+#define MIXXX_SYSEX_BUFFER_LEN 1024 
+
+// String to display for no MIDI devices present
+#define MIXXX_PORTMIDI_NO_DEVICE_STRING "None"
+
+// A PortMidi-based implementation of MidiController
 class PortMidiController : public MidiController {
     Q_OBJECT
   public:
diff --git a/src/test/midicontrollertest.cpp b/src/test/midicontrollertest.cpp
index af5e605a91..aa8bfebe30 100644
--- a/src/test/midicontrollertest.cpp
+++ b/src/test/midicontrollertest.cpp
@@ -40,7 +40,7 @@ class MidiControllerTest : public MixxxTest {
 
     void receive(unsigned char status, unsigned char control,
                  unsigned char value) {
-        m_pController->receive(status, control, value);
+        m_pController->receive(status, control, value, 0);
     }
 
     MidiControllerPreset m_preset;