Add support for sensor to joystick on Linux (evdev)

[meyraud705]

Description

This patch adds support for sensor to joystick on Linux through the evdev interface.

It stores all sensor devices into a list and try to match the sensor and the joystick device with the unique identifier (see #6478 (comment) ) when a SDL_Joystick is opened.

If a new-sensor notification from inotify, evdev callback or fallback detection happens after SDL_JoystickOpen (main device notification -> SDL_JOYDEVICEADDED event -> SDL_JoystickOpen -> sensor device notification) then the SDL_Joystick will not have sensor. It did not happen to me while testing hotplug. I am not sure how much we should wait after a notification to be sure that we get all event sources.

If the sensor is removed, SDL_JOYDEVICEREMOVED event is not sent as I assume both sensor and joy item are removed at the same time.

SDL_EVDEV_GuessDeviceClass is used to find sensor. I don't know if this alone is enough.

I assume that all devices follow the same orientation convention as the DualShock 4.

Existing Issue(s)

This partially fix #6478, you still need to allow read access to sensor device with an udev rule.

Here the udev rule for DualShock 4 Slim:

# DualShock 4 Slim over usb
SUBSYSTEM=="input", ATTRS{idVendor}=="054c", ATTRS{idProduct}=="09cc", MODE="0660", TAG+="uaccess"

# DualShock 4 Slim over bluetooth
SUBSYSTEM=="input", KERNELS=="*054C:09CC*", MODE="0660", TAG+="uaccess"

[meyraud705]

Marked as draft because I noticed I made some mistakes. I will fix them, but not right now.

[meyraud705]

I fixed and tested this with a Dualshock 4. It is ready for review and test with other controllers.

[slouken]

There is a naming convention for the Sony sensor devices. I'm not sure if it's used by all controllers in the latest kernels, but it might be worth looking at as a hint here?

[meyraud705]

I can add name from SDL_ShouldIgnoreGamepad() as an hint: " Motion Sensors", "Nintendo * IMU", " Accelerometer". There is also Wii extension control " IR", " Motion Plus", " Nunchuk", but i don't know if these are accelerometer and gyro.
If there are others names let me know and I'll add them too.

[smcv]

I would personally expect this naming convention to be less reliable than the evdev bitfields, if anything.

I'd suggest folding whatever checks for naming we might be using into SDL_EVDEV_GuessDeviceClass, or a new SDL_EVDEV_IsMotionSensor.

[smcv]

All the Wii accessories report as RX,RY,RZ according to our test data in tests/testevdev.c, even though the Wiimote and Nunchuck are really 3-axis accelerometers (which should be reported as X,Y,Z, presumably either a wrong HID descriptor or a kernel driver bug), so we might need a quirks mechanism to remap those.

The Wii Motion Plus is genuinely a 3-axis gyroscope and correctly reports RX,RY,RZ.

The Wiimote infrared aiming is unlike any other controller, and probably not very useful for anything other than a Wii emulator.

[meyraud705]

For now, Wii U accessories are ignored because they create 3 files while we read only from 1. And I don't know how to check the device type (gyro or accelerometer) other than by name because they report everything on RX, RY, RZ and SDL_EVDEV_GuessDeviceClass() returns SDL_UDEV_DEVICE_ACCELEROMETER for both.

[slouken]

I added some comments, but in general this looks good. Thanks!

[meyraud705]

I force pushed the minor requested changes on the first commit.

In the commit to improve sensor detection, I used names and code from SDL_ShouldIgnoreGamepad() so I moved SDL_endswith() from SDL_gamepad.c to SDL_utils.c.

Closing sensor file when sensors are disabled was simple enough and I think it is worth it because most games don't even enable them.

[Impeta]

Sorry if I don't provide anything as contributing here, but I would love if someone here who has a 8bitdo's SN30 pro+ (or anyone else who has a Pro Controller for that matter) to test this!

[smcv]

It would be great if you could refactor this function so it looks more like this:

static int FdIsSensor(...)
{
    struct input_id inpid;
    unsigned long evbit[NBITS(EV_MAX)] = { 0 };
    unsigned long keybit[NBITS(KEY_MAX)] = { 0 };
    unsigned long absbit[NBITS(ABS_MAX)] = { 0 };
    unsigned long relbit[NBITS(REL_MAX)] = { 0 };

   get all the information from the fd using ioctls;

    result = SDL_EVDEV_IsMotionSensor(&inpid, name, evbit, absbit, keybit, relbit);
    free anything that needs to be freed;
    return result;
}

That way, you'd be able to extend tests/testevdev.c to run SDL_EVDEV_IsMotionSensor against all of the test data in tests/testevdev.c, which would give us test coverage for a whole pile of game controllers without having to physically have them with you, and would make sure the heuristic doesn't regress in future.

[meyraud705]

That is a good idea. I will take a look at how tests/testevdev.c work and refactor this.

[meyraud705]

With #7945 there is not much heuristic left. As long as SDL_EVDEV_GuessDeviceClass() returns the correct class, detection will be fine.

[smcv]

I would personally expect this naming convention to be less reliable than the evdev bitfields, if anything.

I'd suggest folding whatever checks for naming we might be using into SDL_EVDEV_GuessDeviceClass, or a new SDL_EVDEV_IsMotionSensor.

[smcv]

All the Wii accessories report as RX,RY,RZ according to our test data in tests/testevdev.c, even though the Wiimote and Nunchuck are really 3-axis accelerometers (which should be reported as X,Y,Z, presumably either a wrong HID descriptor or a kernel driver bug), so we might need a quirks mechanism to remap those.

The Wii Motion Plus is genuinely a 3-axis gyroscope and correctly reports RX,RY,RZ.

The Wiimote infrared aiming is unlike any other controller, and probably not very useful for anything other than a Wii emulator.

[smcv]

We should maybe rename ACCELEROMETER to something like MOTION_CONTROL since it really means "accelerometer and/or gyro".

[smcv]

... or maybe we shouldn't, since it's a 1:1 representation of udev's ID_INPUT_ACCELEROMETER.

[smcv]

If we're going to apply heuristics to the EVIOCGNAME as a source of machine-readable information, then we should have a field for it in the test data in tests/testevdev.c. We can leave it as NULL for all devices for now - but I can probably populate most or even all of them from the source evemu-describe and steam-runtime-input-monitor logs.

[smcv]

Manufacturers tend to put all sorts of junk in "human-readable name" fields (see also the number of motherboards that identify themselves as "To be changed by OEM"...) so if we want to include or exclude specific devices, it would probably be a lot more reliable to use the (vendor,product) pair.

[meyraud705]

I use SDL_CreateJoystickName() and other parts of SDL use this name to check gamepad support. However if a device has no sensor it should not be added as a sensor even if it matches by name. I'll fix that.

[smcv]

Sorry if I don't provide anything as contributing here, but I would love if someone here who has a 8bitdo's SN30 pro+ (or anyone else who has a Pro Controller for that matter) to test this!

If it's possible to refactor the device identification heuristic to be more like SDL_EVDEV_GuessDeviceClass - working from pre-collected information, rather than from a "live" file descriptor - then it will be possible to do some basic testing against the pre-recorded device descriptions in tests/testevdev.c, even for people who don't own the appropriate controller. Obviously that won't be a full-stack functional test and wouldn't detect bugs like "x and y axis are swapped", but it's a lot better than nothing.

We have this information for DualShock 4 and 5 (3-axis accelerometer + 3-axis gyro), DualShock 3 (3-axis accelerometer), and the various Wii accessories (probably not practically useful for PC gaming). We don't currently have this information for any Switch-related controllers. One of my colleagues has more gamepads than I do, and reports that with current Linux kernels, Switch Joycons don't produce an evdev device node for motion controls.

[smcv]

you still need to allow read access to sensor device with an udev rule

I have tried to get the equivalent of this udev rule upstream into udev, but it is looking as though the systemd/udev maintainers might insist that accelerometers must be accessed via a new interface that has not yet been invented (either a D-Bus-based portal, or a new interface on the Wayland compositor).

[meyraud705]

I have tried to get the equivalent of this udev rule upstream into udev

I hope that SDL being able to read these devices will help push this request.

[slouken]

Merged, thanks!

[Impeta]

We have this information for DualShock 4 and 5 (3-axis accelerometer + 3-axis gyro), DualShock 3 (3-axis accelerometer), and the various Wii accessories (probably not practically useful for PC gaming). We don't currently have this information for any Switch-related controllers. One of my colleagues has more gamepads than I do, and reports that with current Linux kernels, Switch Joycons don't produce an evdev device node for motion controls.

@smcv Yes, that's really the problem. At the moment, nothing of Switch's peripherals, joycons or pro-controllers and such, furthermore 3rd party controllers that are functionally same as Switch pro-controllers, seem to spawn an request for aligning and emulating motion controls, unless using through Cemuhook or the alike. I was hoping with this merged finally, motion controls/anything sensor detectable are natively supported and works OOTB. Just to make sure we're on the same page, sorry, is this what you mean?

Merged, thanks!

@slouken Thank you! For certainty's sake, is this merged into SDL3 or will it be backported/included on current SDL2?

[slouken]

It needs more testing, but this seems reasonable to backport to SDL2.

[Impeta]

but this seems reasonable to backport to SDL2.

After testing and bugreporting as you say, please, consider so! Emulators such as CEMU, Yuzu and Ryujinx won't work at all in my end with natively detecting and supporting motion controls, neither through Switch nor DualShock4 modes from my 8bitdo's SN30+ pro controller, unless again I opt for the DSU server and use Cemuhook.
cemu-project/Cemu#753

[slouken]

They should work with hidraw support out of the box.

[theofficialgman]

They should work with hidraw support out of the box.

@slouken they do not work with HIDRAW out of the box. the same issue with needing additional UDEV rules applies to HIDRAW as it does the IMU event#.
It just happens that most of you (and most users) have steam installed which provides the HIDRAW UDEV rules for many popular controllers and SDL ends up benefitting from that.

See cemu-project/Cemu#1097 (comment) for more information.

Alternatively to use steam's UDEV rules, the PR that was previously linked should solve this as well systemd/systemd#22860 since it adds the necessary UDEV rules to BOTH the event# and hidraw# device nodes for many controllers (but not all the one ones needed, I notice already that Nintendo Switch Joy-Cons are missing and I am sure many many others are as well).