Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard

From: Benjamin Tissoires
Date: Thu Mar 22 2018 - 04:33:38 EST

Next message: Alexandre Torgue: "Re: [PATCH v2] i2c: i2c-stm32f7: fix no check on returned setup"
Previous message: David Rientjes: "Re: [PATCH] memcg, thp: do not invoke oom killer on thp charges"
In reply to: Robert Munteanu: "Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard"
Next in thread: Robert Munteanu: "Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Mar 21, 2018 at 5:33 PM, Robert Munteanu <rombert@xxxxxxxxxx> wrote:
> Hi Benjamin,
>
> On Wed, 2018-03-21 at 17:12 +0100, Benjamin Tissoires wrote:
>> Hi Robert,
>>
>> First, apologies for not answering to the RFC. I missed it and it
>> fell
>> down in my INBOX.
>>
>> On Wed, Mar 21, 2018 at 11:43 AM, Robert Munteanu <rombert@xxxxxxxxxx
>> > wrote:
>> > The microdia family of keyboards uses a non-standard way of sending
>> > modifier keys.
>> >
>> > The down event always sets the first bit to 0x04 and the second
>> > keycode
>>
>> Pretty sure you are talking about bytes here, not bits.
>> And in the HID world, the first byte is usually a report ID.
>
> Right, thanks for spotting that.
>
>>
>> > to a custom value For instance, left shift sends the following bits
>> >
>> > 04 02 00 00 00 00 00 00
>> >
>> > while left control sends
>> >
>> > 04 01 00 00 00 00 00 00
>>
>> This sounds like bit(0) is mapped to LEFT_SHIFT and bit(1) mapped to
>> LEFT_CONTROL in the second byte.
>> Fortunately, LeftControl is designed in HID as 0xe0 in the keyboard
>> HID usage table, and LeftShift is 0xe1. So that would match the
>> behavior you are seeing.
>> If you have 04 04 00 00 00 00 00 00 when pressing LeftAlt, then
>> definitively you are just facing a bitmask, which is fairly common.
>>
>> >
>> > As a result all modifier keys are mapped to left shift and the
>> > keyboard is
>> > non-functional in that respect. To solve the problem, we capture
>> > the
>> > raw data in raw_event and manually generate the correct input
>> > events.
>>
>> I'd like to see the hid-recorder events from these keypresses. The
>> device might be buggy, but I have a gut feeling your solution is not
>> the simplest one.
>> Please grab hid-recorder from http://bentiss.github.io/hid-replay-
>> docs/
>
> See below, I recorded the output for pressing left ctrl, left alt, left
> shift, right ctrl, right alt, right shift, meta.
>
> D: 0
> R: 169 05 0c 09 01 a1 01 85 01 19 00 2a 80 03 15 00 26 80 03 95 01 75
> 10 81 00 c0 05 01 09 80 a1 01 85 02 19 81 29 83 15 00 25 01 75 01 95 03
> 81 02 95 05 81 01 c0 06 00 ff 09 01 a1 01 85 03 1a f1 00 2a f8 00 15 00
> 25 01 75 01 95 08 81 02 c0 05 01 09 06 a1 01 85 04 05 07 19 e0 29 e7 15
> 00 25 01 75 01 95 08 81 00 95 30 75 01 15 00 25 01 05 07 19 00 29 2f 81
> 02 c0 05 01 09 06 a1 01 85 05 95 38 75 01 15 00 25 01 05 07 19 30 29 67
> 81 02 c0 05 01 09 06 a1 01 85 06 95 38 75 01 15 00 25 01 05 07 19 68 29
> 9f 81 02 c0
> N: USB Keyboard
> P: usb-0000:00:14.0-6/input1
> I: 3 0c45 760b
> D: 0
> E: 0.000000 8 04 01 00 00 00 00 00 00
> E: 0.039920 8 04 00 00 00 00 00 00 00
> E: 0.751952 8 04 04 00 00 00 00 00 00
> E: 0.823977 8 04 00 00 00 00 00 00 00
> E: 2.639887 8 04 02 00 00 00 00 00 00
> E: 2.711896 8 04 00 00 00 00 00 00 00
> E: 3.583932 8 04 10 00 00 00 00 00 00
> E: 3.663919 8 04 00 00 00 00 00 00 00
> E: 4.871886 8 04 40 00 00 00 00 00 00
> E: 4.935906 8 04 00 00 00 00 00 00 00
> E: 6.503872 8 04 20 00 00 00 00 00 00
> E: 6.575850 8 04 00 00 00 00 00 00 00
> E: 7.383844 8 04 08 00 00 00 00 00 00
> E: 7.455861 8 04 00 00 00 00 00 00 00

