Running just one slot in a different Thread

Kevin470

@jsulm The problem with my Code is that my SerialCommunication::readyRead() method runs an infinite while loop.
If I move the m_serialPort object to a different/new thread, the SerialCommunication::readyRead() method blocks any other function in that class.
That is why I was thinking if it is possible to run only this method in a separate Thread so that any other function in this class is not blocked by it.
Are there better options to run an Infinite loop in a different thread for Serial Read?

JonB

@Kevin470
The simple(r) answer is precisely not to run an infinite loop for serial reading, not to use your waitForReadyRead(10) (which is a bit flaky anyway). Especially if that is why you are trying to do threading anyway. The Qt/event driven way is to accumulate input asynchronously into a buffer, then you probably wouldn't need any threading.

Kevin470

@JonB Thank you for your response.
My initial Idea was to emit the data that I receive every time from the Serial Port and send (emit) it to another Class which parses the data. (Using connect(&m_serialPort,&QSerialPort::readyRead,this,&SerialCommunication::readyRead,Qt::QueuedConnection))

But the device I connect to (which is ancient btw.) sends it extremely broken . I write a command and expect a response of 50 Bytes. I receive sometimes 1 response with all 50 bytes (and more sometimes) and sometimes 46 responses with single bytes and 2 responses with 2 bytes each.
And sometimes it sends automatic error responses that I have to parse.
That is why I thought of making this while loop because parsing so many broken responses is a lot more complex.
If I could run this slot in another thread which emits me the data if there is a break of 10ms, I can parse it a lot easier.

jsulm

@Kevin470 You need to buffer the data you receive until you got a full package. So, connect a slot to readyRead (without any endless loops!), in that slot read the data and append to the buffer.

Kevin470

@jsulm
That was my initial plan. But since I expect different types of responses with varied byte sized I thought running this infinite loop would be the better idea. I will still try to use a buffer and solve this problem. Thank you all.

JonB

@Kevin470 said in Running just one slot in a different Thread:

But since I expect different types of responses with varied byte sized

You current implementation, with its while and waitForReadyRead(), effectively relies on timing of arrival to determine what constitutes a full "message" (emit newDataReceived(reception_buffer)). This is not an ideal way to arrange things. When you change over to buffering it will work much easier if your "messages" have some actual boundaries, e.g. a byte count at the beginning or a terminator at the end. Do they have such? If not you will have to do some work to deal with time passed in order to reconstruct separate messages.

Kevin470

@JonB Unfortunately they do not have any such boundaries.
Each command has a response of different size. And they do not have any terminators at the end or byte count in the response (like I mentioned earlier, it is a very old firmware). That is why I am finding it difficult to reconstruct every message individually if they are broken.

JonB

@Kevin470
OK, then let's examine what you are going to have to do to behave the same as currently if you move off the blocking loop of while ... waitForReadyRead(1000).

Your current code receives some initial data and then keeps accumulating that into a single message while any new data arrives within 1 second. Once 1 second has passed without any further data arriving that marks the end of the "message" and you emit newDataReceived(reception_buffer).

To achieve that without the loop and the waiting, you are going to need a (single shot) QTimer::singleShot(). You want to keep accumulating into a buffer (class member variable). Each time new data arrives you want to restart the timer with a new 1 second from now. When finally the timer expires/times out that means 1 second has passed without any new data, and at that point your accumulated message data is "complete", you emit the signal and clear out the pending buffer.

So very vaguely I would expect something like:

// `SerialPort` has `reception_buffer` and `timer` as member variables

reception_buffer.clear();
timer.setSingleShot(true);
timer.callOnTimeout(this, &SerialPort::onTimeout);

void SerialPort::onReadyRead()
{
    qDebug() << "reception_buffer appending new data";
    reception_buffer.append(readAll());
    timer.start(1000);
}

void SerialPort::onTimeout()
{
    qDebug() << "reception_buffer ready";
    emit newDataReceived(reception_buffer);
    reception_buffer.clear();
}

kuzulis

You don't need in a separate receiption buffer, like: reception_buffer.append(readAll()); because the QSP already so accumulates the data inside.
If a length of your response depends on a command, then you know that after a command A you expect e.g. a response with the 53 bytes, then it is simple to do like this:

SerialPort::SerialPort()
{
    timer = new QTimer(this);

    connect(timer, &QTimer::timeout, this, &SerialPort::onTimeout);
    timet->setSingleShot(true);
}

void SerialPort::onReadyRead()
{
    const auto bytesReceived = serial->byteaAvailable();
    if (bytesReceived  < 53) { // 53 - expected length of response on a command `A`
        timer->start(1000); // Or any other delay as you want.
   } else {
        const auto packet = serial->read(53);
        emit packetReceived(packet);
   }
       
}

void SerialPort::onTimeout()
{
    qDebug() << "oops, something went wrong"
}

But a better way is co create a some communication protocol with a <length><data><crc>.

Kevin470

@JonB Thanks a lot for your response. I tried your program.

The buffer keeps appending and it gives out this error constantly without break.

reception_buffer appending new data
QObject::startTimer: Timers cannot be started from another thread
reception_buffer appending new data
QObject::killTimer: Timers cannot be stopped from another thread
QObject::startTimer: Timers cannot be started from another thread
reception_buffer appending new data
QObject::killTimer: Timers cannot be stopped from another thread

I am not creating any Thread anywhere

