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

KeyboardBLE+MouseBLE+JoystickBLE #1506

Closed
benjaminaigner opened this issue Jun 6, 2023 · 15 comments
Closed

KeyboardBLE+MouseBLE+JoystickBLE #1506

benjaminaigner opened this issue Jun 6, 2023 · 15 comments
Labels
enhancement New feature or request

Comments

@benjaminaigner
Copy link
Contributor

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
Copy link
Owner

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.

@earlephilhower earlephilhower added the enhancement New feature or request label Jun 6, 2023
@benjaminaigner
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link

matsobdev commented Jun 20, 2023

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

@earlephilhower
Copy link
Owner

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
Copy link
Contributor Author

Dear @earlephilhower ,
I've managed to implement the composite device in BTStack, see:
bluekitchen/btstack#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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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 👍

Greetings

@earlephilhower
Copy link
Owner

Fixed by #1587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants