Re: [SeaBIOS] [PATCH] keyboard: Inject serial port input as keypresses

[Kevin O'Connor]

On Fri, May 20, 2016 at 02:31:09PM +0300, Kyösti Mälkki wrote:
> Maps characters received from serial port as INT16h keypresses.
> This enables making choice of boot media over serial port.
>
> Source extracted from SageBIOS release for PC Engines apu1.
> af6c8ab3b85d1a5a9fbeb41efa30a1ef  pcengines.apu_139_osp.tar.gz

Is there a reason to do this in SeaBIOS versus just using sgabios?
The sgabios code seems to have more features (eg, vga output over
serial port).

See some additional comments below.

[...]
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -411,6 +411,12 @@ menu "BIOS interfaces"
>          default y
>          help
>              Support int 16 keyboard calls.
> config INT16_SERIAL_KEYBOARD
>    bool "Keyboard on serial port"
>    depends on SERIAL && KEYBOARD
>    default n
>    help
>        Translate serial port input to keyboard scancodes.

This would also need to depend on CONFIG_DEBUG_SERIAL (as the code
makes references to CONFIG_DEBUG_SERIAL_PORT), so should probably be
in the debug menu.

[...]
> --- a/src/clock.c
> +++ b/src/clock.c
> @@ -295,6 +295,10 @@ clock_update(void)
>      floppy_tick();
>      usb_check_event();
>      ps2_check_event();
>
> +#if CONFIG_INT16_SERIAL_KEYBOARD
> uart_check_keystrokes();
>
> +#endif

SeaBIOS avoids #if in code - it should read as "if (CONFIG_xyz)
func()".

[...]
> --- a/src/serial.c
> +++ b/src/serial.c
> @@ -315,3 +315,158 @@ handle_17(struct bregs *regs)
>      default:   handle_17XX(regs); break;
>      }
>  }
>
> +#if CONFIG_INT16_SERIAL_KEYBOARD
> +static u8 UartToScanCode[] VAR16 = {
> +//    x0    x1    x2    x3    x4    x5    x6    x7    x8    x9    xa    xb    xc    xd    xe    xf
> +//  =====================================================================================================
> 0x0f, 0x1e, 0x30, 0x2e, 0x20, 0x12, 0x21, 0x22, 0x23, 0x17, 0x24, 0x25, 0x26, 0x32, 0x31, 0x18, // 0x

To fit with SeaBIOS style, the code should be limited to 80 characters
in length.

[...]
> +void
> +uart_check_keystrokes(void)
> +{
> u8 rx_buf[5], rx_bytes = 0, ascii_code = 0, scan_code = 0;
> // check to see if there is a active serial port
> if (inb(CONFIG_DEBUG_SERIAL_PORT + SEROFF_LSR) == 0xFF)
>    return;
> while (inb(CONFIG_DEBUG_SERIAL_PORT + SEROFF_LSR) & 0x01) {
>    if (rx_bytes > sizeof(rx_buf)) {
>        dprintf(1, "uart_check_keystrokes: error too many bytes are available\n");
>        while (inb(CONFIG_DEBUG_SERIAL_PORT + SEROFF_LSR) & 0x01)
>            inb(CONFIG_DEBUG_SERIAL_PORT + SEROFF_DATA);
>        return;
>   }
>   else
>       rx_buf[rx_bytes++] = inb(CONFIG_DEBUG_SERIAL_PORT + SEROFF_DATA);
> }
> if (!rx_bytes)
>    return;
> if (rx_bytes == 1) {

This does not look correct to me - the number of serial port
characters read at one time is likely to be pretty random - it's going
to depend on the speed of the serial port vs the speed of the timer
interrupt that polls the port.  So, this seems like there is a good
chance it would lead to some vt100 sequences not being interpretted
and to some regular keys being dropped due to being misinterpreted as
extended sequences.

FWIW, the sgabios code seems to handle the above correctly.

[...]
> }
> else if (rx_bytes == 2) { // assume it's actually 2 single-byte keystrokes

To keep with SeaBIOS style, "else" statements should be put on the
same line as the preceeding curly bracket.

Thanks,
-Kevin