[U-Boot] [PATCH 2/2] New command bootmenu: ANSI terminal Boot Menu support

Marek Vasut
2012-06-03T10:06:55Z

Dear Pali Roh?r,

> Signed-off-by: Pali Roh?r <pali.rohar@gmail.com>

Try keeping the subject line short and add a patch description instead. Btw. Try
avoiding unicode characters in the patch. Also, is "Pali" your real name that
you have on your IDs etc. ?

> ---
> common/Makefile | 1 +
> common/cmd_bootmenu.c | 446
> ++++++++++++++++++++++++++++++++++++++++++++++ doc/README.bootmenu |
> 61 +++++++
> include/common.h | 20 +++
> include/config_cmd_all.h | 1 +
> 5 files changed, 529 insertions(+)
> create mode 100644 common/cmd_bootmenu.c
> create mode 100644 doc/README.bootmenu
>
> diff --git a/common/Makefile b/common/Makefile
> index 6e23baa..b9d4a4a 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -69,6 +69,7 @@ COBJS-$(CONFIG_CMD_SOURCE) += cmd_source.o
> COBJS-$(CONFIG_CMD_BDI) += cmd_bdinfo.o
> COBJS-$(CONFIG_CMD_BEDBUG) += bedbug.o cmd_bedbug.o
> COBJS-$(CONFIG_CMD_BMP) += cmd_bmp.o
> +COBJS-$(CONFIG_CMD_BOOTMENU) += cmd_bootmenu.o
> COBJS-$(CONFIG_CMD_BOOTLDR) += cmd_bootldr.o
> COBJS-$(CONFIG_CMD_CACHE) += cmd_cache.o
> COBJS-$(CONFIG_CMD_CONSOLE) += cmd_console.o
> diff --git a/common/cmd_bootmenu.c b/common/cmd_bootmenu.c
> new file mode 100644
> index 0000000..935b60a
> --- /dev/null
> +++ b/common/cmd_bootmenu.c
> @@ -0,0 +1,446 @@
> +/*
> + * (C) Copyright 2011-2012 Pali Roh?r <pali.rohar@gmail.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <menu.h>
> +#include <hush.h>
> +#include <watchdog.h>
> +#include <malloc.h>
> +#include <linux/string.h>
> +
> +struct bootmenu_entry {
> + int num; /* unique number 0..99 */
> + char key[3]; /* key idetifier of number */
> + char *title; /* title of entry */
> + char *command; /* hush command of entry */
> + struct bootmenu_data *menu; /* this bootmenu */
> + struct bootmenu_entry *next; /* next menu entry (num+1) */
> +};
> +
> +struct bootmenu_data {
> + int delay; /* delay for autoboot */
> + int active; /* active menu entry */
> + int count; /* total count of menu entries */
> + struct bootmenu_entry *first; /* first menu entry */
> +};
> +
> +static char *bootmenu_getoption(int n)
> +{
> + char name[] = "bootmenu_\0\0";
> +
> + if (n < 0 || n > 99)

Why add this artificial limit?

> + return NULL;
> +
> + sprintf(name+9, "%d", n);
> + return getenv(name);
> +}
> +
> +static void bootmenu_print_entry(void *data)
> +{
> + struct bootmenu_entry *entry = data;
> + int reverse = (entry->menu->active == entry->num);

Why not just write it below into the condition?

> +
> + printf(ANSI_CURSOR_POSITION, entry->num + 4, 1);

What are all these artificial numbers here?

> +
> + if (reverse)
> + puts(ANSI_COLOR_REVERSE);
> +
> + puts(" ");
> + puts(entry->title);
> + puts(ANSI_CLEAR_LINE_TO_END);
> +
> + if (reverse)
> + puts(ANSI_COLOR_RESET);
> +}
> +
> +static char *bootmenu_choice_entry(void *data)
> +{
> + struct bootmenu_data *menu = data;
> +
> + int key = 0; /* 0 - NONE, 1 - UP, 2 - DOWN, 3 - SELECT */
> + int esc = 0;

Can't the spaghetti function below be split into multiple smaller ones? You know
the rule of the thumb, if the code spans multiple screens, something's seriously
wrong.

> + while (1) {
> +
> + if (menu->delay >= 0) {
> +
> + if (menu->delay > 0) {
> + printf(ANSI_CURSOR_POSITION, menu->count+5, 1);
> + printf(" Hit any key to stop autoboot: %2d ",
> + menu->delay);
> + }
> +
> + while (menu->delay > 0) {
> +
> + int i;
> + for (i = 0; i < 100; ++i) {
> + if (tstc()) {

if (!tstc())
continue;

... do your job
break;

You'll get one less level of indent and much more readable code.

> + menu->delay = -1;
> + key = getc();
> +
> + if (key == '\e') {
> + esc = 1;
> + key = 0;
> + } else if (key == '\r')
> + key = 3;
> + else
> + key = 0;
> +
> + break;
> +
> + }
> +
> + WATCHDOG_RESET();
> + udelay(10000);

mdelay() ?

> +
> + }
> +
> + if (menu->delay < 0)
> + break;
> +
> + --menu->delay;

Why do you use predecrement in the above statement? It's pointless here, simply
postdecrement it.

> + printf("\b\b\b%2d ", menu->delay);
> +
> + }
> +
> + if (menu->delay == 0)
> + key = 3;
> +
> + } else {
> +
> + while (!tstc()) {
> + WATCHDOG_RESET();
> + udelay(10000);
> + }
> +
> + key = getc();
> +
> + if (esc == 0) {
> +
> + if (key == '\e') {
> + esc = 1;
> + key = 0;
> + }
> +
> + } else if (esc == 1) {
> +
> + if (key == '[') {
> + esc = 2;
> + key = 0;
> + } else
> + esc = 0;
> +
> + } else if (esc == 2 || esc == 3) {
> +
> + if (esc == 2 && key == '1') {
> + esc = 3;
> + key = 0;
> + } else
> + esc = 0;
> +
> + if (key == 'A')
> + key = 1;
> + else if (key == 'B')
> + key = 2;
> + else
> + key = 0;
> +
> + }
> +
> + if (key == '\r')
> + key = 3;
> +
> + }
> +
> + if (key == 1) {
> + if (menu->active > 0)
> + --menu->active;
> + return ""; /* invalid menu entry, regenerate menu */
> + } else if (key == 2) {
> + if (menu->active < menu->count-1)
> + ++menu->active;

Let me guess ... they taught you at the compiler class that predecrement is
faster than postdecrement, right? Not so much on anything but intel ... and even
on intel, GCC does the decision about putting the instructions there properly to
create the fastest possible code anyway.

> + return ""; /* invalid menu entry, regenerate menu */
> + } else if (key == 3) {
> + int i;
> + struct bootmenu_entry *iter = menu->first;
> + for (i = 0; i < menu->active; ++i)
> + iter = iter->next;
> + return iter->key;
> + }
> +
> + }
> +
> + /* never happends */

Add debug() clause maybe ?

> + return NULL;
> +}
> +
> +static struct bootmenu_data *bootmenu_create(int delay)
> +{
> + int i = 0;
> + const char *option;
> + struct bootmenu_data *menu;
> + struct bootmenu_entry *iter = NULL;
> +
> + menu = malloc(sizeof(struct bootmenu_data));
> + if (!menu)
> + return NULL;
> +
> + menu->delay = delay;
> + menu->active = 0;
> + menu->first = NULL;
> +
> + while ((option = bootmenu_getoption(i))) {
> +
> + int len;
> + char *sep;
> + struct bootmenu_entry *entry;
> +
> + sep = strchr(option, '=');
> + if (!sep)
> + break;
> +
> + entry = malloc(sizeof(struct bootmenu_entry));
> + if (!entry)
> + goto cleanup;
> +
> + len = sep-option;
> + entry->title = malloc(len+1);
> + if (!entry->title) {
> + free(entry);
> + goto cleanup;
> + }
> + memcpy(entry->title, option, len);
> + entry->title[len] = 0;
> +
> + len = strlen(sep+1);
> + entry->command = malloc(len+1);
> + if (!entry->command) {
> + free(entry->title);
> + free(entry);
> + goto cleanup;
> + }
> + memcpy(entry->command, sep+1, len);
> + entry->command[len] = 0;
> +
> + sprintf(entry->key, "%d", i);
> +
> + entry->num = i;
> + entry->menu = menu;
> + entry->next = NULL;
> +
> + if (!iter)
> + menu->first = entry;
> + else
> + iter->next = entry;
> +
> + iter = entry;
> + ++i;
> +
> + if (i >= 100)
> + break;
> +
> + }
> +
> + /* Add U-Boot console entry at the end */
> + if (i < 100) {
> + struct bootmenu_entry *entry = malloc(
> + sizeof(struct bootmenu_entry));
> + if (!entry)
> + goto cleanup;
> +
> + entry->title = strdup("U-Boot console");
> + if (!entry->title) {
> + free(entry);
> + goto cleanup;
> + }
> +
> + entry->command = strdup("");
> + if (!entry->command) {
> + free(entry->title);
> + free(entry);
> + goto cleanup;
> + }
> +
> + entry->num = i;
> + entry->menu = menu;
> + entry->next = NULL;
> +
> + if (!iter)
> + menu->first = entry;
> + else
> + iter->next = entry;
> +
> + iter = entry;
> + ++i;
> +
> + }
> +
> + menu->count = i;
> + return menu;
> +
> +cleanup:
> + iter = menu->first;
> + while (iter) {
> + struct bootmenu_entry *next = iter->next;
> + free(iter->title);
> + free(iter->command);
> + free(iter);
> + iter = next;
> + }
> + free(menu);
> + return NULL;
> +}
> +
> +static void bootmenu_destroy(struct bootmenu_data *menu)
> +{
> + struct bootmenu_entry *iter = menu->first;
> + while (iter) {
> + struct bootmenu_entry *next = iter->next;

I don't like how you declare variables in the middle of code. It has sideeffects
and looking them up is really troublesome.

> + free(iter->title);
> + free(iter->command);
> + free(iter);
> + iter = next;
> + }
> + free(menu);
> +}
> +
> +static void bootmenu_show(int delay)
> +{
> + int init = 0;
> + void *choice = NULL;
> + char *title = NULL;
> + char *command = NULL;
> + struct menu *menu;
> + struct bootmenu_data *bootmenu;
> + struct bootmenu_entry *iter;
> +
> + /* If delay is 0 do not create menu, just run first entry */
> + if (delay == 0) {
> + char *option, *sep;
> + option = bootmenu_getoption(0);
> + if (!option)
> + return;
> + sep = strchr(option, '=');
> + if (!sep)
> + return;
> + run_command(sep+1, 0);
> + return;
> + }
> +
> + bootmenu = bootmenu_create(delay);
> + if (!bootmenu)
> + return;
> +
> + menu = menu_create(NULL, bootmenu->delay, (bootmenu->delay >= 0),
> + bootmenu_print_entry, bootmenu_choice_entry, bootmenu);
> + if (!menu)
> + return;
> +
> + for (iter = bootmenu->first; iter; iter = iter->next)
> + if (!menu_item_add(menu, iter->key, iter))
> + goto cleanup;
> +
> + /* Default menu entry is always first */
> + menu_default_set(menu, "0");
> +
> + puts(ANSI_CURSOR_HIDE);
> + puts(ANSI_CLEAR_CONSOLE);
> + printf(ANSI_CURSOR_POSITION, 1, 1);
> +
> + init = 1;
> +
> + if (menu_get_choice(menu, &choice)) {
> + iter = choice;
> + title = strdup(iter->title);
> + command = strdup(iter->command);
> + }
> +
> +cleanup:
> + menu_destroy(menu);
> + bootmenu_destroy(bootmenu);
> +
> + if (init) {
> + puts(ANSI_CURSOR_SHOW);
> + puts(ANSI_CLEAR_CONSOLE);
> + printf(ANSI_CURSOR_POSITION, 1, 1);
> + }
> +
> + if (title && command) {
> + printf("Starting entry '%s'\n", title);
> + free(title);
> + run_command(command, 0);
> + free(command);
> + }
> +}
> +
> +void menu_display_statusline(void *data)
> +{
> + struct bootmenu_data *menu = data;
> +
> + printf(ANSI_CURSOR_POSITION, 1, 1);
> + puts(ANSI_CLEAR_LINE);
> + printf(ANSI_CURSOR_POSITION, 2, 1);
> + puts(" *** U-Boot BOOT MENU ***");
> + puts(ANSI_CLEAR_LINE_TO_END);
> + printf(ANSI_CURSOR_POSITION, 3, 1);
> + puts(ANSI_CLEAR_LINE);
> +
> + printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
> + puts(ANSI_CLEAR_LINE);
> + printf(ANSI_CURSOR_POSITION, menu->count + 6, 1);
> + puts(" Press UP/DOWN to move, ENTER to select");
> + puts(ANSI_CLEAR_LINE_TO_END);
> + printf(ANSI_CURSOR_POSITION, menu->count + 7, 1);
> + puts(ANSI_CLEAR_LINE);
> +}
> +
> +int menu_show(int bootdelay)
> +{
> + bootmenu_show(bootdelay);
> + return -1; /* -1 - abort boot and run monitor code */
> +}
> +
> +int do_bootmenu(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> + char *delay_str = NULL;
> + int delay = 10;
> +
> + if (argc >= 2)
> + delay_str = argv[1];
> +
> + if (!delay_str)
> + delay_str = getenv("bootmenu_delay");
> +
> + if (delay_str)
> + delay = (int)simple_strtol(delay_str, NULL, 10);
> +
> + bootmenu_show(delay);
> + return 0;
> +}
> +
> +U_BOOT_CMD(
> + bootmenu, 2, 1, do_bootmenu,
> + "ANSI terminal bootmenu",
> + "[delay]\n"
> + " - show ANSI terminal bootmenu with autoboot delay (default 10s)"
> +);
> diff --git a/doc/README.bootmenu b/doc/README.bootmenu
> new file mode 100644
> index 0000000..69ef93b
> --- /dev/null
> +++ b/doc/README.bootmenu
> @@ -0,0 +1,61 @@
> +/*
> + * (C) Copyright 2011-2012 Pali Roh?r <pali.rohar@gmail.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +This is ANSI terminal BootMenu command. It is extension to generic menu,
> +which provide output for ANSI terminals.
> +
> +Configuration is done via env variables bootmenu_delay and bootmenu_<num>:
> +
> + bootmenu_delay=<delay>
> + bootmenu_<num>=" =<commands>"
> +
> + (title and commands are separated by first char '=')
> +
> + <delay> is delay in seconds of autobooting first entry
> + <num> is boot menu entry, starting from zero
> + is text shown in boot screen
> + <commands> are commands which will be executed when menu entry is
> selected +
> +First argument of bootmenu command override bootmenu_delay env
> +If env bootmenu_delay or bootmenu arg is not specified, delay is 10
> seconds +If delay is 0, no entry will be shown on screen and first will be
> called +If delay is less then 0, no autoboot delay will be used
> +Boot Menu will stop finding next menu entry if last was not defined
> +Boot Menu always add menu entry "U-Boot console" at end of all entries
> +
> +Example using:
> +
> + setenv bootmenu_0 Boot 1. kernel=bootm 0x82000000 # Set first menu
> entry + setenv bootmenu_1 Boot 2. kernel=bootm 0x83000000 # Set second
> menu entry + setenv bootmenu_2 Reset board=reset # Set
> third menu entry + setenv bootmenu_3 U-Boot boot order=boot #
> Set fourth menu entry + setenv bootmenu_4 # Empty string is end of all
> bootmenu entries + bootmenu 20 # Run bootmenu with autoboot delay
> 20s
> +
> +
> +To enable ANSI bootmenu comamnd add these definitions to board code:
> +
> + #define CONFIG_BOOTDELAY 30
> + #define CONFIG_AUTOBOOT_KEYED
> + #define CONFIG_MENU
> + #define CONFIG_MENU_SHOW
> + #define CONFIG_CMD_BOOTMENU
> diff --git a/include/common.h b/include/common.h
> index 8564a65..59ecdb9 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -761,6 +761,26 @@ void clear_ctrlc (void); /* clear the Control-C
> condition */ int disable_ctrlc (int); /* 1 to disable, 0 to enable
> Control-C detect */
>
> /*
> + * ANSI terminal
> + */
> +
> +#define ANSI_CURSOR_UP "\e[%dA"
> +#define ANSI_CURSOR_DOWN "\e[%dB"
> +#define ANSI_CURSOR_FORWARD "\e[%dC"
> +#define ANSI_CURSOR_BACK "\e[%dD"
> +#define ANSI_CURSOR_NEXTLINE "\e[%dE"
> +#define ANSI_CURSOR_PREVIOUSLINE "\e[%dF"
> +#define ANSI_CURSOR_COLUMN "\e[%dG"
> +#define ANSI_CURSOR_POSITION "\e[%d;%dH"
> +#define ANSI_CURSOR_SHOW "\e[?25h"
> +#define ANSI_CURSOR_HIDE "\e[?25l"
> +#define ANSI_CLEAR_CONSOLE "\e[2J"
> +#define ANSI_CLEAR_LINE_TO_END "\e[0K"
> +#define ANSI_CLEAR_LINE "\e[2K"
> +#define ANSI_COLOR_RESET "\e[0m"
> +#define ANSI_COLOR_REVERSE "\e[7m"

Isn't it possible to extend the normal bootmenu code with the ansi command
sequences and be done with it?

> +/*
> * STDIO based functions (can always be used)
> */
> /* serial stuff */
> diff --git a/include/config_cmd_all.h b/include/config_cmd_all.h
> index 55f4f7a..d47a860 100644
> --- a/include/config_cmd_all.h
> +++ b/include/config_cmd_all.h
> @@ -19,6 +19,7 @@
> #define CONFIG_CMD_BEDBUG /* Include BedBug Debugger */
> #define CONFIG_CMD_BMP /* BMP support */
> #define CONFIG_CMD_BOOTD /* bootd */
> +#define CONFIG_CMD_BOOTMENU /* ANSI terminal Boot Menu */
> #define CONFIG_CMD_BOOTZ /* boot zImage */
> #define CONFIG_CMD_BSP /* Board Specific functions */
> #define CONFIG_CMD_CACHE /* icache, dcache */