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

[Marek Vasut]

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 ?
What happens if you use the U-Boot menu, does this work in there too?

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

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

> 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 maximum
> length are 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 .

-- 
Best regards,
Marek Vasut