Dear Pali Rohár,

In message <1351769953-13560-3-git-send-email-pali.ro...@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: w...@denx.de
Wenn das dann in die Hose geht, nehme ich es auf meine Kappe.
                                         -- Rudi Völler, 15. Nov 2003
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to