Solved another beginner design question
-
...and by "design" I mean the application, not the UI...
My UI thread has a pushbutton to signal the worker thread. The worker's slot will toggle a run state flag. The worker should behave like this:
while (true) { if (runFlag) { do something } else { sleep for a bit } }
So, it's like the first time this button is pressed, it starts the above routine, and then after that, it needs to signal another routine to toggle the run flag. I can think of a few ways to do this, but they all seem hacky. Can someone please advise on the preferred method of doing this?
Thank you.
-
To run a loop through the event loop use:
QMetaObject::invokeMethod(object, "doSomethingSlot", Qt::QueuedConnection);
or
QTimer::singleShot(0, std::bind(object, &Class::doSomethingSlot));
or
QTimer loopTimer; QObject::connect(&loopTimer, &QTimer::timeout, object, &Class::doSomethingSlot); loopTimer.start(0);
or a variation thereof. In 99% of cases you don't want to run a thread without an event loop and in 100% of those cases you don't want to block the event loop's message processing. In the above snippets
object
is an instance of theQObject
derived classClass
- the worker object. -
@kshegunov said in another beginner design question:
In 99% of cases you don't want to run a thread without an event loop and in 100% of those cases you don't want to block the event loop's message processing.
Repost. just because it's the single most important takeaway here.
basically
while(true)
is almost always a bad idea in this context -
So, the QMetaObject::invokeMethod presumably goes in main() somewhere after I've connected the widget/worker signals and slots, yes?
If a while (true) is a bad idea, how do I code a worker thread that runs continuously (pausing to check the run flag)?
-
@mzimmers said in another beginner design question:
So, the QMetaObject::invokeMethod presumably goes in main() somewhere after I've connected the widget/worker signals and slots, yes?
Nope, in the loop body.
If a while (true) is a bad idea, how do I code a worker thread that runs continuously (pausing to check the run flag)?
class Worker : public QObject { Q_OBJECT public: Worker(QObject * parent = nullptr) : QObject(parent), loopTimer(this) { QObject::connect(&loopTimer, &QTimer::timeout, this, &Worker::doStuffInLoop); } bool isLoopRunning() const { return loopTimer.isActive(); } public slots: void startLoop() { loopTimer.start(0); } void stopLoop() { loopTimer.stop(); } private slots: void doStuffInLoop() { // Something to do in the loop body } private: QTimer loopTimer; };
is the simplest implementation for such a thing. Connect your UI signals to
startLoop
andstopLoop
and that's pretty much it.OR with
invokeMethod
:class Worker : public QObject { Q_OBJECT enum State { Active, Inactive }; public: Worker(QObject * parent = nullptr) : QObject(parent), state(Inactive) { } bool isLoopRunning() const { return state == Active; } public slots: void startLoop() { if (state == Active) return; state = Active; QMetaObject::invokeMethod(this, "doStuffInLoop", Qt::QueuedConnection); } void stopLoop() { state = Inactive; } private slots: void doStuffInLoop() { // Something to do in the loop body // Loop back ;) if (state == Active) QMetaObject::invokeMethod(this, "doStuffInLoop", Qt::QueuedConnection); } private: State state; };
-
kshegunov: thank you for the detailed answer. I am going to use the 2nd example you gave, because I understand it better, and I'd prefer to eliminate the timer from the logic.
I notice that you implemented two slots for run control: startLoop() and stopLoop(). My thinking was to have only one, to toggle the run state. If I do it your way, won't I need to change the slot for the run/slot button pressed every time it's pressed? (From startLoop -> stopLoop -> startLoop -> stopLoop etc.)
I connected the button to startLoop() for now to test, and at least that much is working.
Thanks again for the assistance.
-
@mzimmers said in another beginner design question:
If I do it your way, won't I need to change the slot for the run/slot button pressed every time it's pressed? (From startLoop -> stopLoop -> startLoop -> stopLoop etc.)
It's trivial to merge the two methods together in a
toggleLoop(bool start)
and do different things based on the boolean passed.If you use the second method you probably want to make
state
atomic i.e. changeState state;
tostd::atomic<State> state;
to avoid race conditions in case you callisLoopRunning
from another thread -
@mzimmers said in another beginner design question:
If I do it your way, won't I need to change the slot for the run/slot button pressed every time it's pressed? (From startLoop -> stopLoop -> startLoop -> stopLoop etc.)
Really depends on your use case. I imagine 2 buttons - start and stop, so that's why I put two slots in the example. You can add an additional toggle slot, or merge them if you like.
I connected the button to startLoop() for now to test, and at least that much is working.
Good! I write code directly in the forum - no compiling, no testing, so it's good to know I haven't screwed up. ;)
@VRonin said in another beginner design question:
If you use the second method you probably want to make state atomic i.e. change State state; to std::atomic<State> state; to avoid race conditions in case you call isLoopRunning from another thread
Well, I'd discourage calling
isLoopRunning
from another thread to begin with, rather communicate the state change with a signal, otherwise I agree.PS. The same considerations apply for the first piece of code - the one with the timer, but there you can't use an atomic integer.
-
VRonin - yes, that's how I had it originally. I thought kshegunov had separated them for a reason, but as he mentioned, he envisioned two buttons. I put it back now and it's working just fine.
This thread has been very helpful. I'm still curious, though: why is it considered better to avoid "while (true)" in favor of using the Qt constructs to effect a loop in the worker thread? It certainly is less simple.
Oh, another minor question: where/when does the text string "doStuffInLoop" (2nd parameter to invokeMethod()) get resolved to a pointer?
-
@mzimmers said in another beginner design question:
why is it considered better to avoid "while (true)"
Because Qt relies queued on signals and slots to communicate between threads. If you block the event loop with a
for(;;)
you make the thread deaf of any incoming comunication from anywhere else; it can still talk (i.e. emit signals) though.@mzimmers said in another beginner design question:
when does the text string "doStuffInLoop" (2nd parameter to invokeMethod()) get resolved to a pointer?
At runtime unfortunately, that's why the timer alternative is better as it is resolved at compile time. invokeMethod still uses pre-Qt5 string lookup to invoke a slot. You can inspect (most of) the code in the generated moc file
-
@VRonin: if I put a sleep inside my while (true), will the thread still block the event loop?
I tried kshegunov's first example, and I'm getting an error. Here's the source code for the worker thread:
Worker::Worker(QObject * parent) : QObject(parent), value(0) { QObject::connect(&timer, &QTimer::timeout, this, &Worker::runSimulator); } void Worker::reportRunCount() { emit counterChanged(value); } void Worker::toggleRunState() { runState = ~runState; if (runState) timer.start(); } void Worker::runSimulator() { value++; QThread::msleep(100); // just to slow down the thread a little if (value % 10 == 0) { emit counterChanged(value); } if (runState) timer.start(); }
When I try to start the timer, I get this error:
QObject::startTimer: Timers cannot be started from another thread
I guess that the slot toggleRunState is being run from the context of the main thread. So...how do I initiate the first run of runSimulator()? -
@mzimmers said in another beginner design question:
if I put a sleep inside my while (true), will the thread still block the event loop?
Every time.
I tried kshegunov's first example, and I'm getting an error. Here's the source code for the worker thread:
You forgot to parent the timer object to the worker object, take a close look at the example - the
Worker
class' constructor specifically.PS.
If you want to slow down the looping, just fire the timer at longer intervals:void Worker::toggleRunState() { runState = ~runState; if (runState) timer.start(100); //< Every 100ms or so we'd get a timer event (i.e. the slot will be called). }
PS2:
I can't tell from here whatrunState
is, but don't use bit inversion. If it's a boolean use boolean negation, if it's a enum, use assignment, etc.runState = ~runState;
this just looks like you've shot yourself in the foot, but you still haven't felt it. ;)
-
Oops. Corrected and it works well now. Here are the pertinent routines:
void Worker::toggleRunState() { runState = !runState; if (runState) { timer.start(TIMER_INTERVAL_MSEC); } else { timer.stop(); } } void Worker::runSimulator() { value++; if (value % 10 == 0) { emit counterChanged(value); } if (runState) timer.start(TIMER_INTERVAL_MSEC); }
Now, if I wanted to let the worker thread run as fast as possible, would it just be a matter of setting the timer interval to 0? In other words, the timer would no longer be a timer, but a trigger for an iteration of runSimulator().
I ask this because once the simulator begins doing real work, I'll want it to go full throttle.
EDIT: and if the above is true, as I don't want to be flooding the widgets with updates, will I just implement a second timer to emit the counterChanged() signal?
-
You don't need to re-start the timer in runSimulator. a QTimer is automatically repeated unless you call
setSingleShot(true)
@mzimmers said in another beginner design question:
would it just be a matter of setting the timer interval to 0?
Yes. That tells the event loop to fire the slot connected to the timer as soon as it can
@mzimmers said in another beginner design question:
will I just implement a second timer to emit the counterChanged() signal?
Correct
-
VRonin - thanks for the information. Regarding the timer, and the worker loop, it's essential that one iteration of runSimulator finishes before another starts. Can I depend on this happening, or should I use the one shot, and repeat the timer at the end of runSimulator?
-
@mzimmers said in another beginner design question:
one iteration of runSimulator finishes before another starts
Unless you call
runSimulator
from anothere thread (which you don't atm) there is phisically no way for the same thread to execute a (non recursive) function while the same did not finish yet -
@mzimmers
In this kind of scenarios, you should use timer to call a function periodically. NEVER EVER create a while-loop insider your GUI, this will freezes your app.
A suggested solution is// dialog.cpp #include "dialog.h" #include "ui_dialog.h" #include <QTimer> #include <QDebug> Dialog::Dialog(QWidget *parent) : QDialog(parent), ui(new Ui::Dialog), runFlag(false) { ui->setupUi(this); QTimer *timer = new QTimer(this); connect(timer, SIGNAL(timeout()), this, SLOT(onWorkerThread())); timer->start(1000); // delay } Dialog::~Dialog() { delete ui; } void Dialog::onWorkerThread() { if (runFlag){ qDebug() << "Worker Thread is running..."; }else{ qDebug() << "Worker Thread is NOT running..."; } } void Dialog::on_ActivateButton_clicked() { runFlag=true; } void Dialog::on_DisactivateButton_clicked() { runFlag=false; }
//dialog.h #ifndef DIALOG_H #define DIALOG_H #include <QDialog> namespace Ui { class Dialog; } class Dialog : public QDialog { Q_OBJECT public: explicit Dialog(QWidget *parent = 0); ~Dialog(); private slots: void onWorkerThread(); void on_ActivateButton_clicked(); void on_DisactivateButton_clicked(); private: Ui::Dialog *ui; bool runFlag; }; #endif // DIALOG_H
//main.cpp #include "dialog.h" #include <QApplication> int main(int argc, char *argv[]) { QApplication a(argc, argv); Dialog w; w.show(); return a.exec(); }
The GUI is