[PATCH synaptics] Limit the movement to 20 mm per event
Peter Hutterer
peter.hutterer at who-t.net
Mon Sep 15 20:29:26 PDT 2014
On Tue, Sep 16, 2014 at 08:57:56AM +0600, Alexander E. Patrakov wrote:
> 16.09.2014 07:08, Peter Hutterer wrote:
> >Touchpads are limited by a fixed sampling rate (usually 80Hz). Some finger
> >changes may happen too fast for this sampling rate, resulting in two distinct
> >event sequences:
> >* finger 1 up and finger 2 down in the same EV_SYN frame. Synaptics sees one
> > finger down before and after and the changed coordinates
> >* finger 1 up and finger 2 down _between_ two EV_SYN frames. Synaptics sees one
> > touchpoint move from f1 position to f2 position.
> >
> >That move causes a large cursor jump. The former could be solved (with
> >difficulty) by adding fake EV_SYN handling after releasing touchpoints but
> >that won't fix the latter case.
>
> Hello.
>
> Thanks for recognizing the problem and opting for a simple criterion for
> detecting such jumps.
yeah, sorry about the delay. I needed poking from multiple sides (and
freeing up of some time) to finally get to this issue.
> However, AFAICS, the patch just discards the motion without telling the FSM
> that it is in fact a new touch. So it may be bad for gestures.
>
> Attached is a patch that I use to work around this issue since July that
> does attempt to tell the FSM about a different finger.
Thanks, I haven't yet started on the libinput patch yet anyway, so I'll take
yours as a baseline.
> >So as a solution for now limit the finger movement to 20mm per event.
> >Tests on a T440 and an x220 showed that this is just above what a reasonable
> >finger movement would trigger. If a movement is greater than that limit, reset
> >it to 0/0.
> >
> >On devices without resolution, use 0.25 the touchpad's diagonal instead.
>
> I have no objection to the resolution-aware threshold. My patch doesn't have
> it, but you can obviously replace the heuristic there with your version.
IMO the resolution-aware threshold is needed. e.g. the x220 has different
vert/horiz resolutions, if you just go by propotion you get different
thresholds depending on the position of the two fingers. That can be
quite confusing, though it's hard to guess how often that would be
triggered.
Cheers,
Peter
>
> >
> >Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >---
> >See the git repo below for a simple script to verify valid movement deltas
> >on your device if you feel like it:
> >https://github.com/whot/input-data-analysis/tree/master/touchpad-max-delta
> >Max I got was 9mm on the x220 and 17mm on the T440s but the latter was
> >really ripping the cursor around the screen beyond what I'd deem useful.
> >
> > src/synaptics.c | 33 +++++++++++++++++++++++++++++++++
> > src/synapticsstr.h | 2 ++
> > 2 files changed, 35 insertions(+)
> >
> >diff --git a/src/synaptics.c b/src/synaptics.c
> >index 2e3ad0c..756751d 100644
> >--- a/src/synaptics.c
> >+++ b/src/synaptics.c
> >@@ -780,6 +780,23 @@ set_default_parameters(InputInfoPtr pInfo)
> > pars->resolution_vert = 1;
> > }
> >
> >+ /* Touchpad sampling rate is too low to detect all movements.
> >+ A user may lift one finger and put another one down within the same
> >+ EV_SYN or even between samplings so the driver doesn't notice at all.
> >+
> >+ We limit the movement to 20 mm within one event, that is more than
> >+ recordings showed is needed (17mm on a T440).
> >+ */
> >+ if (pars->resolution_horiz > 1 &&
> >+ pars->resolution_vert > 1)
> >+ pars->maxDeltaMM = 20;
> >+ else {
> >+ /* on devices without resolution set the vector length to 0.25 of
> >+ the touchpad diagonal */
> >+ pars->maxDeltaMM = diag * 0.25;
> >+ }
> >+
> >+
> > /* Warn about (and fix) incorrectly configured TopEdge/BottomEdge parameters */
> > if (pars->top_edge > pars->bottom_edge) {
> > int tmp = pars->top_edge;
> >@@ -2229,6 +2246,13 @@ get_delta(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
> > *dy = integral;
> > }
> >
> >+/* Vector length, but not sqrt'ed, we only need it for comparison */
> >+static inline double
> >+vlenpow2(double x, double y)
> >+{
> >+ return x * x + y * y;
> >+}
> >+
> > /**
> > * Compute relative motion ('deltas') including edge motion.
> > */
> >@@ -2238,6 +2262,7 @@ ComputeDeltas(SynapticsPrivate * priv, const struct SynapticsHwState *hw,
> > {
> > enum MovingState moving_state;
> > double dx, dy;
> >+ double vlen;
> > int delay = 1000000000;
> >
> > dx = dy = 0;
> >@@ -2283,6 +2308,14 @@ ComputeDeltas(SynapticsPrivate * priv, const struct SynapticsHwState *hw,
> > out:
> > priv->prevFingers = hw->numFingers;
> >
> >+ vlen = vlenpow2(dx/priv->synpara.resolution_horiz,
> >+ dy/priv->synpara.resolution_vert);
> >+
> >+ if (vlen > priv->synpara.maxDeltaMM * priv->synpara.maxDeltaMM) {
> >+ dx = 0;
> >+ dy = 0;
> >+ }
> >+
> > *dxP = dx;
> > *dyP = dy;
> >
> >diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> >index 4bd32ac..75f52d5 100644
> >--- a/src/synapticsstr.h
> >+++ b/src/synapticsstr.h
> >@@ -226,6 +226,8 @@ typedef struct _SynapticsParameters {
> > int area_left_edge, area_right_edge, area_top_edge, area_bottom_edge; /* area coordinates absolute */
> > int softbutton_areas[4][4]; /* soft button area coordinates, 0 => right, 1 => middle , 2 => secondary right, 3 => secondary middle button */
> > int hyst_x, hyst_y; /* x and y width of hysteresis box */
> >+
> >+ int maxDeltaMM; /* maximum delta movement (vector length) in mm */
> > } SynapticsParameters;
> >
> > struct _SynapticsPrivateRec {
> >
> From 0ffca2abdd53a278e93fc2ef38e223d1a200f2d0 Mon Sep 17 00:00:00 2001
> From: "Alexander E. Patrakov" <patrakov at gmail.com>
> Date: Fri, 28 Mar 2014 23:57:56 +0600
> Subject: [PATCH] Detect and discard huge coordinate jumps on touchpads.
>
> Such jumps are usually consequences of the touchpad firmware
> failing to notice that a different finger is in fact touching the
> touchpad. If not discarded, such events lead to sudden pointer
> jumps into screen corners.
>
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=76722
> Signed-off-by: Alexander E. Patrakov <patrakov at gmail.com>
> ---
> src/evdev-mt-touchpad.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> src/evdev-mt-touchpad.h | 3 +++
> 2 files changed, 50 insertions(+)
>
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index d831b83..1ef40b8 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -31,6 +31,7 @@
>
> #define DEFAULT_ACCEL_NUMERATOR 1200.0
> #define DEFAULT_HYSTERESIS_MARGIN_DENOMINATOR 700.0
> +#define DEFAULT_JUMP_DETECTION_DENOMINATOR 8.0
>
> static inline int
> tp_hysteresis(int in, int center, int margin)
> @@ -160,6 +161,47 @@ tp_end_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> tp->queued |= TOUCHPAD_EVENT_MOTION;
> }
>
> +static inline void
> +tp_disrupt_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> +{
> + if (t->state == TOUCH_NONE)
> + return;
> +
> + tp_end_touch(tp, t, time);
> + t->state = TOUCH_DISRUPT;
> +}
> +
> +static inline void
> +tp_undisrupt_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> +{
> + if (t->state != TOUCH_DISRUPT)
> + return;
> +
> + tp_begin_touch(tp, t, time);
> +}
> +
> +static inline void
> +tp_detect_jumps(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> +{
> + /* Motivation: https://bugs.freedesktop.org/show_bug.cgi?id=76722 */
> +
> + struct tp_motion *oldpos;
> + int dx, dy;
> +
> + if (t->history.count == 0)
> + return;
> +
> + /* This is called before tp_motion_history_push(),
> + so the latest historical datum is at offset 0.
> + */
> + oldpos = tp_motion_history_offset(t, 0);
> + dx = abs(t->x - oldpos->x);
> + dy = abs(t->y - oldpos->y);
> +
> + if (dx > tp->jump_threshold || dy > tp->jump_threshold)
> + tp_disrupt_touch(tp, t, time);
> +}
> +
> static double
> tp_estimate_delta(int x0, int x1, int x2, int x3)
> {
> @@ -423,6 +465,7 @@ tp_process_state(struct tp_dispatch *tp, uint64_t time)
>
> tp_palm_detect(tp, t, time);
>
> + tp_detect_jumps(tp, t, time);
> tp_motion_hysteresis(tp, t);
> tp_motion_history_push(t);
>
> @@ -451,6 +494,8 @@ tp_post_process_state(struct tp_dispatch *tp, uint64_t time)
> if (!t->dirty)
> continue;
>
> + tp_undisrupt_touch(tp, t, time);
> +
> if (t->state == TOUCH_END)
> t->state = TOUCH_NONE;
> else if (t->state == TOUCH_BEGIN)
> @@ -809,6 +854,8 @@ tp_init(struct tp_dispatch *tp,
> tp->hysteresis.margin_y =
> diagonal / DEFAULT_HYSTERESIS_MARGIN_DENOMINATOR;
>
> + tp->jump_threshold = diagonal / DEFAULT_JUMP_DETECTION_DENOMINATOR;
> +
> if (tp_init_scroll(tp) != 0)
> return -1;
>
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 83edf4f..0ee5fda 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -46,6 +46,7 @@ enum touch_state {
> TOUCH_NONE = 0,
> TOUCH_BEGIN,
> TOUCH_UPDATE,
> + TOUCH_DISRUPT,
> TOUCH_END
> };
>
> @@ -167,6 +168,8 @@ struct tp_dispatch {
> int32_t margin_y;
> } hysteresis;
>
> + int32_t jump_threshold;
> +
> struct motion_filter *filter;
>
> struct {
> --
> 2.0.4
>
More information about the xorg-devel
mailing list