Re: [RFC PATCH 2/3] ir-rc5-sz: Add ir encoding support

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

Hi Antti,

On Monday 10 February 2014 22:09:33 Antti Seppälä wrote:
> >> +static int ir_rc5_sz_encode(u64 protocols,
> >> +                         const struct rc_scancode_filter *scancode,
> >> +                         struct ir_raw_event *events, unsigned int max)
> >> +{
> >> +     int ret;
> >> +     struct ir_raw_event *e = events;
> > 
> > Probably worth checking scancode->mask == 0xfff too?
> 
> I guess so. However if I'm not mistaken this makes all wakeup_filter
> writes fail in user space if wakeup_filter_mask is not set. Is that
> intended?

Good point, although looking at your patch 3, mask==0 is already permitted 
silently by the driver, which I think would make it okay.

I guess to be safe userland would have to do:
wakeup_filter_mask = 0
wakeup_filter = $value
wakeup_filter_mask = 0xfff

which doesn't sound unreasonable in the absence of a way to update them 
atomically (sysfs files doing more than one thing is frowned upon I believe).

> >> +     /* RC5-SZ scancode is raw enough for manchester as it is */
> >> +     ret = ir_raw_gen_manchester(&e, max, &ir_rc5_sz_timings,
> >> RC5_SZ_NBITS, +                                 scancode->data);
> >> +     if (ret < 0)
> >> +             return ret;
> > 
> > I suspect it needs some more space at the end too, to be sure that no
> > more bits afterwards are accepted.
> 
> I'm sorry but I'm not sure I completely understood what you meant
> here. For RC-5-SZ the entire scancode gets encoded and nothing more.
> Do you mean that the encoder should append some ir silence to the end
> result to make sure the ir sample has ended?

Yeh something like that. Certainly the raw decoders I've looked at expect a 
certain amount of space at the end to avoid decoding part of a longer protocol 
(it's in the pulse distance helper as the trailer space timing). Similarly the 
IMG hardware decoder has register fields for the free-time to require at the 
end of the message.

In fact it becomes a bit awkward for the raw IR driver for the IMG hardware 
which uses edge interrupts, as it has to have a timeout to emit a final repeat 
event after 150ms of inactivity, in order for the raw decoders to accept it 
(unless you hold the button down in which case the repeat code edges result in 
the long space).

Cheers
James

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]