[PATCH 0/2] HID: huion: add libinput support

[PATCH 0/2] HID: huion: add libinput support

[Benjamin Tissoires]

Hi Nikolai,

I know you are actually merging hid-huion and hid-uclogic, so we might not want
to take this in this current state. We may need to postpone it when you have
send the merge.

This series makes the Huion tablet (a H610 Pro) behave like a Wacom one from the
libinput point of view.
It will introduce a change in the default behavior for the users (which I
believe is a good change) where the pad part of the tablet will not send
random keyboard shortcuts but actual button events.

I'd be glad if you could validate the changes with the other huions you have
(or the Digimend project), because, having only one PID for all their tablets
is rather weird and difficult to support.

Last, I think we could add these tablets in the libwacom project, so that there
will be a nice GUI to configure the buttons. However, not having the PID to
discriminate between tablets is going to be a problem. Do you have any reliable
way of knowing the model from the kernel or the user space?

Cheers,
Benjamin

Benjamin Tissoires (2):
  HID: huion: enable button mode reporting
  HID: huion: split the stylus and pad in 2 nodes

 drivers/hid/hid-huion.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

-- 
2.1.0

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

[Benjamin Tissoires]

The Huion tablets currently report keyboard shortcuts when the user
hits the various buttons available on the pad.
These shortcuts are rather cumbersome because they contain "Alt-F4",
"Ctrl-C" and other widely used shortcuts.

The Windows driver switches the tablet into a no shortcuts mode, but this
mode violates the HID report descriptor. The buttons are sent through
the report ID 7, and they are not described in the report descriptor
(neither the one given by the hardware, nor the overwritten one).

To solve this, we create a new collection in the report descriptor with
a new report ID. When we see a button event in .raw_event(), we simply
change the report ID to this new collection and we can then rely on
hid-core to properly parse the buttons.

Developed and tested on a H610 Pro.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-huion.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

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};
+
 /* 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)                  */
+	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                      */
 	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);
+	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)))
+			data[0] = 0x08;
+	}
+
 	return 0;
 }
 
-- 
2.1.0

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

[Nikolai Kondrashov]

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.

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

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.

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

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

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

> +			data[0] = 0x08;
> +	}
> +
>   	return 0;
>   }
>
>

Nick

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

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

[Nikolai Kondrashov]

On 02/18/2015 10:24 PM, Benjamin Tissoires wrote:
> 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 :)

Ah, I missed that.

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

That's good, less weirdness to handle :)

>>>   /* 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 :)

Cool :)!

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

Yes, I think you're right.

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

Ah, nice :) Here is mine: https://github.com/DIGImend/hidrd

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

In that case, if we want to mimic the Windows driver we can request 32 bytes
always and do a compile-time check that our parameters fit into that.

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

No, we can't do that to the parameters string, because usb_string() does
utf16s_to_utf8s on the received data.

>>> +		/* 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.

Nice :)

Nick
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nikolai Kondrashov]

On 02/18/2015 12:54 AM, Benjamin Tissoires wrote:
> @@ -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)                   */

Oh, and it's probably safer to have a higher-number report ID in case some
tablets use it for something else.

> +	0x05, 0x0d,             /*          Usage Page (Digitizers)         */
> +	0x09, 0x22,             /*          Usage (Finger)                  */
> +	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                      */
>   	0xC0                    /*  End Collection                          */
>   };

Nick

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:
> >@@ -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)                   */
> 
> Oh, and it's probably safer to have a higher-number report ID in case some
> tablets use it for something else.
>

makes sense. Will amend.

Cheers,
Benjamin
 
> >+	0x05, 0x0d,             /*          Usage Page (Digitizers)         */
> >+	0x09, 0x22,             /*          Usage (Finger)                  */
> >+	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                      */
> >  	0xC0                    /*  End Collection                          */
> >  };
> 
> Nick

[PATCH 2/2] HID: huion: split the stylus and pad in 2 nodes

[Benjamin Tissoires]

Libinput expects the pad node to be different from the stylus one.
We can just use HID_QUIRK_MULTI_INPUT to separate the buttons of the
pad than the actual stylus.
We can also remove the unused interfaces.
Last, we append "Pen" or "Pad" suffix to the appropriate input node
to match what the Wacom driver does and be more convenient for the users
to know which one is which.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-huion.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
index 50fbda4..f298f66 100644
--- a/drivers/hid/hid-huion.c
+++ b/drivers/hid/hid-huion.c
@@ -142,6 +142,33 @@ static __u8 *huion_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 	return rdesc;
 }
 
