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

prex

I have a user interface application, which creates a QThread for the connection to the hardware. The connection interface to the hardware runs in a while loop (actually ros::spin()). I use QProperties and signals and slots for the interaction with the Qml interface. The application runs fine but I have some doubts that I did everything correctly.

The problems I see:

In this ros::spin() while loop, it is clear to me that I can set QProperties and invoke signals. But what about my public slots and QProperties. It seems to work fine, although the documentation says:
“This means that all of QThread’s queued slots and invoked methods will execute in the old thread. Thus, a developer who wishes to invoke slots in the new thread must use the worker-object approach; new slots should not be implemented directly into a subclassed QThread.”
So does this mean I am using it completely wrong or can I do it like this since my functions are so lightweight?
“When subclassing QThread, keep in mind that the constructor executes in the old thread while run() executes in the new thread.”
This seems to be relevant since I’m doing the initialization of the ROS system during construction. It seems like accessing variables from different threads would happen quite often for my case here.
I’m missing the exec() function, but I couldn't figure out why I would need it. I read that it's needed for signals and slots to work properly, but they still worked for me.
I have the ros::spin(), which is basically a while loop. I currently have a closing button, which calls the quit slot of the thread. But it doesn’t seem to end the thread properly by calling the destructor. I need to shutdown the ROS node, which is currently done in the destructor and would lead to a termination of run. My destructor is never called if I push the closing button.

Here are the most important parts of the code:

main.cpp

...
  ros_node::QROSNode *qrosnode = new ros_node::QROSNode(argc, argv);
  // Connections:
  QObject::connect(qrosnode, SIGNAL(finished()), &app, SLOT(quit()),
                   Qt::QueuedConnection);  
  QObject::connect(&app, SIGNAL(aboutToQuit()), qrosnode, SLOT(quit()),
                   Qt::QueuedConnection);  
...

qrosnode.h

....
class QROSNode : public QThread { 
  Q_OBJECT 

    Q_PROPERTY(QVector<double> temp READ temp WRITE setTemp NOTIFY tempChanged) as a QML property, all of the elements are functions

public:
    QROSNode(int argc, char** argv);
  virtual ~QROSNode();

  /**
   *  Q_PROPERTY
   */

  QVector<double> temp() const;         // public getter method of property
  void setTemp(QVector<double> value);  // public setter method of property


 private:
  int init_argc;
  char **init_argv;
  bool initNode();
  void run() override;

  bool init(ros::NodeHandle &nh, ros::NodeHandle &nh_private);

  ros::ServiceClient reset_client_;

  QVector<double> m_temp;

 signals:
  void tempChanged();
 

 public slots:
  void resetSystem();
};
}
...

qrosnode.cpp

...

QROSNode::QROSNode(int argc, char **argv)
    : init_argc(argc),
      init_argv(argv),
      m_temp(8, 0) {
  initNode();  // initializes ROS node and starts QThread with spinner
}

QROSNode::~QROSNode() {
  std::cout << "ROS Node shutdown started" << std::endl;
  if (ros::isStarted()) {
    ros::shutdown(); 
    ros::waitForShutdown();
  }
  wait(); 
}

bool QROSNode::initNode() {
  ros::init(init_argc, init_argv, "nav_gui");
  if (!ros::master::check()) {
    return false;
  }
  ros::start();
  ros::NodeHandle nh;
  ros::NodeHandle nh_private("~");

  // initialize ROS
  init(nh, nh_private);

  start();  // start a QThread, which calls run()
  std::cout << "Successfully initialized node." << std::endl;

  return true;
}

bool QROSNode::init(ros::NodeHandle &nh, ros::NodeHandle &nh_private) {

  reset_client_ = nh.serviceClient<std_srvs::SetBool>("navion/set_enabled");

  return true;
}

void QROSNode::run() {
  // QThread function
  std::cout << "ROS Spin started" << std::endl;
  ros::spin();
  std::cout << "ROS shutdown." << std::endl;
}

/**
 *  Qt Public Slots
 */

