DESIGN: Logitech G710+ keyboard driver

DESIGN: Logitech G710+ keyboard driver

[Jimmy Berry]

There are a variety of Logitech G-series out-of-tree keyboard drivers and a
number of discussions on the linux-input list regarding merging the drivers. The
end result appears to be stagnation. I notice the recent addition of a driver
for the Corsair Vengeance K90 Keyboard (hid-corsair.c) which seems to have a
similar feature set.

Some of the previous mailing list discussions mentioned splitting up the
Logitech drivers by feature instead of device.

> - base input driver handling the "dead" keys
> - adding LEDs
> - adding LCD fb/backlight
> - adding other goodies

Additionally, there was a driver for the G710+ keyboard developed separately
from the other G-series drivers that was submitted to the list, but it appears
no response was given.

I am looking to refactor and submit that G710+ driver and submit. Given the
common base of "extra" features provided by various niche keyboards it seems
reasonable to consider supporting them using a common set of code, but having
only looked at hid-corsair.c and the G710+ code in detail I cannot be certain of
the extent to which the "extra" features are implemented similarly.

With that in mind the corsair driver provides a good reference implementation
with a number of things that should be improved in the G710+ driver.

Looking at the existing Logitech hid code I see the following:

- hid-lg2ff
- hid-lg3ff
- hid-lg4ff
- hid-lgff
- hid-lg
- hid-logitech-dj
- hid-logitech-hidpp

Everything seems unrelated to keyboards except DINOVO* devices, but they appear
to be an unrelated class of keyboard. As such it seems likely a new file would
be the best route forward. The "logitech" spelled out seems to be the newer
choice so perhaps hid-logitech-keyboard or similar?

Would it be preferable to try and extract out the common features, if feasible,
from the existing corsair driver into a common file that can be shared by the
new G710+ and eventually other G-series keyboards, or just build the G710+
driver in a similar manor to corsair and submit? Perhaps refactoring if desired
at a later point.

The extra keys + modifiers/profiles can be implemented in a variety of ways, but
it is unclear which is best. I would appreciate some input.

Both keyboards provide extra keys (G1-N) and modifiers/profiles (M1-3). The M
keys act as modifiers to the G keys or profiles in that they can change the
behavior of the G keys. The corsair driver exposes each of the G and M keys and
keeps track of the active profile exposed through sysfs current_profile. While
this works the approach is dependent on user-space for mapping G keys + a
profile to a specific action. Existing key mapping applications do not have the
concept of a profile. What seems like a better implementation would be to think
of the M keys as modifiers and issue both events when a G key event is
triggered. This is how I modified the existing G710+ driver to operate.

The following is an example simulation:

M1 pressed
- emit M1 down
- emit M1 up
G1 pressed
- emit M1 down (could emit modifier like ctrl)
- emit G1 down
- emit G2 up
- emit M1 up   (could emit modifier like ctrl)

The driver keeps track of the active modifier/profile (which is still be exposed
va sysfs), but it also simulates the modifier keys being pressed with the G
keys. The concept is not too dissimilar from sticky keys except that the
modifier keys are not required to be pressed before each G key. Instead the M
keys may be pressed once and remain active until another M key is pressed.

With the following implementation I mapped the M keys to modifiers using xmodmap
which resulted in key mapping applications picking up the G keys with respect to
the active profile. Presumably the mapping could also be in the driver instead
of using xmodmap if desired (seems like a better solution).

Perhaps emitting proper modifier keys when G keys are pressed and unique codes
when M keys are pressed so user-space could still distinguish them.

Without the above technique nor a user-space application that understands
profiles the full functionality of the keyboards are not usable since there is
effectively only a one usable profile.

For reference:
- https://github.com/Wattos/logitech-g710-linux-driver (original author)
- https://github.com/boombatower/logitech-g710-linux-driver (my fork)

I have detailed notes on the differences in both features, implementation, and
the existing driver code, but that is probably more than is needed for this
initial mail.

I look forward to your feedback.

--
Jimmy

Re: DESIGN: Logitech G710+ keyboard driver

[Clément Vuchener]

