diff mbox series

[12/12] selftests/hid: tablets: be stricter for some transitions

Message ID 20231129-wip-selftests-v1-12-ba15a1fe1b0d@kernel.org (mailing list archive)
State Superseded
Headers show
Series selftests/hid: tablets fixes | expand

Commit Message

Benjamin Tissoires Nov. 29, 2023, 3:24 p.m. UTC 
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(-)

Comments

Peter Hutterer Dec. 1, 2023, 5:50 a.m. UTC | #1 
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
>
diff mbox series

Patch

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":
         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", ...]:
         """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)
 
         @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):