QSerialPort::waitForReadyRead exits with timeout

Bengt 0

@J-Hilk said in QSerialPort::waitForReadyRead exits with timeout:

Thanks a lot, I will study it.

#include <QCoreApplication>

#include <QSerialPort>
#include <QDebug>

QSerialPort serial;
int sendIndex(-1);

static const QVector<QByteArray> toSend{
{"Command1"},
{"Command2"},
{"Command3"},
};

void writeNextCommand(){
sendIndex++;
if(sendIndex < toSend.size()){
serial.write(toSend.at(sendIndex));
} else {
qDebug() << "All Send";
}
}

void onReadyRead(){
QByteArray ba = serial.readAll();
//Your own logic, that ba is complete and correct

//--
qDebug() << "onReadyRead: new bytes:" << ba.toHex(' ');

writeNextCommand();
}

void onBytesWritten(qint64 bytesWritten){
if( bytesWritten == toSend.at(sendIndex).size())
qDebug() << "All bytes of the command send, expecting now answer";
else
qDebug() << bytesWritten << " of" << toSend.at(sendIndex).size();
}

int main(int argc, char *argv[])
{
QCoreApplication a(argc, argv);
//Setup Port
serial.setPortName("PortX");

QObject::connect(&serial, &QSerialPort::bytesWritten,&onBytesWritten);
QObject::connect(&serial, &QSerialPort::readyRead, &onReadyRead);
/*
serial.setParity();
serial.setBaudRate();
serial.setDataBits();
serial.setStopBits();
serial.setFlowControl();
*/
if(serial.open(QIODevice::ReadWrite)){
writeNextCommand();
} else {
qDebug() << "Could not open Serialport";
}
return a.exec();
}

Bengt 0

An observation: if I do NOT connect the readyRead signal to my PortRead, waitForReadyRead() works correctly and does not timeout. But then of course PortRead is never executed...
Interesting.

Bengt 0

@Bengt-0
Even more interesting;
If I remove the ReadLine() from the connected PortRead slot, waitForReadyRead exits normally without timeout. Seems there is some kind of a race here.

KroMignon

@Bengt-0 said in QSerialPort::waitForReadyRead exits with timeout:

Even more interesting;
If I remove the ReadLine() from the connected PortRead slot, waitForReadyRead exits normally without timeout. Seems there is some kind of a race here.

Do you have read QSerialPort documentation?
This is what @J-Hilk warn you about, in his first post: do not mix synchronous (waitForReadyRead() / waitForBytesWritten()) and asynchronous (readyRead() / bytesWritten()) modes

Bengt 0

@KroMignon
Yes, I have read the QSerialPort documentation, at length.
Please point out exactly to the text section you refer to, I would be most grateful. Reading is not the same as understanding, unfortunately.

Bengt 0

@KroMignon

I realise now that my source of information was Assistant only, not your link to QSerialPort docs.
Assistant is convenient but much less complete.
I stand corrected and hope to solve it now.

Bengt 0

@KroMignon
I tried to use the blocking code example on the documentation page exactly, and I got a timeout of 30 seconds as expected, since it is breaking only when bytes==0 and waitFRR==false. This was a surprise since the text indicates that waitFRR==false only in a broken connection or an error, which should be an exceptional case. Or do I misunderstand the text?

In my application, this code works only if I set the timeout value to a reasonable value other than the default 30 seconds. It seems I really need a waitFRR=false to be sure there is no more data in the pipe. If not, I do not always get the last data, this is tested and confirmed.
Unfortunately, I need to turn off my error check in this procedure, since waitFRR=false gives error() = 12.
Is there a better solution to make sure I do not miss data?

    for(;;) {
        numread = read(buf,128);
        numreadTotal +=numread;
        if(numread) strcat(Buffer,buf);
        if(numread == 0 && !waitForReadyRead(10)) break;
    }
    strcpy(str,Buffer);

KroMignon

@Bengt-0 said in QSerialPort::waitForReadyRead exits with timeout:

Is there a better solution to make sure I do not miss data?

If you want to work in polling mode, I would do it like this:

