Dear Pali Rohár,
In message 1351769953-13560-3-git-send-email-pali.rohar@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;elsekey = 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;} elseesc = 0;} else if (esc == 2 || esc == 3) {if (esc == 2 && key == '1') {esc = 3;key = 0;} elseesc = 0;/* key up was pressed */if (key == 'A')key = 1;/* key down was pressed */else if (key == 'B')key = 2;/* other key was pressed */elsekey = 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