search

bluekitchen / btstack

Dual-mode Bluetooth stack, with small memory footprint.
http://bluekitchen-gmbh.com
Other
1.74k stars 618 forks source link

HOG - ATT DB / hids_device_t #483

Closed benjaminaigner closed 1 year ago

benjaminaigner commented 1 year ago

Dear Matthias,

my last contact with BTStack is a long time ago, but I'm glad the RaspberryPi Foundation is using BTStack for the PicoW! I would like to implement a flexible way to use either combination of Mouse, Keyboard and Joystick in Bluetooth as it is working with USB-HID (TinyUSB), see https://github.com/earlephilhower/arduino-pico/issues/1506 . I've done this before with the ESP32 (https://github.com/asterics/esp32_mouse_keyboard), which was of course no fun, but it is working. The problem is: with Bluedroid one is much more flexible with the creation of the ATT DB, I'm currently thinking on how to do this with BTStack, without re-implementing most of the HID stuff.

Is your feature request related to a problem? Please describe. Although HID would allow composite devices (e.g. keyboard+mouse+joystick), BTStack is using a fixed ATT setup with 1 IN, 1 OUT & 1 FEATURE entry.

Describe the solution you'd like Some flexibility what parts are in the hids_device struct and the possibility to get/set a handle to the hids_device_send_input_report function (at least more than 1 input entry)

Describe alternatives you've considered Maybe a flexible list of input,output&feature entries, including the handles & values.

Additional context

Thank you very much, greetings, Benjamin

mringwal commented 1 year ago

Hi Benjamin. Nice you're still interested.

While trying to avoid complexity, we've limited HID over GATT a bit, that's correct. :)

Haven't used HID in a while. If I remember correctly, you can add as many usages into a single report. Is this not possible for LE?

Assuming you need more than one report per type:

In the first case, we had the vague idea to provide params to included gatt service definitions.

Cheers Matthias

matsobdev commented 1 year ago

Hi! Using just one report with hod_keyboard_demo.c and modified descriptor:

const uint8_t hid_descriptor_keyboard_boot_mode[] = {

    0x05, 0x01,                    // Usage Page (Generic Desktop)
    0x09, 0x06,                    // Usage (Keyboard)
    0xa1, 0x01,                    // Collection (Application)

    0x85,  0x01,                   // Report ID 1

    // Modifier byte

    0x75, 0x01,                    //   Report Size (1)
    0x95, 0x08,                    //   Report Count (8)
    0x05, 0x07,                    //   Usage Page (Key codes)
    0x19, 0xe0,                    //   Usage Minimum (Keyboard LeftControl)
    0x29, 0xe7,                    //   Usage Maxium (Keyboard Right GUI)
    0x15, 0x00,                    //   Logical Minimum (0)
    0x25, 0x01,                    //   Logical Maximum (1)
    0x81, 0x02,                    //   Input (Data, Variable, Absolute)

    // Reserved byte

    0x75, 0x01,                    //   Report Size (1)
    0x95, 0x08,                    //   Report Count (8)
    0x81, 0x03,                    //   Input (Constant, Variable, Absolute)

    // LED report + padding

    0x95, 0x05,                    //   Report Count (5)
    0x75, 0x01,                    //   Report Size (1)
    0x05, 0x08,                    //   Usage Page (LEDs)
    0x19, 0x01,                    //   Usage Minimum (Num Lock)
    0x29, 0x05,                    //   Usage Maxium (Kana)
    0x91, 0x02,                    //   Output (Data, Variable, Absolute)

    0x95, 0x01,                    //   Report Count (1)
    0x75, 0x03,                    //   Report Size (3)
    0x91, 0x03,                    //   Output (Constant, Variable, Absolute)

    // Keycodes

    0x95, 0x05,                    //   Report Count (5, was 6) 
    0x75, 0x08,                    //   Report Size (8)
    0x15, 0x00,                    //   Logical Minimum (0)
    0x25, 0xff,                    //   Logical Maximum (1)
    0x05, 0x07,                    //   Usage Page (Key codes)
    0x19, 0x00,                    //   Usage Minimum (Reserved (no event indicated))
    0x29, 0xff,                    //   Usage Maxium (Reserved)
    0x81, 0x00,                    //   Input (Data, Array)

    //0xc0,                        // End collection

    0x05, 0x0C,     // usage page (consumer device) 
    //0x09, 0x01,       // usage -- consumer control 
    //0xA1, 0x01,       // collection (application) 
    //0x85, 0x02,       // report id 

    0x15, 0x00,         // logical minimum 
    0x25, 0xFF,             // logical maximum (ff) 
    0x19, 0x00,         // usage minimum (0) 
    0x29, 0xFF,         // usage maximum (ff) 
    0x95, 0x01,         // report count (1, was 8) 
    0x75, 0x08,         // report size (8) 
    0x81, 0x00,         // input 
    0xC0,               // end collection
    0x05, 0x01,                    // USAGE_PAGE (Generic Desktop)
    0x09, 0x02,                    // USAGE (Mouse)
    0xa1, 0x01,                    // COLLECTION (Application)

    // 0x85,  0x01,                   // Report ID 1
    0x09, 0x01,                    //   USAGE (Pointer)
    0xa1, 0x00,                    //   COLLECTION (Physical)

    0x05, 0x09,                    //     USAGE_PAGE (Button)
    0x19, 0x01,                    //     USAGE_MINIMUM (Button 1)
    0x29, 0x03,                    //     USAGE_MAXIMUM (Button 3)
    0x15, 0x00,                    //     LOGICAL_MINIMUM (0)
    0x25, 0x01,                    //     LOGICAL_MAXIMUM (1)
    0x95, 0x03,                    //     REPORT_COUNT (3)
    0x75, 0x01,                    //     REPORT_SIZE (1)
    0x81, 0x02,                    //     INPUT (Data,Var,Abs)
    0x95, 0x01,                    //     REPORT_COUNT (1)
    0x75, 0x05,                    //     REPORT_SIZE (5)
    0x81, 0x03,                    //     INPUT (Cnst,Var,Abs)

    0x05, 0x01,                    //     USAGE_PAGE (Generic Desktop)
    0x09, 0x30,                    //     USAGE (X)
    0x09, 0x31,                    //     USAGE (Y)
    0x15, 0x81,                    //     LOGICAL_MINIMUM (-127)
    0x25, 0x7f,                    //     LOGICAL_MAXIMUM (127)
    0x75, 0x08,                    //     REPORT_SIZE (8)
    0x95, 0x02,                    //     REPORT_COUNT (2)
    0x81, 0x06,                    //     INPUT (Data,Var,Rel)

    // 0xc0,                          //   END_COLLECTION
     0xc0,                          //   END_COLLECTION
    0xc0                           // END_COLLECTION
};

