Re: Litra Glow on Linux

Re: Litra Glow on Linux

[Alan Stern]

Forwarding this message to the linux-input mailing list, since it 
concerns the input layer and not the USB layer.

Alan Stern

PS: Note that problem 1 below is easily solved with a udev script.

----- Forwarded message from Andreas Bergmeier <abergmeier@gmx.net> -----

Date: Mon, 17 Oct 2022 18:45:30 +0200
From: Andreas Bergmeier <abergmeier@gmx.net>
To: linux-usb@vger.kernel.org
Subject: Litra Glow on Linux

On my Ubuntu machine i am running 5.15.0. Now when I plugin in my
Logitech Litra Glow, it gets detected and the following shows up in my
dmesg:

```
input: Logi Litra Glow Consumer Control as
/devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4.2/3-4.2:1.0/0003:046D:C900.000B/input/input75
hid-generic 0003:046D:C900.000B: input,hiddev0,hidraw2: USB HID v1.11
Device [Logi Litra Glow] on usb-0000:00:14.0-4.2/input0
```

Via (hardware) buttons you can switch the device on, regulate the
color temperature as well as the brightness.
I know of no way to fully control the device from my computer and
would like to change that.

It seems to me like I need to solve 4 problems (in userspace and maybe
kernelspace):
1. Handle plugging in and off
2. Listen to events (button pressed) from the device
3. Get the current state of the device
4. Send events to the device


The device seems to provide a pretty bare HID Report interface with no
alternate configurations:
https://github.com/abergmeier/litra_glow_linux/blob/main/lsusb
The HID seems to define 3 Reports:
https://github.com/abergmeier/litra_glow_linux/blob/main/parsed_descriptor

Ignoring 1. for now.

Trying to solve 2. I wrote a basic HIDDEV application. Using `read` I
only see events from Report 17 (0x11). For all my experimenting with
the device I have never seen a Report 1 or 2.
So I get events, but it seems like the provided
`hiddev_usage_ref.value` is sometimes wrong (seems to be 0 and 1 for
most of the time even if I adjust the brightness).
Doing a recording (turning on, adjusting brightness, turning off) of
the raw HID events seems like the "correct" events are sent from the
device: https://github.com/abergmeier/litra_glow_linux/blob/main/hid-recorder.
So it seems to me like maybe the values get mixed up somewhere in the HID code.
Alternatively I did a `evtest` run on the /dev/input/event* for the
`Logi Litra Glow Consumer Control`:
https://github.com/abergmeier/litra_glow_linux/blob/main/evtest
When pressing (hardware) buttons no events showed up in `evtest´.
Probably not surprising since these would be from Report 1 and 2 IIUC.
Now I am not sure whether the USB interface is sketchy or whether one
needs to activate the _Consumer Control_ somehow.

Trying to solve 3. from what I understand with HID there usually is no
way of reading the current state of the device?

Trying to solve 4. there are userspace libraries in Python and Go
which send events to the device bypassing HID. So there may be some
quirks handling necessary in HID but I would defer that until 2. is
done.

With all that I am pretty much at my wits end and would appreciate any
input how to further analyze the device situation.

Cheers

----- End forwarded message -----

Re: Litra Glow on Linux

[Bastien Nocera]

On Wed, 2022-10-19 at 11:17 -0400, Alan Stern wrote:
> Via (hardware) buttons you can switch the device on, regulate the
> color temperature as well as the brightness.
> I know of no way to fully control the device from my computer and
> would like to change that.

Anything that those libraries and tools can't do?

Re: Litra Glow on Linux

[Bastien Nocera]

On Wed, 2022-10-19 at 11:17 -0400, Alan Stern wrote:
> Via (hardware) buttons you can switch the device on, regulate the
> color temperature as well as the brightness.
> I know of no way to fully control the device from my computer and
> would like to change that.

Anything that those libraries and tools can't do?

https://github.com/search?q=litra+glow

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Wed, 19 Oct 2022 at 17:49, Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-10-19 at 11:17 -0400, Alan Stern wrote:
> > Via (hardware) buttons you can switch the device on, regulate the
> > color temperature as well as the brightness.
> > I know of no way to fully control the device from my computer and
> > would like to change that.
>
> Anything that those libraries and tools can't do?
>
> https://github.com/search?q=litra+glow

From the code I have looked into it seems like some Mac libraries
might be able to do all that. Why, I am not yet clear on.
I have a _feeling_ that maybe Windows and macOS do a different HID
mapping/processing than Linux.

Re: Litra Glow on Linux

[Andreas Bergmeier]

Reading through hid-input it seems like the custom Usage Page of
Logitech gets explicity ignored.
So for Brightness and/or Color Temperature controls I wonder whether
it would be appropriate to map these to ABS_WHEEL.

On Wed, 19 Oct 2022 at 17:17, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Forwarding this message to the linux-input mailing list, since it
> concerns the input layer and not the USB layer.
>
> Alan Stern
>
> PS: Note that problem 1 below is easily solved with a udev script.
>
> ----- Forwarded message from Andreas Bergmeier <abergmeier@gmx.net> -----
>
> Date: Mon, 17 Oct 2022 18:45:30 +0200
> From: Andreas Bergmeier <abergmeier@gmx.net>
> To: linux-usb@vger.kernel.org
> Subject: Litra Glow on Linux
>
> On my Ubuntu machine i am running 5.15.0. Now when I plugin in my
> Logitech Litra Glow, it gets detected and the following shows up in my
> dmesg:
>
> ```
> input: Logi Litra Glow Consumer Control as
> /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4.2/3-4.2:1.0/0003:046D:C900.000B/input/input75
> hid-generic 0003:046D:C900.000B: input,hiddev0,hidraw2: USB HID v1.11
> Device [Logi Litra Glow] on usb-0000:00:14.0-4.2/input0
> ```
>
> Via (hardware) buttons you can switch the device on, regulate the
> color temperature as well as the brightness.
> I know of no way to fully control the device from my computer and
> would like to change that.
>
> It seems to me like I need to solve 4 problems (in userspace and maybe
> kernelspace):
> 1. Handle plugging in and off
> 2. Listen to events (button pressed) from the device
> 3. Get the current state of the device
> 4. Send events to the device
>
>
> The device seems to provide a pretty bare HID Report interface with no
> alternate configurations:
> https://github.com/abergmeier/litra_glow_linux/blob/main/lsusb
> The HID seems to define 3 Reports:
> https://github.com/abergmeier/litra_glow_linux/blob/main/parsed_descriptor
>
> Ignoring 1. for now.
>
> Trying to solve 2. I wrote a basic HIDDEV application. Using `read` I
> only see events from Report 17 (0x11). For all my experimenting with
> the device I have never seen a Report 1 or 2.
> So I get events, but it seems like the provided
> `hiddev_usage_ref.value` is sometimes wrong (seems to be 0 and 1 for
> most of the time even if I adjust the brightness).
> Doing a recording (turning on, adjusting brightness, turning off) of
> the raw HID events seems like the "correct" events are sent from the
> device: https://github.com/abergmeier/litra_glow_linux/blob/main/hid-recorder.
> So it seems to me like maybe the values get mixed up somewhere in the HID code.
> Alternatively I did a `evtest` run on the /dev/input/event* for the
> `Logi Litra Glow Consumer Control`:
> https://github.com/abergmeier/litra_glow_linux/blob/main/evtest
> When pressing (hardware) buttons no events showed up in `evtestด.
> Probably not surprising since these would be from Report 1 and 2 IIUC.
> Now I am not sure whether the USB interface is sketchy or whether one
> needs to activate the _Consumer Control_ somehow.
>
> Trying to solve 3. from what I understand with HID there usually is no
> way of reading the current state of the device?
>
> Trying to solve 4. there are userspace libraries in Python and Go
> which send events to the device bypassing HID. So there may be some
> quirks handling necessary in HID but I would defer that until 2. is
> done.
>
> With all that I am pretty much at my wits end and would appreciate any
> input how to further analyze the device situation.
>
> Cheers
>
> ----- End forwarded message -----

Re: Litra Glow on Linux

[Andreas Bergmeier]

> Date: Mon, 17 Oct 2022 18:45:30 +0200
> From: Andreas Bergmeier <abergmeier@gmx.net>
> To: linux-usb@vger.kernel.org
> Subject: Litra Glow on Linux
>
> On my Ubuntu machine i am running 5.15.0. Now when I plugin in my
> Logitech Litra Glow, it gets detected and the following shows up in my
> dmesg:
>
> ```
> input: Logi Litra Glow Consumer Control as
> /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4.2/3-4.2:1.0/0003:046D:C900.000B/input/input75
> hid-generic 0003:046D:C900.000B: input,hiddev0,hidraw2: USB HID v1.11
> Device [Logi Litra Glow] on usb-0000:00:14.0-4.2/input0
> ```
>
> Via (hardware) buttons you can switch the device on, regulate the
> color temperature as well as the brightness.
So I looked into the sources of `hid-input` and `hig-lg` and I hope
that I have a halfway decent
understanding why the linux modules/dev nodes handle the way they do.
What I do not yet understand is how to map the device to input "primitives".
To recap there are 5 hardware buttons and 3 states:
- State: "Color temperature in range [u, x]" Button: up
- State: "Color temperature in range [u, x]" Button: down
- State/Button On/Off
- State "Brightness in range [y, z]" Button: up
- State "Brightness in range [y, z]" Button: down

What would be a best practice to expose these correctly via HID (as
e.g. ABS_WHEEL?)?
Or at least - what are the canonical options?

Since the exposed Record interface seems "wrong" I assume I need to
write code to fix the device handling.
Now, is this better suited in `hid-input` or `hid-lg`?
The former currently tries to handle the device (wrongly) and the
latter yet ignores the device but seems to be
the one stop to fix Logitech devices.


> I know of no way to fully control the device from my computer and
> would like to change that.
By now I am confident that I will soon have the full device control figured out.

Re: Litra Glow on Linux

[Benjamin Tissoires]

Hi Andreas,

