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