[PATCH v2] touchpad: add wobbling detection
Peter Hutterer
peter.hutterer at who-t.net
Wed Feb 21 00:33:21 UTC 2018
On Tue, Feb 20, 2018 at 08:58:33PM +0300, Konstantin Kharlamov wrote:
> On 20.02.2018 17:31, Konstantin Kharlamov wrote:
> > On 20.02.2018 13:44, Konstantin Kharlamov wrote:
> > > On 20.02.2018 09:34, Peter Hutterer wrote:
> > > > On Sun, Feb 18, 2018 at 11:14:55PM +0300, Konstantin Kharlamov wrote:
> > > > > +static inline void
> > > > > +tp_detect_wobbling(struct tp_dispatch *tp, int x_diff, uint64_t time)
> > > > > +{
> > > > > + if (tp->hysteresis.enabled)
> > > > > + return;
> > > > > +
> > > > > + /* Idea: if we got a tuple of *very* quick moves like
> > > > > {Left, Right, Left}, or
> > > > > + * {Right, Left, Right}, it means touchpad jitters
> > > > > since no human supposed to
> > > > > + * be able to move like that within thresholds
> > > > > + *
> > > > > + * Algo: we encode left moves as zeroes, and right as
> > > > > ones. We also drop the
> > > > > + * array to all zeroes when contraints are not
> > > > > satisfied. Then we search for
> > > > > + * the pattern {1,0,1}. It can't match {Left, Right,
> > > > > Left}, but it does match
> > > > > + * {Left, Right, Left, Right}, so it's okay.
> > > > > + */
> > > >
> > > > the interesting bit here will be whether there are touchpads
> > > > that wobble but
> > > > with more than one event per direction. I suspect there are but
> > > > this should
> > > > be simple enough that it catches the first RLR movement (bound
> > > > to happen at
> > > > some point) and enable it then. The main question is whether there is a
> > > > noticeable period of wobbling until this happens.
> > > > I honestly don't know, we won't know until ppl start shouting at us...
> > >
> > > My touchpad definitely does; however it does so many events for
> > > every touch that I feel like it produces every possible combination
> > > within a second or two. The more that the number for the three slots
> > > is 2³ = 8, which isn't much.
> > >
> > > > > + if (time - tp->hysteresis.last_motion_time > ms2us(20)
> > > > > || x_diff == 0) {
> > > > > + tp->hysteresis.x_motion_in_threshold = 0;
> > > > > + return;
> > > > > + }
> > > >
> > > > empty line here please.
> > > >
> > > > > + tp->hysteresis.x_motion_in_threshold <<= 1;
> > > > > + if (x_diff > 0) { /* right move */
> > > >
> > > > s/x_diff/dx/, we use that everywhere else for deltas.
> > > >
> > > > > + tp->hysteresis.x_motion_in_threshold |= 1;
> > > > > + static const char r_l_r = 5; /* {Right, Left, Right} */
> > > >
> > > > declarations at the top of the block please, separated by an
> > > > empty line. See
> > > > CODING_STYLE in the repository.
> > > >
> > > > > + if (tp->hysteresis.x_motion_in_threshold & r_l_r) {
> > > >
> > > > this one will trigger on the first right movement, you need
> > > > if ((tp->hysteresis.x_motion_in_threshold & r_l_r) == r_l_r)
> > > >
> > > > With that change, it seems to work correctly on my non-wobbling touchpad
> > > > here. Does it still work for you when you add this?
> > >
> > > Thank you, nice catch!
> > >
> > > > > + tp->hysteresis.enabled = true;
> > > > > + evdev_log_debug(tp->device, "hysteresis enabled\n");
> > > > > + }
> > > > > + }
> > > > > +}
> > > > > +
> > > > > static inline void
> > > > > tp_motion_hysteresis(struct tp_dispatch *tp,
> > > > > struct tp_touch *t)
> > > > > @@ -1405,6 +1435,8 @@ tp_process_state(struct tp_dispatch
> > > > > *tp, uint64_t time)
> > > > > tp_thumb_detect(tp, t, time);
> > > > > tp_palm_detect(tp, t, time);
> > > > > + tp_detect_wobbling(tp, tp->hysteresis.prev_x -
> > > > > t->point.x, time);
> > > > > + tp->hysteresis.prev_x = t->point.x;
> > > >
> > > > the dx calculation should happen within tp_detect_wobbling, just
> > > > pass the
> > > > touch in and do the subtraction there. Plus that way you can do
> > > > a simple:
> > > > if (tp->prev.x == 0) dx = 0;
> > > > which makes things more obvious.
> > >
> > > Well, not really can, the comparison for zero here would check
> > > coordinate of previous x, whereas I need to check whether the delta
> > > between prev. and curr. positions is zero, e.g. if a finger is still
> > > and FW filters.
I was referring to your comment that it's unclear what value prev_x should
be initialised to. even though it's not needed, it'll make the code a lot
more obvious if you handle the zero case here. so the code would be
something like:
if (tp->prev.x == 0) /* first invocation */
dx = 0;
else
dx = t->point.x - t->hysteresis.prev.x
also, please make prev a struct point, even if we don't use y at this point,
easier to read and better for type-safety, as much as we get that in C.
> > > > > tp_motion_hysteresis(tp, t);
> > > > > tp_motion_history_push(t);
> > > > > @@ -2918,6 +2950,7 @@ tp_init_hysteresis(struct tp_dispatch *tp)
> > > > > tp->hysteresis.margin.x = res_x/2;
> > > > > tp->hysteresis.margin.y = res_y/2;
> > > > > tp->hysteresis.enabled = false;
> > > > > + tp->hysteresis.x_motion_in_threshold = 0;
> > > >
> > > > 'threshold' usually refers to a distance in libinput, here it's more a
> > > > timeout. But calling it x_motion_history would be best here, IMO.
> > > >
> > > > > }
> > > > > static void
> > > > > diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> > > > > index 442f34a3..398a18ed 100644
> > > > > --- a/src/evdev-mt-touchpad.h
> > > > > +++ b/src/evdev-mt-touchpad.h
> > > > > @@ -276,6 +276,8 @@ struct tp_dispatch {
> > > > > struct device_coords margin;
> > > > > unsigned int other_event_count;
> > > > > uint64_t last_motion_time;
> > > > > + char x_motion_in_threshold;
> > > > > + int prev_x;
> > > >
> > > > both of these need to be in the struct tp_touch, otherwise 2-finger
> > > > scrolling may enable the hysteresis when the fingers move
> > > > independently from
> > > > each other but you're adding them to the same bitmask.
> > >
> > > Thanks for you comments!
> > >
> > > I tried addressing them, and found that the result of the time
> > > calculation "time - tp->hysteresis.last_motion_time" whilst my
> > > finger is down is constantly increasing, i.e. like "20, 30, 50, 100,
> > > 120…". It looks as if "tp->hysteresis.last_motion_time" is not the
> > > last motion time, but the last finger-down time.
> > >
> > > Do you happen to know anything about that?
> >
> > Although I did find what's the matter. Turns out, the last_motion_time
> > have been assigned at tp_maybe_disable_hysteresis() which I did remove.
> > But even there it's been guarded by if:
> >
> > if (tp->queued & TOUCHPAD_EVENT_MOTION)
> > tp->hysteresis.last_motion_time = time;
> >
> > What does that condition mean?
>
> Ok, I see, I was mostly confused by "queued" name. It probably means "events
> queued to get processed by libinput".
the evdev protocol is serial, so we get a bunch of separate events, grouped
into a frame by the subsequent EV_SYN SYN_REPORT event. We cannot process
the events until we get this event, so we need to buffer what has happened
and process the state once we have the SYN_REPORT.
the tp->queued is a bitmask of the things that have happened,
TOUCHPAD_EVENT_MOTION simply means either x or y has updated and we need to
move the touch.
> I wrote a code like:
>
> if (tp->queued & TOUCHPAD_EVENT_MOTION) {
> tp_detect_wobbling(tp, t, time);
> tp->hysteresis.last_motion_time = time;
> t->prev_x = t->point.x;
> }
fwiw, this should all move into tp_detect_wobbling(), with an early return
if tp->queued is emtpy.
> It should work like it previously did; what bothers me however — shouldn't
> that be specific to a touch, but not touchpad? Couldn't that get triggered
> e.g. by 2-finger scrolling? (whatever that is, I dunno, touching with 2
> finger doesn't scroll for me — I do scroll instead using the right edge of
> the touchpad).
if you make it per-touch then yes, it can get triggered by 2-finger
scrolling but it'll work per-touch so either touch has to wobble to disable
the hysteresis. we can assume that if one touch wobbles, all of them will.
2-finger scrolling can be enabled with the
libinput_device_config_set_scroll_method() call, or libinput debug-events
--set-scroll-method=twofinger, you should give it a try. edge scrolling is
going the way of the dodo.
Cheers,
Peter
More information about the wayland-devel
mailing list