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