[RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[David Härdeman]

[-- Attachment #1: use-pulse-space-timings-in-ir-raw --]
[-- Type: text/plain, Size: 25358 bytes --]

drivers/media/IR/ir-raw-event.c is currently written with the assumption that
all "raw" hardware will generate events only on state change (i.e. when
a pulse or space starts).

However, some hardware (like mceusb, probably the most popular IR receiver
out there) only generates duration data (and that data is buffered so using
any kind of timing on the data is futile).

This patch (which is not tested since I haven't yet converted a driver for
any of my hardware to ir-core yet, will do soon) is a RFC on my proposed
interface change...once I get the green light on the interface change itself
I'll make sure that the decoders actually work :)

The rc5 decoder has also gained rc5x support and the use of kfifo's for
intermediate storage is gone (since there is no need for it).

diffstat:
 drivers/media/IR/ir-nec-decoder.c           |  229 +++++++++++-----------------
 drivers/media/IR/ir-raw-event.c             |  143 ++++++++---------
 drivers/media/IR/ir-rc5-decoder.c           |  204 +++++++++++++++---------
 drivers/media/video/saa7134/saa7134-input.c |    4 
 include/media/ir-core.h                     |   18 --
 5 files changed, 291 insertions(+), 307 deletions(-)



Index: ir/drivers/media/IR/ir-raw-event.c
===================================================================
--- ir.orig/drivers/media/IR/ir-raw-event.c	2010-04-06 12:16:27.000000000 +0200
+++ ir/drivers/media/IR/ir-raw-event.c	2010-04-06 12:17:08.856877124 +0200
@@ -15,13 +15,11 @@
 #include <media/ir-core.h>
 #include <linux/workqueue.h>
 #include <linux/spinlock.h>
-
-/* Define the max number of bit transitions per IR keycode */
-#define MAX_IR_EVENT_SIZE	256
+#include <linux/sched.h>
 
 /* Used to handle IR raw handler extensions */
 static LIST_HEAD(ir_raw_handler_list);
-static spinlock_t ir_raw_handler_lock;
+static DEFINE_SPINLOCK(ir_raw_handler_lock);
 
 /**
  * RUN_DECODER()	- runs an operation on all IR decoders
@@ -56,25 +54,14 @@
 int ir_raw_event_register(struct input_dev *input_dev)
 {
 	struct ir_input_dev *ir = input_get_drvdata(input_dev);
-	int rc, size;
+	int rc;
 
 	ir->raw = kzalloc(sizeof(*ir->raw), GFP_KERNEL);
 	if (!ir->raw)
 		return -ENOMEM;
 
-	size = sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE * 2;
-	size = roundup_pow_of_two(size);
-
-	rc = kfifo_alloc(&ir->raw->kfifo, size, GFP_KERNEL);
-	if (rc < 0) {
-		kfree(ir->raw);
-		ir->raw = NULL;
-		return rc;
-	}
-
 	rc = RUN_DECODER(raw_register, input_dev);
 	if (rc < 0) {
-		kfifo_free(&ir->raw->kfifo);
 		kfree(ir->raw);
 		ir->raw = NULL;
 		return rc;
@@ -93,82 +80,84 @@
 
 	RUN_DECODER(raw_unregister, input_dev);
 
-	kfifo_free(&ir->raw->kfifo);
 	kfree(ir->raw);
 	ir->raw = NULL;
 }
 EXPORT_SYMBOL_GPL(ir_raw_event_unregister);
 
-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
+/**
+ * ir_raw_event_reset() - resets the raw decoding state machines
+ * @input_dev:	the struct input_dev device descriptor
+ *
+ * This routine resets the raw ir decoding state machines, useful e.g. when
+ * a timeout occurs or when resetting the hardware.
+ */
+void ir_raw_event_reset(struct input_dev *input_dev)
 {
-	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
-	struct timespec		ts;
-	struct ir_raw_event	event;
-	int			rc;
-
-	if (!ir->raw)
-		return -EINVAL;
-
-	event.type = type;
-	event.delta.tv_sec = 0;
-	event.delta.tv_nsec = 0;
-
-	ktime_get_ts(&ts);
-
-	if (timespec_equal(&ir->raw->last_event, &event.delta))
-		event.type |= IR_START_EVENT;
-	else
-		event.delta = timespec_sub(ts, ir->raw->last_event);
-
-	memcpy(&ir->raw->last_event, &ts, sizeof(ts));
+	RUN_DECODER(reset, input_dev);
+}
+EXPORT_SYMBOL_GPL(ir_raw_event_reset);
 
-	if (event.delta.tv_sec) {
-		event.type |= IR_START_EVENT;
-		event.delta.tv_sec = 0;
-		event.delta.tv_nsec = 0;
-	}
+/**
+ * ir_raw_event_duration() - pass a pulse/space duration to the raw ir decoders
+ * @input_dev:	the struct input_dev device descriptor
+ * @duration:	duration of the pulse or space
+ *
+ * This routine passes a pulse/space duration to the raw ir decoding state
+ * machines. Pulses are signalled as positive values and spaces as negative
+ * values.
+ */
+void ir_raw_event_duration(struct input_dev *input_dev, int duration)
+{
+	struct ir_input_dev *ir = input_get_drvdata(input_dev);
 
-	kfifo_in(&ir->raw->kfifo, &event, sizeof(event));
+	if (!ir->raw || duration == 0)
+		return;
 
-	return rc;
+	RUN_DECODER(decode, input_dev, duration);
 }
-EXPORT_SYMBOL_GPL(ir_raw_event_store);
+EXPORT_SYMBOL_GPL(ir_raw_event_duration);
 
-int ir_raw_event_handle(struct input_dev *input_dev)
+/**
+ * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
+ * @input_dev:	the struct input_dev device descriptor
+ * @type:	the type of the event that has occurred
+ *
+ * This routine is used to notify the raw ir decoders on the beginning of an
+ * ir pulse or space (or the start/end of ir reception). This is used by
+ * hardware which does not provide durations directly but only interrupts
+ * (or similar events) on state change.
+ */
+void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
 {
-	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
-	int				rc;
-	struct ir_raw_event		ev;
-	int 				len, i;
-
-	/*
-	 * Store the events into a temporary buffer. This allows calling more than
-	 * one decoder to deal with the received data
-	 */
-	len = kfifo_len(&ir->raw->kfifo) / sizeof(ev);
-	if (!len)
-		return 0;
-
-	for (i = 0; i < len; i++) {
-		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
-		if (rc != sizeof(ev)) {
-			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
-				   rc, sizeof(ev));
-			return -EINVAL;
-		}
-		IR_dprintk(2, "event type %d, time before event: %07luus\n",
-			ev.type, (ev.delta.tv_nsec + 500) / 1000);
-		rc = RUN_DECODER(decode, input_dev, &ev);
-	}
+	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
+	ktime_t			now;
+	s64			delta; /* us */
+
+	if (!ir->raw)
+		return;
 
-	/*
-	 * Call all ir decoders. This allows decoding the same event with
-	 * more than one protocol handler.
-	 */
+	now = ktime_get();
+	delta = ktime_us_delta(now, ir->raw->last_event);
 
-	return rc;
+	/* Check for a long duration since last event or if we're
+	   being called for the first time */
+	if (delta > USEC_PER_SEC || !ir->raw->last_type)
+		type |= IR_START_EVENT;
+
+	if (type & IR_START_EVENT)
+		ir_raw_event_reset(input_dev);
+	else if (ir->raw->last_type & IR_SPACE)
+		ir_raw_event_duration(input_dev, (int)-delta);
+	else if (ir->raw->last_type & IR_PULSE)
+		ir_raw_event_duration(input_dev, (int)delta);
+	else
+		return;
+
+	ir->raw->last_event = now;
+	ir->raw->last_type = type;
 }
-EXPORT_SYMBOL_GPL(ir_raw_event_handle);
+EXPORT_SYMBOL_GPL(ir_raw_event_edge);
 
 /*
  * Extension interface - used to register the IR decoders
Index: ir/include/media/ir-core.h
===================================================================
--- ir.orig/include/media/ir-core.h	2010-04-06 12:16:27.000000000 +0200
+++ ir/include/media/ir-core.h	2010-04-06 12:17:08.856877124 +0200
@@ -57,14 +57,9 @@
 	void		(*close)(void *priv);
 };
 
-struct ir_raw_event {
-	struct timespec		delta;	/* Time spent before event */
-	enum raw_event_type	type;	/* event type */
-};
-
 struct ir_raw_event_ctrl {
-	struct kfifo			kfifo;		/* fifo for the pulse/space events */
-	struct timespec			last_event;	/* when last event occurred */
+	ktime_t				last_event;	/* when last event occurred */
+	enum raw_event_type		last_type;	/* last event type */
 };
 
 struct ir_input_dev {
@@ -89,8 +84,8 @@
 struct ir_raw_handler {
 	struct list_head list;
 
-	int (*decode)(struct input_dev *input_dev,
-		      struct ir_raw_event *ev);
+	int (*decode)(struct input_dev *input_dev, int duration);
+	int (*reset)(struct input_dev *input_dev);
 	int (*raw_register)(struct input_dev *input_dev);
 	int (*raw_unregister)(struct input_dev *input_dev);
 };
@@ -146,8 +141,9 @@
 /* Routines from ir-raw-event.c */
 int ir_raw_event_register(struct input_dev *input_dev);
 void ir_raw_event_unregister(struct input_dev *input_dev);
-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type);
-int ir_raw_event_handle(struct input_dev *input_dev);
+void ir_raw_event_reset(struct input_dev *input_dev);
+void ir_raw_event_duration(struct input_dev *input_dev, int duration);
+void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type);
 int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
 void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);
 void ir_raw_init(void);
Index: ir/drivers/media/IR/ir-nec-decoder.c
===================================================================
--- ir.orig/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:16:27.000000000 +0200
+++ ir/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:17:08.860846045 +0200
@@ -14,22 +14,25 @@
 
 #include <media/ir-core.h>
 
+/*
+ * Regarding NEC_HEADER_MARK: some NEC remotes use 16, some 8,
+ * some receivers are also good at missing part of the first pulse.
+ */
 #define NEC_NBITS		32
-#define NEC_UNIT		559979 /* ns */
-#define NEC_HEADER_MARK		(16 * NEC_UNIT)
-#define NEC_HEADER_SPACE	(8 * NEC_UNIT)
-#define NEC_REPEAT_SPACE	(4 * NEC_UNIT)
-#define NEC_MARK		(NEC_UNIT)
-#define NEC_0_SPACE		(NEC_UNIT)
-#define NEC_1_SPACE		(3 * NEC_UNIT)
+#define NEC_UNIT		562	/* us */
+#define NEC_HEADER_MARK		6
+#define NEC_HEADER_SPACE	-8
+#define NEC_REPEAT_SPACE	-4
+#define NEC_MARK		1
+#define NEC_0_SPACE		-1
+#define NEC_1_SPACE		-3
 
 /* Used to register nec_decoder clients */
 static LIST_HEAD(decoder_list);
-static spinlock_t decoder_lock;
+static DEFINE_SPINLOCK(decoder_lock);
 
 enum nec_state {
 	STATE_INACTIVE,
-	STATE_HEADER_MARK,
 	STATE_HEADER_SPACE,
 	STATE_MARK,
 	STATE_SPACE,
@@ -37,11 +40,14 @@
 	STATE_TRAILER_SPACE,
 };
 
