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

Zebediah Figura z.figura12 at gmail.com
Sun Feb 10 20:26:43 CST 2019 

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.

The symptoms you describe imply that anything above 0x6000 (?) is 
somehow wrapping around to negative, though that doesn't necessarily 
make very much sense. Can you provide a log with 
+hid,+hidp,+hid_report,+rawinput?

On 2/10/19 7:16 PM, Kai Krakow wrote:
> Hey Zeb!
> 
> Am So., 10. Feb. 2019 um 22:57 Uhr schrieb Zebediah Figura
> <z.figura12 at gmail.com>:
>>
>> 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 <kai at kaishome.de>:
>>>>
>>>> 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 <aric at codeweavers.com>:
>>>>>
>>>>> Signed-off-by: Aric Stewart <aric at codeweavers.com>
>>>>>
>>>>> On 2/5/19 1:09 PM, Zebediah Figura wrote:
>>>>>> From: Zebediah Figura <zfigura at codeweavers.com>
>>>>>>
>>>>>> 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 <zfigura at codeweavers.com>
>>>>>> ---
>>>>>>     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;
>>>>>>                 }
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
> 
>

More information about the wine-devel mailing list