Re: [PATCH] Input: mms114: don't report 0 pressure while still tracking contact(s)

[Peter Hutterer <peter.hutterer@who-t.net>]

On Sat, Jun 06, 2020 at 11:18:06AM -0700, Dmitry Torokhov wrote:
> Hi Denis,
> 
> On Sat, Jun 06, 2020 at 05:50:17AM +0200, Denis 'GNUtoo' Carikli wrote:
> > In the middle of a sliding gesture, we manage to have events
> > that look like that:
> >     Event: time 1571859641.595517, -------------- SYN_REPORT ------------
> >     Event: time 1571859641.606593, type 3 (EV_ABS), code 54 (ABS_MT_POSITION_Y), value 1193
> >     Event: time 1571859641.606593, type 3 (EV_ABS), code 48 (ABS_MT_TOUCH_MAJOR), value 21
> >     Event: time 1571859641.606593, type 3 (EV_ABS), code 58 (ABS_MT_PRESSURE), value 0
> >     Event: time 1571859641.606593, type 3 (EV_ABS), code 1 (ABS_Y), value 1193
> >     Event: time 1571859641.606593, type 3 (EV_ABS), code 24 (ABS_PRESSURE), value 0
> >     Event: time 1571859641.606593, -------------- SYN_REPORT ------------
> > 
> > In such cases, we still have a valid ABS_MT_TRACKING_ID along
> > with an ABS_MT_TOUCH_MAJOR that is > 0, which both indicates
> > that the sliding is still in progress.
> > 
> > However in Documentation/input/multi-touch-protocol.rst, we
> > have:
> >     ABS_MT_PRESSURE
> >         The pressure, in arbitrary units, on the contact
> >         area. May be used instead of TOUCH and WIDTH for
> >         pressure-based devices or any device with a spatial
> >         signal intensity distribution.
> > 
> > Because of that userspace may consider an ABS_MT_PRESSURE
> > of 0 as an indication that the sliding stopped. This has
> > side effects such as making it difficult to unlock the
> > screen under Android.
> > 
> > This fix was tested on the following devices:
> > - GT-I9300 with a glass screen protection
> > - GT-I9300 without any screen protection
> > - GT-N7105 with a glass screen protection
> > 
> > Reported-by: Javi Ferrer <javi.f.o@gmail.com>
> > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> > ---
> >  drivers/input/touchscreen/mms114.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> > index 2ef1adaed9af..adc18cd9a437 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -183,7 +183,10 @@ static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *tou
> >  	if (touch->pressed) {
> >  		touchscreen_report_pos(input_dev, &data->props, x, y, true);
> >  		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, touch->width);
> > -		input_report_abs(input_dev, ABS_MT_PRESSURE, touch->strength);
> > +		if (touch->strength) {
> > +			input_report_abs(input_dev, ABS_MT_PRESSURE,
> > +					 touch->strength);
> 
> So this will result in userspace believing that pressure is unchanged
> from the previous packet. I wonder if we should report
> 
> 		input_report_abs(input_dev, ABS_MT_PRESSURE,
> 				 max(touch->strength, 1));
> 
> instead.

fwiw, this could still be interpreted as a lifted touch, especially if the
pressure values are comparatively high (say you go from 100 to 1).
we have to use thresholds for what is a touch up because a number of devices
give us nonzero pressure for ever-so-slight touches (or even hovers).
so a nonzero pressure isn't a guarantee for touch down.

not on this device, but from my POV I'd rather have the device pretend the
pressure is the same as the last known touch than just forcing it to 1, a
value that may be completely out of place.

Cheers,
   Peter