Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Feedback-ID: i7ce144cd:Fastmail
Date:   Fri, 1 Dec 2023 15:41:27 +1000
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 09/12] selftests/hid: tablets: add variants of states
 with buttons
Message-ID: <20231201054127.GA626305@quokka>
References: <20231129-wip-selftests-v1-0-ba15a1fe1b0d@kernel.org>
 <20231129-wip-selftests-v1-9-ba15a1fe1b0d@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231129-wip-selftests-v1-9-ba15a1fe1b0d@kernel.org>
Precedence: bulk

On Wed, Nov 29, 2023 at 04:24:34PM +0100, Benjamin Tissoires wrote:
> Turns out that there are transitions that are unlikely to happen:
> for example, having both the tip switch and a button being changed
> at the same time (in the same report) would require either a very talented
> and precise user or a very bad hardware with a very low sampling rate.
> 
> So instead of manually building the button test by hand and forgetting
> about some cases, let's reuse the state machine and transitions we have.
> 
> This patch only adds the states and the valid transitions. The actual
> tests will be replaced later.
> 
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
>  tools/testing/selftests/hid/tests/test_tablet.py | 170 +++++++++++++++++++++--
>  1 file changed, 157 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
> index 83f6501fe984..80269d1a0f0a 100644
> --- a/tools/testing/selftests/hid/tests/test_tablet.py
> +++ b/tools/testing/selftests/hid/tests/test_tablet.py
> @@ -21,22 +21,66 @@ logger = logging.getLogger("hidtools.test.tablet")
>  class PenState(Enum):
>      """Pen states according to Microsoft reference:
>      https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> -    """
>  
> -    PEN_IS_OUT_OF_RANGE = (False, None)
> -    PEN_IS_IN_RANGE = (False, libevdev.EV_KEY.BTN_TOOL_PEN)
> -    PEN_IS_IN_CONTACT = (True, libevdev.EV_KEY.BTN_TOOL_PEN)
> -    PEN_IS_IN_RANGE_WITH_ERASING_INTENT = (False, libevdev.EV_KEY.BTN_TOOL_RUBBER)
> -    PEN_IS_ERASING = (True, libevdev.EV_KEY.BTN_TOOL_RUBBER)
> +    We extend it with the various buttons when we need to check them.
> +    """
>  
> -    def __init__(self, touch, tool):

It'd be nice to have a comment here what the False refers to. Even
nicer would be an enum class BtnTouch.DOWN so the code is instantly
readable :)

Cheers,
  Peter

> +    PEN_IS_OUT_OF_RANGE = (False, None, None)
> +    PEN_IS_IN_RANGE = (False, libevdev.EV_KEY.BTN_TOOL_PEN, None)
> +    PEN_IS_IN_RANGE_WITH_BUTTON = (
> +        False,
> +        libevdev.EV_KEY.BTN_TOOL_PEN,
> +        libevdev.EV_KEY.BTN_STYLUS,
> +    )
> +    PEN_IS_IN_RANGE_WITH_SECOND_BUTTON = (
> +        False,
> +        libevdev.EV_KEY.BTN_TOOL_PEN,
> +        libevdev.EV_KEY.BTN_STYLUS2,
> +    )
> +    PEN_IS_IN_CONTACT = (True, libevdev.EV_KEY.BTN_TOOL_PEN, None)
> +    PEN_IS_IN_CONTACT_WITH_BUTTON = (
> +        True,
> +        libevdev.EV_KEY.BTN_TOOL_PEN,
> +        libevdev.EV_KEY.BTN_STYLUS,
> +    )
> +    PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON = (
> +        True,
> +        libevdev.EV_KEY.BTN_TOOL_PEN,
> +        libevdev.EV_KEY.BTN_STYLUS2,
> +    )
> +    PEN_IS_IN_RANGE_WITH_ERASING_INTENT = (False, libevdev.EV_KEY.BTN_TOOL_RUBBER, None)
> +    PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_BUTTON = (
> +        False,
> +        libevdev.EV_KEY.BTN_TOOL_RUBBER,
> +        libevdev.EV_KEY.BTN_STYLUS,
> +    )
> +    PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_SECOND_BUTTON = (
> +        False,
> +        libevdev.EV_KEY.BTN_TOOL_RUBBER,
> +        libevdev.EV_KEY.BTN_STYLUS2,
> +    )
> +    PEN_IS_ERASING = (True, libevdev.EV_KEY.BTN_TOOL_RUBBER, None)
> +    PEN_IS_ERASING_WITH_BUTTON = (
> +        True,
> +        libevdev.EV_KEY.BTN_TOOL_RUBBER,
> +        libevdev.EV_KEY.BTN_STYLUS,
> +    )
> +    PEN_IS_ERASING_WITH_SECOND_BUTTON = (
> +        True,
> +        libevdev.EV_KEY.BTN_TOOL_RUBBER,
> +        libevdev.EV_KEY.BTN_STYLUS2,
> +    )
> +
> +    def __init__(self, touch, tool, button):
>          self.touch = touch
>          self.tool = tool
> +        self.button = button
>  
>      @classmethod
>      def from_evdev(cls, evdev) -> "PenState":
>          touch = bool(evdev.value[libevdev.EV_KEY.BTN_TOUCH])
>          tool = None
> +        button = None
>          if (
>              evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER]
>              and not evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN]
> @@ -53,7 +97,17 @@ class PenState(Enum):
>          ):
>              raise ValueError("2 tools are not allowed")
>  
> -        return cls((touch, tool))
> +        # we take only the highest button in account
> +        for b in [libevdev.EV_KEY.BTN_STYLUS, libevdev.EV_KEY.BTN_STYLUS2]:
> +            if bool(evdev.value[b]):
> +                button = b
> +
> +        # the kernel tends to insert an EV_SYN once removing the tool, so
> +        # the button will be released after
> +        if tool is None:
> +            button = None
> +
> +        return cls((touch, tool, button))
>  
>      def apply(self, events) -> "PenState":
>          if libevdev.EV_SYN.SYN_REPORT in events:
> @@ -62,6 +116,8 @@ class PenState(Enum):
>          touch_found = False
>          tool = self.tool
>          tool_found = False
> +        button = self.button
> +        button_found = False
>  
>          for ev in events:
>              if ev == libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH):
> @@ -76,12 +132,22 @@ class PenState(Enum):
>                  if tool_found:
>                      raise ValueError(f"duplicated BTN_TOOL_* in {events}")
>                  tool_found = True
> -                if ev.value:
> -                    tool = ev.code
> -                else:
> -                    tool = None
> +                tool = ev.code if ev.value else None
> +            elif ev in (
> +                libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS),
> +                libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS2),
> +            ):
> +                if button_found:
> +                    raise ValueError(f"duplicated BTN_STYLUS* in {events}")
> +                button_found = True
> +                button = ev.code if ev.value else None
>  
> -        new_state = PenState((touch, tool))
> +        # the kernel tends to insert an EV_SYN once removing the tool, so
> +        # the button will be released after
> +        if tool is None:
> +            button = None
> +
> +        new_state = PenState((touch, tool, button))
>          assert (
>              new_state in self.valid_transitions()
>          ), f"moving from {self} to {new_state} is forbidden"
> @@ -97,14 +163,20 @@ class PenState(Enum):
>              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,
>              )
> @@ -112,6 +184,8 @@ class PenState(Enum):
>          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,
>                  PenState.PEN_IS_OUT_OF_RANGE,
>              )
> @@ -130,6 +204,38 @@ class PenState(Enum):
>                  PenState.PEN_IS_OUT_OF_RANGE,
>              )
>  
> +        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,
> +                PenState.PEN_IS_OUT_OF_RANGE,
> +            )
> +
> +        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,
> +                PenState.PEN_IS_OUT_OF_RANGE,
> +            )
> +
>          return tuple()
>  
>      @staticmethod
> @@ -364,26 +470,64 @@ class PenDigitizer(base.UHIDTestDevice):
>              pen.xtilt = 0
>              pen.ytilt = 0
>              pen.twist = 0
> +            pen.barrelswitch = False
> +            pen.secondarybarrelswitch = False
>          elif state == PenState.PEN_IS_IN_RANGE:
>              pen.tipswitch = False
>              pen.inrange = True
>              pen.invert = False
>              pen.eraser = False
> +            pen.barrelswitch = False
> +            pen.secondarybarrelswitch = False
>          elif state == PenState.PEN_IS_IN_CONTACT:
>              pen.tipswitch = True
>              pen.inrange = True
>              pen.invert = False
>              pen.eraser = False
> +            pen.barrelswitch = False
> +            pen.secondarybarrelswitch = False
> +        elif state == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
> +            pen.tipswitch = False
> +            pen.inrange = True
> +            pen.invert = False
> +            pen.eraser = False
> +            pen.barrelswitch = True
> +            pen.secondarybarrelswitch = False
> +        elif state == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
> +            pen.tipswitch = True
> +            pen.inrange = True
> +            pen.invert = False
> +            pen.eraser = False
> +            pen.barrelswitch = True
> +            pen.secondarybarrelswitch = False
> +        elif state == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
> +            pen.tipswitch = False
> +            pen.inrange = True
> +            pen.invert = False
> +            pen.eraser = False
> +            pen.barrelswitch = False
> +            pen.secondarybarrelswitch = True
> +        elif state == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
> +            pen.tipswitch = True
> +            pen.inrange = True
> +            pen.invert = False
> +            pen.eraser = False
> +            pen.barrelswitch = False
> +            pen.secondarybarrelswitch = True
>          elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
>              pen.tipswitch = False
>              pen.inrange = True
>              pen.invert = True
>              pen.eraser = False
> +            pen.barrelswitch = False
> +            pen.secondarybarrelswitch = False
>          elif state == PenState.PEN_IS_ERASING:
>              pen.tipswitch = False
>              pen.inrange = True
>              pen.invert = False
>              pen.eraser = True
> +            pen.barrelswitch = False
> +            pen.secondarybarrelswitch = False
>  
>          pen.current_state = state
>  
> 
> -- 
> 2.41.0
>