-struct nec_code {
-	u8	address;
-	u8	not_address;
-	u8	command;
-	u8	not_command;
+union nec_code {
+	struct {
+		u8 address;
+		u8 not_address;
+		u8 command;
+		u8 not_command;
+	} parts;
+	u32 bits;
 };
 
 struct decoder_data {
@@ -51,7 +57,7 @@
 
 	/* State machine control */
 	enum nec_state		state;
-	struct nec_code		nec_code;
+	union nec_code		nec_code;
 	unsigned		count;
 };
 
@@ -62,7 +68,6 @@
  *
  * Returns the struct decoder_data that corresponds to a device
  */
-
 static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
 {
 	struct decoder_data *data = NULL;
@@ -123,20 +128,30 @@
 	.attrs	= decoder_attributes,
 };
 
+static int ir_nec_reset(struct input_dev *input_dev)
+{
+	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct decoder_data *data;
+
+	data = get_decoder_data(ir_dev);
+	if (data)
+		data->state = STATE_INACTIVE;
+	return 0;
+}
 
 /**
  * ir_nec_decode() - Decode one NEC pulse or space
  * @input_dev:	the struct input_dev descriptor of the device
- * @ev:		event array with type/duration of pulse/space
+ * @duration:	duration in us of pulse/space
  *
  * This function returns -EINVAL if the pulse violates the state machine
  */
-static int ir_nec_decode(struct input_dev *input_dev,
-			 struct ir_raw_event *ev)
+static int ir_nec_decode(struct input_dev *input_dev, int duration)
 {
 	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	int bit, last_bit;
+	int d;
+	u32 scancode;
 
 	data = get_decoder_data(ir_dev);
 	if (!data)
@@ -145,150 +160,93 @@
 	if (!data->enabled)
 		return 0;
 
-	/* Except for the initial event, what matters is the previous bit */
-	bit = (ev->type & IR_PULSE) ? 1 : 0;
-
-	last_bit = !bit;
-
-	/* Discards spurious space last_bits when inactive */
-
-	/* Very long delays are considered as start events */
-	if (ev->delta.tv_nsec > NEC_HEADER_MARK + NEC_HEADER_SPACE - NEC_UNIT / 2)
-		data->state = STATE_INACTIVE;
-
-	if (ev->type & IR_START_EVENT)
-		data->state = STATE_INACTIVE;
+	d = DIV_ROUND_CLOSEST(abs(duration), NEC_UNIT);
+	if (duration < 0)
+		d = -d;
 
 	switch (data->state) {
-	case STATE_INACTIVE:
-		if (!bit)		/* PULSE marks the start event */
-			return 0;
 
-		data->count = 0;
-		data->state = STATE_HEADER_MARK;
-		memset (&data->nec_code, 0, sizeof(data->nec_code));
-		return 0;
-	case STATE_HEADER_MARK:
-		if (!last_bit)
-			goto err;
-		if (ev->delta.tv_nsec < NEC_HEADER_MARK - 6 * NEC_UNIT)
-			goto err;
-		data->state = STATE_HEADER_SPACE;
+	case STATE_INACTIVE:
+		if (d > NEC_HEADER_MARK) {
+			data->count = 0;
+			data->state = STATE_HEADER_SPACE;
+		}
 		return 0;
+
 	case STATE_HEADER_SPACE:
-		if (last_bit)
-			goto err;
-		if (ev->delta.tv_nsec >= NEC_HEADER_SPACE - NEC_UNIT / 2) {
+		if (d == NEC_HEADER_SPACE) {
 			data->state = STATE_MARK;
 			return 0;
-		}
-
-		if (ev->delta.tv_nsec >= NEC_REPEAT_SPACE - NEC_UNIT / 2) {
+		} else if (d == NEC_REPEAT_SPACE) {
 			ir_repeat(input_dev);
 			IR_dprintk(1, "Repeat last key\n");
 			data->state = STATE_TRAILER_MARK;
 			return 0;
 		}
-		goto err;
-	case STATE_MARK:
-		if (!last_bit)
-			goto err;
-		if ((ev->delta.tv_nsec > NEC_MARK + NEC_UNIT / 2) ||
-		    (ev->delta.tv_nsec < NEC_MARK - NEC_UNIT / 2))
-			goto err;
-		data->state = STATE_SPACE;
-		return 0;
-	case STATE_SPACE:
-		if (last_bit)
-			goto err;
-
-		if ((ev->delta.tv_nsec >= NEC_0_SPACE - NEC_UNIT / 2) &&
-		    (ev->delta.tv_nsec < NEC_0_SPACE + NEC_UNIT / 2))
-			bit = 0;
-		else if ((ev->delta.tv_nsec >= NEC_1_SPACE - NEC_UNIT / 2) &&
-		         (ev->delta.tv_nsec < NEC_1_SPACE + NEC_UNIT / 2))
-			bit = 1;
-		else {
-			IR_dprintk(1, "Decode failed at %d-th bit (%s) @%luus\n",
-				data->count,
-				last_bit ? "pulse" : "space",
-				(ev->delta.tv_nsec + 500) / 1000);
+		break;
 
-			goto err2;
+	case STATE_MARK:
+		if (d == NEC_MARK) {
+			data->state = STATE_SPACE;
+			return 0;
 		}
+		break;
 
-		/* Ok, we've got a valid bit. proccess it */
-		if (bit) {
-			int shift = data->count;
-
-			/*
-			 * NEC transmit bytes on this temporal order:
-			 * address | not address | command | not command
-			 */
-			if (shift < 8) {
-				data->nec_code.address |= 1 << shift;
-			} else if (shift < 16) {
-				data->nec_code.not_address |= 1 << (shift - 8);
-			} else if (shift < 24) {
-				data->nec_code.command |= 1 << (shift - 16);
-			} else {
-				data->nec_code.not_command |= 1 << (shift - 24);
+	case STATE_SPACE:
+		if (d == NEC_0_SPACE || d == NEC_1_SPACE) {
+			data->nec_code.bits <<= 1;
+			if (d == NEC_1_SPACE)
+				data->nec_code.bits |= 1;
+			data->count++;
+
+			if (data->count != NEC_NBITS)
+				return 0;
+
+			if ((data->nec_code.parts.command ^ data->nec_code.parts.not_command) != 0xff) {
+				IR_dprintk(1, "NEC checksum error: received 0x%08x\n",
+					   data->nec_code.bits);
+				goto out;
 			}
-		}
-		if (++data->count == NEC_NBITS) {
-			u32 scancode;
-			/*
-			 * Fixme: may need to accept Extended NEC protocol?
-			 */
-			if ((data->nec_code.command ^ data->nec_code.not_command) != 0xff)
-				goto checksum_err;
 
-			if ((data->nec_code.address ^ data->nec_code.not_address) != 0xff) {
+			if ((data->nec_code.parts.address ^ data->nec_code.parts.not_address) != 0xff) {
 				/* Extended NEC */
-				scancode = data->nec_code.address << 16 |
-					   data->nec_code.not_address << 8 |
-					   data->nec_code.command;
-				IR_dprintk(1, "NEC scancode 0x%06x\n", scancode);
+				scancode = data->nec_code.bits >> 8;
+				IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
 			} else {
 				/* normal NEC */
-				scancode = data->nec_code.address << 8 |
-					   data->nec_code.command;
+				scancode = data->nec_code.parts.address << 8 |
+					   data->nec_code.parts.command;
 				IR_dprintk(1, "NEC scancode 0x%04x\n", scancode);
 			}
 			ir_keydown(input_dev, scancode, 0);
-
 			data->state = STATE_TRAILER_MARK;
-		} else
-			data->state = STATE_MARK;
-		return 0;
+			return 0;
+		}
+
+		IR_dprintk(1, "Decode failed at %d-th bit (%i units, %ius)\n",
+			   data->count, d, duration);
+		goto out;
+
 	case STATE_TRAILER_MARK:
-		if (!last_bit)
-			goto err;
-		data->state = STATE_TRAILER_SPACE;
-		return 0;
+		if (d > 0) {
+			data->state = STATE_TRAILER_SPACE;
+			return 0;
+		}
+		break;
+
 	case STATE_TRAILER_SPACE:
-		if (last_bit)
-			goto err;
-		data->state = STATE_INACTIVE;
-		return 0;
-	}
+		if (d < 0) {
+			data->state = STATE_INACTIVE;
+			return 0;
+		}
 
-err:
-	IR_dprintk(1, "NEC decoded failed at state %d (%s) @ %luus\n",
-		   data->state,
-		   bit ? "pulse" : "space",
-		   (ev->delta.tv_nsec + 500) / 1000);
-err2:
-	data->state = STATE_INACTIVE;
-	return -EINVAL;
+		break;
+	}
 
-checksum_err:
+	IR_dprintk(1, "NEC decode failed at state %d (%i units, %ius)\n",
+		   data->state, d, duration);
+out:
 	data->state = STATE_INACTIVE;
-	IR_dprintk(1, "NEC checksum error: received 0x%02x%02x%02x%02x\n",
-		   data->nec_code.address,
-		   data->nec_code.not_address,
-		   data->nec_code.command,
-		   data->nec_code.not_command);
 	return -EINVAL;
 }
 
@@ -337,6 +295,7 @@
 }
 
 static struct ir_raw_handler nec_handler = {
+	.reset		= ir_nec_reset,
 	.decode		= ir_nec_decode,
 	.raw_register	= ir_nec_register,
 	.raw_unregister	= ir_nec_unregister,
Index: ir/drivers/media/IR/ir-rc5-decoder.c
===================================================================
--- ir.orig/drivers/media/IR/ir-rc5-decoder.c	2010-04-06 12:16:51.784849187 +0200
+++ ir/drivers/media/IR/ir-rc5-decoder.c	2010-04-06 12:25:20.968874462 +0200
@@ -15,31 +15,22 @@
 /*
  * This code only handles 14 bits RC-5 protocols. There are other variants
  * that use a different number of bits. This is currently unsupported
- * It considers a carrier of 36 kHz, with a total of 14 bits, where
- * the first two bits are start bits, and a third one is a filing bit
  */
 
 #include <media/ir-core.h>
 
-static unsigned int ir_rc5_remote_gap = 888888;
-
-#define RC5_NBITS		14
-#define RC5_BIT			(ir_rc5_remote_gap * 2)
-#define RC5_DURATION		(ir_rc5_remote_gap * RC5_NBITS)
+#define RC5_UNIT		889 /* us */
 
 /* Used to register rc5_decoder clients */
 static LIST_HEAD(decoder_list);
-static spinlock_t decoder_lock;
+static DEFINE_SPINLOCK(decoder_lock);
 
 enum rc5_state {
 	STATE_INACTIVE,
-	STATE_MARKSPACE,
-	STATE_TRAILER,
-};
-
-struct rc5_code {
-	u8	address;
-	u8	command;
+	STATE_BIT_START,
+	STATE_BIT_END,
+	STATE_CHECK_RC5X,
+	STATE_FINISHED,
 };
 
 struct decoder_data {
@@ -49,8 +40,10 @@
 
 	/* State machine control */
 	enum rc5_state		state;
-	struct rc5_code		rc5_code;
-	unsigned		code, elapsed, last_bit, last_code;
+	u32			rc5_bits;
+	int			last_delta;
+	unsigned		count;
+	unsigned		wanted_bits;
 };
 
 
@@ -122,18 +115,35 @@
 };
 
 /**
- * handle_event() - Decode one RC-5 pulse or space
+ * ir_rc5_reset() - reset the RC5 state machine
  * @input_dev:	the struct input_dev descriptor of the device
- * @ev:		event array with type/duration of pulse/space
+ *
+ * This function is used to reset the RC5 state machine, e.g. on hw
+ * reset.
+ */
+static int ir_rc5_reset(struct input_dev *input_dev)
+{
+	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct decoder_data *data;
+
+	data = get_decoder_data(ir_dev);
+	if (data)
+		data->state = STATE_INACTIVE;
+	return 0;
+}
+
+/**
+ * ir_rc5_decode() - Decode one RC-5 pulse or space
+ * @input_dev:	the struct input_dev descriptor of the device
+ * @duration:	duration of pulse/space
  *
  * This function returns -EINVAL if the pulse violates the state machine
  */
-static int ir_rc5_decode(struct input_dev *input_dev,
-			struct ir_raw_event *ev)
+static int ir_rc5_decode(struct input_dev *input_dev, int duration)
 {
 	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	int is_pulse, scancode, delta, toggle;
+	int d;
 
 	data = get_decoder_data(ir_dev);
 	if (!data)
@@ -142,79 +152,110 @@
 	if (!data->enabled)
 		return 0;
 
-	delta = DIV_ROUND_CLOSEST(ev->delta.tv_nsec, ir_rc5_remote_gap);
+	d = DIV_ROUND_CLOSEST(abs(duration), RC5_UNIT);
+	if (duration < 0)
+		d = -d;
 
-	/* The duration time refers to the last bit time */
-	is_pulse = (ev->type & IR_PULSE) ? 1 : 0;
-
-	/* Very long delays are considered as start events */
-	if (delta > RC5_DURATION || (ev->type & IR_START_EVENT))
-		data->state = STATE_INACTIVE;
+again:
+	if (d == 0 && data->state != STATE_FINISHED)
+		return 0;
 
 	switch (data->state) {
+
 	case STATE_INACTIVE:
-	IR_dprintk(2, "currently inative. Start bit (%s) @%uus\n",
-		   is_pulse ? "pulse" : "space",
-		   (unsigned)(ev->delta.tv_nsec + 500) / 1000);
-
-		/* Discards the initial start space */
-		if (!is_pulse)
-			goto err;
-		data->code = 1;
-		data->last_bit = 1;
-		data->elapsed = 0;
-		memset(&data->rc5_code, 0, sizeof(data->rc5_code));
-		data->state = STATE_MARKSPACE;
-		return 0;
-	case STATE_MARKSPACE:
-		if (delta != 1)
-			data->last_bit = data->last_bit ? 0 : 1;
+		if ((d == 1) || (d == 2)) {
+			data->state = STATE_BIT_START;
+			/* We need enough bits to get to STATE_CHECK_RC5X */
+			data->wanted_bits = 19;
+			data->count = 0;
+			d--;
+			goto again;
+		}
+		break;
 
-		data->elapsed += delta;
+	case STATE_BIT_START:
+		if (abs(d) == 1) {
+			data->rc5_bits <<= 1;
+			if (d == -1)
+				data->rc5_bits |= 1;
+			data->count++;
+
+			/*
+			 * If the last bit is zero, a space will "merge"
+			 * with the silence after the command.
+			 */
+			if ((data->count == data->wanted_bits) && (d == 1))
+				data->state = STATE_FINISHED;
+			else
+				data->state = STATE_BIT_END;
 
-		if ((data->elapsed % 2) == 1)
 			return 0;
+		}
+		break;
+
+	case STATE_BIT_END:
+		/* delta and last_delta signedness must differ */
+		if (d * data->last_delta < 0) {
+			if (data->count == data->wanted_bits)
+				data->state = STATE_FINISHED;
+			else if (data->count == 7)
+				data->state = STATE_CHECK_RC5X;
+			else
+				data->state = STATE_BIT_START;
+
+			if (d > 0)
+				d--;
+			else if (d < 0)
+				d++;
+			goto again;
+		}
+		break;
 
-		data->code <<= 1;
-		data->code |= data->last_bit;
+	case STATE_CHECK_RC5X:
+		if (d <= -4) {
+			/* RC5X */
+			data->wanted_bits = 19;
+			d += 4;
+		} else {
+			/* RC5 */
+			data->wanted_bits = 13;
+		}
+		data->state = STATE_BIT_START;
+		goto again;
 
-		/* Fill the 2 unused bits at the command with 0 */
-		if (data->elapsed / 2 == 6)
-			data->code <<= 2;
-
-		if (data->elapsed >= (RC5_NBITS - 1) * 2) {
-			scancode = data->code;
-
-			/* Check for the start bits */
-			if ((scancode & 0xc000) != 0xc000) {
-				IR_dprintk(1, "Code 0x%04x doesn't have two start bits. It is not RC-5\n", scancode);
-				goto err;
-			}
-
-			toggle = (scancode & 0x2000) ? 1 : 0;
-
-			if (scancode == data->last_code) {
-				IR_dprintk(1, "RC-5 repeat\n");
-				ir_repeat(input_dev);
-			} else {
-				data->last_code = scancode;
-				scancode &= 0x1fff;
-				IR_dprintk(1, "RC-5 scancode 0x%04x\n", scancode);
-
-				ir_keydown(input_dev, scancode, 0);
-			}
-			data->state = STATE_TRAILER;
+	case STATE_FINISHED:
+		if (data->wanted_bits == 19) {
+			u8 xdata, command, system, toggle;
+			u32 scancode;
+			xdata    = (data->rc5_bits & 0x0003F) >> 0;
+			command  = (data->rc5_bits & 0x00FC0) >> 6;
+			system   = (data->rc5_bits & 0x1F000) >> 12;
+			toggle   = (data->rc5_bits & 0x20000) ? 1 : 0;
+			command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
+			scancode = system << 16 | command << 8 | xdata;
+
+			IR_dprintk(1, "RC5X scancode 0x%06x (toggle: %u)\n",
+				   scancode, toggle);
+			ir_keydown(input_dev, scancode, toggle);
+		} else {
+			u8 command, system, toggle;
+			u32 scancode;
+                        command  = (data->rc5_bits & 0x0003F) >> 0;
+                        system   = (data->rc5_bits & 0x007C0) >> 6;
+                        toggle   = (data->rc5_bits & 0x00800) ? 1 : 0;
+                        command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
+			scancode = system << 8 | command;
+
+			IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n",
+				   scancode, toggle);
+			ir_keydown(input_dev, scancode, toggle);
 		}
-		return 0;
-	case STATE_TRAILER:
 		data->state = STATE_INACTIVE;
 		return 0;
 	}
 
-err:
-	IR_dprintk(1, "RC-5 decoded failed at %s @ %luus\n",
-		   is_pulse ? "pulse" : "space",
-		   (ev->delta.tv_nsec + 500) / 1000);
+	IR_dprintk(1, "RC5(X) decode failed at state %i (%i units, %uus)\n",
+		   data->state, d, duration);
 	data->state = STATE_INACTIVE;
 	return -EINVAL;
 }
@@ -264,6 +305,7 @@
 }
 
 static struct ir_raw_handler rc5_handler = {
+	.reset		= ir_rc5_reset,
 	.decode		= ir_rc5_decode,
 	.raw_register	= ir_rc5_register,
 	.raw_unregister	= ir_rc5_unregister,
Index: ir/drivers/media/video/saa7134/saa7134-input.c
===================================================================
--- ir.orig/drivers/media/video/saa7134/saa7134-input.c	2010-04-06 12:30:16.428854774 +0200
+++ ir/drivers/media/video/saa7134/saa7134-input.c	2010-04-06 12:34:44.701888094 +0200
@@ -433,8 +433,6 @@
 	struct saa7134_dev *dev = (struct saa7134_dev *)data;
 	struct card_ir *ir = dev->remote;
 
-	ir_raw_event_handle(dev->remote->dev);
-
 	ir->active = 0;
 }
 
@@ -1021,7 +1019,7 @@
 	saa_clearb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
 	saa_setb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
 	space = saa_readl(SAA7134_GPIO_GPSTATUS0 >> 2) & ir->mask_keydown;
-	ir_raw_event_store(dev->remote->dev, space ? IR_SPACE : IR_PULSE);
+	ir_raw_event_edge(dev->remote->dev, space ? IR_SPACE : IR_PULSE);
 
 
 	/*

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Mauro Carvalho Chehab]

Hi David,

Em Tue, 6 Apr 2010 12:48:11 +0200
David Härdeman <david@hardeman.nu> escreveu:

> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline; filename=use-pulse-space-timings-in-ir-raw

Thunderbird 2 really don't like this. It considers the entire body as a file, and
refuses to quote it.

> drivers/media/IR/ir-raw-event.c is currently written with the assumption that
> all "raw" hardware will generate events only on state change (i.e. when
> a pulse or space starts).
> 
> However, some hardware (like mceusb, probably the most popular IR receiver
> out there) only generates duration data (and that data is buffered so using
> any kind of timing on the data is futile).

Am I understanding right and this hardware is not capable of indicating if the 
event is a pulse or a space? It seems hard to auto-detect what is pulse or space,
but IMO such code should belong to mceusb driver and not to the decoders.

Based on the code changes you did, I suspect that one of the things the hardware
provides is a "machine reset" state, right? If you just need to add a code to reset
the state machines, this could be done as easily as adding an event at kfifo with
IR_STOP_EVENT. A three line addition at the decoders event handler would be enough
to use it to reset the state machine:

	if (ev->type & IR_STOP_EVENT) {
		data->state = STATE_INACTIVE;
		return;
	}

This event were not added yet, since no hardware currently ported needs it. Eventually,
we may rename it to IR_RESET_STATE, if you think it is clearer.

> This patch (which is not tested since I haven't yet converted a driver for
> any of my hardware to ir-core yet, will do soon) is a RFC on my proposed
> interface change...once I get the green light on the interface change itself
> I'll make sure that the decoders actually work :)

Yes, better to discuss before changing everything ;)

> The rc5 decoder has also gained rc5x support and the use of kfifo's for
> intermediate storage is gone (since there is no need for it).

The RC-5X addition is welcome, but the better is to add it as a separate patch. 

I won't comment every single bits of the change, since we're more interested on the conceptual
aspects.

> -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

Don't remove the raw_event_store. It is needed by the hardware that gets events from
IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their
own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).

For those, we need something like:

int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)

Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

>  
> -int ir_raw_event_handle(struct input_dev *input_dev)
> +/**
> + * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
> + * @input_dev:	the struct input_dev device descriptor
> + * @type:	the type of the event that has occurred
> + *
> + * This routine is used to notify the raw ir decoders on the beginning of an
> + * ir pulse or space (or the start/end of ir reception). This is used by
> + * hardware which does not provide durations directly but only interrupts
> + * (or similar events) on state change.
> + */
> +void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
>  {
> -	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
> -	int				rc;
> -	struct ir_raw_event		ev;
> -	int 				len, i;
> -
> -	/*
> -	 * Store the events into a temporary buffer. This allows calling more than
> -	 * one decoder to deal with the received data
> -	 */
> -	len = kfifo_len(&ir->raw->kfifo) / sizeof(ev);
> -	if (!len)
> -		return 0;

The removal of kfifo is not a good idea. On several drivers, the event is generated during
IRQ time, or on a very expensive polling loop. So, buffering is needed to release the
IRQ as soon as possible and not adding too much processing during polling.

> -
> -	for (i = 0; i < len; i++) {
> -		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
> -		if (rc != sizeof(ev)) {
> -			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
> -				   rc, sizeof(ev));
> -			return -EINVAL;
> -		}
> -		IR_dprintk(2, "event type %d, time before event: %07luus\n",
> -			ev.type, (ev.delta.tv_nsec + 500) / 1000);
> -		rc = RUN_DECODER(decode, input_dev, &ev);
> -	}
> +	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
> +	ktime_t			now;
> +	s64			delta; /* us */
> +
> +	if (!ir->raw)
> +		return;
>  
> -	/*
> -	 * Call all ir decoders. This allows decoding the same event with
> -	 * more than one protocol handler.
> -	 */
> +	now = ktime_get();
> +	delta = ktime_us_delta(now, ir->raw->last_event);


This won't work, in the cases where the hardware is providing its own timings.

>  
> -	return rc;
> +	/* Check for a long duration since last event or if we're
> +	   being called for the first time */
> +	if (delta > USEC_PER_SEC || !ir->raw->last_type)
> +		type |= IR_START_EVENT;

The "long duration" concept would be better implemented at the driver, since it may
vary with the IR carrier and with the protocol details.

> +
> +	if (type & IR_START_EVENT)
> +		ir_raw_event_reset(input_dev);
> +	else if (ir->raw->last_type & IR_SPACE)
> +		ir_raw_event_duration(input_dev, (int)-delta);
> +	else if (ir->raw->last_type & IR_PULSE)
> +		ir_raw_event_duration(input_dev, (int)delta);

Please, don't use a signal to identify between pulse and space. The IR decoding logic
is tricky enough. Just pass the type to the decoder and let it explicitly check if it is
pulse or space.


> Index: ir/drivers/media/IR/ir-nec-decoder.c
> ===================================================================
> --- ir.orig/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:16:27.000000000 +0200
> +++ ir/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:17:08.860846045 +0200
> @@ -14,22 +14,25 @@
>  
>  #include <media/ir-core.h>
>  
> +/*
> + * Regarding NEC_HEADER_MARK: some NEC remotes use 16, some 8,
> + * some receivers are also good at missing part of the first pulse.
> + */

The NEC decoder improvements should be on a separate patch.

>  #define NEC_NBITS		32
> -#define NEC_UNIT		559979 /* ns */
> -#define NEC_HEADER_MARK		(16 * NEC_UNIT)
> -#define NEC_HEADER_SPACE	(8 * NEC_UNIT)
> -#define NEC_REPEAT_SPACE	(4 * NEC_UNIT)
> -#define NEC_MARK		(NEC_UNIT)
> -#define NEC_0_SPACE		(NEC_UNIT)
> -#define NEC_1_SPACE		(3 * NEC_UNIT)
> +#define NEC_UNIT		562	/* us */

Why changing to microseconds? ktime_t also handles time on nanosseconds.

> +#define NEC_HEADER_MARK		6
> +#define NEC_HEADER_SPACE	-8
> +#define NEC_REPEAT_SPACE	-4
> +#define NEC_MARK		1
> +#define NEC_0_SPACE		-1
> +#define NEC_1_SPACE		-3

Those negative values here is really weird... I can't stop thinking on time 
travels, when I see a negative time ;)

Seriously, this change obfuscates the logic, as it is using the mantissa of a number
to indicate a time duration, and the signal to indicate the presence or absence of
a carrier. Encoding two different measures into a number is not a good thing. I used
to do such tricks when programming in Z80 assembly, back on eighties, basically
due to the absolute lack of enough memory on those machines with a few kilobytes of RAM.
After a few weeks, returning back to the same code to fix were really hard. We don't have
such memory constraints anymore. So, let's keep the code as clearer as possible.

-

I've stripped the decoders code, since they're basically implementing the architectural
changes you're proposing Let's first finish the discussions about the changes.

Btw, I noticed that you've added some improvements to the decoders, like the
changes to support RC-5X. The better is to send it as a separate patch, due to a few reasons:

	- RC-5X addition has nothing to do with "Teach drivers/media/IR/ir-raw-event.c
to use durations" (the subject of this RFC patch);

	- Bigger patches have more chances of getting nacked, since they touch on more parts
of the code. So, you'll need to rework more code;

	- The addition of RC-5X shouldn't break RC-5. By having it as a separate patch,
it is easier to test the changes;

	- If later discovered a regression, it would be easier to bisect and see if the
changes were introduced by RC-5X or by the architectural changes, if the changes are broken
into two patches.

> -static unsigned int ir_rc5_remote_gap = 888888;

The idea of static int is that, on saa7134, this value can be adjustable from userspace.
probably, some hardware use a non-standard carrier, so we'll need to export it also
via sysfs, to avoid regressions.

-- 
Cheers,
Mauro

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Mauro Carvalho Chehab]