QElapsedTimer tmr; // to exit when too long

tmr.start();
QByteArray reply;
// I suppose you are waiting to have 128 bytes?
while(!tmr.hasExpired(30000) && reply.length() < 128)
{
    serial.waitForReadyRead(100);
    while(serial.bytesAvailable())
    {
         reply.append(serial.readall());
    }
}

JonB

@Bengt-0
You may or may not thank me for these observations.

As others have said, if we were you we just would not use the blocking waitForReadyRead(), we would use the signals/slots instead. Period. [EDIT: Though I see @KroMignon has just posted something using the blocking call, leave it to you.]
if(numread) strcat(Buffer,buf);: Probably not related to your issue, but I just wouldn't use this. You are making assumptions of what data will be in buf after read(buf,128) which do not seem to be warranted. This line can potentially put rubbish into your Buffer, or even crash.

Bengt 0

@JonB
That you for your suggestions.
I will try to get the polling version working first, then I will work on the signal/slot version.
However, I obviously lack in-depth understanding of the subject of signals and slots in this case.
My application is based on a timer that sends requests to my HW box periodically every 250ms.
I assume this can be regarded as a blocking mode operation.
If I receive the answer from my HW box by the readyRead signal, can this be regarded as "mixing blocking and non-blocking mode" which is not advisable or recommended?

About the last remark on buffer content, it is of course true.
However, I trust my HW box, and the things that could happen at worst would be: empty, one CR-terminated string data package (which is the correct scenario), or two or more CR-terminated string data packages. I should at least check for overflow, agreed.

Bengt 0

@KroMignon

for the moment, this solution seems to be working:

    for(;;) {
        numread = read(buf,128);
        numreadTotal +=numread;
        if(numread) strcat(Buffer,buf);
        if(numread == 0 && !waitForReadyRead(10)) break;
        if(atEnd()) break;
    }
    strcpy(str,Buffer);

No timeout with error() = 12.
I will try your suggestion, it is simpler.

J.Hilk

@Bengt-0 can you post your whole class?

If I have the time, I'll rework it into Signal&Slot

KroMignon

@Bengt-0 said in QSerialPort::waitForReadyRead exits with timeout:

No timeout with error() = 12.
I will try your suggestion, it is simpler.

It is very difficult for me to understand what exactly is your issue. So I probably not give you the answer you are waiting for.
Just to summarize:

You can work in polled/synchronous mode: no need of QEventLoop but it should not be done on main thread in graphical application, because the EventLoop of this thread will be "looked" during the process.
You can work in asynchronous. In this case an QEventLoop is mandatory
You should not mix them

If you want the polling mode, it is better to clearly define an "ending" condition. There are many things that could happen during transmission and locking the QEventLoop for too long time is not good.

For example you can define

the maximum time to wait for reply
the reply size
the maximum delay between message bytes.

QElapsedTimer is a good way for time measurement in polling mode, in my eyes.
So this could be a good starting point (up to you to adapt it to your requirements):

QElapsedTimer exitTmr; // to exit when too long
QElapsedTimer intervalTmr; // to exit when reception ends

exitTmr.start();
intervalTmr.invalidate();
QByteArray reply;

// Wait up to 30 seconds or 128 byte received or more than 300ms elapsed since last reception
while(!exitTmr.hasExpired(30000) && reply.length() < 128 && !(intervalTmr.isValid() && intervalTmr.hasExpired(300)))
{
    serial.waitForReadyRead(100);
    while(serial.bytesAvailable())
    {
        reply.append(serial.readall());
        intervalTmr.start();
    }
}

Bengt 0

Let me explain the application briefly, to clarify the issue.

The purpose is to measure and process positions of objects using a microscope and a X-Y table with digital readout in a university lab environment. The application is expected to run 24/7 and be operated by several (>50) users with varying expertise and experience. The software has been functioning for about 15 years. Minor code updates and rebuilds have been done a few times per year to add features and to verify that it is still working with the ever changing Windows operating system.

