[PATCH v3 15/23] Add four new motion filters

Peter Hutterer peter.hutterer at who-t.net
Wed Aug 24 17:48:38 PDT 2011


On Thu, Jun 23, 2011 at 11:12:50PM +0100, Daniel Stone wrote:
> From: Derek Foreman <derek.foreman at collabora.co.uk>
> 
> Attempt to decrease the possibility of errant motion as much as possible
> by adding three new configurable filters, disabled by default:
>     - Synaptics Max Jerk: maximum change in acceleration before the
>       packet is declared errant (xorg.conf MaxJerk)
>     - Synaptics Max Accel: maximum acceleration the pointer can have
>       before being declared errant (xorg.conf MaxAccel)
>     - Synaptics Max Error: maximum error from a best-fit prediction of
>       the next position before being declared errant (xorg.conf MaxError)
>     - Synaptics Max Distance: maximum distance a finger can move in a
>       single report before being declared errant (xorg.conf MaxDistance)
> 
> Signed-off-by: Derek Foreman <derek.foreman at collabora.co.uk>
> Reviewed-by: Daniel Stone <daniel at fooishbar.org>
> ---
>  include/synaptics-properties.h |   12 ++++++
>  man/synaptics.man              |   23 ++++++++++++
>  src/properties.c               |   46 +++++++++++++++++++++++-
>  src/synaptics.c                |   79 +++++++++++++++++++++++++++++++++++++++-
>  src/synapticsstr.h             |    6 +++
>  5 files changed, 164 insertions(+), 2 deletions(-)
> 
> v3: Hunks which belonged in this patch moved here from regress() patch.
> 
> diff --git a/include/synaptics-properties.h b/include/synaptics-properties.h
> index c550cef..89fa7b6 100644
> --- a/include/synaptics-properties.h
> +++ b/include/synaptics-properties.h
> @@ -158,4 +158,16 @@
>  /* 32 Bit Integer, 2 values, horizontal hysteresis, vertical hysteresis */
>  #define SYNAPTICS_PROP_NOISE_CANCELLATION "Synaptics Noise Cancellation"
>  
> +/* FLOAT */
> +#define SYNAPTICS_PROP_MAX_DISTANCE "Synaptics Max Distance"
> +
> +/* FLOAT */
> +#define SYNAPTICS_PROP_MAX_JERK "Synaptics Max Jerk"
> +
> +/* FLOAT */
> +#define SYNAPTICS_PROP_MAX_ACCEL "Synaptics Max Accel"
> +
> +/* FLOAT */
> +#define SYNAPTICS_PROP_MAX_ERROR "Synaptics Max Error"
> +

please provide one property with 4 values. They seem to be somewhat related
to begin and it would avoid the confusion Simon pointed out.

>  #endif /* _SYNAPTICS_PROPERTIES_H_ */
> diff --git a/man/synaptics.man b/man/synaptics.man
> index cb5f4c6..5b365e6 100644
> --- a/man/synaptics.man
> +++ b/man/synaptics.man
> @@ -510,6 +510,29 @@ AreaBottomEdge option to any integer value other than zero. If supported by the
>  server (version 1.9 and later), the edge may be specified in percent of
>  the total height of the touchpad. Property: "Synaptics Area"
>  .
> +.TP
> +.BI "Option \*qMaxAccel\*q \*q" integer \*q
> +If acceleration increases beyond the specified value, ignore the packet that
> +provoked it, assuming that it is erroneous. Property: "Synaptics Max Accel"

what's the default value? (applies to all 4)

> +.
> +.TP
> +.BI "Option \*qMaxDistance\*q \*q" integer \*q
> +Ignore any single packet containing movement greater than the specified value
> +in pixels, assuming that it is erroneous. Property: "Synaptics Max Distance"

pixels? shouldn't this be device units?
I recommend accepting a percent option here as well (at least for the
xorg.conf) so that one can configure MaxDistance as 10% of the tablet.

> +.
> +.TP
> +.BI "Option \*qMaxError\*q \*q" integer \*q
> +Ignore any single packet containing movement where the error from a best-fit
> +line determining the expected pointer position from historical values is larger
> +than the specified value, assuming that it is erroneous.
> +Property: "Synaptics Max Error"

this needs some more explanation, I think. I'm somewhat struggling
to understand what this means without reading the code.

> +.
> +.TP
> +.BI "Option \*qMaxJerk\*q \*q" integer \*q
> +If the derivative of acceleration increases beyond the specified value, ignore
> +the packet that provoked it, assuming that it is erroneous.
> +Property: "Synaptics Max Jerk"
> +.

this almost certainly needs some examples for users that don't understand
maths as well. a simple "A good value for Max Jerk is blah or boink" would
suffice.