Hi David,

Em Tue, 6 Apr 2010 12:48:11 +0200
David Härdeman <david@hardeman.nu> escreveu:

> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline; filename=use-pulse-space-timings-in-ir-raw

Thunderbird 2 really don't like this. It considers the entire body as a file, and
refuses to quote it.

> drivers/media/IR/ir-raw-event.c is currently written with the assumption that
> all "raw" hardware will generate events only on state change (i.e. when
> a pulse or space starts).
> 
> However, some hardware (like mceusb, probably the most popular IR receiver
> out there) only generates duration data (and that data is buffered so using
> any kind of timing on the data is futile).

Am I understanding right and this hardware is not capable of indicating if the 
event is a pulse or a space? It seems hard to auto-detect what is pulse or space,
but IMO such code should belong to mceusb driver and not to the decoders.

Based on the code changes you did, I suspect that one of the things the hardware
provides is a "machine reset" state, right? If you just need to add a code to reset
the state machines, this could be done as easily as adding an event at kfifo with
IR_STOP_EVENT. A three line addition at the decoders event handler would be enough
to use it to reset the state machine:

	if (ev->type & IR_STOP_EVENT) {
		data->state = STATE_INACTIVE;
		return;
	}

This event were not added yet, since no hardware currently ported needs it. Eventually,
we may rename it to IR_RESET_STATE, if you think it is clearer.