+static void huion_input_configured(struct hid_device *hdev,
+		struct hid_input *hi)
+{
+	char *name;
+	const char *suffix = NULL;
+	struct hid_field *field = hi->report->field[0];
+
+	switch (field->application) {
+	case HID_GD_KEYPAD:
+		suffix = "Pad";
+		break;
+	case HID_DG_PEN:
+		suffix = "Pen";
+		break;
+	}
+
+	if (suffix) {
+		name = devm_kzalloc(&hi->input->dev,
+				    strlen(hdev->name) + strlen(suffix) + 2,
+				    GFP_KERNEL);
+		if (name) {
+			sprintf(name, "%s %s", hdev->name, suffix);
+			hi->input->name = name;
+		}
+	}
+}
+
 /**
  * Enable fully-functional tablet mode and determine device parameters.
  *
@@ -282,6 +309,9 @@ static int huion_probe(struct hid_device *hdev, const struct hid_device_id *id)
 				hid_err(hdev, "tablet enabling failed\n");
 				return rc;
 			}
+			hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+		} else {
+			return -ENODEV;
 		}
 		break;
 	}
@@ -335,6 +365,7 @@ static struct hid_driver huion_driver = {
 	.probe = huion_probe,
 	.report_fixup = huion_report_fixup,
 	.raw_event = huion_raw_event,
+	.input_configured = huion_input_configured,
 };
 module_hid_driver(huion_driver);
 
-- 
2.1.0

Re: [PATCH 2/2] HID: huion: split the stylus and pad in 2 nodes

[Nikolai Kondrashov]

On 02/18/2015 12:54 AM, Benjamin Tissoires wrote:
> Libinput expects the pad node to be different from the stylus one.
> We can just use HID_QUIRK_MULTI_INPUT to separate the buttons of the
> pad than the actual stylus.
> We can also remove the unused interfaces.
> Last, we append "Pen" or "Pad" suffix to the appropriate input node
> to match what the Wacom driver does and be more convenient for the users
> to know which one is which.

This is very nice and a thing that users asked for and I wanted to add for a
long while. I overlooked the addition of the input_configured callback
support.

>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>   drivers/hid/hid-huion.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>
> diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
> index 50fbda4..f298f66 100644
> --- a/drivers/hid/hid-huion.c
> +++ b/drivers/hid/hid-huion.c
> @@ -142,6 +142,33 @@ static __u8 *huion_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>   	return rdesc;
>   }
>
> +static void huion_input_configured(struct hid_device *hdev,
> +		struct hid_input *hi)
> +{
> +	char *name;
> +	const char *suffix = NULL;
> +	struct hid_field *field = hi->report->field[0];
> +
> +	switch (field->application) {
> +	case HID_GD_KEYPAD:
> +		suffix = "Pad";
> +		break;
> +	case HID_DG_PEN:
> +		suffix = "Pen";
> +		break;
> +	}
> +
> +	if (suffix) {
> +		name = devm_kzalloc(&hi->input->dev,
> +				    strlen(hdev->name) + strlen(suffix) + 2,
> +				    GFP_KERNEL);
> +		if (name) {
> +			sprintf(name, "%s %s", hdev->name, suffix);

I would have preferred an snprintf here instead, with the buffer length
calculated once and specified explicitly.

> +			hi->input->name = name;
> +		}
> +	}
> +}
> +
>   /**
>    * Enable fully-functional tablet mode and determine device parameters.
>    *
> @@ -282,6 +309,9 @@ static int huion_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   				hid_err(hdev, "tablet enabling failed\n");
>   				return rc;
>   			}
> +			hdev->quirks |= HID_QUIRK_MULTI_INPUT;
> +		} else {
> +			return -ENODEV;
>   		}
>   		break;
>   	}
> @@ -335,6 +365,7 @@ static struct hid_driver huion_driver = {
>   	.probe = huion_probe,
>   	.report_fixup = huion_report_fixup,
>   	.raw_event = huion_raw_event,
> +	.input_configured = huion_input_configured,
>   };
>   module_hid_driver(huion_driver);
>
>

Thanks, Benjamin!

Nick

Re: [PATCH 0/2] HID: huion: add libinput support

[Nikolai Kondrashov]

Hi Benjamin,

I'm copying my reply to DIGImend-devel as well.

On 02/18/2015 12:54 AM, Benjamin Tissoires wrote:
> Hi Nikolai,
>
> I know you are actually merging hid-huion and hid-uclogic, so we might not want
> to take this in this current state. We may need to postpone it when you have
> send the merge.

I have it in the out-of-tree driver package [1], which I'll need to release
for users to test first.

> This series makes the Huion tablet (a H610 Pro) behave like a Wacom one from the
> libinput point of view.
> It will introduce a change in the default behavior for the users (which I
> believe is a good change) where the pad part of the tablet will not send
> random keyboard shortcuts but actual button events.

That's awesome, thanks a lot, Benjamin! I tried making something like that
[2], which seems to work reasonably. However, I was not up-to-date with
libinput changes and left the buttons in the same device with the pen. Then I
heard about input groups from Hans and planned to re-make it properly, but now
you did it. Thanks!

> I'd be glad if you could validate the changes with the other huions you have
> (or the Digimend project), because, having only one PID for all their tablets
> is rather weird and difficult to support.

I'll have to incorporate them into the out-of-tree driver [1] to simplify
testing for users. I'll leave some comments to the changes as well, if you
don't mind.

Of course, it would help a lot, if you could contribute your patches to the
out-of-tree driver, from where we could then take them to upstream, after
testing. I can do it myself, though.

> Last, I think we could add these tablets in the libwacom project, so that there
> will be a nice GUI to configure the buttons.

That would be a very welcome change, without doubt, thank you.

However, I can't help wondering, would it be more productive to allow libwacom
to work with just any basic tablet, without the need to add each one to the
database?

> However, not having the PID to discriminate between tablets is going to be a
> problem. Do you have any reliable way of knowing the model from the kernel
> or the user space?

Unfortunately, not. You can take a look at the data I and the users gathered
on some of the Huion tablets [3]. The iProduct string can be used to some
extent, but I wouldn't rely on it to make much sense. Apart from Huion
tablets, there are also Yiynova tablets which work the same way, at least some
Ugee tablets and at least one UC-Logic tablet, but I expect more of the latter
work as well. Among them there is a mix of using dedicated and UC-Logic VIDs
and mostly reusing PIDs. There is something which seems to be an internal
model name in string descriptor 0x7a, but it also doesn't make much sense.

All-in-all it's a mess. I've even seen a tablet with a typo in iManufacturer.

Still, if we make libwacom work with generic tablets not contained in its
database, that wouldn't be a problem.

Nick

[1] https://github.com/DIGImend/digimend-kernel-drivers
[2] https://github.com/DIGImend/digimend-kernel-drivers/blob/huion-abstract-keyboard/hid-huion.c
[3] https://github.com/DIGImend/tablets

Re: [PATCH 0/2] HID: huion: add libinput support

[Benjamin Tissoires]

On Feb 18 2015 or thereabouts, Nikolai Kondrashov wrote:
> Hi Benjamin,
> 
> I'm copying my reply to DIGImend-devel as well.
> 
> On 02/18/2015 12:54 AM, Benjamin Tissoires wrote:
> >Hi Nikolai,
> >
> >I know you are actually merging hid-huion and hid-uclogic, so we might not want
> >to take this in this current state. We may need to postpone it when you have
> >send the merge.
> 
> I have it in the out-of-tree driver package [1], which I'll need to release
> for users to test first.
> 
> >This series makes the Huion tablet (a H610 Pro) behave like a Wacom one from the
> >libinput point of view.
> >It will introduce a change in the default behavior for the users (which I
> >believe is a good change) where the pad part of the tablet will not send
> >random keyboard shortcuts but actual button events.
> 
> That's awesome, thanks a lot, Benjamin! I tried making something like that
> [2], which seems to work reasonably. However, I was not up-to-date with
> libinput changes and left the buttons in the same device with the pen. Then I
> heard about input groups from Hans and planned to re-make it properly, but now
> you did it. Thanks!

Hmm... I should have checked more carefully your branches. I actually
redid the reverse engineering and ended up with nearly the same patch
you already have :)

Sorry for not including you in the loop earlier. We were actually really
focused on Wacom for the past few months and decided to look into the
Huion and other tablets only last week. I believe not having the pad
buttons separated in the huion tablets is not that much of a problem
(for now, the buttons are quite separate from the stylus buttons), but
in the Wacom world, we had some terrible overlapping and making those as
2 separate devices was the obvious change.

> 
> >I'd be glad if you could validate the changes with the other huions you have
> >(or the Digimend project), because, having only one PID for all their tablets
> >is rather weird and difficult to support.
> 
> I'll have to incorporate them into the out-of-tree driver [1] to simplify
> testing for users. I'll leave some comments to the changes as well, if you
> don't mind.

Sure, no problem.

> 
> Of course, it would help a lot, if you could contribute your patches to the
> out-of-tree driver, from where we could then take them to upstream, after
> testing. I can do it myself, though.

OK, will respin the patches for your out of the tree driver.

> 
> >Last, I think we could add these tablets in the libwacom project, so that there
> >will be a nice GUI to configure the buttons.
> 
> That would be a very welcome change, without doubt, thank you.
> 
> However, I can't help wondering, would it be more productive to allow libwacom
> to work with just any basic tablet, without the need to add each one to the
> database?

Actually, libwacom is not tight to Wacom devices at all (or should not
be). It is just a database of devices to add what the kernel doesn't or
can not provide. The things that are in the db are for example how the
buttons are physically mapped on the device, what is the actual layout
(one svg file), if there are LEDs attached to the tablet.

All this needs a fine per-device tuning. We can add a generic
Huion/UClogic tablet too, but having a specific per-device definition
allows to show the exact mapping of the buttons on the overlay when
setting the functions.

> 
> >However, not having the PID to discriminate between tablets is going to be a
> >problem. Do you have any reliable way of knowing the model from the kernel
> >or the user space?
> 
> Unfortunately, not. You can take a look at the data I and the users gathered
> on some of the Huion tablets [3]. The iProduct string can be used to some
> extent, but I wouldn't rely on it to make much sense. Apart from Huion
> tablets, there are also Yiynova tablets which work the same way, at least some
> Ugee tablets and at least one UC-Logic tablet, but I expect more of the latter
> work as well. Among them there is a mix of using dedicated and UC-Logic VIDs
> and mostly reusing PIDs. There is something which seems to be an internal
> model name in string descriptor 0x7a, but it also doesn't make much sense.
> 
> All-in-all it's a mess. I've even seen a tablet with a typo in iManufacturer.
> 
> Still, if we make libwacom work with generic tablets not contained in its
> database, that wouldn't be a problem.

I'll see what I can do. However, it looks like the iProduct and
iManufacturer could help to some extend (until we find a big problem). I
thought they were all labeled "HUION PenTablet" but actually only the
H610 Pro and the Huion W58 share this name. The W58 does not have
buttons so we night be able to remove the pad interface for them and
then libwacom will be happy with only the name of the input node.

Cheers,
Benjamin

> 
> Nick
> 
> [1] https://github.com/DIGImend/digimend-kernel-drivers
> [2] https://github.com/DIGImend/digimend-kernel-drivers/blob/huion-abstract-keyboard/hid-huion.c
> [3] https://github.com/DIGImend/tablets

Re: [PATCH 0/2] HID: huion: add libinput support

[Nikolai Kondrashov]

On 02/18/2015 10:04 PM, Benjamin Tissoires wrote:
> On Feb 18 2015 or thereabouts, Nikolai Kondrashov wrote:
>> Hi Benjamin,
>>
>> I'm copying my reply to DIGImend-devel as well.
>>
>> On 02/18/2015 12:54 AM, Benjamin Tissoires wrote:
>>> Hi Nikolai,
>>>
>>> I know you are actually merging hid-huion and hid-uclogic, so we might not want
>>> to take this in this current state. We may need to postpone it when you have
>>> send the merge.
>>
>> I have it in the out-of-tree driver package [1], which I'll need to release
>> for users to test first.
>>
>>> This series makes the Huion tablet (a H610 Pro) behave like a Wacom one from the
>>> libinput point of view.
>>> It will introduce a change in the default behavior for the users (which I
>>> believe is a good change) where the pad part of the tablet will not send
>>> random keyboard shortcuts but actual button events.
>>
>> That's awesome, thanks a lot, Benjamin! I tried making something like that
>> [2], which seems to work reasonably. However, I was not up-to-date with
>> libinput changes and left the buttons in the same device with the pen. Then I
>> heard about input groups from Hans and planned to re-make it properly, but now
>> you did it. Thanks!
>
> Hmm... I should have checked more carefully your branches. I actually
> redid the reverse engineering and ended up with nearly the same patch
> you already have :)
>
> Sorry for not including you in the loop earlier. We were actually really
> focused on Wacom for the past few months and decided to look into the
> Huion and other tablets only last week.

No problem, I wasn't keeping up with your development myself as I should have.

> I believe not having the pad buttons separated in the huion tablets is not
> that much of a problem (for now, the buttons are quite separate from the
> stylus buttons), but in the Wacom world, we had some terrible overlapping
> and making those as 2 separate devices was the obvious change.

I think this is a good change, which will hopefully make things simpler.

>>> I'd be glad if you could validate the changes with the other huions you have
>>> (or the Digimend project), because, having only one PID for all their tablets
>>> is rather weird and difficult to support.
>>
>> I'll have to incorporate them into the out-of-tree driver [1] to simplify
>> testing for users. I'll leave some comments to the changes as well, if you
>> don't mind.
>
> Sure, no problem.
>
>>
>> Of course, it would help a lot, if you could contribute your patches to the
>> out-of-tree driver, from where we could then take them to upstream, after
>> testing. I can do it myself, though.
>
> OK, will respin the patches for your out of the tree driver.

Thanks a lot, Benjamin!

>>> Last, I think we could add these tablets in the libwacom project, so that there
>>> will be a nice GUI to configure the buttons.
>>
>> That would be a very welcome change, without doubt, thank you.
>>
>> However, I can't help wondering, would it be more productive to allow libwacom
>> to work with just any basic tablet, without the need to add each one to the
>> database?
>
> Actually, libwacom is not tight to Wacom devices at all (or should not
> be). It is just a database of devices to add what the kernel doesn't or
> can not provide. The things that are in the db are for example how the
> buttons are physically mapped on the device, what is the actual layout
> (one svg file), if there are LEDs attached to the tablet.
>
> All this needs a fine per-device tuning. We can add a generic
> Huion/UClogic tablet too, but having a specific per-device definition
> allows to show the exact mapping of the buttons on the overlay when
> setting the functions.

I agree, that's a nice feature. However, I think being able to configure all
the applicable Wacom driver features relatively comfortably, even if the
tablet on screen doesn't exactly match your tablet, is still a win, compared
to not being able to do it.

For the unknown tablets we can just draw abstract tablets, emphasising that
control locations on the screen don't map to the actual locations. For
example, have the tablet drawn like an exploded diagram: surface, buttons,
dials - all separate.  Something like this:

     Buttons: * * * * * * *
       Dials: O O
   Work area: +------------+
              |            |
              |            |
              |            |
              +------------+

I think the users will be able to figure out the mapping by experimentation.

While it's just about possible to keep an up-to-date database of Wacom
tablets, I don't think we'll ever be able to list all those generic tablets
out there. Meanwhile a lot of people are left in the cold of xsetwacom and
xinput.

>>> However, not having the PID to discriminate between tablets is going to be a
>>> problem. Do you have any reliable way of knowing the model from the kernel
>>> or the user space?
>>
>> Unfortunately, not. You can take a look at the data I and the users gathered
>> on some of the Huion tablets [3]. The iProduct string can be used to some
>> extent, but I wouldn't rely on it to make much sense. Apart from Huion
>> tablets, there are also Yiynova tablets which work the same way, at least some
>> Ugee tablets and at least one UC-Logic tablet, but I expect more of the latter
>> work as well. Among them there is a mix of using dedicated and UC-Logic VIDs
>> and mostly reusing PIDs. There is something which seems to be an internal
>> model name in string descriptor 0x7a, but it also doesn't make much sense.
>>
>> All-in-all it's a mess. I've even seen a tablet with a typo in iManufacturer.
>>
>> Still, if we make libwacom work with generic tablets not contained in its
>> database, that wouldn't be a problem.
>
> I'll see what I can do. However, it looks like the iProduct and
> iManufacturer could help to some extend (until we find a big problem). I
> thought they were all labeled "HUION PenTablet" but actually only the
> H610 Pro and the Huion W58 share this name. The W58 does not have
> buttons so we night be able to remove the pad interface for them and
> then libwacom will be happy with only the name of the input node.

Yes it will work for some of them. I have no idea what numerous other models
have in their descriptors, though.

Nick

Re: [PATCH 0/2] HID: huion: add libinput support

[Peter Hutterer]

On Thu, Feb 19, 2015 at 01:54:17PM +0200, Nikolai Kondrashov wrote:
[...]
> >>>Last, I think we could add these tablets in the libwacom project, so that there
> >>>will be a nice GUI to configure the buttons.
> >>
> >>That would be a very welcome change, without doubt, thank you.
> >>
> >>However, I can't help wondering, would it be more productive to allow libwacom
> >>to work with just any basic tablet, without the need to add each one to the
> >>database?
> >
> >Actually, libwacom is not tight to Wacom devices at all (or should not
> >be). It is just a database of devices to add what the kernel doesn't or
> >can not provide. The things that are in the db are for example how the
> >buttons are physically mapped on the device, what is the actual layout
> >(one svg file), if there are LEDs attached to the tablet.
> >
> >All this needs a fine per-device tuning. We can add a generic
> >Huion/UClogic tablet too, but having a specific per-device definition
> >allows to show the exact mapping of the buttons on the overlay when
> >setting the functions.
> 
> I agree, that's a nice feature. However, I think being able to configure all
> the applicable Wacom driver features relatively comfortably, even if the
> tablet on screen doesn't exactly match your tablet, is still a win, compared
> to not being able to do it.
> 
> For the unknown tablets we can just draw abstract tablets, emphasising that
> control locations on the screen don't map to the actual locations. For
> example, have the tablet drawn like an exploded diagram: surface, buttons,
> dials - all separate.  Something like this:
> 
>     Buttons: * * * * * * *
>       Dials: O O
>   Work area: +------------+
>              |            |
>              |            |
>              |            |
>              +------------+
> 
> I think the users will be able to figure out the mapping by experimentation.
> 
> While it's just about possible to keep an up-to-date database of Wacom
> tablets, I don't think we'll ever be able to list all those generic tablets
> out there. Meanwhile a lot of people are left in the cold of xsetwacom and
> xinput.

not a reason to give up, IMO. most of these generic tablets are relatively
simple, so adding a libwacom entry should be a matter of minutes.
we'll never get full support of everything, but perfect is the enemy of good
here.

Cheers,
   Peter

Re: [PATCH 0/2] HID: huion: add libinput support

[Nikolai Kondrashov]

On 02/20/2015 07:34 AM, Peter Hutterer wrote:
> On Thu, Feb 19, 2015 at 01:54:17PM +0200, Nikolai Kondrashov wrote:
> [...]
>>>>> Last, I think we could add these tablets in the libwacom project, so that there
>>>>> will be a nice GUI to configure the buttons.
>>>>
>>>> That would be a very welcome change, without doubt, thank you.
>>>>
>>>> However, I can't help wondering, would it be more productive to allow libwacom
>>>> to work with just any basic tablet, without the need to add each one to the
>>>> database?
>>>
>>> Actually, libwacom is not tight to Wacom devices at all (or should not
>>> be). It is just a database of devices to add what the kernel doesn't or
>>> can not provide. The things that are in the db are for example how the
>>> buttons are physically mapped on the device, what is the actual layout
>>> (one svg file), if there are LEDs attached to the tablet.
>>>
>>> All this needs a fine per-device tuning. We can add a generic
>>> Huion/UClogic tablet too, but having a specific per-device definition
>>> allows to show the exact mapping of the buttons on the overlay when
>>> setting the functions.
>>
>> I agree, that's a nice feature. However, I think being able to configure all
>> the applicable Wacom driver features relatively comfortably, even if the
>> tablet on screen doesn't exactly match your tablet, is still a win, compared
>> to not being able to do it.
>>
>> For the unknown tablets we can just draw abstract tablets, emphasising that
>> control locations on the screen don't map to the actual locations. For
>> example, have the tablet drawn like an exploded diagram: surface, buttons,
>> dials - all separate.  Something like this:
>>
>>      Buttons: * * * * * * *
>>        Dials: O O
>>    Work area: +------------+
>>               |            |
>>               |            |
>>               |            |
>>               +------------+
>>
>> I think the users will be able to figure out the mapping by experimentation.
>>
>> While it's just about possible to keep an up-to-date database of Wacom
>> tablets, I don't think we'll ever be able to list all those generic tablets
>> out there. Meanwhile a lot of people are left in the cold of xsetwacom and
>> xinput.
>
> not a reason to give up, IMO. most of these generic tablets are relatively
> simple, so adding a libwacom entry should be a matter of minutes.
> we'll never get full support of everything, but perfect is the enemy of good
> here.

Hmm, I see having all the tablets listed in the database as perfect and having
generic tablet handling as more practical, i.e. the other way around.

It might be easy for us to add a tablet entry, but not for the general user.
They will need to figure out they need to add that entry first and they'll
need to figure out what to put there and how. This will need to go through us
in the end and we'll need to extract all the information from the user, which
will likely require several e-mails for *each* tablet.

Meanwhile most users just want to draw.

I might be misunderstanding something, though.

Regardless, Benjamin work is making it all better, I cannot complain :)

Nick

Re: [PATCH 0/2] HID: huion: add libinput support

[Peter Hutterer]

On Sun, Feb 22, 2015 at 02:33:53PM +0200, Nikolai Kondrashov wrote:
> On 02/20/2015 07:34 AM, Peter Hutterer wrote:
> >On Thu, Feb 19, 2015 at 01:54:17PM +0200, Nikolai Kondrashov wrote:
> >[...]
> >>>>>Last, I think we could add these tablets in the libwacom project, so that there
> >>>>>will be a nice GUI to configure the buttons.
> >>>>
> >>>>That would be a very welcome change, without doubt, thank you.
> >>>>
> >>>>However, I can't help wondering, would it be more productive to allow libwacom
> >>>>to work with just any basic tablet, without the need to add each one to the
> >>>>database?
> >>>
> >>>Actually, libwacom is not tight to Wacom devices at all (or should not
> >>>be). It is just a database of devices to add what the kernel doesn't or
> >>>can not provide. The things that are in the db are for example how the
> >>>buttons are physically mapped on the device, what is the actual layout
> >>>(one svg file), if there are LEDs attached to the tablet.
> >>>
> >>>All this needs a fine per-device tuning. We can add a generic
> >>>Huion/UClogic tablet too, but having a specific per-device definition
> >>>allows to show the exact mapping of the buttons on the overlay when
> >>>setting the functions.
> >>
> >>I agree, that's a nice feature. However, I think being able to configure all
> >>the applicable Wacom driver features relatively comfortably, even if the
> >>tablet on screen doesn't exactly match your tablet, is still a win, compared
> >>to not being able to do it.
> >>
> >>For the unknown tablets we can just draw abstract tablets, emphasising that
> >>control locations on the screen don't map to the actual locations. For
> >>example, have the tablet drawn like an exploded diagram: surface, buttons,
> >>dials - all separate.  Something like this:
> >>
> >>     Buttons: * * * * * * *
> >>       Dials: O O
> >>   Work area: +------------+
> >>              |            |
> >>              |            |
> >>              |            |
> >>              +------------+
> >>
> >>I think the users will be able to figure out the mapping by experimentation.
> >>
> >>While it's just about possible to keep an up-to-date database of Wacom
> >>tablets, I don't think we'll ever be able to list all those generic tablets
> >>out there. Meanwhile a lot of people are left in the cold of xsetwacom and
> >>xinput.
> >
> >not a reason to give up, IMO. most of these generic tablets are relatively
> >simple, so adding a libwacom entry should be a matter of minutes.
> >we'll never get full support of everything, but perfect is the enemy of good
> >here.
> 
> Hmm, I see having all the tablets listed in the database as perfect and having
> generic tablet handling as more practical, i.e. the other way around.
> 
> It might be easy for us to add a tablet entry, but not for the general user.
> They will need to figure out they need to add that entry first and they'll
> need to figure out what to put there and how. This will need to go through us
> in the end and we'll need to extract all the information from the user, which
> will likely require several e-mails for *each* tablet.

yeah, but the thing is: those emails are only necessary _once_  per tablet.
if they're not in the database, you'll get those questions for the lifetime
of the tablet :)

Also note that libwacom_new_from_path() has a fallback option, you can get a
generic tablet from it and then proceed as normal. gnome-settings-daemon
already uses this.

Cheers,
   Peter

> 
> Meanwhile most users just want to draw.
> 
> I might be misunderstanding something, though.
> 
> Regardless, Benjamin work is making it all better, I cannot complain :)
> 
> Nick

Re: [PATCH 0/2] HID: huion: add libinput support

[Benjamin Tissoires]

On Feb 23 2015 or thereabouts, Peter Hutterer wrote:
> On Sun, Feb 22, 2015 at 02:33:53PM +0200, Nikolai Kondrashov wrote:
> > On 02/20/2015 07:34 AM, Peter Hutterer wrote:
> > >On Thu, Feb 19, 2015 at 01:54:17PM +0200, Nikolai Kondrashov wrote:
> > >[...]
> > >>>>>Last, I think we could add these tablets in the libwacom project, so that there
> > >>>>>will be a nice GUI to configure the buttons.
> > >>>>
> > >>>>That would be a very welcome change, without doubt, thank you.
> > >>>>
> > >>>>However, I can't help wondering, would it be more productive to allow libwacom
> > >>>>to work with just any basic tablet, without the need to add each one to the
> > >>>>database?
> > >>>
> > >>>Actually, libwacom is not tight to Wacom devices at all (or should not
> > >>>be). It is just a database of devices to add what the kernel doesn't or
> > >>>can not provide. The things that are in the db are for example how the
> > >>>buttons are physically mapped on the device, what is the actual layout
> > >>>(one svg file), if there are LEDs attached to the tablet.
> > >>>
> > >>>All this needs a fine per-device tuning. We can add a generic
> > >>>Huion/UClogic tablet too, but having a specific per-device definition
> > >>>allows to show the exact mapping of the buttons on the overlay when
> > >>>setting the functions.
> > >>
> > >>I agree, that's a nice feature. However, I think being able to configure all
> > >>the applicable Wacom driver features relatively comfortably, even if the
> > >>tablet on screen doesn't exactly match your tablet, is still a win, compared
> > >>to not being able to do it.
> > >>
> > >>For the unknown tablets we can just draw abstract tablets, emphasising that
> > >>control locations on the screen don't map to the actual locations. For
> > >>example, have the tablet drawn like an exploded diagram: surface, buttons,
> > >>dials - all separate.  Something like this:
> > >>
> > >>     Buttons: * * * * * * *
> > >>       Dials: O O
> > >>   Work area: +------------+
> > >>              |            |
> > >>              |            |
> > >>              |            |
> > >>              +------------+
> > >>
> > >>I think the users will be able to figure out the mapping by experimentation.
> > >>
> > >>While it's just about possible to keep an up-to-date database of Wacom
> > >>tablets, I don't think we'll ever be able to list all those generic tablets
> > >>out there. Meanwhile a lot of people are left in the cold of xsetwacom and
> > >>xinput.
> > >
> > >not a reason to give up, IMO. most of these generic tablets are relatively
> > >simple, so adding a libwacom entry should be a matter of minutes.
> > >we'll never get full support of everything, but perfect is the enemy of good
> > >here.
> > 
> > Hmm, I see having all the tablets listed in the database as perfect and having
> > generic tablet handling as more practical, i.e. the other way around.
> > 
> > It might be easy for us to add a tablet entry, but not for the general user.
> > They will need to figure out they need to add that entry first and they'll
> > need to figure out what to put there and how. This will need to go through us
> > in the end and we'll need to extract all the information from the user, which
> > will likely require several e-mails for *each* tablet.
> 
> yeah, but the thing is: those emails are only necessary _once_  per tablet.
> if they're not in the database, you'll get those questions for the lifetime
> of the tablet :)
> 
> Also note that libwacom_new_from_path() has a fallback option, you can get a
> generic tablet from it and then proceed as normal. gnome-settings-daemon
> already uses this.
> 

I also expects that the settings configuration tool under wayland will
rely on the actual capability of the devices. This way, even if the
tablet is not registered in libwacom, we will still be able to show a
default configuration UI.

I really believe the registering in libwacom will be just to have a
prettier interface, not a requirement.

Cheers,
Benjamin

> > 
> > Meanwhile most users just want to draw.
> > 
> > I might be misunderstanding something, though.
> > 
> > Regardless, Benjamin work is making it all better, I cannot complain :)
> > 
> > Nick

Re: [PATCH 0/2] HID: huion: add libinput support

[Nikolai Kondrashov]

On 02/24/2015 12:34 AM, Benjamin Tissoires wrote:
> On Feb 23 2015 or thereabouts, Peter Hutterer wrote:
>> yeah, but the thing is: those emails are only necessary _once_  per tablet.
>> if they're not in the database, you'll get those questions for the lifetime
>> of the tablet :)
>>
>> Also note that libwacom_new_from_path() has a fallback option, you can get a
>> generic tablet from it and then proceed as normal. gnome-settings-daemon
>> already uses this.
>>
>
> I also expects that the settings configuration tool under wayland will
> rely on the actual capability of the devices. This way, even if the
> tablet is not registered in libwacom, we will still be able to show a
> default configuration UI.
>
> I really believe the registering in libwacom will be just to have a
> prettier interface, not a requirement.

Well, that sounds awesome! Just what I was looking for :)

What about X.org though? Last time I tried I couldn't configure a
Wacom-handled Huion with the Gnome applet.

Nick

Re: [PATCH 0/2] HID: huion: add libinput support

[Peter Hutterer]

On Tue, Feb 24, 2015 at 01:22:37PM +0200, Nikolai Kondrashov wrote:
> On 02/24/2015 12:34 AM, Benjamin Tissoires wrote:
> >On Feb 23 2015 or thereabouts, Peter Hutterer wrote:
> >>yeah, but the thing is: those emails are only necessary _once_  per tablet.
> >>if they're not in the database, you'll get those questions for the lifetime
> >>of the tablet :)
> >>
> >>Also note that libwacom_new_from_path() has a fallback option, you can get a
> >>generic tablet from it and then proceed as normal. gnome-settings-daemon
> >>already uses this.
> >>
> >
> >I also expects that the settings configuration tool under wayland will
> >rely on the actual capability of the devices. This way, even if the
> >tablet is not registered in libwacom, we will still be able to show a
> >default configuration UI.
> >
> >I really believe the registering in libwacom will be just to have a
> >prettier interface, not a requirement.
> 
> Well, that sounds awesome! Just what I was looking for :)
> 
> What about X.org though? Last time I tried I couldn't configure a
> Wacom-handled Huion with the Gnome applet.

hard to say, settings-daemon/control-center use the xorg wacom driver
properties, so in theory it should work. Best to file a gnome bug for it
and CC me on it, there's no obvious definitive answer I can think of right
now.

Cheers,
   Peter

Re: [PATCH 0/2] HID: huion: add libinput support

[Benjamin Tissoires]

On Feb 19 2015 or thereabouts, Nikolai Kondrashov wrote:
> On 02/18/2015 10:04 PM, Benjamin Tissoires wrote:
> >On Feb 18 2015 or thereabouts, Nikolai Kondrashov wrote:
[snipped]
> >
> >OK, will respin the patches for your out of the tree driver.
> 
> Thanks a lot, Benjamin!
> 

OK, so after a little bit of struggle, I think we need to work both
upstream and on the out of the tree driver. Most of the current patches
in the out of the tree driver can be included right now and there is no
need to make hundreds of tests for these.

If you don't mind, I'll post them tomorrow with the follow up of the
patches I know will not break the tablets. Then, we can sync on the
digimend tree and ask users to test the "abstract buttons events"
reporting mode. It will help me and will also speed up the process of
having everything included sooner.

How does that sound?

Cheers,
Benjamin

Re: [PATCH 0/2] HID: huion: add libinput support

[Nikolai Kondrashov]

On 02/24/2015 12:44 AM, Benjamin Tissoires wrote:
> On Feb 19 2015 or thereabouts, Nikolai Kondrashov wrote:
>> On 02/18/2015 10:04 PM, Benjamin Tissoires wrote:
>>> On Feb 18 2015 or thereabouts, Nikolai Kondrashov wrote:
> [snipped]
>>>
>>> OK, will respin the patches for your out of the tree driver.
>>
>> Thanks a lot, Benjamin!
>>
>
> OK, so after a little bit of struggle, I think we need to work both
> upstream and on the out of the tree driver. Most of the current patches
> in the out of the tree driver can be included right now and there is no
> need to make hundreds of tests for these.
>
> If you don't mind, I'll post them tomorrow with the follow up of the
> patches I know will not break the tablets. Then, we can sync on the
> digimend tree and ask users to test the "abstract buttons events"
> reporting mode. It will help me and will also speed up the process of
> having everything included sooner.
>
> How does that sound?

Sure, go ahead and I'll help as much as I can.

Thanks, Benjamin!

Nick