FIFO mode option for HID OutputReports #11326

JoergAtGithub · 2023-03-04T10:46:42Z

While #4585 makes the Mixxx HID subsystem fast and responsive in overload situations for any device, that use reports in the sense of the USB HID Class specification, these optimizations do not work for the Traktor Kontrol S4 Mk3, which uses HID reports to transport the data of it's own protocol.
First attemp to address this S4 specific protocol was #4692 (Option for resending reports with identical data), but this was not enough. It came out #10828, that the S4 protocol also depend on sequences of reports with the same ReportID.

This PR replaces the Resend-Option from #4692 with an Option to use a FIFO for reports, which must not be skipped.

Closes: #10828

…m HidIoOutputReport

Nowadays the logging system prints this information at begin of each line anyway

…bed the new behavior

src/controllers/hid/hidioglobaloutputreportfifo.cpp

src/controllers/hid/hidioglobaloutputreportfifo.h

src/controllers/hid/hidioglobaloutputreportfifo.cpp

acolombier · 2023-03-07T19:12:11Z

Tested with #11284 and it fixes the issue!

Use QByteArrays prepend function, which will be a "very fast constant time" function when we switch to Qt6

src/controllers/hid/hidcontroller.h

src/controllers/hid/hidioglobaloutputreportfifo.cpp

uklotzde · 2023-04-01T17:36:59Z

src/controllers/hid/hidiothread.cpp

@@ -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(


Why is the mutex locked and this operation invoked even when using the FIFO strategy? The cache and the mutex should be bypassed. Ideally the dispatch strategy for each reportID would be immutable and configured at construction time. At least the FIFO report IDs need to be known in advance to avoid the locking.

The call of updateCachedData is always needed to guarantee seamless transitions between both modes.
If a skipable report is cached, it must be cleared, because otherwise it would be send after the report from the non-skipable report from the FIFO - and would overwrite with outdated data. This happens in updateCachedData as well as the check for errors.

I acknowledge that the current design is not suitable to for this optimization. But in principle the dispatch strategy should be fixed per report ID and not change with every call.

Caching the mode only for the sender would avoid locking the mutex with every call. The cache only needs to be cleared once after changing the mode.

The only datastructure per output report is currently HidIoOutputReport. Adding a additional data structure per output report would prevent the mutex locking in FIFO mode. But this mutex is only locked for the very short time of looking up the map for the map iterator of the report.
This takes nanoseconds, while the sending of a report takes several milliseconds. And in FIFO mode, calling this function must not be done in higher rate than the HID subsystem can send reports to the device. This ratio makes a mutex blocking very unlikely - and even when it happens, it will block the other thread only for several nanoseconds.
I don't think, that it makes sense to add additional code here, because the impact will be insignificant in practice.

However short and tiny the critical section might be, you have no control over the OS scheduler -> dependency inversion. Generous use of mutexes is always a red herring. They are very often used for compensating architectural deficiencies.

https://twitter.com/EshlemanMatthew/status/1641073049746251782

…r logging

…dOutputReport

acolombier · 2023-04-02T15:48:43Z

I did some spot checks on the latest version and this still seems to work just fine.

Update: 30min set and everything looked great with the WIP S4 Mk3

daschuer

Thank you for this PR. In general it should work good, though I am not able to test. But since this is optional we can merge it anyway.
I have added some suggestions to improve performance and readability.

src/controllers/hid/hidioglobaloutputreportfifo.cpp

daschuer · 2023-05-11T05:27:45Z

src/controllers/hid/hidioglobaloutputreportfifo.cpp

+} // namespace
+
+HidIoGlobalOutputReportFifo::HidIoGlobalOutputReportFifo()
+        : m_fifoQueue(kSizeOfFifoInReports) {


Is there a naming missmatch beween InReports in the constant and OutputReport in the class name?
Maybe we can avoid input and output which can change form the perspective of different classes and use Received and Transmit or something.

I think this is a missunderstanding. In is not a abreviation for Input here, its just the word in. The whole code is only for OutputReports.
InReports is the counted unit, like InMillis or InSeconds for times.

daschuer · 2023-05-11T05:37:29Z

src/controllers/hid/hidioglobaloutputreportfifo.cpp

+        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


Why? Prepend is implemented as insert()
https://github.com/qt/qtbase/blob/aaaa1e251ea43e4094e79b7161327c07db60d351/src/corelib/text/qbytearray.cpp#L898
I think by luck there are some reserve bytes. But this call also performs a deep copy of the implicit shared.
They might be some reserve bytes that avoid allocation. They should be also there with Qt5.

https://github.com/qt/qtbase/blob/9e7f1f3e5707c91fcd6dd5cf6fd30b30f4b1bd58/src/corelib/text/qbytearray.cpp#L1971-L1985

The Qt6 documentation explicitly say, that this is very fast: https://doc.qt.io/qt-6/qbytearray.html#prepend

src/controllers/hid/hidiooutputreport.cpp

src/controllers/hid/hidioglobaloutputreportfifo.cpp

daschuer · 2023-05-11T06:10:11Z

src/controllers/hid/hidioglobaloutputreportfifo.cpp

+        return false;
+    }
+
+    QByteArray dataToSend(std::move(*pFront));


