Re: [PATCH] Add Menu Framework

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Aug 19, 2010 at 05:53:22AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> Introduce a menu framework that allow us to create list menu to simplify
> barebox and make it more user-frendly
> 
> 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
> barebox

\o/ Cool! I really like it.

This is simple and still very flexible.

Some things which came up during testing:

- I get a data abort when trying to remove a menu without removing the
  entries first. example:

barebox@Phytec phyCORE-i.MX31:/ menu -a -m boot -d "Boot Menu"
barebox@Phytec phyCORE-i.MX31:/ menu -e -a -m boot -c boot -d "Boot"
barebox@Phytec phyCORE-i.MX31:/ menu -r -m boot
data abort
pc : [<87f05058>]    lr : [<87f05098>]
sp : 87affe20  ip : 87f20794  fp : 00000004
r10: 00000002  r9 : 00000000  r8 : 00200200
r7 : 00100100  r6 : 87b0daf8  r5 : 87b0db18  r4 : 001000f0
r3 : 87b0db18  r2 : 87b0db18  r1 : 003b7fff  r0 : 001000f0
Flags: nzCv  IRQs off  FIQs off  Mode SVC_32
Resetting CPU ...

- do_menu could use a if (!argc) return COMMAND_ERROR_USAGE;

- It would be nice to have an option to directly create a submenu on the
  command line (though this can be added later). So instead of doing

menu -e -a -m boot -c "menu -s -m network" -R -d "Network settings ->"

we could have a

menu -e -a -m boot -u network -R -d "Network settings ->"

We could than automatically add a 'back' entry if a menu is a submenu.

- It shouldn't be possible to add the same menu twice. example:

barebox@Phytec phyCORE-i.MX31:/ menu -a -m boot -d "Boot Menu"
barebox@Phytec phyCORE-i.MX31:/ menu -a -m boot -d "Boot Menu"
barebox@Phytec phyCORE-i.MX31:/ menu -l
boot: Boot Menu
boot: Boot Menu
barebox@Phytec phyCORE-i.MX31:/

- Removing entries does not work as expected. example:

menu -a -m boot -d "Boot Menu"
menu -e -a -m boot -c boot -d "Boot"
menu -e -a -m boot -c reset -d "Reset"
menu -e -a -m boot -c "exit" -d "Command line"
menu -e -r -m boot -n 1
menu -s -m boot

As expected the first entry is missing, but instead an empty line is
printed. When I then try to add an entry after this the menu is corrupt.

- commands should always return positive error codes. A good practice is
  to pass -E* values up to do_menu, use strerror() to print the error
  code and return 1 afterwards.

> +		switch(opt) {
> +		case 'm':
> +			cm.menu = optarg;
> +			break;
> +			break;
> +		case 'l':
> +			cm.action = action_list;
> +			break;
> +		case 's':
> +			cm.action = action_show;
> +			break;
> +#if defined(CONFIG_CMD_MENU_MANAGEMENT)
> +		case 'e':
> +			cm.entry = 1;
> +		case 'a':

There is a 'break' missing here.

> +			cm.action = action_add;
> +			break;
> +		case 'r':
> +			cm.action = action_remove;
> +			break;
> +		case 'c':
> +			cm.command = optarg;
> +			break;

Thank you for this work. I really appreciate it ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox