Skip to content
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

fix(usb): detect USB power mode to fallback to BLE #2458

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

angweekiat
Copy link

@angweekiat angweekiat commented Sep 4, 2024

Update zmk_usb_is_hid_ready to check that HID configuration from the host has been done, before allowing the keyboard to be used as a HID device.

This is done by adding bool is_configured and toggling it off when the device is disconnected, and enabling it when usb_status == USB_DC_CONFIGURED.

This is a naive solution for #841, where having a USB-power cable without data driving the keyboard, like a power bank, still has the firmware thinking that it's connected to a host and futilely sending data over USB.

When the cable to a power source is plugged in, the USB DC status (from Zephyr) goes through the following states:

USB_DC_CONNECTED -> USB_DC_SUSPEND

While for cables to a host machine:

USB_DC_CONNECTED -> USB_DC_SUSPEND -> USB_DC_RESUME -> USB_DC_RESET -> USB_DC_CONFIGURED

The status code list can be found at https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/usb/usb_dc.h#L33-L58

In the case of a power bank being plugged in, the code changes above allow zmk_usb_is_hid_ready to declare that HID isn't ready yet, and thus allows endpoints.c to fallback to BLE connection.

Steps to reproduce the original issue

  1. (Setup) Have ZMK keyboard with Bluetooth profile enabled
  2. Set the keyboard to prefer sending to USB (&out OUT_USB)
  3. Power on the keyboard via USB via a power bank
  4. Type, text should not show up anywhere ('consumed' by power bank)

Testing

  • Built and flashed the firmware with the commit changes, ran the same steps above, and text showed up on the expected device.
  • Tested using 2 laptops (Ubuntu / Windows) using 2 different power banks.
  • Tested for regression, keyboard still works as expected for waking up the host machine (CONFIG_USB_DEVICE_REMOTE_WAKEUP=y) and for boot protocol (CONFIG_ZMK_USB_BOOT=y).

Concerns

  • I can't find any USB or USB HID specifications that explicitly state that the host machine will do the USB configuration step before the USB HID connection is considered complete.
  • It's my first time contributing to ZMK, please let me know how the pull request can be improved!

@angweekiat angweekiat requested a review from a team as a code owner September 4, 2024 15:08
@Nick-Munnich Nick-Munnich added bug Something isn't working usb labels Sep 4, 2024
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

One major question about testing/statuses for another scenario.

app/src/usb.c Outdated
case USB_DC_CONFIGURED:
case USB_DC_RESUME:
case USB_DC_CLEAR_HALT:
case USB_DC_SOF:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you had a chance to confirm the state it enters when a host is connected but sleeps/requests the USB device sleep? We should also confirm this is set up properly when making changes here.

Also, are we sure this is consistent across all the Zephyr USB drivers?

Copy link
Author

@angweekiat angweekiat Sep 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I tested these additional scenarios:

  1. Connecting the keyboard to my laptop, sleeping the laptop, and waking it via keyboard, this works
  2. Sleeping the laptop, then connect the keyboard, then wake it via keyboard, this works as well

Strangely, the keyboard didn't transition to any suspended state with the laptop, which feels concerning. If it helps, the board I'm testing with is nice nano v2, but I'm uncertain if this behaviour should be expected across all USB drivers.

I was being dumb and locked my laptop instead of sleeping it properly, the state does get set into suspended. When suspended, I wasn't able to wake the laptop from the keyboard, but I want to also check if this is expected behavior even from main branch.

To account for other behaviors from other devices, perhaps a better approach is to revert the mapping changes, and keep track that the keyboard has at least gone through the USB_DC_CONFIGURED state once, before allowing it as a possible endpoint? (Need to see if this works with ZMK_USB_BOOT)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was being dumb and locked my laptop instead of sleeping it properly, the state does get set into suspended. When suspended, I wasn't able to wake the laptop from the keyboard, but I want to also check if this is expected behavior even from main branch.

It should, if you have CONFIG_USB_DEVICE_REMOTE_WAKEUP=y and the USB driver supports it (which the nrfx one does). In particular, since "suspended" is still considered USB active, then any attempt to send will still pick the USB endpoint, which ultimately triggers this code: https://github.com/zmkfirmware/zmk/blob/main/app/src/usb_hid.c#L132

To account for other behaviors from other devices, perhaps a better approach is to revert the mapping changes, and keep track that the keyboard has at least gone through the USB_DC_CONFIGURED state once, before allowing it as a possible endpoint? (Need to see if this works with ZMK_USB_BOOT)

