QThread: am I doing it wrong?

JulienMaille

I am currently driving a rotating stage (moving, homing) and would like my main thread not to be frozen while the stage is moving.
But I also want to have blocking calls that keep the GUI refreshed, so I am sure that when the slot returns, the stage is really at its final position.
And last point: I don't want to connect a signal each time I call stage->findHome() or stage->goTo(x) from the main thread.

So I followed "this example":http://stackoverflow.com/questions/3297456/invoke-slot-method-without-connection:
and then modified it a bit to obtain blocking calls:
@class Motor : public QObject
{
Q_OBJECT

protected:
QThread _thread;

public:
Motor();

public slots:
void findHome(bool sync);

protected slots:
bool findHomeImpl();

signals:
void finished();
};@

Motor.cpp:
@Motor::Motor()
{
moveToThread(&_thread);
_thread.start();
}

void Motor::findHome(bool sync)
{
if( sync )
{
// trick to block while still refreshing GUI
QEventLoop loop;
connect(this, SIGNAL(finished()), &loop, SLOT(quit()));
QMetaObject::invokeMethod(this, "findHomeImpl", Qt::QueuedConnection);
loop.exec(QEventLoop::AllEvents|QEventLoop::WaitForMoreEvents);

}
else //return immediately, caller can still connect to finished(&#41; if he'd like to
    QMetaObject::invokeMethod(this, "findHomeImpl", Qt::QueuedConnection&#41;;

}

void Motor::findHomeImpl()
{
doSomeBoringStuff();
emit finished();
}
@

Questions: is it stupid?
Could I do this in a simpler way?
Is there a way to easily handle return values from the called slot?

ckakman

Hi,

It is not for me to say whether you are doing it wrong or not. But let me make some remarks about your code:

First of all, you don't need to use QMetaObject::invokeMethod. Signal-slot connections are more than enough for cross-thread communication.

I would do the following:
You have code that should run before calling findHome(), and you, probably, have code that should run after the slot findHome() finishes (I guess you wouldn't want to block it otherwise).

I would

remove findHomeImpl()
create a signal the would trigger findHome() and connect this signal (default connection type Qt::AutoConnection is just fine)
put the code that I want to run after findHome() finishes into a slot in the object that emits the signal connected to findHome().
connect Motor::finished() to that slot (again Qt::AutoConnection is OK)

The fifth parameter to QObject::connect() is Qt::ConnectionType whose default value is Qt::AutoConnection, so you don't have to specify the fifth parameter explicitly.
I would create a new slot in the object living in the main thread that I would connect to Motor::finished() so that that code would run after the stage is at home.

As a side not, I don't think it is possible to both not block and block a thread at the same time. You either block or you don't. It is not possible to run two separate code block in the same thread at the same time.

JulienMaille

Two remarks:

I said I followed a trick found on stackoverflow in order to avoid connections with the use of QMetaObject::invokeMethod: then why your first suggestion is to do the exact opposite?
I said I don’t want to connect a signal for each asynchroneous call. My sample code shows the findHome() function. But imagine the same trick for goTo(x) and a loop calling goTo(x) 50 times. Do you suggest me to call goTo(x) and connect to a slot 50 times? Sounds unusable.
[quote author="ckakman" date="1323472861"]I don't think it is possible to both not block and block a thread at the same time. You either block or you don't.[/quote]Maybe I wasn't clear: I sometime want an asynchrone call to goTo(x). And I also want synchrone calls that do not block the GUI. Hence the QEventLoop trick.

andre

The main thing that strikes me in this code, is the way you try to move an object onto a thread that is itself member of the same object. That doesn't feel right to me.

Other than that, I guess it could work.

JulienMaille

I agree with you Andre, but it works (maybe for some bad reason).

Back to my questions, what about QtConcurrent::run()?
http://doc.qt.nokia.com/latest/qtconcurrentrun.html

Looks like it is exactly what I need.
Now the question is: how can I use waitForFinished() and still have GUI refreshed?

andre

What you could considder doing, is changing what you do in what thread.
If you want to keep the GUI updated, then, as you know, you need to keep the GUI eventloop spinning. However, it seems you are really insistent on using synchronous methods. So why don't you do all that processing from a separate thread? Then, you can do all the waiting for methods to finish you want, and be sure to never be in the way of the GUI thread. What I mean is, instead of making your Motor class be the complicated thing it is, why don't you let the process using the motor class run in a separate thread?

ckakman

Hi,

bq. 1. I said I followed a trick found on stackoverflow in order to avoid connections with the use of QMetaObject::invokeMethod: then why your first suggestion is to do the exact opposite?

First of all I didn't understand why you wan't to avoid signal-slot connections. Calling QMetaObject::invokeMethod() is essentially the same as emitting a signal.

I suggest the opposite of what is suggests in the stackoverflow post because you asked if there is a simpler way to do it. I just wanted to propose another way of doing the same task (at least I thought it would have the same result)

bq. 2. I said I don’t want to connect a signal for each asynchroneous call. My sample code shows the findHome() function. But imagine the same trick for goTo(x) and a loop calling goTo(x) 50 times. Do you suggest me to call goTo(x) and connect to a slot 50 times? Sounds unusable.

I think there is a little misunderstanding. You don't connect a signal to a slot 50 times if you emit that signal 50 times. You connect once and emit the signal where necessary.

As you haven't provided sample code showing how you use goTo(x) asynchronously and synchronously, I can't speculate on that. But I would never propose a solution where you have to make 50 signal-slot connections.

QObject:connect()'s fifth parameter is very flexible. You can take a look at that and maybe come up with a cleaner design.

bq. Maybe I wasn’t clear: I sometime want an asynchrone call to goTo(x). And I also want synchrone calls that do not block the GUI. Hence the QEventLoop trick.

Maybe you can redesign your code and avoid the need for synchronous calls, or you can follow Andre's suggestion and take everything out of the main thread if it is possible.

Sorry, more detailed responses are not possible for me without more code.

ckakman

[quote author="Julien M" date="1323557677"]Back to my questions, what about QtConcurrent::run()?
http://doc.qt.nokia.com/latest/qtconcurrentrun.html

Looks like it is exactly what I need.
Now the question is: how can I use waitForFinished() and still have GUI refreshed?[/quote]

Well, you would have to do the same QEventLoop trick in order not to block the GUI thread.

JulienMaille

ckakman: Thanks for your answers. Looks like I did not understood what your suggestion represents in terms of code. Maybe we should forget about the asynchrone call, I can live without it.
andre: Maybe your right and my problem is one level higher. So do you think I should move the following randomMotor() function in the Motor class?
Let's say I currently have this kind of code:
@
MainWindow::randomMotor()
{
for (int i=0; i<50; i++ )
{
int nextPos = rand();

    //Blocking call that runs in main thread and freeze GUI
    motor->goTo( nextPos );
    //Call to another class, value depends on motor's position
    //hence we need to make sure motor's movement is over
    int value = otherClass->getSomeInfo();
    pos[i] = nextPos;
    val[i] = value;
}

}
// Plus 10 other functions, where goTo(x) is just called once
// but this is the reason why I would avoid to add code in the functions
// that call goTo(x) and have the sync runs in goTo() itself.
MainWindow::moveMotorSlow() {}
MainWindow::moveMotorFast() {}
MainWindow::moveMotorAndRewind() {}
@

goetz

I would design it like the [[Doc:QNetworkAccessManager]] class: I'ts completely asynchronous. So make the motor class a kind of "worker" class with slots that start/stop movement and signals that indicate a finished movement etc. Put that into a "host thread" (like suggested in the "Threads, Events and QObjects":/wiki/Threads_Events_QObjects wiki article. Internally, in the worker object of the thread, you may block as you need - it would not affect the calling thread.

If you want to block the call in your main object, but want to keep the GUI responsive, you can run a local event loop. An example using QNAM looks like this:

@
QNetworkAccessManager *nam = new QNetworkAccessManager(this);
QNetworkRequest req(QUrl("http://download.qt.nokia.com/qt/source/qt-everywhere-opensource-src-4.7.4.zip"));
QNetworkReply *reply = nam->get(req);

QEventLoop *el = new QEventLoop(this);
connect(reply, SIGNAL(finished()), el, SLOT(quit()));
qDebug() << "run event loop";
// the local event loop stops flow of control here
// exec returns only if the event loop's quit slot has
// been called (in this case by signal finished of the reply)
el->exec&#40;&#41;;
qDebug() << "event loop finished";
delete reply;
delete nam;

@

While the download runs, the GUI is working and other slots in my main GUI are called. If you want to disable parts of your gui, you will need to call setEnabled(false) on the respective controls. And watch out that you can enter the code controlling the motor multiple times!

JulienMaille

Volker: the problem of the QNetworkAccessManager class is I don't really want an asynchronous class, I want a synchronous but run in a thread class. And would like to avoid the "QEventLoop trick" each time I call asynchronous functions (or hide this in the class itself)

goetz

As long as you have a blocking call in your GUI thread, it blocks. Period. Once it blocks, the event loop is stalled and your GUI is not responsive. Either make the call to the worker thread asynchronous (i.e. returning immediatley) and fake the block in the GUI by introducing an event loop or live with the situation. You can't have your cake and eat it too.

JulienMaille

That is why I want to block in another thread, just like in the code from my first post.

goetz

You don't know what you want - or at least you're unable to express it in a manner that others understand it. If I may summarize:

you want to block in your worker thread. That's fine, that's one of the things what they are for
you want your GUI to be responsive. That's fine too, that's what your users want
you want to hold the flow of control in your GUI, because you're waiting your worker to finish its payload. That's not a bad idea
you want to combine everything and get it for free. Here we have a problem

You have a solution of your own, we gave you hints for alternative implementations. Pick your choice.

JulienMaille

Ok maybe I failed in trying to express what I was looking for. Sorry about that, I tried my best but it wasn't enough. Thanks for your patience guys.