[PATCH] DVB: TechnoTrend CT-3650 IR support

[PATCH] DVB: TechnoTrend CT-3650 IR support

[David Henningsson]

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

Hi Linux-media,

As a spare time project I bought myself a TT CT-3650, to see if I could 
get it working. Waling Dijkstra did write a IR & CI patch for this model 
half a year ago, so I was hopeful. (Reference: 
http://www.mail-archive.com/linux-media@vger.kernel.org/msg19860.html )

Having tested the patch, the IR is working (tested all keys via the 
"evtest" tool), however descrambling is NOT working.

Waling's patch was reviewed but never merged. So I have taken the IR 
part of the patch, cleaned it up a little, and hopefully this part is 
ready for merging now. Patch is against linux-2.6.git.

As for the descrambling/CI/CAM part, that'll be the next assignment. 
I'll be happy for some mentoring as I haven't done anything in this part 
of the kernel before - whether I can debug some of it here or if I'll 
have to install Windows and an USB sniffer to see what it does?


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

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

>From 705adeab4da152cf24cede069b724765b8a07d55 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Sun, 26 Dec 2010 07:55:57 +0100
Subject: [PATCH] DVB: TechnoTrend CT-3650 IR support

This patch enables IR support on the TechnoTrend CT-3650 device,
and is half of Waling Dijkstra's patch posted earlier this year,
cleaned up a little.

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

diff --git a/drivers/media/dvb/dvb-usb/ttusb2.c b/drivers/media/dvb/dvb-usb/ttusb2.c
index a6de489..1bacfc8 100644
--- a/drivers/media/dvb/dvb-usb/ttusb2.c
+++ b/drivers/media/dvb/dvb-usb/ttusb2.c
@@ -128,6 +128,76 @@ static struct i2c_algorithm ttusb2_i2c_algo = {
 	.functionality = ttusb2_i2c_func,
 };
 
+/* IR */
+/* Remote Control Stuff for CT-3650 (copied from TT-S1500): */
+static struct dvb_usb_rc_key tt_connect_CT_3650_rc_key[] = {
+	{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}
+};
+
+/* Copy-pasted from dvb-usb-remote.c */
+#define DVB_USB_RC_NEC_KEY_PRESSED     0x01
+
+static int tt3650_rc_query(struct dvb_usb_device *d, u32 *keyevent, int *keystate)
+{
+	u8 keybuf[5];
+	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) {
+		/* got a "press" event */
+		deb_info("%s: cmd=0x%02x sys=0x%02x\n", __func__, rx[2], rx[3]);
+		keybuf[0] = DVB_USB_RC_NEC_KEY_PRESSED;
+		keybuf[1] = rx[3];
+		keybuf[2] = ~keybuf[1]; /* fake checksum */
+		keybuf[3] = rx[2];
+		keybuf[4] = ~keybuf[3]; /* fake checksum */
+		dvb_usb_nec_rc_key_to_event(d, keybuf, keyevent, keystate);
+	}
+	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 +415,11 @@ static struct dvb_usb_device_properties ttusb2_properties_ct3650 = {
 
 	.size_of_priv = sizeof(struct ttusb2_state),
 
+	.rc_key_map = tt_connect_CT_3650_rc_key,
+	.rc_key_map_size = ARRAY_SIZE(tt_connect_CT_3650_rc_key),
+	.rc_query = tt3650_rc_query,
+	.rc_interval = 500,
+
 	.num_adapters = 1,
 	.adapter = {
 		{
diff --git a/drivers/media/dvb/dvb-usb/ttusb2.h b/drivers/media/dvb/dvb-usb/ttusb2.h
index 52a63af..1bd5d54 100644
--- a/drivers/media/dvb/dvb-usb/ttusb2.h
+++ b/drivers/media/dvb/dvb-usb/ttusb2.h
@@ -45,6 +45,9 @@
 #define CMD_DISEQC          0x18
 /* out data: <master=0xff/burst=??> <cmdlen> <cmdbytes>[cmdlen] */
 
+/* command to poll IR receiver (copied from pctv452e.c) */
+#define CMD_GET_IR_CODE     0x1b
+
 #define CMD_PID_ENABLE      0x22
 /* out data: <index> <type: ts=1/sec=2> <pid msb> <pid lsb> */
 
-- 
1.7.1

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

[Mauro Carvalho Chehab]

Hi David,

Em 26-12-2010 07:14, David Henningsson escreveu:
> Hi Linux-media,
> 
> As a spare time project I bought myself a TT CT-3650, to see if I could get it working. Waling Dijkstra did write a IR & CI patch for this model half a year ago, so I was hopeful. (Reference: http://www.mail-archive.com/linux-media@vger.kernel.org/msg19860.html )
> 
> Having tested the patch, the IR is working (tested all keys via the "evtest" tool), however descrambling is NOT working.
> 
> Waling's patch was reviewed but never merged. So I have taken the IR part of the patch, cleaned it up a little, and hopefully this part is ready for merging now. Patch is against linux-2.6.git.

Could you please rebase it to work with the rc_core support? I suspect that you
based it on a kernel older than .36, as the dvb_usb rc struct changed.

The better is to base it over the latest V4L/DVB/RC development git, available at:
	http://git.linuxtv.org/media_tree.git

Basically, the keyboard map, if not already available, should be at:
	/driver/media/rc/keymaps/

And the rc description should be something like:

.rc.core = {
			.rc_interval      = DEFAULT_RC_INTERVAL,
			.rc_codes         = RC_MAP_DIB0700_RC5_TABLE,
			.rc_query         = dib0700_rc_query_old_firmware,
			.allowed_protos   = RC_TYPE_RC5 |
					    RC_TYPE_RC6 |
					    RC_TYPE_NEC,
			.change_protocol = dib0700_change_protocol,
		},

There are a few dvb drivers already converted to the new way (like dib0700).

Cheers,
Mauro.

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

[David Henningsson]

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

On 2010-12-26 12:41, Mauro Carvalho Chehab wrote:
> Hi David,
>
> Em 26-12-2010 07:14, David Henningsson escreveu:
>> Hi Linux-media,
>>
>> As a spare time project I bought myself a TT CT-3650, to see if I could get it working. Waling Dijkstra did write a IR&  CI patch for this model half a year ago, so I was hopeful. (Reference: http://www.mail-archive.com/linux-media@vger.kernel.org/msg19860.html )
>>
>> Having tested the patch, the IR is working (tested all keys via the "evtest" tool), however descrambling is NOT working.
>>
>> Waling's patch was reviewed but never merged. So I have taken the IR part of the patch, cleaned it up a little, and hopefully this part is ready for merging now. Patch is against linux-2.6.git.
>
> Could you please rebase it to work with the rc_core support? I suspect that you
> based it on a kernel older than .36, as the dvb_usb rc struct changed.

Ok, I have now done this, but I'm not completely satisfied, perhaps you 
can help out a little? I'm new to IR/RC stuff, but I feel I'm missing 
correct "repeat" functionality, i e, if you keep a key pressed it 
appears as separate key presses with whatever interval set as 
.rc_interval. (This was probably a problem with the old patch as well.) 
Is there any support for this is rc_core?

I'm attaching a temporary patch (just for review) so you know what I'm 
talking about.

> The better is to base it over the latest V4L/DVB/RC development git, available at:
> 	http://git.linuxtv.org/media_tree.git

Ok. I was probably confused with this entry: 
http://linuxtv.org/news.php?entry=2010-01-19.mchehab
telling me to base it on v4l-dvb.git, which is not updated for four 
months. However, http://git.linuxtv.org/ correctly lists the 
media_tree.git as the repository of choice.

Thanks for the review!

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

[-- Attachment #2: 0001-DVB-IR-CT-3650.patch --]
[-- Type: text/x-patch, Size: 1878 bytes --]

>From 8c42121a08c5dabbd1a943cc1e5726ed99f0d957 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] DVB: IR support for CT-3650

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 drivers/media/dvb/dvb-usb/ttusb2.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 debian/rules

diff --git a/debian/rules b/debian/rules
old mode 100644
new mode 100755
diff --git a/drivers/media/dvb/dvb-usb/ttusb2.c b/drivers/media/dvb/dvb-usb/ttusb2.c
index a6de489..ded8a4b 100644
--- a/drivers/media/dvb/dvb-usb/ttusb2.c
+++ b/drivers/media/dvb/dvb-usb/ttusb2.c
@@ -128,6 +128,27 @@ 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 */
+	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 */
+		deb_info("%s: cmd=0x%02x sys=0x%02x\n", __func__, rx[2], rx[3]);
+		rc_keydown(d->rc_dev, rx[2], 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 +366,13 @@ static struct dvb_usb_device_properties ttusb2_properties_ct3650 = {
 
 	.size_of_priv = sizeof(struct ttusb2_state),
 
+	.rc.core = {
+		.rc_interval      = 250,
+		.rc_codes         = RC_MAP_TT_1500,
+		.rc_query         = tt3650_rc_query,
+		.allowed_protos   = RC_TYPE_UNKNOWN,
+	},
+
 	.num_adapters = 1,
 	.adapter = {
 		{
-- 
1.7.1

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

[Mauro Carvalho Chehab]

Em 26-12-2010 17:38, David Henningsson escreveu:
> On 2010-12-26 12:41, Mauro Carvalho Chehab wrote:
>> Hi David,
>>
>> Em 26-12-2010 07:14, David Henningsson escreveu:
>>> Hi Linux-media,
>>>
>>> As a spare time project I bought myself a TT CT-3650, to see if I could get it working. Waling Dijkstra did write a IR&  CI patch for this model half a year ago, so I was hopeful. (Reference: http://www.mail-archive.com/linux-media@vger.kernel.org/msg19860.html )
>>>
>>> Having tested the patch, the IR is working (tested all keys via the "evtest" tool), however descrambling is NOT working.
>>>
>>> Waling's patch was reviewed but never merged. So I have taken the IR part of the patch, cleaned it up a little, and hopefully this part is ready for merging now. Patch is against linux-2.6.git.
>>
>> Could you please rebase it to work with the rc_core support? I suspect that you
>> based it on a kernel older than .36, as the dvb_usb rc struct changed.
> 
> Ok, I have now done this, but I'm not completely satisfied, perhaps you can help out a little? I'm new to IR/RC stuff, 
> but I feel I'm missing correct "repeat" functionality, i e, if you keep a key pressed it appears as separate key presses
> with whatever interval set as .rc_interval. (This was probably a problem with the old patch as well.) Is there any
> support for this is rc_core?

>From your decode logic, I suspect that the IR hardware decoder has its own logic for repeat.
In this case, there's not much you can do, as it probably uses a very high time for repeat.

> I'm attaching a temporary patch (just for review) so you know what I'm talking about.
> 
>> The better is to base it over the latest V4L/DVB/RC development git, available at:
>>     http://git.linuxtv.org/media_tree.git
> 
> Ok. I was probably confused with this entry: http://linuxtv.org/news.php?entry=2010-01-19.mchehab
> telling me to base it on v4l-dvb.git, which is not updated for four months. However, http://git.linuxtv.org/ correctly lists the media_tree.git as the repository of choice.

Ah... yeah, old news;)

> Thanks for the review!
> 
Em 26-12-2010 17:38, David Henningsson escreveu:
> From 8c42121a08c5dabbd1a943cc1e5726ed99f0d957 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] DVB: IR support for CT-3650
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  drivers/media/dvb/dvb-usb/ttusb2.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
>  mode change 100644 => 100755 debian/rules
> 
> diff --git a/debian/rules b/debian/rules
> old mode 100644
> new mode 100755
> diff --git a/drivers/media/dvb/dvb-usb/ttusb2.c b/drivers/media/dvb/dvb-usb/ttusb2.c
> index a6de489..ded8a4b 100644
> --- a/drivers/media/dvb/dvb-usb/ttusb2.c
> +++ b/drivers/media/dvb/dvb-usb/ttusb2.c
> @@ -128,6 +128,27 @@ 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 */
> +	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.

> +		/* 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);


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.

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.

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.


> +	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 +366,13 @@ static struct dvb_usb_device_properties ttusb2_properties_ct3650 = {
>  
>  	.size_of_priv = sizeof(struct ttusb2_state),
>  
> +	.rc.core = {
> +		.rc_interval      = 250,
> +		.rc_codes         = RC_MAP_TT_1500,
> +		.rc_query         = tt3650_rc_query,
> +		.allowed_protos   = RC_TYPE_UNKNOWN,
> +	},

Seems ok to me, except for the RC_TYPE_UNKNOWN. 

> +
>  	.num_adapters = 1,
>  	.adapter = {
>  		{
> -- 1.7.1


Thanks,
Mauro

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

[David Henningsson]

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:
>>> Hi David,
>>>
>>> Em 26-12-2010 07:14, David Henningsson escreveu:
>>>> Hi Linux-media,
>>>>
>>>> As a spare time project I bought myself a TT CT-3650, to see if I could get it working. Waling Dijkstra did write a IR&   CI patch for this model half a year ago, so I was hopeful. (Reference: http://www.mail-archive.com/linux-media@vger.kernel.org/msg19860.html )
>>>>
>>>> Having tested the patch, the IR is working (tested all keys via the "evtest" tool), however descrambling is NOT working.
>>>>
>>>> Waling's patch was reviewed but never merged. So I have taken the IR part of the patch, cleaned it up a little, and hopefully this part is ready for merging now. Patch is against linux-2.6.git.
>>>
>>> Could you please rebase it to work with the rc_core support? I suspect that you
>>> based it on a kernel older than .36, as the dvb_usb rc struct changed.
>>
>> Ok, I have now done this, but I'm not completely satisfied, perhaps you can help out a little? I'm new to IR/RC stuff,
>> but I feel I'm missing correct "repeat" functionality, i e, if you keep a key pressed it appears as separate key presses
>> with whatever interval set as .rc_interval. (This was probably a problem with the old patch as well.) Is there any
>> support for this is rc_core?
>
>  From your decode logic, I suspect that the IR hardware decoder has its own logic for repeat.
> In this case, there's not much you can do, as it probably uses a very high time for repeat.
>
>> I'm attaching a temporary patch (just for review) so you know what I'm talking about.
>>
>>> The better is to base it over the latest V4L/DVB/RC development git, available at:
>>>      http://git.linuxtv.org/media_tree.git
>>
>> Ok. I was probably confused with this entry: http://linuxtv.org/news.php?entry=2010-01-19.mchehab
>> telling me to base it on v4l-dvb.git, which is not updated for four months. However, http://git.linuxtv.org/ correctly lists the media_tree.git as the repository of choice.
>
> Ah... yeah, old news;)
>
>> Thanks for the review!
>>
> Em 26-12-2010 17:38, David Henningsson escreveu:
>>  From 8c42121a08c5dabbd1a943cc1e5726ed99f0d957 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] DVB: IR support for CT-3650
>>
>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>> ---
>>   drivers/media/dvb/dvb-usb/ttusb2.c |   28 ++++++++++++++++++++++++++++
>>   1 files changed, 28 insertions(+), 0 deletions(-)
>>   mode change 100644 =>  100755 debian/rules
>>
>> diff --git a/debian/rules b/debian/rules
>> old mode 100644
>> new mode 100755
>> diff --git a/drivers/media/dvb/dvb-usb/ttusb2.c b/drivers/media/dvb/dvb-usb/ttusb2.c
>> index a6de489..ded8a4b 100644
>> --- a/drivers/media/dvb/dvb-usb/ttusb2.c
>> +++ b/drivers/media/dvb/dvb-usb/ttusb2.c
>> @@ -128,6 +128,27 @@ 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 */
>> +	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);
   }
}
else if (lastkey) {
   rc_keyup(lastkey);
   lastkey = 0;
}

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.

> 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?

> 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?

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

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

[Mauro Carvalho Chehab]

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).

> 
> 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.

>> 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).

>> 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.

Cheers,
Mauro

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

[David Henningsson]

[-- 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

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

[Mauro Carvalho Chehab]

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.

Thanks,
Mauro

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

[David Henningsson]

[-- 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

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

[Mauro Carvalho Chehab]

Em 29-12-2010 10:04, David Henningsson escreveu:
> 
> 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?

There are some drivers that we actually fixed IR repeat by increasing it
to 500 ms ;)

Those timeouts are somewhat hardware-related, as it depends on what the hardware
(or how the raw IR decoder logic at RC core) decodes the IR code.

Cheers,
Mauro