Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set ff_effects_max correctly by subtracting FF_EFFECT_MIN. #198

Merged
merged 6 commits into from
Dec 14, 2023

Conversation

human9
Copy link
Contributor

@human9 human9 commented Oct 10, 2023

Hi, I believe there is a bug as far as setting ff_effects_max when creating ff-capable uinput devices.

Opening a controller device with fftest, I am told.. Number of simultaneous effects: 16. With python-evdev I create a uinput device that copies the exact capabilities of this device. For this device, fftest reports.. Number of simultaneous effects: 96. Off by 80.

In the kernel input.h, I can see FF_EFFECT_MIN and FF_RUMBLE is defined as 0x50 (80), and represents the offset to calculate the actual number of effects. So FF_EFFECT_MIN should be subtracted from FF_MAX_EFFECTS to set the value expected in ff_effects_max.

This pull request makes the described change in python-evdev's uinput.c. With the change, a copied device will report the correct number of simultaneous effects.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 3, 2023

Are "effects" something like rumbling controllers and the likes? Never heard of it to be honest, never worked with that

@human9
Copy link
Contributor Author

human9 commented Dec 3, 2023

Yes exactly, effects are specific types of force-feedback, take a look here for more info. To get a controller to rumble you have to "upload" effects to it, which python-evdev has the code for. You can only have so many effects uploaded simultaneously to a device. But there's a problem that means the virtual devices created by python-evdev report that they support a larger number of effects than the actual device they're based on, which this should fix.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 4, 2023

Thanks

For future reference, here is a docstring on ff_effects_max: https://github.com/torvalds/linux/blob/33cc938e65a98f1d29d0a18403dbbee050dcad9a/include/uapi/linux/uinput.h#L86

I can confirm that the "Number of simultaneous effects" is correct with this change.

original gamepad:

➜  ~ fftest /dev/input/event20
Force feedback test program.
HOLD FIRMLY YOUR WHEEL OR JOYSTICK TO PREVENT DAMAGES

Device /dev/input/event20 opened
Features:
  * Absolute axes: X, Y, Z, RX, RY, RZ, Hat 0 X, Hat 0 Y, 
    [3F 00 03 00 00 00 00 00 ]
  * Relative axes: 
    [00 00 ]
  * Force feedback effects types: Periodic, Rumble, Gain, 
    Force feedback periodic effects: Square, Triangle, Sine, 
    [00 00 00 00 00 00 00 00 00 00 03 07 01 00 00 00 ]
  * Number of simultaneous effects: 16

Setting master gain to 75% ... OK
Uploading effect #0 (Periodic sinusoidal) ... OK (id 0)
Uploading effect #1 (Constant) ... Error: Invalid argument
Uploading effect #2 (Spring) ... Error: Invalid argument
Uploading effect #3 (Damper) ... Error: Invalid argument
Uploading effect #4 (Strong rumble, with heavy motor) ... OK (id 1)
Uploading effect #5 (Weak rumble, with light motor) ... OK (id 2)
Enter effect number, -1 to exit

uinput:

➜  ~ fftest /dev/input/event21
Force feedback test program.
HOLD FIRMLY YOUR WHEEL OR JOYSTICK TO PREVENT DAMAGES

Device /dev/input/event21 opened
Features:
  * Absolute axes: X, Y, Z, RX, RY, RZ, Hat 0 X, Hat 0 Y, 
    [3F 00 03 00 00 00 00 00 ]
  * Relative axes: 
    [00 00 ]
  * Force feedback effects types: 
    Force feedback periodic effects: 
    [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ]
  * Number of simultaneous effects: 16

Uploading effect #0 (Periodic sinusoidal) ... Error:: Invalid argument
Uploading effect #1 (Constant) ... Error: Invalid argument
Uploading effect #2 (Spring) ... Error: Invalid argument
Uploading effect #3 (Damper) ... Error: Invalid argument
Uploading effect #4 (Strong rumble, with heavy motor) ... Error: Invalid argument
Uploading effect #5 (Weak rumble, with light motor) ... Error: Invalid argument
Enter effect number, -1 to exit

Why does it say "Invalid argument" for 0, 4 and 5, is this a separate problem?

This is what I used to generate the uinput:

import evdev
import time

input_device = evdev.InputDevice("/dev/input/event20")
uinput = evdev.UInput.from_device(input_device)
time.sleep(9999)

@human9
Copy link
Contributor Author

human9 commented Dec 4, 2023

Glad you could reproduce the fix!

