Go Down

Topic: New Arduino library: MenuSystem (Read 10664 times) previous topic - next topic

Nice news!

Regarding the id issue,  I turn down that path at the end. I'm still learning C++ and improving my OO habilities, so it was a little hard to understand the composite pattern, but now I get it and in the way I had learned some other patterns (very ussefull).

Actually I'm extending the lib to have a new type of MenuItem (I called SetupItem). This new SetupItem allows you to have a way to set up values with the menu, so I add 2 new methods (increment and decrease). During this week I will finish it (clean it to be able to show ;) with a serial controlled example to show this new functionality. After I will upload it to my fork of the menuSystem so you can review it and tell me your opinion, maybe we can merge it after all.

For you to make an idea, the mayor changes I had made to mensystem to allow the new SetupItem work are:

  • change add_item, add_menu to only one add method that receive a MenuComponent item, so it can receive any type of mennuComponen derived class. This implies that the responsibility to add a callback function must be passed to the MeItem, and not in the add_item method. (see the example code below)

  • The last change needs to pass the responsibility of setting the callback function to the item class. This means the user will be responsible of adding the callback func to the MenuItem object.
    Whit this last 2 changes you can do something like this:

Code: [Select]

MenuSystem mm;
Menu mu("Main Menu");
MeniItem mi1("Menu Item 1")
mi1.set_select_function(&on_mi1);
mu.add(mi1);

  • Other change was to add a SetupItem, (last changes needed to do before this).




*To make this new SetupItem work I have to move some methods to the MenuComponent and make it virtual so the derived classes can rewrite them, but I had to make a default behavior and implement this virtual methods on the correct classes. Now I'm studying the possibility of making all this functionality with visitors, but I'm not sure. maybe it will be a good idea to show you the work and get the opinion of a more experimented person.

jonblack

Let me see if I understand.

You're adding a 'SetupItem' to the menu in order to use the menusystem interface to change values? The example I can think of is a menu item for setting a clock format to be 12 hour or 24 hour.

If my understanding is right, I don't think 'SetupItem' is needed. The handler for an arbitrary 'MenuItem' can perform the required action. The benefit of the 'SetupItem' is that it stores the setting value automatically, but this is also a downside - it results in the menusystem also being responsible for settings.

I'm also reluctant to add more classes. The arduino doesn't have a huge amount of memory, and generic libraries should be small so more of that memory can be used by the client application.

The interface change in your example also isn't ideal: it would break existing code. That said, I think the 'MenuItem' might be better off if the handler function pointer was passed in the constructor.

Anyway, I don't want to demoralise you. Perhaps I've misunderstood and there is a good use case for this. I've not convinced myself completely that this is a bad idea (Right now I'm thinking of a 'ToggleMenuItem' that when toggled sends the new value to the callback function...but I'm also wondering if a Settings library should really handle this...hmmm).

Quote
I'm also reluctant to add more classes. The arduino doesn't have a huge amount of memory, and generic libraries should be small so more of that memory can be used by the client application.

Yes I agree with you

Quote
The interface change in your example also isn't ideal: it would break existing code. That said, I think the 'MenuItem' might be better off if the handler function pointer was passed in the constructor.

I also agree that it would brake the actual code, but just the menu creation part, at least for me isn't big deal to repair it. Your idea of making it on the constructor look good, today night I will update my mods to allow that, so you can do like this:
Code: [Select]
MenuSystem mm;
Menu mu("Main Menu");
MeniItem mi1("Menu Item 1",&on_mi1);
mu.add(mi1);

or
Code: [Select]
MenuSystem mm;
Menu mu("Main Menu");
MeniItem mi1("Menu Item 1")
mi1.set_select_function(&on_mi1);
mu.add(mi1);

I still propose this change, because as I see, setting the callback function, should be responsibility of the object (menuItem) itself and not of the composite class. Other point that support my change:
Code: [Select]
Menu const* Menu::add_menu(Menu* pMenu)
{
    pMenu->set_parent(this);

    _menu_components[_num_menu_components] = pMenu;

    if (_num_menu_components == 0)
        _p_sel_menu_component = pMenu;

    _num_menu_components++;

    return pMenu;
}
void Menu::add_item(MenuItem* pItem, void (*on_select)(MenuItem*))
{
    // Prevent memory overrun
    if (_num_menu_components == MAX_MENU_ITEMS)
        return;

    _menu_components[_num_menu_components] = pItem;

    pItem->set_select_function(on_select);

    if (_num_menu_components == 0)
        _p_sel_menu_component = pItem;

    _num_menu_components++;
}

