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 >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
