Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

Add HID battery interface, rework USB configuration (and remote wakeup) to run in c2usb thread #331

Closed
wants to merge 6 commits into from

Conversation

benedekkupper
Copy link
Contributor

@benedekkupper benedekkupper commented Oct 13, 2024

This MR is doing a few things together:

  1. Moving the USB configuration changes to be executed in c2usb thread, hopefully getting rid of races around BLE to USB switchover (BLE to USB switchover interface freeze #310)
  2. Adding an HID battery status reporting interface (Create USB HID interface for battery charging status indication #202), which isn't presenting any UI on any OS I've tried, so it's usefulness is limited to having a better idea what USB host the device is connected to. There appeared to be some OS support for reading battery status out of HID, but there's no working reference I could find.
  3. While I was at it, I also added the missing call to pass the battery status info to the BLE service.
  4. The HID input sending double buffer logic was removed, the semaphore handling greatly simplified. This has the potential to fix the existing issues around suspend-resume (#305).

also update battery level on BLE
@mondalaci
Copy link
Member

The issue persists, and a regression has been introduced: after unpairing and pairing my UHK, now it sends HID reports via USB instead of BLE to my phone, even though the BLE connection seems to be established:

Connected to unknown (ac:d6:18:1b:3d:b5)
Passkey for unknown (ac:d6:18:1b:3d:b5): 544406
Type `uhk btacc 1/0` to accept/reject
BLE HID conn params: interval=11 ms, latency=10, timeout=1000 ms
Numeric Match, conn 0x2000cdc8
Security changed: unknown (ac:d6:18:1b:3d:b5), level 4
Pairing completed: unknown (ac:d6:18:1b:3d:b5), bonded 1
BLE HID conn params: interval=7 ms, latency=0, timeout=5000 ms
BLE HID conn params: interval=11 ms, latency=10, timeout=1000 ms
I: starting HID app 1: 1

@benedekkupper Can you reproduce these issues? I want to avoid excessive testing.

@benedekkupper
Copy link
Contributor Author

I added a bunch more changes, please give it another go.

@mondalaci
Copy link
Member

I tested this PR by pairing and unpairing my Android phone and testing the mouse and keyboard interfaces on both Linux and Android. After 3 cycles, the mouse interface was broken both via USB and BLE.

However, after resetting the right half, it endured more than 20 cycles, and it's still running well, and both the keyboard and mouse have been working.

So it's not rock-stable yet, but we're getting very close. Should I merge this PR, and you'll open another one?

@benedekkupper
Copy link
Contributor Author

I added another change, this should get it going, also please check the USB suspend/resume behavior.

@mondalaci
Copy link
Member

Pairing endured 30 pair-unpair cycles without hiccups, and I haven't tested any longer, so it seems rock stable.

Regarding the suspend/resume behavior, the keyboard/mouse interfaces are always dead after I wake up my PC. On such occasions, I have to restart the right half.

@benedekkupper
Copy link
Contributor Author

I think we can merge it then.

Regarding the resume, what OS are you on? Is USB power saving enabled? What are the logs from the device? Can it recover if you unplug-replug the USB cable?

@mondalaci
Copy link
Member

I use Linux Mint, and I'm unsure whether power saving is enabled. I can dig into it if necessary.

The Zephyr log doesn't contain anything upon suspend/resume, however, if I disconnect USB after resume, I get this log:

W: Spurious suspend/resume event
W: aborted IN ep 0x86
E: EP 86 error:-113

Then, when reconnecting USB, it works again.

I'd rather merge this PR if this issue is fixed.

@benedekkupper
Copy link
Contributor Author

Here's my last try at this.

Honestly it seems that you manage to generate a continuous stream of reports out of your mouse interface, that leads to the last one being interrupted by the suspend, before being sent. This either means your hands are more nervous than should be, or that there is something fishy with the mouse movement calculation.

@kareltucek
Copy link
Contributor

Honestly it seems that you manage to generate a continuous stream of reports out of your mouse interface

Hmm, will look into that

@mondalaci
Copy link
Member

The keyboard and mouse interfaces are still dead after wake up until I reconnect USB.

@kareltucek 1. Can you reproduce it?
2. Can you fix it?

@kareltucek
Copy link
Contributor

Honestly it seems that you manage to generate a continuous stream of reports out of your mouse interface

What is your evidence? I don't see such behavior.

The keyboard and mouse interfaces are still dead after wake up until I reconnect USB.

@kareltucek 1. Can you reproduce it?

Yes, if computer is waked via remote wakeup, then the device's keyboard and mouse interfaces are dead until USB reconnect. This also applies if we don't send any usb reports between the wakeup command and L0 callback signal.

  1. Can you fix it?

(No.)

@benedekkupper
Copy link
Contributor Author

Maybe one of you who can reproduce this issue can cross-check with a reference. Please switch to NCS 2.7.0, fetch hid-keyboard sample, and apply this patch on it: https://github.com/zephyrproject-rtos/zephyr/pull/79341/files and test it with a nordic devkit.

@benedekkupper
Copy link
Contributor Author

The other thing to try in the firmware is in udc_mac.cpp , comment out the calls to cancel_all_transfers(); and check if there's a difference.

@mondalaci
Copy link
Member

@benedekkupper Commenting out the two occurrences of cancel_all_transfers() in udc_mac.cpp didn't make any difference.

I'll test the keyboard sample as suggested soon.

In the meantime, would you answer Karel?:

@benedekkupper: Honestly it seems that you manage to generate a continuous stream of reports out of your mouse interface

@kareltucek: What is your evidence? I don't see such behavior.

@mondalaci
Copy link
Member

@benedekkupper The wake feature of the patched hid keyboard sample consistently works. How to proceed now?

@benedekkupper
Copy link
Contributor Author

The next step is to try c2usb reference on zephyr with the nordic devkit. Please follow the readme here:
https://github.com/IntergatedCircuits/c2usb-zephyr-examples

  • If this doesn't work, then c2usb implementation is at fault.
  • If this works, then (at least) one of these are at fault: a) c2usb integration into uhk80 firmware b) zephyr was bugfixed between NCS 2.6.1 and 2.7.0

@mondalaci
Copy link
Member

I followed the readme, and I can't build the project.

$ cd c2usb-zephyr-examples
bash: cd: c2usb-zephyr-examples: No such file or directory

cd c2usb-zephyr-examples.git should be used instead. This is a minor issue worth fixing.

The major issue:

$ west build -b nrf52840dk/nrf52840 usb-keyboard
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base (cached)).
-- Application: /home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/usb-keyboard
-- CMake version: 3.21.0
-- Found Python3: /home/laci/ncs/toolchains/e9dba88316/usr/local/bin/python3.9 (found suitable version "3.9.18", minimum required is "3.8") found components: Interpreter 
-- Cache files will be written to: /home/laci/.cache/zephyr
-- Zephyr version: 3.7.0 (/home/laci/download/test/my-workspace/zephyr)
-- Found west (found suitable version "1.2.0", minimum required is "0.14.0")
CMake Warning at /home/laci/download/test/my-workspace/zephyr/cmake/modules/boards.cmake:136 (message):
  BOARD_ROOT element without a 'boards' subdirectory:

  /home/laci/download/test/my-workspace/c2usb-zephyr-examples.git

  Hints:

    - if your board directory is '/foo/bar/boards/my_board' then add '/foo/bar' to BOARD_ROOT, not the entire board directory
    - if in doubt, use absolute paths
