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

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

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

On 2010-12-27 22:28, Mauro Carvalho Chehab wrote:
> Em 27-12-2010 17:02, David Henningsson escreveu:
>> 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.
>
> Ok.
>>
>>>
>>>>
>>>> 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.
>
> They are good. Applied, thanks!
>
>>
>>>>> 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?
>
> Yeah, this should be OK for me.

Ok, here comes the patch. It seems to be working sufficiently well after 
I discovered I needed a poll interval less than IR_KEYPRESS_TIMEOUT. As 
a side note, grepping for rc_interval seems to reveal a few intervals >= 
250, could we have suboptimal results from these as well?

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

[-- Attachment #2: 0001-media-DVB-IR-support-for-TechnoTrend-CT-3650.patch --]
[-- Type: text/x-patch, Size: 2328 bytes --]

>From 44e2a8503f2db4f5316ec739b804b6a3498111e3 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Sun, 26 Dec 2010 14:23:58 +0100
Subject: [PATCH] [media] DVB: IR support for TechnoTrend CT-3650

Based on Waling Dijkstra's discovery that the IR works the same as
on the TT-1500, this patch has been rewritten to fit with the
rc_core infrastructure.

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

diff --git a/drivers/media/dvb/dvb-usb/ttusb2.c b/drivers/media/dvb/dvb-usb/ttusb2.c
index a6de489..0d4709f 100644
--- a/drivers/media/dvb/dvb-usb/ttusb2.c
+++ b/drivers/media/dvb/dvb-usb/ttusb2.c
@@ -43,6 +43,7 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
 struct ttusb2_state {
 	u8 id;
+	u16 last_rc_key;
 };
 
 static int ttusb2_msg(struct dvb_usb_device *d, u8 cmd,
@@ -128,6 +129,33 @@ static struct i2c_algorithm ttusb2_i2c_algo = {
 	.functionality = ttusb2_i2c_func,
 };
 
+/* 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 */
+	struct ttusb2_state *st = d->priv;
+	ret = ttusb2_msg(d, CMD_GET_IR_CODE, NULL, 0, rx, sizeof(rx));
+	if (ret != 0)
+		return ret;
+
+	if (rx[8] & 0x01) {
+		/* got a "press" event */
+		st->last_rc_key = (rx[3] << 8) | rx[2];
+		deb_info("%s: cmd=0x%02x sys=0x%02x\n", __func__, rx[2], rx[3]);
+		rc_keydown(d->rc_dev, st->last_rc_key, 0);
+	} else if (st->last_rc_key) {
+		rc_keyup(d->rc_dev);
+		st->last_rc_key = 0;
+	}
+
+	return 0;
+}
+
+
 /* Callbacks for DVB USB */
 static int ttusb2_identify_state (struct usb_device *udev, struct
 		dvb_usb_device_properties *props, struct dvb_usb_device_description **desc,
@@ -345,6 +373,13 @@ static struct dvb_usb_device_properties ttusb2_properties_ct3650 = {
 
 	.size_of_priv = sizeof(struct ttusb2_state),
 
+	.rc.core = {
+		.rc_interval      = 150, /* Less than IR_KEYPRESS_TIMEOUT */
+		.rc_codes         = RC_MAP_TT_1500,
+		.rc_query         = tt3650_rc_query,
+		.allowed_protos   = RC_TYPE_UNKNOWN,
+	},
+
 	.num_adapters = 1,
 	.adapter = {
 		{
-- 
1.7.1