[BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[Niels Laukens]

[-- Attachment #1: Type: text/plain, Size: 3599 bytes --]

Hi,

I believe I've found a bug in the NEC decoder for InfraRed remote
controls. The problem manifests itself as an extra keypress that happens
when pushing different buttons in "rapid" succession.

I can reproduce the problem easily (but not always) by pushing DOWN, DOWN,
UP in "rapid" succession. I put "rapid" in quotes, because I don't have to
hurry in any way, it happens when I use it normally. Depending on the
duration of the presses, I get a number of repeats of DOWN. The bug is
that an additional DOWN keypress happens at the time that I press the UP
key (or so it seams).

Attached is kernel-debug.log, which contains the redacted and annotated
dmesg output, illustrating the problem described above. Especially note
lines 31-36 and 54-59, where more than 200ms pass between the end of the
IR-code and the actual emit of the keydown event.


I've debugged this issue, and believe I've found the cause: The keypress
is only emitted in state 5 (STATE_TRAILER_SPACE). This state is only
executed when the space after the message is received, i.e. when the
next pulse (of the next message) starts. It is only then that the length
of the space is known, and that ir_raw_event will fire an event.

The patch below addresses this issue. This is my first kernel patch.
I've tried to follow all guidelines that I could find, but might have
missed a few.

I've tested this patch with the out-of-tree TBS drivers (which seem to
be based on 3.13), and it solves the bug.
I've compared this TBS-version with the current master (1487385). There
are 8 (non-comment) lines that differ, none affect this patch. This
patch applies cleanly to the current master.

Regards,
Niels




>From 071c316e9315f22a055d6713cc8cdcdc73642193 Mon Sep 17 00:00:00 2001
From: Niels Laukens <niels@dest-unreach.be>
Date: Sat, 31 May 2014 10:30:18 +0200
Subject: [PATCH] drivers/media/rc/ir-nec-decode: fix phantom detect

The IR NEC decoder waited until the TRAILER_SPACE state to emit a
keypress. Since the triggering 'space' event will only be sent at the
beginning of the *next* IR-code, this is way to late.

This patch moves the processing to the TRAILER_PULSE state. Since we
arrived here with a 'pulse' event, we know that the pulse has ended and
thus that the space is there (as of yet with unknown length).

Signed-off-by: Niels Laukens <niels@dest-unreach.be>
---
 drivers/media/rc/ir-nec-decoder.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
index 35c42e5..955f99d 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -148,16 +148,6 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		if (!eq_margin(ev.duration, NEC_TRAILER_PULSE, NEC_UNIT / 2))
 			break;
 
-		data->state = STATE_TRAILER_SPACE;
-		return 0;
-
-	case STATE_TRAILER_SPACE:
-		if (ev.pulse)
-			break;
-
-		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
-			break;
-
 		address     = bitrev8((data->bits >> 24) & 0xff);
 		not_address = bitrev8((data->bits >> 16) & 0xff);
 		command	    = bitrev8((data->bits >>  8) & 0xff);
@@ -190,6 +180,16 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			data->necx_repeat = true;
 
 		rc_keydown(dev, scancode, 0);
+		data->state = STATE_TRAILER_SPACE;
+		return 0;
+
+	case STATE_TRAILER_SPACE:
+		if (ev.pulse)
+			break;
+
+		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
+			break;
+
 		data->state = STATE_INACTIVE;
 		return 0;
 	}
-- 
1.8.5.2 (Apple Git-48)