About 6 months ago we experienced intermittent malfunction in the position acquisition, which could be corrected by restarting the software. Not all users would be able to see this and act accordingly. We suspected a correlation to the change of Qt version from 5.9.6 and the compiler from VS2013 to VS2019. Instead of regressing back to Qt 5.9.6 (no longer supported by Qt) and VS2013, I decided to go through the code and try to find the fault, "modernise" it and make it more robust to serial communication failures. The serial port code has not been changed for a long, long time.

One of the features I added was to pick up if a port->error() occurred and make this do a port->clear(). This is why I was reluctant to accept a waitFRR timeout as a repeatedly occurring event in the port read loop. This strategy is working satisfactory, the error and port clear() can be optionally logged.

Up till now the loop has been using blocking code both for port write and read. With a repetition period of 250ms, the impact on the GUI is very minor so I have no strong functional incentives to change to signal/slot mode.

The software has a QTimer based loop, every 250ms the timer event slot is calling a method in the derived QSerialPort class that sends a read request to the XY-stage controller, and reads the answer back. The answer is a string "1.234 5.678\r" which is then returned to the caller. This string is then parsed to make a QPointF object.
The timer can also be turned off, and a point can be acquired in "non-live" mode by pressing a "Log Position" button in the GUI. This has been shown to be problematic since more than one point can be present in the port buffer, so the result of the request may not be the position the XY-stage is currently in. Two or even three push button requests may be needed to get the true current (last) position. In "live mode" this is not a problem, since there will be 4 read requests per second, and the stage position is not moving when the point is being measured. In "non-live" mode it is very important that the sport buffer is empty and the very last position is used, and the previous points are scrapped. This is the reason for all the fuss about emptying the port buffer.

I am very thankful for all of your suggestions, I have learnt a lot. It has been a bit uphill, since the last time I worked with the serial port code was 5-10 years ago. Programming is not my profession nor my daily occupation, as you probably have understood.

Thanks for your patience.

Bengt 0

@J-Hilk
I would be happy to do so, just let me clean up a bit first...

Bengt 0

@J-Hilk

The calling method (every 250msec) , just to set the context:

PAMS_Point
PAMSApp::ReadPosition(bool init)
{
	PAMS_Point pos;
	PAMS_Point mark;
	PAMS_Point errmark(0,0,false);
	QString PosString;
        char str[512]="";
        int len = 0;

	SerialPort *port;
	
	port = fSettings->GetSerialPort();
	if(!port) return errmark;
	if(!port->Status()) return errmark;
        if(init) {port->ZeroSCD();}

        len = port->GetSCDPos(ALL,str);

       if(!len) return errmark;

        PosString = str;

	mark = ParseSCDString(PosString);
			
	return mark;	

}

serialport.cpp

/*
 * Serial port code adapted to SCD PROFILER instruction set.
*/

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "serialport.h"
#include "PAMS_Types.h"

SerialPort::SerialPort(QString portname)
{
	PortName = "";
    status = OpenComPort(portname);
    if(status) PortName = portname;
    lasterror = QSerialPort::NoError;
    reads = 0;
}

SerialPort::~SerialPort()
{
    if(status) close();
}

bool
SerialPort::OpenComPort(QString sPortText)
{
    bool success = false;
    setPortName(sPortText);
    setBaudRate(Baud9600);
    setFlowControl(QSerialPort::NoFlowControl);
    setParity(QSerialPort::NoParity);
    setDataBits(QSerialPort::Data8);
    setStopBits(QSerialPort::OneStop);
    success = open(QIODevice::ReadWrite);
    return success;

}

