Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

QSerialPort readAll() does not work with 5.14.0 anymore



  • Hello,
    after the update to Qt 5.14.0 or later e.g. 5.15.1 (from 5.13.2) my code for serial communication does not work anymore. I use QSerialPort for communication with a Zeiss microscopce for which I don't know all parameters for sure (I had to reverse engineer it a bit), but the code worked fine with 5.13.2 and before. The project is hosted on Github, see https://github.com/BrillouinMicroscopy/BrillouinAcquisition for the complete code.

    My class for serial communication looks like this:

    class com : public QSerialPort {
    public:
    	com() {};
    	com(std::string terminator) : m_terminator(terminator) {};
    
    	virtual qint64 writeToDevice(const char* data);
    
    	std::string receive(std::string request);
    	void send(std::string message);
    
    protected:
    	std::string m_terminator{ "\r" };
    };
    

    The function in question which stopped working after the update is this

    std::string com::receive(std::string request) {
    	request = request + m_terminator;
    	writeToDevice(request.c_str());
    
    	std::string response = "";
    	if (waitForBytesWritten(1000)) {
    		// read response
    		if (waitForReadyRead(1000)) {
    			QByteArray responseData = readAll();
    			while (waitForReadyRead(50))
    				responseData += readAll();
    
    			response = responseData;
    		}
    	}
    
    	return response;
    }
    

    I connect to the microscope like this (m_comObject is an object of type com)

    m_comObject->setPortName("COM1");
    if (!m_comObject->setBaudRate(QSerialPort::Baud9600)) {
    	throw QString("Could not set BaudRate.");
    }
    if (!m_comObject->setFlowControl(QSerialPort::HardwareControl)) {
    	throw QString("Could not set FlowControl.");
    }
    if (!m_comObject->setDataBits(QSerialPort::Data8)) {
    	throw QString("Could not set DataBits.");
    }
    if (!m_comObject->setParity(QSerialPort::NoParity)) {
    	throw QString("Could not set Parity.");
    }
    if (!m_comObject->setStopBits(QSerialPort::OneStop)) {
    	throw QString("Could not set StopBits.");
    }
    m_isConnected = m_comObject->open(QIODevice::ReadWrite);
    if (!m_isConnected) {
    	throw QString("Could not open the serial port.");
    }
    m_comObject->clear();
    

    Sending to the microscope still works fine (it reacts to the command), but Qt never receives any data. So reading from the microscope stopped working with the update for some reason.

    I tried to figure out what has changed for QSerialPort from 5.13.2 to 5.14.0, but couldn't really find anything which I thought to be relevant. I would be grateful for any hint about what has changed and how I can circumvent the problem. Thanks a lot in advance.



  • @rasc
    You may be right and there may be a problem in 5.14, I don't know. But if it were me trying to test for good/bad behaviour, I would at least get rid of all your waitFor...() calls and replace with signals/slots, at least while I tested/debugged.



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

    HardwareControl

    Are you sure in this flow control? Do you use and the RTS/CTS line pinouts?

    You can also check on the received data after the waitForBytesWritten()... Also, make sure that the waitForReadyRead() triggered too.



  • @JonB I now tested the QSerialPort Terminal example https://doc.qt.io/qt-5/qtserialport-terminal-example.html which uses async communication with signal/slots and this does indeed work with 5.14 onwards as well. I guess I could make it work using signal/slots, but I still wonder why it doesn't work anymore when using the blocking approach.

    @kuzulis I am not absolutely sure about the settings, since I determined them by try and error (and by recording the device communication when using the manufacturer software). However, communication with the microscope only worked when using HardwareControl, and it did work with 5.13.2. So I think these settings should be ok.
    I will check whether waitforReadyRead() triggered.



  • @rasc
    So if it works with signals/slots I assume there cannot be a pin control etc. issue.

    The waitFor...()s are a bit "dodgy". It looks like you are reporting a change in their behaviour. If you want to stick with them you probably need to try to see where exactly it does not behave as expected in those blocking calls. For example, I have no idea whether upping the timeouts (e.g. your while (waitForReadyRead(50))) would improve the situation.



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

    HardwareControl

    If you use the TX/RX pins only, then the HardwareControl do not need (it even may cause a problems), because in this mode the serial port driver waits for the RTS/CTS pins handshake.



  • @JonB I am fine with switching to the signals/slots approach. I only used the blocking approach, because it was a bit faster/simpler to implement in the first place. I will anyway try to debug this a bit more, since I really wonder what broke here. But I will switch to async if I cannot fix the blocking approach. Increasing the timeout did not help.

    @kuzulis I don't think the HardwareControl is the issue here, since the handshake works (I think). I can send data to the device and it reacts accordingly. Only receiving data does not work.



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



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



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



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



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


  • Lifetime Qt Champion

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



  • @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. :)



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



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



  • 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);
    


  • 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);
    }
    


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



  • @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?



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



  • @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!



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



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



  • @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?



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



  • @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! :)



  • @kuzulis
    I think you intend to address @rasc, not me, when you say "you"! :)

    I have said that non-blocking is preferable for the OP. But I can anticipate his reluctance to change existing, blocking code --- for right or for wrong --- as he moves between Qt versions and just wants to know why something which worked does not now.... Hence my suggestion to preserve his code's synchronicity. But if he can change over to signals/slots/asynchronous maybe the issue will go away and he will be better in the long-run, I think he is aware of this :)



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

    not me, when you say "you"! :)

    Oops, yes, sorry.. It is my copy/paste. :)



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

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

    I tried to do that and came up with this function as a replacement for waitForReadyRead():

    bool com::waitForReady(int timeout) {
    	waitForReadyRead(50); // Does not work without this line
    	QElapsedTimer elapsed;
    	elapsed.start();
    	while (bytesAvailable() == 0 && elapsed.elapsed() < timeout) {
    		QThread::msleep(3);
    	}
    	if (elapsed.elapsed() < timeout) {
    		return true;
    	}
    	return false;
    }
    

    I just check for bytesAvailable() during the timeout. While this basically means I have to sleep for some miliseconds on every call, it indeed works. But what puzzles me is that bytesAvailable() will only work after I called waitForReadyRead(50) once, otherwise it will be 0 all the time. My understanding of QSP is probably a bit limited, so maybe this is expected (I could imagine it is a threading issue, and calling QThread::msleep() never allows the EventLoop to run while waitForReadyRead() does, I don't know).

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

    I completely understand that, thanks a lot for your feedback. Just in case you decide to try to debug it, I am happy to help if I can.

    @JonB said

    I have said that non-blocking is preferable for the OP. But I can anticipate his reluctance to change existing, blocking code --- for right or for wrong --- as he moves between Qt versions and just wants to know why something which worked does not now.... Hence my suggestion to preserve his code's synchronicity. But if he can change over to signals/slots/asynchronous maybe the issue will go away and he will be better in the long-run, I think he is aware of this :)

    This is a pretty accurate summary :)
    Anyway, since checking bytesAvailable() somehow works, I guess I am fine for now. And since both of you suggest to move to signals/slots, I will try to do that in the long run.

    Should I mark this topic as solved then and accept an answer?



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

    bytesAvailable() will only work after I called waitForReadyRead(50) once

    Yes, the bytes available just returns a number of bytes in an internal QSP buffer. This buffer incremented after the waitForXXX() methods only, because this methods is a 'tricks' to schedule the I/O. Because in the blocking approach the Qt-event loop is not used.

    PS: And yes, you don't need in QThread::msleep(3), because it has not sense (because the sleeping already does by waitForXXX() methods).



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

    completely understand that, thanks a lot for your feedback. Just in case you decide to try to debug it, I am happy to help if I can.

    You can create a bug report for the QSP, and pass there a link to this topic. And then, when I got a free time, then, maybe I try to fix it. :)



  • @kuzulis said in QSerialPort readAll() does not work with 5.14.0 anymore

    PS: And yes, you don't need in QThread::msleep(3), because it has not sense (because the sleeping already does by waitForXXX() methods).

    Ah, ok. I guess

    bool com::waitForReady(int timeout) {
    	waitForReadyRead(1);
    	QElapsedTimer elapsed;
    	elapsed.start();
    	while (bytesAvailable() == 0 && elapsed.elapsed() < timeout) {
    		waitForReadyRead(1);
    	}
    	if (elapsed.elapsed() < timeout) {
    		return true;
    	}
    	return false;
    }
    

    is the "correct" solution then.

    Update: Just for the sake of completeness, here is the patch for my program: https://github.com/BrillouinMicroscopy/BrillouinAcquisition/pull/135


Log in to reply