| Message ID | cover.1615224800.git.nabijaczleweli@nabijaczleweli.xyz (mailing list archive) |
|---|---|
| Headers | show |
| Series | Stylus-on-touchscreen device support | expand |
On Mon, Mar 08, 2021 at 06:41:49PM +0100, наб wrote: > This patchset adds support for stylus-on-touchscreen devices as found on > the OneMix 3 Pro and Dell Inspiron 15 7000 2-in-1 (7591), among others; > with it, they properly behave like a drawing tablet. > > Patches 2 and 4 funxionally depend on patch 1. > Patch 4 needs patch 3 to apply. > > The output of this patchset and the need for a kernel, rather than > userspace, patch was previously discussed here: > https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/558#note_792834 > > Changes in v2: > Patch 4 now ANDs the secondary button with the tip switch, > since it's otherwise borderline useless to the user. > > Ahelenia Ziemiańska (4): > HID: multitouch: require Finger field to mark Win8 reports as MT > HID: multitouch: set Stylus suffix for Stylus-application devices, too > HID: input: replace outdated HID numbers+comments with macros > HID: input: work around Win8 stylus-on-touchscreen reporting > > drivers/hid/hid-input.c | 54 ++++++++++++++++++++++++++++++++++-- > drivers/hid/hid-multitouch.c | 18 +++++++----- > 2 files changed, 62 insertions(+), 10 deletions(-) > > -- > 2.20.1 Bumping this after a monthish ‒ is it missing something? Am I?
On Tue, 20 Apr 2021, наб wrote: > > This patchset adds support for stylus-on-touchscreen devices as found on > > the OneMix 3 Pro and Dell Inspiron 15 7000 2-in-1 (7591), among others; > > with it, they properly behave like a drawing tablet. > > > > Patches 2 and 4 funxionally depend on patch 1. > > Patch 4 needs patch 3 to apply. > > > > The output of this patchset and the need for a kernel, rather than > > userspace, patch was previously discussed here: > > https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/558#note_792834 > > > > Changes in v2: > > Patch 4 now ANDs the secondary button with the tip switch, > > since it's otherwise borderline useless to the user. > > > > Ahelenia Ziemiańska (4): > > HID: multitouch: require Finger field to mark Win8 reports as MT > > HID: multitouch: set Stylus suffix for Stylus-application devices, too > > HID: input: replace outdated HID numbers+comments with macros > > HID: input: work around Win8 stylus-on-touchscreen reporting > > > > drivers/hid/hid-input.c | 54 ++++++++++++++++++++++++++++++++++-- > > drivers/hid/hid-multitouch.c | 18 +++++++----- > > 2 files changed, 62 insertions(+), 10 deletions(-) > > > > -- > > 2.20.1 > > Bumping this after a monthish ‒ is it missing something? Am I? Benjamin had concerns about regressions and wanted to run a full battery of testing on it. Benjamin, is there any outcome of that, please? Thanks,
On Mon, May 3, 2021 at 11:11 AM Jiri Kosina <jikos@kernel.org> wrote: > > On Tue, 20 Apr 2021, наб wrote: > > > > This patchset adds support for stylus-on-touchscreen devices as found on > > > the OneMix 3 Pro and Dell Inspiron 15 7000 2-in-1 (7591), among others; > > > with it, they properly behave like a drawing tablet. > > > > > > Patches 2 and 4 funxionally depend on patch 1. > > > Patch 4 needs patch 3 to apply. > > > > > > The output of this patchset and the need for a kernel, rather than > > > userspace, patch was previously discussed here: > > > https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/558#note_792834 > > > > > > Changes in v2: > > > Patch 4 now ANDs the secondary button with the tip switch, > > > since it's otherwise borderline useless to the user. > > > > > > Ahelenia Ziemiańska (4): > > > HID: multitouch: require Finger field to mark Win8 reports as MT > > > HID: multitouch: set Stylus suffix for Stylus-application devices, too > > > HID: input: replace outdated HID numbers+comments with macros > > > HID: input: work around Win8 stylus-on-touchscreen reporting > > > > > > drivers/hid/hid-input.c | 54 ++++++++++++++++++++++++++++++++++-- > > > drivers/hid/hid-multitouch.c | 18 +++++++----- > > > 2 files changed, 62 insertions(+), 10 deletions(-) > > > > > > -- > > > 2.20.1 > > > > Bumping this after a monthish ‒ is it missing something? Am I? > > Benjamin had concerns about regressions and wanted to run a full battery > of testing on it. > > Benjamin, is there any outcome of that, please? > Sorry, no real outcome here. I ran the test suite, and there were no errors, until I realized that there are no tests regarding tablets, so it can't detect any regressions here. And then, the usual happens, no time to actually work on the test suite... :( I'll do a "normal" review soon (i.e. today) Cheers, Benjamin
On Mon, May 3, 2021 at 11:39 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Mon, May 3, 2021 at 11:11 AM Jiri Kosina <jikos@kernel.org> wrote: > > > > On Tue, 20 Apr 2021, наб wrote: > > > > > > This patchset adds support for stylus-on-touchscreen devices as found on > > > > the OneMix 3 Pro and Dell Inspiron 15 7000 2-in-1 (7591), among others; > > > > with it, they properly behave like a drawing tablet. > > > > > > > > Patches 2 and 4 funxionally depend on patch 1. > > > > Patch 4 needs patch 3 to apply. > > > > > > > > The output of this patchset and the need for a kernel, rather than > > > > userspace, patch was previously discussed here: > > > > https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/558#note_792834 > > > > > > > > Changes in v2: > > > > Patch 4 now ANDs the secondary button with the tip switch, > > > > since it's otherwise borderline useless to the user. > > > > > > > > Ahelenia Ziemiańska (4): > > > > HID: multitouch: require Finger field to mark Win8 reports as MT > > > > HID: multitouch: set Stylus suffix for Stylus-application devices, too > > > > HID: input: replace outdated HID numbers+comments with macros > > > > HID: input: work around Win8 stylus-on-touchscreen reporting > > > > > > > > drivers/hid/hid-input.c | 54 ++++++++++++++++++++++++++++++++++-- > > > > drivers/hid/hid-multitouch.c | 18 +++++++----- > > > > 2 files changed, 62 insertions(+), 10 deletions(-) > > > > > > > > -- > > > > 2.20.1 > > > > > > Bumping this after a monthish ‒ is it missing something? Am I? > > > > Benjamin had concerns about regressions and wanted to run a full battery > > of testing on it. > > > > Benjamin, is there any outcome of that, please? > > > > Sorry, no real outcome here. > > I ran the test suite, and there were no errors, until I realized that > there are no tests regarding tablets, so it can't detect any > regressions here. > And then, the usual happens, no time to actually work on the test suite... :( > > I'll do a "normal" review soon (i.e. today) > So I did a quick pass at the patches: - 1/4 -> I think this one is safe and could go as it is, maybe with CC: stable on it. Any regressions should have been caught by the testsuite, so that's a good one. - 2/4 and 3/4 -> Ack on those 2 too, note stable material, but not necessary v5.13 material - 4/4 -> I honestly have no idea if the patch is correct or not. I would hold on this one until we have proper tests for those. Jiri, would you be ok to split the series as this? наб, would you be OK to work on the test suite at https://gitlab.freedesktop.org/libevdev/hid-tools so we can move forward for your last patch? The problem I see on the last patch is that it is touching a generic path and is not trivial. So adding tests would have 3 benefits: - we ensure we are doing the correct thing - we ensure we are not breaking existing devices (to some extent, given that the tests are non written for the tablets) - we ensure we are not breaking that in the future. Cheers, Benjamin
On Mon, May 03, 2021 at 11:52:43AM +0200, Benjamin Tissoires wrote: > On Mon, May 3, 2021 at 11:39 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Mon, May 3, 2021 at 11:11 AM Jiri Kosina <jikos@kernel.org> wrote: > > > > > > On Tue, 20 Apr 2021, наб wrote: > > > > > > > > This patchset adds support for stylus-on-touchscreen devices as found on > > > > > the OneMix 3 Pro and Dell Inspiron 15 7000 2-in-1 (7591), among others; > > > > > with it, they properly behave like a drawing tablet. > > > > > > > > > > Patches 2 and 4 funxionally depend on patch 1. > > > > > Patch 4 needs patch 3 to apply. > > > > > > > > > > The output of this patchset and the need for a kernel, rather than > > > > > userspace, patch was previously discussed here: > > > > > https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/558#note_792834 > > > > > > > > > > Changes in v2: > > > > > Patch 4 now ANDs the secondary button with the tip switch, > > > > > since it's otherwise borderline useless to the user. > > > > > > > > > > Ahelenia Ziemiańska (4): > > > > > HID: multitouch: require Finger field to mark Win8 reports as MT > > > > > HID: multitouch: set Stylus suffix for Stylus-application devices, too > > > > > HID: input: replace outdated HID numbers+comments with macros > > > > > HID: input: work around Win8 stylus-on-touchscreen reporting > > > > > > > > > > drivers/hid/hid-input.c | 54 ++++++++++++++++++++++++++++++++++-- > > > > > drivers/hid/hid-multitouch.c | 18 +++++++----- > > > > > 2 files changed, 62 insertions(+), 10 deletions(-) > > > > > > > > > > -- > > > > > 2.20.1 > > > > > > > > Bumping this after a monthish ‒ is it missing something? Am I? > > > > > > Benjamin had concerns about regressions and wanted to run a full battery > > > of testing on it. > > > > > > Benjamin, is there any outcome of that, please? > > > > > > > Sorry, no real outcome here. > > > > I ran the test suite, and there were no errors, until I realized that > > there are no tests regarding tablets, so it can't detect any > > regressions here. > > And then, the usual happens, no time to actually work on the test suite... :( > > > > I'll do a "normal" review soon (i.e. today) > > > > So I did a quick pass at the patches: > - 1/4 -> I think this one is safe and could go as it is, maybe with > CC: stable on it. Any regressions should have been caught by the > testsuite, so that's a good one. > - 2/4 and 3/4 -> Ack on those 2 too, note stable material, but not > necessary v5.13 material > - 4/4 -> I honestly have no idea if the patch is correct or not. I > would hold on this one until we have proper tests for those. > > Jiri, would you be ok to split the series as this? Splitting 2/4 away from 1/4 presents a minor cosmetic problem: since 1/4 no longer tags the stylus-on-touchscreen device as MT, the device name turns from "GXTP7386:00 27C6:0113 Stylus" to "GXTP7386:00 27C6:0113", so the user is left with two identically-named devices, the first of which corresponds to the touchscreen, and the second to the stylus. Granted, it might also append "Stylus" to names that could contain it, but I haven't managed to trace where hdev->name is born to determine if that's a concern. > наб, would you be OK to work on the test suite at > https://gitlab.freedesktop.org/libevdev/hid-tools so we can move > forward for your last patch? > > The problem I see on the last patch is that it is touching a generic > path and is not trivial. So adding tests would have 3 benefits: > - we ensure we are doing the correct thing > - we ensure we are not breaking existing devices (to some extent, > given that the tests are non written for the tablets) > - we ensure we are not breaking that in the future. I'd be more than happy to add tests of some sort, but reading the repository and tests/ under it has me positively stumped, not a clue where an entry-point would be, or how I'd instrument a reasonable test around my rdesc, so some sort of vague guidance to that end would be greatly appreciated. Best, наб
On Mon, May 3, 2021 at 3:00 PM наб <nabijaczleweli@nabijaczleweli.xyz> wrote: > > On Mon, May 03, 2021 at 11:52:43AM +0200, Benjamin Tissoires wrote: > > On Mon, May 3, 2021 at 11:39 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > On Mon, May 3, 2021 at 11:11 AM Jiri Kosina <jikos@kernel.org> wrote: > > > > > > > > On Tue, 20 Apr 2021, наб wrote: > > > > > > > > > > This patchset adds support for stylus-on-touchscreen devices as found on > > > > > > the OneMix 3 Pro and Dell Inspiron 15 7000 2-in-1 (7591), among others; > > > > > > with it, they properly behave like a drawing tablet. > > > > > > > > > > > > Patches 2 and 4 funxionally depend on patch 1. > > > > > > Patch 4 needs patch 3 to apply. > > > > > > > > > > > > The output of this patchset and the need for a kernel, rather than > > > > > > userspace, patch was previously discussed here: > > > > > > https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/558#note_792834 > > > > > > > > > > > > Changes in v2: > > > > > > Patch 4 now ANDs the secondary button with the tip switch, > > > > > > since it's otherwise borderline useless to the user. > > > > > > > > > > > > Ahelenia Ziemiańska (4): > > > > > > HID: multitouch: require Finger field to mark Win8 reports as MT > > > > > > HID: multitouch: set Stylus suffix for Stylus-application devices, too > > > > > > HID: input: replace outdated HID numbers+comments with macros > > > > > > HID: input: work around Win8 stylus-on-touchscreen reporting > > > > > > > > > > > > drivers/hid/hid-input.c | 54 ++++++++++++++++++++++++++++++++++-- > > > > > > drivers/hid/hid-multitouch.c | 18 +++++++----- > > > > > > 2 files changed, 62 insertions(+), 10 deletions(-) > > > > > > > > > > > > -- > > > > > > 2.20.1 > > > > > > > > > > Bumping this after a monthish ‒ is it missing something? Am I? > > > > > > > > Benjamin had concerns about regressions and wanted to run a full battery > > > > of testing on it. > > > > > > > > Benjamin, is there any outcome of that, please? > > > > > > > > > > Sorry, no real outcome here. > > > > > > I ran the test suite, and there were no errors, until I realized that > > > there are no tests regarding tablets, so it can't detect any > > > regressions here. > > > And then, the usual happens, no time to actually work on the test suite... :( > > > > > > I'll do a "normal" review soon (i.e. today) > > > > > > > So I did a quick pass at the patches: > > - 1/4 -> I think this one is safe and could go as it is, maybe with > > CC: stable on it. Any regressions should have been caught by the > > testsuite, so that's a good one. > > - 2/4 and 3/4 -> Ack on those 2 too, note stable material, but not > > necessary v5.13 material > > - 4/4 -> I honestly have no idea if the patch is correct or not. I > > would hold on this one until we have proper tests for those. > > > > Jiri, would you be ok to split the series as this? > > Splitting 2/4 away from 1/4 presents a minor cosmetic problem: > since 1/4 no longer tags the stylus-on-touchscreen device as MT, > the device name turns from "GXTP7386:00 27C6:0113 Stylus" > to "GXTP7386:00 27C6:0113", so the user is left with > two identically-named devices, the first of which corresponds > to the touchscreen, and the second to the stylus. > > Granted, it might also append "Stylus" to names that could contain it, > but I haven't managed to trace where hdev->name is born to determine if > that's a concern. > > > наб, would you be OK to work on the test suite at > > https://gitlab.freedesktop.org/libevdev/hid-tools so we can move > > forward for your last patch? > > > > The problem I see on the last patch is that it is touching a generic > > path and is not trivial. So adding tests would have 3 benefits: > > - we ensure we are doing the correct thing > > - we ensure we are not breaking existing devices (to some extent, > > given that the tests are non written for the tablets) > > - we ensure we are not breaking that in the future. > > I'd be more than happy to add tests of some sort, but reading the > repository and tests/ under it has me positively stumped, > not a clue where an entry-point would be, or how I'd instrument a > reasonable test around my rdesc, so some sort of vague guidance > to that end would be greatly appreciated. > Sorry, it was a little bit harsh to ask this from scratch. I created an initial MR you can work on top of it: https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/115 Basically, you need to add your device in `tests/test_tablet.py` (like all the other devices). You probably also need to add more tests to the two I added, to expose the bug. Hopefully, just doing that should expose the bug. If not, you'll need to subclass PenDigitizer and override the `event()` function to actually match what is sent on the wire when you request a high level `Pen()` event. To run the test suite, we are using `pytest`, which means it is all automagic: `sudo pytest -k 'tablet' -v` will run every `test_*` pytest finds with the `tablet` string in it. You can be more specific by just running your device by issuing `sudo pytest -k 'tablet and VENDOR_VID_PID' -v` (replace VENDOR_VID_PID with the name of the class). Note also that the patch 2 here will probably make the `test_creation` fail as this one checks for the name. We will need a separate MR for this depending on how the patches end up being included in the kernel tree. Hope this helps. Cheers, Benjamin
On Mon, 3 May 2021, Benjamin Tissoires wrote: > So I did a quick pass at the patches: > - 1/4 -> I think this one is safe and could go as it is, maybe with > CC: stable on it. Any regressions should have been caught by the > testsuite, so that's a good one. Added stable tag and applied to hid.git#for-5.13/upstream-fixes > - 2/4 and 3/4 -> Ack on those 2 too, note stable material, but not > necessary v5.13 material I have queued 2/4 for to hid.git#for-5.13/upstream-fixes as well, while 3/4 to hid.git#for-5.14/core. Thanks,