> This patch (which is not tested since I haven't yet converted a driver for
> any of my hardware to ir-core yet, will do soon) is a RFC on my proposed
> interface change...once I get the green light on the interface change itself
> I'll make sure that the decoders actually work :)

Yes, better to discuss before changing everything ;)

> The rc5 decoder has also gained rc5x support and the use of kfifo's for
> intermediate storage is gone (since there is no need for it).

The RC-5X addition is welcome, but the better is to add it as a separate patch. 

I won't comment every single bits of the change, since we're more interested on the conceptual
aspects.

> -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

Don't remove the raw_event_store. It is needed by the hardware that gets events from
IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their
own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).

For those, we need something like:

int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)

Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

>  
> -int ir_raw_event_handle(struct input_dev *input_dev)
> +/**
> + * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
> + * @input_dev:	the struct input_dev device descriptor
> + * @type:	the type of the event that has occurred
> + *
> + * This routine is used to notify the raw ir decoders on the beginning of an
> + * ir pulse or space (or the start/end of ir reception). This is used by
> + * hardware which does not provide durations directly but only interrupts
> + * (or similar events) on state change.
> + */
> +void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
>  {
> -	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
> -	int				rc;
> -	struct ir_raw_event		ev;
> -	int 				len, i;
> -
> -	/*
> -	 * Store the events into a temporary buffer. This allows calling more than
> -	 * one decoder to deal with the received data
> -	 */
> -	len = kfifo_len(&ir->raw->kfifo) / sizeof(ev);
> -	if (!len)
> -		return 0;

The removal of kfifo is not a good idea. On several drivers, the event is generated during
IRQ time, or on a very expensive polling loop. So, buffering is needed to release the
IRQ as soon as possible and not adding too much processing during polling.

> -
> -	for (i = 0; i < len; i++) {
> -		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
> -		if (rc != sizeof(ev)) {
> -			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
> -				   rc, sizeof(ev));
> -			return -EINVAL;
> -		}
> -		IR_dprintk(2, "event type %d, time before event: %07luus\n",
> -			ev.type, (ev.delta.tv_nsec + 500) / 1000);
> -		rc = RUN_DECODER(decode, input_dev, &ev);
> -	}
> +	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
> +	ktime_t			now;
> +	s64			delta; /* us */
> +
> +	if (!ir->raw)
> +		return;
>  
> -	/*
> -	 * Call all ir decoders. This allows decoding the same event with
> -	 * more than one protocol handler.
> -	 */
> +	now = ktime_get();
> +	delta = ktime_us_delta(now, ir->raw->last_event);


This won't work, in the cases where the hardware is providing its own timings.

>  
> -	return rc;
> +	/* Check for a long duration since last event or if we're
> +	   being called for the first time */
> +	if (delta > USEC_PER_SEC || !ir->raw->last_type)
> +		type |= IR_START_EVENT;

The "long duration" concept would be better implemented at the driver, since it may
vary with the IR carrier and with the protocol details.

> +
> +	if (type & IR_START_EVENT)
> +		ir_raw_event_reset(input_dev);
> +	else if (ir->raw->last_type & IR_SPACE)
> +		ir_raw_event_duration(input_dev, (int)-delta);
> +	else if (ir->raw->last_type & IR_PULSE)
> +		ir_raw_event_duration(input_dev, (int)delta);

Please, don't use a signal to identify between pulse and space. The IR decoding logic
is tricky enough. Just pass the type to the decoder and let it explicitly check if it is
pulse or space.


> Index: ir/drivers/media/IR/ir-nec-decoder.c
> ===================================================================
> --- ir.orig/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:16:27.000000000 +0200
> +++ ir/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:17:08.860846045 +0200
> @@ -14,22 +14,25 @@
>  
>  #include <media/ir-core.h>
>  
> +/*
> + * Regarding NEC_HEADER_MARK: some NEC remotes use 16, some 8,
> + * some receivers are also good at missing part of the first pulse.
> + */

The NEC decoder improvements should be on a separate patch.

>  #define NEC_NBITS		32
> -#define NEC_UNIT		559979 /* ns */
> -#define NEC_HEADER_MARK		(16 * NEC_UNIT)
> -#define NEC_HEADER_SPACE	(8 * NEC_UNIT)
> -#define NEC_REPEAT_SPACE	(4 * NEC_UNIT)
> -#define NEC_MARK		(NEC_UNIT)
> -#define NEC_0_SPACE		(NEC_UNIT)
> -#define NEC_1_SPACE		(3 * NEC_UNIT)
> +#define NEC_UNIT		562	/* us */

Why changing to microseconds? ktime_t also handles time on nanosseconds.

> +#define NEC_HEADER_MARK		6
> +#define NEC_HEADER_SPACE	-8
> +#define NEC_REPEAT_SPACE	-4
> +#define NEC_MARK		1
> +#define NEC_0_SPACE		-1
> +#define NEC_1_SPACE		-3

Those negative values here is really weird... I can't stop thinking on time 
travels, when I see a negative time ;)

Seriously, this change obfuscates the logic, as it is using the mantissa of a number
to indicate a time duration, and the signal to indicate the presence or absence of
a carrier. Encoding two different measures into a number is not a good thing. I used
to do such tricks when programming in Z80 assembly, back on eighties, basically
due to the absolute lack of enough memory on those machines with a few kilobytes of RAM.
After a few weeks, returning back to the same code to fix were really hard. We don't have
such memory constraints anymore. So, let's keep the code as clearer as possible.

-

I've stripped the decoders code, since they're basically implementing the architectural
changes you're proposing Let's first finish the discussions about the changes.

Btw, I noticed that you've added some improvements to the decoders, like the
changes to support RC-5X. The better is to send it as a separate patch, due to a few reasons:

	- RC-5X addition has nothing to do with "Teach drivers/media/IR/ir-raw-event.c
to use durations" (the subject of this RFC patch);

	- Bigger patches have more chances of getting nacked, since they touch on more parts
of the code. So, you'll need to rework more code;

	- The addition of RC-5X shouldn't break RC-5. By having it as a separate patch,
it is easier to test the changes;

	- If later discovered a regression, it would be easier to bisect and see if the
changes were introduced by RC-5X or by the architectural changes, if the changes are broken
into two patches.

> -static unsigned int ir_rc5_remote_gap = 888888;

The idea of static int is that, on saa7134, this value can be adjustable from userspace.
probably, some hardware use a non-standard carrier, so we'll need to export it also
via sysfs, to avoid regressions.

-- 
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Andy Walls]

On Tue, 2010-04-06 at 11:26 -0300, Mauro Carvalho Chehab wrote:
> Hi David,
> 

> I won't comment every single bits of the change, since we're more interested on the conceptual
> aspects.
> 
> > -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
> 
> Don't remove the raw_event_store. It is needed by the hardware that gets events from
> IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their
> own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).
> 
> For those, we need something like:
> 
> int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)
> 
> Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

Just to clarify what Conexant hardware, and my current driver for it, is
capable of:

1. It provides raw pulse (and space) width measurements.

2. Those measurements can have very high resoltuion (~37 ns IIRC) or
very low resolution (usec or msec IIRC) depending on how the hardware
clock dividers are set up.

3. The hardware provides a timeout when the measurment timer overflows,
meaning that no edge happened for a very long time.  This generates a
special "overflow" measurment value and a receiver timeout interrupt.

4. The hardware has an 8 measurement deep FIFO, which the driver starts
to drain whenever it is half full (i.e. pulse measurement data is
delayed).  This happens in response to a hardware FIFO service request
interrupt.

5. The hardware FIFO is drained by the driver whenever an interrupt is
received and the available measruement data is placed into a kfifo().
This will then signal a work handler that it has work to do.

6. Measurements are scaled to standard time units (i.e. ns) by the
driver when they are read out of the kfifo() by a work handler.  (No
sense in doing time conversions in an interrupt handler).

7. The work handler then begins passing whatever measurements it has,
one at a time, over to a pulse stream decoder.

8. If the pulse stream decoder actually decodes something, it is passed
over to the input subsystem.

I suspect this device's behavior is much closer to what the MCE-USB
device does, than the raw GPIO handlers, but I don't really know the
MCE-USB.

Regards,
Andy

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[David Härdeman]