As far as the "Invalid argument" errors go, this is (mainly)? part of a separate issue. Few things regarding that:

  1. Uinput.from_device specifically filters out EV_FF capabilities here, so you have to pass filtered_types=[e.EV_SYN] explicitly. This fixes the "invalid argument" problem.
  2. If you actually try and test the rumble with the above modification, it will most likely freeze the program. You have to manually handle EV_UINPUT events, as discussed in the tutorial docs here.
  3. Even if you use that exact code from the tutorial, you still won't have a functional rumble. The reason for this is documented in Forwarding EV_UINPUT events for FF is difficult due to undocumented behaviour #199. I've just posted a code snippet over there you can use to test that should work. It may be worth discussing over there though, as I'd consider that a separate issue.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 5, 2023

Thanks, that worked, looks good to me

I hope you don't mind further questions. Someone has to review this, and I seriously never looked into that part of evdev despite being a maintainer here:

Why is there both FF_EFFECT_MAX and FF_MAX_EFFECTS in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h?

I'll merge it afterwards

@human9
Copy link
Contributor Author

human9 commented Dec 6, 2023

I certainly don't mind. It's a great idea in fact because that question has made me notice my reasoning is wrong!

FF_EFFECT_MIN and FF_EFFECT_MAX are about the available effect types, rather than the number of effects that can be uploaded. So actually the FF_MAX_EFFECTS - FF_EFFECT_MIN thing just works out of luck, because they aren't even talking about the same thing... They're also hard coded definitions so we'll always get 16. I think it just happens all the devices we've tested support 16 effects so it keeps working.

FF_MAX_EFFECTS is an upper bound on the number of effects, rather than for a particular device. We can see it how it's used here in the kernel. When creating a uinput device, python-evdev currently sets the maximum number of effects to FF_MAX_EFFECTS (here and here), which is probably fine as a default, but we need to be able to modify it.

