On Fri, 8 Nov 2024, Philippe Mathieu-Daudé wrote:
On 8/11/24 13:13, BALATON Zoltan wrote:On Fri, 8 Nov 2024, Thomas Huth wrote:On 06/11/2024 21.32, BALATON Zoltan wrote:On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote:On 6/11/24 13:00, BALATON Zoltan wrote:On Wed, 6 Nov 2024, 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-ayl...@ilande.co.uk> Reviewed-by: Thomas Huth <h...@tuxfamily.org> --- hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- 1 file changed, 108 insertions(+), 55 deletions(-)-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 NEXTKBD_NO_KEY 0xffNow you don't need this 0xff define any more because you can use 0 as no key value then the [0 ... Q_KEY_CODE__MAX] init below can also be dropped because static variables are 0 init automatically.Whether 0 or 0xff is best for NO_KEY, I don't know. However, definitions are useful when reviewing ...Regards, BALATON Zoltan+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,[...]+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; + } + + /* Shift key currently has no keycode, so handle separately */ + 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; + } + } + + keycode = qcode_to_nextkbd_keycode[qcode]; + if (keycode == NEXTKBD_NO_KEY) {... here ^I this case !keycode is pretty self explanatory IMO.Ok, I'll pick up the patch with this change added on top: diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c --- a/hw/m68k/next-kbd.c +++ b/hw/m68k/next-kbd.c @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -#define NEXTKBD_NO_KEY 0xff - 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,Thinking about it more, removing this may make the array smaller so we'd either need some max value define (or get it something like qcode_to_nextkbd_keycode[ARRAY_SIZE(qcode_to_nextkbd_keycode) - 1] or so) and check if qcode is not > than that or declare the array as [Q_KEY_CODE__MAX] to make sure we're not trying to access values after the end. Maybe it's simplest to do qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] as this is not much wasted space, unless this can't overflow for some other reason I don't know about.Agreed, qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] is future-proof.
Actually there's already a check for array size where it's used but I've only noticed that after writing this so it should be OK without enlarging the array.
Regards, BALATON Zoltan
