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

[Kai Krakow]

Hey Zeb!

Reverting also "winebus.sys: Translate SDL controller axes to unsigned
32-bit values" gets rid of the problem. I think this is what Aric did:
He reverted both patches (I only realized lately that this patch was
made from two commits).

So the problem seems to be in the first commit.

Looking at the code, it probably works the same for X and Y axis. But
still the problem only shows for the X axis. I don't quite understand
the code and how BYTE_REPORT_TAIL[] works but:

1. Isn't the switch statement missing a default case? Previously the
function did set any index. Now it doesn't.

2. Is it possible that bytes are overwritten? Y usually becomes
written after X and could overwrite some index of X.
(sidenote: I was seeing some random jumps while moving the sticks more
or less random but I couldn't reproduce it later)

3. Could LE_WORD(value)+32768 have some problem with the signed-bit?
(but then it should affect both X and Y)

4. Is it safe to add 32768 to LE_WORD() (which is probably 16-bit),
plus "value" is of type short
(but I'm always confused by C types and its sizes/signs, it's probably
signed 16-bit on Linux64)

Thanks,
Kai

Am Mo., 11. Feb. 2019 um 02:16 Uhr schrieb Kai Krakow :
> Hey Zeb!
>
> Am So., 10. Feb. 2019 um 22:57 Uhr schrieb Zebediah Figura
> :
> > Is the controller being reported through udev, then?
>
> Not exactly sure, what you mean... In Steam, I disabled controller
> configuration support for this controller because it messes with the
> axis, converting them to mouse movement in some games. So it probably
> doesn't go through the Steam uinput implementation.
>
> "udevadm monitor" shows the controller when turning it on:
>
> KERNEL[20453.657006] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71
> (bluetooth)
> UDEV  [20453.679748] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71
> (bluetooth)
> KERNEL[20453.756728] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> (hid)
> KERNEL[20453.757194] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19
> (input)
> KERNEL[20453.757408] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/js0
> (input)
> KERNEL[20453.757459] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/event17
> (input)
> KERNEL[20453.757491] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/hidraw/hidraw10
> (hidraw)
> KERNEL[20454.753078] bind
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> (hid)
> KERNEL[20454.770809] change
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> (power_supply)
> UDEV  [20454.778571] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> (hid)
> UDEV  [20454.782705] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19
> (input)
> UDEV  [20454.783105] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/hidraw/hidraw10
> (hidraw)
> UDEV  [20454.792791] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/js0
> (input)
> UDEV  [20455.001865] add
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/event17
> (input)
> UDEV  [20455.004553] bind
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> (hid)
> UDEV  [20455.005809] change
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> (power_supply)
> KERNEL[20455.739024] change
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> (power_supply)
> UDEV  [20455.740310] change
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> (power_supply)
>
> "xpadneo" uses a trick to work around libSDL wrongly remapping its
> buttons and controllers by faking a different interface serial/version
> number. This is needed because "xpadneo" actually exposes compatible
> button and axis mapping while the current firmware of the controller
> puts the controller into "Android" mode when paired via BT in Linux.
> While both Steam and SDL distribute a SDL_Mapping which works around
> this, it still doesn't work for games which access /dev/input/js0
> directly. But the fix implemented by "xpadneo" does work correctly
> here but now has to sidestep "broken" SDL and Steam behavior. But
> that's all it does, it shouldn't break the axis values itself.
> Actually, this fake interface serial has been an idea developed by the
> author of xpadneo and me. OTOH, this means libSDL or Steam do not do
> any remapping with this driver.
> > Does the attached patch improve anything?
>
> Thanks, I'll try...
>
> [some minutes later]
>
> No, it's still the same.
>
> Native Linux games running through Steam: Works just right, axis work
> as usual. "evtest" shows no strange jumps in values.
>
> Windows games running through SteamPlay: Axis are broken.
>
> The following can be observed:
>
> Vertical axis still work normally.
>
> Horizontal axis work normally to the left but will flip over to left
> when pushed further than maybe half-way to the right. According to
> "evtest" that would be somewhere when we cross 24000-26000 (on a scale
> -32768..32767)...
>
> This is really strange, it tells me that there's an underlying problem
> somewhere else. Shouldn't all axis be affected? Maybe there's a buffer
> overflow or overwrite somewhere?
>
> Reverting "hid: Don't sign-extend 16-bit values" does not fix the
> problem as far as I could find out (I don't know why it did for Aric).
> This is another clue that the problem was introduced by another
> commit. Sorry for the confusion at this point.
>
> I'm using Proton rebased to wine-4.1, so it includes all the Proton patches.
>
> Thanks,
> Kai
> > On 2/10/19 1:10 PM, Kai Krakow wrote:
> > > Hello again!
> > >
> > > I'm not sure if this commit is the problem, with it reverted I still
> > > see strange effects: Moving the left stick only half-way right still
> > > works correctly, moving it further turns it mirrored into left. Have
> > > there been any other changes related to this? It worked fine in 4.0,
> > > and the only other related change I could find here in my system was
> > > updating from libSDL2-2.0.8 to 2.0.9 but I AFAIR this version still
> > > worked with 4.0 correctly.
> > >
> > > Thanks,
> > > Kai
> > >
> > > Am So., 10. Feb. 2019 um 19:02 Uhr schrieb Kai Krakow :
> > > > Hello!
> > > >
> > > > This change seems to introduce odd behavior in at least some games:
> > > > Gamepad axis are mirrored to one side only now. This happens in Shadow
> > > > of War (found by me) and Rocket League (reported by a user). I expect
> > > > other games to fail in a similar way but didn't test those yet.
> > > >
> > > > I guess the clue is in "_may_ report a logical range"...
> > > >
> > > > I'm using the xpadneo driver with an Xbox One S wireless controller,
> > > > and it reports -32768..32767 according to the driver (which follows
> > > > the Linux spec on joysticks for this). This used to work perfectly
> > > > before this change.
> > > >
> > > > Regards,
> > > > Kai
> > > >
> > > > Am Mi., 6. Feb. 2019 um 19:42 Uhr schrieb Aric Stewart :
> > > > > Signed-off-by: Aric Stewart
> > > > >
> > > > > On 2/5/19 1:09 PM, Zebediah Figura wrote:
> > > > > > From: Zebediah Figura
> > > > > >
> > > > > > Some controllers (including, with the previous patch, any reported
> > > > > > through SDL) may report a logical range of [0,65535], which takes up
> > > > > > 16 bits but should not be sign-extended.
> > > > > > Signed-off-by: Zebediah Figura
> > > > > >
> > > > > > dlls/hid/hidp.c | 4 ----
> > > > > >    1 file changed, 4 deletions(-)
> > > > > >
> > > > > > diff --git a/dlls/hid/hidp.c b/dlls/hid/hidp.c
> > > > > > index f9978038e3..15d827edf1 100644
> > > > > > --- a/dlls/hid/hidp.c
> > > > > > +++ b/dlls/hid/hidp.c
> > > > > > @@ -277,8 +277,6 @@ NTSTATUS WINAPI HidP_GetScaledUsageValue(HIDP_REPORT_TYPE ReportType, USAGE Usag
> > > > > >                                 element->valueStartBit, element->bitCount, &rawValue);
> > > > > >            if (rc != HIDP_STATUS_SUCCESS)
> > > > > >                return rc;
> > > > > > -
> > > > > >    if (element->caps.value.BitSize == 16)
> > > > > > -
> > > > > >        rawValue = (short)rawValue;
> > > > > >      *UsageValue = rawValue;
> > > > > >  }
> > > > > >
> > > > > > @@ -925,8 +923,6 @@ NTSTATUS WINAPI HidP_GetData(HIDP_REPORT_TYPE ReportType, HIDP_DATA *DataList, U
> > > > > >                                         element->valueStartBit, element->bitCount, &v);
> > > > > >                    if (rc != HIDP_STATUS_SUCCESS)
> > > > > >                        return rc;
> > > > > > -
> > > > > >            if (element->caps.value.BitSize == 16)
> > > > > > -
> > > > > >                v = (short)v;
> > > > > >              DataList[uCount].DataIndex = element->caps.value.u.NotRange.DataIndex;
> > > > > >              DataList[uCount].u.RawValue = v;
> > > > > >          }