[PATCH synaptics] Add hysteresis-based noise reduction

[Peter Hutterer]

On Mon, Feb 07, 2011 at 11:32:27PM +0100, Simon Thum wrote:
> This introduces hysteresis into the driver's processing. It significantly
> reduces noise motion, i.e. now the pad does no longer generate a stream of
> sub-pixel events when just holding the position with the finger down.
> Also, taking off the finger no longer generates additional motion,
> scrolling becomes flicker-free etc.
> 
> Signed-off-by: Simon Thum <simon.thum at gmx.de>
> ---
> 
> What's missing is a complementing property, if the patch is acceptable
> otherwiese I'm willing to add it.

thanks, been meaning to work on this for a while now and never got to it.
ack to the principle, but the patch needs some more work, aside from the
properties.
 
>  man/synaptics.man  |    9 +++++++
>  src/synaptics.c    |   62
> ++++++++++++++++++++++++++++++++++++++++++---------
>  src/synapticsstr.h |    3 ++
>  3 files changed, 63 insertions(+), 11 deletions(-)
> 
> diff --git a/man/synaptics.man b/man/synaptics.man
> index 3f1ca9d..06e4976 100644
> --- a/man/synaptics.man
> +++ b/man/synaptics.man
> @@ -222,6 +222,15 @@ Motion Factor"
>  Greatest setting for pressure motion factor. Property: "Synaptics Pressure
>  Motion Factor"
>  .TP
> +.BI "Option \*qHorizHysteresis\*q \*q" integer \*q
> +The minimum horizontal HW distance required to generate motion events.
> Can be
> +specified as a percentage. Increase if noise motion is a problem for you.
> +Default: 4 percent of the diagonal.
> +.TP
> +.BI "Option \*qVertHysteresis\*q \*q" integer \*q
> +The minimum vertical HW distance required to generate motion events. See
> +\fBHorizHysteresis\fR.
> +.TP
>  .BI "Option \*qUpDownScrolling\*q \*q" boolean \*q
>  If on, the up/down buttons generate button 4/5 events.
>  .

synaptics has quite a nice "configuration details" section in the man page
with more info on the more complicated settings. It'd be nice if you could
add to that describing how the hysteresis works.

> diff --git a/src/synaptics.c b/src/synaptics.c
> index 88bd024..b571d36 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -387,18 +387,19 @@ calculate_edge_widths(SynapticsPrivate *priv, int
> *l, int *r, int *t, int *b)
>   * the log message.
>   */
>  static int set_percent_option(pointer options, const char* optname,
> -                              const int range, const int offset)
> +                              const int range, const int offset,
> +                              const int defVal)

IMO s/defValue/default would be easier to read.

>  {
>      int result;
>  #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 11
> -    int percent = xf86CheckPercentOption(options, optname, -1);
> +    double percent = xf86CheckPercentOption(options, optname, -1);
>  -    if (percent != -1) {
> +    if (percent >= 0.0) {
>          percent = xf86SetPercentOption(options, optname, -1);
>          result = percent/100.0 * range + offset;
>      } else
>  #endif
> -        result = xf86SetIntOption(options, optname, 0);
> +        result = xf86SetIntOption(options, optname, defVal);
>       return result;
>  }
> @@ -427,6 +428,7 @@ static void set_default_parameters(InputInfoPtr pInfo)
>      int horizResolution = 1;
>      int vertResolution = 1;
>      int width, height, diag, range;
> +    int horizHyst, vertHyst;
>       /* read the parameters */
>      if (priv->synshm)
> @@ -458,6 +460,10 @@ static void set_default_parameters(InputInfoPtr pInfo)
>      edgeMotionMaxSpeed = diag * .080;
>      accelFactor = 200.0 / diag; /* trial-and-error */
>  +    /* hysteresis */
> +    horizHyst = diag * 0.005;
> +    vertHyst = diag * 0.005;

this seems at odds with the 4% mentioned in the man page :)
also, the kernel provides us with fuzz for the device, maybe we could use
that as default if present?

