[PATCH RFC libinput] filter: force touchpads to a fixed report rate
Hans de Goede
hdegoede at redhat.com
Mon Jul 10 18:40:42 UTC 2017
Hi,
On 10-07-17 07:11, Peter Hutterer wrote:
> From: Hans de Goede <hdegoede at redhat.com>
>
> Some devices, specifically some bluetooth touchpads generate quite
> unreliable timestamps for their events. The problem seems to be that
> (some of) these touchpads sample at aprox 90 Hz, but the bluetooth stack
> only communicates about every 30 ms (*) and then sends mutiple HID input
> reports in one batch.
>
> This results in 2-4 packets / SYNs every 30 ms. With timestamps really
> close together. The finger coordinate deltas in these packets change by
> aprox. the same amount between each packet when moving a finger at
> constant speed. But the time deltas are e.g. 28 ms, 1 ms, 1 ms resulting
> in calculate_tracker_velocity returning vastly different speeds for the
> 1st and 2nd packet, which in turn results in very "jerky" mouse pointer
> movement.
>
> *) Maybe it is waiting for a transmit time slot or some such.
>
> This commit adds support for a fixed report rate on touchpads. We take the
> assumption that a device with a fixed report rate will send events at that
> rate but those events may be delayed by the kernel or accelerated following
> some previous delay. Any timestamp that falls within the acceptable range (1.5
> times the frequency) is set to the fixed report rate instead.
>
> This does not affect the event timestamps, it only applies to pointer
> acceleration.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> This replaces Hans' two patches in this thread.
> Changes to Hans' patch:
> - both patches squashed together
> - instead of smoothing threshold/value assume a fixed report rate
> - save the fixed timestamp in the trackers
>
> Hans, please give this one a try. btw, what touchpad are we talking about
> here?
This is the touchpad and the Bluetooth keyboard dock of an Asus T100CHI
(with hid-asus patches which are pending upstream to put it in mt mode)
> It's the same principle, but instead of smoothing I figured we should just
> go all the way and encode the report rate, assume that's what the device
> will produce even if the kernel or some stack adds delays and adjust the
> timestamps for that report rate. There's a fixme that needs to be addressed still
> and obviously the 82Hz value measured on my device won't work for everyone
> so it needs to move into a udev property.
I don't think there is much difference between our 2 techniques, as long as
your code hits the:
> + if (tdelta < accel->report_rate_tdelta * 1.5)
> + time = last_time + tdelta;
"time = last_time + tdelta;" path the result is the same. The only
difference is that your code will miss that path sometimes when
time stamps drift, which is unavoidable.
> But please give it try and let me know what you think
I've just given this a try and sorry, but it is no good. The
cursor goes from smooth with my original patch to smooth - jump -
smooth again, doing the jump thingie about 1-2 times a second.
It is slightly better then not having any patch at all, but
not much.
I really think my approach of simply assuming a fixed delta-t
when calculating the speed as long as the measured delta-t is
within certain bounds is better as it does not suffer from the
time stamp drift issues your solution does.
As for simply enabling this for all touchpads, that is something
I considered too since other event delivery mechanisms will
have some jitter too I decided to play it safe, but I do agree
that it is probably a good idea to enable this for all touchpads.
Also having the create_pointer_accelerator_filter_touchpad
argument be a sample-rate/freq rather then a period-time
is fine with me too.
Regards,
Hans
>
> src/evdev-mt-touchpad.c | 2 +-
> src/filter.c | 53 +++++++++++++++++++++++++++++++++++++++++--------
> src/filter.h | 3 ++-
> tools/ptraccel-debug.c | 2 +-
> 4 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index 0a4f4d98..b2968444 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -2140,7 +2140,7 @@ tp_init_accel(struct tp_dispatch *tp)
> tp->device->model_flags & EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81)
> filter = create_pointer_accelerator_filter_lenovo_x230(tp->device->dpi);
> else
> - filter = create_pointer_accelerator_filter_touchpad(tp->device->dpi);
> + filter = create_pointer_accelerator_filter_touchpad(device->dpi, 82);
>
> if (!filter)
> return false;
> diff --git a/src/filter.c b/src/filter.c
> index 7c500f87..d470d633 100644
> --- a/src/filter.c
> +++ b/src/filter.c
> @@ -176,6 +176,10 @@ struct pointer_accelerator {
> double accel; /* unitless factor */
> double incline; /* incline of the function */
>
> + /* For smoothing timestamps from devices with unreliable timing */
> + unsigned int report_rate; /* in Hz */
> + uint64_t report_rate_tdelta; /* in us */
> +
> int dpi;
> };
>
> @@ -208,6 +212,30 @@ feed_trackers(struct pointer_accelerator *accel,
> trackers[i].delta.y += delta->y;
> }
>
> + /* If we have a fixed report rate on the device we don't take the
> + * kernel timestamps, they may be inaccurate. Instead, anything
> + * less than 1.5 times the scanout rate is forced to that scanout
> + * rate.
> + */
> + if (accel->report_rate != 0) {
> + uint64_t last_time;
> + int tdelta = 0;
> +
> + last_time = trackers[accel->cur_tracker].time;
> + /* FIXME: if last_time >= time, the device's report_rate is
> + higher than the one we set. This can be ignored (e.g.
> + bluetooth touchpad that send 3 packets every 30ms,
> + but packets two and three have a delta of 1ms) or it can
> + become a problem (e.g. 80Hz set but 100Hz reported, our
> + timestamps will get more and more out of sync)
> + */
> + if (last_time < time)
> + tdelta = time - last_time;
> +
> + if (tdelta < accel->report_rate_tdelta * 1.5)
> + time = last_time + tdelta;
> + }
> +
> current = (accel->cur_tracker + 1) % NUM_POINTER_TRACKERS;
> accel->cur_tracker = current;
>
> @@ -227,14 +255,18 @@ tracker_by_offset(struct pointer_accelerator *accel, unsigned int offset)
> }
>
> static double
> -calculate_tracker_velocity(struct pointer_tracker *tracker, uint64_t time)
> +calculate_tracker_velocity(struct pointer_accelerator *accel,
> + struct pointer_tracker *tracker, uint64_t time)
> {
> - double tdelta = time - tracker->time + 1;
> - return hypot(tracker->delta.x, tracker->delta.y) / tdelta; /* units/us */
> + uint64_t tdelta = time - tracker->time + 1;
> +
> + return hypot(tracker->delta.x, tracker->delta.y) /
> + (double)tdelta; /* units/us */
> }
>
> static inline double
> -calculate_velocity_after_timeout(struct pointer_tracker *tracker)
> +calculate_velocity_after_timeout(struct pointer_accelerator *accel,
> + struct pointer_tracker *tracker)
> {
> /* First movement after timeout needs special handling.
> *
> @@ -247,7 +279,7 @@ calculate_velocity_after_timeout(struct pointer_tracker *tracker)
> * for really slow movements but provides much more useful initial
> * movement in normal use-cases (pause, move, pause, move)
> */
> - return calculate_tracker_velocity(tracker,
> + return calculate_tracker_velocity(accel, tracker,
> tracker->time + MOTION_TIMEOUT);
> }
>
> @@ -282,11 +314,11 @@ calculate_velocity(struct pointer_accelerator *accel, uint64_t time)
> /* Stop if too far away in time */
> if (time - tracker->time > MOTION_TIMEOUT) {
> if (offset == 1)
> - result = calculate_velocity_after_timeout(tracker);
> + result = calculate_velocity_after_timeout(accel, tracker);
> break;
> }
>
> - velocity = calculate_tracker_velocity(tracker, time);
> + velocity = calculate_tracker_velocity(accel, tracker, time);
>
> /* Stop if direction changed */
> dir &= tracker->dir;
> @@ -1017,16 +1049,21 @@ struct motion_filter_interface accelerator_interface_touchpad = {
> };
>
> struct motion_filter *
> -create_pointer_accelerator_filter_touchpad(int dpi)
> +create_pointer_accelerator_filter_touchpad(int dpi,
> + unsigned int report_rate_hz)
> {
> struct pointer_accelerator *filter;
>
> + assert(report_rate_hz < 1000);
> +
> filter = create_default_filter(dpi);
> if (!filter)
> return NULL;
>
> filter->base.interface = &accelerator_interface_touchpad;
> filter->profile = touchpad_accel_profile_linear;
> + filter->report_rate_tdelta = ms2us(1000/report_rate_hz);
> + filter->report_rate = report_rate_hz;
>
> return &filter->base;
> }
> diff --git a/src/filter.h b/src/filter.h
> index e24c20d4..82758523 100644
> --- a/src/filter.h
> +++ b/src/filter.h
> @@ -114,7 +114,8 @@ struct motion_filter *
> create_pointer_accelerator_filter_linear_low_dpi(int dpi);
>
> struct motion_filter *
> -create_pointer_accelerator_filter_touchpad(int dpi);
> +create_pointer_accelerator_filter_touchpad(int dpi,
> + unsigned int report_rate_hz);
>
> struct motion_filter *
> create_pointer_accelerator_filter_lenovo_x230(int dpi);
> diff --git a/tools/ptraccel-debug.c b/tools/ptraccel-debug.c
> index acb82c69..c026542b 100644
> --- a/tools/ptraccel-debug.c
> +++ b/tools/ptraccel-debug.c
> @@ -314,7 +314,7 @@ main(int argc, char **argv)
> filter = create_pointer_accelerator_filter_linear_low_dpi(dpi);
> profile = pointer_accel_profile_linear_low_dpi;
> } else if (streq(filter_type, "touchpad")) {
> - filter = create_pointer_accelerator_filter_touchpad(dpi);
> + filter = create_pointer_accelerator_filter_touchpad(dpi, 80);
> profile = touchpad_accel_profile_linear;
> } else if (streq(filter_type, "x230")) {
> filter = create_pointer_accelerator_filter_lenovo_x230(dpi);
>
More information about the wayland-devel
mailing list