send_report() looks like that:

static void send_report(uint8_t modifier, uint8_t keycode, uint8_t keycode_last, uint8_t button, int dx, int dy){
    uint8_t report[] = {modifier, 0, keycode, 0, 0, 0, 0, keycode_last, button, dx, dy};
    switch (protocol_mode){
        case 0:
            hids_device_send_boot_keyboard_input_report(con_handle, report, sizeof(report));
            break;
        case 1:
           hids_device_send_input_report(con_handle, report, sizeof(report));
           break;
        default:
            break;
    }
}

And finally some demo to show off its actions:

while(1) {      
        send_report(0x00, 0x00, 0x00, 0, -50, -50);
        send_report(0x00, 0x00, 0x00, 2, 0, 0);
        send_report(0x00, 0x00, 0x00, 0, 0, 0);
        sleep_ms(2500);
     }

I feel sending separate reports for keyboard/mouse (and whatever input device) would be more natural for USB HID standard, although it works for me to USBfy and BLEfy Lenovo G505 keyboard with Pico W.

PS. Last (6th) keyboard keycode is changed to be multimedia key (consume control) here. PS2. Tested only on Samsung S7 and it shows up as a keyboard. I don't know, it there are different icons for composite devices. Maybe first is on to go. PS3. I would play with hids.gatt file and merge mouse and keyboard examples, since - according to Matthias keyboard and mouse would need its own characteristic to be report independent. This file has ORG_BLUETOOTH_CHARACTERISTIC_BOOT_KEYBOARD_INPUT_REPORT and ORG_BLUETOOTH_CHARACTERISTIC_BOOT_MOUSE_INPUT_REPORT under ORG_BLUETOOTH_SERVICE_HUMAN_INTERFACE_DEVICE service. At least some clue. PS4. In HID over GATT https://www.bluetooth.org/docman/handlers/downloaddoc.ashx?doc_id=245141 there is no joystick or other device but keyboard and mouse. So maybe adding joystick will only be a matter of a merged descriptor shared with keyboard or mouse for that kind of an input device (and other as well).

benjaminaigner commented 1 year ago

Dear @mringwal, dear @matsobdev ,

regarding the single report, identified by the report ID: normally you are right, what else is the report id used for :-). But we are working with USB defined protocols and an ugly mixup with BT&BLE standards...

@matsobdev I'm sure your report map works on an Android device, these are the most compatible ones (similar to Linux). If you want to use iOS (mandatory for us) and Windows, there is a BIG difference on how the report map is parsed. As far as I understand you don't use report IDs at all in your setup, so the Samsung device identifies the report type by length (I think).

ad PS: I'm really not sure how this works on all platforms, but I like your creative solution! ad PS2: You have to change the "GAP Appearance" for a different icon ad PS3: thx for the hint, this is exactly what I need, more report characteristics ad PS4: I have it working with the ESP32: https://github.com/asterics/esp32_mouse_keyboard

@mringwal

THX for the hint, it would be option 1, we know the number of reports at compile time. I'm happy with suggestions to implement this!

Greetings, Benjamin

mringwal commented 1 year ago

Hi @benjaminaigner Thanks for sharing your experience. After my iOS extensions (Classic HID), I didn't use HID myself.

To use two or more reports per type, we'd need to a) extend the database, as well as make the service implementation more flexible. For the GATT Compiler, I can image adding a mechanism to repeat one or more lines in the .gatt file controlled by a (global-)variable or even parameters to the #include call.

We'll have some internal discussion and get back.

matsobdev commented 1 year ago

I just compared mouse and keyboard hog examples and was optimistic (possibly prematurely) when saw:

