From cf61194b3f47251bcdc68d7087049210b6c0d057 Mon Sep 17 00:00:00 2001 From: Codecat Date: Tue, 18 Jun 2019 23:30:26 +0200 Subject: [PATCH 1/6] Fix deadlock when hid_read and hid_write are called at the same time --- src/controllers/hid/hidcontroller.cpp | 91 +++++++++------------------ src/controllers/hid/hidcontroller.h | 27 ++------ 2 files changed, 35 insertions(+), 83 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index b07b5167c5e..84c9994eb6a 100644 --- 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(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 @@ -84,6 +53,9 @@ HidController::HidController(const hid_device_info deviceInfo) guessDeviceCategory(); + // Initialize buffer for polling + m_pPollData = new unsigned char[255]; + // Set the Unique Identifier to the serial_number m_sUID = hid_serial; @@ -105,7 +77,6 @@ HidController::HidController(const hid_device_info deviceInfo) // All HID devices are full-duplex setInputDevice(true); setOutputDevice(true); - m_pReader = NULL; } HidController::~HidController() { @@ -114,6 +85,8 @@ HidController::~HidController() { } delete [] hid_path; delete [] hid_serial_raw; + + delete [] m_pPollData; } QString HidController::presetExtension() { @@ -244,20 +217,6 @@ int HidController::open() { 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 +228,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 +239,31 @@ int HidController::close() { return 0; } +bool HidController::poll() { + // 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, m_pPollData, 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, m_pPollData, 255, 500); + + Trace timeout("HidReader timeout"); + if (result > 0) { + Trace process("HidReader process packet"); + //qDebug() << "Read" << result << "bytes, pointer:" << m_pPollData; + QByteArray outData(reinterpret_cast(m_pPollData), result); + receive(outData, mixxx::Time::elapsed()); + } + + return result != -1; +} + +bool HidController::isPolling() const { + return isOpen(); +} + void HidController::send(QList 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 index ac44aa9998b..eb502428ae9 100644 --- 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; + virtual bool poll() override; + virtual 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; }; #endif From 7072de73271e9c54f1f86636c4b6bf39db1fbe36 Mon Sep 17 00:00:00 2001 From: Codecat Date: Tue, 18 Jun 2019 23:38:07 +0200 Subject: [PATCH 2/6] Fix formatting --- src/controllers/hid/hidcontroller.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index eb502428ae9..21c4087553e 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -57,8 +57,8 @@ class HidController final : public Controller { int open() override; int close() override; - virtual bool poll() override; - virtual bool isPolling() const override; + bool poll() override; + bool isPolling() const override; private: // For devices which only support a single report, reportID must be set to From 1cf355a87129396bea0c619e34e21337f898c640 Mon Sep 17 00:00:00 2001 From: Codecat Date: Thu, 20 Jun 2019 23:10:24 +0200 Subject: [PATCH 3/6] Switch to non-blocking hid reading --- src/controllers/hid/hidcontroller.cpp | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 84c9994eb6a..0305010522f 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -214,6 +214,9 @@ int HidController::open() { return -1; } + // Set hid controller to non-blocking + hid_set_nonblocking(m_pHidDevice, 1); + setOpen(true); startEngine(); @@ -240,19 +243,11 @@ int HidController::close() { } bool HidController::poll() { - // 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, m_pPollData, 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, m_pPollData, 255, 500); + Trace hidRead("HidReader read packet"); + int result = hid_read(m_pHidDevice, m_pPollData, 255); - Trace timeout("HidReader timeout"); if (result > 0) { Trace process("HidReader process packet"); - //qDebug() << "Read" << result << "bytes, pointer:" << m_pPollData; QByteArray outData(reinterpret_cast(m_pPollData), result); receive(outData, mixxx::Time::elapsed()); } From 65e4c0515b1039f56591ca7dc8c00b82c6873378 Mon Sep 17 00:00:00 2001 From: Codecat Date: Thu, 20 Jun 2019 23:54:50 +0200 Subject: [PATCH 4/6] Requested changes --- src/controllers/hid/hidcontroller.cpp | 23 +++++++++++++---------- src/controllers/hid/hidcontroller.h | 2 +- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 0305010522f..9dbb370403b 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -53,9 +53,6 @@ HidController::HidController(const hid_device_info deviceInfo) guessDeviceCategory(); - // Initialize buffer for polling - m_pPollData = new unsigned char[255]; - // Set the Unique Identifier to the serial_number m_sUID = hid_serial; @@ -85,8 +82,6 @@ HidController::~HidController() { } delete [] hid_path; delete [] hid_serial_raw; - - delete [] m_pPollData; } QString HidController::presetExtension() { @@ -215,7 +210,9 @@ int HidController::open() { } // Set hid controller to non-blocking - hid_set_nonblocking(m_pHidDevice, 1); + if (hid_set_nonblocking(m_pHidDevice, 1) == -1) { + qWarning() << "Unable to set HID device " << getName() << " to non-blocking"; + } setOpen(true); startEngine(); @@ -243,16 +240,22 @@ int HidController::close() { } bool HidController::poll() { - Trace hidRead("HidReader read packet"); - int result = hid_read(m_pHidDevice, m_pPollData, 255); + Trace hidRead("HidReader poll"); + + for (int i = 0; i < 10; i++) { + int result = hid_read(m_pHidDevice, m_pPollData, sizeof(m_pPollData) / sizeof(m_pPollData[0])); + if (result == -1) { + return false; + } else if (result == 0) { + break; + } - if (result > 0) { Trace process("HidReader process packet"); QByteArray outData(reinterpret_cast(m_pPollData), result); receive(outData, mixxx::Time::elapsed()); } - return result != -1; + return true; } bool HidController::isPolling() const { diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 21c4087553e..4f446c9ae9b 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -91,7 +91,7 @@ class HidController final : public Controller { hid_device* m_pHidDevice; HidControllerPreset m_preset; - unsigned char* m_pPollData; + unsigned char m_pPollData[255]; }; #endif From b785c6fc60e2153e847433184f1e2a2892d1d0af Mon Sep 17 00:00:00 2001 From: Codecat Date: Fri, 21 Jun 2019 11:11:26 +0200 Subject: [PATCH 5/6] Requested changes --- src/controllers/hid/hidcontroller.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 9dbb370403b..c99aef527ad 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -16,6 +16,8 @@ #include "controllers/controllerdebug.h" #include "util/time.h" +#define HIDCONTROLLER_POLL_LIMIT 10 + HidController::HidController(const hid_device_info deviceInfo) : m_pHidDevice(NULL) { // Copy required variables from deviceInfo, which will be freed after @@ -210,8 +212,9 @@ int HidController::open() { } // Set hid controller to non-blocking - if (hid_set_nonblocking(m_pHidDevice, 1) == -1) { + if (hid_set_nonblocking(m_pHidDevice, 1) != 0) { qWarning() << "Unable to set HID device " << getName() << " to non-blocking"; + return -1; } setOpen(true); @@ -242,12 +245,12 @@ int HidController::close() { bool HidController::poll() { Trace hidRead("HidReader poll"); - for (int i = 0; i < 10; i++) { + for (int i = 0; i < HIDCONTROLLER_POLL_LIMIT; i++) { int result = hid_read(m_pHidDevice, m_pPollData, sizeof(m_pPollData) / sizeof(m_pPollData[0])); if (result == -1) { return false; } else if (result == 0) { - break; + return true; } Trace process("HidReader process packet"); @@ -255,6 +258,7 @@ bool HidController::poll() { receive(outData, mixxx::Time::elapsed()); } + qDebug() << "Reached maximum poll loop iterations for HID device " << getName(); return true; } From a76cb602107cedf14a75f83550e27d31d5677b09 Mon Sep 17 00:00:00 2001 From: Codecat Date: Sun, 23 Jun 2019 12:31:04 +0200 Subject: [PATCH 6/6] Only call hid_read once in poll --- src/controllers/hid/hidcontroller.cpp | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index c99aef527ad..919dc30b7d8 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -16,8 +16,6 @@ #include "controllers/controllerdebug.h" #include "util/time.h" -#define HIDCONTROLLER_POLL_LIMIT 10 - HidController::HidController(const hid_device_info deviceInfo) : m_pHidDevice(NULL) { // Copy required variables from deviceInfo, which will be freed after @@ -243,22 +241,17 @@ int HidController::close() { } bool HidController::poll() { - Trace hidRead("HidReader poll"); + Trace hidRead("HidController poll"); - for (int i = 0; i < HIDCONTROLLER_POLL_LIMIT; i++) { - int result = hid_read(m_pHidDevice, m_pPollData, sizeof(m_pPollData) / sizeof(m_pPollData[0])); - if (result == -1) { - return false; - } else if (result == 0) { - return true; - } - - Trace process("HidReader process packet"); + 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(m_pPollData), result); receive(outData, mixxx::Time::elapsed()); } - qDebug() << "Reached maximum poll loop iterations for HID device " << getName(); return true; }