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

[Peter Maydell]

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 ?

>  /* 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