Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <c2196c49d44693b5b0950132ceb42b7910c462dd.camel@apache.org>
References: <20180321104331.22783-1-rombert@apache.org> <CAO-hwJJRZkpqiFr1j5U_x0pgFP3RPN0w_cfKJvKMz_QznH87cA@mail.gmail.com>
 <c2196c49d44693b5b0950132ceb42b7910c462dd.camel@apache.org>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Thu, 22 Mar 2018 09:33:19 +0100
Message-ID: <CAO-hwJL3RJMOaTT9=Vv3_3+wVSVyTD+A4Qk34+uQzzkcEH=btg@mail.gmail.com>
Subject: Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
To:     Robert Munteanu <rombert@apache.org>
Cc:     Jiri Kosina <jikos@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 21, 2018 at 5:33 PM, Robert Munteanu <rombert@apache.org> 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@apache.org
>> > 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@apache.org>
>> > ---
>> >  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
>> >
>