On Sat, Oct 22, 2022 at 2:48 PM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> > Date: Mon, 17 Oct 2022 18:45:30 +0200
> > From: Andreas Bergmeier <abergmeier@gmx.net>
> > To: linux-usb@vger.kernel.org
> > Subject: Litra Glow on Linux
> >
> > On my Ubuntu machine i am running 5.15.0. Now when I plugin in my
> > Logitech Litra Glow, it gets detected and the following shows up in my
> > dmesg:
> >
> > ```
> > input: Logi Litra Glow Consumer Control as
> > /devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4.2/3-4.2:1.0/0003:046D:C900.000B/input/input75
> > hid-generic 0003:046D:C900.000B: input,hiddev0,hidraw2: USB HID v1.11
> > Device [Logi Litra Glow] on usb-0000:00:14.0-4.2/input0
> > ```
> >
> > Via (hardware) buttons you can switch the device on, regulate the
> > color temperature as well as the brightness.
> So I looked into the sources of `hid-input` and `hig-lg` and I hope
> that I have a halfway decent
> understanding why the linux modules/dev nodes handle the way they do.
> What I do not yet understand is how to map the device to input "primitives".

Depending on how the device presents itself, we might have to use
hid-logitech-hidpp, not hid-lg FWIW.

And to answer the question "how to map the device to input
"primitives".", it all depends on how the device exports itself.

> To recap there are 5 hardware buttons and 3 states:
> - State: "Color temperature in range [u, x]" Button: up
> - State: "Color temperature in range [u, x]" Button: down
> - State/Button On/Off
> - State "Brightness in range [y, z]" Button: up
> - State "Brightness in range [y, z]" Button: down

FWIW, you should be able to use hid-recorder[0] (as root) to record
traces of the events, and to parse them on the fly. Also, if you
provide us the output, we can replay the traces locally as if we
virtually plugged in the device.

>
> What would be a best practice to expose these correctly via HID (as
> e.g. ABS_WHEEL?)?

This doesn't look like a wheel at all, but more like simple buttons
from your description. We should let userspace deal with the
representation as a slider by its own.

Also, are the buttons configuring the hardware directly or do you need
some software to actually change the brightness/color?

And FWIW, the device looks like we should probably just export it as a
led class device, with brightness and color support.

> Or at least - what are the canonical options?
>
> Since the exposed Record interface seems "wrong" I assume I need to
> write code to fix the device handling.
> Now, is this better suited in `hid-input` or `hid-lg`?

Again, it depends. If the device speaks the same high level interface
as other consumer Logitech devices (mice, keyboards, etc...)
hid-logitech-hidpp is the best way forward, but if not, hid-lg would
be the place to start.

> The former currently tries to handle the device (wrongly) and the
> latter yet ignores the device but seems to be
> the one stop to fix Logitech devices.
>
>
> > I know of no way to fully control the device from my computer and
> > would like to change that.
> By now I am confident that I will soon have the full device control figured out.
>

Glad to hear that. But again, we need to know the device a little bit more.
FWIW, if the first byte you have to send to control the LED is 0x10,
0x11 or 0x12 then this is a HID++ device that needs to go in
hid-logitech-hidpp and a lot of groundwork is already done in the
kernel for it.

Cheers,
Benjamin

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Tue, 25 Oct 2022 at 09:46, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Depending on how the device presents itself, we might have to use
> hid-logitech-hidpp, not hid-lg FWIW.
Ah, indeed it seems to be a hidpp device.
I so far wrote code to interact with the device using hidraw and
hiddev. What are the implications of a hidpp device?
How is hidpp exposed to userspace?
From a search it seems like there are very few userspace programs that
handle hidpp on Linux.

> And to answer the question "how to map the device to input
> "primitives".", it all depends on how the device exports itself.
All information that I have gathered so far is on
https://github.com/abergmeier/litra_glow_linux.

The output of `hidpp-list-features /dev/hidraw0` is:
```
Logi Litra Glow (046d:c900) is a HID++ 4.2 device
Feature 0x01: [0x0001] Feature set
Feature 0x02: [0x0003] Device FW version
Feature 0x03: [0x0005] Device name
Feature 0x04: [0x1990] ?
Feature 0x05: [0x1eb0] ? (hidden)
Feature 0x06: [0x00c2] DFUcontrol 3 (hidden)
```
Is it possible that Feature 0x04 is the protocol for sending changes
to the device?
All the payload that is sent to the device has a 0x04 directly in
front of the instructions.
As an example, for turning the light on you send raw bytes: [0x11,
0xff, 0x04, 0x10, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]
0x10 seems to indicate setting the on state.

> > To recap there are 5 hardware buttons and 3 states:
> > - State: "Color temperature in range [u, x]" Button: up
> > - State: "Color temperature in range [u, x]" Button: down
> > - State/Button On/Off
> > - State "Brightness in range [y, z]" Button: up
> > - State "Brightness in range [y, z]" Button: down
>
> FWIW, you should be able to use hid-recorder[0] (as root) to record
> traces of the events, and to parse them on the fly. Also, if you
> provide us the output, we can replay the traces locally as if we
> virtually plugged in the device.

See the GitHub repo - there are already initial recordings.

> FWIW, if the first byte you have to send to control the LED is 0x10,
> 0x11 or 0x12 then this is a HID++ device that needs to go in
> hid-logitech-hidpp and a lot of groundwork is already done in the
> kernel for it.
I am trying to get up to speed with hidpp - but I am not there yet.
Do you have any hints on what code to read specifically since
logitech-hidpp is a bit long...

Thanks for the feedback so far

Re: Litra Glow on Linux

[Benjamin Tissoires]

On Wed, Oct 26, 2022 at 9:50 PM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> On Tue, 25 Oct 2022 at 09:46, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > Depending on how the device presents itself, we might have to use
> > hid-logitech-hidpp, not hid-lg FWIW.
> Ah, indeed it seems to be a hidpp device.

\o/

> I so far wrote code to interact with the device using hidraw and
> hiddev. What are the implications of a hidpp device?

It's just Logitech's common HID protocol. The advantage is that if
Logitech reuses the feature on a different hardware, we won't have to
implement anything new in the kernel.

> How is hidpp exposed to userspace?

It's not exposed to userspace. It's all kernel internal, or used by
userspace applications like solaar[0] through hidraw

> From a search it seems like there are very few userspace programs that
> handle hidpp on Linux.

IIRC you have solaar, and fwupd.

>
> > And to answer the question "how to map the device to input
> > "primitives".", it all depends on how the device exports itself.
> All information that I have gathered so far is on
> https://github.com/abergmeier/litra_glow_linux.

great thanks.

>
> The output of `hidpp-list-features /dev/hidraw0` is:
> ```
> Logi Litra Glow (046d:c900) is a HID++ 4.2 device
> Feature 0x01: [0x0001] Feature set
> Feature 0x02: [0x0003] Device FW version
> Feature 0x03: [0x0005] Device name
> Feature 0x04: [0x1990] ?
> Feature 0x05: [0x1eb0] ? (hidden)
> Feature 0x06: [0x00c2] DFUcontrol 3 (hidden)
> ```
> Is it possible that Feature 0x04 is the protocol for sending changes
> to the device?

AFAICT, given the dumps on your github project, this is very much the case:
Events and commands are starting with "11 ff 04" meaning:
- 11: report id (HID++ protocol)
- ff: device index (0xff in case of a USB connected device, but could
be different when used over a wireless receiver.)
- 04: feature index 04, which is defined as 0x1990 in the internal
Logitech specification.

> All the payload that is sent to the device has a 0x04 directly in
> front of the instructions.
> As an example, for turning the light on you send raw bytes: [0x11,
> 0xff, 0x04, 0x10, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]
> 0x10 seems to indicate setting the on state.

You seem to be correct :)
This 4rth byte is the command index. 0x00 is sent by the device on
events, and then the command is supposed to be the command index
(0x10, 0x20, 0x30, etc...) with the program index. So to not confuse
other userspace programs, you should use something like 0x11 or 0x12
(0x01 is used by the kernel FWIW)

>
> > > To recap there are 5 hardware buttons and 3 states:
> > > - State: "Color temperature in range [u, x]" Button: up
> > > - State: "Color temperature in range [u, x]" Button: down
> > > - State/Button On/Off
> > > - State "Brightness in range [y, z]" Button: up
> > > - State "Brightness in range [y, z]" Button: down
> >
> > FWIW, you should be able to use hid-recorder[0] (as root) to record
> > traces of the events, and to parse them on the fly. Also, if you
> > provide us the output, we can replay the traces locally as if we
> > virtually plugged in the device.
>
> See the GitHub repo - there are already initial recordings.
>
> > FWIW, if the first byte you have to send to control the LED is 0x10,
> > 0x11 or 0x12 then this is a HID++ device that needs to go in
> > hid-logitech-hidpp and a lot of groundwork is already done in the
> > kernel for it.
> I am trying to get up to speed with hidpp - but I am not there yet.
> Do you have any hints on what code to read specifically since
> logitech-hidpp is a bit long...

You have some public documentation at [1].

But from where you are now, you should probably be able to implement
the basic on/off feature by looking at the function 0x1000 in the
hid-logitech-hidpp code:
- you need define a few macros for your functionality (the class, the
commands, the events)
- you need to add a hook in connect_event to register the led class
device that will hook on to the actual LED of the device
- you need to also add a hook in hidpp_raw_hidpp_event for when a
button is emitted by the device.

Then you can either directly control the LED from the kernel, or let
userspace deal with it (and probably have some form of hybrid solution
where the kernel directly controls the LED, but also notifies user
space through regular buttons, but you can allow control of the LED
through the LED class).

Note that the kernel doesn't do a full enumeration of the device
features, so you need to register the device as a new class, in the
supported device ids.

>
> Thanks for the feedback so far
>

No worries.

Cheers,
Benjamin

[0] https://pwr-solaar.github.io/Solaar
[1] https://github.com/pwr-Solaar/Solaar/blob/master/docs/hidpp-documentation.txt

Re: Litra Glow on Linux

[Andreas Bergmeier]

Sorry, another set of questions - seems like I am a bit dense.

On Thu, 27 Oct 2022 at 11:44, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> It's just Logitech's common HID protocol. The advantage is that if
> Logitech reuses the feature on a different hardware, we won't have to
> implement anything new in the kernel.
Started implementing some illumination code but will take a while
until I figure out the driver I think.

> But from where you are now, you should probably be able to implement
> the basic on/off feature by looking at the function 0x1000 in the
> hid-logitech-hidpp code:
> - you need define a few macros for your functionality (the class, the
> commands, the events)
So my approach would be to identify the GLOW device and then at some
later point create the
illumination state and from there only handle common illumination.

> - you need to add a hook in connect_event to register the led class
> device that will hook on to the actual LED of the device
I did read all the LED specs/headers that I could find and from what I
have seen all you can currently do with this interface is control
brightness. There seems to be no way of controlling the Color
temperature, though.
So either this then would have to be exposed as a special device or
get handled entirely in userspace.
The latter seems to work against "implementing illumination handling
once in driver and reusing it".