On Mon, Nov 23, 2015 at 01:47:14AM -0600, Jimmy Berry wrote:
> There are a variety of Logitech G-series out-of-tree keyboard drivers and a
> number of discussions on the linux-input list regarding merging the drivers. The
> end result appears to be stagnation. I notice the recent addition of a driver
> for the Corsair Vengeance K90 Keyboard (hid-corsair.c) which seems to have a
> similar feature set.
> 
> Some of the previous mailing list discussions mentioned splitting up the
> Logitech drivers by feature instead of device.
> 
> > - base input driver handling the "dead" keys
> > - adding LEDs
> > - adding LCD fb/backlight
> > - adding other goodies
> 
> Additionally, there was a driver for the G710+ keyboard developed separately
> from the other G-series drivers that was submitted to the list, but it appears
> no response was given.
> 
> I am looking to refactor and submit that G710+ driver and submit. Given the
> common base of "extra" features provided by various niche keyboards it seems
> reasonable to consider supporting them using a common set of code, but having
> only looked at hid-corsair.c and the G710+ code in detail I cannot be certain of
> the extent to which the "extra" features are implemented similarly.
I am CCing Marty Plummer who wanted to write a G107 driver. I had a quick look at your driver and I think the protocols are similar (HID report 3 with a two byte bit-field reporting the special keys, if I understood correctly).
> 
> With that in mind the corsair driver provides a good reference implementation
> with a number of things that should be improved in the G710+ driver.
> 
> Looking at the existing Logitech hid code I see the following:
> 
> - hid-lg2ff
> - hid-lg3ff
> - hid-lg4ff
> - hid-lgff
> - hid-lg
> - hid-logitech-dj
> - hid-logitech-hidpp
> 
> Everything seems unrelated to keyboards except DINOVO* devices, but they appear
> to be an unrelated class of keyboard. As such it seems likely a new file would
> be the best route forward. The "logitech" spelled out seems to be the newer
> choice so perhaps hid-logitech-keyboard or similar?
> 
> Would it be preferable to try and extract out the common features, if feasible,
> from the existing corsair driver into a common file that can be shared by the
> new G710+ and eventually other G-series keyboards,
I don't think there is much you can extract from the corsair driver, it only remaps corsair usages to keys.
> or just build the G710+
> driver in a similar manor to corsair and submit?
I would not mind some standard key codes for those keys. Note that the Corsair K90 has 18 G-keys and there is not much room below 256 for that and key codes above 256 don't work with X11. I can work around that using lirc but I cannot use xmodmap for the K90 like you do.
> Perhaps refactoring if desired
> at a later point.
> 
> The extra keys + modifiers/profiles can be implemented in a variety of ways, but
> it is unclear which is best. I would appreciate some input.
> 
> Both keyboards provide extra keys (G1-N) and modifiers/profiles (M1-3). The M
> keys act as modifiers to the G keys or profiles in that they can change the
> behavior of the G keys. The corsair driver exposes each of the G and M keys and
> keeps track of the active profile exposed through sysfs current_profile. While
> this works the approach is dependent on user-space for mapping G keys + a
> profile to a specific action. Existing key mapping applications do not have the
> concept of a profile. What seems like a better implementation would be to think
> of the M keys as modifiers and issue both events when a G key event is
> triggered. This is how I modified the existing G710+ driver to operate.
> 
> The following is an example simulation:
> 
> M1 pressed
> - emit M1 down
> - emit M1 up
> G1 pressed
> - emit M1 down (could emit modifier like ctrl)
> - emit G1 down
> - emit G2 up
> - emit M1 up   (could emit modifier like ctrl)
I am not sure about that. It means adding key events not tied to physical key presses. Actually the M keys behave more like the "lock" keys than modifiers.
> 
> The driver keeps track of the active modifier/profile (which is still be exposed
> va sysfs), but it also simulates the modifier keys being pressed with the G
> keys. The concept is not too dissimilar from sticky keys except that the
> modifier keys are not required to be pressed before each G key. Instead the M
> keys may be pressed once and remain active until another M key is pressed.
> 
> With the following implementation I mapped the M keys to modifiers using xmodmap
> which resulted in key mapping applications picking up the G keys with respect to
> the active profile. Presumably the mapping could also be in the driver instead
> of using xmodmap if desired (seems like a better solution).
> 
> Perhaps emitting proper modifier keys when G keys are pressed and unique codes
> when M keys are pressed so user-space could still distinguish them.
> 
> Without the above technique nor a user-space application that understands
> profiles the full functionality of the keyboards are not usable since there is
> effectively only a one usable profile.
I have been using the user-space application in order to start commands. In that case, I could test the value of the sysfs attribute and issue different commands depending on that. For generating key sequences, I prefer to use the hardware macros, but I am not sure if your keyboard can do that.

I think it would be best to have an user-space application that understand profiles. We would still need a common interface for exposing the current profile. I don't know if it should be a sysfs attribute or something readable from the event node, I am too new to Linux input subsystem.
> 
> For reference:
> - https://github.com/Wattos/logitech-g710-linux-driver (original author)
> - https://github.com/boombatower/logitech-g710-linux-driver (my fork)
> 
> I have detailed notes on the differences in both features, implementation, and
> the existing driver code, but that is probably more than is needed for this
> initial mail.
> 
> I look forward to your feedback.
> 
> --
> Jimmy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sorry for the resend, Jimmy, I accidentally sent a HTML version to
the mailing list.