int
SerialPort::GetSCDPos(int axis,char *str)
{
    bool ok=true;
    int err=0;
    int len=0;
    QByteArray reply;

    char X_axis[10] = "?pos x\r";
    char Y_axis[10] = "?pos y\r";
    char Z_axis[10] = "?pos z\r";
    char A_axis[10] = "?pos\r";

    Buffer[0] = Buffer[1] = (char)NULL;

	switch(axis)
	{
    case XAXIS:	// X-Axis
			PortWrite(X_axis);
			break;
    case YAXIS:	// Y-Axis
			PortWrite(Y_axis);
			break;
    case ZAXIS:	// Z-Axis
			PortWrite(Z_axis);
			break;
    case ALL:	// All-Axes
			PortWrite(A_axis);
			break;
	default:return 0;
			break;
    }

    err = error();
    lasterror = err;

    ok = waitForBytesWritten(500);
    if(!ok){
        err = error();
        lasterror = err;
    }

    ok = waitForReadyRead(100);
    if(!ok){
        err = error();
        lasterror = err;
    }

    while(bytesAvailable())
    {
        reply.append(readAll());
    }
    
    strcpy_s(str,512,reply.data());
    len = (int)strlen(str);

    if (!lasterror) reads += 1;

    return len;
}

bool
SerialPort::PortClear(void)
{
    bool rc;
    rc = clear();
    clearError();
    lasterror = error();
    return rc;
}

void 
SerialPort::PortWrite(char *request)
{
    if(isOpen()) {
        write(request,strlen(request));
	}

}

void
SerialPort::ZeroSCD(void)
{
    char command[10]="!pos 0 0";

    PortClear();
	
    command[8] = 0x0d;
    command[9] = 0;
	
	PortWrite(command);
	
}

serialport.h

#ifndef _H_SerialPort
#define _H_SerialPort

#include <stdio.h>
#include <QtCore>
#include <QIODevice>
#include <QFile>
#include <QSerialPort>

enum AXIS {XAXIS,YAXIS,ZAXIS,ALL};

class SerialPort : public QSerialPort
{
	Q_OBJECT 
	
	public:
		 					SerialPort(QString inPortname);
		virtual				~SerialPort();

	bool status;


//========================================================================
// Platform specific routines
//========================================================================


public:
    ulong		lasterror, reads;
	bool		Status() { return status; };
    int 		GetSCDPos(int axis, char *PosString);
	QString		name() {return PortName;};
    void 		ZeroSCD(void);
	ulong		GetLastError() {return lasterror;};
    ulong		GetReads() {return reads;};
    bool        PortClear();
		
protected:
	bool		OpenComPort(QString sPortText);	
	
	QString		PortName;
	void 		PortWrite(char *);

};

#endif	// _H_SerialPort

J.Hilk

@Bengt-0
ok, here ya go:

#ifndef SERIALPORT_H
#define SERIALPORT_H

#include <stdio.h>
#include <QtCore>
#include <QIODevice>
#include <QFile>
#include <QSerialPort>

enum AXIS {XAXIS,YAXIS,ZAXIS,ALL};

class SerialPort : public QSerialPort
{
    Q_OBJECT

    public:
                            SerialPort(QString inPortname);
        virtual				~SerialPort();


//========================================================================
// Platform specific routines
//========================================================================

    enum OperationMode {
        NoOperation = 0,
        GetScdPos,
        WriteNoResponse
    }m_Mode{NoOperation};

    struct RequestQueue{
        AXIS axis;
        QByteArray data;
        RequestQueue(AXIS a, QByteArray b) : axis(a), data(b){}
        RequestQueue(AXIS a) : axis(a){data.reserve(512);}
    };

public:
    void requestSCDPos(AXIS axis);
    QString getResponseAsString(){return QString(getOldestResponse());}
    QByteArray getOldestResponse();
    QByteArray getLatestResponse();

    bool		Status() { return isOpen() && error() == QSerialPort::NoError; };

    QString		name() {return PortName;};
    void 		ZeroSCD(void);
    ulong		GetLastError() {return lasterror;};
    ulong		GetReads() {return reads;};
    bool        PortClear();

protected:
    bool		OpenComPort(QString sPortText);

    QString		PortName;
    void 		PortWrite(const char *);

signals:
    void responseReceived();

private slots:
    void requestNextData();
    void onSerialPortError(QSerialPort::SerialPortError error);
    void onReadyRead();
    void onBytesWritten(quint64 bytes);

private:
    ulong lasterror, reads;
    
    QQueue<RequestQueue> m_requestQueue;
    QQueue<RequestQueue> m_resultQueue;
    
