* [PATCH 0/2] drivers/media/rc/ir-nec-decode : add toggle feature @ 2014-06-19 8:24 Niels Laukens 2014-06-19 8:25 ` [PATCH 1/2] " Niels Laukens 2014-06-19 8:25 ` [PATCH 2/2] drivers/media/rc/ir-nec-decode : add toggle feature (2/2) Niels Laukens 0 siblings, 2 replies; 7+ messages in thread From: Niels Laukens @ 2014-06-19 8:24 UTC (permalink / raw) To: Mauro Carvalho Chehab, linux-media Cc: James Hogan, David Härdeman, Antti Seppälä 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature 2014-06-19 8:24 [PATCH 0/2] drivers/media/rc/ir-nec-decode : add toggle feature Niels Laukens @ 2014-06-19 8:25 ` Niels Laukens 2014-06-19 9:05 ` David Härdeman 2014-06-19 8:25 ` [PATCH 2/2] drivers/media/rc/ir-nec-decode : add toggle feature (2/2) Niels Laukens 1 sibling, 1 reply; 7+ messages in thread From: Niels Laukens @ 2014-06-19 8:25 UTC (permalink / raw) To: Mauro Carvalho Chehab, linux-media Cc: James Hogan, David Härdeman, Antti Seppälä >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) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature 2014-06-19 8:25 ` [PATCH 1/2] " Niels Laukens @ 2014-06-19 9:05 ` David Härdeman 2014-06-19 11:59 ` Niels Laukens 0 siblings, 1 reply; 7+ messages in thread From: David Härdeman @ 2014-06-19 9:05 UTC (permalink / raw) To: Niels Laukens Cc: Mauro Carvalho Chehab, linux-media, James Hogan, Antti Seppälä 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature 2014-06-19 9:05 ` David Härdeman @ 2014-06-19 11:59 ` Niels Laukens 2014-07-26 15:47 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Niels Laukens @ 2014-06-19 11:59 UTC (permalink / raw) To: David Härdeman; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature 2014-06-19 11:59 ` Niels Laukens @ 2014-07-26 15:47 ` Mauro Carvalho Chehab 2014-07-26 16:58 ` Niels Laukens 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2014-07-26 15:47 UTC (permalink / raw) To: Niels Laukens; +Cc: David Härdeman, linux-media 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature 2014-07-26 15:47 ` Mauro Carvalho Chehab @ 2014-07-26 16:58 ` Niels Laukens 0 siblings, 0 replies; 7+ messages in thread From: Niels Laukens @ 2014-07-26 16:58 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: David Härdeman, linux-media 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] drivers/media/rc/ir-nec-decode : add toggle feature (2/2) 2014-06-19 8:24 [PATCH 0/2] drivers/media/rc/ir-nec-decode : add toggle feature Niels Laukens 2014-06-19 8:25 ` [PATCH 1/2] " Niels Laukens @ 2014-06-19 8:25 ` Niels Laukens 1 sibling, 0 replies; 7+ messages in thread From: Niels Laukens @ 2014-06-19 8:25 UTC (permalink / raw) To: Mauro Carvalho Chehab, linux-media Cc: James Hogan, David Härdeman, Antti Seppälä >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) ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-07-26 17:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-19 8:24 [PATCH 0/2] drivers/media/rc/ir-nec-decode : add toggle feature Niels Laukens 2014-06-19 8:25 ` [PATCH 1/2] " Niels Laukens 2014-06-19 9:05 ` David Härdeman 2014-06-19 11:59 ` Niels Laukens 2014-07-26 15:47 ` Mauro Carvalho Chehab 2014-07-26 16:58 ` Niels Laukens 2014-06-19 8:25 ` [PATCH 2/2] drivers/media/rc/ir-nec-decode : add toggle feature (2/2) Niels Laukens
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.