On Wed, Apr 07, 2010 at 06:20:07AM -0400, Andy Walls wrote:
> On Tue, 2010-04-06 at 11:26 -0300, Mauro Carvalho Chehab wrote:
> > I won't comment every single bits of the change, since we're more 
> > interested on the conceptual
> > aspects.
> > 
> > > -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
> > 
> > Don't remove the raw_event_store. It is needed by the hardware that gets events from
> > IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their
> > own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).
> > 
> > For those, we need something like:
> > 
> > int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)
> > 
> > Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.
> 
> Just to clarify what Conexant hardware, and my current driver for it, is
> capable of:
> 
> 1. It provides raw pulse (and space) width measurements.
> 
> 2. Those measurements can have very high resoltuion (~37 ns IIRC) or
> very low resolution (usec or msec IIRC) depending on how the hardware
> clock dividers are set up.
> 
> 3. The hardware provides a timeout when the measurment timer overflows,
> meaning that no edge happened for a very long time.  This generates a
> special "overflow" measurment value and a receiver timeout interrupt.
> 
> 4. The hardware has an 8 measurement deep FIFO, which the driver starts
> to drain whenever it is half full (i.e. pulse measurement data is
> delayed).  This happens in response to a hardware FIFO service request
> interrupt.
> 
> 5. The hardware FIFO is drained by the driver whenever an interrupt is
> received and the available measruement data is placed into a kfifo().
> This will then signal a work handler that it has work to do.
> 
> 6. Measurements are scaled to standard time units (i.e. ns) by the
> driver when they are read out of the kfifo() by a work handler.  (No
> sense in doing time conversions in an interrupt handler).
> 
> 7. The work handler then begins passing whatever measurements it has,
> one at a time, over to a pulse stream decoder.
> 
> 8. If the pulse stream decoder actually decodes something, it is passed
> over to the input subsystem.
> 
> I suspect this device's behavior is much closer to what the MCE-USB
> device does, than the raw GPIO handlers, but I don't really know the
> MCE-USB.

This sounds very similar to winbond-cir (the hardware parts that is, 
basically until and including item 5, line 1). The mceusb HW does 
something similar...it sends usb packets with a couple of pulse/space 
duration measurements (of 50us resolution IIRC)...and it automatically 
enters an inactive state after 10000 us of silence.

The ir_raw_event_duration() function of my patch is intended for exactly 
this kind of hardware (which I mentioned in my reply to Mauro which I 
just sent out).

The question is though, is the kfifo and work handler really 
necessary?

-- 
David Härdeman

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[David Härdeman]

On Wed, Apr 07, 2010 at 06:20:07AM -0400, Andy Walls wrote:
> On Tue, 2010-04-06 at 11:26 -0300, Mauro Carvalho Chehab wrote:
> > I won't comment every single bits of the change, since we're more 
> > interested on the conceptual
> > aspects.
> > 
> > > -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
> > 
> > Don't remove the raw_event_store. It is needed by the hardware that gets events from
> > IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their
> > own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).
> > 
> > For those, we need something like:
> > 
> > int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)
> > 
> > Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.
> 
> Just to clarify what Conexant hardware, and my current driver for it, is
> capable of:
> 
> 1. It provides raw pulse (and space) width measurements.
> 
> 2. Those measurements can have very high resoltuion (~37 ns IIRC) or
> very low resolution (usec or msec IIRC) depending on how the hardware
> clock dividers are set up.
> 
> 3. The hardware provides a timeout when the measurment timer overflows,
> meaning that no edge happened for a very long time.  This generates a
> special "overflow" measurment value and a receiver timeout interrupt.
> 
> 4. The hardware has an 8 measurement deep FIFO, which the driver starts
> to drain whenever it is half full (i.e. pulse measurement data is
> delayed).  This happens in response to a hardware FIFO service request
> interrupt.
> 
> 5. The hardware FIFO is drained by the driver whenever an interrupt is
> received and the available measruement data is placed into a kfifo().
> This will then signal a work handler that it has work to do.
> 
> 6. Measurements are scaled to standard time units (i.e. ns) by the
> driver when they are read out of the kfifo() by a work handler.  (No
> sense in doing time conversions in an interrupt handler).
> 
> 7. The work handler then begins passing whatever measurements it has,
> one at a time, over to a pulse stream decoder.
> 
> 8. If the pulse stream decoder actually decodes something, it is passed
> over to the input subsystem.
> 
> I suspect this device's behavior is much closer to what the MCE-USB
> device does, than the raw GPIO handlers, but I don't really know the
> MCE-USB.

This sounds very similar to winbond-cir (the hardware parts that is, 
basically until and including item 5, line 1). The mceusb HW does 
something similar...it sends usb packets with a couple of pulse/space 
duration measurements (of 50us resolution IIRC)...and it automatically 
enters an inactive state after 10000 us of silence.

The ir_raw_event_duration() function of my patch is intended for exactly 
this kind of hardware (which I mentioned in my reply to Mauro which I 
just sent out).

The question is though, is the kfifo and work handler really 
necessary?

-- 
David Härdeman
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Jon Smirl]

I had to rework this portion of code several times in the IR code I posted.

I had the core provide input_ir_queue() which was legal to call from
interrupt context. Calling from interrupt context was an important
aspect I missed in the first versions. I made this a common routine so
that the code didn't get copied into all of the drivers. This code
should have used kfifo but I didn't know about kfifo.

>>The question is though, is the kfifo and work handler really
necessary?

Yes, otherwise it will get duplicated into all of the drivers that run
in interrupt context like this GPIO one. Put this common code into the
core so that the individual drivers writers don't mess it up.

void input_ir_queue(struct input_dev *dev, int sample)
{
	unsigned int next;

	spin_lock(dev->ir->queue.lock);
	dev->ir->queue.samples[dev->ir->queue.head] = sample;
	next = dev->ir->queue.head + 1;
	dev->ir->queue.head = (next >= MAX_SAMPLES ? 0 : next);
	spin_unlock(dev->ir->queue.lock);

	schedule_work(&dev->ir->work);
}

My GPIO implementation simply call input_it_queue() with the timing
data. I collapsed multiple long space interrupts into one very long
space. If you are using protocol engines, there is no need to detect
the long trailing space. The protocol engine will trigger on the last
pulse of the signal.

On the other hand, LIRC in user space needs the last long space to
know when to flush the buffer from kernel space into user space. The
timeout for this flush should be implemented in the LIRC compatibility
driver, not ir-core. In this case my GPIO driver doesn't ever generate
an event for the long space at the end of the message (because it
doesn't end). Instead the LIRC compatibility layer should start a
timer and flush when no data has been received for 200ms or whatever.

static irqreturn_t dpeak_ir_irq(int irq, void *_ir)
{
	struct ir_gpt *ir_gpt = _ir;
	int sample, count, delta, bit, wrap;

	sample = in_be32(&ir_gpt->regs->status);
	out_be32(&ir_gpt->regs->status, 0xF);

	count = sample >> 16;
	wrap = (sample >> 12) & 7;
	bit = (sample >> 8) & 1;

	delta = count - ir_gpt->previous;
	delta += wrap * 0x10000;

	ir_gpt->previous = count;

	if (bit)
		delta = -delta;

	input_ir_queue(ir_gpt->input, delta);

	return IRQ_HANDLED;
}

For MSMCE I converted their format back into simple delays and fed it
into input_ir_queue(). This was not done in interrupt context because
of the way USB works. input_ir_queue() doesn't care - it works
correctly when called from either context.

				if (ir->last.command == 0x80) {
					bit = ((ir->buf_in[i] & MCE_PULSE_BIT) != 0);
					delta = (ir->buf_in[i] & MCE_PULSE_MASK) * MCE_TIME_BASE;

					if ((ir->buf_in[i] & MCE_PULSE_MASK) == 0x7f) {
						if (ir->last.bit == bit)
							ir->last.delta += delta;
						else {
							ir->last.delta = delta;
							ir->last.bit = bit;
						}
						continue;
					}
					delta += ir->last.delta;
					ir->last.delta = 0;
					ir->last.bit = bit;

					dev_dbg(&ir->usbdev->dev, "bit %d delta %d\n", bit, delta);
					if (bit)
						delta = -delta;

					input_ir_queue(ir->input, delta);
				}

These delay messages are then fed into the protocol engines which
process the pulses in parallel. Processing in parallel works, because
that's how IR receivers work. When you shine a remote on an equipment
rack, all of the equipment sees the command in parallel. The protocols
are designed so that parallel decode works properly.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Mauro Carvalho Chehab]

Jon Smirl wrote:
> I had to rework this portion of code several times in the IR code I posted.
> 
> I had the core provide input_ir_queue() which was legal to call from
> interrupt context. Calling from interrupt context was an important
> aspect I missed in the first versions. I made this a common routine so
> that the code didn't get copied into all of the drivers. This code
> should have used kfifo but I didn't know about kfifo.
> 
>>> The question is though, is the kfifo and work handler really
> necessary?
> 
> Yes, otherwise it will get duplicated into all of the drivers that run
> in interrupt context like this GPIO one. Put this common code into the
> core so that the individual drivers writers don't mess it up.
> 
> void input_ir_queue(struct input_dev *dev, int sample)
> {
> 	unsigned int next;
> 
> 	spin_lock(dev->ir->queue.lock);
> 	dev->ir->queue.samples[dev->ir->queue.head] = sample;
> 	next = dev->ir->queue.head + 1;
> 	dev->ir->queue.head = (next >= MAX_SAMPLES ? 0 : next);
> 	spin_unlock(dev->ir->queue.lock);
> 
> 	schedule_work(&dev->ir->work);
> }

The big advantage of using kfifo is that you don't need to use a spinlock, if
there's just one consumer of the event. On the implementation I did, just
one code writes to the kfifo (the driver) and just one code reads from the 
kfifo, and multiplexing the data to the several decoders (and lirc_dev). 
So, no locks.

> 
> My GPIO implementation simply call input_it_queue() with the timing
> data. I collapsed multiple long space interrupts into one very long
> space. If you are using protocol engines, there is no need to detect
> the long trailing space. The protocol engine will trigger on the last
> pulse of the signal.
> 
> On the other hand, LIRC in user space needs the last long space to
> know when to flush the buffer from kernel space into user space. The
> timeout for this flush should be implemented in the LIRC compatibility
> driver, not ir-core. In this case my GPIO driver doesn't ever generate
> an event for the long space at the end of the message (because it
> doesn't end). Instead the LIRC compatibility layer should start a
> timer and flush when no data has been received for 200ms or whatever.

Agreed.

> static irqreturn_t dpeak_ir_irq(int irq, void *_ir)
> {
> 	struct ir_gpt *ir_gpt = _ir;
> 	int sample, count, delta, bit, wrap;
> 
> 	sample = in_be32(&ir_gpt->regs->status);
> 	out_be32(&ir_gpt->regs->status, 0xF);
> 
> 	count = sample >> 16;
> 	wrap = (sample >> 12) & 7;
> 	bit = (sample >> 8) & 1;
> 
> 	delta = count - ir_gpt->previous;
> 	delta += wrap * 0x10000;
> 
> 	ir_gpt->previous = count;
> 
> 	if (bit)
> 		delta = -delta;
> 
> 	input_ir_queue(ir_gpt->input, delta);
> 
> 	return IRQ_HANDLED;
> }
> 
> For MSMCE I converted their format back into simple delays and fed it
> into input_ir_queue(). This was not done in interrupt context because
> of the way USB works. input_ir_queue() doesn't care - it works
> correctly when called from either context.
> 
> 				if (ir->last.command == 0x80) {
> 					bit = ((ir->buf_in[i] & MCE_PULSE_BIT) != 0);
> 					delta = (ir->buf_in[i] & MCE_PULSE_MASK) * MCE_TIME_BASE;
> 
> 					if ((ir->buf_in[i] & MCE_PULSE_MASK) == 0x7f) {
> 						if (ir->last.bit == bit)
> 							ir->last.delta += delta;
> 						else {
> 							ir->last.delta = delta;
> 							ir->last.bit = bit;
> 						}
> 						continue;
> 					}
> 					delta += ir->last.delta;
> 					ir->last.delta = 0;
> 					ir->last.bit = bit;
> 
> 					dev_dbg(&ir->usbdev->dev, "bit %d delta %d\n", bit, delta);
> 					if (bit)
> 						delta = -delta;
> 
> 					input_ir_queue(ir->input, delta);
> 				}
> 
> These delay messages are then fed into the protocol engines which
> process the pulses in parallel. Processing in parallel works, because
> that's how IR receivers work. When you shine a remote on an equipment
> rack, all of the equipment sees the command in parallel. The protocols
> are designed so that parallel decode works properly.

On the implementation I did, each event is passed to each decoder serialized (yet, as one keystroke
is a series of events, it behaves as if they are processed in parallel). We might create separate
kthreads for each decoder, and use a spinlock at kfifo, but I suspect that the end result will be
very close and we'll have more threads interfering at the samples collect, especially on those
(broken) hardware that don't have IRQ's to indicate a state transition, so the driver needs
to poll the samples.

-- 

Cheers,
Mauro

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Jon Smirl]

