consecutively sent HID output messages are skipped

[Be-ing]

HID output with my Kontrol S4 Mk3 mapping was broken by #4585. Initially, all duplicate HID output packets were not sent. That was fixed by #4692. However, there is still another critical bug. Sometimes, Mixxx will neither send the output packet for the right jog wheel motor nor the right jog wheel LEDs. When this happens, HID output for the separate packet controlling the buttons' LEDs continues to work, so the bug seems to be specific to the output packets that are continuously sent. This is not 100% reproducible, so I am guessing there's a race condition involved. In the meantime I've had to use a commit before #4585 was merged.

[Be-ing]

This has been confirmed by @fayaaz and another user on the forum.

[Swiftb0y]

ping @JoergAtGithub

[uklotzde]

I suppose that releasing the mutex in HidIoOutputReport::sendCachedData() could be the culprit. m_lastSentData might be swapped out and modified again while hid_write() is still reading it.

[uklotzde]

@Swiftb0y Please flag this issue as a critical bug.

[uklotzde]

Untested hotfix for my guess:
fix-hidoutputreport-sendcacheddata.zip

[JoergAtGithub]

I'm currently away from my computer and writing from my phone, I can't look for this until Tuesday.
HidIoOutputReport::sendCachedData can only be called from the HidIO thread of the device. Therefore it can't be executed a second time, while hid_write execution is pending.
I guess, that the problem lies in the initialization section of the mapping. The content to be send to OutputReport 48 is immideately replaced by a new value. The HidIO thread has no chance to send it from the cache, before it's superseded by new data: https://github.com/Be-ing/mixxx/blob/355f0ba9fb158f283cde84bcc1463d4da1730c14/res/controllers/Traktor-Kontrol-S4-MK3.js#L1997
@Be-ing Could you please attach a log of the output with --controller-debug activated. There should be a message, that this happens.

[Be-ing]

Untested hotfix for my guess:
fix-hidoutputreport-sendcacheddata.zip

I tested this and it does not fix the bug.

[Be-ing]

@Be-ing Could you please attach a log of the output with --controller-debug activated. There should be a message, that this happens.

mixxx.log

[uklotzde]

The Skipped superseded OutputReport messages are worrisome. This should not happen, especially not during initialization when certain reports might only be sent once.

We probably need to introduce different buffering behavior for output reports. Reports might be discarded by default when a previous sent operation is still pending, but need to be buffered (FIFO) if requested by the caller.

[uklotzde]

Instead of caching only a single output report a ring buffer should be used to enqueue incoming updates. A capacity of 1 then reflects the current behavior.

[Be-ing]

I guess, that the problem lies in the initialization section of the mapping. The content to be send to OutputReport 48 is immideately replaced by a new value. The HidIO thread has no chance to send it from the cache, before it's superseded by new data: https://github.com/Be-ing/mixxx/blob/355f0ba9fb158f283cde84bcc1463d4da1730c14/res/controllers/Traktor-Kontrol-S4-MK3.js#L1997

This seems to be correct. Commenting out the sending of the initialization packets with 19560e2 from before #4585 was merged, turning the controller off and on again, then restarting Mixxx reproduces the bug. I'm not confident this is related to packets being sent at high frequencies.

[JoergAtGithub]

I'm working on a solution. As a bandaid, please add wait times of ~20ms at lines 1998 and 2015 to your mapping.

[Be-ing]

Sending the second initialization packet on a 35 ms timer hacks around the problem.

[Be-ing]

@JoergAtGithub have you had a chance to work on this?

[acolombier]

Any update on this? I have picked up the work from @Be-ing and it seems that this issue is making more than just one problem.
Happy to help with that!

[JoergAtGithub]

I will work on a PR, which implements an alternate mode, for such non HID class compliant use.

[acolombier]

Thanks for the speedy feedback, will give it a spin as soon as you have it :)

[JoergAtGithub]

I've the code ready now, and will provide a PR this weekend!

[acolombier]

Awesome stuff! I don't think the S4 Mk3 PR will be merged by then, so should be able to undo the workaround and test with it