media/rc/ir-nec-decode : phantom keypress

From 071c316e9315f22a055d6713cc8cdcdc73642193 Mon Sep 17 00:00:00 2001

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 <[email protected]>
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 <[email protected]>

---
 drivers/media/rc/ir-nec-decoder.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/media/rc/ir-nec-decoder.c +10 -10

index 35c42e5..955f99d 100644
@@ -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;
 	}

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.

Quoted 100 lines · Niels Laukens

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 <[email protected]>
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 <[email protected]>

---
 drivers/media/rc/ir-nec-decoder.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/media/rc/ir-nec-decoder.c +0 -10

index 35c42e5..955f99d 100644
@@ -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);

drivers/media/rc/ir-nec-decoder.c +10 -0

 		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;

}

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?

Quoted 108 lines

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.

Quoted 103 lines · Niels Laukens

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 <[email protected]>
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 <[email protected]>

---
 drivers/media/rc/ir-nec-decoder.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/media/rc/ir-nec-decoder.c +0 -10

index 35c42e5..955f99d 100644
@@ -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);

drivers/media/rc/ir-nec-decoder.c +10 -0

@@ -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;

     }

Quoted 1 line · David Härdeman

Hi,

Hi, thanks for the response

Quoted 2 lines

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)

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines · Niels Laukens

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).

Quoted 5 lines

Quoted 2 lines

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?

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").

Quoted 5 lines · David Härdeman

Quoted 3 lines · Niels Laukens

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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 1 line

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!

Quoted 7 lines

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?

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

From what I understand from the code, the hardware fires an interrupt