On 2 March 2016 at 00:31, Programmingkid <programmingk...@gmail.com> wrote:
>
> On Mar 1, 2016, at 6:34 PM, Peter Maydell wrote:
>
>> On 1 March 2016 at 22:10, Programmingkid <programmingk...@gmail.com> 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
