QSerialPort and QThread

[maratk1n]

Hi all!

I have a small project:

mainwindow.cpp


MainWindow::MainWindow(QWidget *parent) :
QMainWindow(parent),
ui(new Ui::MainWindow)
{
ui->setupUi(this);

ComPort::get().open();

thread = new QThread(this);
connect(this, SIGNAL(destroyed(QObject*)), thread, SLOT(quit()));

valve = new Valve(7);
connect(valve, SIGNAL(remoteStatus(bool)), this, SLOT(remoteStatus(bool)));

valve->moveToThread(thread);


QTimer *valvesReadTimer = new QTimer(this);
connect(valvesReadTimer, SIGNAL(timeout()), valve, SLOT(getAllStates()));
valvesReadTimer->start(1000);

connect(passform, SIGNAL(manualModeEmit(bool)),
this, SLOT(manualMode(bool)));


emergency = new EmergencyResetOfPressure();
connect(emergency, SIGNAL(openValveSignal(int)), this, SLOT(openValve(int)));
connect(emergency, SIGNAL(closeValveSignal(int)), this, SLOT(closeValve(int)));
//emergency->start();
emergency->moveToThread(emergency);
emergency->start();
thread->start();

initActionConnection();

}

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

void MainWindow::valveSwitch(int id) //переключате� �ь клапанов
{
if (valve->getState(id))
closeValve(id);
else
openValve(id);
}

void MainWindow::openValve(int id)
{
QString str = "Клапан №" + QString::number(id+1);
valveButton[id]->setEnabled(false);
if (valve->open(id)) {
if (manualModeState)
valveButton[id]->setEnabled(true);
//valveButton[id]->setPalette(QPalette(Qt::green));
//valveButton[id]->setStyleSheet(VALVE_OPEN_COLOR);
QString style = QString(DEFAULT_STYLE_BUTTON) + QString(DEFAULT_BACKGROUND_BUTTON);
valveButton[id]->setStyleSheet(style);
ui->mainLabel->setText(str + " открыл! :)");
}
else {
if (manualModeState)
valveButton[id]->setEnabled(true);
ui->mainLabel->setText("Не могу открыть " + str);
remoteStatus(0);
}
}
void MainWindow::closeValve(int id)
{
QString str = "Клапан №" + QString::number(id+1);
valveButton[id]->setEnabled(false);
if (valve->close(id)) {
if (manualModeState)
valveButton[id]->setEnabled(true);
//valveButton[id]->setPalette(style()->standardPalette());
valveButton[id]->setStyleSheet("");
ui->mainLabel->setText(str + " закрыл! :)");
}
else {
if (manualModeState)
valveButton[id]->setEnabled(true);
ui->mainLabel->setText("Не могу закрыть " + str);
remoteStatus(0);
}
}

void MainWindow::pressureDrop() //Испытание по методу "Спад давления"
{
emergency->begin();

ui->mainLabel->setText("Испытание по методу \n Спад давления");
}


void MainWindow::initActionConnection()
{
//обработка нажатий на кнопки клапанов
QSignalMapper* signalMapper = new QSignalMapper (this); //чтобы можно было обработать ф-ю с аргументом в Слоте
for(int i = 0; i < 7; i++)
signalMapper->setMapping(valveButton[i], i);
for(int i = 0; i < 7; i++)
connect(valveButton[i], SIGNAL(clicked()), signalMapper, SLOT(map()));
connect(signalMapper, SIGNAL(mapped(int)), this, SLOT(valveSwitch(int)));


connect(ui->pressureTestButton, SIGNAL(clicked(bool)), this, SLOT(pressureDrop())); //опрессовка и испытание на прочность

}


EmergencyResetOfPressure::EmergencyResetOfPressure (QObject *parent) : QThread(parent)
{

}

EmergencyResetOfPressure::~EmergencyResetOfPressur e()
{

}

void EmergencyResetOfPressure::begin()
{
for (int i = 0; i<7; i++)
{
//sleep(1);
emit openValveSignal(i);
}
for (int i = 0; i<7; i++)
{
//sleep(1);
emit closeValveSignal(i);
}
}



File for working with valves and port (singleton class)