> [0] https://pwr-solaar.github.io/Solaar
> [1] https://github.com/pwr-Solaar/Solaar/blob/master/docs/hidpp-documentation.txt
Thanks. Never would have found the specs on my own.
That said - I read x1990 spec and tried to access getIllumination from
userspace.
The spec seems a bit vague for my limited experience level.
For example I have not yet figured out what the communication (bytes)
difference between _getIllumination()_ and _illuminationChangedEvent_
is.
What seems to work is accessing events:

So I tried:
```c

#define LONG_REPORT_ID 0x11

    struct hiddev_usage_ref_multi multi;
    memset(&multi, 0, sizeof(multi));
    multi.uref.report_type = HID_REPORT_TYPE_INPUT;
    multi.uref.report_id = LONG_REPORT_ID;
    multi.uref.field_index = 0x0;
    multi.uref.usage_index = 0x03;
    multi.uref.usage_code = 0xff430002;
    multi.num_values = 1;

    if (ioctl(fd, HIDIOCGUSAGES, &multi) < 0)
    {
        perror("HIDIOCGUSAGES getIllumination");
        return -11;
    }

    printf("VALUE: %0x\n", multi.values[0]);

```
Which seems to report the illumination state until I press another
hardware button. So this seems to access the last event, which seems
to be _illuminationChangedEvent_ in my case.
No idea currently how to get _getIllumination_ to work.

Cheers
Andreas

Re: Litra Glow on Linux

[Benjamin Tissoires]

[adding linux-leds in Cc]

On Sat, Oct 29, 2022 at 9:21 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> Sorry, another set of questions - seems like I am a bit dense.
>
> On Thu, 27 Oct 2022 at 11:44, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > It's just Logitech's common HID protocol. The advantage is that if
> > Logitech reuses the feature on a different hardware, we won't have to
> > implement anything new in the kernel.
> Started implementing some illumination code but will take a while
> until I figure out the driver I think.
>
> > But from where you are now, you should probably be able to implement
> > the basic on/off feature by looking at the function 0x1000 in the
> > hid-logitech-hidpp code:
> > - you need define a few macros for your functionality (the class, the
> > commands, the events)
> So my approach would be to identify the GLOW device and then at some
> later point create the
> illumination state and from there only handle common illumination.

For identifying the GLOW device you should be adding an id in the
table of hid-logitech-hidpp, with a driver data that tells the driver
to look for 0x1990.

>
> > - you need to add a hook in connect_event to register the led class
> > device that will hook on to the actual LED of the device
> I did read all the LED specs/headers that I could find and from what I
> have seen all you can currently do with this interface is control
> brightness. There seems to be no way of controlling the Color
> temperature, though.

Leds can be multicolor. See drivers/hid/hid-playstation.c for an example.

So I think you should be able to give a color to the LED that can be
controlled as a separate channel, different from the brightness. The
LEDs folks will know better.

> So either this then would have to be exposed as a special device or
> get handled entirely in userspace.
> The latter seems to work against "implementing illumination handling
> once in driver and reusing it".

I would rather have it handled as a standard LED class, a colored LED
one. There might be a special structure for colored LEDs, and if not
you should probably be able to use a multi-color led with just one
color channel.

>
> > [0] https://pwr-solaar.github.io/Solaar
> > [1] https://github.com/pwr-Solaar/Solaar/blob/master/docs/hidpp-documentation.txt
> Thanks. Never would have found the specs on my own.
> That said - I read x1990 spec and tried to access getIllumination from
> userspace.

Oh, good point, Nestor added that spec back in May :)
Thanks, Nestor!

> The spec seems a bit vague for my limited experience level.
> For example I have not yet figured out what the communication (bytes)
> difference between _getIllumination()_ and _illuminationChangedEvent_
> is.
> What seems to work is accessing events:
>
> So I tried:
> ```c
>
> #define LONG_REPORT_ID 0x11
>
>     struct hiddev_usage_ref_multi multi;
>     memset(&multi, 0, sizeof(multi));
>     multi.uref.report_type = HID_REPORT_TYPE_INPUT;
>     multi.uref.report_id = LONG_REPORT_ID;
>     multi.uref.field_index = 0x0;
>     multi.uref.usage_index = 0x03;
>     multi.uref.usage_code = 0xff430002;
>     multi.num_values = 1;
>
>     if (ioctl(fd, HIDIOCGUSAGES, &multi) < 0)
>     {
>         perror("HIDIOCGUSAGES getIllumination");
>         return -11;
>     }
>
>     printf("VALUE: %0x\n", multi.values[0]);
>
> ```
> Which seems to report the illumination state until I press another
> hardware button. So this seems to access the last event, which seems
> to be _illuminationChangedEvent_ in my case.
> No idea currently how to get _getIllumination_ to work.

IIRC, HIDIOCGUSAGES doesn't do anything with the device, you are
querying the kernel of its current state. Which explains why you are
not getting the current state, but the previous known state.

What you need to do, is actually emit a command to the device
(completely untested, of course):
---
#define APPLICATION_ID 0x06 // can be anything between 0x01  and 0xF,
0x1 being the ID from the kernel

#define HIDPP_1990_GET_ILLUMINATION 0x00
#define HIDPP_1990_SET_ILLUMINATION 0x10
#define ....

unsigned char cmd = HIDPP_1990_GET_ILLUMINATION | APPLICATION_ID;
unsigned char buf[20] = {0x11, 0xff, 0x04}; // HIDPP ID, DEVICE INDEX,
FEATURE INDEX in the feature table

buf[3] = cmd;
write(fd, buf, sizeof(buf));
memset(buf, sizeof(buf), 0);
while (buf[3] != cmd)
  read(fd, buf, sizeof(buf));

printf("VALUE: %0x\n", buf[4]);
---

It is important to set an application id so you can differentiate your
requests from the events (the application ID from an event is always
0).

With that in mind, you should now be able to understand why you had to
send {0x11, 0xff, 0x04, 0x10, 0x01} to set the illumination. And
again, for being sure you can get the feedback you should use 0x16 as
the fourth byte here, so you can detect your answer.

I hope it makes more sense now.

Cheers,
Benjamin

Re: Litra Glow on Linux

[Andreas Bergmeier]

See my first stab at the problem below.

