[PATCH] Input: touchscreen: AD7879: prevent invalid finger data reports

[PATCH] Input: touchscreen: AD7879: prevent invalid finger data reports

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Considering following scenario - the touch is present on the screen
at the beginning of the last conversion sequence, but by the time
the last sequence is finished, the finger is lift off.
The AD7879 data available interrupt signals (DAV) completion,
however some X,Y values are not valid because the screen inputs were
floating during the acquisition.

The AD7877 acts differently here, since it only asserts DAV
if the touch is still present when the conversion sequence finished.

Based on the fact that this can only happen in the last sample of the
repeated conversion sequence. We simply skip the last.
(Short glitches are filtered by the AD7879 internal median and average filters)
This doesn't cause noticeable side effects, since the minimum conversion
interval is 9.44ms. We receive ~100 waypoint samples per second,
so we simply delay the result by 9.44ms.

Actually this patch repeats the first waypoint twice and then skips the last.
This patch also rejects samples where pressure > pressure_max.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/input/touchscreen/ad7879.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c
index ba6f0bd..3173589 100644
--- a/drivers/input/touchscreen/ad7879.c
+++ b/drivers/input/touchscreen/ad7879.c
@@ -129,6 +129,9 @@ struct ad7879 {
 	u16			cmd_crtl1;
 	u16			cmd_crtl2;
 	u16			cmd_crtl3;
+	int			x;
+	int			y;
+	int			Rt;	
 };
 
 static int ad7879_read(struct ad7879 *ts, u8 reg)
@@ -175,13 +178,29 @@ static int ad7879_report(struct ad7879 *ts)
 		Rt /= z1;
 		Rt = (Rt + 2047) >> 12;
 
-		if (!timer_pending(&ts->timer))
+		/*
+		 * Sample found inconsistent, pressure is beyond
+		 * the maximum. Don't report it to user space.
+		 */
+		if (Rt > ts->pressure_max)
+			return -EINVAL;
+
+		if (!timer_pending(&ts->timer)) {
+			ts->x = x;
+			ts->y = y;
+			ts->Rt = Rt;
 			input_report_key(input_dev, BTN_TOUCH, 1);
-
-		input_report_abs(input_dev, ABS_X, x);
-		input_report_abs(input_dev, ABS_Y, y);
-		input_report_abs(input_dev, ABS_PRESSURE, Rt);
-		input_sync(input_dev);
+		}
+		
+		input_report_abs(input_dev, ABS_X, ts->x);
+		input_report_abs(input_dev, ABS_Y, ts->y);
+		input_report_abs(input_dev, ABS_PRESSURE, ts->Rt);
+		input_sync(input_dev);		
+
+		ts->x = x;
+		ts->y = y;
+		ts->Rt = Rt;
+		
 		return 0;
 	}
 
-- 
1.6.0.2

Re: [PATCH] Input: touchscreen: AD7879: prevent invalid finger data reports

[Dmitry Torokhov]

Hi Michael,

On Mon, Oct 18, 2010 at 03:33:24PM +0200, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Considering following scenario - the touch is present on the screen
> at the beginning of the last conversion sequence, but by the time
> the last sequence is finished, the finger is lift off.
> The AD7879 data available interrupt signals (DAV) completion,
> however some X,Y values are not valid because the screen inputs were
> floating during the acquisition.
> 
> The AD7877 acts differently here, since it only asserts DAV
> if the touch is still present when the conversion sequence finished.
> 
> Based on the fact that this can only happen in the last sample of the
> repeated conversion sequence. We simply skip the last.
> (Short glitches are filtered by the AD7879 internal median and average filters)
> This doesn't cause noticeable side effects, since the minimum conversion
> interval is 9.44ms. We receive ~100 waypoint samples per second,
> so we simply delay the result by 9.44ms.
> 
> Actually this patch repeats the first waypoint twice and then skips the last.

Input core won't actually pass the 2nd instance through. I think the
whole thing should look like the patch below.

Thanks.

-- 
Dmitry

Input: ad7879 - prevent invalid finger data reports

From: Michael Hennerich <michael.hennerich@analog.com>

