QSerialPort::waitForReadyRead exits with timeout

Bengt 0

Hi,

I am sending a request to a HW box thru QSerialport, and try to receive the answer reliably.
I have a problem with waitForReadyRead. In my context, it always returns false with a timeout.

The send is a simple write without signal/slot connection, while the receive is async using readyRead signal connection to my "ReadPort". My derived class is called "SerialPort".

Code outline:

connect(this, &QSerialPort::readyRead, this, &SerialPort::PortRead); // working
...
lock=true;
WritePort(); // working
ok = waitForBytesWritten(500); // working
// Here I need to wait for the readyRead event and the completion of my ReadPort method.
ok = waitForReadyRead(500); // Always exits "false", error 12 = timeout.
// Here I want to pick up the result from ReadPort.
if(!lock) strcpy_s(str,112,Buffer);

If I skip the waitForReadyRead, the result pickup is reached before the readyRead event triggers ReadPort and I get nothing.
It "works" if I use waitForReadyRead with a short timeout limit (e.g. 10ms), but then it is effectively working as a simple delay, which is not really the idea.

What am I doing wrong?

void
SerialPort::PortRead(void)
{
    qint64 bav=0;
    char buf[128]="";
    buf[0] = buf[1] = (char)NULL;
    bav = bytesAvailable();
    if(bav) {
        QBuffer = readLine();
        strcat_s(Buffer,sizeof(Buffer), QBuffer.data());
    }
    bav = bytesAvailable();
    if (!bav) lock = false;  // No more bytes...allow reading.
    return;
}

BN

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

jsulm

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

the result pickup is reached before the readyRead event triggers ReadPort and I get nothing

I highly doubt that. It is actually other way around: arriving data triggers readyRead signal. How do you know that there is nothing in ready read?
Are you using Buffer which is filled in PortRead in this line:

if(!lock) strcpy_s(str,112,Buffer);

?
If so then that is the problem. You only can use Buffer when it was filled (AFTER executing PortRead), not already after sending the data!

J.Hilk

@Bengt-0 there's a reason, you're not supposed to mix synchronous and a synchronous api.

waitForReadyRead is blocking and depending on implementation, your readyRead signal can get lost.

Please stick with the the signals.

Bengt 0

@jsulm

Sorry, I do not understand your comment.
This is exactly what I am trying to do, read the Buffer AFTER the ReadPort is completed, not before. I assumed waitForReadyRead would release after the readyRead signal is emitted and ReadPort was completed, setting "lock=false", allowing me to safely copy the Buffer content.

Without waitForReadyRead, "lock" is still "true" since ReadPort is not yet executed and I cannot copy the received data.

As I was trying to say, even a small delay makes it work, such as a timeout from waitForReadyRead(10), but it is only a delay, not a message of success. This indicates to me that something is not working as it should.

Bengt 0

@J-Hilk

Thanks for this.

I have a timer which is sending requests every 250ms.

Previously I have used synchronous Write and Read without signals, but this is not safe and I am sometimes missing data. Async seems to be much more stable, if the timing can be handled.

Could you please suggest an outline how to use asynchronous requests to the HW box with a timer?

I am not an experienced programmer, but I am trying to get by.:)

J.Hilk

@Bengt-0

#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";
    }
}

QByteArray receivedData;
void onReadyRead(){
    receivedData += serial.readAll();

    //Your own logic, that ba is complete and correct

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

    //if(dataOK(receivedData))
    writeNextCommand();
}

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

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);
    //Setup Port

    QObject::connect(&serial, &QSerialPort::bytesWritten,&onBytesWritten);
    QObject::connect(&serial, &QSerialPort::readyRead, &onReadyRead);

    serial.setPortName("PortX");
   /*
    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

@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.