On Fri, Jan 15, 2021 at 06:23:01AM -0700, Thomas Frohwein wrote: > On Sat, Jan 09, 2021 at 10:16:16AM +0100, Marcus Glocker wrote: > > On Thu, Jan 07, 2021 at 08:20:35PM +0100, Marcus Glocker wrote: > > > > > > I have heard from others who tried the diff that the PS4 controller is > > > > causing problems with the way it attaches. I ordered one to trial-and- > > > > error this myself at home. Could you share output of lsusb -vv? Thanks > > > > for giving it a try! > > > > > > Sure, here we go. > > > If I can find anything myself in the meantime I let you know. > > > > So the problem doesn't seem to be in your new ujoy(4) code, but how the > > dev/hid/hid.c:hid_is_collection() function tries to cope with the PS4 > > controller. > > So with the hid_is_collection() problem not easy to mitigate [1], > should we table the ujoy(4) proposal for now pending a fix for the > problems with the PS4 controller? Or is this interesting enough for > some to work on moving forward despite this issue and finding a > solution for this specific (and in some ways unusual) device later?
Considering the hid_is_collection() fix from NetBSD proposed by Marcus fixes the issue with ujoy(4) but breaks some other drivers (imt(4) and ims(4)), could we not inline it into ujoy(4) for now? It's fairly small helper function. Like hid_is_joy_collection()? -Bryan. > 3-4 have tested and reported to me so far. It seems so far that the > only new breakage is with the PS4 controller, and there is probably > another solution that can be found later that doesn't break other > drivers like [1]? > > [1] https://marc.info/?l=openbsd-tech&m=161043081617336&q=mbox > > > > > I'm not much in to HID, but when I sync up the hid_is_collection() > > function with NetBSD, the PS4 controller attaches to ujoy(4) as > > expected, and also can be accessed by usbhidctl(1), while my other > > HID devices continue to work as before: > > > > uaudio0 at uhub4 port 4 configuration 1 interface 1 "Sony Interactive > > Entertainment Wireless Controller" rev 2.00/1.00 addr 6 > > uaudio0: class v1, full-speed, sync, channels: 2 play, 1 rec, 4 ctls > > audio1 at uaudio0 > > uhidev6 at uhub4 port 4 configuration 1 interface 3 "Sony Interactive > > Entertainment Wireless Controller" rev 2.00/1.00 addr 6 > > uhidev6: iclass 3/0, 180 report ids > > ujoy0 at uhidev6 reportid 1: input=63, output=0, feature=0 <-- > > uhid6 at uhidev6 reportid 2: input=0, output=0, feature=36 > > uhid7 at uhidev6 reportid 4: input=0, output=0, feature=36 > > uhid8 at uhidev6 reportid 5: input=0, output=31, feature=0 > > uhid9 at uhidev6 reportid 8: input=0, output=0, feature=3 > > uhid10 at uhidev6 reportid 16: input=0, output=0, feature=4 > > uhid11 at uhidev6 reportid 17: input=0, output=0, feature=2 > > uhid12 at uhidev6 reportid 18: input=0, output=0, feature=15 > > uhid13 at uhidev6 reportid 19: input=0, output=0, feature=22 > > uhid14 at uhidev6 reportid 20: input=0, output=0, feature=16 > > uhid15 at uhidev6 reportid 21: input=0, output=0, feature=44 > > uhid16 at uhidev6 reportid 128: input=0, output=0, feature=6 > > uhid17 at uhidev6 reportid 129: input=0, output=0, feature=6 > > uhid18 at uhidev6 reportid 130: input=0, output=0, feature=5 > > uhid19 at uhidev6 reportid 131: input=0, output=0, feature=1 > > uhid20 at uhidev6 reportid 132: input=0, output=0, feature=4 > > uhid21 at uhidev6 reportid 133: input=0, output=0, feature=6 > > uhid22 at uhidev6 reportid 134: input=0, output=0, feature=6 > > uhid23 at uhidev6 reportid 135: input=0, output=0, feature=35 > > uhid24 at uhidev6 reportid 136: input=0, output=0, feature=34 > > uhid25 at uhidev6 reportid 137: input=0, output=0, feature=2 > > uhid26 at uhidev6 reportid 144: input=0, output=0, feature=5 > > uhid27 at uhidev6 reportid 145: input=0, output=0, feature=3 > > uhid28 at uhidev6 reportid 146: input=0, output=0, feature=3 > > uhid29 at uhidev6 reportid 147: input=0, output=0, feature=12 > > uhid30 at uhidev6 reportid 160: input=0, output=0, feature=6 > > uhid31 at uhidev6 reportid 161: input=0, output=0, feature=1 > > uhid32 at uhidev6 reportid 162: input=0, output=0, feature=1 > > uhid33 at uhidev6 reportid 163: input=0, output=0, feature=48 > > uhid34 at uhidev6 reportid 164: input=0, output=0, feature=13 > > uhid35 at uhidev6 reportid 165: input=0, output=0, feature=21 > > uhid36 at uhidev6 reportid 166: input=0, output=0, feature=21 > > uhid37 at uhidev6 reportid 167: input=0, output=0, feature=1 > > uhid38 at uhidev6 reportid 168: input=0, output=0, feature=1 > > uhid39 at uhidev6 reportid 169: input=0, output=0, feature=8 > > uhid40 at uhidev6 reportid 170: input=0, output=0, feature=1 > > uhid41 at uhidev6 reportid 171: input=0, output=0, feature=57 > > uhid42 at uhidev6 reportid 172: input=0, output=0, feature=57 > > uhid43 at uhidev6 reportid 173: input=0, output=0, feature=11 > > uhid44 at uhidev6 reportid 174: input=0, output=0, feature=1 > > uhid45 at uhidev6 reportid 175: input=0, output=0, feature=2 > > uhid46 at uhidev6 reportid 176: input=0, output=0, feature=63 > > uhid47 at uhidev6 reportid 177: input=0, output=0, feature=2 > > uhid48 at uhidev6 reportid 178: input=0, output=0, feature=2 > > uhid49 at uhidev6 reportid 179: input=0, output=0, feature=63 > > uhid50 at uhidev6 reportid 180: input=0, output=0, feature=63 > > > > $ usbhidctl -f /dev/ujoy/0 -l > > Game_Pad.X=126 > > Game_Pad.Y=126 > > Game_Pad.Z=125 > > Game_Pad.Rz=129 > > Game_Pad.Hat_Switch=8 > > Game_Pad.Button_1=0 > > Game_Pad.Button_2=0 > > Game_Pad.Button_3=0 > > Game_Pad.Button_4=0 > > Game_Pad.Button_5=0 > > Game_Pad.Button_6=0 > > Game_Pad.Button_7=0 > > Game_Pad.Button_8=0 > > Game_Pad.Button_9=0 > > Game_Pad.Button_10=0 > > Game_Pad.Button_11=0 > > Game_Pad.Button_12=0 > > Game_Pad.Button_13=0 > > Game_Pad.Button_14=0 > > Game_Pad.0x0020=28 > > Game_Pad.Rx=0 > > Game_Pad.Ry=0 > > [...] > > > > If others want to regression test this diff, and somebody wants to OK > > it. Maybe I should send it out on a separate thread. > > > > Once this has been fixed I would continue to do some more ujoy(4) > > testing, and check about a possible import. We also should agree on > > who/how to fix the ports part. > > > > > > 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); >
