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

["Manuel Schönlaub" <manuel.schoenlaub@gmail.com>]

On Thu, Mar 24, 2022 at 08:54:29PM +0100, Benjamin Tissoires wrote:
> On Thu, Mar 24, 2022 at 4:34 AM Manuel Schönlaub
> <manuel.schoenlaub@gmail.com> wrote:
> >
> > On Wed, Mar 23, 2022 at 09:22:49PM +0000, Filipe Laíns wrote:
> > > On Tue, 2022-03-08 at 16:50 -0700, Manuel Schönlaub wrote:
> > > > 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 <manuel.schoenlaub@gmail.com>
> > > > ---
> > > >  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.
> 
Sure. 
I actually had thought a bit about that and would say that the concept
of breathing, pulse etc.. can be modeled quite well with hardware patterns. 

> > 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.

That's true. I would certainly hate that too.

> 
> 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.

I'd like to give it a shot and come up with a follow-up patch series
implementing e.g. breathing. Let's see how that turns out.

> Cheers,
> Benjamin
> 
> >
> > Best Regards,
> >
> > Manuel
> >
>