Fix modifier keys for Redragon Asura Keyboard
PATCH v3 1/1 Fix modifier keys for Redragon Asura Keyboard
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 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
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.
The keyboard functions mostly as expected now, with only a few minor issues:
- two USB devices are detected instead of 1
- some key combinations are not triggered - e.g.
left shift + left ctrl + p. However, the same combination is recognized with the right shift key.
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
--- 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
drivers/hid/Kconfig +9 -0
@@ -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
drivers/hid/Makefile +1 -1
@@ -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/
drivers/hid/hid-core.c +3 -0
@@ -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_ASURA) }, #endif { } };
drivers/hid/hid-ids.h +3 -0
@@ -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
drivers/hid/hid-microdia.c new file +148 -0
148 lines not on this page · raw message
Signature · 1 line
2.16.2
Hi Robert,
First, apologies for not answering to the RFC. I missed it and it fell down in my INBOX.
Quoted 4 lines · Robert Munteanu
The microdia family of keyboards uses a non-standard way of sending modifier keys.
The down event always sets the first bit to
0x04and 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.
Quoted 7 lines
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.
Quoted 4 lines
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_eventand 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/
Quoted 5 lines
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
Quoted 3 lines
- 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?
more comments inlined:
Quoted 60 lines
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.15Signed-off-by Robert Munteanu <[email protected]>--- 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.cdrivers/hid/Kconfig +9
-0@@ -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 HIDdrivers/hid/Makefile +1
-1@@ -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/drivers/hid/hid-core.c +0
-0
drivers/hid/hid-core.c static const struct hid_device_id hid_have_special_driver[] = { · line 2399
Quoted 7 lines
drivers/hid/hid-core.c +3
-0@@ -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_ASURA) }, #endif
You don't need this hunk anymore since v4.15 IIRC
drivers/hid/hid-microdia.c
Quoted 20 lines
drivers/hid/hid-core.c +0
-0{ } };drivers/hid/hid-ids.h +3
-0@@ -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 drivers/hid/hid-microdia.c new file +0
-0@@ -0,0 +1,148 @@
missing the SPDX identifier
(see Documentation/process/license-rules.rst)
Quoted 15 lines
drivers/hid/hid-microdia.c new file +15
-0+/* +* 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>
drivers/hid/hid-microdia.c line 25
Quoted 10 lines
drivers/hid/hid-microdia.c new file +10
-0+#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)
Quoted 73 lines
drivers/hid/hid-microdia.c new file +73
-0+// +// 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 ;
drivers/hid/hid-microdia.c line 108
Quoted 10 lines
drivers/hid/hid-microdia.c new file +10
-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 :/
drivers/hid/hid-microdia.c line 119
Quoted 11 lines
drivers/hid/hid-microdia.c new file +11
-0+// 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?
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.
Cheers,
Benjamin
Quoted 32 lines
drivers/hid/hid-microdia.c new file +29
-1+ +// 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
Hi Benjamin,
Quoted 11 lines · Benjamin Tissoires
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 <[email protected]
Quoted 4 lines
wrote:
The microdia family of keyboards uses a non-standard way of sending modifier keys.
Quoted 5 lines
Quoted 2 lines
The down event always sets the first bit to
0x04and the second keycodePretty 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.
Quoted 25 lines
Quoted 8 lines
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_SHIFTand bit(1) mapped toLEFT_CONTROLin the second byte.
Fortunately, LeftControl is designed in HID as0xe0in the keyboard HID usage table, and LeftShift is0xe1. 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.Quoted 7 lines
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 inraw_eventand manually generate the correct input events.
Quoted 5 lines
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
Quoted 9 lines
Quoted 7 lines
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.
Quoted 7 lines
Quoted 5 lines
- 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?
Quoted 71 lines
more comments inlined:
Quoted 69 lines
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.15Signed-off-by Robert Munteanu <[email protected]>--- 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.cdrivers/hid/Kconfig +0
-0@@ -548,6 +548,15 @@ config HID_MAYFLASH Say Y here if you have HJZ Mayflash PS3 game controlleradapters
and want to enable force feedback support.drivers/hid/Kconfig +6
-0+config HID_MICRODIA + tristate "Microdia based keyboards" + depends on HID + default !EXPERT + ---help--- + Support for Microdia devices that are not compliant with theHID standard.
drivers/hid/Kconfig +3
-0+ + One known example if the Redragon Asura Keyboard. + config HID_MICROSOFT tristate "Microsoft non-fully HID-compliant devices" depends on HIDdrivers/hid/Makefile +0
-0@@ -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.odrivers/hid/Makefile +1
-1+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/drivers/hid/hid-core.c +0
-0@@ -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) },
Quoted 8 lines
Quoted 6 lines
drivers/hid/hid-core.c +3
-0+#endif +#if IS_ENABLED (CONFIG_HID_MICRODIA ) + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA,
USB_DEVICE_ID_REDRAGON_ASUR
A) },
#endifYou don't need this hunk anymore since v4.15 IIRC
Ack, will update and retest.
Quoted 11 lines
Quoted 11 lines
{ }};
drivers/hid/hid-ids.h +0
-0@@ -1171,4 +1171,7 @@#define USB_VENDOR_ID_UGTIZER 0x2179#define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053
Quoted 14 lines
drivers/hid/hid-microdia.c
Quoted 11 lines
drivers/hid/hid-ids.h +3
-0+#define USB_VENDOR_ID_MICRODIA 0x0c45 +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b +#endif drivers/hid/hid- renamed from drivers/hid/hid-microdia.c +0
-0
microdia.c
new file mode 100644
index 000000000000..f9d8de18a989drivers/hid/hid-microdia.c new file +0
-0@@ -0,0 +1,148 @@missing the SPDX identifier
(seeDocumentation/process/license-rules.rst)
Ack, will update.
Quoted 26 lines
Quoted 26 lines
drivers/hid/hid-microdia.c new file +8
-0+/* +* 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.
+ */drivers/hid/hid-microdia.c new file +10
-0+ +#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" +
Quoted 8 lines
Quoted 4 lines
drivers/hid/hid-microdia.c new file +2
-0+ +// The microdia family of keyboards uses a non-standard way of sending
+// modifier keysUnless it has changed since last time I checked, we do not use C++ comments
(seeDocumentation/process/coding-style.rst)
Ack.
Quoted 95 lines
Quoted 95 lines
drivers/hid/hid-microdia.c new file +2
-0+// +// 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 bitsdrivers/hid/hid-microdia.c new file +8
-0+// +// 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 inraw_eventand manually generate the correct input eventsdrivers/hid/hid-microdia.c new file +5
-0+// +// 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.drivers/hid/hid-microdia.c new file +2
-0+// hexdump does not show them either. +// 2. A second USB keyboard is registered, but it should be removed
drivers/hid/hid-microdia.c new file +7
-0+// 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) ");
drivers/hid/hid-microdia.c new file +7
-0+ +hid_set_drvdata (hdev, hi); + +return 0 ; +} + +static int microdia_input_mapping(struct hid_device *hdev, struct
hid_input*hi,drivers/hid/hid-microdia.c new file +4
-0+ struct hid_field *field, struct hid_usage *usage, + unsigned long **bit, int *max) +{ +// do not map left shift since we will manually generate input
drivers/hid/hid-microdia.c new file +9
-0+// 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
drivers/hid/hid-microdia.c new file +3
-0+// 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
drivers/hid/hid-microdia.c new file +20
-0+// 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
drivers/hid/hid-microdia.c new file +7
-0+if (size !=8 ) +return 0 ; + +if (data[0 ] !=4 ) +return 0 ; + +// TODO - can I somehow use a bit mask? data & 0x00ffffff != 0
Quoted 8 lines
Quoted 6 lines
drivers/hid/hid-microdia.c new file +5
-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.
Quoted 17 lines
Quoted 9 lines
drivers/hid/hid-microdia.c new file +3
-0+// 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
setdrivers/hid/hid-microdia.c line 119
Quoted 6 lines
drivers/hid/hid-microdia.c new file +6
-0+// 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'.
Quoted 7 lines
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
Quoted 37 lines
Cheers,
BenjaminQuoted 33 lines
drivers/hid/hid-microdia.c new file +13
-0+ +// 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)},drivers/hid/hid-microdia.c new file +16
-1+ {} +}; + +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
Quoted 75 lines · Robert Munteanu
Hi Benjamin,
Quoted 11 lines · Benjamin Tissoires
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 <[email protected]
Quoted 4 lines
wrote:
The microdia family of keyboards uses a non-standard way of sending modifier keys.Quoted 5 lines
Quoted 2 lines
The down event always sets the first bit to
0x04and the second keycodePretty 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.
Quoted 25 lines
Quoted 8 lines
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_SHIFTand bit(1) mapped toLEFT_CONTROLin the second byte.
Fortunately, LeftControl is designed in HID as0xe0in the keyboard HID usage table, and LeftShift is0xe1. 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.Quoted 7 lines
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 inraw_eventand manually generate the correct input events.Quoted 5 lines
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
Quoted 8 lines
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.
Quoted 37 lines
Quoted 10 lines
Quoted 8 lines
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.
Quoted 7 lines
Quoted 5 lines
- 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 00On 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
Quoted 2 lines
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
Quoted 330 lines
Quoted 72 lines
more comments inlined:
Quoted 69 lines
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.15Signed-off-by Robert Munteanu <[email protected]>--- 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.cdrivers/hid/Kconfig +0
-0@@ -548,6 +548,15 @@ config HID_MAYFLASH Say Y here if you have HJZ Mayflash PS3 game controlleradapters
and want to enable force feedback support.drivers/hid/Kconfig +6
-0+config HID_MICRODIA + tristate "Microdia based keyboards" + depends on HID + default !EXPERT + ---help--- + Support for Microdia devices that are not compliant with theHID standard.
drivers/hid/Kconfig +3
-0+ + One known example if the Redragon Asura Keyboard. + config HID_MICROSOFT tristate "Microsoft non-fully HID-compliant devices" depends on HIDdrivers/hid/Makefile +0
-0@@ -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.odrivers/hid/Makefile +1
-1+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/drivers/hid/hid-core.c +0
-0@@ -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) },Quoted 8 lines
Quoted 6 lines
drivers/hid/hid-core.c +3
-0+#endif +#if IS_ENABLED (CONFIG_HID_MICRODIA ) + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA,
USB_DEVICE_ID_REDRAGON_ASUR
A) },
#endifYou don't need this hunk anymore since v4.15 IIRC
Ack, will update and retest.
Quoted 11 lines
Quoted 11 lines
{ }};
drivers/hid/hid-ids.h +0
-0@@ -1171,4 +1171,7 @@#define USB_VENDOR_ID_UGTIZER 0x2179#define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053Quoted 14 lines
drivers/hid/hid-microdia.c
Quoted 11 lines
drivers/hid/hid-ids.h +3
-0+#define USB_VENDOR_ID_MICRODIA 0x0c45 +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b +#endif drivers/hid/hid- renamed from drivers/hid/hid-microdia.c +0
-0
microdia.c
new file mode 100644
index 000000000000..f9d8de18a989drivers/hid/hid-microdia.c new file +0
-0@@ -0,0 +1,148 @@missing the SPDX identifier
(seeDocumentation/process/license-rules.rst)Ack, will update.
Quoted 26 lines
Quoted 26 lines
drivers/hid/hid-microdia.c new file +8
-0+/* +* 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.
+ */drivers/hid/hid-microdia.c new file +10
-0+ +#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" +Quoted 8 lines
Quoted 4 lines
drivers/hid/hid-microdia.c new file +2
-0+ +// The microdia family of keyboards uses a non-standard way of sending
+// modifier keysUnless it has changed since last time I checked, we do not use C++ comments
(seeDocumentation/process/coding-style.rst)Ack.
Quoted 95 lines
Quoted 95 lines
drivers/hid/hid-microdia.c new file +2
-0+// +// 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 bitsdrivers/hid/hid-microdia.c new file +8
-0+// +// 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 inraw_eventand manually generate the correct input eventsdrivers/hid/hid-microdia.c new file +5
-0+// +// 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.drivers/hid/hid-microdia.c new file +2
-0+// hexdump does not show them either. +// 2. A second USB keyboard is registered, but it should be removed
drivers/hid/hid-microdia.c new file +7
-0+// 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) ");
drivers/hid/hid-microdia.c new file +7
-0+ +hid_set_drvdata (hdev, hi); + +return 0 ; +} + +static int microdia_input_mapping(struct hid_device *hdev, struct
hid_input*hi,drivers/hid/hid-microdia.c new file +4
-0+ struct hid_field *field, struct hid_usage *usage, + unsigned long **bit, int *max) +{ +// do not map left shift since we will manually generate input
drivers/hid/hid-microdia.c new file +9
-0+// 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
drivers/hid/hid-microdia.c new file +3
-0+// 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
drivers/hid/hid-microdia.c new file +20
-0+// 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
drivers/hid/hid-microdia.c new file +7
-0+if (size !=8 ) +return 0 ; + +if (data[0 ] !=4 ) +return 0 ; + +// TODO - can I somehow use a bit mask? data & 0x00ffffff != 0
Quoted 8 lines
Quoted 6 lines
drivers/hid/hid-microdia.c new file +5
-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.
Quoted 17 lines
Quoted 9 lines
drivers/hid/hid-microdia.c new file +3
-0+// 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
setdrivers/hid/hid-microdia.c line 119
Quoted 6 lines
drivers/hid/hid-microdia.c new file +6
-0+// 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'.
Quoted 7 lines
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
Quoted 37 lines
Cheers,
BenjaminQuoted 33 lines
drivers/hid/hid-microdia.c new file +13
-0+ +// 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)},drivers/hid/hid-microdia.c new file +16
-1+ {} +}; + +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
Quoted 159 lines · Benjamin Tissoires
Quoted 93 lines · Robert Munteanu
Hi Benjamin,
Quoted 13 lines · Benjamin Tissoires
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
Quoted 5 lines
wrote:
The microdia family of keyboards uses a non-standard way of sending
modifier keys.Quoted 5 lines
Quoted 2 lines
The down event always sets the first bit to
0x04and the second keycodePretty 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.
Quoted 30 lines
Quoted 9 lines
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_SHIFTand bit(1) mapped toLEFT_CONTROLin the second byte.
Fortunately, LeftControl is designed in HID as0xe0in the keyboard
HID usage table, and LeftShift is0xe1. 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.Quoted 8 lines
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 inraw_eventand manually generate the correct input events.Quoted 8 lines
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 00Quoted 8 lines
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 00Thanks. 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) 780x09,0x06, // Usage
(Keyboard) 800xa1,0x01, // Collection
(Application) 820x85,0x04, // Report ID
(4) 840x05,0x07, // Usage Page
(Keyboard) 860x19,0xe0, // Usage Minimum
(224) 880x29,0xe7, // Usage Maximum
(231) 900x15,0x00, // Logical Minimum
(0) 920x25,0x01, // Logical Maximum
(1) 940x75,0x01, // Report Size
(1) 960x95,0x08, // Report Count
(8) 980x81,0x00, // Input
(Data,Arr,Abs) 1000x95,0x30, // Report Count
(48) 1020x75,0x01, // Report Size
(1) 1040x15,0x00, // Logical Minimum
(0) 1060x25,0x01, // Logical Maximum
(1) 1080x05,0x07, // Usage Page
(Keyboard) 1100x19,0x00, // Usage Minimum
(0) 1120x29,0x2f, // Usage Maximum
(47) 1140x81,0x02, // Input
(Data,Var,Abs) 1160xc0, // End
Collection 118the 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').
Quoted 3 lines
The best solution for you is to introduce a report descriptor fixup. See
hid-aureal.cin the kernel tree for an example of a simple report descriptor fixup.
Yes, that works! I applied a descriptor fixup and that is the only change actually neded - I dropped all my other code.
I had a sense that the code was not the right approach and that the problem was likely in the HID descriptor or HID parsing, but I was unable to advance. Thanks for the extensive description.
Quoted 38 lines
Quoted 38 lines
Quoted 11 lines
Quoted 9 lines
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.
Quoted 7 lines
Quoted 5 lines
- 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 00On 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
Quoted 6 lines
Quoted 3 lines
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?
I will need to double-check next week, I don't have a Windows machine at hand right now.
Thanks,
Robert
Quoted 361 lines
Cheers,
BenjaminQuoted 357 lines
Quoted 78 lines
more comments inlined:
Quoted 75 lines
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.15Signed-off-by Robert Munteanu <[email protected]>--- 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.cdrivers/hid/Kconfig +0
-0@@ -548,6 +548,15 @@ config HID_MAYFLASH Say Y here if you have HJZ Mayflash PS3 game controlleradapters
and want to enable force feedback support.drivers/hid/Kconfig +6
-0+config HID_MICRODIA + tristate "Microdia based keyboards" + depends on HID + default !EXPERT + ---help--- + Support for Microdia devices that are not compliant withthe
HID standard.drivers/hid/Kconfig +3
-0+ + One known example if the Redragon Asura Keyboard. + config HID_MICROSOFT tristate "Microsoft non-fully HID-compliant devices" depends on HIDdrivers/hid/Makefile +0
-0@@ -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.odrivers/hid/Makefile +1
-0+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/
drivers/hid/Makefile +0
-1-obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/drivers/hid/hid-core.c +1
-0@@ -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) }, +#endifQuoted 8 lines
Quoted 6 lines
drivers/hid/hid-core.c +2
-0+#if IS_ENABLED (CONFIG_HID_MICRODIA ) + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA,
USB_DEVICE_ID_REDRAGON_ASUR
A) },
#endifYou don't need this hunk anymore since v4.15 IIRC
Ack, will update and retest.
Quoted 11 lines
Quoted 11 lines
{ }};
drivers/hid/hid-ids.h +0
-0@@ -1171,4 +1171,7 @@#define USB_VENDOR_ID_UGTIZER 0x2179#define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053Quoted 14 lines
drivers/hid/hid-microdia.c
Quoted 11 lines
drivers/hid/hid-ids.h +3
-0+#define USB_VENDOR_ID_MICRODIA 0x0c45 +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b +#endif drivers/hid/hid- renamed from drivers/hid/hid-microdia.c +0
-0
microdia.c
new file mode 100644
index 000000000000..f9d8de18a989drivers/hid/hid-microdia.c new file +0
-0@@ -0,0 +1,148 @@missing the SPDX identifier
(seeDocumentation/process/license-rules.rst)Ack, will update.
Quoted 30 lines
Quoted 30 lines
drivers/hid/hid-microdia.c new file +8
-0+/* +* 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.
+ */drivers/hid/hid-microdia.c new file +11
-0+ +#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" + +Quoted 8 lines
Quoted 3 lines
+// The microdia family of keyboards uses a non-standard way of sending
+// modifier keysUnless it has changed since last time I checked, we do not use C++
comments
(seeDocumentation/process/coding-style.rst)Ack.
Quoted 106 lines
Quoted 106 lines
drivers/hid/hid-microdia.c new file +2
-0+// +// 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
bitsdrivers/hid/hid-microdia.c new file +8
-0+// +// 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 inraw_eventand manually generate the correct input
eventsdrivers/hid/hid-microdia.c new file +5
-0+// +// 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.drivers/hid/hid-microdia.c new file +2
-0+// 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 cleardrivers/hid/hid-microdia.c new file +6
-0+ +static int microdia_input_configured (struct hid_device *hdev, +struct hid_input *hi) +{ + + hid_info(hdev, "Keyboard configured with limitedsupport
(modifier keys may get stuck, some modifiers combinations with left-shift not working) ");drivers/hid/hid-microdia.c new file +7
-0+ +hid_set_drvdata (hdev, hi); + +return 0 ; +} + +static int microdia_input_mapping(struct hid_device *hdev,struct
hid_input*hi,
+ structhid_field*field, structhid_usage
*usage,drivers/hid/hid-microdia.c new file +3
-0+ unsigned long **bit, int *max) +{ +// do not map left shift since we will manually generate
inputdrivers/hid/hid-microdia.c new file +9
-0+// 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
drivers/hid/hid-microdia.c new file +3
-0+// 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
drivers/hid/hid-microdia.c new file +20
-0+// 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 00drivers/hid/hid-microdia.c new file +7
-0+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++)Quoted 8 lines
Quoted 6 lines
drivers/hid/hid-microdia.c new file +4
-0+if (data[i] !=0 ) +return 0 ; + +// TODO - don't process the keyup event 04 00 00 00 00 00
00 00That'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.
Quoted 11 lines
Quoted 11 lines
drivers/hid/hid-microdia.c new file +3
-0+// 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
setQuoted 8 lines
drivers/hid/hid-microdia.c line 119
Quoted 6 lines
drivers/hid/hid-microdia.c new file +6
-0+// 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'.Quoted 10 lines
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
Quoted 37 lines
Cheers,
BenjaminQuoted 33 lines
drivers/hid/hid-microdia.c new file +13
-0+ +// 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)},drivers/hid/hid-microdia.c new file +16
-1+ {} +}; + +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
drivers/hid/hid-ids.h line 1175
Quoted 2 lines · On Wednesday 21 March 2018 11:43:31 Robert Munteanu wrote:
drivers/hid/hid-ids.h +2
-0+#define USB_VENDOR_ID_MICRODIA 0x0c45 +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
Microdia is most probably an incorrect name. The 0c45 id probably belongs to "Sonix Technology Co., Ltd."
Signature · 1 line
Ondrej Zary
Quoted 7 lines · Ondrej Zary
drivers/hid/hid-ids.h line 1175
Quoted 2 lines · On Wednesday 21 March 2018 11:43:31 Robert Munteanu wrote:
drivers/hid/hid-ids.h +2
-0+#define USB_VENDOR_ID_MICRODIA 0x0c45 +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760bMicrodia is most probably an incorrect name. The 0c45 id probably belongs
to "Sonix Technology Co., Ltd."
Right, I was unsure about that. I'll remove the microdia references and use sonix.
Robert
Quoted 9 lines · Robert Munteanu
Quoted 7 lines · Ondrej Zary
drivers/hid/hid-ids.h line 1175
Quoted 2 lines · On Wednesday 21 March 2018 11:43:31 Robert Munteanu wrote:
drivers/hid/hid-ids.h +2
-0+#define USB_VENDOR_ID_MICRODIA 0x0c45 +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760bMicrodia is most probably an incorrect name. The 0c45 id probably belongs
to "Sonix Technology Co., Ltd."
Quoted 3 lines
Right, I was unsure about that. I'll remove the microdia references and
use sonix.
I see the following in hid-ids.h
define USB_VENDOR_ID_JESS 0x0c45
which I guess I should use since it's already defined in there.
Robert