diff mbox series

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

Message ID 20240811130906.3682-1-max@enpas.org
State New
Headers show
Series [v2] hid-playstation: DS4: Update rumble and lightbar together | expand

Commit Message

Max Staudt Aug. 11, 2024, 1:09 p.m. UTC 
Some 3rd party gamepads expect updates to rumble and lightbar together,
and setting one may cancel the other.

Let's maximise compatibility by always sending rumble and lightbar
updates together, even when only one has been scheduled.

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
---
Changes in v1 -> v2:
 - Simplified the code, comment, and commit message.
---
 drivers/hid/hid-playstation.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Roderick Colenbrander Aug. 14, 2024, 4:17 p.m. UTC | #1 
Hi Max,

That looks like a reasonable workaround. I think it would be fine to
add. We will try to support that with Android as well.

Thanks,
Roderick

On Sun, Aug 11, 2024 at 6:15 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.
>
> Let's maximise compatibility by always sending rumble and lightbar
> updates together, even when only one has been scheduled.
>
> 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
> ---
> Changes in v1 -> v2:
>  - Simplified the code, comment, and commit message.
> ---
>  drivers/hid/hid-playstation.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index e7c309cfe3a0..0d90d7ee693c 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -2143,6 +2143,26 @@ static void dualshock4_output_worker(struct work_struct *work)
>
>         spin_lock_irqsave(&ds4->base.lock, flags);
>
> +       /*
> +        * Some 3rd party gamepads expect updates to rumble and lightbar
> +        * together, and setting one may cancel the other.
> +        *
> +        * Let's maximise compatibility by always sending rumble and lightbar
> +        * updates together, even when only one has been scheduled, resulting
> +        * in:
> +        *
> +        *   ds4->valid_flag0 >= 0x03
> +        *
> +        * Hopefully this will maximise compatibility with third-party pads.
> +        *
> +        * Any further update bits, such as 0x04 for lightbar blinking, will
> +        * be or'd on top of this like before.
> +        */
> +       if (ds4->update_rumble || ds4->update_lightbar) {
> +               ds4->update_rumble = true; /* 0x01 */
> +               ds4->update_lightbar = true; /* 0x02 */
> +       }
> +
>         if (ds4->update_rumble) {
>                 /* Select classic rumble style haptics and enable it. */
>                 common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_MOTOR;
> --
> 2.39.2
>
>
Max Staudt Aug. 14, 2024, 4:40 p.m. UTC | #2 
Great, thank you!

Then I guess the previous patch was just too hard to understand. Lesson 
learned, I'll try to be clearer next time :)


On another note, I've dumped hid->rdesc for my controller which shows 
this problem. It is different from my Sony DS4 2.0 controllers, which 
all have the same descriptor. Do you prefer to guard this hack by a 
quirk, which activates on, say, a CRC32 match of the HID descriptor?


Thank you, and sorry for the extended back and forth!

Max




On 8/15/24 01:17, Roderick Colenbrander wrote:
> Hi Max,
> 
> That looks like a reasonable workaround. I think it would be fine to
> add. We will try to support that with Android as well.
> 
> Thanks,
> Roderick
> 
> On Sun, Aug 11, 2024 at 6:15 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.
>>
>> Let's maximise compatibility by always sending rumble and lightbar
>> updates together, even when only one has been scheduled.
>>
>> 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
>> ---
>> Changes in v1 -> v2:
>>   - Simplified the code, comment, and commit message.
>> ---
>>   drivers/hid/hid-playstation.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
>> index e7c309cfe3a0..0d90d7ee693c 100644
>> --- a/drivers/hid/hid-playstation.c
>> +++ b/drivers/hid/hid-playstation.c
>> @@ -2143,6 +2143,26 @@ static void dualshock4_output_worker(struct work_struct *work)
>>
>>          spin_lock_irqsave(&ds4->base.lock, flags);
>>
>> +       /*
>> +        * Some 3rd party gamepads expect updates to rumble and lightbar
>> +        * together, and setting one may cancel the other.
>> +        *
>> +        * Let's maximise compatibility by always sending rumble and lightbar
>> +        * updates together, even when only one has been scheduled, resulting
>> +        * in:
>> +        *
>> +        *   ds4->valid_flag0 >= 0x03
>> +        *
>> +        * Hopefully this will maximise compatibility with third-party pads.
>> +        *
>> +        * Any further update bits, such as 0x04 for lightbar blinking, will
>> +        * be or'd on top of this like before.
>> +        */
>> +       if (ds4->update_rumble || ds4->update_lightbar) {
>> +               ds4->update_rumble = true; /* 0x01 */
>> +               ds4->update_lightbar = true; /* 0x02 */
>> +       }
>> +
>>          if (ds4->update_rumble) {
>>                  /* Select classic rumble style haptics and enable it. */
>>                  common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_MOTOR;
>> --
>> 2.39.2
>>
>>
Max Staudt Aug. 14, 2024, 4:53 p.m. UTC | #3 
On 8/15/24 01:40, Max Staudt wrote:
> On another note, I've dumped hid->rdesc for my controller which shows 
> this problem. It is different from my Sony DS4 2.0 controllers, which 
> all have the same descriptor. Do you prefer to guard this hack by a 
> quirk, which activates on, say, a CRC32 match of the HID descriptor?

Since I forgot to write this in my last email:

Thank you for pushing me to look harder for a difference by which I 
could tell the controllers apart. I didn't know about hid->rdesc before, 
and therefore missed this opportunity. Sorry about that!


Max
Jiri Kosina Aug. 19, 2024, 6:33 p.m. UTC | #4 
On Wed, 14 Aug 2024, Roderick Colenbrander wrote:

> Hi Max,
> 
> That looks like a reasonable workaround. I think it would be fine to
> add. We will try to support that with Android as well.

Roderick, can I take this as your Acked-by:?

Thanks,
Roderick Colenbrander Aug. 19, 2024, 8:37 p.m. UTC | #5 
On Mon, Aug 19, 2024 at 11:33 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Wed, 14 Aug 2024, Roderick Colenbrander wrote:
>
> > Hi Max,
> >
> > That looks like a reasonable workaround. I think it would be fine to
> > add. We will try to support that with Android as well.
>
> Roderick, can I take this as your Acked-by:?

Consider it Acked-by.

> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
Jiri Kosina Aug. 19, 2024, 8:46 p.m. UTC | #6 
On Mon, 19 Aug 2024, Roderick Colenbrander wrote:

> > > Hi Max,
> > >
> > > That looks like a reasonable workaround. I think it would be fine to
> > > add. We will try to support that with Android as well.
> >
> > Roderick, can I take this as your Acked-by:?
> 
> Consider it Acked-by.

Thanks. I was ->.<- this close to applying it, but apparently it's missing 
Max's signoff ...
Max Staudt Aug. 19, 2024, 9:02 p.m. UTC | #7 
On 8/19/24 22:46, Jiri Kosina wrote:
> On Mon, 19 Aug 2024, Roderick Colenbrander wrote:
> 
>>>> Hi Max,
>>>>
>>>> That looks like a reasonable workaround. I think it would be fine to
>>>> add. We will try to support that with Android as well.
>>>
>>> Roderick, can I take this as your Acked-by:?
>>
>> Consider it Acked-by.
> 
> Thanks. I was ->.<- this close to applying it, but apparently it's missing
> Max's signoff ...

Indeed it does, sorry.

I just got my hands on a few more controllers - let me send a PATCHv3 
with the new findings :)


Sorry again for the back and forth, and for not finding earlier that 
there may indeed be a pattern by which to tell controllers apart.

While most controllers copy the original HID descriptor exactly, the 
controllers that need this patch seem to have something unique. I just 
didn't think this would possibly be the case before.


Max
diff mbox series

Patch

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index e7c309cfe3a0..0d90d7ee693c 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2143,6 +2143,26 @@  static void dualshock4_output_worker(struct work_struct *work)
 
 	spin_lock_irqsave(&ds4->base.lock, flags);
 
+	/*
+	 * Some 3rd party gamepads expect updates to rumble and lightbar
+	 * together, and setting one may cancel the other.
+	 *
+	 * Let's maximise compatibility by always sending rumble and lightbar
+	 * updates together, even when only one has been scheduled, resulting
+	 * in:
+	 *
+	 *   ds4->valid_flag0 >= 0x03
+	 *
+	 * Hopefully this will maximise compatibility with third-party pads.
+	 *
+	 * Any further update bits, such as 0x04 for lightbar blinking, will
+	 * be or'd on top of this like before.
+	 */
+	if (ds4->update_rumble || ds4->update_lightbar) {
+		ds4->update_rumble = true; /* 0x01 */
+		ds4->update_lightbar = true; /* 0x02 */
+	}
+
 	if (ds4->update_rumble) {
 		/* Select classic rumble style haptics and enable it. */
 		common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_MOTOR;