>  
>  .SH CONFIGURATION DETAILS
>  .SS Area handling
> diff --git a/src/properties.c b/src/properties.c
> index 5f11cf2..d371c20 100644
> --- a/src/properties.c
> +++ b/src/properties.c
> @@ -94,6 +94,10 @@ Atom prop_area                  = 0;
>  Atom prop_noise_cancellation    = 0;
>  Atom prop_product_id            = 0;
>  Atom prop_device_node           = 0;
> +Atom prop_max_distance          = 0;
> +Atom prop_max_jerk              = 0;
> +Atom prop_max_accel             = 0;
> +Atom prop_max_error             = 0;
>  
>  static Atom
>  InitTypedAtom(DeviceIntPtr dev, char *name, Atom type, int format, int nvalues,
> @@ -319,6 +323,17 @@ InitDeviceProperties(InputInfoPtr pInfo)
>          XISetDevicePropertyDeletable(pInfo->dev, prop_device_node, FALSE);
>      }
>  
> +    fvalues[0] = para->max_distance;
> +    prop_max_distance = InitFloatAtom(pInfo->dev, SYNAPTICS_PROP_MAX_DISTANCE, 1, fvalues);
> +
> +    fvalues[0] = para->max_jerk;
> +    prop_max_jerk = InitFloatAtom(pInfo->dev, SYNAPTICS_PROP_MAX_JERK, 1, fvalues);
> +
> +    fvalues[0] = para->max_accel;
> +    prop_max_accel = InitFloatAtom(pInfo->dev, SYNAPTICS_PROP_MAX_ACCEL, 1, fvalues);
> +
> +    fvalues[0] = para->max_error;
> +    prop_max_error = InitFloatAtom(pInfo->dev, SYNAPTICS_PROP_MAX_ERROR, 1, fvalues);
>  }
>  
>  int
> @@ -700,8 +715,37 @@ SetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
>              return BadValue;
>          para->hyst_x = hyst[0];
>          para->hyst_y = hyst[1];
> -    } else if (property == prop_product_id || property == prop_device_node)
> +    } else if (property == prop_product_id || property == prop_device_node) {
>          return BadValue; /* read-only */
> +    } else if (property == prop_max_distance) {
> +        float max_distance;
> +        if (prop->size != 1 || prop->format != 32 || prop->type != float_type)
> +            return BadMatch;
> +
> +        max_distance = *(float*)prop->data;
> +        para->max_distance = max_distance;
> +    } else if (property == prop_max_jerk) {
> +        float max_jerk;
> +        if (prop->size != 1 || prop->format != 32 || prop->type != float_type)
> +            return BadMatch;
> +
> +        max_jerk = *(float*)prop->data;
> +        para->max_jerk = max_jerk;
> +    } else if (property == prop_max_accel) {
> +        float max_accel;
> +        if (prop->size != 1 || prop->format != 32 || prop->type != float_type)
> +            return BadMatch;
> +
> +        max_accel = *(float*)prop->data;
> +        para->max_accel = max_accel;
> +    } else if (property == prop_max_error) {
> +        float max_error;
> +        if (prop->size != 1 || prop->format != 32 || prop->type != float_type)
> +            return BadMatch;
> +
> +        max_error = *(float*)prop->data;
> +        para->max_error = max_error;
> +    }
>  
>      return Success;
>  }
> diff --git a/src/synaptics.c b/src/synaptics.c
> index b7f4b8f..0ce8024 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -584,6 +584,11 @@ static void set_default_parameters(InputInfoPtr pInfo)
>      pars->resolution_horiz = xf86SetIntOption(opts, "HorizResolution", horizResolution);
>      pars->resolution_vert = xf86SetIntOption(opts, "VertResolution", vertResolution);
>  
> +    pars->max_jerk = xf86SetRealOption(opts, "MaxJerk", 0.0);
> +    pars->max_distance = xf86SetRealOption(opts, "MaxDistance", 0.0);

accept percent here too please

> +    pars->max_accel = xf86SetRealOption(opts, "MaxAccel", 0.0);
> +    pars->max_error = xf86SetRealOption(opts, "MaxError", 0.0);

I wonder if this one would make sense as a percent option too?