class ComPort : public QObject { //класс синглтон
Q_OBJECT
private:
QString portName;
QSerialPort *serial;
explicit ComPort(QObject *parent = 0);
~ComPort();

//защита от копирования
ComPort(ComPort const&) = delete;
ComPort& operator= (ComPort const&) = delete;

int timeoutCount = 0;
int responseCount = 0;

public:
QByteArray buffer;
static ComPort& get()
{
static ComPort instance;
return instance;
}

void open();
void close();
QByteArray requestResponse(const QByteArray &data);
void write(const QByteArray &data);
bool crcCheck(const QByteArray &data);
private slots:
void readData();
void handleError(QSerialPort::SerialPortError error);
};



Valve::Valve(int size, QObject *parent) : QObject(parent) //конструктор
{
valveState.resize(size);
for(int i = 0; i < size; i++)
{
valveState[i] = false;
}
}

Valve::~Valve() //деструктор
{

}

bool Valve::open(int id)
{
QByteArray arr;
arr.resize(7);
arr[0] = 0xAB;
arr[1] = 0x01;
arr[2] = 0x02;
arr[3] = 0x02;
arr[4] = id+1;
arr[5] = 0xFF;
arr[6] = 0x00 - arr[1] - arr[2] - arr[3] - arr[4] - arr[5];

QByteArray response = ComPort::get().requestResponse(arr);
if(response[0] == arr[0])
{
qDebug() << "клапан №: " << id+1 << " открыт!";
valveState[id] = true;

emit remoteStatus(1);
return 1;
}

emit remoteStatus(0);
return 0;
}

bool Valve::close(int id)
{
QByteArray arr;
arr.resize(7);
arr[0] = 0xAB;
arr[1] = 0x01;
arr[2] = 0x02;
arr[3] = 0x02;
arr[4] = id+1;
arr[5] = 0x00;
arr[6] = 0x00 - arr[1] - arr[2] - arr[3] - arr[4] - arr[5];

QByteArray response = ComPort::get().requestResponse(arr);
if(response[0] == arr[0])
{
qDebug() << "клапан №: " << id+1 << " закрыт!";
valveState[id] = false;

emit remoteStatus(1);
return 1;
}

emit remoteStatus(0);
return 0;
}

/*****************************************
* Класс для работы с COM портом
* **************************************/

ComPort::ComPort(QObject *parent) : QObject(parent)
{
buffer = "";
serial = new QSerialPort();
connect(serial, SIGNAL(error(QSerialPort::SerialPortError)), this, SLOT(handleError(QSerialPort::SerialPortError)));
}
ComPort::~ComPort()
{

}

void ComPort::open()
{
if(serial->isOpen())
close();
if(portName != Config::get().getValue("COM/name").toString())
{
qDebug() << "Порт " << portName << "сменился на " << Config::get().getValue("COM/name").toString();
portName = Config::get().getValue("COM/name").toString();
}
serial->setPortName(portName);
if (serial->open(QIODevice::ReadWrite)) {
if (serial->setBaudRate(QSerialPort::Baud115200)
&& serial->setFlowControl(QSerialPort::NoFlowControl)) {

qDebug() << "Порт открыт";

} else {
//QMessageBox::critical(this, "Error", serial->errorString());
qDebug() << QString(serial->errorString());
serial->close();

}
} else {
//QMessageBox::critical(this, QObject::tr("Error"), serial->errorString());

}
}
QByteArray ComPort::requestResponse(const QByteArray &data)
{
QByteArray readBuf;
qDebug() << "-------------------------";
int attempts = 1;
while (attempts <= REQATTEMPTS) { //3 попытки
if (serial->isWritable())
{
serial->write(data);
qDebug() << "Попытка № " << attempts;
qDebug() << "Запрос: " << data.toHex();
while (serial->waitForReadyRead(WAITFORREADY)) {
readBuf += serial->readAll();
if (crcCheck(readBuf) && data[2] == readBuf[2] ){ //если CRC и команда сошлись -- успех!
qDebug() << "Ответ: " << readBuf.toHex();
responseCount++;
qDebug() << "Кол-во запросов: " << responseCount;
qDebug() << "Кол-во таймаутов: " << timeoutCount;
float percent = timeoutCount * 100;
percent = percent / responseCount;
qDebug() << "Процент косяков: " << QString::number(percent, 'f', 3) << "%";
close();
open();
return readBuf;
}
}
readBuf.clear();
qDebug() << "Таймаут...";
timeoutCount++;
close();
open();
attempts++;
}
else
{
qDebug() << "Порт " << portName << " не пишется!";
return 0;
}

}
// close();
// open();
return 0;
}

