[PATCH 2/2] hid: Don't sign-extend 16-bit values.

[Roderick Colenbrander]

Hi,

I haven't followed the discussion too closely, nor the exact details
of Wine's implementation. I do work a lot on Linux kernel, evdev and
HID. To me the report below is strange. In particular the logical
minimum and maximum don't look right. It should reflect what is
reported over evdev, so for your device -32768 to +32767.

The HID spec is a bit weird about signed/unsigned int for the input
reports. The interpretation depends on logical minimum / maximum. For
reference see section 5.8
https://www.usb.org/sites/default/files/documents/hid1_11.pdf.

I would think Wine's handling should follow the same guidelines. We
shouldn't do any interpretation of the data until we really consume it
I guess. I'm not too sure about how this part of Wine currently works.

Thanks,
Roderick

On Mon, Feb 11, 2019 at 2:17 PM Kai Krakow <kai at kaishome.de> wrote:
>
> Am Mo., 11. Feb. 2019 um 22:26 Uhr schrieb Kai Krakow <kai at kaishome.de>:
> >
> > Hey Zeb!
> >
> > > >> Wine can pick up controllers from udev, either via hidraw or evdev, as
> > > >> well as from SDL. As of bd9e130ee, we shouldn't be using signed offsets
> > > >> anymore when reporting SDL controllers, so I assumed that your
> > > >> controller was coming through hidraw directly.
> > > >
> > > > Not sure, does the log show this? I'm pretty sure it's going through
> > > > SDL because I can apply SDL controller mappings to it through
> > > > environment.
> > >
> > > Log should show this, yes.
> > >
> > > >> The symptoms you describe imply that anything above 0x6000 (?) is
> > > >> somehow wrapping around to negative, though that doesn't necessarily
> > > >> make very much sense.
> > > >
> > > > Indeed, I'm confused too. ;-)
> > >
> > > Well, it's close to an explanation. INSIDE, which prompted these
> > > patches, showed very similar symptoms: the game assumed that the
> > > controller had a range of 0-32767, and so mapped 0-16383 to "left" and
> > > 16384-32767 to "right", and negative values were interpreted as above
> > > 16384 and therefore also "right". Thus the effect was basically the same
> > > as you describe, except inverted.
> > >
> > > >> Can you provide a log with
> > > >> +hid,+hidp,+hid_report,+rawinput?
> > > >
> > > > Sure, I'll do. But please wait, I really need some sleep now, this was
> > > > bothering me for a few hours now. ;-)
> > > >
> > > > Just let me know: Two logs? One with patches reverted and one without?
> > >
> > > Just without any patches reverted (so with the bug visible) will
> > > hopefully be enough.
> >
> > Here's the log:
> > https://gist.github.com/a9b51a975f933482a7125792a66bf3cd
>
> This looks correct:
>
> 0022:trace:hid:debug_print_value_cap INPUT Value: 0x1/0x30: ReportId
> 0, IsAbsolute 1, HasNull 0, Bit Size 16, ReportCount 1, UnitsExp 0,
> Units 0, LogicalMin 0, Logical Max 65535, PhysicalMin 0, PhysicalMax
> 65535 -- StartBit 24/16
> 0022:trace:hid:debug_print_value_cap INPUT Value: 0x1/0x31: ReportId
> 0, IsAbsolute 1, HasNull 0, Bit Size 16, ReportCount 1, UnitsExp 0,
> Units 0, LogicalMin 0, Logical Max 65535, PhysicalMin 0, PhysicalMax
> 65535 -- StartBit 40/16
> 0022:trace:hid:debug_print_value_cap INPUT Value: 0x1/0x33: ReportId
> 0, IsAbsolute 1, HasNull 0, Bit Size 16, ReportCount 1, UnitsExp 0,
> Units 0, LogicalMin 0, Logical Max 65535, PhysicalMin 0, PhysicalMax
> 65535 -- StartBit 56/16
> 0022:trace:hid:debug_print_value_cap INPUT Value: 0x1/0x34: ReportId
> 0, IsAbsolute 1, HasNull 0, Bit Size 16, ReportCount 1, UnitsExp 0,
> Units 0, LogicalMin 0, Logical Max 65535, PhysicalMin 0, PhysicalMax
> 65535 -- StartBit 72/16
> 0022:trace:hid:debug_print_value_cap INPUT Value: 0x1/0x32: ReportId
> 0, IsAbsolute 1, HasNull 0, Bit Size 16, ReportCount 1, UnitsExp 0,
> Units 0, LogicalMin 0, Logical Max 32767, PhysicalMin 0, PhysicalMax
> 32767 -- StartBit 88/16
> 0022:trace:hid:debug_print_value_cap INPUT Value: 0x1/0x35: ReportId
> 0, IsAbsolute 1, HasNull 0, Bit Size 16, ReportCount 1, UnitsExp 0,
> Units 0, LogicalMin 0, Logical Max 32767, PhysicalMin 0, PhysicalMax
> 32767 -- StartBit 104/16
>
> Confirming with evtest:
>
>   Event type 3 (EV_ABS)
>    Event code 0 (ABS_X)
>      Value      0
>      Min   -32768
>      Max    32767
>      Flat    4095
>    Event code 1 (ABS_Y)
>      Value      0
>      Min   -32768
>      Max    32767
>      Flat    4095
>    Event code 2 (ABS_Z)
>      Value      0
>      Min        0
>      Max     1023
>      Fuzz       3
>      Flat      63
>    Event code 3 (ABS_RX)
>      Value      0
>      Min   -32768
>      Max    32767
>      Fuzz     255
>      Flat    4095
>    Event code 4 (ABS_RY)
>      Value      0
>      Min   -32768
>      Max    32767
>      Fuzz     255
>      Flat    4095
>    Event code 5 (ABS_RZ)
>      Value      0
>      Min        0
>      Max     1023
>      Fuzz       3
>      Flat      63
>
> Thanks,
> Kai
>
>