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

[Programmingkid]

On Mar 2, 2016, at 7:38 AM, Peter Maydell wrote:

> 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.

Now that I think about it, you are right. I'm actually using PS/2 to ADB here 
which isn't a bad thing. 

I see what is wrong. I completely forgot to send in my input-keymap.c patch. It 
is this file that I include the Q_KEY_CODE_KP_EQUALS key. Sorry for the 
confusion. 

> 
>>> 
>>>> +    [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.

Would it be easier if I just made a new patch for adb.c that takes the existing 
pc_to_adb_keycode array and just expanded it directly by having the index on 
the left and the value as a named constant on the right. 

Original code:
static const uint8_t pc_to_adb_keycode[256] = {
0, 53, 18, ...

New code:
[0x00] = 0,
[0x01] = MAC_KEY_ESC,
[0x02] = MAC_KEY_1,
...

The order of the values in both arrays would be the same. 

Did you want the index to be in hexadecimal or just decimal?


> 
>> 
>>> 
>>>> +    [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.

This should do it:

#define NO_KEY 0xFF

> 
> 
>>>> +    [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.

The adb.c file's function adb_kbd_request() handles keyboard requests. Its 
arguments include a buffer for storing keycodes and a len argument that keeps 
track of the number of bytes to read. Setting the len value to two is probably 
how a two byte value can be handled.