Slightly off-topic @algernon: Why do you prefer properties (puplic class members) to inline setters/getters? The latter would make printf-style tracing of value changes (when there is no debugger available) and assert-based invariant checking applicable at no run time costs. Moreover, they allow for value clamping and change notifications/hooks, if needed.
Setters and getters are easy to define via macros without degrading readability, as e.g. done in the vtk project.
Code size, mostly. Setters/getters have a small amount of PROGMEM and SRAM cost, and while a single one is small, many settable things can have a significant impact on code size. If I recall correctly, we’re talking about over a hundred bytes saved by using public properiets instead of accessors.
While accessors have a lot of uses, and I’d prefer them on code that runs on less constrained hardware, when resources are as limited as they are on the AVRs used in the Model01, practicality wins over doing things Right.
We could use macros as a compromise, but macros have the disadvantage of being global. I can’t make MouseKeys.setSpeed(x) a macro, and MOUSEKEYS_SETSPEED(x) feels wrong, and unintuitive.
(Moved the discussion to a new topic, this may very well end up being interesting later on too, and a separate discussion feels useful to have on it.)
A friend of mine does embedded programming of military radios. He and I are always getting into good fun arguments about encapsulation… how he, as embedded guy, hates it for “wasting valuable bytes” and is always screaming “just let me flip the bit I need to flip”… and how I, as the enterprise developer, love it for its good API design and state safety.
Thank you for moving the topic.
My query refered to inlined access methods only. The following incurs zero runtime cost when build as release version (-DNEBUG). Both the invariant check and the log message just vaporize.
#include <assert.h>
#ifndef NDEBUG
#define MY_DEBUG_OUT(CLASS, ...) \
std::cout << #CLASS << " (" << (void*)this << "): " << __VA_ARGS__ << std::endl;
#else
#define MY_DEBUG_OUT(CLASS, ...)
#endif
#define PAIR_SETTER(PROPERTY) \
void set##PROPERTY(int PROPERTY) { \
\
/* Assert invariant */ \
assert(0 == PROPERTY % 2); \
\
/* Debug log message */ \
MY_DEBUG_OUT(foo, "Setting " #PROPERTY " to " << PROPERTY); \
\
PROPERTY##_ = PROPERTY; \
}
class foo {
public:
// ... constructor ommited for simplicity
PAIR_SETTER(APairProperty)
private:
int APairProperty_;
};
int main() {
foo myFoo;
myFoo.setAPairProperty(4); // works
myFoo.setAPairProperty(5); // assertion fails when compiled without -DNEBUG
}
After a look at the Kaleidoscope core and some of the plugins, it seems that there is no consistent choice of using public properties or member access methods. Some classes/structs provide setters/getters, some not.
I would opt for consistently using inline setters and getters as they
This list might not be complete, but hopefully, those are enough good reasons.
Tell your friend, he is wrong … and who needs military radios, anyway 
One to add to the list:
I’m thinking of inline virtual functions in particular here where the compiler may choose to ignore it where a virtual function call is actually required.
That’s correct. But virtual methods are out of skope as currently not used by Kaleidoscope (am I right?).
There are actually additional reasons why a compiler might refuse to inline even a non-virtual method. Compilers have complex heuristics about when to inline code. Those mainly depend on the size of the function body. Refusing to inline e.g. tries to avoid pipeline stalls and binary code bloat. Most compilers have compiler attributes to enforce inlining specific functions, though. But those are only for Chuck Norris type programmers that regard themselves smarter than the compiler.
Kaleidoscope implements its own compile time polymorphism by means of its variadic Kaleidoscope::use(...) function. This avoids the unnecessary runtime overhead of calling virtual methods of dynamically polymorphic plugin classes (vtable-access can be costly) as well as the additional storage requirements for vptrs and the vtables. Those are not necessary anyway, as most (if not all) plugins currently use global quasi-singleton instances.
In general I would call Kaleidoscope’s approach a bad bad smell if I would find it in a larger C++ application. But it is, in my view, a perfect solution for an embedded firmware.
The only problem with this implementation (global singleton plugin instances) is that developers might be confused. She/he has to choose between making all plugin members (functions and data) static (globals). When I encounter a singleton I often go this way because it makes the aspect of it being a singleton obvious whenever a class function is called (e.g. MyClass::doSomething()). On the other hand it is of course possible to let all plugin members be ordinary instance members that are operated on by means of non-static methods.
This ambiguity could become a problem if it would one day be wanted to use several instances of the same plugin simultaneously (to me this seems actually quite useful). The implementation of all those plugins whose developers went the all-static-and-global way in the first place, would have to be modified. But the global plugin class instance approach is another topic 
I hope I am not too critical about Kaleidoscope. I really love it. It has a pretty nice design. The plugin concept is a blessing, especially when compared to other monolithic firmwares that are flooded with #ifdef statements.
It’s designers have done a great job! And the best of it all, it is OSS.
Agreed, my background isn’t embedded systems, it’s larger scale C++ systems. I was trying to think of other justifications I’ve used for setters/getters vs. public members. You’ve already mentioned my personal favourite, assertions. I frequently use these to provide pre-conditions.
It’s not mine, either 
There are a few places where we use virtual methods, but not much. The KaleidoscopePlugin and kaleidoscope::Hardware base classes make use of them, as does the LEDMode class. I seem to recall there being a few other places, but indeed, we are trying to minimize their use.
Yeah, it would sometimes be useful to have more instances of the same class, but this being a firmware in a very constrained environment, compromises had to be made. It’s… not the cleanest approach, but it is practical, and not terrible, either.
Embedded is a very different world (I used to do embedded for a living a decade ago, nowadays my dayjob is more about highly distributed systems - they’re nothing alike).
Obviously I did not look close enough. Just checked the KaleidoscopePlugin.
The virtual begin() method could easily be made non-virtual. It is only called in one place and the overloaded variadic run(...) method is already templated.
// Kaleidoscope.h
...
template<typename Plugin__>
inline void use(Plugin__ *p) {
// Check correct type only in debug builds. Make sure to build with RTTI enabled.
assert(dynamic_cast<KaleidoscopePlugin*>(p));
p->begin();
}
...
Removing this would save two bytes per plugin instance and additionaly at least two if not more bytes for each plugin type (the vtable) if the KaleidoscopePlugin’s begin() method would be turned non virtual.
Unfortunately, this would require modifications in all plugins as the keyword final was used in all derived plugin classes to tag the virtual begin() method. When begin() is just removed from KaleidoscopePlugin the compiler will complain as follows.
error: 'void kaleidoscope::DerivedPlugin::begin()' marked final, but is not virtual
But you could easily have multiple instances of the same plugin class, provided that it does not use any global variables for its state. Right now, a global pseudo-singleton is generated for each plugin used. What keeps the user from creating additional global instances of the same plugin in the sketch and passing them to the run(...) method? Why e.g. not having several instances of one light effect that uses different color schemes that are toggled?
Oh, I just forgot … one more word about the getters and setters with assertions and logging. These features become especially useful once Kaleidoscope together with all of its plugins gains a test bench (see Jesse’s post about regression testing).
Of course, logging and assertions come with a lot of string literals that bloat binaries. @craigdissel’s Hardware-Virtual plugin enables running sketches on x86. There, memory resources are almost unlimited compared to the atmega32u4. I would, thus, in general recommend excessive use of assertions to everybody who develops core/plugins. Used in test benches, log output can shorten the time to find errors, sometimes without starting a debugger.
I am currently trying to find a way to simulate the 01 with simavr (see Model 01 Simulator). Even in a simulation of an avr with its naturally very limited resources, assertions and logging both are applicable. How, you might ask, is this supposed to be possible for a firmware sketch that is likely already at the limit, with respect to resource consumption? The answer is simple, however not obvious: Simulations have the intriguing property that they are not real.
What is not real is also not subject to real restrictions. All 8 bit avr processors are binary compatible when it comes to their data types and addressing. With simavr it is possible to run a firmware on an artificial hardware with extended resources. I did this in other occasions with an atmega1280 to debug code that was actually targeted for an atmega32u4, using gdb. Either I will find a avr processor that is like an atmega32u4 (including USB, etc.) with more flash and RAM (if there is such a beast) or I will just create a virtual one (let’s call him atgiga32u4). simavr allows you to specify the properties of the processor.
Eventually, the Hardware-Virtual plugin will find 99% of all errors. Together with a simulation it will hopefully be 99,9%. Make assertions your best friend!
Took me a while to figure out where you are going with this. I’m at work without my Model01, but will test how this works in practice when I get home. A quick attempt is resulted it assembly that doesn’t seem to call the proper begin methods, but I might be reading the decompiled assembly wrong. Will try it on real hardware at home.
The savings seem suspiciously high too, so I’m not entirely convinced this will work. But C++ has surprised me a few times before.
Yeah, but globals saved us a few bytes here and there, as far as I remember.
Nothing. They can shoot themselves in the foot, if so they wish. (Sorry if this sounds a bit harsh, but that’s the gist of it.)
LEDEffect-SolidColor does just that. There are other LED effects that do considerably more than applying a color scheme, so completely different class makes sense there.
Just to make sure that the single argument run(...) method is actually called, you could modify the code as follows. If the compile fails, the method is at least instanciated, which means in your case that is actually called.
// Kaleidoscope.h
...
template<typename Plugin__>
inline void use(Plugin__ *p) {
// I know, the following static_assert looks weird, but it ensures that the assertion is only evaluated
// once the method is instanciated.
static_assert(0 == sizeof(Plugin__), "I'm alive!");
p->begin();
}
...
Hrm. It does get instantiated. Interesting.
The idea that a global variable safes memory compared to a member variable of same type of a global class instance sounds a bit voodoish in this context. It is true that this may be the case if variables end up in a different order in memory, which leads to padding being added by the compiler to ensure proper memory alignment. On an 8 bit system we can eliminate this explanation, though.
It may very well be that the reason we saw a size reduction when aggressively making stuff static was due to something else. I seem to recall discussing a similar topic with @jesse in one of the early issues or pull requests… will try to dig that up too. There were a number of reasons for going the static+singleton way, apart from size considerations.
Yes, that would be nice and quite interesting.
There is one restriction to using multiple instances of the same plugin, though. Hooks currently operate with callback function pointers that lack some sort of context or user-data pointer. Thus, they can only access class inventory of plugins, not instance inventory. For being able to really use multiple plugin instances, it would be desirable to have overloaded hook-registration methods that can deal with member-functions. The std::function template would be well suited to store such callbacks.
See also