Re: [PATCH] HID: elecom: fix the descriptor of the EX-G trackball

Re: [PATCH] HID: elecom: fix the descriptor of the EX-G trackball

[Tomasz Kramkowski]

Since the device only has 6 physical buttons, I don't think unmasking
all 8 bits in the HID report descriptor is appropriate. If you do that
then userspace sees a mouse with 8 buttons and you never know when
software might actually behave differently depending on that number. (I
can imagine fancy mouse configuration program which might allow mouse
button remapping which would enable more button options depending on the
number of mouse buttons.)

I think some work could be done to modify the HID report descriptor
based on which model of trackball the code is working on. (It looks like
ELECOM used very similar descriptors across the board.)

I was going to do this just now actually but then I noticed that someone
had beat me to the punch with the EX-G (I was already surprised when I
found someone had patched the HUGE and DEFT).

On that note, here's some code I wrote a few months ago to address this
specific problem on this specific mouse and adapted just yesterday in
order to begin further work on the driver:

Feel free to re-use bits of it:

diff --git a/drivers/hid/hid-elecom.c b/drivers/hid/hid-elecom.c
index 54aeea57d209..5af0de38db2c 100644
--- a/drivers/hid/hid-elecom.c
+++ b/drivers/hid/hid-elecom.c
@@ -31,6 +37,17 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
                        rdesc[47] = 0x00;
                }
                break;
+       case USB_DEVICE_ID_ELECOM_M_XT3URBK:
+               if (*rsize >= 32 && rdesc[12] == 0x95 && rdesc[13] == 0x05 &&
+                   rdesc[14] == 0x75 && rdesc[15] == 0x01 &&
+                   rdesc[20] == 0x29 && rdesc[21] == 0x05 &&
+                   rdesc[30] == 0x75 && rdesc[31] == 0x03) {
+                       hid_info(hdev, "Fixing up Elecom M-XT3URBK report descriptor\n");
+                       rdesc[13] = 0x06;
+                       rdesc[21] = 0x06;
+                       rdesc[31] = 0x02;
+               }
+               break;

(Note: I just pulled this out of a diff with a bunch of other changes so
it might not apply cleanly.)

Aside from the naming of the PID macro, this should work just fine (in
fact, I tested it).

If you're wondering why I didn't publish this patch many months (a
year?) ago when I first wrote it, the reason is that I noticed that the
mouse has a FEATURE report.

I had thought that this rather bizarre FEATURE report could be used to
do something with the 4 other weird INPUT reports that the device has.

Upon further inspection of the FEATURE report I can't get it to actually
do anything AT ALL. I also noticed that the OSX mouse software Elecom
provides for this mouse doesn't actually touch that FEATURE report (or
well, I poked around in it using radare2, it would be helpful if someone
who has a windows/OSX machine and this mouse could confirm this but
after asking around, I couldn't find any volunteers). This FEATURE
report seems to have been a dead end in the end.

Shows you what happens when you're lazy and put things off until the
next day (or year)...

P.S. Apologies if I missed anything out or added anything extra to CC,
I've had to reply to this email without the original message by piecing
the information together out of the MHonArc header in the spinics.net
archive. This seems to be the only downside of mailing lists.

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/

Re: [PATCH] HID: elecom: fix the descriptor of the EX-G trackball

[Tomasz Kramkowski]

On Sat, Nov 18, 2017 at 10:27:26PM +0000, Tomasz Kramkowski wrote:
> I was going to do this just now actually but then I noticed that someone
> had beat me to the punch with the EX-G (I was already surprised when I
> found someone had patched the HUGE and DEFT).

Actually, I'll put my money where my mouth is.

Just some notes about the patch (maybe I talk too much, feel free to
skip the rambling):