[-- Attachment #2: kernel-debug.log --]
[-- Type: text/plain, Size: 4455 bytes --]

[  587.936293] ir_nec_decode: NEC decode started at state 0 (9078us pulse)
[  587.936298] ir_nec_decode: NEC decode started at state 1 (4441us space)
[  587.952299] ir_nec_decode: NEC decode started at state 2 (627us pulse)
[  587.952304] ir_nec_decode: NEC decode started at state 3 (504us space)
                              ... more states 2||3 ...
[  588.000351] ir_nec_decode: NEC decode started at state 2 (625us pulse)
[  588.000353] ir_nec_decode: NEC decode started at state 3 (503us space)
[  588.000355] ir_nec_decode: NEC decode started at state 4 (623us pulse)
[  588.044379] ir_nec_decode: NEC decode started at state 5 (39684us space)
[  588.044383] ir_nec_decode: NEC scancode 0x0088
[  588.044386] rc_g_keycode_from_table: saa716x IR (TurboSight TBS 6281): scancode 0x0088 keycode 0x6c
[  588.044394] ir_do_keydown: saa716x IR (TurboSight TBS 6281): key down event, key 0x006c, scancode 0x0088


[  588.044420] ir_nec_decode: NEC decode started at state 0 (9080us pulse)
[  588.044422] ir_nec_decode: NEC decode started at state 1 (2184us space)
[  588.044426] ir_nec_decode: Repeat last key
[  588.044428] ir_nec_decode: NEC decode started at state 4 (625us pulse)
[  588.152556] ir_nec_decode: NEC decode started at state 5 (96421us space)
[  588.152559] ir_nec_decode: NEC scancode 0x0088
[  588.152561] rc_g_keycode_from_table: saa716x IR (TurboSight TBS 6281): scancode 0x0088 keycode 0x6c


[  588.260571] ir_nec_decode: NEC decode started at state 0 (9078us pulse)
[  588.276577] ir_nec_decode: NEC decode started at state 1 (4440us space)
[  588.276583] ir_nec_decode: NEC decode started at state 2 (627us pulse)
[  588.276587] ir_nec_decode: NEC decode started at state 3 (504us space)
                              ... more states 2||3 ...
[  588.324632] ir_nec_decode: NEC decode started at state 2 (625us pulse)
[  588.324634] ir_nec_decode: NEC decode started at state 3 (505us space)
[  588.324636] ir_nec_decode: NEC decode started at state 4 (624us pulse)
[  588.404656] ir_do_keyup: keyup key 0x006c
[  588.568833] ir_nec_decode: NEC decode started at state 5 (240550us space)
[  588.568837] ir_nec_decode: NEC scancode 0x0088
[  588.568840] rc_g_keycode_from_table: saa716x IR (TurboSight TBS 6281): scancode 0x0088 keycode 0x6c
[  588.568843] ir_do_keydown: saa716x IR (TurboSight TBS 6281): key down event, key 0x006c, scancode 0x0088


[  588.568863] ir_nec_decode: NEC decode started at state 0 (9076us pulse)
[  588.584848] ir_nec_decode: NEC decode started at state 1 (4445us space)
[  588.584853] ir_nec_decode: NEC decode started at state 2 (621us pulse)
[  588.584855] ir_nec_decode: NEC decode started at state 3 (507us space)
                              ... more states 2||3 ...
[  588.632927] ir_nec_decode: NEC decode started at state 2 (621us pulse)
[  588.632928] ir_nec_decode: NEC decode started at state 3 (509us space)
[  588.632930] ir_nec_decode: NEC decode started at state 4 (620us pulse)
[  588.676927] ir_nec_decode: NEC decode started at state 5 (39683us space)
[  588.676931] ir_nec_decode: NEC scancode 0x0088
[  588.676938] rc_g_keycode_from_table: saa716x IR (TurboSight TBS 6281): scancode 0x0088 keycode 0x6c

[  588.676963] ir_nec_decode: NEC decode started at state 0 (9048us pulse)
[  588.676966] ir_nec_decode: NEC decode started at state 1 (2212us space)
[  588.676970] ir_nec_decode: Repeat last key
[  588.676972] ir_nec_decode: NEC decode started at state 4 (597us pulse)
[  588.929112] ir_do_keyup: keyup key 0x006c
[  588.949162] ir_nec_decode: NEC decode started at state 5 (260338us space)
[  588.949166] ir_nec_decode: NEC scancode 0x0088
[  588.949169] rc_g_keycode_from_table: saa716x IR (TurboSight TBS 6281): scancode 0x0088 keycode 0x6c
[  588.949172] ir_do_keydown: saa716x IR (TurboSight TBS 6281): key down event, key 0x006c, scancode 0x0088


[  588.949192] ir_nec_decode: NEC decode started at state 0 (9014us pulse)
[  588.965177] ir_nec_decode: NEC decode started at state 1 (4509us space)
[  588.965183] ir_nec_decode: NEC decode started at state 2 (582us pulse)
[  588.965187] ir_nec_decode: NEC decode started at state 3 (571us space)
                              ... more states 2||3 ...
[  589.013261] ir_nec_decode: NEC decode started at state 2 (594us pulse)
[  589.013263] ir_nec_decode: NEC decode started at state 3 (534us space)
[  589.013265] ir_nec_decode: NEC decode started at state 4 (596us pulse)
[  589.201348] ir_do_keyup: keyup key 0x006c

Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[Niels Laukens]

I have not received any response on this email... so I hope to bump this
thread back to the more active region in most people's in-boxes.


On 2014-05-31 10:37, Niels Laukens wrote:
> Hi,
> 
> I believe I've found a bug in the NEC decoder for InfraRed remote
> controls. The problem manifests itself as an extra keypress that happens
> when pushing different buttons in "rapid" succession.
> 
> I can reproduce the problem easily (but not always) by pushing DOWN, DOWN,
> UP in "rapid" succession. I put "rapid" in quotes, because I don't have to
> hurry in any way, it happens when I use it normally. Depending on the
> duration of the presses, I get a number of repeats of DOWN. The bug is
> that an additional DOWN keypress happens at the time that I press the UP
> key (or so it seams).
> 
> Attached is kernel-debug.log, which contains the redacted and annotated
> dmesg output, illustrating the problem described above. Especially note
> lines 31-36 and 54-59, where more than 200ms pass between the end of the
> IR-code and the actual emit of the keydown event.
> 
> 
> I've debugged this issue, and believe I've found the cause: The keypress
> is only emitted in state 5 (STATE_TRAILER_SPACE). This state is only
> executed when the space after the message is received, i.e. when the
> next pulse (of the next message) starts. It is only then that the length
> of the space is known, and that ir_raw_event will fire an event.
> 
> The patch below addresses this issue. This is my first kernel patch.
> I've tried to follow all guidelines that I could find, but might have
> missed a few.
> 
> I've tested this patch with the out-of-tree TBS drivers (which seem to
> be based on 3.13), and it solves the bug.
> I've compared this TBS-version with the current master (1487385). There
> are 8 (non-comment) lines that differ, none affect this patch. This
> patch applies cleanly to the current master.
> 
> Regards,
> Niels
> 
> 
> 
> 
> From 071c316e9315f22a055d6713cc8cdcdc73642193 Mon Sep 17 00:00:00 2001
> From: Niels Laukens <niels@dest-unreach.be>
> Date: Sat, 31 May 2014 10:30:18 +0200
> Subject: [PATCH] drivers/media/rc/ir-nec-decode: fix phantom detect
> 
> The IR NEC decoder waited until the TRAILER_SPACE state to emit a
> keypress. Since the triggering 'space' event will only be sent at the
> beginning of the *next* IR-code, this is way to late.
> 
> This patch moves the processing to the TRAILER_PULSE state. Since we
> arrived here with a 'pulse' event, we know that the pulse has ended and
> thus that the space is there (as of yet with unknown length).
> 
> Signed-off-by: Niels Laukens <niels@dest-unreach.be>
> ---
>  drivers/media/rc/ir-nec-decoder.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
> index 35c42e5..955f99d 100644
> --- a/drivers/media/rc/ir-nec-decoder.c
> +++ b/drivers/media/rc/ir-nec-decoder.c
> @@ -148,16 +148,6 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  		if (!eq_margin(ev.duration, NEC_TRAILER_PULSE, NEC_UNIT / 2))
>  			break;
>  
> -		data->state = STATE_TRAILER_SPACE;
> -		return 0;
> -
> -	case STATE_TRAILER_SPACE:
> -		if (ev.pulse)
> -			break;
> -
> -		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
> -			break;
> -
>  		address     = bitrev8((data->bits >> 24) & 0xff);
>  		not_address = bitrev8((data->bits >> 16) & 0xff);
>  		command	    = bitrev8((data->bits >>  8) & 0xff);
> @@ -190,6 +180,16 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  			data->necx_repeat = true;
>  
>  		rc_keydown(dev, scancode, 0);
> +		data->state = STATE_TRAILER_SPACE;
> +		return 0;
> +
> +	case STATE_TRAILER_SPACE:
> +		if (ev.pulse)
> +			break;
> +
> +		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
> +			break;
> +
>  		data->state = STATE_INACTIVE;
>  		return 0;
>  	}
> 

Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[David Härdeman]

