Idea to change sketch structure to avoid Arduino-specific magic and externs

[embedded-creations]

I noticed you're using multiple .ino files for several examples to work around some circular referencing (probably not the right terminology) issues. It seems that's allowed by the Arduino IDE, but not common practice. Even so, you also have to use externs to work around the same issues. I tried to figure out why this is needed, and it seems to come from needing to have a command list containing a function reference while the function also needs a reference to the list, and both definitions are needed at compile time. I couldn't find a good way to avoid using .INOs+externs without dramatically restructuring the code, and also making it more complex. The functions don't need to know the command list and size at compile time though, it can be set during setup(), making the problem easier to solve. I prototyped a solution to get your feedback.

embedded-creations@5547ef6

If it's not obvious to you why this is an improvement, let me know and I'll make a case for why I like it and think it should at least be an option. I'm too tired to write anything more right now ;-)

[CreativeRobotics]

Louis, Thanks for this. The mutual referencing of lists and functions was always a tricky issue to work around so having some other options like the one you have suggested would be good. I'll download your prototype version and have a play with it, and see if there is a way to integrate it into a future update as an option alongside the existing approaches.

The main reason I use several .ino's is just because I like to organise my code across multiple tabs. As far as I understand it when it comes to compiling, Arduino just treats all the .ino files as a single contiguous file so keeping the command handlers and list in their own .ino file was always down to my own preferences, not as a work around for the need to forward declare the list. The closest thing to a neat solution to the forward declaration issue is to use an initialisation function (see the example called Template3InitFunction) This places all the references to the command list in a function that appears after the list is declared so there is no need to forward declare the list and size variables, but that function can still be called from setup() without any issues.

The only potential issue I can see with your approach is whether other functions in the sketch will be visible to the command handlers in the header files. All of the examples in the library just show how to set and get variables, but all of the times I use commander in projects the command handlers are also calling other functions in the sketch.

[embedded-creations]

Arduino just treats all the .ino files as a single contiguous file

It's more than that, it also automatically creates forward declarations of functions. In Template3InitFunction for example, there's forward declarations of initialiseCommander() made, allowing you to define that function after commands is defined. That won't work so well if using code in .h/.cpp files, which is what I want to do.

Specifically I'm trying to adapt your PrefabFileNavigator.h command set to work with generic FS objects. What you do in your PrefabFileExplorer example is similar to what I'm suggesting. You have a function called in setup() that sets the pointer/size for the top layer, so exitHandler() doesn't need to have these values at compile time.

Using CommandCollection I believe just makes what you're already doing in that example a little smoother. (I don't think I studied that example in detail before I started on my solution.) It also makes an opportunity to simplify going up a level, if by just storing a single pointer during transferTo() Commander can go up a level by calling transferBack() with no arguments. (An idea I just had, I didn't prototype it yet) [Edit: this wouldn't scale beyond two levels if stored in Commander, it probably needs to be stored in the CommandCollection]

whether other functions in the sketch will be visible to the command handlers in the header files

I think that will be fine because of the .ino automatic forward declarations, but a good thing to test.

I'll keep thinking about potential improvements as I work on my project.

[embedded-creations]

Another more important use for CommandCollection:

I'm trying to adapt your PrefabFileNavigator.h command set to work with generic FS objects, with the option to work with multiple FS objects in separate menus. e.g. I could type "SD" from the main menu to work with the SD card, or "LITTLEFS" to work with LittleFS. The same commands will work with both filesystems, so I need two instances of a new FilesystemNavigator class. I need to use static methods for handler methods in the class so the handlers are compatible with Commander. (I recently learned pointers to class methods aren't simple to work with.) If Commander calls a static method in my class I won't know which filesystem I'm supposed to be handling. If I'm using a class derived from CommandCollection, and Commander stores the pointer of the current CommandCollection, I can get back to the instance. I just added that functionality and an example:

embedded-creations@e6582a0

[CreativeRobotics]

Yes, I realised Arduino also scans the sketch and creates the function declarations for you. There is a quirk (or maybe it has to work like this) where if the command list comes before setup() and loop() it throws an error because it hasn't seen the functions yet - I'm guessing it puts the function declarations it creates just before the first function and after any global variables?

Anyway, it would be nice to make the whole thing a little neater and more portable.

[CreativeRobotics]

The prefabs were (I thought) a nice idea, but I ran into some problems getting it to work well enough (and I was hitting the limits of my c++ skills) What you are suggesting sounds like it could be a great improvement.

[CreativeRobotics]

What do you think to the idea of incorporating the array of command lengths into your commandCollection object?
Take a look at computeLengths() to see how it is created whenever a new list is loaded. If each collection can have its own array it would speed up loading of lists, and remove a load of dynamic memory allocation that happens when lists are loaded.

[embedded-creations]

If I understand you correctly, commandCollection would now contain uint8_t* commandLengths, allocated dynamically the first time the list is loaded, along with uint8_t longestCommand? Seems fine to me, though it is going to use slightly more memory as potentially all lists will have their array allocated, instead of just the active list.

[CreativeRobotics]

Yes, its a trade off between memory efficiency or faster loading and less dynamic memory use. Perhaps it needs to be a compile time option, or maybe controlled through run time settings...

[embedded-creations]

The prefabs were (I thought) a nice idea, but I ran into some problems getting it to work well enough (and I was hitting the limits of my c++ skills)

The prefabs are a great idea! I did a lot of embedded C programming in school and for work, but pretty much all my C++ skills are from working an Arduino Libraries, hitting roadblocks, and either figuring out a solution or getting help from contributors.

I think using classes in the prefabs is going to unlock a lot of potential. The prefab header file can contain declarations, and the instantiations can go into the sketch.

[embedded-creations]