If you see both methods are almost the same, regarding the fact that add_item check for space (that it would a good idea to check on add_menu to) and that add_item sets the callback function in the process. So if you extract the responsibility of setting the callback funct to the MenuItem class you will be able to have generic add method like this
Code: [Select]
MenuComponent const* Menu::add_menu(MenuComponent* pComponent)
{
    // Prevent memory overrun
    if (_num_menu_components == MAX_MENU_ITEMS)
        return NULL;

    pMenu->set_parent(this);

    _menu_components[_num_menu_components] = pComponent;

    if (_num_menu_components == 0)
        _p_sel_menu_component = pComponent;

    _num_menu_components++;

    return pComponent;
}

The only thing is that set_parent should be virtual on MenuComponent ad we have 2 options:

  • Make a generic method on MenuComponent that return false or anything that do nothing and declare it on Menu (as it)

  • Add a parent private/protected (not sure which one) on the MenuComponent and implement on Menucomponent so it can work for both classes

The first option add program memory
The second one need more ram to accept this new var on all items and menus (doesn't found a case where knowing the parent of an item will be useful)
Anyway you save some program memory transforming two methods into one

Regarding my SetupItem, look at https://github.com/niesteszeck/menusystem/blob/master/examples/LCDAdvancedNav/LCDAdvancedNav.ino
In this example I was making, I show how to toggle a led, how to display an analog read, and how to setup a var (string in this case) that represent a date+time (that is where I want to go). I you see it is really difficult to mix the display+the menusystem+ setting a var.
I need to save states to know if I'm on the menu or in the callback function that is trigger from the menu to it can do setting var related work.
So I decided to expand the menuSystem to allow me to add this new class. So I make something like this (I don't have the exact code here, so I just writing what i remember):
Code: [Select]

SetupItem(char* name, int value)
    : Menucomponent(name),
    _value(value)
{}
boolean SetupItem::increment(){
    _value++;
}
boolean SetupItem::decrease(){
    _value--;
}
char * SetupItem::get_name(){
    sprintf(_display_name,"%s: %d",_name,_value);
    return _display_name:
}
    int get_value() {
    return _value;
}

So I can do something like this:
Code: [Select]

Menu mu("Main Menu");
SetupItem si1("Month", 0);
SetupItem si1("Day", 0);
SetupItem si1("Year", 0);
MenuItem mi1("Set Date", &on_set_date);
MenuItem mi2("Cancel", &on_cancel);
mu.add(mi1);

while(1){
    if (button==up)
        ms.increase();
    if(nutton==down)
        ms.decrese();
    display(); //this method ask the menusystem with get_names to display it over serial or anything
}

void on_cancel () {
    ms.back();
}
void on_set_date(){} // this func ask menusystem for vars values and set the date accordingly


The output should be like
Code: [Select]
Setup var 1: 0
If I press the up button 10 times:
Code: [Select]
Setup var 1: 10

Because the SetupMenu is added to a menu next will be jump to next var to set up (fe: from month to day).

I know that all this changes implies a lot of small changes everywhere, so let me show my code so you can see all together; the example (SerialNav) working with the modifications to show how to set a date (I want to set up .


Other point that I just realize while I was writing this, is that a lot of the changes I had to made to the lib allow anyone to grab menusystem and expand it under they needs, as I did making a new SetupItem class.


I also don't want to say that your lib is poor or anything like that, I just trying to use it and I found this way the easiest to help me do what I want. I am very grateful of your work (it opened my eyes about composite pattern that is a good way to achieve a menu) and hopefully we can expand the functionality of your lib.
*If I'm spelling something really wrong (from programming point) please bright me  ;)

jonblack

I'll take a look at your proposal this coming weekend (I'm pretty busy during the week).

My first thoughts are:

1. MenuSystem::increase() and descrease() don't really make sense. The menu system must not be aware of the current menu item, so the interface must stay generic. You can always use next, prev, select, and back, for example. increase and decrease only make sense for the SetupItem;
2. Moving the callback to the constructor and merging the add_item and add_menu functions seems like a good idea; I think you've highlighted a bug in the missing buffer overrun check too;
3. Whatever happens when a menu item is selected must take place in select() or the callback to maintain the generic interface;
4. ToggleMenuItem feels like something that can be used. It's easy to store a boolean, the constructor has a param specifying the default value and it can easily be flipped in select() and passed in the call to the callback (although this would mean a different callback signature...maybe that's bad...need to think about it);
5. A ValueMenuItem therefore has the problem that you can never communicate how the value changes through the MenuSystem interface, and I doubt there is a generic way to do that.

Anyway...I'm warming up to the idea of "other menu items" so when I take a look at the weekend, maybe I will have an "aha!" moment.

Also, I don't take offence to your comments, I welcome them :)

Sorry for the delay, busy time   :~

I'll take a look at your proposal this coming weekend (I'm pretty busy during the week).

