Re: [PATCH] next-kbd: convert to use qemu_input_handler_register()

[Thomas Huth <huth@tuxfamily.org>]

Am Wed, 30 Oct 2024 09:18:03 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler()
> function to use qemu_input_handler_register().
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-kbd.c | 158 +++++++++++++++++++++++++++++----------------
>  1 file changed, 103 insertions(+), 55 deletions(-)
> 
> diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c
> index bc67810f31..85ef784491 100644
> --- a/hw/m68k/next-kbd.c
> +++ b/hw/m68k/next-kbd.c
> @@ -68,7 +68,6 @@ struct NextKBDState {
>      uint16_t shift;
>  };
>  
> -static void queue_code(void *opaque, int code);
>  
>  /* lots of magic numbers here */
>  static uint32_t kbd_read_byte(void *opaque, hwaddr addr)
> @@ -166,68 +165,79 @@ static const MemoryRegionOps kbd_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static void nextkbd_event(void *opaque, int ch)
> -{
> -    /*
> -     * Will want to set vars for caps/num lock
> -     * if (ch & 0x80) -> key release
> -     * there's also e0 escaped scancodes that might need to be handled
> -     */
> -    queue_code(opaque, ch);
> -}
> -
> -static const unsigned char next_keycodes[128] = {
> -    0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F,
> -    0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00,
> -    0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06,
> -    0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A,
> -    0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C,
> -    0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34,
> -    0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00,
> -    0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
> +#define NEXTKDB_NO_KEY 0xff
> +
> +static const int qcode_to_nextkbd_keycode[] = {
> +    /* Make sure future additions are automatically set to NEXTKDB_NO_KEY */
> +    [0 ... 0xff]               = NEXTKDB_NO_KEY,

I think it would be better to use Q_KEY_CODE__MAX here instead of 0xff ?

> +    [Q_KEY_CODE_ESC]           = 0x49,
> +    [Q_KEY_CODE_1]             = 0x4a,
> +    [Q_KEY_CODE_2]             = 0x4b,
> +    [Q_KEY_CODE_3]             = 0x4c,
> +    [Q_KEY_CODE_4]             = 0x4d,
> +    [Q_KEY_CODE_5]             = 0x50,
> +    [Q_KEY_CODE_6]             = 0x4f,
> +    [Q_KEY_CODE_7]             = 0x4e,
> +    [Q_KEY_CODE_8]             = 0x1e,
> +    [Q_KEY_CODE_9]             = 0x1f,
> +    [Q_KEY_CODE_0]             = 0x20,
> +    [Q_KEY_CODE_MINUS]         = 0x1d,
> +    [Q_KEY_CODE_EQUAL]         = 0x1c,
> +    [Q_KEY_CODE_BACKSPACE]     = 0x1b,
> +    [Q_KEY_CODE_TAB]           = 0x00,
> +
> +    [Q_KEY_CODE_Q]             = 0x42,
> +    [Q_KEY_CODE_W]             = 0x43,
> +    [Q_KEY_CODE_E]             = 0x44,
> +    [Q_KEY_CODE_R]             = 0x45,
> +    [Q_KEY_CODE_T]             = 0x48,
> +    [Q_KEY_CODE_Y]             = 0x47,
> +    [Q_KEY_CODE_U]             = 0x46,
> +    [Q_KEY_CODE_I]             = 0x06,
> +    [Q_KEY_CODE_O]             = 0x07,
> +    [Q_KEY_CODE_P]             = 0x08,
> +    [Q_KEY_CODE_RET]           = 0x2a,
> +    [Q_KEY_CODE_A]             = 0x39,
> +    [Q_KEY_CODE_S]             = 0x3a,
> +
> +    [Q_KEY_CODE_D]             = 0x3b,
> +    [Q_KEY_CODE_F]             = 0x3c,
> +    [Q_KEY_CODE_G]             = 0x3d,
> +    [Q_KEY_CODE_H]             = 0x40,
> +    [Q_KEY_CODE_J]             = 0x3f,
> +    [Q_KEY_CODE_K]             = 0x3e,
> +    [Q_KEY_CODE_L]             = 0x2d,
> +    [Q_KEY_CODE_SEMICOLON]     = 0x2c,
> +    [Q_KEY_CODE_APOSTROPHE]    = 0x2b,
> +    [Q_KEY_CODE_GRAVE_ACCENT]  = 0x26,
> +    [Q_KEY_CODE_SHIFT]         = 0x00,
> +    [Q_KEY_CODE_BACKSLASH]     = 0x00,
> +    [Q_KEY_CODE_Z]             = 0x31,
> +    [Q_KEY_CODE_X]             = 0x32,
> +    [Q_KEY_CODE_C]             = 0x33,
> +    [Q_KEY_CODE_V]             = 0x34,
> +
> +    [Q_KEY_CODE_B]             = 0x35,
> +    [Q_KEY_CODE_N]             = 0x37,
> +    [Q_KEY_CODE_M]             = 0x36,
> +    [Q_KEY_CODE_COMMA]         = 0x2e,
> +    [Q_KEY_CODE_DOT]           = 0x2f,
> +    [Q_KEY_CODE_SLASH]         = 0x30,
> +    [Q_KEY_CODE_SHIFT_R]       = 0x00,
> +
> +    [Q_KEY_CODE_SPC]           = 0x38,
>  };
>  
> -static void queue_code(void *opaque, int code)
> +static void nextkbd_put_keycode(NextKBDState *s, int keycode)
>  {
> -    NextKBDState *s = NEXTKBD(opaque);
>      KBDQueue *q = &s->queue;
> -    int key = code & KD_KEYMASK;
> -    int release = code & 0x80;
> -    static int ext;
> -
> -    if (code == 0xE0) {
> -        ext = 1;
> -    }
> -
> -    if (code == 0x2A || code == 0x1D || code == 0x36) {
> -        if (code == 0x2A) {
> -            s->shift = KD_LSHIFT;
> -        } else if (code == 0x36) {
> -            s->shift = KD_RSHIFT;
> -            ext = 0;
> -        } else if (code == 0x1D && !ext) {
> -            s->shift = KD_LCOMM;
> -        } else if (code == 0x1D && ext) {
> -            ext = 0;
> -            s->shift = KD_RCOMM;
> -        }
> -        return;
> -    } else if (code == (0x2A | 0x80) || code == (0x1D | 0x80) ||
> -               code == (0x36 | 0x80)) {
> -        s->shift = 0;
> -        return;
> -    }
>  
>      if (q->count >= KBD_QUEUE_SIZE) {
>          return;
>      }
>  
> -    q->data[q->wptr] = next_keycodes[key] | release;
> -
> +    q->data[q->wptr] = keycode;
>      if (++q->wptr == KBD_QUEUE_SIZE) {
>          q->wptr = 0;
>      }
> @@ -241,6 +251,44 @@ static void queue_code(void *opaque, int code)
>      /* s->update_irq(s->update_arg, 1); */
>  }
>  
> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, InputEvent *evt)
> +{
> +    NextKBDState *s = NEXTKBD(dev);
> +    int qcode, keycode;
> +    bool key_down = evt->u.key.data->down;
> +
> +    qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key);
> +    if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) {
> +        return;
> +    }
> +
> +    keycode = qcode_to_nextkbd_keycode[qcode];
> +    if (keycode == NEXTKDB_NO_KEY) {
> +        return;
> +    }
> +
> +    if (qcode == Q_KEY_CODE_SHIFT) {
> +        s->shift = key_down ? KD_LSHIFT : 0;
> +    }
> +
> +    if (qcode == Q_KEY_CODE_SHIFT_R) {
> +        s->shift = key_down ? KD_RSHIFT : 0;
> +    }