> +
>      /* Warn about (and fix) incorrectly configured TopEdge/BottomEdge parameters */
>      if (pars->top_edge > pars->bottom_edge) {
>  	int tmp = pars->top_edge;
> @@ -1787,14 +1792,66 @@ get_edge_speed(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>  /*
>   * Fit a line through the three most recent points in the motion
>   * history and return relative co-ordinates.
> + *
> + * Four forms of filtering are present:
> + *      Acceleration - finger accelerating too fast
> + *      Jerk - too great a change in finger acceleration
> + *      Error - finger position deviates too much from predicted position
> + *      Distance - finger has moved too far in a single report

can we please split those four out into helper functions? regress is getting
a bit unwieldly otherwise and they all seem to be nicely contained in a
block each anyway.

Rest looks good though.

Cheers,
  Peter

> + *
> + * If any of these filters are active and are tripped, the motion will
> + * be declared erroneous and discarded, with the motion history being
> + * zapped on the assumption that it is now useless for predicting further
> + * motion.
> + *
> + * Note - The current state is used for filtering, but only its time is used
> + *        in the delta calculation.
>   */
>  static void regress(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>                      double *dx, double *dy, CARD32 start_time)
>  {
> +    SynapticsParameters *pars = &priv->synpara;
>      int i;
>      int packet_count = MIN(priv->count_packet_finger, 3);
>      double ym = 0, xm = 0, tm = 0;
>      double yb1n = 0, xb1n = 0, b1d = 0, xb1, yb1;
> +    double dista, distb, distc, vela, velb, velc, acca, accb, jerk;
> +
> +    /* Check if the position has jumped too far for a single report. */
> +    if (pars->max_distance > 0 &&
> +        hypot(hw->x - HIST(0).x, hw->y - HIST(0).y) > pars->max_distance)
> +        goto filtered;
> +
> +    if (pars->max_accel > 0 || pars->max_jerk > 0) {
> +        /* We need at least three packets to filter on, else we can't
> +         * determine the acceleration, let alone the change in acceleration.
> +         * Discard the motion packets, but we don't want to zap the motion
> +         * history, else we'd never leave this state. */
> +        if (packet_count < 3) {
> +            *dx = 0;
> +            *dy = 0;
> +            return;
> +        }
> +
> +        /* Determine distance and acceleration, and see if acceleration is
> +         * higher than MaxAccel. */
> +        dista = hypot(HIST(0).x - hw->x, HIST(0).y - hw->y);
> +        distb = hypot(HIST_DELTA(1, 0, x), HIST_DELTA(1, 0, y));
> +        distc = hypot(HIST_DELTA(2, 1, x), HIST_DELTA(2, 1, y));
> +        vela = dista / (HIST(0).millis - hw->millis);
> +        velb = distb / HIST_DELTA(1, 0, millis);
> +        velc = distc / HIST_DELTA(2, 1, millis);
> +        acca = (vela - velb) / (HIST(1).millis - hw->millis);
> +        if (pars->max_accel > 0 && fabs(acca) > pars->max_accel)
> +            goto filtered;
> +
> +        /* Determine change in acceleration, and see if it's higher than
> +         * MaxJerk. */
> +        accb = (velc - velb) / HIST_DELTA(2, 0, millis);
> +        jerk = (acca - accb) / (HIST(2).millis - hw->millis);
> +        if (pars->max_jerk > 0 && fabs(jerk) > pars->max_jerk)
> +            goto filtered;
> +    }
>  
>      /* If there's only one packet, we can't really fit a line.  However, we
>       * don't want to lose very short interactions with the pad, so we pass on
> @@ -1834,6 +1891,20 @@ static void regress(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>      xb1 = xb1n/b1d;
>      yb1 = yb1n/b1d;
>  
> +    /* Check if the new position falls too far outside our estimate of where
> +     * it should be, and discard it if so. */
> +    if (pars->max_error > 0) {
> +        double X, Y, e, xb0, yb0;
> +
> +        xb0 = xm - xb1*tm;
> +        yb0 = ym - yb1*tm;
> +        X = xb1 * (hw->millis - HIST(0).millis) + xb0;
> +        Y = yb1 * (hw->millis - HIST(0).millis) + yb0;
> +        e = hypot(hw->x - X, hw->y - Y);
> +        if (e > pars->max_error)
> +            goto filtered;
> +    }
> +
>      /*
>       * Here we use the slope component (b1) of the regression line as a speed
>       * estimate, and calculate how far the contact would have moved between
> @@ -1846,6 +1917,11 @@ static void regress(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>      *dx = -xb1 * (start_time - hw->millis);
>      *dy = -yb1 * (start_time - hw->millis);
>      return;
> +
> +filtered:
> +    *dx = 0;
> +    *dy = 0;
> +    priv->count_packet_finger = 0;
>  }
>  
>  static void
> @@ -1859,7 +1935,8 @@ get_delta(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>      int x_edge_speed = 0;
>      int y_edge_speed = 0;
>  
> -    /* regress() performs the actual motion prediction. */
> +    /* regress() performs the actual motion prediction, as well as filtering
> +     * of erroneous events. */
>      regress(priv, hw, dx, dy, priv->last_motion_millis);
>      priv->last_motion_millis = hw->millis;
>  
> diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> index 42abef5..67ca27b 100644
> --- a/src/synapticsstr.h
> +++ b/src/synapticsstr.h
> @@ -163,6 +163,12 @@ typedef struct _SynapticsParameters
>      Bool tap_and_drag_gesture;		    /* Switches the tap-and-drag gesture on/off */
>      unsigned int resolution_horiz;          /* horizontal resolution of touchpad in units/mm */
>      unsigned int resolution_vert;           /* vertical resolution of touchpad in units/mm */
> +
> +    double max_distance;                    /* maximum distance in a single report for valid input */
> +    double max_jerk;                        /* maximum jerk for valid input */
> +    double max_accel;                       /* maximum acceleration for valid input */
> +    double max_error;                       /* maximum deviation from estimated position for valid input */
> +
>      int area_left_edge, area_right_edge, area_top_edge, area_bottom_edge; /* area coordinates absolute */
>      int hyst_x, hyst_y;                     /* x and y width of hysteresis box */
>  } SynapticsParameters;
> -- 
> 1.7.5.4
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 


More information about the xorg-devel mailing list