Dedicated HID IO thread to prevent blocking by HIDAPIs slow processing of OutputReports #4585

JoergAtGithub · 2021-12-27T22:02:46Z

This PR fixes performance issues by slow execution of the hid_write command of HIDAPI. This can block the controller mapping thread for several milliseconds (in my case of Traktor Z2 on Windows it takes ~7ms for each OutputReport). This is a performance regression introduced in #2179. The thread safe usage is guaranteed here as well, because all HIDAPI IO happens serialized in the same thread.

Improvements of this PR:
-Ensure that the ring-buffer of InputReports is polled between hid_write of two OutputReports
-Move HID IO in a dedicated thread, this allows the JavaScript controller mapping to continue, while HIDAPI waits for completion of the data transfer of the OutputReport
-Skip sending of OutputReports, if the data didn't changed - as already implemented for polled InputReports
-Skip sending superseded OutputReports, if the mapping sends them faster than HIDAPI can send them
-Added timing information to --controllerDebug output, that mapping developers can understand the timing behavior
-It does no longer relies on timers, which behave unreliable for the needed short periods and depend on the different OSes.

… block the controller thread for several milliseconds per OutputReport: -Ensure that the ring-buffer of InputReports is polled between hid_write of two OutputReports -Move HID IO in a dedicated thread, this allows the JavaScript controller mapping to continue, while HIDAPI waits for completion of the data transfer of the OutputReport -Skip sending of OutputReports, if the data didn't changed - as already implemented for polled InputReports -Added timing information to --controllerDebug output, that mapping developers can understand the timing behavior

Be-ing · 2021-12-28T00:26:31Z

Does this risk bringing back the deadlock issue solved in #2179?

JoergAtGithub · 2021-12-28T00:40:35Z

Does this risk bringing back the deadlock issue solved in #2179?

No, the reason for the deadlock was that hid_read and hid_write were used in different threads, but both used the same non-thread-safe device datastructure of hidapi.
This is not the case here, all HID IO is implemented in the same thread.

src/controllers/hid/hidio.h

src/controllers/hid/hidio.cpp

src/controllers/hid/hidcontroller.cpp

src/controllers/hid/hidio.cpp

src/controllers/hid/hidio.h

uklotzde · 2021-12-28T16:33:26Z

Please note that I am not planning to decide when the PR is ready to be merged. I am done with C++ and consider myself unable to do this. Just wanted to point out my serious concerns.

