2022-04-25 06:49:35

by José Expósito

[permalink] [raw]
Subject: Re: Status on hid xppen patch

Hi Stefan,

On Sun, Apr 24, 2022 at 01:19:30AM +0200, Stefan Berzl wrote:
> >> Hello everynyan!
> >>
> >> A while ago I sent in a patch to add support for newer Xp-pen tablets
> >> that even made it into patchwork:
> >>
> >> https://patchwork.kernel.org/project/linux-input/patch/[email protected]/
> >>
> >> I have never actually gotten any feedback on it though and am wondering
> >> if everything is to your liking. Anyway it doesn't build against the
> >> current tree anymore. Assuming there is any value it, should I rewrite
> >> it against hid master or hid uclogic?
> >>
> >> Many thanks
> >>
> >> Stefan Berzl
> >
> > Hi Stefan,
> >
> > I just saw your email in the mailing list.
> >
> > Your patch doesn't apply against the current tree because of the changes
> > from Nikolai/the DIGImend project I'm sending upstream. For reference,
> > here is the latest batch of patches, with links to the previous ones:
> >
> > https://lore.kernel.org/linux-input/[email protected]/T/
> >
> > Please note that I'm not the maintainer of the driver, I'm just a web
> > developer who does free software as a hobby, i.e., this is not my area
> > of expertise, so take my words as suggestions, not as the path to
> > follow ;)
> >
> > The development of the uclogic driver takes place on the DIGImend
> > project (inactive right now):
> > https://github.com/DIGImend/digimend-kernel-drivers
> >
> > Like you, I wanted to add support for my tablet/improve my kernel
> > dev skills, but I noticed that I needed some patches from DIGImend, so,
> > instead of taking what I needed, I decided to upstream all the patches.
> >
> > At the moment of writing this email, 24 patches from DIGImend have been
> > merged, 5 are under review and 7 more need to be sent. We are close to
> > the end.
> >
> > My tablet (Parblo A610 PLUS V2) also needs some magic data to be enabled.
> > Actually, the data is pretty similar to the one in your patch.
> > You can see my implementation here:
> > https://github.com/JoseExposito/linux/commit/f1f24e57fab45a2bcf4e0af5ba9d8f5a2245670b
> >
> > I just refactored my patch and extracted the code to send the magic data
> > to its own function (uclogic_probe_interface), so we can share it.
> >
> > I'd suggest rebasing your code on top of DIGImend's code + my patch
> > so we can share some code and once all patches are upstream, you can
> > send it with minor or without conflicts.
> >
> > What do you think?
> >
> > Jos? Exp?sito
> >
>
> Hello Jos?,
>
> nice hearing from you. I was actually thinking of just asking if you
> want to take my patch under your wing, since you seem to be doing a lot
> of uclogic related work. But your idea is even better, just submitting
> my patch once everything has settled down seems to be the way to go.

Cool, let's do that. I'll cc you on the last batch of patches so you
get notified when they get merged.

> You are actually the first person to ever contact me about this. I was
> pretty worried that the patch is no good, but then I saw that there is
> quite a backlog in the maintainers patchwork and some of your patches
> haven't been reviewed either. Originally I tried showing it to Nikolai,
> but he said he was rather busy too and then some guys in the Digimend
> project tried to get me interested in some userspace driver that works
> around all the issues of like init-packets and stuff. I always imagined
> the kernel as this highly organzied, well-structured endeavor where
> things would be quickly scrutinized and suggestions offered to assure
> the highest quality. Yet here we have two web developers working on the
> graphics tablet driver. Let me say that it's really great you take the
> time and effort to mainline these patches and help everyone enjoy their
> tablets on linux!

Well, the kernel is well-structured, but as you mentioned, there is a
*lot* of work on the maintainers side and I think there is not a big
community supporting this driver. It's normal that it takes some time
to get everything reviewed.