I just tested this with the wired (I think there's a wireless variant
but I don't have it) ELECOM EX-G trackball mouse.

I've pulled in the previous relevant contributors to this file as CC.

I've dropped the big diff thing because I think that if you know the HID
spec you can probably understand most of what is happening from the
simple comment before the mouse_button_fixup function and the contents
of the function itself. Also, it would be a bit cumbersome to add a diff
for every potentially relevant device (I'm pretty sure the DEFT, HUGE
and EX-G aren't the only mice with this issue).

If removing the diff is a major problem then I propose just removing the
RHS and having just a visual representation of the relevant descriptor
fields.

At one point I thought of having a name parameter on mouse_button_fixup
which would vary the hid_info message depending on the device model but
then I decided that it was unnecessary complexity so I dropped it, but
if specifying EXACTLY which model of device the driver is fixing up is a
must then that's something that I can quickly add.

I've added a note about the aforementioned mystery FEATURE report to
hopefully prevent anyone else from going down that rabbit hole.

And finally, I didn't want to steal Yuxuan's thunder here so that's why
I didn't just send this in as a patch, but if he doesn't protest then
I'll just re-send it.

---
 drivers/hid/Kconfig      |  1 +
 drivers/hid/hid-core.c   |  1 +
 drivers/hid/hid-elecom.c | 76 +++++++++++++++++++++++++-----------------------
 drivers/hid/hid-ids.h    |  1 +
 4 files changed, 43 insertions(+), 36 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 779c5ae47f36..772f695d4e8c 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -280,6 +280,7 @@ config HID_ELECOM
 	---help---
 	Support for ELECOM devices:
 	  - BM084 Bluetooth Mouse
+	  - EX-G Trackball
 	  - DEFT Trackball (Wired and wireless)
 	  - HUGE Trackball (Wired and wireless)
 
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f3fcb836a1f9..18912c045816 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2034,6 +2034,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 #endif
 #if IS_ENABLED(CONFIG_HID_ELECOM)
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_M_XT3URBK) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRED) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRELESS) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRED) },
diff --git a/drivers/hid/hid-elecom.c b/drivers/hid/hid-elecom.c
index 54aeea57d209..d81cad44cc76 100644
--- a/drivers/hid/hid-elecom.c
+++ b/drivers/hid/hid-elecom.c
@@ -1,9 +1,15 @@
 /*
- *  HID driver for ELECOM devices.
+ *  HID driver for ELECOM devices:
+ *  - BM084 (bluetooth mouse)
+ *  - EX-G (trackball)
+ *  - DEFT (trackball, wired and wireless)
+ *  - HUGE (trackball, wired and wireless)
+ *
  *  Copyright (c) 2010 Richard Nauber <Richard.Nauber@gmail.com>
  *  Copyright (c) 2016 Yuxuan Shui <yshuiv7@gmail.com>
  *  Copyright (c) 2017 Diego Elio Pettenò <flameeyes@flameeyes.eu>
  *  Copyright (c) 2017 Alex Manoussakis <amanou@gnu.org>
+ *  Copyright (c) 2017 Tomasz Kramkowski <tk@the-tk.com>
  */
 
 /*
@@ -19,6 +25,34 @@
 
 #include "hid-ids.h"
 
+/*
+ * Certain ELECOM mice misreport their button count meaning that they only work
+ * correctly with the ELECOM mouse assistant software which is unavailable for
+ * Linux. Four extra INPUT reports and a FEATURE report are described by the
+ * report descriptor but it does not appear that these enable software to
+ * control what the extra buttons map to. The only simple and straightforward
+ * solution seems to involve fixing up the report descriptor.
+ *
+ * Report descriptor format:
+ * Positions 13, 15, 21 and 31 store the button bit count, button usage minimum,
+ * button usage maximum and padding bit count respectively.
+ */
+#define MOUSE_BUTTONS_MAX 8
+static void mouse_button_fixup(struct hid_device *hdev,
+			       __u8 *rdesc, unsigned int *rsize,
+			       int nbuttons)
+{
+	if (*rsize < 32 || rdesc[12] != 0x95 ||
+	    rdesc[14] != 0x75 || rdesc[15] != 0x01 ||
+	    rdesc[20] != 0x29 || rdesc[30] != 0x75)
+		return;
+	hid_info(hdev, "Fixing up Elecom mouse button count\n");
+	nbuttons = clamp(nbuttons, 0, MOUSE_BUTTONS_MAX);
+	rdesc[13] = nbuttons;
+	rdesc[21] = nbuttons;
+	rdesc[31] = MOUSE_BUTTONS_MAX - nbuttons;
+}
+
 static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
@@ -31,45 +65,14 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 			rdesc[47] = 0x00;
 		}
 		break;