Hi,

the problem with triggering a keypress as soon as 32 bits have been 
received (i.e. before the trailing silence is detected) is that it would 
cause phantom keypresses on some other protocols (I'm thinking of NEC48, 
which does exist in the wild).

Now, the question is why the trailing silence isn't generated within a 
reasonable time. Which hardware decoder do you use?

//David

On 2014-06-11 10:06, Niels Laukens wrote:
> I have not received any response on this email... so I hope to bump 
> this
> thread back to the more active region in most people's in-boxes.
> 
> 
> On 2014-05-31 10:37, Niels Laukens wrote:
>> Hi,
>> 
>> I believe I've found a bug in the NEC decoder for InfraRed remote
>> controls. The problem manifests itself as an extra keypress that 
>> happens
>> when pushing different buttons in "rapid" succession.
>> 
>> I can reproduce the problem easily (but not always) by pushing DOWN, 
>> DOWN,
>> UP in "rapid" succession. I put "rapid" in quotes, because I don't 
>> have to
>> hurry in any way, it happens when I use it normally. Depending on the
>> duration of the presses, I get a number of repeats of DOWN. The bug is
>> that an additional DOWN keypress happens at the time that I press the 
>> UP
>> key (or so it seams).
>> 
>> Attached is kernel-debug.log, which contains the redacted and 
>> annotated
>> dmesg output, illustrating the problem described above. Especially 
>> note
>> lines 31-36 and 54-59, where more than 200ms pass between the end of 
>> the
>> IR-code and the actual emit of the keydown event.
>> 
>> 
>> I've debugged this issue, and believe I've found the cause: The 
>> keypress
>> is only emitted in state 5 (STATE_TRAILER_SPACE). This state is only
>> executed when the space after the message is received, i.e. when the
>> next pulse (of the next message) starts. It is only then that the 
>> length
>> of the space is known, and that ir_raw_event will fire an event.
>> 
>> The patch below addresses this issue. This is my first kernel patch.
>> I've tried to follow all guidelines that I could find, but might have
>> missed a few.
>> 
>> I've tested this patch with the out-of-tree TBS drivers (which seem to
>> be based on 3.13), and it solves the bug.
>> I've compared this TBS-version with the current master (1487385). 
>> There
>> are 8 (non-comment) lines that differ, none affect this patch. This
>> patch applies cleanly to the current master.
>> 
>> Regards,
>> Niels
>> 
>> 
>> 
>> 
>> From 071c316e9315f22a055d6713cc8cdcdc73642193 Mon Sep 17 00:00:00 2001
>> From: Niels Laukens <niels@dest-unreach.be>
>> Date: Sat, 31 May 2014 10:30:18 +0200
>> Subject: [PATCH] drivers/media/rc/ir-nec-decode: fix phantom detect
>> 
>> The IR NEC decoder waited until the TRAILER_SPACE state to emit a
>> keypress. Since the triggering 'space' event will only be sent at the
>> beginning of the *next* IR-code, this is way to late.
>> 
>> This patch moves the processing to the TRAILER_PULSE state. Since we
>> arrived here with a 'pulse' event, we know that the pulse has ended 
>> and
>> thus that the space is there (as of yet with unknown length).
>> 
>> Signed-off-by: Niels Laukens <niels@dest-unreach.be>
>> ---
>>  drivers/media/rc/ir-nec-decoder.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/media/rc/ir-nec-decoder.c 
>> b/drivers/media/rc/ir-nec-decoder.c
>> index 35c42e5..955f99d 100644
>> --- a/drivers/media/rc/ir-nec-decoder.c
>> +++ b/drivers/media/rc/ir-nec-decoder.c
>> @@ -148,16 +148,6 @@ static int ir_nec_decode(struct rc_dev *dev, 
>> struct ir_raw_event ev)
>>  		if (!eq_margin(ev.duration, NEC_TRAILER_PULSE, NEC_UNIT / 2))
>>  			break;
>> 
>> -		data->state = STATE_TRAILER_SPACE;
>> -		return 0;
>> -
>> -	case STATE_TRAILER_SPACE:
>> -		if (ev.pulse)
>> -			break;
>> -
>> -		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
>> -			break;
>> -
>>  		address     = bitrev8((data->bits >> 24) & 0xff);
>>  		not_address = bitrev8((data->bits >> 16) & 0xff);
>>  		command	    = bitrev8((data->bits >>  8) & 0xff);
>> @@ -190,6 +180,16 @@ static int ir_nec_decode(struct rc_dev *dev, 
>> struct ir_raw_event ev)
>>  			data->necx_repeat = true;
>> 
>>  		rc_keydown(dev, scancode, 0);
>> +		data->state = STATE_TRAILER_SPACE;
>> +		return 0;
>> +
>> +	case STATE_TRAILER_SPACE:
>> +		if (ev.pulse)
>> +			break;
>> +
>> +		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
>> +			break;
>> +
>>  		data->state = STATE_INACTIVE;
>>  		return 0;
>>  	}
>> 

Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[Niels Laukens]

