From 8399b46e5c9d8c71f9da1ab9df103888b7ed3860 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sun, 2 Jan 2022 20:28:35 +0100 Subject: [PATCH] Replaced DirectConnections by Mutex-Protected function calls Use standard Event Loop for HidIoThread Removed no longer needed implementation of IsPolling --- src/controllers/hid/hidcontroller.cpp | 68 +++++++-------------------- src/controllers/hid/hidcontroller.h | 45 +++++++----------- src/controllers/hid/hidio.cpp | 49 ++++++++++++------- src/controllers/hid/hidio.h | 36 +++++++------- 4 files changed, 87 insertions(+), 111 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 550247512c2..e1bc3537da9 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -14,7 +14,7 @@ HidController::HidController( : Controller(deviceInfo.formatName()), m_deviceInfo(std::move(deviceInfo)), m_pHidDevice(nullptr), - m_pHidIo(nullptr) { + m_pHidIoThread(nullptr) { setDeviceCategory(mixxx::hid::DeviceCategory::guessFromDeviceInfo(m_deviceInfo)); // All HID devices are full-duplex @@ -102,48 +102,32 @@ int HidController::open() { setOpen(true); startEngine(); - if (m_pHidIo != nullptr) { + if (m_pHidIoThread != nullptr) { qWarning() << "HidIoThread already present for" << getName(); } else { - m_pHidIo = new HidIoThread(m_pHidDevice, + m_pHidIoThread = new HidIoThread(m_pHidDevice, std::move(m_deviceInfo), m_logBase, m_logInput, m_logOutput); - m_pHidIo->setObjectName(QString("HidIoThread %1").arg(getName())); + m_pHidIoThread->setObjectName(QString("HidIoThread %1").arg(getName())); - connect(m_pHidIo, + connect(m_pHidIoThread, &HidIoThread::receive, this, &HidController::receive, Qt::QueuedConnection); - connect(this, - &HidController::getInputReport, - m_pHidIo, - &HidIoThread::getInputReport, - Qt::DirectConnection); // Enforces syncronisation of mapping and IO thread - connect(this, &HidController::sendOutputReport, - m_pHidIo, + m_pHidIoThread, &HidIoThread::sendOutputReport, Qt::QueuedConnection); - connect(this, - &HidController::getFeatureReport, - m_pHidIo, - &HidIoThread::getFeatureReport, - Qt::DirectConnection); // Enforces syncronisation of mapping and IO thread - connect(this, - &HidController::sendFeatureReport, - m_pHidIo, - &HidIoThread::sendFeatureReport, - Qt::DirectConnection); // Enforces syncronisation of mapping and IO thread - // Controller input needs to be prioritized since it can affect the // audio directly, like when scratching - m_pHidIo->start(QThread::HighPriority); + m_pHidIoThread->start(QThread::HighPriority); + m_pHidIoThread->startPollTimer(); } return 0; } @@ -157,47 +141,33 @@ int HidController::close() { qCInfo(m_logBase) << "Shutting down HID device" << getName(); // Stop the reading thread - if (m_pHidIo == nullptr) { + if (m_pHidIoThread == nullptr) { qWarning() << "HidIoThread not present for" << getName() << "yet the device is open!"; } else { - disconnect(m_pHidIo, + disconnect(m_pHidIoThread, &HidIoThread::receive, this, &HidController::receive); - disconnect(this, - &HidController::getInputReport, - m_pHidIo, - &HidIoThread::getInputReport); - disconnect(this, &HidController::sendOutputReport, - m_pHidIo, + m_pHidIoThread, &HidIoThread::sendOutputReport); - disconnect(this, - &HidController::getFeatureReport, - m_pHidIo, - &HidIoThread::getFeatureReport); - disconnect(this, - &HidController::sendFeatureReport, - m_pHidIo, - &HidIoThread::sendFeatureReport); - - m_pHidIo->stop(); + m_pHidIoThread->stop(); hid_set_nonblocking(m_pHidDevice, 1); // Quit blocking qDebug() << "Waiting on IO thread to finish"; - m_pHidIo->wait(); + m_pHidIoThread->wait(); } // Stop controller engine here to ensure it's done before the device is closed // in case it has any final parting messages stopEngine(); - if (m_pHidIo != nullptr) { - delete m_pHidIo; - m_pHidIo = nullptr; + if (m_pHidIoThread != nullptr) { + delete m_pHidIoThread; + m_pHidIoThread = nullptr; } // Close device @@ -209,12 +179,6 @@ int HidController::close() { return 0; } - - -bool HidController::isPolling() const { - return isOpen(); -} - void HidController::sendReport(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 49df74d5ea4..4217a71993c 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -40,31 +40,9 @@ class HidController final : public Controller { int close() override; private: - bool isPolling() const override; - signals: - // getInputReport receives an input report on request. - // This can be used on startup to initialize the knob positions in Mixxx - // to the physical position of the hardware knobs on the controller. - // The returned data structure for the input reports is the same - // as in the polling functionality (including ReportID in first byte). - // The returned list can be used to call the incomingData - // function of the common-hid-packet-parser. - QByteArray getInputReport(unsigned int reportID); - void sendOutputReport(const QByteArray& data, unsigned int reportID); - void sendFeatureReport(const QByteArray& reportData, unsigned int reportID); - - // getFeatureReport receives a feature reports on request. - // HID doesn't support polling feature reports, therefore this is the - // only method to get this information. - // Usually, single bits in a feature report need to be set without - // changing the other bits. The returned list matches the input - // format of sendFeatureReport, allowing it to be read, modified - // and sent it back to the controller. - QByteArray getFeatureReport(unsigned int reportID); - private: // For devices which only support a single report, reportID must be set to // 0x0. @@ -73,7 +51,7 @@ class HidController final : public Controller { const mixxx::hid::DeviceInfo m_deviceInfo; hid_device* m_pHidDevice; - HidIoThread* m_pHidIo; + HidIoThread* m_pHidIoThread; std::shared_ptr m_pMapping; friend class HidControllerJSProxy; @@ -95,19 +73,32 @@ class HidControllerJSProxy : public ControllerJSProxy { m_pHidController->sendReport(data, length, reportID); } + // getInputReport receives an input report on request. + // This can be used on startup to initialize the knob positions in Mixxx + // to the physical position of the hardware knobs on the controller. + // The returned data structure for the input reports is the same + // as in the polling functionality (including ReportID in first byte). + // The returned list can be used to call the incomingData + // function of the common-hid-packet-parser. Q_INVOKABLE QByteArray getInputReport( unsigned int reportID) { - return emit m_pHidController->getInputReport(reportID); + return m_pHidController->m_pHidIoThread->getInputReport(reportID); } Q_INVOKABLE void sendFeatureReport( const QByteArray& reportData, unsigned int reportID) { - emit m_pHidController->sendFeatureReport(reportData, reportID); + m_pHidController->m_pHidIoThread->sendFeatureReport(reportData, reportID); } - + // getFeatureReport receives a feature reports on request. + // HID doesn't support polling feature reports, therefore this is the + // only method to get this information. + // Usually, single bits in a feature report need to be set without + // changing the other bits. The returned list matches the input + // format of sendFeatureReport, allowing it to be read, modified + // and sent it back to the controller. Q_INVOKABLE QByteArray getFeatureReport( unsigned int reportID) { - return emit m_pHidController->getFeatureReport(reportID); + return m_pHidController->m_pHidIoThread->getFeatureReport(reportID); } private: diff --git a/src/controllers/hid/hidio.cpp b/src/controllers/hid/hidio.cpp index 48a74f3b46f..6e69439ea6e 100644 --- a/src/controllers/hid/hidio.cpp +++ b/src/controllers/hid/hidio.cpp @@ -2,6 +2,8 @@ #include +#include + #include "controllers/defs_controllers.h" #include "controllers/hid/legacyhidcontrollermappingfilehandler.h" #include "moc_hidio.cpp" @@ -67,7 +69,9 @@ HidIoThread::HidIoThread(hid_device* device, m_logInput(logInput), m_logOutput(logOutput), m_pHidDevice(device), - m_deviceInfo(std::move(deviceInfo)) { + m_deviceInfo(std::move(deviceInfo)), + m_HidDeviceMutex(QT_RECURSIVE_MUTEX_INIT), + mPollTimerId(0) { // This isn't strictly necessary but is good practice. for (int i = 0; i < kNumBuffers; i++) { memset(m_pPollData[i], 0, kBufferSize); @@ -75,17 +79,25 @@ HidIoThread::HidIoThread(hid_device* device, m_lastPollSize = 0; } -void HidIoThread::run() { - atomicStoreRelaxed(m_stop, 0); - while (atomicLoadRelaxed(m_stop) == 0) { - poll(); - usleep(1000); +void HidIoThread::timerEvent(QTimerEvent* event) { + poll(); +} + +void HidIoThread::startPollTimer() { + if (mPollTimerId == 0) { + mPollTimerId = startTimer(1, Qt::PreciseTimer); } } +void HidIoThread::stop() { + printf("Stop HidIoThread\n"); + killTimer(mPollTimerId); + quit(); +} + void HidIoThread::poll() { Trace hidRead("HidIoThread poll"); - + auto lock = lockMutex(&m_HidDeviceMutex); // This loop risks becoming a high priority endless loop in case processing // the mapping JS code takes longer than the controller polling rate. // This could stall other low priority tasks. @@ -134,6 +146,7 @@ void HidIoThread::processInputReport(int bytesRead) { QByteArray HidIoThread::getInputReport(unsigned int reportID) { auto startOfHidGetInputReport = mixxx::Time::elapsed(); + auto lock = lockMutex(&m_HidDeviceMutex); int bytesRead; m_pPollData[m_pollingBufferIndex][0] = reportID; @@ -159,19 +172,21 @@ QByteArray HidIoThread::getInputReport(unsigned int reportID) { return QByteArray(); } - return QByteArray::fromRawData( - reinterpret_cast(m_pPollData[m_pollingBufferIndex]), bytesRead); + // Return deep-copy of the polled data, for thread safety. + return QByteArray(reinterpret_cast(m_pPollData[m_pollingBufferIndex]), bytesRead); } void HidIoThread::sendOutputReport(const QByteArray& data, unsigned int reportID) { - if (m_outputReports.find(reportID) == m_outputReports.end()) { - std::unique_ptr pNewOutputReport; - m_outputReports[reportID] = std::make_unique( - reportID, m_pHidDevice, std::move(m_deviceInfo), m_logOutput); + { + auto lock = lockMutex(&m_HidDeviceMutex); + if (m_outputReports.find(reportID) == m_outputReports.end()) { + std::unique_ptr pNewOutputReport; + m_outputReports[reportID] = std::make_unique( + reportID, m_pHidDevice, std::move(m_deviceInfo), m_logOutput); + } + // SendOutputReports executes a hardware operation, which take several milliseconds + m_outputReports[reportID]->sendOutputReport(data); } - // SendOutputReports executes a hardware operation, which take several milliseconds - m_outputReports[reportID]->sendOutputReport(data); - // Ensure that all InputReports are read from the ring buffer, before the next OutputReport blocks the IO again poll(); // Polling available Input-Reports is a cheap software only operation, which takes insignificiant time } @@ -179,6 +194,7 @@ void HidIoThread::sendOutputReport(const QByteArray& data, unsigned int reportID void HidIoThread::sendFeatureReport( const QByteArray& reportData, unsigned int reportID) { auto startOfHidSendFeatureReport = mixxx::Time::elapsed(); + auto lock = lockMutex(&m_HidDeviceMutex); QByteArray dataArray; dataArray.reserve(kReportIdSize + reportData.size()); @@ -213,6 +229,7 @@ void HidIoThread::sendFeatureReport( QByteArray HidIoThread::getFeatureReport( unsigned int reportID) { auto startOfHidGetFeatureReport = mixxx::Time::elapsed(); + auto lock = lockMutex(&m_HidDeviceMutex); unsigned char dataRead[kReportIdSize + kBufferSize]; dataRead[0] = reportID; diff --git a/src/controllers/hid/hidio.h b/src/controllers/hid/hidio.h index 4a073c8d659..f3b8772274e 100644 --- a/src/controllers/hid/hidio.h +++ b/src/controllers/hid/hidio.h @@ -6,6 +6,7 @@ #include "controllers/controller.h" #include "controllers/hid/hiddevice.h" #include "util/compatibility/qatomic.h" +#include "util/compatibility/qmutex.h" #include "util/duration.h" class HidIoReport { @@ -34,38 +35,41 @@ class HidIoThread : public QThread { const RuntimeLoggingCategory& logInput, const RuntimeLoggingCategory& logOutput); - void stop() { - atomicStoreRelaxed(m_stop, 1); - } + void stop(); - static constexpr int kNumBuffers = 2; - static constexpr int kBufferSize = 255; - unsigned char m_pPollData[kNumBuffers][kBufferSize]; - int m_lastPollSize; - int m_pollingBufferIndex; - const RuntimeLoggingCategory m_logBase; - const RuntimeLoggingCategory m_logInput; - const RuntimeLoggingCategory m_logOutput; + QByteArray getInputReport(unsigned int reportID); + void sendFeatureReport(const QByteArray& reportData, unsigned int reportID); + QByteArray getFeatureReport(unsigned int reportID); + void startPollTimer(); signals: /// Signals that a HID InputReport received by Interrupt triggered from HID device void receive(const QByteArray& data, mixxx::Duration timestamp); public slots: - QByteArray getInputReport(unsigned int reportID); void sendOutputReport(const QByteArray& reportData, unsigned int reportID); - void sendFeatureReport(const QByteArray& reportData, unsigned int reportID); - QByteArray getFeatureReport(unsigned int reportID); protected: - void run(); + void timerEvent(QTimerEvent* event) override; private: + static constexpr int kNumBuffers = 2; + static constexpr int kBufferSize = 255; + unsigned char m_pPollData[kNumBuffers][kBufferSize]; + int m_lastPollSize; + int m_pollingBufferIndex; + const RuntimeLoggingCategory m_logBase; + const RuntimeLoggingCategory m_logInput; + const RuntimeLoggingCategory m_logOutput; + void poll(); void processInputReport(int bytesRead); hid_device* const m_pHidDevice; // const pointer to the C data structure, which hidapi uses for communication between functions const mixxx::hid::DeviceInfo m_deviceInfo; - QAtomicInt m_stop; std::map> m_outputReports; + + // Must be locked when using the m_pHidDevice and it's properties, which is not thread-safe for hidapi backends + QT_RECURSIVE_MUTEX m_HidDeviceMutex; + int mPollTimerId; };