[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