This does not work properly when you press both shift keys together, e.g.
press the left shift key first and keep it pressed, then shortly press the
right shift key and release it, then type some letters while you still hold
down the left shift key.

I think you should rather treat the shift value like a bitfield here, e.g.:

    if (qcode == Q_KEY_CODE_SHIFT) {
        if (key_down) {
            s->shift |= KD_LSHIFT;
        } else {
            s->shift &= ~KD_LSHIFT;
        }
    }

    if (qcode == Q_KEY_CODE_SHIFT_R) {
        if (key_down) {
            s->shift |= KD_RSHIFT;
        } else {
            s->shift &= ~KD_RSHIFT;
        }
    }

 Thomas

> +    /* If key release event, create keyboard break code */
> +    if (!key_down) {
> +        keycode = keycode | 0x80;
> +    }
> +
> +    nextkbd_put_keycode(s, keycode);
> +}
> +
> +static const QemuInputHandler nextkbd_handler = {
> +    .name  = "QEMU NeXT Keyboard",
> +    .mask  = INPUT_EVENT_MASK_KEY,
> +    .event = nextkbd_event,
> +};
> +
>  static void nextkbd_reset(DeviceState *dev)
>  {
>      NextKBDState *nks = NEXTKBD(dev);
> @@ -256,7 +304,7 @@ static void nextkbd_realize(DeviceState *dev, Error **errp)
>      memory_region_init_io(&s->mr, OBJECT(dev), &kbd_ops, s, "next.kbd", 0x1000);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
>  
> -    qemu_add_kbd_event_handler(nextkbd_event, s);
> +    qemu_input_handler_register(dev, &nextkbd_handler);
>  }
>  
>  static const VMStateDescription nextkbd_vmstate = {