From: Peter Hutterer <peter.hutterer@who-t.net>
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Shuah Khan <shuah@kernel.org>,
linux-input@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 12/12] selftests/hid: tablets: be stricter for some transitions
Date: Fri, 1 Dec 2023 15:50:26 +1000 [thread overview]
Message-ID: <20231201055026.GA615081@quokka> (raw)
In-Reply-To: <20231129-wip-selftests-v1-12-ba15a1fe1b0d@kernel.org>
On Wed, Nov 29, 2023 at 04:24:37PM +0100, Benjamin Tissoires wrote:
> To accommodate for legacy devices, we rely on the last state of a
> transition to be valid:
> for example when we test PEN_IS_OUT_OF_RANGE to PEN_IS_IN_CONTACT,
> any "normal" device that reports an InRange bit would insert a
> PEN_IS_IN_RANGE state between the 2.
>
> This is of course valid, but this solution prevents to detect false
> releases emitted by some firmware:
> when pressing an "eraser mode" button, they might send an extra
> PEN_IS_OUT_OF_RANGE that we may want to filter.
>
> So define 2 sets of transitions: one that is the ideal behavior, and
> one that is OK, it won't break user space, but we have serious doubts
> if we are doing the right thing. And depending on the test, either
> ask only for valid transitions, or tolerate weird ones.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> tools/testing/selftests/hid/tests/test_tablet.py | 122 +++++++++++++++++++----
> 1 file changed, 104 insertions(+), 18 deletions(-)
>
> diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
> index f24cf2e168a4..625dd9dcb935 100644
> --- a/tools/testing/selftests/hid/tests/test_tablet.py
> +++ b/tools/testing/selftests/hid/tests/test_tablet.py
> @@ -109,7 +109,7 @@ class PenState(Enum):
>
> return cls((touch, tool, button))
>
> - def apply(self, events) -> "PenState":
> + def apply(self, events, strict) -> "PenState":
fwiw, if you're doing type annotations anyway, it'd be nice to do them
for args as well, `strict: bool` in this case.
> if libevdev.EV_SYN.SYN_REPORT in events:
> raise ValueError("EV_SYN is in the event sequence")
> touch = self.touch
> @@ -148,13 +148,97 @@ class PenState(Enum):
> button = None
>
> new_state = PenState((touch, tool, button))
> - assert (
> - new_state in self.valid_transitions()
> - ), f"moving from {self} to {new_state} is forbidden"
> + if strict:
> + assert (
> + new_state in self.valid_transitions()
> + ), f"moving from {self} to {new_state} is forbidden"
> + else:
> + assert (
> + new_state in self.historical_tolerated_transitions()
> + ), f"moving from {self} to {new_state} is forbidden"
>
> return new_state
>
> def valid_transitions(self) -> Tuple["PenState", ...]:
> + """Following the state machine in the URL above.
> +
> + Note that those transitions are from the evdev point of view, not HID"""
> + if self == PenState.PEN_IS_OUT_OF_RANGE:
> + return (
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_ERASING,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE:
> + return (
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT:
> + return (
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_ERASING,
> + )
> +
> + if self == PenState.PEN_IS_ERASING:
> + return (
> + PenState.PEN_IS_ERASING,
> + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
> + return (
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
> + return (
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + )
> +
> + return tuple()
> +
> + def historical_tolerated_transitions(self) -> Tuple["PenState", ...]:
s/historically/ to be grammatically correct, I guess.
> """Following the state machine in the URL above, with a couple of addition
> for skipping the in-range state, due to historical reasons.
>
> @@ -678,10 +762,12 @@ class BaseTest:
> self.debug_reports(r, uhdev, events)
> return events
>
> - def validate_transitions(self, from_state, pen, evdev, events):
> + def validate_transitions(self, from_state, pen, evdev, events, allow_intermediate_states):
> # check that the final state is correct
> pen.assert_expected_input_events(evdev)
>
> + state = from_state
> +
> # check that the transitions are valid
> sync_events = []
> while libevdev.InputEvent(libevdev.EV_SYN.SYN_REPORT) in events:
> @@ -691,12 +777,12 @@ class BaseTest:
> events = events[idx + 1 :]
>
> # now check for a valid transition
> - from_state = from_state.apply(sync_events)
> + state = state.apply(sync_events, not allow_intermediate_states)
>
> if events:
> - from_state = from_state.apply(sync_events)
> + state = state.apply(sync_events, not allow_intermediate_states)
>
> - def _test_states(self, state_list, scribble):
> + def _test_states(self, state_list, scribble, allow_intermediate_states):
> """Internal method to test against a list of
> transition between states.
> state_list is a list of PenState objects
> @@ -711,7 +797,7 @@ class BaseTest:
> p = Pen(50, 60)
> uhdev.move_to(p, PenState.PEN_IS_OUT_OF_RANGE)
> events = self.post(uhdev, p)
> - self.validate_transitions(cur_state, p, evdev, events)
> + self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
>
> cur_state = p.current_state
>
> @@ -720,14 +806,14 @@ class BaseTest:
> p.x += 1
> p.y -= 1
> events = self.post(uhdev, p)
> - self.validate_transitions(cur_state, p, evdev, events)
> + self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
> assert len(events) >= 3 # X, Y, SYN
> uhdev.move_to(p, state)
> if scribble and state != PenState.PEN_IS_OUT_OF_RANGE:
> p.x += 1
> p.y -= 1
> events = self.post(uhdev, p)
> - self.validate_transitions(cur_state, p, evdev, events)
> + self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
> cur_state = p.current_state
>
> @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
> @@ -740,7 +826,7 @@ class BaseTest:
> we don't have Invert nor Erase bits, so just move in/out-of-range or proximity.
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> """
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
not everyone's cup of tea but code like this becomes more immediately
readable as:
self._test_states(state_list, scribble, allow_intermediate_states=False)
Anyway, this looks good to me (esp the intention) and is
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Cheers,
Peter
>
> @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
> @pytest.mark.parametrize(
> @@ -754,7 +840,7 @@ class BaseTest:
> """This is not adhering to the Windows Pen Implementation state machine
> but we should expect the kernel to behave properly, mostly for historical
> reasons."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, True)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Barrel Switch" not in uhdev.fields,
> @@ -770,7 +856,7 @@ class BaseTest:
> )
> def test_valid_primary_button_pen_states(self, state_list, scribble):
> """Rework the transition state machine by adding the primary button."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields,
> @@ -786,7 +872,7 @@ class BaseTest:
> )
> def test_valid_secondary_button_pen_states(self, state_list, scribble):
> """Rework the transition state machine by adding the secondary button."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Invert" not in uhdev.fields,
> @@ -806,7 +892,7 @@ class BaseTest:
> to erase.
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> """
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Invert" not in uhdev.fields,
> @@ -826,7 +912,7 @@ class BaseTest:
> to erase.
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> """
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, True)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Invert" not in uhdev.fields,
> @@ -843,7 +929,7 @@ class BaseTest:
> For example, a pen that has the eraser button might wobble between
> touching and erasing if the tablet doesn't enforce the Windows
> state machine."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, True)
>
>
> class GXTP_pen(PenDigitizer):
>
> --
> 2.41.0
>
prev parent reply other threads:[~2023-12-01 5:50 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-29 15:24 [PATCH 00/12] selftests/hid: tablets fixes Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 01/12] selftests/hid: vmtest.sh: update vm2c and container Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 02/12] selftests/hid: vmtest.sh: allow finer control on the build steps Benjamin Tissoires
2023-12-01 5:34 ` Peter Hutterer
2023-11-29 15:24 ` [PATCH 03/12] selftests/hid: base: allow for multiple skip_if_uhdev Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 04/12] selftests/hid: tablets: remove unused class Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 05/12] selftests/hid: tablets: move the transitions to PenState Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 06/12] selftests/hid: tablets: move move_to function to PenDigitizer Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 07/12] selftests/hid: tablets: do not set invert when the eraser is used Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 08/12] selftests/hid: tablets: set initial data for tilt/twist Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 09/12] selftests/hid: tablets: add variants of states with buttons Benjamin Tissoires
2023-12-01 5:41 ` Peter Hutterer
2023-11-29 15:24 ` [PATCH 10/12] selftests/hid: tablets: convert the primary button tests Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 11/12] selftests/hid: tablets: add a secondary barrel switch test Benjamin Tissoires
2023-11-29 15:24 ` [PATCH 12/12] selftests/hid: tablets: be stricter for some transitions Benjamin Tissoires
2023-12-01 5:50 ` Peter Hutterer [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231201055026.GA615081@quokka \
--to=peter.hutterer@who-t.net \
--cc=benjamin.tissoires@redhat.com \
--cc=bentiss@kernel.org \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox