Proper way of creating an interface with a while-loop in a QThread

prex

Not related to the spin():
So this means there is no way to use public slots with a subclass thread? Or is exec() executing the slot functions in the thread?

SGaist

The problem is not using slots, it's in which thread they are executed in. The QThread object has affinity with the thread that created it. So if you subclass QThread and re-implement run (no problem with that) and add custom slots to your subclass, these slots will execute in the thread that created your subclass object, not in the thread that will execute the run method.

As for your subclass of QThread, you can use the structure shown in the QThread::isInterruptionRequested in addition to rose::ok() and call ros::spinOnce() in your loop. That way you have a clean stopping point.

prex

To be clear, I need to process the other public slots (connected to the UI) and the ros::spin() at the same time. So I don't see how the worker-approach could be used here at all, since the spin() would block it from running other public slots. And that's why a subclass seems to be the way to go. Do you agree?

Then the second thing. Since I have two event loops (exec() and spin()), it seems to make sense to use a QTimer with the subclass method. This leads to a proper event handling and running ROS system in the same thread.

So what I still don't understand is how to bring my public slots to the thread. I read in the documentation: "new slots should not be implemented directly into a subclassed Qthread". And I have the situation that someone presses a button in the GUI, which invokes a signal connected to the public slot which calls the ROS service. This public slot should then be in the same thread as the ros::spin() now running in the Qtimer, I assume. But how?

It's hard for me to understand why there even exits an exec() function in Qthread if I can not use slots with the subclass method.

@SGaist

add custom slots to your subclass, these slots will execute in the thread that created your subclass object, not in the thread that will execute the run method.

I understand this. But what is the solution then?

Edit: Another approach which I could think of would be to use the worker approach and having one function which defines the QTimer which calls the ROS spinOnce(). And then I can define several public slots in the worker for handling the signals. Is this what you two proposed? But is the event loop running in the worker approach? Otherwise QTimer won't be executed I guess.

Reference:
https://doc.qt.io/qt-5/qthread.html
https://wiki.qt.io/Threads_Events_QObjects#Events_and_the_event_loop

kshegunov

@prex said in Proper way of creating an interface with a while-loop in a QThread:

To be clear, I need to process the other public slots (connected to the UI) and the ros::spin() at the same time. So I don't see how the worker-approach could be used here at all, since the spin() would block it from running other public slots. And that's why a subclass seems to be the way to go. Do you agree?

No. That's why I asked for the implementation. Does ros::spin block for a long period, or is it intermediately blocking? That'd be a deciding factor of how to implement. If it blocks but for a short time, e.g. to process something and then returns the control, then probably what you want is the worker object approach with a suitable timer to spin your loop. If on the other hand it is a long-blocking operation, you should research how to make it interruptable (both approaches need this).

Then the second thing. Since I have two event loops (exec() and spin()), it seems to make sense to use a QTimer with the subclass method. This leads to a proper event handling and running ROS system in the same thread.

So what I still don't understand is how to bring my public slots to the thread. I read in the documentation: "new slots should not be implemented directly into a subclassed Qthread". And I have the situation that someone presses a button in the GUI, which invokes a signal connected to the public slot which calls the ROS service. This public slot should then be in the same thread as the ros::spin() now running in the Qtimer, I assume. But how?

The timer generates periodic events that are the "ticks" of your loop. Minimalistic implementation could look like this:

void QROSNode::run()
{
    QTimer * timer = new QTimer();
    QObject::connect(timer, &QTimer::timeout, timer, [this] () -> void  {
        if (!ros::ok())
            QMetaObject::invokeMethod(this, &QThread::quit);  // This is executed in the timer's thread context
        ros::spinOnce();
    });
    QObject::connect(this, &QThread::finished, timer, &QObject::deleteLater);
    timer->start(100); // 100ms ticks

    QThread::run();  // Just calls QEventLoop::exec()
}

It's hard for me to understand why there even exits an exec() function in Qthread if I can not use slots with the subclass method.

You can't execute slots, because you've taken control of the program flow and you never return that control back to the/a event loop. A thread is nothing more than a function and there's no way Qt can process events if you never give it an opportunity to do so. Basically you've went lowest-level threading there possibly could be with Qt by subclassing the thread. What this is useful for is for when you don't need an event loop, like crunching numbers. Then you do the thread synchronizations (i.e. making sure each piece of data is accessed by one thread, and one thread alone) by hand through QMutex, QSemaphore, etc.

@SGaist

add custom slots to your subclass, these slots will execute in the thread that created your subclass object, not in the thread that will execute the run method.

I understand this. But what is the solution then?

