From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Fri, 23 Feb 2018 06:50:09 +0100 Subject: [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer In-Reply-To: <8b6d550e-1c7f-deb4-1e61-e63a210f9dcf@denx.de> References: <20180222120418.12832-1-xypron.glpk@gmx.de> <20180222120418.12832-2-xypron.glpk@gmx.de> <3dd144c7-e6d8-d146-70a2-b6093ea09316@denx.de> <8b6d550e-1c7f-deb4-1e61-e63a210f9dcf@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>>>>> --- >>>>>>    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