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

[Derek Foreman]

On 06/15/11 05:12, Daniel Kurtz wrote:
> 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.

Do you have reason to believe that PS/2 bus contention and kernel 
processing latencies will add up to more than a few microseconds?  I'm 
not convinced there will be enough timing jitter to cause perceptible 
errant motion.

This driver also handles non-synaptics devices, like the USB based ones 
in some Apple laptops.  I think they report at a different rate (8ms 
comes to mind.).

The kernel input layer "defuzz" code also filters events out completely 
from time to time, and while I'm currently proposing a slight change to 
those semantics on the linux-input list, they're the parameters we have 
to work with.

>
> 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?

That's because during our initial testing you raised the concern that 
"really short" interactions with the pad could be lost waiting for the 
motion history to fill up.  We tried passing up the first delta without 
passing it through the regression routine, and it subjectively felt very 
responsive, even on a synaptics AGM device in its 40Hz reporting mode.

In the > 1 case we only ever output predicted motion.  This is so (when 
a subsequent patch adds an "error filter") we can test if the current 
hardware input deviates radically from the regression line, and not 
generate motion based on that.

> 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;

That's another potentially valid approach.  It loses the ability to 
filter based on a maximum deviation from the predicted line, and I'm not 
sure it will track an accelerating finger as accurately as the 
linear-regression approach.

It's hard to guess how that would feel without anyone having written it 
though.  Have you written and tested an estimator based on that code?

>> + */
>> +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?

Oops, that should have been moved to a later patch that adds filtering.

>> +
>> +    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);

Agreed.

>> +       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...?

An artifact from the same rebase as above, will also be corrected in the 
v3 patch set.

Thanks,
Derek