From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47328) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeb9M-0008E7-AU for qemu-devel@nongnu.org; Fri, 11 Mar 2016 23:27:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aeb9J-0007VV-1q for qemu-devel@nongnu.org; Fri, 11 Mar 2016 23:27:36 -0500 Received: from mail-ig0-x243.google.com ([2607:f8b0:4001:c05::243]:36031) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeb9I-0007VM-QA for qemu-devel@nongnu.org; Fri, 11 Mar 2016 23:27:32 -0500 Received: by mail-ig0-x243.google.com with SMTP id ir4so1014616igb.3 for ; Fri, 11 Mar 2016 20:27:32 -0800 (PST) Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Programmingkid In-Reply-To: Date: Fri, 11 Mar 2016 23:27:26 -0500 Content-Transfer-Encoding: quoted-printable Message-Id: References: <8768A4C2-CEBF-411A-9B43-9F43EA755238@gmail.com> Subject: Re: [Qemu-devel] [PATCH v4 4/4] hw/input/adb.c: implement QKeyCode support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Gerd Hoffmann , qemu-devel qemu-devel On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote: > On 11 March 2016 at 09:32, Programmingkid = wrote: >> Remove the old pc_to_adb_keycode array and replace it with QKeyCode = support. >>=20 >> Signed-off-by: John Arbuckle >> --- >> 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. >=20 > 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.=20 >> /* debug ADB */ >> //#define DEBUG_ADB >> @@ -59,6 +62,9 @@ do { printf("ADB: " fmt , ## __VA_ARGS__); } while = (0) >> /* error codes */ >> #define ADB_RET_NOTPRESENT (-2) >>=20 >> +/* 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; >>=20 >> -static const uint8_t pc_to_adb_keycode[256] =3D { >> - 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[] =3D { >> + [Q_KEY_CODE_SHIFT] =3D ADB_KEY_LEFT_SHIFT, >> + [Q_KEY_CODE_SHIFT_R] =3D 123, /* ADB_KEY_RIGHT_SHIFT, */ >=20 > 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] =3D ADB_KEY_LEFT, It looks wrong, but it works. >=20 >> + [Q_KEY_CODE_ALT] =3D ADB_KEY_LEFT_OPTION, >> + [Q_KEY_CODE_ALT_R] =3D 124, /* ADB_KEY_RIGHT_OPTION,*/ >> + [Q_KEY_CODE_ALTGR] =3D ADB_KEY_RIGHT_OPTION, >> + [Q_KEY_CODE_CTRL] =3D 54, /* ADB_KEY_LEFT_CONTROL, */ >> + [Q_KEY_CODE_CTRL_R] =3D 125, /* ADB_KEY_RIGHT_CONTROL, */ >> + [Q_KEY_CODE_META_L] =3D 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] =3D ADB_KEY_LEFT_COMMAND, >> + [Q_KEY_CODE_SPC] =3D ADB_KEY_SPACEBAR, >> + >> + [Q_KEY_CODE_ESC] =3D ADB_KEY_ESC, >> + [Q_KEY_CODE_1] =3D ADB_KEY_1, >> + [Q_KEY_CODE_2] =3D ADB_KEY_2, >> + [Q_KEY_CODE_3] =3D ADB_KEY_3, >> + [Q_KEY_CODE_4] =3D ADB_KEY_4, >> + [Q_KEY_CODE_5] =3D ADB_KEY_5, >> + [Q_KEY_CODE_6] =3D ADB_KEY_6, >> + [Q_KEY_CODE_7] =3D ADB_KEY_7, >> + [Q_KEY_CODE_8] =3D ADB_KEY_8, >> + [Q_KEY_CODE_9] =3D ADB_KEY_9, >> + [Q_KEY_CODE_0] =3D ADB_KEY_0, >> + [Q_KEY_CODE_MINUS] =3D ADB_KEY_MINUS, >> + [Q_KEY_CODE_EQUAL] =3D ADB_KEY_EQUAL, >> + [Q_KEY_CODE_BACKSPACE] =3D ADB_KEY_DELETE, >> + [Q_KEY_CODE_TAB] =3D ADB_KEY_TAB, >> + [Q_KEY_CODE_Q] =3D ADB_KEY_Q, >> + [Q_KEY_CODE_W] =3D ADB_KEY_W, >> + [Q_KEY_CODE_E] =3D ADB_KEY_E, >> + [Q_KEY_CODE_R] =3D ADB_KEY_R, >> + [Q_KEY_CODE_T] =3D ADB_KEY_T, >> + [Q_KEY_CODE_Y] =3D ADB_KEY_Y, >> + [Q_KEY_CODE_U] =3D ADB_KEY_U, >> + [Q_KEY_CODE_I] =3D ADB_KEY_I, >> + [Q_KEY_CODE_O] =3D ADB_KEY_O, >> + [Q_KEY_CODE_P] =3D ADB_KEY_P, >> + [Q_KEY_CODE_BRACKET_LEFT] =3D ADB_KEY_LEFT_BRACKET, >> + [Q_KEY_CODE_BRACKET_RIGHT] =3D ADB_KEY_RIGHT_BRACKET, >> + [Q_KEY_CODE_RET] =3D ADB_KEY_RETURN, >> + [Q_KEY_CODE_A] =3D ADB_KEY_A, >> + [Q_KEY_CODE_S] =3D ADB_KEY_S, >> + [Q_KEY_CODE_D] =3D ADB_KEY_D, >> + [Q_KEY_CODE_F] =3D ADB_KEY_F, >> + [Q_KEY_CODE_G] =3D ADB_KEY_G, >> + [Q_KEY_CODE_H] =3D ADB_KEY_H, >> + [Q_KEY_CODE_J] =3D ADB_KEY_J, >> + [Q_KEY_CODE_K] =3D ADB_KEY_K, >> + [Q_KEY_CODE_L] =3D ADB_KEY_L, >> + [Q_KEY_CODE_SEMICOLON] =3D ADB_KEY_SEMICOLON, >> + [Q_KEY_CODE_APOSTROPHE] =3D ADB_KEY_APOSTROPHE, >> + [Q_KEY_CODE_GRAVE_ACCENT] =3D ADB_KEY_GRAVE_ACCENT, >> + [Q_KEY_CODE_BACKSLASH] =3D ADB_KEY_BACKSLASH, >> + [Q_KEY_CODE_Z] =3D ADB_KEY_Z, >> + [Q_KEY_CODE_X] =3D ADB_KEY_X, >> + [Q_KEY_CODE_C] =3D ADB_KEY_C, >> + [Q_KEY_CODE_V] =3D ADB_KEY_V, >> + [Q_KEY_CODE_B] =3D ADB_KEY_B, >> + [Q_KEY_CODE_N] =3D ADB_KEY_N, >> + [Q_KEY_CODE_M] =3D ADB_KEY_M, >> + [Q_KEY_CODE_COMMA] =3D ADB_KEY_COMMA, >> + [Q_KEY_CODE_DOT] =3D ADB_KEY_PERIOD, >> + [Q_KEY_CODE_SLASH] =3D ADB_KEY_FORWARD_SLASH, >> + [Q_KEY_CODE_ASTERISK] =3D ADB_KEY_KP_MULTIPLY, >> + [Q_KEY_CODE_CAPS_LOCK] =3D ADB_KEY_CAPS_LOCK, >> + >> + [Q_KEY_CODE_F1] =3D ADB_KEY_F1, >> + [Q_KEY_CODE_F2] =3D ADB_KEY_F2, >> + [Q_KEY_CODE_F3] =3D ADB_KEY_F3, >> + [Q_KEY_CODE_F4] =3D ADB_KEY_F4, >> + [Q_KEY_CODE_F5] =3D ADB_KEY_F5, >> + [Q_KEY_CODE_F6] =3D ADB_KEY_F6, >> + [Q_KEY_CODE_F7] =3D ADB_KEY_F7, >> + [Q_KEY_CODE_F8] =3D ADB_KEY_F8, >> + [Q_KEY_CODE_F9] =3D ADB_KEY_F9, >> + [Q_KEY_CODE_F10] =3D ADB_KEY_F10, >> + [Q_KEY_CODE_F11] =3D ADB_KEY_F11, >> + [Q_KEY_CODE_F12] =3D ADB_KEY_F12, >> + [Q_KEY_CODE_PRINT] =3D ADB_KEY_F13, >> + [Q_KEY_CODE_SYSRQ] =3D ADB_KEY_F13, >> + [Q_KEY_CODE_SCROLL_LOCK] =3D ADB_KEY_F14, >> + [Q_KEY_CODE_PAUSE] =3D ADB_KEY_F15, >> + [Q_KEY_CODE_POWER] =3D ADB_KEY_POWER, >> + >> + [Q_KEY_CODE_NUM_LOCK] =3D ADB_KEY_KP_CLEAR, >> + [Q_KEY_CODE_KP_EQUALS] =3D ADB_KEY_KP_EQUAL, >> + [Q_KEY_CODE_KP_DIVIDE] =3D ADB_KEY_KP_DIVIDE, >> + [Q_KEY_CODE_KP_MULTIPLY] =3D ADB_KEY_KP_MULTIPLY, >> + [Q_KEY_CODE_KP_SUBTRACT] =3D ADB_KEY_KP_SUBTRACT, >> + [Q_KEY_CODE_KP_ADD] =3D ADB_KEY_KP_PLUS, >> + [Q_KEY_CODE_KP_ENTER] =3D ADB_KEY_KP_ENTER, >> + [Q_KEY_CODE_KP_DECIMAL] =3D ADB_KEY_KP_PERIOD, >> + [Q_KEY_CODE_KP_0] =3D ADB_KEY_KP_0, >> + [Q_KEY_CODE_KP_1] =3D ADB_KEY_KP_1, >> + [Q_KEY_CODE_KP_2] =3D ADB_KEY_KP_2, >> + [Q_KEY_CODE_KP_3] =3D ADB_KEY_KP_3, >> + [Q_KEY_CODE_KP_4] =3D ADB_KEY_KP_4, >> + [Q_KEY_CODE_KP_5] =3D ADB_KEY_KP_5, >> + [Q_KEY_CODE_KP_6] =3D ADB_KEY_KP_6, >> + [Q_KEY_CODE_KP_7] =3D ADB_KEY_KP_7, >> + [Q_KEY_CODE_KP_8] =3D ADB_KEY_KP_8, >> + [Q_KEY_CODE_KP_9] =3D ADB_KEY_KP_9, >> + >> + [Q_KEY_CODE_UP] =3D 62, /* ADB_KEY_UP, */ >> + [Q_KEY_CODE_DOWN] =3D 61, /* ADB_KEY_DOWN, */ >> + [Q_KEY_CODE_LEFT] =3D 59, /* ADB_KEY_LEFT, */ >> + [Q_KEY_CODE_RIGHT] =3D 60, /* ADB_KEY_RIGHT, */ >> + >> + [Q_KEY_CODE_HELP] =3D ADB_KEY_HELP, >> + [Q_KEY_CODE_INSERT] =3D ADB_KEY_HELP, >> + [Q_KEY_CODE_DELETE] =3D ADB_KEY_FORWARD_DELETE, >> + [Q_KEY_CODE_HOME] =3D ADB_KEY_HOME, >> + [Q_KEY_CODE_END] =3D ADB_KEY_END, >> + [Q_KEY_CODE_PGUP] =3D ADB_KEY_PAGE_UP, >> + [Q_KEY_CODE_PGDN] =3D ADB_KEY_PAGE_DOWN, >> + >> + [Q_KEY_CODE_LESS] =3D NO_KEY, >> + [Q_KEY_CODE_STOP] =3D NO_KEY, >> + [Q_KEY_CODE_AGAIN] =3D NO_KEY, >> + [Q_KEY_CODE_PROPS] =3D NO_KEY, >> + [Q_KEY_CODE_UNDO] =3D NO_KEY, >> + [Q_KEY_CODE_FRONT] =3D NO_KEY, >> + [Q_KEY_CODE_COPY] =3D NO_KEY, >> + [Q_KEY_CODE_OPEN] =3D NO_KEY, >> + [Q_KEY_CODE_PASTE] =3D NO_KEY, >> + [Q_KEY_CODE_FIND] =3D NO_KEY, >> + [Q_KEY_CODE_CUT] =3D NO_KEY, >> + [Q_KEY_CODE_LF] =3D NO_KEY, >> + [Q_KEY_CODE_COMPOSE] =3D NO_KEY, >=20 > 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 ] =3D NO_KEY, >=20 > which sets a default value for the whole array, > and then after that you put all the > [Q_KEY_CODE_WHATEVER] =3D ADB_KEY_THING, > entries. >=20 > (For instance we do this with the 'map' array in hw/arm/z2.c.) Sounds good. >=20 >=20 >> }; >>=20 >> static void adb_kbd_put_keycode(void *opaque, int keycode) >> @@ -220,35 +341,25 @@ static void adb_kbd_put_keycode(void *opaque, = int keycode) >>=20 >> static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf) >> { >> - static int ext_keycode; >> KBDState *s =3D ADB_KEYBOARD(d); >> - int adb_keycode, keycode; >> + int keycode; >> int olen; >>=20 >> olen =3D 0; >> - for(;;) { >> - if (s->count =3D=3D 0) >> - break; >> - keycode =3D s->data[s->rptr]; >> - if (++s->rptr =3D=3D sizeof(s->data)) >> - s->rptr =3D 0; >> - s->count--; >> - >> - if (keycode =3D=3D 0xe0) { >> - ext_keycode =3D 1; >> - } else { >> - if (ext_keycode) >> - adb_keycode =3D pc_to_adb_keycode[keycode | 0x80]; >> - else >> - adb_keycode =3D pc_to_adb_keycode[keycode & 0x7f]; >> - obuf[0] =3D adb_keycode | (keycode & 0x80); >> - /* NOTE: could put a second keycode if needed */ >> - obuf[1] =3D 0xff; >> - olen =3D 2; >> - ext_keycode =3D 0; >> - break; >> - } >> + if (s->count <=3D 0) { >> + return olen; >=20 > olen is always 0 here so why not just return 0 ? ok. >=20 >> + } >> + keycode =3D s->data[s->rptr]; >> + if (++s->rptr =3D=3D sizeof(s->data)) { >> + s->rptr =3D 0; >> } >> + s->count--; >> + >> + obuf[0] =3D keycode; >=20 > 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] =3D (keycode & 0xff00) >> 8; obuf[1] =3D keycode & 0x00ff; obuf[2] =3D 0xff; olen =3D 3; } return olen; >=20 >> + /* NOTE: could put a second keycode if needed */ >> + obuf[1] =3D 0xff; >> + olen =3D 2; >> + >> return olen; >> } >>=20 >> @@ -313,6 +424,24 @@ static int adb_kbd_request(ADBDevice *d, uint8_t = *obuf, >> return olen; >> } >>=20 >> +/* The is where keyboard events enter this file */ >> +static void adb_keyboard_event(DeviceState *dev, QemuConsole *src, >> + InputEvent *evt) >> +{ >> + KBDState *s =3D (KBDState *)dev; >> + int qcode, keycode; >> + >> + qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); >=20 > 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. >=20 >> + qcode =3D qemu_input_key_value_to_qcode(evt->u.key->key); >> + keycode =3D qcode_to_adb_keycode[qcode]; >=20 > 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.=