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

[Wolfgang Denk]

Dear Pali Rohár,

In message <1351769953-13560-3-git-send-email-pali.rohar at gmail.com> you wrote:
> This patch adding ANSI terminal bootmenu command. It is extension to generic
> menu which provide output for ANSI terminals.
...
> +					if (key == '\e') {
> +						esc = 1;
> +						key = 0;
> +					} else if (key == '\r')
> +						key = 3;
> +					else
> +						key = 0;

use switch() ?

> +			if (esc == 0) {
> +
> +				/* Start of ANSI escape sequence */
> +				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;
> +
> +				/* key up was pressed */
> +				if (key == 'A')
> +					key = 1;
> +				/* key down was pressed */
> +				else if (key == 'B')
> +					key = 2;
> +				/* other key was pressed */
> +				else
> +					key = 0;
> +
> +			}

use switch() ?

Can we please avoid using hard-coded magic constants like 'A', 'B'
etc. here?

> +		/* key up */
> +		if (key == 1) {
> +			if (menu->active > 0)
> +				--menu->active;
> +			return ""; /* invalid menu entry, regenerate menu */
> +		/* key down */
> +		} else if (key == 2) {
> +			if (menu->active < menu->count-1)
> +				++menu->active;
> +			return ""; /* invalid menu entry, regenerate menu */
> +		/* enter */
> +		} 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;
> +		}

use switch() ?

> +	/* never happends */

Typo.

> +	while ((option = bootmenu_getoption(i))) {
> +
> +		sep = strchr(option, '=');
> +		if (!sep)
> +			break;

Is there any specific reason for inventing yet another data format
here?  Can we not for example re-use what we already have in the
hwconfig command, and use common code for parsing the format?

Using a '=' as separator here is not a good idea, IMO.



> +	/* Add U-Boot console entry at the end */
> +	if (i < 100) {

Magic constant 100 ?

> +cleanup:
> +	iter = menu->first;
> +	while (iter) {
> +		entry = iter->next;
> +		free(iter->title);
> +		free(iter->command);
> +		free(iter);
> +		iter = entry;
> +	}
> +	free(menu);

Make this a separate function?

> +static void bootmenu_destroy(struct bootmenu_data *menu)
> +{
> +	struct bootmenu_entry *iter = menu->first;
> +	struct bootmenu_entry *next;
> +	while (iter) {
> +		next = iter->next;
> +		free(iter->title);
> +		free(iter->command);
> +		free(iter);
> +		iter = next;
> +	}
> +	free(menu);

and use it here again?

> +	/* If delay is 0 do not create menu, just run first entry */
> +	if (delay == 0) {
> +		option = bootmenu_getoption(0);
> +		if (!option)
> +			return;
> +		sep = strchr(option, '=');
> +		if (!sep)
> +			return;

That would be an error condition, would it not?  Should this not raise
some error message, then?

> +	for (iter = bootmenu->first; iter; iter = iter->next)
> +		if (!menu_item_add(menu, iter->key, iter))
> +			goto cleanup;

Need braces for multi-line "for".

> +int menu_show(int bootdelay)
> +{
> +	bootmenu_show(bootdelay);
> +	return -1; /* -1 - abort boot and run monitor code */
> +}

If this function never returns anything else, why do we need the
return value at all?

> +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;
> +}

Umm... don't we handle any errors at all?

> +This is ANSI terminal BootMenu command. It is extension to generic menu,

Please no camel-caps.

> +First argument of bootmenu command override bootmenu_delay env

I cannot parse this.

> +If env bootmenu_delay or bootmenu arg is not specified, delay is 10 seconds

Why not using CONFIG_BOOTDELAY as default instead?

> +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

What will happen then?  How is this different from delay==0 ?

> +Boot Menu will stop finding next menu entry if last was not defined

I cannot parse this.

> +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

You might give an example here what the resulting screen will look
like.

> +To enable ANSI bootmenu comamnd add these definitions to board code:

Typo.

> +  #define CONFIG_BOOTDELAY 30
> +  #define CONFIG_AUTOBOOT_KEYED

Do we really need CONFIG_AUTOBOOT_KEYED ?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Wenn das dann in die Hose geht, nehme ich es auf meine Kappe.
                                         -- Rudi Völler, 15. Nov 2003