Yeah, I think "has gone through configured state but hasn't gone to disconnected" may be the best way to really track this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers! I changed the code to what we mentioned above, and regression tested that both the wake and boot protocol functionality still works as expected, and also updated the PR description to match.

I did it by adding another is_configured bool, but if space is a concern, it's possible to change usb_status to store both the status code and this flag as a bitfield and parsing out the values separately, at the cost of readability.

Update `zmk_usb_get_conn_state` mapping to only treat
`USB_DC_CONFIGURED` and `USB_DC_RESUME` as succesful HID connection.

This is a naive solution for zmkfirmware#841,
where having a USB-power cable without data driving the keyboard, like a
power bank, still has the firmware thinking that it's connected to a
host and futilely send data over USB.

When the power cable is plugged in, the USB DC status (from Zephyr) goes
through the following states:
```
USB_DC_CONNECTED -> USB_DC_SUSPEND
```
While for data cables:
```
USB_DC_CONNECTED -> USB_DC_SUSPEND -> USB_DC_RESUME -> USB_DC_RESET -> USB_DC_CONFIGURED
```
The enum mapping changes do allow the firmware to differentiate the 2
cases, but I'm unable to find any USB or USB HID specifications that
explicitly states that the host machine will do the USB configuration
step before the USB HID connection is considered complete. Just for
safety sake, I added `USB_DC_RESUME` to be considered as successful too.

Steps to reproduce original issue
---------------------------------
1. (Setup) Have ZMK keyboard with bluetooth profile enabled, set to prefer sending to USB
2. Power on keyboard via USB via power bank
3. Type, text should not show up anywhere ('consumed' by power bank)

Testing
------
Built and flashed firmware with the commit changes, ran the same steps
above, text showed up on expected device.

Tested using 2 laptops (Ubuntu / Windows) using 2 different power banks.

Logs (before)
-----------------
```
--- Started keyboard via battery
[00:00:00.554,992] <inf> zmk: Welcome to ZMK!
...
[00:00:01.133,514] <dbg> zmk: zmk_usb_get_conn_state: state: 11

--- Plugged power bank to keyboard
[00:00:03.959,777] <inf> usb_hid: Device connected
[00:00:03.959,808] <dbg> zmk: zmk_usb_get_conn_state: state: 2
[00:00:03.959,838] <dbg> zmk: zmk_usb_get_conn_state: state: 2
[00:00:03.959,869] <dbg> zmk: get_selected_transport: Only BLE is ready.
[00:00:03.962,890] <inf> usb_hid: Device suspended
[00:00:03.962,921] <dbg> zmk: zmk_usb_get_conn_state: state: 5
[00:00:03.962,951] <dbg> zmk: zmk_usb_get_conn_state: state: 5
[00:00:03.962,982] <dbg> zmk: get_selected_transport: Both endpoint transports are ready. Using 0
[00:00:03.962,982] <dbg> zmk: zmk_endpoints_send_report: usage page 0x07
[00:00:03.963,012] <dbg> zmk: zmk_endpoints_send_report: usage page 0x0C
[00:00:03.963,073] <inf> zmk: Endpoint changed: USB
```

Logs with mapping changes (after)
---------------------------------
```
--- Started keyboard via battery
[00:00:00.547,943] <inf> zmk: Welcome to ZMK!
...
[00:00:00.549,804] <dbg> zmk: zmk_usb_get_conn_state: state: 11
...

--- Plugged power bank to keyboard
[00:00:03.686,065] <inf> usb_hid: Device connected
[00:00:03.686,126] <dbg> zmk: zmk_usb_get_conn_state: state: 2
[00:00:03.686,126] <dbg> zmk: zmk_usb_get_conn_state: state: 2
[00:00:03.686,157] <dbg> zmk: get_selected_transport: Only BLE is ready.
[00:00:03.689,208] <inf> usb_hid: Device suspended
[00:00:03.689,239] <dbg> zmk: zmk_usb_get_conn_state: state: 5
[00:00:03.689,270] <dbg> zmk: zmk_usb_get_conn_state: state: 5
[00:00:03.689,270] <dbg> zmk: get_selected_transport: Only BLE is ready.
```

Concerns
--------
- This should work for USB Boot Protocol Support, but I have yet to test it.
- TODO: Check what happens when keyboard that doesn't have bluetooth profiles has a power bank plugged into it
- Above test isn't comprehensive. Maybe this can be a config option instead to catch unknown cases?
@angweekiat angweekiat force-pushed the fix/usb-power-detection-ble-fallback branch from 3dc8662 to 78fdfa2 Compare September 14, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working usb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants