Hi Samuel, On Wed, 17 Aug 2022 at 20:16, Samuel Dionne-Riel <sam...@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 <sam...@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 <sam...@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 <sam...@dionne-riel.com> > + * > + * Based on tests for echo: > + * Copyright 2020, Heinrich Schuchadt <xypron.g...@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