Merge pull request #2179 from codecat/fix-hid-thread

Fix deadlock when hid_read and hid_write are called at the same time
mixxxdj · Nov 20, 2019 · 5eb0594 · 5eb0594
2 parents 1dfecbe + a76cb60
commit 5eb0594
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 83 deletions.
diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp
@@ -16,37 +16,6 @@
 #include "controllers/controllerdebug.h"
 #include "util/time.h"
 
-HidReader::HidReader(hid_device* device)
-        : QThread(),
-          m_pHidDevice(device) {
-}
-
-HidReader::~HidReader() {
-}
-
-void HidReader::run() {
-    m_stop = 0;
-    unsigned char *data = new unsigned char[255];
-    while (m_stop.load() == 0) {
-        // Blocked polling: The only problem with this is that we can't close
-        // the device until the block is released, which means the controller
-        // has to send more data
-        //result = hid_read_timeout(m_pHidDevice, data, 255, -1);
-
-        // This relieves that at the cost of higher CPU usage since we only
-        // block for a short while (500ms)
-        int result = hid_read_timeout(m_pHidDevice, data, 255, 500);
-        Trace timeout("HidReader timeout");
-        if (result > 0) {
-            Trace process("HidReader process packet");
-            //qDebug() << "Read" << result << "bytes, pointer:" << data;
-            QByteArray outData(reinterpret_cast<char*>(data), result);
-            emit(incomingData(outData, mixxx::Time::elapsed()));
-        }
-    }
-    delete [] data;
-}
-
 HidController::HidController(const hid_device_info deviceInfo)
         : m_pHidDevice(NULL) {
     // Copy required variables from deviceInfo, which will be freed after
@@ -105,7 +74,6 @@ HidController::HidController(const hid_device_info deviceInfo)
     // All HID devices are full-duplex
     setInputDevice(true);
     setOutputDevice(true);
-    m_pReader = NULL;
 }
 
 HidController::~HidController() {
@@ -241,23 +209,15 @@ int HidController::open() {
         return -1;
     }
 
+    // Set hid controller to non-blocking
+    if (hid_set_nonblocking(m_pHidDevice, 1) != 0) {
+        qWarning() << "Unable to set HID device " << getName() << " to non-blocking";
+        return -1;
+    }
+
     setOpen(true);
     startEngine();
 
-    if (m_pReader != NULL) {
-        qWarning() << "HidReader already present for" << getName();
-    } else {
-        m_pReader = new HidReader(m_pHidDevice);
-        m_pReader->setObjectName(QString("HidReader %1").arg(getName()));
-
-        connect(m_pReader, SIGNAL(incomingData(QByteArray, mixxx::Duration)),
-                this, SLOT(receive(QByteArray, mixxx::Duration)));
-
-        // Controller input needs to be prioritized since it can affect the
-        // audio directly, like when scratching
-        m_pReader->start(QThread::HighPriority);
-    }
-
     return 0;
 }
 
@@ -269,21 +229,6 @@ int HidController::close() {
 
     qDebug() << "Shutting down HID device" << getName();
 
-    // Stop the reading thread
-    if (m_pReader == NULL) {
-        qWarning() << "HidReader not present for" << getName()
-                   << "yet the device is open!";
-    } else {
-        disconnect(m_pReader, SIGNAL(incomingData(QByteArray, mixxx::Duration)),
-                   this, SLOT(receive(QByteArray, mixxx::Duration)));
-        m_pReader->stop();
-        hid_set_nonblocking(m_pHidDevice, 1);   // Quit blocking
-        controllerDebug("  Waiting on reader to finish");
-        m_pReader->wait();
-        delete m_pReader;
-        m_pReader = NULL;
-    }
-
     // Stop controller engine here to ensure it's done before the device is closed
     //  in case it has any final parting messages
     stopEngine();
@@ -295,6 +240,25 @@ int HidController::close() {
     return 0;
 }
 
+bool HidController::poll() {
+    Trace hidRead("HidController poll");
+
+    int result = hid_read(m_pHidDevice, m_pPollData, sizeof(m_pPollData) / sizeof(m_pPollData[0]));
+    if (result == -1) {
+        return false;
+    } else if (result > 0) {
+        Trace process("HidController process packet");
+        QByteArray outData(reinterpret_cast<char*>(m_pPollData), result);
+        receive(outData, mixxx::Time::elapsed());
+    }
+
+    return true;
+}
+
+bool HidController::isPolling() const {
+    return isOpen();
+}
+
 void HidController::send(QList<int> data, unsigned int length, unsigned int reportID) {
     Q_UNUSED(length);
     QByteArray temp;

diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h
@@ -17,27 +17,6 @@
 #include "controllers/hid/hidcontrollerpresetfilehandler.h"
 #include "util/duration.h"
 
-class HidReader : public QThread {
-    Q_OBJECT
-  public:
-    HidReader(hid_device* device);
-    virtual ~HidReader();
-
-    void stop() {
-        m_stop = 1;
-    }
-
-  signals:
-    void incomingData(QByteArray data, mixxx::Duration timestamp);
-
-  protected:
-    void run();
-
-  private:
-    hid_device* m_pHidDevice;
-    QAtomicInt m_stop;
-};
-
 class HidController final : public Controller {
     Q_OBJECT
   public:
@@ -78,6 +57,9 @@ class HidController final : public Controller {
     int open() override;
     int close() override;
 
+    bool poll() override;
+    bool isPolling() const override;
+
   private:
     // For devices which only support a single report, reportID must be set to
     // 0x0.
@@ -107,8 +89,9 @@ class HidController final : public Controller {
 
     QString m_sUID;
     hid_device* m_pHidDevice;
-    HidReader* m_pReader;
     HidControllerPreset m_preset;
+
+    unsigned char m_pPollData[255];
 };
 
 #endif