Mathieu,
Quite possibly! I discovered this while trying to implement a user-space
application for a commercial product, which uses multiple reports quite
heavily, as it's a lot more complex than the usual keyboard or mouse.
I hope your project works as expected once this patch is applied, and
that it leads to more companies developing more complex HID products now
that it should work correctly. You shouldn't need any special "is
Bluetooth" handling with this fix, as the reports should appear
identical to a regular USB HID device (as it should be).
The HID report byte has caused untold amounts of bugs and confusion
after it was decided to omit it for single report devices in the
original USB HID spec to save space on the wire, which was a neccesary
evil at the time to make the basic reports fit into a Low Speed USB
device packet, but still causes trouble today. The fact that it's not
sent over the air along with the report on HOG just compounds the confusion.
- Dean
On 21/11/2020 9:10 am, mathieu.stephan@xxxxxxxxx wrote:
Hello Dean and thanks for your reply,
I had completely missed the preprended byte would only be there for
multiple HOG devices.
I fully agree with your assessment, as we indeed discovered segfaults
when our 2 reports HoG would connect to Linux
(https://github.com/mooltipass/moolticute/issues/671
<https://github.com/mooltipass/moolticute/issues/671>).
That makes me think we (until recently, maybe) are the only ones to have
such a device and (unlucky) scenario.
Mathieu
On Fri, Nov 20, 2020 at 11:00 PM Dean Camera <dean@xxxxxxxxxxxxxxxxxxxxx
<mailto:dean@xxxxxxxxxxxxxxxxxxxxx>> wrote:
The existing behaviour for a HOG device with report IDs was broken, and
likely wouldn't have worked at all as the first byte of the response
was
being discarded.
The missing prepended report ID is theoretically something that would
cause an observable difference to userspace, but as it's only added in
the multiple report case that was broken I don't think anyone will be
affected.
Single report HOG devices make up for the majority of consumer HID
devices, which is probably why this has gone unnoticed for so long, and
the behaviour of those is unaffected by my patch.
The only use case I can see that would be broken would be a userspace
app using HIDRAW to communicate with a multiple report HOG device,
which
was already tolerant of missing the first byte if the report.
Cheers,
- Dean
On 21/11/2020 6:56 am, mathieu.stephan@xxxxxxxxx
<mailto:mathieu.stephan@xxxxxxxxx> wrote:
> Hello All,
>
> Is there a way to communicate to users that particular change?
> I'm reacting as this is something our team heavily relies upon
>
(https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91
<https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91>
>
<https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91
<https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91>>)
> and I'm guessing we're far from the only ones :)
>
> Regards,
> Mathieu
>
> On Fri, Nov 20, 2020 at 6:52 PM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx <mailto:luiz.dentz@xxxxxxxxx>
<mailto:luiz.dentz@xxxxxxxxx <mailto:luiz.dentz@xxxxxxxxx>>> wrote:
>
> Hi Dean,
>
> On Thu, Nov 19, 2020 at 5:47 PM Dean Camera
> <dean@xxxxxxxxxxxxxxxxxxxxx
<mailto:dean@xxxxxxxxxxxxxxxxxxxxx>
<mailto:dean@xxxxxxxxxxxxxxxxxxxxx
<mailto:dean@xxxxxxxxxxxxxxxxxxxxx>>> wrote:
> >
> > If the HID subsystem requests a HID report to be read from the
> > device, we currently incorrectly strip off the first byte
of the
> > response, if the device has report IDs set in the HID report
> > descriptor.
> >
> > This is incorrect; unlike USB HID, the report ID is *not*
included
> > in the HOG profile's HID reports, and instead exists out
of band
> > in a descriptor on the report's bluetooth characteristic
in the
> > device.
> >
> > In this patch, we remove the erroneous stripping of the first
> > byte of the report, and (if report IDs are enabled)
prepend the
> > report ID to the front of the result. This makes the HID
report
> > returned indentical in format to that of a USB HID report, so
> > that the upper HID drivers can consume HOG device reports
in the
> > same way as USB.
> > ---
> > profiles/input/hog-lib.c | 18 +++++++++++-------
> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/profiles/input/hog-lib.c
b/profiles/input/hog-lib.c
> > index 78018aad3..49d459e21 100644
> > --- a/profiles/input/hog-lib.c
> > +++ b/profiles/input/hog-lib.c
> > @@ -779,7 +779,8 @@ fail:
> > static void get_report_cb(guint8 status, const guint8 *pdu,
> guint16 len,
> >
gpointer
> user_data)
> > {
> > - struct bt_hog *hog = user_data;
> > + struct report *report = user_data;
> > + struct bt_hog *hog = report->hog;
> > struct uhid_event rsp;
> > int err;
> >
> > @@ -808,13 +809,16 @@ static void get_report_cb(guint8
status, const
> > guint8 *pdu, guint16 len,
> >
> > --len;
> > ++pdu;
> > +
> > if (hog->has_report_id && len > 0) {
> > - --len;
> > - ++pdu;
> > + rsp.u.get_report_reply.size = len + 1;
> > + rsp.u.get_report_reply.data[0] = report->id;
> > + memcpy(&rsp.u.get_report_reply.data[1],
pdu, len);
> > + }
> > + else {
> > + rsp.u.get_report_reply.size = len;
> > + memcpy(rsp.u.get_report_reply.data, pdu, len);
> > }
> > -
> > - rsp.u.get_report_reply.size = len;
> > - memcpy(rsp.u.get_report_reply.data, pdu, len);
> >
> > exit:
> > rsp.u.get_report_reply.err = status;
> > @@ -846,7 +850,7 @@ static void get_report(struct
uhid_event *ev,
> void
> > *user_data)
> >
> > hog->getrep_att = gatt_read_char(hog->attrib,
> >
report->value_handle,
> > -
get_report_cb, hog);
> > + get_report_cb,
> report);
> > if (!hog->getrep_att) {
> > err = ENOMEM;
> > goto fail;
> > --
> > 2.29.2.windows.2
> >
>
> Applied, thanks.
>
> --
> Luiz Augusto von Dentz
>
