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

[Nikolai Kondrashov <spbnick@gmail.com>]

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