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

[M A]

On Mar 14, 2016, at 7:06 PM, Programmingkid wrote:

> 
> On Mar 13, 2016, at 11:40 AM, Peter Maydell wrote:
> 
>> On 12 March 2016 at 05:40, Programmingkid <address@hidden> wrote:
>>> 
>>> On Mar 11, 2016, at 10:30 PM, Peter Maydell wrote:
>>> 
>>>> 
>>>>> +    }
>>>>> +    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;
>>>>> }
>>> 
>>> Is this ok?
>>> 
>>>   /* The power key is the only two byte value key, so it is a special case. 
>>> */
>>>   if (keycode == (ADB_KEY_POWER & 0x00ff)) {
>>>       obuf[0] = ADB_KEY_POWER & 0x00ff;
>>>       obuf[1] = ADB_KEY_POWER & 0xff00 >> 8;
>>>       olen = 2;
>>>   } else {
>>>       obuf[0] = keycode;
>>>       /* NOTE: could put a second keycode if needed */
>>>       obuf[1] = 0xff;
>>>       olen = 2;
>>>   }
>>> 
>>> The keycode value comes from an 8 bit array so holding the
>>> full value of the power key is not possible.
>> 
>> Ah, I hadn't noticed that -- that is not a good approach.
>> You should either:
>> (a) deal with the fact that ADB_KEY values may be 16 bits
>> all the way through (including having that array be uint16_t
>> rather than uint8_t)
>> (b) have the power key be a completely special case which
>> is handled by
>> if (qcode == Q_KEY_CODE_POWER) {
>>   /* put power key into buffer */
>>   [...]
>> } else {
>>   keycode = qcode_to_adb_keycode[...];
>>   etc;
>> }
>> and not put it into the array at all.
>> 
>> Also, you need to handle the power-key key-release
>> scancode; as far as I can tell (by looking for info via
>> google about the ADB protocol) power-key-down is
>> 0x7f 0x7f, and power-key-up is 0xff 0xff. (This is
>> kind of weird and suggests we should just take option
>> (a) and special case the power key completely.)
> 
> The power-key-up (break) keycode is 0xff 0xff? Could you send me your source 
> for this information? I always thought the up (break) code was the keycode 
> AND with 0x80. Or this if code is easier to understand:
> break_keycode = keycode & 0x80.

* sorry I meant OR in the above code
break_keycode = keycode | 0x80.