QSerialPort::waitForReadyRead exits with timeout

[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

[QtTester]

Hi,all ,I have a prolbem with waitForReadyRead, in qthread.run():

void run() override{
QByteArray ba;
QSerialPort sp;

sp.setPortName(p()->m_comPort);
sp.setBaudRate(p()->m_bd);
sp.setParity(QSerialPort::NoParity);
sp.setDataBits(QSerialPort::Data8);
sp.setStopBits(QSerialPort::OneStop);
sp.setFlowControl(QSerialPort::NoFlowControl);
 if(!sp.open(QIODevice::ReadWrite)){
      emit p()->sigHardwareError(-1);
      goto fail;
 }
do{
/* Method1: not work when mess data received , it always return false
if(sp.waitForReadyRead(50)){
    ba.clear();
    while(sp.bytesAvailable())
        ba += sp.readAll(); 
}*/
// Method2: need to change like this:
sp.waitForReadyRead(50);
 if(sp.bytesAvailable()){
     ba.clear();
     while(sp.bytesAvailable())
            ba += sp.readAll();
   }
}while(!isInterruptionRequested());
fail:
if(sp.isOpen())
    sp.close();
}

why waitForReadyRead() always return false with Method1 ?