case    HIDS_SUBEVENT_BOOT_KEYBOARD_INPUT_REPORT_ENABLE:
        con_handle = hids_subevent_boot_keyboard_input_report_enable_get_con_handle(packet);
        printf("Boot Keyboard Characteristic Subscribed %u\n", hids_subevent_boot_keyboard_input_report_enable_get_enable(packet));
        break;
case    HIDS_SUBEVENT_BOOT_MOUSE_INPUT_REPORT_ENABLE:
        con_handle = hids_subevent_boot_mouse_input_report_enable_get_con_handle(packet);
        printf("Boot Mouse Characteristic Subscribed %u\n", hids_subevent_boot_mouse_input_report_enable_get_enable(packet));
        break;

in hog_mouse_demo.c. Does that mean that mouse example is already capable of doing keyboard (separate characteristics) with descriptor with separate report_id for mouse and keyboard? After I read what's above, you'll prove me wrong I guess.

benjaminaigner commented 1 year ago

Dear @matsobdev ,

this is a special mode for using the BLE mouse&keyboard before the OS is loaded. AFAIK it is a basic mode, where the input report has a specific format. No HID report descriptor is used in this case.

Personally, I've never saw any device using the boot mode. All major OS use the report mode (in your send_report example is the if statement which type is used).

matsobdev commented 1 year ago

I was aware of that mode and wasn't sure but had an idea how it would work. Was too optimistic and missed that boot word, thanks for clarification.

matsobdev commented 1 year ago

Victory! I've put content of hog_device_test.gatt file into hids.gatt file - to get two HID devices and voila. What wasn't working before - consumer control under different report_id, now is working. Descriptor below:

    0x05, 0x01, // USAGE_PAGE (Generic Desktop)
    0x09, 0x06, // USAGE (Keyboard)
    0xa1, 0x01, // COLLECTION (Application)
     0x85, 0x01,
    0x05, 0x07, // USAGE_PAGE (Keyboard)
    0x19, 0xe0, // USAGE_MINIMUM (Keyboard LeftControl)
    0x29, 0xe7, // USAGE_MAXIMUM (Keyboard Right GUI)
    0x15, 0x00, // LOGICAL_MINIMUM (0)
    0x25, 0x01, // LOGICAL_MAXIMUM (1)
    0x75, 0x01, // REPORT_SIZE (1)
    0x95, 0x08, // REPORT_COUNT (8)
    0x81, 0x02, // INPUT (Data,Var,Abs)
    0x95, 0x01, // REPORT_COUNT (1)
    0x75, 0x08, // REPORT_SIZE (8)
    0x81, 0x03, // INPUT (Cnst,Var,Abs)
    0x95, 0x05, // REPORT_COUNT (5)
    0x75, 0x01, // REPORT_SIZE (1)
    0x05, 0x08, // USAGE_PAGE (LEDs)
    0x19, 0x01, // USAGE_MINIMUM (Num Lock)
    0x29, 0x05, // USAGE_MAXIMUM (Kana)
    0x91, 0x02, // OUTPUT (Data,Var,Abs)
    0x95, 0x01, // REPORT_COUNT (1)
    0x75, 0x03, // REPORT_SIZE (3)
    0x91, 0x03, // OUTPUT (Cnst,Var,Abs)
    0x95, 0x06, // REPORT_COUNT (6)
    0x75, 0x08, // REPORT_SIZE (8)
    0x15, 0x00, // LOGICAL_MINIMUM (0)
    0x25, 0xFF, // LOGICAL_MAXIMUM (255)
    0x05, 0x07, // USAGE_PAGE (Keyboard)
    0x19, 0x00, // USAGE_MINIMUM (Reserved (no event indicated))
    0x29, 0xff, // USAGE_MAXIMUM (Keyboard Application)
    0x81, 0x00, // INPUT (Data,Ary,Abs)
    0xc0,        // END_COLLECTION
     0x05, 0x0C,                                    /* usage page (consumer device) */
    0x09, 0x01,                                 /* usage -- consumer control */
    0xA1, 0x01,                                 /* collection (application) */
    0x85, 0x04,         /* report id */
    /* 4 Media Keys */
    0x15, 0x00,                                 /* logical minimum */
    0x25, 0xFF,                                 /* logical maximum (3ff) */
    0x19, 0x00,                                 /* usage minimum (0) */
    0x29, 0xFF,                             /* usage maximum (3ff) */
    0x95, 0x08,                                 /* report count (4) */
    0x75, 0x08,                                 /* report size (16) */
    0x81, 0x00,                                 /* input */
    0xC0, /* end collection */

And send_report():

static void send_report(uint8_t report_id, uint8_t modifier, uint8_t keycode){
    uint8_t report[] = {report_id, modifier, 0, keycode, 0, 0, 0, 0, 0};
    switch (protocol_mode){
        case 0:
            hids_device_send_boot_keyboard_input_report(con_handle, report, sizeof(report));
            break;
        case 1:
           hids_device_send_input_report(con_handle, report, sizeof(report));
           break;
        default:
            break;
    }
}

And test code:

while(1) {
    send_report(0x04, 0x00, 0xEA); //Volume down
    send_report(0x04, 0x00, 0x00);
    sleep_ms(1500);
}

It is Android tho, but wasn't working before, so progress. It might work with mouse the same way like consumer control - different report_id stuff in descriptor.

Sorry, too fast, only one is working... 0x01 rep id not.

benjaminaigner commented 1 year ago