Removed unnecessary parent QObject Make the pointers to hid_device structure const, but the structure itself is not const. Added a comment to explain this. Use default constructor for m_lastSentOutputReport Deleted empty deconstructors Use atomicStoreRelaxed(m_stop instead of m_stop =

Be-ing · 2021-12-28T23:25:14Z

Please separate distinct changes into separate commits whenever feasible. You don't need to go back and split up the last commit, but please keep that in mind going forward. If we want to squash them at the end, we can.

Use standard Event Loop for HidIoThread Removed no longer needed implementation of IsPolling

…ray buildup

src/controllers/hid/hidio.h

… the performance of the logging statements from milliseconds to some microseconds

CMakeLists.txt

src/controllers/hid/hidiooutputreport.cpp

src/controllers/hid/hidiothread.cpp

src/controllers/hid/hidcontroller.cpp

src/controllers/hid/hidiothread.h

uklotzde · 2022-02-20T09:32:19Z

@ywwg Please update your review after you requested changes for issues that have long been resolved.

…tects the input report polling data Improved the comment describing this mutex

uklotzde

Thank you, Jörg! Your commit messages helped a lot to keep track of the changes for the re-reviews.

I haven't tested the code in action yet, maybe later this week. But this should not block merging. It has already been confirmed that outsourcing the interaction with the HID API into a separate thread has improved the situation.

No promises that the code is correct. Nevertheless, LGTM

uklotzde · 2022-02-20T19:44:01Z

We should probably stash the commits before merging!

JoergAtGithub · 2022-02-20T19:57:29Z

Thank you all for your review feedback - without this, the code wouldn't have reached the maturity, that it has now!

Be-ing · 2022-02-23T18:31:08Z

src/controllers/hid/hidiooutputreport.cpp

+        // This means there is always a one to one relationship to the state of control(s)/LED(s),
+        // and if the state is not changed, there's no need to execute the time consuming hid_write again.


This is incorrect and has broken the output messages driving the motor in my Kontrol S4 Mk3 mapping. The motor stops turning if no output packets are sent. To keep a constant velocity, identical output packets must be sent repeatedly.

I can think of three ways forward:

Remove this optimization

Add an optional parameter to the send function to opt in to allowing duplicate packets.

Add a separate function like sendAllowDuplicates for this

I will look how to solve this!

The default should behavior should ignore duplicate reports. The timing and jitter of the reports is unpredictable and usually not suitable for implementing a real-time control cycle that requires exact cycle times.

I vote for adding an extra parameter to override the default behavior if needed, e.g. sendDuplicateReport

I would be fine with solution 2. too!

But I could also imagine fourth solution:
4. Add a function configureOutputReport(int ReportID, bool sendDuplicates = false) which needs to be called only once in the init section of the JS mapping code.

I would prefer option 2 to add an optional parameter to the existing send function rather than add more functions for this edge case.

I recommend to avoid stateful code if possible. Let the script pass an additional parameter on each invocation. It is less error prone and provides more flexibility.

Ok, than I will provide a PR for 2.

github-actions bot added the build label Dec 27, 2021

JoergAtGithub force-pushed the hid_io_thread2 branch 4 times, most recently from 039e29b to 387bc55 Compare December 27, 2021 23:02

JoergAtGithub force-pushed the hid_io_thread2 branch from 387bc55 to 42d5608 Compare December 27, 2021 23:11

uklotzde suggested changes Dec 28, 2021

View reviewed changes

JoergAtGithub force-pushed the hid_io_thread2 branch from 939a02e to ce1fe8b Compare December 28, 2021 21:15

JoergAtGithub force-pushed the hid_io_thread2 branch from ce1fe8b to 4ebaec9 Compare December 28, 2021 22:21

Replaced DirectConnections by Mutex-Protected function calls

8399b46

Use standard Event Loop for HidIoThread Removed no longer needed implementation of IsPolling

JoergAtGithub force-pushed the hid_io_thread2 branch from 3350280 to 8399b46 Compare January 2, 2022 22:57

JoergAtGithub added 2 commits January 3, 2022 00:05

Added QUnused for timerEvent argument

daf55d3

Replaced QByteArray prepend function with more efficient method of ar…

3cd3c3c

…ray buildup

JoergAtGithub force-pushed the hid_io_thread2 branch from 6fc976f to 3cd3c3c Compare January 4, 2022 20:01

Holzhaus reviewed Jan 5, 2022

View reviewed changes

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

JoergAtGithub force-pushed the hid_io_thread2 branch 2 times, most recently from 22007fc to da8b9b7 Compare January 5, 2022 21:49

Created local RuntimeLoggingCategories in HidIoThread, which improved…

f35c6db

… the performance of the logging statements from milliseconds to some microseconds

JoergAtGithub force-pushed the hid_io_thread2 branch 2 times, most recently from 8d65947 to f2fac26 Compare January 5, 2022 23:35

Separate class HidIoReport into dedicated files

600d02a

JoergAtGithub force-pushed the hid_io_thread2 branch from f2fac26 to 600d02a Compare January 5, 2022 23:48

Holzhaus reviewed Jan 6, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Use thread-safe deep-copy for polled data to emit, and for return values

c7dd3b9

uklotzde reviewed Feb 16, 2022

View reviewed changes

src/controllers/hid/hidiooutputreport.cpp Show resolved Hide resolved

uklotzde reviewed Feb 16, 2022

View reviewed changes

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

uklotzde reviewed Feb 16, 2022

View reviewed changes

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

uklotzde reviewed Feb 16, 2022

View reviewed changes

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

JoergAtGithub added 4 commits February 16, 2022 23:41

Spelling

65b0744

Replaced unnecessary for loop in sendFeatureReport

d4786ff

Use range-base for loop instead of Qts foreach macro

8ab1895

Use return {};

ca3e246

JoergAtGithub force-pushed the hid_io_thread2 branch from 86106e4 to ca3e246 Compare February 17, 2022 21:08

Improved mutex locker naming and style

3ba0f1c

JoergAtGithub force-pushed the hid_io_thread2 branch from b42028f to 3ba0f1c Compare February 17, 2022 23:02

JoergAtGithub requested a review from uklotzde February 17, 2022 23:53

uklotzde reviewed Feb 20, 2022

View reviewed changes

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

Renamed hid dievice protection mutex, to make clear, that it also pro…

463519c

…tects the input report polling data Improved the comment describing this mutex

uklotzde approved these changes Feb 20, 2022

View reviewed changes

uklotzde merged commit 924f7ed into mixxxdj:main Feb 20, 2022

Be-ing reviewed Feb 23, 2022

View reviewed changes

JoergAtGithub mentioned this pull request Mar 1, 2022

HID Switch to force sending of duplicate data and cleanup of JS API functions #4692

Merged

daschuer added this to the 2.4.0 milestone Jun 21, 2022

mixxxbot mentioned this pull request Aug 23, 2022

HID Same group controls jams/stutters #10438

Closed

JoergAtGithub mentioned this pull request Aug 24, 2022

HID controller thread hangs randomly on Windows #7877

Closed

JoergAtGithub deleted the hid_io_thread2 branch August 24, 2022 17:38

JoergAtGithub mentioned this pull request Aug 24, 2022

Sync Controller polling with audio buffer (Latency) #6952

Open

ronso0 linked an issue Aug 24, 2022 that may be closed by this pull request

Sync Controller polling with audio buffer (Latency) #6952

Open

Be-ing mentioned this pull request Aug 27, 2022

consecutively sent HID output messages are skipped #10828

Closed

Be-ing mentioned this pull request Feb 20, 2023

Kontrol S4 Mk3: add controller mapping #11284

Merged

JoergAtGithub mentioned this pull request Mar 4, 2023

FIFO mode option for HID OutputReports #11326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dedicated HID IO thread to prevent blocking by HIDAPIs slow processing of OutputReports #4585

Dedicated HID IO thread to prevent blocking by HIDAPIs slow processing of OutputReports #4585

JoergAtGithub commented Dec 27, 2021 •

edited

Loading

Be-ing commented Dec 28, 2021

JoergAtGithub commented Dec 28, 2021

uklotzde commented Dec 28, 2021

Be-ing commented Dec 28, 2021

uklotzde commented Feb 20, 2022

uklotzde left a comment

uklotzde commented Feb 20, 2022

JoergAtGithub commented Feb 20, 2022

Be-ing Feb 23, 2022

Be-ing Feb 23, 2022

JoergAtGithub Feb 23, 2022

uklotzde Feb 23, 2022 •

edited

Loading

JoergAtGithub Feb 23, 2022

Be-ing Feb 23, 2022

uklotzde Feb 24, 2022 •

edited

Loading

JoergAtGithub Feb 24, 2022

		// This means there is always a one to one relationship to the state of control(s)/LED(s),
		// and if the state is not changed, there's no need to execute the time consuming hid_write again.

Dedicated HID IO thread to prevent blocking by HIDAPIs slow processing of OutputReports #4585

Dedicated HID IO thread to prevent blocking by HIDAPIs slow processing of OutputReports #4585

Conversation

JoergAtGithub commented Dec 27, 2021 • edited Loading

Be-ing commented Dec 28, 2021

JoergAtGithub commented Dec 28, 2021

uklotzde commented Dec 28, 2021

Be-ing commented Dec 28, 2021

uklotzde commented Feb 20, 2022

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde commented Feb 20, 2022

JoergAtGithub commented Feb 20, 2022

Be-ing Feb 23, 2022

Choose a reason for hiding this comment

Be-ing Feb 23, 2022

Choose a reason for hiding this comment

JoergAtGithub Feb 23, 2022

Choose a reason for hiding this comment

uklotzde Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

JoergAtGithub Feb 23, 2022

Choose a reason for hiding this comment

Be-ing Feb 23, 2022

Choose a reason for hiding this comment

uklotzde Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

JoergAtGithub Feb 24, 2022

Choose a reason for hiding this comment

JoergAtGithub commented Dec 27, 2021 •

edited

Loading

uklotzde Feb 23, 2022 •

edited

Loading

uklotzde Feb 24, 2022 •

edited

Loading