On Wed, Apr 7, 2010 at 9:29 AM, Mauro Carvalho Chehab
<mchehab@infradead.org> wrote:
> On the implementation I did, each event is passed to each decoder serialized (yet, as one keystroke
> is a series of events, it behaves as if they are processed in parallel). We might create separate
> kthreads for each decoder, and use a spinlock at kfifo, but I suspect that the end result will be
> very close and we'll have more threads interfering at the samples collect, especially on those
> (broken) hardware that don't have IRQ's to indicate a state transition, so the driver needs
> to poll the samples.

Polling should be the driver's problem. They can set up a timer
interrupt and do it that way. Do all of the protocols have a long
enough lead one for a timer tick to catch them? If so, look for it in
the timer event, then go into a polling loop. You'd be way better off
buying new hardware since your video is going to stop while this
pooling loop runs. Do modern serial ports interrupt on DTR or whatever
those Iguana devices use? What is an example of a polled input device?
I can't think of one, even IR diode on mic input is interrupt driven
(that require a special ALSA driver to pass the data into RC core).

No need to use different kthreads for each protocol decoder, but don't
lock up the default kernel thread waiting for a user space response.
What I meant by parallel was that pulses are fed one at a time into
each of the decoders, don't wait for a long space and then feed the
entire message into the decoders.

>
> --
>
> Cheers,
> Mauro
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Andy Walls]

On Wed, 2010-04-07 at 13:42 +0200, David Härdeman wrote:
> On Wed, Apr 07, 2010 at 06:20:07AM -0400, Andy Walls wrote:
> > On Tue, 2010-04-06 at 11:26 -0300, Mauro Carvalho Chehab wrote:
> > > I won't comment every single bits of the change, since we're more 
> > > interested on the conceptual
> > > aspects.
> > > 
> > > > -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
> > > 
> > > Don't remove the raw_event_store. It is needed by the hardware that gets events from
> > > IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their
> > > own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).
> > > 
> > > For those, we need something like:
> > > 
> > > int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)
> > > 
> > > Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.
> > 
> > Just to clarify what Conexant hardware, and my current driver for it, is
> > capable of:
> > 
> > 1. It provides raw pulse (and space) width measurements.
> > 
> > 2. Those measurements can have very high resoltuion (~37 ns IIRC) or
> > very low resolution (usec or msec IIRC) depending on how the hardware
> > clock dividers are set up.
> > 
> > 3. The hardware provides a timeout when the measurment timer overflows,
> > meaning that no edge happened for a very long time.  This generates a
> > special "overflow" measurment value and a receiver timeout interrupt.
> > 
> > 4. The hardware has an 8 measurement deep FIFO, which the driver starts
> > to drain whenever it is half full (i.e. pulse measurement data is
> > delayed).  This happens in response to a hardware FIFO service request
> > interrupt.
> > 
> > 5. The hardware FIFO is drained by the driver whenever an interrupt is
> > received and the available measruement data is placed into a kfifo().
> > This will then signal a work handler that it has work to do.
> > 
> > 6. Measurements are scaled to standard time units (i.e. ns) by the
> > driver when they are read out of the kfifo() by a work handler.  (No
> > sense in doing time conversions in an interrupt handler).
> > 
> > 7. The work handler then begins passing whatever measurements it has,
> > one at a time, over to a pulse stream decoder.
> > 
> > 8. If the pulse stream decoder actually decodes something, it is passed
> > over to the input subsystem.
> > 
> > I suspect this device's behavior is much closer to what the MCE-USB
> > device does, than the raw GPIO handlers, but I don't really know the
> > MCE-USB.
> 
> This sounds very similar to winbond-cir (the hardware parts that is, 
> basically until and including item 5, line 1). The mceusb HW does 
> something similar...it sends usb packets with a couple of pulse/space 
> duration measurements (of 50us resolution IIRC)...and it automatically 
> enters an inactive state after 10000 us of silence.
> 
> The ir_raw_event_duration() function of my patch is intended for exactly 
> this kind of hardware (which I mentioned in my reply to Mauro which I 
> just sent out).
> 
> The question is though, is the kfifo and work handler really 
> necessary?

Yes.  I like to keep hard IRQ handler durations as short as possible. 

This CX2388[58] PCIe IRQ is raised not only for IR, but for the rest of
the bridge chip functions too.  The IR irq handling is just one of the
functions that may need service and video related functions are really
more important.

This Conexant IR hardware is used on the CX2388[58], CX23418,
CX2584[0123], and CX2310[123]. Right now I only have the driver
implemented for the CX23888 and a test implementation for the CX23885.
But I plan to have a somewhat common code base working for all the chips
eventually.  I think it is better to go with a slightly more complicated
handoff for reading IR pulses with a deferred work handler, than to
degrade the hard IRQ handler peformance for all of these drivers.  The
CX23418 is really a pain about it's IRQ handler timeline.


BTW, the ns unit of resolution we have in the kernel so far is likely
due to me.  I have hardware that can report measurements with a
resolution of (2 * 4)/108MHz = 74 ns, so I just settled on ns as the
basic time unit.  I think it's overkill too for existing protocols.  It
is however nice for measuring and characterizing noise in the
environment (something that an enduser will never need) or investigating
an unknown protocol.

Regards,
Andy

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[David Härdeman]

On Tue, Apr 06, 2010 at 11:26:35AM -0300, Mauro Carvalho Chehab wrote:
> Hi David,
> 
> Em Tue, 6 Apr 2010 12:48:11 +0200
> David Härdeman <david@hardeman.nu> escreveu:
> 
> > Content-Type: text/plain; charset=us-ascii
> > Content-Disposition: inline; filename=use-pulse-space-timings-in-ir-raw
> 
> Thunderbird 2 really don't like this. It considers the entire body as a file, and
> refuses to quote it.

Never had people complain when I use quilt before but I'll see what I 
can do.

> > drivers/media/IR/ir-raw-event.c is currently written with the assumption that
> > all "raw" hardware will generate events only on state change (i.e. when
> > a pulse or space starts).
> > 
> > However, some hardware (like mceusb, probably the most popular IR receiver
> > out there) only generates duration data (and that data is buffered so using
> > any kind of timing on the data is futile).
> 
> Am I understanding right and this hardware is not capable of indicating if the 
> event is a pulse or a space? It seems hard to auto-detect what is pulse or space,
> but IMO such code should belong to mceusb driver and not to the decoders.

No, the driver for mceusb sends a usb packet which contains a couple of 
pulse/space durations in the form of signed integers representing pulse 
(positive) and space (negative) durations in microseconds. It's a pretty 
common arrangement. winbond-cir also has a mode (which is the one I'm 
planning on using in the future) where pulse/space durations are 
accumulated in the UART buffer and an IRQ is generated once the buffer 
level reaches a threshold.

> Based on the code changes you did, I suspect that one of the things the hardware
> provides is a "machine reset" state, right? If you just need to add a code to reset
> the state machines, this could be done as easily as adding an event at kfifo with
> IR_STOP_EVENT. A three line addition at the decoders event handler would be enough
> to use it to reset the state machine:
> 
> 	if (ev->type & IR_STOP_EVENT) {
> 		data->state = STATE_INACTIVE;
> 		return;
> 	}
> 
> This event were not added yet, since no hardware currently ported needs it. Eventually,
> we may rename it to IR_RESET_STATE, if you think it is clearer.

Not a particular state per se, I just added a function which the 
hardware can use to reset the state machines when necessary (think 
hardware reset, suspend/resume, switching from RX to TX and back again, 
etc).

I think this:

	/* Hardware has been reset, notify ir-core */
	ir_raw_event_reset(input_dev);

is a hell lot clearer than this (your current code):

	/* Hardware has been reset, notify ir-core */
	rc = ir_raw_event_store(input_dev, IR_STOP_EVENT);
	if (rc) {
		/* Uh oh, what do we do now? */
		...
	}
	rc = ir_raw_event_handle(input_dev);
	if (rc) {
		/* Not again... */
		...
	}

> > This patch (which is not tested since I haven't yet converted a 
> > driver for
> > any of my hardware to ir-core yet, will do soon) is a RFC on my proposed
> > interface change...once I get the green light on the interface change itself
> > I'll make sure that the decoders actually work :)
> 
> Yes, better to discuss before changing everything ;)
>
> > The rc5 decoder has also gained rc5x support and the use of kfifo's 
> > for
> > intermediate storage is gone (since there is no need for it).
> 
> The RC-5X addition is welcome, but the better is to add it as a separate patch. 

Using durations (instead of a combination of struct timespec and enum 
raw_event_type) as the argument to the decoder necessitates rewriting 
most of the decoders, so it seemed like a good time to add it. RC5X or 
not will anyway only mean a couple of lines of difference but I can send 
it as a separate patch if that helps you.
 
> I won't comment every single bits of the change, since we're more interested on the conceptual
> aspects.
> 
> > -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
> 
> Don't remove the raw_event_store. It is needed by the hardware that gets events from
> IRQ/polling.

See the comments for kfifo below.

> For sure another interface is needed, for the cases where the hardware 
> pass their
> own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).
> 
> For those, we need something like:
> 
> int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)
> 
> Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

Um, this sounds exactly like ir_raw_event_duration() which was the main 
point of my patch.
 
> >  
> > -int ir_raw_event_handle(struct input_dev *input_dev)
> > +/**
> > + * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
> > + * @input_dev:	the struct input_dev device descriptor
> > + * @type:	the type of the event that has occurred
> > + *
> > + * This routine is used to notify the raw ir decoders on the beginning of an
> > + * ir pulse or space (or the start/end of ir reception). This is used by
> > + * hardware which does not provide durations directly but only interrupts
> > + * (or similar events) on state change.
> > + */
> > +void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
> >  {
> > -	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
> > -	int				rc;
> > -	struct ir_raw_event		ev;
> > -	int 				len, i;
> > -
> > -	/*
> > -	 * Store the events into a temporary buffer. This allows calling more than
> > -	 * one decoder to deal with the received data
> > -	 */
> > -	len = kfifo_len(&ir->raw->kfifo) / sizeof(ev);
> > -	if (!len)
> > -		return 0;
> 
> The removal of kfifo is not a good idea. On several drivers, the event is generated during
> IRQ time, or on a very expensive polling loop. So, buffering is needed to release the
> IRQ as soon as possible and not adding too much processing during polling.

Have you seen any real case where this is a problem or is this just 
conjecture on your behalf? I've written ir decoders for embedded 
hardware which pass the "event" (duration) through the state machines 
directly and it works great on hardware with a fraction of the computing 
power compared to the machines you're using.

The state machines shouldn't have to do much more than rounding of the 
duration followed by a couple of integer comparisons (and possibly some 
bitops). I fail to see how using a kfifo would provide any real 
improvement.
 
> > -
> > -	for (i = 0; i < len; i++) {
> > -		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
> > -		if (rc != sizeof(ev)) {
> > -			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
> > -				   rc, sizeof(ev));
> > -			return -EINVAL;
> > -		}
> > -		IR_dprintk(2, "event type %d, time before event: %07luus\n",
> > -			ev.type, (ev.delta.tv_nsec + 500) / 1000);
> > -		rc = RUN_DECODER(decode, input_dev, &ev);
> > -	}
> > +	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
> > +	ktime_t			now;
> > +	s64			delta; /* us */
> > +
> > +	if (!ir->raw)
> > +		return;
> >  
> > -	/*
> > -	 * Call all ir decoders. This allows decoding the same event with
> > -	 * more than one protocol handler.
> > -	 */
> > +	now = ktime_get();
> > +	delta = ktime_us_delta(now, ir->raw->last_event);
> 
> 
> This won't work, in the cases where the hardware is providing its own 
> timings.

Again, see ir_raw_event_duration()
 
> >  
> > -	return rc;
> > +	/* Check for a long duration since last event or if we're
> > +	   being called for the first time */
> > +	if (delta > USEC_PER_SEC || !ir->raw->last_type)
> > +		type |= IR_START_EVENT;
> 
> The "long duration" concept would be better implemented at the driver, since it may
> vary with the IR carrier and with the protocol details.