JonB

@Kevin470 said in Running just one slot in a different Thread:

I am not creating any Thread anywhere

I think you are, else you wouldn't get that message.... Since you can see I don't create threads it can't be me. The whole point of this was for you to get rid of your dedicated thread doing a blocking while loop, so you clearly have not done that.....

You:

If I move the m_serialPort object to a different/new thread

Me:

The Qt/event driven way is to accumulate input asynchronously into a buffer, then you probably wouldn't need any threading.

Kevin470

@JonB I understand. But I am not running the moveToThread method at all.
This is my source currently.

There is no Threading involved except showing which function runs on which thread

SerialCommunication::SerialCommunication(QObject *parent)
    : QObject{parent}
{
    qInfo() << "SerialCommunication Constructor Thread: " << QThread::currentThread();

    m_serialPort.setBaudRate(QSerialPort::Baud57600,QSerialPort::AllDirections);
    m_serialPort.setDataBits(QSerialPort::Data8);
    m_serialPort.setParity(QSerialPort::NoParity);
    m_serialPort.setStopBits(QSerialPort::OneStop);
    m_serialPort.setPortName("COM5");

    connect(&m_serialPort,&QSerialPort::errorOccurred,this,&SerialCommunication::serialPortErrorOccured,Qt::QueuedConnection);
    connect(&m_serialPort,&QSerialPort::readyRead,this,&SerialCommunication::onReadyRead,Qt::QueuedConnection);

    m_receptionBuffer.clear();
    m_receptionTimer.setSingleShot(true);
    m_receptionTimer.callOnTimeout(this, &SerialCommunication::onTimeout);

    openPort();
}

SerialCommunication::~SerialCommunication()
{
    closePort();
}

bool SerialCommunication::openPort()
{
    m_serialPort.open(QIODevice::ReadWrite);

    if (m_serialPort.isOpen())
    {
        qDebug() << "Serial port is open...";
        return true;

    }
    else
    {
        qDebug() << "OPEN ERROR: " << m_serialPort.errorString();
        return false;
    }
}

bool SerialCommunication::closePort()
{
    if (m_serialPort.isOpen())
        {
            m_serialPort.close();
            qDebug() << "...serial port is closed!";
        }
    return true;
}

bool SerialCommunication::write(QByteArray &data)
{
    qInfo() << "SerialCommunication Write Function Thread: " << QThread::currentThread();
    if (m_serialPort.isWritable())
    {
        if(!m_serialPort.write(data))
        {
            qDebug() << "Serial Data Cannot be Written!";
            return false;
        }
        else
        {
            m_serialPort.flush();
            qInfo() << "Serial Data:" << data << "Written!";
            return true;
        }
      return true;
    }
    else
    {
        qDebug() << "Serial Data Cannot be Written!";
        return false;
    }
}

void SerialCommunication::onReadyRead()
{
    qDebug() << "reception_buffer appending new data";
    m_receptionBuffer.append(m_serialPort.readAll());
    m_receptionTimer.start(1000);
}

void SerialCommunication::onTimeout()
{
    qDebug() << "reception_buffer ready";
    emit newDataReceived(m_receptionBuffer);
    m_receptionBuffer.clear();
}

void SerialCommunication::serialPortErrorOccured(QSerialPort::SerialPortError error)
{
    switch (error)
    {
    case (QSerialPort::NoError):
        break;
    case (QSerialPort::DeviceNotFoundError):
        qCritical() << "Serial Port not Found";
        break;
    case (QSerialPort::PermissionError):
        qCritical() << "Serial Port already in use or Permission required";
        break;
    case (QSerialPort::OpenError):
        qCritical() << "Serial Port Cannot be opened!";
        break;
    case (QSerialPort::NotOpenError):
        qCritical() << "Operation not Successful. Serial Port not Open";
        break;
    case (QSerialPort::WriteError):
        qCritical() << "Serial Port Error while writing Data!";
        break;
    case (QSerialPort::ReadError):
        qCritical() << "Serial Port Error while reading Data!";
        break;
    case (QSerialPort::ResourceError):
        qCritical() << "Serial Port Communication lost. Please try again!";
        break;
    case (QSerialPort::UnsupportedOperationError):
        qCritical() << "Serial Port Operation not Supported";
        break;
    case (QSerialPort::TimeoutError):
        qCritical() << "Serial Port Timeout";
        break;
    case (QSerialPort::UnknownError):
        qCritical() << "Unknown Serial Port Error!";
        break;

    default:
        qCritical() << "Unknown Serial Port Error!";
        break;
    }
}

JonB

@Kevin470 said in Running just one slot in a different Thread:

There is no Threading involved except showing which function runs on which thread

Then why do you think you get that error message? :) And why would you report which thread the function runs in if you don't have multiple threads somewhere? And how does that output compare to whatever is your main thread?

You only show your SerialCommunication class. You show nothing about where you create the instance, other parts of the program. Could you please search the whole of your project's code for thread case-insensitive and report back....

Kevin470

@JonB Sorry about the inconvenience. I really did have moved it to a QThread in another class. I did not notice it earlier. I should have checked it properly.
Thank you so much. And it works really well. 👍

Kevin470

@kuzulis Thank you so much for your response. That is a great idea.
But you are right, the communication protocol would make a lot more sense if it followed the <length><data><crc> structure.

@JonB 's Idea works well with my existing Program structure.