Fix deadlock when hid_read and hid_write are called at the same time #2179
Conversation
|
Looks fine to me at first glance, but I don't remember why we added the HIDReader in the first place. @rryan do you? |
|
Maybe the thread priority? I don't know what the thread priority is for the controller though. |
|
Sidenote, I dunno if the AppVeyor build is supposed to fail, but I'm not sure why it's failing. |
The ControllerManager constructor starts the controller thread with QThread::HighPriority like the HidReader thread was.
I think it timed out. I manually restarted it. |
|
Thank you for investigating this issue and working on it. Could you sign our Contributor Agreement before this is merged? Please leave a comment when you have signed it. |
|
I’ve signed the contributor agreement. |
|
Ummm.. using the latest artifacts from this pull, Something is wrong.. the midi to screen response times are incredibly sluggish. I turn my eq knobs, and takes a second to turn inside mixxx, and doesn't show inbetween states. one second its at 12oclock, the next, where I set it to. same with playing, and lol. scratching is not happening. Midi controller - Pioneer DDJ-SR2 |
|
@WaylonR this branch makes no changes for MIDI controllers. |
|
Does each controller get its own thread? |
|
... unless you're using a MIDI controller and an HID controller at the same time, in which case the blocking read for the HID controller would affect the MIDI controller too. |
Apparently not. Perhaps that would be a good idea. On the other hand, maybe having multiple threads polling at a high frequency isn't a great idea. |
|
Moved to another pr, #2172 and, the problem i described above, disappeared. This does affect midi controllers. |
|
Do you have both HID and Midi controllers enabled? Because that could be why. |
|
I committed a change making it non-blocking now. It works well for me. @WaylonR, can you try this PR again? |
|
Should be ok now! 👌 |
|
Committed those changes! 👍 |
|
Not for me - in my tests earlier it will return 0 after roughly 70 iterations. |
|
So - what's the conclusion? What do you guys think should be done here? I don't have any other HID-based controller I can test with so it's a bit hard for me to make any more helpful judgements, I think. |
|
@Be-ing Ready to merge? I don't have any HID devices for testing. |
|
Would appreciate a merge, as I could really use this in the stable version of Mixxx. I can work around this issue by not writing anything to the device, but that's not a great solution. For what it's worth, I've been using this with my controller for a long time and it has always worked well. |
|
Sorry for going quiet on this. This still has not been tested with controllers that use multiple different input packets like the Kontrol S4 Mk2, which I am afraid this may break. Ideally, I would also like to verify that this works on Linux and macOS, but unfortunately not many contributors have HID controllers to test with.
As discussed earlier on Zulip, the issue you encountered writing your script should only become a problem when rapidly writing HID output constantly. Mixxx should indeed not break when this happens, but on the other hand scripts shouldn't be doing that. Scripts should only send output when the state of Mixxx changes, not constantly on a timer. |
That is what my script has been doing now, and it still locks up. I have it hooked up to stuff like VuMeter as well though which is probably called a lot. |
|
I asked some users who have a Kontrol S4 Mk2 to test this. I am still nervous to merge this without confirmation that controllers that use multiple input packets still work.
Hmm, other HID scripts do that without a problem... |
|
The script is here, if you're curious: https://github.com/codecat/mixxx-gemini-gmx/blob/master/Gemini%20GMX-hid-scripts.js |
|
I did some tests using the Traktor S4 MK2 with Mixxx. My laptop is a Windows 10 PC. Here's all the problems I found: Audio tears/clipping. No indication in the library that shows what tracks are currently playing. Can only see what's playing by looking at each individual deck. Traktor playlists has no indication showing what tracks were previously played (lacks the checkmark box). Crossfader assignment for each channel is off (Unable to turn on the crossfader cut) Moving the platter while the track is stopped will jolt even after the platter slows down or isnt' moving. Crossfader doesn't always cut off music, especially when doing fast and small cuts. Error message: Laptop battery seemed to drain fast(?) Jogwheel pitch bend needs more sensitivity (barely able to pitch bend spinning the platter by the edge) Preview button sometimes replays the same track/fails to pause and replays the track again (too sensitive?) Seems like the signal is double inputted. Preview button sometimes previews previous tracks that were heard. Moog Ladder filter "hi" frequency doesn't completely kill the audio when applied. |
|
Thank you for testing @nPrevail, but what we are interested regarding this pull request is only whether the controller works. Does every part of the controller work (as described on its wiki page) while using this? How long did you test for? The script error you posted look like a bug in the mapping, which is a separate issue from the changes being discussed here. Any issues with the mapping you can discuss in the Zulip topic for that controller. |
|
Everything seemed to respond. I tried all the features. For some buttons, I wasn't sure what they did. I didn't understand what the Snap or Quantize button did (I know one changed the view from browser mode to DJ mode). But the controller worked, the lights lit up, and HID responded. The only thing I wasn't able to fully test out were my audio master and booth outputs. I used the controller for a couple of hours. |
|
Thank you very much for testing @nPrevail. I'm not sure how this is working, but it is, so I'll go ahead and merge this now. I am guessing that for controllers with multiple input packets, it is a matter of chance which type of packet is received when Mixxx polls for an update, but the polling happens frequently enough that it doesn't matter that all packets aren't processed simultaneously. |
|
I see. I think some of this lingo is beyond me, but I'm glad I was able to
help!
Not sure what else I can do in the meantime...
…On Tue, Nov 19, 2019 at 4:25 PM Be ***@***.***> wrote:
Merged #2179 <#2179> into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2179?email_source=notifications&email_token=ANYETW3EPS57HKMXBWFFEB3QUR7WHA5CNFSM4HZECG72YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOU6ZHKSQ#event-2813490506>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANYETW2FQT3X3JJUY2PTRILQUR7WHANCNFSM4HZECG7Q>
.
--
Nelson "nPrevail" Nailat
(224) 36N-PREV
(224) 366-7738
www.nprevail.bandcamp.com
www.mixcloud.com/hasi_la_pheeque
"You got to have vision."
|
|
Nice!! I'm very happy this is merged. Thank you! |
|
Hi @codecat, I like to put your name on the contributor list in the Mixxx about box. Is it OK If I use your real name? |
|
Oh cool! Yes, Melissa is ok 🥰 |
Otherwise, it seems messages build up in a queue which produces a dramatic lag on Linux when moving faders quickly. This regression was introduced between Mixxx 2.2 and 2.3 beta, likely by switching HidController to nonblocking polling in PR mixxxdj#2179.
Otherwise, it seems messages build up in a queue which produces a dramatic lag on Linux when moving faders quickly. This regression was introduced between Mixxx 2.2 and 2.3 beta, likely by switching HidController to nonblocking polling in PR mixxxdj#2179.
Otherwise, it seems messages build up in a queue which produces a dramatic lag on Linux when moving faders quickly. This regression was introduced between Mixxx 2.2 and 2.3 beta, likely by switching HidController to nonblocking polling in PR mixxxdj#2179.
This fixes a problem where the controller thread would deadlock when
hid_read_timeoutandhid_writewere called at the same time.As discussed on Zulip, this was fixed by removing
HidReaderin favor of using thepoll()method so that HID reading runs on the same thread as writing.