Qt MainWindow is not updating in multithreaded application

peterus07

I am using Qt to generate a Window. Additionally I use libnfc to get access to a nfc reader, so far so good. In my self written nfc-class i generate a new thread, this thread is polling for new tags on the reader. If there is a new tag, the thread will start a signal event for the MainWindow. In the main window I have just a QWebView which will show different websites on different states (after start, new tag, tag removed), just realy basic stuff.

My problem is now: that the main window (or the QWebView) is not updating. If i switch to another programm and go back to my app, the window will be updated. I was already searching with google and trying different stuff but nothing helps.

Here the thread code:
@class NFC_Thread : public QThread
{
Q_OBJECT
public:
NFC_Thread(NFC_Reader * Reader);
void run();

signals:
void NewTarget(nfc_target Target);
void TargetRemoved(nfc_target Target);

private:
int mError;
bool mStopPolling;
};

void NFC_Thread::run()
{
mError = 0;
mStopPolling = false;
while(!mStopPolling)
{
nfc_target Target;
mError = nfc_initiator_poll_target(mReader->GetDevice(), nmModulations, szModulations, mPollNr, mPollPeriod, &Target);
if(mError > 0)
{
cout << "NFC: found target" << endl;
}
#warning Bug in driver: Timeout generate a NFC_EIO Error, 'https://code.google.com/p/libnfc/issues/detail?id=224'
else if(mError > 0)
{
cout << "NFC: Error" << endl;
mStopPolling = true;
}
else
{
cout << "NFC: no target found" << endl;
}
}
}
@

MainWindow Code:
@class MainWindow : public QMainWindow
{
Q_OBJECT

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

public slots:
void SetNewTarget(nfc_target Target);
void doTargetRemoved(nfc_target Target);

private:
bool event(QEvent *event);
void resizeEvent(QResizeEvent *);
void adjust();

Ui::MainWindow *ui;
QWebView * mWebView;

};

MainWindow::MainWindow(QWidget *parent) :
QMainWindow(parent),
ui(new Ui::MainWindow)
{
ui->setupUi(this);
mWebView = new QWebView(this);
mWebView->load(QUrl("http://www.pbuchegger.at/"));
mWebView->show();
}

void MainWindow::SetNewTarget(nfc_target Target)
{
QString str = "NEW TARGET: \n";
{
char * s;
str_nfc_target(&s, Target, false);
str += s;
delete s;
}
//cout << "NFC: Target: " << str << endl;
mWebView->load(QUrl("http://www.google.at"));
update();
repaint();
mWebView->update();
qApp->processEvents();
/QMessageBox msgBox;
msgBox.setText(str);
msgBox.exec();/
}

void MainWindow::doTargetRemoved(nfc_target Target)
{
QString str = "TARGET REMOVED: \n";
{
char * s;
str_nfc_target(&s, Target, false);
str += s;
delete s;
}
//cout << "NFC: Target: " << str << endl;
mWebView->load(QUrl("http://www.cde.at"));
update();
repaint();
mWebView->update();
qApp->processEvents();
/QMessageBox msgBox;
msgBox.setText(str);
msgBox.exec();/
}

bool MainWindow::event(QEvent *event)
{
if(event->type() == QEvent::Resize)
{
adjust();
return true;
}
return false;
}

void MainWindow::resizeEvent(QResizeEvent *)
{
adjust();
}

void MainWindow::adjust()
{
mWebView->setGeometry(0, 0, ui->centralWidget->geometry().width(), ui->centralWidget->geometry().height());
}
@

main code:

@int main(int argc, char *argv[])
{
QApplication a(argc, argv);
qRegisterMetaType<nfc_target>("nfc_target");

MainWindow w;
w.setWindowState(Qt::WindowMaximized);

NFC_Reader Reader;
nfc_device_string devs;
size_t nr;
QString str = "";

Reader.GetDevices(devs, nr);
if(nr > 0)
{
    if(!Reader.InitReader(NULL))
    {
        str += "Error on init!";
    }
    else
    {
        Reader.Start_Polling();
        str += "Started Polling!";
    }
}
else
{
    str += "No Device found!";
}
w.SetText(str);

SignalHelper Helper;

QObject::connect(Reader.GetThread(), SIGNAL(NewTarget(nfc_target)), &Helper, SLOT(doNewTarget(nfc_target)));
QObject::connect(Reader.GetThread(), SIGNAL(TargetRemoved(nfc_target)), &Helper, SLOT(doTargetRemoved(nfc_target)));
QObject::connect(&Helper, SIGNAL(NewTarget(nfc_target)), &w, SLOT(SetNewTarget(nfc_target)));
QObject::connect(&Helper, SIGNAL(TargetRemoved(nfc_target)), &w, SLOT(doTargetRemoved(nfc_target)));

w.show();
int ret = a.exec&#40;&#41;;
Reader.Abort_Polling();
return ret;

}
@

