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

Mark Cave-Ayland posted 1 patch 2 days, 22 hours ago
There is a newer version of this series
hw/m68k/next-kbd.c | 158 +++++++++++++++++++++++++++++----------------
1 file changed, 103 insertions(+), 55 deletions(-)
[PATCH] next-kbd: convert to use qemu_input_handler_register()
Posted by Mark Cave-Ayland 2 days, 22 hours ago
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,
+
+    [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;
+    }
+
+    /* 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 = {
-- 
2.39.5
Re: [PATCH] next-kbd: convert to use qemu_input_handler_register()
Posted by Thomas Huth 22 hours ago
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 = {
Re: [PATCH] next-kbd: convert to use qemu_input_handler_register()
Posted by Mark Cave-Ayland 12 hours ago
On 01/11/2024 09:26, Thomas Huth wrote:

> 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 ?

Ah I wasn't aware of that, but it makes sense - I ended up using adb-kbd.c as a guide 
which is where the (NEXTKBD_)NO_KEY constant comes from.

>> +    [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;
>          }
>      }

Thanks for this! I mistook the checks for !ext and ext in the current version as 
being part of the scancode state machine, but as you point out they are in fact 
needed for the shift key logic to work correctly.

I prefer your version of the logic above using a bitfield, and making the change is 
easy since the device is currently marked as non-migratable.

I'll go ahead and make both these changes for v3.

>   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 = {


ATB,

Mark.
Re: [PATCH] next-kbd: convert to use qemu_input_handler_register()
Posted by Alex Bennée 2 days, 21 hours ago
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler()
> function to use qemu_input_handler_register().

If that is the last user we should probably remove the function as well.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH] next-kbd: convert to use qemu_input_handler_register()
Posted by Mark Cave-Ayland 1 day, 22 hours ago
On 30/10/2024 10:28, Alex Bennée wrote:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> 
>> Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler()
>> function to use qemu_input_handler_register().
> 
> If that is the last user we should probably remove the function as well.

True. I can send a follow-up patch to do this later unless anyone beats me to it.


ATB,

Mark.