QSerialPort readAll() does not work with 5.14.0 anymore

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

JonB

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

kuzulis

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

rasc

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