For quite some time, I’ve been wondering if it is good practice or not to call the Thread::start(...) method in the class constructor, what are the risks and drawbacks, etc.
I have the following:
class Widget
{
public:
// is this good practice?
Widget() { _thread.start({&_event_queue, &events::EventQueue::dispatch_forever}); }
void doSomething() {
// do something using the event queue
}
private:
rtos::Thread _thread {};
events::EventQueue _event_queue {};
};
auto w = Widget {};
int main() {
while (true) {
w.doSomething();
rtos::ThisThread::sleep_for(1s);
}
return 0;
}
Also, on a side note, I’m wondering is it’s better to have them as class members or as references and use dependency injection?
I don’t see a reason against it, if you are sure that the call will succeed. If you require a return value from the constructor, you could pass a return value reference to it anyway (ugly workaround due to the lack of exceptions).
As for your second suggestion, I am not a big fan of lazy initializations, which can be hard to track, e.g. the mbed_event_queue(), which must be called from a thread context before getting called from an ISR context, otherwise it fails…
BTW why do you create the widget instance outside of main? The initialization order of global instances are undefined and could create weird bugs too. I would rather create it in main and pass its reference to the dependent classes to their constructors.
After giving it a second thought, I came to the same conclusion.
Can you elaborate on that?
That’s a good question!
We used to do it in main, but “decided” to move them out for clarity and because we thought it might make them more “static”, if that makes sense, but it might be a terrible misconception.
Actually it is not, as long as they are in the same translation unit, which is the case with main.cpp.
non-local variables: within a single translation unit, initialization of these variables is always sequenced in exact order their definitions appear in the source code.
From my understanding, I should be ok, but I might be missing something super important, please correct me if I’m wrong.
Would the following be better?
// ...
static auto w = Widget {};
int main() {
while (true) {
w.doSomething();
rtos::ThisThread::sleep_for(1s);
}
return 0;
}
Correct, but what if you decide to split the some functionality into another file in the future? Furthermore, they get created before main is called, which could be undesirable too, if you’re doing some pre-main stuff…
Using the static keyword is always a good practice to limit the scopes.
Check out the source of mbed_event_queue(), aka the shared event queue. It is exactly like your second proposal, it only creates the thread in the first call. Therefore if you try to call it from an ISR for the first time, it fails due to mutexes not being allowed in ISR context.
Only in .cpp files, in headers included in multiple translation unit directly or indirectly it will not do what I thought it would… I ended up multiply a 4096 bytes buffer several times, not understanding why everything was crashing down. Turns out what I really wanted to do was just inline auto var = ...
I have to admit I don’t know what this means. What is pre-main stuff? I’ve seen it multiple times but I could not figure out what it was and why/when I would need that?