void QROSNode::resetSystem() {
  std::cout << "System Reset" << std::endl;
  std_srvs::SetBool srv;
  srv.request.data = true;
  if (reset_client_.call(srv)) {
    ROS_INFO_STREAM("Service called. Success: " << srv.response.success);
  } else {
    ROS_INFO_STREAM("Service call failed.");
  }
}
...

SGaist

Hi,

If you want to stop cleanly, you should add a method that properly shuts down ros so that your node exits as expected.

prex

Hi,
Would you recommend an isInterruptionRequested() or stopping ROS from another slot function? Right now, ROS gets terminated somehow, but I don't see why. And why is my destructor not called, which should do exactly what you proposed: Shutting down the node properly?
I was also reading that Qthread::quit() is only related to exec(), which I don't have...

kshegunov

What does ros::spin(); do? Is it interruptable, is it possible to call it with a timeout?

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

But what about my public slots and QProperties. It seems to work fine, although the documentation says:

This would depend on how it's implemented. In the general case, no, queued slot calls won't be called unless you return control to the event loop. And as it seems you don't start one, then I'd say no, queued slot calls won't be invoked ever.

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

This seems to be relevant since I’m doing the initialization of the ROS system during construction. It seems like accessing variables from different threads would happen quite often for my case here.

Don't! The QThread constructor is to initialize the object, not the thread data. Prepare your thread after you've entered QThread::run, that's the stack root of your new thread, the QThread object is just managing it (despite its name).

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

I’m missing the exec() function, but I couldn't figure out why I would need it. I read that it's needed for signals and slots to work properly, but they still worked for me.

It's needed to have slots defer-invoked. That is you need it if a signal from another thread is supposed to trigger a slot in your thread. If you connect signal and slots across your own thread the call defaults to Qt::DirectConnection and is equivalent to a direct invocation. Emitting signals itself doesn't require an event loop and is going to work regardless of you having a running event loop or not.

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

I have the ros::spin(), which is basically a while loop. I currently have a closing button, which calls the quit slot of the thread. But it doesn’t seem to end the thread properly by calling the destructor. I need to shutdown the ROS node, which is currently done in the destructor and would lead to a termination of run. My destructor is never called if I push the closing button.

This is expected. The button is in a different thread and it queues the call to quit in your thread. However your thread doesn't have a running event loop so it never processes the event, thus it never calls the slot, thus it never quits. You really need to rethink the design and importantly so it's going to depend on the implementation of ros::spin.

prex

@kshegunov

Ok, much to learn for me. Let's start with these:

What does ros::spin(); do? Is it interruptable, is it possible to call it with a timeout?

ros::spin() is responsible for processing the callbacks in ROS. It defines when to process the ROS subscriptions, services, etc. in your node by calling the user defined callback functions. The ros::spin() function can also be replaced by:

while (ros::ok()) {
    ros::spinOnce();
}

If ros::shutdown() is called, the while-loop terminates.

However your thread doesn't have a running event loop so it never processes the event, thus it never calls the slot, thus it never quits.

I see that I need to call exec() in run(). But where do I put my ROS spinner now? Can I add it to exec() somehow? Seems like I have a combination of two loops (exec() and ros::spin()) which both need to run in the same thread.

In the general case, no, queued slot calls won't be called unless you return control to the event loop. And as it seems you don't start one, then I'd say no, queued slot calls won't be invoked ever.

I think I understand why this worked before. The public slots (e.g. QROSNode::resetSystem()) were still in the old thread and made the service call from there. I don't know how this is implemented, but obviously ROS spin could access this planned service call from inside the thread. So I'm wondering how do I get these public slot functions inside the thread (e.g. QROSNode::resetSystem())?

Prepare your thread after you've entered QThread::run.

Alright, will do this. But what about class objects and class variables like all the ones from the Q_PROPERTIES. Do I need to define them in a special way to make sure they're only accessed by the thread?

Am I safe with defining functions which are just called from inside the thread? They live also in the thread if called by the ROS spinner, right?

SGaist

At which speed should spin run ?

Depending on that you might use a QTimer and the worker object approach.

prex

@SGaist Our other nodes run at 100 Hz.

SGaist

Then try the QTimer approach.

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.