On Sun, 10 Jan 2021 00:04:21 +0100 Marcus Glocker <[email protected]> wrote:
> On Sat, 9 Jan 2021 21:51:03 +0100 (CET) > Mark Kettenis <[email protected]> wrote: > > > > Date: Sat, 9 Jan 2021 10:38:20 +0100 > > > From: Marcus Glocker <[email protected]> > > > > > > I have not much clue about HID, but when we did some testing for > > > the new ujoy(4) driver it turned out that the PS4 controller > > > doesn't get handled correctly by hid.c:hid_is_collection(). This > > > made me peek in to the NetBSD code where I could find an update > > > in this function. > > > > > > Syncing it up makes the PS4 controller attachment work correctly, > > > while not breaking my other HID devices. > > > > > > The change was introduced in NetBSD with hid.c revision 1.25 on > > > the 19.06.2006, obviously as part of a bigger change, not saying > > > something about the hid_is_collection() update itself: > > > > > > -- > > > > > > Initial import of bluetooth stack on behalf of Iain Hibbert. > > > (plunky@, NetBSD Foundation Membership still pending.) This stack > > > was written by Iain under sponsorship from Itronix Inc. > > > > > > The stack includes support for rfcomm networking (networking via > > > your bluetooth enabled cell phone), hid devices (keyboards/mice), > > > and headsets. > > > > > > Drivers for both PCMCIA and USB bluetooth controllers are > > > included. > > > > > > -- > > > > > > http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/usb/Attic/hid.c.diff?r1=1.24&r2=1.25&only_with_tag=MAIN > > > > > > More testers? Comments? OKs? > > > > Probably needs some testing with touchpad and touchscreen devices. > > Hmm, yes. I just quickly tested this on my X1 with ums(4) > touchscreen. Beside that it still works, the diff seems to fix > something in the ums attach. Before the diff: > > uhidev0 at uhub0 port 10 configuration 1 interface 0 "Raydium > Corporation Raydium Touch System" rev 2.01/1.08 addr 4 uhidev0: > iclass 3/0, 68 report ids uhid0 at uhidev0 reportid 1: input=0, > output=63, feature=0 uhid1 at uhidev0 reportid 2: input=63, output=0, > feature=0 uhid2 at uhidev0 reportid 3: input=0, output=63, feature=0 > uhid3 at uhidev0 reportid 4: input=0, output=63, feature=0 > uhid4 at uhidev0 reportid 5: input=0, output=63, feature=0 > uhid5 at uhidev0 reportid 6: input=63, output=0, feature=0 > uhid6 at uhidev0 reportid 7: input=0, output=63, feature=0 > uhid7 at uhidev0 reportid 8: input=0, output=63, feature=0 > uhid8 at uhidev0 reportid 9: input=63, output=0, feature=0 > ums0 at uhidev0 reportid 10: 1 button, tip > wsmouse2 at ums0 mux 0 > uhid9 at uhidev0 reportid 11: input=0, output=0, feature=1 > uhid10 at uhidev0 reportid 12: input=0, output=0, feature=255 > uhid11 at uhidev0 reportid 13: input=0, output=0, feature=255 > ums1 at uhidev0 reportid 68 > ums1: mouse has no X report > > I didn't notice before, but ums1 seems not to make sense, since there > is no second ums device attached here. > > After the diff the reportid 68 just turns in another uhid of uhidev0 > which makes more sense I guess: > > uhidev0 at uhub0 port 10 configuration 1 interface 0 "Raydium > Corporation Raydium Touch System" rev 2.01/1.08 addr 4 uhidev0: > iclass 3/0, 68 report ids uhid0 at uhidev0 reportid 1: input=0, > output=63, feature=0 uhid1 at uhidev0 reportid 2: input=63, output=0, > feature=0 uhid2 at uhidev0 reportid 3: input=0, output=63, feature=0 > uhid3 at uhidev0 reportid 4: input=0, output=63, feature=0 > uhid4 at uhidev0 reportid 5: input=0, output=63, feature=0 > uhid5 at uhidev0 reportid 6: input=63, output=0, feature=0 > uhid6 at uhidev0 reportid 7: input=0, output=63, feature=0 > uhid7 at uhidev0 reportid 8: input=0, output=63, feature=0 > uhid8 at uhidev0 reportid 9: input=63, output=0, feature=0 > ums0 at uhidev0 reportid 10: 1 button, tip > wsmouse2 at ums0 mux 0 > uhid9 at uhidev0 reportid 11: input=0, output=0, feature=1 > uhid10 at uhidev0 reportid 12: input=0, output=0, feature=255 > uhid11 at uhidev0 reportid 13: input=0, output=0, feature=255 > uhid12 at uhidev0 reportid 68: input=0, output=0, feature=255 jcs@ found that it breaks imt(4), and maybe ims(4). So unfortunately that doesn't seem to be a solution either. > > > Index: dev/hid/hid.c > > > =================================================================== > > > RCS file: /cvs/src/sys/dev/hid/hid.c,v > > > retrieving revision 1.3 > > > diff -u -p -u -p -r1.3 hid.c > > > --- dev/hid/hid.c 4 Jun 2020 23:03:43 -0000 1.3 > > > +++ dev/hid/hid.c 9 Jan 2021 08:43:30 -0000 > > > @@ -630,6 +630,25 @@ hid_get_udata(const uint8_t *buf, int le > > > return (hid_get_data_sub(buf, len, loc, 0)); > > > } > > > > > > +/* > > > + * hid_is_collection(desc, size, id, usage) > > > + * > > > + * This function is broken in the following way. > > > + * > > > + * It is used to discover if the given 'id' is part of 'usage' > > > collection > > > + * in the descriptor in order to match report id against device > > > type. > > > + * > > > + * The semantics of hid_start_parse() means though, that only a > > > single > > > + * kind of report is considered. The current HID code that uses > > > this for > > > + * matching is actually only looking for input reports, so this > > > works > > > + * for now. > > > + * > > > + * This function could try all report kinds (input, output and > > > feature) > > > + * consecutively if necessary, but it may be better to integrate > > > the > > > + * libusbhid code which can consider multiple report kinds > > > simultaneously > > > + * > > > + * Needs some thought. > > > + */ > > > int > > > hid_is_collection(const void *desc, int size, uint8_t id, int32_t > > > usage) { > > > @@ -637,17 +656,25 @@ hid_is_collection(const void *desc, int > > > struct hid_item hi; > > > uint32_t coll_usage = ~0; > > > > > > - hd = hid_start_parse(desc, size, hid_none); > > > + hd = hid_start_parse(desc, size, hid_input); > > > + if (hd == NULL) > > > + return (0); > > > > > > DPRINTF("%s: id=%d usage=0x%x\n", __func__, id, usage); > > > while (hid_get_item(hd, &hi)) { > > > DPRINTF("%s: kind=%d id=%d usage=0x%x(0x%x)\n", > > > __func__, hi.kind, hi.report_ID, hi.usage, coll_usage); > > > + > > > if (hi.kind == hid_collection && > > > hi.collection == HCOLL_APPLICATION) > > > coll_usage = hi.usage; > > > - if (hi.kind == hid_endcollection && > > > - coll_usage == usage && hi.report_ID == id) { > > > + > > > + if (hi.kind == hid_endcollection) > > > + coll_usage = ~0; > > > + > > > + if (hi.kind == hid_input && > > > + coll_usage == usage && > > > + hi.report_ID == id) { > > > DPRINTF("%s: found\n", __func__); > > > hid_end_parse(hd); > > > return (1); > > > > > > >
