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
