Re: [Qemu-devel] [PATCH] hw/input/adb.c: Replace pc_to_adb_keycode with more detailed array

[Peter Maydell]

On 2 March 2016 at 00:31, Programmingkid <address@hidden> wrote:
>
> On Mar 1, 2016, at 6:34 PM, Peter Maydell wrote:
>
>> On 1 March 2016 at 22:10, Programmingkid <address@hidden> wrote:
>>> The pc_to_adb_keycode array was not very easy to work with. The replacement
>>> array number_to_adb_keycode list all the element indexes on the left and its
>>> value on the right. This makes finding a particular index or the meaning for
>>> that index very easy.
>>>

>> Scancodes are guaranteed to be single byte so you can just use
>> [256] like the old array.
>>
>> In any case this whole array ought at some point to be
>> replaced with a Q_KEY code to ADB code lookup -- at the
>> moment we will convert Q_KEY to pc scancode to ADB code,
>> which is unfortunate if the pc scancodes don't include
>> some keys that ADB and the host keyboard do. (In fact,
>> wasn't this the reason why you wanted to do these patches?)
>
> Yes it was. There was no keypad equal key and the QKeyCode looked
> like it provided this key.

But your changes don't seem to be actually using QKeyCodes
in the ADB keyboard model, so I'm confused about how you
can get the keypad-equal to go through it.

>>
>>> +    [0x2a] = MAC_KEY_LEFT_SHIFT,
>>> +    [0x36] = MAC_KEY_LEFT,
>>
>> This part of the patch is going to be very painful to review,
>> because I have to go through and manually correlate
>> the new array against the old table (including cross
>> referencing it against your new MAC_KEY_* codes) to
>> see if there are any changes in here beyond the format.
>
> If you have a Mac OS X guest, you could use Key Caps.

I think it would be helpful if you make sure that you use
separate patches for "just rearranging how this array is
written" from "changing functionality". Then in reviewing
the former I know I just need to check that the array
contents are the same, and in reviewing the latter I
only need to think about the consequences of the function
changes.

>
>>
>>> +    [0x38] = MAC_KEY_LEFT_OPTION,
>>> +    [0xb8] = MAC_KEY_RIGHT,
>>> +    [0x64] = MAC_KEY_LEFT_OPTION,
>>> +    [0xe4] = MAC_KEY_RIGHT_OPTION,
>>> +    [0x1d] = MAC_KEY_RIGHT_COMMAND,
>>> +    [0x9d] = MAC_KEY_DOWN,
>>> +    [0xdb] = MAC_KEY_LEFT_COMMAND,
>>> +    [0xdc] = MAC_KEY_LEFT_COMMAND,
>>> +    [0xdd] = 0, /* no Macintosh equivalent to the menu key */
>>
>> Not a new problem, but you'll note that MAC_KEY_A is 0, so
>> all these zero entries don't mean "no key, ignore", but "send
>> A instead"... (A fix for that bug deserves a patch of its own.)
>
> So you want another value in place of zero. What value did you want?

Any value which isn't a possible valid ADB scancode.


>>> +    [0x5e] = MAC_KEY_POWER,
>>
>> MAC_KEY_POWER is a two byte scancode, but...
>
> It works!

How? You need to have a change which handles this deliberately
and is clear about why it works. Just happening to work by
accident isn't sufficient.

>
>>
>>> };
>>>
>>> static void adb_kbd_put_keycode(void *opaque, int keycode)
>>> @@ -237,10 +336,11 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
>>>         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];
>>> +            if (ext_keycode) {
>>> +                adb_keycode = number_to_adb_keycode[keycode | 0x80];
>>> +            } else {
>>> +                adb_keycode = number_to_adb_keycode[keycode & 0x7f];
>>> +            }
>>>             obuf[0] = adb_keycode | (keycode & 0x80);
>>>             /* NOTE: could put a second keycode if needed */
>>>             obuf[1] = 0xff;
>>
>> ...this code which writes keycodes into the output buffer assumes
>> all ADB scancodes are single byte.
>
> Maybe the cuda code does something with the power button. I'm not sure.

We're writing bytes into the obuf[] array. It's not possible to
put a 16 bit value into an 8 bit slot. Something odd is presumably
happening by lucky accident, but you need to look at how to make
it work correctly and do whatever the keyboard hardware does with
2 byte scancodes. (Which you need to go and find the hardware
documentation for.)

thanks
-- PMM