Solution to? What @SGaist suggests is to use the worker object approach which is event driven, meaning that you'd scrap your current subclass and move the relevant code to a QObject subclass. Then an object of that class is to be moved to the thread and the slots are going to be triggered there, in the new thread.

https://doc.qt.io/qt-5/qthread.html
Top example is what he's talking about.

Edit: Another approach which I could think of would be to use the worker approach and having one function which defines the QTimer which calls the ROS spinOnce(). And then I can define several public slots in the worker for handling the signals. Is this what you two proposed?

Yes. That's the basic idea.

But is the event loop running in the worker approach? Otherwise QTimer won't be executed I guess.

Unless you block it for a long time with the call to ros::spinOnce (or another function), yes, the event loop is going to be running.

prex

@kshegunov

Minimalistic implementation could look like this:

Are you refering to a worker-approach example or a subclass example? Not so clear since you call both methods run(). So in this example it seems like you create the QTimer outside of the thread. And I thought a thread should never be started with QThread::run(), but rather with QThread::start(). Or should this QThread::run() rather be QThread::exec(). Then it would make sense again.

Just for information:
ROS gives me two ways of handling their events. ros::spin() is blocking forever. But ros::spinOnce() is returning the control. So I can see how I can implement the ROS spinner with both methods (-> what you showed in your QTimer example, thanks). But since public slots are only available in the worker approach, it seems like I have no choice.

kshegunov

@prex said in Proper way of creating an interface with a while-loop in a QThread:

Are you refering to a worker-approach example or a subclass example?

Sort of both, sort of neither. I'm lazy so I hacked a hybrid between the two, as you already had subclassed the QThread. It basically does what you'd get if you were to use the worker-object approach. I just used the run() of your subclass to do the init, which normally would've been done in a slot of your worker object connected to the QThead::started() signal. Again, I'm lazy, so I'm not sure that even compiles out of the box, although it probably should (if I haven't missed a semicolon or something).

And I thought a thread should never be started with QThread::run(), but rather with QThread::start(). Or should this QThread::run() rather be QThread::exec(). Then it would make sense again.

I don't start the thread with run(), I just delegate the run() call to the parent class after the override has done its job. You start the actual thread the usual way:

QROSNode * node = new QROSNode(...);
node->start();

start() is going to call run() in the new thread's context when everything's been set up.

But since public slots are only available in the worker approach, it seems like I have no choice.

Yes and no. You can drive the event loop manually, but I really would advise against that. It's a poor choice of tooling for the purpose you're after.

prex

Thanks. I think I got the big picture. I have the worker approach running.

What it didn't solve is the issue with quitting everything properly. No single destructor is called.

My structure now:

main.cpp -> starts QGuiApplication event loop
controller.cpp (moves Worker to workerThread)
worker.cpp (creates a QTimer)

What I would like to do is if my main is terminated either by closing the GUI or a crash, it also quits the thread. The Controller has a destructor:

Controller::~Controller() {
    std::cout << "Controller Destructor" << std::endl;
    ControllerThread.quit();
    ControllerThread.wait();
}

I would expect that this one is executed if main finishes. But this is not the case. Why? Even when adding a connection to aboutToQuit, it is not called:

QObject::connect(&app, &QGuiApplication::aboutToQuit, controller, &QObject::deleteLater, Qt::QueuedConnection);

The other way round, I would like to terminate my main application (the GUI), if the thread is terminated. I used to have this connection in main:

QObject::connect(..., SIGNAL(...), &app, SLOT(quit()), Qt::QueuedConnection);

However, to what signal can I connect from main?

And the last thing is quitting the thread on a condition and deleting the QTimer as proposed by @kshegunov :

if (!ros::ok())
            QMetaObject::invokeMethod(this, &QThread::quit);

QObject::connect(this, &QThread::finished, timer, &QObject::deleteLater);

Now in the worker approach, I do not have access to QThread. So how can I do this now?

JKSH

@prex said in Proper way of creating an interface with a while-loop in a QThread:

I would expect that this one is executed if main finishes. But this is not the case. Why? Even when adding a connection to aboutToQuit, it is not called:

That suggests that your QApplication object might not be shutting down properly.

How do you stop the event loop in main()?

The other way round, I would like to terminate my main application (the GUI), if the thread is terminated.... However, to what signal can I connect from main?

When the QThread finishes running and shuts down properly, it emits the QThread::finished() signal. You can connect this to QCoreApplication::quit().

Note: To guarantee that QThread::finished() is emitted, you must allow your thread to shut down properly.

Now in the worker approach, I do not have access to QThread.

