[U-Boot] [PATCH v2 2/4] New command bootmenu: ANSI terminal Boot Menu support

Pali Rohár
2012-11-14T22:38:39Z

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 */
> > + else
> > + key = 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 ?
>

-th menu entry is stored in env bootmenu_ (you can look to
README for API). So I need to create string "bootmenu_" 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.

--
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121114/43f0332a/attachment.pgp>