Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver

[Sean Young <sean@mess.org>]

Hi Marko,

On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote:
> I finally took the time to get a deeper understanding of the infrared remote
> control subsystem. I think that I now understand the translation into
> key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat()
> will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will
> handle the repeat. For lirc_scancode_event() it will never set the
> LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does
> support the toggle bit. That might qualify as a bug.

You are right, this was missed. Patches welcome.

> Sat, Feb 12, 2022 at 04:32:19PM +0000, Sean Young wrote:
> > This device presents an IR buffer, which can be read and cleared.
> > Clearing the buffer is racey with receiving IR, so wait until the IR
> > message is finished before clearing it.
> > 
> > This should minimize the chance of the buffer clear happening while
> > IR is being received, although we cannot avoid this completely.
> 
> I just realized that this limitation of the interface may be causing exactly
> what I was observing when I was testing this. If a constant stream of data
> is being received because a button is being held down, a buffer overflow or
> wrap-around glitch is inevitable, maybe expect if the wrap-around occurs
> exactly at the 128-byte boundary.
> 
> How about the following improvement? If IR_RX_BC is a simple cursor to the
> 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending
> refresh_tab[] but simply remember where the previous call left off. We could
> always read the 128 bytes at IR_RX_BUF, and process everything between the
> previous position reported by IR_RX_BC and the current position reported by
> IR_RX_BC, and treat buf[] as a ring buffer.

This is a great idea. Very original.

> Last time I tested it, the patch was a significant improvement. I think that
> "perfect" is the enemy of "good enough", and the patch should be included in
> the kernel.

The idea sounds really good. I'll review/test the patch and get back to you.

Thanks,

Sean