Allow controller mapping to discard polling

[acolombier]

As I'm working on the screen support for S4 MK3, I realized that the BulkReader was keeping its thread under a 100% CPU usage. After some libusb debugging, it looks like the BulkReader was constantly initiating a bulk transfer, despite the device returning an empty buffer (the bulk endpoint on S4 Mk3 is write-only).

This PR allows mapping to provide information on whether or not a device should be polled or not. It also set the foundations to inhibit writing to a device.

[daschuer]

Oh, it looks like you have discovered a more serious problem. The busy loop around libusb_bulk_transfer() is IMHO a bad design. We should always limit such loops to a certain polling rate.
In this case we need to consider the audio buffer size, because we cannot consume more than one status change per audio cycle per control. I am no controller expert so I cannot elaborate details for the bulk case. I think the most critical action is turning the jog wheel.

What could be a solution for this root issue?

The solution in this PR looks reasonable, thank you. Just let's verify that it will fit to a possible solution for the root case.

[acolombier]

What could be a solution for this root issue?

So on my specific problem - devices not meant to be read/written to - I guess an extension of the mapping manifest to completely set exclusively polling/sending capabilities is the most effective solution I can think about.

For the problem you've raised - the spam of libusb_bulk_transfer - perhaps we could follow a similar approach than for HID and perform a 250us sleep if no input was process during a loop cycle?

Just let's verify that it will fit to a possible solution for the root case.

Perhaps it might be best to handle those as two separate issue? Happy to look into that polling refactor, but I'd rather do it in another PR if that okay with you!

Btw, is any Windows guru able to tell me what is wrong on Windows? My gut feeling is that a syntax isn't supported or one of my symbol naming would clash with a reserved keyword but cannot clearly tell...

[daschuer]

Perhaps it might be best to handle those as two separate issue?

Yes that's perfect. I just want to make sure both solutions are not conflicting.

[daschuer]

such issuers normally happen a ; or } is missing in the code before probably in one of the included headers.

[JoergAtGithub]

For the problem you've raised - the spam of libusb_bulk_transfer - perhaps we could follow a similar approach than for HID and perform a 250us sleep if no input was process during a loop cycle?

The approach with the free-running HidIoThread and 250us sleep proved to be very performant on all platforms, while not consuming too many resources. The main reason why it works so well, is that it is completely asyncronous to the processing cycles of other threads like audio buffer processing, and vsync. It virtually synchronizes with the timing of the HID kernel driver, and therefore utilizes the transport bus efficiently, without unused idle time.
The HidIoThread worker thread has no Qt event loop, so the time interval depends only on the kernel's CPU scheduler, which provides a very deterministic, reliable timing. But without Qt event loop, no Signal-Slot connection in the direction to the HidIoThread where possible.

[JoergAtGithub]

Btw, is any Windows guru able to tell me what is wrong on Windows? My gut feeling is that a syntax isn't supported or one of my symbol naming would clash with a reserved keyword but cannot clearly tell...

The keywords IN and OUT are macros defined in the Windows SDK headers, which are included in the Qt headers. Just rename them.
The all upper-case naming isn't compliant to our coding style anyway: https://github.com/mixxxdj/mixxx/wiki/Coding-Guidelines#forward-declared--strongly-typed-enums

[acolombier]

The keywords IN and OUT are macros defined in the Windows SDK headers, which are included in the Qt headers. Just rename them.

Thanks for your help! Hopefully my naming doesn't clash with anything anymore.

It virtually synchronizes with the timing of the HID kernel driver, and therefore utilizes the transport bus efficiently, without unused idle time.

(Disclaimer: this is my first time ever working on USB communication so please be kind if that's completely wrong 😅)

I believe the BulkReader polling thread also doesn't rely on the EventLoop at the minute, and I guess we could keep it this way and rely on kernel's CPU scheduler too in order to keep a maximum level of precision. However, from my understanding so far, bulk endpoint cannot provide any kernel synchronization like HID would, so the only way to optimize the polling is to reduce IO.
I had some more thought on the matter and I'm wondering if we could introduce some kind of exponential back-off logic on the pause between transfer, something along the line of:

• Wait 100us between polls
• If no data read for the last 5 second, change the wait to 250us
• If no data read for the next 10 second, change the wait to 500us
• If no data read for the next 15 second, change the wait to 5ms
  (values are only here for the example)

This way, bulk communication will keep low latency capabilities (e.g while using the jog wheel, faders or pots) but will also try and reduce the load on the USB stack when unused. Aligned with the ability for controllers to completely turn off polling (this PR), I guess this should allow us to get bulk in a better position. What do you think?

[JoergAtGithub]

I believe the BulkReader polling thread also doesn't rely on the EventLoop at the minute, and I guess we could keep it this way and rely on kernel's CPU scheduler too in order to keep a maximum level of precision.

Yes, that's the already implemented as it should be!

However, from my understanding so far, bulk endpoint cannot provide any kernel synchronization like HID would, so the only way to optimize the polling is to reduce IO.

It depends much on the operating systems USB implementation. I know it only for HID but guess that general USB is the same, there Windows is the only OS that allows overlapping IO (sending the next transfer, before receiving acknowlege for the last). This is why the cross-platform wrapper hidapi strictly serializes all reports. It can take several milliseconds until the acknowledgement is received.
This means, that for optimal bus utilization, we must start a pending transfer, immediately when we receive acknowledge of the last transfer. But if no transfer is pending, the thread should sleep. This sleep is not neccessary if a transfer happens, because in this case the kernel driver set the thread to sleep while libusb_bulk_transfer is executed. There is no need to vary the sleep time, 250us proved to be suitable on all OSes.

BTW: I think it's also possible to determine the direction from libusb, by evaluating the endpoint descriptor: https://libusb.sourceforge.io/api-1.0/structlibusb__endpoint__descriptor.html#a111d087a09cbeded8e15eda9127e23d2

[ywwg]

reviving this because it's necessary for S4MK3 support in general. The basic idea seems good to me, are there any objections to merging it?

[JoergAtGithub]

The functionality itself is the right thing to do, for sure. But why let the mapping creator specify the device direction, when it is always available in the USB endpoint descriptor, which can be read by libusb?

[acolombier]

In theory I agree with you @JoergAtGithub - definitely think that should be detected by libusb, but I didn't have the skillset at the time I wrote that. With my recent Windows support work, I gain some so I might give it a go, except if you would know how to implement this easily?
@ywwg worth to point that this isn't a hard blocker in the sense that the screen will still work fine as the saturated thread is isolated (and unused).

[JoergAtGithub]

Can you add a TODO comment, that this should be changed in future - and add it as task to #13004

[acolombier]

Done here - let me know if that's good enough. Also added a task via the comment (no edit permission)

[JoergAtGithub]

Done here - let me know if that's good enough. Also added a task via the comment (no edit permission)

Moved it up to description.

PR LGTM! Waiting for CI! Thank you!