-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retry opening of wiimote channels on initial failure #4940
Conversation
Please run the change through clang-format (or fix the lint errors manually, as shown here: https://buildbot.dolphin-emu.org/builders/lint/builds/5017/steps/shell/logs/stdio) |
@JosJuice Oops! Okay. |
9fa82c6
to
c7c53d9
Compare
Can you update that comment to include a brief summary of what you added to your commit message? We can see that you wait and try again from what is done; but it isn't obvious why this is done. |
@BhaaLseN I'm not sure what I can add. There seems to be a race condition between a peripheral device connecting to the bluetooth controller and it being ready to use. It's very short and it depends upon the controller, some appear to connect synchronously and block until the device is ready, others report the device upon discovery but do not allow communication straight away. I don't know which is the correct behaviour, or whether it depends on the peripheral, controller or both. Anyway, Dolphin waits for a remote to appear and immediately attempts to open the communication channels, this can fail because the device isn't ready yet, delay, try again, and it works. I'm willing to bet this actually makes wiimotes work with most bluetooth controllers which otherwise fail. |
When in doubt, just add that part of your commit message. Otherwise nobody knows why you do the sleep there, and might remove it thinking it has no purpose. |
c7c53d9
to
988e635
Compare
@BhaaLseN I've updated the commit message based on my reply above. |
@dolphin-emu-bot rebuild |
There are other (unlikely) chances the device is busy at random moments after this initial race condition. |
On Thu, 2017-02-23 at 15:21 -0800, Patrick A. Ferry wrote:
There are other (unlikely) chances the device is busy at random
moments after this initial race condition.
Could you loop around try to reconnect 2-3 times, it would be more
robust for the chaotic and fragmented world of bluetooth
adaptors/devices/driver stacks?
Sure, it makes sense. I'll update the patch.
|
988e635
to
5c204d6
Compare
Updated patch with loops instead of in-line retries. |
@ZephyrSurfer I've put in multiple retries as you suggested. |
|
||
// Output channel | ||
addr.l2_psm = htobs(WM_OUTPUT_CHANNEL); | ||
if ((m_cmd_sock = socket(AF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP)) == -1 || | ||
connect(m_cmd_sock, (sockaddr*)&addr, sizeof(addr)) < 0) | ||
retry = 0; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@sjnewbury It seems the original patch is from here Could you bump the attempts up one more time? |
@ZephyrSurfer I'll put that reference into the commit message. |
5c204d6
to
f152981
Compare
@ZephyrSurfer As requested, one more attempt. |
Looks like this will need a rebase + some (quick and easy) lint fixes. |
Sorry for the delay, I've just been distracted with other things. I'll take a look now. |
m_cmd_sock = -1; | ||
return false; | ||
} | ||
retry++; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
return false; | ||
} | ||
retry++; | ||
sleep(1); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There's no point in retrying opening of the socket, it either works or it doesn't, so I've changed the code to just try opening the sockets, if it succeeds, then retry the connections. Will push after testing... |
f152981
to
c33d6c8
Compare
connect(m_cmd_sock, (sockaddr*)&addr, sizeof(addr)) < 0) | ||
if (m_cmd_sock = socket(AF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP)) | ||
{ | ||
retry = 0; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
connect(m_int_sock, (sockaddr*)&addr, sizeof(addr)) < 0) | ||
if (m_int_sock = socket(AF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP)) | ||
{ | ||
retry = 0; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
c33d6c8
to
e692fa6
Compare
Looks like I need to rebase again ... |
There seems to be a race condition between a peripheral device connecting to the bluetooth controller and it being ready to use. It's very short and it depends upon the controller, some appear to connect synchronously and block until the device is ready, others report the device upon discovery but do not allow communication straight away. I don't know which is the correct behaviour, or whether it depends on the peripheral, controller or both. Anyway, Dolphin waits for a remote to appear and immediately attempts to open the communication channels, this can fail because the device isn't ready yet, delay, try again, and it works. There are other (unlikely) chances the device is busy at random moments after this initial race condition so it loops around try to reconnect. This was inspired by an earlier patch, see here: https://bugs.dolphin-emu.org/issues/5997#note-20 I can confirm that it works perfectly for me on a bluetooth controller where otherwise it's impossible to connect (Dell 380 Bluetooth 4.0).
e692fa6
to
e9a696b
Compare
Rebased |
It's not uncommon on some Bluetooth controllers for a
peripheral device to be "present" before it's "ready".
There is a window where connection can fail because the
device communication didn't succeed initially, but
will always work on retry after a short delay.
I'm not the original author of this patch and I can't
remember where I found it. I can confirm that it
works perfectly for me though...