ir-core multi-protocol decode and mceusb

ir-core multi-protocol decode and mceusb

[Jarod Wilson]

So I'm inching closer to a viable mceusb driver submission -- both a
first-gen and a third-gen transceiver are now working perfectly with
multiple different mce remotes. However, that's only when I make sure
the mceusb driver is loaded w/only the rc6 decoder loaded. When
ir-core comes up, it requests all decoders to load, starting with the
nec decoder, followed by the rc5 decoder, then the rc6 decoder and so
on (init_decoders() in ir-raw-event.c). When I call
ir_raw_event_handle, all decoders get run on the ir data buffer,
starting with nec. Well, the nec decoder doesn't like the rc6 data, so
it pukes. The RUN_DECODER macro break's out of the routine when that
happens, and the rc6 decoder never gets a chance to run. (Similarly,
if only ir-nec-decoder has been removed, the rc5 decoder pukes on the
rc6 data, same problem). If I'm thinking clearly, rather than breaking
out of the loop in RUN_DECODER, we really ought to be issuing a
continue to go on to the next decoder, and possibly be accumulating
failures, though I don't know that _sumrc actually matters other than
"greater than zero" (i.e., at least one decoder was successfully able
to decode the signal). If I'm not thinking clearly, a pointer to what
I'm missing would be appreciated. :)

The next fun thing I'm starting to look at is this MCE IR
keyboard/mouse combo device I've got here[1]... It uses a Philips RC
variant (I suspect RC-MM at first glance[2]) for the mouse
functionality and most of the keyboard keys (except many of the
multimedia keys, which are RC6 like their matching counterparts on the
MCE remotes). There's a hack atop an old version of lirc_mceusb out
there that supports this thing[3], which actually has some pretty
decent looking documentation on its site (though the code itself is...
well, its hacked in atop an old lirc driver...).

Support for the keyboard can wait though. I think the issue
w/RUN_DECODER is really all that still needs to be resolved for an
initial submission.

[1] http://www.amazon.com/Microsoft-Remote-Keyboard-Windows-ZV1-00004/dp/B000AOAAN8
[2] http://www.sbprojects.com/knowledge/ir/rcmm.htm
[3] http://mod-mce.sourceforge.net/

-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Jarod Wilson]

On Fri, May 28, 2010 at 12:47 AM, Jarod Wilson <jarod@wilsonet.com> wrote:
> So I'm inching closer to a viable mceusb driver submission -- both a
> first-gen and a third-gen transceiver are now working perfectly with
> multiple different mce remotes. However, that's only when I make sure
> the mceusb driver is loaded w/only the rc6 decoder loaded. When
> ir-core comes up, it requests all decoders to load, starting with the
> nec decoder, followed by the rc5 decoder, then the rc6 decoder and so
> on (init_decoders() in ir-raw-event.c). When I call
> ir_raw_event_handle, all decoders get run on the ir data buffer,
> starting with nec. Well, the nec decoder doesn't like the rc6 data, so
> it pukes. The RUN_DECODER macro break's out of the routine when that
> happens, and the rc6 decoder never gets a chance to run. (Similarly,
> if only ir-nec-decoder has been removed, the rc5 decoder pukes on the
> rc6 data, same problem).

A quasi-related problem: I seem to be unable to ever get the module
refcount on ir-core down to zero to be able to unload it. Seems
sometimes the ir-foo-decoder modules' unload doesn't reduce the
refcount, but even if I remove drivers first, then keymaps, then
decoders, then ir-common, I'm left w/ir-core having a refcount of 1.
Maybe I don't have the right ordering or something, but I suspect
something is slightly wonky here, shouldn't be that hard to unload
ir-core.

> If I'm thinking clearly, rather than breaking
> out of the loop in RUN_DECODER, we really ought to be issuing a
> continue to go on to the next decoder, and possibly be accumulating
> failures, though I don't know that _sumrc actually matters other than
> "greater than zero" (i.e., at least one decoder was successfully able
> to decode the signal). If I'm not thinking clearly, a pointer to what
> I'm missing would be appreciated. :)

I've tested this out, it works. I've got a 4-patch series that
includes this that I'll submit for review shortly...

-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Andy Walls]

On Fri, 2010-05-28 at 00:47 -0400, Jarod Wilson wrote:
> So I'm inching closer to a viable mceusb driver submission -- both a
> first-gen and a third-gen transceiver are now working perfectly with
> multiple different mce remotes. However, that's only when I make sure
> the mceusb driver is loaded w/only the rc6 decoder loaded. When
> ir-core comes up, it requests all decoders to load, starting with the
> nec decoder, followed by the rc5 decoder, then the rc6 decoder and so
> on (init_decoders() in ir-raw-event.c). When I call
> ir_raw_event_handle, all decoders get run on the ir data buffer,
> starting with nec. Well, the nec decoder doesn't like the rc6 data, so
> it pukes. The RUN_DECODER macro break's out of the routine when that
> happens, and the rc6 decoder never gets a chance to run. (Similarly,
> if only ir-nec-decoder has been removed, the rc5 decoder pukes on the
> rc6 data, same problem).

Yes, if the system kernel is going to attempt to discriminate between
various input singals, it needs to let all its "correlators" run and
produce a "confidence" number from each.

Then ideally one would take the result with the highest confidence.

Right now it looks like all the confidence determinations are boolean (0
or -EINVAL) and there is no chance to deal with the case that two
different decoders validly decode something.  The first decoder that
declares a match "wins" and sends an event.



>  If I'm thinking clearly, rather than breaking
> out of the loop in RUN_DECODER, we really ought to be issuing a
> continue to go on to the next decoder, and possibly be accumulating
> failures, though I don't know that _sumrc actually matters other than
> "greater than zero" (i.e., at least one decoder was successfully able
> to decode the signal). If I'm not thinking clearly, a pointer to what
> I'm missing would be appreciated. :)

You will have to deal with the case that two or more decoders may match
and each sends an IR event.  (Unless the ir-core already deals with this
somehow...)

Regards,
Andy

Re: ir-core multi-protocol decode and mceusb

[Jarod Wilson]

On Sat, May 29, 2010 at 8:39 AM, Andy Walls <awalls@md.metrocast.net> wrote:
> On Fri, 2010-05-28 at 00:47 -0400, Jarod Wilson wrote:
>> So I'm inching closer to a viable mceusb driver submission -- both a
>> first-gen and a third-gen transceiver are now working perfectly with
>> multiple different mce remotes. However, that's only when I make sure
>> the mceusb driver is loaded w/only the rc6 decoder loaded. When
>> ir-core comes up, it requests all decoders to load, starting with the
>> nec decoder, followed by the rc5 decoder, then the rc6 decoder and so
>> on (init_decoders() in ir-raw-event.c). When I call
>> ir_raw_event_handle, all decoders get run on the ir data buffer,
>> starting with nec. Well, the nec decoder doesn't like the rc6 data, so
>> it pukes. The RUN_DECODER macro break's out of the routine when that
>> happens, and the rc6 decoder never gets a chance to run. (Similarly,
>> if only ir-nec-decoder has been removed, the rc5 decoder pukes on the
>> rc6 data, same problem).
>
> Yes, if the system kernel is going to attempt to discriminate between
> various input singals, it needs to let all its "correlators" run and
> produce a "confidence" number from each.
>
> Then ideally one would take the result with the highest confidence.
>
> Right now it looks like all the confidence determinations are boolean (0
> or -EINVAL) and there is no chance to deal with the case that two
> different decoders validly decode something.  The first decoder that
> declares a match "wins" and sends an event.

Yeah, it does look that way. I wonder how likely it is that e.g. a
valid RC6 signal would be decoded to something by say the NEC decoder,
with a resulting value that matched an entry in the (RC6) keymap
loaded for the remote... Certainly seems like something that *could*
happen somehow, but probably unlikely? I dunno... We do have the
option to disable all but the relevant protocol handler on a
per-device basis though, if that's a problem. Hrm, the key tables also
have a protocol tied to them, not sure if that's taken into account
when doing matching... Still getting to know the code. :)

>>  If I'm thinking clearly, rather than breaking
>> out of the loop in RUN_DECODER, we really ought to be issuing a
>> continue to go on to the next decoder, and possibly be accumulating
>> failures, though I don't know that _sumrc actually matters other than
>> "greater than zero" (i.e., at least one decoder was successfully able
>> to decode the signal). If I'm not thinking clearly, a pointer to what
>> I'm missing would be appreciated. :)
>
> You will have to deal with the case that two or more decoders may match
> and each sends an IR event.  (Unless the ir-core already deals with this
> somehow...)

Well, its gotta decode correctly to a value, and then match a value in
the loaded key table for an input event to get sent through. At least
for the RC6 MCE remotes, I haven't seen any of the other decoders take
the signal and interpret it as valid -- which ought to be by design,
if you consider that people use several different remotes with varying
ir signals with different devices all receiving them all the time
without problems (usually). And if we're not already, we could likely
add some logic to give higher precedence to values arrived at using
the protocol decoder that matches the key table we've got loaded for a
given device.

In looking closer at what we're doing now with RUN_DECODER(), it seems
only __ir_input_register() actually cares about the retval, and
unfortunately, it seems to be checking for a ret < 0 to indicate
failure, which isn't possible -- its _sumrc that gets returned, and
its initialized to 0, then only ever adjusted if _rc >= 0, so we'll
never see it as a failure in __ir_input_register().

Given that bit of data, I'd say the patch I submitted yesterday isn't
quite correct. I think RUN_DECODER should actually be adding _rc to
_sumrc in all cases -- instead of an if/else on _rc < 0, just always
do the _sumrc += _rc and return _sumrc, which will either be 0 or some
negative value. I don't think we need to worry at all about the retval
we get from the decode functions though -- either we get one or more
decoded values to match against our key table or we don't...