On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Sat, Oct 29, 2022 at 9:21 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
> >
> > Sorry, another set of questions - seems like I am a bit dense.
> >
> > On Thu, 27 Oct 2022 at 11:44, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > It's just Logitech's common HID protocol. The advantage is that if
> > > Logitech reuses the feature on a different hardware, we won't have to
> > > implement anything new in the kernel.
> > Started implementing some illumination code but will take a while
> > until I figure out the driver I think.
> >
> > > But from where you are now, you should probably be able to implement
> > > the basic on/off feature by looking at the function 0x1000 in the
> > > hid-logitech-hidpp code:
> > > - you need define a few macros for your functionality (the class, the
> > > commands, the events)
> > So my approach would be to identify the GLOW device and then at some
> > later point create the
> > illumination state and from there only handle common illumination.
>
> For identifying the GLOW device you should be adding an id in the
> table of hid-logitech-hidpp, with a driver data that tells the driver
> to look for 0x1990.
Currently compiles and should have all handling that seemed necessary to me.
Will now try to get the compiled kernel running on an Ubuntu - that
might take a while.
RFC:
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 71a9c258a20b..bdc2949d1be5 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -11,6 +11,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/device.h>
+#include <linux/dmi.h>
#include <linux/input.h>
#include <linux/usb.h>
#include <linux/hid.h>
@@ -99,6 +100,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL BIT(7)
#define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL BIT(8)
#define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL BIT(9)
+#define HIDPP_CAPABILITY_ILLUMINATION_LIGHT BIT(10)
#define lg_map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max,
EV_KEY, (c))
@@ -206,7 +208,10 @@ struct hidpp_device {
struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
- u8 wireless_feature_index;
+ union {
+ u8 wireless_feature_index;
+ u8 illumination_feature_index;
+ };
};
/* HID++ 1.0 error codes */
@@ -862,6 +867,8 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
#define CMD_ROOT_GET_FEATURE 0x00
#define CMD_ROOT_GET_PROTOCOL_VERSION 0x10
+#define HIDPP_FEATURE_TYPE_HIDDEN 0x70
+
static int hidpp_root_get_feature(struct hidpp_device *hidpp, u16 feature,
u8 *feature_index, u8 *feature_type)
{
@@ -1729,6 +1736,294 @@ static int
hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
return ret;
}
+/* --------------------------------------------------------------------------
*/
+/* 0x1990: Illumination Light */
+/* --------------------------------------------------------------------------
*/
+
+#define HIDPP_PAGE_ILLUMINATION_LIGHT 0x1990
+
+#define HIDPP_ILLUMINATION_FUNC_GET 0x00
+#define HIDPP_ILLUMINATION_FUNC_SET 0x10
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_INFO 0x20
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS 0x30
+#define HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS 0x40
+
+/* Not yet supported
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_LEVELS 0x50
+#define HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS_LEVELS 0x60
+*/
+
+#define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_INFO 0x70
+#define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE 0x80
+#define HIDPP_ILLUMINATION_FUNC_SET_COLOR_TEMPERATURE 0x90
+
+#define HIDPP_ILLUMINATION_EVENT_CHANGE 0x00
+#define HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE 0x10
+#define HIDPP_ILLUMINATION_EVENT_COLOR_TEMPERATURE_CHANGE 0x20
+
+#define HIDPP_ILLUMINATION_CAP_NON_LINEAR_LEVELS 0x04
+#define HIDPP_ILLUMINATION_CAP_LINEAR_LEVELS 0x02
+#define HIDPP_ILLUMINATION_CAP_EVENTS 0x01
+
+struct led_data {
+ struct led_classdev cdev;
+ struct hidpp_device *drv_data;
+ struct hid_device *hdev;
+ struct work_struct work;
+ u16 feature_index;
+ bool on;
+ unsigned int brightness;
+ unsigned int color_temperature;
+ bool removed;
+};
+
+static enum led_brightness led_brightness_get(struct led_classdev *led_cdev) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+
+ if (!led->on) {
+ return LED_OFF;
+ }
+
+ return led->brightness + 1;
+}
+
+static void led_brightness_set(struct led_classdev *led_cdev, enum
led_brightness brightness) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+ led->on = brightness != 0;
+ if (led->on) {
+ led->brightness = brightness - 1;
+ }
+
+ schedule_work(&led->work);
+}
+
+static unsigned int led_color_temperature_get(struct led_classdev *led_cdev) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+ return led->color_temperature;
+}
+
+static void led_color_temperature_set(struct led_classdev *led_cdev,
unsigned int color_temperature) {
+ struct led_data *led = container_of(led_cdev, struct led_data,
+ cdev);
+
+ led->color_temperature = color_temperature;
+ schedule_work(&led->work);
+}
+
+static void led_work(struct work_struct *work)
+{
+ struct hidpp_device *hidpp;
+ u8 params[20];
+ int ret;
+ struct hidpp_report report;
+ struct led_data *led = container_of(work, struct led_data, work);
+
+ if (led->removed)
+ return;
+
+ hidpp = led->drv_data;
+
+ memset(&params, 0, sizeof(params)/sizeof(*params));
+ if (!led->on) {
+ ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index, HIDPP_ILLUMINATION_FUNC_SET,
params, 20, &report);
+ if (ret) {
+ return;
+ }
+ }
+
+ ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index,
HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS, params, 20, &report);
+ if (ret) {
+ return;
+ }
+
+ ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index,
HIDPP_ILLUMINATION_FUNC_SET_COLOR_TEMPERATURE, params, 20, &report);
+ if (ret) {
+ return;
+ }
+}
+
+struct control_info{
+ u16 min;
+ u16 max;
+ u16 res;
+ u8 capabilities;
+ u8 max_levels;
+};
+
+static int get_brightness_info_sync(struct hidpp_device *hidpp,
struct control_info* info) {
+ struct hidpp_report resp;
+ int ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index,
HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_INFO, NULL, 0, &resp);
+ if (ret)
+ return ret;
+
+ info->capabilities = resp.fap.params[0];
+ info->min = be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2] << 0);
+ info->max = be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4] << 0);
+ info->res = be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6] << 0);
+ info->max_levels = resp.fap.params[7];
+ return 0;
+}
+
+static int get_color_temperature_info_sync(struct hidpp_device
*hidpp, struct control_info* info) {
+ struct hidpp_report resp;
+ int ret = hidpp_send_fap_command_sync(hidpp,
hidpp->illumination_feature_index,
HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_INFO, NULL, 0, &resp);
+ if (ret)
+ return ret;
+
+ info->capabilities = resp.fap.params[0];
+ info->min = be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2] << 0);
+ info->max = be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4] << 0);
+ info->res = be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6] << 0);
+ info->max_levels = resp.fap.params[7];
+ return 0;
+}
+
+static int register_led(struct hidpp_device *hidpp)
+{
+ struct control_info bi, cti;
+ struct led_data *ld;
+ int ret = get_brightness_info_sync(hidpp, &bi);
+ if (ret)
+ return ret;
+
+ if (bi.res != 1) {
+ // FAIL - not supported
+ return -1;
+ }
+
+ ret = get_color_temperature_info_sync(hidpp, &cti);
+ if (ret)
+ return ret;
+
+ if (cti.res != 1) {
+ // FAIL - not supported
+ return -1;
+ }
+
+ ld = devm_kzalloc(&hidpp->hid_dev->dev,
+ sizeof(struct led_data),
+ GFP_KERNEL);
+ if (!ld)
+ return -ENOMEM;
+
+ ld->drv_data = hidpp;
+ ld->removed = false;
+ ld->cdev.name = hidpp->name;
+ ld->cdev.flags = LED_HW_PLUGGABLE | LED_BRIGHT_HW_CHANGED |
LED_COLOR_TEMP_HW_CHANGED;
+ /* kernel led interface designates 0 as off. To not lose the ability to chose
+ * minimal brightness, we thus need to increase the reported range by 1
+ */
+ ld->cdev.max_brightness = bi.max - bi.min + 1;
+ if (bi.max == bi.min) {
+ /* According to docs set value is not supported under these
+ * conditions
+ */
+ ld->cdev.brightness_set = NULL;
+ } else {
+ ld->cdev.brightness_set = led_brightness_set;
+ }
+ ld->cdev.brightness_get = led_brightness_get;
+
+ ld->cdev.min_color_temperature = cti.min;
+ ld->cdev.max_color_temperature = cti.max;
+ if (bi.max == bi.min) {
+ /* According to docs set value is not supported under these
+ * conditions
+ */
+ ld->cdev.color_temperature_set = NULL;
+ } else {
+ ld->cdev.color_temperature_set = led_color_temperature_set;
+ }
+ ld->cdev.color_temperature_get = led_color_temperature_get;
+ INIT_WORK(&ld->work, led_work);
+ ret = devm_led_classdev_register(&hidpp->hid_dev->dev, &ld->cdev);
+ if (ret < 0) {
+ /* No need to have this still around */
+ devm_kfree(&hidpp->hid_dev->dev, ld);
+ }
+ hidpp->private_data = ld;
+ return 0;
+}
+
+/* TODO: Do we need this
+static int unregister_led(struct hidpp_device *hidpp,) {
+ devm_led_classdev_unregister(&hdev->dev, &ld->cdev);
+}
+*/
+
+static int hidpp20_illumination_raw_event(struct hidpp_device *hidpp,
u8 *data, int size) {
+
+ struct led_data *led;
+ struct hidpp_report *report = (struct hidpp_report *)data;
+ switch (report->report_id) {
+ case REPORT_ID_HIDPP_LONG:
+ /* size is already checked in hidpp_raw_event.
+ * only leave long through
+ */
+ break;
+ default:
+ return 0;
+ }
+
+ if (report->fap.feature_index != hidpp->illumination_feature_index) {
+ return 0;
+ }
+
+ led = (struct led_data*)hidpp->private_data;
+
+ if (report->fap.funcindex_clientid == HIDPP_ILLUMINATION_EVENT_CHANGE) {
+ led->on = report->fap.params[0] & 0x1;
+ if (led->on )
+ led_classdev_notify_brightness_hw_changed(&led->cdev, led->brightness + 1);
+ else
+ led_classdev_notify_brightness_hw_changed(&led->cdev, LED_OFF);
+ return 0;
+ }
+
+ if (report->fap.funcindex_clientid ==
HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE) {
+ u16 brightness = be16_to_cpu(report->fap.params[0] << 8 |
report->fap.params[1] << 0);
+ led->brightness = brightness;
+ led_classdev_notify_brightness_hw_changed(&led->cdev, led->brightness + 1);
+ return 0;
+ }
+
+ if (report->fap.funcindex_clientid ==
HIDPP_ILLUMINATION_EVENT_COLOR_TEMPERATURE_CHANGE) {
+ u16 color_temperature = be16_to_cpu(report->fap.params[0] << 8 |
report->fap.params[1] << 0);
+ led->color_temperature = color_temperature;
+ led_classdev_notify_color_temperature_hw_changed(&led->cdev,
led->color_temperature);
+ return 0;
+ }
+
+ return 0;
+}
+
+static int hidpp20_illumination_connect(struct hidpp_device *hidpp) {
+
+ u8 feature_index, feature_type;
+ int ret = hidpp_root_get_feature(hidpp,
+ HIDPP_PAGE_ILLUMINATION_LIGHT,
+ &feature_index,
+ &feature_type);
+ if (ret)
+ return ret;
+
+ if (feature_type & HIDPP_FEATURE_TYPE_HIDDEN) {
+ /* According to docs the host should ignore this feature */
+ return -ENOENT;
+ }
+
+ hidpp->illumination_feature_index = feature_index;
+
+ ret = register_led(hidpp);
+ if (!ret)
+ hidpp->capabilities |= HIDPP_CAPABILITY_ILLUMINATION_LIGHT;
+
+ return ret;
+}
+
/* -------------------------------------------------------------------------- */
/* 0x2120: Hi-resolution scrolling */
/* -------------------------------------------------------------------------- */
@@ -3648,6 +3943,12 @@ static int hidpp_raw_hidpp_event(struct
hidpp_device *hidpp, u8 *data,
return ret;
}
+ if (hidpp->capabilities & HIDPP_CAPABILITY_ILLUMINATION_LIGHT) {
+ ret = hidpp20_illumination_raw_event(hidpp, data, size);
+ if (ret != 0)
+ return ret;
+ }
+
if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
ret = hidpp10_wheel_raw_event(hidpp, data, size);
if (ret != 0)
@@ -4013,6 +4314,8 @@ static void hidpp_connect_event(struct
hidpp_device *hidpp)
}
hidpp->delayed_input = input;
+
+ ret = hidpp20_illumination_connect(hidpp);
}
static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);


