bt_att_write_hid_report() must take into account GATT characteristic properties

DJm00n commented 1 year ago

BlueRetro firmware version

1.8.3

BlueRetro firmware specification

HW1

BlueRetro firmware variant

System specific

BlueRetro hardware type

External adapter with detachable cord

Manufacturer

BlueRetro Gaming Store

System used

Sony PlayStation 2

Bluetooth controller brand & name

Google Stadia Controller

What is problem? (only list ONE problem per report)

According to the HIDS spec Read/Write/Write Without Response properties are mandatory for Report: Output Report Type characteristic.

But seems some devices are not conforming to the spec and can omit Write Without Response or Write characteristic properties and fail to handle output HID reports and in result vibration is not working.

For example Stadia Controller with new "Bluetooth mode" firmware is known to have such issue:

What did you expect to happen?

bt_att_write_hid_report() code should read GATT characteristic properties and do corresponding ATT_OP_WRITE_REQ or ATT_OP_WRITE_CMD operation if possible and skip Report ID byte if only one output report with zero id is declared in Report Descriptor: https://github.com/darthcloud/BlueRetro/blob/a7b2d7678bab85e8affcca5d21c37002530f4c68/main/bluetooth/att_hid.c#L83-L89

Something like this:

if (report->decl->properties & BT_GATT_CHRC_WRITE)
    bt_att_cmd_write_req(..) -> ATT_OP_WRITE_REQ (0x12)
else if (report->decl->properties & BT_GATT_CHRC_WRITE_WITHOUT_RESP)
    bt_att_cmd_write_cmd(...) -> ATT_OP_WRITE_CMD (0x52)

Attach files like logs or Bluetooth traces here

Here is Wireshark dump from the PC<=>Stadia Controller - this is the moment of connection of already paired controller: stadia_bluetoothle.pcapng.gz

DJm00n commented 1 year ago

darthcloud commented 1 year ago

BlueRetro is already using the write with rsp cmd (0x12) just like in your wireshark trace.

So while doing this would be the right way to do things in this case it's not the cause of the problem.

Stadia report BT_GATT_CHRC_WRITE and BlueRetro use cmd 0x12

darthcloud commented 1 year ago

The problem is that the HID parser find no rumble usage in the Stadia descriptor: # 3 0139 0 4, 0912 8 15 0130 24 8, 0131 32 8, 0132 40 8, 0135 48 8, 02C5 56 8, 02C4 64 8, 0CE9 72 2 0CCD 74 1 rtype: 2 dtype: 0 sub: 0 # 5

That 5 with nothing after is the rumble report

For reference a Xbox X|S: # 1 0130 0 16, 0131 16 16, 0132 32 16, 0135 48 16, 02C5 64 10, 02C4 80 10, 0139 96 4, 0901 104 15 0CB2 120 1 rtype: 2 dtype: 3 sub: 9 # 3 0F21 0 4, 0F70 8 8, 0F70 16 8, 0F70 24 8, 0F70 32 8, 0F50 40 8, 0FA7 48 8, 0F7C 56 8, rtype: 4 dtype: 0 sub: 0

rtype 4 means it recognized the report as a rumble one.

Without this no packet can be send to the controller.

Here the stadia descriptor for report 5: 0x85, 0x05, // Report ID (5) 0x06, 0x0F, 0x00, // Usage Page (PID Page) 0x09, 0x97, // Usage (0x97) 0x75, 0x10, // Report Size (16) 0x95, 0x02, // Report Count (2) 0x27, 0xFF, 0xFF, 0x00, 0x00, // Logical Maximum (65534) 0x91, 0x02, // Output (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile) 0xC0, // End Collection

The problem is that the Usage Page used is not USAGE_GEN_PHYS_INPUT as @JPZV code expect.

Another issue is that it's a 16bits page which BlueRetro currently do not support.

It's late now but I'll look at supporting all page size and add the PID page to the rumble list and see what happen.

DJm00n commented 1 year ago

@darthcloud thank you for looking into it.

Regarding BT_GATT_CHRC_WRITE_WITHOUT_RESP property - yes, it seems it is not related in this case with Stadia. But this thing that could be improvement in general BLE handling. Just task for a backlog then.

darthcloud / BlueRetro