> +
>      range = priv->maxp - priv->minp;
>       /* scaling based on defaults and a pressure of 256 */
> @@ -513,10 +519,13 @@ static void set_default_parameters(InputInfoPtr pInfo)
>      pars->top_edge = xf86SetIntOption(opts, "TopEdge", t);
>      pars->bottom_edge = xf86SetIntOption(opts, "BottomEdge", b);
>  -    pars->area_top_edge = set_percent_option(opts, "AreaTopEdge",
> height, priv->miny);
> -    pars->area_bottom_edge = set_percent_option(opts, "AreaBottomEdge",
> height, priv->miny);
> -    pars->area_left_edge = set_percent_option(opts, "AreaLeftEdge",
> width, priv->minx);
> -    pars->area_right_edge = set_percent_option(opts, "AreaRightEdge",
> width, priv->minx);
> +    pars->area_top_edge = set_percent_option(opts, "AreaTopEdge",
> height, priv->miny, 0);
> +    pars->area_bottom_edge = set_percent_option(opts, "AreaBottomEdge",
> height, priv->miny, 0);
> +    pars->area_left_edge = set_percent_option(opts, "AreaLeftEdge",
> width, priv->minx, 0);
> +    pars->area_right_edge = set_percent_option(opts, "AreaRightEdge",
> width, priv->minx, 0);
> +
> +    pars->hyst_x = set_percent_option(opts, "HorizHysteresis", width,
> 0, horizHyst);
> +    pars->hyst_y = set_percent_option(opts, "VertHysteresis", height,
> 0, vertHyst);
>       pars->finger_low = xf86SetIntOption(opts, "FingerLow", fingerLow);
>      pars->finger_high = xf86SetIntOption(opts, "FingerHigh", fingerHigh);
> @@ -1713,6 +1722,26 @@ estimate_delta(double x0, double x1, double x2,
> double x3)
>      return x0 * 0.3 + x1 * 0.1 - x2 * 0.1 - x3 * 0.3;
>  }
>  +/**
> + * applies a hysteresis. center is shifted such that it is in range with
> + * in by the margin again.

please document the parameters and return values.
also, given that we don't use the return value anywhere, why not just return
the shifted centre?

> + */
> +static int hysteresis(int in, int* center, int margin) {
> +    int diff = in - *center;
> +    if (abs(diff) < margin)

shouldn't this be '<=' ?

> +	return 0;
> +
> +    if (diff > margin) {
> +	*center += diff - margin;
> +	return diff - margin;
> +    } else if (diff < -margin) {
> +	*center += diff + margin;
> +	return diff + margin;
> +    } else {
> +	return 0;
> +    }
> +}

please use an extra variable so we only have a single return. much easier to
debug.

> +
>  static int
>  ComputeDeltas(SynapticsPrivate *priv, const struct SynapticsHwState *hw,
>  	      edge_type edge, int *dxP, int *dyP, Bool inside_area)
> @@ -1746,9 +1775,11 @@ ComputeDeltas(SynapticsPrivate *priv, const
> struct SynapticsHwState *hw,
>  	!priv->vert_scroll_edge_on && !priv->horiz_scroll_edge_on &&
>  	!priv->vert_scroll_twofinger_on && !priv->horiz_scroll_twofinger_on &&
>  	!priv->circ_scroll_on && priv->prevFingers == hw->numFingers) {
> -	/* FIXME: Wtf?? what's with 13? */
> -	delay = MIN(delay, 13);
> -	if (priv->count_packet_finger > 3) { /* min. 3 packets */
> +	/* to create fluid edge motion, call back 'soon'
> +	 * even in the absence of new hardware events */
> +	delay = 13;

shouldn't this part be in a separate patch?

> +
> +	if (priv->count_packet_finger > 3) { /* min. 3 packets, see below */
>  	    double tmpf;
>  	    int x_edge_speed = 0;
>  	    int y_edge_speed = 0;
> @@ -1761,6 +1792,7 @@ ComputeDeltas(SynapticsPrivate *priv, const struct
> SynapticsHwState *hw,
>  		dx = dx * dtime * para->trackstick_speed;
>  		dy = dy * dtime * para->trackstick_speed;
>  	    } else if (moving_state == MS_TOUCHPAD_RELATIVE) {
> +		/* HIST is full enough: priv->count_packet_finger > 3 */

same with this one

>  		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);
>  @@ -2384,6 +2416,14 @@ HandleState(InputInfoPtr pInfo, struct
> SynapticsHwState *hw)
>  	edge = NO_EDGE;
>   	dx = dy = 0;
> +    } else {
> +	/* apply hysteresis before doing anything serious. This cancels
> +	 * out a lot of noise which might surface in strange phenomena
> +	 * like flicker in scrolling or noise motion. */
> +	hysteresis(hw->x, &priv->hyst_center_x, para->hyst_x);
> +	hysteresis(hw->y, &priv->hyst_center_y, para->hyst_y);
> +	hw->x = priv->hyst_center_x;
> +	hw->y = priv->hyst_center_y;

wouldn't you need to apply this before testing for the active area? the
active area has some side-effects, so switching in/out because of fuzzing
is suboptimal.

>      }
>       /* these two just update hw->left, right, etc. */
> diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> index 9ad8638..9ee3b7b 100644
> --- a/src/synapticsstr.h
> +++ b/src/synapticsstr.h
> @@ -160,6 +160,7 @@ typedef struct _SynapticsParameters
>      unsigned int resolution_horiz;          /* horizontal resolution of
> touchpad in units/mm */
>      unsigned int resolution_vert;           /* vertical resolution of
> touchpad in units/mm */
>      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 */

spaces instead of tabs here (yeah, I know the code is mixed)

Cheers,
  Peter

>  } SynapticsParameters;
>   @@ -183,6 +184,8 @@ typedef struct _SynapticsPrivateRec
>      Bool absolute_events;               /* post absolute motion events
> instead of relative */
>      SynapticsMoveHistRec move_hist[SYNAPTICS_MOVE_HISTORY]; /* movement
> history */
>      int hist_index;			/* Last added entry in move_hist[] */
> +    int hyst_center_x;			/* center x of hysteresis*/
> +    int hyst_center_y;			/* center y of hysteresis*/
>      int scroll_y;			/* last y-scroll position */
>      int scroll_x;			/* last x-scroll position */
>      double scroll_a;			/* last angle-scroll position */
> -- 
> 1.7.3.4
>