[v2,6/9] rc: ir-rc5-sz-decoder: Add ir encoding support

  From: Antti Seppälä <a.seppala@gmail.com>

The encoding in rc5-sz first inserts a pulse and then simply utilizes the
generic Manchester encoder available in rc-core.

Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
Signed-off-by: James Hogan <james@albanarts.com>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: David Härdeman <david@hardeman.nu>
---
Changes in v2 (James Hogan):
 - Turn ir_rc5_sz_encode() comment into kerneldoc and update to reflect
   new API, with the -ENOBUFS return value for when the buffer is full
   and only a partial message was encoded.
 - Be more flexible around accepted scancode masks, as long as all the
   important bits are set (0x2fff) and none of the unimportant bits are
   set in the data. Also mask off the unimportant bits before passing to
   ir_raw_gen_manchester().
 - Explicitly enable leader bit in Manchester modulation timings.
---
 drivers/media/rc/ir-rc5-sz-decoder.c | 45 ++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
  

On 15 March 2014 01:04, James Hogan <james@albanarts.com> wrote:
> From: Antti Seppälä <a.seppala@gmail.com>
>
> The encoding in rc5-sz first inserts a pulse and then simply utilizes the
> generic Manchester encoder available in rc-core.
>
> Signed-off-by: Antti Seppälä <a.seppala@gmail.com>
> Signed-off-by: James Hogan <james@albanarts.com>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: David Härdeman <david@hardeman.nu>
> ---
> Changes in v2 (James Hogan):
>  - Turn ir_rc5_sz_encode() comment into kerneldoc and update to reflect
>    new API, with the -ENOBUFS return value for when the buffer is full
>    and only a partial message was encoded.
>  - Be more flexible around accepted scancode masks, as long as all the
>    important bits are set (0x2fff) and none of the unimportant bits are
>    set in the data. Also mask off the unimportant bits before passing to
>    ir_raw_gen_manchester().
>  - Explicitly enable leader bit in Manchester modulation timings.
> ---
>  drivers/media/rc/ir-rc5-sz-decoder.c | 45 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/drivers/media/rc/ir-rc5-sz-decoder.c b/drivers/media/rc/ir-rc5-sz-decoder.c
> index dc18b74..a342a4f 100644
> --- a/drivers/media/rc/ir-rc5-sz-decoder.c
> +++ b/drivers/media/rc/ir-rc5-sz-decoder.c
> @@ -127,9 +127,54 @@ out:
>         return -EINVAL;
>  }
>
> +static struct ir_raw_timings_manchester ir_rc5_sz_timings = {
> +       .leader                 = 1,
> +       .pulse_space_start      = 0,
> +       .clock                  = RC5_UNIT,
> +       .trailer_space          = RC5_UNIT * 10,
> +};
> +
> +/**
> + * ir_rc5_sz_encode() - Encode a scancode as a stream of raw events
> + *
> + * @protocols: allowed protocols
> + * @scancode:  scancode filter describing scancode (helps distinguish between
> + *             protocol subtypes when scancode is ambiguous)
> + * @events:    array of raw ir events to write into
> + * @max:       maximum size of @events
> + *
> + * Returns:    The number of events written.
> + *             -ENOBUFS if there isn't enough space in the array to fit the
> + *             encoding. In this case all @max events will have been written.
> + *             -EINVAL if the scancode is ambiguous or invalid.
> + */
> +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;
> +
> +       /* all important bits of scancode should be set in mask */
> +       if (~scancode->mask & 0x2fff)

Do we want to be so restrictive here? In my opinion it's quite nice to
be able to encode also the toggle bit if needed. Therefore a check
against 0x3fff would be a better choice.

I think the ability to encode toggle bit might also be nice to have
for rc-5(x) also.

> +               return -EINVAL;
> +       /* extra bits in mask should be zero in data */
> +       if (scancode->mask & scancode->data & ~0x2fff)
> +               return -EINVAL;
> +
> +       /* 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 & 0x2fff);

I'm not sure that the & 0x2fff is necessary. It has the ill effect of
eventually writing something else to hardware while still committing
the filter as unmodified original value. This will result in
difference between what sysfs states was written when read back and
what was actually written.

I think checks above are good enough to restrict the values and as
little modification as possible of the data should be done after that.

> +       if (ret < 0)
> +               return ret;
> +
> +       return e - events;
> +}
> +
>  static struct ir_raw_handler rc5_sz_handler = {
>         .protocols      = RC_BIT_RC5_SZ,
>         .decode         = ir_rc5_sz_decode,
> +       .encode         = ir_rc5_sz_encode,
>  };
>
>  static int __init ir_rc5_sz_decode_init(void)
> --
> 1.8.3.2
>
--
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
  

Hi Antti,

On Sunday 16 March 2014 10:34:31 Antti Seppälä wrote:
> > +/**
> > + * ir_rc5_sz_encode() - Encode a scancode as a stream of raw events
> > + *
> > + * @protocols: allowed protocols
> > + * @scancode:  scancode filter describing scancode (helps distinguish
> > between + *             protocol subtypes when scancode is ambiguous)
> > + * @events:    array of raw ir events to write into
> > + * @max:       maximum size of @events
> > + *
> > + * Returns:    The number of events written.
> > + *             -ENOBUFS if there isn't enough space in the array to fit
> > the + *             encoding. In this case all @max events will have been
> > written. + *             -EINVAL if the scancode is ambiguous or invalid.
> > + */
> > +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;
> > +
> > +       /* all important bits of scancode should be set in mask */
> > +       if (~scancode->mask & 0x2fff)
> 
> Do we want to be so restrictive here? In my opinion it's quite nice to
> be able to encode also the toggle bit if needed. Therefore a check
> against 0x3fff would be a better choice.
> 
> I think the ability to encode toggle bit might also be nice to have
> for rc-5(x) also.
> 