    quint64 bytesToWrite = 0;

};

#endif	// _H_SerialPort

/*
 * Serial port code adapted to SCD PROFILER instruction set.
*/

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "serialport.h"
//#include "PAMS_Types.h"

//Console output for test cases
#include <QDebug>

SerialPort::SerialPort(QString portname)
{
    PortName = "";
    if(OpenComPort(portname)) 
        PortName = portname;
    lasterror = QSerialPort::NoError;
    reads = 0;

    //Sets up the connection from the base class QSerialPort to our own slot onSerialPortError
    connect(this, &QSerialPort::errorOccurred, this, &SerialPort::onSerialPortError);
    connect(this, &QSerialPort::readyRead, this, &SerialPort::onReadyRead);
    connect(this, &QSerialPort::bytesWritten, this, &SerialPort::onBytesWritten);
    
    //Automatically try to call next data package, when a response was succesfully extracted
    connect(this, &SerialPort::responseReceived, this, &SerialPort::requestNextData);
}

SerialPort::~SerialPort()
{
    if(isOpen())
        close();
}

bool SerialPort::OpenComPort(QString sPortText)
{
    bool success = false;
    setPortName(sPortText);
    setBaudRate(Baud9600);
    setFlowControl(QSerialPort::NoFlowControl);
    setParity(QSerialPort::NoParity);
    setDataBits(QSerialPort::Data8);
    setStopBits(QSerialPort::OneStop);
    success = open(QIODevice::ReadWrite);
    return success;

}

void SerialPort::requestSCDPos(AXIS axis)
{
    m_requestQueue.enqueue(RequestQueue(axis));
    
    if(m_Mode == NoOperation)
        requestNextData();
}

QByteArray SerialPort::getOldestResponse()
{
    if(m_resultQueue.isEmpty())
        return QByteArray();
    return  m_resultQueue.dequeue().data;
}

QByteArray SerialPort::getLatestResponse()
{
    //This kind of goes behind the idea of a queue, but it's here just in case
    if(m_resultQueue.isEmpty())
        return  QByteArray();
    return m_resultQueue.takeLast().data;
}

bool
SerialPort::PortClear(void)
{
    bool rc;
    rc = clear();
    clearError();
    lasterror = error();
    return rc;
}

void
SerialPort::PortWrite(const char *request)
{
    if(isOpen()) {
        write(request,strlen(request));
    }
    
}

void SerialPort::requestNextData()
{
    if(m_resultQueue.isEmpty()){
        m_Mode = NoOperation;
        return;
    } 
    m_Mode = GetScdPos;
    AXIS axis = m_requestQueue.first().axis;
    
    constexpr char X_axis[10] = "?pos x\r";
    constexpr char Y_axis[10] = "?pos y\r";
    constexpr char Z_axis[10] = "?pos z\r";
    constexpr char A_axis[10] = "?pos\r";


    switch(axis)
    {
    case XAXIS:	// X-Axis
            PortWrite(X_axis);
            break;
    case YAXIS:	// Y-Axis
            PortWrite(Y_axis);
            break;
    case ZAXIS:	// Z-Axis
            PortWrite(Z_axis);
            break;
    case ALL:	// All-Axes
            PortWrite(A_axis);
            break;
    }
}

//If the Serialport has an error, the signal that the SerialPort emits will call this function
void SerialPort::onSerialPortError(QSerialPort::SerialPortError error)
{
    lasterror = error;
    qDebug() << Q_FUNC_INFO << error;
}

void SerialPort::onReadyRead()
{
    if(bytesAvailable() < 512)
        //Not Enough bytes in the buffer, 512 should be small enough so that we don't have to empty the buffer on each readyRead signal
        //And we can let it be in the internal buffer
        return;
    
    RequestQueue queue = m_resultQueue.dequeue();
    quint64 read = readData(queue.data.data(), 512);
    
    if(read != 512)
        qDebug() << Q_FUNC_INFO << read << "of" << 512;
    
    m_resultQueue.enqueue(queue);
    reads++;
    emit responseReceived();
}

