Re: [PATCH v2 2/5] winebus.sys: Build device reports for linux event devices
Hi Aric, I have had a quick look over this patch and have some initial feedback, maybe more later if time permits. For the absolute and relative axes don't hard code the minimum and maximum values for the report descriptors. These are not correct for many devices e.g. Xbox controllers use 16-bit absolute axes, while DS3/DS4 for example use 8-bit. In addition xbox controllers use signed data, while DS3/DS4 use unsigned (0-255)Also many devices use signed data. You should rely on the absinfo data returned by EVIOCGABS and base your input ranges on that. Thanks, Roderick On Mon, Feb 27, 2017 at 9:32 AM, Aric Stewart <aric(a)codeweavers.com> wrote:
v2: Updates from Sebastian Lackner
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/winebus.sys/bus_udev.c | 438 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 436 insertions(+), 2 deletions(-)
On 3/2/17 12:34 AM, Roderick Colenbrander wrote:
Hi Aric,
I have had a quick look over this patch and have some initial feedback, maybe more later if time permits.
For the absolute and relative axes don't hard code the minimum and maximum values for the report descriptors. These are not correct for many devices e.g. Xbox controllers use 16-bit absolute axes, while DS3/DS4 for example use 8-bit. In addition xbox controllers use signed data, while DS3/DS4 use unsigned (0-255)Also many devices use signed data.
You should rely on the absinfo data returned by EVIOCGABS and base your input ranges on that.
Thanks, Roderick
Hi Rodrick, Thanks for looking at these, your input is very valuable given all your experience. I am a bit swamped right at the moment so I will need a moment to circle back but any in depth review you give will not be ignored. I would love to get this part working. -aric
On Mon, Feb 27, 2017 at 9:32 AM, Aric Stewart <aric(a)codeweavers.com> wrote:
v2: Updates from Sebastian Lackner
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/winebus.sys/bus_udev.c | 438 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 436 insertions(+), 2 deletions(-)
On Thu, Mar 2, 2017 at 11:01 AM, Aric Stewart <aric(a)codeweavers.com> wrote:
On 3/2/17 12:34 AM, Roderick Colenbrander wrote:
Hi Aric,
I have had a quick look over this patch and have some initial feedback, maybe more later if time permits.
For the absolute and relative axes don't hard code the minimum and maximum values for the report descriptors. These are not correct for many devices e.g. Xbox controllers use 16-bit absolute axes, while DS3/DS4 for example use 8-bit. In addition xbox controllers use signed data, while DS3/DS4 use unsigned (0-255)Also many devices use signed data.
You should rely on the absinfo data returned by EVIOCGABS and base your input ranges on that.
Thanks, Roderick
Hi Rodrick,
Thanks for looking at these, your input is very valuable given all your experience.
I am a bit swamped right at the moment so I will need a moment to circle back but any in depth review you give will not be ignored. I would love to get this part working.
-aric
On Mon, Feb 27, 2017 at 9:32 AM, Aric Stewart <aric(a)codeweavers.com> wrote:
v2: Updates from Sebastian Lackner
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/winebus.sys/bus_udev.c | 438 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 436 insertions(+), 2 deletions(-)
Hi Aric, Had a high-level look at some other aspects. As you are aware mapping from evdev to HID is not ideal as not everything can be expressed perfectly and there is limited metadata (in particular for buttons). For sure we can't restore the original HID reports, so applications which hard coded reading of raw reports for particular devices just can't work, so those would need to use hidraw, but that's okay. The area I'm a bit worried about is the axis mapping. Many devices initially rely on the hid-core layer of the Linux kernel to do mapping of axes, buttons etcetera based on the HID reports. Many devices do report fixups or override the mapping as descriptors are often buggy and not everything can be expressed well in the descriptors. One buggy area is the mapping of the second stick. There is no good way for expressing the second stick in HID. Basically HID (and evdev) talks about X/Y/Z as being for linear displacements and RX/RY/RZ for rotational displacements (e.g. flight sticks you can rotate a bit). Over the years at least Linux started (ab?)using RX/RY/RZ for second stick and trigger. Some devices e.g. ds3/ds4 that I'm aware of and some others use Z/RZ for the second stick and ry/rz for the triggers, though for ds4 I fixed this in Linux 4.10 and ds3 will be fixed in 4.12 (though you need to detect the different mapping, we are or'ing the version with | 0x8000). It looks like the Switch Pro controller is doing the same right now (will likely be fixed when an official driver appears in the kernel). Not sure about other devices. For dinput this mapping may not be much of a concern as expressing axes is more flexible. In my opinion the popular console gamepads which do map well to xinput, should be supported in xinput as well. This is where some of the concern comes in. You would need a per device mapping table at minimum and maybe even a per platform one (as I said mapping can change between drivers for a given device). Another concern about axes mapping, though less of a concern, is there are many more axes which some drivers report just because the standard HID parser is basic and there are not enough good suitable evdev axes. Some devices will leak into other axes ranges (e.g. ABS_MISC / ABS_MT), though such axes leaking for typically analog buttons could be ignored. How do you intend to report other information which comes in through HID? A good example is battery information which depending on the device is part of the regular input reports. On Linux you can only get this data through sysfs. It is not too important yet, but there should be a plan. Similar how do you intend to support force feedback / rumble? Of course it would be output reports, but I forgot if the HID spec supports them properly, it is typically done in device specific ways (at least basic rumble is). Thanks, Roderick
participants (2)
-
Aric Stewart -
Roderick Colenbrander