>
> >
> > > - you need to add a hook in connect_event to register the led class
> > > device that will hook on to the actual LED of the device
> > I did read all the LED specs/headers that I could find and from what I
> > have seen all you can currently do with this interface is control
> > brightness. There seems to be no way of controlling the Color
> > temperature, though.
>
> Leds can be multicolor. See drivers/hid/hid-playstation.c for an example.
>
> So I think you should be able to give a color to the LED that can be
> controlled as a separate channel, different from the brightness. The
> LEDs folks will know better.
>
> > So either this then would have to be exposed as a special device or
> > get handled entirely in userspace.
> > The latter seems to work against "implementing illumination handling
> > once in driver and reusing it".
>
> I would rather have it handled as a standard LED class, a colored LED
> one. There might be a special structure for colored LEDs, and if not
> you should probably be able to use a multi-color led with just one
> color channel.
>
> >
> > > [0] https://pwr-solaar.github.io/Solaar
> > > [1] https://github.com/pwr-Solaar/Solaar/blob/master/docs/hidpp-documentation.txt
> > Thanks. Never would have found the specs on my own.
> > That said - I read x1990 spec and tried to access getIllumination from
> > userspace.
>
> Oh, good point, Nestor added that spec back in May :)
> Thanks, Nestor!
>
> > The spec seems a bit vague for my limited experience level.
> > For example I have not yet figured out what the communication (bytes)
> > difference between _getIllumination()_ and _illuminationChangedEvent_
> > is.
> > What seems to work is accessing events:
> >
> > So I tried:
> > ```c
> >
> > #define LONG_REPORT_ID 0x11
> >
> >     struct hiddev_usage_ref_multi multi;
> >     memset(&multi, 0, sizeof(multi));
> >     multi.uref.report_type = HID_REPORT_TYPE_INPUT;
> >     multi.uref.report_id = LONG_REPORT_ID;
> >     multi.uref.field_index = 0x0;
> >     multi.uref.usage_index = 0x03;
> >     multi.uref.usage_code = 0xff430002;
> >     multi.num_values = 1;
> >
> >     if (ioctl(fd, HIDIOCGUSAGES, &multi) < 0)
> >     {
> >         perror("HIDIOCGUSAGES getIllumination");
> >         return -11;
> >     }
> >
> >     printf("VALUE: %0x\n", multi.values[0]);
> >
> > ```
> > Which seems to report the illumination state until I press another
> > hardware button. So this seems to access the last event, which seems
> > to be _illuminationChangedEvent_ in my case.
> > No idea currently how to get _getIllumination_ to work.
>
> IIRC, HIDIOCGUSAGES doesn't do anything with the device, you are
> querying the kernel of its current state. Which explains why you are
> not getting the current state, but the previous known state.
>
> What you need to do, is actually emit a command to the device
> (completely untested, of course):
> ---
> #define APPLICATION_ID 0x06 // can be anything between 0x01  and 0xF,
> 0x1 being the ID from the kernel
>
> #define HIDPP_1990_GET_ILLUMINATION 0x00
> #define HIDPP_1990_SET_ILLUMINATION 0x10
> #define ....
>
> unsigned char cmd = HIDPP_1990_GET_ILLUMINATION | APPLICATION_ID;
> unsigned char buf[20] = {0x11, 0xff, 0x04}; // HIDPP ID, DEVICE INDEX,
> FEATURE INDEX in the feature table
>
> buf[3] = cmd;
> write(fd, buf, sizeof(buf));
> memset(buf, sizeof(buf), 0);
> while (buf[3] != cmd)
>   read(fd, buf, sizeof(buf));
>
> printf("VALUE: %0x\n", buf[4]);
> ---
>
> It is important to set an application id so you can differentiate your
> requests from the events (the application ID from an event is always
> 0).
>
> With that in mind, you should now be able to understand why you had to
> send {0x11, 0xff, 0x04, 0x10, 0x01} to set the illumination. And
> again, for being sure you can get the feedback you should use 0x16 as
> the fourth byte here, so you can detect your answer.
>
> I hope it makes more sense now.
>
> Cheers,
> Benjamin
>

Re: Litra Glow on Linux

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Hi!

> See my first stab at the problem below.

> index 71a9c258a20b..bdc2949d1be5 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -11,6 +11,7 @@
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> #include <linux/device.h>
> +#include <linux/dmi.h>
> #include <linux/input.h>
> #include <linux/usb.h>
> #include <linux/hid.h>

> @@ -206,7 +208,10 @@ struct hidpp_device {
> struct hidpp_battery battery;
> struct hidpp_scroll_counter vertical_wheel_counter;
> - u8 wireless_feature_index;
> + union {
> + u8 wireless_feature_index;
> + u8 illumination_feature_index;
> + };
> };
> /* HID++ 1.0 error codes */

Something very wrong is going on with your email -- all spaces are
deleted or something. You'll need to adjust your mail settings.

Best regards,
								Pavel
								
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Litra Glow on Linux

[Andreas Bergmeier]

Finally I have an environment where I can test my kernel code.

On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> For identifying the GLOW device you should be adding an id in the
> table of hid-logitech-hidpp, with a driver data that tells the driver
> to look for 0x1990.
>
> >
> > > - you need to add a hook in connect_event to register the led class
> > > device that will hook on to the actual LED of the device
Sadly my tests did not go very far. The code fails already when
calling the `probe` callback (`hidpp_probe`).
When it calls into `hidpp_root_get_protocol_version` it seems to
receive `HIDPP_ERROR_RESOURCE_ERROR`.
Which then leads to an error message: Device not connected
Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
documentation what it means in code.
From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
devices. Thus I am wondering how the code knows
that it is a problem with connectivity. Couldn't it also mean that the
device is not supporting getting the protocol version?
And why is protocol version only enforced for non unifying devices?

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> Finally I have an environment where I can test my kernel code.
>
> On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > For identifying the GLOW device you should be adding an id in the
> > table of hid-logitech-hidpp, with a driver data that tells the driver
> > to look for 0x1990.
> >
> > >
> > > > - you need to add a hook in connect_event to register the led class
> > > > device that will hook on to the actual LED of the device
> Sadly my tests did not go very far. The code fails already when
> calling the `probe` callback (`hidpp_probe`).
> When it calls into `hidpp_root_get_protocol_version` it seems to
> receive `HIDPP_ERROR_RESOURCE_ERROR`.
> Which then leads to an error message: Device not connected
> Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> documentation what it means in code.
> From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> devices. Thus I am wondering how the code knows
> that it is a problem with connectivity. Couldn't it also mean that the
> device is not supporting getting the protocol version?
> And why is protocol version only enforced for non unifying devices?
Also, looking into `supported_reports` turned out to be 2 (very long).
Inside of `hidpp_root_get_protocol_version` it does upgrade SHORT to
LONG if the former is not supported.
On a whim I then added upgrade of LONG to VERY LONG if the former is
not supported. Sadly, the results stayed the same.

Re: Litra Glow on Linux

[Benjamin Tissoires]

On Thu, Nov 10, 2022 at 4:29 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
> On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
> >
> > Finally I have an environment where I can test my kernel code.
> >
> > On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > For identifying the GLOW device you should be adding an id in the
> > > table of hid-logitech-hidpp, with a driver data that tells the driver
> > > to look for 0x1990.
> > >
> > > >
> > > > > - you need to add a hook in connect_event to register the led class
> > > > > device that will hook on to the actual LED of the device
> > Sadly my tests did not go very far. The code fails already when
> > calling the `probe` callback (`hidpp_probe`).
> > When it calls into `hidpp_root_get_protocol_version` it seems to
> > receive `HIDPP_ERROR_RESOURCE_ERROR`.
> > Which then leads to an error message: Device not connected
> > Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> > documentation what it means in code.
> > From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> > devices. Thus I am wondering how the code knows
> > that it is a problem with connectivity.

From the top of my memory, this was told to us that this is the way we
detect if the device was connected or not in the unifying case. Though
in your case, it's a USB device, so there is no such thing as "not
connected"...

> > Couldn't it also mean that the
> > device is not supporting getting the protocol version?

Probably. What happens if you comment out that protocol version
request and force connected to be true?

> > And why is protocol version only enforced for non unifying devices?

Unifying devices are wireless, and when we probe the device, we are
actually talking to the receiver. So The device might not be
connected, and we should wait for the device to be present and not
reject it. On non unifying devices, if the device is not connected,
this likely means that the device is not behaving properly, and so we
can not handle it in the driver.

In your case though, it would be interesting to know if we should
bypass that verification.

> Also, looking into `supported_reports` turned out to be 2 (very long).

Oops, you mistook the bit definition with the value:
#define HIDPP_REPORT_SHORT_SUPPORTED  BIT(0)  -> value of 1
#define HIDPP_REPORT_LONG_SUPPORTED  BIT(1)  -> value of 2
#define HIDPP_REPORT_VERY_LONG_SUPPORTED  BIT(2)  -> value of 4

Which is coherent with what your device exports: only one report ID of
value 0x11, HIDPP_REPORT_LONG.

> Inside of `hidpp_root_get_protocol_version` it does upgrade SHORT to
> LONG if the former is not supported.

Yep, this should be good for your device.

> On a whim I then added upgrade of LONG to VERY LONG if the former is
> not supported. Sadly, the results stayed the same.
>

And this is expected because you don't have VERY_LONG support on your device.

Cheers,
Benjamin

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Thu, 10 Nov 2022, Benjamin Tissoires wrote:

> On Thu, Nov 10, 2022 at 4:29 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
> >
> > On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
> > >
> > > Finally I have an environment where I can test my kernel code.
> > >
> > > On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > > For identifying the GLOW device you should be adding an id in the
> > > > table of hid-logitech-hidpp, with a driver data that tells the driver
> > > > to look for 0x1990.
> > > >
> > > > >
> > > > > > - you need to add a hook in connect_event to register the led class
> > > > > > device that will hook on to the actual LED of the device
> > > Sadly my tests did not go very far. The code fails already when
> > > calling the `probe` callback (`hidpp_probe`).
> > > When it calls into `hidpp_root_get_protocol_version` it seems to
> > > receive `HIDPP_ERROR_RESOURCE_ERROR`.
> > > Which then leads to an error message: Device not connected
> > > Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> > > documentation what it means in code.
> > > From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> > > devices. Thus I am wondering how the code knows
> > > that it is a problem with connectivity.
>
> From the top of my memory, this was told to us that this is the way we
> detect if the device was connected or not in the unifying case. Though
> in your case, it's a USB device, so there is no such thing as "not
> connected"...
So isn't the current error handling at a minimum misleading?


> > > Couldn't it also mean that the
> > > device is not supporting getting the protocol version?
>
> Probably. What happens if you comment out that protocol version
> request and force connected to be true?
Well I went the other way around. I had a look at the hidpp utility
sources:
https://github.com/cvuchener/hidpp/blob/057407fbb7248bbc6cefcfaa860758d0711c01b9/src/libhidpp/hidpp/Device.cpp#L82
Which seems to do a similar thing. From the top of my head the only
difference seems to be that they are sending `0x1` as a ping value instead
of `0x5a`. Might give that a shot next.
Anyway hidpp-list-features successfully reads the protocol version in
userspace (4, 2) as seen here:
https://github.com/abergmeier/litra_glow_linux/blob/main/hidpp-list-features

> In your case though, it would be interesting to know if we should
> bypass that verification.
Since reading the protocol version seems generally possible I think we
should understand what logitech-hidpp does wrong first.


> > Also, looking into `supported_reports` turned out to be 2 (very long).
>
> Oops, you mistook the bit definition with the value:
> #define HIDPP_REPORT_SHORT_SUPPORTED  BIT(0)  -> value of 1
> #define HIDPP_REPORT_LONG_SUPPORTED  BIT(1)  -> value of 2
> #define HIDPP_REPORT_VERY_LONG_SUPPORTED  BIT(2)  -> value of 4
Ah indeed, thx.

> And this is expected because you don't have VERY_LONG support on your device.
True. The question remains whether the upgrade from LONG to VERY_LONG
could be needed should a device only support VERY_LONG.

Re: Litra Glow on Linux

[Benjamin Tissoires]

