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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5317 bytes --]

andrzej zaborowski wrote:
> On 22/05/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>> 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.
>>
>>  Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>>  ---
>>   hw/musicpal.c |    6 ------
>>   monitor.c     |   57 ++++++++++++++++++++++++++++++++++++++-------------------
>>   2 files changed, 38 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
>>  @@ -925,14 +922,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) {
>>  +        keycode = keycodes[nb_pending_keycodes-1];
>>  +        if (keycode & 0x80)
>>  +            kbd_put_keycode(0xe0);
>>  +        kbd_put_keycode(keycode | 0x80);
>>  +        nb_pending_keycodes--;
>>  +    }
>>  +}
>>  +
>>  +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);
>>  @@ -943,7 +963,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;
>>              }
>>  @@ -953,26 +973,22 @@ 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) + 1000000LL*hold_time);
> 
> This should use ticks_per_sec.

Will fix.

> 
> Also, 100ms seems high for the default value, I think you rarely press
> keys for that long.

That's what I measured for human-typed keys in the monitor, so I simply
applied that value to keep things consistent for kbd consumers.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]