Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
On Fri, Apr 09, 2010 at 11:00:41AM -0300, Mauro Carvalho Chehab wrote:
struct {
unsigned mark : 1;
unsigned duration :31;
}
There's no memory spend at all: it will use just one unsigned int and it is
clearly indicated what's mark and what's duration.
If all three of you agree on this approch, I'll write a patch to convert
ir-core to use it instead.
--
David Härdeman
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
On Sat, 2010-04-10 at 08:48 +0200, David Härdeman wrote:
On Fri, Apr 09, 2010 at 11:00:41AM -0300, Mauro Carvalho Chehab wrote:
struct {
unsigned mark : 1;
unsigned duration :31;
}
There's no memory spend at all: it will use just one unsigned int and it is
clearly indicated what's mark and what's duration.
If all three of you agree on this approch, I'll write a patch to convert
ir-core to use it instead.
I'm OK with it.
I haven't been paying close attention,so I must ask: What will the units
of duration be?
a. If we use nanoseconds the max duration is 2.147 seconds.
If passing pulse measurments out to LIRC, there are cases where irrecord
and lircd want the duration of the long silence between the
transmissions from the remote. Do any remotes have silence periods
longer than 2.1 seconds?
b. If we use microseconds, the max duration is 214.7 seconds or 3.6
minutes. That's too high to be useful.
c. Something in between, like 1/8 (or 1/2, 1/4, or 1/10) of a
microsecond? 1/8 gives a max duration of 26.8 seconds and a little
extra precision.
Regards,
Andy
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
Andy Walls wrote:
On Sat, 2010-04-10 at 08:48 +0200, David Härdeman wrote:
On Fri, Apr 09, 2010 at 11:00:41AM -0300, Mauro Carvalho Chehab wrote:
struct {
unsigned mark : 1;
unsigned duration :31;
}
There's no memory spend at all: it will use just one unsigned int and it is
clearly indicated what's mark and what's duration.
If all three of you agree on this approch, I'll write a patch to convert
ir-core to use it instead.
I'm OK with it.
I haven't been paying close attention,so I must ask: What will the units
of duration be?
a. If we use nanoseconds the max duration is 2.147 seconds.
If passing pulse measurments out to LIRC, there are cases where irrecord
and lircd want the duration of the long silence between the
transmissions from the remote. Do any remotes have silence periods
longer than 2.1 seconds?
b. If we use microseconds, the max duration is 214.7 seconds or 3.6
minutes. That's too high to be useful.
c. Something in between, like 1/8 (or 1/2, 1/4, or 1/10) of a
microsecond? 1/8 gives a max duration of 26.8 seconds and a little
extra precision.
(c) is really ugly.
(b) max limit is too high. Currently, the core assumes that everything longer
than one second is enough to re-start the state machine. So, I think (a)
is the better option.
Another way to see it: it is not reasonable for someone to press a key and wait
for 2.1 seconds to see one bit of the key to be recognized.
So, IMHO, let's just use nanoseconds with 31 bits. the sampling event function
should check for ktime value: if bigger than 2^32-1, then assume it is a
long event, resetting the state machine.
Cheers,
Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
On Sat, Apr 10, 2010 at 2:48 AM, David Härdeman da...@hardeman.nu wrote:
On Fri, Apr 09, 2010 at 11:00:41AM -0300, Mauro Carvalho Chehab wrote:
struct {
unsigned mark : 1;
unsigned duration :31;
}
There's no memory spend at all: it will use just one unsigned int and it is
clearly indicated what's mark and what's duration.
If all three of you agree on this approach, I'll write a patch to convert
ir-core to use it instead.
Fine with me.
--
David Härdeman
--
Jon Smirl
jonsm...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
On Sat, 2010-04-10 at 09:10 -0300, Mauro Carvalho Chehab wrote:
Andy Walls wrote:
On Sat, 2010-04-10 at 08:48 +0200, David Härdeman wrote:
On Fri, Apr 09, 2010 at 11:00:41AM -0300, Mauro Carvalho Chehab wrote:
struct {
unsigned mark : 1;
unsigned duration :31;
}
There's no memory spend at all: it will use just one unsigned int and it
is
clearly indicated what's mark and what's duration.
If all three of you agree on this approch, I'll write a patch to convert
ir-core to use it instead.
I'm OK with it.
I haven't been paying close attention,so I must ask: What will the units
of duration be?
a. If we use nanoseconds the max duration is 2.147 seconds.
If passing pulse measurments out to LIRC, there are cases where irrecord
and lircd want the duration of the long silence between the
transmissions from the remote. Do any remotes have silence periods
longer than 2.1 seconds?
b. If we use microseconds, the max duration is 214.7 seconds or 3.6
minutes. That's too high to be useful.
c. Something in between, like 1/8 (or 1/2, 1/4, or 1/10) of a
microsecond? 1/8 gives a max duration of 26.8 seconds and a little
extra precision.
(c) is really ugly.
(b) max limit is too high. Currently, the core assumes that everything longer
than one second is enough to re-start the state machine. So, I think (a)
is the better option.
Another way to see it: it is not reasonable for someone to press a key and
wait
for 2.1 seconds to see one bit of the key to be recognized.
True enough.
So, IMHO, let's just use nanoseconds with 31 bits. the sampling event function
should check for ktime value: if bigger than 2^32-1, then assume it is a
long event, resetting the state machine.
Sounds OK to me. Thanks for the reply.
Regards,
Andy
Cheers,
Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
On Thu, 2010-04-08 at 13:39 +0200, David Härdeman wrote:
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).
Furthermore, using signed int's to represent pulse/space durations in ms
is a well-known approach to anyone with experience in writing ir decoders.
This patch (which has been tested this time) is still a RFC on my proposed
interface changes.
Changes since last version:
o s64's are used to represent pulse/space durations in ns.
o Lots of #defines are used in the decoders
o Refreshed to apply cleanly on top of Mauro's current git tree
o Jon's comments wrt. interrupt-context safe functions have been added
Index: ir/drivers/media/IR/ir-raw-event.c
===
--- ir.orig/drivers/media/IR/ir-raw-event.c 2010-04-08 12:30:28.036098192
+0200
+++ ir/drivers/media/IR/ir-raw-event.c2010-04-08 12:45:19.780145403
+0200
@@ -15,9 +15,10 @@
#include media/ir-core.h
#include linux/workqueue.h
#include linux/spinlock.h
+#include linux/sched.h
-/* Define the max number of bit transitions per IR keycode */
-#define MAX_IR_EVENT_SIZE256
+/* Define the max number of pulse/space transitions to buffer */
+#define MAX_IR_EVENT_SIZE 512
/* Used to handle IR raw handler extensions */
static LIST_HEAD(ir_raw_handler_list);
@@ -53,19 +54,30 @@
/* Used to load the decoders */
static struct work_struct wq_load;
+static void ir_raw_event_work(struct work_struct *work)
+{
+ s64 d;
+ struct ir_raw_event_ctrl *raw =
+ container_of(work, struct ir_raw_event_ctrl, rx_work);
+
+ while (kfifo_out(raw-kfifo, d, sizeof(d)) == sizeof(d))
+ RUN_DECODER(decode, raw-input_dev, d);
+}
+
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);
+ ir-raw-input_dev = input_dev;
+ INIT_WORK(ir-raw-rx_work, ir_raw_event_work);
- rc = kfifo_alloc(ir-raw-kfifo, size, GFP_KERNEL);
+ rc = kfifo_alloc(ir-raw-kfifo, sizeof(s64) * MAX_IR_EVENT_SIZE,
+ GFP_KERNEL);
if (rc 0) {
kfree(ir-raw);
ir-raw = NULL;
@@ -90,6 +102,7 @@
if (!ir-raw)
return;
+ cancel_work_sync(ir-raw-rx_work);
RUN_DECODER(raw_unregister, input_dev);
kfifo_free(ir-raw-kfifo);
@@ -97,74 +110,90 @@
ir-raw = NULL;
}
-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
+/**
+ * ir_raw_event_store() - 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 in ns
+ *
+ * This routine (which may be called from an interrupt context) stores a
+ * pulse/space duration for the raw ir decoding state machines. Pulses are
+ * signalled as positive values and spaces as negative values. A zero value
+ * will reset the decoding state machines.
+ */
+int ir_raw_event_store(struct input_dev *input_dev, s64 duration)
{
- struct ir_input_dev *ir = input_get_drvdata(input_dev);
- struct timespec ts;
- struct ir_raw_event event;
- int rc;
+ struct ir_input_dev *ir = input_get_drvdata(input_dev);
if (!ir-raw)
return -EINVAL;
- event.type = type;
- event.delta.tv_sec = 0;
- event.delta.tv_nsec = 0;
+ if (kfifo_in(ir-raw-kfifo, duration, sizeof(duration)) !=
sizeof(duration))
+ return -ENOMEM;
- ktime_get_ts(ts);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ir_raw_event_store);
- if (timespec_equal(ir-raw-last_event, event.delta))
- event.type |= IR_START_EVENT;
- else
- event.delta = timespec_sub(ts, ir-raw-last_event);
+/**
+ * ir_raw_event_store_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 (which may be called from an interrupt context) is used to
+ * store the beginning of an ir pulse or space (or the start/end of ir
+ * reception) for the raw ir decoding state machines. This is used by
+ * hardware which does not provide durations directly but only interrupts
+ * (or similar
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
On Fri, Apr 9, 2010 at 7:25 AM, Andy Walls awa...@md.metrocast.net wrote:
On Thu, 2010-04-08 at 13:39 +0200, David Härdeman wrote:
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).
Furthermore, using signed int's to represent pulse/space durations in ms
is a well-known approach to anyone with experience in writing ir decoders.
This patch (which has been tested this time) is still a RFC on my proposed
interface changes.
Changes since last version:
o s64's are used to represent pulse/space durations in ns.
o Lots of #defines are used in the decoders
o Refreshed to apply cleanly on top of Mauro's current git tree
o Jon's comments wrt. interrupt-context safe functions have been added
Index: ir/drivers/media/IR/ir-raw-event.c
===
--- ir.orig/drivers/media/IR/ir-raw-event.c 2010-04-08 12:30:28.036098192
+0200
+++ ir/drivers/media/IR/ir-raw-event.c 2010-04-08 12:45:19.780145403
+0200
@@ -15,9 +15,10 @@
#include media/ir-core.h
#include linux/workqueue.h
#include linux/spinlock.h
+#include linux/sched.h
-/* Define the max number of bit transitions per IR keycode */
-#define MAX_IR_EVENT_SIZE 256
+/* Define the max number of pulse/space transitions to buffer */
+#define MAX_IR_EVENT_SIZE 512
/* Used to handle IR raw handler extensions */
static LIST_HEAD(ir_raw_handler_list);
@@ -53,19 +54,30 @@
/* Used to load the decoders */
static struct work_struct wq_load;
+static void ir_raw_event_work(struct work_struct *work)
+{
+ s64 d;
+ struct ir_raw_event_ctrl *raw =
+ container_of(work, struct ir_raw_event_ctrl, rx_work);
+
+ while (kfifo_out(raw-kfifo, d, sizeof(d)) == sizeof(d))
+ RUN_DECODER(decode, raw-input_dev, d);
+}
+
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);
+ ir-raw-input_dev = input_dev;
+ INIT_WORK(ir-raw-rx_work, ir_raw_event_work);
- rc = kfifo_alloc(ir-raw-kfifo, size, GFP_KERNEL);
+ rc = kfifo_alloc(ir-raw-kfifo, sizeof(s64) * MAX_IR_EVENT_SIZE,
+ GFP_KERNEL);
if (rc 0) {
kfree(ir-raw);
ir-raw = NULL;
@@ -90,6 +102,7 @@
if (!ir-raw)
return;
+ cancel_work_sync(ir-raw-rx_work);
RUN_DECODER(raw_unregister, input_dev);
kfifo_free(ir-raw-kfifo);
@@ -97,74 +110,90 @@
ir-raw = NULL;
}
-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type
type)
+/**
+ * ir_raw_event_store() - 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 in ns
+ *
+ * This routine (which may be called from an interrupt context) stores a
+ * pulse/space duration for the raw ir decoding state machines. Pulses are
+ * signalled as positive values and spaces as negative values. A zero value
+ * will reset the decoding state machines.
+ */
+int ir_raw_event_store(struct input_dev *input_dev, s64 duration)
{
- struct ir_input_dev *ir = input_get_drvdata(input_dev);
- struct timespec ts;
- struct ir_raw_event event;
- int rc;
+ struct ir_input_dev *ir = input_get_drvdata(input_dev);
if (!ir-raw)
return -EINVAL;
- event.type = type;
- event.delta.tv_sec = 0;
- event.delta.tv_nsec = 0;
+ if (kfifo_in(ir-raw-kfifo, duration, sizeof(duration)) !=
sizeof(duration))
+ return -ENOMEM;
- ktime_get_ts(ts);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ir_raw_event_store);
- if (timespec_equal(ir-raw-last_event, event.delta))
- event.type |= IR_START_EVENT;
- else
- event.delta = timespec_sub(ts, ir-raw-last_event);
+/**
+ * ir_raw_event_store_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 (which may be called from an interrupt context) is used to
+ * store the beginning of an ir pulse or space (or the start/end of ir
+ * reception) for the raw ir decoding state machines. This is used by
+ * hardware which does not provide durations directly
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
Jon Smirl wrote:
+/* macros for ir decoders */
+#define PULSE(units) ((units))
+#define SPACE(units) (-(units))
Encoding pulse vs space with a negative sign, even if now hidden with
macros, is still just using a sign instead of a boolean. Memory in
modern computers (and now microcontrollers) is cheap and only getting
cheaper. Don't give up readability, flexibility, or mainatainability,
for the sake of saving memory.
That was my point since the beginning: the amount of saved memory doesn't
justify the lack of readability. I understand such constraints when using
a hardware implementation on a microcontroller chip that offers just a very
few limited registers and a very small or no RAM. Also, if you define it with
something like:
struct {
unsigned mark : 1;
unsigned duration :31;
}
There's no memory spend at all: it will use just one unsigned int and it is
clearly indicated what's mark and what's duration.
I agree with this. I did it with signed ints in my first version, then
ripped it out and switched to duration + boolean. The duration/boolean
pair was much easier to understand. This is a matter of style, both
schemes work.
Yes. It shouldn't be hard to convert the code to better represent the
type/duration
vector in the future. Actually, that's one of the things i took into
consideration
when accepting the patch: the code readability were not seriously compromised
with
the usage of the macros, and, if needed, a patch converting it to a structured
type
wouldn't be hard.
#endif /* _IR_CORE */
Index: ir/drivers/media/IR/ir-nec-decoder.c
===
--- ir.orig/drivers/media/IR/ir-nec-decoder.c 2010-04-08 12:30:28.0
+0200
+++ ir/drivers/media/IR/ir-nec-decoder.c 2010-04-08 12:35:02.276484204
+0200
@@ -13,15 +13,16 @@
*/
#include media/ir-core.h
+#include linux/bitrev.h
#define NEC_NBITS32
-#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 562500 /* ns */
Have you got a spec on the NEC protocol that justifies 562.5 usec?
From the best I can tell from the sources I have read and some deductive
reasoning, 560 usec is the actual number. Here's one:
http://www.audiodevelopers.com/temp/Remote_Controls.ppt
Note:
560 usec * 38 kHz ~= 4192/197
In the PPT you reference there are three numbers...
http://www.sbprojects.com/knowledge/ir/nec.htm
560us
1.12ms
2.25ms
I think those are rounding errors.
562.5 * 2 = 1.125ms * 2 = 2.25ms
Most IR protocols are related in a power of two pattern for their
timings to make them easy to decode.
The protocol doesn't appear to be based on an even number of 38Khz cycles.
These are easy things to change as we get better data on the protocols.
I don't think that the actual number really matters much. The decoders are
reliable enough to work with such small differences. I suspect that, in
practice, hardware developers just use a close frequency that can be divided
by some existing XTAL clock already available at the machines. In the case of
video devices, most of them use a 27 MHz clock. If divided by 711, this gives
a clock of 37.974 kHz, and the closest timings are 579 us and 605 us.
So, in practical, I think we'll see much more devices using 579 us than
560 us or 562 us.
and that the three numbers that yield ~560 usec don't evenly divide each
other:
$ factor 4192 197 38000
4192: 2 2 2 2 2 131
197: 197
38000: 2 2 2 2 5 5 5 19
which strikes me as being done on purpose (maybe only by me?).
Also note that:
4192 / 38 kHz = 110.32 usec
and public sources list 110 usec as the NEC repeat period.
+#define NEC_HEADER_PULSE PULSE(16)
+#define NEC_HEADER_SPACE SPACE(8)
+#define NEC_REPEAT_SPACE SPACE(4)
+#define NEC_BIT_PULSEPULSE(1)
+#define NEC_BIT_0_SPACE SPACE(1)
+#define NEC_BIT_1_SPACE SPACE(3)
This is slightly better than your previous patch, but the original
#defines were still clearer. A maintainer coming through has to spend
time and energy on asking 16 what? for example.
The units can be expressed as a comment:
#define NEC_BIT_PULSEPULSE(1) /* nec units */
A patch like that is welcome.
--
Cheers,
Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
On Fri, Apr 9, 2010 at 10:00 AM, Mauro Carvalho Chehab
mche...@infradead.org wrote:
Jon Smirl wrote:
+/* macros for ir decoders */
+#define PULSE(units) ((units))
+#define SPACE(units) (-(units))
Encoding pulse vs space with a negative sign, even if now hidden with
macros, is still just using a sign instead of a boolean. Memory in
modern computers (and now microcontrollers) is cheap and only getting
cheaper. Don't give up readability, flexibility, or mainatainability,
for the sake of saving memory.
That was my point since the beginning: the amount of saved memory doesn't
justify the lack of readability. I understand such constraints when using
a hardware implementation on a microcontroller chip that offers just a very
few limited registers and a very small or no RAM. Also, if you define it with
something like:
struct {
unsigned mark : 1;
unsigned duration :31;
}
There's no memory spend at all: it will use just one unsigned int and it is
clearly indicated what's mark and what's duration.
I agree with this. I did it with signed ints in my first version, then
ripped it out and switched to duration + boolean. The duration/boolean
pair was much easier to understand. This is a matter of style, both
schemes work.
Yes. It shouldn't be hard to convert the code to better represent the
type/duration
vector in the future. Actually, that's one of the things i took into
consideration
when accepting the patch: the code readability were not seriously compromised
with
the usage of the macros, and, if needed, a patch converting it to a
structured type
wouldn't be hard.
#endif /* _IR_CORE */
Index: ir/drivers/media/IR/ir-nec-decoder.c
===
--- ir.orig/drivers/media/IR/ir-nec-decoder.c 2010-04-08
12:30:28.0 +0200
+++ ir/drivers/media/IR/ir-nec-decoder.c 2010-04-08
12:35:02.276484204 +0200
@@ -13,15 +13,16 @@
*/
#include media/ir-core.h
+#include linux/bitrev.h
#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 562500 /* ns */
Have you got a spec on the NEC protocol that justifies 562.5 usec?
From the best I can tell from the sources I have read and some deductive
reasoning, 560 usec is the actual number. Here's one:
http://www.audiodevelopers.com/temp/Remote_Controls.ppt
Note:
560 usec * 38 kHz ~= 4192/197
In the PPT you reference there are three numbers...
http://www.sbprojects.com/knowledge/ir/nec.htm
560us
1.12ms
2.25ms
I think those are rounding errors.
562.5 * 2 = 1.125ms * 2 = 2.25ms
Most IR protocols are related in a power of two pattern for their
timings to make them easy to decode.
The protocol doesn't appear to be based on an even number of 38Khz cycles.
These are easy things to change as we get better data on the protocols.
I don't think that the actual number really matters much. The decoders are
reliable enough to work with such small differences. I suspect that, in
I found that the ratios between the numbers are the critical item, not
the numbers themselves.
The absolute numbers are used to differentiate the protocol families.
The total length of the messages is also important in differentiating
the families.
The protocol decoders should reconcile the total message length as
another check to make sure they aren't triggering on a message in the
wrong protocol. Add up the durations of everything seen and see if it
is within 5-10% of the expected message length.
practice, hardware developers just use a close frequency that can be divided
by some existing XTAL clock already available at the machines. In the case of
video devices, most of them use a 27 MHz clock. If divided by 711, this gives
a clock of 37.974 kHz, and the closest timings are 579 us and 605 us.
So, in practical, I think we'll see much more devices using 579 us than
560 us or 562 us.
and that the three numbers that yield ~560 usec don't evenly divide each
other:
$ factor 4192 197 38000
4192: 2 2 2 2 2 131
197: 197
38000: 2 2 2 2 5 5 5 19
which strikes me as being done on purpose (maybe only by me?).
Also note that:
4192 / 38 kHz = 110.32 usec
and public sources list 110 usec as the NEC repeat period.
+#define NEC_HEADER_PULSE PULSE(16)
+#define NEC_HEADER_SPACE SPACE(8)
+#define NEC_REPEAT_SPACE SPACE(4)
+#define NEC_BIT_PULSE PULSE(1)
+#define NEC_BIT_0_SPACE SPACE(1)
+#define NEC_BIT_1_SPACE SPACE(3)
This is slightly better than your previous patch,
Found NEC IR specification in NEC uPD6122 datasheet (Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations)
On Fri, 2010-04-09 at 11:00 -0300, Mauro Carvalho Chehab wrote: Jon Smirl wrote: #define NEC_NBITS32 -#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 562500 /* ns */ Have you got a spec on the NEC protocol that justifies 562.5 usec? From the best I can tell from the sources I have read and some deductive reasoning, 560 usec is the actual number. Here's one: http://www.audiodevelopers.com/temp/Remote_Controls.ppt Note: 560 usec * 38 kHz ~= 4192/197 In the PPT you reference there are three numbers... http://www.sbprojects.com/knowledge/ir/nec.htm 560us 1.12ms 2.25ms I think those are rounding errors. 562.5 * 2 = 1.125ms * 2 = 2.25ms Most IR protocols are related in a power of two pattern for their timings to make them easy to decode. The protocol doesn't appear to be based on an even number of 38Khz cycles. These are easy things to change as we get better data on the protocols. I just found authoritative data. It is in the datasheet for the uPD6122 authored by NEC Corporation: http://www.datasheetcatalog.org/datasheet/nec/UPD6122G-002.pdf Looking at page 11, especially line (5), it appears that all the timings are derived in terms of 1/3 of a carrier period and powers of 2. So Resonator frequency: fr = 455 kHz (AM IF parts are cheap apparently) Carrier frequency: fc = fr / 12 = 37.91667 kHz Duty cycle: 1/3 unit pulse: 64/3 / fc = 562.637 us (Jon was closer than me) header pulse: 16 * 64/3 / fc = 9.002 ms header space: 8 * 64/3 / fc = 4.501 ms repeat space: 4 * 64/3 / fc = 2.250 ms '1' symbol: 4 * 64/3 / fc = 2.250 ms '0' symbol: 2 * 64/3 / fc = 1.125 ms repeat time: 192 * 64/3 / fc = 108.026 ms Page 15 also shows that the older chips had a silence gap that could result in signals coming closer than 108 ms. Whew! I'm glad I've worked through my fit of Obsessive Compulsive Disorder for now. :) Regards, Andy -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
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).
Furthermore, using signed int's to represent pulse/space durations in ms
is a well-known approach to anyone with experience in writing ir decoders.
This patch (which has been tested this time) is still a RFC on my proposed
interface changes.
Changes since last version:
o s64's are used to represent pulse/space durations in ns.
o Lots of #defines are used in the decoders
o Refreshed to apply cleanly on top of Mauro's current git tree
o Jon's comments wrt. interrupt-context safe functions have been added
Index: ir/drivers/media/IR/ir-raw-event.c
===
--- ir.orig/drivers/media/IR/ir-raw-event.c 2010-04-08 12:30:28.036098192
+0200
+++ ir/drivers/media/IR/ir-raw-event.c 2010-04-08 12:45:19.780145403 +0200
@@ -15,9 +15,10 @@
#include media/ir-core.h
#include linux/workqueue.h
#include linux/spinlock.h
+#include linux/sched.h
-/* Define the max number of bit transitions per IR keycode */
-#define MAX_IR_EVENT_SIZE 256
+/* Define the max number of pulse/space transitions to buffer */
+#define MAX_IR_EVENT_SIZE 512
/* Used to handle IR raw handler extensions */
static LIST_HEAD(ir_raw_handler_list);
@@ -53,19 +54,30 @@
/* Used to load the decoders */
static struct work_struct wq_load;
+static void ir_raw_event_work(struct work_struct *work)
+{
+ s64 d;
+ struct ir_raw_event_ctrl *raw =
+ container_of(work, struct ir_raw_event_ctrl, rx_work);
+
+ while (kfifo_out(raw-kfifo, d, sizeof(d)) == sizeof(d))
+ RUN_DECODER(decode, raw-input_dev, d);
+}
+
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);
+ ir-raw-input_dev = input_dev;
+ INIT_WORK(ir-raw-rx_work, ir_raw_event_work);
- rc = kfifo_alloc(ir-raw-kfifo, size, GFP_KERNEL);
+ rc = kfifo_alloc(ir-raw-kfifo, sizeof(s64) * MAX_IR_EVENT_SIZE,
+GFP_KERNEL);
if (rc 0) {
kfree(ir-raw);
ir-raw = NULL;
@@ -90,6 +102,7 @@
if (!ir-raw)
return;
+ cancel_work_sync(ir-raw-rx_work);
RUN_DECODER(raw_unregister, input_dev);
kfifo_free(ir-raw-kfifo);
@@ -97,74 +110,90 @@
ir-raw = NULL;
}
-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
+/**
+ * ir_raw_event_store() - 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 in ns
+ *
+ * This routine (which may be called from an interrupt context) stores a
+ * pulse/space duration for the raw ir decoding state machines. Pulses are
+ * signalled as positive values and spaces as negative values. A zero value
+ * will reset the decoding state machines.
+ */
+int ir_raw_event_store(struct input_dev *input_dev, s64 duration)
{
- struct ir_input_dev *ir = input_get_drvdata(input_dev);
- struct timespec ts;
- struct ir_raw_event event;
- int rc;
+ struct ir_input_dev *ir = input_get_drvdata(input_dev);
if (!ir-raw)
return -EINVAL;
- event.type = type;
- event.delta.tv_sec = 0;
- event.delta.tv_nsec = 0;
+ if (kfifo_in(ir-raw-kfifo, duration, sizeof(duration)) !=
sizeof(duration))
+ return -ENOMEM;
- ktime_get_ts(ts);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ir_raw_event_store);
- if (timespec_equal(ir-raw-last_event, event.delta))
- event.type |= IR_START_EVENT;
- else
- event.delta = timespec_sub(ts, ir-raw-last_event);
+/**
+ * ir_raw_event_store_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 (which may be called from an interrupt context) is used to
+ * store the beginning of an ir pulse or space (or the start/end of ir
+ * reception) for the raw ir decoding state machines. This is used by
+ * hardware which does not provide durations directly but only interrupts
+ * (or similar events) on state change.
+ */
+int ir_raw_event_store_edge(struct input_dev *input_dev, enum raw_event_type
type)
+{
+
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
David Härdeman wrote:
Your're fast!
OK, the code looks good. I'll test and apply it, if it passes on the test.
The only missed thing is the comment about the kfifo size (see the email
I just sent). If you prefer, I can add a one line comment when applying it,
to avoid you to re-send the code.
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).
Furthermore, using signed int's to represent pulse/space durations in ms
is a well-known approach to anyone with experience in writing ir decoders.
This patch (which has been tested this time) is still a RFC on my proposed
interface changes.
Changes since last version:
o s64's are used to represent pulse/space durations in ns.
o Lots of #defines are used in the decoders
o Refreshed to apply cleanly on top of Mauro's current git tree
o Jon's comments wrt. interrupt-context safe functions have been added
Index: ir/drivers/media/IR/ir-raw-event.c
===
--- ir.orig/drivers/media/IR/ir-raw-event.c 2010-04-08 12:30:28.036098192
+0200
+++ ir/drivers/media/IR/ir-raw-event.c2010-04-08 12:45:19.780145403
+0200
@@ -15,9 +15,10 @@
#include media/ir-core.h
#include linux/workqueue.h
#include linux/spinlock.h
+#include linux/sched.h
-/* Define the max number of bit transitions per IR keycode */
-#define MAX_IR_EVENT_SIZE256
+/* Define the max number of pulse/space transitions to buffer */
+#define MAX_IR_EVENT_SIZE 512
/* Used to handle IR raw handler extensions */
static LIST_HEAD(ir_raw_handler_list);
@@ -53,19 +54,30 @@
/* Used to load the decoders */
static struct work_struct wq_load;
+static void ir_raw_event_work(struct work_struct *work)
+{
+ s64 d;
+ struct ir_raw_event_ctrl *raw =
+ container_of(work, struct ir_raw_event_ctrl, rx_work);
+
+ while (kfifo_out(raw-kfifo, d, sizeof(d)) == sizeof(d))
+ RUN_DECODER(decode, raw-input_dev, d);
+}
+
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);
+ ir-raw-input_dev = input_dev;
+ INIT_WORK(ir-raw-rx_work, ir_raw_event_work);
- rc = kfifo_alloc(ir-raw-kfifo, size, GFP_KERNEL);
+ rc = kfifo_alloc(ir-raw-kfifo, sizeof(s64) * MAX_IR_EVENT_SIZE,
+ GFP_KERNEL);
if (rc 0) {
kfree(ir-raw);
ir-raw = NULL;
@@ -90,6 +102,7 @@
if (!ir-raw)
return;
+ cancel_work_sync(ir-raw-rx_work);
RUN_DECODER(raw_unregister, input_dev);
kfifo_free(ir-raw-kfifo);
@@ -97,74 +110,90 @@
ir-raw = NULL;
}
-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)
+/**
+ * ir_raw_event_store() - 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 in ns
+ *
+ * This routine (which may be called from an interrupt context) stores a
+ * pulse/space duration for the raw ir decoding state machines. Pulses are
+ * signalled as positive values and spaces as negative values. A zero value
+ * will reset the decoding state machines.
+ */
+int ir_raw_event_store(struct input_dev *input_dev, s64 duration)
{
- struct ir_input_dev *ir = input_get_drvdata(input_dev);
- struct timespec ts;
- struct ir_raw_event event;
- int rc;
+ struct ir_input_dev *ir = input_get_drvdata(input_dev);
if (!ir-raw)
return -EINVAL;
- event.type = type;
- event.delta.tv_sec = 0;
- event.delta.tv_nsec = 0;
+ if (kfifo_in(ir-raw-kfifo, duration, sizeof(duration)) !=
sizeof(duration))
+ return -ENOMEM;
- ktime_get_ts(ts);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ir_raw_event_store);
- if (timespec_equal(ir-raw-last_event, event.delta))
- event.type |= IR_START_EVENT;
- else
- event.delta = timespec_sub(ts, ir-raw-last_event);
+/**
+ * ir_raw_event_store_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 (which may be called from an interrupt context) is used to
Re: [RFC3] Teach drivers/media/IR/ir-raw-event.c to use durations
David Härdeman wrote: 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). Furthermore, using signed int's to represent pulse/space durations in ms is a well-known approach to anyone with experience in writing ir decoders. This patch (which has been tested this time) is still a RFC on my proposed interface changes. Changes since last version: o s64's are used to represent pulse/space durations in ns. o Lots of #defines are used in the decoders o Refreshed to apply cleanly on top of Mauro's current git tree o Jon's comments wrt. interrupt-context safe functions have been added Ok, tested it with a variety of NEC/NEC extended/RC-5 IR's I have, with the saa7134 hardware. All worked. There's just a few checkpatch.pl complains, and the most important thing: It lacks your SOB ;) Please fix the checkpatch.pl errors, add a kfifo size comment and your SOB and resend it to me (or, if you prefer, just send your SOB. I can take care of the rest, as they're just trivial things). -- Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
