Puzzled by HID-over-GATT, feature, report ID

[Dvad]

I have been playing a bit with a HOGP device (microsoft surface dial) under linux and found a strange behavior when using linux's hidraw interface to get and send feature reports. I am sharing this with you because it is puzzling me, and because I might have found a bug in bluez but do not have enough familiarity with Bluetooth stack to recognize it.

What I do:

1. I send a HID feature report with report_id 1 and the following sequence of bytes [255, 0, 1, 3, 1, 80, 0] as payload. This is a totally valid payload for the device, although its meaning does not matter here.
2. I request just after a HID feature report with same id and look at the payload.

What I expect:
The buffer should contain the byte I just sent [255, 0, 1, 3, 1, 80, 0] as detailed in hidraw docs :

HIDIOCGFEATURE(len): Get a Feature Report
[...] The report will be returned starting at
the first byte of the buffer (ie: the report number is not returned).

What I observe:
The returned buffer contains [0, 1, 3, 1, 80, 0, 0]. It seems to be shifted by one byte and truncated (255 is not here anymore).

---

I did some investigation, and ended up -- after trying to find anything in linux hidp module 🤦‍♂️ -- in the implementation of HOGP in bluez.
I found this in get_report_cb :

bluez/profiles/input/hog-lib.c

Lines 823 to 828 in a106a3a

	--len;
	++pdu;
	if (hog->has_report_id && len > 0) {
	--len;
	++pdu;
	}

I am not familiar at all with the bluetooth stack so I don't quite understand why the two first bytes are removed from the buffer here. However, this seems to be the only place that could cause my issue in the entire stack.
After digging a bit more, I also read here that in the case of HOGP, the report id of HID are not serialized with the data sent over the air. (which I also kind of decrypt from the section 4.8.1 of the HOGP spec but it is not clear).

That makes me wonder if the report_id is really present in pdu buffer, and if the code should rather be:

	--len;
	++pdu;
        // the following is useless as the HID is not returned over GATT
	//if (hog->has_report_id && len > 0) {
	//	--len;
	//	++pdu;
	//}

And indeed, testing this gives me the expected behavior.

This solution looks very hacky dark magic to me : I guess such a deep change in the stack would break other things.
What do you think? Should I look elsewhere? The only alternative I can think of is that I am dealing with a non-compliant device. 🙃

---

The following python code snippet can be used to reproduce. It will be surely not useful for someone without the device, posting it for completeness.

# hidraw comes from
# https://github.com/vpelletier/python-hidraw simple
# passtrhough around hidraw ioctl, seems fine
import hidraw
import struct
a = hidraw.HIDRaw(open("/dev/hidraw0", "w+b"))

# byte payload, no report-id, will be added just before sendinf
toSend = struct.pack(forma_inv, 255, 0 , 1, 3, 1, 80, 0)

# 1 = report_id
a.sendFeatureReport(toSend, 1)

# 1 = report_id, 6 = size of expected buffer - 1 (weirdness of hidraw)
ret = a.getFeatureReport(1, length=6)  

print("Payload sent", struct.unpack("BBBBBBB", toSend))
print("Payload received", struct.unpack("BBBBBBB", ret)) 

print

Payload sent (255, 0, 1, 3, 1, 80, 0)
Payload received (0, 1, 3, 1, 80, 0, 0)

[Vudentz]

I wonder if the original intend was to actually skip the report_id in case there are multiple reports over the same characteristic, which I believe it would be against the spec, so I guess we either have to fix has_report_id to really detect the presence of report_id on the payload or just assume there is never suppose to be a report_id which is basically what you did.

[Dvad]

Hi @Vudentz thanks for reading me and for maintaining this project!

That would makes sense. I wonder how many client rely on this specific behaviour, I expected plenty, but maybe getting features report are not so common. I would actually not really need it for writing this device driver and was just hacking around when I discovered it.

How easy it would be to safely detect if report_id is actually there?
Just testing the first byte of the payload would not be safe, as even having report id here might be a coincidence with a valid report.
Maybe we can detect the fact that multiple reports goes over the same characteristics somehow ?

[abcminiuser]

I just ran into this as well - in my case, I have a conformant device that does not include the report ID in the bluetooth characteristic, and as a result the returned report data has the first data byte lost entirely.

I suggest that we don't massage the returned buffer, just give back the read bytes as-is. This is perhaps safest as it gives the widest range of compatibility and moves the responsibility of dealing with device-specific quirks upstream where it it more able to handle it. In Linux, that means either the regular hid input subsystem would need to length check and deal with the report ID byte if it was included, or in the case of hidraw, the end-user application would have to do the check.

As it stands now we have the worst of both worlds: non-compliant devices' data is returned correctly, while compliant devices have the first data byte discarded, breaking user applications.

[abcminiuser]

This should now be resolved by 35a2c50.

[Dvad]

Awesome! Thanks @abcminiuser.