Re: [PATCH v3 1/2] next-kbd: convert to use qemu_input_handler_register()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Nov 04, 2024 at 10:46:45PM +0000, Mark Cave-Ayland wrote:
> On 04/11/2024 10:04, Daniel P. Berrangé wrote:
> 
> > On Fri, Nov 01, 2024 at 08:11:05PM +0000, Mark Cave-Ayland wrote:
> > > 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 | 166 ++++++++++++++++++++++++++++++---------------
> > >   1 file changed, 111 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c
> > > index bc67810f31..283e98e9eb 100644
> > > --- a/hw/m68k/next-kbd.c
> > > +++ b/hw/m68k/next-kbd.c
> > 
> > 
> > > -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
> > 
> > The original code used '0' (or equivalently 0x00) to
> > indicate an unmapped scancode.
> > 
> > > +#define NEXTKBD_NO_KEY 0xff
> > 
> > Now you're using 0xff for missing mappings, but....
> > 
> > > +
> > > +static const int qcode_to_nextkbd_keycode[] = {
> > > +    /* Make sure future additions are automatically set to NEXTKBD_NO_KEY */
> > > +    [0 ... Q_KEY_CODE__MAX]    = NEXTKBD_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,
> > 
> > ...you've left 0x00 here and....
> 
> Ooops yes that line can be removed.
> 
> > > +    [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,
> 
> This should be kept.
> 
> > > +    [Q_KEY_CODE_BACKSLASH]     = 0x00,
> > 
> > ...here, and ...
> 
> Indeed, that line can also be removed.
> 
> > > +    [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,
> > 
> > ...here, which is surely not a correct conversion
> 
> And this one should also be kept.
> 
> The reason the two shift keys need to be kept as zero is so that they pass
> the "if (keycode == NEXTKBD_NO_KEY) { return; }" check in nextkbd_event().

IMHO that code sould just be flipped in ordering. Do the
special case

 if (qcode == Q_KEY_CODE_SHIFT_R)

handling before even running the table lookup, since the
lookup is redundant, at which point you don't need to
have 2 distinct special values in the lookup table.


> > I'm going to see about adding NeXT scancodes to the giant
> > database of keycodes at:
> > 
> >    https://gitlab.com/keycodemap/keycodemapdb
> > 
> > then we can auto-generate this table as we do for most of
> > the other QEMU keyboard drivers.
> 
> That would be great! Is this also possible for the ADB keyboard device,
> since that is where I looked for inspiration when looking at next-kbd?

Yes, that's likely possible - we've already got data in keycodemapdb for
Apple ADB


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|