QSerialPort readAll() does not work with 5.14.0 anymore

kuzulis

I just checked the QSP with the Qt 5.15.0 (there are no changes between 5.14.0 and 5.15.x) using the virtual com0com serial ports and the following simple example:

#include <QCoreApplication>
#include <QSerialPort>
#include <QDebug>

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);
    QSerialPort sp("COM3");
    qDebug() << "OPEN:" << sp.open(QIODevice::ReadWrite);
    for (;;) {
        sp.write("foo\r\n");
        if (sp.waitForBytesWritten(1000)) {
            QByteArray recv;
            while (sp.waitForReadyRead(1000))
                recv += sp.readAll();
            qDebug() << "DATA:" << recv;
        } else {
            qDebug() << "WR OOPS:" << sp.error();
        }
    }
    return a.exec();
}

and it does work for me.

This example opens the COM3, send the 'foo' data and waits for the response. On other side I had open the Putty on the COM4, and put some symbols there.. And I see all received data on the COM3.

rasc

I now checked with the blockingmaster example https://doc.qt.io/qt-5/qtserialport-blockingmaster-example.html. With 5.13.2 the example code works, with 5.14.0 onwards it does not. So it doesn't seem to be a problem of my code (I was a bit afraid that it could be a threading issue, but the blocking master example only uses a single thread and still doesn't work).

I see that waitForReadyRead(1000) always runs into a timeout no matter the value and never resolves, so readAll() is never called. However, when I run readAll() anyway, it correctly catches the answer from the device. So communication actually works, but waitForReadyRead() seems to be broken for me.

What makes this especially weird is that the readyRead() signal seems to be emitted correctly, since this code

QEventLoop loop;
auto connection = QWidget::connect(
    this,
    &QSerialPort::readyRead,
    &loop,
    &QEventLoop::quit
);
write(request.c_str());
loop.exec();

correctly runs through and reading afterwards gives (a part) of the devices answer. I am a bit lost now, why waitForReadyRead() does not work.

JonB

@rasc said in QSerialPort readAll() does not work with 5.14.0 anymore:

I am a bit lost now, why waitForReadyRead() does not work.

I wrote you earlier:

The waitFor...()s are a bit "dodgy".

You have never said what platform you are on. Docs used to say waitFor...s were "dodgy" under Windows at least, though I believe that has been removed, so maybe they are supposed to be robust now. But your experiences seem to indicate.....

rasc

@JonB said in QSerialPort readAll() does not work with 5.14.0 anymore:

I wrote you earlier:

The waitFor...()s are a bit "dodgy".

Yes, that's for sure true.
I am indeed on Windows, but it has worked fine so far until 5.14.0.

Moving to signal/slots is sure possible, but probably requires quite a bit of work. I call the receive function in a for loop to control a translation stage on my microscope and raster a sample, so it has to work synchronously at the moment. Moving to signal/slots means changing how I control the stage. Also, I wanted to understand why it broke.

Thanks for the help, I guess signal/slots is the only solution now.

JonB

@rasc
I take the point, and I hope my answering has not dissuaded others chiming in if they know. Personally I don't know if there is anything wrong in your wait code. You may wish to raise this as Qt bug if you feel they may be able to help.

mrjj

Hi
I would wait with Qt bug report until @kuzulis answers. After all he wrote the class. :)

rasc

@JonB

Personally I don't know if there is anything wrong in your wait code.

Well, I basically copied the example code and the blockingmaster example doesn't work anymore either. So something in the behavior of QSerialPort has changed from 5.13.2 to 5.14.0 onwards. If it is an intended change or not, I don't dare to judge. If @kuzulis wrote the class, I guess he is the best expert on this. :)

kuzulis

@rasc said in QSerialPort readAll() does not work with 5.14.0 anymore:

waitForReadyRead() seems to be broken for me.

It is possible that now the waitForBytesWritten() handles the read pending requests too.. So, you may try to check that the new data are available after the waitForBytesWritten() call.

kuzulis

@rasc said in QSerialPort readAll() does not work with 5.14.0 anymore:

I basically copied the example code and the blockingmaster example doesn't work anymore either.

For me it does work with the virtual com0com serial ports. :)

A main issue is that it is hard to reproduce this problem to create the auto-tests. And yes, the waitForXXX() methods it is devil methods, it would be good to remove this at all and keep only the asynchronous API.

@rasc said in QSerialPort readAll() does not work with 5.14.0 anymore:

So something in the behavior of QSerialPort has changed from 5.13.2 to 5.14.0

Yes, there are a main change is that now the I/O uses the Windows alertable functions, like {Read|Write}FileEx() instead of previous {Read|Write}File() && QWinOverlapepdIONotifier (which was buggy and removed too).

kuzulis

A better solution is to use the asynchronous approach with the signals. :)

UPD: The QSP already has the similar 'blocking' auto-test, but it does not reproduces the problem for me:

    // first stage
    senderPort.write(newlineArray);
    senderPort.waitForBytesWritten(waitMsecs);
    QTest::qSleep(waitMsecs);
    receiverPort.waitForReadyRead(waitMsecs);
    QCOMPARE(receiverPort.bytesAvailable(), qint64(newlineArray.size()));

    receiverPort.write(receiverPort.readAll());
    receiverPort.waitForBytesWritten(waitMsecs);
    QTest::qSleep(waitMsecs);
    senderPort.waitForReadyRead(waitMsecs);
    QCOMPARE(senderPort.bytesAvailable(), qint64(newlineArray.size()));
    QCOMPARE(senderPort.readAll(), newlineArray);

