[PATCH libinput 10/11] touchpad: use pressure values for touch is-down decision
Peter Hutterer
peter.hutterer at who-t.net
Thu Feb 9 22:55:58 UTC 2017
On Thu, Feb 09, 2017 at 05:25:36PM +0100, Hans de Goede wrote:
> Hi,
>
> First of all patches 1-9 and 11 looks good to me and are:
>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
thx, fwiw, I already pushed these, so fixups will come as a separate patch.
> 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;
I don't think that's correct. tp->nfingers_down includes fake touches being
down, whereas fake touches merely counts the BTN_TOOL_ bits set. So
tp->nfingers_down > nfake_touches can only be true on the bcm5974 when 6
fingers are down (that's the only touchpad that has >5 slots, iirc).
and if it's == nfake_touches, then we already have all fingers down, so we
would never unhover, returning here isn't right.
this condition really checks that "we have more slots than fake fingers
down" which is the condition for "don't care about fake fingers".
Come to think of it, I wonder if it'd be useful to disable BTN_TOOL_* for
those < num_slots at the evdev level to avoid mixing up the two...
> 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.
hmm, I'm not sure I read this the same way. If we get here, we have at least
num_slots + 1 fake touches down. As the comments says, we assume that this
means *all* fingers are down with pressure.
so we loop through the touches we have and put all of them down that are
currently hovering, starting with the real touches. Until we eventually meet
the fake touch count. That break is mostly to exit the loop since we may
have 3 fake touches but 5 potential touches.
Cheers,
Peter
>
> > +
> > + 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