On 2014-06-12 12:42, David Härdeman wrote:
> Hi,

Hi, thanks for the response


> the problem with triggering a keypress as soon as 32 bits have been
> received (i.e. before the trailing silence is detected)

Just for clarity: this patch does wait for the trailing silence. It does
NOT wait for the trailing silence to have (at least) a specific length.
(The pulse event is only fired after the pulse has ended, because the
length of the pulse needs to be known)


> is that it would
> cause phantom keypresses on some other protocols (I'm thinking of NEC48,
> which does exist in the wild).

I don't think the current code is able to decode NEC48. Is NEC48
recognizable in some other way than just being longer?

In that case, the alternative would be to start a timer when the
TRAILING_SPACE is entered, and trigger the key-event after, say 2 bit-times.


> Now, the question is why the trailing silence isn't generated within a
> reasonable time. Which hardware decoder do you use?

I use the IR receiver built in to the TBS6281 DVB-T tuner card. I also
have a TBS6982 DVB-S card, but I guess it's the same hardware.

It also depends on what "reasonable" means. I've found 300+ms, which is
unusable long.


Niels



> On 2014-06-11 10:06, Niels Laukens wrote:
>> I have not received any response on this email... so I hope to bump this
>> thread back to the more active region in most people's in-boxes.
>>
>>
>> On 2014-05-31 10:37, Niels Laukens wrote:
>>> Hi,
>>>
>>> I believe I've found a bug in the NEC decoder for InfraRed remote
>>> controls. The problem manifests itself as an extra keypress that happens
>>> when pushing different buttons in "rapid" succession.
>>>
>>> I can reproduce the problem easily (but not always) by pushing DOWN,
>>> DOWN,
>>> UP in "rapid" succession. I put "rapid" in quotes, because I don't
>>> have to
>>> hurry in any way, it happens when I use it normally. Depending on the
>>> duration of the presses, I get a number of repeats of DOWN. The bug is
>>> that an additional DOWN keypress happens at the time that I press the UP
>>> key (or so it seams).
>>>
>>> Attached is kernel-debug.log, which contains the redacted and annotated
>>> dmesg output, illustrating the problem described above. Especially note
>>> lines 31-36 and 54-59, where more than 200ms pass between the end of the
>>> IR-code and the actual emit of the keydown event.
>>>
>>>
>>> I've debugged this issue, and believe I've found the cause: The keypress
>>> is only emitted in state 5 (STATE_TRAILER_SPACE). This state is only
>>> executed when the space after the message is received, i.e. when the
>>> next pulse (of the next message) starts. It is only then that the length
>>> of the space is known, and that ir_raw_event will fire an event.
>>>
>>> The patch below addresses this issue. This is my first kernel patch.
>>> I've tried to follow all guidelines that I could find, but might have
>>> missed a few.
>>>
>>> I've tested this patch with the out-of-tree TBS drivers (which seem to
>>> be based on 3.13), and it solves the bug.
>>> I've compared this TBS-version with the current master (1487385). There
>>> are 8 (non-comment) lines that differ, none affect this patch. This
>>> patch applies cleanly to the current master.
>>>
>>> Regards,
>>> Niels
>>>
>>>
>>>
>>>
>>> From 071c316e9315f22a055d6713cc8cdcdc73642193 Mon Sep 17 00:00:00 2001
>>> From: Niels Laukens <niels@dest-unreach.be>
>>> Date: Sat, 31 May 2014 10:30:18 +0200
>>> Subject: [PATCH] drivers/media/rc/ir-nec-decode: fix phantom detect
>>>
>>> The IR NEC decoder waited until the TRAILER_SPACE state to emit a
>>> keypress. Since the triggering 'space' event will only be sent at the
>>> beginning of the *next* IR-code, this is way to late.
>>>
>>> This patch moves the processing to the TRAILER_PULSE state. Since we
>>> arrived here with a 'pulse' event, we know that the pulse has ended and
>>> thus that the space is there (as of yet with unknown length).
>>>
>>> Signed-off-by: Niels Laukens <niels@dest-unreach.be>
>>> ---
>>>  drivers/media/rc/ir-nec-decoder.c | 20 ++++++++++----------
>>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/ir-nec-decoder.c
>>> b/drivers/media/rc/ir-nec-decoder.c
>>> index 35c42e5..955f99d 100644
>>> --- a/drivers/media/rc/ir-nec-decoder.c
>>> +++ b/drivers/media/rc/ir-nec-decoder.c
>>> @@ -148,16 +148,6 @@ static int ir_nec_decode(struct rc_dev *dev,
>>> struct ir_raw_event ev)
>>>          if (!eq_margin(ev.duration, NEC_TRAILER_PULSE, NEC_UNIT / 2))
>>>              break;
>>>
>>> -        data->state = STATE_TRAILER_SPACE;
>>> -        return 0;
>>> -
>>> -    case STATE_TRAILER_SPACE:
>>> -        if (ev.pulse)
>>> -            break;
>>> -
>>> -        if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
>>> -            break;
>>> -
>>>          address     = bitrev8((data->bits >> 24) & 0xff);
>>>          not_address = bitrev8((data->bits >> 16) & 0xff);
>>>          command        = bitrev8((data->bits >>  8) & 0xff);
>>> @@ -190,6 +180,16 @@ static int ir_nec_decode(struct rc_dev *dev,
>>> struct ir_raw_event ev)
>>>              data->necx_repeat = true;
>>>
>>>          rc_keydown(dev, scancode, 0);
>>> +        data->state = STATE_TRAILER_SPACE;
>>> +        return 0;
>>> +
>>> +    case STATE_TRAILER_SPACE:
>>> +        if (ev.pulse)
>>> +            break;
>>> +
>>> +        if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
>>> +            break;
>>> +
>>>          data->state = STATE_INACTIVE;
>>>          return 0;
>>>      }
>>>

Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[David Härdeman]