kuzulis

This test passes for me with the com0com serial port (I have not the USB/Serial adapters):

void tst_QSerialPort::twoStageSynchronousLoopback()
{
    QSerialPort senderPort(m_senderPortName);
    QVERIFY(senderPort.open(QSerialPort::ReadWrite));

    QSerialPort receiverPort(m_receiverPortName);
    QVERIFY(receiverPort.open(QSerialPort::ReadWrite));

    const int waitMsecs = 50;

    // first stage
    QVERIFY(senderPort.write(newlineArray) > 0);
    QVERIFY(senderPort.waitForBytesWritten(waitMsecs));
    QTest::qSleep(waitMsecs);
    QVERIFY(receiverPort.waitForReadyRead(waitMsecs));
    QCOMPARE(receiverPort.bytesAvailable(), qint64(newlineArray.size()));

    QVERIFY(receiverPort.write(receiverPort.readAll()) > 0);
    QVERIFY(receiverPort.waitForBytesWritten(waitMsecs));
    QTest::qSleep(waitMsecs);
    QVERIFY(senderPort.waitForReadyRead(waitMsecs));
    QCOMPARE(senderPort.bytesAvailable(), qint64(newlineArray.size()));
    QCOMPARE(senderPort.readAll(), newlineArray);

    // second stage
    QVERIFY(senderPort.write(newlineArray) > 0);
    QVERIFY(senderPort.waitForBytesWritten(waitMsecs));
    QTest::qSleep(waitMsecs);
    QVERIFY(receiverPort.waitForReadyRead(waitMsecs));
    QCOMPARE(receiverPort.bytesAvailable(), qint64(newlineArray.size()));
    QVERIFY(receiverPort.write(receiverPort.readAll()) > 0);
    QVERIFY(receiverPort.waitForBytesWritten(waitMsecs));
    QTest::qSleep(waitMsecs);
    QVERIFY(senderPort.waitForReadyRead(waitMsecs));
    QCOMPARE(senderPort.bytesAvailable(), qint64(newlineArray.size()));
    QCOMPARE(senderPort.readAll(), newlineArray);
}

rasc

@kuzulis Thanks for all your feedback. I for sure trust you that your auto-tests all pass. However, they don't seem to test for the issue I have, which might very well be related to how the hardware I have behaves.

I followed your idea to test for available data after the waitForBytesWritten() call. I did

write(data);
if (waitForBytesWritten(1000)) {
	flush();		// just for testing, doesn't change anything
	auto available1 = bytesAvailable(); // <- yields 0
	waitForReadyRead(10000);	// <- runs into timeout
	auto available2 = bytesAvailable(); // <- yields correct length
	auto result = readAll();	// yields correct data
}

So the data is in the read buffer, but waitForReadyRead() is not notified about that.

JonB

@rasc
I assume you tested this under 5.15.1? Can you confirm to @kuzulis that you have also tried just this code under 5.13.2 and it does not timeout on your waitForReadyRead(10000); // <- runs into timeout?

rasc

@JonB Sorry if I have been unclear here.
The code above runs fine on 5.13.2 (Win), no timeout and waitForReadyRead(10000); returns true.
The very same code on 5.15.0 (Win) runs into the timeout and waitForReadyRead(10000); returns false.

JonB

@rasc
Hopefully @kuzulis will read this and may have a comment to make!

I will just say this is why I have never used to waitFor...() methods, as stated earlier!

rasc

@JonB said in QSerialPort readAll() does not work with 5.14.0 anymore:

I will just say this is why I have never used to waitFor...() methods, as stated earlier!

Yes, I am afraid this will be the only solution for me as well.

kuzulis

@JonB said in QSerialPort readAll() does not work with 5.14.0 anymore:

Hopefully @kuzulis will read this and may have a comment to make!

Yes, maybe it is a bug in QSP >= 5.14.x, but a problem is that it is hard to reprocuce it.. So...

JonB

@rasc
Never tried it, but I would have assumed you can "emulate" waitFor...() by setting up your signals and using QEventLoop::exec() + QEventLoop::exit(), plus a QTimer(), to get the "synchronous" behaviour.

Now, I do not know how that compares to what is actually in the waitFor...()s. And since @kuzulis wrote that, and apparently its behaviour has changed, it may be more complex, and his code may cover more cases than this approach. I don't know if you want to test that out to see how it compares....

@kuzulis Since you have just replied, is it worth OP trying this, or is it more complex/hassle than that?

kuzulis

@rasc,

Just don't rely on the return value from waitForReadyRead () then, just check the bytesAvailable() then after waitForReadyRead() (if you want to use a blocking approach).

PS: I have no time and resources now to fix it.

kuzulis

@JonB said in QSerialPort readAll() does not work with 5.14.0 anymore:

OP trying this, or is it more complex/hassle than that?

An non-blocking approach it is a best/right choose for you! :)