As u can see, I have a "Helper" class, this class is just getting the signal in a slot and starting again a signal which will be forward to the mainwindow. If i want to forward the signal directly to the mainwindow, nothing is happening (like the signal is not fired), but i was checking it with the Qt-About box, and the box is showing up.

Helper class:
@class SignalHelper : public QObject
{
Q_OBJECT
public slots:
void doNewTarget(nfc_target Target);
void doTargetRemoved(nfc_target Target);
signals:
void NewTarget(nfc_target Target);
void TargetRemoved(nfc_target Target);
};

void SignalHelper::doNewTarget(nfc_target Target)
{
emit NewTarget(Target);
}

void SignalHelper::doTargetRemoved(nfc_target Target)
{
emit TargetRemoved(Target);
}
@

no compiler errors or linker errors. this code shows just the important stuff, all the unimportant stuff is removed. just for your information the project file:
@QT += core gui testlib
QT += webkit

greaterThan(QT_MAJOR_VERSION, 4) {
QT += widgets
}

TARGET = NFC_GUI
TEMPLATE = app

SOURCES += main.cpp
mainwindow.cpp
nfc_thread.cpp
nfc_reader.cpp
signal_helper.cpp

HEADERS += mainwindow.h nfc_thread.h nfc_reader.h signal_helper.h

FORMS += mainwindow.ui

LIBS += -lnfc@

peterus07

got a tip:
I have to return the event handler from the upper classes:
@return QMainWindow::event(event);@

KA51O

There are a number of problems in your code, but in the end they can all be summed up by: "You're doing it wrong":http://blog.qt.digia.com/blog/2010/06/17/youre-doing-it-wrong/. I am assuming the code you posted is not everything, otherwise the signals defined in the QThread subclass would be obsolete. Your QThread subclass doesn't have a working event loop because you forgot to start it by calling exec() in the run() functions reimplementation. This results in the fact that your subclass cannot emit or receive signals. If this was fixed still all the signals emited by the QThread subclass are emitted in the context of the main thread. But in the end it all boils down to the advice to not subclass QThread but to use "the worker object approach":http://qt-project.org/wiki/QThreads_general_usage. Alternatively you could implement a subclass of "QRunnable":http://qt-project.org/doc/qt-4.8/qrunnable.html. This would be more similar to your current implementation but less error prone.

For more informations about multithreading in Qt you can have a look at these links:
"Thread basics":http://doc-snapshot.qt-project.org/5.0/qtcore/thread-basics.html
"Thread support in Qt":http://doc-snapshot.qt-project.org/5.0/qtcore/threads.html

Unfortunatly the above two links do still propagate the old subclassing approach. An attempt to correct this by a community member (JKSH see "link to discussion":http://qt-project.org/forums/viewthread/22205 ) didn't make it into the 5.0 version. You can read his draft "here":https://docs.google.com/file/d/0B71n5SNZeRJyNzZ6U0tVUUM5bUk/edit?pli=1.

peterus07

thanks for your detailed information! i was thinking qt threading is similar to other implementations styles so i was creating a subclass. I will take your advice and will rewrite the classes for the nfc part (i was also not very happy with this approach)

KA51O

Of course you can subclass QThread, but it has some pitfalls (which you unfortunatly all discovered). So using the worker object approach or another threading purpose class (e.g. QRunnable, QtConcurrent) can safe you a lot of headaches.

tzander

As KA510 said, you can subclass, but I think you'd be better off doing what the blog said.
Also a good read http://qt-project.org/wiki/QThreads_general_usage

andre

Actually, a thread without an event loop can send signals, just not receive them. And these signals are emitted in the context of the thread, not of the main thread.

The documentation has in fact been "modified":/doc/qt-5.0/qtcore/qthread.html, and has been "modified":https://codereview.qt-project.org/#change,45271 again afterwards. The worker approach is now the prime example in the docs, but the subclassing of QThread is also still there. For some use cases, it is just a better approach.

Note that there is another issue in the code: the stop flag won't work. The check for it will be optimized out. I'd suggest using a QAtomicInt for the flag, or at least make the bool volatile.

peterus07

thank you for your help guys!
I changed the class to a worker object with will be moved to the thread.

now i have another problem:
I have i QTimer which should come around 500ms (is not a big deal if it is a little bit later).
But with the new implementation i get no event from the timer anymore. I think the problem is, what Andre was writing, that the new thread has no event loop.

some code:

@NFC_Thread::NFC_Thread(NFC_Reader * Reader)
{
mReader = Reader;
mError = 0;
mStopPolling = false;

if(mpTimer != 0)
{
    delete mpTimer;
}
mpTimer = new QTimer;
QObject::connect(mpTimer, SIGNAL(timeout()), this, SLOT(ResetTargets()));
mpTimer->start(mTimerMS);

}

void NFC_Thread::doPolling()
{
mError = 0;
mStopPolling = false;

if(mpTimer != 0)
{
    delete mpTimer;
}
mpTimer = new QTimer;
QObject::connect(mpTimer, SIGNAL(timeout()), this, SLOT(ResetTargets()));
mpTimer->start(mTimerMS);

while(!mStopPolling)
{
nfc_target Target;
mError = nfc_initiator_poll_target(mReader->GetDevice(), nmModulations, szModulations, mPollNr, mPollPeriod, &Target);
if(mError > 0)
{
cout << "NFC: found target" << endl;
}
#warning Bug in driver: Timeout generate a NFC_EIO Error, 'https://code.google.com/p/libnfc/issues/detail?id=224'
else if(mError > 0)
{
cout << "NFC: Error" << endl;
mStopPolling = true;
}
else
{
cout << "NFC: no target found" << endl;
}
}
}

void NFC_Thread::ResetTargets()
{
for(QList<nfc_target>::iterator iter = mTargets.begin(); iter != mTargets.end(); iter++)
{
emit TargetRemoved(*iter);
}
mTargets.clear();
mpTimer->start(mTimerMS);
Debug("NFC: Reseting List with timeout!");
}@

I create the Thread object in a other class with this:
@
mpThread = new NFC_Thread(this);
mpRunningThread = new QThread;
mpThread->moveToThread(mpRunningThread);

connect(mpRunningThread, SIGNAL(started()), mpThread, SLOT(doPolling()));
connect(mpThread, SIGNAL(finished()), mpRunningThread, SLOT(quit()));
connect(mpThread, SIGNAL(finished()), mpThread, SLOT(deleteLater()));
connect(mpRunningThread, SIGNAL(finished()), mpRunningThread, SLOT(deleteLater()));

@

and start the thread with this:
@mpRunningThread->start();@

now the question is, where should i put the event loop...
the main problem is: i want to write some classes around a lib, to get support of signals and slots.

KA51O

By default a QThread already has its own eventloop. The default implementation of run() already calls exec(). Your problem is the busy loop :
@
while(!mStopPolling)
{
nfc_target Target;
.
.
.
}
@
while this is running your code does not return to the eventloop. So no signals are handled. Infinite loops or polling in loops is not a good idea in an event driven framework like Qt.

peterus07

Ok! But i have this libnfc, and i can not change it, so i want to write a wrapper around it (from poll to event driven).
what is the best option for me to do this?

tzander

don't loop forever, use a timer to poll every n ms instead. Returning quickly when there is nothing.

KA51O

How long do you estimate does this call need to return ?
@
mError = nfc_initiator_poll_target(mReader->GetDevice(), nmModulations, szModulations, mPollNr, mPollPeriod, &Target);
@

andre

Sometimes a tight loop is a good solution, but then you just have to make sure that you don't use any slots. If you're in a loop, and you need a timer-like solution, you could also just check a [[doc:QElapsedTimer]] in every iteration.

peterus07

The poll needs currently 5 sek to return (still a bug in the lib), so it is like really long. I think the best is, to wait until the bug is fixed, making the poll short as possible and i can change the howl algorithm to something better. the timer is just there because the poll needs so long. Example: If i am returning a tag from the reader and putting the tag again in the 5 sek, the lib does not recognizing that there is a "new" tag.

tzander

KA510; I don't know how long it takes to return from nfc_initiator_poll_target(), there is no way to know without measuring since I don't know that call.

I suggest using a timer and start it just before the call, and print 'elapsed()' directly after the call completed.

Make sure you do it in different situations and repeatedly to get a good idea how long it takes on average.

KA51O

[quote author="Thomas Zander" date="1360915643"]KA510; I don't know how long it takes to return from nfc_initiator_poll_target(), there is no way to know without measuring since I don't know that call.

I suggest using a timer and start it just before the call, and print 'elapsed()' directly after the call completed.

Make sure you do it in different situations and repeatedly to get a good idea how long it takes on average.[/quote]

I wasn't asking you but the original poster of course. Why would you think that question was directed to you?

On topic: If the call takes 5 seconds you can not expect a timer with a 500 ms timeout to work as expected ...
Instead of the loop just call the slot doProlling() at start of the thread (as you do now, but without the loop). When the nfc call returns emit a signal connected to a slot in the worker object, which then checks all your abort flags and such (you could also update your targets here) and then call the doPolling slot again if the abort flag is not true. This way your thread is still blocked while executing the nfc blocking call but atleast returns to the event loop right after that.

peterus07

this sounds good to me, i will give change it to this structure.
I will also change to another lib for the nfc part because the reader is not very well supported.