> On the technical side, the magic bytes really are very similar, the only
> difference being the two additional trailing 0x00 for mine. Also my
> tablet sends a response after activating this new interface which can
> get interpreted as a button press if it's not discarded. Is there a nice
> way to work around this, with subreports or anything?

Same here, my tablet responds with: 02 b1 04 00 00 00 00 00 00 00.

In my case, button presses look like: 02 F0 XX XX 00 00 00 00 00 00.
Because of the 0xF0 subreport, I think this data gets ignored, but I'll
do some extra testing just in case.

How did you test it?

> Lastly, do you have an idea in which kernel version the bulk of your
> patches will have been merged, just so I know when to start looking at
> this again? I don't mean to belittle the kernel maintainers of course,
> for I am sure they have more pressing and difficult issues on their
> plate, like eBPF and such.

I couldn't tell, I can imagine that 5.19 or 5.20. Anyway, I'll cc you
so you can have a good idea about what's going on.

Best wishes,
Jos? Exp?sito

> Lets fixie wixie this fucksy uppsy!
>
> Bye
>
>
> Stefan Berzl


2022-05-14 02:44:12

by José Expósito

[permalink] [raw]
Subject: Re: Status on hid xppen patch

Hi Stefan,

A quick update on my work on XP-PEN tablets.

> > nice hearing from you. I was actually thinking of just asking if you
> > want to take my patch under your wing, since you seem to be doing a lot
> > of uclogic related work. But your idea is even better, just submitting
> > my patch once everything has settled down seems to be the way to go.
>
> Cool, let's do that. I'll cc you on the last batch of patches so you
> get notified when they get merged.

As promised, I cc'ed you on the last patchset, which has been merged
already.
At this point, the kernel and DIGImend have the same code.

In the meantime, in order to make the driver code as generic as
possible, I bought a couple of XP-PEN tablets.

The tablets are the Deco Mini 4 and the Deco L, both of them are UGEE
tablets. I already had a UGEE Parblo A610 Pro tablet and after having a
look to the Windows driver traffic, I found out that after sending a
chunk of magic data to enable the tablet, it requests a string
descriptor ("uclogic_params_get_str_desc" can be used here) and the
tablets respond with their parameters.

The information is encoded, in bytes, as:

02 + 03 - UCLOGIC_RDESC_PEN_PH_ID_X_LM
04 + 05 - UCLOGIC_RDESC_PEN_PH_ID_Y_LM
06 - Number of buttons
07 - Dial present or not
08 + 09 - UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM
10 + 11 - Resolution

Bytes 12 and 13 are present but set to 0, probably indicating my
tablets are lacking some feature.

Could you confirm that your tablet returns similar information, please?

In case you want to have a look to the implementation, I'm working on
this branch:
https://github.com/JoseExposito/linux/commits/patch-xppen-deco-l

I had to introduce new functionalities to the templating system of the
driver, that's the reason for the KUnit tests.
The last patch is work in progress (hopefully I'll have time to finish
it this weekend), only the HID descriptors are missing.

I'll cc you when in the patchset so you can add your IDs :)

Best wishes,
Jose

2022-05-14 02:50:36

by Stefan Berzl

[permalink] [raw]
Subject: Re: Status on hid xppen patch

Hello José,

> The tablets are the Deco Mini 4 and the Deco L, both of them are UGEE
> tablets. I already had a UGEE Parblo A610 Pro tablet and after having a
> look to the Windows driver traffic, I found out that after sending a
> chunk of magic data to enable the tablet, it requests a string
> descriptor ("uclogic_params_get_str_desc" can be used here) and the
> tablets respond with their parameters.
>
> The information is encoded, in bytes, as:
>
> 02 + 03 - UCLOGIC_RDESC_PEN_PH_ID_X_LM
> 04 + 05 - UCLOGIC_RDESC_PEN_PH_ID_Y_LM
> 06 - Number of buttons
> 07 - Dial present or not
> 08 + 09 - UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM
> 10 + 11 - Resolution
>
> Bytes 12 and 13 are present but set to 0, probably indicating my
> tablets are lacking some feature.
>
> Could you confirm that your tablet returns similar information, please?