On 2014-06-12 13:22, Niels Laukens wrote:
> On 2014-06-12 12:42, David Härdeman wrote:
>> Hi,
> 
> Hi, thanks for the response
> 
> 
>> the problem with triggering a keypress as soon as 32 bits have been
>> received (i.e. before the trailing silence is detected)
> 
> Just for clarity: this patch does wait for the trailing silence. It 
> does
> NOT wait for the trailing silence to have (at least) a specific length.
> (The pulse event is only fired after the pulse has ended, because the
> length of the pulse needs to be known)

True.

Interpret "trailing silence" above as "silence long enough to indicate 
end of message" :)

>> is that it would
>> cause phantom keypresses on some other protocols (I'm thinking of 
>> NEC48,
>> which does exist in the wild).
> 
> I don't think the current code is able to decode NEC48.

No, but it would still be nice not to interpret a NEC48 signal as NEC32.

> Is NEC48 recognizable in some other way than just being longer?

IIRC, no.

> In that case, the alternative would be to start a timer when the
> TRAILING_SPACE is entered, and trigger the key-event after, say 2 
> bit-times.

Another alternative is fix the driver to implement a timeout so that 
"unreasonable" values are not generated (I saw a 240550us space in your 
log).

That's basically what the filtering version of the raw interface does 
(cf. the use of dev->timeout in ir_raw_event_store_with_filter()).

And it's what most of the popular hardware does. For instance, the 
mceusb hardware will send a USB packet with timings including that 
trailing silence. And the decoder can only do their work once a packet 
has arrived (which will contain a number of samples). That also 
demonstrates a potential problem with your suggested approach (i.e. 
timings can be buffered so calls to the decoders are not necessarily 
"real-time").

>> Now, the question is why the trailing silence isn't generated within a
>> reasonable time. Which hardware decoder do you use?
> 
> I use the IR receiver built in to the TBS6281 DVB-T tuner card. I also
> have a TBS6982 DVB-S card, but I guess it's the same hardware.

Which driver?

> It also depends on what "reasonable" means. I've found 300+ms, which is
> unusable long.

Agreed...

//David

Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[Niels Laukens]

On 2014-06-12 13:51, David Härdeman wrote:
> On 2014-06-12 13:22, Niels Laukens wrote:
>> In that case, the alternative would be to start a timer when the
>> TRAILING_SPACE is entered, and trigger the key-event after, say 2
>> bit-times.
> 
> Another alternative is fix the driver to implement a timeout so that
> "unreasonable" values are not generated (I saw a 240550us space in your
> log).

OK, that sounds like a good way to solve this as well.
I'm very new to this subsystem, so I don't know what layer should
perform what function.


>>> Now, the question is why the trailing silence isn't generated
>>> within a reasonable time. Which hardware decoder do you use?
>>
>> I use the IR receiver built in to the TBS6281 DVB-T tuner card. I
>> also have a TBS6982 DVB-S card, but I guess it's the same hardware.
>
> Which driver?

I think it's the out-of-tree saa716x_tbs_dvb driver:

[    7.670565] input: saa716x IR (TurboSight TBS 6281) as
/devices/pci0000:00/0000:00:1c.0/0000:02:00.0/rc/rc0/input6
[    7.671156] rc0: saa716x IR (TurboSight TBS 6281) as
/devices/pci0000:00/0000:00:1c.0/0000:02:00.0/rc/rc0



> And it's what most of the popular hardware does.

So I'll have to rework this patch to function at this lower level, and
try to upstream it to TBS. Thank you for your time!


