QIODevice::readLine (in a QSerialPort object) putting more than MAX characters into buffer.

teux

I'm trying to debug some intermittent serial port issues. This is on a raspberry pi compute module running Qt 4.8, with a manually compiled QSerialPort class. I'm talking to a rather slow micro-controller that will take ~20ms to start clocking out response uart data after it gets the command.

It's possible I'm using the library wrong, but I've been having trouble finding documentation that matches my use case.

One problem is that I have a readLine call as such:

char rx[16];
rs485.readLine(rx, 16);

which, according to the documentation here, should always append a terminating null to the string.

I have the following line for debugging these errors:

if(atoi(rx) == 0)
    qDebug(QString("received possibly erroneous string: \"%1\"").arg(rx).remove('\r').remove('\n').toLatin1().data());

This being the case, I got the following error right before my program crashed due to a corrupted double-linked list (which I'm 99% sure is because of a c-string terminator missing):

received possibly erroneous string: "j«!`xµ�E.0.1.1      "

It's confusing for two reasons. One, that data should never be on my serial port, and two, that's twenty characters, which is over the max of 16 I specified in readline(). So where did that terminating null go?

Thank you for any ideas/help. The entire method is shown below. The microsecond sleeps were put in to reduce transmission errors, which they have, although I'm suspect of their use. There's probably a better way to do that.

This is all running on its own thread, not on any UI thread or event loop.

uint Comm::get_reading(int channel)
{
    if(channel > 0 && channel < 13)
    {
        QString command = QString("\rr %1 rd %2\r").arg(this->serial).arg(channel);

        QMutexLocker locker(&this->comm_lock);

        rs485.clear();
        usleep(25000);

        rs485.write(command.toLatin1().data(), command.length());
        rs485.waitForBytesWritten(1000);

        usleep(25000);

        char rx[16];
        rs485.waitForReadyRead(1000);
        rs485.readLine(rx, 16);

        locker.unlock();

        if(atoi(rx) == 0)
            qDebug(QString("received possibly erroneous string: \"%1\"").arg(rx).remove('\r').remove('\n').toLatin1().data());

        return static_cast<uint>(atoi(rx));
    }

    return 0;
}

jsulm

@teux said in QIODevice::readLine (in a QSerialPort object) putting more than MAX characters into buffer.:

So where did that terminating null go?

readLine() probably failed (you should check its return value), so there is no 0x00 in the string and the string simply contains garbage.
Why do you have usleep(25000); before waitForReadyRead?
Are you calling this function from different threads? If so, maybe you should move the command string under the mutex locker.

teux

@jsulm said in QIODevice::readLine (in a QSerialPort object) putting more than MAX characters into buffer.:

readLine() probably failed (you should check its return value), so there is no 0x00 in the string and the string simply contains garbage.

Ah, yeah, I should be doing that. Thanks.

Why do you have usleep(25000); before waitForReadyRead?

That one is there to give the (slow) microcontroller on the other end of the serial line enough time to process and respond to the command. The event loop gets around to processing the comms buffer every15-20ms.

Are you calling this function from different threads?

No, this one is only called from one thread. There are other methods from this class that are though, and that's a really good point that I overlooked. Thanks.

aha_1980

@teux

Just some more comments to your code from my side. Feel free to ignore them...

First of all, the waitFor(ReadyRead|BytesWritten) functions should only be used if your serial communication is running in its own thread. Otherwise use Signals&Slots.

QMutexLocker locker(&this->comm_lock);

Locking a mutex for a long time as you do here is most often a bad software design.

    rs485.clear();
    usleep(25000);

What's the reason for the wait here?

    rs485.write(command.toLatin1().data(), command.length());
    rs485.waitForBytesWritten(1000);

    usleep(25000);

In my experience, waitForBytesWritten() most often returns too early, as it only transfers the data to the operating system buffers. You can try to use flush(), but if you expect an answer from the other side after each command, waitForReadyRead() with appropriate timeout is most often enough.

    char rx[16];
    rs485.waitForReadyRead(1000);
    rs485.readLine(rx, 16);

Maybe it's easier to use QByteArray rx = rs485.readLine() here (Docu) as it implicitely contains the lenght. Please note that the assumption that you can read a line after the wait can lead to wrong results, as waitForReadyRead() just tells that that any data is there, not that this is a complete line (which can be checked with canReadLine())

Regards

teux

Thank you all. My problem was resolved with the understanding that waitForReadyRead() returns as soon as bytes are available, and not necessarily when I can read an entire line. (Which absolutely makes sense), as well as checking the return output of readLine to see if it was successful. It's working flawlessly now. No transmission errors in megabytes of send/receive at 57600 baud.

To answer questions @aha_1980 (thank you for the insight):

Locking a mutex for a long time as you do here is most often a bad software design.

That lock is for access to the serial port hardware, so multiple threads don't try to control it at once. I don't know how else to ensure that a command and response interaction is atomically sent and received over RS485.

I can't get around the fact that I won't get a response until about 25ms later. And in that time, no other thread can be allowed to take control of the serial hardware. What other way would there be of doing this?

What's the reason for the wait here?

No reason, and it didn't really make sense. It seemed like the UART buffer was inadvertently filling with trash data, and I was trying to debug that. It wasn't the case, and I was actually just exceeding the bounds of my RX cstring that didn't have a null terminator. My fault for not checking the return code.

aha_1980

@teux said in QIODevice::readLine (in a QSerialPort object) putting more than MAX characters into buffer.:

That lock is for access to the serial port hardware, so multiple threads don't try to control it at once. I don't know how else to ensure that a command and response interaction is atomically sent and received over RS485.

I already thought that, but why using multiple threads if you block all of them during the transmission? A mutex is thought to block for nanoseconds, just to ensure a variable is atomically changed.

I don't know your program, but it seems you have:

one thread that does the serial communication
multiple threads that need to exchange data over serial port

?

In that case, what about a command queue within the serial thread that is filled by the other threads and executed one by one. The result can be sent back to the other treads by signals.

Therefore the other threads can at least do other work as long as the serial communication is running.

Just a idea.

teux

@aha_1980 said in QIODevice::readLine (in a QSerialPort object) putting more than MAX characters into buffer.:

In that case, what about a command queue within the serial thread that is filled by the other threads and executed one by one. The result can be sent back to the other treads by signals.
Therefore the other threads can at least do other work as long as the serial communication is running.
Just a idea.

Thanks. I did consider that design, but I suppose in my case, there's not really a difference? At least, enough of one to justify the development overhead of writing an event queue. There's a large command set the microcontroller takes, and a large range of responses it gives.

None of the blocked threads are UI threads, and they each have discrete functions to perform on the serial port data. So either they'll be blocked by a mutex, or spinning in the event loop (also doing nothing) waiting for a signal to proceed. Obviously signals are more in line with the Qt paradigm, but I believe the end result is the same in this case. Just less future proof. And a blocked thread (at least, on Linux) will just sleep, and stay out of the CPU/scheduler until the mutex is free and it's woken.