From ab9b82909e9baa5b559d6457487525cfd4df2738 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:04 +0100 Subject: [PATCH] selftests/hid: tablets: be stricter for some transitions 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. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-13-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 132 +++++++++++++++--- 1 file changed, 113 insertions(+), 19 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index a82db66264c59..9374bd7524efa 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -13,7 +13,7 @@ import libevdev import logging import pytest -from typing import Dict, Optional, Tuple +from typing import Dict, List, Optional, Tuple logger = logging.getLogger("hidtools.test.tablet") @@ -124,7 +124,7 @@ def from_evdev(cls, evdev) -> "PenState": return cls((touch, tool, button)) - def apply(self, events) -> "PenState": + def apply(self, events: List[libevdev.InputEvent], strict: bool) -> "PenState": if libevdev.EV_SYN.SYN_REPORT in events: raise ValueError("EV_SYN is in the event sequence") touch = self.touch @@ -163,13 +163,97 @@ def apply(self, events) -> "PenState": 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.historically_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 historically_tolerated_transitions(self) -> Tuple["PenState", ...]: """Following the state machine in the URL above, with a couple of addition for skipping the in-range state, due to historical reasons. @@ -693,10 +777,14 @@ def post(self, uhdev, pen): 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: @@ -706,12 +794,12 @@ def validate_transitions(self, from_state, pen, evdev, events): 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 @@ -726,7 +814,9 @@ def _test_states(self, state_list, scribble): 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 @@ -735,14 +825,18 @@ def _test_states(self, state_list, scribble): 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"]) @@ -755,7 +849,7 @@ def test_valid_pen_states(self, state_list, scribble): 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, allow_intermediate_states=False) @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) @pytest.mark.parametrize( @@ -769,7 +863,7 @@ def test_tolerated_pen_states(self, state_list, scribble): """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, allow_intermediate_states=True) @pytest.mark.skip_if_uhdev( lambda uhdev: "Barrel Switch" not in uhdev.fields, @@ -785,7 +879,7 @@ def test_tolerated_pen_states(self, state_list, scribble): ) 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, allow_intermediate_states=False) @pytest.mark.skip_if_uhdev( lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields, @@ -801,7 +895,7 @@ def test_valid_primary_button_pen_states(self, state_list, scribble): ) 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, allow_intermediate_states=False) @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, @@ -821,7 +915,7 @@ def test_valid_invert_pen_states(self, state_list, scribble): 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, allow_intermediate_states=False) @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, @@ -841,7 +935,7 @@ def test_tolerated_invert_pen_states(self, state_list, scribble): 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, allow_intermediate_states=True) @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, @@ -858,7 +952,7 @@ def test_tolerated_broken_pen_states(self, state_list, scribble): 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, allow_intermediate_states=True) class GXTP_pen(PenDigitizer):