dataToSend has the report ID prepended. This is not clear initially.
Can we improve the situation by always putting the report ID at the beginning?
This will also solve the reallocation of the buffer when prepending the report ID before putting it into the buffer.

The same benefit goes for
void HidController::send(QByteArray data, unsigned int reportID)
The we just not store the modified buffer.

Our Javascript API has seperated arguments for ReportID and report data. This is not only needed for backwards compatibility, but is also the cleanest way to define such an API.
I will improve naming and add a comment, to make clear that this variable contains ReportID + data.

JoergAtGithub · 2023-05-31T22:04:02Z

This is still ready to be merged:

Review remarks addressed
That the adjusted functionality now really fix the issue (Bug consecutively sent HID output messages are skipped #10828) was confirmed above
Flawless operation over a longer period of time is also confirmed above

daschuer

Thank you.
Than let's quickly add it to the 2.4-beta to have the chance for user feedback before release.

JoergAtGithub · 2023-05-31T22:26:43Z

Thank you! No HID support in 2.4 is complete without known bugs. I will prepare a blogpost in the comming days.

JoergAtGithub added 3 commits March 4, 2023 09:50

Removed resendUnchangedReport implementation from PR mixxxdj#4692 for…

9b3be50

…m HidIoOutputReport

Removed redundant printing of device name and serial number

cd0d4bc

Nowadays the logging system prints this information at begin of each line anyway

Renamed option resendUnchangedReport to useNonSkippingFIFO and descri…

32ab470

…bed the new behavior

github-actions bot added the build label Mar 4, 2023

JoergAtGithub added controller mappings hid and removed build labels Mar 4, 2023

JoergAtGithub added 2 commits March 4, 2023 12:10

Added implementation of FIFO for OutputReports

c7be39f

Use the new OutputReport FIFO as alternative mode in HidIoThread

a3cc7d5

JoergAtGithub force-pushed the hid_fifo_mode3 branch from 8a82da4 to a3cc7d5 Compare March 4, 2023 11:14

github-actions bot added the build label Mar 4, 2023

JoergAtGithub removed the build label Mar 4, 2023

uklotzde reviewed Mar 4, 2023

View reviewed changes

src/controllers/hid/hidioglobaloutputreportfifo.cpp Outdated Show resolved Hide resolved

src/controllers/hid/hidioglobaloutputreportfifo.h Outdated Show resolved Hide resolved

daschuer reviewed Mar 5, 2023

View reviewed changes

src/controllers/hid/hidioglobaloutputreportfifo.cpp Outdated Show resolved Hide resolved

daschuer reviewed Mar 5, 2023

View reviewed changes

src/controllers/hid/hidioglobaloutputreportfifo.cpp Outdated Show resolved Hide resolved

acolombier mentioned this pull request Mar 9, 2023

Kontrol S4 Mk3: add controller mapping #11284

Merged

JoergAtGithub added 2 commits March 25, 2023 11:47

Merge remote-tracking branch 'upstream/2.4' into hid_fifo_mode3

06ab280

Changed HidIoGlobalOutputReportFifo implementation to lockless SPSCQueue

49f5d06

Use QByteArrays prepend function, which will be a "very fast constant time" function when we switch to Qt6

JoergAtGithub force-pushed the hid_fifo_mode3 branch from 00a64c0 to 49f5d06 Compare March 25, 2023 12:47

Merge remote-tracking branch 'upstream/2.4' into hid_fifo_mode3

01e60de

github-actions bot added the build label Apr 1, 2023

uklotzde suggested changes Apr 1, 2023

View reviewed changes

JoergAtGithub added 4 commits April 1, 2023 21:21

Removed costly m_fifoQueue.size() operation, which was only needed fo…

6b58048

…r logging

Added commend about performance of QByteArrays prepend method

e3157c6

Removed pointless std::move on const QByteArray&

c6d8937

Deduplicated comments for argument useNonSkippingFIFO of send and sen…

0a4833e

…dOutputReport

JoergAtGithub requested a review from daschuer April 7, 2023 17:10

JoergAtGithub added this to the 2.4.0 milestone May 1, 2023

daschuer requested changes May 11, 2023

View reviewed changes

JoergAtGithub added 3 commits May 13, 2023 10:11

Use size_t for buffer size and string len

6d7ba07

Use type explicit instead of auto where the type is out of sight

95adfca

Clarified that array contains the ReportID

af6e60a

JoergAtGithub requested a review from daschuer May 13, 2023 09:45

JoergAtGithub mentioned this pull request May 25, 2023

Kontrol S4 Mk3 ES6 mapping #4056

Closed

daschuer approved these changes May 31, 2023

View reviewed changes

daschuer merged commit 53920a2 into mixxxdj:2.4 May 31, 2023

JoergAtGithub deleted the hid_fifo_mode3 branch May 31, 2023 22:26

JoergAtGithub mentioned this pull request Jun 18, 2023

hid-controller-api.d.ts: Adapted the description from hidcontroller.h for parameter useNonSkippingFIFO #11664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIFO mode option for HID OutputReports #11326

FIFO mode option for HID OutputReports #11326

JoergAtGithub commented Mar 4, 2023

acolombier commented Mar 7, 2023 •

edited

Loading

uklotzde Apr 1, 2023

JoergAtGithub Apr 1, 2023

uklotzde Apr 1, 2023

uklotzde Apr 1, 2023

JoergAtGithub Apr 2, 2023

uklotzde Apr 2, 2023 •

edited

Loading

acolombier commented Apr 2, 2023 •

edited

Loading

daschuer left a comment

daschuer May 11, 2023

JoergAtGithub May 13, 2023

daschuer May 11, 2023

Swiftb0y May 11, 2023

JoergAtGithub May 13, 2023

daschuer May 11, 2023

JoergAtGithub May 13, 2023

JoergAtGithub commented May 31, 2023

daschuer left a comment

JoergAtGithub commented May 31, 2023

FIFO mode option for HID OutputReports #11326

FIFO mode option for HID OutputReports #11326

Conversation

JoergAtGithub commented Mar 4, 2023

acolombier commented Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

acolombier commented Apr 2, 2023 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoergAtGithub commented May 31, 2023

daschuer left a comment

Choose a reason for hiding this comment

JoergAtGithub commented May 31, 2023

acolombier commented Mar 7, 2023 •

edited

Loading

uklotzde Apr 2, 2023 •

edited

Loading

acolombier commented Apr 2, 2023 •

edited

Loading