[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v4 2/5] adb.c: add support for QKeyCode
|
From: |
David Gibson |
|
Subject: |
Re: [Qemu-ppc] [PATCH v4 2/5] adb.c: add support for QKeyCode |
|
Date: |
Tue, 16 Aug 2016 13:46:41 +1000 |
|
User-agent: |
Mutt/1.6.2 (2016-07-01) |
On Mon, Aug 15, 2016 at 02:56:31PM -0400, Programmingkid wrote:
>
> On Aug 15, 2016, at 8:07 AM, David Gibson wrote:
>
> > On Fri, Aug 12, 2016 at 08:10:00PM -0400, John Arbuckle wrote:
> >> The old pc scancode translation is replaced with QEMU's QKeyCode.
> >>
> >> Signed-off-by: John Arbuckle <address@hidden>
> >> Reviewed-by: Peter Maydell <address@hidden>
> >> ---
> >> *v4 changes
> >> Replaced ADB_KEY_LEFT_COMMAND with ADB_KEY_COMMAND.
> >> Removed ADB_KEY_RIGHT_COMMAND comment.
> >>
> >> *v3 changes
> >> Kept original pc_to_adb_keycode mapping.
> >>
> >> *v2 changes
> >> Changed order of this patch.
> >>
> >> hw/input/adb.c | 219
> >> +++++++++++++++++++++++++++++++++++++++++++++------------
> >> 1 file changed, 173 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/hw/input/adb.c b/hw/input/adb.c
> >> index f0ad0d4..bbf0f44 100644
> >> --- a/hw/input/adb.c
> >> +++ b/hw/input/adb.c
> >> @@ -25,6 +25,9 @@
> >> #include "hw/hw.h"
> >> #include "hw/input/adb.h"
> >> #include "ui/console.h"
> >> +#include "include/hw/input/adb-keys.h"
> >> +#include "ui/input.h"
> >> +#include "sysemu/sysemu.h"
> >>
> >> /* debug ADB */
> >> //#define DEBUG_ADB
> >> @@ -187,23 +190,135 @@ typedef struct ADBKeyboardClass {
> >> DeviceRealize parent_realize;
> >> } ADBKeyboardClass;
> >>
> >> -static const uint8_t pc_to_adb_keycode[256] = {
> >> - 0, 53, 18, 19, 20, 21, 23, 22, 26, 28, 25, 29, 27, 24, 51, 48,
> >> - 12, 13, 14, 15, 17, 16, 32, 34, 31, 35, 33, 30, 36, 54, 0, 1,
> >> - 2, 3, 5, 4, 38, 40, 37, 41, 39, 50, 56, 42, 6, 7, 8, 9,
> >> - 11, 45, 46, 43, 47, 44,123, 67, 58, 49, 57,122,120, 99,118, 96,
> >> - 97, 98,100,101,109, 71,107, 89, 91, 92, 78, 86, 87, 88, 69, 83,
> >> - 84, 85, 82, 65, 0, 0, 10,103,111, 0, 0,110, 81, 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, 94, 0, 93, 0, 0, 0, 0, 0, 0,104,102, 0, 0,
> >> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 76,125, 0, 0,
> >> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,105, 0, 0, 0, 0, 0,
> >> - 0, 0, 0, 0, 0, 75, 0, 0,124, 0, 0, 0, 0, 0, 0, 0,
> >> - 0, 0, 0, 0, 0, 0, 0,115, 62,116, 0, 59, 0, 60, 0,119,
> >> - 61,121,114,117, 0, 0, 0, 0, 0, 0, 0, 55,126, 0,127, 0,
> >> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> >> - 0, 0, 0, 0, 0, 95, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> >> +int qcode_to_adb_keycode[] = {
> >> +
> >> + [Q_KEY_CODE_SHIFT] = ADB_KEY_LEFT_SHIFT,
> >> + [Q_KEY_CODE_SHIFT_R] = ADB_KEY_RIGHT_SHIFT,
> >> + [Q_KEY_CODE_ALT] = ADB_KEY_LEFT_OPTION,
> >> + [Q_KEY_CODE_ALT_R] = ADB_KEY_RIGHT_OPTION,
> >> + [Q_KEY_CODE_ALTGR] = 0,
> >
> > If I'm reading adb-keys.h correctly this maps AltGr to 'A', which
> > doesn't seem right.
>
> Peter Maydell wanted each logical set of changes to be its own patch. Do you
> want
> me to combine this patch (2/5) with the rest of the patches?
No - sorry, I didn't fully see what was going on here, and missed the
fact that the existing keymap already mapped a bunch of things to 'A'
weirdly.
Fixing up the mappings in a separate patch is better, but for the sake
of reviewers your commit message should say that this patch is
essentially just a mechanical substitution, which a number of broken
mappings left in.
>
> >
> >> + [Q_KEY_CODE_CTRL] = ADB_KEY_LEFT_CONTROL,
> >> + [Q_KEY_CODE_CTRL_R] = ADB_KEY_RIGHT_CONTROL,
> >> + [Q_KEY_CODE_META_L] = ADB_KEY_COMMAND,
> >> + [Q_KEY_CODE_META_R] = ADB_KEY_COMMAND,
> >> + [Q_KEY_CODE_SPC] = ADB_KEY_SPACEBAR,
> >> +
> >> + [Q_KEY_CODE_ESC] = ADB_KEY_ESC,
> >> + [Q_KEY_CODE_1] = ADB_KEY_1,
> >> + [Q_KEY_CODE_2] = ADB_KEY_2,
> >> + [Q_KEY_CODE_3] = ADB_KEY_3,
> >> + [Q_KEY_CODE_4] = ADB_KEY_4,
> >> + [Q_KEY_CODE_5] = ADB_KEY_5,
> >> + [Q_KEY_CODE_6] = ADB_KEY_6,
> >> + [Q_KEY_CODE_7] = ADB_KEY_7,
> >> + [Q_KEY_CODE_8] = ADB_KEY_8,
> >> + [Q_KEY_CODE_9] = ADB_KEY_9,
> >> + [Q_KEY_CODE_0] = ADB_KEY_0,
> >> + [Q_KEY_CODE_MINUS] = ADB_KEY_MINUS,
> >> + [Q_KEY_CODE_EQUAL] = ADB_KEY_EQUAL,
> >> + [Q_KEY_CODE_BACKSPACE] = ADB_KEY_DELETE,
> >> + [Q_KEY_CODE_TAB] = ADB_KEY_TAB,
> >> + [Q_KEY_CODE_Q] = ADB_KEY_Q,
> >> + [Q_KEY_CODE_W] = ADB_KEY_W,
> >> + [Q_KEY_CODE_E] = ADB_KEY_E,
> >> + [Q_KEY_CODE_R] = ADB_KEY_R,
> >> + [Q_KEY_CODE_T] = ADB_KEY_T,
> >> + [Q_KEY_CODE_Y] = ADB_KEY_Y,
> >> + [Q_KEY_CODE_U] = ADB_KEY_U,
> >> + [Q_KEY_CODE_I] = ADB_KEY_I,
> >> + [Q_KEY_CODE_O] = ADB_KEY_O,
> >> + [Q_KEY_CODE_P] = ADB_KEY_P,
> >> + [Q_KEY_CODE_BRACKET_LEFT] = ADB_KEY_LEFT_BRACKET,
> >> + [Q_KEY_CODE_BRACKET_RIGHT] = ADB_KEY_RIGHT_BRACKET,
> >> + [Q_KEY_CODE_RET] = ADB_KEY_RETURN,
> >> + [Q_KEY_CODE_A] = ADB_KEY_A,
> >> + [Q_KEY_CODE_S] = ADB_KEY_S,
> >> + [Q_KEY_CODE_D] = ADB_KEY_D,
> >> + [Q_KEY_CODE_F] = ADB_KEY_F,
> >> + [Q_KEY_CODE_G] = ADB_KEY_G,
> >> + [Q_KEY_CODE_H] = ADB_KEY_H,
> >> + [Q_KEY_CODE_J] = ADB_KEY_J,
> >> + [Q_KEY_CODE_K] = ADB_KEY_K,
> >> + [Q_KEY_CODE_L] = ADB_KEY_L,
> >> + [Q_KEY_CODE_SEMICOLON] = ADB_KEY_SEMICOLON,
> >> + [Q_KEY_CODE_APOSTROPHE] = ADB_KEY_APOSTROPHE,
> >> + [Q_KEY_CODE_GRAVE_ACCENT] = ADB_KEY_GRAVE_ACCENT,
> >> + [Q_KEY_CODE_BACKSLASH] = ADB_KEY_BACKSLASH,
> >> + [Q_KEY_CODE_Z] = ADB_KEY_Z,
> >> + [Q_KEY_CODE_X] = ADB_KEY_X,
> >> + [Q_KEY_CODE_C] = ADB_KEY_C,
> >> + [Q_KEY_CODE_V] = ADB_KEY_V,
> >> + [Q_KEY_CODE_B] = ADB_KEY_B,
> >> + [Q_KEY_CODE_N] = ADB_KEY_N,
> >> + [Q_KEY_CODE_M] = ADB_KEY_M,
> >> + [Q_KEY_CODE_COMMA] = ADB_KEY_COMMA,
> >> + [Q_KEY_CODE_DOT] = ADB_KEY_PERIOD,
> >> + [Q_KEY_CODE_SLASH] = ADB_KEY_FORWARD_SLASH,
> >> + [Q_KEY_CODE_ASTERISK] = ADB_KEY_KP_MULTIPLY,
> >> + [Q_KEY_CODE_CAPS_LOCK] = ADB_KEY_CAPS_LOCK,
> >> +
> >> + [Q_KEY_CODE_F1] = ADB_KEY_F1,
> >> + [Q_KEY_CODE_F2] = ADB_KEY_F2,
> >> + [Q_KEY_CODE_F3] = ADB_KEY_F3,
> >> + [Q_KEY_CODE_F4] = ADB_KEY_F4,
> >> + [Q_KEY_CODE_F5] = ADB_KEY_F5,
> >> + [Q_KEY_CODE_F6] = ADB_KEY_F6,
> >> + [Q_KEY_CODE_F7] = ADB_KEY_F7,
> >> + [Q_KEY_CODE_F8] = ADB_KEY_F8,
> >> + [Q_KEY_CODE_F9] = ADB_KEY_F9,
> >> + [Q_KEY_CODE_F10] = ADB_KEY_F10,
> >> + [Q_KEY_CODE_F11] = ADB_KEY_F11,
> >> + [Q_KEY_CODE_F12] = ADB_KEY_F12,
> >> + [Q_KEY_CODE_PRINT] = 0,
> >> + [Q_KEY_CODE_SYSRQ] = 0,
> >
> > Likewise here and various below.
> >
> >> + [Q_KEY_CODE_SCROLL_LOCK] = ADB_KEY_F14,
> >> + [Q_KEY_CODE_PAUSE] = 0,
> >> + [Q_KEY_CODE_NUM_LOCK] = ADB_KEY_KP_CLEAR,
> >> + [Q_KEY_CODE_KP_EQUALS] = 0,
> >> + [Q_KEY_CODE_KP_DIVIDE] = ADB_KEY_KP_DIVIDE,
> >> + [Q_KEY_CODE_KP_MULTIPLY] = ADB_KEY_KP_MULTIPLY,
> >> + [Q_KEY_CODE_KP_SUBTRACT] = ADB_KEY_KP_SUBTRACT,
> >> + [Q_KEY_CODE_KP_ADD] = ADB_KEY_KP_PLUS,
> >> + [Q_KEY_CODE_KP_ENTER] = ADB_KEY_KP_ENTER,
> >> + [Q_KEY_CODE_KP_DECIMAL] = ADB_KEY_KP_PERIOD,
> >> + [Q_KEY_CODE_KP_0] = ADB_KEY_KP_0,
> >> + [Q_KEY_CODE_KP_1] = ADB_KEY_KP_1,
> >> + [Q_KEY_CODE_KP_2] = ADB_KEY_KP_2,
> >> + [Q_KEY_CODE_KP_3] = ADB_KEY_KP_3,
> >> + [Q_KEY_CODE_KP_4] = ADB_KEY_KP_4,
> >> + [Q_KEY_CODE_KP_5] = ADB_KEY_KP_5,
> >> + [Q_KEY_CODE_KP_6] = ADB_KEY_KP_6,
> >> + [Q_KEY_CODE_KP_7] = ADB_KEY_KP_7,
> >> + [Q_KEY_CODE_KP_8] = ADB_KEY_KP_8,
> >> + [Q_KEY_CODE_KP_9] = ADB_KEY_KP_9,
> >> +
> >> + [Q_KEY_CODE_UP] = ADB_KEY_UP,
> >> + [Q_KEY_CODE_DOWN] = ADB_KEY_DOWN,
> >> + [Q_KEY_CODE_LEFT] = ADB_KEY_LEFT,
> >> + [Q_KEY_CODE_RIGHT] = ADB_KEY_RIGHT,
> >> +
> >> + [Q_KEY_CODE_HELP] = 0,
> >> + [Q_KEY_CODE_INSERT] = ADB_KEY_HELP,
> >> + [Q_KEY_CODE_DELETE] = ADB_KEY_FORWARD_DELETE,
> >> + [Q_KEY_CODE_HOME] = ADB_KEY_HOME,
> >> + [Q_KEY_CODE_END] = ADB_KEY_END,
> >> + [Q_KEY_CODE_PGUP] = ADB_KEY_PAGE_UP,
> >> + [Q_KEY_CODE_PGDN] = ADB_KEY_PAGE_DOWN,
> >> +
> >> + [Q_KEY_CODE_LESS] = 0xa,
> >
> > Why no ADB_KEY constant for this one?
>
> There is no 0xa constant.
>
> >
> >> + [Q_KEY_CODE_STOP] = 0,
> >> + [Q_KEY_CODE_AGAIN] = 0,
> >> + [Q_KEY_CODE_PROPS] = 0,
> >> + [Q_KEY_CODE_UNDO] = 0,
> >> + [Q_KEY_CODE_FRONT] = 0,
> >> + [Q_KEY_CODE_COPY] = 0,
> >> + [Q_KEY_CODE_OPEN] = 0,
> >> + [Q_KEY_CODE_PASTE] = 0,
> >> + [Q_KEY_CODE_FIND] = 0,
> >> + [Q_KEY_CODE_CUT] = 0,
> >> + [Q_KEY_CODE_LF] = 0,
> >> + [Q_KEY_CODE_COMPOSE] = 0,
> >> };
> >>
> >> static void adb_kbd_put_keycode(void *opaque, int keycode)
> >> @@ -220,35 +335,25 @@ static void adb_kbd_put_keycode(void *opaque, int
> >> keycode)
> >>
> >> static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
> >> {
> >> - static int ext_keycode;
> >> KBDState *s = ADB_KEYBOARD(d);
> >> - int adb_keycode, keycode;
> >> + int keycode;
> >> int olen;
> >>
> >> olen = 0;
> >> - for(;;) {
> >> - if (s->count == 0)
> >> - break;
> >> - keycode = s->data[s->rptr];
> >> - if (++s->rptr == sizeof(s->data))
> >> - s->rptr = 0;
> >> - s->count--;
> >> -
> >> - if (keycode == 0xe0) {
> >> - ext_keycode = 1;
> >> - } else {
> >> - if (ext_keycode)
> >> - adb_keycode = pc_to_adb_keycode[keycode | 0x80];
> >> - else
> >> - adb_keycode = pc_to_adb_keycode[keycode & 0x7f];
> >> - obuf[0] = adb_keycode | (keycode & 0x80);
> >> - /* NOTE: could put a second keycode if needed */
> >> - obuf[1] = 0xff;
> >> - olen = 2;
> >> - ext_keycode = 0;
> >> - break;
> >> - }
> >> + if (s->count == 0) {
> >> + return 0;
> >> + }
> >> + keycode = s->data[s->rptr];
> >> + s->rptr++;
> >> + if (s->rptr == sizeof(s->data)) {
> >> + s->rptr = 0;
> >> }
> >> + s->count--;
> >> + obuf[0] = keycode;
> >> + /* NOTE: could put a second keycode if needed */
> >> + obuf[1] = 0xff;
> >> + olen = 2;
> >> +
> >
> > You seem to be substantially changing the semantics of this function
> > (it's returning raw rather than translated keycodes), without changing
> > the name. This is usually a bad idea.
> >
> > More to the point I don't see any corresponding change in the callers,
> > which seems very odd.
> >
> >> return olen;
> >> }
> >>
> >> @@ -313,6 +418,26 @@ static int adb_kbd_request(ADBDevice *d, uint8_t
> >> *obuf,
> >> return olen;
> >> }
> >>
> >> +/* This is where keyboard events enter this file */
> >> +static void adb_keyboard_event(DeviceState *dev, QemuConsole *src,
> >> + InputEvent *evt)
> >> +{
> >> + KBDState *s = (KBDState *)dev;
> >> + int qcode, keycode;
> >> +
> >> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key);
> >> + if (qcode >= ARRAY_SIZE(qcode_to_adb_keycode)) {
> >> + return;
> >
> > It's probably a good idea to have a trace event here to debug
> > untranslatable keycodes.
>
> I'm not familiar with trace events. Is ADB_DPRINTF() ok?
Uh, I guess so. In general local dprintf()s are discouraged in favour
of trace events, but since ADB_DPRINTF() is already there, you might
as well use it.
Trace events are pretty straightforward though: put a signature and
format string into the trace-events file, then call it from your code
as 'trace_foo(x, y, z)' the existing makefiles will deal with the rest.
> >
> >> + }
> >> + keycode = qcode_to_adb_keycode[qcode];
> >> +
> >> + if (evt->u.key.data->down == false) { /* if key release event */
> >> + keycode = keycode | 0x80; /* create keyboard break code */
> >> + }
> >> +
> >> + adb_kbd_put_keycode(s, keycode);
> >> +}
> >> +
> >> static const VMStateDescription vmstate_adb_kbd = {
> >> .name = "adb_kbd",
> >> .version_id = 2,
> >> @@ -342,18 +467,20 @@ static void adb_kbd_reset(DeviceState *dev)
> >>
> >> static void adb_kbd_realizefn(DeviceState *dev, Error **errp)
> >> {
> >> - ADBDevice *d = ADB_DEVICE(dev);
> >> ADBKeyboardClass *akc = ADB_KEYBOARD_GET_CLASS(dev);
> >> -
> >> akc->parent_realize(dev, errp);
> >> -
> >> - qemu_add_kbd_event_handler(adb_kbd_put_keycode, d);
> >> }
> >>
> >> +static QemuInputHandler adb_keyboard_handler = {
> >> + .name = "QEMU ADB Keyboard",
> >> + .mask = INPUT_EVENT_MASK_KEY,
> >> + .event = adb_keyboard_event,
> >> +};
> >> +
> >> static void adb_kbd_initfn(Object *obj)
> >> {
> >> ADBDevice *d = ADB_DEVICE(obj);
> >> -
> >> + qemu_input_handler_register((DeviceState *)d, &adb_keyboard_handler);
> >
> > You've moved the handler registration from realize() to initfn() time,
> > which looks like a mistake. Generally your device shouldn't become
> > "active" until realize() time.
>
> The qemu_input_handler_register() function is dependent on the d variable. It
> is available in this function. The d variable is dependent on the obj
> variable. It is only available in
> this function. Moving the qemu_input_handler_register() function to the
> realize() function
> makes the code not compile. <Goes back and looks at adb_kbd_realizefn()>
> Never mind. The adb_kbd_realizefn() function does have a DeviceState pointer
> variable so moving things there would work.
Uh.. yeah, first statement doesn't make much sense. The ADBState is a
subclass of DeviceState is a subclass of Object, you can easily move
between them.
> >
> >> d->devaddr = ADB_DEVID_KEYBOARD;
> >> }
> >>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature