* [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
@ 2014-05-31 8:37 Niels Laukens
2014-06-11 8:06 ` Niels Laukens
0 siblings, 1 reply; 8+ messages in thread
From: Niels Laukens @ 2014-05-31 8:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, linux-media
Cc: James Hogan, David Härdeman, Antti Seppälä
[-- 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
2014-05-31 8:37 [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress Niels Laukens
@ 2014-06-11 8:06 ` Niels Laukens
2014-06-12 10:42 ` David Härdeman
0 siblings, 1 reply; 8+ messages in thread
From: Niels Laukens @ 2014-06-11 8:06 UTC (permalink / raw)
To: Mauro Carvalho Chehab, linux-media
Cc: James Hogan, David Härdeman, Antti Seppälä
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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
2014-06-11 8:06 ` Niels Laukens
@ 2014-06-12 10:42 ` David Härdeman
2014-06-12 11:22 ` Niels Laukens
0 siblings, 1 reply; 8+ messages in thread
From: David Härdeman @ 2014-06-12 10:42 UTC (permalink / raw)
To: Niels Laukens
Cc: Mauro Carvalho Chehab, linux-media, James Hogan, Antti Seppälä
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;
>> }
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
2014-06-12 10:42 ` David Härdeman
@ 2014-06-12 11:22 ` Niels Laukens
2014-06-12 11:51 ` David Härdeman
0 siblings, 1 reply; 8+ messages in thread
From: Niels Laukens @ 2014-06-12 11:22 UTC (permalink / raw)
To: David Härdeman
Cc: Mauro Carvalho Chehab, linux-media, James Hogan, Antti Seppälä
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;
>>> }
>>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
2014-06-12 11:22 ` Niels Laukens
@ 2014-06-12 11:51 ` David Härdeman
2014-06-12 12:12 ` Niels Laukens
0 siblings, 1 reply; 8+ messages in thread
From: David Härdeman @ 2014-06-12 11:51 UTC (permalink / raw)
To: Niels Laukens
Cc: Mauro Carvalho Chehab, linux-media, James Hogan, Antti Seppälä
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
2014-06-12 11:51 ` David Härdeman
@ 2014-06-12 12:12 ` Niels Laukens
2014-06-12 12:42 ` David Härdeman
0 siblings, 1 reply; 8+ messages in thread
From: Niels Laukens @ 2014-06-12 12:12 UTC (permalink / raw)
To: David Härdeman
Cc: Mauro Carvalho Chehab, linux-media, James Hogan, Antti Seppälä
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
2014-06-12 12:12 ` Niels Laukens
@ 2014-06-12 12:42 ` David Härdeman
2014-06-12 15:19 ` Niels Laukens
0 siblings, 1 reply; 8+ messages in thread
From: David Härdeman @ 2014-06-12 12:42 UTC (permalink / raw)
To: Niels Laukens
Cc: Mauro Carvalho Chehab, linux-media, James Hogan, Antti Seppälä
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...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress
2014-06-12 12:42 ` David Härdeman
@ 2014-06-12 15:19 ` Niels Laukens
0 siblings, 0 replies; 8+ messages in thread
From: Niels Laukens @ 2014-06-12 15:19 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-06-12 15:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-31 8:37 [BUG & PATCH] media/rc/ir-nec-decode : phantom keypress Niels Laukens
2014-06-11 8:06 ` Niels Laukens
2014-06-12 10:42 ` David Härdeman
2014-06-12 11:22 ` Niels Laukens
2014-06-12 11:51 ` David Härdeman
2014-06-12 12:12 ` Niels Laukens
2014-06-12 12:42 ` David Härdeman
2014-06-12 15:19 ` Niels Laukens
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).