Considering following scenario - the touch is present on the screen
at the beginning of the last conversion sequence, but by the time
the last sequence is finished, the finger is lift off. The AD7879 data
available interrupt signals (DAV) completion, however some X,Y values
are not valid because the screen inputs were floating during the
acquisition.

The AD7877 acts differently here, since it only asserts DAV if the
touch is still present when the conversion sequence finished.

Based on the fact that this can only happen in the last sample of the
repeated conversion sequence, we simply skip the last (short glitches
are filtered by the AD7879 internal median and average filters).
This doesn't cause noticeable side effects, since the minimum conversion
interval is 9.44ms. We receive ~100 waypoint samples per second, so we
simply delay the result by 9.44ms.

We also reject samples where pressure is greater than pressure_max.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/touchscreen/ad7879.c |   32 +++++++++++++++++++++++++++-----
 1 files changed, 27 insertions(+), 5 deletions(-)


diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c
index ba6f0bd..bc3b518 100644
--- a/drivers/input/touchscreen/ad7879.c
+++ b/drivers/input/touchscreen/ad7879.c
@@ -129,6 +129,9 @@ struct ad7879 {
 	u16			cmd_crtl1;
 	u16			cmd_crtl2;
 	u16			cmd_crtl3;
+	int			x;
+	int			y;
+	int			Rt;
 };
 
 static int ad7879_read(struct ad7879 *ts, u8 reg)
@@ -175,13 +178,32 @@ static int ad7879_report(struct ad7879 *ts)
 		Rt /= z1;
 		Rt = (Rt + 2047) >> 12;
 
-		if (!timer_pending(&ts->timer))
+		/*
+		 * Sample found inconsistent, pressure is beyond
+		 * the maximum. Don't report it to user space.
+		 */
+		if (Rt > ts->pressure_max)
+			return -EINVAL;
+
+		/*
+		 * Note that we delay reporting events by one sample.
+		 * This is done to avoid reporting last sample of the
+		 * touch sequence, which may be incomplete if finger
+		 * leaves the surface before last reading is taken.
+		 */
+		if (timer_pending(&ts->timer)) {
+			/* Touch continues */
 			input_report_key(input_dev, BTN_TOUCH, 1);
+			input_report_abs(input_dev, ABS_X, ts->x);
+			input_report_abs(input_dev, ABS_Y, ts->y);
+			input_report_abs(input_dev, ABS_PRESSURE, ts->Rt);
+			input_sync(input_dev);
+		}
+
+		ts->x = x;
+		ts->y = y;
+		ts->Rt = Rt;
 
-		input_report_abs(input_dev, ABS_X, x);
-		input_report_abs(input_dev, ABS_Y, y);
-		input_report_abs(input_dev, ABS_PRESSURE, Rt);
-		input_sync(input_dev);
 		return 0;
 	}
 

RE: [PATCH] Input: touchscreen: AD7879: prevent invalid finger data reports

[Hennerich, Michael]

Dmitry Torokhov wrote on 2010-10-22:
> Hi Michael,
>
> On Mon, Oct 18, 2010 at 03:33:24PM +0200, michael.hennerich@analog.com
> wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> Considering following scenario - the touch is present on the screen at
>> the beginning of the last conversion sequence, but by the time the last
>> sequence is finished, the finger is lift off. The AD7879 data available
>> interrupt signals (DAV) completion, however some X,Y values are not
>> valid because the screen inputs were floating during the acquisition.
>>
>> The AD7877 acts differently here, since it only asserts DAV if the
>> touch is still present when the conversion sequence finished.
>>
>> Based on the fact that this can only happen in the last sample of the
>> repeated conversion sequence. We simply skip the last. (Short glitches
>> are filtered by the AD7879 internal median and average filters) This
>> doesn't cause noticeable side effects, since the minimum conversion
>> interval is 9.44ms. We receive ~100 waypoint samples per second, so we
>> simply delay the result by 9.44ms.
>>
>> Actually this patch repeats the first waypoint twice and then skips
> the last.
>
> Input core won't actually pass the 2nd instance through. I think the
> whole thing should look like the patch below.

I know input core suppresses identical events.
Your patch looks nicer, however it will delay the first response by one
Interrupt. Considering the minimum automatic conversion interval of 9.44ms
- Not sure how much this really matters.

