[v1] hid-playstation: DS4: Update rumble and lightbar together

Some 3rd party gamepads expect updates to rumble and lightbar together,
and setting one may cancel the other.

This patch maximises compatibility by always sending rumble and lightbar
updates whenever updates are sent to the gamepad: valid_flag0 is always
>= 0x03.

Further background reading:

- Apparently the PS4 always sends rumble and lightbar updates together:

  https://eleccelerator.com/wiki/index.php?title=DualShock_4#0x11_2

- 3rd party gamepads may not implement lightbar_blink, and may simply
  ignore updates with 0x07 set, according to:

  https://github.com/Ryochan7/DS4Windows/pull/1839

This patch leaves the lightbar blink feature as-is: Likely, most users
are unaware that it exists, hence it's unlikely that a packet with
0x07 set will even be sent in practice. Let's leave the function there,
so users of first-party gamepads can continue to use it.
---
 drivers/hid/hid-playstation.c | 55 ++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 24 deletions(-)

Hi Max,

Thanks for your patch. I'm a little hesitant to make changes here. The
console itself does the updates similar to this, though there are
different userspace kind of ways there to send things (so some paths
may kind of do what you saw). These flags are often used also to
enable other features in the output report (e.g. audio settings or
many others).

This driver is used in a lot of devices. Different vendors ranging
e.g. car manufacturers, phones, TVs,.. have their own unit /
regression tests suites to test integration. Where they send FF rumble
for example and check the bits in the output reports

We went through a lot of pain with many of such vendors (and are still
going through some). I would rather not shake things up for such a
narrow use cases for non-official devices.

Thanks,
Roderick

