Re: [PATCH 1/2] HID: huion: enable button mode reporting

[Benjamin Tissoires]

On Feb 18 2015 or thereabouts, Nikolai Kondrashov wrote:
> On 02/18/2015 12:54 AM, Benjamin Tissoires wrote:
> >diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
> >index 61b68ca..50fbda4 100644
> >--- a/drivers/hid/hid-huion.c
> >+++ b/drivers/hid/hid-huion.c
> >@@ -34,6 +34,9 @@ enum huion_ph_id {
> >  	HUION_PH_ID_NUM
> >  };
> >
> >+/* header of a button report sent through the Pen report */
> >+static const u8 button_report[]  = {0x07, 0xa0, 0x01, 0x01};
> 
> Hmm, I see the second byte being 0xe0 on Huion H610, the rest is the same.
> Considering this, the fact that bit 7 is always 1 and bit 6 is pen proximity,
> I think we can assume that bit 5 in byte 2 indicates button reports and get
> away with just a "data[1] & 0x20" test.

that would be a nicer approach. Thanks for the analysis.
Actually, I understood the difference. I tested the bits _after_ the
driver reverts the in-range bit :)

The raw data is {0x07, 0xe0, 0x01, 0x01} on the H610 Pro too.

> 
> >  /* Report descriptor template placeholder */
> >  #define HUION_PH(_ID) HUION_PH_HEAD, HUION_PH_ID_##_ID
> >
> >@@ -81,6 +84,31 @@ static const __u8 huion_tablet_rdesc_template[] = {
> >  	HUION_PH(PRESSURE_LM),  /*          Logical Maximum (PLACEHOLDER),  */
> >  	0x81, 0x02,             /*          Input (Variable),               */
> >  	0xC0,                   /*      End Collection,                     */
> >+	0x05, 0x01,             /*      Usage Page (Desktop)                */
> >+	0x09, 0x07,             /*      Usage (Keypad)                      */
> >+	0xa1, 0x01,             /*      Collection (Application)            */
> >+	0x85, 0x08,             /*          Report ID (8)                   */
> >+	0x05, 0x0d,             /*          Usage Page (Digitizers)         */
> >+	0x09, 0x22,             /*          Usage (Finger)                  */
> 
> I'd say "Finger" usage is wrong here. The spec says:
> 
>     Finger
> 
>         CL â Any human appendage used as a transducer, such as a finger
>         touching a touch screen to set the location of the screen cursor.  A
>         digitizer typically reports the coordinates of center of the finger.
>         In the Finger collection a Pointer physical collection will contain
>         the axes reported by the finger.
> 
> I.e. the buttons are not a pointing device. The specification contains another
> collection usage which seems more suitable:
> 
>     Tablet Function Keys
> 
>         CL â These controls are located on the surface of a digitizing tablet,
>         and may be implemented as actual switches, or as soft keys actuated by
>         the digitizing transducer. These are often used to trigger
>         location-independent macros or other events.

Actually, the kernel knows about it: HID_DG_TABLETFUNCTIONKEY.
I don't think it should influence to have it set. The hid processing
would work on the BTN usages, not on the physical.

[5 min later]

yep, just works :)

> 
> However the kernel doesn't seem to know anything about it (but we can fix
> that). In my version of this I simply used a keyboard with buttons:
> 
>     0x05, 0x01,             /*  Usage Page (Desktop),                   */
>     0x09, 0x06,             /*  Usage (Keyboard),                       */
>     0xA1, 0x01,             /*  Collection (Application),               */
>     0x85, 0xF7,             /*      Report ID (247),                    */
>     0x05, 0x09,             /*      Usage Page (Button),                */
>     0x75, 0x01,             /*      Report Size (1),                    */
>     0x95, 0x18,             /*      Report Count (24),                  */
>     0x81, 0x03,             /*      Input (Constant, Variable),         */
>     0x19, 0x01,             /*      Usage Minimum (01h),                */
>     0x29, 0x08,             /*      Usage Maximum (08h),                */
>     0x95, 0x08,             /*      Report Count (8),                   */
>     0x81, 0x02,             /*      Input (Variable),                   */
>     0xC0                    /*  End Collection                          */
> 
> Although it might not be entirely correct either.

Even if no-one but hid-core uses the report descriptor, I would rather
not declare ourself as a keyboard. It's shooting on our own foot if
someone decides to actually merge a keyboard and a tablet.