Yes you do. workerThread is a QThread, right?

QObject::connect(&workerThread, &QThread::finished, ...

prex

@JKSH

That suggests that your QApplication object might not be shutting down properly.

How do you stop the event loop in main()?

I use two methods: I'm either calling Qt.quit() from my Qml ApplicationWindow or I just use the close button on the top bar. I'm assuming that both return from app.exec() (QGuiApplication app;) in main.cpp and so the application properly terminates.

When the QThread finishes running and shuts down properly, it emits the QThread::finished() signal. You can connect this to QCoreApplication::quit().

Ok, an important point. The "hierarchy" of my file is actually main->Controller->Worker. So I don't know how to connect to this QThread::finished() from main because main doesn't "know" anything about the thread. Not sure if I missed something important about signals and slots here, but I thought this is not working with "child of child".

Yes you do. workerThread is a QThread, right? QObject::connect(&workerThread, &QThread::finished, ...

Right, but I can't do this from inside the Worker, because the thread was not declared in this scope. And I need to stop this thread in which the Worker is running somehow if !ros::ok().

JKSH

@prex said in Proper way of creating an interface with a while-loop in a QThread:

How do you stop the event loop in main()?

I use two methods: I'm either calling Qt.quit() from my Qml ApplicationWindow or I just use the close button on the top bar. I'm assuming that both return from app.exec() (QGuiApplication app;) in main.cpp and so the application properly terminates.

OK, that looks fine.

Is your Controller allocated on the stack or the heap?

When the QThread finishes running and shuts down properly, it emits the QThread::finished() signal. You can connect this to QCoreApplication::quit().

The "hierarchy" of my file is actually main->Controller->Worker. So I don't know how to connect to this QThread::finished() from main because main doesn't "know" anything about the thread.

main() doesn't need to know that there's a thread inside the Controller. It just needs to know that the Controller has stopped.

You can "daisy-chain" signals by connecting a signal to another signal. So, create a signal in your controller like Controller::finished(). Then,

Connect QThread::finished() to Controller::finished()
Connect Controller::finished() to QGuiApplication::quit()

I can't do this from inside the Worker, because the thread was not declared in this scope. And I need to stop this thread in which the Worker is running somehow if !ros::ok().

It's good that you've designed your architecture such that main() doesn't "know" anything about the thread. This leads to the question: Does the Worker need to "know" that it's running in a thread?

It would be nice if the Worker code was thread-agnostic. It just needs to notify the world that it has to stop; it doesn't need to call the functions that perform the actual stopping:

// Inside the Worker:
if (!ros::ok())
    emit rosFinished(); // Or pick a more creative name

// Inside the Controller:
connect(worker, &Worker::rosFinished,
        workerThread, &QThread::quit);

Now, if you have set up the daisy-chain as I described earlier, the signal-slot sequence becomes:

Worker::rosFinished() -> QThread::quit() -> QThread::finished() -> Controller::finished() -> QGuiApplication::quit()

And after you've finished implementing this, you should check: Does your Worker need to run in a thread? Since it is doing non-blocking operations, is it performant enough to run directly in main()?

prex

@JKSH said in Proper way of creating an interface with a while-loop in a QThread:

Is your Controller allocated on the stack or the heap?

Both the Controller, as well as the Worker are allocated on the heap.

You can "daisy-chain" signals by connecting a signal to another signal. So, create a signal in your controller like Controller::finished().

I was a little bit afraid of daisy-chaining signals. I already have 4 public slots and 10 Q_Properties in the Worker, which need to be connected in main. So is daisy-chaining the way to go?

It would be nice if the Worker code was thread-agnostic. It just needs to notify the world that it has to stop;

Thanks, I implemented the sequence.

Does your Worker need to run in a thread? Since it is doing non-blocking operations, is it performant enough to run directly in main()?

I am afraid that the Qml Gui starts freezing. The duration of ros::spinOnce() highly depends on the callback functions (which can be computationally intense.)

JKSH

@prex said in Proper way of creating an interface with a while-loop in a QThread:

Both the Controller, as well as the Worker are allocated on the heap.

I'm guessing you have a memory leak. Heap-allocated objects are not automatically destroyed when the application exits (this is standard C++ behaviour) -- that's probably why your Controller's destructor is never called. Note that QCoreApplication::quit() doesn't destroy any objects; it simply stops the main event loop and allows main() to return.

You must somehow ensure the Controller gets deleted before main() returns. One way is to allocate your Controller on the stack in main() -- objects stored in local variables get auto-destroyed before the function returns.

I was a little bit afraid of daisy-chaining signals. I already have 4 public slots and 10 Q_Properties in the Worker, which need to be connected in main. So is daisy-chaining the way to go?

I think daisy-chaining is the least risky way to continue building on your existing code (but I acknowledge it would a bit time-consuming to implement many chains).

The important thing is to get an implementation that works correctly and reliably. Once you have that, you can start learning other ways to implement the same thing more cleanly.

I am afraid that the Qml Gui starts freezing. The duration of ros::spinOnce() highly depends on the callback functions (which can be computationally intense.)

OK, you now have solid proof that you need a dedicated thread.

prex

@JKSH said in Proper way of creating an interface with a while-loop in a QThread:

You must somehow ensure the Controller gets deleted before main() returns.

I thought the deleteLater should do this job:

QObject::connect(&app, &QGuiApplication::aboutToQuit, controller, &QObject::deleteLater, Qt::QueuedConnection);

Same with the Worker. The &QThread::finished signal is emitted. But the destructor of the Worker is never called:

QObject::connect(&workerThread, &QThread::finished, worker, &QObject::deleteLater, Qt::QueuedConnection);

Edit: Just tested &QGuiApplication::aboutToQuit with a simple public slot outputing to the console. And it is now clear that after Qt::quit, aboutToQuit does not execute any public slots. But still don't know why.

The important thing is to get an implementation that works correctly and reliably.

I'm currently using setContextProperty to make things available in Qml:

Controller *controller = new Controller;
engine.rootContext()->setContextProperty("controller", controller);

But since the public slots and Q_PROPERTIES I want to access are now in Worker, what would be the best way to make them available?

I think these two cases need to be distinguished:

Public Slots:

I tried controller.worker.resetSystem(); which unfortunately does not work (created public object worker). However, by "daisy-chaining" the slots, it works fine:

Qml:

reset_button.onClicked: Controller.resetSystem();

Controller:

signals:
    void resetSystem();

QObject::connect(this, &Controller::resetSystem, worker, &Worker::resetSystem, Qt::QueuedConnection);

I hope calling the "signal" from Qml is a proper way of chaining?

Q_PROPERTY:

This is still my main concern. In the worker I have around 14 of

Q_PROPERTY(QVector<double> temp READ temp WRITE setTemp NOTIFY tempChanged)

I opened another topic about this a while ago. But since I presented my situation here as a whole, I would like to ask about the best way to make these Q_PROPERTIES available in my Qml again.

JKSH

@prex said in Proper way of creating an interface with a while-loop in a QThread:

You must somehow ensure the Controller gets deleted before main() returns.

I thought the deleteLater should do this job:
QObject::connect(&app, &QGuiApplication::aboutToQuit, controller, &QObject::deleteLater, Qt::QueuedConnection);

When the QGuiApplication emits aboutToQuit(), it is saying, "This is the very last iteration of the event loop!"

Qt::QueuedConnection tells the event loop to schedule an event for the next loop iteration. However, there are no more iterations after aboutToQuit(), so those scheduled events will never be acted upon.

Remove the Qt::QueuedConnection argument here. In fact, do this for all your connections. There is rarely a need for you to specify the connection type -- it is best to let Qt choose the connection type automatically.

In any case, why don't you just allocate the Controller on the stack in main()? That will make your life a lot easier.

I'm currently using setContextProperty to make things available in Qml:

OK

I hope calling the "signal" from Qml is a proper way of chaining?

Yes. This causes the QML object to emit the signal.

This is still my main concern. In the worker I have around 14 of
Q_PROPERTY(QVector<double> temp READ temp WRITE setTemp NOTIFY tempChanged)

You've got a tempChanged() signal; you can use signals and slots to transfer your data back to the main thread.

Given that your Worker is a private object inside the Controller, does the Worker still need Properties?

...since I presented my situation here as a whole, I would like to ask about the best way to make these Q_PROPERTIES available in my Qml again.

This is really asking for systems design consultation, which is not easy to do on an online forum. You have provided a lot more info compared to the other thread, but it still doesn't show us a full overview of your application. (Having said that, it is not quite appropriate to post an entire project for a forum unless it's a tiny project)

Make use of the tempChanged() signal first. Get it working. Then, you will be in a much better position to look for the "best" way.

prex

@JKSH said in Proper way of creating an interface with a while-loop in a QThread:

Remove the Qt::QueuedConnection argument here. In fact, do this for all your connections. There is rarely a need for you to specify the connection type -- it is best to let Qt choose the connection type automatically.

You're my hero! Finally destructing them properly. I somehow picked up that I should ALWAYS use queued connections in order not to miss something. But for a real-time control application, queuing up would probably be the totally wrong approach anyway.

Given that your Worker is a private object inside the Controller, does the Worker still need Properties?

I thought this would make my application look nicer than connecting all these signals to the GUI manually. I really like the functionality of Q_Properties in Qml since I can use them both as "receiver" and "sender" at the same time. Which means I'm sometimes setting a Q_Property to a number, which needs to be processed by my worker and I'm sometimes sending a number from my worker, which are shown in the GUI.

This is really asking for systems design consultation, which is not easy to do on an online forum.

I know. But I think I'm nearly there. Design consultation by someone experienced would be a next step if I have it up and running.

kshegunov

One important detail I think we haven't mentioned is you really should wait for the thread to actually close before allowing the application to quit (just as in your original example). Assuming the thread's a member of your controller class you'd want to have this (or equivalent) in the destructor:

Controller::~Controller()
{
    thread->quit();
    thread->wait(); //< Before allowing the stack to unwind you want to be sure the thread has really finished
}

JKSH

@prex said in Proper way of creating an interface with a while-loop in a QThread:

Finally destructing them properly.

Great!

I somehow picked up that I should ALWAYS use queued connections in order not to miss something.

Definitely not.

But for a real-time control application, queuing up would probably be the totally wrong approach anyway.

Yep.

Given that your Worker is a private object inside the Controller, does the Worker still need Properties?

I thought this would make my application look nicer than connecting all these signals to the GUI manually. I really like the functionality of Q_Properties in Qml since I can use them both as "receiver" and "sender" at the same time. Which means I'm sometimes setting a Q_Property to a number, which needs to be processed by my worker and I'm sometimes sending a number from my worker, which are shown in the GUI.

Right now, the fact that you have a worker object is merely an implementation detail.

As far as your GUI is concerned, it only has to deal with the "ROS Controller". So, you could implement the properties in your Controller (and give it a new name).

@kshegunov said in Proper way of creating an interface with a while-loop in a QThread:

Controller::~Controller()
{
    thread->quit();
    thread->wait(); //< Before allowing the stack to unwind you want to be sure the thread has really finished
}

Good reminder. @prex does has this in place: https://forum.qt.io/post/574148

kshegunov

@JKSH said in Proper way of creating an interface with a while-loop in a QThread:

Good reminder. @prex does has this in place:

As noted, I put it there so to make sure it isn't removed when switching to the WO.

prex

@kshegunov said in Proper way of creating an interface with a while-loop in a QThread:

One important detail I think we haven't mentioned is you really should wait for the thread to actually close before allowing the application to quit (just as in your original example).

Thanks, indeed, I didn't forget about this.

I just tried the approach with a single Q_Property holding the complete QROSNode as a pointer. This didn't work out too well:

QQmlEngine: Illegal attempt to connect to Worker(0x5654bb6f8800) that is in a different thread than the QML engine QQmlApplicationEngine(0x7fffa29553f0.

So let's better take a look at your approach:

@JKSH said in Proper way of creating an interface with a while-loop in a QThread:

Right now, the fact that you have a worker object is merely an implementation detail.
As far as your GUI is concerned, it only has to deal with the "ROS Controller". So, you could implement the properties in your Controller (and give it a new name).

So what you propose is to put my 14 Q_Properties in Controller and not having any Q_Property in the Worker anymore (but keeping the signals and slots)? Why a new name? Or is there a smart way to "daisy-chain" Q_Properties?

I tried to come up with a possible solution:

I'm not sure if I should just call my service functions in the Worker as a class method, or make a connection. Is there any difference?

And the second thing is that I will need a second "WRITE" function (setter fct), which sets my m_temp variable and emits NOTIFY. If I would connect to WRITE from my Worker, this would call my service slot again, although it should just update the GUI.

Unfortunately, this results in 14 additional setter functions and 28 additional connections.

What do you think?

kshegunov

@prex said in Proper way of creating an interface with a while-loop in a QThread:

I'm not sure if I should just call my service functions in the Worker as a class method, or make a connection. Is there any difference?

Since your controller is in the GUI thread (assuming there you created it), and the worker is living in another thread you shouldn't call stuff on the worker directly. If you decide to do so, you must be very, very careful and synchronize the threads manually.

And the second thing is that I will need a second "WRITE" function (setter fct), which sets my m_temp variable and emits NOTIFY. If I would connect to WRITE from my Worker, this would call my service slot again, although it should just update the GUI.

You need to elaborate a bit, are these properties you have in the controller important for the worker to operate, and how? Is the worker going to modify them? Is the worker only going to report changes to them? Are they supposed to be both written and read to respectively from the worker thread?