Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

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



  • 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:

    1. 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?

    2. “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.

    3. 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.

    4. 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.");
      }
    }
    ...
    

  • Lifetime Qt Champion

    Hi,

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



  • 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...


  • Qt Champions 2017

    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.



  • @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?


  • Lifetime Qt Champion

    At which speed should spin run ?

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



  • @SGaist Our other nodes run at 100 Hz.


  • Lifetime Qt Champion

    Then try the QTimer approach.



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


  • Lifetime Qt Champion

    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.



  • 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


  • Qt Champions 2017

    @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.



  • @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.


  • Qt Champions 2017

    @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.



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


  • Moderators

    @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, ...



  • @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().


  • Moderators

    @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,

    1. Connect QThread::finished() to Controller::finished()
    2. 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()?



  • @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.)


  • Moderators

    @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.



  • @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.


  • Moderators

    @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.



  • @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.


  • Qt Champions 2017

    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
    }
    

  • Moderators

    @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


  • Qt Champions 2017

    @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.



  • @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:

    connections_small.jpg

    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?


  • Qt Champions 2017

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



  • 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.


  • Qt Champions 2017

    @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).



  • @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.


  • Qt Champions 2017

    @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();
    }
    

Log in to reply