> For instance, the
> mceusb hardware will send a USB packet with timings including that
> trailing silence. And the decoder can only do their work once a packet
> has arrived (which will contain a number of samples). That also
> demonstrates a potential problem with your suggested approach (i.e.
> timings can be buffered so calls to the decoders are not necessarily
> "real-time").

I see what you mean, but I don't see how the proposed patch fails in
this sense. Or were you referring to the proposal of adding a timer at
the ir-nec-decoder level?


Niels

Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[David Härdeman]

On 2014-06-12 14:12, Niels Laukens wrote:
> On 2014-06-12 13:51, David Härdeman wrote:
>> On 2014-06-12 13:22, Niels Laukens wrote:
>>> In that case, the alternative would be to start a timer when the
>>> TRAILING_SPACE is entered, and trigger the key-event after, say 2
>>> bit-times.
>> 
>> Another alternative is fix the driver to implement a timeout so that
>> "unreasonable" values are not generated (I saw a 240550us space in 
>> your
>> log).
> 
> OK, that sounds like a good way to solve this as well.
> I'm very new to this subsystem, so I don't know what layer should
> perform what function.

I'm not 100% sure that would be the right fix, but I think so. Haven't 
looked at the driver.

>>>> Now, the question is why the trailing silence isn't generated
>>>> within a reasonable time. Which hardware decoder do you use?
>>> 
>>> I use the IR receiver built in to the TBS6281 DVB-T tuner card. I
>>> also have a TBS6982 DVB-S card, but I guess it's the same hardware.
>> 
>> Which driver?
> 
> I think it's the out-of-tree saa716x_tbs_dvb driver:
> 
> [    7.670565] input: saa716x IR (TurboSight TBS 6281) as
> /devices/pci0000:00/0000:00:1c.0/0000:02:00.0/rc/rc0/input6
> [    7.671156] rc0: saa716x IR (TurboSight TBS 6281) as
> /devices/pci0000:00/0000:00:1c.0/0000:02:00.0/rc/rc0

Could you paste the output from lsmod?

Where did you get the driver? Is it this one?
http://www.tbsdtv.com/download/document/common/tbs-linux-drivers_v140425.zip


>> And it's what most of the popular hardware does.
> 
> So I'll have to rework this patch to function at this lower level, and
> try to upstream it to TBS. Thank you for your time!
> 
> 
>> For instance, the
>> mceusb hardware will send a USB packet with timings including that
>> trailing silence. And the decoder can only do their work once a packet
>> has arrived (which will contain a number of samples). That also
>> demonstrates a potential problem with your suggested approach (i.e.
>> timings can be buffered so calls to the decoders are not necessarily
>> "real-time").
> 
> I see what you mean, but I don't see how the proposed patch fails in
> this sense. Or were you referring to the proposal of adding a timer at
> the ir-nec-decoder level?

Yes. The ir-nec-decoder timer doesn't know what the hardware is up to so 
it could timeout because it didn't get more data in time while at the 
same time the driver is buffering data...

Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress

[Niels Laukens]

On 2014-06-12 14:42, David Härdeman wrote:
> Could you paste the output from lsmod?

At the end of this mail.


> Where did you get the driver? Is it this one?
> http://www.tbsdtv.com/download/document/common/tbs-linux-drivers_v140425.zip

Yes, inside the zip is `linux-tbs-drivers.tar.bz2`, where the actual
drivers live. I believe we should be looking at
linux-tbs-drivers/linux/drivers/media/common/saa716x/saa716x_input.c

I think the interesting parts are at the end of the file, where the two
IRQ-handling functions are.

I don't understand why there is a 15ms timer before
ir_raw_event_handle() is called (the comment just says what the code
does, not why it does it). I assume this throttles the decoders a bit.
Does that make sense?


>From what I understand from the code, the hardware fires an interrupt
every time there is an edge, so I need to start a timer myself to call
ir_raw_event_set_idle() some time in the future. (And possible
re-schedule the ir_raw_event_handle() call as well). Does that sound right?

I've been reading ir-raw.c, but I don't see any timers there. I see the
timeout-check in ir_raw_event_store_with_filter(), but that will only
fire when called, and won't trigger by itself.

Thanks again for your time,
Niels




