[PATCH v2 0/4] Stylus-on-touchscreen device support

STALE1922d

Revision v2 of 2 in this series.

12 messages, 3 authors, 2021-05-05 · open the first message on its own page

[PATCH v2 0/4] Stylus-on-touchscreen device support

From: наб <hidden>
Date: 2021-03-08 17:42:50

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

[PATCH v2 3/4] HID: input: replace outdated HID numbers+comments with macros

From: Ahelenia Ziemiańska <hidden>
Date: 2021-03-08 17:42:50

These were untouched since 2.3.99-pre3, and the explanatory comment for
HID_DG_TIPPRESSURE is TipPressure in other places

Signed-off-by: Ahelenia Ziemiańska <redacted>
---
 drivers/hid/hid-input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index f23027d2795b..a5ba92978473 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1301,12 +1301,12 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
-	if (usage->hid == (HID_UP_DIGITIZER | 0x003c)) { /* Invert */
+	if (usage->hid == HID_DG_INVERT) {
 		*quirks = value ? (*quirks | HID_QUIRK_INVERT) : (*quirks & ~HID_QUIRK_INVERT);
 		return;
 	}
 
-	if (usage->hid == (HID_UP_DIGITIZER | 0x0032)) { /* InRange */
+	if (usage->hid == HID_DG_INRANGE) {
 		if (value) {
 			input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1);
 			return;
@@ -1316,7 +1316,7 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
-	if (usage->hid == (HID_UP_DIGITIZER | 0x0030) && (*quirks & HID_QUIRK_NOTOUCH)) { /* Pressure */
+	if (usage->hid == HID_DG_TIPPRESSURE && (*quirks & HID_QUIRK_NOTOUCH)) {
 		int a = field->logical_minimum;
 		int b = field->logical_maximum;
 		input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
-- 
2.20.1

[PATCH v2 1/4] HID: multitouch: require Finger field to mark Win8 reports as MT

From: Ahelenia Ziemiańska <hidden>
Date: 2021-03-08 17:42:50

This effectively changes collection_is_mt from
  contact ID in report->field
to
  (device is Win8 => collection is finger) && contact ID in report->field

Some devices erroneously report Pen for fingers,
and Win8 stylus-on-touchscreen devices report contact ID,
but mark the accompanying touchscreen device's collection correctly

Signed-off-by: Ahelenia Ziemiańska <redacted>
---
 drivers/hid/hid-multitouch.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 8429ebe7097e..8580ace596c2 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -604,9 +604,13 @@ static struct mt_report_data *mt_allocate_report_data(struct mt_device *td,
 		if (!(HID_MAIN_ITEM_VARIABLE & field->flags))
 			continue;
 
-		for (n = 0; n < field->report_count; n++) {
-			if (field->usage[n].hid == HID_DG_CONTACTID)
-				rdata->is_mt_collection = true;
+		if (field->logical == HID_DG_FINGER || td->hdev->group != HID_GROUP_MULTITOUCH_WIN_8) {
+			for (n = 0; n < field->report_count; n++) {
+				if (field->usage[n].hid == HID_DG_CONTACTID) {
+					rdata->is_mt_collection = true;
+					break;
+				}
+			}
 		}
 	}
 
-- 
2.20.1

[PATCH v2 2/4] HID: multitouch: set Stylus suffix for Stylus-application devices, too

From: Ahelenia Ziemiańska <hidden>
Date: 2021-03-08 17:42:50

This re-adds the suffix to Win8 stylus-on-touchscreen devices,
now that they aren't erroneously marked as MT

Signed-off-by: Ahelenia Ziemiańska <redacted>
---
 drivers/hid/hid-multitouch.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 8580ace596c2..e5a3704b9fe8 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1580,13 +1580,13 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 		/* we do not set suffix = "Touchscreen" */
 		hi->input->name = hdev->name;
 		break;
-	case HID_DG_STYLUS:
-		/* force BTN_STYLUS to allow tablet matching in udev */
-		__set_bit(BTN_STYLUS, hi->input->keybit);
-		break;
 	case HID_VD_ASUS_CUSTOM_MEDIA_KEYS:
 		suffix = "Custom Media Keys";
 		break;
+	case HID_DG_STYLUS:
+		/* force BTN_STYLUS to allow tablet matching in udev */
+		__set_bit(BTN_STYLUS, hi->input->keybit);
+		fallthrough;
 	case HID_DG_PEN:
 		suffix = "Stylus";
 		break;
-- 
2.20.1

[PATCH v2 4/4] HID: input: work around Win8 stylus-on-touchscreen reporting

From: Ahelenia Ziemiańska <hidden>
Date: 2021-03-08 17:43:20

With this, these devices now behave as tablets as expected by userspace

The search in hidinput_is_win8_touching() terminates at f=0, u=0
on Goodix screens (27C6:0111, 27C6:0113), but I expect it
to have negligible impact on devices that don't have TipSwitch
as the first report as well

Signed-off-by: Ahelenia Ziemiańska <redacted>
---

Notes:
    changes in v2:
      * hidinput_fixup_win8_inrange() became hidinput_is_win8_touching()
      * BarrelSwitch now anded with TipSwitch

 drivers/hid/hid-input.c | 48 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index a5ba92978473..aee1f1283c1d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1273,6 +1273,42 @@ static void hidinput_handle_scroll(struct hid_usage *usage,
 	input_event(input, EV_REL, usage->code, hi_res);
 }
 
+/*
+ * Win8 tablet stylus devices send, in order:
+ *   HID_DG_TIPSWITCH (BTN_TOUCH)
+ *   HID_DG_INVERT    (BTN_TOOL_RUBBER)
+ *   HID_DG_ERASER    (BTN_TOUCH)
+ *   HID_DG_INRANGE   (BTN_TOOL_PEN)
+ *
+ * For each of these states:
+ *   hover     :                         INRANGE
+ *   touching  : TIPSWITCH
+ *   hover+2   :           INVERT        INRANGE
+ *   touching+2:                  ERASER INRANGE
+ *
+ * Which means we'd send BTN_TOUCH=0 + BTN_TOOL_PEN=1 on proximity,
+ * then BTN_TOUCH=1 and BTN_TOOL_PEN=0 in consecutive groups when touched,
+ * indicating the stylus leaving the screen as soon as the two meet.
+ *
+ * Additionally, HID_DG_BARRELSWITCH corresponds directly to the button,
+ * regardless of the tip switch, making it borderline impossible to use precisely.
+ */
+static bool hidinput_is_win8_touching(struct hid_device *hid, struct hid_field *field)
+{
+	unsigned f, u;
+	struct hid_field *rfield;
+
+	for (f = 0; f < field->report->maxfield; ++f) {
+		rfield = field->report->field[f];
+		for (u = 0; u < rfield->maxusage; ++u) {
+			if (rfield->usage[u].hid == HID_DG_TIPSWITCH)
+				return rfield->value[u];
+		}
+	}
+
+	return false;
+}
+
 void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value)
 {
 	struct input_dev *input;
@@ -1306,7 +1342,13 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
+	if (usage->hid == HID_DG_ERASER && value)
+		*quirks |= HID_QUIRK_INVERT;
+
 	if (usage->hid == HID_DG_INRANGE) {
+		if (hid->group == HID_GROUP_MULTITOUCH_WIN_8)
+			value = value || hidinput_is_win8_touching(hid, field);
+
 		if (value) {
 			input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1);
 			return;
@@ -1322,6 +1364,12 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
 	}
 
+	if (usage->hid == HID_DG_BARRELSWITCH && hid->group == HID_GROUP_MULTITOUCH_WIN_8) {
+		value = value && hidinput_is_win8_touching(hid, field);
+		input_event(input, usage->type, usage->code, value);
+		return;
+	}
+
 	if (usage->hid == (HID_UP_PID | 0x83UL)) { /* Simultaneous Effects Max */
 		dbg_hid("Maximum Effects - %d\n",value);
 		return;
-- 
2.20.1

Re: [PATCH v2 0/4] Stylus-on-touchscreen device support

From: наб <hidden>
Date: 2021-04-20 13:26:22

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?

Re: [PATCH v2 0/4] Stylus-on-touchscreen device support

From: Jiri Kosina <jikos@kernel.org>
Date: 2021-05-03 09:11:26

On Tue, 20 Apr 2021, наб wrote:
quoted
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,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 0/4] Stylus-on-touchscreen device support

From: Benjamin Tissoires <hidden>
Date: 2021-05-03 09:39:50

On Mon, May 3, 2021 at 11:11 AM Jiri Kosina [off-list ref] wrote:
On Tue, 20 Apr 2021, наб wrote:
quoted
quoted
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

Re: [PATCH v2 0/4] Stylus-on-touchscreen device support

From: Benjamin Tissoires <hidden>
Date: 2021-05-03 09:53:02

On Mon, May 3, 2021 at 11:39 AM Benjamin Tissoires
[off-list ref] wrote:
On Mon, May 3, 2021 at 11:11 AM Jiri Kosina [off-list ref] wrote:
quoted
On Tue, 20 Apr 2021, наб wrote:
quoted
quoted
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

Re: [PATCH v2 0/4] Stylus-on-touchscreen device support

From: наб <hidden>
Date: 2021-05-03 13:06:30

On Mon, May 03, 2021 at 11:52:43AM +0200, Benjamin Tissoires wrote:
On Mon, May 3, 2021 at 11:39 AM Benjamin Tissoires
[off-list ref] wrote:
quoted
On Mon, May 3, 2021 at 11:11 AM Jiri Kosina [off-list ref] wrote:
quoted
On Tue, 20 Apr 2021, наб wrote:
quoted
quoted
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,
наб

Re: [PATCH v2 0/4] Stylus-on-touchscreen device support

From: Benjamin Tissoires <hidden>
Date: 2021-05-04 13:35:29

On Mon, May 3, 2021 at 3:00 PM наб [off-list ref] wrote:
On Mon, May 03, 2021 at 11:52:43AM +0200, Benjamin Tissoires wrote:
quoted
On Mon, May 3, 2021 at 11:39 AM Benjamin Tissoires
[off-list ref] wrote:
quoted
On Mon, May 3, 2021 at 11:11 AM Jiri Kosina [off-list ref] wrote:
quoted
On Tue, 20 Apr 2021, наб wrote:
quoted
quoted
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.
quoted
наб, 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

Re: [PATCH v2 0/4] Stylus-on-touchscreen device support

From: Jiri Kosina <jikos@kernel.org>
Date: 2021-05-05 13:08:04

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,

-- 
Jiri Kosina
SUSE Labs
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help