[PATCH synaptics] Add hysteresis-based noise reduction
Simon Thum
simon.thum at gmx.de
Tue Feb 8 03:00:43 PST 2011
On 02/08/2011 01:23 AM, Peter Hutterer wrote:
> 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.
Sure.
>
>> 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.
>
>> {
>> 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?
>
>> +
>> 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."
>
>> + */
>> +static int hysteresis(int in, int* center, int margin) {
>> + int diff = in - *center;
>> + if (abs(diff) < margin)
>
> shouldn't this be '<=' ?
I want diff to be meaningful in the margin == 0 case (no hysteresis).
Also a "general enough" thingy.
>
>> + 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.
I like that style, but for debugging's sake I'll reshuffle.
>
>> +
>> 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
Yeah, it's leftovers from the first approach. I'll separate them.
>
>> 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.
Ah, I assumed the inactive area is a sort of HW bug we need to ignore.
I'll move it up.
>
>> }
>> /* 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...
>
> 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
>>
>
More information about the xorg-devel
mailing list