Change structure of library to organize things better
Opened this issue · 5 comments
Please hear me out. I'd like to suggest reworking the hexbright library into separate concerns per file. Same class as before, but defined by function, such as:
- hexbright.h
- hexbright.cpp
- power.cpp
- accel.cpp
- leds.cpp
- lighting.cpp
- buttons.cpp
As far as I can see the only technical issue preventing this is the crazy BUILD_HACK stuff and the idea that .ino files should control how the library compiles (strobe, etc).
Here is what I have in mind to tweak: The library should be compiled as a library, and library configuration should be in hexbright/settings.h. This would hold settings such as which main library features to turn on or off, which light level code to use, etc... it would allow you to easily review and comment all the configuration options without mixing them up with the actual code. hexbright.h would require settings.h, so the settings are available everywhere. hexbright.h would contain the class declaration, the .cpp files would contain all the definitions.
It might mean a few libraries would ALSO require tweaking settings, but I don't think this is the end of the world. I could be documented... or in some of cases it could be avoiding by dealing with a few additional things at run-time. Strobes is currently the biggest example of this behavior I see right now (an optional feature changing the core library).
Dealing with strobes
All update_spin does really is burn CPU cycles for 8333 microseconds (well subtracting out your programs loop cost). Instead of replacing the function wholesale at compile time I suggest we have a function pointer to an idler configurable function. The default would be none and update_spin would look pretty much as it does now. Libraries that required super fine grained control of the hardware could provide their own idler function and it would be called inside the update_spin do/while loop. Of course this would be documented and explain the caveats of using this (timing is critical) - pretty much the same as writing your own update_spin now.
So update_spin would just be responsible for deciding when to start the next "cycle" (as the default does now) and the idler function provided by strobe.cpp would be responsible for cycling the strobe during the idle time of update_spin.
This would add a few bytes for the function pointer and calls, etc... but it shouldn't be significant since we're not really duplicating much (if any) code... idler isn't a full rewrite of update_spin, just the strobe timing/functionality.
Thoughts?
Strobe could then inject and remove the idler if you had modes such that strobe could be turned on and off. set_strobe could inject the idler routine and set_strobe(OFF) could remove it, etc.
Short:
I am all for splitting up hexbright into files, but I am less inclined to go back to global settings (input on this is welcome).
Long:
Previously, the library had 2 main files: hexbright.cpp and hexbright.h. hexbright.h had a bunch of configuration options at the beginning, which functioned as your proposed settings.h. We are mid conversion from 2 files to individual files for each feature.
I don't love the BUILD_HACK define; I probably should have had a hexbright_h.h and hexbright_cpp.h instead.
The main disadvantage with the function pointer approach for update_spin is that the compiler would be unable to trim down the library.
The main 2 advantages with the current approach (.ino files control feature set) are:
- a new user doesn't have to figure out the necessary settings to successfully compile a program
- and unused features take up 0 space
It's also easier to rapidly switch between .ino files for testing, etc.
A disadvantage with this approach is that it uses more #defines, which can make the code more confusing.
The main disadvantage with the function pointer approach for update_spin is that the compiler would be unable to trim down the library.
There would be nothing to trim. All code would be used with no duplication. The only price you pay is the actual logic around the pointers and extra jumps. Instead of one big function you have two: update_spin and idler. And a user still doesn't have to figure anything extra out - the library still works seamlessly, just at run-time vs compile-time.
.ino files control feature set
Where else is this currently used other than strobe (to swap out update_spin)?
Where else is this currently used other than strobe (to swap out update_spin)?
I guess where I'm going with this is that maybe most things in the "kernel" shouldn't need to be recompiled/changed from sketch to sketch... perhaps some settings could be decided they are truly more "global" and others could be pushed down into software... perhaps you could have a PLUGINS define that turned on a few hooks into the "kernel" so that users who still wanted the smallest build possible could have it by disabling plugins.
This didn't start as an attack on BUILD_HACK or the process... I just wanted the files to be nicely organized and the existing build requirements are kind of at odds with that... perhaps there is another solution I'm not aware of. All of this because Arduino doesn't have any type of compile build flag settings of it's own... :-/
Is it really common for a lot of users to hit the 14k limit outside of debugging purposes? I'm working on a custom build with SOS, strobe, button locking, toggle mode (low, med, high, highest, etc), click counting, etc... and currently weighing in at 6.6k. I'm sure it helps that I haven't found a use for the accelerameter yet. :)
I need to test the actual program but so far the code is actually 128 bytes smaller doing it in run-time vs compile time. Now of course what that really means is there were likely a lot of further optimizations that could have been done to the existing idling system... but still... not bad.
A lot of the savings come from killing the "if we're at the end of a cycle but a strobe is coming up soon" math and logic. This can be replaced with an additional manual call to idle() at the end of update()... this would allow a strobe to potentially happen right before we hand execution back to the users loop. If our whole timing system is 8333 micros we shouldn't delay for potentially 4000 micros just for a more accurate strobe. We could encourage users to add idle() calls to their own loop (co-operative multi-tasking) if it's crazy slow and if that's not good enough we should look into interrupts. :)
Note: idle() is just a shortcut to run the provider idler function if there is one.
I'll play with my code in practice and see how it actually works on the hardware.