[PATCH synaptics v2 14/21] Replace the motion estimator

[Daniel Kurtz]

On Wed, Jun 15, 2011 at 1:06 AM, Daniel Stone <daniel at fooishbar.org> wrote:
> From: Derek Foreman <derek.foreman at collabora.co.uk>
>
> Use a smarter motion estimator that attempts to draw a best-fit line
> through the history where possible, including taking acceleration into
> account.
>
> Signed-off-by: Derek Foreman <derek.foreman at collabora.co.uk>
> Reviewed-by: Daniel Stone <daniel at fooishbar.org>
> Reviewed-by: Simon Thum <simon.thum at gmx.de>
> ---
>  src/synaptics.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 52 insertions(+), 4 deletions(-)
>
> v2: Split a few hunks out into separate patches.
>
> NOTE: I realise 'outlier' is unused; I've already fixed this in my local tree.
>
> diff --git a/src/synaptics.c b/src/synaptics.c
> index 23751f4..cfd1384 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -1803,6 +1803,54 @@ get_edge_speed(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>     }
>  }
>

First of all, in the Synaptics case, I believe the touchpad firmware
is generating samples at a very consistent 80 Hz rate.
The reporting rate is only variable due to the way the firmware does
multitouch reporting with its "Advanced Gesture Mode".
Therefore, in the single-touch case, this math mostly just adds in
jitter from PS/2 bus contention and kernel processing latencies.
In that respect, the previous fixed-time approach may have actually
been more accurate.
There may be simpler ways to compensate for AGM-induced variable
timing...  but, that would be a different patch set.


As for the implementation below, I'm a little confused, since it
appears to have two different behaviors depending on
count_packet_finger:
(1) If == 1:  Return actual delta for last sample time, using current position.
(2) If > 1: Return estimated delta for last sample time, ignoring
current position.

Why do you use the current position in the == 1 case, but ignore it in
the > 1 case?
I believe the original estimate_delta did, in fact use the current
position (hw->x,y).  However, I seem to recall that this was a source
of some error, since I believe this position is pre-scaling (whereas
the positions stored in the HISTORY buffer are post-scaling), and the
delta estimate was taken before it was determined that this new
position was really part of the same gesture.

> +/*
> + * Fit a line through the three most recent points in the motion
> + * history and return relative co-ordinates.

Maybe:
  (1) compute the average velocity over the past N positions (for N up
to 3) [including the current hw position?]
  (2) given this average velocity, return the expected position delta
over the most recent sample time:
       delta = (HIST(0).millis - hw->millis) * velocity_estimate;

> + */
> +static void regress(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
> +                    double *dx, double *dy)
> +{
> +    const SynapticsParameters *pars = &priv->synpara;
> +    int i, j;
> +    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;

Not used?

> +
> +    if (priv->count_packet_finger == 1) {
> +       *dx = hw->x - HIST(0).x;
> +       *dy = hw->y - HIST(0).y;
> +       return;
> +    }
> +
> +    /* Determine the best fit line through the 3 most recent history entries */
> +    for (i = 0; i < MIN(priv->count_packet_finger, 3); i++) {

It might help readability to declare:
int packet_count = MIN(priv->count_packet_finger, 3);

> +       ym += HIST(i).y;
> +       xm += HIST(i).x;
> +       tm += HIST_DELTA(i, 0, millis);
> +    }
> +    ym /= MIN(priv->count_packet_finger, 3);
> +    tm /= MIN(priv->count_packet_finger, 3);
> +    xm /= MIN(priv->count_packet_finger, 3);
> +
> +    for (i = 0; i < MIN(priv->count_packet_finger, 3); i++) {
> +       double t = HIST_DELTA(i, 0, millis);
> +       yb1n += (t - tm) * (HIST(i).y - ym);
> +       xb1n += (t - tm) * (HIST(i).x - xm);
> +       b1d += (t - tm) * (t - tm);
> +    }
> +    xb1 = xb1n/b1d;
> +    yb1 = yb1n/b1d;
> +
> +    *dx = -xb1 * (HIST(0).millis - hw->millis);
> +    *dy = -yb1 * (HIST(0).millis - hw->millis);
> +    return;
> +
> +filtered:

There's no goto...?

> +    *dx = 0;
> +    *dy = 0;
> +    priv->count_packet_finger = 0;
> +}
> +
>  static void
>  get_delta(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>           edge_type edge, double *dx, double *dy)
> @@ -1813,10 +1861,10 @@ get_delta(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>     double tmpf;
>     int x_edge_speed = 0;
>     int y_edge_speed = 0;
> +    Bool outlier = FALSE;
>
> -    /* HIST is full enough: priv->count_packet_finger > 3 */
> -    *dx = estimate_delta(hw->x, HIST(0).x, HIST(1).x, HIST(2).x);
> -    *dy = estimate_delta(hw->y, HIST(0).y, HIST(1).y, HIST(2).y);
> +    /* regress() performs the actual motion prediction. */
> +    regress(priv, hw, dx, dy);
>
>     if ((priv->tap_state == TS_DRAG) || para->edge_motion_use_always)
>         get_edge_speed(priv, hw, edge, &x_edge_speed, &y_edge_speed);
> @@ -1877,7 +1925,7 @@ ComputeDeltas(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>      * POLL_MS declaration. */
>     delay = MIN(delay, POLL_MS);
>
> -    if (priv->count_packet_finger <= 3) /* min. 3 packets, see get_delta() */
> +    if (priv->count_packet_finger < 1) /* min. 1 packet, see regress() */
>         goto out; /* skip the lot */
>
>     if (priv->moving_state == MS_TRACKSTICK)
> --
> 1.7.5.3
>
> _______________________________________________
> 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
>