Sorry, too fast, only one is working... 0x01 rep id not.

And which one is working?

I've put content of hog_device_test.gatt file into hids.gatt file

Nice, thanks for the hint, so an adaption of the .gatt file is necessary. I think in addition, you need the correct handler to send the report to. (see here in the esp-idf implementation: https://github.com/asterics/esp32_mouse_keyboard/blob/d4ac64ef02ff0f6199efd0a675f9caed09d9d780/main/hid_dev.c#L45)

benjaminaigner commented 1 year ago

BTW: I'm working with the RaspberryPi Pico W with Earle Philhower's arduino-core: https://github.com/earlephilhower/arduino-pico

matsobdev commented 1 year ago

Yes, I'm on Pico W as well. I'm not gonna call it a success, but successfully used hardcoded output report (not boot) inside hids.gatt file - that is called by project .gatt file by the way. Changing there:

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 2, type = Output (2)
REPORT_REFERENCE, READ, 2, 1 //last 2 to 1, to set it as an input - I guess this is how it works

So output report is now input report_id 2. And demo code for mousing and keyboarding:

 while(1) {     
    hids_device_send_input_report(con_handle, (uint8_t[]) {0, 0, 4, 0, 0, 0, 0, 0}, 8);
    sleep_ms(5);
    hids_device_send_input_report(con_handle, (uint8_t[]) {0, 0, 0, 0, 0, 0, 0, 0}, 8);
    sleep_ms(5);
    hids_device_send_output_report(con_handle, (uint8_t[]) {2, 10, 10, 0}, 4);
    hids_device_send_output_report(con_handle, (uint8_t[]) {0, 10, 10, 0}, 4);
    sleep_ms(1500);
 }

This time with proper combo descriptor:

    0x05, 0x01,                    // Usage Page (Generic Desktop)
    0x09, 0x06,                    // Usage (Keyboard)
    0xa1, 0x01,                    // Collection (Application)

    0x85,  0x01,                   // Report ID 1

    // Modifier byte

    0x75, 0x01,                    //   Report Size (1)
    0x95, 0x08,                    //   Report Count (8)
    0x05, 0x07,                    //   Usage Page (Key codes)
    0x19, 0xe0,                    //   Usage Minimum (Keyboard LeftControl)
    0x29, 0xe7,                    //   Usage Maxium (Keyboard Right GUI)
    0x15, 0x00,                    //   Logical Minimum (0)
    0x25, 0x01,                    //   Logical Maximum (1)
    0x81, 0x02,                    //   Input (Data, Variable, Absolute)

    // Reserved byte

    0x75, 0x01,                    //   Report Size (1)
    0x95, 0x08,                    //   Report Count (8)
    0x81, 0x03,                    //   Input (Constant, Variable, Absolute)

    // LED report + padding

    0x95, 0x05,                    //   Report Count (5)
    0x75, 0x01,                    //   Report Size (1)
    0x05, 0x08,                    //   Usage Page (LEDs)
    0x19, 0x01,                    //   Usage Minimum (Num Lock)
    0x29, 0x05,                    //   Usage Maxium (Kana)
    0x91, 0x02,                    //   Output (Data, Variable, Absolute)

    0x95, 0x01,                    //   Report Count (1)
    0x75, 0x03,                    //   Report Size (3)
    0x91, 0x03,                    //   Output (Constant, Variable, Absolute)

    // Keycodes

    0x95, 0x06,                    //   Report Count (6)
    0x75, 0x08,                    //   Report Size (8)
    0x15, 0x00,                    //   Logical Minimum (0)
    0x25, 0xff,                    //   Logical Maximum (1)
    0x05, 0x07,                    //   Usage Page (Key codes)
    0x19, 0x00,                    //   Usage Minimum (Reserved (no event indicated))
    0x29, 0xff,                    //   Usage Maxium (Reserved)
    0x81, 0x00,                    //   Input (Data, Array)

    0xc0,                          // End collection

    0x05, 0x01,                    // USAGE_PAGE (Generic Desktop)
    0x09, 0x02,                    // USAGE (Mouse)
    0xa1, 0x01,                    // COLLECTION (Application)

    0x85,  0x02,                    // Report ID 1

    0x09, 0x01,                    //   USAGE (Pointer)

    0xa1, 0x00,                    //   COLLECTION (Physical)

    0x05, 0x09,                    //     USAGE_PAGE (Button)
    0x19, 0x01,                    //     USAGE_MINIMUM (Button 1)
    0x29, 0x03,                    //     USAGE_MAXIMUM (Button 3)
    0x15, 0x00,                    //     LOGICAL_MINIMUM (0)
    0x25, 0x01,                    //     LOGICAL_MAXIMUM (1)
    0x95, 0x03,                    //     REPORT_COUNT (3)
    0x75, 0x01,                    //     REPORT_SIZE (1)
    0x81, 0x02,                    //     INPUT (Data,Var,Abs)
    0x95, 0x01,                    //     REPORT_COUNT (1)
    0x75, 0x05,                    //     REPORT_SIZE (5)
    0x81, 0x03,                    //     INPUT (Cnst,Var,Abs)

    0x05, 0x01,                    //     USAGE_PAGE (Generic Desktop)
    0x09, 0x30,                    //     USAGE (X)
    0x09, 0x31,                    //     USAGE (Y)
    0x15, 0x81,                    //     LOGICAL_MINIMUM (-127)
    0x25, 0x7f,                    //     LOGICAL_MAXIMUM (127)
    0x75, 0x08,                    //     REPORT_SIZE (8)
    0x95, 0x02,                    //     REPORT_COUNT (2)
    0x81, 0x06,                    //     INPUT (Data,Var,Rel)

    0xc0,                          //   END_COLLECTION
    0xc0                           // END_COLLECTION

I was searching throughout BTStack, maybe hids_device.c if a key to make separate extra characteristic instead of borrowing output one (no problem to add one inside .gatt file, but to put data on it). Full project dir: hog_kbd_test.zip

And .uf2 file but without mouse clicking, only moving and capital A typing: hog_kbd.zip for quick compatibility test. And maybe the same as above, but with that last keyboard key as a consumer device (pressing volume down here) under the same report_id 1 as keyboard to prove if it will (or not) work under Windows/Mac with BLE: hog_kbd_mutimedia.zip

mringwal commented 1 year ago

THX for the hint, it would be option 1, we know the number of reports at compile time. I'm happy with suggestions to implement this!

We did a first analysis of the requirements and sketched out changes to the gatt compiler and hids_device.c. The GATT compiler needs to support parameters for #include to specify the number of reports per type, a way to have an array of { characteristic, report reference } tuples, and also some kind of counter to enumerate the report ids. The hids_device.c needs storage for the additional reports and an index field for all send function.

Did we miss anything here?

We'll try to make the changes to the hid_device.c first as you can manually copy & paste & modify the current hids_device.gatt file and adapt it to your needs.

benjaminaigner commented 1 year ago

We did a first analysis of the requirements and sketched out changes to the gatt compiler and hids_device.c. The GATT compiler needs to support parameters for #include to specify the number of reports per type, a way to have an array of { characteristic, report reference } tuples, and also some kind of counter to enumerate the report ids. The hids_device.c needs storage for the additional reports and an index field for all send function.

That sounds exactly like what we need :+1:

Did we miss anything here?

I don't think so.

We'll try to make the changes to the hid_device.c first as you can manually copy & paste & modify the current hids_device.gatt file and adapt it to your needs.

Thank you very much, I think as well that the hid_device changes are important, the GATT file can be adapted manually.

benjaminaigner commented 1 year ago

@matsobdev Thank you very much for the intensive experimenting!

@mringwal Do you know how long it would take from your implementation to be included in the pico-sdk :-)?