Call Stack (most recent call first):
  /home/laci/download/test/my-workspace/zephyr/cmake/modules/zephyr_default.cmake:132 (include)
  /home/laci/download/test/my-workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /home/laci/download/test/my-workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:97 (include_boilerplate)
  CMakeLists.txt:4 (find_package)


-- Board: nrf52840dk, qualifiers: nrf52840
-- Found host-tools: zephyr 0.16.5 (/home/laci/ncs/toolchains/e9dba88316/opt/zephyr-sdk)
-- Found toolchain: zephyr 0.16.5 (/home/laci/ncs/toolchains/e9dba88316/opt/zephyr-sdk)
-- Found Dtc: /home/laci/ncs/toolchains/e9dba88316/usr/bin/dtc (found suitable version "1.5.0", minimum required is "1.4.6") 
-- Found BOARD.dts: /home/laci/download/test/my-workspace/zephyr/boards/nordic/nrf52840dk/nrf52840dk_nrf52840.dts
-- Generated zephyr.dts: /home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/build/zephyr/zephyr.dts
-- Generated devicetree_generated.h: /home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/build/zephyr/include/generated/zephyr/devicetree_generated.h
-- Including generated dts.cmake file: /home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/build/zephyr/dts.cmake
Parsing /home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/usb-keyboard/Kconfig
/home/laci/download/test/my-workspace/zephyr/scripts/kconfig/kconfig.py: /home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/lib/Kconfig:1: '/home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/lib/c2usb/c2usb/port/zephyr/Kconfig' not found (in 'rsource "c2usb/c2usb/port/zephyr/Kconfig"'). Check that environment variables are set correctly (e.g. $srctree, which is set to '/home/laci/download/test/my-workspace/zephyr'). Also note that unset environment variables expand to the empty string.
CMake Error at /home/laci/download/test/my-workspace/zephyr/cmake/modules/kconfig.cmake:389 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /home/laci/download/test/my-workspace/zephyr/cmake/modules/zephyr_default.cmake:132 (include)
  /home/laci/download/test/my-workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /home/laci/download/test/my-workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:97 (include_boilerplate)
  CMakeLists.txt:4 (find_package)


