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

[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?

[prex]

So I made the connection as proposed and the whole thing is running without any issues now.

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

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.

Ok!

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

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?

The Q_Properties are written and read from both the GUI and the Worker (thread).

The user can input a value, e.g. temperature in the GUI, and it is sent to the "hardware" in the Worker. On the other side, the worker can receive a temperature from the hardware and changes it in the GUI. I realized that most of the time, these connections are not bi-directional, but there are some situations.

So my idea in the sketch was to connect the GUI to the Q_Properties in the controller (read and write from the GUI). The WRITE slot then calls the function in the Worker thread (public slot). To send the same parameter from the Worker to the GUI, I can not connect to the same WRITE slot, since this would call my Worker public slot. Therefore, I just use a second public slot in the Controller, which can change the Q_Property variable and emit NOTIFY, but is not linked to the Q_Property.

[kshegunov]

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

The Q_Properties are written and read from both the GUI and the Worker (thread).
The user can input a value, e.g. temperature in the GUI, and it is sent to the "hardware" in the Worker. On the other side, the worker can receive a temperature from the hardware and changes it in the GUI. I realized that most of the time, these connections are not bi-directional, but there are some situations.
So my idea in the sketch was to connect the GUI to the Q_Properties in the controller (read and write from the GUI). The WRITE slot then calls the function in the Worker thread (public slot). To send the same parameter from the Worker to the GUI, I can not connect to the same WRITE slot, since this would call my Worker public slot. Therefore, I just use a second public slot in the Controller, which can change the Q_Property variable and emit NOTIFY, but is not linked to the Q_Property.

You lost me here. As far as I understand your setup, you need to duplicate the setters and signals between the worker object and the controller object. That's okay in principle, as long as you connect the worker's notify to the controller's setter, and the controller's notify to the worker's setter you shouldn't have much trouble.

Calling stuff directly (be it setters/getters/slots or w/e) is ill advised; it's a race condition. If you don't want to have the signals you can sync the threads manually (but through the event loop) with:

QMetaObject::invokeMethod(object, std::bind(&ReceiverClass::receiverMethod, object, argumentToPassToMethod));

You can use that either with default connection type (same semantics as with QObject::connect), which in this case is Qt::QueuedConnection as the call is across thread boundaries, or with Qt::BlockingQueuedConnection if you need to actually enforce a barrier on the threads (i.e. the sender's waiting for the receiver to process the slot).

[prex]

@kshegunov I don't really understand.

I don't have any Q_Properties in the Worker anymore. I currently only see a use for them between the Controller and the Qml Application.

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

That's okay in principle, as long as you connect the worker's notify to the controller's setter, and the controller's notify to the worker's setter you shouldn't have much trouble.

So you send the value with your NOTIFY?

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

Calling stuff directly (be it setters/getters/slots or w/e) is ill advised;

You mean over different threads or also in the same function?

I will try to explain again what I'm doing as in the sketch above:
I have all my Q_Properties in my Controller. Both the GUI as well as the Worker need to access READ and WRITE. For the Qml GUI, this is perfectly fine with setContextProperty(). For the Worker, I made the connections myself (Controller's NOTIFY to Worker's public slot). However, if the Worker writes to the Q_Property by using a connection to WRITE, this calls NOTIFY again, which calls the public slot in the Worker again, ending up in a loop.

[kshegunov]

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

I don't have any Q_Properties in the Worker anymore. I currently only see a use for them between the Controller and the Qml Application.

Nor should you. You are correct to expose the Q_PROPERTY methods only throught the controller.
(Note: property is also a term that is used to refer to a data member of a class' object in C++ context)

That's okay in principle, as long as you connect the worker's notify to the controller's setter, and the controller's notify to the worker's setter you shouldn't have much trouble.

So you send the value with your NOTIFY?

Okay, let's clear some stuff out of the way. The NOTIFY, WRITE, READ etc macros are for Qt's meta-type system, not for you. You need it for QML, but not for regular C++ code (with the exception of using QObject::setProperty). By "notify" I meant the notification signal, as we are interested in the C++ facet of the code and as far as I understand you've done that already - connecting the notification signal of the first object to the setter of the second, and vice versa.

Calling stuff directly (be it setters/getters/slots or w/e) is ill advised;

You mean over different threads or also in the same function?

A thread is a function, so yes, I mean over different threads. Basically what I say is that an object that's associated with a given thread shouldn't have its methods called directly from another thread (unless special care is taken).

However, if the Worker writes to the Q_Property by using a connection to WRITE, this calls NOTIFY again, which calls the public slot in the Worker again, ending up in a loop.

You can break the loop by emitting the notification signal only if the member's been changed. Meaning your slots should look something like this:

void MyClass::setProperty(const QString & value)
{
    if (m_property == value)
        return;  //< If nothing's changed give up early

    m_property = value;
    emit propertyChanged();
}