WiimoteReal: Don't block on refresh #3988
Conversation
|
Very, very nice. |
|
As I was just saying on IRC: it feels like the logic here is being overly complex and that it could be simplified much further. For example, if we're always going to do scanning in a thread, we can potentially get rid of a tons of things that were assuming scanning could happen in two different threads. We can also keep the thread running continuously instead of having StopScanning with two behaviours (stop / do not stop thread). etc. |
leoetlino
force-pushed
the
scanning-block
branch
from
481d7aa
to
4023993
Compare
This changes Refresh() to use the existing scanning thread to scan for devices, instead of running the scan on the UI thread and blocking it. Also makes the UI thread not block when Continuous Scanning is disabled and removes duplicated code. Should fix issue 8992. Under the hood: * The scanning thread is now always active, even when continuous scanning is disabled. * The initialize code which waits for Wiimotes to be connected also uses the scanning thread instead of scanning on yet another thread. * The scanning thread now always checks for disconnected devices, to avoid Dolphin thinking a Wiimote is still connected when it isn't. So we now check if we need new Wiimotes or a Balance Board at scan time.
leoetlino
force-pushed
the
scanning-block
branch
from
4023993
to
c1b2530
Compare
|
I have switched to enums (instead of booleans), and removed code that shouldn't be necessary anymore since scanning now only happens on one thread. Also made it clear that the scanning thread is always running. |
|
I'm not convinced about the thread safety of the new code. For example: These two functions now perform unsynchronized accesses to g_wiimotes but nothing guarantees that they would be called from the scanning thread. In fact, g_wiimotes itself is global, which is a fairly terrible design... Can you explain why removing the lock is safe? I understand that it was recursive before because of the potential "double scan" issue, and that's probably useless now, but still feels like it should be a simple mutex. |
This replaces a recursive mutex with a normal mutex.
leoetlino
force-pushed
the
scanning-block
branch
from
c1b2530
to
80fc5e2
Compare
|
You're right; that was not very safe. I have added back a simple mutex in places where they are needed. |
|
Do you have an idea about android? https://forums.dolphin-emu.org/Thread-wiimote-dolphinbar-not-refreshed-after-5-0-151 |
|
Huh, that's a weird regression… So basically stopping a game prevents it from working again? My first guess would be that it relies on WiimoteReal::Refresh() (which is basically now a noop if continuous scanning is enabled) and for some reason it needs the scanning thread to really be stopped and then restarted. I can't test it on Android (no ARMv8 device), but I'll try to see if there is something I can do… |
This changes Refresh() to use the existing scanning thread to scan for devices, instead of running the scan on the UI thread and blocking it. (Should fix issue 8992)
This also makes the UI thread not block when Continuous Scanning is enabled then disabled, and removes a random 0-500ms delay on exit.
This change is