-- Configuring incomplete, errors occurred!
FATAL ERROR: command exited with status 1: /home/laci/ncs/toolchains/e9dba88316/usr/local/bin/cmake -DWEST_PYTHON=/home/laci/ncs/toolchains/e9dba88316/usr/local/bin/python3.9 -B/home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/build -GNinja -S/home/laci/download/test/my-workspace/c2usb-zephyr-examples.git/usb-keyboard

@benedekkupper
Copy link
Contributor Author

sorry about that, do a git submodule update --init --recursive just before the west build
I also just attach the binary to flash in case there's more hiccups
zephyr.hex.zip

@mondalaci
Copy link
Member

I flashed the hex file, and button 4 consistently wakes my PC.

What should be the next step?

@benedekkupper
Copy link
Contributor Author

Please also check that after wake, button 1 and led 1 are caps lock function, they should still be synced.
Here's a hex built on a zephyr version that's used for NCS 2.6.1:
zephyr.hex.zip

If this also works, then we need to investigate how the USB_RemoteWakeup() is called, and make sure it's called exactly one time. I added printing for that on this branch now.

@mondalaci
Copy link
Member

I flashed the hex. I could toggle Caps Lock via button 1. Then I put my PC to sleep. I woke it using button 1. Afterward, button 1 didn't toggle Caps Lock anymore.

What's next?

@benedekkupper
Copy link
Contributor Author

benedekkupper commented Oct 25, 2024

Is the behavior the same for both hex files? And if yes, could you test this part of the functionality on the zephyr sample as well? It also uses button 1 and led 1 for caps lock IIRC.

@mondalaci
Copy link
Member

Unlike the second sample, the first sample not only wakes the PC but Caps Lock always works.

@benedekkupper
Copy link
Contributor Author

That leads me to conclude that the fix we need comes from zephyr upstream. Then I will pick up the NCS update to 2.7.0 task on the weekend, I have already found out about the board reorganization topic, so it shouldn't be that complicated.

@mondalaci
Copy link
Member

Great, thanks! Will you continue #213? I'm asking because it contains few changes, and it must be rebased, so you may want to start a new branch, in which case I'd close it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants