Re: [DIGImend-devel] PT-1001 digitizer

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

Hi Yann,

On 01/28/2015 12:42 PM, Yann Vernier wrote:
> On Mon, 26 Jan 2015 10:33:16 +0200
> Nikolai Kondrashov <sp...@gm...> wrote:
>> Could you please keep the maillist in the CC? Thank you.
>
> Certainly. I made a mistake when hitting reply, for which I apologize.
> It was not my intent to exclude the list.

Sure, no problem :)

>> Hmm, from your description it seems to me that Linux notices it is missing and
>> actively drops the interface with a bad descriptor, doesn't it? Or did I
>> understand you wrong?
>
> It only drops it after failing to parse uninitialized data.
>
> drivers/hid/usbhid/hid-core.c usbhid_parse and hid_post_reset both call
> hid_get_class_descriptor, which does check the size, but gives up after
> four tries. Even so, having received some data it returns the incomplete
> descriptor, i.e. instead of error (negative) it returns rsize-1 (as the
> last byte is missing). It never attempted a larger read, so it did not
> work around the bug in the PT-1001.
>
> In usbhid_parse (and hid_post_reset), the return value is only checked
> for negative, not for wrong size, so the error is lost at this point.
> This subtle bug changes the meaning of the received descriptor from
> incomplete (truncated) to corrupted (padded, in this case with zero,
> but kmalloc does not guarantee that).
>
> Since the error was lost, Linux proceeds to hid_parse_report. As the last
> byte was never received, it has an erroneous value, and that's when the
> parsing fails. Linux attempted to parse a byte that did not come from the
> device. Had the descriptor been different up to that point, the corruption
> could have gone unnoticed, producing arbitrary results. Linux only detected
> a problem because a C0 was required.

Thank you for a detailed explanation. Yes, it seems the error handling needs
to be improved here.

>> I think it is intentional, although presents a Windows-compatibility problem.
>> Basically, IIRC, if Linux encounters a field with usage which should be mapped
>> to the same event as a field previously seen in the same report descriptor, it
>> assigns this field a higher event number. This might be limited to some event
>> types. The HID_QUIRK_MULTI_INPUT quirk is used to make the driver create
>> separate event devices for different report IDs to avoid this.
>
> Interesting. MULTI_INPUT might be appropriate to separate mouse and tablet
> mode in the PT-1001, as they produce different reports.

Yes, it will help there.

> The strange part, comparing to the report descriptors, is that the button
> meanings generally don't match up at all, even on the second interface where
> all buttons have usages and none collide. The anti-collission support one
> might expect would allocate different generic buttons rather than six copies
> of "unknown" sprinkled among a rather random selection.

Yes, it's quite hairy.

> I have attached an initial draft of a pt1001 descriptor patch driver, but
> it still has some issues:
>
> For some reason, hid-generic still takes priority when choosing driver.
> I have to unbind hid-generic to use this driver.

Yes, the generic driver takes over everything it doesn't know about and the
official solution for out-of-tree drivers is to rebind. For that reason
digimend-kernel-drivers installs the rebinding script and udev rules. See
hid-rebind.rules, and hid-rebind.

> The button usages still don't show through, and worse, the stylus tip and
> second barrel (reported as eraser) button both show as button 1. Using
> input-events seems to explain this as them being reported as touch,
> stylus and button 0; the expected set was stylus, barrel and eraser.
> With this button setup xinput mapping cannot separate them.

I'm not sure why, but Eraser usage (0x45) doesn't seem to be mapped explicitly
and goes to the "unknown" label in hidinput_configure_usage(), probably being
mapped to BTN_0 in map_key() from there.

> multi_input did help in that both tablet and mouse mode work, and the
> modified descriptor (with digitizer rather than pointer) makes tablet
> mode function correctly except for the button SNAFU.
>
> I will probably pick this little project up again after FOSDEM.

Cool, you're going to FOSDEM, I'm envious :) I'll be going to devconf.cz next
week myself, so will likely reply even slower than usual.

The code looks fine to me, but I have a few comments on the report descriptor.

I think it is better to omit report ID's, features and fields that are not
used by the driver, for clarity.

Physical extents are useless without units and probably don't even apply to
some usages below, like resolution multiplier, so it's better to remove them.
However, adding the units, unit exponent and physical extents to the pen
coordinates is useful. The result will be visible in "evtest" output and
likely used by Wayland.

I'm not sure how repeated report ID items in a single collection even work.
Also duplicate report IDs sound fishy. All-in-all, this is some really weird
report descriptor and would be difficult to reason about. I would start from
scratch, taking one of the fixed report descriptors from other tablets and
adjusting it to the bit fields this one reports.

>   /* I expect patching in these values (from interface 2)
>      would get Xorg to use tablet mode: */
>   0x09, 0x42,         /*          Usage (Tip Switch),         */
>   0x09, 0x44,         /*          Usage (Barrel Switch),      */
>   0x09, 0x45,         /*          Usage (Eraser),             */  /* Really on the barrel too */

I suggest to use "Tablet Pick" here. I added it to the kernel and use it for
the second barrel button on all of the tablets. It will map to the third
button.

>   /* The original usages should have mapped to mouse buttons, but Linux
>      happily reports them as buttons 10-12. */
>   /* 0x05, 0x0f,         / *          Usage Page (Button),                    */
>   /* 0x19, 0x01,         / *          Usage Minimum (01h),                    */
>   /* 0x29, 0x03,         / *          Usage Maximum (03h),                    */
>   /* End of Stylus button usage */

I think having six usages and only three reports wouldn't work.

>   0x15, 0x00,         /*          Logical Minimum (0),                    */
>   0x25, 0x01,         /*          Logical Maximum (1),                    */
>   0x95, 0x03,         /*          Report Count (3),                       */
>   0x75, 0x01,         /*          Report Size (1),                        */
>   0x81, 0x02,         /*          Input (Variable),                       */
>   0x95, 0x05,         /*          Report Count (5),                       */
>   0x81, 0x01,         /*          Input (Constant),                       */
>   0x05, 0x01,         /*          Usage Page (Desktop),                   */
>   0x09, 0x30,         /*          Usage (X),                              */
>   0x09, 0x31,         /*          Usage (Y),                              */
>   0x15, 0x00,         /*          Logical Minimum (0),                    */
>   0x26, 0x00, 0x10,   /*          Logical Maximum (4096),                 */
>   0x35, 0x00,         /*          Physical Minimum (0),                   */
>   0x46, 0x00, 0x10,   /*          Physical Maximum (4096),                */

Is the actual physical drawing area square?

Nick