| Message ID | 20261003-b4-button-live-monitor-v1-1-6a2934e71fbd@protonmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | cmd: button: add live monitor subcommand | expand |
Hi Petr, On 2026-10-03T03:44:03, Petr Hodina via B4 Relay <devnull+petr.hodina.protonmail.com@kernel.org> 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 <petr.hodina@protonmail.com> > > 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
diff --git a/cmd/button.c b/cmd/button.c index 3e6db3f5b8e..daab7b6fb6c 100644 --- a/cmd/button.c +++ b/cmd/button.c @@ -8,7 +8,11 @@ #include <command.h> #include <dm.h> #include <button.h> +#include <stdio.h> +#include <time.h> +#include <vsprintf.h> #include <dm/uclass-internal.h> +#include <linux/delay.h> static const char *const state_label[] = { [BUTTON_OFF] = "off", @@ -53,6 +57,48 @@ static int list_buttons(void) return 0; } +static int monitor_buttons(unsigned long timeout_ms) +{ + ulong start = get_timer(0); + ulong elapsed; + + /* Clear screen and move cursor to row 1, column 1 */ + puts("\x1b[2J\x1b[H"); + puts("Button monitor\r\n\r\n"); + + for (;;) { + struct udevice *dev; + + elapsed = get_timer(start); + if (elapsed >= timeout_ms) + break; + + puts("\x1b[3;1H"); + printf("%lu.%lus left\x1b[K\r\n", + (timeout_ms - elapsed) / 1000, + ((timeout_ms - elapsed) % 1000) / 100); + puts("--------------------\r\n"); + + 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"); + } + + mdelay(100); + } + + return 0; +} + int do_button(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { const char *button_label; @@ -65,6 +111,13 @@ int do_button(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) button_label = argv[1]; if (strncmp(button_label, "list", 4) == 0) return list_buttons(); + if (strncmp(button_label, "monitor", 7) == 0) { + unsigned long timeout_ms = 5000; + + if (argc >= 3) + timeout_ms = dectoul(argv[2], NULL); + return monitor_buttons(timeout_ms); + } ret = button_get_by_label(button_label, &dev); if (ret) { @@ -78,8 +131,10 @@ int do_button(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) } U_BOOT_CMD( - button, 2, 1, do_button, + button, 3, 1, do_button, "manage buttons", "<button_label> \tGet button state\n" - "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)" );