-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Andy Walls]

On Sat, 2010-05-29 at 12:58 -0400, Jarod Wilson wrote:
> On Sat, May 29, 2010 at 8:39 AM, Andy Walls <awalls@md.metrocast.net> wrote:
> > On Fri, 2010-05-28 at 00:47 -0400, Jarod Wilson wrote:
> >> So I'm inching closer to a viable mceusb driver submission -- both a
> >> first-gen and a third-gen transceiver are now working perfectly with
> >> multiple different mce remotes. However, that's only when I make sure
> >> the mceusb driver is loaded w/only the rc6 decoder loaded. When
> >> ir-core comes up, it requests all decoders to load, starting with the
> >> nec decoder, followed by the rc5 decoder, then the rc6 decoder and so
> >> on (init_decoders() in ir-raw-event.c). When I call
> >> ir_raw_event_handle, all decoders get run on the ir data buffer,
> >> starting with nec. Well, the nec decoder doesn't like the rc6 data, so
> >> it pukes. The RUN_DECODER macro break's out of the routine when that
> >> happens, and the rc6 decoder never gets a chance to run. (Similarly,
> >> if only ir-nec-decoder has been removed, the rc5 decoder pukes on the
> >> rc6 data, same problem).
> >
> > Yes, if the system kernel is going to attempt to discriminate between
> > various input singals, it needs to let all its "correlators" run and
> > produce a "confidence" number from each.
> >
> > Then ideally one would take the result with the highest confidence.
> >
> > Right now it looks like all the confidence determinations are boolean (0
> > or -EINVAL) and there is no chance to deal with the case that two
> > different decoders validly decode something.  The first decoder that
> > declares a match "wins" and sends an event.
> 
> Yeah, it does look that way. I wonder how likely it is that e.g. a
> valid RC6 signal would be decoded to something by say the NEC decoder,

NEC is a pulse position code and RC-6 is manchester encoded, so that
particular case would be unlikely.

I would think one would have a better chance of false positiive
detections between similar encoding types: pulse position, pulse width,
or manchester.

Looking at slide 11 of this:

	http://www.audiodevelopers.com/temp/Remote_Controls.ppt

It looks like the pulse position protocols with a header space of 8T
(where 8T is about 4ms) would be the only ones that could get confused.

