[PATCH] media: rc: Always report LIRC repeat flag

[PATCH] media: rc: Always report LIRC repeat flag

[Marko Mäkelä]

The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
Previously it was only set by rc_repeat(), but not all protocol
decoders invoke that function.

Signed-off-by: Marko Mäkelä <marko.makela@iki.fi>
---
 drivers/media/rc/rc-main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index b90438a71c80..d914197245eb 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
 			  dev->last_toggle   != toggle);
 	struct lirc_scancode sc = {
 		.scancode = scancode, .rc_proto = protocol,
-		.flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
+		.flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
+		(!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
 		.keycode = keycode
 	};
 
-- 
2.36.1

Re: [PATCH] media: rc: Always report LIRC repeat flag

[Sean Young]

Hi Marko,

On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
> The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
> Previously it was only set by rc_repeat(), but not all protocol
> decoders invoke that function.

This should say _why_ you are making this change, not _what_ the change
is. See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

This also applies the first line of the commit.

Thanks
Sean

> 
> Signed-off-by: Marko Mäkelä <marko.makela@iki.fi>
> ---
>  drivers/media/rc/rc-main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index b90438a71c80..d914197245eb 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
>  			  dev->last_toggle   != toggle);
>  	struct lirc_scancode sc = {
>  		.scancode = scancode, .rc_proto = protocol,
> -		.flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
> +		.flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
> +		(!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
>  		.keycode = keycode
>  	};
>  
> -- 
> 2.36.1

Re: [PATCH] media: rc: Always report LIRC repeat flag

[Marko Mäkelä]

Hi Sean,

Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
>On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
>> The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
>> Previously it was only set by rc_repeat(), but not all protocol
>> decoders invoke that function.
>
>This should say _why_ you are making this change, not _what_ the change
>is.

How would you find the following?

---
media: lirc: ensure lirc device receives repeats

Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in 
the LIRC messages.

Commit b66218fddfd29f315a103db811152ab0c95fb054
("media: lirc: ensure lirc device receives nec repeats") fixed it up for
those protocol drivers that may call rc_repeat().
---

Would you prefer to be mentioned as a co-developer?

Best regards,

	Marko

Re: [PATCH] media: rc: Always report LIRC repeat flag

[Sean Young]

Hi Marko,

On Wed, Jul 06, 2022 at 07:39:17PM +0300, Marko Mäkelä wrote:
> Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
> > On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
> > > The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
> > > Previously it was only set by rc_repeat(), but not all protocol
> > > decoders invoke that function.
> > 
> > This should say _why_ you are making this change, not _what_ the change
> > is.
> 
> How would you find the following?
> 
> ---
> media: lirc: ensure lirc device receives repeats
> 
> Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
> reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in the
> LIRC messages.
> 
> Commit b66218fddfd29f315a103db811152ab0c95fb054
> ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
> those protocol drivers that may call rc_repeat().
> ---

That's no good. See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

The heading is called "Describe your changes".


Sean

Re: [PATCH] media: rc: Always report LIRC repeat flag

[Marko Mäkelä]

Hi Sean,

Thu, Jul 07, 2022 at 09:57:10AM +0100, Sean Young wrote:
>Hi Marko,
>
>On Wed, Jul 06, 2022 at 07:39:17PM +0300, Marko Mäkelä wrote:
>> Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
>> > On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
>> > > The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
>> > > Previously it was only set by rc_repeat(), but not all protocol
>> > > decoders invoke that function.
>> >
>> > This should say _why_ you are making this change, not _what_ the change
>> > is.
>>
>> How would you find the following?
>>
>> ---
>> media: lirc: ensure lirc device receives repeats
>>
>> Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
>> reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in the
>> LIRC messages.
>>
>> Commit b66218fddfd29f315a103db811152ab0c95fb054
>> ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
>> those protocol drivers that may call rc_repeat().
>> ---
>
>That's no good. See:
>
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
>The heading is called "Describe your changes".

I see. A quick read of "git log --oneline drivers/media/rc" suggests 
that the first line of the commit message is expected to be a summary 
_what_ the change is, not _why_ it was made. Would the commit message be 
acceptable after adding a "why" part right after the heading line, like 
this? If not, I would appreciate specific suggestions.

---
media: lirc: ensure lirc device receives repeats

For remote controls using RC5 and similar protocols that include a
"toggle" flag, the LIRC device never set the "repeat" flag to distinguish
repeated messages that were sent several times per second due to a
long keypress, and messages sent due to repeated short keypresses.

While a user-space program may implement logic around the "toggle" flag
to distinguish long keypresses, it would be simpler to be able to rely 
on the "repeat" flag for any type of protocol.

Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in
the LIRC messages.

Commit b66218fddfd29f315a103db811152ab0c95fb054
("media: lirc: ensure lirc device receives nec repeats") fixed it up for
those protocol drivers that may call rc_repeat().
---

Best regards,

	Marko

Re: [PATCH] media: rc: Always report LIRC repeat flag

[Sean Young]

Hi Marko,

On Thu, Jul 07, 2022 at 02:09:49PM +0300, Marko Mäkelä wrote:
> Thu, Jul 07, 2022 at 09:57:10AM +0100, Sean Young wrote:
> > On Wed, Jul 06, 2022 at 07:39:17PM +0300, Marko Mäkelä wrote:
> > > Tue, Jul 05, 2022 at 06:43:55PM +0100, Sean Young wrote:
> > > > On Tue, Jul 05, 2022 at 11:53:58AM +0300, Marko Mäkelä wrote:
> > > > > The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
> > > > > Previously it was only set by rc_repeat(), but not all protocol
> > > > > decoders invoke that function.
> > > >
> > > > This should say _why_ you are making this change, not _what_ the change
> > > > is.
> > > 
> > > How would you find the following?
> > > 
> > > ---
> > > media: lirc: ensure lirc device receives repeats
> > > 
> > > Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
> > > reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in the
> > > LIRC messages.
> > > 
> > > Commit b66218fddfd29f315a103db811152ab0c95fb054
> > > ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
> > > those protocol drivers that may call rc_repeat().
> > > ---
> > 
> > That's no good. See:
> > 
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> > 
> > The heading is called "Describe your changes".
> 
> I see. A quick read of "git log --oneline drivers/media/rc" suggests that
> the first line of the commit message is expected to be a summary _what_ the
> change is, not _why_ it was made. Would the commit message be acceptable
> after adding a "why" part right after the heading line, like this? If not, I
> would appreciate specific suggestions.

This is much better, thank you.

> ---
> media: lirc: ensure lirc device receives repeats
> 
> For remote controls using RC5 and similar protocols that include a
> "toggle" flag, the LIRC device never set the "repeat" flag to distinguish
> repeated messages that were sent several times per second due to a
> long keypress, and messages sent due to repeated short keypresses.
> 
> While a user-space program may implement logic around the "toggle" flag
> to distinguish long keypresses, it would be simpler to be able to rely on
> the "repeat" flag for any type of protocol.

I'm not sure how relevant the toggle is. This change is relevant for all 
protocols that do not use rc_repeat() and simply repeat the original
message when a key is being held down. This includes the sony protocol, 
imon, and the nec protocol (in case the remote does *not* use the repeat
message).

> Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
> reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag in
> the LIRC messages.
> 
> Commit b66218fddfd29f315a103db811152ab0c95fb054
> ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
> those protocol drivers that may call rc_repeat().
> ---

Thanks

Sean

[PATCH] media: lirc: ensure lirc device receives repeats

[Marko Mäkelä]

Pressing a button on a remote control unit will typically lead to
messages being sent several times per second until the button is released.

Some remote control units indicate long key presses by sending
special "repeat" messages, for which the protocol driver calls
rc_repeat(). Other units repeat the same message over and over,
which will be handled by calling rc_keydown().

The function rc_keydown() never set the LIRC "repeat" flag to distinguish
repeated messages that were sent due to a long keypress, and messages
sent due to repeated short keypresses. While a user-space program may
implement special logic to distinguish long keypresses, it is much simpler
to be able to rely on the flag.

Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag.
Commit b66218fddfd29f315a103db811152ab0c95fb054
("media: lirc: ensure lirc device receives nec repeats") fixed it up for
rc_repeat() but not rc_keydown().

Signed-off-by: Marko Mäkelä <marko.makela@iki.fi>
Co-developed-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/rc-main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index b90438a71c80..d914197245eb 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
 			  dev->last_toggle   != toggle);
 	struct lirc_scancode sc = {
 		.scancode = scancode, .rc_proto = protocol,
-		.flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
+		.flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
+		(!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
 		.keycode = keycode
 	};
 
-- 
2.36.1

Re: [PATCH] media: lirc: ensure lirc device receives repeats

[Sean Young]

On Fri, Jul 08, 2022 at 11:44:59AM +0300, Marko Mäkelä wrote:
> Pressing a button on a remote control unit will typically lead to
> messages being sent several times per second until the button is released.
> 
> Some remote control units indicate long key presses by sending
> special "repeat" messages, for which the protocol driver calls
> rc_repeat(). Other units repeat the same message over and over,
> which will be handled by calling rc_keydown().
> 
> The function rc_keydown() never set the LIRC "repeat" flag to distinguish
> repeated messages that were sent due to a long keypress, and messages
> sent due to repeated short keypresses. While a user-space program may
> implement special logic to distinguish long keypresses, it is much simpler
> to be able to rely on the flag.
> 
> Commit de142c32410649e64d44928505ffad2176a96a9e ("media: lirc: implement
> reading scancode") would never set the LIRC_SCANCODE_FLAG_REPEAT flag.
> Commit b66218fddfd29f315a103db811152ab0c95fb054
> ("media: lirc: ensure lirc device receives nec repeats") fixed it up for
> rc_repeat() but not rc_keydown().

Perfect, thanks.

Sean

> 
> Signed-off-by: Marko Mäkelä <marko.makela@iki.fi>
> Co-developed-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/rc-main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index b90438a71c80..d914197245eb 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
>  			  dev->last_toggle   != toggle);
>  	struct lirc_scancode sc = {
>  		.scancode = scancode, .rc_proto = protocol,
> -		.flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
> +		.flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
> +		(!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
>  		.keycode = keycode
>  	};
>  
> -- 
> 2.36.1