* [U-Boot] [PATCH 0/2] usb: kbd: implement special keys @ 2018-02-22 12:04 Heinrich Schuchardt 2018-02-22 12:04 ` [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer Heinrich Schuchardt 2018-02-22 12:04 ` [U-Boot] [PATCH 2/2] usb: kbd: implement special keys Heinrich Schuchardt 0 siblings, 2 replies; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-22 12:04 UTC (permalink / raw) To: u-boot Correct support for arrow keys: use the standard xterm escape sequences. Provide support for F1-F12, Insert, Delete, Home, End, Page Up, Page Down. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Heinrich Schuchardt (2): usb: kbd: allow multibyte sequences to be put into ring buffer usb: kbd: implement special keys common/usb_kbd.c | 161 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 114 insertions(+), 47 deletions(-) -- 2.14.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-22 12:04 [U-Boot] [PATCH 0/2] usb: kbd: implement special keys Heinrich Schuchardt @ 2018-02-22 12:04 ` Heinrich Schuchardt 2018-02-22 14:20 ` Marek Vasut 2018-02-22 12:04 ` [U-Boot] [PATCH 2/2] usb: kbd: implement special keys Heinrich Schuchardt 1 sibling, 1 reply; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-22 12:04 UTC (permalink / raw) To: u-boot 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. 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@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]; } - - data->usb_kbd_buffer[data->usb_in_pointer] = c; } /* @@ -237,7 +245,7 @@ static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode, /* Report keycode if any */ if (keycode) { debug("%c", keycode); - usb_kbd_put_queue(data, keycode); + usb_kbd_put_queue(data, &keycode, 1); } return 0; -- 2.14.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-22 12:04 ` [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer Heinrich Schuchardt @ 2018-02-22 14:20 ` Marek Vasut 2018-02-22 18:06 ` Heinrich Schuchardt 0 siblings, 1 reply; 16+ messages in thread From: Marek Vasut @ 2018-02-22 14:20 UTC (permalink / raw) To: u-boot 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 ? > 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@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() ? > } > - > - data->usb_kbd_buffer[data->usb_in_pointer] = c; > } > > /* > @@ -237,7 +245,7 @@ static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode, > /* Report keycode if any */ > if (keycode) { > debug("%c", keycode); > - usb_kbd_put_queue(data, keycode); > + usb_kbd_put_queue(data, &keycode, 1); > } > > return 0; > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-22 14:20 ` Marek Vasut @ 2018-02-22 18:06 ` Heinrich Schuchardt 2018-02-22 19:55 ` Marek Vasut 0 siblings, 1 reply; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-22 18:06 UTC (permalink / raw) To: u-boot 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. - Connection from Linux via serial cable. Linux screen or minicom transfer xterm escape sequences when typing special keys. 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. 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@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. Best regards Heinrich > >> } >> - >> - data->usb_kbd_buffer[data->usb_in_pointer] = c; >> } >> >> /* >> @@ -237,7 +245,7 @@ static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode, >> /* Report keycode if any */ >> if (keycode) { >> debug("%c", keycode); >> - usb_kbd_put_queue(data, keycode); >> + usb_kbd_put_queue(data, &keycode, 1); >> } >> >> return 0; >> > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-22 18:06 ` Heinrich Schuchardt @ 2018-02-22 19:55 ` Marek Vasut 2018-02-22 20:53 ` Heinrich Schuchardt 0 siblings, 1 reply; 16+ messages in thread From: Marek Vasut @ 2018-02-22 19:55 UTC (permalink / raw) To: u-boot 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@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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-22 19:55 ` Marek Vasut @ 2018-02-22 20:53 ` Heinrich Schuchardt 2018-02-22 23:02 ` Marek Vasut 0 siblings, 1 reply; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-22 20:53 UTC (permalink / raw) To: u-boot 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 > 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. > >> - 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. > >> 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 > >> 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@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. Regards Heinrich ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-22 20:53 ` Heinrich Schuchardt @ 2018-02-22 23:02 ` Marek Vasut 2018-02-23 5:50 ` Heinrich Schuchardt 0 siblings, 1 reply; 16+ messages in thread From: Marek Vasut @ 2018-02-22 23:02 UTC (permalink / raw) To: u-boot 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 ? >> 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@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. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-22 23:02 ` Marek Vasut @ 2018-02-23 5:50 ` Heinrich Schuchardt 2018-02-23 6:04 ` Marek Vasut 0 siblings, 1 reply; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-23 5:50 UTC (permalink / raw) To: u-boot 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@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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-23 5:50 ` Heinrich Schuchardt @ 2018-02-23 6:04 ` Marek Vasut 2018-02-23 7:09 ` Heinrich Schuchardt 0 siblings, 1 reply; 16+ messages in thread From: Marek Vasut @ 2018-02-23 6:04 UTC (permalink / raw) To: u-boot On 02/23/2018 06:50 AM, Heinrich Schuchardt wrote: [...] >>> 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 confused me was the constant usage of xterm control sequences to describe terminal control sequences. Now it makes sense. [...] >>>>>>> + /* 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; Can the code be tweaked so it doesn't use the circular buffer, but a linear one , thus two memcpy()s are not needed ? The whole usb keyboard code is not nice and could use some cleanup. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer 2018-02-23 6:04 ` Marek Vasut @ 2018-02-23 7:09 ` Heinrich Schuchardt 0 siblings, 0 replies; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-23 7:09 UTC (permalink / raw) To: u-boot On 02/23/2018 07:04 AM, Marek Vasut wrote: > On 02/23/2018 06:50 AM, Heinrich Schuchardt wrote: > [...] >>>> 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 confused me was the constant usage of xterm control sequences to > describe terminal control sequences. Now it makes sense. > > [...] > >>>>>>>> + /* 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; > > Can the code be tweaked so it doesn't use the circular buffer, but a > linear one , thus two memcpy()s are not needed ? This would require copying the whole buffer with memcpy() when withdrawing a byte. Somehow we tend to reinvent the wheel. Let's use lib/circbuf.c here. Regards Heinrich > > The whole usb keyboard code is not nice and could use some cleanup. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 2/2] usb: kbd: implement special keys 2018-02-22 12:04 [U-Boot] [PATCH 0/2] usb: kbd: implement special keys Heinrich Schuchardt 2018-02-22 12:04 ` [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer Heinrich Schuchardt @ 2018-02-22 12:04 ` Heinrich Schuchardt 2018-02-23 20:59 ` Simon Glass 1 sibling, 1 reply; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-22 12:04 UTC (permalink / raw) To: u-boot Correct support for arrow keys: use the standard xterm escape sequences. Provide support for F1-F12, Insert, Delete, Home, End, Page Up, Page Down. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- common/usb_kbd.c | 121 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 90 insertions(+), 31 deletions(-) diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 706cc350a6..e8adccf219 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -75,12 +75,13 @@ static const unsigned char usb_kbd_num_keypad[] = { }; /* - * map arrow keys to ^F/^B ^N/^P, can't really use the proper - * ANSI sequence for arrow keys because the queuing code breaks - * when a single keypress expands to 3 queue elements + * map arrow keys to CSI A..D */ static const unsigned char usb_kbd_arrow[] = { - 0x6, 0x2, 0xe, 0x10 + 'C', /* 0x4f, right */ + 'D', /* 0x50, left */ + 'B', /* 0x51, down */ + 'A', /* 0x52, up */ }; /* @@ -175,7 +176,9 @@ static void usb_kbd_setled(struct usb_device *dev) static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode, unsigned char modifier, int pressed) { - uint8_t keycode = 0; + uint8_t keycode[8] = {0}; + int i; + int len = 1; /* Key released */ if (pressed == 0) { @@ -191,41 +194,96 @@ static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode, data->repeat_delay = REPEAT_DELAY; } - /* Alphanumeric values */ - if ((scancode > 3) && (scancode <= 0x1d)) { - keycode = scancode - 4 + 'a'; + if (data->flags & USB_KBD_CTRL) { + keycode[0] = scancode - 0x3; + } else if ((scancode > 3) && (scancode <= 0x1d)) { + /* Alphanumeric values */ + keycode[0] = scancode - 4 + 'a'; if (data->flags & USB_KBD_CAPSLOCK) - keycode &= ~CAPITAL_MASK; + keycode[0] &= ~CAPITAL_MASK; if (modifier & (LEFT_SHIFT | RIGHT_SHIFT)) { /* Handle CAPSLock + Shift pressed simultaneously */ - if (keycode & CAPITAL_MASK) - keycode &= ~CAPITAL_MASK; + if (keycode[0] & CAPITAL_MASK) + keycode[0] &= ~CAPITAL_MASK; else - keycode |= CAPITAL_MASK; + keycode[0] |= CAPITAL_MASK; } - } - - if ((scancode > 0x1d) && (scancode < 0x39)) { + } else if ((scancode >= 0x1e) && (scancode <= 0x38)) { /* Shift pressed */ if (modifier & (LEFT_SHIFT | RIGHT_SHIFT)) - keycode = usb_kbd_numkey_shifted[scancode - 0x1e]; + keycode[0] = usb_kbd_numkey_shifted[scancode - 0x1e]; else - keycode = usb_kbd_numkey[scancode - 0x1e]; + keycode[0] = usb_kbd_numkey[scancode - 0x1e]; + } else if ((scancode >= 0x3a) && (scancode <= 0x3d)) { + /* F1 - F4 */ + keycode[0] = 0x1b; + keycode[1] = 'O'; + keycode[2] = scancode - 0x3a + 'P'; + len = 3; + } else if ((scancode >= 0x3e) && (scancode <= 0x45)) { + /* F5 - F12 */ + keycode[0] = 0x1b; + keycode[1] = '['; + if (scancode <= 0x41) { + keycode[2] = '1'; + if (scancode == 0x3e) + /* F5 */ + keycode[3] = '5'; + else + /* F6 - F7 */ + keycode[3] = scancode - 0x3f + '7'; + } else { + keycode[2] = '2'; + if (scancode <= 0x43) + /* F9, F10 */ + keycode[3] = scancode - 0x42 + '0'; + else + /* F11, F12 */ + keycode[3] = scancode - 0x44 + '3'; + } + keycode[4] = '~'; + len = 5; + } else if ((scancode >= 0x49) && (scancode <= 0x4e)) { + /* Insert, Home, Page Up, Delete, End, Page Down */ + len = 4; + keycode[0] = 0x1b; + keycode[1] = '['; + keycode[3] = '~'; + switch (scancode) { + case 0x49: /* Insert */ + keycode[2] = '2'; + break; + case 0x4a: /* Home */ + keycode[2] = 'H'; + len = 3; + break; + case 0x4b: /* Page Up */ + keycode[2] = '5'; + break; + case 0x4c: /* Delete */ + keycode[2] = '3'; + break; + case 0x4d: /* End */ + keycode[2] = 'F'; + len = 3; + break; + case 0x4e: /* Page Down */ + keycode[2] = '6'; + break; + } + } else if ((scancode >= 0x4f) && (scancode <= 0x52)) { + /* Arrow keys */ + keycode[0] = 0x1b; + keycode[1] = '['; + keycode[2] = usb_kbd_arrow[scancode - 0x4f]; + len = 3; + } else if ((scancode >= 0x54) && (scancode <= 0x67)) { + /* Numeric keypad */ + keycode[0] = usb_kbd_num_keypad[scancode - 0x54]; } - /* Arrow keys */ - if ((scancode >= 0x4f) && (scancode <= 0x52)) - keycode = usb_kbd_arrow[scancode - 0x4f]; - - /* Numeric keypad */ - if ((scancode >= 0x54) && (scancode <= 0x67)) - keycode = usb_kbd_num_keypad[scancode - 0x54]; - - if (data->flags & USB_KBD_CTRL) - keycode = scancode - 0x3; - if (pressed == 1) { if (scancode == NUM_LOCK) { data->flags ^= USB_KBD_NUMLOCK; @@ -243,9 +301,10 @@ static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode, } /* Report keycode if any */ - if (keycode) { - debug("%c", keycode); - usb_kbd_put_queue(data, &keycode, 1); + if (keycode[0]) { + for (i = 0; i < len; ++i) + debug("%02x ", (unsigned int)keycode); + usb_kbd_put_queue(data, keycode, len); } return 0; -- 2.14.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 2/2] usb: kbd: implement special keys 2018-02-22 12:04 ` [U-Boot] [PATCH 2/2] usb: kbd: implement special keys Heinrich Schuchardt @ 2018-02-23 20:59 ` Simon Glass 2018-02-24 11:46 ` Heinrich Schuchardt 0 siblings, 1 reply; 16+ messages in thread From: Simon Glass @ 2018-02-23 20:59 UTC (permalink / raw) To: u-boot Hi Heinrich, On 22 February 2018 at 05:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > Correct support for arrow keys: use the standard xterm escape sequences. > > Provide support for F1-F12, Insert, Delete, Home, End, Page Up, Page Down. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > common/usb_kbd.c | 121 +++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 90 insertions(+), 31 deletions(-) > Is there any way this code could be shared with input.c? It has translation tables. Regards, Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 2/2] usb: kbd: implement special keys 2018-02-23 20:59 ` Simon Glass @ 2018-02-24 11:46 ` Heinrich Schuchardt 2018-03-08 22:04 ` Heinrich Schuchardt 0 siblings, 1 reply; 16+ messages in thread From: Heinrich Schuchardt @ 2018-02-24 11:46 UTC (permalink / raw) To: u-boot On 02/23/2018 09:59 PM, Simon Glass wrote: > Hi Heinrich, > > On 22 February 2018 at 05:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> Correct support for arrow keys: use the standard xterm escape sequences. >> >> Provide support for F1-F12, Insert, Delete, Home, End, Page Up, Page Down. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> common/usb_kbd.c | 121 +++++++++++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 90 insertions(+), 31 deletions(-) >> > > Is there any way this code could be shared with input.c? It has > translation tables. Hello Simon, input.c seems to be another incomplete implementation of a keyboard driver. Yes it would make sense to avoid duplicating code. But unfortunately input.c lacks any documentation. As maintainer I hope you can answer the following: When configuring a German keyboard and typing Right-Alt + M the output is 0xE6. This is mju (µ) in code page 437. Wouldn't we always use UTF-8 in U-Boot? The scan codes for PS/2 serial keyboards and USB keyboards differ (cf. "USB HID to PS/2 Scan Code Translation Table" http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/translate.pdf). Is input.c meant to handle PS/2 scan codes or USB scan codes? Regards Heinrich ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 2/2] usb: kbd: implement special keys 2018-02-24 11:46 ` Heinrich Schuchardt @ 2018-03-08 22:04 ` Heinrich Schuchardt 2018-03-19 17:59 ` Simon Glass 0 siblings, 1 reply; 16+ messages in thread From: Heinrich Schuchardt @ 2018-03-08 22:04 UTC (permalink / raw) To: u-boot On 03/08/2018 09:30 PM, Simon Glass wrote: > Hi Heinrich, > > input.c uses PS/2 scan codes at present. However these are somewhat > internal. You should see docs in input.h > > I am not sure of the best approach, but one would be to convert USB > scan codes to PS/2 codes. > > Regards, > Simon > Hello Simon, hello Marek, thanks for the clarification. Between input.c and usb_kbd.c we have the following differences: - Most keycodes are different. - The way ALT, CTRL, SHIFT, META are transferred differ. - Input.c has implemented support for different keyboard layouts (EN, DE) which is not easily expandable to other layouts. So if we wanted to have a common layer this would require a complete rewrite of both input.c and usb_kbd.c. I do not have access to any device using input.c. So I am unable to test any of it. So I would prefer if we could just patch usb_kbd.c to provide the missing special keys and keep the code separate. Having usb_kbd.c in directory common/ looks like a misplacement. It could be put under drivers/input/ or under drivers/usb/hid/. I think in future we should have also mouse and touchscreen as hid drivers. Otherwise using an EFI menu on a tablet will not be possible. So I would prefer moving usb_kbd to drivers/usb/hid/. Best regards Heinrich ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 2/2] usb: kbd: implement special keys 2018-03-08 22:04 ` Heinrich Schuchardt @ 2018-03-19 17:59 ` Simon Glass 2018-03-20 0:12 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Simon Glass @ 2018-03-19 17:59 UTC (permalink / raw) To: u-boot +Tom for comment Hi Heinrich, On 8 March 2018 at 15:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 03/08/2018 09:30 PM, Simon Glass wrote: >> Hi Heinrich, >> >> input.c uses PS/2 scan codes at present. However these are somewhat >> internal. You should see docs in input.h >> >> I am not sure of the best approach, but one would be to convert USB >> scan codes to PS/2 codes. >> >> Regards, >> Simon >> > > Hello Simon, hello Marek, > > thanks for the clarification. > > Between input.c and usb_kbd.c we have the following differences: > > - Most keycodes are different. Yes, although if you call input_add_table() you can supply your own. > - The way ALT, CTRL, SHIFT, META are transferred differ. Indeed > - Input.c has implemented support for different keyboard layouts > (EN, DE) which is not easily expandable to other layouts. That's just the tables though, right? > > So if we wanted to have a common layer this would require a complete rewrite > of both input.c and usb_kbd.c. I hope that in fact only the latter would need a rewrite. But I admit I have not done it. > > I do not have access to any device using input.c. So I am unable to test any > of it. > > So I would prefer if we could just patch usb_kbd.c to provide the missing > special keys and keep the code separate. Actually sandbox has tests for this. Try running with -l and you should get a keyboard on the simulated LCD screen, which uses the input layer. > > Having usb_kbd.c in directory common/ looks like a misplacement. > It could be put under drivers/input/ or under drivers/usb/hid/. > I think in future we should have also mouse and touchscreen as hid drivers. > Otherwise using an EFI menu on a tablet will not be possible. > So I would prefer moving usb_kbd to drivers/usb/hid/. Well yes it would be better out of common. I think it would be possible to use the input_send_keycodes() interface from the USB keyboard driver, given a suitable keymap. There is quite a lot of code that looks much the same, and unifying them was always my intent. But I understand that it is not a trivial undertaking, so I suppose it might be a bridge too far. Regards, Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 2/2] usb: kbd: implement special keys 2018-03-19 17:59 ` Simon Glass @ 2018-03-20 0:12 ` Tom Rini 0 siblings, 0 replies; 16+ messages in thread From: Tom Rini @ 2018-03-20 0:12 UTC (permalink / raw) To: u-boot On Mon, Mar 19, 2018 at 11:59:45AM -0600, Simon Glass wrote: > +Tom for comment > > Hi Heinrich, > > On 8 March 2018 at 15:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 03/08/2018 09:30 PM, Simon Glass wrote: > >> Hi Heinrich, > >> > >> input.c uses PS/2 scan codes at present. However these are somewhat > >> internal. You should see docs in input.h > >> > >> I am not sure of the best approach, but one would be to convert USB > >> scan codes to PS/2 codes. > >> > >> Regards, > >> Simon > >> > > > > Hello Simon, hello Marek, > > > > thanks for the clarification. > > > > Between input.c and usb_kbd.c we have the following differences: > > > > - Most keycodes are different. > > Yes, although if you call input_add_table() you can supply your own. > > > - The way ALT, CTRL, SHIFT, META are transferred differ. > > Indeed > > > - Input.c has implemented support for different keyboard layouts > > (EN, DE) which is not easily expandable to other layouts. > > That's just the tables though, right? > > > > > So if we wanted to have a common layer this would require a complete rewrite > > of both input.c and usb_kbd.c. > > I hope that in fact only the latter would need a rewrite. But I admit > I have not done it. > > > > > I do not have access to any device using input.c. So I am unable to test any > > of it. > > > > So I would prefer if we could just patch usb_kbd.c to provide the missing > > special keys and keep the code separate. > > Actually sandbox has tests for this. Try running with -l and you > should get a keyboard on the simulated LCD screen, which uses the > input layer. > > > > Having usb_kbd.c in directory common/ looks like a misplacement. > > It could be put under drivers/input/ or under drivers/usb/hid/. > > I think in future we should have also mouse and touchscreen as hid drivers. > > Otherwise using an EFI menu on a tablet will not be possible. > > So I would prefer moving usb_kbd to drivers/usb/hid/. > > Well yes it would be better out of common. > > I think it would be possible to use the input_send_keycodes() > interface from the USB keyboard driver, given a suitable keymap. There > is quite a lot of code that looks much the same, and unifying them was > always my intent. But I understand that it is not a trivial > undertaking, so I suppose it might be a bridge too far. Yes, we should probably try and get that code unified, or at least have an idea of how long it would take to do so. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180319/ef748509/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-03-20 0:12 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-22 12:04 [U-Boot] [PATCH 0/2] usb: kbd: implement special keys Heinrich Schuchardt 2018-02-22 12:04 ` [U-Boot] [PATCH 1/2] usb: kbd: allow multibyte sequences to be put into ring buffer Heinrich Schuchardt 2018-02-22 14:20 ` Marek Vasut 2018-02-22 18:06 ` Heinrich Schuchardt 2018-02-22 19:55 ` Marek Vasut 2018-02-22 20:53 ` Heinrich Schuchardt 2018-02-22 23:02 ` Marek Vasut 2018-02-23 5:50 ` Heinrich Schuchardt 2018-02-23 6:04 ` Marek Vasut 2018-02-23 7:09 ` Heinrich Schuchardt 2018-02-22 12:04 ` [U-Boot] [PATCH 2/2] usb: kbd: implement special keys Heinrich Schuchardt 2018-02-23 20:59 ` Simon Glass 2018-02-24 11:46 ` Heinrich Schuchardt 2018-03-08 22:04 ` Heinrich Schuchardt 2018-03-19 17:59 ` Simon Glass 2018-03-20 0:12 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.