Re: Suspected bug in hid-microsoft.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Adding back the linux-input ML]

On Wed, Feb 7, 2024 at 6:34 AM Taco Jerkface <tacodog311@xxxxxxxxx> wrote:
>
> On Tue, Feb 6, 2024 at 8:11 AM Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxxx> wrote:
> >
> > On Tue, Feb 6, 2024 at 6:25 AM Taco Jerkface <tacodog311@xxxxxxxxx> wrote:
> > >
> > > Thanks!
> > >
> > > > 1. (easiest) tune your udev rule to also give user access to the
> > > > hidraw subsystem on this device. SDL should be able to read it
> > > > directly, and handle it properly, but this won't solve for future
> > > > users
> > >
> > > This udev rule does in fact "fix" the problem
> > >
> > > KERNELS=="*:045E:0B22.*", MODE="0666"
> >
> > Right :)
> >
> > Well you could restrict it further by using the recommendations from SDL:
> > https://github.com/libsdl-org/SDL/blob/main/src/hidapi/udev/69-hid.rules
> >
> > # HIDAPI/hidraw
> > KERNEL=="hidraw*", ATTRS{idVendor}=="045E", ATTRS{idProduct}=="0B22",
> > TAG+="uaccess"
> >
> > (not entirely sure about the capitals in vendor/product)
>
> I tried that and couldn't get it to work.  My (incomplete)
> understanding is that the rule needs to match the fields from "udevadm
> info --name=/dev/hidraw1 --attribute-walk" which looks like this.  The
> "KERNELS" line was the best I could find with both the vendor and
> product fields.  I wildcarded the 0005 because it changed when I
> reconnected.

Yeah, so I vaguely remember that some properties are inherited from
the parents, and VendorID and ProductID might be inherited. And given
that this is a BLE device and that it's then using uhid, those
properties are not set.

TL;DR: seems like you got the correct match. You should probably use
TAG+="uaccess" instead of MODE="0666" to restrict the opening of the
device from the current user of the current session only.

