| Message ID | 8768A4C2-CEBF-411A-9B43-9F43EA755238@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11 March 2016 at 09:32, Programmingkid <programmingkidx@gmail.com> wrote: > Remove the old pc_to_adb_keycode array and replace it with QKeyCode support. > > Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > --- > Some of the keys do not translate as logically as we would think they would. For > example the Q_KEY_CODE_CTRL_R does not work with ADB_KEY_RIGHT_CONTROL. The > wrong key would show up in the guest. These problem keys are commmented out and > replaced with the number that does work correctly. This patch can be easily > tested with the Linux command xev or Mac OS's Key Caps. I'm not sure what you mean here. If you press right-control on the host then shouldn't this correspond to right-control on the guest ? > /* debug ADB */ > //#define DEBUG_ADB > @@ -59,6 +62,9 @@ do { printf("ADB: " fmt , ## __VA_ARGS__); } while (0) > /* error codes */ > #define ADB_RET_NOTPRESENT (-2) > > +/* The adb keyboard doesn't have every key imaginable */ > +#define NO_KEY 0xff > + > static void adb_device_reset(ADBDevice *d) > { > qdev_reset_all(DEVICE(d)); > @@ -187,23 +193,138 @@ 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] = 123, /* ADB_KEY_RIGHT_SHIFT, */ These should definitely be using some ADB_KEY_* constant on the RHS, not a decimal constant. > + [Q_KEY_CODE_ALT] = ADB_KEY_LEFT_OPTION, > + [Q_KEY_CODE_ALT_R] = 124, /* ADB_KEY_RIGHT_OPTION,*/ > + [Q_KEY_CODE_ALTGR] = ADB_KEY_RIGHT_OPTION, > + [Q_KEY_CODE_CTRL] = 54, /* ADB_KEY_LEFT_CONTROL, */ > + [Q_KEY_CODE_CTRL_R] = 125, /* ADB_KEY_RIGHT_CONTROL, */ > + [Q_KEY_CODE_META_L] = ADB_KEY_LEFT_COMMAND, > + > + /* 126 works as right super in Linux */ > + /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */ > + [Q_KEY_CODE_META_R] = ADB_KEY_LEFT_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] = ADB_KEY_F13, > + [Q_KEY_CODE_SYSRQ] = ADB_KEY_F13, > + [Q_KEY_CODE_SCROLL_LOCK] = ADB_KEY_F14, > + [Q_KEY_CODE_PAUSE] = ADB_KEY_F15, > + [Q_KEY_CODE_POWER] = ADB_KEY_POWER, > + > + [Q_KEY_CODE_NUM_LOCK] = ADB_KEY_KP_CLEAR, > + [Q_KEY_CODE_KP_EQUALS] = ADB_KEY_KP_EQUAL, > + [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] = 62, /* ADB_KEY_UP, */ > + [Q_KEY_CODE_DOWN] = 61, /* ADB_KEY_DOWN, */ > + [Q_KEY_CODE_LEFT] = 59, /* ADB_KEY_LEFT, */ > + [Q_KEY_CODE_RIGHT] = 60, /* ADB_KEY_RIGHT, */ > + > + [Q_KEY_CODE_HELP] = ADB_KEY_HELP, > + [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] = NO_KEY, > + [Q_KEY_CODE_STOP] = NO_KEY, > + [Q_KEY_CODE_AGAIN] = NO_KEY, > + [Q_KEY_CODE_PROPS] = NO_KEY, > + [Q_KEY_CODE_UNDO] = NO_KEY, > + [Q_KEY_CODE_FRONT] = NO_KEY, > + [Q_KEY_CODE_COPY] = NO_KEY, > + [Q_KEY_CODE_OPEN] = NO_KEY, > + [Q_KEY_CODE_PASTE] = NO_KEY, > + [Q_KEY_CODE_FIND] = NO_KEY, > + [Q_KEY_CODE_CUT] = NO_KEY, > + [Q_KEY_CODE_LF] = NO_KEY, > + [Q_KEY_CODE_COMPOSE] = NO_KEY, This is a little bit fragile, because if somebody adds a new Q_KEY_CODE later then its array entry won't be NO_KEY. Instead you can use a GCC extension: as the first thing in this array you put [ 0 ... Q_KEY_CODE__MAX ] = NO_KEY, which sets a default value for the whole array, and then after that you put all the [Q_KEY_CODE_WHATEVER] = ADB_KEY_THING, entries. (For instance we do this with the 'map' array in hw/arm/z2.c.) > }; > > static void adb_kbd_put_keycode(void *opaque, int keycode) > @@ -220,35 +341,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 olen; olen is always 0 here so why not just return 0 ? > + } > + keycode = s->data[s->rptr]; > + if (++s->rptr == sizeof(s->data)) { > + s->rptr = 0; > } > + s->count--; > + > + obuf[0] = keycode; You are still trying to put a two byte keycode (ADB_KEY_POWER) into this one-byte array slot. I don't know what the right way to send a two-byte keycode is but this is obviously not it, as I said before. > + /* NOTE: could put a second keycode if needed */ > + obuf[1] = 0xff; > + olen = 2; > + > return olen; > } > > @@ -313,6 +424,24 @@ static int adb_kbd_request(ADBDevice *d, uint8_t *obuf, > return olen; > } > > +/* The 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; > + > + qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); I'm curious why you added this wakeup_request call -- does anything in the machines that use the ADB keyboard support suspending the machine? > + qcode = qemu_input_key_value_to_qcode(evt->u.key->key); > + keycode = qcode_to_adb_keycode[qcode]; You should really have a check here that qcode is < the array size before you dereference into it. > + > + if (evt->u.key->down == false) { /* if key up event */ > + keycode = keycode | 0x80; /* create keyboard break code */ > + } > + > + adb_kbd_put_keycode(s, keycode); > +} thanks -- PMM
On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: > On 11 March 2016 at 09:32, Programmingkid <programmingkidx@gmail.com> wrote: >> Remove the old pc_to_adb_keycode array and replace it with QKeyCode support. >> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >> --- >> Some of the keys do not translate as logically as we would think they would. For >> example the Q_KEY_CODE_CTRL_R does not work with ADB_KEY_RIGHT_CONTROL. The >> wrong key would show up in the guest. These problem keys are commmented out and >> replaced with the number that does work correctly. This patch can be easily >> tested with the Linux command xev or Mac OS's Key Caps. > > I'm not sure what you mean here. If you press right-control on the host > then shouldn't this correspond to right-control on the guest ? It should. It makes logical sense. But when I tried it using a Mac OS X and Linux guest, the wrong key would be pressed. The theories I have are incorrect keyboard detection to CUDA translation problems. >> /* debug ADB */ >> //#define DEBUG_ADB >> @@ -59,6 +62,9 @@ do { printf("ADB: " fmt , ## __VA_ARGS__); } while (0) >> /* error codes */ >> #define ADB_RET_NOTPRESENT (-2) >> >> +/* The adb keyboard doesn't have every key imaginable */ >> +#define NO_KEY 0xff >> + >> static void adb_device_reset(ADBDevice *d) >> { >> qdev_reset_all(DEVICE(d)); >> @@ -187,23 +193,138 @@ 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] = 123, /* ADB_KEY_RIGHT_SHIFT, */ > > These should definitely be using some ADB_KEY_* constant on > the RHS, not a decimal constant. Ok. It would look something like this: [Q_KEY_CODE_SHIFT_R] = ADB_KEY_LEFT, It looks wrong, but it works. > >> + [Q_KEY_CODE_ALT] = ADB_KEY_LEFT_OPTION, >> + [Q_KEY_CODE_ALT_R] = 124, /* ADB_KEY_RIGHT_OPTION,*/ >> + [Q_KEY_CODE_ALTGR] = ADB_KEY_RIGHT_OPTION, >> + [Q_KEY_CODE_CTRL] = 54, /* ADB_KEY_LEFT_CONTROL, */ >> + [Q_KEY_CODE_CTRL_R] = 125, /* ADB_KEY_RIGHT_CONTROL, */ >> + [Q_KEY_CODE_META_L] = ADB_KEY_LEFT_COMMAND, >> + >> + /* 126 works as right super in Linux */ >> + /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */ >> + [Q_KEY_CODE_META_R] = ADB_KEY_LEFT_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] = ADB_KEY_F13, >> + [Q_KEY_CODE_SYSRQ] = ADB_KEY_F13, >> + [Q_KEY_CODE_SCROLL_LOCK] = ADB_KEY_F14, >> + [Q_KEY_CODE_PAUSE] = ADB_KEY_F15, >> + [Q_KEY_CODE_POWER] = ADB_KEY_POWER, >> + >> + [Q_KEY_CODE_NUM_LOCK] = ADB_KEY_KP_CLEAR, >> + [Q_KEY_CODE_KP_EQUALS] = ADB_KEY_KP_EQUAL, >> + [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] = 62, /* ADB_KEY_UP, */ >> + [Q_KEY_CODE_DOWN] = 61, /* ADB_KEY_DOWN, */ >> + [Q_KEY_CODE_LEFT] = 59, /* ADB_KEY_LEFT, */ >> + [Q_KEY_CODE_RIGHT] = 60, /* ADB_KEY_RIGHT, */ >> + >> + [Q_KEY_CODE_HELP] = ADB_KEY_HELP, >> + [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] = NO_KEY, >> + [Q_KEY_CODE_STOP] = NO_KEY, >> + [Q_KEY_CODE_AGAIN] = NO_KEY, >> + [Q_KEY_CODE_PROPS] = NO_KEY, >> + [Q_KEY_CODE_UNDO] = NO_KEY, >> + [Q_KEY_CODE_FRONT] = NO_KEY, >> + [Q_KEY_CODE_COPY] = NO_KEY, >> + [Q_KEY_CODE_OPEN] = NO_KEY, >> + [Q_KEY_CODE_PASTE] = NO_KEY, >> + [Q_KEY_CODE_FIND] = NO_KEY, >> + [Q_KEY_CODE_CUT] = NO_KEY, >> + [Q_KEY_CODE_LF] = NO_KEY, >> + [Q_KEY_CODE_COMPOSE] = NO_KEY, > > This is a little bit fragile, because if somebody adds a new > Q_KEY_CODE later then its array entry won't be NO_KEY. Instead > you can use a GCC extension: as the first thing in this > array you put > [ 0 ... Q_KEY_CODE__MAX ] = NO_KEY, > > which sets a default value for the whole array, > and then after that you put all the > [Q_KEY_CODE_WHATEVER] = ADB_KEY_THING, > entries. > > (For instance we do this with the 'map' array in hw/arm/z2.c.) Sounds good. > > >> }; >> >> static void adb_kbd_put_keycode(void *opaque, int keycode) >> @@ -220,35 +341,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 olen; > > olen is always 0 here so why not just return 0 ? ok. > >> + } >> + keycode = s->data[s->rptr]; >> + if (++s->rptr == sizeof(s->data)) { >> + s->rptr = 0; >> } >> + s->count--; >> + >> + obuf[0] = keycode; > > You are still trying to put a two byte keycode (ADB_KEY_POWER) > into this one-byte array slot. I don't know what the right way to > send a two-byte keycode is but this is obviously not it, as > I said before. I didn't notice a problem wit the power key but I think you want something like this: /* if using a two byte value */ if (keycode > 0xff) { obuf[0] = (keycode & 0xff00) >> 8; obuf[1] = keycode & 0x00ff; obuf[2] = 0xff; olen = 3; } return olen; > >> + /* NOTE: could put a second keycode if needed */ >> + obuf[1] = 0xff; >> + olen = 2; >> + >> return olen; >> } >> >> @@ -313,6 +424,24 @@ static int adb_kbd_request(ADBDevice *d, uint8_t *obuf, >> return olen; >> } >> >> +/* The 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; >> + >> + qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); > > I'm curious why you added this wakeup_request call -- does anything > in the machines that use the ADB keyboard support suspending the > machine? Gerd said to model my changes after the code found in a URL he gave me. It had that call in it. I do not know if anything supports suspending the Macintosh emulator, so I don't mind removing that call. > >> + qcode = qemu_input_key_value_to_qcode(evt->u.key->key); >> + keycode = qcode_to_adb_keycode[qcode]; > > You should really have a check here that qcode is < the array > size before you dereference into it. ok. Thank you very much for reviewing my code.
On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: > >> + } >> + keycode = s->data[s->rptr]; >> + if (++s->rptr == sizeof(s->data)) { >> + s->rptr = 0; >> } >> + s->count--; >> + >> + obuf[0] = keycode; > > You are still trying to put a two byte keycode (ADB_KEY_POWER) > into this one-byte array slot. I don't know what the right way to > send a two-byte keycode is but this is obviously not it, as > I said before. > >> + /* NOTE: could put a second keycode if needed */ >> + obuf[1] = 0xff; >> + olen = 2; >> + >> return olen; >> } Is this ok? /* The power key is the only two byte value key, so it is a special case. */ if (keycode == (ADB_KEY_POWER & 0x00ff)) { obuf[0] = ADB_KEY_POWER & 0x00ff; obuf[1] = ADB_KEY_POWER & 0xff00 >> 8; olen = 2; } else { obuf[0] = keycode; /* NOTE: could put a second keycode if needed */ obuf[1] = 0xff; olen = 2; } The keycode value comes from an 8 bit array so holding the full value of the power key is not possible. That is the reason for the "if (keycode == (ADB_KEY_POWER & 0x00ff))". The code might be a little more efficient if we did this: /* The power key is the only two byte value key, so it is a special case. */ if (keycode == 0x7f) { obuf[0] = 0x7f; obuf[1] = 0x7f; olen = 2; } else { obuf[0] = keycode; /* NOTE: could put a second keycode if needed */ obuf[1] = 0xff; olen = 2; } The speed difference isn't noticeable so either way works well.
On 12 March 2016 at 05:40, Programmingkid <programmingkidx@gmail.com> wrote: > > On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: > >> >>> + } >>> + keycode = s->data[s->rptr]; >>> + if (++s->rptr == sizeof(s->data)) { >>> + s->rptr = 0; >>> } >>> + s->count--; >>> + >>> + obuf[0] = keycode; >> >> You are still trying to put a two byte keycode (ADB_KEY_POWER) >> into this one-byte array slot. I don't know what the right way to >> send a two-byte keycode is but this is obviously not it, as >> I said before. >> >>> + /* NOTE: could put a second keycode if needed */ >>> + obuf[1] = 0xff; >>> + olen = 2; >>> + >>> return olen; >>> } > > Is this ok? > > /* The power key is the only two byte value key, so it is a special case. */ > if (keycode == (ADB_KEY_POWER & 0x00ff)) { > obuf[0] = ADB_KEY_POWER & 0x00ff; > obuf[1] = ADB_KEY_POWER & 0xff00 >> 8; > olen = 2; > } else { > obuf[0] = keycode; > /* NOTE: could put a second keycode if needed */ > obuf[1] = 0xff; > olen = 2; > } > > The keycode value comes from an 8 bit array so holding the > full value of the power key is not possible. Ah, I hadn't noticed that -- that is not a good approach. You should either: (a) deal with the fact that ADB_KEY values may be 16 bits all the way through (including having that array be uint16_t rather than uint8_t) (b) have the power key be a completely special case which is handled by if (qcode == Q_KEY_CODE_POWER) { /* put power key into buffer */ [...] } else { keycode = qcode_to_adb_keycode[...]; etc; } and not put it into the array at all. Also, you need to handle the power-key key-release scancode; as far as I can tell (by looking for info via google about the ADB protocol) power-key-down is 0x7f 0x7f, and power-key-up is 0xff 0xff. (This is kind of weird and suggests we should just take option (a) and special case the power key completely.) thanks -- PMM
On 12 March 2016 at 04:27, Programmingkid <programmingkidx@gmail.com> wrote: > > On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: > >> On 11 March 2016 at 09:32, Programmingkid <programmingkidx@gmail.com> wrote: >>> Remove the old pc_to_adb_keycode array and replace it with QKeyCode support. >>> >>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >>> --- >>> Some of the keys do not translate as logically as we would think they would. For >>> example the Q_KEY_CODE_CTRL_R does not work with ADB_KEY_RIGHT_CONTROL. The >>> wrong key would show up in the guest. These problem keys are commmented out and >>> replaced with the number that does work correctly. This patch can be easily >>> tested with the Linux command xev or Mac OS's Key Caps. >> >> I'm not sure what you mean here. If you press right-control on the host >> then shouldn't this correspond to right-control on the guest ? > > It should. It makes logical sense. But when I tried it using a Mac OS X and > Linux guest, the wrong key would be pressed. The theories I have are > incorrect keyboard detection to CUDA translation problems. > >>> + [Q_KEY_CODE_SHIFT] = ADB_KEY_LEFT_SHIFT, >>> + [Q_KEY_CODE_SHIFT_R] = 123, /* ADB_KEY_RIGHT_SHIFT, */ >> >> These should definitely be using some ADB_KEY_* constant on >> the RHS, not a decimal constant. > > Ok. It would look something like this: > [Q_KEY_CODE_SHIFT_R] = ADB_KEY_LEFT, I think we definitely need to figure out what is going on here. Sending the key-left code for right-shift is definitely wrong. (Presumably this also implies that the actual left-arrow key is broken...) Possibly relatedly, the Apple Extended Keyboard apparently won't send the separate keycodes for right-shift, right-option, right-control until the guest OS sends the keyboard a command to enable them. (see http://www.archive.org/stream/apple-guide-macintosh-family-hardware/Apple_Guide_to_the_Macintosh_Family_Hardware_2e#page/n347/mode/2up page 309). I suggest that for this patchset you leave the code so that it continues to send the same ADB keycodes for these keys that it has done before (whatever those are). Then once we've got the conversion to using qcodes in we can look at fixing this bug as a separate patch. (Similarly, you might want to split out the code to support the power key as a separate patch.) thanks -- PMM
On Mar 13, 2016, at 11:40 AM, Peter Maydell wrote: > On 12 March 2016 at 05:40, Programmingkid <programmingkidx@gmail.com> wrote: >> >> On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: >> >>> >>>> + } >>>> + keycode = s->data[s->rptr]; >>>> + if (++s->rptr == sizeof(s->data)) { >>>> + s->rptr = 0; >>>> } >>>> + s->count--; >>>> + >>>> + obuf[0] = keycode; >>> >>> You are still trying to put a two byte keycode (ADB_KEY_POWER) >>> into this one-byte array slot. I don't know what the right way to >>> send a two-byte keycode is but this is obviously not it, as >>> I said before. >>> >>>> + /* NOTE: could put a second keycode if needed */ >>>> + obuf[1] = 0xff; >>>> + olen = 2; >>>> + >>>> return olen; >>>> } >> >> Is this ok? >> >> /* The power key is the only two byte value key, so it is a special case. */ >> if (keycode == (ADB_KEY_POWER & 0x00ff)) { >> obuf[0] = ADB_KEY_POWER & 0x00ff; >> obuf[1] = ADB_KEY_POWER & 0xff00 >> 8; >> olen = 2; >> } else { >> obuf[0] = keycode; >> /* NOTE: could put a second keycode if needed */ >> obuf[1] = 0xff; >> olen = 2; >> } >> >> The keycode value comes from an 8 bit array so holding the >> full value of the power key is not possible. > > Ah, I hadn't noticed that -- that is not a good approach. > You should either: > (a) deal with the fact that ADB_KEY values may be 16 bits > all the way through (including having that array be uint16_t > rather than uint8_t) I did try this but a bunch of errors showed up. To see all the errors just make this change in adb.c: typedef struct KBDState { /*< private >*/ ADBDevice parent_obj; /*< public >*/ uint16_t data[128]; <----- was uint8_t int rptr, wptr, count; } KBDState; The errors: /include/migration/vmstate.h:248:48: error: invalid operands to binary - (have 'uint8_t (*)[256]' and 'uint16_t (*)[128]') #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0) /include/migration/vmstate.h:261:6: note: in expansion of macro 'type_check_array' type_check_array(_type, typeof_field(_state, _field), _num)) So I'm not sure now changing the type to 16 bit is the best thing to do. It would require a lot more changes to other files.
On 13 March 2016 at 16:39, Programmingkid <programmingkidx@gmail.com> wrote: > I did try this but a bunch of errors showed up. > /include/migration/vmstate.h:248:48: error: invalid operands to binary - (have 'uint8_t (*)[256]' and 'uint16_t (*)[128]') > #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0) > > /include/migration/vmstate.h:261:6: note: in expansion of macro 'type_check_array' > type_check_array(_type, typeof_field(_state, _field), _num)) > > So I'm not sure now changing the type to 16 bit is the best thing to do. It would require a lot more changes to other files. Right, you would need to also change the migration state to say the array was 16 bit. (This is a migration compat break, so awkward anyway.) I made a typo in that email which unfortunately completely reversed the meaning -- I meant to say "we should just take option (b)"... thanks -- PMM
On Mar 13, 2016, at 11:40 AM, Peter Maydell wrote: > On 12 March 2016 at 05:40, Programmingkid <programmingkidx@gmail.com> wrote: >> >> On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: >> >>> >>>> + } >>>> + keycode = s->data[s->rptr]; >>>> + if (++s->rptr == sizeof(s->data)) { >>>> + s->rptr = 0; >>>> } >>>> + s->count--; >>>> + >>>> + obuf[0] = keycode; >>> >>> You are still trying to put a two byte keycode (ADB_KEY_POWER) >>> into this one-byte array slot. I don't know what the right way to >>> send a two-byte keycode is but this is obviously not it, as >>> I said before. >>> >>>> + /* NOTE: could put a second keycode if needed */ >>>> + obuf[1] = 0xff; >>>> + olen = 2; >>>> + >>>> return olen; >>>> } >> >> Is this ok? >> >> /* The power key is the only two byte value key, so it is a special case. */ >> if (keycode == (ADB_KEY_POWER & 0x00ff)) { >> obuf[0] = ADB_KEY_POWER & 0x00ff; >> obuf[1] = ADB_KEY_POWER & 0xff00 >> 8; >> olen = 2; >> } else { >> obuf[0] = keycode; >> /* NOTE: could put a second keycode if needed */ >> obuf[1] = 0xff; >> olen = 2; >> } >> >> The keycode value comes from an 8 bit array so holding the >> full value of the power key is not possible. > > Ah, I hadn't noticed that -- that is not a good approach. > You should either: > (a) deal with the fact that ADB_KEY values may be 16 bits > all the way through (including having that array be uint16_t > rather than uint8_t) > (b) have the power key be a completely special case which > is handled by > if (qcode == Q_KEY_CODE_POWER) { > /* put power key into buffer */ > [...] > } else { > keycode = qcode_to_adb_keycode[...]; > etc; > } > and not put it into the array at all. > > Also, you need to handle the power-key key-release > scancode; as far as I can tell (by looking for info via > google about the ADB protocol) power-key-down is > 0x7f 0x7f, and power-key-up is 0xff 0xff. (This is > kind of weird and suggests we should just take option > (a) and special case the power key completely.) The power-key-up (break) keycode is 0xff 0xff? Could you send me your source for this information? I always thought the up (break) code was the keycode AND with 0x80. Or this if code is easier to understand: break_keycode = keycode & 0x80.
On Mar 14, 2016, at 7:06 PM, Programmingkid wrote: > > On Mar 13, 2016, at 11:40 AM, Peter Maydell wrote: > >> On 12 March 2016 at 05:40, Programmingkid <programmingkidx@gmail.com> wrote: >>> >>> On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: >>> >>>> >>>>> + } >>>>> + keycode = s->data[s->rptr]; >>>>> + if (++s->rptr == sizeof(s->data)) { >>>>> + s->rptr = 0; >>>>> } >>>>> + s->count--; >>>>> + >>>>> + obuf[0] = keycode; >>>> >>>> You are still trying to put a two byte keycode (ADB_KEY_POWER) >>>> into this one-byte array slot. I don't know what the right way to >>>> send a two-byte keycode is but this is obviously not it, as >>>> I said before. >>>> >>>>> + /* NOTE: could put a second keycode if needed */ >>>>> + obuf[1] = 0xff; >>>>> + olen = 2; >>>>> + >>>>> return olen; >>>>> } >>> >>> Is this ok? >>> >>> /* The power key is the only two byte value key, so it is a special case. */ >>> if (keycode == (ADB_KEY_POWER & 0x00ff)) { >>> obuf[0] = ADB_KEY_POWER & 0x00ff; >>> obuf[1] = ADB_KEY_POWER & 0xff00 >> 8; >>> olen = 2; >>> } else { >>> obuf[0] = keycode; >>> /* NOTE: could put a second keycode if needed */ >>> obuf[1] = 0xff; >>> olen = 2; >>> } >>> >>> The keycode value comes from an 8 bit array so holding the >>> full value of the power key is not possible. >> >> Ah, I hadn't noticed that -- that is not a good approach. >> You should either: >> (a) deal with the fact that ADB_KEY values may be 16 bits >> all the way through (including having that array be uint16_t >> rather than uint8_t) >> (b) have the power key be a completely special case which >> is handled by >> if (qcode == Q_KEY_CODE_POWER) { >> /* put power key into buffer */ >> [...] >> } else { >> keycode = qcode_to_adb_keycode[...]; >> etc; >> } >> and not put it into the array at all. >> >> Also, you need to handle the power-key key-release >> scancode; as far as I can tell (by looking for info via >> google about the ADB protocol) power-key-down is >> 0x7f 0x7f, and power-key-up is 0xff 0xff. (This is >> kind of weird and suggests we should just take option >> (a) and special case the power key completely.) > > The power-key-up (break) keycode is 0xff 0xff? Could you send me your source for this information? I always thought the up (break) code was the keycode AND with 0x80. Or this if code is easier to understand: > break_keycode = keycode & 0x80. * sorry I meant OR in the above code break_keycode = keycode | 0x80.
On 14 March 2016 at 23:06, Programmingkid <programmingkidx@gmail.com> wrote: > On Mar 13, 2016, at 11:40 AM, Peter Maydell wrote: >> Also, you need to handle the power-key key-release >> scancode; as far as I can tell (by looking for info via >> google about the ADB protocol) power-key-down is >> 0x7f 0x7f, and power-key-up is 0xff 0xff. (This is >> kind of weird and suggests we should just take option >> (a) and special case the power key completely.) > > The power-key-up (break) keycode is 0xff 0xff? Could you send me your > source for this information? https://developer.apple.com/legacy/library/technotes/hw/hw_01.html (section "Apple Keyboard Protocol"). Power-key is a weird one both for key-up and key-down. thanks -- PMM
On Mar 15, 2016, at 5:22 AM, Peter Maydell wrote: > On 14 March 2016 at 23:06, Programmingkid <programmingkidx@gmail.com> wrote: >> On Mar 13, 2016, at 11:40 AM, Peter Maydell wrote: >>> Also, you need to handle the power-key key-release >>> scancode; as far as I can tell (by looking for info via >>> google about the ADB protocol) power-key-down is >>> 0x7f 0x7f, and power-key-up is 0xff 0xff. (This is >>> kind of weird and suggests we should just take option >>> (a) and special case the power key completely.) >> >> The power-key-up (break) keycode is 0xff 0xff? Could you send me your >> source for this information? > > https://developer.apple.com/legacy/library/technotes/hw/hw_01.html > (section "Apple Keyboard Protocol"). > > Power-key is a weird one both for key-up and key-down. I did several tests and found out that the USB keyboard actually works a lot better than the ADB keyboard. Would you accept a patch that changes the default keyboard and mouse to USB for the Mac99 target? This target will always have USB available. USB peripherals are what the real counterpart uses.
On 15 March 2016 at 15:24, Programmingkid <programmingkidx@gmail.com> wrote: > I did several tests and found out that the USB keyboard actually > works a lot better than the ADB keyboard. Would you accept a patch > that changes the default keyboard and mouse to USB for the Mac99 > target? Not my call, ask the PPC maintainer. (Fixing the ADB keyboard is a useful cleanup anyway I think.) thanks -- PMM
diff --git a/hw/input/adb.c b/hw/input/adb.c index f0ad0d4..d176d39 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 @@ -59,6 +62,9 @@ do { printf("ADB: " fmt , ## __VA_ARGS__); } while (0) /* error codes */ #define ADB_RET_NOTPRESENT (-2) +/* The adb keyboard doesn't have every key imaginable */ +#define NO_KEY 0xff + static void adb_device_reset(ADBDevice *d) { qdev_reset_all(DEVICE(d)); @@ -187,23 +193,138 @@ 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] = 123, /* ADB_KEY_RIGHT_SHIFT, */ + [Q_KEY_CODE_ALT] = ADB_KEY_LEFT_OPTION, + [Q_KEY_CODE_ALT_R] = 124, /* ADB_KEY_RIGHT_OPTION,*/ + [Q_KEY_CODE_ALTGR] = ADB_KEY_RIGHT_OPTION, + [Q_KEY_CODE_CTRL] = 54, /* ADB_KEY_LEFT_CONTROL, */ + [Q_KEY_CODE_CTRL_R] = 125, /* ADB_KEY_RIGHT_CONTROL, */ + [Q_KEY_CODE_META_L] = ADB_KEY_LEFT_COMMAND, + + /* 126 works as right super in Linux */ + /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */ + [Q_KEY_CODE_META_R] = ADB_KEY_LEFT_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] = ADB_KEY_F13, + [Q_KEY_CODE_SYSRQ] = ADB_KEY_F13, + [Q_KEY_CODE_SCROLL_LOCK] = ADB_KEY_F14, + [Q_KEY_CODE_PAUSE] = ADB_KEY_F15, + [Q_KEY_CODE_POWER] = ADB_KEY_POWER, + + [Q_KEY_CODE_NUM_LOCK] = ADB_KEY_KP_CLEAR, + [Q_KEY_CODE_KP_EQUALS] = ADB_KEY_KP_EQUAL, + [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] = 62, /* ADB_KEY_UP, */ + [Q_KEY_CODE_DOWN] = 61, /* ADB_KEY_DOWN, */ + [Q_KEY_CODE_LEFT] = 59, /* ADB_KEY_LEFT, */ + [Q_KEY_CODE_RIGHT] = 60, /* ADB_KEY_RIGHT, */ + + [Q_KEY_CODE_HELP] = ADB_KEY_HELP, + [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] = NO_KEY, + [Q_KEY_CODE_STOP] = NO_KEY, + [Q_KEY_CODE_AGAIN] = NO_KEY, + [Q_KEY_CODE_PROPS] = NO_KEY, + [Q_KEY_CODE_UNDO] = NO_KEY, + [Q_KEY_CODE_FRONT] = NO_KEY, + [Q_KEY_CODE_COPY] = NO_KEY, + [Q_KEY_CODE_OPEN] = NO_KEY, + [Q_KEY_CODE_PASTE] = NO_KEY, + [Q_KEY_CODE_FIND] = NO_KEY, + [Q_KEY_CODE_CUT] = NO_KEY, + [Q_KEY_CODE_LF] = NO_KEY, + [Q_KEY_CODE_COMPOSE] = NO_KEY, }; static void adb_kbd_put_keycode(void *opaque, int keycode) @@ -220,35 +341,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 olen; + } + keycode = s->data[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; + return olen; } @@ -313,6 +424,24 @@ static int adb_kbd_request(ADBDevice *d, uint8_t *obuf, return olen; } +/* The 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; + + qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); + qcode = qemu_input_key_value_to_qcode(evt->u.key->key); + keycode = qcode_to_adb_keycode[qcode]; + + if (evt->u.key->down == false) { /* if key up 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 +471,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); d->devaddr = ADB_DEVID_KEYBOARD; }
Remove the old pc_to_adb_keycode array and replace it with QKeyCode support. Signed-off-by: John Arbuckle <programmingkidx@gmail.com> --- Some of the keys do not translate as logically as we would think they would. For example the Q_KEY_CODE_CTRL_R does not work with ADB_KEY_RIGHT_CONTROL. The wrong key would show up in the guest. These problem keys are commmented out and replaced with the number that does work correctly. This patch can be easily tested with the Linux command xev or Mac OS's Key Caps. hw/input/adb.c | 223 +++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 177 insertions(+), 46 deletions(-)