Re: [Qemu-devel] [PATCH v4 4/4] hw/input/adb.c: implement QKeyCode support

[Programmingkid]

On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote:

> On 11 March 2016 at 09:32, Programmingkid <address@hidden> wrote:
>> Remove the old pc_to_adb_keycode array and replace it with QKeyCode support.
>> 
>> Signed-off-by: John Arbuckle <address@hidden>
>> ---
>> 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.