My first thoughts are:

1. MenuSystem::increase() and descrease() don't really make sense. The menu system must not be aware of the current menu item, so the interface must stay generic. You can always use next, prev, select, and back, for example. increase and decrease only make sense for the SetupItem;

You are right, still thinking how to do it (now is with this :()
Quote

2. Moving the callback to the constructor and merging the add_item and add_menu functions seems like a good idea; I think you've highlighted a bug in the missing buffer overrun check too;
When I was writing the last message I realize that to. Watch the code to see how I implement the merging of the add method, maybe it convince you
Quote

3. Whatever happens when a menu item is selected must take place in select() or the callback to maintain the generic interface;
4. ToggleMenuItem feels like something that can be used. It's easy to store a boolean, the constructor has a param specifying the default value and it can easily be flipped in select() and passed in the call to the callback (although this would mean a different callback signature...maybe that's bad...need to think about it);
As you say, expanding the lib to make something not general doesn't looks right (lib must be general), so maybe we could make it easy to expand (just thinking)
Quote

5. A ValueMenuItem therefore has the problem that you can never communicate how the value changes through the MenuSystem interface, and I doubt there is a generic way to do that.
I'm still thinking how to do it generic


My changes and the modify example is on
https://github.com/niesteszeck/menusystem/tree/SetupItemTest


*I had make a lcd example and a serial example in case you don't use a lcd

jonblack

#35
Dec 20, 2013, 01:18 pm Last Edit: Dec 20, 2013, 01:23 pm by jonblack Reason: 1
UPDATE: version 1.0.1 released

I've released a new version (1.0.1) of the menusystem library, which is now called arduino-menusystem. This update includes a fix for the buffer overflow mention in this thread, which also means that there is no longer a limit of five items per Menu - they are now dynamically allocated, so you can have as many as your arduino can support in it's little memory.

As the name has changed, so has the github link: https://github.com/jonblack/arduino-menusystem

NOTE: The arduino IDE has a hissy-fit when loading the library because the project now has a dash in its name. You can rename it locally without it being a problem, just remove the dash. I have no idea why the arduino IDE cares about this, but I'm not changing the name :)

@niesteszeck
As much as I wanted to I couldn't find a generic way to implement a setup menu item. My first thoughts were right on this. I've been writing a clock using an LED matrix, and I solved this by using the menu to change a global variable called "current_mode". Depending on the mode, different things are displayed on the screen and the buttons do different things. When in a mode for setting the display brightness, the buttons change the value of another global variable. I hope this helps.

Don't worry, Is difficult to make it generic.

lurtiz

I know this thread hasn't been updated in a while but I'll take a chance. I'm pretty new to C programming and Arduino. I'm using this library for a project because it's simple and doesn't use as much memory as some of the other menu libraries that are loaded with bells and whistles that I don't need. The only problem I'm running into is my RAM is running very low and I still have a few more menus to add. Is there any plan to modify this library to use PROGMEM instead of precious RAM for storing the menu strings? I've looked over the code for a while now but my programming skills aren't sharp enough to modify it myself yet. Hoping someone can help me out here.... Thanks

Go Up