Re: [DIGImend-devel] [PATCH 2/2] huion: Switch to reporting abstract button events

[Benjamin T. <bti...@re...>]

----- Original Message -----
> From: "Nikolai Kondrashov" <sp...@gm...>
> To: "Benjamin Tissoires" <ben...@re...>, "DIGImend-devel" <DIG...@li...>
> Sent: Sunday, March 15, 2015 12:02:41 PM
> Subject: Re: [DIGImend-devel] [PATCH 2/2] huion: Switch to reporting abstract button events
> 
> On 03/13/2015 11:28 PM, Benjamin Tissoires wrote:
> > Based on a patch from: Nikolai Kondrashov <Nik...@re...>
> >
> > Enable abstract keyboard mode for Huion tablets, which makes them report
> > frame buttons using the pen interface and report ID. Divert these
> > reports to a virtual report ID describing them.
> >
> > This makes the tablet compatible with xf86-input-wacom and libinput,
> > but stops the frame buttons from reporting keyboard events.
> > ---
> >
> > Sorry it took so long to actually propose this patch. I am fixing at the
> > moment
> > gnome-control-center, gnome-settings-daemon and libwacom to be able to
> > handle
> > also the generic devices like the Huion ones.
> 
> This is excellent news which we'll be welcome by all users, I'm sure.

Yeah. It turns out that there was a little bit of work to do to be able to support these tablets. Most of the problem came from the fact that the tablets don't have an eraser and gnome-control-center rejected such tablets.

I think I'll make an announce here once I got the bugs fixed, but I think it should be safe to assume that gnome 3.16 will be able to handle the Huion tablets seamlessly. And I'll make sure Fedora 22 get all the patches. We will need to upgrade libwacom too, and I already started adding the Huion H610 Pro (with the button layout) to it:
http://sourceforge.net/p/linuxwacom/mailman/linuxwacom-devel/thread/1426281693-12291-3-git-send-email-benjamin.tissoires%40redhat.com/#msg33595219

We will then have to add the various tablets we want to have a nice Button mapping gui to libwacom.
If the entry is not in libwacom, the button mapping won't be configurable by the user, but the tablet will show up in g-c-c.