Thanks. So it's indeed an issue with the report descriptor. Not sure
how this can also work under Windows.
FYI, the Report ID 4 translates to:

0x05, 0x01, // Usage Page (Generic Desktop) 78
0x09, 0x06, // Usage (Keyboard) 80
0xa1, 0x01, // Collection (Application) 82
0x85, 0x04, // Report ID (4) 84
0x05, 0x07, // Usage Page (Keyboard) 86
0x19, 0xe0, // Usage Minimum (224) 88
0x29, 0xe7, // Usage Maximum (231) 90
0x15, 0x00, // Logical Minimum (0) 92
0x25, 0x01, // Logical Maximum (1) 94
0x75, 0x01, // Report Size (1) 96
0x95, 0x08, // Report Count (8) 98
0x81, 0x00, // Input (Data,Arr,Abs) 100
0x95, 0x30, // Report Count (48) 102
0x75, 0x01, // Report Size (1) 104
0x15, 0x00, // Logical Minimum (0) 106
0x25, 0x01, // Logical Maximum (1) 108
0x05, 0x07, // Usage Page (Keyboard) 110
0x19, 0x00, // Usage Minimum (0) 112
0x29, 0x2f, // Usage Maximum (47) 114
0x81, 0x02, // Input (Data,Var,Abs) 116
0xc0, // End Collection 118

the problem lies in the byte offset 100. If you use "Input
(Data,Arr,Abs)", the logical min/max should not be 0,1 but 0,8 and the
keyboard should translate the various values based on this offset.
Here, the keyboard is using a plain bitfield, and so it should be
using 'Input (Data,Var,Abs)', which translates to '0x81, 0x02'
(instead of '0x81, 0x00').

The best solution for you is to introduce a report descriptor fixup.
See hid-aureal.c in the kernel tree for an example of a simple report
descriptor fixup.

>
>>
>> >
>> > The keyboard functions mostly as expected now, with only a few
>> > minor
>> > issues:
>> >
>> > - two USB devices are detected instead of 1
>>
>> Well, that should be easy enough to solve
>
> I think so, but I could not find a way how to do that.
>
>>
>> > - some key combinations are not triggered - e.g.
>> > left shift + left ctrl + p. However, the same combination is
>> > recognized
>> > with the right shift key.
>>
>> Could you add this combination in the hid-recorder output too?
>
> This is what Left Ctrl + Shift + U produces:
>
> E: 24.575686 8 04 01 00 00 00 00 00 00
> E: 24.951689 8 04 03 00 00 00 00 00 00
> E: 26.719615 8 04 01 00 00 00 00 00 00
> E: 26.727611 8 04 00 00 00 00 00 00 00
>
> On the other hand, Right Ctrl + Shift + U produces:
>
> E: 55.911268 8 04 10 00 00 00 00 00 00
> E: 56.119280 8 04 30 00 00 00 00 00 00
> E: 56.599319 8 04 30 00 00 00 01 00 00
> E: 56.703282 8 04 30 00 00 00 00 00 00
> E: 56.815278 8 04 20 00 00 00 00 00 00
> E: 56.823255 8 04 00 00 00 00 00 00 00
>
> It looks suspicious that the keypress is not registered. Do I need to
> boot a kernel without my patch applied?

