From 9b3be50a899b8102d844392091a5586f28257e51 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 4 Mar 2023 09:41:51 +0100 Subject: Removed resendUnchangedReport implementation from PR #4692 form HidIoOutputReport --- src/controllers/hid/hidiooutputreport.cpp | 3 +-- src/controllers/hid/hidiooutputreport.h | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/controllers/hid/hidiooutputreport.cpp b/src/controllers/hid/hidiooutputreport.cpp index 80765177ac..cfffc5d8eb 100644 --- a/src/controllers/hid/hidiooutputreport.cpp +++ b/src/controllers/hid/hidiooutputreport.cpp @@ -66,7 +66,6 @@ void HidIoOutputReport::updateCachedData(const QByteArray& data, data.constData(), data.size()); m_possiblyUnsentDataCached = true; - m_resendUnchangedReport = resendUnchangedReport; } bool HidIoOutputReport::sendCachedData(QMutex* pHidDeviceAndPollMutex, @@ -82,7 +81,7 @@ bool HidIoOutputReport::sendCachedData(QMutex* pHidDeviceAndPollMutex, return false; } - if (!(m_resendUnchangedReport || m_lastSentData.compare(m_cachedData))) { + if (!m_lastSentData.compare(m_cachedData)) { // An HID OutputReport can contain only HID OutputItems. // HID OutputItems are defined to represent the state of one or more similar controls or LEDs. // Only HID Feature items may be attributes of other items. diff --git a/src/controllers/hid/hidiooutputreport.h b/src/controllers/hid/hidiooutputreport.h index 61bd9bd2df..1dd88f5a8b 100644 --- a/src/controllers/hid/hidiooutputreport.h +++ b/src/controllers/hid/hidiooutputreport.h @@ -11,7 +11,6 @@ class HidIoOutputReport { /// Caches new report data, which will later send by the IO thread void updateCachedData(const QByteArray& data, - const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput, bool resendUnchangedReport); @@ -28,12 +27,11 @@ class HidIoOutputReport { QByteArray m_lastSentData; /// Mutex must be locked when reading/writing m_cachedData - /// or m_possiblyUnsentDataCached, m_resendUnchangedReport + /// or m_possiblyUnsentDataCached QMutex m_cachedDataMutex; QByteArray m_cachedData; bool m_possiblyUnsentDataCached; - bool m_resendUnchangedReport; /// Due to swapping of the QbyteArrays, we need to store /// this information independent of the QBytearray size -- cgit v1.2.3 From cd0d4bcf051d8e11922a8e1280f32174b5d3b831 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 4 Mar 2023 10:02:18 +0100 Subject: Removed redundant printing of device name and serial number Nowadays the logging system prints this information at begin of each line anyway --- src/controllers/hid/hidiooutputreport.cpp | 32 +++++++++++++------------------ 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/controllers/hid/hidiooutputreport.cpp b/src/controllers/hid/hidiooutputreport.cpp index cfffc5d8eb..59b6613e9b 100644 --- a/src/controllers/hid/hidiooutputreport.cpp +++ b/src/controllers/hid/hidiooutputreport.cpp @@ -39,21 +39,17 @@ void HidIoOutputReport::updateCachedData(const QByteArray& data, } else { if (m_possiblyUnsentDataCached) { qCDebug(logOutput) << "t:" << mixxx::Time::elapsed().formatMillisWithUnit() - << "Skipped superseded OutputReport" - << deviceInfo.formatName() << "serial #" - << deviceInfo.serialNumber() << "(Report ID" - << m_reportId << ")"; + << "skipped superseded OutputReport data for ReportID" + << m_reportId; } // The size of an HID report is defined in a HID device and can't vary at runtime if (m_lastCachedDataSize != data.size()) { - qCWarning(logOutput) << "Size of report (with Report ID" - << m_reportId << ") changed from" - << m_lastCachedDataSize - << "to" << data.size() << "for" - << deviceInfo.formatName() << "serial #" - << deviceInfo.serialNumber() - << "- This indicates a bug in the mapping code!"; + qCWarning(logOutput) + << "Size of OutputReport ( with ReportID" << m_reportId + << ") changed from" << m_lastCachedDataSize << "to" + << data.size() + << "- This indicates a bug in the mapping code!"; m_lastCachedDataSize = data.size(); } } @@ -95,10 +91,8 @@ bool HidIoOutputReport::sendCachedData(QMutex* pHidDeviceAndPollMutex, cacheLock.unlock(); qCDebug(logOutput) << "t:" << startOfHidWrite.formatMillisWithUnit() - << " Skipped identical Output Report for" - << deviceInfo.formatName() << "serial #" - << deviceInfo.serialNumber() << "(Report ID" - << m_reportId << ")"; + << "Skipped sending identical OutputReport data from cache for ReportID" + << m_reportId; // Return with false, to signal the caller, that no time consuming IO operation was necessary return false; @@ -122,7 +116,7 @@ bool HidIoOutputReport::sendCachedData(QMutex* pHidDeviceAndPollMutex, reinterpret_cast(m_lastSentData.constData()), m_lastSentData.size()); if (result == -1) { - qCWarning(logOutput) << "Unable to send data to" << deviceInfo.formatName() << ":" + qCWarning(logOutput) << "Unable to send data to device :" << mixxx::convertWCStringToQString( hid_error(pHidDevice), kMaxHidErrorMessageSize); @@ -146,9 +140,9 @@ bool HidIoOutputReport::sendCachedData(QMutex* pHidDeviceAndPollMutex, } qCDebug(logOutput) << "t:" << startOfHidWrite.formatMillisWithUnit() << " " - << result << "bytes sent to" << deviceInfo.formatName() - << "serial #" << deviceInfo.serialNumber() - << "(including report ID of" << m_reportId << ") - Needed: " + << result << "bytes ( including ReportID of" + << static_cast(m_reportId) + << ") sent from skipping cache - Needed:" << (mixxx::Time::elapsed() - startOfHidWrite).formatMicrosWithUnit(); // Return with true, to signal the caller, that the time consuming hid_write operation was executed -- cgit v1.2.3 From 32ab470360bb8bc2b814137b8f49696cabd82ec6 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 4 Mar 2023 10:13:47 +0100 Subject: Renamed option resendUnchangedReport to useNonSkippingFIFO and described the new behavior --- src/controllers/hid/hidcontroller.h | 44 ++++++++++++++++++++++++++----- src/controllers/hid/hidiooutputreport.cpp | 2 +- src/controllers/hid/hidiooutputreport.h | 2 +- src/controllers/hid/hidiothread.cpp | 4 +-- src/controllers/hid/hidiothread.h | 2 +- 5 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index d735fc8db6..05e57ed366 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -71,32 +71,64 @@ class HidControllerJSProxy : public ControllerJSProxy { /// @param dataList Data to send as list of bytes /// @param length Unused but mandatory argument /// @param reportID 1...255 for HID devices that uses ReportIDs - or 0 for devices, which don't use ReportIDs - /// @param resendUnchangedReport If set, the report will also be send, if the data are unchanged since last sending + /// @param useNonSkippingFIFO (optional) + /// - False (default): + /// - Reports with identical data will be sent only once. + /// - If reports were superseded by newer data before they could be sent, + /// the oudated data will be skipped. + /// - This mode works for all USB HID class compatible reports, + /// in these each field represents the state of a control (e.g. an LED). + /// - This mode works best in overload situations, where more reports + /// are to be sent, than can be processed. + /// - True: + /// - The report will not be skipped under any circumstances, + /// except FIFO memory overflow. + /// - All reports with useNonSkippingFIFO set True will be send before + /// any cached report with useNonSkippingFIFO set False. + /// - All reports with useNonSkippingFIFO set True will be send in + /// strict First In / First Out (FIFO) order. + /// - Limit the use of this mode to the places, where it is really necessary. Q_INVOKABLE void send(const QList& dataList, unsigned int length, quint8 reportID, - bool resendUnchangedReport = false) { + bool useNonSkippingFIFO = false) { Q_UNUSED(length); QByteArray dataArray; dataArray.reserve(dataList.size()); for (int datum : dataList) { dataArray.append(datum); } - sendOutputReport(reportID, dataArray, resendUnchangedReport); + sendOutputReport(reportID, dataArray, useNonSkippingFIFO); } /// @brief Sends an OutputReport to HID device /// @param reportID 1...255 for HID devices that uses ReportIDs - or 0 for devices, which don't use ReportIDs /// @param dataArray Data to send as byte array (Javascript type Uint8Array) - /// @param resendUnchangedReport If set, the report will also be send, if the data are unchanged since last sending + /// @param useNonSkippingFIFO (optional) + /// - False (default): + /// - Reports with identical data will be sent only once. + /// - If reports were superseded by newer data before they could be sent, + /// the oudated data will be skipped. + /// - This mode works for all USB HID class compatible reports, + /// in these each field represents the state of a control (e.g. an LED). + /// - This mode works best in overload situations, where more reports + /// are to be sent, than can be processed. + /// - True: + /// - The report will not be skipped under any circumstances, + /// except FIFO memory overflow. + /// - All reports with useNonSkippingFIFO set True will be send before + /// any cached report with useNonSkippingFIFO set False. + /// - All reports with useNonSkippingFIFO set True will be send in + /// strict First In / First Out (FIFO) order. + /// - Limit the use of this mode to the places, where it is really necessary. Q_INVOKABLE void sendOutputReport(quint8 reportID, const QByteArray& dataArray, - bool resendUnchangedReport = false) { + bool useNonSkippingFIFO = false) { VERIFY_OR_DEBUG_ASSERT(m_pHidController->m_pHidIoThread) { return; } m_pHidController->m_pHidIoThread->updateCachedOutputReportData( - reportID, dataArray, resendUnchangedReport); + reportID, dataArray, useNonSkippingFIFO); } /// @brief getInputReport receives an InputReport from the HID device on request. diff --git a/src/controllers/hid/hidiooutputreport.cpp b/src/controllers/hid/hidiooutputreport.cpp index 59b6613e9b..353388e129 100644 --- a/src/controllers/hid/hidiooutputreport.cpp +++ b/src/controllers/hid/hidiooutputreport.cpp @@ -29,7 +29,7 @@ HidIoOutputReport::HidIoOutputReport( void HidIoOutputReport::updateCachedData(const QByteArray& data, const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput, - bool resendUnchangedReport) { + bool useNonSkippingFIFO) { auto cacheLock = lockMutex(&m_cachedDataMutex); if (!m_lastCachedDataSize) { diff --git a/src/controllers/hid/hidiooutputreport.h b/src/controllers/hid/hidiooutputreport.h index 1dd88f5a8b..4358bc8425 100644 --- a/src/controllers/hid/hidiooutputreport.h +++ b/src/controllers/hid/hidiooutputreport.h @@ -13,7 +13,7 @@ class HidIoOutputReport { void updateCachedData(const QByteArray& data, const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput, - bool resendUnchangedReport); + bool useNonSkippingFIFO); /// Sends the OutputReport to the HID device, when changed data are cached. /// Returns true if a time consuming hid_write operation was executed. diff --git a/src/controllers/hid/hidiothread.cpp b/src/controllers/hid/hidiothread.cpp index 6b295f64d9..fc585f3959 100644 --- a/src/controllers/hid/hidiothread.cpp +++ b/src/controllers/hid/hidiothread.cpp @@ -188,7 +188,7 @@ QByteArray HidIoThread::getInputReport(quint8 reportID) { void HidIoThread::updateCachedOutputReportData(quint8 reportID, const QByteArray& data, - bool resendUnchangedReport) { + bool useNonSkippingFIFO) { auto mapLock = lockMutex(&m_outputReportMapMutex); if (m_outputReports.find(reportID) == m_outputReports.end()) { std::unique_ptr pNewOutputReport; @@ -205,7 +205,7 @@ void HidIoThread::updateCachedOutputReportData(quint8 reportID, mapLock.unlock(); actualOutputReportIterator->second->updateCachedData( - data, m_deviceInfo, m_logOutput, resendUnchangedReport); + data, m_deviceInfo, m_logOutput, useNonSkippingFIFO); } bool HidIoThread::sendNextCachedOutputReport() { diff --git a/src/controllers/hid/hidiothread.h b/src/controllers/hid/hidiothread.h index 324d95d242..12f7029908 100644 --- a/src/controllers/hid/hidiothread.h +++ b/src/controllers/hid/hidiothread.h @@ -44,7 +44,7 @@ class HidIoThread : public QThread { void updateCachedOutputReportData(quint8 reportID, const QByteArray& reportData, - bool resendUnchangedReport); + bool useNonSkippingFIFO); QByteArray getInputReport(quint8 reportID); void sendFeatureReport(quint8 reportID, const QByteArray& reportData); QByteArray getFeatureReport(quint8 reportID); -- cgit v1.2.3 From c7be39f16d6cf9da4abb87d4354abd9025edc150 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 4 Mar 2023 10:24:11 +0100 Subject: Added implementation of FIFO for OutputReports --- CMakeLists.txt | 1 + .../hid/hidioglobaloutputreportfifo.cpp | 131 +++++++++++++++++++++ src/controllers/hid/hidioglobaloutputreportfifo.h | 41 +++++++ 3 files changed, 173 insertions(+) create mode 100644 src/controllers/hid/hidioglobaloutputreportfifo.cpp create mode 100644 src/controllers/hid/hidioglobaloutputreportfifo.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 27d67fa50c..f2b74c695a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2935,6 +2935,7 @@ if(HID) target_sources(mixxx-lib PRIVATE src/controllers/hid/hidcontroller.cpp src/controllers/hid/hidiothread.cpp + src/controllers/hid/hidioglobaloutputreportfifo.cpp src/controllers/hid/hidiooutputreport.cpp src/controllers/hid/hiddevice.cpp src/controllers/hid/hidenumerator.cpp diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp new file mode 100644 index 0000000000..c89e2d10d8 --- /dev/null +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -0,0 +1,131 @@ +#include "controllers/hid/hidioglobaloutputreportfifo.h" + +#include + +#include "controllers/defs_controllers.h" +#include "controllers/hid/legacyhidcontrollermappingfilehandler.h" +#include "util/compatibility/qbytearray.h" +#include "util/string.h" +#include "util/time.h" +#include "util/trace.h" + +namespace { +constexpr int kReportIdSize = 1; +constexpr int kMaxHidErrorMessageSize = 512; +} // namespace + +HidIoGlobalOutputReportFifo::HidIoGlobalOutputReportFifo() + : m_indexOfLastSentReport(0), + m_indexOfLastCachedReport(0) { +} + +void HidIoGlobalOutputReportFifo::addReportDatasetToFifo(const quint8 reportId, + const QByteArray& data, + const mixxx::hid::DeviceInfo& deviceInfo, + const RuntimeLoggingCategory& logOutput) { + auto cacheLock = lockMutex(&m_fifoMutex); + + unsigned int indexOfReportToCache; + + if (m_indexOfLastCachedReport + 1 < kSizeOfFifoInReports) { + indexOfReportToCache = m_indexOfLastCachedReport + 1; + } else { + indexOfReportToCache = 0; + } + + // Handle the case, that the FIFO is full - which is an error case + if (m_indexOfLastSentReport == indexOfReportToCache) { + // If the FIFO is full, we skip the report dataset even + // in non-skipping mode, to keep the controller mapping thread + // responsive for InputReports from the controller. + // Alternative would be to block processing of the controller + // mapping thread, until the FIFO has space again. + qCWarning(logOutput) + << "FIFO overflow: Unable to add OutputReport " << reportId + << "to the global cache for non-skipping sending of OututReports for" + << deviceInfo.formatName(); + return; + } + + // First byte must always contain the correct ReportID-Byte, + // also after swapping + QByteArray cachedData; + cachedData.reserve(kReportIdSize + data.size()); + cachedData.append(reportId); + cachedData.append(data); + + // Swap report data to FIFO + cachedData.swap(m_outputReportFifo[indexOfReportToCache]); + + m_indexOfLastCachedReport = indexOfReportToCache; +} + +bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPollMutex, + hid_device* pHidDevice, + const mixxx::hid::DeviceInfo& deviceInfo, + const RuntimeLoggingCategory& logOutput) { + auto startOfHidWrite = mixxx::Time::elapsed(); + + auto fifoLock = lockMutex(&m_fifoMutex); + + if (m_indexOfLastSentReport == m_indexOfLastCachedReport) { + // No data in FIFO to be send + // Return with false, to signal the caller, that no time consuming IO + // operation was ncessary + return false; + } + + // Store old values for use in controller debug output after fifoLock.unlock() + unsigned int indexOfLastCachedReport = m_indexOfLastCachedReport; + unsigned int indexOfLastSentReport = m_indexOfLastSentReport; + + // Preemptively set m_indexOfLastSentReport and swap + // m_outputReportFifo[m_indexOfLastSentReport], to release the fifoLock + // mutex before the time consuming hid_write operation. + if (m_indexOfLastSentReport + 1 < kSizeOfFifoInReports) { + m_indexOfLastSentReport++; + } else { + m_indexOfLastSentReport = 0; + } + + QByteArray dataToSend; + dataToSend.swap(m_outputReportFifo[m_indexOfLastSentReport]); + + fifoLock.unlock(); + + auto hidDeviceLock = lockMutex(pHidDeviceAndPollMutex); + + // hid_write can take several milliseconds, because hidapi synchronizes + // the asyncron HID communication from the OS + int result = hid_write(pHidDevice, + reinterpret_cast(dataToSend.constData()), + dataToSend.size()); + if (result == -1) { + qCWarning(logOutput) << "Unable to send data to" << deviceInfo.formatName() << ":" + << mixxx::convertWCStringToQString( + hid_error(pHidDevice), + kMaxHidErrorMessageSize); + } + + hidDeviceLock.unlock(); + + if (result != -1) { + qCDebug(logOutput) << "t:" << startOfHidWrite.formatMillisWithUnit() + << " " << result << "bytes (including ReportID of" + << static_cast(dataToSend[0]) + << ") sent from non-skipping FIFO (" + << (indexOfLastCachedReport > indexOfLastSentReport + ? indexOfLastCachedReport - + indexOfLastSentReport + : kSizeOfFifoInReports - + indexOfLastSentReport + + indexOfLastCachedReport) + << "/" << kSizeOfFifoInReports << "used) - Needed: " + << (mixxx::Time::elapsed() - startOfHidWrite) + .formatMicrosWithUnit(); + } + + // Return with true, to signal the caller, that the time consuming hid_write + // operation was executed + return true; +} diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.h b/src/controllers/hid/hidioglobaloutputreportfifo.h new file mode 100644 index 0000000000..67fdab15d2 --- /dev/null +++ b/src/controllers/hid/hidioglobaloutputreportfifo.h @@ -0,0 +1,41 @@ +#pragma once + +#include "controllers/controller.h" +#include "controllers/hid/hiddevice.h" +#include "util/compatibility/qmutex.h" +#include "util/duration.h" + +namespace { +constexpr int kSizeOfFifoInReports = 32; +} + +/// Stores and sends OutputReports (independent of the ReportID) in First In / +/// First Out (FIFO) order +class HidIoGlobalOutputReportFifo { + public: + HidIoGlobalOutputReportFifo(); + + /// Caches new OutputReport to the FIFO, which will later be send by the IO thread + void addReportDatasetToFifo(const quint8 reportId, + const QByteArray& data, + const mixxx::hid::DeviceInfo& deviceInfo, + const RuntimeLoggingCategory& logOutput); + + /// Sends the next OutputReport from FIFO to the HID device, + /// when if any report is cached in FIFO. + /// Returns true if a time consuming hid_write operation was executed. + bool sendNextReportDataset(QMutex* pHidDeviceAndPollMutex, + hid_device* pHidDevice, + const mixxx::hid::DeviceInfo& deviceInfo, + const RuntimeLoggingCategory& logOutput); + + private: + QByteArray m_outputReportFifo[kSizeOfFifoInReports]; + unsigned int m_indexOfLastSentReport; + unsigned int m_indexOfLastCachedReport; + + /// Mutex must be locked when reading/writing + /// m_outputReportFifo, m_indexOfLastSentReport + /// or m_indexOfLastCachedReport + QMutex m_fifoMutex; +}; -- cgit v1.2.3 From a3cc7d590e80768ea472c7dddf30f8c91f65b6f6 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 4 Mar 2023 12:13:31 +0100 Subject: Use the new OutputReport FIFO as alternative mode in HidIoThread --- src/controllers/hid/hidiooutputreport.cpp | 12 +++++++++++- src/controllers/hid/hidiooutputreport.h | 2 ++ src/controllers/hid/hidiothread.cpp | 22 +++++++++++++++++++++- src/controllers/hid/hidiothread.h | 3 +++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/controllers/hid/hidiooutputreport.cpp b/src/controllers/hid/hidiooutputreport.cpp index 353388e129..ed3675b33e 100644 --- a/src/controllers/hid/hidiooutputreport.cpp +++ b/src/controllers/hid/hidiooutputreport.cpp @@ -29,6 +29,7 @@ HidIoOutputReport::HidIoOutputReport( void HidIoOutputReport::updateCachedData(const QByteArray& data, const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput, + HidIoGlobalOutputReportFifo* pGlobalOutputReportFifo, bool useNonSkippingFIFO) { auto cacheLock = lockMutex(&m_cachedDataMutex); @@ -37,7 +38,7 @@ void HidIoOutputReport::updateCachedData(const QByteArray& data, m_lastCachedDataSize = data.size(); } else { - if (m_possiblyUnsentDataCached) { + if (m_possiblyUnsentDataCached && !useNonSkippingFIFO) { qCDebug(logOutput) << "t:" << mixxx::Time::elapsed().formatMillisWithUnit() << "skipped superseded OutputReport data for ReportID" << m_reportId; @@ -54,6 +55,15 @@ void HidIoOutputReport::updateCachedData(const QByteArray& data, } } + // m_possiblyUnsentDataCached must be set while m_cachedDataMutex is locked + // This step covers the case that data for the report are cached in skipping mode, + // succeed by a non-skipping send of the same report + if (useNonSkippingFIFO) { + m_possiblyUnsentDataCached = false; + m_lastSentData.clear(); + return; + } + // Deep copy with reusing the already allocated heap memory // The first byte with the ReportID is not overwritten qByteArrayReplaceWithPositionAndSize(&m_cachedData, diff --git a/src/controllers/hid/hidiooutputreport.h b/src/controllers/hid/hidiooutputreport.h index 4358bc8425..e185410d58 100644 --- a/src/controllers/hid/hidiooutputreport.h +++ b/src/controllers/hid/hidiooutputreport.h @@ -2,6 +2,7 @@ #include "controllers/controller.h" #include "controllers/hid/hiddevice.h" +#include "controllers/hid/hidioglobaloutputreportfifo.h" #include "util/compatibility/qmutex.h" #include "util/duration.h" @@ -13,6 +14,7 @@ class HidIoOutputReport { void updateCachedData(const QByteArray& data, const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput, + HidIoGlobalOutputReportFifo* pGlobalOutputReportFifo, bool useNonSkippingFIFO); /// Sends the OutputReport to the HID device, when changed data are cached. diff --git a/src/controllers/hid/hidiothread.cpp b/src/controllers/hid/hidiothread.cpp index fc585f3959..4778c6ccff 100644 --- a/src/controllers/hid/hidiothread.cpp +++ b/src/controllers/hid/hidiothread.cpp @@ -43,6 +43,7 @@ HidIoThread::HidIoThread( m_pHidDevice(pHidDevice), m_lastPollSize(0), m_pollingBufferIndex(0), + m_globalOutputReportFifo(), m_runLoopSemaphore(1) { // Initializing isn't strictly necessary but is good practice. for (int i = 0; i < kNumBuffers; i++) { @@ -204,11 +205,30 @@ void HidIoThread::updateCachedOutputReportData(quint8 reportID, mapLock.unlock(); + // If useNonSkippingFIFO is false, the report data are cached here + // If useNonSkippingFIFO is true, this cache is cleared actualOutputReportIterator->second->updateCachedData( - data, m_deviceInfo, m_logOutput, useNonSkippingFIFO); + data, m_deviceInfo, m_logOutput, &m_globalOutputReportFifo, useNonSkippingFIFO); + + // If useNonSkippingFIFO is true, put the new report dataset on the FIFO + if (useNonSkippingFIFO) { + m_globalOutputReportFifo.addReportDatasetToFifo(reportID, data, m_deviceInfo, m_logOutput); + } } bool HidIoThread::sendNextCachedOutputReport() { + // 1.) Send non-skipping reports from FIFO + if (m_globalOutputReportFifo.sendNextReportDataset(&m_hidDeviceAndPollMutex, + m_pHidDevice, + m_deviceInfo, + m_logOutput)) { + // Return after each time consuming sendCachedData + return true; + } + + // 2.) If non non-skipping reports were in the FIFO, send the skipable reports + // from the m_outputReports cache + // m_outputReports.size() doesn't need mutex protection, because the value of i is not used. // i is just a counter to prevent infinite loop execution. // If the map size increases, this loop will execute one iteration more, diff --git a/src/controllers/hid/hidiothread.h b/src/controllers/hid/hidiothread.h index 12f7029908..96faf5b98c 100644 --- a/src/controllers/hid/hidiothread.h +++ b/src/controllers/hid/hidiothread.h @@ -7,6 +7,7 @@ #include "controllers/controller.h" #include "controllers/hid/hiddevice.h" +#include "controllers/hid/hidioglobaloutputreportfifo.h" #include "controllers/hid/hidiooutputreport.h" #include "util/compatibility/qmutex.h" #include "util/duration.h" @@ -92,6 +93,8 @@ class HidIoThread : public QThread { OutputReportMap m_outputReports; OutputReportMap::iterator m_outputReportIterator; + HidIoGlobalOutputReportFifo m_globalOutputReportFifo; + /// State of the HidIoThread lifecycle QAtomicInt m_state; -- cgit v1.2.3 From 49f5d062b636e36489b73be6e8371ee91fe30fa2 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 25 Mar 2023 13:19:30 +0100 Subject: Changed HidIoGlobalOutputReportFifo implementation to lockless SPSCQueue Use QByteArrays prepend function, which will be a "very fast constant time" function when we switch to Qt6 --- .../hid/hidioglobaloutputreportfifo.cpp | 70 ++++++---------------- src/controllers/hid/hidioglobaloutputreportfifo.h | 18 ++---- 2 files changed, 22 insertions(+), 66 deletions(-) diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp index c89e2d10d8..91db0703da 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.cpp +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -5,36 +5,33 @@ #include "controllers/defs_controllers.h" #include "controllers/hid/legacyhidcontrollermappingfilehandler.h" #include "util/compatibility/qbytearray.h" +#include "util/compatibility/qmutex.h" #include "util/string.h" #include "util/time.h" #include "util/trace.h" namespace { -constexpr int kReportIdSize = 1; constexpr int kMaxHidErrorMessageSize = 512; +constexpr int kSizeOfFifoInReports = 32; } // namespace HidIoGlobalOutputReportFifo::HidIoGlobalOutputReportFifo() - : m_indexOfLastSentReport(0), - m_indexOfLastCachedReport(0) { + : m_fifoQueue(kSizeOfFifoInReports) { } void HidIoGlobalOutputReportFifo::addReportDatasetToFifo(const quint8 reportId, - const QByteArray& data, + const QByteArray& reportData, const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput) { - auto cacheLock = lockMutex(&m_fifoMutex); + // First byte must always be the ReportID-Byte + QByteArray report(std::move(reportData)); + report.prepend(reportId); - unsigned int indexOfReportToCache; + // Swap report to lockless FIFO queue + bool success = m_fifoQueue.try_emplace(std::move(report)); - if (m_indexOfLastCachedReport + 1 < kSizeOfFifoInReports) { - indexOfReportToCache = m_indexOfLastCachedReport + 1; - } else { - indexOfReportToCache = 0; - } - - // Handle the case, that the FIFO is full - which is an error case - if (m_indexOfLastSentReport == indexOfReportToCache) { + // Handle the case, that the FIFO queue is full - which is an error case + if (!success) { // If the FIFO is full, we skip the report dataset even // in non-skipping mode, to keep the controller mapping thread // responsive for InputReports from the controller. @@ -44,20 +41,7 @@ void HidIoGlobalOutputReportFifo::addReportDatasetToFifo(const quint8 reportId, << "FIFO overflow: Unable to add OutputReport " << reportId << "to the global cache for non-skipping sending of OututReports for" << deviceInfo.formatName(); - return; } - - // First byte must always contain the correct ReportID-Byte, - // also after swapping - QByteArray cachedData; - cachedData.reserve(kReportIdSize + data.size()); - cachedData.append(reportId); - cachedData.append(data); - - // Swap report data to FIFO - cachedData.swap(m_outputReportFifo[indexOfReportToCache]); - - m_indexOfLastCachedReport = indexOfReportToCache; } bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPollMutex, @@ -66,32 +50,19 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol const RuntimeLoggingCategory& logOutput) { auto startOfHidWrite = mixxx::Time::elapsed(); - auto fifoLock = lockMutex(&m_fifoMutex); + auto pFront = m_fifoQueue.front(); - if (m_indexOfLastSentReport == m_indexOfLastCachedReport) { + if (pFront == nullptr) { // No data in FIFO to be send // Return with false, to signal the caller, that no time consuming IO // operation was ncessary return false; } - // Store old values for use in controller debug output after fifoLock.unlock() - unsigned int indexOfLastCachedReport = m_indexOfLastCachedReport; - unsigned int indexOfLastSentReport = m_indexOfLastSentReport; - - // Preemptively set m_indexOfLastSentReport and swap - // m_outputReportFifo[m_indexOfLastSentReport], to release the fifoLock - // mutex before the time consuming hid_write operation. - if (m_indexOfLastSentReport + 1 < kSizeOfFifoInReports) { - m_indexOfLastSentReport++; - } else { - m_indexOfLastSentReport = 0; - } - - QByteArray dataToSend; - dataToSend.swap(m_outputReportFifo[m_indexOfLastSentReport]); + auto fifoUsedEntries = m_fifoQueue.size(); - fifoLock.unlock(); + QByteArray dataToSend(std::move(*pFront)); + m_fifoQueue.pop(); auto hidDeviceLock = lockMutex(pHidDeviceAndPollMutex); @@ -114,13 +85,8 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol << " " << result << "bytes (including ReportID of" << static_cast(dataToSend[0]) << ") sent from non-skipping FIFO (" - << (indexOfLastCachedReport > indexOfLastSentReport - ? indexOfLastCachedReport - - indexOfLastSentReport - : kSizeOfFifoInReports - - indexOfLastSentReport + - indexOfLastCachedReport) - << "/" << kSizeOfFifoInReports << "used) - Needed: " + << fifoUsedEntries << "/" << kSizeOfFifoInReports + << "used) - Needed: " << (mixxx::Time::elapsed() - startOfHidWrite) .formatMicrosWithUnit(); } diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.h b/src/controllers/hid/hidioglobaloutputreportfifo.h index 67fdab15d2..d9735b818e 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.h +++ b/src/controllers/hid/hidioglobaloutputreportfifo.h @@ -2,13 +2,9 @@ #include "controllers/controller.h" #include "controllers/hid/hiddevice.h" -#include "util/compatibility/qmutex.h" +#include "rigtorp/SPSCQueue.h" #include "util/duration.h" -namespace { -constexpr int kSizeOfFifoInReports = 32; -} - /// Stores and sends OutputReports (independent of the ReportID) in First In / /// First Out (FIFO) order class HidIoGlobalOutputReportFifo { @@ -17,7 +13,7 @@ class HidIoGlobalOutputReportFifo { /// Caches new OutputReport to the FIFO, which will later be send by the IO thread void addReportDatasetToFifo(const quint8 reportId, - const QByteArray& data, + const QByteArray& reportData, const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput); @@ -30,12 +26,6 @@ class HidIoGlobalOutputReportFifo { const RuntimeLoggingCategory& logOutput); private: - QByteArray m_outputReportFifo[kSizeOfFifoInReports]; - unsigned int m_indexOfLastSentReport; - unsigned int m_indexOfLastCachedReport; - - /// Mutex must be locked when reading/writing - /// m_outputReportFifo, m_indexOfLastSentReport - /// or m_indexOfLastCachedReport - QMutex m_fifoMutex; + // Lockless FIFO queue + rigtorp::SPSCQueue m_fifoQueue; }; -- cgit v1.2.3 From 6b580481bfa7fedc93ae43ca3280b5cdfc3a4466 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 1 Apr 2023 21:21:33 +0200 Subject: Removed costly m_fifoQueue.size() operation, which was only needed for logging --- src/controllers/hid/hidioglobaloutputreportfifo.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp index 91db0703da..260cc9545c 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.cpp +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -59,8 +59,6 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol return false; } - auto fifoUsedEntries = m_fifoQueue.size(); - QByteArray dataToSend(std::move(*pFront)); m_fifoQueue.pop(); @@ -84,9 +82,7 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol qCDebug(logOutput) << "t:" << startOfHidWrite.formatMillisWithUnit() << " " << result << "bytes (including ReportID of" << static_cast(dataToSend[0]) - << ") sent from non-skipping FIFO (" - << fifoUsedEntries << "/" << kSizeOfFifoInReports - << "used) - Needed: " + << ") sent from non-skipping FIFO - Needed: " << (mixxx::Time::elapsed() - startOfHidWrite) .formatMicrosWithUnit(); } -- cgit v1.2.3 From e3157c6c29decf00c4b747418cf06225a7586c85 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 1 Apr 2023 21:26:06 +0200 Subject: Added commend about performance of QByteArrays prepend method --- src/controllers/hid/hidioglobaloutputreportfifo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp index 260cc9545c..56a6581852 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.cpp +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -25,7 +25,7 @@ void HidIoGlobalOutputReportFifo::addReportDatasetToFifo(const quint8 reportId, const RuntimeLoggingCategory& logOutput) { // First byte must always be the ReportID-Byte QByteArray report(std::move(reportData)); - report.prepend(reportId); + report.prepend(reportId); // In Qt6 this is a very fast operation without reallocation // Swap report to lockless FIFO queue bool success = m_fifoQueue.try_emplace(std::move(report)); -- cgit v1.2.3 From c6d8937db5aba33e208ec164f6d3cdbc391384f0 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 1 Apr 2023 21:36:31 +0200 Subject: Removed pointless std::move on const QByteArray& --- src/controllers/hid/hidioglobaloutputreportfifo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp index 56a6581852..62a0781cc2 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.cpp +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -24,7 +24,7 @@ void HidIoGlobalOutputReportFifo::addReportDatasetToFifo(const quint8 reportId, const mixxx::hid::DeviceInfo& deviceInfo, const RuntimeLoggingCategory& logOutput) { // First byte must always be the ReportID-Byte - QByteArray report(std::move(reportData)); + QByteArray report(reportData); report.prepend(reportId); // In Qt6 this is a very fast operation without reallocation // Swap report to lockless FIFO queue -- cgit v1.2.3 From 0a4833e43714d61c6543b998c8ca78df5c4db322 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 1 Apr 2023 21:55:58 +0200 Subject: Deduplicated comments for argument useNonSkippingFIFO of send and sendOutputReport --- src/controllers/hid/hidcontroller.h | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 05e57ed366..6f7f81108f 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -70,24 +70,11 @@ class HidControllerJSProxy : public ControllerJSProxy { /// @brief Sends HID OutputReport to HID device /// @param dataList Data to send as list of bytes /// @param length Unused but mandatory argument - /// @param reportID 1...255 for HID devices that uses ReportIDs - or 0 for devices, which don't use ReportIDs + /// @param reportID 1...255 for HID devices that uses ReportIDs - or 0 for + /// devices, which don't use ReportIDs /// @param useNonSkippingFIFO (optional) - /// - False (default): - /// - Reports with identical data will be sent only once. - /// - If reports were superseded by newer data before they could be sent, - /// the oudated data will be skipped. - /// - This mode works for all USB HID class compatible reports, - /// in these each field represents the state of a control (e.g. an LED). - /// - This mode works best in overload situations, where more reports - /// are to be sent, than can be processed. - /// - True: - /// - The report will not be skipped under any circumstances, - /// except FIFO memory overflow. - /// - All reports with useNonSkippingFIFO set True will be send before - /// any cached report with useNonSkippingFIFO set False. - /// - All reports with useNonSkippingFIFO set True will be send in - /// strict First In / First Out (FIFO) order. - /// - Limit the use of this mode to the places, where it is really necessary. + /// Same as argument useNonSkippingFIFO of the sendOutputReport function, + /// which is documented below Q_INVOKABLE void send(const QList& dataList, unsigned int length, quint8 reportID, -- cgit v1.2.3 From 6d7ba077f102170295e9ac3688fd128c4725b0bd Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 13 May 2023 10:11:17 +0200 Subject: Use size_t for buffer size and string len --- src/controllers/hid/hidioglobaloutputreportfifo.cpp | 4 ++-- src/controllers/hid/hidiooutputreport.cpp | 2 +- src/controllers/hid/hidiothread.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp index 62a0781cc2..39feccb9f8 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.cpp +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -11,8 +11,8 @@ #include "util/trace.h" namespace { -constexpr int kMaxHidErrorMessageSize = 512; -constexpr int kSizeOfFifoInReports = 32; +constexpr size_t kMaxHidErrorMessageSize = 512; +constexpr size_t kSizeOfFifoInReports = 32; } // namespace HidIoGlobalOutputReportFifo::HidIoGlobalOutputReportFifo() diff --git a/src/controllers/hid/hidiooutputreport.cpp b/src/controllers/hid/hidiooutputreport.cpp index ed3675b33e..2cfd7f9587 100644 --- a/src/controllers/hid/hidiooutputreport.cpp +++ b/src/controllers/hid/hidiooutputreport.cpp @@ -11,7 +11,7 @@ namespace { constexpr int kReportIdSize = 1; -constexpr int kMaxHidErrorMessageSize = 512; +constexpr size_t kMaxHidErrorMessageSize = 512; } // namespace HidIoOutputReport::HidIoOutputReport( diff --git a/src/controllers/hid/hidiothread.cpp b/src/controllers/hid/hidiothread.cpp index 4778c6ccff..dcde35af2d 100644 --- a/src/controllers/hid/hidiothread.cpp +++ b/src/controllers/hid/hidiothread.cpp @@ -13,7 +13,7 @@ namespace { constexpr int kReportIdSize = 1; -constexpr int kMaxHidErrorMessageSize = 512; +constexpr size_t kMaxHidErrorMessageSize = 512; // Sleep time of run loop, in idle case, when no time consuming operation was executed before // The lower the time the more even the CPU load is spread over time -- cgit v1.2.3 From 95adfcac5ca916057b11dce0213183a356e0ccf4 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 13 May 2023 10:48:48 +0200 Subject: Use type explicit instead of auto where the type is out of sight --- src/controllers/hid/hidioglobaloutputreportfifo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp index 39feccb9f8..8198341f7c 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.cpp +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -50,7 +50,7 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol const RuntimeLoggingCategory& logOutput) { auto startOfHidWrite = mixxx::Time::elapsed(); - auto pFront = m_fifoQueue.front(); + QByteArray* pFront = m_fifoQueue.front(); if (pFront == nullptr) { // No data in FIFO to be send -- cgit v1.2.3 From af6e60ac48b20f5d7b097976680992350bce3fac Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 13 May 2023 10:58:14 +0200 Subject: Clarified that array contains the ReportID --- src/controllers/hid/hidioglobaloutputreportfifo.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/controllers/hid/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp index 8198341f7c..c8cb0c958f 100644 --- a/src/controllers/hid/hidioglobaloutputreportfifo.cpp +++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp @@ -59,7 +59,8 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol return false; } - QByteArray dataToSend(std::move(*pFront)); + // Array containing the ReportID byte followed by the data to be send + QByteArray reportToSend(std::move(*pFront)); m_fifoQueue.pop(); auto hidDeviceLock = lockMutex(pHidDeviceAndPollMutex); @@ -67,8 +68,8 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol // hid_write can take several milliseconds, because hidapi synchronizes // the asyncron HID communication from the OS int result = hid_write(pHidDevice, - reinterpret_cast(dataToSend.constData()), - dataToSend.size()); + reinterpret_cast(reportToSend.constData()), + reportToSend.size()); if (result == -1) { qCWarning(logOutput) << "Unable to send data to" << deviceInfo.formatName() << ":" << mixxx::convertWCStringToQString( @@ -81,7 +82,7 @@ bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPol if (result != -1) { qCDebug(logOutput) << "t:" << startOfHidWrite.formatMillisWithUnit() << " " << result << "bytes (including ReportID of" - << static_cast(dataToSend[0]) + << static_cast(reportToSend[0]) << ") sent from non-skipping FIFO - Needed: " << (mixxx::Time::elapsed() - startOfHidWrite) .formatMicrosWithUnit(); -- cgit v1.2.3