another beginner design question

mzimmers

...and by "design" I mean the application, not the UI...

My UI thread has a pushbutton to signal the worker thread. The worker's slot will toggle a run state flag. The worker should behave like this:

while (true)
{
   if (runFlag)
   {
      do something
   }
   else
   {
      sleep for a bit
   }
}

So, it's like the first time this button is pressed, it starts the above routine, and then after that, it needs to signal another routine to toggle the run flag. I can think of a few ways to do this, but they all seem hacky. Can someone please advise on the preferred method of doing this?

Thank you.

kshegunov

To run a loop through the event loop use:

QMetaObject::invokeMethod(object, "doSomethingSlot", Qt::QueuedConnection);

or

QTimer::singleShot(0, std::bind(object, &Class::doSomethingSlot));

or

QTimer loopTimer;
QObject::connect(&loopTimer, &QTimer::timeout, object, &Class::doSomethingSlot);
loopTimer.start(0);

or a variation thereof. In 99% of cases you don't want to run a thread without an event loop and in 100% of those cases you don't want to block the event loop's message processing. In the above snippets object is an instance of the QObject derived class Class - the worker object.

VRonin

@kshegunov said in another beginner design question:

In 99% of cases you don't want to run a thread without an event loop and in 100% of those cases you don't want to block the event loop's message processing.

Repost. just because it's the single most important takeaway here.

basically while(true) is almost always a bad idea in this context

mzimmers

So, the QMetaObject::invokeMethod presumably goes in main() somewhere after I've connected the widget/worker signals and slots, yes?

If a while (true) is a bad idea, how do I code a worker thread that runs continuously (pausing to check the run flag)?

kshegunov

@mzimmers said in another beginner design question:

So, the QMetaObject::invokeMethod presumably goes in main() somewhere after I've connected the widget/worker signals and slots, yes?

Nope, in the loop body.

If a while (true) is a bad idea, how do I code a worker thread that runs continuously (pausing to check the run flag)?

class Worker : public QObject
{
    Q_OBJECT

public:
    Worker(QObject * parent = nullptr)
        : QObject(parent), loopTimer(this)
    {
        QObject::connect(&loopTimer, &QTimer::timeout, this, &Worker::doStuffInLoop);
    }

    bool isLoopRunning() const
    {
        return loopTimer.isActive();
    }

public slots:
    void startLoop()
    {
        loopTimer.start(0);
    }

    void stopLoop()
    {
        loopTimer.stop();
    }

private slots:
    void doStuffInLoop()
    {
        // Something to do in the loop body
    }

private:
    QTimer loopTimer;
};

is the simplest implementation for such a thing. Connect your UI signals to startLoop and stopLoop and that's pretty much it.

OR with invokeMethod:

class Worker : public QObject
{
    Q_OBJECT

    enum State { Active, Inactive };

public:
    Worker(QObject * parent = nullptr)
        : QObject(parent), state(Inactive)
    {
    }

    bool isLoopRunning() const
    {
        return state == Active;
    }

public slots:
    void startLoop()
    {
        if (state == Active)
            return;

        state = Active;
        QMetaObject::invokeMethod(this, "doStuffInLoop", Qt::QueuedConnection);
    }

    void stopLoop()
    {
        state = Inactive;
    }

private slots:
    void doStuffInLoop()
    {
        // Something to do in the loop body

        // Loop back ;)
        if (state == Active)
            QMetaObject::invokeMethod(this, "doStuffInLoop", Qt::QueuedConnection);
    }

private:
    State state;
};

mzimmers

kshegunov: thank you for the detailed answer. I am going to use the 2nd example you gave, because I understand it better, and I'd prefer to eliminate the timer from the logic.

I notice that you implemented two slots for run control: startLoop() and stopLoop(). My thinking was to have only one, to toggle the run state. If I do it your way, won't I need to change the slot for the run/slot button pressed every time it's pressed? (From startLoop -> stopLoop -> startLoop -> stopLoop etc.)

