From: Programmingkid <programmingkidx@gmail.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>,
qemu-devel qemu-devel <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH v4 4/4] hw/input/adb.c: implement QKeyCode support
Date: Fri, 11 Mar 2016 23:27:26 -0500 [thread overview]
Message-ID: <FDD65B10-69AB-4A6C-97CD-F3529CB5B5FE@gmail.com> (raw)
In-Reply-To: <CAFEAcA9e2iYOXfmZ+bpvarDqBwgn58yuWqGwZW4VaRw1RMb8qA@mail.gmail.com>
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.
next prev parent reply other threads:[~2016-03-12 4:27 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-11 2:32 [Qemu-devel] [PATCH v4 4/4] hw/input/adb.c: implement QKeyCode support Programmingkid
2016-03-12 3:30 ` Peter Maydell
2016-03-12 4:27 ` Programmingkid [this message]
2016-03-13 16:00 ` Peter Maydell
2016-03-12 5:40 ` Programmingkid
2016-03-13 15:40 ` Peter Maydell
2016-03-13 16:39 ` Programmingkid
2016-03-13 16:42 ` Peter Maydell
2016-03-14 23:06 ` Programmingkid
2016-03-14 23:10 ` M A
2016-03-15 9:22 ` Peter Maydell
2016-03-15 15:24 ` Programmingkid
2016-03-15 15:34 ` Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=FDD65B10-69AB-4A6C-97CD-F3529CB5B5FE@gmail.com \
--to=programmingkidx@gmail.com \
--cc=kraxel@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).