Re: [PATCH] DVB: TechnoTrend CT-3650 IR support

[David Henningsson <david.henningsson@canonical.com>]

[-- Attachment #1: Type: text/plain, Size: 5689 bytes --]

On 2010-12-27 17:51, Mauro Carvalho Chehab wrote:
> Em 27-12-2010 13:54, David Henningsson escreveu:
>> On 2010-12-27 10:54, Mauro Carvalho Chehab wrote:
>>> Em 26-12-2010 17:38, David Henningsson escreveu:
>>>> On 2010-12-26 12:41, Mauro Carvalho Chehab wrote:
>
>>>> +/* command to poll IR receiver (copied from pctv452e.c) */
>>>> +#define CMD_GET_IR_CODE     0x1b
>>>> +
>>>> +/* IR */
>>>> +static int tt3650_rc_query(struct dvb_usb_device *d)
>>>> +{
>>>> +    int ret;
>>>> +    u8 rx[9]; /* A CMD_GET_IR_CODE reply is 9 bytes long */
>>>> +    ret = ttusb2_msg(d, CMD_GET_IR_CODE, NULL, 0, rx, sizeof(rx));
>>>> +    if (ret != 0)
>>>> +        return ret;
>>>> +
>>>> +    if (rx[8]&   0x01) {
>>>
>>> Maybe (rx[8]&   0x01) == 0 indicates a keyup event. If so, if you map both keydown
>>> and keyup events, the in-kernel repeat logic will work.
>>
>> Hmm. If I should fix keyup events, the most reliable version would probably be something like:
>>
>> if (rx[8]&  0x01) {
>>    int currentkey = rx[2]; // or (rx[3]<<   8) | rx[2];
>>    if (currentkey == lastkey)
>>      rc_repeat(lastkey);
>>    else {
>>      if (lastkey)
>>        rc_keyup(lastkey);
>>      lastkey = currentkey;
>>      rc_keydown(currentkey);
>>    }
>
> rc_keydown() already handles repeat events (see ir_do_keydown and rc_keydown, at
> rc-main.c), so, you don't need it.
>
>> }
>> else if (lastkey) {
>>    rc_keyup(lastkey);
>>    lastkey = 0;
>> }
>
> Yeah, this makes sense, if bit 1 of rx[8] indicates keyup/keydown or repeat.
>
> You need to double check if you are not receiving any packet with this bit unset,
> when you press and hold a key, as some devices use a bit to just indicate that
> the info there is valid or not (a "done" bit).

As far as I can understand, a value of "1" indicates that a key is 
currently pressed, and a value of "0" indicates that no key is pressed.

>
>>
>> Does this sound reasonable to you?
>>
>>>
>>>> +        /* got a "press" event */
>>>> +        deb_info("%s: cmd=0x%02x sys=0x%02x\n", __func__, rx[2], rx[3]);
>>>> +        rc_keydown(d->rc_dev, rx[2], 0);
>>>> +    }
>>>
>>> As you're receiving both command+address, please use the complete code:
>>>      rc_keydown(d->rc_dev, (rx[3]<<   8) | rx[2], 0);
>>
>> I've tried this, but it stops working. evtest shows only scancode events, so my guess is that this makes it incompatible with RC_MAP_TT_1500, which lists only the lower byte.
>
> yeah, you'll need either to create another table or to fix it. The better is to fix
> the table and to use .scanmask = 0xff at the old drivers. This way, the same table
> will work for both the legacy/incomplete get_scancode function and for the new one.

Ok. I did a grep for RC_MAP_TT_1500 and found one place only, so I'm 
attaching two patches that should fix this, feel free to commit them if 
they look good to you.

>>> Also as it is receiving 8 bytes from the device, maybe the IR decoding logic is
>>> capable of decoding more than just one protocol. Such feature is nice, as it
>>> allows replacing the original keycode table by a more complete one.
>>
>> I've tried dumping all nine bytes but I can't make much out of it as I'm unfamiliar with RC protocols and decoders.
>>
>> Typical reply is (no key pressed):
>>
>> cc 35 0b 15 00 03 00 00 00
>>
>> Does this tell you anything?
>
> This means nothing to me, but the only way to double check is to test the device
> with other remote controllers. On several hardware, it is possible to use
> RC5 remote controllers as well. As there are some empty (zero) fields, maybe
> this device also supports RC6 protocols (that have more than 16 bits) and
> NEC extended (24 bits or 32 bits, on a few variants).

Ok.

>>> One of the most interesting features of the new RC code is that it offers
>>> a sysfs class and some additional logic to allow dynamically change/replace
>>> the keymaps and keycodes via userspace. The idea is to remove all in-kernel
>>> keymaps in the future, using, instead, the userspace way, via ir-keytable
>>> tool, available at:
>>>      http://git.linuxtv.org/v4l-utils.git
>>>
>>> The tool already supports auto-loading the keymap via udev.
>>>
>>> For IR's where we don't know the protocol or that we don't have the full scancode,
>>> loading the keymap via userspace will not bring any new feature. But, for those
>>> devices where we can be sure about the protocol and for those that also allow
>>> using other protocols, users can just replace the device-provided IR with a more
>>> powerful remote controller with more keys.
>>
>> Yeah, that sounds like a really nice feature.
>>
>>> So, it would be wonderful if you could identify what's the supported protocol(s)
>>> instead of using RC_TYPE_UNKNOWN. You can double check the protocol if you have
>>> with you another RC device that supports raw decoding. The rc-core internal decoders
>>> will tell you what protocol was used to decode a keycode, if you enable debug.
>>
>> I don't have any such RC receiver device. I do have a Logitech Harmony 525, so I tried pointing that one towards the CT 3650, but CMD_GET_IR_CODE didn't change for any of the devices I've currently told my Harmony to emulate.
>>
>> So I don't really see how I can help further in this case?
>
> I don't have a Logitech Harmony, so I'm not sure about it. Maybe Jarod may have some
> info about it.

Would you like me to provide a patch with RC_TYPE_UNKNOWN at this point 
(i e what I showed you earlier + your review comments), and when I or 
somebody else can provide more complete information, we make an 
additional patch with better protocol support? Does that make sense to you?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

[-- Attachment #2: 0001-DVB-Set-scanmask-for-Budget-SAA7146-cards.patch --]
[-- Type: text/x-patch, Size: 1045 bytes --]

>From 7435de9c183fa2b62c03311ae8e08ddd436cb287 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Mon, 27 Dec 2010 19:41:58 +0100
Subject: [PATCH 1/2] DVB: Set scanmask for Budget/SAA7146 cards

These devices do not return the full command+address, so set
scanmask accordingly.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 drivers/media/dvb/ttpci/budget-ci.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/dvb/ttpci/budget-ci.c b/drivers/media/dvb/ttpci/budget-ci.c
index 8ae67c1..b82756d 100644
--- a/drivers/media/dvb/ttpci/budget-ci.c
+++ b/drivers/media/dvb/ttpci/budget-ci.c
@@ -184,6 +184,7 @@ static int msp430_ir_init(struct budget_ci *budget_ci)
 	dev->input_phys = budget_ci->ir.phys;
 	dev->input_id.bustype = BUS_PCI;
 	dev->input_id.version = 1;
+	dev->scanmask = 0xff;
 	if (saa->pci->subsystem_vendor) {
 		dev->input_id.vendor = saa->pci->subsystem_vendor;
 		dev->input_id.product = saa->pci->subsystem_device;
-- 
1.7.1


[-- Attachment #3: 0002-MEDIA-RC-Provide-full-scancodes-for-TT-1500-remote-c.patch --]
[-- Type: text/x-patch, Size: 2897 bytes --]

>From 404319567f67d5c63001239ca4f960aaf775a075 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Mon, 27 Dec 2010 19:45:19 +0100
Subject: [PATCH 2/2] MEDIA: RC: Provide full scancodes for TT-1500 remote control

Add 0x15 prefix to scancodes for TT-1500 remote control.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 drivers/media/rc/keymaps/rc-tt-1500.c |   78 ++++++++++++++++----------------
 1 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/media/rc/keymaps/rc-tt-1500.c b/drivers/media/rc/keymaps/rc-tt-1500.c
index bb19487..295f373 100644
--- a/drivers/media/rc/keymaps/rc-tt-1500.c
+++ b/drivers/media/rc/keymaps/rc-tt-1500.c
@@ -15,45 +15,45 @@
 /* for the Technotrend 1500 bundled remotes (grey and black): */
 
 static struct rc_map_table tt_1500[] = {
-	{ 0x01, KEY_POWER },
-	{ 0x02, KEY_SHUFFLE },		/* ? double-arrow key */
-	{ 0x03, KEY_1 },
-	{ 0x04, KEY_2 },
-	{ 0x05, KEY_3 },
-	{ 0x06, KEY_4 },
-	{ 0x07, KEY_5 },
-	{ 0x08, KEY_6 },
-	{ 0x09, KEY_7 },
-	{ 0x0a, KEY_8 },
-	{ 0x0b, KEY_9 },
-	{ 0x0c, KEY_0 },
-	{ 0x0d, KEY_UP },
-	{ 0x0e, KEY_LEFT },
-	{ 0x0f, KEY_OK },
-	{ 0x10, KEY_RIGHT },
-	{ 0x11, KEY_DOWN },
-	{ 0x12, KEY_INFO },
-	{ 0x13, KEY_EXIT },
-	{ 0x14, KEY_RED },
-	{ 0x15, KEY_GREEN },
-	{ 0x16, KEY_YELLOW },
-	{ 0x17, KEY_BLUE },
-	{ 0x18, KEY_MUTE },
-	{ 0x19, KEY_TEXT },
-	{ 0x1a, KEY_MODE },		/* ? TV/Radio */
-	{ 0x21, KEY_OPTION },
-	{ 0x22, KEY_EPG },
-	{ 0x23, KEY_CHANNELUP },
-	{ 0x24, KEY_CHANNELDOWN },
-	{ 0x25, KEY_VOLUMEUP },
-	{ 0x26, KEY_VOLUMEDOWN },
-	{ 0x27, KEY_SETUP },
-	{ 0x3a, KEY_RECORD },		/* these keys are only in the black remote */
-	{ 0x3b, KEY_PLAY },
-	{ 0x3c, KEY_STOP },
-	{ 0x3d, KEY_REWIND },
-	{ 0x3e, KEY_PAUSE },
-	{ 0x3f, KEY_FORWARD },
+	{ 0x1501, KEY_POWER },
+	{ 0x1502, KEY_SHUFFLE },		/* ? double-arrow key */
+	{ 0x1503, KEY_1 },
+	{ 0x1504, KEY_2 },
+	{ 0x1505, KEY_3 },
+	{ 0x1506, KEY_4 },
+	{ 0x1507, KEY_5 },
+	{ 0x1508, KEY_6 },
+	{ 0x1509, KEY_7 },
+	{ 0x150a, KEY_8 },
+	{ 0x150b, KEY_9 },
+	{ 0x150c, KEY_0 },
+	{ 0x150d, KEY_UP },
+	{ 0x150e, KEY_LEFT },
+	{ 0x150f, KEY_OK },
+	{ 0x1510, KEY_RIGHT },
+	{ 0x1511, KEY_DOWN },
+	{ 0x1512, KEY_INFO },
+	{ 0x1513, KEY_EXIT },
+	{ 0x1514, KEY_RED },
+	{ 0x1515, KEY_GREEN },
+	{ 0x1516, KEY_YELLOW },
+	{ 0x1517, KEY_BLUE },
+	{ 0x1518, KEY_MUTE },
+	{ 0x1519, KEY_TEXT },
+	{ 0x151a, KEY_MODE },		/* ? TV/Radio */
+	{ 0x1521, KEY_OPTION },
+	{ 0x1522, KEY_EPG },
+	{ 0x1523, KEY_CHANNELUP },
+	{ 0x1524, KEY_CHANNELDOWN },
+	{ 0x1525, KEY_VOLUMEUP },
+	{ 0x1526, KEY_VOLUMEDOWN },
+	{ 0x1527, KEY_SETUP },
+	{ 0x153a, KEY_RECORD },		/* these keys are only in the black remote */
+	{ 0x153b, KEY_PLAY },
+	{ 0x153c, KEY_STOP },
+	{ 0x153d, KEY_REWIND },
+	{ 0x153e, KEY_PAUSE },
+	{ 0x153f, KEY_FORWARD },
 };
 
 static struct rc_map_list tt_1500_map = {
-- 
1.7.1