On Thu, Nov 10, 2022 at 1:24 PM Andreas Bergmeier <abergmeier@gmx.net> wrote:
>
>
>
> On Thu, 10 Nov 2022, Benjamin Tissoires wrote:
>
> > On Thu, Nov 10, 2022 at 4:29 AM Andreas Bergmeier <abergmeier@gmx.net> wrote:
> > >
> > > On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@gmx.net> wrote:
> > > >
> > > > Finally I have an environment where I can test my kernel code.
> > > >
> > > > On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires
> > > > <benjamin.tissoires@redhat.com> wrote:
> > > > > For identifying the GLOW device you should be adding an id in the
> > > > > table of hid-logitech-hidpp, with a driver data that tells the driver
> > > > > to look for 0x1990.
> > > > >
> > > > > >
> > > > > > > - you need to add a hook in connect_event to register the led class
> > > > > > > device that will hook on to the actual LED of the device
> > > > Sadly my tests did not go very far. The code fails already when
> > > > calling the `probe` callback (`hidpp_probe`).
> > > > When it calls into `hidpp_root_get_protocol_version` it seems to
> > > > receive `HIDPP_ERROR_RESOURCE_ERROR`.
> > > > Which then leads to an error message: Device not connected
> > > > Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no
> > > > documentation what it means in code.
> > > > From a look into the docs it says that 9 is UNSUPPORTED error for 2.0
> > > > devices. Thus I am wondering how the code knows
> > > > that it is a problem with connectivity.
> >
> > From the top of my memory, this was told to us that this is the way we
> > detect if the device was connected or not in the unifying case. Though
> > in your case, it's a USB device, so there is no such thing as "not
> > connected"...
> So isn't the current error handling at a minimum misleading?
>

Maybe, but this is the first time we have that error on a non wireless
device... so it is not for the supported devices :)

>
> > > > Couldn't it also mean that the
> > > > device is not supporting getting the protocol version?
> >
> > Probably. What happens if you comment out that protocol version
> > request and force connected to be true?
> Well I went the other way around. I had a look at the hidpp utility
> sources:
> https://github.com/cvuchener/hidpp/blob/057407fbb7248bbc6cefcfaa860758d0711c01b9/src/libhidpp/hidpp/Device.cpp#L82
> Which seems to do a similar thing. From the top of my head the only
> difference seems to be that they are sending `0x1` as a ping value instead
> of `0x5a`. Might give that a shot next.
> Anyway hidpp-list-features successfully reads the protocol version in
> userspace (4, 2) as seen here:
> https://github.com/abergmeier/litra_glow_linux/blob/main/hidpp-list-features

Hmm... It would seem wrong for me if the firmware suddenly expects to
have a specific ping value.
If it works in userspace, it might also mean that the timing is not
right and we are talking to the device too early, and it can't answer
yet.

>
> > In your case though, it would be interesting to know if we should
> > bypass that verification.
> Since reading the protocol version seems generally possible I think we
> should understand what logitech-hidpp does wrong first.
>
>
> > > Also, looking into `supported_reports` turned out to be 2 (very long).
> >
> > Oops, you mistook the bit definition with the value:
> > #define HIDPP_REPORT_SHORT_SUPPORTED  BIT(0)  -> value of 1
> > #define HIDPP_REPORT_LONG_SUPPORTED  BIT(1)  -> value of 2
> > #define HIDPP_REPORT_VERY_LONG_SUPPORTED  BIT(2)  -> value of 4
> Ah indeed, thx.
>
> > And this is expected because you don't have VERY_LONG support on your device.
> True. The question remains whether the upgrade from LONG to VERY_LONG
> could be needed should a device only support VERY_LONG.
>

I don't think we want that. AFAICT, devices supporting VERY_LONG are
not very common, and also the length of the report makes it not
convenient for the generic protocol to be used there. I am not in the
head of Logitech's engineers, but removing the 20 bytes long report in
favor of 64 bytes long seems a little bit overkill (the 7 bytes long
is a different story).

And given that we generally add manual support of new devices, I think
we are safe not implementing something we haven't seen in the wild.

Cheers,
Benjamin

Re: Litra Glow on Linux

[Andreas Bergmeier]

So after some tinkering I have code now that succeeds in retrieving state
via sending reports once. After that all following sends time out.
I am at a loss what I am doing wrong, tbh. RFC below.

On Thu, 10 Nov 2022, Benjamin Tissoires wrote:

>
> >
> > I had a look at the hidpp utility
> > sources:
> > https://github.com/cvuchener/hidpp/blob/057407fbb7248bbc6cefcfaa860758d0711c01b9/src/libhidpp/hidpp/Device.cpp#L82
> > Which seems to do a similar thing. From the top of my head the only
> > difference seems to be that they are sending `0x1` as a ping value instead
> > of `0x5a`. Might give that a shot next.
> > Anyway hidpp-list-features successfully reads the protocol version in
> > userspace (4, 2) as seen here:
> > https://github.com/abergmeier/litra_glow_linux/blob/main/hidpp-list-features
>
> Hmm... It would seem wrong for me if the firmware suddenly expects to
> have a specific ping value.
> If it works in userspace, it might also mean that the timing is not
> right and we are talking to the device too early, and it can't answer
> yet.
I needed to set some specific quirk flags to make communication work. See
below.

RFC on current PATCH:
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index dad953f66996..78265f7235ce 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -856,6 +856,7 @@
 #define USB_DEVICE_ID_MX5500_RECEIVER_MOUSE_DEV		0xc71c
 #define USB_DEVICE_ID_DINOVO_MINI_RECEIVER_KBD_DEV	0xc71e
 #define USB_DEVICE_ID_DINOVO_MINI_RECEIVER_MOUSE_DEV	0xc71f
+#define USB_DEVICE_ID_LOGITECH_LITRA_GLOW   0xc900
 #define USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2	0xca03
 #define USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL	0xca04

diff --git a/drivers/hid/hid-logitech-hidpp.c
b/drivers/hid/hid-logitech-hidpp.c
index 71a9c258a20b..949fd09d2b43 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -11,6 +11,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

 #include <linux/device.h>
+#include <linux/dmi.h>
 #include <linux/input.h>
 #include <linux/usb.h>
 #include <linux/hid.h>
@@ -99,6 +100,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL	BIT(7)
 #define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL	BIT(8)
 #define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL	BIT(9)
+#define HIDPP_CAPABILITY_ILLUMINATION_LIGHT		BIT(10)

 #define lg_map_key_clear(c)  hid_map_usage_clear(hi, usage, bit, max,
EV_KEY, (c))

@@ -206,7 +208,10 @@ struct hidpp_device {
 	struct hidpp_battery battery;
 	struct hidpp_scroll_counter vertical_wheel_counter;

-	u8 wireless_feature_index;
+	union {
+		u8 wireless_feature_index;
+		u8 illumination_feature_index;
+	};
 };

 /* HID++ 1.0 error codes */
@@ -355,15 +360,16 @@ static int hidpp_send_fap_command_sync(struct
hidpp_device *hidpp,
 }

 static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