On Sun, Jun 16, 2024 at 9:40 AM Max Staudt <max@enpas.org> wrote:
>
> Some 3rd party gamepads expect updates to rumble and lightbar together,
> and setting one may cancel the other.
>
> This patch maximises compatibility by always sending rumble and lightbar
> updates whenever updates are sent to the gamepad: valid_flag0 is always
> >= 0x03.
>
> Further background reading:
>
> - Apparently the PS4 always sends rumble and lightbar updates together:
>
>   https://eleccelerator.com/wiki/index.php?title=DualShock_4#0x11_2
>
> - 3rd party gamepads may not implement lightbar_blink, and may simply
>   ignore updates with 0x07 set, according to:
>
>   https://github.com/Ryochan7/DS4Windows/pull/1839
>
> This patch leaves the lightbar blink feature as-is: Likely, most users
> are unaware that it exists, hence it's unlikely that a packet with
> 0x07 set will even be sent in practice. Let's leave the function there,
> so users of first-party gamepads can continue to use it.
> ---
>  drivers/hid/hid-playstation.c | 55 ++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index e7c309cfe3a0..986a4ca8b664 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -387,12 +387,10 @@ struct dualshock4 {
>         bool update_bt_poll_interval;
>         uint8_t bt_poll_interval;
>
> -       bool update_rumble;
>         uint8_t motor_left;
>         uint8_t motor_right;
>
>         /* Lightbar leds */
> -       bool update_lightbar;
>         bool update_lightbar_blink;
>         bool lightbar_enabled; /* For use by global LED control. */
>         uint8_t lightbar_red;
> @@ -2092,8 +2090,6 @@ static int dualshock4_led_set_brightness(struct led_classdev *led, enum led_brig
>                 }
>         }
>
> -       ds4->update_lightbar = true;
> -
>         spin_unlock_irqrestore(&ds4->base.lock, flags);
>
>         dualshock4_schedule_work(ds4);
> @@ -2143,26 +2139,39 @@ static void dualshock4_output_worker(struct work_struct *work)
>
>         spin_lock_irqsave(&ds4->base.lock, flags);
>
> -       if (ds4->update_rumble) {
> -               /* Select classic rumble style haptics and enable it. */
> -               common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_MOTOR;
> -               common->motor_left = ds4->motor_left;
> -               common->motor_right = ds4->motor_right;
> -               ds4->update_rumble = false;
> -       }
> +       /*
> +        * PS4 seems to send 0xf3 updates by default, according to a HCI trace:
> +        *   https://eleccelerator.com/wiki/index.php?title=DualShock_4#0x11_2
> +        *
> +        * This seems to be a very compatible value with third-party pads:
> +        *   https://github.com/Ryochan7/DS4Windows/pull/1839
> +        *
> +        * However, hid-playstation in v6.10 does not set the upper nibble,
> +        * and neither does hid-sony in v6.2, or BlueRetro. We should stick
> +        * to that for now, to minimise the chance of unexpected changes.
> +        *
> +        * So let's always update rumble and lightbar at the same time, with
> +        * the upper nibble cleared, resulting in valid_flag0 == 0x03.
> +        * Hopefully this will maximise compatibility with third-party pads.
> +        */
> +       common->valid_flag0 = DS4_OUTPUT_VALID_FLAG0_MOTOR |
> +                             DS4_OUTPUT_VALID_FLAG0_LED;
>
> -       if (ds4->update_lightbar) {
> -               common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_LED;
> -               /* Comptabile behavior with hid-sony, which used a dummy global LED to
> -                * allow enabling/disabling the lightbar. The global LED maps to
> -                * lightbar_enabled.
> -                */
> -               common->lightbar_red = ds4->lightbar_enabled ? ds4->lightbar_red : 0;
> -               common->lightbar_green = ds4->lightbar_enabled ? ds4->lightbar_green : 0;
> -               common->lightbar_blue = ds4->lightbar_enabled ? ds4->lightbar_blue : 0;
> -               ds4->update_lightbar = false;
> -       }
> +       common->motor_left = ds4->motor_left;
> +       common->motor_right = ds4->motor_right;
> +
> +       /* Compatible behavior with hid-sony, which used a dummy global LED to
> +        * allow enabling/disabling the lightbar. The global LED maps to
> +        * lightbar_enabled.
> +        */
> +       common->lightbar_red = ds4->lightbar_enabled ? ds4->lightbar_red : 0;
> +       common->lightbar_green = ds4->lightbar_enabled ? ds4->lightbar_green : 0;
> +       common->lightbar_blue = ds4->lightbar_enabled ? ds4->lightbar_blue : 0;
>
> +       /*
> +        * Output reports updating lightbar_blink will simply be ignored
> +        * by incompatible controllers, since valid_flag0 == 0x07.
> +        */
>         if (ds4->update_lightbar_blink) {
>                 common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_LED_BLINK;
>                 common->lightbar_blink_on = ds4->lightbar_blink_on;
> @@ -2459,7 +2468,6 @@ static int dualshock4_play_effect(struct input_dev *dev, void *data, struct ff_e
>                 return 0;
>
>         spin_lock_irqsave(&ds4->base.lock, flags);
> -       ds4->update_rumble = true;
>         ds4->motor_left = effect->u.rumble.strong_magnitude / 256;
>         ds4->motor_right = effect->u.rumble.weak_magnitude / 256;
>         spin_unlock_irqrestore(&ds4->base.lock, flags);
> @@ -2520,7 +2528,6 @@ static void dualshock4_set_default_lightbar_colors(struct dualshock4 *ds4)
>         ds4->lightbar_green = player_colors[player_id][1];
>         ds4->lightbar_blue = player_colors[player_id][2];
>
> -       ds4->update_lightbar = true;
>         dualshock4_schedule_work(ds4);
>  }
>
> --
> 2.39.2
>
>

Hi Roderick,

So as far as I understand, my suggested driver behaviour is sound, because both the console's own behaviour as well as other drivers show that flags == 0x03 is working perfectly fine with original controllers. Is my understanding correct?

In fact, hid-sony used to send these updates at the same time (it had flags == 0x07), so for some 3rd party controllers, the move to hid-playstation has already been a regression in the FF/lightbar department.

Do you see a way to move forward with this patch and get it merged, even if it is with some delay? Is there something that I can improve?


As for downstream users' regression tests, this argument confuses me. Could you please give me a bit of help here?

My understanding, so far, is as follows:

Tests checking the FF bit should not fail if, say, the lightbar bit is also set. If they fail, then that means that the test is too sensitive. After all, the patch does not change anything from userspace's point of view, nor from the actual human user's point of view. The DualShock 4 behaves all the same, and it's just the wire protocol that changes.

So if a downstream user wishes to do a full end-to-end integration test, technically they would need to connect a real DualShock 4 and test that. But I can see that this is not easily automatable ;) so they may test at the HID level instead. The result is that, depending on how they structure their tests, they might no longer be testing end-to-end, but rather testing an implementation and its quirks. This bears the risk that the test will fail because of a legitimate change in the driver, or elsewhere in the kernel.

I suppose this is what you want to avoid, but... isn't avoiding such changes the reason why LTS kernels exist?

And there is only one LTS kernel with this driver, v6.6, released 8 months ago. How did it become necessary to ossify the driver's wire behaviour in this time frame?

Hence I'm confused why changing the wire protocol upstream, without breaking any functionality on the original controller or backporting to LTS kernels, creates problems. Either the tests are correctly written in a way to not be affected by this change, mimicking the original gamepad which is unaffected. Or the tests are protected from breaking by using LTS kernels. In the latter case, the tests will break on a kernel version bump - legitimately so, and fixing them should be easy.


Am I missing something?


Do you see a way to get this patch in?

Would it help you to have some time for warning your downstream contacts to stabilise their tests, and I could re-send this patch in 6 months from now?


Hopeful greetings,
Max