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

[Daniel Kurtz]

On Thu, Jun 16, 2011 at 12:45 AM, Derek Foreman <derek at collabora.co.uk> wrote:
> 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.

Yes, on some hardware the latencies can add up to several milliseconds.
Especially when there is keyboard contention on PS/2 (like shift/ctrl + drag).

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

In this case, does userspace not even get the EV_SYN?

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

This sounds reasonable.  Please add a comment to this affect.  It is a
quite subtle implementation detail.

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

Sorry for the confusion, I'm suggesting another way of explaining what
this function actually does, not an alternative implementation.  The
computed "linear regression slope of position vs. time" is the average
velocity.

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



-- 
Daniel Kurtz
Software Engineer, Chrome OS
Google, Inc.
Taipei, Taiwan
djkurtz at google.com