Re: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Mauro Carvalho Chehab wrote: > Andy Walls wrote: >> I have an RC-5 decoder in cx23885-input.c that isn't as clean as the NEC >> protocol decoder I developed. The cx23885-input.c RC-5 decoder is not a >> very explicit state machine however (it is a bit hack-ish). > > The state machine seems to be working fine with the code, but I think I > found the issue: it was expecting 14 bits after the start+toggle bits, instead > of a total of 14 bits. I'll fix it. I'll probably end by simplifying it to > have > only 3 states: inactive, mark-space and trailer. Done. I've re-written the state machine logic. The code is now simpler to understand, require less processing and works properly with RC-5. Instead of generating an intermediate code, like the code in ir-functions, it measures directly the length of each pulse or space event and generate the corresponding bit directly, putting it into a shift register. At the end of the 14 bits reception, the shift register will contain the scancode. When compared with saa7134 original RC5 decoder, this code is much more reliable, since it doesn't propagate the errors, if the frequency is not precisely 36 kHz. I tested here with my device and it is properly recognizing the Hauppauge Grey IR keys. Both NEC and RC-5 decoders can run in parallel. The patch here: http://git.linuxtv.org/mchehab/ir.git?a=commitdiff;h=37b215ea1280a621d652469cd35328a208f8ef77 And the complete code: http://git.linuxtv.org/mchehab/ir.git?a=blob;f=drivers/media/IR/ir-rc5-decoder.c;h=a62277b625a8ed78028e7060a677598eeae03ffe;hb=37b215ea1280a621d652469cd35328a208f8ef77 I'll likely send an email to the ML with the RC patches that are on my experimental tree, to properly document, and merge it at the -git, together with the other pending requests. -- 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: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Andy Walls wrote: >>> 2. A common glitch filtering library function that can be used by all >>> decoders, and that also can accept a decoder specified minimum >>> acceptable pulse width. >> Seems a nice improvement. I doubt I'll have time for handling it right now, >> since there are still many things to do, but I'll put it on my todo list. >> Of course, patches adding it are wellcome ;) > > :) > > OK. When I find time I'll hack something up as a prototype. Thanks! >> Btw, I added a RC-5 decoder there, at my IR experimental tree: >> http://git.linuxtv.org/mchehab/ir.git > > I'll try to review it some time this week. Streaming state machine > decoders do seem to be best way to go with these decoders. > > I have an RC-5 decoder in cx23885-input.c that isn't as clean as the NEC > protocol decoder I developed. The cx23885-input.c RC-5 decoder is not a > very explicit state machine however (it is a bit hack-ish). The state machine seems to be working fine with the code, but I think I found the issue: it was expecting 14 bits after the start+toggle bits, instead of a total of 14 bits. I'll fix it. I'll probably end by simplifying it to have only 3 states: inactive, mark-space and trailer. >> Unfortunately, there's some problem with either my Remote Controller or >> with the saa7134 driver. After 11 bits received, after the 2 start bits, >> it receives a pause (see the enclosed sequence). > > -ENOATTACHMENT Sorry! It is basically the same output as yours. See enclosed. >> I'm starting to suspect that the Hauppauge Grey IR produces a sequence with >> shorter >> bits, but, as the hardware decoders are capable or receiving IR codes, it may >> also be a hardware problem. > > The fundamental unit in RC-5 is 32 cycles / 36 kHz = 89 ns ~= 889 us. > > I turned on the cx23888-ir.c debugging on the HVR-1850 and using a > Hauppague grey remote (address 0x1e IIRC) and got this as just one > example: > > cx23885[1]/888-ir: rx read: 802037 ns mark > cx23885[1]/888-ir: rx read: 852704 ns space > cx23885[1]/888-ir: rx read: 775370 ns mark > cx23885[1]/888-ir: rx read: 852407 ns space > cx23885[1]/888-ir: rx read: 802037 ns mark > cx23885[1]/888-ir: rx read: 852852 ns space > cx23885[1]/888-ir: rx read: 775667 ns mark > cx23885[1]/888-ir: rx read: 852407 ns space > cx23885[1]/888-ir: rx read: 801741 ns mark > cx23885[1]/888-ir: rx read: 852852 ns space > cx23885[1]/888-ir: rx read: 775667 ns mark > cx23885[1]/888-ir: rx read: 852407 ns space > cx23885[1]/888-ir: rx read:1602926 ns mark > cx23885[1]/888-ir: rx read: 852407 ns space > cx23885[1]/888-ir: rx read: 801741 ns mark > cx23885[1]/888-ir: rx read: 852852 ns space > cx23885[1]/888-ir: rx read: 775074 ns mark > cx23885[1]/888-ir: rx read: 853148 ns space > cx23885[1]/888-ir: rx read: 801593 ns mark > cx23885[1]/888-ir: rx read: 852704 ns space > cx23885[1]/888-ir: rx read: 775667 ns mark > cx23885[1]/888-ir: rx read: 852556 ns space > cx23885[1]/888-ir: rx read: 801741 ns mark > cx23885[1]/888-ir: rx read: 852259 ns space > cx23885[1]/888-ir: rx read: 775963 ns mark > cx23885[1]/888-ir: rx read: end of rx > > That should be a press of '0' on the remote. > > 'end of rx' means the hardware measured a really long space. > > I also had the hardware low pass filter on. I think that would effect > the space measurements by making them shorter, if IR noise caused a > glitch. > > Note that many of the marks are a bit shorter than the ideal 889 us. In > fact the single marks from the grey remote seem to alternate between 775 > us and 802 us. The same happened here. The carrier doesn't seem to be precisely 36 kHz. The code on saa7134 has a way to adjust the time, plus a logic a timer to adjust the end of a RC5 code reception. It seems a good idea to allow adjusting those timers via sysfs. > I have attached a larger capture of (attempted) single presses of the > digits '0' through '9' and then an intentionally held down press of '7'. > > With a quick glance, I don't see pauses from the grey remote. Thanks for your dumps! It is not clear that the saa7134 is doing the right thing, and that the IR uses less bits than the standard. > > Regards, > Andy > -- Cheers, Mauro The ir-raw-event output for digit '0' is: [ 2803.106396] ir_raw_event_handle: event type 6, time before event: 000us [ 2803.106404] ir_raw_event_handle: event type 1, time before event: 919us [ 2803.106409] ir_raw_event_handle: event type 2, time before event: 868us [ 2803.106412] ir_raw_event_handle: event type 1, time before event: 893us [ 2803.106415] ir_raw_event_handle: event type 2, time before event: 869us [ 2803.106418] ir_raw_event_handle: event type 1, time before event: 921us [ 2803.106424] ir_raw_event_handle: event type 2, time before event: 869us [ 2803.106427] ir_raw_event_handle: event type 1, time before eve
Re: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
On Sun, 2010-04-04 at 15:00 -0300, Mauro Carvalho Chehab wrote: > Andy Walls wrote: > > And when you have time: > > A way to generate random IR > > glitches is with bright sunlight reflecting off of a basin of water > > that's surface is being disturbed to make waves. > > I have a better way: just let my IR sensor to be pointed to the fluorescent > lamp I have on my room... It produces _lots_ of glitches. :) > > Since a glitch filter is probably going to be needed by a number of > > drivers and since the minimum acceptable pulse depends slightly on the > > protocol, it probably makes sense for > > > > 1. A driver to indicate if its raw events need glitch filtering > > > > 2. A common glitch filtering library function that can be used by all > > decoders, and that also can accept a decoder specified minimum > > acceptable pulse width. > > Seems a nice improvement. I doubt I'll have time for handling it right now, > since there are still many things to do, but I'll put it on my todo list. > Of course, patches adding it are wellcome ;) :) OK. When I find time I'll hack something up as a prototype. > Btw, I added a RC-5 decoder there, at my IR experimental tree: > http://git.linuxtv.org/mchehab/ir.git I'll try to review it some time this week. Streaming state machine decoders do seem to be best way to go with these decoders. I have an RC-5 decoder in cx23885-input.c that isn't as clean as the NEC protocol decoder I developed. The cx23885-input.c RC-5 decoder is not a very explicit state machine however (it is a bit hack-ish). > Unfortunately, there's some problem with either my Remote Controller or > with the saa7134 driver. After 11 bits received, after the 2 start bits, > it receives a pause (see the enclosed sequence). -ENOATTACHMENT > I'm starting to suspect that the Hauppauge Grey IR produces a sequence with > shorter > bits, but, as the hardware decoders are capable or receiving IR codes, it may > also be a hardware problem. The fundamental unit in RC-5 is 32 cycles / 36 kHz = 89 ns ~= 889 us. I turned on the cx23888-ir.c debugging on the HVR-1850 and using a Hauppague grey remote (address 0x1e IIRC) and got this as just one example: cx23885[1]/888-ir: rx read: 802037 ns mark cx23885[1]/888-ir: rx read: 852704 ns space cx23885[1]/888-ir: rx read: 775370 ns mark cx23885[1]/888-ir: rx read: 852407 ns space cx23885[1]/888-ir: rx read: 802037 ns mark cx23885[1]/888-ir: rx read: 852852 ns space cx23885[1]/888-ir: rx read: 775667 ns mark cx23885[1]/888-ir: rx read: 852407 ns space cx23885[1]/888-ir: rx read: 801741 ns mark cx23885[1]/888-ir: rx read: 852852 ns space cx23885[1]/888-ir: rx read: 775667 ns mark cx23885[1]/888-ir: rx read: 852407 ns space cx23885[1]/888-ir: rx read:1602926 ns mark cx23885[1]/888-ir: rx read: 852407 ns space cx23885[1]/888-ir: rx read: 801741 ns mark cx23885[1]/888-ir: rx read: 852852 ns space cx23885[1]/888-ir: rx read: 775074 ns mark cx23885[1]/888-ir: rx read: 853148 ns space cx23885[1]/888-ir: rx read: 801593 ns mark cx23885[1]/888-ir: rx read: 852704 ns space cx23885[1]/888-ir: rx read: 775667 ns mark cx23885[1]/888-ir: rx read: 852556 ns space cx23885[1]/888-ir: rx read: 801741 ns mark cx23885[1]/888-ir: rx read: 852259 ns space cx23885[1]/888-ir: rx read: 775963 ns mark cx23885[1]/888-ir: rx read: end of rx That should be a press of '0' on the remote. 'end of rx' means the hardware measured a really long space. I also had the hardware low pass filter on. I think that would effect the space measurements by making them shorter, if IR noise caused a glitch. Note that many of the marks are a bit shorter than the ideal 889 us. In fact the single marks from the grey remote seem to alternate between 775 us and 802 us. I have attached a larger capture of (attempted) single presses of the digits '0' through '9' and then an intentionally held down press of '7'. With a quick glance, I don't see pauses from the grey remote. Regards, Andy hpg-grey-ir-pulses.txt.gz Description: GNU Zip compressed data
Re: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Andy Walls wrote: > On Sat, 2010-04-03 at 19:56 -0300, Mauro Carvalho Chehab wrote: >> Andy Walls wrote: >>> On Fri, 2010-04-02 at 22:32 -0300, Mauro Carvalho Chehab wrote: Andy Walls wrote: >>> > I haven't taken a very hard look at this since I'm very busy this month. > > It looks OK so far. Thank you for your review. One general comment: my main target of writing the NEC decoder is to have one decoder for testing. I know that there are several other implementations, so I didn't try to write a perfect decoder, but, instead, I wrote a code that works, and that allows me to continue the Remote Controller subsystem design. >>> Understood. >> Btw, I just finish rewriting the nec decoder: >> >> http://git.linuxtv.org/mchehab/ir.git?a=blob;f=drivers/media/IR/ir-nec-decoder.c;h=33b260f517f509efd9c55067eb89c8cd748ed12c;hb=09b1808271b3d705b839ee3239fd1c85b7289f41 >> >> I've got your constants and a few of your ideas, but the code is different: >> instead of getting a pulse/mark pair, the code handles event by event. Also, >> it is controlled by a state machine. >> >> The end result is that the code seems very reliable. It also handles both NEC >> and NEC extended. > > Looks good. > > On intervals that are supposed to be longer than 1 NEC_UNIT, the > tolerance of NEC_UNIT / 2 is a little unforgiving. However, if this > decoder is called not knowing apriori that the protocol is NEC, it is > probably the right thing to do. Yes. That's why the code is a little rigid. We want to reduce the risk of badly decoded codes. > > And when you have time: > > I think all that is missing is a glitch (low pass) filter to discard > pulses much shorter than 1 NEC_UNIT. A way to generate random IR > glitches is with bright sunlight reflecting off of a basin of water > that's surface is being disturbed to make waves. I have a better way: just let my IR sensor to be pointed to the fluorescent lamp I have on my room... It produces _lots_ of glitches. > LIRC has a software glitch filter implementation in > > lirc-0.8.5/drivers/lirc_serial/lirc_serial.c:frbwrite() > > but it's not the simplest code to understand and it keeps its state in > static variables in the function. > > (My kernel NEC decoder implementation didn't have a software glitch > filter, because there was a filter provided by the hardware. For NEC, I > decided to discard any pulse less than 5/8 * NEC_UNIT. For RC-5, I set > it to discard pulses less than 3/4 of a pulse time.) > > > Since a glitch filter is probably going to be needed by a number of > drivers and since the minimum acceptable pulse depends slightly on the > protocol, it probably makes sense for > > 1. A driver to indicate if its raw events need glitch filtering > > 2. A common glitch filtering library function that can be used by all > decoders, and that also can accept a decoder specified minimum > acceptable pulse width. Seems a nice improvement. I doubt I'll have time for handling it right now, since there are still many things to do, but I'll put it on my todo list. Of course, patches adding it are wellcome ;) Btw, I added a RC-5 decoder there, at my IR experimental tree: http://git.linuxtv.org/mchehab/ir.git Unfortunately, there's some problem with either my Remote Controller or with the saa7134 driver. After 11 bits received, after the 2 start bits, it receives a pause (see the enclosed sequence). I'm starting to suspect that the Hauppauge Grey IR produces a sequence with shorter bits, but, as the hardware decoders are capable or receiving IR codes, it may also be a hardware problem. > > >>> Is it the case that some drivers will only be able to perform leading >>> edge detection (measuring time between marks) or trailing edge detection >>> (measuring time between spaces)? >> In the specific case of saa7134, the IRQ can be enabled for a positive and/or >> for a negative edge. >> >> I'm not sure about the other IRQ driven hardware. On cx88, there's no IRQ >> code >> for IR - maybe it is not supported. I haven't check yet how IRQ's work on >> bttv. >> I've no idea about the other devices that support raw IR decoding. > > I can look into what the cx88 and bttv chips can do. How the boards are > wired up is a different issue. Thanks. Unfortunately, the cx88 devices I have here in hand have hardware IR decoders, or no IR support. I haven't check yet the bttv devices. Btw, by playing with one of them I got one that has a broken^Hincomplete decoding support: It validates the address internally, and returns only the command, if the address is the expected one. > > > I found one NEC IR here that uses the extended protocol. The issue I have here is that maybe it could be interesting to allow enable or disable a more pedantic check. At least on the room I'm working, I have two strong fluorescent lamps that interfere on the IR sensor of the saa7134 board
Re: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
On Sat, 2010-04-03 at 19:56 -0300, Mauro Carvalho Chehab wrote: > Andy Walls wrote: > > On Fri, 2010-04-02 at 22:32 -0300, Mauro Carvalho Chehab wrote: > >> Andy Walls wrote: > > > >>> I haven't taken a very hard look at this since I'm very busy this month. > >>> > >>> It looks OK so far. > >> Thank you for your review. > >> > >> One general comment: my main target of writing the NEC decoder is to have > >> one > >> decoder for testing. I know that there are several other implementations, > >> so I > >> didn't try to write a perfect decoder, but, instead, I wrote a code that > >> works, and that allows me to continue the Remote Controller subsystem > >> design. > > > > Understood. > > Btw, I just finish rewriting the nec decoder: > > http://git.linuxtv.org/mchehab/ir.git?a=blob;f=drivers/media/IR/ir-nec-decoder.c;h=33b260f517f509efd9c55067eb89c8cd748ed12c;hb=09b1808271b3d705b839ee3239fd1c85b7289f41 > > I've got your constants and a few of your ideas, but the code is different: > instead of getting a pulse/mark pair, the code handles event by event. Also, > it is controlled by a state machine. > > The end result is that the code seems very reliable. It also handles both NEC > and NEC extended. Looks good. On intervals that are supposed to be longer than 1 NEC_UNIT, the tolerance of NEC_UNIT / 2 is a little unforgiving. However, if this decoder is called not knowing apriori that the protocol is NEC, it is probably the right thing to do. And when you have time: I think all that is missing is a glitch (low pass) filter to discard pulses much shorter than 1 NEC_UNIT. A way to generate random IR glitches is with bright sunlight reflecting off of a basin of water that's surface is being disturbed to make waves. LIRC has a software glitch filter implementation in lirc-0.8.5/drivers/lirc_serial/lirc_serial.c:frbwrite() but it's not the simplest code to understand and it keeps its state in static variables in the function. (My kernel NEC decoder implementation didn't have a software glitch filter, because there was a filter provided by the hardware. For NEC, I decided to discard any pulse less than 5/8 * NEC_UNIT. For RC-5, I set it to discard pulses less than 3/4 of a pulse time.) Since a glitch filter is probably going to be needed by a number of drivers and since the minimum acceptable pulse depends slightly on the protocol, it probably makes sense for 1. A driver to indicate if its raw events need glitch filtering 2. A common glitch filtering library function that can be used by all decoders, and that also can accept a decoder specified minimum acceptable pulse width. > > Is it the case that some drivers will only be able to perform leading > > edge detection (measuring time between marks) or trailing edge detection > > (measuring time between spaces)? > > In the specific case of saa7134, the IRQ can be enabled for a positive and/or > for a negative edge. > > I'm not sure about the other IRQ driven hardware. On cx88, there's no IRQ code > for IR - maybe it is not supported. I haven't check yet how IRQ's work on > bttv. > I've no idea about the other devices that support raw IR decoding. I can look into what the cx88 and bttv chips can do. How the boards are wired up is a different issue. > >> I found one NEC IR here that uses the extended protocol. The issue I have > >> here is that > >> maybe it could be interesting to allow enable or disable a more pedantic > >> check. > >> At least on the room I'm working, I have two strong fluorescent lamps that > >> interfere > >> on the IR sensor of the saa7134 board. I'll probably add a sysfs node to > >> allow enable/ > >> disable the strict check for non-extended protocol. > > > > Would that make the setting global or would it be on a per remote > > control basis? > > The protocol sysfs nodes are per device. Yet, for now, I haven't created > such node. Per device means per IR receiver device? > > I don't know if a driver or end user can set the expectaion of a remote > > control's NEC address in your recent design (or if the intent was to not > > require it and use discovery). > > Well, bet to let it as-is. If later needed, it would be easy to add a sysfs > node parameter to control it. OK. 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: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Andy Walls wrote:
> On Fri, 2010-04-02 at 22:32 -0300, Mauro Carvalho Chehab wrote:
>> Andy Walls wrote:
>
>>> I haven't taken a very hard look at this since I'm very busy this month.
>>>
>>> It looks OK so far.
>> Thank you for your review.
>>
>> One general comment: my main target of writing the NEC decoder is to have one
>> decoder for testing. I know that there are several other implementations, so
>> I
>> didn't try to write a perfect decoder, but, instead, I wrote a code that
>> works, and that allows me to continue the Remote Controller subsystem design.
>
> Understood.
Btw, I just finish rewriting the nec decoder:
http://git.linuxtv.org/mchehab/ir.git?a=blob;f=drivers/media/IR/ir-nec-decoder.c;h=33b260f517f509efd9c55067eb89c8cd748ed12c;hb=09b1808271b3d705b839ee3239fd1c85b7289f41
I've got your constants and a few of your ideas, but the code is different:
instead of getting a pulse/mark pair, the code handles event by event. Also,
it is controlled by a state machine.
The end result is that the code seems very reliable. It also handles both NEC
and NEC extended.
(btw, I just noticed that I named the bit0/bit1 symbols wrong - the timings
there
are just for space - I'll write a patch fixing it).
>> I've changed on a latter patch my decoder to work with just the duration
>> of the bits.
>>
>> After reviewing the datasheet, now I think I know how to program IRQ to
>> trigger on both edges. So, my idea is to enable it and rewrite the decoder.
>
> So that brings up and interesting decoder design requirement.
>
> Is it the case that some drivers will only be able to perform leading
> edge detection (measuring time between marks) or trailing edge detection
> (measuring time between spaces)?
In the specific case of saa7134, the IRQ can be enabled for a positive and/or
for a negative edge.
I'm not sure about the other IRQ driven hardware. On cx88, there's no IRQ code
for IR - maybe it is not supported. I haven't check yet how IRQ's work on bttv.
I've no idea about the other devices that support raw IR decoding.
>> Yes, I know. By max/min, I've meant to handle delta variations around
>> the main time, since the driver may miss the exact moment where it were
>> supposed to collect the timestamp.
>
> Yes, and the remote's oscillator can be pretty far off for the ideal
> timing too.
I noticed.
>>> Both of the NEC remotes that I own use the extended protocol, IIRC.
>> I found one NEC IR here that uses the extended protocol. The issue I have
>> here is that
>> maybe it could be interesting to allow enable or disable a more pedantic
>> check.
>> At least on the room I'm working, I have two strong fluorescent lamps that
>> interfere
>> on the IR sensor of the saa7134 board. I'll probably add a sysfs node to
>> allow enable/
>> disable the strict check for non-extended protocol.
>
> Would that make the setting global or would it be on a per remote
> control basis?
The protocol sysfs nodes are per device. Yet, for now, I haven't created
such node.
> The way I handled extended NEC addressing or stardard NEC addressing was
> implicit given the specified expected remote control address.
>
> For setting the adress for which to watch I did something like
>
> if ((specified_address & 0xff00) == 0) {
> /* Store the 8 bit NEC address in 16 bits as A'A */
> ir_input->addr = (specified_address ^ 0xff) << 8 |
> specified_address;
> } else {
> /* Store a 16 bit Extended NEC address directly */
> ir_input->addr = specified_address;
> }
Seems ok to me. I've used the same concept on my code.
> And then when checking the incoming remote code:
>
> if (ir_input->addr != decoded_addr)
> return;
>
> The requirement for proper bit complement of an 8-bit address was
> encoded in the expected 16-bit address. So if what it decoded as an
> address didn't match the expected 16 bits, it discarded the code.
>
>
> I don't know if a driver or end user can set the expectaion of a remote
> control's NEC address in your recent design (or if the intent was to not
> require it and use discovery).
Well, bet to let it as-is. If later needed, it would be easy to add a sysfs
node parameter to control it.
--
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: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
On Fri, 2010-04-02 at 22:32 -0300, Mauro Carvalho Chehab wrote:
> Andy Walls wrote:
> > I haven't taken a very hard look at this since I'm very busy this month.
> >
> > It looks OK so far.
>
> Thank you for your review.
>
> One general comment: my main target of writing the NEC decoder is to have one
> decoder for testing. I know that there are several other implementations, so
> I
> didn't try to write a perfect decoder, but, instead, I wrote a code that
> works, and that allows me to continue the Remote Controller subsystem design.
Understood.
> >> +/* Start time: 4.5 ms */
> >> +#define MIN_START_TIME390
> >> +#define MAX_START_TIME510
> >
> > Hmmm.
> >
> > An NEC header pulse is nominally16 * 560 us = 8.96 ms
> > An NEC header space is nominally 8 * 560 us = 4.48 ms
> > An NEC repeat header space is nominally 4 * 560 us = 2.24 ms
> >
> > I think you need a more explicit name than {MIN,MAX}_START_TIME.
>
> Part of the problem with this decoder is that it was conceived to work with
> the saa7134 driver. The driver is currently programmed to trigger IRQ on just
> one of the edge (positive or negative, I need to double check). Due to that,
> this time is just half of the time it should be.
Hmm. It is the right time duration for the space in a normal,
non-repeat, header.
> I've changed on a latter patch my decoder to work with just the duration
> of the bits.
>
> After reviewing the datasheet, now I think I know how to program IRQ to
> trigger on both edges. So, my idea is to enable it and rewrite the decoder.
So that brings up and interesting decoder design requirement.
Is it the case that some drivers will only be able to perform leading
edge detection (measuring time between marks) or trailing edge detection
(measuring time between spaces)?
> >> +/* Pulse time: 560 us */
> >> +#define MIN_PULSE_TIME46
> >> +#define MAX_PULSE_TIME66
> >> +
> >> +/* Bit 1 space time: 2.25ms-560 us */
> >> +#define MIN_BIT1_TIME 149
> >> +#define MAX_BIT1_TIME 189
> >> +
> >> +/* Bit 0 space time: 1.12ms-560 us */
> >> +#define MIN_BIT0_TIME 36
> >> +#define MAX_BIT0_TIME 76
> >> +
> >
> > The fundamental unit of time in the NEC protocol is ideally:
> >
> > 4192/197 cycles / 38 kHz = 559978.6 ns ~= 560 ns
> >
> > All other time durations in the NEC protocol are multiples of this unit.
>
> Yes, I know. By max/min, I've meant to handle delta variations around
> the main time, since the driver may miss the exact moment where it were
> supposed to collect the timestamp.
Yes, and the remote's oscillator can be pretty far off for the ideal
timing too.
> > Both of the NEC remotes that I own use the extended protocol, IIRC.
>
> I found one NEC IR here that uses the extended protocol. The issue I have
> here is that
> maybe it could be interesting to allow enable or disable a more pedantic
> check.
> At least on the room I'm working, I have two strong fluorescent lamps that
> interfere
> on the IR sensor of the saa7134 board. I'll probably add a sysfs node to
> allow enable/
> disable the strict check for non-extended protocol.
Would that make the setting global or would it be on a per remote
control basis?
The way I handled extended NEC addressing or stardard NEC addressing was
implicit given the specified expected remote control address.
For setting the adress for which to watch I did something like
if ((specified_address & 0xff00) == 0) {
/* Store the 8 bit NEC address in 16 bits as A'A */
ir_input->addr = (specified_address ^ 0xff) << 8 |
specified_address;
} else {
/* Store a 16 bit Extended NEC address directly */
ir_input->addr = specified_address;
}
And then when checking the incoming remote code:
if (ir_input->addr != decoded_addr)
return;
The requirement for proper bit complement of an 8-bit address was
encoded in the expected 16-bit address. So if what it decoded as an
address didn't match the expected 16 bits, it discarded the code.
I don't know if a driver or end user can set the expectaion of a remote
control's NEC address in your recent design (or if the intent was to not
require it and use discovery).
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: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Andy Walls wrote:
> On Thu, 2010-04-01 at 14:56 -0300, Mauro Carvalho Chehab wrote:
>> Adds a method to pass IR raw pulse/code events into ir-core. This is
>> needed in order to support LIRC. It also helps to move common code
>> from the drivers into the core.
>>
>> In order to allow testing, it implements a simple NEC protocol decoder
>> at ir-nec-decoder.c file. The logic is about the same used at saa7134
>> driver that handles Avermedia M135A and Encore FM53 boards.
>>
>> Signed-off-by: Mauro Carvalho Chehab
>>
>> create mode 100644 drivers/media/IR/ir-nec-decoder.c
>> create mode 100644 drivers/media/IR/ir-raw-event.c
>
> Hi Mauro,
>
> I haven't taken a very hard look at this since I'm very busy this month.
>
> It looks OK so far.
Thank you for your review.
One general comment: my main target of writing the NEC decoder is to have one
decoder for testing. I know that there are several other implementations, so I
didn't try to write a perfect decoder, but, instead, I wrote a code that
works, and that allows me to continue the Remote Controller subsystem design.
In other words, for sure there are lots of space for improvements there ;)
> I do have some comments
>
>
>> diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
>> index 171890e..18794c7 100644
>> --- a/drivers/media/IR/Makefile
>> +++ b/drivers/media/IR/Makefile
>> @@ -1,5 +1,5 @@
>> ir-common-objs := ir-functions.o ir-keymaps.o
>> -ir-core-objs:= ir-keytable.o ir-sysfs.o
>> +ir-core-objs:= ir-keytable.o ir-sysfs.o ir-raw-event.o
>> ir-nec-decoder.o
>>
>> obj-$(CONFIG_IR_CORE) += ir-core.o
>> obj-$(CONFIG_VIDEO_IR) += ir-common.o
>> diff --git a/drivers/media/IR/ir-nec-decoder.c
>> b/drivers/media/IR/ir-nec-decoder.c
>> new file mode 100644
>> index 000..16360eb
>> --- /dev/null
>> +++ b/drivers/media/IR/ir-nec-decoder.c
>> @@ -0,0 +1,131 @@
>> +/* ir-raw-event.c - handle IR Pulse/Space event
>> + *
>> + * Copyright (C) 2010 by Mauro Carvalho Chehab
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include
>> +
>> +/* Start time: 4.5 ms */
>> +#define MIN_START_TIME 390
>> +#define MAX_START_TIME 510
>
> Hmmm.
>
> An NEC header pulse is nominally16 * 560 us = 8.96 ms
> An NEC header space is nominally 8 * 560 us = 4.48 ms
> An NEC repeat header space is nominally 4 * 560 us = 2.24 ms
>
> I think you need a more explicit name than {MIN,MAX}_START_TIME.
Part of the problem with this decoder is that it was conceived to work with
the saa7134 driver. The driver is currently programmed to trigger IRQ on just
one of the edge (positive or negative, I need to double check). Due to that,
this time is just half of the time it should be.
I've changed on a latter patch my decoder to work with just the duration
of the bits.
After reviewing the datasheet, now I think I know how to program IRQ to
trigger on both edges. So, my idea is to enable it and rewrite the decoder.
>
>
>> +/* Pulse time: 560 us */
>> +#define MIN_PULSE_TIME 46
>> +#define MAX_PULSE_TIME 66
>> +
>> +/* Bit 1 space time: 2.25ms-560 us */
>> +#define MIN_BIT1_TIME 149
>> +#define MAX_BIT1_TIME 189
>> +
>> +/* Bit 0 space time: 1.12ms-560 us */
>> +#define MIN_BIT0_TIME 36
>> +#define MAX_BIT0_TIME 76
>> +
>
> The fundamental unit of time in the NEC protocol is ideally:
>
> 4192/197 cycles / 38 kHz = 559978.6 ns ~= 560 ns
>
> All other time durations in the NEC protocol are multiples of this unit.
Yes, I know. By max/min, I've meant to handle delta variations around
the main time, since the driver may miss the exact moment where it were
supposed to collect the timestamp.
> See:
>
> http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2#l1.96
>
> If you define the your above constants in terms of that time unit, it
> makes the tolerances you added in explicitly visible when reading the
> source.
I'll take a look on your code and work to improve this decoder. The way you've
declared is for sure cleaner than mine.
>> +/** Decode NEC pulsecode. This code can take up to 76.5 ms to run.
>> +Unfortunately, using IRQ to decode pulse didn't work, since it uses
>> +a pulse train of 38KHz. This means one pulse on each 52 us
>> +*/
>> +
>> +int ir_nec_decode(struct input_dev *input_dev,
>> + struct ir_raw_event *evs,
>> + int len)
>> +{
>> +int i, count = -1;
>> +int ircode = 0, not_code = 0;
>> +#if 0
>> +/* Needed
Re: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Andy Walls wrote:
> On Fri, 2010-04-02 at 19:39 -0400, Andy Walls wrote:
>> On Thu, 2010-04-01 at 14:56 -0300, Mauro Carvalho Chehab wrote:
>
>>> +enum raw_event_type {
>>> + IR_SPACE= (1 << 0),
>>> + IR_PULSE= (1 << 1),
>>> + IR_START_EVENT = (1 << 2),
>>> + IR_STOP_EVENT = (1 << 3),
>>> +};
>>> +
>> Why are these events encoded as bit flags? Shouldn't they all be
>> orthogonal?
> ^^
> Argh, wrong word.
Why is it wrong? It seems appropriate to me.
>
> Shouldn't they all be mutually exclusive?
space x pulse are mutually exclusive, and start x stop are also
mutually exclusive, but you may have several possible combinations
for an event. The hole set of possibilities are:
IR_SPACE
IR_PULSE
IR_SPACE | IR_START_EVENT
IR_SPACE | IR_STOP_EVENT
IR_PULSE | IR_START_EVENT
IR_PULSE | IR_STOP_EVENT
With bit flags, it is possible to cover all the above combinations.
In a matter of fact, the driver is currently not using the stop events.
--
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: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
On Fri, 2010-04-02 at 19:39 -0400, Andy Walls wrote:
> On Thu, 2010-04-01 at 14:56 -0300, Mauro Carvalho Chehab wrote:
> > +enum raw_event_type {
> > + IR_SPACE= (1 << 0),
> > + IR_PULSE= (1 << 1),
> > + IR_START_EVENT = (1 << 2),
> > + IR_STOP_EVENT = (1 << 3),
> > +};
> > +
>
> Why are these events encoded as bit flags? Shouldn't they all be
> orthogonal?
^^
Argh, wrong word.
Shouldn't they all be mutually exclusive?
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: [PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
On Thu, 2010-04-01 at 14:56 -0300, Mauro Carvalho Chehab wrote:
> Adds a method to pass IR raw pulse/code events into ir-core. This is
> needed in order to support LIRC. It also helps to move common code
> from the drivers into the core.
>
> In order to allow testing, it implements a simple NEC protocol decoder
> at ir-nec-decoder.c file. The logic is about the same used at saa7134
> driver that handles Avermedia M135A and Encore FM53 boards.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> create mode 100644 drivers/media/IR/ir-nec-decoder.c
> create mode 100644 drivers/media/IR/ir-raw-event.c
Hi Mauro,
I haven't taken a very hard look at this since I'm very busy this month.
It looks OK so far. I do have some comments
> diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
> index 171890e..18794c7 100644
> --- a/drivers/media/IR/Makefile
> +++ b/drivers/media/IR/Makefile
> @@ -1,5 +1,5 @@
> ir-common-objs := ir-functions.o ir-keymaps.o
> -ir-core-objs := ir-keytable.o ir-sysfs.o
> +ir-core-objs := ir-keytable.o ir-sysfs.o ir-raw-event.o ir-nec-decoder.o
>
> obj-$(CONFIG_IR_CORE) += ir-core.o
> obj-$(CONFIG_VIDEO_IR) += ir-common.o
> diff --git a/drivers/media/IR/ir-nec-decoder.c
> b/drivers/media/IR/ir-nec-decoder.c
> new file mode 100644
> index 000..16360eb
> --- /dev/null
> +++ b/drivers/media/IR/ir-nec-decoder.c
> @@ -0,0 +1,131 @@
> +/* ir-raw-event.c - handle IR Pulse/Space event
> + *
> + * Copyright (C) 2010 by Mauro Carvalho Chehab
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +
> +/* Start time: 4.5 ms */
> +#define MIN_START_TIME 390
> +#define MAX_START_TIME 510
Hmmm.
An NEC header pulse is nominally16 * 560 us = 8.96 ms
An NEC header space is nominally 8 * 560 us = 4.48 ms
An NEC repeat header space is nominally 4 * 560 us = 2.24 ms
I think you need a more explicit name than {MIN,MAX}_START_TIME.
> +/* Pulse time: 560 us */
> +#define MIN_PULSE_TIME 46
> +#define MAX_PULSE_TIME 66
> +
> +/* Bit 1 space time: 2.25ms-560 us */
> +#define MIN_BIT1_TIME149
> +#define MAX_BIT1_TIME189
> +
> +/* Bit 0 space time: 1.12ms-560 us */
> +#define MIN_BIT0_TIME36
> +#define MAX_BIT0_TIME76
> +
The fundamental unit of time in the NEC protocol is ideally:
4192/197 cycles / 38 kHz = 559978.6 ns ~= 560 ns
All other time durations in the NEC protocol are multiples of this unit.
See:
http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2#l1.96
If you define the your above constants in terms of that time unit, it
makes the tolerances you added in explicitly visible when reading the
source.
> +/** Decode NEC pulsecode. This code can take up to 76.5 ms to run.
> + Unfortunately, using IRQ to decode pulse didn't work, since it uses
> + a pulse train of 38KHz. This means one pulse on each 52 us
> +*/
> +
> +int ir_nec_decode(struct input_dev *input_dev,
> + struct ir_raw_event *evs,
> + int len)
> +{
> + int i, count = -1;
> + int ircode = 0, not_code = 0;
> +#if 0
> + /* Needed only after porting the event code to the decoder */
> + struct ir_input_dev *ir = input_get_drvdata(input_dev);
> +#endif
> +
> + /* Be sure that the first event is an start one and is a pulse */
> + for (i = 0; i < len; i++) {
> + if (evs[i].type & (IR_START_EVENT | IR_PULSE))
> + break;
> + }
> + i++;/* First event doesn't contain data */
> +
> + if (i >= len)
> + return 0;
> +
> + /* First space should have 4.5 ms otherwise is not NEC protocol */
> + if ((evs[i].delta.tv_nsec < MIN_START_TIME) |
> + (evs[i].delta.tv_nsec > MAX_START_TIME) |
> + (evs[i].type != IR_SPACE))
> + goto err;
> +
> + /*
> + * FIXME: need to implement the repeat sequence
> + */
I have an NEC protocol decoder here:
http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2
If you would find it useful, please feel free to borrow ideas or parts
of the code to implement any features you are missing. (That code works
by converting a mark-space pair to an "nec_symbol", and then taking
action based on the symbol.)
I suspect you will want to implement the repeat sequence. It is hard
not to get a repeat sequence from a remote.
NEC ideally sends a repeat at intervals of:
4192 cycles * 38 kHz = 110.316 ms
> + count = 0;
> + for (i++; i < len;
[PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Adds a method to pass IR raw pulse/code events into ir-core. This is
needed in order to support LIRC. It also helps to move common code
from the drivers into the core.
In order to allow testing, it implements a simple NEC protocol decoder
at ir-nec-decoder.c file. The logic is about the same used at saa7134
driver that handles Avermedia M135A and Encore FM53 boards.
Signed-off-by: Mauro Carvalho Chehab
create mode 100644 drivers/media/IR/ir-nec-decoder.c
create mode 100644 drivers/media/IR/ir-raw-event.c
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index 171890e..18794c7 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -1,5 +1,5 @@
ir-common-objs := ir-functions.o ir-keymaps.o
-ir-core-objs := ir-keytable.o ir-sysfs.o
+ir-core-objs := ir-keytable.o ir-sysfs.o ir-raw-event.o ir-nec-decoder.o
obj-$(CONFIG_IR_CORE) += ir-core.o
obj-$(CONFIG_VIDEO_IR) += ir-common.o
diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
new file mode 100644
index 000..16360eb
--- /dev/null
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -0,0 +1,131 @@
+/* ir-raw-event.c - handle IR Pulse/Space event
+ *
+ * Copyright (C) 2010 by Mauro Carvalho Chehab
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+
+/* Start time: 4.5 ms */
+#define MIN_START_TIME 390
+#define MAX_START_TIME 510
+
+/* Pulse time: 560 us */
+#define MIN_PULSE_TIME 46
+#define MAX_PULSE_TIME 66
+
+/* Bit 1 space time: 2.25ms-560 us */
+#define MIN_BIT1_TIME 149
+#define MAX_BIT1_TIME 189
+
+/* Bit 0 space time: 1.12ms-560 us */
+#define MIN_BIT0_TIME 36
+#define MAX_BIT0_TIME 76
+
+
+/** Decode NEC pulsecode. This code can take up to 76.5 ms to run.
+ Unfortunately, using IRQ to decode pulse didn't work, since it uses
+ a pulse train of 38KHz. This means one pulse on each 52 us
+*/
+
+int ir_nec_decode(struct input_dev *input_dev,
+ struct ir_raw_event *evs,
+ int len)
+{
+ int i, count = -1;
+ int ircode = 0, not_code = 0;
+#if 0
+ /* Needed only after porting the event code to the decoder */
+ struct ir_input_dev *ir = input_get_drvdata(input_dev);
+#endif
+
+ /* Be sure that the first event is an start one and is a pulse */
+ for (i = 0; i < len; i++) {
+ if (evs[i].type & (IR_START_EVENT | IR_PULSE))
+ break;
+ }
+ i++;/* First event doesn't contain data */
+
+ if (i >= len)
+ return 0;
+
+ /* First space should have 4.5 ms otherwise is not NEC protocol */
+ if ((evs[i].delta.tv_nsec < MIN_START_TIME) |
+ (evs[i].delta.tv_nsec > MAX_START_TIME) |
+ (evs[i].type != IR_SPACE))
+ goto err;
+
+ /*
+* FIXME: need to implement the repeat sequence
+*/
+
+ count = 0;
+ for (i++; i < len; i++) {
+ int bit;
+
+ if ((evs[i].delta.tv_nsec < MIN_PULSE_TIME) |
+ (evs[i].delta.tv_nsec > MAX_PULSE_TIME) |
+ (evs[i].type != IR_PULSE))
+ goto err;
+
+ if (++i >= len)
+ goto err;
+ if (evs[i].type != IR_SPACE)
+ goto err;
+
+ if ((evs[i].delta.tv_nsec > MIN_BIT1_TIME) &&
+ (evs[i].delta.tv_nsec < MAX_BIT1_TIME))
+ bit = 1;
+ else if ((evs[i].delta.tv_nsec > MIN_BIT0_TIME) &&
+(evs[i].delta.tv_nsec < MAX_BIT0_TIME))
+ bit = 0;
+ else
+ goto err;
+
+ if (bit) {
+ int shift = count;
+ /* Address first, then command */
+ if (shift < 8) {
+ shift += 8;
+ ircode |= 1 << shift;
+ } else if (shift < 16) {
+ not_code |= 1 << shift;
+ } else if (shift < 24) {
+ shift -= 16;
+ ircode |= 1 << shift;
+ } else {
+ shift -= 24;
+ not_code |= 1 << shift;
+ }
+ }
+ if (++count == 32)
+ break;
+ }
+
+ /*
+* Fixme: may need to accept Extended NEC protocol?
+*
