Merge pull request #11326 from JoergAtGithub/hid_fifo_mode3

FIFO mode option for HID OutputReports

8 files changed, 210 insertions, 39 deletions

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 47f96f1ff7..883430f564 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -3028,6 +3028,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/hidcontroller.h b/src/controllers/hid/hidcontroller.h
index d735fc8db6..6f7f81108f 100644
--- a/src/controllers/hid/hidcontroller.h
+++ b/src/controllers/hid/hidcontroller.h
@@ -70,33 +70,52 @@ 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 resendUnchangedReport If set, the report will also be send, if the data are unchanged since last sending
+    /// @param reportID 1...255 for HID devices that uses ReportIDs - or 0 for
+    /// devices, which don't use ReportIDs
+    /// @param useNonSkippingFIFO (optional)
+    ///  Same as argument useNonSkippingFIFO of the sendOutputReport function,
+    ///  which is documented below
     Q_INVOKABLE void send(const QList<int>& 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/hidioglobaloutputreportfifo.cpp b/src/controllers/hid/hidioglobaloutputreportfifo.cpp
new file mode 100644
index 0000000000..c8cb0c958f
--- /dev/null
+++ b/src/controllers/hid/hidioglobaloutputreportfifo.cpp
@@ -0,0 +1,94 @@
+#include "controllers/hid/hidioglobaloutputreportfifo.h"
+
+#include <hidapi.h>
+
+#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 size_t kMaxHidErrorMessageSize = 512;
+constexpr size_t kSizeOfFifoInReports = 32;
+} // namespace
+
+HidIoGlobalOutputReportFifo::HidIoGlobalOutputReportFifo()
+        : m_fifoQueue(kSizeOfFifoInReports) {
+}
+
+void HidIoGlobalOutputReportFifo::addReportDatasetToFifo(const quint8 reportId,
+        const QByteArray& reportData,
+        const mixxx::hid::DeviceInfo& deviceInfo,
+        const RuntimeLoggingCategory& logOutput) {
+    // First byte must always be the ReportID-Byte
+    QByteArray report(reportData);
+    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));
+
+    // 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.
+        // 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();
+    }
+}
+
+bool HidIoGlobalOutputReportFifo::sendNextReportDataset(QMutex* pHidDeviceAndPollMutex,
+        hid_device* pHidDevice,
+        const mixxx::hid::DeviceInfo& deviceInfo,
+        const RuntimeLoggingCategory& logOutput) {
+    auto startOfHidWrite = mixxx::Time::elapsed();
+
+    QByteArray* pFront = m_fifoQueue.front();
+
+    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;
+    }
+
+    // Array containing the ReportID byte followed by the data to be send
+    QByteArray reportToSend(std::move(*pFront));
+    m_fifoQueue.pop();
+
+    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<const unsigned char*>(reportToSend.constData()),
+            reportToSend.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<quint8>(reportToSend[0])
+                           << ") sent from non-skipping FIFO - 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..d9735b818e
--- /dev/null
+++ b/src/controllers/hid/hidioglobaloutputreportfifo.h
@@ -0,0 +1,31 @@
+#pragma once
+
+#include "controllers/controller.h"
+#include "controllers/hid/hiddevice.h"
+#include "rigtorp/SPSCQueue.h"
+#include "util/duration.h"
+
+/// 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& reportData,
+            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:
+    // Lockless FIFO queue
+    rigtorp::SPSCQueue<QByteArray> m_fifoQueue;
+};
diff --git a/src/controllers/hid/hidiooutputreport.cpp b/src/controllers/hid/hidiooutputreport.cpp
index 80765177ac..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(
@@ -29,7 +29,8 @@ HidIoOutputReport::HidIoOutputReport(
 void HidIoOutputReport::updateCachedData(const QByteArray& data,
         const mixxx::hid::DeviceInfo& deviceInfo,
         const RuntimeLoggingCategory& logOutput,
-        bool resendUnchangedReport) {
+        HidIoGlobalOutputReportFifo* pGlobalOutputReportFifo,
+        bool useNonSkippingFIFO) {
     auto cacheLock = lockMutex(&m_cachedDataMutex);
 
     if (!m_lastCachedDataSize) {
@@ -37,27 +38,32 @@ 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"
-                               << 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();
         }
     }
 
+    // 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,
@@ -66,7 +72,6 @@ void HidIoOutputReport::updateCachedData(const QByteArray& data,
             data.constData(),
             data.size());
     m_possiblyUnsentDataCached = true;
-    m_resendUnchangedReport = resendUnchangedReport;
 }
 
 bool HidIoOutputReport::sendCachedData(QMutex* pHidDeviceAndPollMutex,
@@ -82,7 +87,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.
@@ -96,10 +101,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;
@@ -123,7 +126,7 @@ bool HidIoOutputReport::sendCachedData(QMutex* pHidDeviceAndPollMutex,
             reinterpret_cast<const unsigned char*>(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);
@@ -147,9 +150,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<quint8>(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
diff --git a/src/controllers/hid/hidiooutputreport.h b/src/controllers/hid/hidiooutputreport.h
index 61bd9bd2df..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"
 
@@ -11,10 +12,10 @@ 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);
+            HidIoGlobalOutputReportFifo* pGlobalOutputReportFifo,
+            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.
@@ -28,12 +29,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
diff --git a/src/controllers/hid/hidiothread.cpp b/src/controllers/hid/hidiothread.cpp
index 6b295f64d9..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
@@ -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++) {
@@ -188,7 +189,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<HidIoOutputReport> pNewOutputReport;
@@ -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, resendUnchangedReport);
+            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 324d95d242..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"
@@ -44,7 +45,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);
@@ -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;