I was able to add support for navigating two filesystems with two instances of a prefab class in my PrefabFileExplorerFS example. I merged in your changes and pushed it to my existing structure branch

[embedded-creations]

I made another pass as simplifying PrefabFileExplorerFS: making a class to hold the top level menu, and the menu scales as FileNavigator objects are added to it. I added some begin() methods inside Commander to support CommandCollection and fixed some bugs related to CommandCollection.

Making a prefab but dynamic menu like FileNavigatorMainMenu is making me think that creating a generic dynamic menu object that scales as you add commandList_t entries to it at runtime would be useful. With a few more prefabs, Commander could become a useful swiss army knife where you could pick and choose which prefabs and which unique commands you want to include in your sketch. I don't have time to flesh out this idea let alone code it right now, but something I'll keep in mind for later.

[embedded-creations]

I simplified things further, and made a class that holds the Commander object and main menu, and was able to add filesystem browsing to my AnimatedGIFs sketch with only 8 additional lines of code. I haven't published the GIF sketch changes yet, but you can see the simplification here: https://github.com/embedded-creations/Commander/blob/structure/examples/PrefabFileExplorerFS/PrefabFileExplorerFS.ino

[embedded-creations]

OK, I woke up thinking about this so I'm making time to flesh out the idea:

I'd like to be able to add prefab commands and menus to Commander without having to write a lot of code to integrate them. e.g. for my AnimatedGIFs sketch, I want to add the filesystem navigator prefab, and a TBD AnimatedGIFs controls prefab, without having either of the two prefabs know about each other, and without filling my sketch with a lot of Commander-specific code.

In my sketch I'd like to do:

cmd.addMenu(myNavigatorMenu);
cmd.addMenu(myGifMenu);

And have the result be a working top level menu combining the two menus, where I can enter into any submenus that exist and return back to the top level menu after calling "exit":

$ help
filesystemStatus | part of myNavigatorMenu
SD    | part of myNavigatorMenu (enters submenu)
setGifTimer | part of myGifMenu
loadGifPlaylist | part of myGifMenu
...

Using the "SD" command would enter a submenu, and exit within that submenu would return to the top level menu (without having me write any code in my sketch to enable that feature).

That's my ideal case, which is going to take a lot of thought and code on how to put multiple menus at the same level. A slight modification could be done with very little extra code:

cmd.addSubMenu(myNavigatorMenu, "FS");
cmd.addSubMenu(myGifMenu, "GIF");
cmd.addCommand(cmdHandler, "cmd", "example command");

This would create this top level menu, and again I can enter and exit the submenus without having to write any additional code in my sketch:

$ help
FS | enter FS submenu
GIF | enter GIF submenu
cmd | example command
...

addSubMenu() depends on having a dynamic command list object (let's say DynamicList), and Commander would store a DynamicList to hold the top level menu. DynamicList::addCommand() does just that, similar to what you do in FunctionSwap, but with a dynamic object, so no need to reference specific items in an array. DynamicList::addSubMenu(collection,...) stores a pointer to the CommandCollection, calls addCommand() with a wrapper method that just calls transferTo() for the collection. CommandCollection needs to be modified to store a topLevelCollection, so each collection can just call transferBack(topLevel) without having to know anything about where it is in the menu hierarchy. addSubMenu adds the topLevel collection to the submenu.

With your fileNavigator prefab, you run some checks to see if the menu is functional before calling transferTo() (in sdFileHandler()). I think that can be automated as part of transferTo() by adding an optional entryHandler inside CommandCollection. If entryHandler exists, then Commander calls entryHandler() as part of transferTo(), and if entryHandler() returns 1, then an error message is printed and control goes back to the top level menu. This would keep functionality contained within relevant menu classes, so e.g. you can add a filesystem navigator as a submenu to a top level menu, without the top level menu needing to know how to check if the filesystem is functional.

I have the start of this with the code I already wrote to enable and abstract away the details of my prefab and menu. One piece that requires a larger Commander API change: I need to know the index of the menu item called by handleCommand(), to avoid the hacky solution I came up with having unique placeholder methods that know their index (e.g. fsHandler0()..fsHandler3()instead of Commander directly callingfsHandler(index)`).

I think with these changes we'd want to make CommandCollection the default way to interact with Commander, but provide backwards compatibility by keeping a default collection inside Commander, and the methods that directly interact with command list, size, name would set those values inside of the default collection.

If this idea seems promising I'll try to find time to prototype it, but I don't want to put the work in if you think it's too big of a change.

[CreativeRobotics]

Ok, this sounds great, but I'm keen to avoid making the entire command list creation dynamic unless the user chooses this option - I may be being over cautious here about dynamic memory allocation ...
I like the overall approach though of simplifying what the user has to do and its part of the core design goals of the library (like you said, a swiss army knife) The less the user has to write code to impliment a functional command line interface the better, just so long as the library stays fairly lean and fast. When I first launched the library it would run usefully on an ATMega32U4, I haven't tried it on one of these devices for a while though as I almost exclusively use the ARM devices that have more RAM and flash.

Go ahead and do as much work on this as you feel able, I think it could significantly improve the library.

Regarding the index of the menu item that calls the handler, I don't know if this helps but this value is stored in the CommandIndex variable. I have just added a public method that returns this value so from the handler you can call Cmdr.getCommandIndex().

One idea that came to mind but I haven't had time to think about, so it may be irrelevant or useless, is to have a method of dividing up a single command list into sub lists. In memory you could have one single array, but Commander will treat items within certain index ranges as sub menus and only display them when asked ...

[embedded-creations]

I should have clarified that the dynamic list would be optional. I think the memory increase for most existing sketches will be minimal, just a few extra pointers per menu.

Getting index manually is a perfect solution

Sub lists sounds promising, let’s chat more about that once my initial prototype is ready