earlephilhower / arduino-pico

Raspberry Pi Pico Arduino core, for all RP2040 and RP2350 boards
GNU Lesser General Public License v2.1
2.03k stars 422 forks source link

KeyboardBLE+MouseBLE+JoystickBLE #1506

Closed benjaminaigner closed 1 year ago

benjaminaigner commented 1 year ago

Dear @earlephilhower ,

thx for the progress on this BSP the last months!

Regarding the BLE implementation of Keyboard/Mouse/Joystick:

Is there any suggestion how to use all of them together?

The implementation for USB works as expected (Joystick, Keyboard and Mouse together), but in BLE (I didn't test BT Classic) only one HID device is working.

I'm willing to implement this, but I want your opinion on how to integrate it most seamlessly in the arduino-pico core.

Greetings

earlephilhower commented 1 year ago

I'm not a Bluetooth expert, so take this with a grain of salt, but right now this is because the BTstack GATT/HID handler is hardcoded to only support a single descriptor and report back.

https://github.com/bluekitchen/btstack/blob/0d212321a995ed2f9a80988f73ede854c7ad23b8/src/ble/gatt-service/hids_device.c

In plain USB you can have one HID endpoint with multiple reports, which is how we do Mouse+KBD+joystick. Concatenate all 3 descriptors when setting up, and then send back each report with a unique ID associated w/each separate desriptor.

Not sure if the HID emulation in BLE supports this. If it does, then you'd need to handle concatenation of the HID descriptors (see the RP2040USB.cpp file for one way) and then modify the hids_device.cpp handler to support different report IDs.

If the BLE HID service doesn't support multiple desriptors and reports, then I suppose you'd need to expose separate services for each one and then updaet the hids_device to support this.

What would help is if you had a multi-HID function BLE device to check out. That way you could see how those multiple HIDs are exposed. I only seem to have BLE mice and nothing else, so can't offer any suggestions.

benjaminaigner commented 1 year ago

I'm not a Bluetooth expert, so take this with a grain of salt, but right now this is because the BTstack GATT/HID handler is hardcoded to only support a single descriptor and report back.

https://github.com/bluekitchen/btstack/blob/0d212321a995ed2f9a80988f73ede854c7ad23b8/src/ble/gatt-service/hids_device.c

As you can see there, the ATT service consists of Boot mode mouse & keyboard and Report mode (in & out).

AFAIK merging the 3 report descriptors with appropriate report IDs should work here as well, I've done this successfully for the ESP32: https://github.com/asterics/esp32_mouse_keyboard

Would you recommend to create a MouseKeyboardJoystickBLE library, which concatenates the report descriptors from TinyUSB and provides all 3 interfaces?

earlephilhower commented 1 year ago

That would be great. I'm not sure how to orchestrate it given they will all start at different times, but it would be very nice if we could only expose the included HID devices (i.e. if you #include <MouseBLE.h> and #include <KeyboardBLE.h> the BLE would only expose a mouse and keyboard, not the joystick) that would be perfect, but right now I think we'll take what we can get. :)

benjaminaigner commented 1 year ago

I'll try to figure something out. Do you think it would be possible to determine in each class (KeyboardBLE/MouseBLE/JoystickBLE) if the corresponding header guards are set and create the report map accordingly? Than it would be possible to adapt the ctor/begin() to use the correct reportmap for more than one device.

In TinyUSB it was quite easy with the weak handler:

// Weak function override to add our descriptor to the TinyUSB list
void __USBInstallJoystick() { /* noop */ }

Anyway, AFAIK the consumer keys of the Keyboard need a report id as well, so this implementation would allow media keys as well.

earlephilhower commented 1 year ago

I think the same weak stuff (with a different naming convention ex. "__BLEInstallJoystick") can be used (not sure header guards will be usable...they're only defined on a per compilation unit basis so your .cpp won't know what the main.ino is including).

benjaminaigner commented 1 year ago

Dear @earlephilhower , I'm currently in contact with Mr. Ringwald from Bluekitchen & the development branch of BTStack includes now the infrastructure to work on this issue.

during development, I ran into a few issues:

  1. Updating BTStack into arduino-pico core: I finally found the make-libpico.sh script to build the new BTStack, but maybe this is some information for a developer README
  2. How to turn on the BTStack debug output? In btstack_config.h log info/debug are defined, but I cannot get any debug output. Could you please give me a hint?

I've changed all the necessary stuff here, if I'm finished I will open a PR: https://github.com/benjaminaigner/arduino-pico/tree/BLEHID_Composite

earlephilhower commented 1 year ago