# lsmod
Module                  Size  Used by
rpcsec_gss_krb5        35573  1
nfsv4                 465643  2
tbsfe                  13023  2
nfsd                  280297  2
auth_rpcgss            59338  3 nfsd,rpcsec_gss_krb5
nfs_acl                12837  1 nfsd
nfs                   236636  2 nfsv4
lockd                  93977  2 nfs,nfsd
sunrpc                284404  10
nfs,nfsd,rpcsec_gss_krb5,auth_rpcgss,lockd,nfsv4,nfs_acl
fscache                63988  2 nfs,nfsv4
hid_generic            12548  0
usbhid                 52616  0
hid                   106148  2 hid_generic,usbhid
tbs62x1fe              55345  2
snd_hda_codec_hdmi     46207  2
snd_hda_codec_realtek    61438  1
ir_lirc_codec          12898  0
lirc_dev               19166  1 ir_lirc_codec
snd_seq_midi           13324  0
snd_seq_midi_event     14899  1 snd_seq_midi
snd_rawmidi            30144  1 snd_seq_midi
ir_mce_kbd_decoder     12845  0
ir_sony_decoder        12549  0
ir_jvc_decoder         12546  0
intel_rapl             18773  0
x86_pkg_temp_thermal    14205  0
ir_rc6_decoder         12546  0
intel_powerclamp       14705  0
kvm_intel             143060  0
kvm                   451511  1 kvm_intel
ir_rc5_decoder         12546  0
crct10dif_pclmul       14289  0
ir_nec_decoder         12546  0
crc32_pclmul           13113  0
ghash_clmulni_intel    13259  0
rc_tbs_nec             12502  0
aesni_intel            55624  0
snd_seq                61560  2 snd_seq_midi_event,snd_seq_midi
aes_x86_64             17131  1 aesni_intel
lrw                    13286  1 aesni_intel
gf128mul               14951  1 lrw
glue_helper            13990  1 aesni_intel
snd_hda_intel          52355  5
ablk_helper            13597  1 aesni_intel
cryptd                 20359  3 ghash_clmulni_intel,aesni_intel,ablk_helper
snd_hda_codec         192906  3
snd_hda_codec_realtek,snd_hda_codec_hdmi,snd_hda_intel
snd_hwdep              13602  1 snd_hda_codec
i915                  783485  1
serio_raw              13462  0
snd_pcm               102099  3
snd_hda_codec_hdmi,snd_hda_codec,snd_hda_intel
saa716x_tbs_dvb        76784  0
tbs6982fe              22408  1 saa716x_tbs_dvb
tbs6680fe              17791  1 saa716x_tbs_dvb
tbs6923fe              22408  1 saa716x_tbs_dvb
tbs6985se              17882  1 saa716x_tbs_dvb
tbs6928se              17884  1 saa716x_tbs_dvb
tbs6982se              22408  1 saa716x_tbs_dvb
tbs6991fe              17785  1 saa716x_tbs_dvb
tbs6618fe              17791  1 saa716x_tbs_dvb
saa716x_core           50899  1 saa716x_tbs_dvb
tbs6922fe              22478  1 saa716x_tbs_dvb
tbs6928fe              17785  1 saa716x_tbs_dvb
tbs6991se              17882  1 saa716x_tbs_dvb
tbs6290fe              50747  1 saa716x_tbs_dvb
stv090x                70414  1 saa716x_tbs_dvb
dvb_core              109932  2 saa716x_core,saa716x_tbs_dvb
rc_core                26933  11
ir_lirc_codec,ir_rc5_decoder,ir_nec_decoder,ir_sony_decoder,rc_tbs_nec,saa716x_tbs_dvb,ir_mce_kbd_decoder,ir_jvc_decoder,ir_rc6_decoder
snd_seq_device         14497  3 snd_seq,snd_rawmidi,snd_seq_midi
snd_page_alloc         18710  2 snd_pcm,snd_hda_intel
lpc_ich                21080  0
snd_timer              29482  2 snd_pcm,snd_seq
snd                    69238  21
snd_hda_codec_realtek,snd_hwdep,snd_timer,snd_hda_codec_hdmi,snd_pcm,snd_seq,snd_rawmidi,snd_hda_codec,snd_hda_intel,snd_seq_device,snd_seq_midi
nvidia              10675249  29
mei_me                 18627  0
mei                    82274  1 mei_me
soundcore              12680  1 snd
video                  19476  1 i915
drm_kms_helper         52758  1 i915
mac_hid                13205  0
drm                   302817  4 i915,drm_kms_helper,nvidia
i2c_algo_bit           13413  2 i915,saa716x_tbs_dvb
nct6775                55222  0
hwmon_vid              12783  1 nct6775
coretemp               13435  0
lp                     17759  0
parport                42348  1 lp
psmouse               102222  0
ahci                   25819  4
r8169                  67581  0
libahci                32168  1 ahci
mii                    13934  1 r8169