Chibios USB: Take into account if host wants remote wakeup or not #21287
Conversation
According to the USB 2.0 spec, remote wakeup should be disabled by default, and should only be enabled if the host explicitly requests it. The chibios driver code already takes care of storing this information, and returning it on GET_STATUS requests. However our application code has been ignoring it so far. This is a USB compliance issue, but also a bug that causes trouble in some cases: On RP2040 targets this has been causing problems if a key is held down while the keyboard is plugged in. The keyboard would fail to enumerate until all keys are released. With this change that behavior is fixed. Note that for LUFA targets this is already done correctly.
There was a problem hiding this comment.
I'd love to see this merged into master under bugfix criteria. Immediately useful. Guaranteed to resolve bewildering boot failures caused by soldering or other assembly errors, which are most often made by folks who lack the equipment and/or experience to realize they even have a hardware problem...let alone the knowledge required to apply the changes in this PR without causing repository headaches later.
Personally: I was attempting to test firmware written for integrated RP2040 using the only device I had handy, a WeAct Pi Pico. GP23 is a column pin in this firmware's COL2ROW matrix.
Turns out the KEY button on WeAct's Pi Pico causes GP23 to be pulled to ground until pressed. Couldn't get it to enumerate until a hunch got me to rebase on this PR; only then did I realize what was going on with the tact switch.
Will try to test on other ChibiOS-supported hardware when I am able.
|
@drashna Should I rebase and retarget it to master, or leave it as it is? A strict reading of the docs suggests all core changes go to develop, but I have had a different bugfix for a core regression merged to master recently, so seems like there are exceptions sometimes. @lesshonor Please correct me if I'm wrong, I notice that your approval is a different color on github from drashna's and even though I have seen you very active around here, I think that implies you're not on the official list of approvers, right? I agree with all you said, and would be happy to rebase to master, if everybody agrees. When it comes to non-rp2040 targets, I played around with STM32 in the past, I don't remember seeing the boot failure behavior, but if that's the case, it's probably because some differences in the USB stack. I believe it's still wrong and a USB compliance issue to not check that bit, an example scenario where we are probably non-usb compliant would be if we have one of our keyboards and another device on a hub, and if the OS is configured to not wake up from the keyboard, but wake up from the other device, (so we would never receive a set_feature to enable remote wakeup) maybe the hub could be combining the wakeup signals, so we could end up waking the host even though the host didn't want it. I am not sure if host/hub ports remember if there are any remote wakeup enabled devices under them. I didn't test if this actually happens. |
|
@purdeaandrei thanks for your investigation and subsequent bug fix, as the problem is real but an edge case which is not encountered to frequently (at least that is my impression but I can be wrong ofc) I would rather merge this into |
…k#21287) According to the USB 2.0 spec, remote wakeup should be disabled by default, and should only be enabled if the host explicitly requests it. The chibios driver code already takes care of storing this information, and returning it on GET_STATUS requests. However our application code has been ignoring it so far. This is a USB compliance issue, but also a bug that causes trouble in some cases: On RP2040 targets this has been causing problems if a key is held down while the keyboard is plugged in. The keyboard would fail to enumerate until all keys are released. With this change that behavior is fixed. Note that for LUFA targets this is already done correctly.
…k#21287) According to the USB 2.0 spec, remote wakeup should be disabled by default, and should only be enabled if the host explicitly requests it. The chibios driver code already takes care of storing this information, and returning it on GET_STATUS requests. However our application code has been ignoring it so far. This is a USB compliance issue, but also a bug that causes trouble in some cases: On RP2040 targets this has been causing problems if a key is held down while the keyboard is plugged in. The keyboard would fail to enumerate until all keys are released. With this change that behavior is fixed. Note that for LUFA targets this is already done correctly.
…k#21287) According to the USB 2.0 spec, remote wakeup should be disabled by default, and should only be enabled if the host explicitly requests it. The chibios driver code already takes care of storing this information, and returning it on GET_STATUS requests. However our application code has been ignoring it so far. This is a USB compliance issue, but also a bug that causes trouble in some cases: On RP2040 targets this has been causing problems if a key is held down while the keyboard is plugged in. The keyboard would fail to enumerate until all keys are released. With this change that behavior is fixed. Note that for LUFA targets this is already done correctly.
…k#21287) According to the USB 2.0 spec, remote wakeup should be disabled by default, and should only be enabled if the host explicitly requests it. The chibios driver code already takes care of storing this information, and returning it on GET_STATUS requests. However our application code has been ignoring it so far. This is a USB compliance issue, but also a bug that causes trouble in some cases: On RP2040 targets this has been causing problems if a key is held down while the keyboard is plugged in. The keyboard would fail to enumerate until all keys are released. With this change that behavior is fixed. Note that for LUFA targets this is already done correctly.
…k#21287) According to the USB 2.0 spec, remote wakeup should be disabled by default, and should only be enabled if the host explicitly requests it. The chibios driver code already takes care of storing this information, and returning it on GET_STATUS requests. However our application code has been ignoring it so far. This is a USB compliance issue, but also a bug that causes trouble in some cases: On RP2040 targets this has been causing problems if a key is held down while the keyboard is plugged in. The keyboard would fail to enumerate until all keys are released. With this change that behavior is fixed. Note that for LUFA targets this is already done correctly.
…k#21287) According to the USB 2.0 spec, remote wakeup should be disabled by default, and should only be enabled if the host explicitly requests it. The chibios driver code already takes care of storing this information, and returning it on GET_STATUS requests. However our application code has been ignoring it so far. This is a USB compliance issue, but also a bug that causes trouble in some cases: On RP2040 targets this has been causing problems if a key is held down while the keyboard is plugged in. The keyboard would fail to enumerate until all keys are released. With this change that behavior is fixed. Note that for LUFA targets this is already done correctly.
Description
According to the USB 2.0 spec, remote wakeup should be disabled by
default, and should only be enabled if the host explicitly requests
it. The chibios driver code already takes care of storing this
information, and returning it on GET_STATUS requests. However our
application code has been ignoring it so far.
This is a USB compliance issue, but also a bug that causes trouble
in some cases: On RP2040 targets this has been causing problems if
a key is held down while the keyboard is plugged in. The keyboard
would fail to enumerate until all keys are released. With this
change that behavior is fixed.
Note that for LUFA targets this is already done correctly.
Types of Changes
Issues Fixed or Closed by This PR
Checklist