[PATCH synaptics] Add hysteresis-based noise reduction

[Peter Hutterer]

On Tue, Feb 08, 2011 at 12:00:43PM +0100, Simon Thum wrote:
> >> 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.
> I fully agree. But "default" is a keyword which means even if gcc gulps
> it down others will break.

default_value then? or just leave defVal ;)

> >>  {
> >>      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?
> Yes, but not here - fuzz would be evdev-specific, I guess. Can you
> outline how to add that?

yeah. you'd have to read it from the kernel through the eventcomm interface
and pop it into the default values for the Hysteresis. We already do the
same for min/max, should be easy enough to add that.
for non-evdev, just leave it at -1 and use the defaults as calculated above.
And I'd say the same for a fuzz of 0, because that seems to be the standard
value.

> >> +
> >>      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?
> The thinking was that I'd knock up something which is general enough to
> be used in other drivers (or as a xf86 utility fn). And the delta may
> well be quite interesting depending on what you're doing.
> 
> For this case alone, "yes."

well, it's an internal function at this point and until the libinputdriver
appears (which I've started a few times and given up again before typing
code) might as well keep it simple.

> >>      }
> >>       /* 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)
> I had it that way and then noticed it'd be the only line around...

heh. I only saw the patch context and the resolution_horiz are the only ones
with spaces. So leave the tabs in for this, we'll fix up the others at some
point too.

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