I just took a quick look at the code. Looks nice and clean! When you're ready, throw a PR this way and we'll see about getting it into the next release.

Are you saying, though, that it needs a new BTStack version? That may be a problem as I'm really trying not to modify the PicoSDK upstream. But I didn't see an updated sdk git submodule in your repo, so maybe I misunderstood?

For the libpico docs, I did kind of document it in the tools/README.md where it lives. I guess it's not obvious, though. Not generally needed, thankfully.

For BTStack debug, I think they use ::printf. If so, then you just need to set Tools->Debug Port->Serial (or 1 or 2) in the IDE menus.

benjaminaigner commented 1 year ago

Dear @earlephilhower,

unfortunately, the function hids_device_init_with_storage is only available in develop branch of BTStack: https://github.com/bluekitchen/btstack/blob/08840d74f4ed5d00e72400b4d160b3ab08916229/src/ble/gatt-service/hids_device.c#L283

This function is needed to have a variable count of reports transmitted to the host.

I personally don't know why (it seems so unnecessary complicated) every report ID needs its own GATT characteristic. There is already a report characteristic and like in USB it should be possible to determine via the report map (also an extra characteristic) how to handle incoming data.

So if you want to try:

  1. change arduino-pico remote to my repo & get dev branch
  2. update BTStack to dev branch
  3. rebuild the pico-sdk library
    git remote set-url origin https://github.com/benjaminaigner/arduino-pico.git
    git checkout BLEHID_Composite
    cd ~/Arduino/hardware/pico/rp2040/pico-sdk/lib/btstack
    git checkout develop
    git pull
    cd ~/Arduino/hardware/pico/rp2040/tools/libpico
    bash make-libpico.sh

Regarding the upstream: I would wait until this change in BTStack is merged in their release & then request an update at the pico-sdk repo. And finally update the pico-sdk submodule here :-).

For the libpico docs, I did kind of document it in the tools/README.md where it lives. I guess it's not obvious, though. Not generally needed, thankfully.

Yeah, you are right, I just wasn't looking enough :-).

For BTStack debug, I think they use ::printf. If so, then you just need to set Tools->Debug Port->Serial (or 1 or 2) in the IDE menus.

I tried that, but nothing happened. (I tried Serial for USB output). I'll try again and write here if I found something.

matsobdev commented 1 year ago

Hi! You can #define WANT_HCI_DUMP 1 in btstack_config.h to get what's going on.

earlephilhower commented 1 year ago

Really great stuff, @benjaminaigner and I hope it can come in soon.

If the BTstack changes take too long to land in the BTStack repo (and then they would need to wait for a PicoSDK update, too) we can probably pull them in to the cores/rp2040/sdkoverrides simply enough (famous last words).

benjaminaigner commented 1 year ago

Dear @earlephilhower , I've managed to implement the composite device in BTStack, see: https://github.com/bluekitchen/btstack/pull/493

Thanks for the hint with SDK overrides. AFAIK the additional change in BTStack is the possibility to add multiple handles (HID reports) when parsing the ATT DB.

I'll work on the implementation in this core now, hopefully I can finish it this week.

benjaminaigner commented 1 year ago

Hi again :-).

I've implemented all necessary changes, but I'm struggling now with adding the new hids_device.c/.h file into sdkoverride.

Basically even the header file is not working (types & functions are not available).

Do you have a recommendation to use the header from here: https://github.com/benjaminaigner/arduino-pico/tree/BLEHID_Composite/cores/rp2040/sdkoverride

in the PicoBluetoothBLEHID.h file?

Either the declarations are not found (when using the unmodified .h file) or there are conflicting declarations (if changing the header guards to a new name).

The linker will be interesting as well.

Thank you very much

earlephilhower commented 1 year ago

Assuming that header isn't used elsewhere in the btstack code(if so, you need a rebuild of the libpico stuff and to keep the modified headers sync'd), just use a form like #include <sdkoverride/file.h>

As for the duplicate definitions, you can either remove the compilation unit from the libpico libs (see tools/libpico/Cmakefile) or wrap it using lib/platform_wrap and rename the functions __wrap_xxxx . I'd go with the first one as it is much less work.

benjaminaigner commented 1 year ago

Dear all, I've managed now that the composite device is working, also in all combinations!

But currently, I'm changing the BTSTack submodule to the current develop branch and rebuild libpico.

I'll try to fit in the BTSTack changes into an override file, to avoid changing pico-sdk, then I'll open a PR :+1:

Greetings

earlephilhower commented 1 year ago

Fixed by #1587