Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On 29/03/14 16:11, David Härdeman wrote:
Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
and the nec decoder without any loss of functionality.
In order to maintain backwards compatibility, some heuristics are added
in rc-main.c to convert scancodes to NEC32 as necessary.
I plan to introduce a different ioctl later which makes the protocol
explicit (and which expects all NEC scancodes to be 32 bit, thereby
removing the need for guesswork).
Signed-off-by: David Härdeman da...@hardeman.nu
---
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c
b/drivers/media/rc/img-ir/img-ir-nec.c
index 40ee844..133ea45 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -5,42 +5,20 @@
*/
#include img-ir-hw.h
-#include linux/bitrev.h
/* Convert NEC data to a scancode */
static int img_ir_nec_scancode(int len, u64 raw, enum rc_type *protocol,
u32 *scancode, u64 enabled_protocols)
{
- unsigned int addr, addr_inv, data, data_inv;
/* a repeat code has no data */
if (!len)
return IMG_IR_REPEATCODE;
+
if (len != 32)
return -EINVAL;
- /* raw encoding: ddDDaaAA */
- addr = (raw 0) 0xff;
- addr_inv = (raw 8) 0xff;
- data = (raw 16) 0xff;
- data_inv = (raw 24) 0xff;
- if ((data_inv ^ data) != 0xff) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: AAaaDDdd (LSBit first) */
- *scancode = bitrev8(addr) 24 |
- bitrev8(addr_inv) 16 |
- bitrev8(data) 8 |
- bitrev8(data_inv);
- } else if ((addr_inv ^ addr) != 0xff) {
- /* Extended NEC */
- /* scan encoding: AAaaDD */
- *scancode = addr 16 |
- addr_inv 8 |
- data;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- *scancode = addr 8 |
- data;
- }
+
+ /* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */
+ *scancode = swab32((u32)raw);
What's the point of the byte swapping?
Surely the most natural NEC encoding would just treat it as a single
32-bit (LSBit first) field rather than 4 8-bit fields that needs swapping.
Cheers
James
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On 2014-03-31 11:44, James Hogan wrote:
On 29/03/14 16:11, David Härdeman wrote:
Using the full 32 bits for all kinds of NEC scancodes simplifies
rc-core
and the nec decoder without any loss of functionality.
In order to maintain backwards compatibility, some heuristics are
added
in rc-main.c to convert scancodes to NEC32 as necessary.
I plan to introduce a different ioctl later which makes the protocol
explicit (and which expects all NEC scancodes to be 32 bit, thereby
removing the need for guesswork).
Signed-off-by: David Härdeman da...@hardeman.nu
---
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c
b/drivers/media/rc/img-ir/img-ir-nec.c
index 40ee844..133ea45 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -5,42 +5,20 @@
*/
#include img-ir-hw.h
-#include linux/bitrev.h
/* Convert NEC data to a scancode */
static int img_ir_nec_scancode(int len, u64 raw, enum rc_type
*protocol,
u32 *scancode, u64 enabled_protocols)
{
- unsigned int addr, addr_inv, data, data_inv;
/* a repeat code has no data */
if (!len)
return IMG_IR_REPEATCODE;
+
if (len != 32)
return -EINVAL;
- /* raw encoding: ddDDaaAA */
- addr = (raw 0) 0xff;
- addr_inv = (raw 8) 0xff;
- data = (raw 16) 0xff;
- data_inv = (raw 24) 0xff;
- if ((data_inv ^ data) != 0xff) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: AAaaDDdd (LSBit first) */
- *scancode = bitrev8(addr) 24 |
- bitrev8(addr_inv) 16 |
- bitrev8(data) 8 |
- bitrev8(data_inv);
- } else if ((addr_inv ^ addr) != 0xff) {
- /* Extended NEC */
- /* scan encoding: AAaaDD */
- *scancode = addr 16 |
- addr_inv 8 |
- data;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- *scancode = addr 8 |
- data;
- }
+
+ /* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */
+ *scancode = swab32((u32)raw);
What's the point of the byte swapping?
Surely the most natural NEC encoding would just treat it as a single
32-bit (LSBit first) field rather than 4 8-bit fields that needs
swapping.
Thanks for having a look at the patches, I agree with your comments on
the other patches (and I have to respin some of them because I missed
two drivers), but the comments to this patch confuses me a bit.
That the NEC data is transmitted as 32 bits encoded with LSB bit order
within each byte is AFAIK just about the only thing that all
sources/documentation of the protocal can agree on (so bitrev:ing the
bits within each byte makes sense, unless the hardware has done it
already).
As for the byte order, AAaaDDdd corresponds to the transmission order
and seems to be what most drivers expect/use for their RX data.
Are you suggesting that rc-core should standardize on ddDDaaAA order?
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On 31/03/14 11:19, David Härdeman wrote:
On 2014-03-31 11:44, James Hogan wrote:
On 29/03/14 16:11, David Härdeman wrote:
Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
and the nec decoder without any loss of functionality.
In order to maintain backwards compatibility, some heuristics are added
in rc-main.c to convert scancodes to NEC32 as necessary.
I plan to introduce a different ioctl later which makes the protocol
explicit (and which expects all NEC scancodes to be 32 bit, thereby
removing the need for guesswork).
Signed-off-by: David Härdeman da...@hardeman.nu
---
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c
b/drivers/media/rc/img-ir/img-ir-nec.c
index 40ee844..133ea45 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -5,42 +5,20 @@
*/
#include img-ir-hw.h
-#include linux/bitrev.h
/* Convert NEC data to a scancode */
static int img_ir_nec_scancode(int len, u64 raw, enum rc_type
*protocol,
u32 *scancode, u64 enabled_protocols)
{
-unsigned int addr, addr_inv, data, data_inv;
/* a repeat code has no data */
if (!len)
return IMG_IR_REPEATCODE;
+
if (len != 32)
return -EINVAL;
-/* raw encoding: ddDDaaAA */
-addr = (raw 0) 0xff;
-addr_inv = (raw 8) 0xff;
-data = (raw 16) 0xff;
-data_inv = (raw 24) 0xff;
-if ((data_inv ^ data) != 0xff) {
-/* 32-bit NEC (used by Apple and TiVo remotes) */
-/* scan encoding: AAaaDDdd (LSBit first) */
-*scancode = bitrev8(addr) 24 |
-bitrev8(addr_inv) 16 |
-bitrev8(data) 8 |
-bitrev8(data_inv);
-} else if ((addr_inv ^ addr) != 0xff) {
-/* Extended NEC */
-/* scan encoding: AAaaDD */
-*scancode = addr 16 |
-addr_inv 8 |
-data;
-} else {
-/* Normal NEC */
-/* scan encoding: AADD */
-*scancode = addr 8 |
-data;
-}
+
+/* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */
+*scancode = swab32((u32)raw);
What's the point of the byte swapping?
Surely the most natural NEC encoding would just treat it as a single
32-bit (LSBit first) field rather than 4 8-bit fields that needs
swapping.
Thanks for having a look at the patches, I agree with your comments on
the other patches (and I have to respin some of them because I missed
two drivers), but the comments to this patch confuses me a bit.
That the NEC data is transmitted as 32 bits encoded with LSB bit order
within each byte is AFAIK just about the only thing that all
sources/documentation of the protocal can agree on (so bitrev:ing the
bits within each byte makes sense, unless the hardware has done it
already).
Agreed (in the case of img-ir there's a bit orientation setting which
ensures that the u64 raw has the correct bit order, in the case of NEC
the first bit received goes in the lowest order bit of the raw data).
As for the byte order, AAaaDDdd corresponds to the transmission order
and seems to be what most drivers expect/use for their RX data.
AAaaDDdd is big endian rendering, no? (like %08x)
If it should be interpreted as LSBit first, then the first bits received
should go in the low bits of the scancode, and by extension the first
bytes received in the low bytes of the scancode, i.e. at the end of the
inherently big-endian hexadecimal rendering of the scancode.
Are you suggesting that rc-core should standardize on ddDDaaAA order?
Yes (where ddDDaaAA means something like scancode
0x(~cmd)(cmd)(~addr)(addr))
This would mean that if the data is put in the right bit order (first
bit received in BIT(0), last bit received in BIT(31)), then the scancode
= raw, and if the data is received in the reverse bit order (like the
raw decoder, shifting the data left and inserting the last bit in
BIT(0)) then the scancode = bitrev32(raw).
Have I missed something?
Cheers
James
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
Hi David,
Em Mon, 31 Mar 2014 12:19:10 +0200
David Härdeman da...@hardeman.nu escreveu:
On 2014-03-31 11:44, James Hogan wrote:
On 29/03/14 16:11, David Härdeman wrote:
Using the full 32 bits for all kinds of NEC scancodes simplifies
rc-core
and the nec decoder without any loss of functionality.
In order to maintain backwards compatibility, some heuristics are
added
in rc-main.c to convert scancodes to NEC32 as necessary.
I plan to introduce a different ioctl later which makes the protocol
explicit (and which expects all NEC scancodes to be 32 bit, thereby
removing the need for guesswork).
Signed-off-by: David Härdeman da...@hardeman.nu
---
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c
b/drivers/media/rc/img-ir/img-ir-nec.c
index 40ee844..133ea45 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -5,42 +5,20 @@
*/
#include img-ir-hw.h
-#include linux/bitrev.h
/* Convert NEC data to a scancode */
static int img_ir_nec_scancode(int len, u64 raw, enum rc_type
*protocol,
u32 *scancode, u64 enabled_protocols)
{
- unsigned int addr, addr_inv, data, data_inv;
/* a repeat code has no data */
if (!len)
return IMG_IR_REPEATCODE;
+
if (len != 32)
return -EINVAL;
- /* raw encoding: ddDDaaAA */
- addr = (raw 0) 0xff;
- addr_inv = (raw 8) 0xff;
- data = (raw 16) 0xff;
- data_inv = (raw 24) 0xff;
- if ((data_inv ^ data) != 0xff) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: AAaaDDdd (LSBit first) */
- *scancode = bitrev8(addr) 24 |
- bitrev8(addr_inv) 16 |
- bitrev8(data) 8 |
- bitrev8(data_inv);
- } else if ((addr_inv ^ addr) != 0xff) {
- /* Extended NEC */
- /* scan encoding: AAaaDD */
- *scancode = addr 16 |
- addr_inv 8 |
- data;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- *scancode = addr 8 |
- data;
- }
+
+ /* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */
+ *scancode = swab32((u32)raw);
What's the point of the byte swapping?
Surely the most natural NEC encoding would just treat it as a single
32-bit (LSBit first) field rather than 4 8-bit fields that needs
swapping.
Thanks for having a look at the patches, I agree with your comments on
the other patches (and I have to respin some of them because I missed
two drivers), but the comments to this patch confuses me a bit.
That the NEC data is transmitted as 32 bits encoded with LSB bit order
within each byte is AFAIK just about the only thing that all
sources/documentation of the protocal can agree on (so bitrev:ing the
bits within each byte makes sense, unless the hardware has done it
already).
As for the byte order, AAaaDDdd corresponds to the transmission order
and seems to be what most drivers expect/use for their RX data.
Are you suggesting that rc-core should standardize on ddDDaaAA order?
Let's better name this, as AAaaDDdd implies that:
aa = ~AA
dd = ~DD
As described at the NEC protocol.
The 24 or 32 bits variation is actually a violation of the NEC protocol.
What some IRs actually provide is:
xxyyADDdd (24 bits NEC)
where:
Address = yyxx
Data = DD
As described as Extended NEC protocol at:
http://www.sbprojects.com/knowledge/ir/nec.php
or:
xxyyADDzz (32 bits NEC)
where:
Address = zzxxyy
Data = DD
Also, currently, there's just one IR table with 32 bits nec:
rc-tivo.c, used by the mceusb driver.
Well, changing the NEC decoders to always send a 32 bits code has
several issues:
1) It makes the normal NEC protocol as an exception, and not as a
rule;
2) It breaks all in-kernel tables for 16 bits and 24 bits NEC.
As already said, currently, there's just one driver using 32
bits NEC, and just for one IR type (RC_MAP_TIVO);
3) It causes regressions to userspace, as userspace tables won't
work anymore;
4) Your to_nec32() macro will break support for 24-bits IRs
shipped with devices that can only provide 16 bits.
In order to explain (4), let's see what happens when a 24-bits
NEC code is received by a in-hardware decoder.
There are a wide range of Chinese IR devices shipped with widely
used media hardware that produce a 24-bit NEC code. One of the
most popular of such manufacturers use the address = 0x866b
(btw, the get_key_beholdm6xx() function at saa7134 driver seems
to be wrong, as the keytables for behold device has the address of
this vendor mapped as 0x6b86).
The way those codes are handled inside each in-hardware NEC
decoder are
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On 2014-03-31 14:14, Mauro Carvalho Chehab wrote:
Em Mon, 31 Mar 2014 12:19:10 +0200
David Härdeman da...@hardeman.nu escreveu:
On 2014-03-31 11:44, James Hogan wrote:
On 29/03/14 16:11, David Härdeman wrote:
Using the full 32 bits for all kinds of NEC scancodes simplifies
rc-core
and the nec decoder without any loss of functionality.
In order to maintain backwards compatibility, some heuristics are
added
in rc-main.c to convert scancodes to NEC32 as necessary.
I plan to introduce a different ioctl later which makes the protocol
explicit (and which expects all NEC scancodes to be 32 bit, thereby
removing the need for guesswork).
Signed-off-by: David Härdeman da...@hardeman.nu
---
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c
b/drivers/media/rc/img-ir/img-ir-nec.c
index 40ee844..133ea45 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -5,42 +5,20 @@
*/
#include img-ir-hw.h
-#include linux/bitrev.h
/* Convert NEC data to a scancode */
static int img_ir_nec_scancode(int len, u64 raw, enum rc_type
*protocol,
u32 *scancode, u64 enabled_protocols)
{
- unsigned int addr, addr_inv, data, data_inv;
/* a repeat code has no data */
if (!len)
return IMG_IR_REPEATCODE;
+
if (len != 32)
return -EINVAL;
- /* raw encoding: ddDDaaAA */
- addr = (raw 0) 0xff;
- addr_inv = (raw 8) 0xff;
- data = (raw 16) 0xff;
- data_inv = (raw 24) 0xff;
- if ((data_inv ^ data) != 0xff) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: AAaaDDdd (LSBit first) */
- *scancode = bitrev8(addr) 24 |
- bitrev8(addr_inv) 16 |
- bitrev8(data) 8 |
- bitrev8(data_inv);
- } else if ((addr_inv ^ addr) != 0xff) {
- /* Extended NEC */
- /* scan encoding: AAaaDD */
- *scancode = addr 16 |
- addr_inv 8 |
- data;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- *scancode = addr 8 |
- data;
- }
+
+ /* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */
+ *scancode = swab32((u32)raw);
What's the point of the byte swapping?
Surely the most natural NEC encoding would just treat it as a single
32-bit (LSBit first) field rather than 4 8-bit fields that needs
swapping.
Thanks for having a look at the patches, I agree with your comments on
the other patches (and I have to respin some of them because I missed
two drivers), but the comments to this patch confuses me a bit.
That the NEC data is transmitted as 32 bits encoded with LSB bit order
within each byte is AFAIK just about the only thing that all
sources/documentation of the protocal can agree on (so bitrev:ing the
bits within each byte makes sense, unless the hardware has done it
already).
As for the byte order, AAaaDDdd corresponds to the transmission order
and seems to be what most drivers expect/use for their RX data.
Are you suggesting that rc-core should standardize on ddDDaaAA order?
Let's better name this, as AAaaDDdd implies that:
aa = ~AA
dd = ~DD
As described at the NEC protocol.
I really don't think James and I had any trouble understanding each
other :)
The 24 or 32 bits variation is actually a violation of the NEC
protocol.
Violation is a misnomer. NEC created the 24 bit version, it's an
extension. Many companies (such as your employer :)) have created
further variations.
What some IRs actually provide is:
xxyyADDdd (24 bits NEC)
where:
Address = yyxx
Data = DD
As described as Extended NEC protocol at:
http://www.sbprojects.com/knowledge/ir/nec.php
or:
xxyyADDzz (32 bits NEC)
where:
Address = zzxxyy
Data = DD
No need to explain the protocol to me.
Also, currently, there's just one IR table with 32 bits nec:
rc-tivo.c, used by the mceusb driver.
Yes, I know.
Well, changing the NEC decoders to always send a 32 bits code has
several issues:
1) It makes the normal NEC protocol as an exception, and not as a
rule;
It's not an exception. I just makes all 32 bits explicit.
And the lack of that explicit information currently makes the scancode
ambiguous. Right now if I give you a NEC scancode of 0xff00 (like we
give to userspace with the EV_SCAN event), you can't tell what it
means...it could, for example, be a 32 bit code of 0xff00...
2) It breaks all in-kernel tables for 16 bits and 24 bits NEC.
As already said, currently, there's just one driver using 32
bits NEC, and just for one IR type (RC_MAP_TIVO);
No, the proposed patch
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
Em Mon, 31 Mar 2014 14:58:10 +0200
David Härdeman da...@hardeman.nu escreveu:
On 2014-03-31 14:14, Mauro Carvalho Chehab wrote:
Em Mon, 31 Mar 2014 12:19:10 +0200
David Härdeman da...@hardeman.nu escreveu:
On 2014-03-31 11:44, James Hogan wrote:
On 29/03/14 16:11, David Härdeman wrote:
Using the full 32 bits for all kinds of NEC scancodes simplifies
rc-core
and the nec decoder without any loss of functionality.
In order to maintain backwards compatibility, some heuristics are
added
in rc-main.c to convert scancodes to NEC32 as necessary.
I plan to introduce a different ioctl later which makes the protocol
explicit (and which expects all NEC scancodes to be 32 bit, thereby
removing the need for guesswork).
Signed-off-by: David Härdeman da...@hardeman.nu
---
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c
b/drivers/media/rc/img-ir/img-ir-nec.c
index 40ee844..133ea45 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -5,42 +5,20 @@
*/
#include img-ir-hw.h
-#include linux/bitrev.h
/* Convert NEC data to a scancode */
static int img_ir_nec_scancode(int len, u64 raw, enum rc_type
*protocol,
u32 *scancode, u64 enabled_protocols)
{
- unsigned int addr, addr_inv, data, data_inv;
/* a repeat code has no data */
if (!len)
return IMG_IR_REPEATCODE;
+
if (len != 32)
return -EINVAL;
- /* raw encoding: ddDDaaAA */
- addr = (raw 0) 0xff;
- addr_inv = (raw 8) 0xff;
- data = (raw 16) 0xff;
- data_inv = (raw 24) 0xff;
- if ((data_inv ^ data) != 0xff) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: AAaaDDdd (LSBit first) */
- *scancode = bitrev8(addr) 24 |
- bitrev8(addr_inv) 16 |
- bitrev8(data) 8 |
- bitrev8(data_inv);
- } else if ((addr_inv ^ addr) != 0xff) {
- /* Extended NEC */
- /* scan encoding: AAaaDD */
- *scancode = addr 16 |
- addr_inv 8 |
- data;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- *scancode = addr 8 |
- data;
- }
+
+ /* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */
+ *scancode = swab32((u32)raw);
What's the point of the byte swapping?
Surely the most natural NEC encoding would just treat it as a single
32-bit (LSBit first) field rather than 4 8-bit fields that needs
swapping.
Thanks for having a look at the patches, I agree with your comments on
the other patches (and I have to respin some of them because I missed
two drivers), but the comments to this patch confuses me a bit.
That the NEC data is transmitted as 32 bits encoded with LSB bit order
within each byte is AFAIK just about the only thing that all
sources/documentation of the protocal can agree on (so bitrev:ing the
bits within each byte makes sense, unless the hardware has done it
already).
As for the byte order, AAaaDDdd corresponds to the transmission order
and seems to be what most drivers expect/use for their RX data.
Are you suggesting that rc-core should standardize on ddDDaaAA order?
Let's better name this, as AAaaDDdd implies that:
aa = ~AA
dd = ~DD
As described at the NEC protocol.
I really don't think James and I had any trouble understanding each
other :)
Ok, but others on reading this thread may misunderstand the meanings.
The 24 or 32 bits variation is actually a violation of the NEC
protocol.
Violation is a misnomer. NEC created the 24 bit version, it's an
extension. Many companies (such as your employer :)) have created
further variations.
I'm fine if you call it as an extension, but the original NEC _is_
16 bits, and most drivers are compliant with it.
We should not break what's working.
What some IRs actually provide is:
xxyyADDdd (24 bits NEC)
where:
Address = yyxx
Data = DD
As described as Extended NEC protocol at:
http://www.sbprojects.com/knowledge/ir/nec.php
or:
xxyyADDzz (32 bits NEC)
where:
Address = zzxxyy
Data = DD
No need to explain the protocol to me.
Also, currently, there's just one IR table with 32 bits nec:
rc-tivo.c, used by the mceusb driver.
Yes, I know.
Well, changing the NEC decoders to always send a 32 bits code has
several issues:
1) It makes the normal NEC protocol as an exception, and not as a
rule;
It's not an
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On 2014-03-31 12:56, James Hogan wrote: On 31/03/14 11:19, David Härdeman wrote: On 2014-03-31 11:44, James Hogan wrote: On 29/03/14 16:11, David Härdeman wrote: +/* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */ +*scancode = swab32((u32)raw); What's the point of the byte swapping? Surely the most natural NEC encoding would just treat it as a single 32-bit (LSBit first) field rather than 4 8-bit fields that needs swapping. Thanks for having a look at the patches, I agree with your comments on the other patches (and I have to respin some of them because I missed two drivers), but the comments to this patch confuses me a bit. That the NEC data is transmitted as 32 bits encoded with LSB bit order within each byte is AFAIK just about the only thing that all sources/documentation of the protocal can agree on (so bitrev:ing the bits within each byte makes sense, unless the hardware has done it already). Agreed (in the case of img-ir there's a bit orientation setting which ensures that the u64 raw has the correct bit order, in the case of NEC the first bit received goes in the lowest order bit of the raw data). As for the byte order, AAaaDDdd corresponds to the transmission order and seems to be what most drivers expect/use for their RX data. AAaaDDdd is big endian rendering, no? (like %08x) Yeah, you could call it that. If it should be interpreted as LSBit first, then the first bits received should go in the low bits of the scancode, and by extension the first bytes received in the low bytes of the scancode, i.e. at the end of the inherently big-endian hexadecimal rendering of the scancode. I'm not saying the whole scancode should be interpreted as one 32 bit LSBit integer, just that the endianness within each byte should be respected. Are you suggesting that rc-core should standardize on ddDDaaAA order? Yes (where ddDDaaAA means something like scancode 0x(~cmd)(cmd)(~addr)(addr)) Yes, that's what I meant. This would mean that if the data is put in the right bit order (first bit received in BIT(0), last bit received in BIT(31)), then the scancode = raw, and if the data is received in the reverse bit order (like the raw decoder, shifting the data left and inserting the last bit in BIT(0)) then the scancode = bitrev32(raw). Have I missed something? I just think we have to agree to disagree :) For me, storing/presenting the scancode as 0xAAaaDDdd is obviously the clearest and least confusing interpretation. But I might have spent too long time using that notation in code and mentally to be able to find anything else intuitive :) 0xAAaaDDdd means that you read/parse/print it left to right, just as you would if you drew a pulse-space chart showing the received IR pulse (time normally progresses to the right...modulo the per-byte bitrev). It kind of matches the other protocol scancodes as well (the address bits high, cmd bits low, the high bits tend to remain constant for one given remote, the low bits change, although it's not a hard rule) and it matches most software I've ever seen (AFAIK, LIRC represents NEC32 scancodes this way, as does e.g. the Pronto software and protocol). That said...I think we at least agree that we need *a* representation and that it should be used consistently in all drivers, right? //David -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On 2014-03-31 15:15, Mauro Carvalho Chehab wrote: Em Mon, 31 Mar 2014 14:58:10 +0200 David Härdeman da...@hardeman.nu escreveu: On 2014-03-31 14:14, Mauro Carvalho Chehab wrote: The 24 or 32 bits variation is actually a violation of the NEC protocol. Violation is a misnomer. NEC created the 24 bit version, it's an extension. Many companies (such as your employer :)) have created further variations. I'm fine if you call it as an extension, but the original NEC _is_ 16 bits, and most drivers are compliant with it. We should not break what's working. You're misrepresenting the proposed changes now. I'm trying to fixup the scancode handling in the best way possible, I'm not willfully breaking anything. Some things are inconsistent right now between the drivers, in such a situation when driver A says first X then Y and driver B says first Y then X the situation is: a) already broken; and b) can't be fixed without introducing breakage of a different kind to either A or B Well, changing the NEC decoders to always send a 32 bits code has several issues: 1) It makes the normal NEC protocol as an exception, and not as a rule; It's not an exception. I just makes all 32 bits explicit. Well, if all drivers but one only have 16 or 24 bits tables, this is an exception. Not really. 32 bits are transmitted no matter what you call the protocol. I'm proposing storing those 32 bits in the scancode-keycode table. Not what I'd call an exception (this particular point starts to feel a bit off-topic though so I think we can drop it). And the lack of that explicit information currently makes the scancode ambiguous. Right now if I give you a NEC scancode of 0xff00 (like we give to userspace with the EV_SCAN event), you can't tell what it means...it could, for example, be a 32 bit code of 0xff00... You didn't answer this part. It's actually one of the biggest reasons for introducing the full scancode everywhere. 2) It breaks all in-kernel tables for 16 bits and 24 bits NEC. As already said, currently, there's just one driver using 32 bits NEC, and just for one IR type (RC_MAP_TIVO); No, the proposed patch doesn't break all in-kernel tables. The in-kernel tables are converted on the fly to NEC32 when loaded. That's messy. We should either change everything in Kernelspace to 32 bits or keep as is. No problem, I could respin the patch to also patch the keytables (which is what I did first), but I'll wait until we've agreed on something). If such emulation is needed, it should be only for userspace tables. 3) It causes regressions to userspace, as userspace tables won't work anymore; I know it may cause troubles for userspace, however: a) You've already accepted patches that change the scancode format of the NEC decoder within the last few weeks so you've already set the stage for the same kind of trouble (even if I agree with James on parts of that patch) If I let this pass, we should revert it before it reaches upstream. What patch caused regressions? 18bc17448147e93f31cc9b1a83be49f1224657b2, since it changes the scancode it'll break userspace keytables, it's mentioned in patch 4/11 in my patchset. b) The current code is broken as well...using the same remote will generate different scancodes depending on the driver (even if the old and new hardware *can* receive the full scancode), meaning that your keytable will suddenly stop working if you change HW. That's bad. On the devices I have here, it is not broken. Let's fix it where this is broken, and not use it as an excuse to break even more things. Whether the hardware you happen to have agrees is beside the point? (btw, the get_key_beholdm6xx() function at saa7134 driver seems to be wrong, as the keytables for behold device has the address of this vendor mapped as 0x6b86). I know, I've already identified and fixed that problem in a separate patch that's posted to the list. And it will also break out-of-kernel user-defined keymaps. Any inconsistency is a no-win situation. And we *do* have inconsistencies right now. Yes. That's one of the reasons why this was not fixed yet (and the other one is that I don't have any of such device in hands, in order to be sure that this is not another vendor that, by coincidence, has address 0x6b86). I know we can't be 100% sure, but the byte order in the driver itself also supports the notion that the address bytes have been reversed. The way those codes are handled inside each in-hardware NEC decoder are different. I've seen all those alternatives: a) the full 24-bits code is received by the driver; b) some hardware will simply discard the MSB of the address; c) a few hardware will discard the entire keycode, as the checksum bytes won't match. I know there's a lot of variety, another example is drivers that discard (possibly after matching address) everything but the command part of the scancode. That should not be used as an excuse
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On 31/03/14 14:22, David Härdeman wrote: On 2014-03-31 12:56, James Hogan wrote: This would mean that if the data is put in the right bit order (first bit received in BIT(0), last bit received in BIT(31)), then the scancode = raw, and if the data is received in the reverse bit order (like the raw decoder, shifting the data left and inserting the last bit in BIT(0)) then the scancode = bitrev32(raw). Have I missed something? I just think we have to agree to disagree :) For me, storing/presenting the scancode as 0xAAaaDDdd is obviously the clearest and least confusing interpretation. But I might have spent too long time using that notation in code and mentally to be able to find anything else intuitive :) 0xAAaaDDdd means that you read/parse/print it left to right, just as you would if you drew a pulse-space chart showing the received IR pulse (time normally progresses to the right...modulo the per-byte bitrev). Sure, but the NEC bit order is little endian, and the scancode is a 32bit value not an array of 4 bytes, so it's artificial to expect it to make any sense when read as big endian. E.g. if you extended the transmission to 48 bits you'd expect the hex printed scancode to extend to the left not the right. The bits in the 32-bit word also become discontinuous for no good reason, especially considering the cases we're trying to take into account (NEC-32 and NEC-24) both effectively have 16-bit fields. It kind of matches the other protocol scancodes as well (the address bits high, cmd bits low, the high bits tend to remain constant for one given remote, the low bits change, although it's not a hard rule) and it Very true, but you still have the low byte of the command in the 2nd lowest byte, which is why my original suggestion was: 0xaaAAddDD I.e. swap 16bit halves, each 16bit field intact. matches most software I've ever seen (AFAIK, LIRC represents NEC32 scancodes this way, as does e.g. the Pronto software and protocol). That said...I think we at least agree that we need *a* representation and that it should be used consistently in all drivers, right? Yes, that would be nice. Cheers James signature.asc Description: OpenPGP digital signature
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
Em Mon, 31 Mar 2014 15:22:47 +0200
David Härdeman da...@hardeman.nu escreveu:
On 2014-03-31 12:56, James Hogan wrote:
On 31/03/14 11:19, David Härdeman wrote:
On 2014-03-31 11:44, James Hogan wrote:
On 29/03/14 16:11, David Härdeman wrote:
+/* raw encoding : ddDDaaAA - scan encoding: AAaaDDdd */
+*scancode = swab32((u32)raw);
What's the point of the byte swapping?
Surely the most natural NEC encoding would just treat it as a single
32-bit (LSBit first) field rather than 4 8-bit fields that needs
swapping.
Thanks for having a look at the patches, I agree with your comments on
the other patches (and I have to respin some of them because I missed
two drivers), but the comments to this patch confuses me a bit.
That the NEC data is transmitted as 32 bits encoded with LSB bit order
within each byte is AFAIK just about the only thing that all
sources/documentation of the protocal can agree on (so bitrev:ing the
bits within each byte makes sense, unless the hardware has done it
already).
Agreed (in the case of img-ir there's a bit orientation setting which
ensures that the u64 raw has the correct bit order, in the case of NEC
the first bit received goes in the lowest order bit of the raw data).
As for the byte order, AAaaDDdd corresponds to the transmission order
and seems to be what most drivers expect/use for their RX data.
AAaaDDdd is big endian rendering, no? (like %08x)
Yeah, you could call it that.
If it should be interpreted as LSBit first, then the first bits
received
should go in the low bits of the scancode, and by extension the first
bytes received in the low bytes of the scancode, i.e. at the end of the
inherently big-endian hexadecimal rendering of the scancode.
I'm not saying the whole scancode should be interpreted as one 32 bit
LSBit integer, just that the endianness within each byte should be
respected.
Are you suggesting that rc-core should standardize on ddDDaaAA order?
Yes (where ddDDaaAA means something like scancode
0x(~cmd)(cmd)(~addr)(addr))
Yes, that's what I meant.
This would mean that if the data is put in the right bit order (first
bit received in BIT(0), last bit received in BIT(31)), then the
scancode
= raw, and if the data is received in the reverse bit order (like the
raw decoder, shifting the data left and inserting the last bit in
BIT(0)) then the scancode = bitrev32(raw).
Have I missed something?
I just think we have to agree to disagree :)
For me, storing/presenting the scancode as 0xAAaaDDdd is obviously the
clearest and least confusing interpretation. But I might have spent too
long time using that notation in code and mentally to be able to find
anything else intuitive :)
Inside the RC core, for all other protocols, the order always
ADDRESS + COMMAND.
Up to NEC-24 bits, this is preserved, as the command is always 0xDD
and the address is either 0xaaAA or 0xAA.
The 32-bits NEC is a little ackward, if we consider the command as
also being 8 bits, and the address having 24 bits.
The Tivo keytable is weird:
{ 0x3085f009, KEY_MEDIA }, /* TiVo Button */
{ 0x3085e010, KEY_POWER2 }, /* TV Power */
{ 0x3085e011, KEY_TV }, /* Live TV/Swap */
{ 0x3085c034, KEY_VIDEO_NEXT }, /* TV Input */
{ 0x3085e013, KEY_INFO },
{ 0x3085a05f, KEY_CYCLEWINDOWS }, /* Window */
{ 0x0085305f, KEY_CYCLEWINDOWS },
{ 0x3085c036, KEY_EPG },/* Guide */
...
There, the only part of the scancode that doesn't change is 0x85.
It seems that they're using 8 bits for address (0xaa) and 24
bits for command (0xAADDdd).
So, it seems that they're actually sending address/command as:
[command 24Address][(command 8) 0xff][command 0xff]
With seems too awkward.
IMHO, it would make more sense to store those data as:
addresscommand
So, KEY_MEDIA, for example, would be:
+ { 0x8530f009, KEY_MEDIA }, /* TiVo Button */
However, I'm not sure how other 32 bits NEC scancodes might be.
So, I think we should keep the internal representation as-is,
for now, while we're not sure about how other vendors handle
it, as, for now, there's just one IR table with 32 bits nec.
That's said, I don't mind much how this is internally stored at
the Kernel level, as we can always change it, but we should provide
backward compatibility for userspace, when userspace sends
to Kernel a 16 bit or a 24 bit keytable.
So, I think we should first focus on how to properly get/set the
bitsize at the API in a way that this is backward compatible.
Ok, the API actually sends the bit size of each keycode, as the
size length is variable, but I'm not sure if this is reliable enough,
as I think that the current userspace just sets it to 32 bits, even
when passing a 16 bits key.
In any case, it doesn't make any sense to require userspace to
convert a 16 bits normal NEC table (or a 24
Re: [PATCH 10/11] [RFC] rc-core: use the full 32 bits for NEC scancodes
On Mon, Mar 31, 2014 at 12:26:56PM -0300, Mauro Carvalho Chehab wrote:
Inside the RC core, for all other protocols, the order always
ADDRESS + COMMAND.
Up to NEC-24 bits, this is preserved, as the command is always 0xDD
and the address is either 0xaaAA or 0xAA.
The 32-bits NEC is a little ackward, if we consider the command as
also being 8 bits, and the address having 24 bits.
The Tivo keytable is weird:
{ 0x3085f009, KEY_MEDIA }, /* TiVo Button */
{ 0x3085e010, KEY_POWER2 }, /* TV Power */
{ 0x3085e011, KEY_TV }, /* Live TV/Swap */
{ 0x3085c034, KEY_VIDEO_NEXT }, /* TV Input */
{ 0x3085e013, KEY_INFO },
{ 0x3085a05f, KEY_CYCLEWINDOWS }, /* Window */
{ 0x0085305f, KEY_CYCLEWINDOWS },
{ 0x3085c036, KEY_EPG },/* Guide */
...
There, the only part of the scancode that doesn't change is 0x85.
It seems that they're using 8 bits for address (0xaa) and 24
bits for command (0xAADDdd).
So, it seems that they're actually sending address/command as:
[command 24Address][(command 8) 0xff][command 0xff]
With seems too awkward.
IMHO, it would make more sense to store those data as:
addresscommand
So, KEY_MEDIA, for example, would be:
+ { 0x8530f009, KEY_MEDIA }, /* TiVo Button */
However, I'm not sure how other 32 bits NEC scancodes might be.
And it's completely irrelevant. There's little to no value in trying to
determine what's a command and what's an address. We have to
standardize on one in-memory representation of the 32 bits, and then we
should just treat it as that...as a u32 lookup key for the
scancode-keycode table which lacks any further meaning.
So, I think we should keep the internal representation as-is,
for now, while we're not sure about how other vendors handle
it, as, for now, there's just one IR table with 32 bits nec.
It doesn't matter how other vendors handle (i.e. interpret) the
different bits, that's what we want to get away from, it's the whole
point of this discussion.
That's said, I don't mind much how this is internally stored at
the Kernel level, as we can always change it, but we should provide
backward compatibility for userspace, when userspace sends
to Kernel a 16 bit or a 24 bit keytable.
Yes, which is part of what I've proposed.
It's not a coicidence that I've proposed a new ioctl and the NEC32
standardization at the same time. A new ioctl is the perfect time and
place to get this right once and for all.
So with the new ioctl, the protocol is made explicit, and the
definition of a scancode follows from the protocol (protocol as in
RC_TYPE_*).
For RC_TYPE_NEC, that scancode would be a 32 bit int (exact byte and bit
order to be determined, but not terribly important for this discussion).
That removes *all* ambiguity and makes RC_TYPE_NEC behave *exactly* like
all other protocols. At the same time it removes pointless policy from
the kernel and causes a reduction in code (mostly thinking of the
pointless NEC16/24/32 parsing code that gets duplicated across drivers).
So, I think we should first focus on how to properly get/set the
bitsize at the API in a way that this is backward compatible.
No, adding bitsizes adds complexity and additional layers of abstraction
for no good reason. And it is not needed for *any other protocol*. Why?
Because the protocol already defines the bitsize. And so would NEC if we
would just use all 32 bits throughout.
With that change, the bitsize is implicit in *each protocol* and the new
ioctl I proposed makes the protocol explicit (while providing at least a
best-effort guess for NEC scancodes when the legacy ioctl is used).
(and no, please, don't suggest we add RC_TYPE_NEC, RC_TYPE_NEC24,
RC_TYPE_NEC32...)
Ok, the API actually sends the bit size of each keycode, as the
size length is variable, but I'm not sure if this is reliable enough,
as I think that the current userspace just sets it to 32 bits, even
when passing a 16 bits key.
That won't work as you've noted yourself.
In any case, it doesn't make any sense to require userspace to
convert a 16 bits normal NEC table (or a 24 bits extended NEC
table) into a 32 bits data+checksum bitpack on userspace.
I disagree. Strongly.
It makes perfect sense. Policy doesn't belong in the kernel and all
that. Asking userspace to provide a full description of the 32 bits that
are transmitted removes all ambiguity and makes any bitsize
irrelevant. For all the other protocols we support, the bitsize is
known on a per-protocol basis. The same can be true for RC_TYPE_NEC.
And userspace can still write nice user-friendly 16 bit keymaps if it
likes and convert to kernel scancode notation on the fly. That's
something userspace anyways has to do today.
Consider the 32 bit scancode as simply being the communication protocol
between userspace - kernel if you like. There's no reason to
complicate that with bitsizes and/or multiple protocols when a single 32
bit scancode describes exactly