Now you're criticizing your own code...this was one of the few concepts 
I carried over from your original code. Specifically, ir-raw-event.c, 
lines 116 - 129 from your current tree:

	ktime_get_ts(&ts);

	if (timespec_equal(&ir->raw->last_event, &event.delta))
		event.type |= IR_START_EVENT;
	else
		event.delta = timespec_sub(ts, ir->raw->last_event);

	memcpy(&ir->raw->last_event, &ts, sizeof(ts));

	if (event.delta.tv_sec) {
		event.type |= IR_START_EVENT;
		event.delta.tv_sec = 0;
		event.delta.tv_nsec = 0;
	}

(Note the check on event.delta.tv_sec)

> > +
> > +	if (type & IR_START_EVENT)
> > +		ir_raw_event_reset(input_dev);
> > +	else if (ir->raw->last_type & IR_SPACE)
> > +		ir_raw_event_duration(input_dev, (int)-delta);
> > +	else if (ir->raw->last_type & IR_PULSE)
> > +		ir_raw_event_duration(input_dev, (int)delta);
> 
> Please, don't use a signal to identify between pulse and space. The IR decoding logic
> is tricky enough. Just pass the type to the decoder and let it explicitly check if it is
> pulse or space.

No idea what you mean here. Why would it be clearer to put the 
equivalent code in every single decoder instead of adding it once to the 
place which calls the decoders?
 
> > Index: ir/drivers/media/IR/ir-nec-decoder.c
> > ===================================================================
> > --- ir.orig/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:16:27.000000000 +0200
> > +++ ir/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:17:08.860846045 +0200
> > @@ -14,22 +14,25 @@
> >  
> >  #include <media/ir-core.h>
> >  
> > +/*
> > + * Regarding NEC_HEADER_MARK: some NEC remotes use 16, some 8,
> > + * some receivers are also good at missing part of the first pulse.
> > + */
> 
> The NEC decoder improvements should be on a separate patch.

They can't since the entire patch is about changing the core raw logic 
over to primarily use durations, which means the existing nec and rc5 
decoders need to change in the same patch or bisectability will be 
broken.
 
> >  #define NEC_NBITS		32
> > -#define NEC_UNIT		559979 /* ns */
> > -#define NEC_HEADER_MARK		(16 * NEC_UNIT)
> > -#define NEC_HEADER_SPACE	(8 * NEC_UNIT)
> > -#define NEC_REPEAT_SPACE	(4 * NEC_UNIT)
> > -#define NEC_MARK		(NEC_UNIT)
> > -#define NEC_0_SPACE		(NEC_UNIT)
> > -#define NEC_1_SPACE		(3 * NEC_UNIT)
> > +#define NEC_UNIT		562	/* us */
> 
> Why changing to microseconds? ktime_t also handles time on nanosseconds.

Interesting question but you managed to turn it on its head. Why not use 
femtoseconds? Because it's complete and utter overkill. Not a single IR 
protocol needs that kind of precision. Both LIRC and the Microsoft IR 
API's are based on microseconds. Programmable IR hardware (like the 
Philips Pronto) is based on microseconds. The proper question to ask is 
why you'd use nanoseconds...

> > +#define NEC_HEADER_MARK		6
> > +#define NEC_HEADER_SPACE	-8
> > +#define NEC_REPEAT_SPACE	-4
> > +#define NEC_MARK		1
> > +#define NEC_0_SPACE		-1
> > +#define NEC_1_SPACE		-3
> 
> Those negative values here is really weird... I can't stop thinking on time 
> travels, when I see a negative time ;)

Better get used to it, if you're going to maintain ir-core you're going 
to see it in mailing list discussions once ir-core gains a userbase.

> Seriously, this change obfuscates the logic, as it is using the mantissa of a number
> to indicate a time duration, and the signal to indicate the presence or absence of
> a carrier. Encoding two different measures into a number is not a good thing. I used
> to do such tricks when programming in Z80 assembly, back on eighties, basically
> due to the absolute lack of enough memory on those machines with a few kilobytes of RAM.
> After a few weeks, returning back to the same code to fix were really hard. We don't have
> such memory constraints anymore. So, let's keep the code as clearer as possible.

Describing a received ir signal as a number of signed integers 
describing the duration of pulses (positive) and spaces (negative) in 
microseconds is pretty much standard (to the extent that any standard 
exists) in any discussion of IR protocols.

The decoders Jon Smirl implemented used signed integers, the decoders 
I've implemented use signed integers. Microsoft's IR API uses signed 
integers. LIRC uses signed integers (kinda, 23 bits of microsecond 
duration, one bit for pulse or space - as far as I can remember from 
LIRC_MODE_MODE2). Other projects also use it (e.g. the developers of 
decodeir.dll which is probably one of the most used IR API's on the MS 
platform outside of Microsoft's own API).

Instead you want to go with a model where you pass in total three 
arguments to the decoders (struct timespec with nsec and sec + enum 
type). I do not understand how you would consider this clearer or 
better.

> I've stripped the decoders code, since they're basically implementing 
> the architectural
> changes you're proposing Let's first finish the discussions about the changes.
> 
> Btw, I noticed that you've added some improvements to the decoders, like the
> changes to support RC-5X. The better is to send it as a separate patch, due to a few reasons:
> 
> 	- RC-5X addition has nothing to do with "Teach drivers/media/IR/ir-raw-event.c
> to use durations" (the subject of this RFC patch);
> 
> 	- Bigger patches have more chances of getting nacked, since they touch on more parts
> of the code. So, you'll need to rework more code;
> 
> 	- The addition of RC-5X shouldn't break RC-5. By having it as a separate patch,
> it is easier to test the changes;
> 
> 	- If later discovered a regression, it would be easier to bisect and see if the
> changes were introduced by RC-5X or by the architectural changes, if the changes are broken
> into two patches.
> 

I've already addressed RC5X above.

> > -static unsigned int ir_rc5_remote_gap = 888888;
> 
> The idea of static int is that, on saa7134, this value can be adjustable from userspace.
> probably, some hardware use a non-standard carrier, so we'll need to export it also
> via sysfs, to avoid regressions.

The decoders in my patch have a +/-50% tolerance for pulse/space unit 
durations, if it turns out to be insufficient, then it's time to look at 
solutions, doing it now is premature.

-- 
David Härdeman

Re: [RFC] Teach drivers/media/IR/ir-raw-event.c to use durations

[Mauro Carvalho Chehab]

David Härdeman wrote:
> On Tue, Apr 06, 2010 at 11:26:35AM -0300, Mauro Carvalho Chehab wrote:
>> Hi David,
>>
>> Em Tue, 6 Apr 2010 12:48:11 +0200
>> David Härdeman <david@hardeman.nu> escreveu:
>>
>>> Content-Type: text/plain; charset=us-ascii
>>> Content-Disposition: inline; filename=use-pulse-space-timings-in-ir-raw
>> Thunderbird 2 really don't like this. It considers the entire body as a file, and
>> refuses to quote it.
> 
> Never had people complain when I use quilt before but I'll see what I 
> can do.

Thanks. I'll also see if I can get some extension to thunderbird to fix it, or consider
moving (again) to another emailer.

>>> drivers/media/IR/ir-raw-event.c is currently written with the assumption that
>>> all "raw" hardware will generate events only on state change (i.e. when
>>> a pulse or space starts).
>>>
>>> However, some hardware (like mceusb, probably the most popular IR receiver
>>> out there) only generates duration data (and that data is buffered so using
>>> any kind of timing on the data is futile).
>> Am I understanding right and this hardware is not capable of indicating if the 
>> event is a pulse or a space? It seems hard to auto-detect what is pulse or space,
>> but IMO such code should belong to mceusb driver and not to the decoders.
> 
> No, the driver for mceusb sends a usb packet which contains a couple of 
> pulse/space durations in the form of signed integers representing pulse 
> (positive) and space (negative) durations in microseconds. It's a pretty 
> common arrangement. winbond-cir also has a mode (which is the one I'm 
> planning on using in the future) where pulse/space durations are 
> accumulated in the UART buffer and an IRQ is generated once the buffer 
> level reaches a threshold.

Ok.

>> Based on the code changes you did, I suspect that one of the things the hardware
>> provides is a "machine reset" state, right? If you just need to add a code to reset
>> the state machines, this could be done as easily as adding an event at kfifo with
>> IR_STOP_EVENT. A three line addition at the decoders event handler would be enough
>> to use it to reset the state machine:
>>
>> 	if (ev->type & IR_STOP_EVENT) {
>> 		data->state = STATE_INACTIVE;
>> 		return;
>> 	}
>>
>> This event were not added yet, since no hardware currently ported needs it. Eventually,
>> we may rename it to IR_RESET_STATE, if you think it is clearer.
> 
> Not a particular state per se, I just added a function which the 
> hardware can use to reset the state machines when necessary (think 
> hardware reset, suspend/resume, switching from RX to TX and back again, 
> etc).
> 
> I think this:
> 
> 	/* Hardware has been reset, notify ir-core */
> 	ir_raw_event_reset(input_dev);
> 
> is a hell lot clearer than this (your current code):
> 
> 	/* Hardware has been reset, notify ir-core */
> 	rc = ir_raw_event_store(input_dev, IR_STOP_EVENT);
> 	if (rc) {
> 		/* Uh oh, what do we do now? */
> 		...

Ok. What about:

#define ir_raw_event_reset(input_dev) ir_raw_event_store(input_dev, IR_STOP_EVENT)

This will save some code and avoid one more symbol to be exported.

> 	}
> 	rc = ir_raw_event_handle(input_dev);
> 	if (rc) {
> 		/* Not again... */
> 		...
> 	}
> 
>>> This patch (which is not tested since I haven't yet converted a 
>>> driver for
>>> any of my hardware to ir-core yet, will do soon) is a RFC on my proposed
>>> interface change...once I get the green light on the interface change itself
>>> I'll make sure that the decoders actually work :)
>> Yes, better to discuss before changing everything ;)
>>
>>> The rc5 decoder has also gained rc5x support and the use of kfifo's 
>>> for
>>> intermediate storage is gone (since there is no need for it).
>> The RC-5X addition is welcome, but the better is to add it as a separate patch. 
> 
> Using durations (instead of a combination of struct timespec and enum 
> raw_event_type) as the argument to the decoder necessitates rewriting 
> most of the decoders, so it seemed like a good time to add it. RC5X or 
> not will anyway only mean a couple of lines of difference but I can send 
> it as a separate patch if that helps you.

Ok, thanks.

>> I won't comment every single bits of the change, since we're more interested on the conceptual
>> aspects.
>>
>>> -int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
>> Don't remove the raw_event_store. It is needed by the hardware that gets events from
>> IRQ/polling.
> 
> See the comments for kfifo below.
> 
>> For sure another interface is needed, for the cases where the hardware 
>> pass their
>> own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).
>>
>> For those, we need something like:
>>
>> int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)
>>
>> Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.
> 
> Um, this sounds exactly like ir_raw_event_duration() which was the main 
> point of my patch.

The better is to change rename the original function, passing the timestamp 
as the original argument, and then add a function or a macro that adds the
get time logic, but see bellow.