matsobdev commented 1 year ago

Couldn't it be the way it is now, like adding an extra input report_id 4 characteristic in the project .gatt file, that will be corresponding to combo descriptor report_id 4 (value of 0x85 byte in the descriptor)? When I switch feature report to be an input one, and change mouse descriptor byte:

0x85,  0x03,                    // Report ID 3

this:

hids_device_send_feature_report(con_handle, (uint8_t[]) {0, 100, 100, 0}, 4);

works as expected.

mringwal commented 1 year ago

Do you know how long it would take from your implementation to be included in the pico-sdk :-)?

This will take a while as they currently prepare their 1.5.1 what uses our latest v1.5.6.2 release. But... you can just checkout the pico-sdk repo and update the btstack submodule to our develop branch for your development (and release binaries if needed).

mringwal commented 1 year ago

@benjaminaigner surprise! We've pushed a first update that supports multiple reports per type on the develop branch. Please manually add { report, report_reference } tuples to the .gatt file, and don't forget to set the report id in the report reference to be unique. When you'll use the new hids_device_init_with_storage, you can then send a report with a report id.

benjaminaigner commented 1 year ago

Wow, thank you very much! I'll test it and report back (unfortunately so many other things to do, so it will be next week)

matsobdev commented 1 year ago

It is a side question, but still HOG. Should in, out and feature (out and feature to be exact) had different properties? Like here https://www.bluetooth.org/docman/handlers/downloaddoc.ashx?doc_id=245140, table 2.1 for example feature:

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 3, type = Feature (3)
REPORT_REFERENCE, READ, 3, 3

without NOTIFY, like in a spec, and again table 2.4 the same document.

mringwal commented 1 year ago

@matsobdev good catch! yes, they are different. I've updated the hids.gatt file on develop to match the spec.

matsobdev commented 1 year ago

Noticed changes, but still output has extra not permitted NOTIFY.

Tricky matter. Chapter 2.2 of the same spec. document says:

2.2 Characteristic Overview The HID Service is composed of the following characteristics used to provide access to HID data. Unless otherwise specified, only one instance of each characteristic is permitted within a HID Service.

and in hids_devices.h for hids_device_init_with_storage():

Set up HIDS Device for multiple instances of INPUT, OUTPUT and FEATURE reports

Proper keyboard will have output for LEDs, might have feature for configuration and input no doubt about it. Now, for combo devices everything will be times 2, 3 or more. It is one device, so one channel for configuration will be more the enough I guess. With keyboard-mice combo will be unused output, since there is no output in mice (standard descriptor, some custom might have one, but now there will be no way to get rid of that unused output for standard mice for example). Multiple input reports tied with descriptor are that Unless otherwise specified?

