Re: [PATCH 1/2] rc-core: use the full 32 bits for NEC scancodes

2015-06-13 Thread David Härdeman 

On 2015-05-20 22:24, David Härdeman wrote:

On Thu, May 14, 2015 at 05:57:39PM -0300, Mauro Carvalho Chehab wrote:

Em Mon, 06 Apr 2015 13:23:08 +0200
David Härdeman da...@hardeman.nu escreveu:



+static inline enum rc_type guess_protocol(struct rc_dev *rdev)
+{
+   struct rc_map *rc_map = rdev-rc_map;
+
+   if (hweight64(rdev-enabled_protocols) == 1)
+   return rc_bitmap_to_type(rdev-enabled_protocols);
+   else if (hweight64(rdev-allowed_protocols) == 1)
+   return rc_bitmap_to_type(rdev-allowed_protocols);
+   else
+   return rc_map-rc_type;
+}



This function is the most important one to understand in order to
understand how the heuristics work...


+
+/**
+ * to_nec32() - helper function to try to convert misc NEC scancodes 
to NEC32

+ * @orig:  original scancode
+ * @return:NEC32 scancode
+ *
+ * This helper routine is used to provide backwards compatibility.
+ */
+static u64 to_nec32(u64 orig)
+{
+   u8 b3 = (u8)(orig  16);
+   u8 b2 = (u8)(orig   8);
+   u8 b1 = (u8)(orig   0);
+
+   if (orig = 0x)
+   /* Plain old NEC */
+   return b2  24 | ((u8)~b2)  16 |  b1  8 | ((u8)~b1);
+   else if (orig = 0xff)
+   /* NEC extended */
+   return b3  24 | b2  16 |  b1  8 | ((u8)~b1);
+   else
+   /* NEC32 */
+   return orig;
+}
+
+/**
  * ir_setkeycode() - set a keycode in the scancode-keycode table
  * @idev:  the struct input_dev device descriptor
  * @scancode:  the desired scancode
@@ -349,6 +392,9 @@ static int ir_setkeycode(struct input_dev *idev,
if (retval)
goto out;

+   if (guess_protocol(rdev) == 0
+   scancode = to_nec32(scancode);


This function can be called from userspace. I can't see how this would 
do

the right thing if more than one protocol is enabled.


+
index = ir_establish_scancode(rdev, rc_map, scancode, true);
if (index = rc_map-len) {
retval = -ENOMEM;
@@ -389,7 +435,10 @@ static int ir_setkeytable(struct rc_dev *dev,

for (i = 0; i  from-size; i++) {
index = ir_establish_scancode(dev, rc_map,
- from-scan[i].scancode, false);
+ from-rc_type == RC_TYPE_NEC ?
+ to_nec32(from-scan[i].scancode) :
+ from-scan[i].scancode,
+ false);
if (index = rc_map-len) {
rc = -ENOMEM;
break;
@@ -463,6 +512,8 @@ static int ir_getkeycode(struct input_dev *idev,
if (retval)
goto out;

+   if (guess_protocol(rdev) == RC_TYPE_NEC)
+   scancode = to_nec32(scancode);


This also can be called from userspace. It should not return different
scancodes for the same mapping if just NEC is enabled or if more 
protocols

are enabled.


There is no way to do this in a 100% backwards compatible way, that's
why the patch description uses the word heuristics.

I've tried different approaches (such as introducing and using a
kernel-internal RC_TYPE_ANY protocol for legacy ioctl() calls) but none
of them solve the problem 100%.

The current API is also broken, but in a different way. If you set a
scancode - keycode mapping right now using the current ioctl()s, you
can get different results with the exact same mapping but with 
different

RX hardware (which defeats the whole idea of having a kernel API for
remote controls...) even though there is enough information to do the
right thing (one example is already given in the patch comments)...that
is BAD.

The heuristics can also get it wrong...in slightly different situations
(e.g. if you load a hardware driver that supports nec and rc-5, but has
a rc-5 default keymap, then enable both nec and rc-5 from userspace, 
and

finally set keymap entries using nec scancodes...in which case they'll
be interpreted as rc-5 keymap scancodes).

So, we trade one kind of breakage for another...but the alternative 
kind

that I'm proposing here at least paves the way for the updated ioctls
which solve the ambiguity.

And distributions can make sure to ship updated userspace tools 
together

with an updated kernel (configuration tool updates together with kernel
updates is one of those special cases that e.g. LWN covers every now
and then).

I'm not saying the situation is ideal. But at least I'm trying to fix 
it

once and for all.

Do you have a better solution in mind other than to simply keep 
throwing

away all protocol information and ignoring scancode overlaps and
inconsistencies?


It wasn't a rhetorical question...this is an issue that needs to be 
fixed one way or another...do you have a better solution

Re: [PATCH 1/2] rc-core: use the full 32 bits for NEC scancodes

2015-05-20 Thread David Härdeman 
On Thu, May 14, 2015 at 05:57:39PM -0300, Mauro Carvalho Chehab wrote:
Em Mon, 06 Apr 2015 13:23:08 +0200
David Härdeman da...@hardeman.nu escreveu:

 Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
 and the nec decoder without any loss of functionality. At the same time
 it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
 removes any ambiguity.
 
 For example, before this patch, consider these two NEC messages:
 NEC16 message to address 0x05, command 0x03
 NEC24 message to address 0x0005, command 0x03
 
 They'll both have scancode 0x0503, and there's no way to tell which
 message was received.
 
 In order to maintain backwards compatibility, some heuristics are added
 in rc-main.c to convert scancodes to NEC32 as necessary when userspace
 adds entries to the keytable using the regular input ioctls.
 
 No conversion will be made for the newer rc_keymap_entry based ioctls
 (see the next patch).
 
 Signed-off-by: David Härdeman da...@hardeman.nu

Checkpatch has something to say about this patch:

WARNING: else is not generally useful after a break or return
#140: FILE: drivers/media/rc/rc-main.c:357:
+  return b3  24 | b2  16 |  b1  8 | ((u8)~b1);
+  else

That seems like a bogus warning?...the return line is after an else if...

WARNING: line over 80 characters
#259: FILE: drivers/media/usb/dvb-usb-v2/af9035.c:1778:
+ RC_SCANCODE_NEC32(b[0]  24 | b[1]  16 | b[2]  8 | 
b[3]),

That file isn't 80 chars per line clean anyway (and 80 char lines do
not seem to be a hard rule?). Anyway, I've changed it in my local
version...

ERROR: space prohibited after that open parenthesis '('
#479: FILE: include/media/rc-map.h:79:
+  ((( (addr)  0xff)  24) | \

ERROR: space prohibited after that open parenthesis '('
#481: FILE: include/media/rc-map.h:81:
+   (( (cmd)   0xff)  8 ) | \

ERROR: space prohibited before that close parenthesis ')'
#481: FILE: include/media/rc-map.h:81:
+   (( (cmd)   0xff)  8 ) | \

ERROR: space prohibited before that close parenthesis ')'
#482: FILE: include/media/rc-map.h:82:
+   ((~(cmd)   0xff)  0 ))

ERROR: space prohibited after that open parenthesis '('
#484: FILE: include/media/rc-map.h:84:
+  ((( (addr)  0x)  16) | \

ERROR: space prohibited after that open parenthesis '('
#485: FILE: include/media/rc-map.h:85:
+   (( (cmd)   0x00ff)  8)  | \

I don't think fixing any of these help readability, but I've changed
them in my local version.

 ---
  drivers/media/rc/ir-nec-decoder.c|   26 ++
  drivers/media/rc/rc-main.c   |   54 
 +-
  drivers/media/usb/dvb-usb-v2/af9015.c|   22 ++--
  drivers/media/usb/dvb-usb-v2/af9035.c|   25 +++---
  drivers/media/usb/dvb-usb-v2/az6007.c|   16 ++---
  drivers/media/usb/dvb-usb-v2/rtl28xxu.c  |   20 +++
  drivers/media/usb/dvb-usb/dib0700_core.c |   24 +++--
  drivers/media/usb/em28xx/em28xx-input.c  |   37 +
  include/media/rc-map.h   |   16 +++--
  9 files changed, 102 insertions(+), 138 deletions(-)
 
 diff --git a/drivers/media/rc/ir-nec-decoder.c 
 b/drivers/media/rc/ir-nec-decoder.c
 index 7b81fec..16907c1 100644
 --- a/drivers/media/rc/ir-nec-decoder.c
 +++ b/drivers/media/rc/ir-nec-decoder.c
 @@ -50,7 +50,6 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
 ir_raw_event ev)
  struct nec_dec *data = dev-raw-nec;
  u32 scancode;
  u8 address, not_address, command, not_command;
 -bool send_32bits = false;
  
  if (!(dev-enabled_protocols  RC_BIT_NEC))
  return 0;
 @@ -163,28 +162,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
 ir_raw_event ev)
  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);
 -}
 +scancode = RC_SCANCODE_NEC32(address  24 |

Re: [PATCH 1/2] rc-core: use the full 32 bits for NEC scancodes

2015-05-14 Thread Mauro Carvalho Chehab 
Em Mon, 06 Apr 2015 13:23:08 +0200
David Härdeman da...@hardeman.nu escreveu:

 Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
 and the nec decoder without any loss of functionality. At the same time
 it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
 removes any ambiguity.
 
 For example, before this patch, consider these two NEC messages:
 NEC16 message to address 0x05, command 0x03
 NEC24 message to address 0x0005, command 0x03
 
 They'll both have scancode 0x0503, and there's no way to tell which
 message was received.
 
 In order to maintain backwards compatibility, some heuristics are added
 in rc-main.c to convert scancodes to NEC32 as necessary when userspace
 adds entries to the keytable using the regular input ioctls.
 
 No conversion will be made for the newer rc_keymap_entry based ioctls
 (see the next patch).
 
 Signed-off-by: David Härdeman da...@hardeman.nu

Checkpatch has something to say about this patch:

WARNING: else is not generally useful after a break or return
#140: FILE: drivers/media/rc/rc-main.c:357:
+   return b3  24 | b2  16 |  b1  8 | ((u8)~b1);
+   else

WARNING: line over 80 characters
#259: FILE: drivers/media/usb/dvb-usb-v2/af9035.c:1778:
+  RC_SCANCODE_NEC32(b[0]  24 | b[1]  16 | b[2]  8 | 
b[3]),

ERROR: space prohibited after that open parenthesis '('
#479: FILE: include/media/rc-map.h:79:
+   ((( (addr)  0xff)  24) | \

ERROR: space prohibited after that open parenthesis '('
#481: FILE: include/media/rc-map.h:81:
+(( (cmd)   0xff)  8 ) | \

ERROR: space prohibited before that close parenthesis ')'
#481: FILE: include/media/rc-map.h:81:
+(( (cmd)   0xff)  8 ) | \

ERROR: space prohibited before that close parenthesis ')'
#482: FILE: include/media/rc-map.h:82:
+((~(cmd)   0xff)  0 ))

ERROR: space prohibited after that open parenthesis '('
#484: FILE: include/media/rc-map.h:84:
+   ((( (addr)  0x)  16) | \

ERROR: space prohibited after that open parenthesis '('
#485: FILE: include/media/rc-map.h:85:
+(( (cmd)   0x00ff)  8)  | \




 ---
  drivers/media/rc/ir-nec-decoder.c|   26 ++
  drivers/media/rc/rc-main.c   |   54 
 +-
  drivers/media/usb/dvb-usb-v2/af9015.c|   22 ++--
  drivers/media/usb/dvb-usb-v2/af9035.c|   25 +++---
  drivers/media/usb/dvb-usb-v2/az6007.c|   16 ++---
  drivers/media/usb/dvb-usb-v2/rtl28xxu.c  |   20 +++
  drivers/media/usb/dvb-usb/dib0700_core.c |   24 +++--
  drivers/media/usb/em28xx/em28xx-input.c  |   37 +
  include/media/rc-map.h   |   16 +++--
  9 files changed, 102 insertions(+), 138 deletions(-)
 
 diff --git a/drivers/media/rc/ir-nec-decoder.c 
 b/drivers/media/rc/ir-nec-decoder.c
 index 7b81fec..16907c1 100644
 --- a/drivers/media/rc/ir-nec-decoder.c
 +++ b/drivers/media/rc/ir-nec-decoder.c
 @@ -50,7 +50,6 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
 ir_raw_event ev)
   struct nec_dec *data = dev-raw-nec;
   u32 scancode;
   u8 address, not_address, command, not_command;
 - bool send_32bits = false;
  
   if (!(dev-enabled_protocols  RC_BIT_NEC))
   return 0;
 @@ -163,28 +162,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
 ir_raw_event ev)
   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);
 - }
 + scancode = RC_SCANCODE_NEC32(address  24 | not_address  16 |
 +  command  8  | not_command);
 + IR_dprintk(1, NEC scancode 0x%08x\n, scancode);
  
   if (data-is_nec_x)
   data-necx_repeat = true;
 diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
 index d068c4e..3379379 100644
 ---