> 
> >+	0xa0,                   /*          Collection (Physical)           */
> >+	0x14,                   /*              Logical Minimum (0)         */
> >+	0x25, 0x01,             /*              Logical Maximum (1)         */
> >+	0x75, 0x08,             /*              Report Size (8)             */
> >+	0x95, 0x03,             /*              Report Count (3)            */
> >+	0x81, 0x03,             /*              Input (Cnst,Var,Abs)        */
> >+	0x05, 0x09,             /*              Usage Page (Button)         */
> >+	0x19, 0x01,             /*              Usage Minimum (1)           */
> >+	0x29, 0x08,             /*              Usage Maximum (8)           */
> >+	0x14,                   /*              Logical Minimum (0)         */
> >+	0x25, 0x01,             /*              Logical Maximum (1)         */
> >+	0x75, 0x01,             /*              Report Size (1)             */
> >+	0x95, 0x08,             /*              Report Count (8)            */
> >+	0x81, 0x02,             /*              Input (Data,Var,Abs)        */
> >+	0x75, 0x08,             /*              Report Size (8)             */
> >+	0x95, 0x03,             /*              Report Count (3)            */
> >+	0x81, 0x03,             /*              Input (Cnst,Var,Abs)        */
> >+	0xc0,                   /*          End Collection                  */
> >+	0xc0,                   /*      End Collection                      */
> 
> Which tool did you use to generate this?

My own custom-made:
https://github.com/bentiss/hid-replay/blob/master/tools/editor.py

not 100% implemented, but it works for me :)

> 
> >  	0xC0                    /*  End Collection                          */
> >  };
> >
> >@@ -205,6 +233,25 @@ static int huion_tablet_enable(struct hid_device *hdev)
> >  		}
> >  	}
> >
> >+	/* switch to the button mode reporting */
> >+	rc = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
> >+				USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
> >+				(USB_DT_STRING << 8) + 0x7b,
> >+				0x0409, buf, len,
> >+				USB_CTRL_GET_TIMEOUT);
> 
> I'm a bit uncomfortable about reusing a buffer which was sized specifically
> for another task, as it's confusing. But it will work as is, so it's OK.

Yes, and no :)

Actually, I would prefer that we stick to what the Windows driver do.
But it requests 32 bytes in each requests, and we receive 14 and 22
IIRC. The trick I exploited here is that the ctrl message answers back
at most len data, so we are find in both cases because 12 is less than
14 and 22. I am not sure we should check at all the length of the
returning buffer (though for the first command, we have to be sure that
we received enough to get the values in the buffer).

Side note: the huion-abstract-keyboard branch uses usb_string() instead
of a plain usb_control_msg(). I like this much better and I think we
should change the first call with that. This way, it will be clear that
the tablet is not fully HID compatible and that we need to keep the usb
access.

> 
> >+	if (rc == -EPIPE) {
> >+		hid_err(hdev, "button mode switch not found\n");
> >+		rc = -ENODEV;
> >+		goto cleanup;
> >+	} else if (rc < 0) {
> >+		hid_err(hdev, "failed to switch to button mode: %d\n", rc);
> >+		rc = -ENODEV;
> >+		goto cleanup;
> >+	} else if (rc != len) {
> >+		hid_err(hdev, "invalid button mode switch\n");
> >+		rc = -ENODEV;
> >+		goto cleanup;
> >+	}
> >  	rc = 0;
> >
> >  cleanup:
> >@@ -262,10 +309,16 @@ static int huion_raw_event(struct hid_device *hdev, struct hid_report *report,
> >  	/* If this is a pen input report */
> >  	if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
> >  	    report->type == HID_INPUT_REPORT &&
> >-	    report->id == 0x07 && size >= 2)
> >+	    report->id == 0x07 && size >= 2) {
> >  		/* Invert the in-range bit */
> >  		data[1] ^= 0x40;
> >
> >+		/* check for buttons events and change the report ID */
> >+		if (size >= sizeof(button_report) &&
> >+		    !memcmp(data, button_report, sizeof(button_report)))
> 
> So, yes, I think it's better to have a "data[1] & 0x20" test here instead.

Yep, works just fine.

Cheers,
Benjamin

> 
> >+			data[0] = 0x08;
> >+	}
> >+
> >  	return 0;
> >  }
> >
> >
> 
> Nick
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/