Re: [PATCH v2 2/3] input: appletouch: use better cursor movement smoothing algorithm

[Henrik Rydberg <rydberg@euromail.se>]

Hi Clinton,

> Use smoothed version of sensor array data to calculate movement and add weight
to prior values when calculating average. This gives more granular and more
predictable movement.
> 
> Signed-off-by: Clinton Sprain <clintonsprain@gmail.com>
> ---
>  drivers/input/mouse/appletouch.c |   60 ++++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 18 deletions(-)

I like this patch, but there are some technical glitches, comments below.

> diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
> index d48181b..edbdd95 100644
> --- a/drivers/input/mouse/appletouch.c
> +++ b/drivers/input/mouse/appletouch.c
> @@ -338,21 +338,51 @@ static void atp_reinit(struct work_struct *work)
>  static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
>  			     int *z, int *fingers)
>  {
> -	int i;
> +	int i, k;
> +	int smooth[nb_sensors + 2];
> +	int smooth_tmp[nb_sensors + 2];
> +
>  	/* values to calculate mean */
>  	int pcum = 0, psum = 0;
>  	int is_increasing = 0;
>  
>  	*fingers = 0;
>  
> +	/*
> +	 * Use a smoothed version of sensor data for movement calculations, to
> +	 * combat noise without needing to toss out values based on a threshold.
> +	 * This improves tracking. Finger count is calculated separately based
> +	 * on raw data.
> +	 */
> +
> +	for (i = 0; i < nb_sensors; i++) {           /* Scale up to minimize */
> +		smooth[i] = xy_sensors[i] << 12;     /* rounding/truncation. */
> +	}
> +
> +	/* Mitigate some of the data loss from smoothing on the edge sensors. */
> +	smooth[-1] = smooth[0] >> 2;
> +	smooth[nb_sensors] = smooth[nb_sensors - 1] >> 2;

Out of bounds array... also, the boundary condition seems odd. If you want to
extrapolate the edge velocity, and assuming smooth[0] is the first sensor, you
would get something like (N = nb_sensors)

	smooth_tmp[0] = 2 * smooth[1] - smooth[2];
	smooth_tmp[N-1] = 2 * smooth[N-2] - smooth[N-3];

> +	for (k = 0; k < 4; k++) {
> +		for (i = 0; i < nb_sensors; i++) {   /* Blend with neighbors. */

And here would be for (i = 1; i < nb_sensors - 1; i++)

> +			smooth_tmp[i] = (smooth[i - 1] + smooth[i] * 2 + smooth[i + 1]) >> 2;
> +		}
> +
> +		for (i = 0; i < nb_sensors; i++) {
> +			smooth[i] = smooth_tmp[i];
> +			if (k == 3)     /* Last go-round, so scale back down. */
> +				smooth[i] = smooth[i] >> 12;

The scale-up is done separately, so why not make the scale-down separately too?

> +		}
> +
> +		smooth[-1] = smooth[0] >> 2;
> +		smooth[nb_sensors] = smooth[nb_sensors - 1] >> 2;

And these would be dropped.

> +	}
> +
>  	for (i = 0; i < nb_sensors; i++) {
>  		if (xy_sensors[i] < threshold) {
>  			if (is_increasing)
>  				is_increasing = 0;
>  
> -			continue;
> -		}
> -
>  		/*
>  		 * Makes the finger detection more versatile.  For example,
>  		 * two fingers with no gap will be detected.  Also, my
> @@ -367,7 +397,8 @@ static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
>  		 *
>  		 * - Jason Parekh <jasonparekh@gmail.com>
>  		 */
> -		if (i < 1 ||
> +
> +		} else if (i < 1 ||
>  		    (!is_increasing && xy_sensors[i - 1] < xy_sensors[i])) {
>  			(*fingers)++;
>  			is_increasing = 1;
> @@ -375,15 +406,8 @@ static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
>  			is_increasing = 0;
>  		}
>  
> -		/*
> -		 * Subtracts threshold so a high sensor that just passes the
> -		 * threshold won't skew the calculated absolute coordinate.
> -		 * Fixes an issue where slowly moving the mouse would
> -		 * occasionally jump a number of pixels (slowly moving the
> -		 * finger makes this issue most apparent.)
> -		 */
> -		pcum += (xy_sensors[i] - threshold) * i;
> -		psum += (xy_sensors[i] - threshold);
> +		pcum += (smooth[i]) * i;
> +		psum += (smooth[i]);

Great, this makes so much more sense.

>  	}
>  
>  	if (psum > 0) {
> @@ -565,8 +589,8 @@ static void atp_complete_geyser_1_2(struct urb *urb)
>  
>  	if (x && y) {
>  		if (dev->x_old != -1) {
> -			x = (dev->x_old * 3 + x) >> 2;
> -			y = (dev->y_old * 3 + y) >> 2;
> +			x = (dev->x_old * 7 + x) >> 3;
> +			y = (dev->y_old * 7 + y) >> 3;
>  			dev->x_old = x;
>  			dev->y_old = y;
>  
> @@ -677,8 +701,8 @@ static void atp_complete_geyser_3_4(struct urb *urb)
>  
>  	if (x && y) {
>  		if (dev->x_old != -1) {
> -			x = (dev->x_old * 3 + x) >> 2;
> -			y = (dev->y_old * 3 + y) >> 2;
> +			x = (dev->x_old * 7 + x) >> 3;
> +			y = (dev->y_old * 7 + y) >> 3;
>  			dev->x_old = x;
>  			dev->y_old = y;

Would you say that the cursor slow-down here is noticeable, or are there enough
samples per second that it does not matter?

Thanks,
Henrik