diff mbox

[v3,3/5] adb.c: correct several key assignments

Message ID 2C310CE8-A8DE-4383-BC90-C4589FCE9D79@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Programmingkid May 6, 2016, 2:37 a.m. UTC
The original pc_to_adb_keycode mapping did have several keys that were
incorrectly mapped. This patch fixes these mappings.

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
---
 hw/input/adb.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

Comments

Peter Maydell May 16, 2016, 6:04 p.m. UTC | #1
On 6 May 2016 at 03:37, Programmingkid <programmingkidx@gmail.com> wrote:
> The original pc_to_adb_keycode mapping did have several keys that were
> incorrectly mapped. This patch fixes these mappings.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> ---
>  hw/input/adb.c | 33 ++++++++++++---------------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/hw/input/adb.c b/hw/input/adb.c
> index 715642f..6d4f4dc 100644
> --- a/hw/input/adb.c
> +++ b/hw/input/adb.c
> @@ -62,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
> +

This...

>  static void adb_device_reset(ADBDevice *d)
>  {
>      qdev_reset_all(DEVICE(d));
> @@ -191,19 +194,21 @@ typedef struct ADBKeyboardClass {
>  } ADBKeyboardClass;
>
>  int qcode_to_adb_keycode[] = {
> +     /* Make sure future additions are automatically set to NO_KEY */
> +    [0 ... 0xff]               = NO_KEY,

...and this (and the removal of the "= 0" entries) should
be in the same patch which adds the "don't send key if
NO_KEY" check.

>      [Q_KEY_CODE_SHIFT]         = ADB_KEY_LEFT_SHIFT,
>      [Q_KEY_CODE_SHIFT_R]       = ADB_KEY_RIGHT_SHIFT,
>      [Q_KEY_CODE_ALT]           = ADB_KEY_LEFT_OPTION,
>      [Q_KEY_CODE_ALT_R]         = ADB_KEY_RIGHT_OPTION,
> -    [Q_KEY_CODE_ALTGR]         = 0,
> +    [Q_KEY_CODE_ALTGR]         = ADB_KEY_RIGHT_OPTION,
>      [Q_KEY_CODE_CTRL]          = ADB_KEY_LEFT_CONTROL,
>      [Q_KEY_CODE_CTRL_R]        = ADB_KEY_RIGHT_CONTROL,
>      [Q_KEY_CODE_META_L]        = ADB_KEY_LEFT_COMMAND,
>
>       /* ADB_KEY_RIGHT_COMMAND works as right super in Linux */
>       /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */
> -    [Q_KEY_CODE_META_R]        = 0x7e,
> +    [Q_KEY_CODE_META_R]        = ADB_KEY_LEFT_COMMAND,

This looks weird. Given that the Apple Extended Keyboard
hardware documentation just says that both left and right
Command keys return 0x37, we should probably just call
the #define ADB_KEY_COMMAND. (That in turn means that the
comments are unnecessary and you can just delete them
from the patch you put them in.)

>      [Q_KEY_CODE_SPC]           = ADB_KEY_SPACEBAR,
>
>      [Q_KEY_CODE_ESC]           = ADB_KEY_ESC,
> @@ -272,13 +277,13 @@ int qcode_to_adb_keycode[] = {
>      [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]         = 0,
> -    [Q_KEY_CODE_SYSRQ]         = 0,
> +    [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]         = 0,
> +    [Q_KEY_CODE_PAUSE]         = ADB_KEY_F15,
>
>      [Q_KEY_CODE_NUM_LOCK]      = ADB_KEY_KP_CLEAR,
> -    [Q_KEY_CODE_KP_EQUALS]     = 0,
> +    [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,
> @@ -301,27 +306,13 @@ int qcode_to_adb_keycode[] = {
>      [Q_KEY_CODE_LEFT]          = ADB_KEY_LEFT,
>      [Q_KEY_CODE_RIGHT]         = ADB_KEY_RIGHT,
>
> -    [Q_KEY_CODE_HELP]          = 0,
> +    [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]          = 0xa,
> -    [Q_KEY_CODE_STOP]          = 0,
> -    [Q_KEY_CODE_AGAIN]         = 0,
> -    [Q_KEY_CODE_PROPS]         = 0,
> -    [Q_KEY_CODE_UNDO]          = 0,
> -    [Q_KEY_CODE_FRONT]         = 0,
> -    [Q_KEY_CODE_COPY]          = 0,
> -    [Q_KEY_CODE_OPEN]          = 0,
> -    [Q_KEY_CODE_PASTE]         = 0,
> -    [Q_KEY_CODE_FIND]          = 0,
> -    [Q_KEY_CODE_CUT]           = 0,
> -    [Q_KEY_CODE_LF]            = 0,
> -    [Q_KEY_CODE_COMPOSE]       = 0,

The rest of these changes look OK.

thanks
-- PMM
Programmingkid May 16, 2016, 8:42 p.m. UTC | #2
On May 16, 2016, at 2:04 PM, Peter Maydell wrote:

> On 6 May 2016 at 03:37, Programmingkid <programmingkidx@gmail.com> wrote:
>> The original pc_to_adb_keycode mapping did have several keys that were
>> incorrectly mapped. This patch fixes these mappings.
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> ---
>> hw/input/adb.c | 33 ++++++++++++---------------------
>> 1 file changed, 12 insertions(+), 21 deletions(-)
>> 
>> diff --git a/hw/input/adb.c b/hw/input/adb.c
>> index 715642f..6d4f4dc 100644
>> --- a/hw/input/adb.c
>> +++ b/hw/input/adb.c
>> @@ -62,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
>> +
> 
> This...
> 
>> static void adb_device_reset(ADBDevice *d)
>> {
>>     qdev_reset_all(DEVICE(d));
>> @@ -191,19 +194,21 @@ typedef struct ADBKeyboardClass {
>> } ADBKeyboardClass;
>> 
>> int qcode_to_adb_keycode[] = {
>> +     /* Make sure future additions are automatically set to NO_KEY */
>> +    [0 ... 0xff]               = NO_KEY,
> 
> ...and this (and the removal of the "= 0" entries) should
> be in the same patch which adds the "don't send key if
> NO_KEY" check.
> 
>>     [Q_KEY_CODE_SHIFT]         = ADB_KEY_LEFT_SHIFT,
>>     [Q_KEY_CODE_SHIFT_R]       = ADB_KEY_RIGHT_SHIFT,
>>     [Q_KEY_CODE_ALT]           = ADB_KEY_LEFT_OPTION,
>>     [Q_KEY_CODE_ALT_R]         = ADB_KEY_RIGHT_OPTION,
>> -    [Q_KEY_CODE_ALTGR]         = 0,
>> +    [Q_KEY_CODE_ALTGR]         = ADB_KEY_RIGHT_OPTION,
>>     [Q_KEY_CODE_CTRL]          = ADB_KEY_LEFT_CONTROL,
>>     [Q_KEY_CODE_CTRL_R]        = ADB_KEY_RIGHT_CONTROL,
>>     [Q_KEY_CODE_META_L]        = ADB_KEY_LEFT_COMMAND,
>> 
>>      /* ADB_KEY_RIGHT_COMMAND works as right super in Linux */
>>      /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */
>> -    [Q_KEY_CODE_META_R]        = 0x7e,
>> +    [Q_KEY_CODE_META_R]        = ADB_KEY_LEFT_COMMAND,
> 
> This looks weird. Given that the Apple Extended Keyboard
> hardware documentation just says that both left and right
> Command keys return 0x37, we should probably just call
> the #define ADB_KEY_COMMAND. (That in turn means that the
> comments are unnecessary and you can just delete them
> from the patch you put them in.)

I liked the idea of giving someone who might need to tell the
difference between left and right command keys a way to
accomplish their goal. 

If I did replace ADB_KEY_LEFT_COMMAND and ADB_KEY_RIGHT_COMMAND
with ADB_KEY_COMMAND, then patches 1 and 2 would have to be
changed.

> 
>>     [Q_KEY_CODE_SPC]           = ADB_KEY_SPACEBAR,
>> 
>>     [Q_KEY_CODE_ESC]           = ADB_KEY_ESC,
>> @@ -272,13 +277,13 @@ int qcode_to_adb_keycode[] = {
>>     [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]         = 0,
>> -    [Q_KEY_CODE_SYSRQ]         = 0,
>> +    [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]         = 0,
>> +    [Q_KEY_CODE_PAUSE]         = ADB_KEY_F15,
>> 
>>     [Q_KEY_CODE_NUM_LOCK]      = ADB_KEY_KP_CLEAR,
>> -    [Q_KEY_CODE_KP_EQUALS]     = 0,
>> +    [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,
>> @@ -301,27 +306,13 @@ int qcode_to_adb_keycode[] = {
>>     [Q_KEY_CODE_LEFT]          = ADB_KEY_LEFT,
>>     [Q_KEY_CODE_RIGHT]         = ADB_KEY_RIGHT,
>> 
>> -    [Q_KEY_CODE_HELP]          = 0,
>> +    [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]          = 0xa,
>> -    [Q_KEY_CODE_STOP]          = 0,
>> -    [Q_KEY_CODE_AGAIN]         = 0,
>> -    [Q_KEY_CODE_PROPS]         = 0,
>> -    [Q_KEY_CODE_UNDO]          = 0,
>> -    [Q_KEY_CODE_FRONT]         = 0,
>> -    [Q_KEY_CODE_COPY]          = 0,
>> -    [Q_KEY_CODE_OPEN]          = 0,
>> -    [Q_KEY_CODE_PASTE]         = 0,
>> -    [Q_KEY_CODE_FIND]          = 0,
>> -    [Q_KEY_CODE_CUT]           = 0,
>> -    [Q_KEY_CODE_LF]            = 0,
>> -    [Q_KEY_CODE_COMPOSE]       = 0,
> 
> The rest of these changes look OK.
> 
> thanks
> -- PMM
Peter Maydell May 16, 2016, 8:48 p.m. UTC | #3
On 16 May 2016 at 21:42, Programmingkid <programmingkidx@gmail.com> wrote:
>
> On May 16, 2016, at 2:04 PM, Peter Maydell wrote:
>
>> On 6 May 2016 at 03:37, Programmingkid <programmingkidx@gmail.com> wrote:
>>>      /* ADB_KEY_RIGHT_COMMAND works as right super in Linux */
>>>      /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */
>>> -    [Q_KEY_CODE_META_R]        = 0x7e,
>>> +    [Q_KEY_CODE_META_R]        = ADB_KEY_LEFT_COMMAND,
>>
>> This looks weird. Given that the Apple Extended Keyboard
>> hardware documentation just says that both left and right
>> Command keys return 0x37, we should probably just call
>> the #define ADB_KEY_COMMAND. (That in turn means that the
>> comments are unnecessary and you can just delete them
>> from the patch you put them in.)
>
> I liked the idea of giving someone who might need to tell the
> difference between left and right command keys a way to
> accomplish their goal.

We're emulating a real bit of hardware here (ie the Apple
Extended Keyboard). If that hardware does not have distinct
left and right command keys then that's what we have to emulate.

thanks
-- PMM
Programmingkid May 16, 2016, 9:18 p.m. UTC | #4
On May 16, 2016, at 4:48 PM, Peter Maydell wrote:

> On 16 May 2016 at 21:42, Programmingkid <programmingkidx@gmail.com> wrote:
>> 
>> On May 16, 2016, at 2:04 PM, Peter Maydell wrote:
>> 
>>> On 6 May 2016 at 03:37, Programmingkid <programmingkidx@gmail.com> wrote:
>>>>     /* ADB_KEY_RIGHT_COMMAND works as right super in Linux */
>>>>     /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */
>>>> -    [Q_KEY_CODE_META_R]        = 0x7e,
>>>> +    [Q_KEY_CODE_META_R]        = ADB_KEY_LEFT_COMMAND,
>>> 
>>> This looks weird. Given that the Apple Extended Keyboard
>>> hardware documentation just says that both left and right
>>> Command keys return 0x37, we should probably just call
>>> the #define ADB_KEY_COMMAND. (That in turn means that the
>>> comments are unnecessary and you can just delete them
>>> from the patch you put them in.)
>> 
>> I liked the idea of giving someone who might need to tell the
>> difference between left and right command keys a way to
>> accomplish their goal.
> 
> We're emulating a real bit of hardware here (ie the Apple
> Extended Keyboard). If that hardware does not have distinct
> left and right command keys then that's what we have to emulate.

I think the Apple Extended keyboard could tell the difference between
left and right command keys, but Apple just decided not to document
this feature. I know the USB keyboards can see the difference and
Apple did not document that feature. In the end I suppose it is just
easier to have just an ADB_KEY_COMMAND constant.
diff mbox

Patch

diff --git a/hw/input/adb.c b/hw/input/adb.c
index 715642f..6d4f4dc 100644
--- a/hw/input/adb.c
+++ b/hw/input/adb.c
@@ -62,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));
@@ -191,19 +194,21 @@  typedef struct ADBKeyboardClass {
 } ADBKeyboardClass;
 
 int qcode_to_adb_keycode[] = {
+     /* Make sure future additions are automatically set to NO_KEY */
+    [0 ... 0xff]               = NO_KEY,
 
     [Q_KEY_CODE_SHIFT]         = ADB_KEY_LEFT_SHIFT,
     [Q_KEY_CODE_SHIFT_R]       = ADB_KEY_RIGHT_SHIFT,
     [Q_KEY_CODE_ALT]           = ADB_KEY_LEFT_OPTION,
     [Q_KEY_CODE_ALT_R]         = ADB_KEY_RIGHT_OPTION,
-    [Q_KEY_CODE_ALTGR]         = 0,
+    [Q_KEY_CODE_ALTGR]         = ADB_KEY_RIGHT_OPTION,
     [Q_KEY_CODE_CTRL]          = ADB_KEY_LEFT_CONTROL,
     [Q_KEY_CODE_CTRL_R]        = ADB_KEY_RIGHT_CONTROL,
     [Q_KEY_CODE_META_L]        = ADB_KEY_LEFT_COMMAND,
 
      /* ADB_KEY_RIGHT_COMMAND works as right super in Linux */
      /* Use ADB_KEY_LEFT_COMMAND for Mac OS compatibility */
-    [Q_KEY_CODE_META_R]        = 0x7e,
+    [Q_KEY_CODE_META_R]        = ADB_KEY_LEFT_COMMAND,
     [Q_KEY_CODE_SPC]           = ADB_KEY_SPACEBAR,
 
     [Q_KEY_CODE_ESC]           = ADB_KEY_ESC,
@@ -272,13 +277,13 @@  int qcode_to_adb_keycode[] = {
     [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]         = 0,
-    [Q_KEY_CODE_SYSRQ]         = 0,
+    [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]         = 0,
+    [Q_KEY_CODE_PAUSE]         = ADB_KEY_F15,
 
     [Q_KEY_CODE_NUM_LOCK]      = ADB_KEY_KP_CLEAR,
-    [Q_KEY_CODE_KP_EQUALS]     = 0,
+    [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,
@@ -301,27 +306,13 @@  int qcode_to_adb_keycode[] = {
     [Q_KEY_CODE_LEFT]          = ADB_KEY_LEFT,
     [Q_KEY_CODE_RIGHT]         = ADB_KEY_RIGHT,
 
-    [Q_KEY_CODE_HELP]          = 0,
+    [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]          = 0xa,
-    [Q_KEY_CODE_STOP]          = 0,
-    [Q_KEY_CODE_AGAIN]         = 0,
-    [Q_KEY_CODE_PROPS]         = 0,
-    [Q_KEY_CODE_UNDO]          = 0,
-    [Q_KEY_CODE_FRONT]         = 0,
-    [Q_KEY_CODE_COPY]          = 0,
-    [Q_KEY_CODE_OPEN]          = 0,
-    [Q_KEY_CODE_PASTE]         = 0,
-    [Q_KEY_CODE_FIND]          = 0,
-    [Q_KEY_CODE_CUT]           = 0,
-    [Q_KEY_CODE_LF]            = 0,
-    [Q_KEY_CODE_COMPOSE]       = 0,
 };
 
 static void adb_kbd_put_keycode(void *opaque, int keycode)