For example, here: https://github.com/TexasInstruments/HOGP-BLE-HID-EXAMPLE/blob/main/source/ti/ble5stack/profiles/hid/hidkbdservice.h, they have:

// HID Report IDs for the service
// MODIFIED
#define HID_RPT_ID_CONSUMER_IN   0x3  // Consumer input report ID
#define HID_RPT_ID_KEY_IN        0x1  // Keyboard input report ID
#define HID_RPT_ID_MOUSE_IN      0x2  // Mouse input report ID
#define HID_RPT_ID_LED_OUT       0x0  // LED output report ID
#define HID_RPT_ID_FEATURE       0x0  // Feature report ID

Concluding, shouldn't flexibility be like adding one report characteristic at a time (instead of a full set) that is of a customisable type?

PS.: Again table 2.1. Now examples for keyboard and mice shares the same hids.gatt file, but according to the spec, legend C.2 and C.3 keyboard shall have ORG_BLUETOOTH_CHARACTERISTIC_BOOT_KEYBOARD_INPUT_REPORT and ORG_BLUETOOTH_CHARACTERISTIC_BOOT_KEYBOARD_OUTPUT_REPORT. ORG_BLUETOOTH_CHARACTERISTIC_BOOT_MOUSE_INPUT_REPORT will be excluded then and vice versa for mice. Only combo will have both (or actually three to be exact).

mringwal commented 1 year ago

Hi @matsobdev. Ah, only Input reports can notify. Noted and fixed. We then should also drop the send report for Feature and Output reports. It also looks like our implementation doesn't provide a way to get/set Input / Output reports either. I'll make a note about it.

BTstack avoids dynamic memory, so we assume that an app knows which reports it needs. With the hids_device_init_with_storage() you can specify how many reports you'd like to have and need to provide a suitable .gatt file for that. You're free to select any combination or reports, incl. e.g. no output reports.

You're correct that in boot mode for e.g. a keyboard, there should not be a mouse input report. With our architecture, this would require custom .gatt files. With the planned extension of the gatt compiler, we could model that.

matsobdev commented 1 year ago

We then should also drop the send report for Feature and Output reports. It also looks like our implementation doesn't provide a way to get/set Input / Output reports either. I'll make a note about it.

To be honest I was looking for something to READ or WRITE inside hids_device.c. So according to your answer (no need for hids_device_send_feature_report() and hids_device_send_output_report()) hids_device_send_input_report() is to NOTIFY?

mringwal commented 1 year ago

Yupp. HID Device uses a GATT Service and the only way to send something is by sending a Notification. For the Feature / Output Report, these are set by the HID Host, so it would be enough to send an event to the application upon Write/Write Without Response on these.

benjaminaigner commented 1 year ago

@mringwal @matsobdev

Thank you very much for your work on this issue! I'll try to test it today!

THX

benjaminaigner commented 1 year ago

@mringwal

If tried to work with a fixed .gatt setup right now, simply because I'll try to build it dynamically when the other stuff is working :-). Is this file correct (I've added the other mandatory services here as well)?

PRIMARY_SERVICE, GAP_SERVICE
CHARACTERISTIC, GAP_DEVICE_NAME, READ, "HID Mouse"

// add Battery Service
#import <battery_service.gatt>

// add Device ID Service
#import <device_information_service.gatt>

// Human Interface Device 1812
PRIMARY_SERVICE, ORG_BLUETOOTH_SERVICE_HUMAN_INTERFACE_DEVICE
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_PROTOCOL_MODE, DYNAMIC | READ | WRITE_WITHOUT_RESPONSE,

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 1, type = Input (1)
REPORT_REFERENCE, READ, 1, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | WRITE_WITHOUT_RESPONSE | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 1, type = Output (2)
REPORT_REFERENCE, READ, 1, 2

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 3, type = Input (1)
REPORT_REFERENCE, READ, 3, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 4, type = Input (1)
REPORT_REFERENCE, READ, 4, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 5, type = Input (1)
REPORT_REFERENCE, READ, 5, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 0, type = Feature (3)
REPORT_REFERENCE, READ, 0, 3

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT_MAP, DYNAMIC | READ,
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_BOOT_KEYBOARD_INPUT_REPORT, DYNAMIC | READ | WRITE | NOTIFY,
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_BOOT_KEYBOARD_OUTPUT_REPORT, DYNAMIC | READ | WRITE | WRITE_WITHOUT_RESPONSE,
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_BOOT_MOUSE_INPUT_REPORT, DYNAMIC | READ | WRITE | NOTIFY,
// bcdHID = 0x101 (v1.0.1), bCountryCode 0, remote wakeable = 0 | normally connectable 2
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_HID_INFORMATION,  READ, 01 01 00 02
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_HID_CONTROL_POINT, DYNAMIC | WRITE_WITHOUT_RESPONSE,

When using this ATT DB, it is connecting and Wireshark shows properly the characteristics declaration. BUT: the host tries to read all of them, it works until the protocol mode. The first read of the first report is not answered by the PicoW, and after ~30s the host disconnects.

Do you have any hint for me? Thanks!

mringwal commented 1 year ago

Hi @benjaminaigner In your .gatt file, the report ids are not unique, the first two use the same. While I don't know if these only need to be unique per report type, our code currently assumes that they are unique.

If a ATT Request is not answered by a GATT Server, the transaction failed and the connection might get closed as it cannot be used anymore.

