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.
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". 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;
}
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).
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
