[PATCH v3] cmd: Add pause command
Simon Glass
sjg at chromium.org
Thu Aug 18 19:49:47 CEST 2022
Hi Samuel,
On Wed, 17 Aug 2022 at 20:16, Samuel Dionne-Riel <samuel at dionne-riel.com>
wrote:
>
> This command is being introduced with the goal of allowing user-friendly
> "generic use case" U-Boot builds to pause until user input under some
> situations.
>
> The main use case would be when a boot failure happens, to pause until
> the user has had time to acknowledge the current state.
>
> Tested using:
>
> make && ./u-boot -v -T -c 'ut lib lib_test_hush_pause'
Looks good. I suppose you know that if you leave out the -v you don't see
the output.
>
> Signed-off-by: Samuel Dionne-Riel <samuel at dionne-riel.com>
> ---
> Hi,
>
> I hit a snag when sending v2, and lines ended-up wrapped. In addition
> I also forgot to include the changelog.
>
> It seems the patch on patchwork was also broken in a way it was not
> on my end. I do not know why the lines ended-up mis-ordered.
>
> Please tell if I should have used RESEND v2, since technically the
> content of the patch are unchanged.
>
> Changes for v3
> - No functional change in patch
> - Sent with lines unwrapped
> - Added changelog
>
> Changes for v2
> - Added test, as requested by Tom
> - Made CMD_PAUSE default n
>
> ---
> cmd/Kconfig | 7 +++++
> cmd/Makefile | 1 +
> cmd/pause.c | 35 +++++++++++++++++++++
> configs/sandbox64_defconfig | 1 +
> configs/sandbox_defconfig | 1 +
> test/cmd/Makefile | 3 ++
> test/cmd/test_pause.c | 62 +++++++++++++++++++++++++++++++++++++
> 7 files changed, 110 insertions(+)
> create mode 100644 cmd/pause.c
> create mode 100644 test/cmd/test_pause.c
Please also add doc/usage/cmd/pause.rst
(check with 'make htmldocs')
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 3625ff2a50b..e774670a35c 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1961,6 +1961,13 @@ config CMD_GETTIME
> milliseconds. See also the 'bootstage' command which provides
more
> flexibility for boot timing.
>
> +config CMD_PAUSE
> + bool "pause command"
> + default n
The default is n so drop this
> + help
> + Delay execution waiting for any user input.
> + Useful to allow the user to read a failure log.
> +
> config CMD_RNG
> bool "rng command"
> depends on DM_RNG
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 5e43a1e022e..98a6224bdc1 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -102,6 +102,7 @@ obj-$(CONFIG_CMD_MFSL) += mfsl.o
> obj-$(CONFIG_CMD_MII) += mii.o
> obj-$(CONFIG_CMD_MISC) += misc.o
> obj-$(CONFIG_CMD_MDIO) += mdio.o
> +obj-$(CONFIG_CMD_PAUSE) += pause.o
Hmm the sorting has got a bit wonky here over time!
> obj-$(CONFIG_CMD_SLEEP) += sleep.o
> obj-$(CONFIG_CMD_MMC) += mmc.o
> obj-$(CONFIG_CMD_OPTEE_RPMB) += optee_rpmb.o
> diff --git a/cmd/pause.c b/cmd/pause.c
> new file mode 100644
> index 00000000000..07bf346f3d1
> --- /dev/null
> +++ b/cmd/pause.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2021
> + * Samuel Dionne-Riel <samuel at dionne-riel.com>
> + */
> +
> +#include <command.h>
> +#include <stdio.h>
> +
> +static int do_pause(struct cmd_tbl *cmdtp, int flag, int argc, char
*const argv[])
> +{
> + char *message = "Press any key to continue...";
> +
> + if (argc > 2)
> + return CMD_RET_USAGE;
I think this happened automatically since you specify 2 as the max args
below.
> +
> + if (argc == 2)
> + message = argv[1];
> +
> + /* No newline, so it sticks to the bottom of the screen */
> + printf("%s", message);
> +
> + /* Wait on "any" key... */
> + (void) getchar();
> +
> + /* Since there was no newline, we need it now. */
can drop the .
> + printf("\n");
> +
> + return CMD_RET_SUCCESS;
> +}
> +
> +U_BOOT_CMD(pause, 2, 1, do_pause,
> + "delay until user input",
> + "pause [prompt] - Wait until users presses any key. [prompt] can
be used to customize the message.\n"
> +);
> diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> index 6553568e768..0af582d642d 100644
> --- a/configs/sandbox64_defconfig
> +++ b/configs/sandbox64_defconfig
> @@ -67,6 +67,7 @@ CONFIG_CMD_BMP=y
> CONFIG_CMD_EFIDEBUG=y
> CONFIG_CMD_RTC=y
> CONFIG_CMD_TIME=y
> +CONFIG_CMD_PAUSE=y
> CONFIG_CMD_TIMER=y
> CONFIG_CMD_SOUND=y
> CONFIG_CMD_QFW=y
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index eba7bcbb483..d856d9b0942 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -96,6 +96,7 @@ CONFIG_CMD_BOOTCOUNT=y
> CONFIG_CMD_EFIDEBUG=y
> CONFIG_CMD_RTC=y
> CONFIG_CMD_TIME=y
> +CONFIG_CMD_PAUSE=y
> CONFIG_CMD_TIMER=y
> CONFIG_CMD_SOUND=y
> CONFIG_CMD_QFW=y
> diff --git a/test/cmd/Makefile b/test/cmd/Makefile
> index c331757425e..1bb02d93a23 100644
> --- a/test/cmd/Makefile
> +++ b/test/cmd/Makefile
> @@ -5,6 +5,9 @@
> ifdef CONFIG_HUSH_PARSER
> obj-$(CONFIG_CONSOLE_RECORD) += test_echo.o
> endif
> +ifdef CONFIG_CONSOLE_RECORD
> +obj-$(CONFIG_CMD_PAUSE) += test_pause.o
> +endif
> obj-y += mem.o
> obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
> obj-$(CONFIG_CMD_FDT) += fdt.o
> diff --git a/test/cmd/test_pause.c b/test/cmd/test_pause.c
> new file mode 100644
> index 00000000000..9c55c6302bc
> --- /dev/null
> +++ b/test/cmd/test_pause.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Tests for pause command
> + *
> + * Copyright 2022, Samuel Dionne-Riel <samuel at dionne-riel.com>
> + *
> + * Based on tests for echo:
> + * Copyright 2020, Heinrich Schuchadt <xypron.glpk at gmx.de>
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <asm/global_data.h>
> +#include <display_options.h>
That should go up one. See:
https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files
> +#include <test/lib.h>
> +#include <test/test.h>
> +#include <test/ut.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct test_data {
> + char *cmd;
> + char *expected;
> + int expected_ret;
> +};
> +
> +static struct test_data pause_data[] = {
> + /* Test default message */
> + {"pause",
> + "Press any key to continue...",
> + 0},
> + /* Test provided message */
> + {"pause 'Prompt for pause...'",
> + "Prompt for pause...",
> + 0},
> + /* Test providing more than one params */
> + {"pause a b",
> + "pause - delay until user input", /* start of help message */
> + 1},
I think this would be better written out fully in code without any data.
The test below is a little hard to understand.
> +};
> +
> +static int lib_test_hush_pause(struct unit_test_state *uts)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(pause_data); ++i) {
> + ut_silence_console(uts);
Don't need this - it is the default on sandbox
> + console_record_reset_enable();
> + /* Only cook a newline when the command is expected to
pause. */
> + if (pause_data[i].expected_ret == 0)
if (!pause_data[i].expected_ret)
> + console_in_puts("\n");
> + ut_asserteq(pause_data[i].expected_ret,
run_command(pause_data[i].cmd, 0));
> + ut_unsilence_console(uts);
Don't need this - it is the default on sandbox
> + console_record_readline(uts->actual_str,
> + sizeof(uts->actual_str));
> + ut_asserteq_str(pause_data[i].expected, uts->actual_str);
> + ut_asserteq(pause_data[i].expected_ret,
ut_check_console_end(uts));
> + }
> + return 0;
> +}
> +
I know it is strange but we try to keep the LIB_TEST() thing right update
against the function it relates to, so drop this newline
> +LIB_TEST(lib_test_hush_pause, 0);
> --
> 2.35.1
Regards,
Simon
More information about the U-Boot
mailing list