The other thing, as mentioned in reply to @matsobdev, is that there's currently no support for Feature or Output write. Could you attache a .pklg file here? Does the host actually read the Report Characteristics?

benjaminaigner commented 1 year ago

I'm using the same report ID for keyboard in & out as well as 0 for the feature report. This works with all tested OS. Of course, if the BTStack implementation requires unique IDs, I'll change that.

Should I remove the out & feature reports?

The host sends an ATT read request to handle 0x001f, which is the first report characteristics handle.

Here is a Wireshark capture of the traffic: hog_composite.pcapng.gz

mringwal commented 1 year ago

Thanks. Ok, another look into the spec won't hurt here - although the USB HID Spec might have the final word. Yes, please use unique report ids at least for now.

Two things here: a) the host really reads the input report. At that time, the HOG Device Service doesn't know what to answer. I guess the user application would need to provide a memory buffer to store the report data for each report, such that it can properly handle the read. It would need to know at least the size to send back an empty report. b) there's no response to that read. Even if the HOG Device Service returns 0 for the att_read_callback, the att_server should still send a response back.

If you like, please run your test code on a desktop and try to find out why BTstack doesn't send anything back. (it passes att_handle_request -> handle_read_request -> handle_read_request2 which at least returns 1, but att_server will send the response if > 0)

benjaminaigner commented 1 year ago

ad a) maybe use something similar to HIDS_SUBEVENT_CAN_SEND_NOW? This would trigger a report response?

I could try to setup a Desktop test, but currently I'm working directly with the PicoW (and the arduino-pico core), so I need to boil it down to a HOG composite example for working without Arduino.

mringwal commented 1 year ago

GATT Server has a way to postpone a Read/Write operation, but I don't like it/would prefer to avoid it. If there's a Read, it requires a value to return, we cannot ignore it and send a Notify instead. So either the the deferred response mechanism, or a buffer that can be accessed by the service. (the service could emit an event which requires the app to call a function back, but that's not elegant). Yet another option would be read report callback.

Test code that's close to our examples makes debugging much easier.

benjaminaigner commented 1 year ago

Dear @mringwal ,

I think I found a bug in hids_device_get_report_for_client_configuration_handle & hids_device_get_report_for_id:

In both functions, the pos counter is not increased and BTStack hangs up in these while loops.

Adding following at the end of the while loop:

pos++; //get next handle / report
if(pos == 0xFF) return NULL; //avoid any wrap-around endless loops (e.g. if all report_nums are 0)

Fixes the hangup and the device connects (still no success with composite devices :-))

benjaminaigner commented 1 year ago

Hi again @mringwal,

I'm not sure, but I think I found another problem within hids_device_init_with_storage here: https://github.com/bluekitchen/btstack/blob/249b703b04d67c7b50dac2cf26bd62a06ecf703d/src/ble/gatt-service/hids_device.c#LL352C53-L352C53

Is it possible you need hids_device_report_t * report = &report_storage[assigned_reports_num]; or hids_device_report_t * report = &instance->hid_reports[assigned_reports_num];

instead of

hids_device_report_t * report = &hid_reports_storage[assigned_reports_num];

?

Simply because:

mringwal commented 1 year ago

Hi @benjaminaigner Thanks for pointing out the bugs. I've taken care of these and also cleaned up the API. You're also right about different report types with the same report id. Looks like I got it backwards starting from the HOG specification. If you start with the USB HID, it becomes clear: the hid descriptor describes reports. A report may have a report id and contain input, output and feature fields. Filtering by type, we get up to three reports with the same report id...

Could you check the current version? I think sending input reports should be complete now. What's missing is: a) providing the data for SetReport to the application (for all types) b) support GetReport by the HID Host (again for all types)

The current idea would be to emit events for a) and allow the application to set the data for each {report id/type} that is returned by GetReport. This would allow the app also to validate data from the HID Host, e.g. the host sets invalid data, but won't read it back as the app can ignore or sanitize it.

matsobdev commented 1 year ago

Hi! Again with the same what's above:

// HID Report IDs for the service
// MODIFIED
#define HID_RPT_ID_CONSUMER_IN   0x3  // Consumer input report ID
#define HID_RPT_ID_KEY_IN        0x1  // Keyboard input report ID
#define HID_RPT_ID_MOUSE_IN      0x2  // Mouse input report ID
#define HID_RPT_ID_LED_OUT       0x0  // LED output report ID
#define HID_RPT_ID_FEATURE       0x0  // Feature report ID

Output and feature with the same 0x0 id and leave input ids mess (feature and output) free. The same for simple (input plus output/feature) and composite device (multiple inputs plus output/feature).

PS. But still, a far I'm concerned for example pairs input/output, output/feature still shall have separate characteristics - they have different mandatory and excluded properties - table 2.4 of https://www.bluetooth.org/docman/handlers/downloaddoc.ashx?doc_id=245140

benjaminaigner commented 1 year ago

@mringwal

THX for the fixes, the lookup is working, the ATT DB is created in the correct location.

One thing is still not working:

hids_device.c:400: start_chr_handle = chr_client_configuration_handle + 1; This line is still repeating the handle creation, where value handles don't get a unique number.

Replacing it with this one works:

start_chr_handle = chr_value_handle + 2;

