Hi Petr, On 2026-10-03T03:44:03, Petr Hodina via B4 Relay <[email protected]> wrote: > cmd: button: add live monitor subcommand > > `button list` prints state once and exits. The new subcommand `monitor` > loops the input states with default timeout 5s so live verification > on the device is easier.
Please can you avoid backticks in the commit message and reword 'loops the input states'? Something like: "Add a 'monitor' subcommand which redraws the state of all buttons every 100ms until a timeout (default 5s) expires, so that buttons can be checked live on a device". > > Signed-off-by: Petr Hodina <[email protected]> > > cmd/button.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 57 insertions(+), 2 deletions(-) > diff --git a/cmd/button.c b/cmd/button.c > @@ -53,6 +57,48 @@ static int list_buttons(void) > + for (;;) { > + struct udevice *dev; > + > + elapsed = get_timer(start); > + if (elapsed >= timeout_ms) > + break; There is no way to stop this early. Please can you check ctrlc() in the loop, as other long-running commands do (e.g. cmd/mem.c)? It might also be useful to let a timeout of 0 mean 'run until Ctrl-C', since a 5s default leaves little time to press anything. > diff --git a/cmd/button.c b/cmd/button.c > @@ -53,6 +57,48 @@ static int list_buttons(void) > + /* Clear screen and move cursor to row 1, column 1 */ > + puts("\x1b[2J\x1b[H"); > + puts("Button monitor\r\n\r\n"); Please use the macros from include/ansi.h (ANSI_CLEAR_CONSOLE, ANSI_CURSOR_POSITION, ANSI_CLEAR_LINE_TO_END) rather than raw escape codes. Also, the console layer already handles '\n', so '\r\n' isn't needed here or below. > diff --git a/cmd/button.c b/cmd/button.c > @@ -53,6 +57,48 @@ static int list_buttons(void) > + for (uclass_first_device(UCLASS_BUTTON, &dev); > + dev; > + uclass_next_device(&dev)) { > + struct button_uc_plat *plat = dev_get_uclass_plat(dev); > + int state; > + > + if (!plat->label) > + continue; > + state = button_get_state(dev); > + > + printf("%-15s %-3s\x1b[K\r\n", plat->label, > + (state == BUTTON_ON) ? "ON" : "off"); This shows any error from button_get_state() as 'off', which is misleading. Please can you print the error as list_buttons() does, and reuse the existing state_label[] array? At present 'list' prints 'on' while 'monitor' prints 'ON'. Also, uclass_first_device() probes every button, whereas 'list' uses uclass_find_first_device() and shows '<inactive>' for unprobed devices. Probing is probably what you want here, but a button whose probe fails is then skipped silently. Since the device list can't change while the loop runs, it may be simpler to collect the devices once before the loop. > diff --git a/cmd/button.c b/cmd/button.c > @@ -65,6 +111,13 @@ int do_button(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > + if (strncmp(button_label, "monitor", 7) == 0) { > + unsigned long timeout_ms = 5000; > + > + if (argc >= 3) > + timeout_ms = dectoul(argv[2], NULL); With strncmp(), a button labelled e.g. monitor_key can no longer be read with 'button <label>', so please use strcmp() here. Also, an invalid argument silently becomes a timeout of 0 and the command exits immediately; please reject it with CMD_RET_USAGE instead. > diff --git a/cmd/button.c b/cmd/button.c > @@ -78,8 +131,10 @@ int do_button(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > - "button list\t\tShow a list of buttons" > + "button list\t\tShow a list of buttons\n" > + "button monitor [timeout_ms]\tLive table, updates every 100ms,\n" > + " \tquits on timeout (default 5000ms)" Since you are adding a new feature, please can you update doc/usage/cmd/button.rst (synopsis, description and return value) and add a test to test/py/tests/test_button.py? Even 'button monitor 200' on sandbox would show that the output includes button1 and button2. Regards, Simon