I don't believe the toggle bit is encoded in the scancode though, so I'm not 
sure it makes sense to treat it like that. I'm not an expert on RC-5 like 
protocols or the use of the toggle bit though.

> > +               return -EINVAL;
> > +       /* extra bits in mask should be zero in data */
> > +       if (scancode->mask & scancode->data & ~0x2fff)
> > +               return -EINVAL;
> > +
> > +       /* 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 &
> > 0x2fff);
> 
> I'm not sure that the & 0x2fff is necessary. It has the ill effect of
> eventually writing something else to hardware while still committing
> the filter as unmodified original value. This will result in
> difference between what sysfs states was written when read back and
> what was actually written.
> 
> I think checks above are good enough to restrict the values and as
> little modification as possible of the data should be done after that.

I suspect it was the toggle bit I was thinking about, e.g.
echo 0x3fff > wakeup_filter
echo 0x2fff > wakeup_filter_mask

I had assumed shouldn't be able to affect the toggle bit (it's not set in 
mask).

Cheers
James
  

Hi James.

On 16 March 2014 13:50, James Hogan <james@albanarts.com> wrote:
> Hi Antti,
>
> On Sunday 16 March 2014 10:34:31 Antti Seppälä wrote:
>> > +
>> > +       /* all important bits of scancode should be set in mask */
>> > +       if (~scancode->mask & 0x2fff)
>>
>> Do we want to be so restrictive here? In my opinion it's quite nice to
>> be able to encode also the toggle bit if needed. Therefore a check
>> against 0x3fff would be a better choice.
>>
>> I think the ability to encode toggle bit might also be nice to have
>> for rc-5(x) also.
>>
>
> I don't believe the toggle bit is encoded in the scancode though, so I'm not
> sure it makes sense to treat it like that. I'm not an expert on RC-5 like
> protocols or the use of the toggle bit though.
>

Well I'm not an expert either but at least streamzap tends to have the
toggle bit enabled quite often when sending ir pulses.

When decoding the toggle is always removed from the scancode but when
encoding it would be useful to have the possibility to encode it in.
This is because setting the toggle bit into wakeup makes it easier to
wake the system with nuvoton hw as it is difficult to press the remote
key short time enough (less than around 112ms) to generate a pulse
without the toggle bit set.

-Antti
--
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
  

On Sunday 16 March 2014 14:14:58 Antti Seppälä wrote:
> Hi James.
> 
> On 16 March 2014 13:50, James Hogan <james@albanarts.com> wrote:
> > Hi Antti,
> > 
> > On Sunday 16 March 2014 10:34:31 Antti Seppälä wrote:
> >> > +
> >> > +       /* all important bits of scancode should be set in mask */
> >> > +       if (~scancode->mask & 0x2fff)
> >> 
> >> Do we want to be so restrictive here? In my opinion it's quite nice to
> >> be able to encode also the toggle bit if needed. Therefore a check
> >> against 0x3fff would be a better choice.
> >> 
> >> I think the ability to encode toggle bit might also be nice to have
> >> for rc-5(x) also.
> > 
> > I don't believe the toggle bit is encoded in the scancode though, so I'm
> > not sure it makes sense to treat it like that. I'm not an expert on RC-5
> > like protocols or the use of the toggle bit though.
> 
> Well I'm not an expert either but at least streamzap tends to have the
> toggle bit enabled quite often when sending ir pulses.
> 
> When decoding the toggle is always removed from the scancode but when
> encoding it would be useful to have the possibility to encode it in.
> This is because setting the toggle bit into wakeup makes it easier to
> wake the system with nuvoton hw as it is difficult to press the remote
> key short time enough (less than around 112ms) to generate a pulse
> without the toggle bit set.

Fair enough. So changing the minimum rc5-sz masks to 0x3fff sounds reasonable 
to allow toggle to be controlled.

Just to clarify though, so you mean that the remote uses toggle=1 first (and 
in repeat codes) unless you press it a second time (new keypress) within a 
short amount of time?
I.e. like this?
Press	message toggle=1
		repeat toggle=1
		repeat toggle=1
unpress
Press	message toggle=!last_toggle only if within X ms, 1 otherwise

Sounds like for RC-5/RC-5X toggle should probably be taken from 0x00002000, 
0x00200000 of scancode respectively (just above "system" in both cases).

Cheers
James
  

On 16 March 2014 23:18, James Hogan <james@albanarts.com> wrote:
> Fair enough. So changing the minimum rc5-sz masks to 0x3fff sounds reasonable
> to allow toggle to be controlled.
>
> Just to clarify though, so you mean that the remote uses toggle=1 first (and
> in repeat codes) unless you press it a second time (new keypress) within a
> short amount of time?
> I.e. like this?
> Press   message toggle=1
>                 repeat toggle=1
>                 repeat toggle=1
> unpress
> Press   message toggle=!last_toggle only if within X ms, 1 otherwise
>

Actually studying this a little closer it seems that it indeed behaves
like a "toggle":

Press   message toggle=1
                repeat toggle=1
                repeat toggle=1
unpress
Press   message toggle=!last_toggle, always

So the toggle is inverted between presses and its value is kept during
repeat. It however seems to behave a little bit sporadically here
tending to set the toggle bit on more often than off.

Anyway I think that allowing the toggle bit to be set in the scancode
does not really hurt. I guess most of the time people will use the
scancodes without the toggle bit.

Br,
-Antti
--
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