[PATCH libinput 10/11] touchpad: use pressure values for touch is-down decision
Hans de Goede
hdegoede at redhat.com
Thu Feb 9 16:25:36 UTC 2017
Hi,
First of all patches 1-9 and 11 looks good to me and are:
Reviewed-by: Hans de Goede <hdegoede at redhat.com>
I've some remarks inline on this one.
On 30-01-17 01:58, Peter Hutterer wrote:
> Don't rely on BTN_TOUCH for "finger down", the value for that is hardcoded in
> the kernel and not always suitable. Some devices need a different value to
> avoid reacting to accidental touches or hovering fingers.
>
> Implement a basic Schmitt trigger, same as we have in the synaptics driver. We
> also take the default values from there but these will likely see some
> updates.
>
> A special case is when we have more fingers down than slots. Since we can't
> detect the pressure on fake fingers (we only get a bit for 'is down') we
> assume that *all* fingers are down with sufficient pressure. It's too much of
> a niche case to have this work any other way.
>
> This patch drops the handling of ABS_DISTANCE because it's simply not needed
> anymore.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> src/evdev-mt-touchpad.c | 129 +++++++++++++++++++++++++++++------
> src/evdev-mt-touchpad.h | 10 ++-
> test/litest-device-alps-dualpoint.c | 14 ++++
> test/litest-device-alps-semi-mt.c | 14 ++++
> test/litest-device-atmel-hover.c | 14 ++++
> test/litest-device-synaptics-hover.c | 14 ++++
> test/litest-device-synaptics-st.c | 15 +++-
> 7 files changed, 185 insertions(+), 25 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index a47c59f..ff9ec41 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -322,9 +322,6 @@ tp_process_absolute(struct tp_dispatch *tp,
> case ABS_MT_SLOT:
> tp->slot = e->value;
> break;
> - case ABS_MT_DISTANCE:
> - t->distance = e->value;
> - break;
> case ABS_MT_TRACKING_ID:
> if (e->value != -1)
> tp_new_touch(tp, t, time);
> @@ -365,6 +362,11 @@ tp_process_absolute_st(struct tp_dispatch *tp,
> t->dirty = true;
> tp->queued |= TOUCHPAD_EVENT_MOTION;
> break;
> + case ABS_PRESSURE:
> + t->pressure = e->value;
> + t->dirty = true;
> + tp->queued |= TOUCHPAD_EVENT_OTHERAXIS;
> + break;
> }
> }
>
> @@ -795,26 +797,72 @@ out:
> }
>
> static void
> -tp_unhover_abs_distance(struct tp_dispatch *tp, uint64_t time)
> +tp_unhover_pressure(struct tp_dispatch *tp, uint64_t time)
> {
> struct tp_touch *t;
> - unsigned int i;
> + int i;
> + unsigned int nfake_touches;
> + unsigned int real_fingers_down = 0;
>
> - for (i = 0; i < tp->ntouches; i++) {
> + nfake_touches = tp_fake_finger_count(tp);
> + if (nfake_touches == FAKE_FINGER_OVERFLOW)
> + nfake_touches = 0;
> +
> + for (i = 0; i < (int)tp->num_slots; i++) {
> t = tp_get_touch(tp, i);
>
> - if (!t->dirty)
> - continue;
> + if (t->dirty) {
> + if (t->state == TOUCH_HOVERING) {
> + if (t->pressure >= tp->pressure.high) {
> + /* avoid jumps when landing a finger */
> + tp_motion_history_reset(t);
> + tp_begin_touch(tp, t, time);
> + }
> + } else {
> + if (t->pressure < tp->pressure.low)
> + tp_end_touch(tp, t, time);
> + }
> + }
> +
> + if (t->state == TOUCH_BEGIN ||
> + t->state == TOUCH_UPDATE)
> + real_fingers_down++;
> + }
>
> + if (nfake_touches <= tp->num_slots ||
> + tp->nfingers_down == 0)
> + return;
Maybe:
if (tp->nfingers_down >= nfake_touches ||
tp->nfingers_down == 0)
return;
Otherwise this:
> +
> + /* if we have more fake fingers down than slots, we assume
> + * _all_ fingers have enough pressure, even if some of the slotted
> + * ones don't. Anything else gets insane quickly.
> + */
> + for (i = 0; i < (int)tp->ntouches; i++) {
> + t = tp_get_touch(tp, i);
> if (t->state == TOUCH_HOVERING) {
<snip removed bit>
> + /* avoid jumps when landing a finger */
> + tp_motion_history_reset(t);
> + tp_begin_touch(tp, t, time);
> +
> + if (tp->nfingers_down >= nfake_touches)
> + break;
> + }
> + }
Will always unhover one more hovering touch even if we
already have enough, I don't know if we can have more touches
in down + hovering state then nfake_touches but otherwise
the
if (tp->nfingers_down >= nfake_touches)
break;
Is not necessary.
> +
> + if (tp->nfingers_down > nfake_touches ||
> + real_fingers_down == 0) {
> + for (i = tp->ntouches - 1; i >= 0; i--) {
> + t = tp_get_touch(tp, i);
> +
> + if (t->state == TOUCH_HOVERING ||
> + t->state == TOUCH_NONE)
> + continue;
> +
> + tp_end_touch(tp, t, time);
> +
> + if (real_fingers_down > 0 &&
> + tp->nfingers_down == nfake_touches)
> + break;
> }
> }
> }
> @@ -881,8 +929,8 @@ tp_unhover_fake_touches(struct tp_dispatch *tp, uint64_t time)
> static void
> tp_unhover_touches(struct tp_dispatch *tp, uint64_t time)
> {
> - if (tp->reports_distance)
> - tp_unhover_abs_distance(tp, time);
> + if (tp->pressure.use_pressure)
> + tp_unhover_pressure(tp, time);
> else
> tp_unhover_fake_touches(tp, time);
>
> @@ -926,6 +974,7 @@ tp_position_fake_touches(struct tp_dispatch *tp)
> continue;
>
> t->point = topmost->point;
> + t->pressure = topmost->pressure;
> if (!t->dirty)
> t->dirty = topmost->dirty;
> }
> @@ -1747,7 +1796,13 @@ tp_sync_touch(struct tp_dispatch *tp,
> &t->point.y))
> t->point.y = libevdev_get_event_value(evdev, EV_ABS, ABS_Y);
>
> - libevdev_fetch_slot_value(evdev, slot, ABS_MT_DISTANCE, &t->distance);
> + if (!libevdev_fetch_slot_value(evdev,
> + slot,
> + ABS_MT_PRESSURE,
> + &t->pressure))
> + t->pressure = libevdev_get_event_value(evdev,
> + EV_ABS,
> + ABS_PRESSURE);
> }
>
> static inline void
> @@ -2271,6 +2326,38 @@ tp_init_hysteresis(struct tp_dispatch *tp)
> return;
> }
>
> +static void
> +tp_init_pressure(struct tp_dispatch *tp,
> + struct evdev_device *device)
> +{
> + const struct input_absinfo *abs;
> + unsigned int range;
> + unsigned int code = ABS_PRESSURE;
> +
> + if (tp->has_mt)
> + code = ABS_MT_PRESSURE;
> +
> + if (!libevdev_has_event_code(device->evdev, EV_ABS, code)) {
> + tp->pressure.use_pressure = false;
> + return;
> + }
> +
> + tp->pressure.use_pressure = true;
> +
> + abs = libevdev_get_abs_info(device->evdev, code);
> + assert(abs);
> +
> + range = abs->maximum - abs->minimum;
> +
> + /* Approximately the synaptics defaults */
> + tp->pressure.high = abs->minimum + 0.12 * range;
> + tp->pressure.low = abs->minimum + 0.10 * range;
> +
> + log_debug(evdev_libinput_context(device),
> + "%s: using pressure-based touch detection\n",
> + device->devname);
> +}
> +
> static int
> tp_init(struct tp_dispatch *tp,
> struct evdev_device *device)
> @@ -2288,9 +2375,7 @@ tp_init(struct tp_dispatch *tp,
>
> evdev_device_init_abs_range_warnings(device);
>
> - tp->reports_distance = libevdev_has_event_code(device->evdev,
> - EV_ABS,
> - ABS_MT_DISTANCE);
> + tp_init_pressure(tp, device);
>
> /* Set the dpi to that of the x axis, because that's what we normalize
> to when needed*/
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index abe885f..eae0af9 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -153,7 +153,6 @@ struct tp_touch {
> bool dirty;
> struct device_coords point;
> uint64_t millis;
> - int distance; /* distance == 0 means touch */
> int pressure;
>
> bool was_down; /* if distance == 0, false for pure hovering
> @@ -232,7 +231,6 @@ struct tp_dispatch {
> unsigned int slot; /* current slot */
> bool has_mt;
> bool semi_mt;
> - bool reports_distance; /* does the device support true hovering */
>
> /* true if we're reading events (i.e. not suspended) but we're
> * ignoring them */
> @@ -248,6 +246,14 @@ struct tp_dispatch {
> */
> unsigned int fake_touches;
>
> + /* if pressure goes above high -> touch down,
> + if pressure then goes below low -> touch up */
> + struct {
> + bool use_pressure;
> + int high;
> + int low;
> + } pressure;
> +
> struct device_coords hysteresis_margin;
>
> struct {
> diff --git a/test/litest-device-alps-dualpoint.c b/test/litest-device-alps-dualpoint.c
> index de025e5..90fdbca 100644
> --- a/test/litest-device-alps-dualpoint.c
> +++ b/test/litest-device-alps-dualpoint.c
> @@ -60,9 +60,23 @@ static struct input_event move[] = {
> { .type = -1, .code = -1 },
> };
>
> +static int
> +get_axis_default(struct litest_device *d, unsigned int evcode, int32_t *value)
> +{
> + switch (evcode) {
> + case ABS_PRESSURE:
> + case ABS_MT_PRESSURE:
> + *value = 30;
> + return 0;
> + }
> + return 1;
> +}
> +
> static struct litest_device_interface interface = {
> .touch_down_events = down,
> .touch_move_events = move,
> +
> + .get_axis_default = get_axis_default,
> };
>
> static struct input_id input_id = {
> diff --git a/test/litest-device-alps-semi-mt.c b/test/litest-device-alps-semi-mt.c
> index d78adca..de0eb3a 100644
> --- a/test/litest-device-alps-semi-mt.c
> +++ b/test/litest-device-alps-semi-mt.c
> @@ -60,9 +60,23 @@ static struct input_event move[] = {
> { .type = -1, .code = -1 },
> };
>
> +static int
> +get_axis_default(struct litest_device *d, unsigned int evcode, int32_t *value)
> +{
> + switch (evcode) {
> + case ABS_PRESSURE:
> + case ABS_MT_PRESSURE:
> + *value = 30;
> + return 0;
> + }
> + return 1;
> +}
> +
> static struct litest_device_interface interface = {
> .touch_down_events = down,
> .touch_move_events = move,
> +
> + .get_axis_default = get_axis_default,
> };
>
> static struct input_id input_id = {
> diff --git a/test/litest-device-atmel-hover.c b/test/litest-device-atmel-hover.c
> index c856d08..942744e 100644
> --- a/test/litest-device-atmel-hover.c
> +++ b/test/litest-device-atmel-hover.c
> @@ -76,10 +76,24 @@ static struct input_event up[] = {
> { .type = -1, .code = -1 },
> };
>
> +static int
> +get_axis_default(struct litest_device *d, unsigned int evcode, int32_t *value)
> +{
> + switch (evcode) {
> + case ABS_PRESSURE:
> + case ABS_MT_PRESSURE:
> + *value = 30;
> + return 0;
> + }
> + return 1;
> +}
> +
> static struct litest_device_interface interface = {
> .touch_down_events = down,
> .touch_move_events = move,
> .touch_up_events = up,
> +
> + .get_axis_default = get_axis_default,
> };
>
> static struct input_id input_id = {
> diff --git a/test/litest-device-synaptics-hover.c b/test/litest-device-synaptics-hover.c
> index 8439f10..9c0c7bd 100644
> --- a/test/litest-device-synaptics-hover.c
> +++ b/test/litest-device-synaptics-hover.c
> @@ -60,9 +60,23 @@ static struct input_event move[] = {
> { .type = -1, .code = -1 },
> };
>
> +static int
> +get_axis_default(struct litest_device *d, unsigned int evcode, int32_t *value)
> +{
> + switch (evcode) {
> + case ABS_PRESSURE:
> + case ABS_MT_PRESSURE:
> + *value = 30;
> + return 0;
> + }
> + return 1;
> +}
> +
> static struct litest_device_interface interface = {
> .touch_down_events = down,
> .touch_move_events = move,
> +
> + .get_axis_default = get_axis_default,
> };
>
> static struct input_id input_id = {
> diff --git a/test/litest-device-synaptics-st.c b/test/litest-device-synaptics-st.c
> index 7e45d56..bbde1ef 100644
> --- a/test/litest-device-synaptics-st.c
> +++ b/test/litest-device-synaptics-st.c
> @@ -36,7 +36,7 @@ litest_synaptics_touchpad_setup(void)
> static struct input_event down[] = {
> { .type = EV_ABS, .code = ABS_X, .value = LITEST_AUTO_ASSIGN },
> { .type = EV_ABS, .code = ABS_Y, .value = LITEST_AUTO_ASSIGN },
> - { .type = EV_ABS, .code = ABS_PRESSURE, .value = 30 },
> + { .type = EV_ABS, .code = ABS_PRESSURE, .value = LITEST_AUTO_ASSIGN },
> { .type = EV_ABS, .code = ABS_TOOL_WIDTH, .value = 7 },
> { .type = EV_SYN, .code = SYN_REPORT, .value = 0 },
> { .type = -1, .code = -1 },
> @@ -54,10 +54,23 @@ struct input_event up[] = {
> { .type = -1, .code = -1 },
> };
>
> +static int
> +get_axis_default(struct litest_device *d, unsigned int evcode, int32_t *value)
> +{
> + switch (evcode) {
> + case ABS_PRESSURE:
> + *value = 30;
> + return 0;
> + }
> + return 1;
> +}
> +
> static struct litest_device_interface interface = {
> .touch_down_events = down,
> .touch_move_events = move,
> .touch_up_events = up,
> +
> + .get_axis_default = get_axis_default,
> };
>
> static struct input_absinfo absinfo[] = {
>
Otherwise looks good.
Regards,
Hans
More information about the wayland-devel
mailing list