This seems like a firmware issue, and I doubt we will be able to fix
it. Does the keyboard under Windows behave sanely?

Cheers,
Benjamin

>
>> more comments inlined:
>>
>> >
>> > Changelog:
>> >
>> > - v2: modifier keys work, some combinations are still troublesome
>> > - v3: style cleanup, rebase on top of 4.14
>> > - v4: remove most debugging calls, make init info useful for user,
>> > rebased on top of 4.15
>> >
>> > Signed-off-by: Robert Munteanu <rombert@xxxxxxxxxx>
>> > ---
>> > drivers/hid/Kconfig | 9 +++
>> > drivers/hid/Makefile | 2 +-
>> > drivers/hid/hid-core.c | 3 +
>> > drivers/hid/hid-ids.h | 3 +
>> > drivers/hid/hid-microdia.c | 148
>> > +++++++++++++++++++++++++++++++++++++++++++++
>> > 5 files changed, 164 insertions(+), 1 deletion(-)
>> > create mode 100644 drivers/hid/hid-microdia.c
>> >
>> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> > index 779c5ae47f36..c3350f2ec4ea 100644
>> > --- a/drivers/hid/Kconfig
>> > +++ b/drivers/hid/Kconfig
>> > @@ -548,6 +548,15 @@ config HID_MAYFLASH
>> > Say Y here if you have HJZ Mayflash PS3 game controller
>> > adapters
>> > and want to enable force feedback support.
>> >
>> > +config HID_MICRODIA
>> > + tristate "Microdia based keyboards"
>> > + depends on HID
>> > + default !EXPERT
>> > + ---help---
>> > + Support for Microdia devices that are not compliant with the
>> > HID standard.
>> > +
>> > + One known example if the Redragon Asura Keyboard.
>> > +
>> > config HID_MICROSOFT
>> > tristate "Microsoft non-fully HID-compliant devices"
>> > depends on HID
>> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> > index 235bd2a7b333..e66a305876c5 100644
>> > --- a/drivers/hid/Makefile
>> > +++ b/drivers/hid/Makefile
>> > @@ -62,6 +62,7 @@ obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-
>> > dj.o
>> > obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
>> > obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
>> > obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
>> > +obj-$(CONFIG_HID_MICRODIA) += hid-microdia.o
>> > obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
>> > obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
>> > obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
>> > @@ -121,4 +122,3 @@ obj-$(CONFIG_USB_KBD) += usbhid/
>> >
>> > obj-$(CONFIG_I2C_HID) += i2c-hid/
>> >
>> > -obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> > index 0c3f608131cf..b36c2df4b755 100644
>> > --- a/drivers/hid/hid-core.c
>> > +++ b/drivers/hid/hid-core.c
>> > @@ -2393,6 +2393,9 @@ static const struct hid_device_id
>> > hid_have_special_driver[] = {
>> > #endif
>> > #if IS_ENABLED(CONFIG_HID_ZYDACRON)
>> > { HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON,
>> > USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
>> > +#endif
>> > +#if IS_ENABLED(CONFIG_HID_MICRODIA)
>> > + {
>> > HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_ASUR
>> > A) },
>> > #endif
>>
>> You don't need this hunk anymore since v4.15 IIRC
>
> Ack, will update and retest.
>
>>
>> > { }
>> > };
>> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> > index 5da3d6256d25..146869e55c5a 100644
>> > --- a/drivers/hid/hid-ids.h
>> > +++ b/drivers/hid/hid-ids.h
>> > @@ -1171,4 +1171,7 @@
>> > #define USB_VENDOR_ID_UGTIZER 0x2179
>> > #define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053
>> >
>> > +#define USB_VENDOR_ID_MICRODIA 0x0c45
>> > +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
>> > +
>> > #endif
>> > diff --git a/drivers/hid/hid-microdia.c b/drivers/hid/hid-
>> > microdia.c
>> > new file mode 100644
>> > index 000000000000..f9d8de18a989
>> > --- /dev/null
>> > +++ b/drivers/hid/hid-microdia.c
>> > @@ -0,0 +1,148 @@
>>
>> missing the SPDX identifier
>> (see Documentation/process/license-rules.rst)
>
> Ack, will update.
>
>>
>> > +/*
>> > + * HID driver for Microdia-based keyboards
>> > + *
>> > + * Copyright (c) 2017 Robert Munteanu
>> > + */
>> > +
>> > +/*
>> > + * This program is free software; you can redistribute it and/or
>> > modify it
>> > + * under the terms of the GNU General Public License as published
>> > by the Free
>> > + * Software Foundation; either version 2 of the License, or (at
>> > your option)
>> > + * any later version.
>> > + */
>> > +
>> > +#include <linux/device.h>
>> > +#include <linux/input.h>
>> > +#include <linux/hid.h>
>> > +#include <linux/module.h>
>> > +#include <linux/log2.h>
>> > +#include <linux/input-event-codes.h>
>> > +
>> > +#include "hid-ids.h"
>> > +
>> > +
>> > +// The microdia family of keyboards uses a non-standard way of
>> > sending
>> > +// modifier keys
>>
>> Unless it has changed since last time I checked, we do not use C++
>> comments
>> (see Documentation/process/coding-style.rst)
>
> Ack.
>
>>
>> > +//
>> > +// The down event always sets the first bit to 0x04 and the second
>> > keycode
>> > +// to a custom value. For instance, left shift sends the following
>> > bits
>> > +//
>> > +// 04 02 00 00 00 00 00 00
>> > +//
>> > +// while left control sends
>> > +//
>> > +// 04 01 00 00 00 00 00 00
>> > +//
>> > +// Unfortunately these are all mapped to left shift and the
>> > keyboard is
>> > +// non-functional in that respect. To solve the problem, we
>> > capture the
>> > +// raw data in raw_event and manually generate the correct input
>> > events
>> > +//
>> > +// TODO
>> > +//
>> > +// 1. Some modifiers keys still don't work, e.g. Ctrl-Shift-P
>> > +// Interestingly enough, this happens when pressing the
>> > physical
>> > +// left shift key, but now when pressing the physical right
>> > shift key.
>> > +// hexdump does not show them either.
>> > +// 2. A second USB keyboard is registered, but it should be
>> > removed
>> > +// 3. Modifier keys sometimes get stuck, need to re-press to clear
>> > +
>> > +static int microdia_input_configured(struct hid_device *hdev,
>> > + struct hid_input *hi)
>> > +{
>> > +
>> > + hid_info(hdev, "Keyboard configured with limited support
>> > (modifier keys may get stuck, some modifiers combinations with
>> > left-shift not working) ");
>> > +
>> > + hid_set_drvdata(hdev, hi);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static int microdia_input_mapping(struct hid_device *hdev, struct
>> > hid_input *hi,
>> > + struct hid_field *field, struct hid_usage *usage,
>> > + unsigned long **bit, int *max)
>> > +{
>> > + // do not map left shift since we will manually generate
>> > input
>> > + // events in microdia_raw_event
>> > + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_KEYBOARD)
>> > + if ((usage->hid & HID_USAGE) == 0xe1)
>> > + return -1;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +// array index of a modifier matches the bit index in the data
>> > payload
>> > +// the modifier data is always stored in the second int
>> > +// e.g. for left alt the 1 bit is set - 04 04 ...
>> > +// TODO - second entry should be left shift, but that's not
>> > possible
>> > +// since we ignore it in the mapping
>> > +static int microdia_modifiers[7] = {
>> > + KEY_LEFTCTRL,
>> > + KEY_RIGHTSHIFT,
>> > + KEY_LEFTALT,
>> > + KEY_LEFTMETA,
>> > + KEY_RIGHTCTRL,
>> > + KEY_RIGHTSHIFT,
>> > + KEY_RIGHTALT
>> > +};
>> > +
>> > +static int microdia_last_handled_modifier;
>> > +
>> > +static int microdia_raw_event(struct hid_device *hdev,
>> > + struct hid_report *report, u8 *data, int size)
>> > +{
>> > + int i, changed_key, new_value, mapped_key;
>> > + struct hid_input *hi;
>> > +
>> > + // 1. validate that we get 8 bits of the for 04 xx 00 00 00
>> > 00 00 00
>> > + if (size != 8)
>> > + return 0;
>> > +
>> > + if (data[0] != 4)
>> > + return 0;
>> > +
>> > + // TODO - can I somehow use a bit mask? data & 0x00ffffff
>> > != 0
>> > + for (i = 2; i < size; i++)
>> > + if (data[i] != 0)
>> > + return 0;
>> > +
>> > + // TODO - don't process the keyup event 04 00 00 00 00 00
>> > 00 00
>>
>> That's a lot of TODO in such a simple driver :/
>
> Yes, well that's what I get for learning to write hid drivers by
> example ... and C isn't my primary programming language either.
>
>>
>> > + // if we don't expect a modifier key 'up' event
>> > +
>> > + // 2. detect based on previous data what key was pressed or
>> > depressed
>> > + // a key was pressed or depressed if its bit has a
>> > different value
>> > + // between this and the previous invocation. If the bit is
>> > set
>> > + // it was pressed
>> > +
>> > + changed_key = data[1] ^ microdia_last_handled_modifier;
>> > + new_value = (data[1] & changed_key) != 0;
>> > + // TODO - is ilog2 really needed?
>> > + mapped_key = microdia_modifiers[ilog2(changed_key)];
>>
>> What if you get 2 changed keys at a time?
>
> This could explain the problems that I've been getting with modifier
> keys being 'stuck'.
>
>>
>> Anyway, before giving you a ACK or NACK on the driver, the behavior
>> from the keyboard firmware looks sane and pretty common. So my guess
>> is that there is something wrong in the report descriptors, and I
>> need
>> the hid-recorder output to have a better idea of what is wrong with
>> our current handling of this keyboard.
>
> The output is inline. That being said, I'm aware that re-firing the
> keypress events myself does not belong in a HID driver, but I'm eager
> to work on this being massaged into something proper for inclusion.
>
> Thanks for taking the time to review.
>
> Robert
>
>>
>> Cheers,
>> Benjamin
>>
>> > +
>> > + // 3. report the key event and track what was sent
>> > + hi = hid_get_drvdata(hdev);
>> > +
>> > + input_report_key(hi->input, mapped_key, new_value);
>> > +
>> > + microdia_last_handled_modifier = data[1];
>> > +
>> > + return 1;
>> > +}
>> > +
>> > +static const struct hid_device_id microdia_devices[] = {
>> > + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA,
>> > USB_DEVICE_ID_REDRAGON_ASURA)},
>> > + {}
>> > +};
>> > +
>> > +MODULE_DEVICE_TABLE(hid, microdia_devices);
>> > +
>> > +static struct hid_driver microdia_driver = {
>> > + .name = "microdia",
>> > + .input_mapping = microdia_input_mapping,
>> > + .id_table = microdia_devices,
>> > + .raw_event = microdia_raw_event,
>> > + .input_configured = microdia_input_configured,
>> > +};
>> > +
>> > +module_hid_driver(microdia_driver);
>> > +
>> > +MODULE_LICENSE("GPL");
>> > --
>> > 2.16.2
>> >
>

Next message: Alexandre Torgue: "Re: [PATCH v2] i2c: i2c-stm32f7: fix no check on returned setup"
Previous message: David Rientjes: "Re: [PATCH] memcg, thp: do not invoke oom killer on thp charges"
In reply to: Robert Munteanu: "Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard"
Next in thread: Robert Munteanu: "Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]