-	u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int
param_count,
+	u8 sub_id, u8 reg_address, u8 *params, int param_count,
 	struct hidpp_report *response)
 {
 	struct hidpp_report *message;
 	int ret, max_count;
+	u8 report_id;

-	/* Send as long report if short reports are not supported. */
-	if (report_id == REPORT_ID_HIDPP_SHORT &&
-	    !(hidpp_dev->supported_reports &
HIDPP_REPORT_SHORT_SUPPORTED))
+	if (hidpp_dev->supported_reports & HIDPP_REPORT_SHORT_SUPPORTED)
+		report_id = REPORT_ID_HIDPP_SHORT;
+	else
 		report_id = REPORT_ID_HIDPP_LONG;

 	switch (report_id) {
@@ -544,7 +550,6 @@ static int hidpp10_set_register(struct hidpp_device
*hidpp_dev,
 	u8 params[3] = { 0 };

 	ret = hidpp_send_rap_command_sync(hidpp_dev,
-					  REPORT_ID_HIDPP_SHORT,
 					  HIDPP_GET_REGISTER,
 					  register_address,
 					  NULL, 0, &response);
@@ -557,7 +562,6 @@ static int hidpp10_set_register(struct hidpp_device
*hidpp_dev,
 	params[byte] |= value & mask;

 	return hidpp_send_rap_command_sync(hidpp_dev,
-					   REPORT_ID_HIDPP_SHORT,
 					   HIDPP_SET_REGISTER,
 					   register_address,
 					   params, 3, &response);
@@ -653,7 +657,6 @@ static int hidpp10_query_battery_status(struct
hidpp_device *hidpp)
 	int ret, status;

 	ret = hidpp_send_rap_command_sync(hidpp,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_REGISTER,
 					HIDPP_REG_BATTERY_STATUS,
 					NULL, 0, &response);
@@ -705,7 +708,6 @@ static int hidpp10_query_battery_mileage(struct
hidpp_device *hidpp)
 	int ret, status;

 	ret = hidpp_send_rap_command_sync(hidpp,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_REGISTER,
 					HIDPP_REG_BATTERY_MILEAGE,
 					NULL, 0, &response);
@@ -777,7 +779,6 @@ static char *hidpp_unifying_get_name(struct
hidpp_device *hidpp_dev)
 	int len;

 	ret = hidpp_send_rap_command_sync(hidpp_dev,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_LONG_REGISTER,
 					HIDPP_REG_PAIRING_INFORMATION,
 					params, 1, &response);
@@ -811,7 +812,6 @@ static int hidpp_unifying_get_serial(struct
hidpp_device *hidpp, u32 *serial)
 	u8 params[1] = { HIDPP_EXTENDED_PAIRING };

 	ret = hidpp_send_rap_command_sync(hidpp,
-					REPORT_ID_HIDPP_SHORT,
 					HIDPP_GET_LONG_REGISTER,
 					HIDPP_REG_PAIRING_INFORMATION,
 					params, 1, &response);
@@ -862,6 +862,8 @@ static int hidpp_unifying_init(struct hidpp_device
*hidpp)
 #define CMD_ROOT_GET_FEATURE				0x00
 #define CMD_ROOT_GET_PROTOCOL_VERSION			0x10

+#define HIDPP_FEATURE_TYPE_HIDDEN 0x70
+
 static int hidpp_root_get_feature(struct hidpp_device *hidpp, u16
feature,
 	u8 *feature_index, u8 *feature_type)
 {
@@ -893,9 +895,8 @@ static int hidpp_root_get_protocol_version(struct
hidpp_device *hidpp)
 	int ret;

 	ret = hidpp_send_rap_command_sync(hidpp,
-			REPORT_ID_HIDPP_SHORT,
 			HIDPP_PAGE_ROOT_IDX,
-			CMD_ROOT_GET_PROTOCOL_VERSION,
+			CMD_ROOT_GET_PROTOCOL_VERSION |
LINUX_KERNEL_SW_ID,
 			ping_data, sizeof(ping_data), &response);

 	if (ret == HIDPP_ERROR_INVALID_SUBID) {
@@ -1729,6 +1730,361 @@ static int hidpp_set_wireless_feature_index(struct
hidpp_device *hidpp)
 	return ret;
 }

+/*
--------------------------------------------------------------------------
*/
+/* 0x1990: Illumination Light
*/
+/*
--------------------------------------------------------------------------
*/
+
+#define HIDPP_PAGE_ILLUMINATION_LIGHT 0x1990
+
+#define HIDPP_ILLUMINATION_FUNC_GET 0x00
+#define HIDPP_ILLUMINATION_FUNC_SET 0x10
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_INFO 0x20
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS 0x30
+#define HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS 0x40
+
+/* Not yet supported
+#define HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_LEVELS 0x50
+#define HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS_LEVELS 0x60
+*/
+
+#define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_INFO 0x70
+#define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE 0x80
+#define HIDPP_ILLUMINATION_FUNC_SET_COLOR_TEMPERATURE 0x90
+
+/* Not yet supported
+#define HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_LEVELS 0xA0
+#define HIDPP_ILLUMINATION_FUNC_SET_COLOR_TEMPERATURE_LEVELS 0xB0
+*/
+
+#define HIDPP_ILLUMINATION_EVENT_CHANGE 0x00
+#define HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE 0x10
+#define HIDPP_ILLUMINATION_EVENT_COLOR_TEMPERATURE_CHANGE 0x20
+
+#define HIDPP_ILLUMINATION_CAP_EVENTS BIT(0)
+#define HIDPP_ILLUMINATION_CAP_LINEAR_LEVELS BIT(1)
+#define HIDPP_ILLUMINATION_CAP_NON_LINEAR_LEVELS BIT(2)
+
+struct control_info {
+	u16 min;
+	u16 max;
+	u16 res;
+	u8 capabilities;
+	u8 max_levels;
+};
+
+struct led_data {
+	struct led_classdev cdev;
+	struct hidpp_device *drv_data;
+	struct hid_device *hdev;
+	u16 feature_index;
+	bool on;
+	u16 brightness;
+	struct control_info brightness_info;
+	struct control_info color_temperature_info;
+	char dirname[256];
+	bool removed;
+};
+
+/* kernel led interface designates 0 as off. To not lose the ability to
chose
+ * minimal brightness, we thus need to increase the reported range by 1
+ */
+static unsigned device_to_led_brightness_value(struct led_data* led, u16
device_brightness) {
+	u16 relative = device_brightness - led->brightness_info.min;
+	u16 step = relative / led->brightness_info.res;
+	return step + 1;
+}
+
+static unsigned device_to_led_brightness(struct led_data* led) {
+	return device_to_led_brightness_value(led, led->brightness);
+}
+
+static u16 led_to_device_brightness_value(struct led_data* led, unsigned
led_brightness) {
+	unsigned step = led_brightness - 1;
+	unsigned relative = step * led->brightness_info.res;
+	return led->brightness_info.min + relative;
+}
+
+static enum led_brightness led_brightness_get(struct led_classdev
*led_cdev)
+{
+	struct led_data *led = container_of(led_cdev, struct led_data,
cdev);
+	struct hidpp_device *hidpp = led->drv_data;
+	u8 params[1] = { 0 };
+	struct hidpp_report report;
+	int ret;
+	u16 be_brightness;
+
+
+	ret = hidpp_send_fap_command_sync(hidpp,
+
hidpp->illumination_feature_index,
+					  HIDPP_ILLUMINATION_FUNC_GET,
params,
+					  0, &report);
+	if (ret) {
+		hid_err(hidpp->hid_dev, "Getting Illumination failed\n");
+		goto exit;
+	}
+
+
+	led->on = report.fap.params[0] & 0x01;
+	if (!led->on) {
+		return LED_OFF;
+	}
+
+	ret = hidpp_send_fap_command_sync(
+		hidpp, hidpp->illumination_feature_index,
+		HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS, params, 0,
&report);
+	if (ret) {
+		hid_err(hidpp->hid_dev,
+			"Getting Illumination Brightness failed\n");
+		goto exit;
+	}
+
+	be_brightness = (report.fap.params[0] << 8) |
+			(report.fap.params[1] << 0);
+	led->brightness = be16_to_cpu(be_brightness);
+exit:
+	return device_to_led_brightness(led);
+}
+
+static void led_brightness_set_dummy(struct led_classdev *led_cdev,
+			enum led_brightness brightness) {
+}
+
+static int led_brightness_set_sync(struct led_classdev *led_cdev,
+			       enum led_brightness brightness)
+{
+	struct led_data *led = container_of(led_cdev, struct led_data,
cdev);
+	struct hidpp_device *hidpp = led->drv_data;
+	u16 be_brightness;
+	struct hidpp_report report;
+	u8 params[2];
+	int params_count = sizeof(params) / sizeof(*params);
+	int ret;
+
+
+	be_brightness = cpu_to_be16(led->brightness);
+	led->on = brightness != 0;
+	if (led->on) {
+		led->brightness = led_to_device_brightness_value(led,
brightness);
+	}
+
+	memzero_explicit(params, params_count);
+	params[0] = led->on ? 0x01 : 0x00;
+	ret = hidpp_send_fap_command_sync(hidpp,
+
hidpp->illumination_feature_index,
+					  HIDPP_ILLUMINATION_FUNC_SET,
params,
+					  params_count, &report);
+	if (ret) {
+		hid_err(hidpp->hid_dev, "Setting Illumination failed\n");
+		return ret;
+	}
+
+	if (!led->on)
+		return 0;
+	params[0] = (be_brightness & 0xFF00) >> 8;
+	params[1] = (be_brightness & 0x00FF) >> 0;
+	ret = hidpp_send_fap_command_sync(
+		hidpp, hidpp->illumination_feature_index,
+		HIDPP_ILLUMINATION_FUNC_SET_BRIGHTNESS, params,
params_count,
+		&report);
+	if (ret) {
+		hid_err(hidpp->hid_dev,
+			"Setting Illumination Brightness failed\n");
+		return ret;
+	}
+	return ret;
+}
+
+static int get_brightness_info_sync(struct hidpp_device *hidpp,
+				    struct control_info *info)
+{
+	struct hidpp_report resp;
+	int ret = hidpp_send_fap_command_sync(
+		hidpp, hidpp->illumination_feature_index,
+		HIDPP_ILLUMINATION_FUNC_GET_BRIGHTNESS_INFO, NULL, 0,
&resp);
+	if (ret) {
+		hid_err(hidpp->hid_dev,
+			"get_brightness_info_sync failed with %d\n", ret);
+		return ret;
+	}
+
+	info->capabilities = resp.fap.params[0];
+	info->min =
+		be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2]
<< 0);
+	info->max =
+		be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4]
<< 0);
+	info->res =
+		be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6]
<< 0);
+	info->max_levels = resp.fap.params[7];
+	return 0;
+}
+
+static int get_color_temperature_info_sync(struct hidpp_device *hidpp,
+					   struct control_info *info)
+{
+	struct hidpp_report resp;
+	int ret = hidpp_send_fap_command_sync(
+		hidpp, hidpp->illumination_feature_index,
+		HIDPP_ILLUMINATION_FUNC_GET_COLOR_TEMPERATURE_INFO, NULL,
0,
+		&resp);
+	if (ret) {
+		hid_err(hidpp->hid_dev,
+			"get_color_temperature_info_sync failed with
%d\n",
+			ret);
+		return ret;
+	}
+
+	info->capabilities = resp.fap.params[0];
+	info->min =
+		be16_to_cpu(resp.fap.params[1] << 8 | resp.fap.params[2]
<< 0);
+	info->max =
+		be16_to_cpu(resp.fap.params[3] << 8 | resp.fap.params[4]
<< 0);
+	info->res =
+		be16_to_cpu(resp.fap.params[5] << 8 | resp.fap.params[6]
<< 0);
+	info->max_levels = resp.fap.params[7];
+	return 0;
+}
+
+static int register_led(struct hidpp_device *hidpp)
+{
+	char buf[256];
+	int ret;
+	unsigned brightness_range, r = 0, w = 0;
+	struct led_data *led = devm_kzalloc(&hidpp->hid_dev->dev,
sizeof(struct led_data),
+			  GFP_KERNEL);
+
+	if (!led)
+		return -ENOMEM;
+
+	ret = get_brightness_info_sync(hidpp, &led->brightness_info);
+	if (ret)
+		goto cleanup;
+
+	ret = get_color_temperature_info_sync(hidpp,
+
&led->color_temperature_info);
+	if (ret)
+		goto cleanup;
+
+	led->drv_data = hidpp;
+	led->removed = false;
+	memzero_explicit(buf, 256);
+	strscpy(buf, hidpp->name, 256);
+	while (w != 256) {
+		char c = buf[r];
+		if (c == '\'' || c == '\"') {
+			if (r != 255) {
+				r++;
+			}
+			continue;
+		}
+		buf[w] = buf[r];
+		w++;
+		if (r != 255) {
+			r++;
+		}
+	}
+	strreplace(buf, ' ', '_');
+	snprintf(led->dirname, sizeof(led->dirname) /
sizeof(*led->dirname),
+		 "%s::illumination", buf);
+	led->cdev.name = led->dirname;
+	led->cdev.flags = LED_HW_PLUGGABLE | LED_BRIGHT_HW_CHANGED;
+	led->cdev.max_brightness = device_to_led_brightness_value(led,
led->brightness_info.max);
+	if (brightness_range == 0) {
+		/* According to docs set value is not supported under
these
+		 * conditions.
+		 * LED interface enforces a set function.
+		 */
+		led->cdev.brightness_set = led_brightness_set_dummy;
+	} else {
+		led->cdev.brightness_set_blocking =
led_brightness_set_sync;
+	}
+	led->cdev.brightness_get = led_brightness_get;
+
+	ret = devm_led_classdev_register(&hidpp->hid_dev->dev,
&led->cdev);
+	if (ret < 0) {
+		goto cleanup;
+	}
+	hidpp->private_data = led;
+	return 0;
+cleanup:
+	devm_kfree(&hidpp->hid_dev->dev, led);
+	return ret;
+}
+
+static int hidpp_initialize_illumination(struct hidpp_device *hidpp)
+{
+	int ret;
+	unsigned long capabilities = hidpp->capabilities;
+
+	if (hidpp->protocol_major >= 2) {
+		u8 feature_index;
+		u8 feature_type;
+
+		ret = hidpp_root_get_feature(hidpp,
+
HIDPP_PAGE_ILLUMINATION_LIGHT,
+					     &feature_index,
&feature_type);
+		if (!ret && !(feature_type & HIDPP_FEATURE_TYPE_HIDDEN)) {
+			hidpp->capabilities |=
+				HIDPP_CAPABILITY_ILLUMINATION_LIGHT;
+			hidpp->illumination_feature_index = feature_index;
+			hid_dbg(hidpp->hid_dev,
+				"Detected HID++ 2.0 Illumination
Light\n");
+			return 0;
+		}
+	}
+
+	if (hidpp->capabilities == capabilities)
+		hid_dbg(hidpp->hid_dev,
+			"Did not detect HID++ Illumination Light hardware
support\n");
+	return 0;
+}
+
+static int hidpp20_illumination_raw_event(struct hidpp_device *hidpp, u8
*data,
+					  int size)
+{
+	struct led_data *led = (struct led_data *)hidpp->private_data;
+	struct hidpp_report *report = (struct hidpp_report *)data;
+	switch (report->report_id) {
+	case REPORT_ID_HIDPP_LONG:
+		/* size is already checked in hidpp_raw_event.
+		 * only leave long through
+		 */
+		break;
+	default:
+		return 0;
+	}
+
+	if (report->fap.feature_index !=
hidpp->illumination_feature_index) {
+		return 0;
+	}
+
+
+	if (report->fap.funcindex_clientid ==
HIDPP_ILLUMINATION_EVENT_CHANGE) {
+		led->on = report->fap.params[0] & 0x1;
+		if (led->on) {
+			unsigned led_brightness =
device_to_led_brightness(led);
+			led_classdev_notify_brightness_hw_changed(
+				&led->cdev, led_brightness);
+		} else
+
led_classdev_notify_brightness_hw_changed(&led->cdev,
+
LED_OFF);
+		return 0;
+	}
+
+	if (report->fap.funcindex_clientid ==
+	    HIDPP_ILLUMINATION_EVENT_BRIGHTNESS_CHANGE) {
+		unsigned led_brightness;
+		u16 brightness = be16_to_cpu(report->fap.params[0] << 8 |
+					     report->fap.params[1] << 0);
+		led->brightness = brightness;
+		led_brightness = device_to_led_brightness(led);
+		led_classdev_notify_brightness_hw_changed(&led->cdev,
+							  led_brightness);
+		return 0;
+	}
+
+	return 0;
+}
+
 /*
--------------------------------------------------------------------------
*/
 /* 0x2120: Hi-resolution scrolling
*/
 /*
--------------------------------------------------------------------------
*/
@@ -2929,7 +3285,6 @@ static int m560_send_config_command(struct
hid_device *hdev, bool connected)

 	return hidpp_send_rap_command_sync(
 		hidpp_dev,
-		REPORT_ID_HIDPP_SHORT,
 		M560_SUB_ID,
 		M560_BUTTON_MODE_REGISTER,
 		(u8 *)m560_config_parameter,
@@ -3468,7 +3823,6 @@ static int hidpp_initialize_hires_scroll(struct
hidpp_device *hidpp)
 		struct hidpp_report response;

 		ret = hidpp_send_rap_command_sync(hidpp,
-						  REPORT_ID_HIDPP_SHORT,
 						  HIDPP_GET_REGISTER,

HIDPP_ENABLE_FAST_SCROLL,
 						  NULL, 0, &response);
@@ -3648,6 +4002,12 @@ static int hidpp_raw_hidpp_event(struct
hidpp_device *hidpp, u8 *data,
 			return ret;
 	}

+	if (hidpp->capabilities & HIDPP_CAPABILITY_ILLUMINATION_LIGHT) {
+		ret = hidpp20_illumination_raw_event(hidpp, data, size);
+		if (ret != 0)
+			return ret;
+	}
+
 	if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
 		ret = hidpp10_wheel_raw_event(hidpp, data, size);
 		if (ret != 0)
@@ -3972,6 +4332,7 @@ static void hidpp_connect_event(struct hidpp_device
*hidpp)

 	hidpp_initialize_battery(hidpp);
 	hidpp_initialize_hires_scroll(hidpp);
+	hidpp_initialize_illumination(hidpp);

 	/* forward current battery state */
 	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
@@ -3994,6 +4355,14 @@ static void hidpp_connect_event(struct hidpp_device
*hidpp)
 	if (hidpp->capabilities & HIDPP_CAPABILITY_HI_RES_SCROLL)
 		hi_res_scroll_enable(hidpp);

+	if (hidpp->capabilities & HIDPP_CAPABILITY_ILLUMINATION_LIGHT) {
+		ret = register_led(hidpp);
+		if (ret) {
+			hid_err(hdev, "Registering leds failed.\n");
+			return;
+		}
+	}
+
 	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) ||
hidpp->delayed_input)
 		/* if the input nodes are already created, we can stop now
*/
 		return;
@@ -4187,12 +4556,16 @@ static int hidpp_probe(struct hid_device *hdev,
const struct hid_device_id *id)
 	if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
 		hidpp_unifying_init(hidpp);

-	connected = hidpp_root_get_protocol_version(hidpp) == 0;
+	ret = hidpp_root_get_protocol_version(hidpp);
+	connected = ret == 0;
 	atomic_set(&hidpp->connected, connected);
 	if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) {
 		if (!connected) {
+			if (ret == -ETIMEDOUT)
+				hid_err(hdev, "Device connection timed
out");
+			else
+				hid_err(hdev, "Device not connected");
 			ret = -ENODEV;
-			hid_err(hdev, "Device not connected");
 			goto hid_hw_init_fail;
 		}

@@ -4357,6 +4730,9 @@ static const struct hid_device_id hidpp_devices[] =
{
 		.driver_data = HIDPP_QUIRK_CLASS_G920 |
HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
 	{ /* Logitech G Pro Gaming Mouse over USB */
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
+	{ /* Logitech Litra Glow over USB*/
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_LITRA_GLOW),
+		.driver_data = HIDPP_QUIRK_DELAYED_INIT |
HIDPP_QUIRK_FORCE_OUTPUT_REPORTS },

 	{ /* MX5000 keyboard over Bluetooth */
 	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 50e1c717fc0a..0332662692d2 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -491,6 +491,7 @@ static const struct hid_device_id
hid_have_special_driver[] = {
 #endif
 #if IS_ENABLED(CONFIG_HID_LOGITECH_HIDPP)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_G920_WHEEL) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_LITRA_GLOW) },
 #endif
 #if IS_ENABLED(CONFIG_HID_MAGICMOUSE)
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_MAGICMOUSE) },

Re: Litra Glow on Linux

[Andreas Bergmeier]

On Sat, 19 Nov 2022, Andreas Bergmeier wrote:

> So after some tinkering I have code now that succeeds in retrieving state
> via sending reports once. After that all following sends time out.
> I am at a loss what I am doing wrong, tbh. RFC below.
I noticed that by default hidraw, hiddev and hid-input are loaded for
Glow. Probably because it exposes a Consumer Control page in Reports.
Is it possible that hid-input interferes with hid-logitech-hidpp?
I was trying to disable hid-input using
- HIDPP_QUIRK_NO_HIDINPUT
- hid_ignore_list

For the former it still seems to load hid-input.
For the latter it doesn't seem to load hid-logitech-hidpp.
I couldn't yet untangle all the hid driver indirections and loading.

Litra Glow on Linux

[Andreas Bergmeier]

On my Ubuntu machine i am running 5.15.0. Now when I plugin in my
Logitech Litra Glow, it gets detected and the following shows up in my
dmesg:

```
input: Logi Litra Glow Consumer Control as
/devices/pci0000:00/0000:00:14.0/usb3/3-4/3-4.2/3-4.2:1.0/0003:046D:C900.000B/input/input75
hid-generic 0003:046D:C900.000B: input,hiddev0,hidraw2: USB HID v1.11
Device [Logi Litra Glow] on usb-0000:00:14.0-4.2/input0
```

Via (hardware) buttons you can switch the device on, regulate the
color temperature as well as the brightness.
I know of no way to fully control the device from my computer and
would like to change that.

It seems to me like I need to solve 4 problems (in userspace and maybe
kernelspace):
1. Handle plugging in and off
2. Listen to events (button pressed) from the device
3. Get the current state of the device
4. Send events to the device


The device seems to provide a pretty bare HID Report interface with no
alternate configurations:
https://github.com/abergmeier/litra_glow_linux/blob/main/lsusb
The HID seems to define 3 Reports:
https://github.com/abergmeier/litra_glow_linux/blob/main/parsed_descriptor

Ignoring 1. for now.

Trying to solve 2. I wrote a basic HIDDEV application. Using `read` I
only see events from Report 17 (0x11). For all my experimenting with
the device I have never seen a Report 1 or 2.
So I get events, but it seems like the provided
`hiddev_usage_ref.value` is sometimes wrong (seems to be 0 and 1 for
most of the time even if I adjust the brightness).
Doing a recording (turning on, adjusting brightness, turning off) of
the raw HID events seems like the "correct" events are sent from the
device: https://github.com/abergmeier/litra_glow_linux/blob/main/hid-recorder.
So it seems to me like maybe the values get mixed up somewhere in the HID code.
Alternatively I did a `evtest` run on the /dev/input/event* for the
`Logi Litra Glow Consumer Control`:
https://github.com/abergmeier/litra_glow_linux/blob/main/evtest
When pressing (hardware) buttons no events showed up in `evtest´.
Probably not surprising since these would be from Report 1 and 2 IIUC.
Now I am not sure whether the USB interface is sketchy or whether one
needs to activate the _Consumer Control_ somehow.

Trying to solve 3. from what I understand with HID there usually is no
way of reading the current state of the device?

Trying to solve 4. there are userspace libraries in Python and Go
which send events to the device bypassing HID. So there may be some
quirks handling necessary in HID but I would defer that until 2. is
done.

With all that I am pretty much at my wits end and would appreciate any
input how to further analyze the device situation.

Cheers