I connected the button to startLoop() for now to test, and at least that much is working.

Thanks again for the assistance.

VRonin

@mzimmers said in another beginner design question:

If I do it your way, won't I need to change the slot for the run/slot button pressed every time it's pressed? (From startLoop -> stopLoop -> startLoop -> stopLoop etc.)

It's trivial to merge the two methods together in a toggleLoop(bool start) and do different things based on the boolean passed.

If you use the second method you probably want to make state atomic i.e. change State state; to std::atomic<State> state; to avoid race conditions in case you call isLoopRunning from another thread

kshegunov

@mzimmers said in another beginner design question:

If I do it your way, won't I need to change the slot for the run/slot button pressed every time it's pressed? (From startLoop -> stopLoop -> startLoop -> stopLoop etc.)

Really depends on your use case. I imagine 2 buttons - start and stop, so that's why I put two slots in the example. You can add an additional toggle slot, or merge them if you like.

I connected the button to startLoop() for now to test, and at least that much is working.

Good! I write code directly in the forum - no compiling, no testing, so it's good to know I haven't screwed up. ;)

@VRonin said in another beginner design question:

If you use the second method you probably want to make state atomic i.e. change State state; to std::atomic<State> state; to avoid race conditions in case you call isLoopRunning from another thread

Well, I'd discourage calling isLoopRunning from another thread to begin with, rather communicate the state change with a signal, otherwise I agree.

PS. The same considerations apply for the first piece of code - the one with the timer, but there you can't use an atomic integer.

mzimmers

VRonin - yes, that's how I had it originally. I thought kshegunov had separated them for a reason, but as he mentioned, he envisioned two buttons. I put it back now and it's working just fine.

This thread has been very helpful. I'm still curious, though: why is it considered better to avoid "while (true)" in favor of using the Qt constructs to effect a loop in the worker thread? It certainly is less simple.

Oh, another minor question: where/when does the text string "doStuffInLoop" (2nd parameter to invokeMethod()) get resolved to a pointer?

VRonin

@mzimmers said in another beginner design question:

why is it considered better to avoid "while (true)"

Because Qt relies queued on signals and slots to communicate between threads. If you block the event loop with a for(;;) you make the thread deaf of any incoming comunication from anywhere else; it can still talk (i.e. emit signals) though.

@mzimmers said in another beginner design question:

when does the text string "doStuffInLoop" (2nd parameter to invokeMethod()) get resolved to a pointer?

At runtime unfortunately, that's why the timer alternative is better as it is resolved at compile time. invokeMethod still uses pre-Qt5 string lookup to invoke a slot. You can inspect (most of) the code in the generated moc file

mzimmers

@VRonin: if I put a sleep inside my while (true), will the thread still block the event loop?

I tried kshegunov's first example, and I'm getting an error. Here's the source code for the worker thread:

Worker::Worker(QObject * parent)
    : QObject(parent), value(0)
{
    QObject::connect(&timer, &QTimer::timeout, this, &Worker::runSimulator);
}

void Worker::reportRunCount()
{
    emit counterChanged(value);
}

void Worker::toggleRunState()
{
    runState = ~runState;
    if (runState)
        timer.start();
}

void Worker::runSimulator()
{
    value++;
    QThread::msleep(100); // just to slow down the thread a little

    if (value % 10 == 0)
    {
        emit counterChanged(value);
    }
    if (runState)
        timer.start();
}

When I try to start the timer, I get this error:
QObject::startTimer: Timers cannot be started from another thread
I guess that the slot toggleRunState is being run from the context of the main thread. So...how do I initiate the first run of runSimulator()?

kshegunov

@mzimmers said in another beginner design question:

if I put a sleep inside my while (true), will the thread still block the event loop?

Every time.

I tried kshegunov's first example, and I'm getting an error. Here's the source code for the worker thread:

You forgot to parent the timer object to the worker object, take a close look at the example - the Worker class' constructor specifically.

