Running just one slot in a different Thread

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.