With this fix, the mouse moves with a combined example, but I've still problems with keyboard & joystick. I'll try to figure it out and come back here. When I'm finished I can provide you an example for BTStack.

@matsobdev

I've changed the report ID assignment for the BTStack example, but it is definitely working on the Bluedroid stack (ESP32).

mringwal commented 1 year ago

@benjaminaigner thanks for the feedback. What's your current .gatt file that causes the issue with the handle lookup?

benjaminaigner commented 1 year ago

Dear @mringwal

I used this gatt file, mouse is working with this one when applying my fix:

PRIMARY_SERVICE, GAP_SERVICE
CHARACTERISTIC, GAP_DEVICE_NAME, READ, "HID Mouse"

// add Battery Service
#import <battery_service.gatt>

// add Device ID Service
#import <device_information_service.gatt>

// Human Interface Device 1812
PRIMARY_SERVICE, ORG_BLUETOOTH_SERVICE_HUMAN_INTERFACE_DEVICE
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_PROTOCOL_MODE, DYNAMIC | READ | WRITE_WITHOUT_RESPONSE,

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 1, type = Input (1); keycodes
REPORT_REFERENCE, READ, 1, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | WRITE_WITHOUT_RESPONSE | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 5, type = Output (2) LED
REPORT_REFERENCE, READ, 5, 2

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 2, type = Input (1) consumer
REPORT_REFERENCE, READ, 2, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 3, type = Input (1) mouse
REPORT_REFERENCE, READ, 3, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | NOTIFY | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 4, type = Input (1) gamepad
REPORT_REFERENCE, READ, 4, 1

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT, DYNAMIC | READ | WRITE | ENCRYPTION_KEY_SIZE_16,
// fixed report id = 6, type = Feature (3)
REPORT_REFERENCE, READ, 6, 3

CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_REPORT_MAP, DYNAMIC | READ,
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_BOOT_KEYBOARD_INPUT_REPORT, DYNAMIC | READ | WRITE | NOTIFY,
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_BOOT_KEYBOARD_OUTPUT_REPORT, DYNAMIC | READ | WRITE | WRITE_WITHOUT_RESPONSE,
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_BOOT_MOUSE_INPUT_REPORT, DYNAMIC | READ | WRITE | NOTIFY,
// bcdHID = 0x101 (v1.0.1), bCountryCode 0, remote wakeable = 0 | normally connectable 2
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_HID_INFORMATION,  READ, 01 01 00 02
CHARACTERISTIC, ORG_BLUETOOTH_CHARACTERISTIC_HID_CONTROL_POINT, DYNAMIC | WRITE_WITHOUT_RESPONSE,

EDIT: I'm not quite sure how to implement the LED output report, because in my working HID report map, the output is integrated with the keyboard application and has no unique report ID (there is only one output report, therefore no ID necessary). This may mess up the keyboard interpretation. I'm working on it & report back here.

EDIT2/Update: The keyboard keys work as well, still missing or to be tested is the consumer report & the joystick. If its working, I will provide the example code and we are finished here. Thank you very much for your help @mringwal & @matsobdev

mringwal commented 1 year ago

@benjaminaigner A generic keyboard HID descriptor uses a single report for the keys and the LEDs. In HOG, these results in two report characteristics with the same report id. As mentioned before, the read/write operations are not yet implemented, so LEDs cannot work. We'll get there, but I'll look at the reported issue with the handle lookup next/soon-ish. :)

benjaminaigner commented 1 year ago

No hurry, LEDs are the least priority for us as well :-)

benjaminaigner commented 1 year ago

Everything is working, PR:

https://github.com/bluekitchen/btstack/pull/493

Thanks for your help, I think I can close this issue now.

mringwal commented 1 year ago

Thanks again for pointing out that the logic doesn't work for reports without notify. It's fixed on the develop branch.

benjaminaigner commented 1 year ago

Perfect, THX!

benjaminaigner commented 1 year ago

Dear @mringwal ,

thx for your help on this issue, I've now completed the implementation of the easy Arduino libraries (Mouse Keyboard Joystick) using BTStack on the RaspberryPi PicoW.

There is one additional note regarding license/sourcecode within Earle Philhowers arduino-pico core:

Because the official pico-sdk will take some time to implement the new functionality and Earle doesn't want to fiddle around with the upstream pico-sdk, we handled it by copying the att_db & hid_device source code files into the sdkoverride folder. I'm not sure, but I hope it is OK for Bluekitchen, we will remove this file of course as soon as this change is pushed into release of BTStack and pico-sdk updates its BTStack version.

And of course, there is no copyright removal or anything else, hope this is OK for you.

Greetings

mringwal commented 1 year ago

@benjaminaigner So far I haven't used ESP32 and Pico W with Arduino. Where are these files in the arduino-pico repo? In general, temporarily copying these files without modification is ok.

benjaminaigner commented 1 year ago

The files are here (currently an open PR): https://github.com/earlephilhower/arduino-pico/pull/1587/files (folder cores/rp2040/sdkoverride)

I just changed the guard:

#if defined ENABLE_CLASSIC

(otherwise the header would be compiled, even if the BT stack is disabled in the menu).

Greetings

mringwal commented 1 year ago

Thanks for the link. Ok, looks good. Please try to remove the duplicate files after the next Pico SDK release.