PS.
If you want to slow down the looping, just fire the timer at longer intervals:

void Worker::toggleRunState()
{
    runState = ~runState;
    if (runState)
        timer.start(100); //< Every 100ms or so we'd get a timer event (i.e. the slot will be called).
}

PS2:
I can't tell from here what runState is, but don't use bit inversion. If it's a boolean use boolean negation, if it's a enum, use assignment, etc.

    runState = ~runState;

this just looks like you've shot yourself in the foot, but you still haven't felt it. ;)

mzimmers

Oops. Corrected and it works well now. Here are the pertinent routines:

void Worker::toggleRunState()
{
    runState = !runState;

    if (runState)
    {
        timer.start(TIMER_INTERVAL_MSEC);
    }
    else
    {
        timer.stop();
    }
}

void Worker::runSimulator()
{
    value++;

    if (value % 10 == 0)
    {
        emit counterChanged(value);
    }
    if (runState)
        timer.start(TIMER_INTERVAL_MSEC);
}

Now, if I wanted to let the worker thread run as fast as possible, would it just be a matter of setting the timer interval to 0? In other words, the timer would no longer be a timer, but a trigger for an iteration of runSimulator().

I ask this because once the simulator begins doing real work, I'll want it to go full throttle.

EDIT: and if the above is true, as I don't want to be flooding the widgets with updates, will I just implement a second timer to emit the counterChanged() signal?

VRonin

You don't need to re-start the timer in runSimulator. a QTimer is automatically repeated unless you call setSingleShot(true)

@mzimmers said in another beginner design question:

would it just be a matter of setting the timer interval to 0?

Yes. That tells the event loop to fire the slot connected to the timer as soon as it can

@mzimmers said in another beginner design question:

will I just implement a second timer to emit the counterChanged() signal?

Correct

mzimmers

VRonin - thanks for the information. Regarding the timer, and the worker loop, it's essential that one iteration of runSimulator finishes before another starts. Can I depend on this happening, or should I use the one shot, and repeat the timer at the end of runSimulator?

VRonin

@mzimmers said in another beginner design question:

one iteration of runSimulator finishes before another starts

Unless you call runSimulator from anothere thread (which you don't atm) there is phisically no way for the same thread to execute a (non recursive) function while the same did not finish yet

CroCo

@mzimmers
In this kind of scenarios, you should use timer to call a function periodically. NEVER EVER create a while-loop insider your GUI, this will freezes your app.
A suggested solution is

// dialog.cpp
#include "dialog.h"
#include "ui_dialog.h"
#include <QTimer>
#include <QDebug>

Dialog::Dialog(QWidget *parent) :
    QDialog(parent),
    ui(new Ui::Dialog),
    runFlag(false)
{
    ui->setupUi(this);
    QTimer *timer = new QTimer(this);
    connect(timer, SIGNAL(timeout()), this, SLOT(onWorkerThread()));
    timer->start(1000); // delay
}

Dialog::~Dialog()
{
    delete ui;
}

void Dialog::onWorkerThread()
{
    if (runFlag){
          qDebug() << "Worker Thread is running...";
    }else{
          qDebug() << "Worker Thread is NOT running...";
    }
}

void Dialog::on_ActivateButton_clicked()
{
    runFlag=true;
}

void Dialog::on_DisactivateButton_clicked()
{
    runFlag=false;
}

//dialog.h
#ifndef DIALOG_H
#define DIALOG_H

#include <QDialog>

namespace Ui {
class Dialog;
}

class Dialog : public QDialog
{
    Q_OBJECT

public:
    explicit Dialog(QWidget *parent = 0);
    ~Dialog();

private slots:
    void onWorkerThread();
    void on_ActivateButton_clicked();
    void on_DisactivateButton_clicked();

private:
    Ui::Dialog *ui;
    bool runFlag;
};

#endif // DIALOG_H

//main.cpp
#include "dialog.h"
#include <QApplication>

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    Dialog w;
    w.show();

    return a.exec();
}

The GUI is