[PATCH libinput 10/11] touchpad: use pressure values for touch is-down decision
Hans de Goede
hdegoede at redhat.com
Fri Feb 10 09:57:11 UTC 2017
Hi,
On 09-02-17 23:55, Peter Hutterer wrote:
> 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...
I only changed the if to match the break condition in the loop below,
but I realize now that if we then need lift fake touches we won't
do that, so I think you're right.
>> 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.
My point is this break is either unnecessary, or it should be done before
the tp_begin_touch otherwise each time we go through this function
we add one more fake touch before reaching the break. I think it is simply
unnecessary, just making sure it is.
If it is unnecessary the code would IMHO be more readable if we simply
drop it.
Regards,
Hans
>
> 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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list