>
>   looking at device
> '/devices/virtual/misc/uhid/0005:045E:0B22.0005/hidraw/hidraw1':
>    KERNEL=="hidraw1"
>    SUBSYSTEM=="hidraw"
>    DRIVER==""
>    ATTR{power/control}=="auto"
>    ATTR{power/runtime_active_time}=="0"
>    ATTR{power/runtime_status}=="unsupported"
>    ATTR{power/runtime_suspended_time}=="0"
>
>  looking at parent device '/devices/virtual/misc/uhid/0005:045E:0B22.0005':
>    KERNELS=="0005:045E:0B22.0005"
>    SUBSYSTEMS=="hid"
>    DRIVERS=="microsoft"
>    ATTRS{country}=="00"
>    ATTRS{power/control}=="auto"
>    ATTRS{power/runtime_active_time}=="0"
>    ATTRS{power/runtime_status}=="unsupported"
>    ATTRS{power/runtime_suspended_time}=="0"
>
>  looking at parent device '/devices/virtual/misc/uhid':
>    KERNELS=="uhid"
>    SUBSYSTEMS=="misc"
>    DRIVERS==""
>    ATTRS{power/control}=="auto"
>    ATTRS{power/runtime_active_time}=="0"
>
> >
> >
> > >
> > >
> > > > 2. (no kernel compilation required) we can try to fix the report
> > > > descriptor of the device through HID-BPF. Assuming you have
> > > > CONFIG_HID_BPF enabled in your kernel, we can relatively easily change
> > > > the way the device is exported/handled by the kernel, to make it
> > > > useful hopefully
> > > > 3. (hardest IMO as you'll have to recompile your kernel for the tests)
> > > > we can try to tune hid-microsoft.c to properly export these buttons
> > > >
> > > > For 2 and 3, I'll need some events from your device with hid-recorder.
> > > > You only gave me the report descriptor, but no events which are hard
> > > > to deduce based on the long report descriptor.
> > >
> > > With the udev rule, I attached the hid-recorder output when pressing
> > > the paddles.  I see the 01 02 04 08 showing up at the end of the line.
> >
> > Thanks for that. And as you can see, they are using the "Assign
> > Selection" usage, which is... new to me :)
> >
> > >
> > >
> > > > Also for 2 and 3 we need to have a BTN_* button code to use, and I
> > > > don't know which ones should be used from the top of my head. HID-BPF
> > > > would be easiest to use as we can let the user decide of it, while
> > > > we'll need to have a more formal usage in case we fix hid-microsoft.
> > >
> > > When connected with USB, evemu-record shows button names
> > > "BTN_TRIGGER_HAPPY5" "BTN_TRIGGER_HAPPY6" "BTN_TRIGGER_HAPPY7"
> > > "BTN_TRIGGER_HAPPY8".  I assume these are the button codes we can use?
> > >
> > > ################################
> > > #      Waiting for events      #
> > > ################################
> > > E: 0.000001 0001 02c4 0001      # EV_KEY / BTN_TRIGGER_HAPPY5   1
> > > E: 0.000001 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +0ms
> > > E: 0.143950 0001 02c4 0000      # EV_KEY / BTN_TRIGGER_HAPPY5   0
> > > E: 0.143950 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +143ms
> > > E: 6.543984 0001 02c5 0001      # EV_KEY / BTN_TRIGGER_HAPPY6   1
> > > E: 6.543984 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +6400ms
> > > E: 6.615981 0001 02c5 0000      # EV_KEY / BTN_TRIGGER_HAPPY6   0
> > > E: 6.615981 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +72ms
> > > E: 7.520034 0001 02c6 0001      # EV_KEY / BTN_TRIGGER_HAPPY7   1
> > > E: 7.520034 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +905ms
> > > E: 7.648127 0001 02c6 0000      # EV_KEY / BTN_TRIGGER_HAPPY7   0
> > > E: 7.648127 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +128ms
> > > E: 8.344035 0001 02c7 0001      # EV_KEY / BTN_TRIGGER_HAPPY8   1
> > > E: 8.344035 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +696ms
> > > E: 8.480049 0001 02c7 0000      # EV_KEY / BTN_TRIGGER_HAPPY8   0
> > > E: 8.480049 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +136ms
> > >
> >
> > Good point. Then we can easily tweak the device to use those BTN_* instead
> >
> > >
> > > >
> > > > For 1, maybe SDL (or Steam) already ships some udev rules, and
> > > > submitting a fix there would make things working for everybody.
> > > >
> > > > Anyway, depending on how much you want this to be fixed and what you
> > > > can do (is CONFIG_HID_BPF enabled in your distro? and can you
> > > > recompile a kernel module?) we can figure out the next step.
> > >
> > > Yes HID-BPF is configured in my kernel:
> > >
> > > cat /proc/config.gz | gunzip
> > > #
> > > # HID-BPF support
> > > #
> > > CONFIG_HID_BPF=y
> > > # end of HID-BPF support
> >
> > Even better, it'll be way easier for the tests :)
> >
> > I've opened a new MR with your device fix:
> > https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/32
> >
> > You should be able to head to the last job in latest pipeline (in the
> > pipeline tab) and download the artifact:
> > https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/jobs/54687597/artifacts/browse
> >
> > Then unpack and run `sudo ./install --verbose`.
> >
> > Then reconnect the device over Bluetooth, and see in evemu-record that
> > the paddles are now reporting BTN_TRIGGER_HAPPY[5-8].
> >
>
> Thanks.  This does indeed work (with my udev rule still active).

\o/

>
> > I'd then be curious about 2 things:
> > - are the paddle now reporting the same events over USB and Bluetooth?
> > (is there no inversions between the BTN_TRIGGER*?, and are all the
> > buttons correctly handled?)
>
> The button reports are nearly identical now, though Bluetooth outputs
> more information.

\o/

>
> This is paddle1, paddl2, paddle3, paddle4 pressed in that order each way.
>
> USB:
> E: 0.000001 0001 02c4 0001      # EV_KEY / BTN_TRIGGER_HAPPY5   1
> E: 0.000001 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +0ms
> E: 0.080031 0001 02c4 0000      # EV_KEY / BTN_TRIGGER_HAPPY5   0
> E: 0.080031 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +80ms
> E: 1.520025 0001 02c5 0001      # EV_KEY / BTN_TRIGGER_HAPPY6   1
> E: 1.520025 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +1440ms
> E: 1.631883 0001 02c5 0000      # EV_KEY / BTN_TRIGGER_HAPPY6   0
> E: 1.631883 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +111ms
> E: 2.183887 0001 02c6 0001      # EV_KEY / BTN_TRIGGER_HAPPY7   1
> E: 2.183887 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +552ms
> E: 2.295985 0001 02c6 0000      # EV_KEY / BTN_TRIGGER_HAPPY7   0
> E: 2.295985 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +112ms
> E: 2.800031 0001 02c7 0001      # EV_KEY / BTN_TRIGGER_HAPPY8   1
> E: 2.800031 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +505ms
> E: 2.879964 0001 02c7 0000      # EV_KEY / BTN_TRIGGER_HAPPY8   0
> E: 2.879964 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +79ms
>
> Bluetooth:
> E: 0.000001 0004 0004 589845    # EV_MSC / MSC_SCAN             589845
> E: 0.000001 0001 02c4 0001      # EV_KEY / BTN_TRIGGER_HAPPY5   1
> E: 0.000001 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +0ms
> E: 0.033496 0004 0004 589845    # EV_MSC / MSC_SCAN             589845
> E: 0.033496 0001 02c4 0000      # EV_KEY / BTN_TRIGGER_HAPPY5   0
> E: 0.033496 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +33ms
> E: 1.726818 0004 0004 589846    # EV_MSC / MSC_SCAN             589846
> E: 1.726818 0001 02c5 0001      # EV_KEY / BTN_TRIGGER_HAPPY6   1
> E: 1.726818 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +1693ms
> E: 1.843421 0004 0004 589846    # EV_MSC / MSC_SCAN             589846
> E: 1.843421 0001 02c5 0000      # EV_KEY / BTN_TRIGGER_HAPPY6   0
> E: 1.843421 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +117ms
> E: 2.736883 0004 0004 589847    # EV_MSC / MSC_SCAN             589847
> E: 2.736883 0001 02c6 0001      # EV_KEY / BTN_TRIGGER_HAPPY7   1
> E: 2.736883 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +893ms
> E: 2.826849 0004 0004 589847    # EV_MSC / MSC_SCAN             589847
> E: 2.826849 0001 02c6 0000      # EV_KEY / BTN_TRIGGER_HAPPY7   0
> E: 2.826849 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +90ms
> E: 3.300247 0004 0004 589848    # EV_MSC / MSC_SCAN             589848
> E: 3.300247 0001 02c7 0001      # EV_KEY / BTN_TRIGGER_HAPPY8   1
> E: 3.300247 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +474ms
> E: 3.373377 0004 0004 589848    # EV_MSC / MSC_SCAN             589848
> E: 3.373377 0001 02c7 0000      # EV_KEY / BTN_TRIGGER_HAPPY8   0
> E: 3.373377 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +73ms
>
> Also via USB, evemu recognizes the controller as "Microsoft X-Box One
> Elite 2 pad".  With Bluetooth it's seen as "Xbox Wireless Controller".
> This happens with or without your merge request.

This is expected and also kind of normal. We are not connecting
through the same transport, so one should at least have the "wireless"
in it. Rest is just how the device itself has those strings stored, so
we should probably just say it's fine.

>
> USB
>
> > evemu-record
> Available devices:
> /dev/input/event17:     Microsoft X-Box One Elite 2 pad
>
> Bluetooth
>
> > evemu-record
> Available devices:
> /dev/input/event16:     Xbox Wireless Controller
>
>
> > - does this mess up with SDL over hidraw (so keeping your udev rule in place)
>
> SDL seems to recognize the controller as 'Xbox One Elite 2 Controller'
> over both USB and Bluetooth.  Though the ProductID is "000b" with USB
> and "220b" via bluetooth.  Also, SDL, recognized the "hat" via USB,
> but sees them as 4 more buttons through BT.  I'm using
> Grumbel/sdl-jstest.git to test.  This happens with or without your MR.

Is this hat difference an issue? If so we can try to fix it, again,
and if not we can then leave it as it is.

>
> USB:
> Joystick Name:     'Xbox One Elite 2 Controller'
> Joystick GUID:     030010f85e040000000b000011050000
> Joystick Number:    0
> Number of Axes:     6
> Number of Buttons: 15
> Number of Hats:     1
> Number of Balls:    0
> GameControllerConfig:
>  Name:    'Xbox One Elite 2 Controller'
>  Mapping: '030010f85e040000000b000011050000,Xbox One Elite 2
> Controller,a:b0,b:b1,x:b2,y:b3,back:b6,guide:b8,start:b7,leftstick:b9,rightstick:b10,leftshoulder:b4,rightshould
> er:b5,dpup:h0.1,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,paddle1:b11,paddle2:b13,paddle3:b12,paddle4:b14,leftx:a0,lefty:a1,rightx:a3,righty:a4,lefttrigger:a2,righttrigger:a5,crc:
> f810,platform:Linux'
>
> Bluetooth:
> Joystick Name:     'Xbox One Elite 2 Controller'
> Joystick GUID:     030018dc5e040000220b000000006800
> Joystick Number:    0
> Number of Axes:     6
> Number of Buttons: 19
> Number of Hats:     0
> Number of Balls:    0
> GameControllerConfig:
>  Name:    'Xbox One Elite 2 Controller'
>  Mapping: '030018dc5e040000220b000000006800,*,a:b0,b:b1,back:b4,dpdown:b12,dpleft:b13,dpright:b14,dpup:b11,guide:b5,leftshoulder:b9,leftstick:b7,lefttrigger:a4,leftx:a0,left
> y:a1,rightshoulder:b10,rightstick:b8,righttrigger:a5,rightx:a2,righty:a3,start:b6,x:b2,y:b3,paddle1:b15,paddle2:b17,paddle3:b16,paddle4:b18,crc:dc18,platform:Linux'
>
>
> Otherwise, no issues, Steam recognizes all buttons including paddles
> correctly (with or without your MR).  But it also names the controller
> "Xbox One Elite 2 Controller" via USB and "Xbox One Controller" when
> using Bluetooth.  This doesn't seem to have any effect on registering
> the paddles correctly though.

\o/

>
>
> >
> > >
> > > I've never recompiled a module before, but I've compiled lots of other
> > > stuff before.  How hard can it be?
> > >
> >
> > Technically it's not hard, I'm sure there is a proper Arch howto
> > available. But it's painful because you'll have to maintain &
> > recompile your kernel until your fix gets in the standard kernel of
> > your distribution (archlinux if I'm not wrong).
> >
> > But the good point is that with HID-BPF that pain is relieved:  I can
> > provide you with the binary, you can check it if you like (the loader
> > is in rust and the bpf itself is C), and then once installed you can
> > forget about it.
>
> Let's try it!

Well the more I think of it, the more I think we'll stick to the current state:
- SDL is actually fixed by the udev rule (they just prefer talk over hidraw)
- for evemu users, we have a bpf filter that works. I'll eventually
merge those in the kernel so they are automatically shipped.

So I don't really see the point of fixing a driver for the sake of
fixing it and just losing both of our time.

Thanks a lot for the quick testing.

Cheers,
Benjamin






[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux