rc: gpio-ir-recv: allow flush space on idle

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

This patch adds an optional device tree node "flush-ms" which, if present, will use a jiffie-based timer to complete the last pulse stream and allow decode.

Quoted 7 lines · Eric Nelson

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

This is a problem other IR drivers suffer from too. It might be better to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, with the duration set to what the timeout was. That is what irraw timeouts are for; much better than fake transitions.

Quoted 3 lines

This patch adds an optional device tree node "flush-ms" which, if present, will use a jiffie-based timer to complete the last pulse stream and allow decode.

A common value for this is 100ms, I'm not sure what use it has to have it configurable. It's nice to have it exposed in rc_dev->timeout.

Thanks Shawn,

Quoted 9 lines · Sean Young

Quoted 7 lines · Eric Nelson

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

Quoted 5 lines

This is a problem other IR drivers suffer from too. It might be better to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, with the duration set to what the timeout was. That is what irraw timeouts are for; much better than fake transitions.

If I'm understanding this correctly, this would require modification of each decoder to handle what seems to be a special case regarding the GPIO IR driver (which needs an edge to trigger an interrupt).

Isn't it better to have the device interface handle this in one place?

Quoted 7 lines

Quoted 3 lines

This patch adds an optional device tree node "flush-ms" which, if present, will use a jiffie-based timer to complete the last pulse stream and allow decode.

A common value for this is 100ms, I'm not sure what use it has to have it configurable. It's nice to have it exposed in rc_dev->timeout.

I'm enough of a n00b regarding the details of the various decoders not to know that...

I looked through the couple of decoders my customer was using (NEC and RC6) and came up with a value of 100ms though...

Implementing this through DT and having the default as 0 (disabled) provides an interim solution if the choice is made to change each of the decoders, since I would expect that to take a while and a bunch of remote control devices for testing.

Quoted 18 lines · Eric Nelson

Thanks Shawn,

Quoted 9 lines · Sean Young

Quoted 7 lines · Eric Nelson

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

Quoted 5 lines

This is a problem other IR drivers suffer from too. It might be better to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, with the duration set to what the timeout was. That is what irraw timeouts are for; much better than fake transitions.

Quoted 3 lines

If I'm understanding this correctly, this would require modification of each decoder to handle what seems to be a special case regarding the GPIO IR driver (which needs an edge to trigger an interrupt).

No, this is not a special case. Many drivers do have extra code to generate some sort of end-of-signal message: redrat3; igorplugusb; st_rc. They don't handle it consistently but this should be fixed.

Secondly, the decoders already handle it. A timeout event matches is_timing_event(), so it's processed by the decoders. The duration should be set correctly.

Quoted 16 lines

Isn't it better to have the device interface handle this in one place?

Quoted 7 lines

Quoted 3 lines

This patch adds an optional device tree node "flush-ms" which, if present, will use a jiffie-based timer to complete the last pulse stream and allow decode.

A common value for this is 100ms, I'm not sure what use it has to have it configurable. It's nice to have it exposed in rc_dev->timeout.

I'm enough of a n00b regarding the details of the various decoders not to know that...

I looked through the couple of decoders my customer was using (NEC and RC6) and came up with a value of 100ms though...

Quoted 4 lines

Implementing this through DT and having the default as 0 (disabled) provides an interim solution if the choice is made to change each of the decoders, since I would expect that to take a while and a bunch of remote control devices for testing.

Many other drivers use 100ms just fine and I don't remember ever seeing any bug reports on that.

Thanks again Sean,

Quoted 23 lines · Sean Young

Quoted 18 lines · Eric Nelson

Thanks Shawn,

Quoted 9 lines · Sean Young

Quoted 7 lines · Eric Nelson

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

Quoted 5 lines

This is a problem other IR drivers suffer from too. It might be better to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, with the duration set to what the timeout was. That is what irraw timeouts are for; much better than fake transitions.

Quoted 3 lines

If I'm understanding this correctly, this would require modification of each decoder to handle what seems to be a special case regarding the GPIO IR driver (which needs an edge to trigger an interrupt).

Quoted 8 lines

No, this is not a special case. Many drivers do have extra code to generate some sort of end-of-signal message: redrat3; igorplugusb; st_rc. They don't handle it consistently but this should be fixed.

Secondly, the decoders already handle it. A timeout event matches is_timing_event(), so it's processed by the decoders. The duration should be set correctly.

