bt_att_write_hid_report() must take into account GATT characteristic properties

[DJm00n]

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:

Related code in Bluez:
https://github.com/bluez/bluez/blob/a1736d8990ff56bba453ff81a25156316bdd118f/profiles/input/hog-lib.c#L806-L812

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:

BlueRetro/main/bluetooth/att_hid.c

Lines 83 to 89 in a7b2d76

	void bt_att_write_hid_report(struct bt_dev *device, uint8_t report_id, uint8_t *data, uint32_t len) {
	uint16_t att_handle = bt_att_get_report_handle(&att_hid[device->ids.id], report_id);
	if (att_handle) {
	bt_att_cmd_write_req(device->acl_handle, att_handle, data, len);
	}
	}

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]

Related issues: #578 #622

[darthcloud]

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]

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]

@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.