[U-Boot] [RFC/PATCH 1/2] Add menu Framework

[Jean-Christophe PLAGNIOL-VILLARD]

On 02:04 Mon 15 Jun     , Mike Frysinger wrote:
> On Saturday 13 June 2009 15:13:01 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Introduce a menu framework that allow us to create list menu to simplify
> > u-boot and make it more convivial for the end-user.
> >
> > This kind of menu is very usefull when you do not have a keyboard or a
> > serial console attached to your board to allow you to interract with
> > u-boot
> >
> > For the develloper part,
> > The framework introduce two API
> >
> > 1) C
> > that allow you to create menu, submenu, entry and complex menu action
> >
> > 2) Command
> > that allow you as the C API to create menu, submenu, entry and complex
> > menu action but this time the actions will be store in a env var and
> > then be evaluated and excecuted.
> 
> so you could create a multiple choice menu without writing a single line of C 
> code ?  that would certainly be preferred as writing C code seems error prone 
> and silly for a static menu setup.
yes you can create menu without a C line
this was mandatory for this framework when he was created
I've also some generic menu implemented to manage board settings
as example ethernet setting
I'll finish to merge them mainline and I public them
> 
> could you give an example of using the menu command ?  e.g. openmoko presents 
> a menu with a few options:
>  - boot
>  - set console to usb
>  - set console to serial
>  - reset
> 
> so using only the menu command, how could you achieve the same thing ?
yes and I'll post an example
> 
> > +	INIT_LIST_HEAD(&(menus.list));
> 
> you use &(...) in a lot of places where the parenthesis are unnecessary
> 
> > +	if (m->name)
> > +		free(m->name);
> > +	if (m->display)
> > +		free(m->display);
> 
> free(NULL) works fine, so the if() is unnecessary
ok
> 
> > +int menu_add(struct menu *m)
> > +{
> > +	if (!m || !m->name)
> > +		return -1;
> 
> would all of these sanity checks make more sense as debug-oly checks ?  or 
> does the code rely on these in the normal running of things ?
it prevent that you add a menu when you do not have enough memmory reverse for
the malloc
> 
> > +		if(strcmp(m->name, name) == 0)
> 
> should do a search to make sure you're using "if ()" and not "if()" and 
> similar
I known I've not finish to clean it, the original version does not follow Linux
coding style
> 
> > +	do {
> > +		ch = getc();
> > +		switch(ch) {
> > +		case 0x1b:
> > +			escape = 1;
> > +			break;
> > +		case '[':
> > +			if (escape)
> > +				break;
> > +		case 'A': /* up */
> > +			escape = 0;
> > ...
> > +		case 'B': /* down */
> > +			escape = 0;
> > ...
> 
> i'm guessing you're parsing arrow keys here (comment should say "up key" 
> rather than just "up").  but if you get just a '[' or 'A' or 'B', then this 
> doesnt work right.  you probably want something like:
> switch (ch) {
> 	case 0x1b:
> 		escape = 1;
> 		break;
> 	case '[':
> 		if (escape == 1)
> 			escape = 2;
> 		break;
> 	case 'A':
> 		if (escape != 2)
> 			break;
> 	...
> 
> then again, this kind of key parsing is duplicated in quite a few places in u-
> boot.  we really should have this centralized so people can say getkey() and 
> have it return cooked values.
will take a look
> 
> > +int menu_action_exit(struct menu *m, struct menu_entry *me)
> > +{
> > +	return 0;
> > +}
> 
> what's the point ?
just exit the menu with a prompt
> 
> > --- a/include/console.h
> > +++ b/include/console.h
> > +#define printf_reverse(fmt,args...)	printf("\e[7m" fmt "\e[m",##args)
> > +#define puts_reverse(fmt)		puts("\e[7m" fmt "\e[m")
> > +#define gotoXY(row, col)		printf("\e[%d;%dH", row, col)
> > +#define clear()				puts("\e[2J")
> 
> i'm guessing this works with serial consoles and linux terminals.  how does 
> this work with framebuffer consoles ?  (the answer may be obvious as i'm not 
> familiar with the framebuffer console layers that may exist in u-boot)
I've not yet test the framebuffer but IIRC yes

I've plan to test it ASAP

Best Regards,
J.