yes, you can haz string descriptor:
0e 03 0b 8b cb 56 08 00 ff 1f d8 13

Byte 12 are 13 are set and zero, as in your case.

> In case you want to have a look to the implementation, I'm working on
> this branch:
> https://github.com/JoseExposito/linux/commits/patch-xppen-deco-l
>
> I had to introduce new functionalities to the templating system of the
> driver, that's the reason for the KUnit tests.
> The last patch is work in progress (hopefully I'll have time to finish
> it this weekend), only the HID descriptors are missing.

I hope you make it, but otherwise you can always use mine, as it's quite
the same as yours. Only the logical minimum and maximum are -60 and 60.

> I'll cc you when in the patchset so you can add your IDs :)

Is that all you want me to do?

Kind regards,
Stefan Berzl

2022-05-16 15:40:32

by José Expósito

[permalink] [raw]
Subject: Re: Status on hid xppen patch

Hi Stefan,

On Fri, May 13, 2022 at 07:22:49PM +0200, Stefan Berzl wrote:
> Hello Jos?,
>
> > The tablets are the Deco Mini 4 and the Deco L, both of them are UGEE
> > tablets. I already had a UGEE Parblo A610 Pro tablet and after having a
> > look to the Windows driver traffic, I found out that after sending a
> > chunk of magic data to enable the tablet, it requests a string
> > descriptor ("uclogic_params_get_str_desc" can be used here) and the
> > tablets respond with their parameters.
> >
> > The information is encoded, in bytes, as:
> >
> > 02 + 03 - UCLOGIC_RDESC_PEN_PH_ID_X_LM
> > 04 + 05 - UCLOGIC_RDESC_PEN_PH_ID_Y_LM
> > 06 - Number of buttons
> > 07 - Dial present or not
> > 08 + 09 - UCLOGIC_RDESC_PEN_PH_ID_PRESSURE_LM
> > 10 + 11 - Resolution
> >
> > Bytes 12 and 13 are present but set to 0, probably indicating my
> > tablets are lacking some feature.
> >
> > Could you confirm that your tablet returns similar information, please?
>
> yes, you can haz string descriptor:
> 0e 03 0b 8b cb 56 08 00 ff 1f d8 13
>
> Byte 12 are 13 are set and zero, as in your case.

Sweet, that means that we are on the right track and the implementation
can be generic... Meaning that we won't need to write and maintain a
bunch of HID descriptors :)

> > In case you want to have a look to the implementation, I'm working on
> > this branch:
> > https://github.com/JoseExposito/linux/commits/patch-xppen-deco-l
> >
> > I had to introduce new functionalities to the templating system of the
> > driver, that's the reason for the KUnit tests.
> > The last patch is work in progress (hopefully I'll have time to finish
> > it this weekend), only the HID descriptors are missing.
>
> I hope you make it, but otherwise you can always use mine, as it's quite
> the same as yours. Only the logical minimum and maximum are -60 and 60.

You are right, 60 and not 127 is the right value. Actually, I think
that -61 - 60 is the correct range, because of the 0.
Running "libinput-debug-tablet" makes it easier to debug.

I also had to fix the descriptor to avoid an issue with the pressure
causing issues with the Deco L, but other than that, it should be
correct now.

> > I'll cc you when in the patchset so you can add your IDs :)
>
> Is that all you want me to do?

Code comments and suggestions are very welcome, as well as testing.

Also, I don't know if you have seen this error after connecting the
tablet:

xhci_hcd 0000:2a:00.3: WARN urb submitted to disabled ep
usb 3-2: reset full-speed USB device number 6 using xhci_hcd
usb 3-2: reset full-speed USB device number 6 using xhci_hcd
[...]

It happens with the Deco Mini 4, even when using the hid-generic
driver. I need to rebase my patches on 5.19 and test again, just to
make sure the problem is not somewhere else.

Jose