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

[Nikolai K. <sp...@gm...>]

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.

> 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.

> +	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:

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


> @@ -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!

Nick