There's a special way to query the number of effects a device really supports, and python-evdev actually did it all along! It's right in device.py, ff_effects_count. This is the actual maximum number of effects we want to have for our uinput device (so long as we're basing it on a real device), not FF_MAX_EFFECTS.

So, I would propose I change this pull request (or make a new one) to do the following:

  1. Add a parameter to the UInput class in uinput.py, max_effects, that will default to FF_MAX_EFFECTS.
  2. Set max_effects based on the target device's ff_effects_count when from_device is used.

Then we need to pass max_effects through to the C code. That'll involve:

  1. Passing max_effects to the _uinput.setup function here
  2. That comes out as uinput_setup in uinput.c in which ff_effects_max will need be set based on the new max_effects parameter.

Sorry, that's a lot of text. Let me know if that all sounds reasonable!

John Salamon and others added 2 commits December 6, 2023 15:51
The default value will be set to FF_MAX_EFFECTS, so if forwarding
events to a real device, pass max_effects=device.ff_effects_count
Adjusts PyArg_ParseTuple as required.
See definition of ff_effects_max as u32 in include/uapi/linux/uinput.h
@human9
Copy link
Contributor Author

human9 commented Dec 6, 2023

During actually doing what I said I noticed that from_device can take multiple devices, therefore I can't implement point 2 above (unless I add a special case for len(devices) == 1). But the rest is implemented.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 8, 2023

So when I upload effects into the uinput, they end up in the controller even without using from_device.

What happens if I have multiple gamepads connected? I only have one here to try it out

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 8, 2023

If the upload just goes into an arbitrary device, or all of them, why is the maximum number of simultaneous effects of the actual gamepad related to what is happening with the uinput? If evdev is making an arbitrary wild guess, then copying ff_effects_max of the gamepad to the uinput seems like an arbitrary wild guess as well.

@human9
Copy link
Contributor Author

human9 commented Dec 8, 2023

What happens if I have multiple gamepads connected?

Where the uploaded effects go is determined by what the user does during handling of the EV_UINPUT event types. If there are two gamepads a and b connected, a user could decide they only want to upload ff effects to a. In this case they should setup the uinput device with max_effects=a.ff_effects_count. If for some reason they wanted to upload effects to both at the same time they could take the minimum ff_effects_count of both to make sure both are supported.

If the upload just goes into an arbitrary device, or all of them, why is the maximum number of simultaneous effects of the actual gamepad related to what is happening with the uinput?

You're right that it's not necessarily related, but if you intend to forward effects uploaded to the uinput device to a real gamepad, the uinput device cannot report supporting more effects than the target gamepad. You do have to set it to something, which is currently FF_MAX_EFFECTS. I think if you use the from_device function, the ff_effects_count of that device would be a better guess, but still a guess. I haven't implemented that.

The best approach is to have the maximum simultaneous effects be a manually configurable parameter. That's what this pull request does implement now.

Now you can do this:

uinput = evdev.UInput.from_device(input_device, max_effects=number)

Or this:

uinput = evdev.UInput(capabilities, max_effects=number)

If you omit max_effects the default is FF_MAX_EFFECTS, same as it was before.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 9, 2023

a user could decide they only want to upload ff effects to a. In this case they should setup the uinput device with max_effects=a.ff_effects_count

This reads like ff_effects_count is treated like an id. Meaning that the uinput only uploads effects to the device with a matching ff_effects_count.

However, when I set a different max_effects number for my uinput, then effects are still uploaded to the controller. It's just that only a limited number of effects are available.

uinput = UInput({
    ecodes.EV_FF: [80, 81, 88, 89, 90, 96],
    ecodes.EV_KEY: [ecodes.KEY_A, ecodes.KEY_B]
}, max_effects=1)

Makes fftest go

Setting master gain to 75% ... OK
Uploading effect #0 (Periodic sinusoidal) ... OK (id 0)
Uploading effect #1 (Constant) ... Error: Invalid argument
Uploading effect #2 (Spring) ... Error: Invalid argument
Uploading effect #3 (Damper) ... Error: Invalid argument
Uploading effect #4 (Strong rumble, with heavy motor) ... Error: No space left on device
Uploading effect #5 (Weak rumble, with light motor) ... Error: No space left on device
Enter effect number, -1 to exit
0
Now Playing: Sine vibration
Enter effect number, -1 to exit

and effect #0 ends up in the gamepad, and it rumbles.

I wish I had a second gamepad to experiment with them...

@human9
Copy link
Contributor Author

human9 commented Dec 9, 2023

This reads like ff_effects_count is treated like an id. Meaning that the uinput only uploads effects to the device with a matching ff_effects_count.

Sorry, I didn't mean to make it read that way. It's definitely not an id. Handling of EV_UINPUT event types determines where effects are uploaded, and handling of EV_FF events determines what actually rumbles.

I have two controllers here, both with 16 maximum simultaneous effects, and only the one I actually forward these events to will rumble. Let me know if there's something specific you want me to test.

I say the same ff_effects_count should be used for practical reasons. As you found it'll still work if you setup with a lower max_effects, but you won't be able to upload all the effects you otherwise could. If you setup with a higher max_effects than the real device, programs might try to allocate more effects than is possible, which will probably cause a crash unless handled properly.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 10, 2023

So when I upload effects into the uinput, they end up in the controller even without using from_device.

Whoops I made a really stupid mistake there in my code. They of course end up in the device of input_device.upload_effect + input_device.write

All right, thanks.

If you setup with a higher max_effects than the real device, programs might try to allocate more effects than is possible, which will probably cause a crash unless handled properly.

It would crash with OSError: [Errno 28] No space left on device, and programs using the uinput need to read the ff_effects_count from it to limit themselves.

During actually doing what I said I noticed that from_device can take multiple devices, therefore I can't implement point 2 above (unless I add a special case for len(devices) == 1). But the rest is implemented.

I would be in favor of using the smallest number of ff_effects_count of all input_devices passed into from_device

@human9
Copy link
Contributor Author

human9 commented Dec 10, 2023

I would be in favor of using the smallest number of ff_effects_count of all input_devices passed into from_device

I also think that would make sense, have added a commit.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Dec 12, 2023

Works, thank you :)

For the records, here is the script I used to test it:

import time
import asyncio
from evdev import ff, InputDevice, UInput, categorize, ecodes

def test_takes_the_min():
    uinput_1 = UInput(max_effects=5)
    uinput_2 = UInput(max_effects=10)
    uinput_3 = UInput()
    uinput_4 = UInput.from_device(
        uinput_1.device,
        uinput_2.device,
        uinput_3.device,
    )

    print(uinput_1.device.ff_effects_count)
    print(uinput_2.device.ff_effects_count)
    print(uinput_3.device.ff_effects_count)
    print(uinput_4.device.ff_effects_count)

    print(uinput_4.device.path)

test_takes_the_min()
time.sleep(50)

And then I went into fftest to check if the "Number of simultaneous effects" matches.

@sezanzeb
Copy link
Collaborator

Ready to merge?

@human9
Copy link
Contributor Author

human9 commented Dec 12, 2023

Yep, looks good to me!

@human9
Copy link
Contributor Author

human9 commented Dec 12, 2023

Actually maybe hold off a moment, I'll just add some documentation.

@human9
Copy link
Contributor Author

human9 commented Dec 12, 2023

Okay, all documented and ready for merge!

@sezanzeb sezanzeb merged commit c688c9e into gvalkov:main Dec 14, 2023
@sezanzeb
Copy link
Collaborator

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants