Re: [PATCH v2 6/6] kern/term: Accept ESC, F4 and holding SHIFT as user interrupt keys

[Daniel Kiper]

On Tue, Apr 07, 2020 at 11:03:51AM +0200, Javier Martinez Canillas wrote:
> From: Hans de Goede <address@hidden>
>
> On some devices the ESC key is the hotkey to enter the BIOS/EFI setup
> screen, making it really hard to time pressing it right. Besides that
> ESC is also pretty hard to discover for a user who does not know it
> will unhide the menu.
>
> This commit makes F4, which was chosen because is not used as a hotkey
> to enter the BIOS setup by any vendor, also interrupt sleeps / stop the
> menu countdown.
>
> This solves the ESC gets into the BIOS setup and also somewhat solves
> the discoverability issue, but leaves the timing issue unresolved.
>
> This commit fixes the timing issue by also adding support for keeping
> SHIFT pressed during boot to stop the menu countdown. This matches
> what Ubuntu is doing, which should also help with discoverability.
>
> Signed-off-by: Hans de Goede <address@hidden>
> Signed-off-by: Javier Martinez Canillas <address@hidden>
> ---
>
> Changes in v2:
> - Fix commit message that had left overs from when F8 was used instead of F4.
> - Update documentation to explain that F4 and SHIFT are used now besides ESC.
> - Also fix comments to reflect that F4 is used now and use proper style.
>
>  docs/grub.texi             | 32 ++++++++++++++++----------------
>  grub-core/commands/sleep.c |  2 +-
>  grub-core/kern/term.c      | 21 +++++++++++++++++++++
>  grub-core/normal/menu.c    |  2 +-
>  include/grub/term.h        |  1 +
>  5 files changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 8e6f9acecfa..25e81b6d57a 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1330,12 +1330,12 @@ menu and then wait for the timeout set by 
> @samp{GRUB_TIMEOUT} to expire
>  before booting the default entry.  Pressing a key interrupts the timeout.
>
>  If this option is set to @samp{countdown} or @samp{hidden}, then, before
> -displaying the menu, GRUB will wait for the timeout set by
> -@samp{GRUB_TIMEOUT} to expire.  If @key{ESC} is pressed during that time, it
> -will display the menu and wait for input.  If a hotkey associated with a
> -menu entry is pressed, it will boot the associated menu entry immediately.
> -If the timeout expires before either of these happens, it will boot the
> -default entry.  In the @samp{countdown} case, it will show a one-line
> +displaying the menu, GRUB will wait for the timeout set by 
> @samp{GRUB_TIMEOUT}
> +to expire.  If @key{ESC} or @key{F4} are pressed, or @key{SHIFT} is held down
> +during that time, it will display the menu and wait for input.  If a hotkey
> +associated with a menu entry is pressed, it will boot the associated menu 
> entry
> +immediately. If the timeout expires before either of these happens, it will
> +boot the default entry.  In the @samp{countdown} case, it will show a 
> one-line
>  indication of the remaining time.
>
>  @item GRUB_DEFAULT_BUTTON
> @@ -1559,16 +1559,16 @@ configurations, but have better replacements:
>
>  @table @samp
>  @item GRUB_HIDDEN_TIMEOUT
> -Wait this many seconds before displaying the menu.  If @key{ESC} is pressed
> -during that time, display the menu and wait for input according to
> -@samp{GRUB_TIMEOUT}.  If a hotkey associated with a menu entry is pressed,
> -boot the associated menu entry immediately.  If the timeout expires before
> -either of these happens, display the menu for the number of seconds
> -specified in @samp{GRUB_TIMEOUT} before booting the default entry.
> +Wait this many seconds before displaying the menu.  If @key{ESC} or @key{F4} 
> are
> +pressed, or @key{SHIFT} is held down during that time, display the menu and 
> wait
> +for input according to @samp{GRUB_TIMEOUT}.  If a hotkey associated with a 
> menu
> +entry is pressed, boot the associated menu entry immediately.  If the timeout
> +expires before either of these happens, display the menu for the number of
> +seconds specified in @samp{GRUB_TIMEOUT} before booting the default entry.
>
>  If you set @samp{GRUB_HIDDEN_TIMEOUT}, you should also set
>  @samp{GRUB_TIMEOUT=0} so that the menu is not displayed at all unless
> -@key{ESC} is pressed.
> +@key{ESC} or @key{F4} are pressed, or @key{SHIFT} is held down.
>
>  This option is unset by default, and is deprecated in favour of the less
>  confusing @samp{GRUB_TIMEOUT_STYLE=countdown} or
> @@ -5162,9 +5162,9 @@ Alias for @code{hashsum --hash sha512 arg @dots{}}. See 
> command @command{hashsum
>
>  @deffn Command sleep [@option{--verbose}] [@option{--interruptible}] count
>  Sleep for @var{count} seconds. If option @option{--interruptible} is given,
> -allow @key{ESC} to interrupt sleep. With @option{--verbose} show countdown
> -of remaining seconds. Exit code is set to 0 if timeout expired and to 1
> -if timeout was interrupted by @key{ESC}.
> +allow pressing @key{ESC}, @key{F4} or holding down @key{SHIFT} to interrupt
> +sleep.  With @option{--verbose} show countdown of remaining seconds. Exit 
> code
> +is set to 0 if timeout expired and to 1 if timeout was interrupted by 
> @key{ESC}.

What about F4 and SHIFT? I think that both should do the same. Could you
clarify this somehow here?

>  @end deffn
>
>
> diff --git a/grub-core/commands/sleep.c b/grub-core/commands/sleep.c
> index e77e7900fac..a1370b710c9 100644
> --- a/grub-core/commands/sleep.c
> +++ b/grub-core/commands/sleep.c
> @@ -55,7 +55,7 @@ grub_interruptible_millisleep (grub_uint32_t ms)
>    start = grub_get_time_ms ();
>
>    while (grub_get_time_ms () - start < ms)
> -    if (grub_getkey_noblock () == GRUB_TERM_ESC)
> +    if (grub_key_is_interrupt (grub_getkey_noblock ()))
>        return 1;
>
>    return 0;
> diff --git a/grub-core/kern/term.c b/grub-core/kern/term.c
> index 93bd3378d18..510fd5b8ddd 100644
> --- a/grub-core/kern/term.c
> +++ b/grub-core/kern/term.c
> @@ -138,6 +138,27 @@ grub_getkeystatus (void)
>    return status;
>  }
>
> +int
> +grub_key_is_interrupt (int key)
> +{
> +  /*
> +   * ESC sometimes is the BIOS setup hotkey and may be hard to discover, also
> +   * check F4, which was chosen because is not used as a hotkey to enter the
> +   * BIOS setup by any vendor.
> +   */
> +  if (key == GRUB_TERM_ESC || key == GRUB_TERM_KEY_F4)
> +    return 1;
> +
> +  /*
> +   * Pressing keys at the right time during boot is hard to time, also allow
> +   * interrupting sleeps / the menu countdown by keeping shift pressed.
> +   */
> +  if (grub_getkeystatus() & 
> (GRUB_TERM_STATUS_LSHIFT|GRUB_TERM_STATUS_RSHIFT))

s/|/ | /

Otherwise LGTM...

Daniel