+	case USB_DEVICE_ID_ELECOM_EX_G_WIRED:
+		mouse_button_fixup(hdev, rdesc, rsize, 6);
+		break;
 	case USB_DEVICE_ID_ELECOM_DEFT_WIRED:
 	case USB_DEVICE_ID_ELECOM_DEFT_WIRELESS:
 	case USB_DEVICE_ID_ELECOM_HUGE_WIRED:
 	case USB_DEVICE_ID_ELECOM_HUGE_WIRELESS:
-		/* The DEFT/HUGE trackball has eight buttons, but its descriptor
-		 * only reports five, disabling the three Fn buttons on the top
-		 * of the mouse.
-		 *
-		 * Apply the following diff to the descriptor:
-		 *
-		 * Collection (Physical),              Collection (Physical),
-		 *     Report ID (1),                      Report ID (1),
-		 *     Report Count (5),           ->      Report Count (8),
-		 *     Report Size (1),                    Report Size (1),
-		 *     Usage Page (Button),                Usage Page (Button),
-		 *     Usage Minimum (01h),                Usage Minimum (01h),
-		 *     Usage Maximum (05h),        ->      Usage Maximum (08h),
-		 *     Logical Minimum (0),                Logical Minimum (0),
-		 *     Logical Maximum (1),                Logical Maximum (1),
-		 *     Input (Variable),                   Input (Variable),
-		 *     Report Count (1),           ->      Report Count (0),
-		 *     Report Size (3),                    Report Size (3),
-		 *     Input (Constant),                   Input (Constant),
-		 *     Report Size (16),                   Report Size (16),
-		 *     Report Count (2),                   Report Count (2),
-		 *     Usage Page (Desktop),               Usage Page (Desktop),
-		 *     Usage (X),                          Usage (X),
-		 *     Usage (Y),                          Usage (Y),
-		 *     Logical Minimum (-32768),           Logical Minimum (-32768),
-		 *     Logical Maximum (32767),            Logical Maximum (32767),
-		 *     Input (Variable, Relative),         Input (Variable, Relative),
-		 * End Collection,                     End Collection,
-		 */
-		if (*rsize == 213 && rdesc[13] == 5 && rdesc[21] == 5) {
-			hid_info(hdev, "Fixing up Elecom DEFT/HUGE Fn buttons\n");
-			rdesc[13] = 8; /* Button/Variable Report Count */
-			rdesc[21] = 8; /* Button/Variable Usage Maximum */
-			rdesc[29] = 0; /* Button/Constant Report Count */
-		}
+		mouse_button_fixup(hdev, rdesc, rsize, 8);
 		break;
 	}
 	return rdesc;
@@ -77,6 +80,7 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static const struct hid_device_id elecom_devices[] = {
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRED) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRED) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRELESS) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRED) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5da3d6256d25..d757c78e7e15 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -370,6 +370,7 @@
 
 #define USB_VENDOR_ID_ELECOM		0x056e
 #define USB_DEVICE_ID_ELECOM_BM084	0x0061
+#define USB_DEVICE_ID_ELECOM_EX_G_WIRED	0x00fb
 #define USB_DEVICE_ID_ELECOM_DEFT_WIRED	0x00fe
 #define USB_DEVICE_ID_ELECOM_DEFT_WIRELESS	0x00ff
 #define USB_DEVICE_ID_ELECOM_HUGE_WIRED	0x010c
-- 
2.15.0

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/

Re: [PATCH] HID: elecom: fix the descriptor of the EX-G trackball

[Tomasz Kramkowski]

On Sun, Nov 19, 2017 at 12:31:45AM +0000, Diego Elio Pettenò wrote:
> Please do not drop the explicit documentation of the diff. Looking at what
> a driver does in three years is not going to be obvious, whether you know
> HID or not.

I'm sure there are situations where this is true but I've just gone
through the first six HID drivers in the order that grep found them in
when I searched for "report_fixup" and in all but one I was either
informed of the issue and the fix using a short and concise comment,
annotations on the replacement descriptor or the commit message. The one
report_fixup which was unclear had been moved out of hid-input-quirks.c
(when it was a thing) and I didn't actually feel like finding its
original commit message, it could probably have been clarified by a
rather simple comment.

Even the existing report_fixup of the BM084 has a one line comment which
entirely explains what the problem is.

I really don't think a big diagram showing the difference between two
report descriptors, which may or may not completely overlap with the
report descriptors of other devices exhibiting a similar issue, is
necessary.

Please also note that the documentation is not gone, it's just
shortened and generalised, there is still more than enough information
to work out exactly what the report change is.

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/