Acked-by: Michael Hennerich <michael.hennerich@analog.com>

Greetings,
Michael

Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Re: [PATCH] Input: touchscreen: AD7879: prevent invalid finger data reports

[Dmitry Torokhov]

On Fri, Oct 22, 2010 at 08:16:48AM +0100, Hennerich, Michael wrote:
> Dmitry Torokhov wrote on 2010-10-22:
> > Hi Michael,
> >
> > On Mon, Oct 18, 2010 at 03:33:24PM +0200, michael.hennerich@analog.com
> > wrote:
> >> From: Michael Hennerich <michael.hennerich@analog.com>
> >>
> >> Considering following scenario - the touch is present on the screen at
> >> the beginning of the last conversion sequence, but by the time the last
> >> sequence is finished, the finger is lift off. The AD7879 data available
> >> interrupt signals (DAV) completion, however some X,Y values are not
> >> valid because the screen inputs were floating during the acquisition.
> >>
> >> The AD7877 acts differently here, since it only asserts DAV if the
> >> touch is still present when the conversion sequence finished.
> >>
> >> Based on the fact that this can only happen in the last sample of the
> >> repeated conversion sequence. We simply skip the last. (Short glitches
> >> are filtered by the AD7879 internal median and average filters) This
> >> doesn't cause noticeable side effects, since the minimum conversion
> >> interval is 9.44ms. We receive ~100 waypoint samples per second, so we
> >> simply delay the result by 9.44ms.
> >>
> >> Actually this patch repeats the first waypoint twice and then skips
> > the last.
> >
> > Input core won't actually pass the 2nd instance through. I think the
> > whole thing should look like the patch below.
> 
> I know input core suppresses identical events.
> Your patch looks nicer, however it will delay the first response by one
> Interrupt. Considering the minimum automatic conversion interval of 9.44ms
> - Not sure how much this really matters.
> 
> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> 

Thanks Michael. I think 10-15ms delay should not be noticeable for user
input, however you have the hardware, any chance you could try and see?

-- 
Dmitry

RE: [PATCH] Input: touchscreen: AD7879: prevent invalid finger data reports

[Hennerich, Michael]

Dmitry Torokhov wrote on 2010-10-22:
> On Fri, Oct 22, 2010 at 08:16:48AM +0100, Hennerich, Michael wrote:
>> Dmitry Torokhov wrote on 2010-10-22:
>>> Hi Michael,
>>>
>>> On Mon, Oct 18, 2010 at 03:33:24PM +0200,
>>> michael.hennerich@analog.com
>>> wrote:
>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>
>>>> Considering following scenario - the touch is present on the screen
>>>> at the beginning of the last conversion sequence, but by the time the
>>>> last sequence is finished, the finger is lift off. The AD7879 data
>>>> available interrupt signals (DAV) completion, however some X,Y values
>>>> are not valid because the screen inputs were floating during the
>>>> acquisition.
>>>>
>>>> The AD7877 acts differently here, since it only asserts DAV if
>>>> the touch is still present when the conversion sequence finished.
>>>>
>>>> Based on the fact that this can only happen in the last sample of
>>>> the repeated conversion sequence. We simply skip the last. (Short
>>>> glitches are filtered by the AD7879 internal median and average
>>>> filters) This doesn't cause noticeable side effects, since the
>>>> minimum conversion interval is 9.44ms. We receive ~100 waypoint
>>>> samples per second, so we simply delay the result by 9.44ms.
>>>>
>>>> Actually this patch repeats the first waypoint twice and then
> skips
>>> the last.
>>>
>>> Input core won't actually pass the 2nd instance through. I think the
>>> whole thing should look like the patch below.
>>
>> I know input core suppresses identical events.
>> Your patch looks nicer, however it will delay the first response by
>> one Interrupt. Considering the minimum automatic conversion interval
>> of 9.44ms
>> - Not sure how much this really matters.
>>
>> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
>>
>
> Thanks Michael. I think 10-15ms delay should not be noticeable for
> user input, however you have the hardware, any chance you could try and see?

I can confirm that 10ms delay is not noticeable. However this version of the patch
may skip if the touch is very short, so it also implements event filtering of contacts
shorter than 20ms.

Greetings,
Michael

Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif