I am trying to have a minimal GUI starting an endless process that communicates over a custom protocol over a CAN bus.
Based on what I have read here, I structured my code as follows:
I have on one hand a class handling my GUI with 2 simple push buttons "start" and "stop", namely MainWindow.
And on the other hand, a class that manages my custom protocol with a state machine as described in the link above, namely Worker.
In the middle of these, I have a controller that links the whole together. This controller is here because some other tasks are handled but this is not the purpose of this post.
I have connected my buttons signals (released()) to signals from the Controller. So the GUI does not know what exactly is starting.
Those Controller's signals are connected to slots from the Worker. These slots are there to start and stop the process.
The Worker instance lives in its own QThread. Other tasks may be involved so I figured out it would be better to handle each one in its own thread.
One started, the process of the worker is handled through signals/slots that make the state machine evolved across the states regarding the transitions. Because of the signal/slot mechanism, the thread's event loop can process events from its queue, if I'm correct.
My starting signal is correctly sent to the worker, starting the process and therefore the state machine. This machine is cyclic until the stop signal is requested by the user. But, when the user clicks on the button "stop", the slot associated is not called. Meanwhile, the machine continues to run endlessly and does not see the stop request (I have put some debug messages to see what was really executed).
Here are code snippets. MainWindow.h
#ifndef MAINWINDOW_H
#define MAINWINDOW_H
#include <QMainWindow>
#include "controller.h"
class QPushButton;
class QWidget;
class QVBoxLayout;
class MainWindow : public QMainWindow
{
Q_OBJECT
public:
explicit MainWindow(Controller& controller, QWidget *parent = 0);
~MainWindow();
private:
Controller& controller;
QPushButton* startButton;
QPushButton* stopButton;
QWidget* centralWidget;
QVBoxLayout* layout;
};
#endif // MAINWINDOW_H
MainWindow.cpp
#include "mainwindow.h"
#include <QWidget>
#include <QVBoxLayout>
#include <QPushButton>
MainWindow::MainWindow(Controller &controller, QWidget *parent) :
QMainWindow(parent), controller(controller)
{
centralWidget = new QWidget(this);
setCentralWidget(centralWidget);
layout = new QVBoxLayout();
startButton = new QPushButton("START", this);
stopButton = new QPushButton("STOP", this);
layout->addWidget(startButton);
layout->addWidget(stopButton);
centralWidget->setLayout(layout);
connect(startButton, SIGNAL(released()), &controller, SIGNAL(startSignal()));
connect(stopButton, SIGNAL(released()), &controller, SIGNAL(stopSignal()));
}
MainWindow::~MainWindow()
{
delete stopButton;
delete startButton;
delete layout;
delete centralWidget;
}
Controller.h
#ifndef CONTROLLER_H
#define CONTROLLER_H
#include <QObject>
#include <QThread>
class MainWindow;
class Worker;
class Controller : public QObject
{
Q_OBJECT
public:
Controller();
virtual ~Controller();
signals:
void startSignal() const;
void stopSignal() const;
private:
MainWindow* mainWindow;
QThread workerThread;
Worker* worker;
};
#endif // CONTROLLER_H
Controller.cpp (inherits public QObject)
#include "controller.h"
#include "mainwindow.h"
#include "worker.h"
Controller::Controller()
{
mainWindow = new MainWindow(*this);
mainWindow->show();
worker = new Worker();
worker->moveToThread(&workerThread);
connect(this, SIGNAL(startSignal()), worker, SLOT(startProcess()));
connect(this, SIGNAL(stopSignal()), worker, SLOT(stopProcess()));
workerThread.start();
}
Controller::~Controller()
{
workerThread.quit();
workerThread.wait();
delete worker;
delete mainWindow;
}
Worker handles a state machine with State and Transition which are enumerations.
Worker.h
#ifndef WORKER_H
#define WORKER_H
#include <QObject>
class Worker : public QObject
{
Q_OBJECT
public:
enum State { IDLE, STATE_1, STATE_2 };
enum Transition { OK, ERROR };
enum Mode { MODE_1, MODE_2 };
explicit Worker();
void read();
public slots:
void startProcess();
void stopProcess();
void processEvent(const Transition& transition);
signals:
void sendSignal(const Transition& transition) const;
private:
State currentState;
Mode selectedMode;
bool stopRequested;
};
#endif // WORKER_H
Worker.cpp (inherits public QObject)
#include "worker.h"
#include <QDebug>
#include <QThread>
Worker::Worker() : QObject()
{
stopRequested = false;
currentState = IDLE;
connect(this, SIGNAL(sendSignal(Transition)), this, SLOT(processEvent(Transition)));
}
void Worker::read()
{
qDebug() << "Reading...";
QThread::msleep(500);
emit sendSignal(OK);
}
void Worker::startProcess()
{
qDebug() << "Start requested";
selectedMode = MODE_1;
stopRequested = false;
emit sendSignal(OK);
}
void Worker::stopProcess()
{
qDebug() << "Stop requested";
stopRequested = true;
}
void Worker::processEvent(const Worker::Transition &transition)
{
qDebug() << "Process event";
switch(currentState) {
case IDLE:
switch(selectedMode) {
case MODE_1:
currentState = STATE_1;
read();
break;
case MODE_2:
currentState = STATE_2;
break;
}
break;
case STATE_1:
if (!stopRequested) {
if (transition == OK) {
read();
} else {
currentState = IDLE;
// No emission. The state machine stops on error
}
}
break;
case STATE_2:
// Not implemented yet
break;
}
}
.pro file
QT += core gui
greaterThan(QT_MAJOR_VERSION, 4): QT += widgets
TARGET = sample_project
TEMPLATE = app
DEFINES += QT_DEPRECATED_WARNINGS
SOURCES += main.cpp\
mainwindow.cpp \
controller.cpp \
worker.cpp
HEADERS += mainwindow.h \
controller.h \
worker.h
DISCLAIMER The code does not quit properly. Better launch it in your IDE so your can kill it easily.
These code snippets have been built with Qt5.8.0 MinGW 32bits. To reproduce the problem, just hit "start", debug messages appear in the console. Then hit "stop" and the messages keep coming and do not stop as they should.
I have found a workaround by calling directly stopProcess() from the Controller instead of using a signal. Doing so sets correctly stopRequested and stops the process.
Though, I was wondering why the event queue is never processing the signal from the Controller ? Even with the state machine handled with signal/slots, allowing the event queue to process events as they arrive.
(I have tried putting a intermediary slot in the Controller that sends the signal to the Worker to see if the GUI correctly sent the signal and this slot is indeed executed. But the stopProcess() slot remains uncalled.)
Any thoughts ?
As pointed out by Oktalist, the problem is that you are never returning to the Qt's event loop in your worker thread. By default Qt uses a Qt::AutoConnection, which is a Qt::DirectConnection if the receiver lives in the same thread. So, Qt calls processEvent recursively in an infinite way.
Solution 1: write/read stopRequested from both threads.
As you suggested, calling stopProcess from the Controller directly may solve your problem, but is not thread safe. You can define stopRequested as volatile, but this will only work on windows and will probably work in other situations.
A better approach is to define it as std::atomic if C++11 is an option for you.
Solution 2: avoid recursive function call
You can specify as fifth argument of QObject::connect which type of connection you want. Choosing a Qt::QueuedConnection will break your recursive action. In this way, Qt will be able to handle your stopRequested signal.
The advantage of this method is that all the thread safety concerns are handled transparently by Qt, but this will make your state machine somewhat slower.