On Mon, Aug 07, 2017 at 03:31:24PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> The 'Press and Hold' operation was not correctly implemented, in
> particular the requirement that the repeat doesn't start until
> the second identical keypress arrives. The REP_DELAY value also
> had to be adjusted (see the comment in the code) to achieve the
> desired behavior.
I'm afraid I've caused some confusion; I had not read your last message
about autorepeat on irc correctly, when I said "exactly".
So if the input layer has not received a key up event after a key down
event, after REP_DELAY it will generate another key down event every
REP_PERIOD. So for example, here I'm holding a button on a rc-5 device
for some time. Comments on lines with parentheses.
# ir-keytable -t
Testing events. Please, press CTRL-C to abort.
1502138577.703695: event type EV_MSC(0x04): scancode = 0x1e11
(each time a driver receives something, scancode is reported.)
1502138577.703695: event type EV_KEY(0x01) key_down: KEY_VOLUMEDOWN(0x0072)
1502138577.703695: event type EV_SYN(0x00).
1502138577.817682: event type EV_MSC(0x04): scancode = 0x1e11
1502138577.817682: event type EV_SYN(0x00).
(rc-5 repeats the command after 115ms).
1502138577.930676: event type EV_MSC(0x04): scancode = 0x1e11
1502138577.930676: event type EV_SYN(0x00).
1502138578.044682: event type EV_MSC(0x04): scancode = 0x1e11
1502138578.044682: event type EV_SYN(0x00).
1502138578.181690: event type EV_MSC(0x04): scancode = 0x1e11
1502138578.181690: event type EV_SYN(0x00).
1502138578.205667: event type EV_KEY(0x01) key_down: KEY_VOLUMEDOWN(0x0072)
(this is 500ms after the initial key down, so this key down is generated
by the input layer).
1502138578.205667: event type EV_SYN(0x00).
1502138578.333667: event type EV_KEY(0x01) key_down: KEY_VOLUMEDOWN(0x0072)
(this is 500 + 125 ms, so another key down event generated by input layer).
1502138578.333667: event type EV_SYN(0x00).
1502138578.437662: event type EV_KEY(0x01) key_up: KEY_VOLUMEDOWN(0x0072)
1502138578.437662: event type EV_SYN(0x00).
(key up generated by rc-core after 250ms after last scancode received)
So I think the autorepeat can do exactly what you want, without cec
having any special code for it.
On a side note, here you can see that for rc-5 the IR_KEYPRESS_TIMEOUT
should be 115ms (with a little extra margin).
My apologies.
Sean
>
> The 'enabled_protocols' field was also never set, fix that too. Since
> CEC is a fixed protocol the driver has to set this field.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/cec/cec-adap.c | 56 ++++++++++++++++++++++++++++++++++++++++----
> drivers/media/cec/cec-core.c | 13 ++++++++++
> include/media/cec.h | 5 ++++
> 3 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
> index 1a021828c8d4..6a2f38f000e8 100644
> --- a/drivers/media/cec/cec-adap.c
> +++ b/drivers/media/cec/cec-adap.c
> @@ -1766,6 +1766,9 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> int la_idx = cec_log_addr2idx(adap, dest_laddr);
> bool from_unregistered = init_laddr == 0xf;
> struct cec_msg tx_cec_msg = { };
> +#ifdef CONFIG_MEDIA_CEC_RC
> + int scancode;
> +#endif
>
> dprintk(2, "%s: %*ph\n", __func__, msg->len, msg->msg);
>
> @@ -1854,11 +1857,9 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> */
> case 0x60:
> if (msg->len == 2)
> - rc_keydown(adap->rc, RC_TYPE_CEC,
> - msg->msg[2], 0);
> + scancode = msg->msg[2];
> else
> - rc_keydown(adap->rc, RC_TYPE_CEC,
> - msg->msg[2] << 8 | msg->msg[3], 0);
> + scancode = msg->msg[2] << 8 | msg->msg[3];
> break;
> /*
> * Other function messages that are not handled.
> @@ -1871,11 +1872,54 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> */
> case 0x56: case 0x57:
> case 0x67: case 0x68: case 0x69: case 0x6a:
> + scancode = -1;
> break;
> default:
> - rc_keydown(adap->rc, RC_TYPE_CEC, msg->msg[2], 0);
> + scancode = msg->msg[2];
> + break;
> + }
> +
> + /* Was repeating, but keypress timed out */
> + if (adap->rc_repeating && !adap->rc->keypressed) {
> + adap->rc_repeating = false;
> + adap->rc_last_scancode = -1;
> + }
> + /* Different keypress from last time, ends repeat mode */
> + if (adap->rc_last_scancode != scancode) {
> + rc_keyup(adap->rc);
> + adap->rc_repeating = false;
> + }
> + /* We can't handle this scancode */
> + if (scancode < 0) {
> + adap->rc_last_scancode = scancode;
> + break;
> + }
> +
> + /* Send key press */
> + rc_keydown(adap->rc, RC_TYPE_CEC, scancode, 0);
> +
> + /* When in repeating mode, we're done */
> + if (adap->rc_repeating)
> + break;
> +
> + /*
> + * We are not repeating, but the new scancode is
> + * the same as the last one, and this second key press is
> + * within 550 ms (the 'Follower Safety Timeout') from the
> + * previous key press, so we now enable the repeating mode.
> + */
> + if (adap->rc_last_scancode == scancode &&
> + msg->rx_ts - adap->rc_last_keypress < 550 * NSEC_PER_MSEC) {
> + adap->rc_repeating = true;
> break;
> }
> + /*
> + * Not in repeating mode, so avoid triggering repeat mode
> + * by calling keyup.
> + */
> + rc_keyup(adap->rc);
> + adap->rc_last_scancode = scancode;
> + adap->rc_last_keypress = msg->rx_ts;
> #endif
> break;
>
> @@ -1885,6 +1929,8 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> break;
> #ifdef CONFIG_MEDIA_CEC_RC
> rc_keyup(adap->rc);
> + adap->rc_repeating = false;
> + adap->rc_last_scancode = -1;
> #endif
> break;
>
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index 52f085ba104a..018a95cae6b0 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -276,9 +276,11 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> adap->rc->input_id.version = 1;
> adap->rc->driver_name = CEC_NAME;
> adap->rc->allowed_protocols = RC_BIT_CEC;
> + adap->rc->enabled_protocols = RC_BIT_CEC;
> adap->rc->priv = adap;
> adap->rc->map_name = RC_MAP_CEC;
> adap->rc->timeout = MS_TO_NS(100);
> + adap->rc_last_scancode = -1;
> #endif
> return adap;
> }
> @@ -310,6 +312,17 @@ int cec_register_adapter(struct cec_adapter *adap,
> adap->rc = NULL;
> return res;
> }
> + /*
> + * The REP_DELAY for CEC is really the time between the initial
> + * 'User Control Pressed' message and the second. The first
> + * keypress is always seen as non-repeating, the second
> + * (provided it has the same UI Command) will start the 'Press
> + * and Hold' (aka repeat) behavior. By setting REP_DELAY to the
> + * same value as REP_PERIOD the expected CEC behavior is
> + * reproduced.
> + */
> + adap->rc->input_dev->rep[REP_DELAY] =
> + adap->rc->input_dev->rep[REP_PERIOD];
> }
> #endif
>
> diff --git a/include/media/cec.h b/include/media/cec.h
> index 224a6e225c52..be3b243a0d5e 100644
> --- a/include/media/cec.h
> +++ b/include/media/cec.h
> @@ -187,6 +187,11 @@ struct cec_adapter {
>
> u32 tx_timeouts;
>
> +#ifdef CONFIG_MEDIA_CEC_RC
> + bool rc_repeating;
> + int rc_last_scancode;
> + u64 rc_last_keypress;
> +#endif
> #ifdef CONFIG_CEC_NOTIFIER
> struct cec_notifier *notifier;
> #endif
> --
> 2.13.2
