[U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer

Fri Feb 23 05:50:09 UTC 2018

On 02/23/2018 12:02 AM, Marek Vasut wrote:
> On 02/22/2018 09:53 PM, Heinrich Schuchardt wrote:
>> On 02/22/2018 08:55 PM, Marek Vasut wrote:
>>> On 02/22/2018 07:06 PM, Heinrich Schuchardt wrote:
>>>> On 02/22/2018 03:20 PM, Marek Vasut wrote:
>>>>> On 02/22/2018 01:04 PM, Heinrich Schuchardt wrote:
>>>>>> The USB keyboard driver provides a ring buffer for key strokes.
>>>>>>
>>>>>> Function keys cannot be encoded as single bytes. Instead xterm control
>>>>>> sequences have to be put into the ring buffer.
>>>>>
>>>>> Does it work without xterm or with any other terminal ?
>>>> I use two testing environments:
>>>>
>>>> - USB keyboard and HDMI monitor. This does not involve xterm.
>>>
>>> So how are the "xterm control sequences" interpreted then?
>>
>> I already pointed you to U-Boot function cread_line().
>> Here only the sequences for left, right, up, and down are used.
>>
>> When starting an EFI application the xterm escape sequences are
>> translated to EFI scan codes. See lib/efi_loader/efi_console.c
> 
> So this is only usable if you have display connected to the board ?

Why? We are talking about input.

Neither local nor remote display is needed to hit the F10 key on a USB 
keyboard.

> 
>>> What happens if you use the U-Boot menu, does this work in there too?
>>
>> Function bootmenu_loop() only looks for
>> ESC [ A and
>> ESC [ B
>>
>> So without the patch series you are not able to navigate inside the
>> U-Boot menu using a USB keyboard.
> 
> Ha
> 
>>>> - Connection from Linux via serial cable. Linux screen or minicom
>>>>     transfer xterm escape sequences when typing special keys.
>>>
>>> Did you try this ie. on kms console , outside of X11 ?
>>
>> I never heard of a kms console.
>>
>> If I boot into the plain Linux console (i.e. without X11), key
>> pressesare still provided as xterm escape sequences.
> 
> But there is no xterm ?
> 
>>>> My target is calling iPXE and Grub as EFI applications. For navigation
>>>> in Grub we need at least up, down, left, right, delete and F10. In
>>>> lib/efi_loader/efi_console.c xterm escape sequences are translated to
>>>> EFI scan codes.
>>>
>>>> Before the patch series the U-Boot USB keyboard driver signals up, down,
>>>> left and right as non-standard character codes 0x6, 0x2, 0xe, 0x10.
>>>> F1-F12, Insert, Delete, Home, End, Page Up, and Page Down are not
>>>> supported.
>>>>
>>>> Function cread_line() (in common/cli_readline.c) interprets both xterm
>>>> escape sequences and non-standard control characters provided by the USB
>>>> keyboard driver.
>>>
>>> Wait, are those xterm or vt100 sequences ?
>>
>> VT100 largely overlaps with xterm. See
>> https://invisible-island.net/xterm/ctlseqs/ctlseqs.pdf
> 
> So vt100 , OK.
> 
>>>> JinHua Luo's patch
>>>> Add readline cmdline-editing extension
>>>> 501090aaa67b6072ebe8b721c8328d32be607660
>>>> which introduced command line editing unfortunately does not describe
>>>> why it chose CTRL+B, CTRL+F, CTRL+N, and CTRL+P for navigation. These
>>>> codes are not documented in any README.
>>>>
>>>>>
>>>>>> This preparatory patch changes function usb_kbd_put_queue() to allow
>>>>>> adding
>>>>>> multiple characters at once. If the buffer cannot accommodate the
>>>>>> whole
>>>>>> sequence, it is rejected completely.
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>>> ---
>>>>>>     common/usb_kbd.c | 42 +++++++++++++++++++++++++-----------------
>>>>>>     1 file changed, 25 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>>>>> index 8cbdba6ac2..706cc350a6 100644
>>>>>> --- a/common/usb_kbd.c
>>>>>> +++ b/common/usb_kbd.c
>>>>>> @@ -125,24 +125,32 @@ extern int __maybe_unused net_busy_flag;
>>>>>>     /* The period of time between two calls of usb_kbd_testc(). */
>>>>>>     static unsigned long __maybe_unused kbd_testc_tms;
>>>>>>     -/* Puts character in the queue and sets up the in and out
>>>>>> pointer. */
>>>>>> -static void usb_kbd_put_queue(struct usb_kbd_pdata *data, char c)
>>>>>> +/*
>>>>>> + * Put characters into ring buffer.
>>>>>> + *
>>>>>> + * @data:    private data
>>>>>> + * @buf        buffer with data to be queued
>>>>>> + * @count:    number of characters to be queued
>>>>>> + */
>>>>>> +static void usb_kbd_put_queue(struct usb_kbd_pdata *data,
>>>>>> +                  uint8_t *buf, int count)
>>>>>>     {
>>>>>> -    if (data->usb_in_pointer == USB_KBD_BUFFER_LEN - 1) {
>>>>>> -        /* Check for buffer full. */
>>>>>> -        if (data->usb_out_pointer == 0)
>>>>>> -            return;
>>>>>> -
>>>>>> -        data->usb_in_pointer = 0;
>>>>>> -    } else {
>>>>>> -        /* Check for buffer full. */
>>>>>> -        if (data->usb_in_pointer == data->usb_out_pointer - 1)
>>>>>> -            return;
>>>>>> -
>>>>>> -        data->usb_in_pointer++;
>>>>>> +    int i, used;
>>>>>> +
>>>>>> +    /* Check if buffer holds at least 'count' free slots */
>>>>>> +    used = data->usb_in_pointer - data->usb_out_pointer;
>>>>>> +    if (used < 0)
>>>>>> +        used += USB_KBD_BUFFER_LEN;
>>>>>> +    if (used + count >= USB_KBD_BUFFER_LEN)
>>>>>> +        return;
>>>>>> +
>>>>>> +    /* Copy to buffer */
>>>>>> +    for (i = 0; i < count; ++i) {
>>>>>> +        ++data->usb_in_pointer;
>>>>>> +        if (data->usb_in_pointer == USB_KBD_BUFFER_LEN)
>>>>>> +            data->usb_in_pointer = 0;
>>>>>> +        data->usb_kbd_buffer[data->usb_in_pointer] = buf[i];
>>>>>
>>>>> memcpy() ?
>>>>
>>>> Typically we copy only one byte. But escape sequences have a maximum
>>>> length of 8 bytes (e.g. CTRL+F8).
>>>>
>>>> We have to consider the case with wrap around. This would require two
>>>> memcpy() calls.
>>>>
>>>> The coding would neither get faster in the average nor less complex
>>>> using memcpy(). So let's keep it as it is.
>>>
>>> I suspect this block of code needs cleanup .
>>>
>>
>> Could you, please, give some indication of what you dislike.
> 
> At least the part which looks like ad-hoc implementation of memcpy() ,
> any other cleanups are welcome.
> 

Do you really want that ugly monster below for typically copying a 
single byte?

As said it is slower, has more lines, and gives a larger executable.

int remaining;

/* Copy to buffer */
remaining = data->usb_in_pointer + count - USB_KBD_BUFFER_LEN;
if (remaining < 0)
	remaining = 0;
memcpy(&usb_kbd_buffer[data->usb_in_pointer], buf, count - remaining);
if (remaining)
	memcpy(usb_kbd_buffer, buf + count - remaining, remaining);
data->usb_in_pointer += count;
if (data->usb_in_pointer >= USB_KBD_BUFFER_LEN)
	data->usb_in_pointer -= USB_KBD_BUFFER_LEN;

Best regards

Heinrich