"Unlimited" buses

[blazoncek]

Add ability to have up to 36 (theoretical limit) buses, physical or virtual.

Stuff also done:

• converted BusManager from class to namespace
• modified BusConfig to use vector
• modified ColorOrderMap::getPixelColorOrder() to use vector (and removed ColorOrderMap from BusDigital)
• updated loading config into UI (LED settings, buttons, etc.)

WARNING web server may not support that many variables when creating buses so actual limit may be less.

Many thanks to @willmmiles @TripleWhy and @PaoloTK for helpful comments and suggestions.

[blazoncek]

Apparently will require #4484 to be merged 1st.

[TripleWhy]

should be moved to a different header file

[blazoncek]

True, but I want to limit number of modified files for transparency sake.
Once toolchain is updated we may not need this.

[TripleWhy]

for transparency sake

The git diff won't look much different

Once toolchain is updated

Will that actually happen? ^^
Anyway it should be easier and cleaner to delete a polyfill.h than to delete a function here and check if other files included bus_manager.h just for the sake of make_unique

[TripleWhy]

One way to make this safer would be to call freeData() here, and then override freeData() in the busses that don't need to do anything with an empty function

[blazoncek]

Can you elaborate a bit more?
I was thinking if Bus does not allocate any data for itself (but provides non-virtual methods) a derived class should take care of deallocation if it allocates anything.
My first thoughts were to implement automatic deallocation upon destruction, but some derived classes assign _data pointer to class member variables to reduce heap fragmentation. In such case no deallocation should happen.

[TripleWhy]

I was thinking about something like this:

//Bus
virtual Bus::~Bus() { freeData(); }
virtual void Bus::freeData() { if (_data != nullptr) free(_data); _data = nullptr; }

//Bus1 uses _data and doesn't override freeData()

//Bus2 doesn't use _data
virtual void Bus::freeData() {}

However:
freeData checks if _data is nullptr, so really I don't think you'd have to do anything like that:

//Bus
virtual Bus::~Bus() { freeData(); }
void Bus::freeData() { if (_data != nullptr) free(_data); _data = nullptr; }

//Bus1 uses _data and doesn't override freeData()
//Bus2 doesn't use _data and doesn't override freeData()

Should work just fine, shouldn't it?

Another option would be to move _data into the sub classes, so each class would be responsible for their own memory.

[willmmiles]

Another option would be to move _data into the sub classes, so each class would be responsible for their own memory.

This would be my preferred option. It also lets each class use type-safe storage and descriptive names for their state data as well.

[blazoncek]

I find that counterintuitive.

All derived classes may find use of _data pointer but some may allocate space on heap, some on stack (or on heap within its instance). For those that use heap, allocateData() and freeData() are provided.
If I understand virtual methods correctly if you override virtual base method in a derived class then you have to call base method explicitly if you want to use its implementation. I.e.

void Derived::freeData() {
  ... some stuff ...
  Base::freeData();
  ... more stuff ...
}

or implement functionality of Base::freeData() in derived (a nonsense). Having virtual Base::freeData() that does nothing (and is not pure virtual) and having pure virtual (that needs to be implemented in derived class) seems counterproductive in this particular instance as allocateData() does something.

As allocateData() and freeData() are not virtual, two less pointers need to be allocated for derived class' vtables (which is good in our case). Of course that means that when derived class uses heap allocated data, it needs to call allocateData() at some point and then (at least in destructor) call freeData(). All is well and also well defined.
One could add heap/stack detection into virtual Base::freeData() (if that approach is taken) so that correct de-allocation may happen, but I think that is a hack best avoided as it is architecture dependant.

BTW: All of the derived classes use _data pointer in one way or another, but only BusDigital and BusNetwork use heap allocated buffers. When I first implemented buffers, _data was defined in those two derived classes (as suggested), but that meant to also implement 2 allocateData() and 2 freeData() methods. I found that redundant and prone to error more than forgetting to call base class freeData().

[TripleWhy]

do we need allocateData or freeData?

allocateData is called exactly once, during construction (in 2 classes). Which means there is no need for gating or freeing _data before allocation. The zero length check is probably also unnecessary, leaving only one effective call: calloc. Does it make sense to abstract that? Not really. Does it improve anything to have allocateData that can be used in two classes instead of calloc (which can also be used in both classes)? Well, no.

What about freeData? Once again, it is called exactly once, during destruction (in 2 classes). Oh but it performs a nullptr check, that has to be useful, right? well, no; free does nothing when called with nullptr. Ok but it also sets _data to nullptr when it's done. Which sounds like a good safety move, but remember, this happens during destruction, so even reading the value _data would be unsafe. So really this function can be replaced with a single free call as well.

About the error-proneness of this all: using calloc and free is error prone. If you are worried about that, consider unique_ptr or other type safe per-class solutions instead :)

[blazoncek]

What about Bus::_data then? It should also be eliminated and moved into those 2 classes that use it. Right?

[TripleWhy]

Well, in an ideal world, yes. I'm not sure how other functions that currently use _data would work then though.

[TripleWhy]

I still think adding public and extern gloabals is a really bad idea

[blazoncek]

Even if enclosed in namespace?
I could do without but then functions could no longer be inline (not that this is good or bad).

[TripleWhy]

• They are public now. Anyone can access and modify them. Functional safety isn't about not doing the wrong thing because you know what you are doing, its about preventing the wrong thing even if not everyone knows what they are doing.
• You now have to separate definition and initialization.
• extern can prevent optimizations

Avoiding this would work by keeping the class or moving the variables to the cpp. I don't know how the latter affects embedded code though.

[willmmiles]

Avoiding this would work by keeping the class or moving the variables to the cpp. I don't know how the latter affects embedded code though.

Moving the declarations in to the cpp would give up any inlined access -- everything would have to be a function call. We don't have link-time optimization to help us here. :(

The previous code (a non-instantiated class, where everything is static) is the only way I know of to get both access control and still support inlining.

[blazoncek]

WLED is full of globals, accessible/accessed throughout the code.
If you'll look many may be shadowed in certain functions which is prone to error (forget or accidentally remove definition and you are using global).
Using namespaced variable reduces the possibility to inadvertently use such global. But yes, I agree that having globals is poor programming practice.

As we discuss these issues it may indeed be better to forego namespace and keep using instanceless class.