>>> -int ir_raw_event_handle(struct input_dev *input_dev)
>>> +/**
>>> + * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
>>> + * @input_dev:	the struct input_dev device descriptor
>>> + * @type:	the type of the event that has occurred
>>> + *
>>> + * This routine is used to notify the raw ir decoders on the beginning of an
>>> + * ir pulse or space (or the start/end of ir reception). This is used by
>>> + * hardware which does not provide durations directly but only interrupts
>>> + * (or similar events) on state change.
>>> + */
>>> +void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
>>>  {
>>> -	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
>>> -	int				rc;
>>> -	struct ir_raw_event		ev;
>>> -	int 				len, i;
>>> -
>>> -	/*
>>> -	 * Store the events into a temporary buffer. This allows calling more than
>>> -	 * one decoder to deal with the received data
>>> -	 */
>>> -	len = kfifo_len(&ir->raw->kfifo) / sizeof(ev);
>>> -	if (!len)
>>> -		return 0;
>> The removal of kfifo is not a good idea. On several drivers, the event is generated during
>> IRQ time, or on a very expensive polling loop. So, buffering is needed to release the
>> IRQ as soon as possible and not adding too much processing during polling.
> 
> Have you seen any real case where this is a problem or is this just 
> conjecture on your behalf? I've written ir decoders for embedded 
> hardware which pass the "event" (duration) through the state machines 
> directly and it works great on hardware with a fraction of the computing 
> power compared to the machines you're using.
> 
> The state machines shouldn't have to do much more than rounding of the 
> duration followed by a couple of integer comparisons (and possibly some 
> bitops). I fail to see how using a kfifo would provide any real 
> improvement.

See the comments at the ML about the noise that the IR handling causes on several
devices. They interfere on applications behavior, cause excess of power consumption,
etc.

There are cases where you need have a timer to get 1ms samples, in order 
to get the IR pulses on devices that don't use an IRQ line. There are lots 
of reports of misc troubles caused by polling the device with high polling rates. 
So, the minimal amount of time used to get the event, the better.

Btw, I think we'll end by needing to have protocol decoders that are capable
to handle just pulses, to avoid adding the extra penalty of doubling the 
sampling rate on such devices. Maybe the better would be to have two versions
of the most popular decoders (NEC, RC-5), one pulse only and the other pulse/space,
in order to support those hardware. This would probably be better than adding
a more complex logic to handle both cases at the same state machine.

In the specific case of hardware-driven events like the mceusb, maybe we can
have two different implementations: one for the cases where the sampling needs to
be done in software, via kfifo, and another one where the hardware is passing a
buffer of events, so, there's no need to double buffering.

I've already added a driver_type field to distinguish between pure hardware IR
decoders and pure software IR decoders:
	http://git.linuxtv.org/mchehab/ir.git?a=commitdiff;h=5b023d2cd652ef42ab8e836cad8d6f077819b83a

So, we may add another driver_type there for the cases where the hardware generates samples.

>>> -
>>> -	for (i = 0; i < len; i++) {
>>> -		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
>>> -		if (rc != sizeof(ev)) {
>>> -			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
>>> -				   rc, sizeof(ev));
>>> -			return -EINVAL;
>>> -		}
>>> -		IR_dprintk(2, "event type %d, time before event: %07luus\n",
>>> -			ev.type, (ev.delta.tv_nsec + 500) / 1000);
>>> -		rc = RUN_DECODER(decode, input_dev, &ev);
>>> -	}
>>> +	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
>>> +	ktime_t			now;
>>> +	s64			delta; /* us */
>>> +
>>> +	if (!ir->raw)
>>> +		return;
>>>  
>>> -	/*
>>> -	 * Call all ir decoders. This allows decoding the same event with
>>> -	 * more than one protocol handler.
>>> -	 */
>>> +	now = ktime_get();
>>> +	delta = ktime_us_delta(now, ir->raw->last_event);
>>
>> This won't work, in the cases where the hardware is providing its own 
>> timings.
> 
> Again, see ir_raw_event_duration()
>  
>>>  
>>> -	return rc;
>>> +	/* Check for a long duration since last event or if we're
>>> +	   being called for the first time */
>>> +	if (delta > USEC_PER_SEC || !ir->raw->last_type)
>>> +		type |= IR_START_EVENT;
>> The "long duration" concept would be better implemented at the driver, since it may
>> vary with the IR carrier and with the protocol details.
> 
> Now you're criticizing your own code...this was one of the few concepts 
> I carried over from your original code. Specifically, ir-raw-event.c, 
> lines 116 - 129 from your current tree:
> 
> 	ktime_get_ts(&ts);
> 
> 	if (timespec_equal(&ir->raw->last_event, &event.delta))
> 		event.type |= IR_START_EVENT;
> 	else
> 		event.delta = timespec_sub(ts, ir->raw->last_event);
> 
> 	memcpy(&ir->raw->last_event, &ts, sizeof(ts));
> 
> 	if (event.delta.tv_sec) {
> 		event.type |= IR_START_EVENT;
> 		event.delta.tv_sec = 0;
> 		event.delta.tv_nsec = 0;
> 	}
> 
> (Note the check on event.delta.tv_sec)

Ok, you beat me on that ;)

The above code is there just to simplify the decoders logic, as no IR 
protocol accept a valid delay of 1 sec. So, basically, the decoders
don't need to use tv_sec. This simplified the decoders logic.

An obvious cleanup would be to just send tv_nsec to the decoders, saving
4 bytes/event at the kfifo, by not sending tv_sec.

>>> +
>>> +	if (type & IR_START_EVENT)
>>> +		ir_raw_event_reset(input_dev);
>>> +	else if (ir->raw->last_type & IR_SPACE)
>>> +		ir_raw_event_duration(input_dev, (int)-delta);
>>> +	else if (ir->raw->last_type & IR_PULSE)
>>> +		ir_raw_event_duration(input_dev, (int)delta);
>> Please, don't use a signal to identify between pulse and space. The IR decoding logic
>> is tricky enough. Just pass the type to the decoder and let it explicitly check if it is
>> pulse or space.
> 
> No idea what you mean here. Why would it be clearer to put the 
> equivalent code in every single decoder instead of adding it once to the 
> place which calls the decoders?

I mean: Just use IR_SPACE/IR_PULSE at the decoders, instead of a signal.

>>> Index: ir/drivers/media/IR/ir-nec-decoder.c
>>> ===================================================================
>>> --- ir.orig/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:16:27.000000000 +0200
>>> +++ ir/drivers/media/IR/ir-nec-decoder.c	2010-04-06 12:17:08.860846045 +0200
>>> @@ -14,22 +14,25 @@
>>>  
>>>  #include <media/ir-core.h>
>>>  
>>> +/*
>>> + * Regarding NEC_HEADER_MARK: some NEC remotes use 16, some 8,
>>> + * some receivers are also good at missing part of the first pulse.
>>> + */
>> The NEC decoder improvements should be on a separate patch.
> 
> They can't since the entire patch is about changing the core raw logic 
> over to primarily use durations, which means the existing nec and rc5 
> decoders need to change in the same patch or bisectability will be 
> broken.

I mean: if you're changing the state machine to support the "missing part of
the first pulse", this specific improvement would be on a separate patch,
as it might cause regressions (the same comments I did for the RC5X applies here).

>>>  #define NEC_NBITS		32
>>> -#define NEC_UNIT		559979 /* ns */
>>> -#define NEC_HEADER_MARK		(16 * NEC_UNIT)
>>> -#define NEC_HEADER_SPACE	(8 * NEC_UNIT)
>>> -#define NEC_REPEAT_SPACE	(4 * NEC_UNIT)
>>> -#define NEC_MARK		(NEC_UNIT)
>>> -#define NEC_0_SPACE		(NEC_UNIT)
>>> -#define NEC_1_SPACE		(3 * NEC_UNIT)
>>> +#define NEC_UNIT		562	/* us */
>> Why changing to microseconds? ktime_t also handles time on nanosseconds.
> 
> Interesting question but you managed to turn it on its head. Why not use 
> femtoseconds? Because it's complete and utter overkill. Not a single IR 
> protocol needs that kind of precision. Both LIRC and the Microsoft IR 
> API's are based on microseconds. Programmable IR hardware (like the 
> Philips Pronto) is based on microseconds. The proper question to ask is 
> why you'd use nanoseconds...

Because the high precision timers (ktime_t, timeval) in Linux are currently based
on nanoseconds. The worst case we have are the devices that need to be polled
to generate a sample for every (IR unit) / 2. We need to focus on saving CPU cycles
at the sampling event logic for those. By not converting it to something else,
we save one division at the event collect.

As Andy explained, it happens that cx18 also sends data in nanoseconds, so
whatever decision taken, some of the devices with in-hardware samplers will
need to convert to whatever used in Kernel.

> 
>>> +#define NEC_HEADER_MARK		6
>>> +#define NEC_HEADER_SPACE	-8
>>> +#define NEC_REPEAT_SPACE	-4
>>> +#define NEC_MARK		1
>>> +#define NEC_0_SPACE		-1
>>> +#define NEC_1_SPACE		-3
>> Those negative values here is really weird... I can't stop thinking on time 
>> travels, when I see a negative time ;)
> 
> Better get used to it, if you're going to maintain ir-core you're going 
> to see it in mailing list discussions once ir-core gains a userbase.
> 
>> Seriously, this change obfuscates the logic, as it is using the mantissa of a number
>> to indicate a time duration, and the signal to indicate the presence or absence of
>> a carrier. Encoding two different measures into a number is not a good thing. I used
>> to do such tricks when programming in Z80 assembly, back on eighties, basically
>> due to the absolute lack of enough memory on those machines with a few kilobytes of RAM.
>> After a few weeks, returning back to the same code to fix were really hard. We don't have
>> such memory constraints anymore. So, let's keep the code as clearer as possible.
> 
> Describing a received ir signal as a number of signed integers 
> describing the duration of pulses (positive) and spaces (negative) in 
> microseconds is pretty much standard (to the extent that any standard 
> exists) in any discussion of IR protocols.
> 
> The decoders Jon Smirl implemented used signed integers, the decoders 
> I've implemented use signed integers. Microsoft's IR API uses signed 
> integers. LIRC uses signed integers (kinda, 23 bits of microsecond 
> duration, one bit for pulse or space - as far as I can remember from 
> LIRC_MODE_MODE2). Other projects also use it (e.g. the developers of 
> decodeir.dll which is probably one of the most used IR API's on the MS 
> platform outside of Microsoft's own API).
> 
> Instead you want to go with a model where you pass in total three 
> arguments to the decoders (struct timespec with nsec and sec + enum 
> type). I do not understand how you would consider this clearer or 
> better.

There are two arguments only: time and type. No matter how encoded, 
they'll be there. So, it is just a matter of better representing it.

As pulse/mark can be distinguished by just one bit, and negative time is 
meaningless, I can understand why hardware developers will use a signal 
bit to pass this information from the hardware to the OS: this saves one 
register on a limited hardware. It is a pretty common practice of abusing
the signal bits in assembler, on similar cases.

Yet, on some moment, this will need to be splitted into time and type again.

One of the main characteristics of Linux development is that the code should
be easy to be read, in order to allow its maintainable by the community. So,
the less obfuscated the code, the better.

For an IR expert, this might be clear that it represents 8 units of space:
	#define NEC_HEADER_SPACE	-8

but, for most, this is just a negative magic number.

On the other hand, by looking at:
	#define NEC_HEADER_SPACE	(8 * NEC_UNIT)

All developers will understand that this represents 8 units of space.

>> I've stripped the decoders code, since they're basically implementing 
>> the architectural
>> changes you're proposing Let's first finish the discussions about the changes.
>>
>> Btw, I noticed that you've added some improvements to the decoders, like the
>> changes to support RC-5X. The better is to send it as a separate patch, due to a few reasons:
>>
>> 	- RC-5X addition has nothing to do with "Teach drivers/media/IR/ir-raw-event.c
>> to use durations" (the subject of this RFC patch);
>>
>> 	- Bigger patches have more chances of getting nacked, since they touch on more parts
>> of the code. So, you'll need to rework more code;
>>
>> 	- The addition of RC-5X shouldn't break RC-5. By having it as a separate patch,
>> it is easier to test the changes;
>>
>> 	- If later discovered a regression, it would be easier to bisect and see if the
>> changes were introduced by RC-5X or by the architectural changes, if the changes are broken
>> into two patches.
>>
> 
> I've already addressed RC5X above.

Ok.

>>> -static unsigned int ir_rc5_remote_gap = 888888;
>> The idea of static int is that, on saa7134, this value can be adjustable from userspace.
>> probably, some hardware use a non-standard carrier, so we'll need to export it also
>> via sysfs, to avoid regressions.
> 
> The decoders in my patch have a +/-50% tolerance for pulse/space unit 
> durations, if it turns out to be insufficient, then it's time to look at 
> solutions, doing it now is premature.

Yes, the current ir-r5-decoder already have this tolerance. 

The saa7134-input RC-5 decoder has a problem: it counts events durations from the start
event. So, if the frequency is shifted, the probability of having a decoding error
on the last bit is higher than the probability of a decoding error on the first bit.
Maybe that's why this parameter was needed.

Ok, let's convert it into a constant for now.


-- 

Cheers,
Mauro