Since these are streaming decoders, it looks like JVC would come up with
false detections first, since it has the shortest payload of the pulse
position protocols.  I think JVC will always claim to decode an NEC
pulse train.  (I'll try to test that sometime.) 


> with a resulting value that matched an entry in the (RC6) keymap
> loaded for the remote... Certainly seems like something that *could*
> happen somehow, but probably unlikely? I dunno...

I don't know either.  There appears to be a chance for the first 16 bits
of a transmitted NEC (Addr:Addr') or Extended NEC (AddrHi:AddrLo)
sequence, to be interpreted as JVC (Addr:Cmd), and the JVC decoder
matching a scancode in the keytable for the NEC remote.

		


>  We do have the
> option to disable all but the relevant protocol handler on a
> per-device basis though, if that's a problem. Hrm, the key tables also
> have a protocol tied to them, not sure if that's taken into account
> when doing matching... Still getting to know the code. :)

It does not look like

	ir_keydown()
		ir_g_keycode_from_table()
			ir_getkeycode()

bother to check the ir_type (e.g. IR_TYPE_NEC) of the keymap against the
decoders type.  Neither do the decoders themselves.


If a decoder decodes something and thinks its valid, it tries to send a
key event with ir_keydown().  ir_keydown() won't send a key event if the
lookup comes back KEY_RESERVED, but it doesn't tell the decoder about
the failure to find a key mapping.  A decoder can come back saying it
did it's job, without knowing whether or not the decoding corresponded
to a valid key in the loaded keymap. :(


> > You will have to deal with the case that two or more decoders may match
> > and each sends an IR event.  (Unless the ir-core already deals with this
> > somehow...)
> 
> Well, its gotta decode correctly to a value, and then match a value in
> the loaded key table for an input event to get sent through. At least
> for the RC6 MCE remotes, I haven't seen any of the other decoders take
> the signal and interpret it as valid -- which ought to be by design,
> if you consider that people use several different remotes with varying
> ir signals with different devices all receiving them all the time
> without problems (usually). And if we're not already, we could likely
> add some logic to give higher precedence to values arrived at using
> the protocol decoder that matches the key table we've got loaded for a
> given device.

After looking at things, the only potential problem I can see right now
is with the JVC decoder and NEC remotes.

I think that problem is most easily eliminated either by

a. having ir_keydown() (or the functions it calls) check to see that the
decoder matches the loaded keymap, or

b. only calling the decoder that matches the loaded keymap's protocol

Of the above, b. saves processor cycles and frees up the global
ir_raw_handler spin lock sooner.  That spin lock is serializing pulse
decoding for all the IR receivers in the system  (pulse decoding can
still be interleaved, just only one IR receiver's pulses are be
processed at any time).  What's the point of running decoders that
should never match the loaded keymap?

Regards,
Andy

Re: ir-core multi-protocol decode and mceusb

[Jarod Wilson]

On Sat, May 29, 2010 at 4:01 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> On Sat, 2010-05-29 at 12:58 -0400, Jarod Wilson wrote:
>> On Sat, May 29, 2010 at 8:39 AM, Andy Walls <awalls@md.metrocast.net> wrote:
>> > On Fri, 2010-05-28 at 00:47 -0400, Jarod Wilson wrote:
>> >> So I'm inching closer to a viable mceusb driver submission -- both a
>> >> first-gen and a third-gen transceiver are now working perfectly with
>> >> multiple different mce remotes. However, that's only when I make sure
>> >> the mceusb driver is loaded w/only the rc6 decoder loaded. When
>> >> ir-core comes up, it requests all decoders to load, starting with the
>> >> nec decoder, followed by the rc5 decoder, then the rc6 decoder and so
>> >> on (init_decoders() in ir-raw-event.c). When I call
>> >> ir_raw_event_handle, all decoders get run on the ir data buffer,
>> >> starting with nec. Well, the nec decoder doesn't like the rc6 data, so
>> >> it pukes. The RUN_DECODER macro break's out of the routine when that
>> >> happens, and the rc6 decoder never gets a chance to run. (Similarly,
>> >> if only ir-nec-decoder has been removed, the rc5 decoder pukes on the
>> >> rc6 data, same problem).
>> >
>> > Yes, if the system kernel is going to attempt to discriminate between
>> > various input singals, it needs to let all its "correlators" run and
>> > produce a "confidence" number from each.
>> >
>> > Then ideally one would take the result with the highest confidence.
>> >
>> > Right now it looks like all the confidence determinations are boolean (0
>> > or -EINVAL) and there is no chance to deal with the case that two
>> > different decoders validly decode something.  The first decoder that
>> > declares a match "wins" and sends an event.
>>
>> Yeah, it does look that way. I wonder how likely it is that e.g. a
>> valid RC6 signal would be decoded to something by say the NEC decoder,
>
> NEC is a pulse position code and RC-6 is manchester encoded, so that
> particular case would be unlikely.
>
> I would think one would have a better chance of false positiive
> detections between similar encoding types: pulse position, pulse width,
> or manchester.
>
> Looking at slide 11 of this:
>
>        http://www.audiodevelopers.com/temp/Remote_Controls.ppt
>
> It looks like the pulse position protocols with a header space of 8T
> (where 8T is about 4ms) would be the only ones that could get confused.
>
> Since these are streaming decoders, it looks like JVC would come up with
> false detections first, since it has the shortest payload of the pulse
> position protocols.  I think JVC will always claim to decode an NEC
> pulse train.  (I'll try to test that sometime.)
>
>
>> with a resulting value that matched an entry in the (RC6) keymap
>> loaded for the remote... Certainly seems like something that *could*
>> happen somehow, but probably unlikely? I dunno...
>
> I don't know either.  There appears to be a chance for the first 16 bits
> of a transmitted NEC (Addr:Addr') or Extended NEC (AddrHi:AddrLo)
> sequence, to be interpreted as JVC (Addr:Cmd), and the JVC decoder
> matching a scancode in the keytable for the NEC remote.
>
>
>
>
>>  We do have the
>> option to disable all but the relevant protocol handler on a
>> per-device basis though, if that's a problem. Hrm, the key tables also
>> have a protocol tied to them, not sure if that's taken into account
>> when doing matching... Still getting to know the code. :)
>
> It does not look like
>
>        ir_keydown()
>                ir_g_keycode_from_table()
>                        ir_getkeycode()
>
> bother to check the ir_type (e.g. IR_TYPE_NEC) of the keymap against the
> decoders type.  Neither do the decoders themselves.
>
>
> If a decoder decodes something and thinks its valid, it tries to send a
> key event with ir_keydown().  ir_keydown() won't send a key event if the
> lookup comes back KEY_RESERVED, but it doesn't tell the decoder about
> the failure to find a key mapping.  A decoder can come back saying it
> did it's job, without knowing whether or not the decoding corresponded
> to a valid key in the loaded keymap. :(
>
>
>> > You will have to deal with the case that two or more decoders may match
>> > and each sends an IR event.  (Unless the ir-core already deals with this
>> > somehow...)
>>
>> Well, its gotta decode correctly to a value, and then match a value in
>> the loaded key table for an input event to get sent through. At least
>> for the RC6 MCE remotes, I haven't seen any of the other decoders take
>> the signal and interpret it as valid -- which ought to be by design,
>> if you consider that people use several different remotes with varying
>> ir signals with different devices all receiving them all the time
>> without problems (usually). And if we're not already, we could likely
>> add some logic to give higher precedence to values arrived at using
>> the protocol decoder that matches the key table we've got loaded for a
>> given device.
>
> After looking at things, the only potential problem I can see right now
> is with the JVC decoder and NEC remotes.
>
> I think that problem is most easily eliminated either by
>
> a. having ir_keydown() (or the functions it calls) check to see that the
> decoder matches the loaded keymap, or
>
> b. only calling the decoder that matches the loaded keymap's protocol
>
> Of the above, b. saves processor cycles and frees up the global
> ir_raw_handler spin lock sooner.  That spin lock is serializing pulse
> decoding for all the IR receivers in the system  (pulse decoding can
> still be interleaved, just only one IR receiver's pulses are be
> processed at any time).  What's the point of running decoders that
> should never match the loaded keymap?

For the daily use case where a known-good keymap is in place, I'm
coming to the conclusion that there's no point, we're only wasting
resources. For initial "figure out what this remote is" type of stuff,
running all decoders makes sense. One thought I had was that perhaps
we start by running through the decoder that is listed in the keymap.
If it decodes to a scancode and we find a valid key in the key table
(i.e., not KEY_RESERVED), we're done. If decoding fails or we don't
find a valid key, then try the other decoders. However, this is
possibly also wasteful -- most people with any somewhat involved htpc
setup are going to be constantly sending IR signals intended for other
devices that we pick up and try to decode.

So I'd say we go with your option b, and only call the decoder that
matches the loaded keymap. One could either pass in a modparam or
twiddle a sysfs attr or use ir-keytable to put the receiver into a
mode that called all decoders -- i.e., set protocol to
IR_TYPE_UNKNOWN, with the intention being to figure it out based on
running all decoders, and create a new keymap where IR_TYPE_FOO is
known.


-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Mauro Carvalho Chehab]

Em 29-05-2010 23:24, Jarod Wilson escreveu:
> On Sat, May 29, 2010 at 4:01 PM, Andy Walls <awalls@md.metrocast.net> wrote:
>> On Sat, 2010-05-29 at 12:58 -0400, Jarod Wilson wrote:
>>> On Sat, May 29, 2010 at 8:39 AM, Andy Walls <awalls@md.metrocast.net> wrote:
>>>> On Fri, 2010-05-28 at 00:47 -0400, Jarod Wilson wrote:
>>>>> So I'm inching closer to a viable mceusb driver submission -- both a
>>>>> first-gen and a third-gen transceiver are now working perfectly with
>>>>> multiple different mce remotes. However, that's only when I make sure
>>>>> the mceusb driver is loaded w/only the rc6 decoder loaded. When
>>>>> ir-core comes up, it requests all decoders to load, starting with the
>>>>> nec decoder, followed by the rc5 decoder, then the rc6 decoder and so
>>>>> on (init_decoders() in ir-raw-event.c). When I call
>>>>> ir_raw_event_handle, all decoders get run on the ir data buffer,
>>>>> starting with nec. Well, the nec decoder doesn't like the rc6 data, so
>>>>> it pukes. The RUN_DECODER macro break's out of the routine when that
>>>>> happens, and the rc6 decoder never gets a chance to run. (Similarly,
>>>>> if only ir-nec-decoder has been removed, the rc5 decoder pukes on the
>>>>> rc6 data, same problem).
>>>>
>>>> Yes, if the system kernel is going to attempt to discriminate between
>>>> various input singals, it needs to let all its "correlators" run and
>>>> produce a "confidence" number from each.
>>>>
>>>> Then ideally one would take the result with the highest confidence.
>>>>
>>>> Right now it looks like all the confidence determinations are boolean (0
>>>> or -EINVAL) and there is no chance to deal with the case that two
>>>> different decoders validly decode something.  The first decoder that
>>>> declares a match "wins" and sends an event.
>>>
>>> Yeah, it does look that way. I wonder how likely it is that e.g. a
>>> valid RC6 signal would be decoded to something by say the NEC decoder,
>>
>> NEC is a pulse position code and RC-6 is manchester encoded, so that
>> particular case would be unlikely.
>>
>> I would think one would have a better chance of false positiive
>> detections between similar encoding types: pulse position, pulse width,
>> or manchester.
>>
>> Looking at slide 11 of this:
>>
>>        http://www.audiodevelopers.com/temp/Remote_Controls.ppt
>>
>> It looks like the pulse position protocols with a header space of 8T
>> (where 8T is about 4ms) would be the only ones that could get confused.
>>
>> Since these are streaming decoders, it looks like JVC would come up with
>> false detections first, since it has the shortest payload of the pulse
>> position protocols.  I think JVC will always claim to decode an NEC
>> pulse train.  (I'll try to test that sometime.)
>>
>>
>>> with a resulting value that matched an entry in the (RC6) keymap
>>> loaded for the remote... Certainly seems like something that *could*
>>> happen somehow, but probably unlikely? I dunno...
>>
>> I don't know either.  There appears to be a chance for the first 16 bits
>> of a transmitted NEC (Addr:Addr') or Extended NEC (AddrHi:AddrLo)
>> sequence, to be interpreted as JVC (Addr:Cmd), and the JVC decoder
>> matching a scancode in the keytable for the NEC remote.
>>
>>
>>
>>
>>>  We do have the
>>> option to disable all but the relevant protocol handler on a
>>> per-device basis though, if that's a problem. Hrm, the key tables also
>>> have a protocol tied to them, not sure if that's taken into account
>>> when doing matching... Still getting to know the code. :)
>>
>> It does not look like
>>
>>        ir_keydown()
>>                ir_g_keycode_from_table()
>>                        ir_getkeycode()
>>
>> bother to check the ir_type (e.g. IR_TYPE_NEC) of the keymap against the
>> decoders type.  Neither do the decoders themselves.
>>
>>
>> If a decoder decodes something and thinks its valid, it tries to send a
>> key event with ir_keydown().  ir_keydown() won't send a key event if the
>> lookup comes back KEY_RESERVED, but it doesn't tell the decoder about
>> the failure to find a key mapping.  A decoder can come back saying it
>> did it's job, without knowing whether or not the decoding corresponded
>> to a valid key in the loaded keymap. :(
>>
>>
>>>> You will have to deal with the case that two or more decoders may match
>>>> and each sends an IR event.  (Unless the ir-core already deals with this
>>>> somehow...)
>>>
>>> Well, its gotta decode correctly to a value, and then match a value in
>>> the loaded key table for an input event to get sent through. At least
>>> for the RC6 MCE remotes, I haven't seen any of the other decoders take
>>> the signal and interpret it as valid -- which ought to be by design,
>>> if you consider that people use several different remotes with varying
>>> ir signals with different devices all receiving them all the time
>>> without problems (usually). And if we're not already, we could likely
>>> add some logic to give higher precedence to values arrived at using
>>> the protocol decoder that matches the key table we've got loaded for a
>>> given device.
>>
>> After looking at things, the only potential problem I can see right now
>> is with the JVC decoder and NEC remotes.
>>
>> I think that problem is most easily eliminated either by
>>
>> a. having ir_keydown() (or the functions it calls) check to see that the
>> decoder matches the loaded keymap, or
>>
>> b. only calling the decoder that matches the loaded keymap's protocol
>>
>> Of the above, b. saves processor cycles and frees up the global
>> ir_raw_handler spin lock sooner.  That spin lock is serializing pulse
>> decoding for all the IR receivers in the system  (pulse decoding can
>> still be interleaved, just only one IR receiver's pulses are be
>> processed at any time).  What's the point of running decoders that
>> should never match the loaded keymap?
> 
> For the daily use case where a known-good keymap is in place, I'm
> coming to the conclusion that there's no point, we're only wasting
> resources. For initial "figure out what this remote is" type of stuff,
> running all decoders makes sense. One thought I had was that perhaps
> we start by running through the decoder that is listed in the keymap.
> If it decodes to a scancode and we find a valid key in the key table
> (i.e., not KEY_RESERVED), we're done. If decoding fails or we don't
> find a valid key, then try the other decoders. However, this is
> possibly also wasteful -- most people with any somewhat involved htpc
> setup are going to be constantly sending IR signals intended for other
> devices that we pick up and try to decode.
> 
> So I'd say we go with your option b, and only call the decoder that
> matches the loaded keymap. One could either pass in a modparam or
> twiddle a sysfs attr or use ir-keytable to put the receiver into a
> mode that called all decoders -- i.e., set protocol to
> IR_TYPE_UNKNOWN, with the intention being to figure it out based on
> running all decoders, and create a new keymap where IR_TYPE_FOO is
> known.

There's no need to extra parameters. Decoders can be disabled by userspace,
per each rc sysfs node. Btw, the current version of ir-keytable already sets
the enabled protocols based on the protocol reported by the rc keymap.

What it makes sense is to add a patch at RC core that will properly enable/disable
the protocols based on IR_TYPE, when the rc-map is stored in-kernel.

Cheers,
Mauro.

Re: ir-core multi-protocol decode and mceusb

[Jarod Wilson]

On Sun, May 30, 2010 at 10:02 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 29-05-2010 23:24, Jarod Wilson escreveu:
>> On Sat, May 29, 2010 at 4:01 PM, Andy Walls <awalls@md.metrocast.net> wrote:
...
>>>>  We do have the
>>>> option to disable all but the relevant protocol handler on a
>>>> per-device basis though, if that's a problem. Hrm, the key tables also
>>>> have a protocol tied to them, not sure if that's taken into account
>>>> when doing matching... Still getting to know the code. :)
>>>
>>> It does not look like
>>>
>>>        ir_keydown()
>>>                ir_g_keycode_from_table()
>>>                        ir_getkeycode()
>>>
>>> bother to check the ir_type (e.g. IR_TYPE_NEC) of the keymap against the
>>> decoders type.  Neither do the decoders themselves.
>>>
>>>
>>> If a decoder decodes something and thinks its valid, it tries to send a
>>> key event with ir_keydown().  ir_keydown() won't send a key event if the
>>> lookup comes back KEY_RESERVED, but it doesn't tell the decoder about
>>> the failure to find a key mapping.  A decoder can come back saying it
>>> did it's job, without knowing whether or not the decoding corresponded
>>> to a valid key in the loaded keymap. :(
>>>
>>>
>>>>> You will have to deal with the case that two or more decoders may match
>>>>> and each sends an IR event.  (Unless the ir-core already deals with this
>>>>> somehow...)
>>>>
>>>> Well, its gotta decode correctly to a value, and then match a value in
>>>> the loaded key table for an input event to get sent through. At least
>>>> for the RC6 MCE remotes, I haven't seen any of the other decoders take
>>>> the signal and interpret it as valid -- which ought to be by design,
>>>> if you consider that people use several different remotes with varying
>>>> ir signals with different devices all receiving them all the time
>>>> without problems (usually). And if we're not already, we could likely
>>>> add some logic to give higher precedence to values arrived at using
>>>> the protocol decoder that matches the key table we've got loaded for a
>>>> given device.
>>>
>>> After looking at things, the only potential problem I can see right now
>>> is with the JVC decoder and NEC remotes.
>>>
>>> I think that problem is most easily eliminated either by
>>>
>>> a. having ir_keydown() (or the functions it calls) check to see that the
>>> decoder matches the loaded keymap, or
>>>
>>> b. only calling the decoder that matches the loaded keymap's protocol
>>>
>>> Of the above, b. saves processor cycles and frees up the global
>>> ir_raw_handler spin lock sooner.  That spin lock is serializing pulse
>>> decoding for all the IR receivers in the system  (pulse decoding can
>>> still be interleaved, just only one IR receiver's pulses are be
>>> processed at any time).  What's the point of running decoders that
>>> should never match the loaded keymap?
>>
>> For the daily use case where a known-good keymap is in place, I'm
>> coming to the conclusion that there's no point, we're only wasting
>> resources. For initial "figure out what this remote is" type of stuff,
>> running all decoders makes sense. One thought I had was that perhaps
>> we start by running through the decoder that is listed in the keymap.
>> If it decodes to a scancode and we find a valid key in the key table
>> (i.e., not KEY_RESERVED), we're done. If decoding fails or we don't
>> find a valid key, then try the other decoders. However, this is
>> possibly also wasteful -- most people with any somewhat involved htpc
>> setup are going to be constantly sending IR signals intended for other
>> devices that we pick up and try to decode.
>>
>> So I'd say we go with your option b, and only call the decoder that
>> matches the loaded keymap. One could either pass in a modparam or
>> twiddle a sysfs attr or use ir-keytable to put the receiver into a
>> mode that called all decoders -- i.e., set protocol to
>> IR_TYPE_UNKNOWN, with the intention being to figure it out based on
>> running all decoders, and create a new keymap where IR_TYPE_FOO is
>> known.
>
> There's no need to extra parameters. Decoders can be disabled by userspace,
> per each rc sysfs node. Btw, the current version of ir-keytable already sets
> the enabled protocols based on the protocol reported by the rc keymap.
>
> What it makes sense is to add a patch at RC core that will properly enable/disable
> the protocols based on IR_TYPE, when the rc-map is stored in-kernel.

Ah, yeah, that does make sense. And if we add that, ir-keytable
doesn't actually have to worry about doing similar itself any longer.
If you're not already working on it, I can try to whip something up,
though I'm knee-deep in an ir-lirc-codec bridge right now...

-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Jarod Wilson]

On Sun, May 30, 2010 at 3:57 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
> On Sun, May 30, 2010 at 10:02 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 29-05-2010 23:24, Jarod Wilson escreveu:
>>> On Sat, May 29, 2010 at 4:01 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> ...
>>>>>  We do have the
>>>>> option to disable all but the relevant protocol handler on a
>>>>> per-device basis though, if that's a problem. Hrm, the key tables also
>>>>> have a protocol tied to them, not sure if that's taken into account
>>>>> when doing matching... Still getting to know the code. :)
>>>>
>>>> It does not look like
>>>>
>>>>        ir_keydown()
>>>>                ir_g_keycode_from_table()
>>>>                        ir_getkeycode()
>>>>
>>>> bother to check the ir_type (e.g. IR_TYPE_NEC) of the keymap against the
>>>> decoders type.  Neither do the decoders themselves.
>>>>
>>>>
>>>> If a decoder decodes something and thinks its valid, it tries to send a
>>>> key event with ir_keydown().  ir_keydown() won't send a key event if the
>>>> lookup comes back KEY_RESERVED, but it doesn't tell the decoder about
>>>> the failure to find a key mapping.  A decoder can come back saying it
>>>> did it's job, without knowing whether or not the decoding corresponded
>>>> to a valid key in the loaded keymap. :(
>>>>
>>>>
>>>>>> You will have to deal with the case that two or more decoders may match
>>>>>> and each sends an IR event.  (Unless the ir-core already deals with this
>>>>>> somehow...)
>>>>>
>>>>> Well, its gotta decode correctly to a value, and then match a value in
>>>>> the loaded key table for an input event to get sent through. At least
>>>>> for the RC6 MCE remotes, I haven't seen any of the other decoders take
>>>>> the signal and interpret it as valid -- which ought to be by design,
>>>>> if you consider that people use several different remotes with varying
>>>>> ir signals with different devices all receiving them all the time
>>>>> without problems (usually). And if we're not already, we could likely
>>>>> add some logic to give higher precedence to values arrived at using
>>>>> the protocol decoder that matches the key table we've got loaded for a
>>>>> given device.
>>>>
>>>> After looking at things, the only potential problem I can see right now
>>>> is with the JVC decoder and NEC remotes.
>>>>
>>>> I think that problem is most easily eliminated either by
>>>>
>>>> a. having ir_keydown() (or the functions it calls) check to see that the
>>>> decoder matches the loaded keymap, or
>>>>
>>>> b. only calling the decoder that matches the loaded keymap's protocol
>>>>
>>>> Of the above, b. saves processor cycles and frees up the global
>>>> ir_raw_handler spin lock sooner.  That spin lock is serializing pulse
>>>> decoding for all the IR receivers in the system  (pulse decoding can
>>>> still be interleaved, just only one IR receiver's pulses are be
>>>> processed at any time).  What's the point of running decoders that
>>>> should never match the loaded keymap?
>>>
>>> For the daily use case where a known-good keymap is in place, I'm
>>> coming to the conclusion that there's no point, we're only wasting
>>> resources. For initial "figure out what this remote is" type of stuff,
>>> running all decoders makes sense. One thought I had was that perhaps
>>> we start by running through the decoder that is listed in the keymap.
>>> If it decodes to a scancode and we find a valid key in the key table
>>> (i.e., not KEY_RESERVED), we're done. If decoding fails or we don't
>>> find a valid key, then try the other decoders. However, this is
>>> possibly also wasteful -- most people with any somewhat involved htpc
>>> setup are going to be constantly sending IR signals intended for other
>>> devices that we pick up and try to decode.
>>>
>>> So I'd say we go with your option b, and only call the decoder that
>>> matches the loaded keymap. One could either pass in a modparam or
>>> twiddle a sysfs attr or use ir-keytable to put the receiver into a
>>> mode that called all decoders -- i.e., set protocol to
>>> IR_TYPE_UNKNOWN, with the intention being to figure it out based on
>>> running all decoders, and create a new keymap where IR_TYPE_FOO is
>>> known.
>>
>> There's no need to extra parameters. Decoders can be disabled by userspace,
>> per each rc sysfs node. Btw, the current version of ir-keytable already sets
>> the enabled protocols based on the protocol reported by the rc keymap.
>>
>> What it makes sense is to add a patch at RC core that will properly enable/disable
>> the protocols based on IR_TYPE, when the rc-map is stored in-kernel.
>
> Ah, yeah, that does make sense. And if we add that, ir-keytable
> doesn't actually have to worry about doing similar itself any longer.
> If you're not already working on it, I can try to whip something up,
> though I'm knee-deep in an ir-lirc-codec bridge right now...

I've now got an ir-lirc-codec bridge passing data over to a completely
unmodified lirc_dev, with the data then making its way out to the
lircd userspace where its even getting properly decoded. I don't have
the transmit side of things in ir-lirc-codec wired up just yet, but
I'd like to submit what I've got (after some cleanup) tomorrow, and
will then incrementally work on transmit. I'm pretty sure wiring up
transmit is going to require some of the bits we'd be using for native
transmit as well, so there may be some discussion required. Will give
a look at setting enabled/disabled decoders tomorrow too, hopefully.


-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Andy Walls]

On Mon, 2010-05-31 at 02:20 -0400, Jarod Wilson wrote:
> On Sun, May 30, 2010 at 3:57 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
> > On Sun, May 30, 2010 at 10:02 AM, Mauro Carvalho Chehab
> > <mchehab@redhat.com> wrote:
> >> Em 29-05-2010 23:24, Jarod Wilson escreveu:
> >>> On Sat, May 29, 2010 at 4:01 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > ...
> >>>>>  We do have the
> >>>>> option to disable all but the relevant protocol handler on a
> >>>>> per-device basis though, if that's a problem. Hrm, the key tables also
> >>>>> have a protocol tied to them, not sure if that's taken into account
> >>>>> when doing matching... Still getting to know the code. :)
> >>>>
> >>>> It does not look like
> >>>>
> >>>>        ir_keydown()
> >>>>                ir_g_keycode_from_table()
> >>>>                        ir_getkeycode()
> >>>>
> >>>> bother to check the ir_type (e.g. IR_TYPE_NEC) of the keymap against the
> >>>> decoders type.  Neither do the decoders themselves.
> >>>>
> >>>>
> >>>> If a decoder decodes something and thinks its valid, it tries to send a
> >>>> key event with ir_keydown().  ir_keydown() won't send a key event if the
> >>>> lookup comes back KEY_RESERVED, but it doesn't tell the decoder about
> >>>> the failure to find a key mapping.  A decoder can come back saying it
> >>>> did it's job, without knowing whether or not the decoding corresponded
> >>>> to a valid key in the loaded keymap. :(
> >>>>
> >>>>
> >>>>>> You will have to deal with the case that two or more decoders may match
> >>>>>> and each sends an IR event.  (Unless the ir-core already deals with this
> >>>>>> somehow...)
> >>>>>
> >>>>> Well, its gotta decode correctly to a value, and then match a value in
> >>>>> the loaded key table for an input event to get sent through. At least
> >>>>> for the RC6 MCE remotes, I haven't seen any of the other decoders take
> >>>>> the signal and interpret it as valid -- which ought to be by design,
> >>>>> if you consider that people use several different remotes with varying
> >>>>> ir signals with different devices all receiving them all the time
> >>>>> without problems (usually). And if we're not already, we could likely
> >>>>> add some logic to give higher precedence to values arrived at using
> >>>>> the protocol decoder that matches the key table we've got loaded for a
> >>>>> given device.
> >>>>
> >>>> After looking at things, the only potential problem I can see right now
> >>>> is with the JVC decoder and NEC remotes.
> >>>>
> >>>> I think that problem is most easily eliminated either by
> >>>>
> >>>> a. having ir_keydown() (or the functions it calls) check to see that the
> >>>> decoder matches the loaded keymap, or
> >>>>
> >>>> b. only calling the decoder that matches the loaded keymap's protocol
> >>>>
> >>>> Of the above, b. saves processor cycles and frees up the global
> >>>> ir_raw_handler spin lock sooner.  That spin lock is serializing pulse
> >>>> decoding for all the IR receivers in the system  (pulse decoding can
> >>>> still be interleaved, just only one IR receiver's pulses are be
> >>>> processed at any time).  What's the point of running decoders that
> >>>> should never match the loaded keymap?
> >>>
> >>> For the daily use case where a known-good keymap is in place, I'm
> >>> coming to the conclusion that there's no point, we're only wasting
> >>> resources. For initial "figure out what this remote is" type of stuff,
> >>> running all decoders makes sense. One thought I had was that perhaps
> >>> we start by running through the decoder that is listed in the keymap.
> >>> If it decodes to a scancode and we find a valid key in the key table
> >>> (i.e., not KEY_RESERVED), we're done. If decoding fails or we don't
> >>> find a valid key, then try the other decoders. However, this is
> >>> possibly also wasteful -- most people with any somewhat involved htpc
> >>> setup are going to be constantly sending IR signals intended for other
> >>> devices that we pick up and try to decode.
> >>>
> >>> So I'd say we go with your option b, and only call the decoder that
> >>> matches the loaded keymap. One could either pass in a modparam or
> >>> twiddle a sysfs attr or use ir-keytable to put the receiver into a
> >>> mode that called all decoders -- i.e., set protocol to
> >>> IR_TYPE_UNKNOWN, with the intention being to figure it out based on
> >>> running all decoders, and create a new keymap where IR_TYPE_FOO is
> >>> known.
> >>
> >> There's no need to extra parameters. Decoders can be disabled by userspace,
> >> per each rc sysfs node. Btw, the current version of ir-keytable already sets
> >> the enabled protocols based on the protocol reported by the rc keymap.
> >>
> >> What it makes sense is to add a patch at RC core that will properly enable/disable
> >> the protocols based on IR_TYPE, when the rc-map is stored in-kernel.
> >
> > Ah, yeah, that does make sense. And if we add that, ir-keytable
> > doesn't actually have to worry about doing similar itself any longer.
> > If you're not already working on it, I can try to whip something up,
> > though I'm knee-deep in an ir-lirc-codec bridge right now...
> 
> I've now got an ir-lirc-codec bridge passing data over to a completely
> unmodified lirc_dev, with the data then making its way out to the
> lircd userspace where its even getting properly decoded. I don't have
> the transmit side of things in ir-lirc-codec wired up just yet, but
> I'd like to submit what I've got (after some cleanup) tomorrow, and
> will then incrementally work on transmit. I'm pretty sure wiring up
> transmit is going to require some of the bits we'd be using for native
> transmit as well, so there may be some discussion required. Will give
> a look at setting enabled/disabled decoders tomorrow too, hopefully.


Since you're looking at Tx, please take a look at the v4l2_subdev
interface for ir devices.  See 

linux/include/media/v4l2-subdev.h: struct v4l2_subdev_ir_ops 

I was wondering how this interface could be modified to interface nicely
to lirc (or I guess ir-lirc-codec) for transmit functionality.


Right now, only the cx23885 driver uses it:

linux/drivers/media/video/cx23885/cx23888-ir.[ch]

I have the skeleton of transmit for the device implemented (it does need
some fixing up).

(The CX23888 hardware is nice in that it only deals with raw pulses so
one can decode any protocol and transmit any protocols.  The hardware
provides hardware counter/timers for measuring incoming pulses and
sending outgoing pulses.)

Regards,
Andy

Re: ir-core multi-protocol decode and mceusb

[Mauro Carvalho Chehab]

Hi Andy,

Em 31-05-2010 09:15, Andy Walls escreveu:
>> I've now got an ir-lirc-codec bridge passing data over to a completely
>> unmodified lirc_dev, with the data then making its way out to the
>> lircd userspace where its even getting properly decoded. I don't have
>> the transmit side of things in ir-lirc-codec wired up just yet, but
>> I'd like to submit what I've got (after some cleanup) tomorrow, and
>> will then incrementally work on transmit. I'm pretty sure wiring up
>> transmit is going to require some of the bits we'd be using for native
>> transmit as well, so there may be some discussion required. Will give
>> a look at setting enabled/disabled decoders tomorrow too, hopefully.
> 
> 
> Since you're looking at Tx, please take a look at the v4l2_subdev
> interface for ir devices.  See 
> 
> linux/include/media/v4l2-subdev.h: struct v4l2_subdev_ir_ops 
> 
> I was wondering how this interface could be modified to interface nicely
> to lirc (or I guess ir-lirc-codec) for transmit functionality.

> Right now, only the cx23885 driver uses it:
> 
> linux/drivers/media/video/cx23885/cx23888-ir.[ch]
> 
> I have the skeleton of transmit for the device implemented (it does need
> some fixing up).
> 
> (The CX23888 hardware is nice in that it only deals with raw pulses so
> one can decode any protocol and transmit any protocols.  The hardware
> provides hardware counter/timers for measuring incoming pulses and
> sending outgoing pulses.)

This interface is bound to V4L needs. As the Remote Controller subsystem
is meant to support not only V4L or DVB IR's, but also other kinds of remote
controllers that aren't associated to media devices, it makes no sense on
binding TX to this interface. 

The biggest advantage of V4L subdev interface is that a command, like VIDIOC_S_STD
could be sent to several devices that may need to know what's the current standard,
in order to configure audio, video, etc. It also provides a nice way to access
devices on a device-internal bus. In the case of RC, I don't see any similar
need. So, IMO, the better is to use an in interface similar to RX for TX, e. g.,
something like:
	rc_register_tx()
	rc_unregister_tx()
	rc_send_code()

Of course, inside the V4L drivers, it may actually make sense to keep using the 
v4l2-subdev if the IR is accessed, for example, via I2C bus.

Cheers,
Mauro.

Re: ir-core multi-protocol decode and mceusb

[Andy Walls]

On Mon, 2010-05-31 at 16:06 -0300, Mauro Carvalho Chehab wrote:
> Hi Andy,
> 
> Em 31-05-2010 09:15, Andy Walls escreveu:
> >> I've now got an ir-lirc-codec bridge passing data over to a completely
> >> unmodified lirc_dev, with the data then making its way out to the
> >> lircd userspace where its even getting properly decoded. I don't have
> >> the transmit side of things in ir-lirc-codec wired up just yet, but
> >> I'd like to submit what I've got (after some cleanup) tomorrow, and
> >> will then incrementally work on transmit. I'm pretty sure wiring up
> >> transmit is going to require some of the bits we'd be using for native
> >> transmit as well, so there may be some discussion required. Will give
> >> a look at setting enabled/disabled decoders tomorrow too, hopefully.
> > 
> > 
> > Since you're looking at Tx, please take a look at the v4l2_subdev
> > interface for ir devices.  See 
> > 
> > linux/include/media/v4l2-subdev.h: struct v4l2_subdev_ir_ops 
> > 
> > I was wondering how this interface could be modified to interface nicely
> > to lirc (or I guess ir-lirc-codec) for transmit functionality.
> 
> > Right now, only the cx23885 driver uses it:
> > 
> > linux/drivers/media/video/cx23885/cx23888-ir.[ch]
> > 
> > I have the skeleton of transmit for the device implemented (it does need
> > some fixing up).
> > 
> > (The CX23888 hardware is nice in that it only deals with raw pulses so
> > one can decode any protocol and transmit any protocols.  The hardware
> > provides hardware counter/timers for measuring incoming pulses and
> > sending outgoing pulses.)
> 
> This interface is bound to V4L needs. As the Remote Controller subsystem
> is meant to support not only V4L or DVB IR's, but also other kinds of remote
> controllers that aren't associated to media devices, it makes no sense on
> binding TX to this interface. 
> 
> The biggest advantage of V4L subdev interface is that a command, like VIDIOC_S_STD
> could be sent to several devices that may need to know what's the current standard,
> in order to configure audio, video, etc. It also provides a nice way to access
> devices on a device-internal bus. In the case of RC, I don't see any similar
> need. So, IMO, the better is to use an in interface similar to RX for TX, e. g.,
> something like:
> 	rc_register_tx()
> 	rc_unregister_tx()
> 	rc_send_code()

Right, I agree.  The v4l2_subdev ir_ops is a detail hidden by the bridge
driver and the bridge driver needs to deal with that.  A registration
process seems to be the proper way to do things.

BTW, maybe

	rc_set_tx_parameters()

is needed as well to set up the parameters for the transmitter.

I haven't looked very hard at rc_register_rx() and related functions so
I will soon.  

Regards,
Andy

Re: ir-core multi-protocol decode and mceusb

[Mauro Carvalho Chehab]

Em 31-05-2010 16:38, Andy Walls escreveu:
> On Mon, 2010-05-31 at 16:06 -0300, Mauro Carvalho Chehab wrote:
>> Hi Andy,
>>
>> Em 31-05-2010 09:15, Andy Walls escreveu:
>>>> I've now got an ir-lirc-codec bridge passing data over to a completely
>>>> unmodified lirc_dev, with the data then making its way out to the
>>>> lircd userspace where its even getting properly decoded. I don't have
>>>> the transmit side of things in ir-lirc-codec wired up just yet, but
>>>> I'd like to submit what I've got (after some cleanup) tomorrow, and
>>>> will then incrementally work on transmit. I'm pretty sure wiring up
>>>> transmit is going to require some of the bits we'd be using for native
>>>> transmit as well, so there may be some discussion required. Will give
>>>> a look at setting enabled/disabled decoders tomorrow too, hopefully.
>>>
>>>
>>> Since you're looking at Tx, please take a look at the v4l2_subdev
>>> interface for ir devices.  See 
>>>
>>> linux/include/media/v4l2-subdev.h: struct v4l2_subdev_ir_ops 
>>>
>>> I was wondering how this interface could be modified to interface nicely
>>> to lirc (or I guess ir-lirc-codec) for transmit functionality.
>>
>>> Right now, only the cx23885 driver uses it:
>>>
>>> linux/drivers/media/video/cx23885/cx23888-ir.[ch]
>>>
>>> I have the skeleton of transmit for the device implemented (it does need
>>> some fixing up).
>>>
>>> (The CX23888 hardware is nice in that it only deals with raw pulses so
>>> one can decode any protocol and transmit any protocols.  The hardware
>>> provides hardware counter/timers for measuring incoming pulses and
>>> sending outgoing pulses.)
>>
>> This interface is bound to V4L needs. As the Remote Controller subsystem
>> is meant to support not only V4L or DVB IR's, but also other kinds of remote
>> controllers that aren't associated to media devices, it makes no sense on
>> binding TX to this interface. 
>>
>> The biggest advantage of V4L subdev interface is that a command, like VIDIOC_S_STD
>> could be sent to several devices that may need to know what's the current standard,
>> in order to configure audio, video, etc. It also provides a nice way to access
>> devices on a device-internal bus. In the case of RC, I don't see any similar
>> need. So, IMO, the better is to use an in interface similar to RX for TX, e. g.,
>> something like:
>> 	rc_register_tx()
>> 	rc_unregister_tx()
>> 	rc_send_code()
> 
> Right, I agree.  The v4l2_subdev ir_ops is a detail hidden by the bridge
> driver and the bridge driver needs to deal with that.  A registration
> process seems to be the proper way to do things.
> 
> BTW, maybe
> 
> 	rc_set_tx_parameters()
> 
> is needed as well to set up the parameters for the transmitter.
> 
> I haven't looked very hard at rc_register_rx() and related functions so
> I will soon.  

They are declared at include/media/ir-core.h. The actual name is ir_input_register().

The RX parameters are configured at the IR structs, before calling the function.

I may be wrong (since we didn't write any TX support), but I think that a
rc_set_tx_parameters() wouldn't be necessary, as I don't see why the driver will
change the parameters after registering, and without any userspace request.

If we consider that some userspace sysfs nodes will allow changing some parameters,
then the better is to have a callback function call, passed via the registering function,
that will allow calling a function inside the driver to change the TX parameters.

For example, something like:

struct rc_tx_props {
...
	int	(*change_protocol)(...);
...
};


rc_register_tx(..., struct rc_tx_props *props)


Cheers,
Mauro.

Re: ir-core multi-protocol decode and mceusb

[Andy Walls]

On Mon, 2010-05-31 at 17:58 -0300, Mauro Carvalho Chehab wrote:
> Em 31-05-2010 16:38, Andy Walls escreveu:
> > On Mon, 2010-05-31 at 16:06 -0300, Mauro Carvalho Chehab wrote:
> >> Hi Andy,
> >>
> >> Em 31-05-2010 09:15, Andy Walls escreveu:
> >>>> I've now got an ir-lirc-codec bridge passing data over to a completely
> >>>> unmodified lirc_dev, with the data then making its way out to the
> >>>> lircd userspace where its even getting properly decoded. I don't have
> >>>> the transmit side of things in ir-lirc-codec wired up just yet, but
> >>>> I'd like to submit what I've got (after some cleanup) tomorrow, and
> >>>> will then incrementally work on transmit. I'm pretty sure wiring up
> >>>> transmit is going to require some of the bits we'd be using for native
> >>>> transmit as well, so there may be some discussion required. Will give
> >>>> a look at setting enabled/disabled decoders tomorrow too, hopefully.
> >>>
> >>>
> >>> Since you're looking at Tx, please take a look at the v4l2_subdev
> >>> interface for ir devices.  See 
> >>>
> >>> linux/include/media/v4l2-subdev.h: struct v4l2_subdev_ir_ops 
> >>>
> >>> I was wondering how this interface could be modified to interface nicely
> >>> to lirc (or I guess ir-lirc-codec) for transmit functionality.
> >>
> >>> Right now, only the cx23885 driver uses it:
> >>>
> >>> linux/drivers/media/video/cx23885/cx23888-ir.[ch]
> >>>
> >>> I have the skeleton of transmit for the device implemented (it does need
> >>> some fixing up).
> >>>
> >>> (The CX23888 hardware is nice in that it only deals with raw pulses so
> >>> one can decode any protocol and transmit any protocols.  The hardware
> >>> provides hardware counter/timers for measuring incoming pulses and
> >>> sending outgoing pulses.)
> >>
> >> This interface is bound to V4L needs. As the Remote Controller subsystem
> >> is meant to support not only V4L or DVB IR's, but also other kinds of remote
> >> controllers that aren't associated to media devices, it makes no sense on
> >> binding TX to this interface. 
> >>
> >> The biggest advantage of V4L subdev interface is that a command, like VIDIOC_S_STD
> >> could be sent to several devices that may need to know what's the current standard,
> >> in order to configure audio, video, etc. It also provides a nice way to access
> >> devices on a device-internal bus. In the case of RC, I don't see any similar
> >> need. So, IMO, the better is to use an in interface similar to RX for TX, e. g.,
> >> something like:
> >> 	rc_register_tx()
> >> 	rc_unregister_tx()
> >> 	rc_send_code()
> > 
> > Right, I agree.  The v4l2_subdev ir_ops is a detail hidden by the bridge
> > driver and the bridge driver needs to deal with that.  A registration
> > process seems to be the proper way to do things.
> > 
> > BTW, maybe
> > 
> > 	rc_set_tx_parameters()
> > 
> > is needed as well to set up the parameters for the transmitter.
> > 
> > I haven't looked very hard at rc_register_rx() and related functions so
> > I will soon.  
> 
> They are declared at include/media/ir-core.h. The actual name is ir_input_register().
> 
> The RX parameters are configured at the IR structs, before calling the function.



> I may be wrong (since we didn't write any TX support), but I think that a
> rc_set_tx_parameters() wouldn't be necessary, as I don't see why the driver will
> change the parameters after registering, and without any userspace request.

Yes, my intent was to handle a user space request to change the
transmitter setup parameters to handle the protocol.

I also don't want to worry about having to code in kernel parameter
tables for any bizzare protocol userspace may know about.


> If we consider that some userspace sysfs nodes will allow changing some parameters,
> then the better is to have a callback function call, passed via the registering function,
> that will allow calling a function inside the driver to change the TX parameters.
> 
> For example, something like:
> 
> struct rc_tx_props {
> ...
> 	int	(*change_protocol)(...);
> ...
> };
> 
> 
> rc_register_tx(..., struct rc_tx_props *props)

A callback is likely needed.  I'm not sure I would have chosen the name
change_protocol(), because transmitter parameters can be common between
protocols (at least RC-5 and RC-6 can be supported with one set of
parameters), or not match any existing in-kernel protocol.  As long as
it is flexible enough to change individual transmitter parameters
(modulated/baseband, carrier freq, duty cycle, etc.) it will be fine.

Currently LIRC userspace changes Tx parameters using an ioctl().  It
asks the hardware to change transmitter parameters, because the current
model is that the transmitters don't need to know about protocols. (LIRC
userspace knows the parameters of the protocol it wants to use, so the
driver's don't have too).


I notice IR Rx also has a change_protocol() callback that is not
currently in use.  If sending raw pulses to userspace, it would be also
nice to expose that callback so userspace could set the receiver
parameters.


Regards,
Andy

Re: ir-core multi-protocol decode and mceusb

[Mauro Carvalho Chehab]

Em 31-05-2010 18:45, Andy Walls escreveu:
> On Mon, 2010-05-31 at 17:58 -0300, Mauro Carvalho Chehab wrote:

>> I may be wrong (since we didn't write any TX support), but I think that a
>> rc_set_tx_parameters() wouldn't be necessary, as I don't see why the driver will
>> change the parameters after registering, and without any userspace request.
> 
> Yes, my intent was to handle a user space request to change the
> transmitter setup parameters to handle the protocol.
> 
> I also don't want to worry about having to code in kernel parameter
> tables for any bizzare protocol userspace may know about.

Makes sense.
> 
> 
>> If we consider that some userspace sysfs nodes will allow changing some parameters,
>> then the better is to have a callback function call, passed via the registering function,
>> that will allow calling a function inside the driver to change the TX parameters.
>>
>> For example, something like:
>>
>> struct rc_tx_props {
>> ...
>> 	int	(*change_protocol)(...);
>> ...
>> };
>>
>>
>> rc_register_tx(..., struct rc_tx_props *props)
> 
> A callback is likely needed.  I'm not sure I would have chosen the name
> change_protocol(), because transmitter parameters can be common between
> protocols (at least RC-5 and RC-6 can be supported with one set of
> parameters), or not match any existing in-kernel protocol.  As long as
> it is flexible enough to change individual transmitter parameters
> (modulated/baseband, carrier freq, duty cycle, etc.) it will be fine.

I just used this name as an example, as the same name exists on RX.

Depending on how we code the userspace API, we may use just one set_parameters
function, or a set of per-attribute changes.

In other words, if we implement severa sysfs nodes to change several parameters,
maybe it makes sense to have several callbacks. Another alternative would be
to have a "commit" sysfs node to apply a set of parameters at once. 

> Currently LIRC userspace changes Tx parameters using an ioctl().  It
> asks the hardware to change transmitter parameters, because the current
> model is that the transmitters don't need to know about protocols. (LIRC
> userspace knows the parameters of the protocol it wants to use, so the
> driver's don't have too).
> 
> 
> I notice IR Rx also has a change_protocol() callback that is not
> currently in use.  

It is used only by em28xx, where the hardware decoder can work either with
RC-5 or NEC (newer chips also support RC-6, but this is currently not
implemented).

> If sending raw pulses to userspace, it would be also
> nice to expose that callback so userspace could set the receiver
> parameters.

Raw pulse transmission is probably the easiest case. Probably, there's nothing
or a very few things that might need adjustments.

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

Re: ir-core multi-protocol decode and mceusb

[Jarod Wilson]

On Mon, May 31, 2010 at 6:31 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 31-05-2010 18:45, Andy Walls escreveu:
>> On Mon, 2010-05-31 at 17:58 -0300, Mauro Carvalho Chehab wrote:
>
>>> I may be wrong (since we didn't write any TX support), but I think that a
>>> rc_set_tx_parameters() wouldn't be necessary, as I don't see why the driver will
>>> change the parameters after registering, and without any userspace request.
>>
>> Yes, my intent was to handle a user space request to change the
>> transmitter setup parameters to handle the protocol.
>>
>> I also don't want to worry about having to code in kernel parameter
>> tables for any bizzare protocol userspace may know about.
>
> Makes sense.
>>
>>
>>> If we consider that some userspace sysfs nodes will allow changing some parameters,
>>> then the better is to have a callback function call, passed via the registering function,
>>> that will allow calling a function inside the driver to change the TX parameters.
>>>
>>> For example, something like:
>>>
>>> struct rc_tx_props {
>>> ...
>>>      int     (*change_protocol)(...);
>>> ...
>>> };
>>>
>>>
>>> rc_register_tx(..., struct rc_tx_props *props)
>>
>> A callback is likely needed.  I'm not sure I would have chosen the name
>> change_protocol(), because transmitter parameters can be common between
>> protocols (at least RC-5 and RC-6 can be supported with one set of
>> parameters), or not match any existing in-kernel protocol.  As long as
>> it is flexible enough to change individual transmitter parameters
>> (modulated/baseband, carrier freq, duty cycle, etc.) it will be fine.
>
> I just used this name as an example, as the same name exists on RX.
>
> Depending on how we code the userspace API, we may use just one set_parameters
> function, or a set of per-attribute changes.
>
> In other words, if we implement severa sysfs nodes to change several parameters,
> maybe it makes sense to have several callbacks. Another alternative would be
> to have a "commit" sysfs node to apply a set of parameters at once.
>
>> Currently LIRC userspace changes Tx parameters using an ioctl().  It
>> asks the hardware to change transmitter parameters, because the current
>> model is that the transmitters don't need to know about protocols. (LIRC
>> userspace knows the parameters of the protocol it wants to use, so the
>> driver's don't have too).

The list of transmit-related ioctls implemented in the lirc_mceusb driver:

- LIRC_SET_TRANSMITTER_MASK -- these devices have two IR tx outputs,
default is to send the signal out both, but you can also select just a
specific one (i.e., two set top boxes, only want to send command to
one or the other of them).

- LIRC_GET_SEND_MODE -- get current transmit mode

- LIRC_SET_SEND_MODE -- set current transmit mode

- LIRC_SET_SEND_CARRIER -- set the transmit carrier freq

- LIRC_GET_FEATURE -- get both the send and receive capabilities of the device


>> I notice IR Rx also has a change_protocol() callback that is not
>> currently in use.
>
> It is used only by em28xx, where the hardware decoder can work either with
> RC-5 or NEC (newer chips also support RC-6, but this is currently not
> implemented).

The imon driver also implements change_protocol for the current-gen
devices, which are capable of decoding either mce remote signals or
the native imon remote signals. I was originally thinking I'd need to
implement change_protocol for the mceusb driver, but its ultimately a
no-op, since the hardware doesn't give a damn (and there's a note
somewhere that mentions its only relevant for hardware decode devices
that need to be put into a specific mode). Although, on something like
the mceusb driver, change_protocol *could* be wired up to mark only
the desired protocol enabled -- which might reduce complexity for
ir-keytable when loading a new keymap. (I went with a rather simple
approach for marking only the desired decoder enabled at initial
keymap load time which won't help here -- patch coming tomorrow for
that).

>> If sending raw pulses to userspace, it would be also
>> nice to expose that callback so userspace could set the receiver
>> parameters.
>
> Raw pulse transmission is probably the easiest case. Probably, there's nothing
> or a very few things that might need adjustments.

Transmitter mask, carrier frequency and a repeat count are the things
I can see needing to set regularly. From experience, at least with
motorola set top box hardware, you need to send a given signal 2-3
times, not just once, for the hardware to pick it up. There's a
min_repeat parameter in lirc config files only used on the transmit
side of the house to specify how many repeats of each blasted signal
to send.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Jarod Wilson]

On Tue, Jun 1, 2010 at 1:22 AM, Jarod Wilson <jarod@wilsonet.com> wrote:
> On Mon, May 31, 2010 at 6:31 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 31-05-2010 18:45, Andy Walls escreveu:
>>> On Mon, 2010-05-31 at 17:58 -0300, Mauro Carvalho Chehab wrote:
>>
>>>> I may be wrong (since we didn't write any TX support), but I think that a
>>>> rc_set_tx_parameters() wouldn't be necessary, as I don't see why the driver will
>>>> change the parameters after registering, and without any userspace request.
>>>
>>> Yes, my intent was to handle a user space request to change the
>>> transmitter setup parameters to handle the protocol.
>>>
>>> I also don't want to worry about having to code in kernel parameter
>>> tables for any bizzare protocol userspace may know about.
>>
>> Makes sense.
>>>
>>>
>>>> If we consider that some userspace sysfs nodes will allow changing some parameters,
>>>> then the better is to have a callback function call, passed via the registering function,
>>>> that will allow calling a function inside the driver to change the TX parameters.
>>>>
>>>> For example, something like:
>>>>
>>>> struct rc_tx_props {
>>>> ...
>>>>      int     (*change_protocol)(...);
>>>> ...
>>>> };
>>>>
>>>>
>>>> rc_register_tx(..., struct rc_tx_props *props)
>>>
>>> A callback is likely needed.  I'm not sure I would have chosen the name
>>> change_protocol(), because transmitter parameters can be common between
>>> protocols (at least RC-5 and RC-6 can be supported with one set of
>>> parameters), or not match any existing in-kernel protocol.  As long as
>>> it is flexible enough to change individual transmitter parameters
>>> (modulated/baseband, carrier freq, duty cycle, etc.) it will be fine.
>>
>> I just used this name as an example, as the same name exists on RX.
>>
>> Depending on how we code the userspace API, we may use just one set_parameters
>> function, or a set of per-attribute changes.
>>
>> In other words, if we implement severa sysfs nodes to change several parameters,
>> maybe it makes sense to have several callbacks. Another alternative would be
>> to have a "commit" sysfs node to apply a set of parameters at once.
>>
>>> Currently LIRC userspace changes Tx parameters using an ioctl().  It
>>> asks the hardware to change transmitter parameters, because the current
>>> model is that the transmitters don't need to know about protocols. (LIRC
>>> userspace knows the parameters of the protocol it wants to use, so the
>>> driver's don't have too).
>
> The list of transmit-related ioctls implemented in the lirc_mceusb driver:
>
> - LIRC_SET_TRANSMITTER_MASK -- these devices have two IR tx outputs,
> default is to send the signal out both, but you can also select just a
> specific one (i.e., two set top boxes, only want to send command to
> one or the other of them).
>
> - LIRC_GET_SEND_MODE -- get current transmit mode
>
> - LIRC_SET_SEND_MODE -- set current transmit mode
>
> - LIRC_SET_SEND_CARRIER -- set the transmit carrier freq
>
> - LIRC_GET_FEATURE -- get both the send and receive capabilities of the device
>
>
>>> I notice IR Rx also has a change_protocol() callback that is not
>>> currently in use.
>>
>> It is used only by em28xx, where the hardware decoder can work either with
>> RC-5 or NEC (newer chips also support RC-6, but this is currently not
>> implemented).
>
> The imon driver also implements change_protocol for the current-gen
> devices, which are capable of decoding either mce remote signals or
> the native imon remote signals. I was originally thinking I'd need to
> implement change_protocol for the mceusb driver, but its ultimately a
> no-op, since the hardware doesn't give a damn (and there's a note
> somewhere that mentions its only relevant for hardware decode devices
> that need to be put into a specific mode). Although, on something like
> the mceusb driver, change_protocol *could* be wired up to mark only
> the desired protocol enabled -- which might reduce complexity for
> ir-keytable when loading a new keymap. (I went with a rather simple
> approach for marking only the desired decoder enabled at initial
> keymap load time which won't help here -- patch coming tomorrow for
> that).
>
>>> If sending raw pulses to userspace, it would be also
>>> nice to expose that callback so userspace could set the receiver
>>> parameters.
>>
>> Raw pulse transmission is probably the easiest case. Probably, there's nothing
>> or a very few things that might need adjustments.
>
> Transmitter mask, carrier frequency and a repeat count are the things
> I can see needing to set regularly. From experience, at least with
> motorola set top box hardware, you need to send a given signal 2-3
> times, not just once, for the hardware to pick it up. There's a
> min_repeat parameter in lirc config files only used on the transmit
> side of the house to specify how many repeats of each blasted signal
> to send.

I keep bouncing between two machines, so I finally got smart(ish) and
pushed a working git tree to have both push and pull from.

http://git.wilsonet.com/linux-2.6-ir-wip.git/

Just pushed 3 patches with which I can now transmit IR via an mceusb
device. I've only added three callbacks to ir_dev_props, the rest of
the magic is done in mceusb.c and ir-lirc-codec.c. I'm still not sure
what sort of non-lirc interface we want for transmitting IR, and we
don't (yet) have in-kernel IR encoders...

I see Mauro was scrawling red ink all over the lirc_dev patch
tonight... ;) I'll try to reply to review comments tomorrow, I'm
spent! (I know the compat ioctl thing sucks, but changing it breaks
existing lirc userspace (for 32-bit users, iirc), which I'd like to
avoid).

