Re: [Qemu-devel] [PATCH] Enhance sendkey with key hold time - v2

[Anthony Liguori <anthony@codemonkey.ws>]

Jan Kiszka wrote:
> [ Addressing reviewer comments ]
>
> Current key injection via the monitor basically generates no key hold
> time. This is fine for keyboard emulations that have their own queues,
> but it causes troubles for those how don't (like the MusicPal - it
> simply does not work with injected keys). Moreover, I would like to use
> this mechanism to simulate pressed buttons during power-up.
>
> Therefore, this patch enhances the key injection with a configurable
> release delay (by default 100 virtual milliseconds).
>
> This feature allows to get rid of the initial sleep() in musicpal_init
> because one can now simply start qemu with -S and issue "sendkey m 1000"
> and "continue" in the monitor to achieve the desired effect of a pressed
> menu button during power-up. So there is no need for a per-musicpal or
> even qemu-wide "-hold-button" switch.
>   

Looks good to me.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>  hw/musicpal.c |    6 ------
>  monitor.c     |   58 +++++++++++++++++++++++++++++++++++++++-------------------
>  2 files changed, 39 insertions(+), 25 deletions(-)
>
> Index: b/hw/musicpal.c
> ===================================================================
> --- a/hw/musicpal.c
> +++ b/hw/musicpal.c
> @@ -1504,12 +1504,6 @@ static void musicpal_init(ram_addr_t ram
>  
>      qemu_add_kbd_event_handler(musicpal_key_event, pic[MP_GPIO_IRQ]);
>  
> -    /*
> -     * Wait a bit to catch menu button during U-Boot start-up
> -     * (to trigger emergency update).
> -     */
> -    sleep(1);
> -
>      mv88w8618_eth_init(&nd_table[0], MP_ETH_BASE, pic[MP_ETH_IRQ]);
>  
>      mixer_i2c = musicpal_audio_init(MP_AUDIO_BASE, pic[MP_AUDIO_IRQ]);
> Index: b/monitor.c
> ===================================================================
> --- a/monitor.c
> +++ b/monitor.c
> @@ -35,10 +35,7 @@
>  #include "audio/audio.h"
>  #include "disas.h"
>  #include <dirent.h>
> -
> -#ifdef CONFIG_PROFILER
> -#include "qemu-timer.h" /* for ticks_per_sec */
> -#endif
> +#include "qemu-timer.h"
>  
>  //#define DEBUG
>  //#define DEBUG_COMPLETION
> @@ -920,14 +917,37 @@ static int get_keycode(const char *key)
>      return -1;
>  }
>  
> -static void do_sendkey(const char *string)
> +#define MAX_KEYCODES 16
> +static uint8_t keycodes[MAX_KEYCODES];
> +static int nb_pending_keycodes;
> +static QEMUTimer *key_timer;
> +
> +static void release_keys(void *opaque)
> +{
> +    int keycode;
> +
> +    while (nb_pending_keycodes > 0) {
> +        nb_pending_keycodes--;
> +        keycode = keycodes[nb_pending_keycodes];
> +        if (keycode & 0x80)
> +            kbd_put_keycode(0xe0);
> +        kbd_put_keycode(keycode | 0x80);
> +    }
> +}
> +
> +static void do_sendkey(const char *string, int has_hold_time, int hold_time)
>  {
> -    uint8_t keycodes[16];
> -    int nb_keycodes = 0;
>      char keyname_buf[16];
>      char *separator;
>      int keyname_len, keycode, i;
>  
> +    if (nb_pending_keycodes > 0) {
> +        qemu_del_timer(key_timer);
> +        release_keys(NULL);
> +    }
> +    if (!has_hold_time)
> +        hold_time = 100;
> +    i = 0;
>      while (1) {
>          separator = strchr(string, '-');
>          keyname_len = separator ? separator - string : strlen(string);
> @@ -937,7 +957,7 @@ static void do_sendkey(const char *strin
>                  term_printf("invalid key: '%s...'\n", keyname_buf);
>                  return;
>              }
> -            if (nb_keycodes == sizeof(keycodes)) {
> +            if (i == MAX_KEYCODES) {
>                  term_printf("too many keys\n");
>                  return;
>              }
> @@ -947,26 +967,23 @@ static void do_sendkey(const char *strin
>                  term_printf("unknown key: '%s'\n", keyname_buf);
>                  return;
>              }
> -            keycodes[nb_keycodes++] = keycode;
> +            keycodes[i++] = keycode;
>          }
>          if (!separator)
>              break;
>          string = separator + 1;
>      }
> +    nb_pending_keycodes = i;
>      /* key down events */
> -    for(i = 0; i < nb_keycodes; i++) {
> +    for (i = 0; i < nb_pending_keycodes; i++) {
>          keycode = keycodes[i];
>          if (keycode & 0x80)
>              kbd_put_keycode(0xe0);
>          kbd_put_keycode(keycode & 0x7f);
>      }
> -    /* key up events */
> -    for(i = nb_keycodes - 1; i >= 0; i--) {
> -        keycode = keycodes[i];
> -        if (keycode & 0x80)
> -            kbd_put_keycode(0xe0);
> -        kbd_put_keycode(keycode | 0x80);
> -    }
> +    /* delayed key up events */
> +    qemu_mod_timer(key_timer,
> +                   qemu_get_clock(vm_clock) + ticks_per_sec * hold_time);
>  }
>  
>  static int mouse_button_state;
> @@ -1353,8 +1370,8 @@ static term_cmd_t term_cmds[] = {
>      { "i", "/ii.", do_ioport_read,
>        "/fmt addr", "I/O port read" },
>  
> -    { "sendkey", "s", do_sendkey,
> -      "keys", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1')" },
> +    { "sendkey", "si?", do_sendkey,
> +      "keys [hold_ms]", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1', default hold time=100 ms)" },
>      { "system_reset", "", do_system_reset,
>        "", "reset the system" },
>      { "system_powerdown", "", do_system_powerdown,
> @@ -2638,6 +2655,9 @@ void monitor_init(CharDriverState *hd, i
>      int i;
>  
>      if (is_first_init) {
> +        key_timer = qemu_new_timer(vm_clock, release_keys, NULL);
> +        if (!key_timer)
> +            return;
>          for (i = 0; i < MAX_MON; i++) {
>              monitor_hd[i] = NULL;
>          }
>
>
>