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 <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 188 +++++++++++++++++++++++++++++++
1 file changed, 188 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 81de88ab2..0b6c9c4b8 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -24,6 +24,8 @@
#include <linux/atomic.h>
#include <linux/fixp-arith.h>
#include <asm/unaligned.h>
+#include <linux/leds.h>
+#include <linux/led-class-multicolor.h>
#include "usbhid/usbhid.h"
#include "hid-ids.h"
@@ -96,6 +98,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_CAPABILITY_BATTERY_VOLTAGE BIT(4)
#define HIDPP_CAPABILITY_BATTERY_PERCENTAGE BIT(5)
#define HIDPP_CAPABILITY_UNIFIED_BATTERY BIT(6)
+#define HIDPP_CAPABILITY_HIDPP20_COLORED_LEDS BIT(7)
#define lg_map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
@@ -159,6 +162,12 @@ struct hidpp_battery {
u8 supported_levels_1004;
};
+struct hidpp_leds {
+ u8 feature_index;
+ u8 count;
+ struct led_classdev_mc leds[];
+};
+
/**
* struct hidpp_scroll_counter - Utility class for processing high-resolution
* scroll events.
@@ -201,6 +210,7 @@ struct hidpp_device {
u8 supported_reports;
struct hidpp_battery battery;
+ struct hidpp_leds *leds;
struct hidpp_scroll_counter vertical_wheel_counter;
u8 wireless_feature_index;
@@ -1708,6 +1718,134 @@ static int hidpp_battery_get_property(struct power_supply *psy,
return ret;
}
+/* -------------------------------------------------------------------------- */
+/* 0x8070: Color LED effect */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_LED_EFFECTS 0x8070
+
+#define CMD_COLOR_LED_EFFECTS_GET_INFO 0x00
+
+#define CMD_COLOR_LED_EFFECTS_SET_ZONE_STATE 0x31
+
+static int hidpp20_color_led_effect_get_info(struct hidpp_device *hidpp_dev,
+ u8 feature_index, u8 *count)
+{
+ struct hidpp_report response;
+ int ret;
+ u8 *params = (u8 *)response.fap.params;
+
+ ret = hidpp_send_fap_command_sync(hidpp_dev, feature_index,
+ CMD_COLOR_LED_EFFECTS_GET_INFO,
+ NULL, 0, &response);
+
+ if (ret > 0) {
+ hid_err(hidpp_dev->hid_dev,
+ "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return -EPROTO;
+ }
+ if (ret)
+ return ret;
+
+ *count = params[0];
+ return 0;
+}
+
+static int hidpp20_color_effect_set(struct hidpp_device *hidpp_dev,
+ u8 zone, bool enabled,
+ u8 r, u8 b, u8 g)
+{
+ int ret;
+ u8 params[5];
+ struct hidpp_report response;
+
+ params[0] = zone;
+ params[1] = enabled ? 1 : 0;
+ params[2] = r;
+ params[3] = g;
+ params[4] = b;
+
+ ret = hidpp_send_fap_command_sync(hidpp_dev,
+ hidpp_dev->leds->feature_index,
+ CMD_COLOR_LED_EFFECTS_SET_ZONE_STATE,
+ params, sizeof(params), &response);
+
+ if (ret)
+ return ret;
+ return 0;
+}
+
+static int hidpp_set_brightness(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ int n;
+ struct device *dev = cdev->dev->parent;
+ struct hid_device *hid = to_hid_device(dev);
+ struct hidpp_device *hidpp = hid_get_drvdata(hid);
+
+ struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+ u8 red, green, blue;
+
+ led_mc_calc_color_components(mc_cdev, brightness);
+ red = mc_cdev->subled_info[0].brightness;
+ green = mc_cdev->subled_info[1].brightness;
+ blue = mc_cdev->subled_info[2].brightness;
+
+ for (n = 0; n < hidpp->leds->count; n++) {
+ if (cdev == &hidpp->leds->leds[n].led_cdev) {
+ return hidpp20_color_effect_set(hidpp, n,
+ brightness > 0,
+ red, green, blue);
+ }
+ }
+
+ return LED_OFF;
+}
+
+static int hidpp_mc_led_register(struct hidpp_device *hidpp_dev,
+ struct led_classdev_mc *mc_dev,
+ int zone)
+{
+ struct hid_device *hdev = hidpp_dev->hid_dev;
+ struct mc_subled *mc_led_info;
+ struct led_classdev *cdev;
+ int ret;
+
+ mc_led_info = devm_kmalloc_array(&hdev->dev, 3,
+ sizeof(*mc_led_info),
+ GFP_KERNEL | __GFP_ZERO);
+ if (!mc_led_info)
+ return -ENOMEM;
+
+ mc_led_info[0].color_index = LED_COLOR_ID_RED;
+ mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
+ mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
+
+ mc_dev->subled_info = mc_led_info;
+ mc_dev->num_colors = 3;
+
+ cdev = &mc_dev->led_cdev;
+ cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+ "%s:rgb:indicator-%d", hdev->uniq, zone);
+
+ if (!cdev->name)
+ return -ENOMEM;
+
+ cdev->brightness = 0;
+ cdev->max_brightness = 255;
+ cdev->flags |= LED_CORE_SUSPENDRESUME;
+ cdev->brightness_set_blocking = hidpp_set_brightness;
+
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, mc_dev);
+ if (ret < 0) {
+ hid_err(hdev, "Cannot register multicolor LED device: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
/* -------------------------------------------------------------------------- */
/* 0x1d4b: Wireless device status */
/* -------------------------------------------------------------------------- */
@@ -3699,6 +3837,54 @@ static int hidpp_event(struct hid_device *hdev, struct hid_field *field,
return 1;
}
+static int hidpp_initialize_leds(struct hidpp_device *hidpp_dev)
+{
+ u8 count;
+ u8 feature_index;
+ u8 feature_type;
+ int i;
+ int ret;
+ struct hid_device *hdev;
+
+ hdev = hidpp_dev->hid_dev;
+ if (hidpp_dev->leds)
+ return 0;
+ if (hidpp_dev->protocol_major >= 2) {
+ ret = hidpp_root_get_feature(hidpp_dev,
+ HIDPP_PAGE_LED_EFFECTS,
+ &feature_index,
+ &feature_type);
+ if (ret)
+ return ret;
+
+ ret = hidpp20_color_led_effect_get_info(hidpp_dev, feature_index, &count);
+ if (ret)
+ return ret;
+
+ hidpp_dev->capabilities |= HIDPP_CAPABILITY_HIDPP20_COLORED_LEDS;
+ hidpp_dev->leds = devm_kzalloc(&hdev->dev,
+ struct_size(hidpp_dev->leds, leds, count),
+ GFP_KERNEL);
+
+ if (!hidpp_dev->leds)
+ return -ENOMEM;
+
+ hidpp_dev->leds->feature_index = feature_index;
+ hidpp_dev->leds->count = count;
+
+ for (i = 0; i < count; i++) {
+ ret = hidpp_mc_led_register(hidpp_dev, &hidpp_dev->leds->leds[i], i);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+
+ } else {
+ return 0;
+ }
+}
+
static int hidpp_initialize_battery(struct hidpp_device *hidpp)
{
static atomic_t battery_no = ATOMIC_INIT(0);
@@ -3943,6 +4129,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
if (hidpp->battery.ps)
power_supply_changed(hidpp->battery.ps);
+ hidpp_initialize_leds(hidpp);
+
if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL)
hi_res_scroll_enable(hidpp);
--
2.30.2
On Wed, 2022-03-23 at 21:22 +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 <[email protected]>
> > ---
> > 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.
There's already an API for LEDs in the kernel, why shouldn't it be used
to avoid user-space needing to know how to configure Logitech, and
every other brand of keyboards?
systemd has code to save and restore LED status, as well as code to
change the level of backlight. I can imagine that it wouldn't take much
to make it aware of RGB LEDs so it handles them properly, whether it's
for Logitech, or another brand of keyboards, or laptops.
Hi!
> 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 <[email protected]>
Please cc LEDs stuff to the LED lists.
> +static int hidpp_mc_led_register(struct hidpp_device *hidpp_dev,
> + struct led_classdev_mc *mc_dev,
> + int zone)
> +{
> + struct hid_device *hdev = hidpp_dev->hid_dev;
> + struct mc_subled *mc_led_info;
> + struct led_classdev *cdev;
> + int ret;
> +
> + mc_led_info = devm_kmalloc_array(&hdev->dev, 3,
> + sizeof(*mc_led_info),
> + GFP_KERNEL | __GFP_ZERO);
> + if (!mc_led_info)
> + return -ENOMEM;
> +
> + mc_led_info[0].color_index = LED_COLOR_ID_RED;
> + mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
> + mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
> +
> + mc_dev->subled_info = mc_led_info;
> + mc_dev->num_colors = 3;
> +
> + cdev = &mc_dev->led_cdev;
> + cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
> + "%s:rgb:indicator-%d", hdev->uniq, zone);
So this is keyboard backlight? We should add the documentation at the
very least, so that other drivers use same name.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
On Wed, Mar 23, 2022 at 10:04:23PM +0100, Pavel Machek wrote:
> Hi!
>
> > 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 <[email protected]>
>
> Please cc LEDs stuff to the LED lists.
>
Will do. Though it seems like first we should discuss whether the kernel
in fact is the right place, no?
> > +static int hidpp_mc_led_register(struct hidpp_device *hidpp_dev,
> > + struct led_classdev_mc *mc_dev,
> > + int zone)
> > +{
> > + struct hid_device *hdev = hidpp_dev->hid_dev;
> > + struct mc_subled *mc_led_info;
> > + struct led_classdev *cdev;
> > + int ret;
> > +
> > + mc_led_info = devm_kmalloc_array(&hdev->dev, 3,
> > + sizeof(*mc_led_info),
> > + GFP_KERNEL | __GFP_ZERO);
> > + if (!mc_led_info)
> > + return -ENOMEM;
> > +
> > + mc_led_info[0].color_index = LED_COLOR_ID_RED;
> > + mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
> > + mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
> > +
> > + mc_dev->subled_info = mc_led_info;
> > + mc_dev->num_colors = 3;
> > +
> > + cdev = &mc_dev->led_cdev;
> > + cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
> > + "%s:rgb:indicator-%d", hdev->uniq, zone);
>
> So this is keyboard backlight? We should add the documentation at the
> very least, so that other drivers use same name.
>
> Best regards,
> Pavel
>
> --
> People of Russia, stop Putin before his war on Ukraine escalates.
I do not own a Logitech keyboard, but some mice. There are RGB leds
that you can normally control with Windows software.
I'd suppose (but could not verify) that supported keyboards by Logitech
work with the same feature.
Best Regards,
Manuel
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 <[email protected]>
> ---
> 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
On Wed, Mar 23, 2022 at 11:24:18PM +0100, Bastien Nocera wrote:
> On Wed, 2022-03-23 at 21:22 +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 <[email protected]>
> > > ---
> > > ?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.
>
> There's already an API for LEDs in the kernel, why shouldn't it be used
> to avoid user-space needing to know how to configure Logitech, and
> every other brand of keyboards?
>
> systemd has code to save and restore LED status, as well as code to
> change the level of backlight. I can imagine that it wouldn't take much
> to make it aware of RGB LEDs so it handles them properly, whether it's
> for Logitech, or another brand of keyboards, or laptops.
Teaching systemd-backlight about mulicolor backlights might be a nice project
too. But their use case seems to be more about screen backlights as it seems.
Did I overlook something here?
Oh and yeah, IMHO another argument could be that obviously at some point
the LED management could be removed from those user space tools, as the
kernel would already know about them.
After all the LED class devices should be there for a reason ;-)
Cheers,
Manuel
On Thu, Mar 24, 2022 at 4:34 AM Manuel Schönlaub
<[email protected]> 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 <[email protected]>
> > > ---
> > > 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
>
On Wed, 2022-03-23 at 21:28 -0600, Manuel Schönlaub wrote:
> On Wed, Mar 23, 2022 at 11:24:18PM +0100, Bastien Nocera wrote:
> > On Wed, 2022-03-23 at 21:22 +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 <[email protected]>
> > > > ---
> > > > 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.
> >
> > There's already an API for LEDs in the kernel, why shouldn't it be
> > used
> > to avoid user-space needing to know how to configure Logitech, and
> > every other brand of keyboards?
> >
> > systemd has code to save and restore LED status, as well as code to
> > change the level of backlight. I can imagine that it wouldn't take
> > much
> > to make it aware of RGB LEDs so it handles them properly, whether
> > it's
> > for Logitech, or another brand of keyboards, or laptops.
>
> Teaching systemd-backlight about mulicolor backlights might be a nice
> project
> too. But their use case seems to be more about screen backlights as
> it seems.
> Did I overlook something here?
From rules.d/99-systemd.rules.in:
# Pull in backlight save/restore for all backlight devices and
# keyboard backlights
SUBSYSTEM=="backlight", TAG+="systemd", IMPORT{builtin}="path_id", ENV{SYSTEMD_WANTS}+="systemd-backlight@backlight:$name.service"
SUBSYSTEM=="leds", KERNEL=="*kbd_backlight", TAG+="systemd", IMPORT{builtin}="path_id", ENV{SYSTEMD_WANTS}+="systemd-backlight@leds:$name.service"
And from the NEWS file for systemd 243:
* systemd-logind now exposes a per-session SetBrightness() bus call,
which may be used to securely change the brightness of a kernel
brightness device, if it belongs to the session's seat. By using this
call unprivileged clients can make changes to "backlight" and "leds"
devices securely with strict requirements on session membership.
Desktop environments may use this to generically make brightness
changes to such devices without shipping private SUID binaries or
udev rules for that purpose.
It's clear that it's not just displays.
>
> Oh and yeah, IMHO another argument could be that obviously at some
> point
> the LED management could be removed from those user space tools, as
> the
> kernel would already know about them.
>
> After all the LED class devices should be there for a reason ;-)
>
> Cheers,
>
> Manuel
>
On Thu, Mar 24, 2022 at 10:32:21AM +0100, Bastien Nocera wrote:
> On Wed, 2022-03-23 at 21:28 -0600, Manuel Sch?nlaub wrote:
> > On Wed, Mar 23, 2022 at 11:24:18PM +0100, Bastien Nocera wrote:
> > > On Wed, 2022-03-23 at 21:22 +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 <[email protected]>
> > > > > ---
> > > > > ?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.
> > >
> > > There's already an API for LEDs in the kernel, why shouldn't it be
> > > used
> > > to avoid user-space needing to know how to configure Logitech, and
> > > every other brand of keyboards?
> > >
> > > systemd has code to save and restore LED status, as well as code to
> > > change the level of backlight. I can imagine that it wouldn't take
> > > much
> > > to make it aware of RGB LEDs so it handles them properly, whether
> > > it's
> > > for Logitech, or another brand of keyboards, or laptops.
> >
> > Teaching systemd-backlight about mulicolor backlights might be a nice
> > project
> > too. But their use case seems to be more about screen backlights as
> > it seems.
> > Did I overlook something here?
>
> From rules.d/99-systemd.rules.in:
> # Pull in backlight save/restore for all backlight devices and
> # keyboard backlights
> SUBSYSTEM=="backlight", TAG+="systemd", IMPORT{builtin}="path_id", ENV{SYSTEMD_WANTS}+="systemd-backlight@backlight:$name.service"
> SUBSYSTEM=="leds", KERNEL=="*kbd_backlight", TAG+="systemd", IMPORT{builtin}="path_id", ENV{SYSTEMD_WANTS}+="systemd-backlight@leds:$name.service"
>
> And from the NEWS file for systemd 243:
> * systemd-logind now exposes a per-session SetBrightness() bus call,
> which may be used to securely change the brightness of a kernel
> brightness device, if it belongs to the session's seat. By using this
> call unprivileged clients can make changes to "backlight" and "leds"
> devices securely with strict requirements on session membership.
> Desktop environments may use this to generically make brightness
> changes to such devices without shipping private SUID binaries or
> udev rules for that purpose.
>
> It's clear that it's not just displays.
>
> >
> > Oh and yeah, IMHO another argument could be that obviously at some
> > point
> > the LED management could be removed from those user space tools, as
> > the
> > kernel would already know about them.
> >
> > After all the LED class devices should be there for a reason ;-)
> >
> > Cheers,
> >
> > Manuel
> >
>
Yeah the SetBrightness in systemd / logind should work out of box.
Though I just noticed something: For this to be useful, the default
multi_intensity for each component of the multicolor LED in the kernel should be set to
max_brightness, effectively producing white (on RGB LEDs at least).
If it is zero, like now, SetBrightness would IMHO not actually switch
the LED on because of how the calculation of color components works.
That way systemd wouldn't need to care about whether the LED is
multicolor or not.
The save/restore stuff IMHO works only for backlights and kbd_backlight LEDs,
as seen from the udev rules. At least out of the box. It even seems to stop
working once you would have two kbd_backlights for the same device
(as the name would be kbd_backlight-1, kbd_backlight-2, ... as per the
documentation on LED classdevs.)
Now there are three solutions:
1) Naming the Logitech LEDs <device>:rgb:kbd_backlight-N even on mice
and at some point send a patch to systemd adapting the udev rules to
care for devices with nuemrical suffixes.
2) Naming the LEDs <device>:rgb:backlight-N and send a patch to systemd
adding a new udev rule to cater for backlight LEDs in general.
3) Like 2 but expect users to write their own udev rules if they want
save/restore for Logitech devices.
What's your opinion on the naming? IMHO mice are not exactly keyboards,
so it probably should be a "general" backlight.
(Instead of "indicator", as in version 1 of the patch)
Cheers
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
> <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
> >
>
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 <[email protected]>
> > ---
> > ?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.
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?
Best Regards,
Manuel
Hi!
> > > 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 <[email protected]>
> >
> > Please cc LEDs stuff to the LED lists.
> >
>
> Will do. Though it seems like first we should discuss whether the kernel
> in fact is the right place, no?
Well, on embedded systems keyboard backlight is handled in kernel.
> > > + cdev = &mc_dev->led_cdev;
> > > + cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
> > > + "%s:rgb:indicator-%d", hdev->uniq, zone);
> >
> > So this is keyboard backlight? We should add the documentation at the
> > very least, so that other drivers use same name.
>
> I do not own a Logitech keyboard, but some mice. There are RGB leds
> that you can normally control with Windows software.
>
> I'd suppose (but could not verify) that supported keyboards by Logitech
> work with the same feature.
And I guess we should do the same for Logitech keyboards. Userspace
does not need to go to /sys/class/gpio... to control keyboard
backlight on Nokia cellphone, it should not need to talk USB directly
to control backlight on Logitech keyboards.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi!
> Yeah the SetBrightness in systemd / logind should work out of box.
>
> Though I just noticed something: For this to be useful, the default
> multi_intensity for each component of the multicolor LED in the kernel should be set to
> max_brightness, effectively producing white (on RGB LEDs at least).
Agreed we should have multi_intensity set to something nonzero. Note
that max on all channels may not result in white.
> Now there are three solutions:
>
> 1) Naming the Logitech LEDs <device>:rgb:kbd_backlight-N even on mice
> and at some point send a patch to systemd adapting the udev rules to
> care for devices with nuemrical suffixes.
>
> 2) Naming the LEDs <device>:rgb:backlight-N and send a patch to systemd
> adding a new udev rule to cater for backlight LEDs in general.
If it is known to be mouse, use mouse_backlight or something like
that. Just document it so that others use same string.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi!
> > > 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.
Yes please.
Note that many devices have different patterns with different
limitations; we need to somehow solve that, anyway.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.