-- 
Jarod Wilson
jarod@wilsonet.com

Re: ir-core multi-protocol decode and mceusb

[Mauro Carvalho Chehab]

Em 03-06-2010 03:27, Jarod Wilson escreveu:
> On Tue, Jun 1, 2010 at 1:22 AM, Jarod Wilson <jarod@wilsonet.com> wrote:
>> On Mon, May 31, 2010 at 6:31 PM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> wrote:
>>> Em 31-05-2010 18:45, Andy Walls escreveu:
>>>> On Mon, 2010-05-31 at 17:58 -0300, Mauro Carvalho Chehab wrote:
>>>
>>>>> I may be wrong (since we didn't write any TX support), but I think that a
>>>>> rc_set_tx_parameters() wouldn't be necessary, as I don't see why the driver will
>>>>> change the parameters after registering, and without any userspace request.
>>>>
>>>> Yes, my intent was to handle a user space request to change the
>>>> transmitter setup parameters to handle the protocol.
>>>>
>>>> I also don't want to worry about having to code in kernel parameter
>>>> tables for any bizzare protocol userspace may know about.
>>>
>>> Makes sense.
>>>>
>>>>
>>>>> If we consider that some userspace sysfs nodes will allow changing some parameters,
>>>>> then the better is to have a callback function call, passed via the registering function,
>>>>> that will allow calling a function inside the driver to change the TX parameters.
>>>>>
>>>>> For example, something like:
>>>>>
>>>>> struct rc_tx_props {
>>>>> ...
>>>>>      int     (*change_protocol)(...);
>>>>> ...
>>>>> };
>>>>>
>>>>>
>>>>> rc_register_tx(..., struct rc_tx_props *props)
>>>>
>>>> A callback is likely needed.  I'm not sure I would have chosen the name
>>>> change_protocol(), because transmitter parameters can be common between
>>>> protocols (at least RC-5 and RC-6 can be supported with one set of
>>>> parameters), or not match any existing in-kernel protocol.  As long as
>>>> it is flexible enough to change individual transmitter parameters
>>>> (modulated/baseband, carrier freq, duty cycle, etc.) it will be fine.
>>>
>>> I just used this name as an example, as the same name exists on RX.
>>>
>>> Depending on how we code the userspace API, we may use just one set_parameters
>>> function, or a set of per-attribute changes.
>>>
>>> In other words, if we implement severa sysfs nodes to change several parameters,
>>> maybe it makes sense to have several callbacks. Another alternative would be
>>> to have a "commit" sysfs node to apply a set of parameters at once.
>>>
>>>> Currently LIRC userspace changes Tx parameters using an ioctl().  It
>>>> asks the hardware to change transmitter parameters, because the current
>>>> model is that the transmitters don't need to know about protocols. (LIRC
>>>> userspace knows the parameters of the protocol it wants to use, so the
>>>> driver's don't have too).
>>
>> The list of transmit-related ioctls implemented in the lirc_mceusb driver:
>>
>> - LIRC_SET_TRANSMITTER_MASK -- these devices have two IR tx outputs,
>> default is to send the signal out both, but you can also select just a
>> specific one (i.e., two set top boxes, only want to send command to
>> one or the other of them).
>>
>> - LIRC_GET_SEND_MODE -- get current transmit mode
>>
>> - LIRC_SET_SEND_MODE -- set current transmit mode
>>
>> - LIRC_SET_SEND_CARRIER -- set the transmit carrier freq
>>
>> - LIRC_GET_FEATURE -- get both the send and receive capabilities of the device
>>
>>
>>>> I notice IR Rx also has a change_protocol() callback that is not
>>>> currently in use.
>>>
>>> It is used only by em28xx, where the hardware decoder can work either with
>>> RC-5 or NEC (newer chips also support RC-6, but this is currently not
>>> implemented).
>>
>> The imon driver also implements change_protocol for the current-gen
>> devices, which are capable of decoding either mce remote signals or
>> the native imon remote signals. I was originally thinking I'd need to
>> implement change_protocol for the mceusb driver, but its ultimately a
>> no-op, since the hardware doesn't give a damn (and there's a note
>> somewhere that mentions its only relevant for hardware decode devices
>> that need to be put into a specific mode). Although, on something like
>> the mceusb driver, change_protocol *could* be wired up to mark only
>> the desired protocol enabled -- which might reduce complexity for
>> ir-keytable when loading a new keymap. (I went with a rather simple
>> approach for marking only the desired decoder enabled at initial
>> keymap load time which won't help here -- patch coming tomorrow for
>> that).
>>
>>>> If sending raw pulses to userspace, it would be also
>>>> nice to expose that callback so userspace could set the receiver
>>>> parameters.
>>>
>>> Raw pulse transmission is probably the easiest case. Probably, there's nothing
>>> or a very few things that might need adjustments.
>>
>> Transmitter mask, carrier frequency and a repeat count are the things
>> I can see needing to set regularly. From experience, at least with
>> motorola set top box hardware, you need to send a given signal 2-3
>> times, not just once, for the hardware to pick it up. There's a
>> min_repeat parameter in lirc config files only used on the transmit
>> side of the house to specify how many repeats of each blasted signal
>> to send.
> 
> I keep bouncing between two machines, so I finally got smart(ish) and
> pushed a working git tree to have both push and pull from.
> 
> http://git.wilsonet.com/linux-2.6-ir-wip.git/
> 
> Just pushed 3 patches with which I can now transmit IR via an mceusb
> device. I've only added three callbacks to ir_dev_props, the rest of
> the magic is done in mceusb.c and ir-lirc-codec.c.

