Solved proper design for first project
-
Great post, kshegunov. I particularly appreciate the link to Maya's post; she and you saved me from disappearing down what was looking like a particularly nasty rabbit hole.
Here's how I envision my little program (and I invite critique):
- there are two threads: the main (UI) thread and the worker thread
- there are two elements that need to be shared by the threads:
-
the state of the start/stop button (modified by UI, used to "gate" worker)
-
the value of the counter (modified by worker, displayed by UI)
- To my conventional mind, the way to do this is to pass pointers to both objects to the worker thread upon creation. The worker will also signal the UI whenever the counter is updated.
How does this look to you?
-
Well, the beauty of the worker object is that you don't need to actually "gate" the thread (assuming I've understood properly what you mean). You can start a worker thread with an event loop and it will just sleep and do nothing until you give it something to do - in the case of the worker object that is a slot being executed in response to a signal. Also Qt makes it easy in such a way that signal-slot calls across threads are by default thread-safe, by introducing thread-affinity - basically each
QObject
"belongs" to a thread and its slots are executed in that thread irrespective of where the triggering signal originated. This means, that your start/stop buttons don't need to start the thread but just initiate the counting (or other computation) and at each step you can signal back the GUI thread by raising a signal from your worker object. Consider the following simple example:class Worker : public QObject { Q_OBJECT public: Worker(QObject * parent = nullptr) : QObject(parent), counter(0), counterTimer(this) { // You should set the parent to all "child" objects appropriately, so they will be moved to the worker thread along with the worker object, as done here for counterTimer // On each thick of the timer we call the signalCounterChange slot QObject::connect(&counterTimer, &QTimer::timeout, this, &Worker::signalCounterChange); } public slots: void startCounter() { counter = 0; counterTimer.start(100); // Counter will be increased every 100ms } void stopCounter() { counterTimer.stop(); } signals: void counterChanged(qint32); private slots: void signalCounterChange() { emit counterChanged(counter); counter++; } private: qint32 counter; QTimer counterTimer; };
And this is all, really, for the worker object, you can then use it by connecting the signals and slots as appropriate:
int main(int argc, char ** argv) { QApplication app(argc, argv); // Initialize GUI here or later ... // Start the worker thread QThread workerThread; workerThread.start(); // Create the worker object and move it to the worker thread Worker * workerObject = new Worker(); //< No parent, otherwise you can't move it to the thread worker->moveToThread(&workerThread); // Connect the signals for cleanup QObject::connect(&app, &QApplication::aboutToQuit, &workerThread, &QThread::quit); // When the app is quitting - stop the thread QObject::connect(&workerThread, &QThread::finished, workerObject, &QObject::deleteLater); // Free the worker object on thread exiting // Connect signals/slots from the worker object to your widget or window here ... // Run the main event loop int result = QApplication::exec(); workerThread.wait(); //< Wait for the worker thread to finish before exiting main. return result; }
-
I'm going to try to get your example to work, then remove the timer. Currently I'm experiencing
two issues:main refers to "app" in a connect() statement; where does this come from?brain dead- the line:
Worker *workerObject = new Worker(); //< No parent, otherwise you can't move it to the thread
Won't compile without a parent argument.
-
@mzimmers said in proper design for first project:
Won't compile without a parent argument.
A typo on my part, the constructor should allow for a default
null
argument:Worker(QObject * parent = nullptr) : QObject(parent), counter(0), counterTimer(this) { //...
-
Yeah, I should have figured that out myself.
So, I'm trying to get this working, and I'm hitting two compile errors that I can't figure out. Here's my main()"
int main(int argc, char *argv[]) { QApplication a(argc, argv); QThread workerThread; SimulatorWidget *widget = new SimulatorWidget(); SimulatorWorker *worker = new SimulatorWorker(); //< No parent, otherwise you can't move it to the thread widget->show(); // Start the worker thread and move the worker object to the worker thread workerThread.start(); worker->moveToThread(&workerThread); // Connect the signals for cleanup QObject::connect(&a, &QApplication::aboutToQuit, &workerThread, &QThread::quit); // When the app is quitting - stop the thread QObject::connect(&workerThread, &QThread::finished, worker, &QObject::deleteLater); // Free the worker object on thread exiting // Connect signals/slots from the worker object to your widget or window here ... QObject::connect(widget, &SimulatorWidget::buttonPressed, worker, &SimulatorWorker::startStopTimer); QObject::connect(worker, &SimulatorWorker::counterChanged(qint32), widget, &SimulatorWidget::doStuff);
On the last line, I'm getting this error:
/home/mzimmers/hatchco/simulator/main.cpp:26: error: expected primary-expression before ‘)’ token
QObject::connect(worker, &SimulatorWorker::counterChanged(qint32), widget, &SimulatorWidget::doStuff);
^I imagine it's some lame syntax error, but I can't see it.
More troubling is this error:
/opt/Qt/5.9/5.9/gcc_64/include/QtCore/qglobal.h:733: error: static assertion failed: No Q_OBJECT in the class with the signal
#define Q_STATIC_ASSERT_X(Condition, Message) static_assert(bool(Condition), Message)
^
This is generated from the line:QObject::connect(widget, &SimulatorWidget::buttonPressed, worker, &SimulatorWorker::startStopTimer);
Does the problem stem from the fact that SimulatorWidget isn't derived from QObject?
Thanks.
-
Remove the
qint32
from the connect statement. Using that version you only pass the address of the function. -
@mzimmers said in proper design for first project:
Does the problem stem from the fact that SimulatorWidget isn't derived from QObject?
If it's a widget then it's a
QObject
, make sure you have theQ_OBJECT
macro at the top of your class. -
Thank you to SGaist and kshegunov. Both suggestions were exactly right. I did have to re-run qmake after adding the Q_OBJECT macro, though, to get rid of some undefined error references.
Now that it builds, I'll do some debugging and report back. Thanks again...
-
This connect statement isn't doing what I expect:
QObject::connect(widget, &SimulatorWidget::buttonPressed, worker, &SimulatorWorker::startStopTimer);
The signal is firing (as evinced by a breakpoint in the function) but the slot routine isn't being called. Here's the widget code:
SimulatorWidget::SimulatorWidget(QWidget *parent) : QWidget(parent) { setLayout(new QVBoxLayout()); lcd = new QLCDNumber(); layout()->addWidget(lcd); QPushButton* button = new QPushButton("Start/Stop"); layout()->addWidget(button); connect(button, &QPushButton::clicked, this, &SimulatorWidget::buttonPressed); } void SimulatorWidget::buttonPressed() { ; // do nothing }
Any ideas what I'm doing wrong? And I realize this example is somewhat odd, but I couldn't think of a more direct way (to send the button signal to the worker slot).
Thanks.
-
Aren't you connecting two slots there ?
buttonPressed should be a signal.
-
Here's what I was trying to do: get the button clicked signal to fire a slot function within the widget, and that slot would also be a signal to the worker.
From your question, I gather that this isn't right, so how do I connect the button press to the worker?
-
Ohhh...I think I just realized something: the signal isn't a routine defined by me. It just gets created for me (by the MOC?).
So, I made these changes:
void SimulatorWidget::buttonPressed() { emit notifyWorker(); }
and in main:
QObject::connect(widget, &SimulatorWidget::notifyWorker, worker, &SimulatorWorker::startStopTimer);
The worker slot now gets called when the button is pressed.
Now, though, my worker slot for the timer:
QObject::connect(&counterTimer, &QTimer::timeout, this, &SimulatorWorker::signalCounterChange);
isn't working. The slot signalCounterChange() is never called. Any ideas here?
Thanks.
-
I'd check that
counterTimer.start(1000)
is really called. -
Yeah, that was basically it. When I stitched together the examples from above, I neglected to notice that my worker object had two timers declared, and I wasn't consistent on which I used.
Works now. I've tested multiple starts and stops, and verified that the "finished" signal works, too. I do believe I have a functional example. I also modified the program so the worker passes the value to the widget for display.
So, to summarize what I've learned here:
- run at least two threads: one for the UI and one to do work. This prevents compute-intensive tasks from blocking the UI updates.
- create a class for the worker, and instantiate an object in main().
- use moveToThread to move the worker object to another thread.
- make your worker interrupt-driven (work depends mostly on signals from UI).
- use the arguments in the signal/slot routines to pass data.
I think I have a basis for reasonable design now. Thank you to everyone who participated in this.