void ComPort::close()
{
serial->close();
qDebug() << "Порт закрыт";
}
void ComPort::write(const QByteArray &data)
{
serial->write(data);
qDebug() << "Запрос: " << data.toHex();
}
void ComPort::readData()
{
buffer = serial->readAll();
if (ComPort::get().crcCheck(buffer))
{
qDebug() << "Ответ: " << buffer.toHex();
qDebug() << "---------------------------";
}
}
void ComPort::handleError(QSerialPort::SerialPortError error)
{
if (error == QSerialPort::ResourceError) {
ComPort::get().close();
qDebug() << "что-то не так!";
}
}





While working, I get an error:

QObject: Cannot create children for a parent that is in a different thread.
(Parent is QSerialPort(0x197b7f8), parent's thread is QThread(0xc16e50), current thread is QThread(0x197c620)

How can I get rid of this error?
I hope for any help. Thanks!

[maratk1n]

getAllStates method:


bool Valve::getAllStates()
{
QByteArray arr;
arr.resize(5);
arr[0] = 0xAB;
arr[1] = 0x01;
arr[2] = 0x00;
arr[3] = 0x00;
arr[4] = 0x00 - arr[1] - arr[2] - arr[3];

QByteArray response = ComPort::get().requestResponse(arr);
if(response[0] == arr[0])
{
QBitArray bitStates(16); //8 на клапана, 8 на фиттинги

for (int i = 4; i<6; i++) // конвертируе� � 4й и 5й байты (HEX) в биты (BIN)
for (int b = 0; b<8; b++)
bitStates.setBit((i-4)*8+b, response.at(i)&(1<<b));

//qDebug() << bitStates;

for (int i = 0; i < valveState.size(); i++) //обновляем состояния клапанов
valveState[i] = bitStates[i];
for (uint i = 0; i < sizeof(fittingState); i++) //обновляем состояния фиттингов
fittingState[i] = bitStates[i+8];

emit remoteStatus(1);
return 1;
}

emit remoteStatus(0);
return 0;
}

[high_flyer]

are you allocating objects in your ComPort class?

[maratk1n]

are you allocating objects in your ComPort class?

Only this, if you about this ...

serial = new QSerialPort();

ComPort::ComPort(QObject *parent) : QObject(parent)
{
buffer = "";
serial = new QSerialPort();
connect(serial, SIGNAL(error(QSerialPort::SerialPortError)), this, SLOT(handleError(QSerialPort::SerialPortError)));
}

[d_stranz]

ComPort is statically allocated on the stack in your main thread, and the QSerialPort instance it allocates is also in the main thread. You are using it from the Valve thread, so probably in the internals of QSerialPort it is trying to allocate an instance of some QObject-based class for its use while reading. As the error message says, objects created in one thread can't allocate children in a different thread. Time for some re-design.

[maratk1n]

ComPort is statically allocated on the stack in your main thread, and the QSerialPort instance it allocates is also in the main thread. You are using it from the Valve thread, so probably in the internals of QSerialPort it is trying to allocate an instance of some QObject-based class for its use while reading. As the error message says, objects created in one thread can't allocate children in a different thread. Time for some re-design.

Thanks for the answer!
How can I create a single instance of a class to access it from different parts of the program? Not using a singleton ..

[d_stranz]

Not using a singleton ..

Well, as you have implemented it, ComPort -is- a Singleton. The problem is not the definition of the class as a single instance, it's that the variable contained within the class (QSerialPort) has to further allocate memory in order to do its business. Objects living in one thread can't allocate memory in another thread and that is what is happening in when you try to use your ComPort instance in the Valve thread..

So I would take a step back and examine why you think you need a single-instance class in this case. Can't each thread have its own instance? If not, how could you use a single instance class along with signals and slots (which can communicate across threads) to accomplish the same purpose.

[maratk1n]

Well, as you have implemented it, ComPort -is- a Singleton. The problem is not the definition of the class as a single instance, it's that the variable contained within the class (QSerialPort) has to further allocate memory in order to do its business. Objects living in one thread can't allocate memory in another thread and that is what is happening in when you try to use your ComPort instance in the Valve thread..

So I would take a step back and examine why you think you need a single-instance class in this case. Can't each thread have its own instance? If not, how could you use a single instance class along with signals and slots (which can communicate across threads) to accomplish the same purpose.

I can make a regular class. But if I create several instances, I will create several instances of the port (QSerialPort). And this is not very good, as it seems to me

[high_flyer]

You could also make your singleton allocate on the heap by changing the get() method:


static ComPort* get()
{
static ComPort *instance = nullptr;
if(!instance) {
instance = new ComPort();
}
return instance;
}


That wont solve any of your problems though.
I don't think the problem is the fact you are allocating on the stack, just the fact they are not allocated in the same thread.
You should move your ComPort instance to the thread where Valve is as well (for example).

A side note:
Your code is very thread unsafe.
You have no protection against multiple threads creating race conditions on resources in your classes, this could lead to all kinds of weird behavior which is very hard to debug.
Writing thread safe code needs very good planning and somewhat different thinking than non threaded code.
moveToThread() should be done on objects that are as "specific" or as "closed" as possible, and if you need things allocated, it short be when needed and for as short time as possible.
You should analyze what it is you really need done in another thread, and try to pack that in to an object of its own, without the whole machinery around it which is not needed during the work in the other thread.
Or even better, ask your self if a threaded configuration is really necessary in your case, or if there are other, easier to maintain ways to do it.

And as d_stranz said, globlas and singletons *usually* tend to point to a design error.
Singletons and globals should be avoided as much as possible for many very strong and important reasons.

[maratk1n]

You could also make your singleton allocate on the heap by changing the get() method:


static ComPort& get()
{
static ComPort *instance = nullptr;
if(!instance) {
instance = new ComPort();
}
return instance;
}



Error:


D:\WorkWolder\Libraries\RSProtocol_COMport.h:91: ошибка: invalid initialization of reference of type 'ComPort&' from expression of type 'ComPort*'
return instance;
^

[d_stranz]

Try to think about the code you are writing, don't just blindly copy it from the forum. high_flyer's code had a small typo. It should be "return *instance;", and you should know enough C++ to see that for yourself.

[maratk1n]

Try to think about the code you are writing, don't just blindly copy it from the forum. high_flyer's code had a small typo. It should be "return *instance;", and you should know enough C++ to see that for yourself.

You are absolutely right... Thanks.
Recently, I'm not myself, my head is spinning.

This did not solve the problem. Apparently, it is necessary to do through signals and slots.

P.S. I made a singleton to access a single instance of the port from anywhere in the program. I do not know how else to solve this problem.

[high_flyer]

high_flyer's code had a small typo.
Sorry about that. (can happen when you write code here and can't check with a compiler)
The actual mistake i did was I forgot to change the return reference to a pointer, which I now edited.
The solution d_stranz offered will work too of course, take your pick.


This did not solve the problem.
I told you it wont, I just showed how the allocation on stack vs heap can be solved, its not your problem.
The problem as d_stranz pointed out is the fact that the QSerialPort() is allocated in the main thread and in turn it allocates stuff further while parenting, and this allocation is taking place in your valve thread causing the serialport to be a parent from a different thread to the object it allocates.


P.S. I made a singleton to access a single instance of the port from anywhere in the program. I do not know how else to solve this problem.
That is not what singletons are for.
Singletons are to FORCE only one instance of an object in the system, or forbid multiple instance of that object.
Just accessing the same instance does not require a singleton.
Simply create the object and make it available where you need it.

As I already suggested in my previous post, one way you maybe be able to get rid of the error is to move the ComPort object to the thread that uses it.
This wont help you however if you are using it from more than one thread.
Alternatively you could fully initialize it before you make it available to the worker threads - please note, that I don't know if any operations you call on the serialport might cause it to allocate stuff again, in which case this wont help you either.

It also looks strange that on one hand you have a singleton - and on the other hand you want to use it in multiple threads.
This by definition will lead to procedural behavior (well in your case it will lead to undefined behavior since you are not mutexing anything) so the advantage of using threads is fully lost.

I would dare to say that you simply don't need threads in this case.