Nice!

> I'm still not sure
> what sort of non-lirc interface we want for transmitting IR, and we
> don't (yet) have in-kernel IR encoders...

Well, as we don't have any usecase yet, maybe we could just add the lirc
interface for now.

> I see Mauro was scrawling red ink all over the lirc_dev patch
> tonight... ;) 

:)

It was not that serious: just BKL, compat32 and postponing the definition of
the ioctls that aren't yet used.

> I'll try to reply to review comments tomorrow, I'm
> spent! (I know the compat ioctl thing sucks, but changing it breaks
> existing lirc userspace (for 32-bit users, iirc), which I'd like to
> avoid).

If we remove the compat32 stuff and define everything using __u32
instead of unsigned int, this won't break compatibility if both kernespace
and userspace are 32 bits.

The breakage will happen with a 64 bits userspace application, compiled with
the old lirc include.

A re-compilation of the application will solve the issue.

This kind of breakage is more serious where the userspace is provided as
binary only, as the user can't compile it.

I won't bother much about such breakage, since:

1) Currently, lirc is a set of out-of-tree patches. So, the official kernels
are not bound to the lirc interface;

2) After adding lirc-dev in kernel, a new version of lirc should be produced
anyway, properly documenting that lirc-dev shouldn't be compiled with the
newer kernels and eventually changing the build procedures;

3) The kernel-driver aware version of lirc can be indicated as the minimum requirement
for lirc-dev at Documentation/Changes;

4) An older version of lirc will have its own lirc-dev patches. If someone is using
it, the out-of-tree lirc-dev were already used;

5) We can add a way for newer lirc userspace apps to check for the lirc kernelspace
version. This way, we can move the compat32 stuff to userspace during a transitory
period, where people may have been using the new lirc with an older kernel;

6) With compat32 being on userspace, after a  few kernel cycles (or after being sure that
most distros had already applied the in-tree lirc-dev), this compat32 can be removed on 
userspace. As kernelspace user API's are forever, the same won't occur if we
let the patch be applied as-is.

On the other hand, keeping the additional complexity of a compat layer for a new driver
just due to a temporary need of for a transitory period doesn't seem the right thing
to do.

Cheers,
Mauro.