> 
> > I need to go through all the Huion known device and see what they answer
> > when
> > querying the string 0x7b, but I believe checking if "HK On" is returned
> > should
> > allow us to prevent activating a pad interface if the device does not
> > support
> > it.
> 
> We can add requesting that string to huion-tools:
> 
>          https://github.com/DIGImend/huion-tools
> 
> And then ask people to run it and report results.
> 
> I should probably rename that to uclogic-tools, BTW.
> 
> > I am quite tempted to send this one upstream directly too. So please, test
> > it
> > and report if there are obvious problems.
> 
> I understand :) Once we get this merged and I clean up the general state and
> update the README.md, I'll make a release and people will test it.
> 
> Nick
> 
> > Cheers,
> > Benjamin
> >
> >   hid-uclogic.c | 103
> >   +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 98 insertions(+), 5 deletions(-)
> >
> > diff --git a/hid-uclogic.c b/hid-uclogic.c
> > index 88c0914..ea57d9f 100644
> > --- a/hid-uclogic.c
> > +++ b/hid-uclogic.c
> > @@ -613,6 +613,27 @@ static const __u8 uclogic_tablet_rdesc_template[] = {
> >   	0xC0                    /*  End Collection                          */
> >   };
> >
> > +/* Fixed report descriptor template */
> > +static const __u8 uclogic_buttonpad_rdesc_template[] = {
> 
> This isn't a template, but just a descriptor. I think it is better to drop
> "template" from here.

Sure, will fix.

> 
> > +	0x05, 0x01,             /*  Usage Page (Desktop),                   */
> > +	0x09, 0x07,             /*  Usage (Keypad),                         */
> > +	0xA1, 0x01,             /*  Collection (Application),               */
> > +	0x85, 0xF7,             /*      Report ID (247),                    */
> > +	0x05, 0x0D,             /*      Usage Page (Digitizers),            */
> > +	0x09, 0x39,             /*      Usage (Tablet Function Keys),       */
> > +	0xA0,                   /*      Collection (Physical),              */
> > +	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                      */
> > +	0xC0                    /*  End Collection                          */
> > +};
> > +
> >   /* Parameter indices */
> >   enum uclogic_prm {
> >   	UCLOGIC_PRM_X_LM	= 1,
> > @@ -628,6 +649,7 @@ struct uclogic_drvdata {
> >   	unsigned int rsize;
> >   	bool invert_pen_inrange;
> >   	bool ignore_pen_interface;
> > +	bool has_virtual_pad_interface;
> >   };
> >
> >   static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > @@ -874,6 +896,70 @@ cleanup:
> >   	return rc;
> >   }
> >
> > +/**
> > + * Enable actual button mode.
> > + *
> > + * @hdev:	HID device
> > + */
> > +static int uclogic_button_enable(struct hid_device *hdev)
> > +{
> > +	int rc;
> > +	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
> > +	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
> > +	char *str_buf;
> > +	size_t str_len = 16;
> > +	unsigned char *rdesc;
> > +	size_t rdesc_len;
> > +
> > +	str_buf = kzalloc(str_len, GFP_KERNEL);
> > +	if (str_buf == NULL) {
> > +		hid_err(hdev, "failed to allocate string buffer\n");
> > +		rc = -ENOMEM;
> > +		goto cleanup;
> > +	}
> > +
> > +	/* Enable abstract keyboard mode */
> > +	rc = usb_string(usb_dev, 0x7b, str_buf, str_len);
> > +	if (rc == -EPIPE) {
> > +		hid_info(hdev, "button mode setting not found\n");
> > +		rc = 0;
> > +		goto cleanup;
> > +	} else if (rc < 0) {
> > +		hid_err(hdev, "failed to enable abstract keyboard\n");
> > +		goto cleanup;
> > +	} else if (strncmp(str_buf, "HK On", rc)) {
> > +		hid_info(hdev, "invalid answer when requesting buttons: '%s'\n",
> > +			str_buf);
> > +		rc = -EINVAL;
> > +		goto cleanup;
> > +	}
> > +
> > +	/* Re-allocate fixed report descriptor */
> > +	rdesc_len = drvdata->rsize + sizeof(uclogic_buttonpad_rdesc_template);
> > +	rdesc = devm_kzalloc(&hdev->dev, rdesc_len, GFP_KERNEL);
> > +	if (!rdesc) {
> > +		rc = -ENOMEM;
> > +		goto cleanup;
> > +	}
> > +
> > +	memcpy(rdesc, drvdata->rdesc, drvdata->rsize);
> > +
> > +	/* Append the buttonpad descriptor */
> > +	memcpy(rdesc + drvdata->rsize, uclogic_buttonpad_rdesc_template,
> > +	       sizeof(uclogic_buttonpad_rdesc_template));
> > +
> > +	/* clean up old rdesc and use the new one */
> > +	drvdata->rsize = rdesc_len;
> > +	devm_kfree(&hdev->dev, drvdata->rdesc);
> > +	drvdata->rdesc = rdesc;
> > +
> > +	rc = 0;
> > +
> > +cleanup:
> > +	kfree(str_buf);
> > +	return rc;
> > +}
> > +
> >   static int uclogic_probe(struct hid_device *hdev,
> >   		const struct hid_device_id *id)
> >   {
> > @@ -907,6 +993,9 @@ static int uclogic_probe(struct hid_device *hdev,
> >   				return rc;
> >   			}
> >   			drvdata->invert_pen_inrange = true;
> > +
> > +			rc = uclogic_button_enable(hdev);
> > +			drvdata->has_virtual_pad_interface = !rc;
> >   		} else {
> >   			drvdata->ignore_pen_interface = true;
> >   		}
> 
> I think the button enabling should also work on TWHA60 v3 handled in the
> "case" just below.  We can move it above and let it fall down into this when
> bNumInterfaces == 3. I.e.:
> 
> 	case USB_DEVICE_ID_UCLOGIC_TABLET_TWHA60:
> 		/*
> 		 * If it is not the pen interface of the three-interface
> 		 * version, which is known to respond to initialization.
> 		 */
> 		if (udev->config->desc.bNumInterfaces != 3)
> 		    break;
> 
> 	case USB_DEVICE_ID_HUION_TABLET:
> 	case USB_DEVICE_ID_YIYNOVA_TABLET:

True. But upstream doesn't have support of the TWHA60 v3. So I'd prefer keep the patch in that way, and then add an extra commit for TWHA60 v3. Then the final support of TWHA60 v3 can be merged upstream too.

> 
> Another nice thing to do here is to somehow disable the keyboard interface
> (#2), but I'm not sure how to do that best.

Not sure either. With the proper names, we have a lot of extra input nodes, but the user can easily ignore them seeing that one is "Keyboard".
If Huion did not reused the same VID/PID for every tablets, things would have been easier :(

> 
> 
> > @@ -947,12 +1036,16 @@ static int uclogic_raw_event(struct hid_device
> > *hdev, struct hid_report *report,
> >   {
> >   	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
> >
> > -	if ((drvdata->invert_pen_inrange) &&
> > -	    (report->type == HID_INPUT_REPORT) &&
> > +	if ((report->type == HID_INPUT_REPORT) &&
> >   	    (report->id == UCLOGIC_PEN_REPORT_ID) &&
> > -	    (size >= 2))
> > -		/* Invert the in-range bit */
> > -		data[1] ^= 0x40;
> > +	    (size >= 2)) {
> > +		if (drvdata->has_virtual_pad_interface && (data[1] & 0x20))
> > +			/* Change to virtual frame button report ID */
> > +			data[0] = 0xf7;
> > +		else if (drvdata->invert_pen_inrange)
> > +			/* Invert the in-range bit */
> > +			data[1] ^= 0x40;
> > +	}
> >
> >   	return 0;
> >   }
> 
> This is neat.
> 
> Thanks, Benjamin!
> 

You are welcome.

Cheers,
Benjamin