void SerialPort::onBytesWritten(quint64 bytes)
{
    if(m_Mode == WriteNoResponse) {
        bytesToWrite -= bytes;
        if(bytesToWrite == 0)
            m_Mode = NoOperation;
    }
}

//No Idea what this is supposed to do (If it will trigger a readyRead or not) I assume not and implemented the bytesWritten signal/slot logic for no response
void
SerialPort::ZeroSCD(void)
{
    char command[10]="!pos 0 0";

    PortClear();

    command[8] = 0x0d;
    command[9] = 0;

    bytesToWrite = 10;
    m_Mode = WriteNoResponse;
    
    PortWrite(command);

}

The idea here is you call the requestSCDPos(AXIS axis)function with your 250 ms second timer.

Once the serialport class has received all data for one response, the signal void responseReceived(); is emitted. In your other class you simply connect to that signal and request the oldest response with getOldestResponse or QString getResponseAsString

I implemented a queue system, to make the whole system more robust, just in case.

Bengt 0

@J-Hilk
Thanks a lot, this will be an exciting exercise!

Comments:
ZeroSCD is sending a command to zero the XY-pos (0.000 0.000) of the stage measurement device at the current position, no response expected.

Actually, I would prefer the newest position rather than the oldest.
If I cannot figure it out myself, I will ask again.

Bengt 0

@J-Hilk

Hi,

Designing and debugging a thing like this is difficult or impossible without a real device that answers the requests. Your code is impressive in this respect.

A few things..

In requestNewData() I changed from

void SerialPort::requestNextData()
{
  if(m_resultQueue.isEmpty()){
        m_Mode = NoOperation;
        return;
    } 
    m_Mode = GetScdPos;
    AXIS axis = m_requestQueue.first().axis;

to

void SerialPort::requestNextData()
{
 if(m_requestQueue.isEmpty()){
       m_Mode = NoOperation;
       return;
   } 
   m_Mode = GetScdPos;
   AXIS axis = m_requestQueue.first().axis;

because there are no results initially, so it will not proceed. First requests, then results. Would you agree this was a misprint?
Then I can see that the 512 byte buffer is never filled, so the initial condition bytesAvailable>512 in onReadyRead makes it return immediately. This may be because the OperationMode is never changed from "GetScdPos" after requestNextData() so requestNextData() is never entered again in requestSCDPos(). I am not sure where it should be changed.

Finally, I changed

    RequestQueue queue = m_resultQueue.dequeue();
    quint64 read = readData(queue.data.data(), 512);

to

    RequestQueue queue = m_requestQueue.dequeue();
    qint64 rd = read(b, 512);
    queue.data = b;

It simply did not read, I have no idea why. Anyway this is no functional difference.

So it is 90% working, but some things remains.
Comments are welcome.

J.Hilk

@Bengt-0 said in QSerialPort::waitForReadyRead exits with timeout:

A few things..
In requestNewData() I changed from
...
to
...
because there are no results initially, so it will not proceed. First requests, then results. Would you agree this was a misprint?

Yes, absolutely, that was an error on my part 😱

It simply did not read, I have no idea why. Anyway this is no functional difference.

there is, may be due to badly chosen names on my part! Read, same as readData expects a char* as the first argument. Seems like b is a QByteArray, so I'm surprised it worked 🤔

try:

RequestQueue queue = m_resultQueue.dequeue();
    QByteArray b = read(512);
    
    if(b.size() != 512)
        qDebug() << Q_FUNC_INFO << b.size() << "of" << 512;
    queue.data = b;

This may be because the OperationMode is never changed from "GetScdPos" after requestNextData() so requestNextData() is never entered again in requestSCDPos().

acutually, the idea was, this connect

//Automatically try to call next data package, when a response was succesfully extracted
    connect(this, &SerialPort::responseReceived, this, &SerialPort::requestNextData);

should automatically call requestNextData, once it received content of a full package, the queue would be empty -> the mode would be set to NoOperation and requestNextData exited early, setting the Serialport in "Idel" mode until next call of requestSCDPos