Hi, I will fix all mentioned style problems.
On Tuesday 13 November 2012 09:27:41 Wolfgang Denk wrote:
/* 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?
And what to use? ANSI sequence for key up is \e[1A (down is B).
- 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.
So which char is better for separator? Bootmenu line has config "name=command" and I think '=' is the best char for separating key and value pair.
- /* Add U-Boot console entry at the end */
- if (i < 100) {
Magic constant 100 ?
<i>-th menu entry is stored in env bootmenu_<i> (you can look to README for API). So I need to create string "bootmenu_<i>" and call getenv. I think it is very bad idea to use malloc for very allocating string of size: strlen("bootmenu_") + log10(i) + 2. I'm using array of size strlen("bootmenu_") + 3 allocated at stack which is enught for number less then 100. And I think 100 menu entries in uboot is really a lot of. If you do not think I can increase number to 1000 or 10000 (which increase size of string +2)...
+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?
Same as function bootmenu_destroy, so I changed code to call bootmenu_destroy.
- /* 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?
Ok, I added error message.
+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?
You can see that this function is not static. Reason is that menu_show is uboot API function which must return int. -1 means abort booting and run monitor code.
+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?
I do not understand what you mean.
+First argument of bootmenu command override bootmenu_delay env
I cannot parse this.
First argument of bootmenu command is delay and override env bootmenu_delay.
+If env bootmenu_delay or bootmenu arg is not specified, delay is 10 seconds
Why not using CONFIG_BOOTDELAY as default instead?
Ok.
+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 ?
If delay is less then 0, bootmenu will be shown and autoboot will be disabled. So there will not be any timer which automatically boot first entry in time $delay.
+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.
Ok, I can here text output. If you want to see screenshot from bootmenu rendered to framebuffer (in qemu) here is: http://atrey.karlin.mff.cuni.cz/~pali/u-boot-bootmenu.png
- #define CONFIG_BOOTDELAY 30
- #define CONFIG_AUTOBOOT_KEYED
Do we really need CONFIG_AUTOBOOT_KEYED ?
Yes, because menu_show() call in common/main.c is called only of CONFIG_AUTOBOOT_KEYED is defined. You can look at this condition.