I think I did misunderstand you.

You're suggesting that I re-work the patch to gpio-ir-recv.c to produce a timeout instead of an edge. Is that right?

Quoted 21 lines

Quoted 20 lines

Isn't it better to have the device interface handle this in one place?

Quoted 7 lines

Quoted 3 lines

This patch adds an optional device tree node "flush-ms" which, if present, will use a jiffie-based timer to complete the last pulse stream and allow decode.

A common value for this is 100ms, I'm not sure what use it has to have it configurable. It's nice to have it exposed in rc_dev->timeout.

I'm enough of a n00b regarding the details of the various decoders not to know that...

I looked through the couple of decoders my customer was using (NEC and RC6) and came up with a value of 100ms though...

Implementing this through DT and having the default as 0 (disabled) provides an interim solution if the choice is made to change each of the decoders, since I would expect that to take a while and a bunch of remote control devices for testing.

Quoted 3 lines

Many other drivers use 100ms just fine and I don't remember ever seeing any bug reports on that.

So you'd like to see this as a constant?

A default timeout value of 100ms is workable for most decoders. Declare a constant to help standardize its' use.

Signed-off-by Eric Nelson <[email protected]>

---
 include/media/rc-core.h | 1 +
 1 file changed, 1 insertion(+)

include/media/rc-core.h +0 -0

index ec921f6..62c64bd 100644

Em Mon, 21 Sep 2015 12:08:43 -0700
Eric Nelson <[email protected]> escreveu:

Quoted 2 lines

A default timeout value of 100ms is workable for most decoders. Declare a constant to help standardize its' use.

I guess the worse case scenario is the NEC protocol:
http://www.sbprojects.com/knowledge/ir/nec.php

with allows a repeat message to be sent on every 110ms. As the repeat message is 11.25 ms, that would mean a maximum time without data for 98.75 ms. So, in thesis, 100 ms would be ok. However, IR timings are not always precise and may affected by the battery charge.

So, I think that a timeout of 100ms is too close to 98.75 and may cause troubles.

S, IMHO, it is safer to define the default timeout as 125ms.

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

This patch initializes and uses a timer and the timeout field of rcdev to complete the stream and allow decode.

The timeout can be overridden through the use of the LIRC_SET_REC_TIMEOUT ioctl.

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/media/rc/gpio-ir-recv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/media/rc/gpio-ir-recv.c +4 -0

index 7dbc9ca..d3b216a 100644
@@ -30,6 +30,7 @@ struct gpio_rc_dev {
 	struct rc_dev *rcdev;
 	int gpio_nr;
 	bool active_low;
+	struct timer_list flush_timer;
 };
 
 #ifdef CONFIG_OF

@@ -93,12 +94,26 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 	if (rc < 0)
 		goto err_get_value;
 
+	mod_timer(&gpio_dev->flush_timer,
+		  jiffies + nsecs_to_jiffies(gpio_dev->rcdev->timeout));
+
 	ir_raw_event_handle(gpio_dev->rcdev);
 
 err_get_value:

drivers/media/rc/gpio-ir-recv.c +7 -0

+	ir_raw_event_store(gpio_dev->rcdev, &ev);
+	ir_raw_event_handle(gpio_dev->rcdev);
+}
+
 static int gpio_ir_recv_probe(struct platform_device *pdev)
 {
 	struct gpio_rc_dev *gpio_dev;

@@ -144,6 +159,9 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 	rcdev->input_id.version = 0x0100;
 	rcdev->dev.parent = &pdev->dev;
 	rcdev->driver_name = GPIO_IR_DRIVER_NAME;
+	rcdev->min_timeout = 1;
+	rcdev->timeout = IR_DEFAULT_TIMEOUT;
+	rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
 	if (pdata->allowed_protos)
 		rcdev->allowed_protocols = pdata->allowed_protos;
 	else

Quoted 8 lines · Eric Nelson

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

Quoted 5 lines

This patch initializes and uses a timer and the timeout field of rcdev to complete the stream and allow decode.

The timeout can be overridden through the use of the LIRC_SET_REC_TIMEOUT ioctl.

Thanks, this is much nicer.

Quoted 30 lines

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/media/rc/gpio-ir-recv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/media/rc/gpio-ir-recv.c +4 -0

index 7dbc9ca..d3b216a 100644
@@ -30,6 +30,7 @@ struct gpio_rc_dev {
 	struct rc_dev *rcdev;
 	int gpio_nr;
 	bool active_low;
+	struct timer_list flush_timer;
 };
 
 #ifdef CONFIG_OF

@@ -93,12 +94,26 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 	if (rc < 0)
 		goto err_get_value;
 
+	mod_timer(&gpio_dev->flush_timer,
+		  jiffies + nsecs_to_jiffies(gpio_dev->rcdev->timeout));
+
 	ir_raw_event_handle(gpio_dev->rcdev);
 
 err_get_value:
 	return IRQ_HANDLED;
 }
 

drivers/media/rc/gpio-ir-recv.c static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id) · line 112

Quoted 7 lines

drivers/media/rc/gpio-ir-recv.c +7 -0

+static void flush_timer(unsigned long arg)
+{
+	struct gpio_rc_dev *gpio_dev = (struct gpio_rc_dev *)arg;
+	DEFINE_IR_RAW_EVENT(ev);
+
+	ev.timeout = true;
+	ev.duration =  gpio_dev->rcdev->timeout;

Nitpick: two spaces, checkpatch would have found this.

Quoted 21 lines

drivers/media/rc/gpio-ir-recv.c +7 -0

+	ir_raw_event_store(gpio_dev->rcdev, &ev);
+	ir_raw_event_handle(gpio_dev->rcdev);
+}
+
 static int gpio_ir_recv_probe(struct platform_device *pdev)
 {
 	struct gpio_rc_dev *gpio_dev;

@@ -144,6 +159,9 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 	rcdev->input_id.version = 0x0100;
 	rcdev->dev.parent = &pdev->dev;
 	rcdev->driver_name = GPIO_IR_DRIVER_NAME;
+	rcdev->min_timeout = 1;
+	rcdev->timeout = IR_DEFAULT_TIMEOUT;
+	rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
 	if (pdata->allowed_protos)
 		rcdev->allowed_protocols = pdata->allowed_protos;
 	else

@@ -154,6 +172,10 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 	gpio_dev->gpio_nr = pdata->gpio_nr;
 	gpio_dev->active_low = pdata->active_low;
 

drivers/media/rc/gpio-ir-recv.c static int gpio_ir_recv_probe(struct platform_device *pdev) · line 177

Quoted 3 lines

drivers/media/rc/gpio-ir-recv.c +3 -0

+	init_timer(&gpio_dev->flush_timer);
+	gpio_dev->flush_timer.function = flush_timer;
+	gpio_dev->flush_timer.data = (unsigned long)gpio_dev;

You could use "setup_timer(&gpio_dev->flush_timer, flush_timer, (unsigned long)gpio_dev);" here.

drivers/media/rc/gpio-ir-recv.c static int gpio_ir_recv_probe(struct platform_device *pdev) · line 181

Quoted 4 lines

drivers/media/rc/gpio-ir-recv.c +1 -0

+
 	rc = gpio_request(pdata->gpio_nr, "gpio-ir-recv");
 	if (rc < 0)
 		goto err_gpio_request;

You'll need a "del_timer_sync(&gpio_dev->flush_timer);" in gpio_ir_recv_remove() or you'll have a race on remove.

Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode.

Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver.

This patch initializes and uses a timer and the timeout field of rcdev to complete the stream and allow decode.

The timeout can be overridden through the use of the LIRC_SET_REC_TIMEOUT ioctl.

Signed-off-by Eric Nelson <[email protected]>

Add timeout support to the gpio-ir-recv driver as discussed in this original patch:

	https://patchwork.ozlabs.org/patch/516827/

V2 uses the timeout field of the rcdev instead of a device tree field to set the timeout value as suggested by Sean Young.

Eric Nelson (2):
rc-core: define a default timeout for drivers
rc: gpio-ir-recv: add timeout on idle

Em Mon, 21 Sep 2015 12:08:42 -0700
Eric Nelson <[email protected]> escreveu:

Quoted 8 lines

Add timeout support to the gpio-ir-recv driver as discussed in this original patch:

	https://patchwork.ozlabs.org/patch/516827/

V2 uses the timeout field of the rcdev instead of a device tree field to set the timeout value as suggested by Sean Young.

Quoted 3 lines

Eric Nelson (2):
rc-core: define a default timeout for drivers
rc: gpio-ir-recv: add timeout on idle

I'm ok on having a default timeout for drivers, but the better would be to implement it at the RC core, and not inside each driver.