[PATCH 0/2] drivers/media/rc/ir-nec-decode : add toggle feature

[PATCH 0/2] drivers/media/rc/ir-nec-decode : add toggle feature

[Niels Laukens]

Hi,

The IR NEC protocol decoder does not handle repeated key presses very
well. It is regarded the same as a long key press, an thus triggers the
auto-repeat functionality, which is not what I expected.

The first patch solves the issue; the second patch fixes indentation
inside the (new) if-block. I kept these 2 separate to make it more clear
what the functional changes are, and which lines were only indented/reflown.

Niels

[PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature

[Niels Laukens]

>From 83aa9f9fa0eaf9eb8005af49f5ce93d24a0b9f2e Mon Sep 17 00:00:00 2001
From: Niels Laukens <niels.laukens@vrt.be>
Date: Thu, 19 Jun 2014 10:05:11 +0200
Subject: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature (1/2)

Made the distinction between repeated key presses, and a single long
press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but
has specific repeat-codes.

This patch identifies a repeat code, and skips the scancode calculations
and the rc_keydown() in that case. In the case of a full code, it makes
sure that the rc_keydown() is regarded as a new event by using the
toggle feature.

Signed-off-by: Niels Laukens <niels@dest-unreach.be>
---
 drivers/media/rc/ir-nec-decoder.c | 7 ++++++-
 drivers/media/rc/rc-core-priv.h   | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
index 35c42e5..1f2482a 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -79,6 +79,7 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			break;
 
 		data->count = 0;
+		data->nec_repeat = false;
 		data->state = STATE_HEADER_SPACE;
 		return 0;
 
@@ -93,6 +94,7 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			if (!dev->keypressed) {
 				IR_dprintk(1, "Discarding last key repeat: event after key up\n");
 			} else {
+				data->nec_repeat = true;
 				rc_repeat(dev);
 				IR_dprintk(1, "Repeat last key\n");
 				data->state = STATE_TRAILER_PULSE;
@@ -158,6 +160,7 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
 			break;
 
+		if (!data->nec_repeat) {
 		address     = bitrev8((data->bits >> 24) & 0xff);
 		not_address = bitrev8((data->bits >> 16) & 0xff);
 		command	    = bitrev8((data->bits >>  8) & 0xff);
@@ -189,7 +192,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		if (data->is_nec_x)
 			data->necx_repeat = true;
 
-		rc_keydown(dev, scancode, 0);
+		rc_keydown(dev, scancode, !dev->last_toggle);
+		}
+
 		data->state = STATE_INACTIVE;
 		return 0;
 	}
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index da536c9..37f3b74 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -49,6 +49,7 @@ struct ir_raw_event_ctrl {
 		u32 bits;
 		bool is_nec_x;
 		bool necx_repeat;
+		bool nec_repeat;
 	} nec;
 	struct rc5_dec {
 		int state;
-- 
1.8.5.2 (Apple Git-48)

[PATCH 2/2] drivers/media/rc/ir-nec-decode : add toggle feature (2/2)

[Niels Laukens]

>From a8fca4a8c37cb35ea4527a708a6894745f81c661 Mon Sep 17 00:00:00 2001
From: Niels Laukens <niels.laukens@vrt.be>
Date: Thu, 19 Jun 2014 10:06:00 +0200
Subject: [PATCH 2/2] drivers/media/rc/ir-nec-decode : add toggle feature (2/2)

Fixes indentation. Kept as separate patch to keep patch 1/2 more to the point.

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

diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
index 1f2482a..ff8b5d7 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -161,38 +161,41 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			break;
 
 		if (!data->nec_repeat) {
-		address     = bitrev8((data->bits >> 24) & 0xff);
-		not_address = bitrev8((data->bits >> 16) & 0xff);
-		command	    = bitrev8((data->bits >>  8) & 0xff);
-		not_command = bitrev8((data->bits >>  0) & 0xff);
-
-		if ((command ^ not_command) != 0xff) {
-			IR_dprintk(1, "NEC checksum error: received 0x%08x\n",
-				   data->bits);
-			send_32bits = true;
-		}
+			address     = bitrev8((data->bits >> 24) & 0xff);
+			not_address = bitrev8((data->bits >> 16) & 0xff);
+			command	    = bitrev8((data->bits >>  8) & 0xff);
+			not_command = bitrev8((data->bits >>  0) & 0xff);
+
+			if ((command ^ not_command) != 0xff) {
+				IR_dprintk(1, "NEC checksum error: received 0x%08x\n",
+					   data->bits);
+				send_32bits = true;
+			}
 
-		if (send_32bits) {
-			/* NEC transport, but modified protocol, used by at
-			 * least Apple and TiVo remotes */
-			scancode = data->bits;
-			IR_dprintk(1, "NEC (modified) scancode 0x%08x\n", scancode);
-		} else if ((address ^ not_address) != 0xff) {
-			/* Extended NEC */
-			scancode = address     << 16 |
-				   not_address <<  8 |
-				   command;
-			IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
-		} else {
-			/* Normal NEC */
-			scancode = address << 8 | command;
-			IR_dprintk(1, "NEC scancode 0x%04x\n", scancode);
-		}
+			if (send_32bits) {
+				/* NEC transport, but modified protocol, used
+				 * by at least Apple and TiVo remotes */
+				scancode = data->bits;
+				IR_dprintk(1, "NEC (modified) scancode 0x%08x\n",
+					   scancode);
+			} else if ((address ^ not_address) != 0xff) {
+				/* Extended NEC */
+				scancode = address     << 16 |
+					   not_address <<  8 |
+					   command;
+				IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n",
+					   scancode);
+			} else {
+				/* Normal NEC */
+				scancode = address << 8 | command;
+				IR_dprintk(1, "NEC scancode 0x%04x\n",
+					   scancode);
+			}
 
-		if (data->is_nec_x)
-			data->necx_repeat = true;
+			if (data->is_nec_x)
+				data->necx_repeat = true;
 
-		rc_keydown(dev, scancode, !dev->last_toggle);
+			rc_keydown(dev, scancode, !dev->last_toggle);
 		}
 
 		data->state = STATE_INACTIVE;
-- 1.8.5.2 (Apple Git-48) 

Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature

[David Härdeman]

On Thu, Jun 19, 2014 at 10:25:29AM +0200, Niels Laukens wrote:
>Made the distinction between repeated key presses, and a single long
>press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but
>has specific repeat-codes.

Not all NEC remotes use repeat codes. Some just transmit the full code
at fixed intervals...IIRC, Pioneer remotes is (was?) one example... 

>This patch identifies a repeat code, and skips the scancode calculations
>and the rc_keydown() in that case. In the case of a full code, it makes
>sure that the rc_keydown() is regarded as a new event by using the
>toggle feature.

Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature

[Niels Laukens]

On 2014-06-19 11:05, David Härdeman wrote:
> On Thu, Jun 19, 2014 at 10:25:29AM +0200, Niels Laukens wrote:
>> Made the distinction between repeated key presses, and a single long
>> press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but
>> has specific repeat-codes.
> 
> Not all NEC remotes use repeat codes. Some just transmit the full code
> at fixed intervals...IIRC, Pioneer remotes is (was?) one example... 

A way to cover this, is to make this mechanism optional, and
auto-activate as soon as a repeat code is seen. But that will only work
reliably with a single (type of) remote per system. Is this a better
solution?


Niels

Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature

[Mauro Carvalho Chehab]

Em Thu, 19 Jun 2014 13:59:49 +0200
Niels Laukens <niels@dest-unreach.be> escreveu:

> On 2014-06-19 11:05, David Härdeman wrote:
> > On Thu, Jun 19, 2014 at 10:25:29AM +0200, Niels Laukens wrote:
> >> Made the distinction between repeated key presses, and a single long
> >> press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but
> >> has specific repeat-codes.
> > 
> > Not all NEC remotes use repeat codes. Some just transmit the full code
> > at fixed intervals...IIRC, Pioneer remotes is (was?) one example... 
> 
> A way to cover this, is to make this mechanism optional, and
> auto-activate as soon as a repeat code is seen. But that will only work
> reliably with a single (type of) remote per system. Is this a better
> solution?

No, auto-activating is a very bad idea, as it means that any NEC remote,
if ever pressed in the room, will change the driver behavior. We should be
able to support both cases: the one with specific repeat codes and the
ones that don't support, at the same time.

Regards,
Mauro

Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature

[Niels Laukens]

On 2014-07-26 17:47, Mauro Carvalho Chehab wrote:
> Em Thu, 19 Jun 2014 13:59:49 +0200
> Niels Laukens <niels@dest-unreach.be> escreveu:
> 
>> On 2014-06-19 11:05, David Härdeman wrote:
>>> On Thu, Jun 19, 2014 at 10:25:29AM +0200, Niels Laukens wrote:
>>>> Made the distinction between repeated key presses, and a single long
>>>> press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but
>>>> has specific repeat-codes.
>>>
>>> Not all NEC remotes use repeat codes. Some just transmit the full code
>>> at fixed intervals...IIRC, Pioneer remotes is (was?) one example... 
>>
>> A way to cover this, is to make this mechanism optional, and
>> auto-activate as soon as a repeat code is seen. But that will only work
>> reliably with a single (type of) remote per system. Is this a better
>> solution?
> 
> No, auto-activating is a very bad idea, as it means that any NEC remote,
> if ever pressed in the room, will change the driver behavior. We should be
> able to support both cases: the one with specific repeat codes and the
> ones that don't support, at the same time.

I've changed the patch to still auto-activate, but "locally": a new
key-event will only be forced if the previously received code was a
repeat-code. This is means that non-compliant remotes will work as
before, and compliant remotes will work better: key's need to be pressed
at least for 1 repeat interval (nominally 100ms) for this feature to
activate.

I've tried this on my setup, and while it's better than the original, it
is, for me, still unacceptable. The previous version of this patch allowed
me to do a quick "3 times down". This version (and the original) doesn't.
So I'm not even posting this one.

Which makes me wonder: It seems as if it's impossible to differentiate
between the two types of remotes before the first repeat is sent, and by
then it's too late.  And it's not acceptable to latch the
repeat-code-capable flag in to the kernel. Would it be an option to make
this user-configurable? Either by splitting NEC into two variants, or by
using a parameter somehow? And if so: what is the preferred way?

Niels