Re: DESIGN: Logitech G710+ keyboard driver

[Tolga Cakir]

I think we need a decision about what needs to be done in kernel and 
what is better handled in user-space. I've walked through the Corsair 
Vengeance K90 driver (hid-corsair.c) and I must say, I'm not really 
happy about the desicions made in the design. In my opinion, we should 
only do essential stuff in kernel space and do the rest in user-space.

To be more precise about hid-corsair.c (and possibly future gaming 
keyboard implementations):

1. We should not handle profile switching and exposing a sysfs ABI for 
profile-number in kernel-space. This has no advantage over keeping track 
of profiles in user-space. We need to use user-space programs anyway in 
order to handle macros and profile-sensitive key-handling. Using a 
keyboard specific ABI for parsing the profile just adds complexity to 
kernel- and user-space.

2. We should not map false keycodes to keys. hid-corsair.c is using 
BTN_TRIGGER_HAPPY[N] - this is a huge design flaw in my opinion. 
Eventhough this might get the job done, I'm questioning the design 
decision. First of all, we're generally talking about KEYs on a 
keyboard, not BTNs. Second, they are not TRIGGERs. Third, they are not 
HAPPY. Also, BTN_TRIGGER_HAPPY[N] is only specified for up to 40 extra 
keys. What do we do with gaming keyboards, which offer more? The 
Microsoft Sidewinder X6 has 33 non-standard extra keys for example 
(agreed, it's less than 40, but still near the maximum). Do we really 
need to map random linux/input.h keycodes to non-standard keys?

I think mapping linux/input.h keycodes to non-standard keys has only one 
reason: to keep things simple with X11. As Clement mentioned, keycodes 
above 256 don't work with X11. However, this should not be a reason to 
hack workarounds into kernel. We can use hidraw / hiddev in user-space 
instead to catch those events and map them accordingly to our needs 
(they are designed to be fully programmable keys after all). This adds 
complexity in user-space, but also adds flexibility and we don't need to 
break / workaround any HID stuff.

3. We should either have common ABI for gaming keyboards or we shouldn't 
have them at all (or just in very special cases). This just adds 
complexity in kernel- and user-space, as lots of keyboards need to be 
handled their own way. Gaming keyboards have things in common, like 
LEDs. Creating an ABI for setting extra LEDs is totally legit, but we 
should define standards, so user-space applications can rely on 
something. Most keyboards have multiple profile LEDs, 1 recording LED, 
sometimes 1 gaming mode LED (disabling Windows key) and sometimes 
keyboard backlight is also adjustable.

We could define a common ABI for them:
- read-only profile_led_count (or profile_led_max): parse number of 
profile_leds
- rw profile_led_[N]: setting and parsing profile_led_[N] status, 0 and 1
- rw recording_led: setting and parsing recording_led status, 0 and 1
- rw gaming_led: setting and parsing gaming_led status, 0 and 1
- read-only backlight_max: parse maximum brightness
- rw backlight: setting and parsing backlight intensity, 0 - backlight_max

Exposing a read-write interface for switching HW / SW mode is also a 
legit use-case for ABI.

A major drawback of ABI (and user-space programs depending on it): we 
cannot respond fast enough to new devices, as people need to wait 
several months / years for the updated kernel versions or run a patched 
kernel (including all the drawbacks associated with using a custom 
kernel for the average user).

The kernel's role would be creating an interface to interact with the 
keyboard's extras and it's up to the user-space, how to make use of them 
(e.g. profile-switching macro tool using LEDs for active profile number, 
or custom script using them in a blinking pattern for incoming E-Mails).

Now about the Logitech G710+. Things, that need and should be handled in 
kernel space (in my opinion):

1. Bugs. Thanks to Jimmy Berry, there is already a G710+ bugfix applied 
to 4.4 (http://www.spinics.net/lists/linux-input/msg42186.html). There 
are no other bugs for the G710+ I'm aware of.

2. The Logitech G710+ is a composite device: interface #1 is used for G1 
- G6 keys + usual standard keyboard. Please note, that the G1 - G6 keys 
are outputting KEY_1 - KEY_6. Interface #2 is used for M1 - M3, MR key 
(macro recording and profile switching / LEDs) and also the G1 - G6 keys 
(outputting non-standard HID events!). As you can see, G1 - G6 keys are 
double mapped for 2 devices. In case, an operating system doesn't 
recognize G1 - G6 keys on interface #2, we're simply getting KEY_1 - 
KEY6 from interface #1. The Windows Logitech driver is sending out a 
specific packet, so the G1 - G6 keys on interface #1 get completely 
disabled (maybe other changes aswell, I don't know). This is a perfect 
use case for kernel-space.

Sorry for the wall of text.

Cheers,
Tolga

Re: DESIGN: Logitech G710+ keyboard driver

[Clément Vuchener]

On 12/04/2015 07:40 AM, Tolga Cakir wrote:
> I think we need a decision about what needs to be done in kernel and what is better handled in user-space. I've walked through the Corsair Vengeance K90 driver (hid-corsair.c) and I must say, I'm not really happy about the desicions made in the design. In my opinion, we should only do essential stuff in kernel space and do the rest in user-space.
>
> To be more precise about hid-corsair.c (and possibly future gaming keyboard implementations):
>
> 1. We should not handle profile switching and exposing a sysfs ABI for profile-number in kernel-space. This has no advantage over keeping track of profiles in user-space. We need to use user-space programs anyway in order to handle macros and profile-sensitive key-handling. Using a keyboard specific ABI for parsing the profile just adds complexity to kernel- and user-space.
>
> 2. We should not map false keycodes to keys. hid-corsair.c is using BTN_TRIGGER_HAPPY[N] - this is a huge design flaw in my opinion. Eventhough this might get the job done, I'm questioning the design decision. First of all, we're generally talking about KEYs on a keyboard, not BTNs. Second, they are not TRIGGERs. Third, they are not HAPPY. Also, BTN_TRIGGER_HAPPY[N] is only specified for up to 40 extra keys. What do we do with gaming keyboards, which offer more? The Microsoft Sidewinder X6 has 33 non-standard extra keys for example (agreed, it's less than 40, but still near the maximum). Do we really need to map random linux/input.h keycodes to non-standard keys?
I choose the TRIGGER_HAPPY buttons because the only use I found for it was extra buttons on some devices. It seems the most appropriate. It would be nice to have special key codes for this kind of keys, I asked about that with my original patch but I did not get any answer so the BTN_TRIGGER_HAPPY stayed there.

They are keys the user may be interested in, I don't see why they should not have key events.
>
> I think mapping linux/input.h keycodes to non-standard keys has only one reason: to keep things simple with X11. As Clement mentioned, keycodes above 256 don't work with X11. However, this should not be a reason to hack workarounds into kernel. We can use hidraw / hiddev in user-space instead to catch those events and map them accordingly to our needs (they are designed to be fully programmable keys after all). This adds complexity in user-space, but also adds flexibility and we don't need to break / workaround any HID stuff.
It does not work with X11 but it works with existing remapping software based on evdev. I don't understand what you mean with "break / workaround", I am remapping *invalid* HID usage code from the keyboard. If I don't do that pressing those keys triggers unwanted key events.
>
> 3. We should either have common ABI for gaming keyboards or we shouldn't have them at all (or just in very special cases). This just adds complexity in kernel- and user-space, as lots of keyboards need to be handled their own way. Gaming keyboards have things in common, like LEDs. Creating an ABI for setting extra LEDs is totally legit, but we should define standards, so user-space applications can rely on something. Most keyboards have multiple profile LEDs, 1 recording LED, sometimes 1 gaming mode LED (disabling Windows key) and sometimes keyboard backlight is also adjustable.
>
> We could define a common ABI for them:
> - read-only profile_led_count (or profile_led_max): parse number of profile_leds
> - rw profile_led_[N]: setting and parsing profile_led_[N] status, 0 and 1
Isn't this the same as the current_profile attribute you were arguing against?
> - rw recording_led: setting and parsing recording_led status, 0 and 1
> - rw gaming_led: setting and parsing gaming_led status, 0 and 1
> - read-only backlight_max: parse maximum brightness
> - rw backlight: setting and parsing backlight intensity, 0 - backlight_max
LED class devices already do that.
>
> Exposing a read-write interface for switching HW / SW mode is also a legit use-case for ABI.
>
> A major drawback of ABI (and user-space programs depending on it): we cannot respond fast enough to new devices, as people need to wait several months / years for the updated kernel versions or run a patched kernel (including all the drawbacks associated with using a custom kernel for the average user).
Drivers can be added as out-of-tree modules.
>
> The kernel's role would be creating an interface to interact with the keyboard's extras and it's up to the user-space, how to make use of them (e.g. profile-switching macro tool using LEDs for active profile number, or custom script using them in a blinking pattern for incoming E-Mails).
>
> Now about the Logitech G710+. Things, that need and should be handled in kernel space (in my opinion):
>
> 1. Bugs. Thanks to Jimmy Berry, there is already a G710+ bugfix applied to 4.4 (http://www.spinics.net/lists/linux-input/msg42186.html). There are no other bugs for the G710+ I'm aware of.
>
> 2. The Logitech G710+ is a composite device: interface #1 is used for G1 - G6 keys + usual standard keyboard. Please note, that the G1 - G6 keys are outputting KEY_1 - KEY_6. Interface #2 is used for M1 - M3, MR key (macro recording and profile switching / LEDs) and also the G1 - G6 keys (outputting non-standard HID events!). As you can see, G1 - G6 keys are double mapped for 2 devices. In case, an operating system doesn't recognize G1 - G6 keys on interface #2, we're simply getting KEY_1 - KEY6 from interface #1. The Windows Logitech driver is sending out a specific packet, so the G1 - G6 keys on interface #1 get completely disabled (maybe other changes aswell, I don't know). This is a perfect use case for kernel-space.
>
> Sorry for the wall of text.
>
> Cheers,
> Tolga

Re: DESIGN: Logitech G710+ keyboard driver

[Tolga Cakir]

Am 04.12.2015 um 12:22 schrieb Clément Vuchener:
> On 12/04/2015 07:40 AM, Tolga Cakir wrote:
>> I think we need a decision about what needs to be done in kernel and what is better handled in user-space. I've walked through the Corsair Vengeance K90 driver (hid-corsair.c) and I must say, I'm not really happy about the desicions made in the design. In my opinion, we should only do essential stuff in kernel space and do the rest in user-space.
>>
>> To be more precise about hid-corsair.c (and possibly future gaming keyboard implementations):
>>
>> 1. We should not handle profile switching and exposing a sysfs ABI for profile-number in kernel-space. This has no advantage over keeping track of profiles in user-space. We need to use user-space programs anyway in order to handle macros and profile-sensitive key-handling. Using a keyboard specific ABI for parsing the profile just adds complexity to kernel- and user-space.
>>
>> 2. We should not map false keycodes to keys. hid-corsair.c is using BTN_TRIGGER_HAPPY[N] - this is a huge design flaw in my opinion. Eventhough this might get the job done, I'm questioning the design decision. First of all, we're generally talking about KEYs on a keyboard, not BTNs. Second, they are not TRIGGERs. Third, they are not HAPPY. Also, BTN_TRIGGER_HAPPY[N] is only specified for up to 40 extra keys. What do we do with gaming keyboards, which offer more? The Microsoft Sidewinder X6 has 33 non-standard extra keys for example (agreed, it's less than 40, but still near the maximum). Do we really need to map random linux/input.h keycodes to non-standard keys?
> I choose the TRIGGER_HAPPY buttons because the only use I found for it was extra buttons on some devices. It seems the most appropriate. It would be nice to have special key codes for this kind of keys, I asked about that with my original patch but I did not get any answer so the BTN_TRIGGER_HAPPY stayed there.
>
> They are keys the user may be interested in, I don't see why they should not have key events.

Hi Clement,

I agree, but instead of using key events designed for joysticks and 
gamepads, we should define and use keycodes for our specific use-case.

How come 40 individual keycodes have been added for a single joystick 
(Saitek X52 Pro Flight System), whereas macro keys and profile switching 
keys can be found on hundreds of different gaming keyboards and we have 
no fitting keycode for that?

I'm quoting linux/input.h:
 > We avoid low common keys in module aliases so they don't get huge.

Luckily, macro keys and profile switch keys are very common these days 
on gaming keyboards. So that shouldn't be a problem.

I think it's time to define KEY_G1 - KEY_G30 and KEY_M1 - KEY_M5, so we 
can finally have appropiate keycodes for these kind of things. They can 
be found on nearly every gaming keyboard, so that should be enough to 
justify them. There is still enough room for additional keycodes. What 
are you thinking about this?

>> I think mapping linux/input.h keycodes to non-standard keys has only one reason: to keep things simple with X11. As Clement mentioned, keycodes above 256 don't work with X11. However, this should not be a reason to hack workarounds into kernel. We can use hidraw / hiddev in user-space instead to catch those events and map them accordingly to our needs (they are designed to be fully programmable keys after all). This adds complexity in user-space, but also adds flexibility and we don't need to break / workaround any HID stuff.
> It does not work with X11 but it works with existing remapping software based on evdev. I don't understand what you mean with "break / workaround", I am remapping *invalid* HID usage code from the keyboard. If I don't do that pressing those keys triggers unwanted key events.

By "break / workaround" I mean mapping keycodes, which have not been 
designed for this use-case. BTN suggests, that they are not designed for 
keyboards in the first place.

>> 3. We should either have common ABI for gaming keyboards or we shouldn't have them at all (or just in very special cases). This just adds complexity in kernel- and user-space, as lots of keyboards need to be handled their own way. Gaming keyboards have things in common, like LEDs. Creating an ABI for setting extra LEDs is totally legit, but we should define standards, so user-space applications can rely on something. Most keyboards have multiple profile LEDs, 1 recording LED, sometimes 1 gaming mode LED (disabling Windows key) and sometimes keyboard backlight is also adjustable.
>>
>> We could define a common ABI for them:
>> - read-only profile_led_count (or profile_led_max): parse number of profile_leds
>> - rw profile_led_[N]: setting and parsing profile_led_[N] status, 0 and 1
> Isn't this the same as the current_profile attribute you were arguing against?

Nope. Exposing an interface to interact with the profile LEDs isn't the 
same as creating an ABI for the active profile. Turning on profile_led_1 
for example does not has to mean, that profile 1 must be active. It just 
means, that the LED for profile 1 is turned on.

We should not tie profile to profile_led in kernel space and give 
user-space applications all flexibility, how to use these LEDs and 
keeping track of profiles (really, instead of polling / parsing 
active_profile ABIs, they can easily keep track of them on their own and 
set LEDs accordingly).

>> - rw recording_led: setting and parsing recording_led status, 0 and 1
>> - rw gaming_led: setting and parsing gaming_led status, 0 and 1
>> - read-only backlight_max: parse maximum brightness
>> - rw backlight: setting and parsing backlight intensity, 0 - backlight_max
> LED class devices already do that.

Perfect!

Cheers,
Tolga
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DESIGN: Logitech G710+ keyboard driver

[Clément Vuchener]

On 12/04/2015 03:38 PM, Tolga Cakir wrote:
> Am 04.12.2015 um 12:22 schrieb Clément Vuchener:
>> On 12/04/2015 07:40 AM, Tolga Cakir wrote:
>>> I think we need a decision about what needs to be done in kernel and what is better handled in user-space. I've walked through the Corsair Vengeance K90 driver (hid-corsair.c) and I must say, I'm not really happy about the desicions made in the design. In my opinion, we should only do essential stuff in kernel space and do the rest in user-space.
>>>
>>> To be more precise about hid-corsair.c (and possibly future gaming keyboard implementations):
>>>
>>> 1. We should not handle profile switching and exposing a sysfs ABI for profile-number in kernel-space. This has no advantage over keeping track of profiles in user-space. We need to use user-space programs anyway in order to handle macros and profile-sensitive key-handling. Using a keyboard specific ABI for parsing the profile just adds complexity to kernel- and user-space.
>>>
>>> 2. We should not map false keycodes to keys. hid-corsair.c is using BTN_TRIGGER_HAPPY[N] - this is a huge design flaw in my opinion. Eventhough this might get the job done, I'm questioning the design decision. First of all, we're generally talking about KEYs on a keyboard, not BTNs. Second, they are not TRIGGERs. Third, they are not HAPPY. Also, BTN_TRIGGER_HAPPY[N] is only specified for up to 40 extra keys. What do we do with gaming keyboards, which offer more? The Microsoft Sidewinder X6 has 33 non-standard extra keys for example (agreed, it's less than 40, but still near the maximum). Do we really need to map random linux/input.h keycodes to non-standard keys?
>> I choose the TRIGGER_HAPPY buttons because the only use I found for it was extra buttons on some devices. It seems the most appropriate. It would be nice to have special key codes for this kind of keys, I asked about that with my original patch but I did not get any answer so the BTN_TRIGGER_HAPPY stayed there.
>>
>> They are keys the user may be interested in, I don't see why they should not have key events.
>
> Hi Clement,
>
> I agree, but instead of using key events designed for joysticks and gamepads, we should define and use keycodes for our specific use-case.
>
> How come 40 individual keycodes have been added for a single joystick (Saitek X52 Pro Flight System), whereas macro keys and profile switching keys can be found on hundreds of different gaming keyboards and we have no fitting keycode for that?
>
> I'm quoting linux/input.h:
> > We avoid low common keys in module aliases so they don't get huge.
>
> Luckily, macro keys and profile switch keys are very common these days on gaming keyboards. So that shouldn't be a problem.
>
> I think it's time to define KEY_G1 - KEY_G30 and KEY_M1 - KEY_M5, so we can finally have appropiate keycodes for these kind of things. They can be found on nearly every gaming keyboard, so that should be enough to justify them. There is still enough room for additional keycodes. What are you thinking about this?
I don't know what would be the correct number of key codes.
For Corsair keyboards I know, there is either 18 or 6 G-keys and 3 profile keys (M-keys). So that would be enough for me.
My current driver also exposes the MR (macro/mem record) key. But with two different key codes for start and stop. I am not sure it is really useful, I may just remove it when I update the key codes.

You should make this proposition to a maintainer. I will update the corsair driver when it is accepted.
>
>>> I think mapping linux/input.h keycodes to non-standard keys has only one reason: to keep things simple with X11. As Clement mentioned, keycodes above 256 don't work with X11. However, this should not be a reason to hack workarounds into kernel. We can use hidraw / hiddev in user-space instead to catch those events and map them accordingly to our needs (they are designed to be fully programmable keys after all). This adds complexity in user-space, but also adds flexibility and we don't need to break / workaround any HID stuff.
>> It does not work with X11 but it works with existing remapping software based on evdev. I don't understand what you mean with "break / workaround", I am remapping *invalid* HID usage code from the keyboard. If I don't do that pressing those keys triggers unwanted key events.
>
> By "break / workaround" I mean mapping keycodes, which have not been designed for this use-case. BTN suggests, that they are not designed for keyboards in the first place.
>
>>> 3. We should either have common ABI for gaming keyboards or we shouldn't have them at all (or just in very special cases). This just adds complexity in kernel- and user-space, as lots of keyboards need to be handled their own way. Gaming keyboards have things in common, like LEDs. Creating an ABI for setting extra LEDs is totally legit, but we should define standards, so user-space applications can rely on something. Most keyboards have multiple profile LEDs, 1 recording LED, sometimes 1 gaming mode LED (disabling Windows key) and sometimes keyboard backlight is also adjustable.
>>>
>>> We could define a common ABI for them:
>>> - read-only profile_led_count (or profile_led_max): parse number of profile_leds
>>> - rw profile_led_[N]: setting and parsing profile_led_[N] status, 0 and 1
>> Isn't this the same as the current_profile attribute you were arguing against?
>
> Nope. Exposing an interface to interact with the profile LEDs isn't the same as creating an ABI for the active profile. Turning on profile_led_1 for example does not has to mean, that profile 1 must be active. It just means, that the LED for profile 1 is turned on.
>
> We should not tie profile to profile_led in kernel space and give user-space applications all flexibility, how to use these LEDs and keeping track of profiles (really, instead of polling / parsing active_profile ABIs, they can easily keep track of them on their own and set LEDs accordingly).
I won't be able to do that for the corsair driver, current profile and profile LEDs are tied in the hardware.
>
>>> - rw recording_led: setting and parsing recording_led status, 0 and 1
>>> - rw gaming_led: setting and parsing gaming_led status, 0 and 1
>>> - read-only backlight_max: parse maximum brightness
>>> - rw backlight: setting and parsing backlight intensity, 0 - backlight_max
>> LED class devices already do that.
>
> Perfect!
The LED class documentation, defines that LED names should be "devicename:colour:function". I think we should use common function names.

For the K90 (and similar keyboards, I expect K40 and K95), I cannot add the profile LEDs as explained above and the "gaming" LED exists but cannot be controlled or read (it is completely managed on the hardware).

>
> Cheers,
> Tolga

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DESIGN: Logitech G710+ keyboard driver

[Tolga Cakir]

>> Hi Clement,
>>
>> I agree, but instead of using key events designed for joysticks and gamepads, we should define and use keycodes for our specific use-case.
>>
>> How come 40 individual keycodes have been added for a single joystick (Saitek X52 Pro Flight System), whereas macro keys and profile switching keys can be found on hundreds of different gaming keyboards and we have no fitting keycode for that?
>>
>> I'm quoting linux/input.h:
>>> We avoid low common keys in module aliases so they don't get huge.
>> Luckily, macro keys and profile switch keys are very common these days on gaming keyboards. So that shouldn't be a problem.
>>
>> I think it's time to define KEY_G1 - KEY_G30 and KEY_M1 - KEY_M5, so we can finally have appropiate keycodes for these kind of things. They can be found on nearly every gaming keyboard, so that should be enough to justify them. There is still enough room for additional keycodes. What are you thinking about this?
> I don't know what would be the correct number of key codes.
> For Corsair keyboards I know, there is either 18 or 6 G-keys and 3 profile keys (M-keys). So that would be enough for me.
> My current driver also exposes the MR (macro/mem record) key. But with two different key codes for start and stop. I am not sure it is really useful, I may just remove it when I update the key codes.
>
> You should make this proposition to a maintainer. I will update the corsair driver when it is accepted.

The Sidewinder X6 has 30 individual macro keys; that's the highest 
amount of G-Keys I've come across, so that could be a starting point. 
There are gaming peripherals with up to 22 individual macro keys 
(Logitech G13), which could also benefit from such keycodes. So, I think 
KEY_G1 - KEY_G30 would be enough for most devices. Most keyboards either 
have 3 or 5 profile keys (KEY_M1 - KEY_M5). To keep things simple, you 
could map both keycodes of MR-key to KEY_M4 / KEY_M5 (or KEY_RECORD / 
KEY_MACRO).

I will prepare a patch and talk with Dmitry Torokhov and Jiri Kosina 
about it.

>>>> 3. We should either have common ABI for gaming keyboards or we shouldn't have them at all (or just in very special cases). This just adds complexity in kernel- and user-space, as lots of keyboards need to be handled their own way. Gaming keyboards have things in common, like LEDs. Creating an ABI for setting extra LEDs is totally legit, but we should define standards, so user-space applications can rely on something. Most keyboards have multiple profile LEDs, 1 recording LED, sometimes 1 gaming mode LED (disabling Windows key) and sometimes keyboard backlight is also adjustable.
>>>>
>>>> We could define a common ABI for them:
>>>> - read-only profile_led_count (or profile_led_max): parse number of profile_leds
>>>> - rw profile_led_[N]: setting and parsing profile_led_[N] status, 0 and 1
>>> Isn't this the same as the current_profile attribute you were arguing against?
>> Nope. Exposing an interface to interact with the profile LEDs isn't the same as creating an ABI for the active profile. Turning on profile_led_1 for example does not has to mean, that profile 1 must be active. It just means, that the LED for profile 1 is turned on.
>>
>> We should not tie profile to profile_led in kernel space and give user-space applications all flexibility, how to use these LEDs and keeping track of profiles (really, instead of polling / parsing active_profile ABIs, they can easily keep track of them on their own and set LEDs accordingly).
> I won't be able to do that for the corsair driver, current profile and profile LEDs are tied in the hardware.

Interesting. The Logitech G710+ doesn't have any onboard memory, so it 
relies on software for managing everything. We're free to control LEDs 
via specific packets.

>>>> - rw recording_led: setting and parsing recording_led status, 0 and 1
>>>> - rw gaming_led: setting and parsing gaming_led status, 0 and 1
>>>> - read-only backlight_max: parse maximum brightness
>>>> - rw backlight: setting and parsing backlight intensity, 0 - backlight_max
>>> LED class devices already do that.
>> Perfect!
> The LED class documentation, defines that LED names should be "devicename:colour:function". I think we should use common function names.
>
> For the K90 (and similar keyboards, I expect K40 and K95), I cannot add the profile LEDs as explained above and the "gaming" LED exists but cannot be controlled or read (it is completely managed on the hardware).

The gaming LED exists on the G710+ aswell, but is also managed 
internally on hardware.

Well, it looks like eventhough the Corsair K90 and Logitech G710+ have 
many things in common, they have their own ways of handling things.

To summarize things so far:

- I think most gaming keyboards would benefit from having KEY_G1 - 
KEY_G30 and KEY_M1 - KEY_M5 defined.

- we need to send the magic packet to G710+ in kernel-space, so we can 
enable this keyboard's extra functionalities.

- I think it's a good idea to expose the LEDs and backlight via an ABI, 
but we should keep it identical to other implementations so far (and if 
needed, just omit flexibility in terms of consistency).

- I agree on Jimmy's point of having a new file, something like 
hid-logitech-kbd.

- exposing profile-count via ABI and polling / parsing it in user-space 
has no real advantage over handling it directly in user-space. However, 
since the Corsair K90 is not able to seperate profile-counting from 
LEDs, we might think about keeping things consistent across 
implementations and simply do this in kernel-space instead. I think this 
point needs further discussion.

- it should completely be up to the user-space, what to do, if e.g. 
profile 2 is active and G1 has been pressed. This has nothing to do in 
kernel-space (and hid-corsair.c doesn't have anything like that, 
either). Therefore, I think we can omit the idea of having M1 - M3 keys 
as modifiers.

- we need user-space programs for full functionality either way. 
Therefore, we should design the kernel-space module with that in mind, 
so we can write efficient and maintainable code in both, user- and 
kernel-space. My idea behind this discussion was to have kernel-space 
just for the most essential stuff (like fixing bugs, sending magic 
packets, sending keycodes) and do everything in user-space, that can be 
done in user-space.

Cheers,
Tolga