Re: [PATCH] HID: logitech-hidpp: support Color LED feature (8071).

On Thu, Mar 24, 2022 at 4:34 AM Manuel Schönlaub
[off-list ref] wrote:

On Wed, Mar 23, 2022 at 09:22:49PM +0000, Filipe Laíns wrote:

quoted

On Tue, 2022-03-08 at 16:50 -0700, Manuel Schönlaub wrote:

quoted

The HID++ protocol allows to set multicolor (RGB) to a static color.
Multiple of such LED zones per device are supported.
This patch exports said LEDs so that they can be set from userspace.

Signed-off-by: Manuel Schönlaub <redacted>
---
 drivers/hid/hid-logitech-hidpp.c | 188 +++++++++++++++++++++++++++++++
 1 file changed, 188 insertions(+)

*snip*

Hi Manuel,

Thanks for putting this forward, although I am not sure if this is the best way
to handle this.

Before anything, could you elaborate a bit on what lead to you wanting this?

There are a couple of reasons why merging this in the kernel might be
problematic.

1) I don't think we will ever support the full capabilities of the devices, so
configuration via userspace apps will always be required, and here we are
introducing a weird line between the two.

2) There is already an ecosystem of userspace configuration apps, with which
this would conflict. They might not be in the best maintenance state due to lack
of time from the maintainers, but moving this functionality to the kernel, which
is harder change, and harder to ship to users, will only make that worse.

Cheers,
Filipe Laíns

Hi Filipe,

sure.

While I realize that there is e.g. ratbagd which supports a great deal of the
HIDPP features and should allow you to control LEDs, unfortunately for my G305
it does not support the LED (and as far as I remember my G403 does not
work at all with it).

Then I figured that actually having the LEDs in kernel would allow led triggers
to work with them, so you could do fancy stuff like showing disk or CPU activity
or free physical memory... and here we are now.

The one thing that concerns me with those gaming LEDs, is that there
is much more than just color/intensity.
Those LEDs have effects that you can enable (breathing, pulse, color
changing, etc...) and I am not sure how much you are going to be able
to sync with the simple LED class.

As for supporting the full capabilities of these devices: The patch just adds
RGB leds, which is something already quite standardized in the linux kernel for
a variety of devices.
Some roccat mice even have support for changing the actual DPI in their kernel
driver, which arguably is a whole different story though and not scope of this patch.
There are also other features (like on-board profiles) which I would definitely
see being better off in user space, especially as long as there is no additional
benefit in having them in the kernel.

Regarding the conflict in userspace: ratbagd currently seems to always write
LED state in RAM and the on-board profiles at the same time, so I would
argue that the use case here is different: The user space tools want to
set the LED color in a persistent way, while here we want to have interaction with
LED triggers and a more transient way. E.g. the driver would overwrite
only the transient LED color, not the onboard-profiles.

If that is already too much, what about a module option that allows a user to
deactivate the feature?

Please no. I am tired of having way too many options that nobody uses
except for a couple of people and we can not remove/change them
because of those 2 persons.

Either you manage to sync the LED class state somehow (in a sensible
manner), or I don't think having such LEDs in the kernel is a good
thing because we are going to fight against userspace.

Cheers,
Benjamin

Best Regards,

Manuel