Create object in another object

[Damian7546]

Hi,

I do not how it is possible that below creating objects in new thread works when below code run in main.cpp :

QQueue<RfidFrame*> outFrameQueue;
    QThread *threadRfidSerial = new QThread();
    RfidSerialWorker *rfidSerialWorker = new RfidSerialWorker(&outFrameQueue);
    RfidFrameProcessor *rfidFrameProcessor = new RfidFrameProcessor(&outFrameQueue);

    rfidSerialWorker->moveToThread(threadRfidSerial);
    QObject::connect(rfidSerialWorker, SIGNAL(frameReceived(RfidFrame*)), rfidFrameProcessor, SLOT(FrameIncoming(RfidFrame*)));
    QObject::connect(rfidSerialWorker, SIGNAL(workRequested()), threadRfidSerial, SLOT(start()));
    QObject::connect(threadRfidSerial, SIGNAL(started()), rfidSerialWorker, SLOT(doWork()));
    QObject::connect(rfidSerialWorker, SIGNAL(finished()), threadRfidSerial, SLOT(quit()), Qt::DirectConnection);


    rfidSerialWorker->abort();
    threadRfidSerial->wait(); // If the thread is not running, this will immediately return.
    rfidSerialWorker->requestWork();

but doesn't work when I above code move to other class constructor.... When I run above code in constructor other class the applicaation crashed. in while loop, when program check m_outFrameQueue variable .....

void RfidSerialWorker::doWork()
{
.
.
.
    while(!abort)
    {
        mutex.lock();
        abort = _abort;
        mutex.unlock();


        if(!m_outFrameQueue->isEmpty())
        {
.
.
.
    }}}

[JonB]

@Damian7546 said in Create object in another object:

QQueue<RfidFrame*> outFrameQueue;
RfidSerialWorker *rfidSerialWorker = new RfidSerialWorker(&outFrameQueue);
RfidFrameProcessor *rfidFrameProcessor = new RfidFrameProcessor(&outFrameQueue);
...
timer->start(2000);

If I understand your code right. QQueue<RfidFrame*> outFrameQueue is a local variable in some method? But once you have set off the timer you exit that method, right? So the QQueue<RfidFrame*> outFrameQueue goes out of scope and gets destroyed at that point. Yet you have passed that queue as a parameter to RfidSerialWorker/RfidFrameProcessor, they have set a m_outFrameQueue to point to that queue passed in, it is no longer valid, but they still use it => disaster. Is my analysis correct for your code?

I'm not sure, but it sounds like your QQueue<RfidFrame*> outFrameQueue; should not be local variable in a method but rather a persistent member variable?

[jsulm]

@Damian7546 said in Create object in another object:

when program check m_outFrameQueue variable

So, did you initialise it?

[Damian7546]

Without initialization, only assignment in constructor:

RfidSerialWorker::RfidSerialWorker(QQueue<RfidFrame*> *outFrameQueue, QObject *parent) :
    QObject(parent)
{

    m_outFrameQueue = outFrameQueue;
}

[jsulm]

@Damian7546 said in Create object in another object:

m_outFrameQueue = outFrameQueue;

And is outFrameQueue a valid pointer?

[Damian7546]

This is my declaration
QQueue<RfidFrame*> *m_outFrameQueue;

With RfidFrame I want to prepare my frame to send:

#include <QObject>

class RfidFrame : public QObject
{
    Q_OBJECT
public:
    static const quint8 FRAME_ADDR_RFiD = 0x01;
    static const int INDEX_ADDR_RFiD = 0;
    static const int INDEX_DATA_LENGTH = 1;
    static const int INDEX_DATA_CMD = 2;
    static const int INDEX_FIRST_DATA_BYTE = 3;
    static const int INDEX_FIRST_ID_DATA = 5;

    explicit RfidFrame(QObject *parent = nullptr);
    explicit RfidFrame(quint8 addr, quint8 cmd, quint8 data, QObject *parent = nullptr);
    explicit RfidFrame(quint8 addr, quint8 cmd, qint8 data, QObject *parent = nullptr);
    explicit RfidFrame(quint8 addr, quint8 cmd, quint16 data, QObject *parent = nullptr);
    explicit RfidFrame(quint8 addr, quint8 cmd, qint16 data, QObject *parent = nullptr);
    explicit RfidFrame(quint8 addr, quint8 cmd, quint32 data, QObject *parent = nullptr);
    explicit RfidFrame(quint8 addr, quint8 cmd, qint32 data, QObject *parent = nullptr);
    explicit RfidFrame(quint8 addr, quint8 cmd, QByteArray data, QObject *parent = nullptr);
    ~RfidFrame();

    quint16 CalculateCRC2_in(QByteArray data);

private:
    QByteArray m_buffer;

};

For example sending this frame I do in this way

RfidFrame *frameToSend = new RfidFrame(addr, C_WriteSourceRSx, data);
    m_outFrameQueue->enqueue(frameToSend);

Like I said, in main.cpp it works...

[Damian7546]

@jsulm probably it is not a problem with initialisation.

I added to my code first frame to send like below:

QQueue<RfidFrame*> outFrameQueue;

    RfidFrame *frameToSend = new RfidFrame(1, 1, 1);
    outFrameQueue.enqueue(frameToSend);

    QThread *threadRfidSerial = new QThread();
    RfidSerialWorker *rfidSerialWorker = new RfidSerialWorker(&outFrameQueue);
    RfidFrameProcessor *rfidFrameProcessor = new RfidFrameProcessor(&outFrameQueue);

    rfidSerialWorker->moveToThread(threadRfidSerial);
    QObject::connect(rfidSerialWorker, SIGNAL(frameReceived(RfidFrame*)), rfidFrameProcessor, SLOT(FrameIncoming(RfidFrame*)));
    QObject::connect(rfidSerialWorker, SIGNAL(workRequested()), threadRfidSerial, SLOT(start()));
    QObject::connect(threadRfidSerial, SIGNAL(started()), rfidSerialWorker, SLOT(doWork()));
    QObject::connect(rfidSerialWorker, SIGNAL(finished()), threadRfidSerial, SLOT(quit()), Qt::DirectConnection);


    rfidSerialWorker->abort();
    threadRfidSerial->wait(); // If the thread is not running, this will immediately return.
    rfidSerialWorker->requestWork();

[Christian Ehrlicher]

@Damian7546 said in Create object in another object:

rfidSerialWorker->abort();
threadRfidSerial->wait(); // If the thread is not running, this will immediately return.
rfidSerialWorker->requestWork();

What should this do? Where is this called? Directly after the connect() statements? How is this supposed to work then and why a thread for this stuff at all?

[Damian7546]

Please see my connect functions and below:

void RfidSerialWorker::requestWork()
{
    mutex.lock();
    _working = true;
    _abort = false;
    qDebug()<<"Request worker start in Thread "<<thread()->currentThreadId();
    mutex.unlock();


    emit workRequested();
}
void RfidSerialWorker::abort()
{
    mutex.lock();
    if (_working) {
        _abort = true;
        qDebug()<<"Request worker aborting in Thread "<<thread()->currentThreadId();
    }
    mutex.unlock();
}

[Christian Ehrlicher]

This does not answer my questions - why do you call abort() directly after creating all the stuff and then block the main event loop until the thread is finished - why (and why a thread at all - what's the advantage except strange problems like yours?)

[Damian7546]

I simplified my code as below: And still the same problem.

RfidSystemController::RfidSystemController( QObject *parent)
    : QObject(parent)
{

    QQueue<RfidFrame*> outFrameQueue;
    QThread *threadRfidSerial = new QThread();
    RfidSerialWorker *rfidSerialWorker = new RfidSerialWorker(&outFrameQueue);
    RfidFrameProcessor *rfidFrameProcessor = new RfidFrameProcessor(&outFrameQueue);

    rfidSerialWorker->moveToThread(threadRfidSerial);
    connect(rfidSerialWorker, SIGNAL(frameReceived(RfidFrame*)), rfidFrameProcessor, SLOT(FrameIncoming(RfidFrame*)));
    threadRfidSerial->start();
    connect(threadRfidSerial, SIGNAL(started()), rfidSerialWorker, SLOT(doWork()));

}

And class where is a problem :

#include "rfidserialworker.h"
#include <QTimer>
#include <QEventLoop>
#include <QThread>
#include <QDebug>
#include <QFile>

RfidSerialWorker::RfidSerialWorker(QQueue<RfidFrame*> *outFrameQueue, QObject *parent) :
    QObject(parent)
{
    m_Serial = nullptr;
    m_outFrameQueue = outFrameQueue;
}

RfidSerialWorker::~RfidSerialWorker()
{
    if(m_Serial != nullptr)
        delete m_Serial;
}

void RfidSerialWorker::doWork()
{

    qDebug()<<"Starting worker process in Thread "<<thread()->currentThreadId();

    // Serial Port Initialization
    m_Serial = new QSerialPort();
    m_Serial->setPortName("COM1");
    m_Serial->setBaudRate(QSerialPort::Baud9600);
    m_Serial->setDataBits(QSerialPort::Data8);
    m_Serial->setParity(QSerialPort::NoParity);
    m_Serial->setStopBits(QSerialPort::OneStop);
    m_Serial->setFlowControl(QSerialPort::NoFlowControl);

    QObject::connect(m_Serial,SIGNAL(readyRead()),this, SLOT(recive()));
    m_Serial->open(QIODevice::ReadWrite);
    qDebug() << "SerialPort Status: " << m_Serial->isOpen();

    emit comPortStatus(m_Serial->isOpen());


    while(1)
    {
            if(!m_outFrameQueue->isEmpty())
            {
                RfidFrame *outFrame = m_outFrameQueue->dequeue();
                sendFrame(outFrame);
                delete outFrame;

            }
            else
            {
                m_Serial->waitForReadyRead(10);
            }
    }
    if(m_Serial != nullptr)
    {
        m_Serial->close();
        qDebug() << "SerialPort Closed";
        delete m_Serial;
        qDebug() << "SerialPort destroyed";

    }
    qDebug()<<"Worker process finished in Thread "<<thread()->currentThreadId();
    emit finished();

}
void RfidSerialWorker::recive()
{
    receivedData = receivedData + m_Serial->readAll();
    if(!receivedData.isEmpty() && receivedData.count() > 2 )
    {
        if(receivedData.count() == quint8(receivedData[RfidFrame::INDEX_DATA_LENGTH] ) )
        {
            qDebug() << "We have the full frame";
            receivedFrame = receivedData;
            receivedData.clear();
            parseRecivedData();
        }
    }
}

In While loop should works like : While sending buffer is not empty then send frame, otherwise waitForReadyRead(10) to give a chance to call recive() function from connect(m_Serial,SIGNAL(readyRead()),this, SLOT(recive())); signal. ...

[jsulm]

@Damian7546 Again: why do you need a thread? Qt is assynchronous framework, QSerialPort is also assynchronous...

[Damian7546]

I would like to separate above task form others. This project is for learning programming purposes only.
In my example using thread it is a mistake ?

[jsulm]

@Damian7546 said in Create object in another object:

In my example using thread it is a mistake ?

You should avoid threads if they are not needed as it is easy to do mistakes and the code becomes more complex. As I wrote: Qt is an asynchronous framework, that means you usually do not need threads. QSerialPort is also asynchronous - you can use it without threads and it will still not block your main thread. Threads are usually used to move heavy calculations to other threads, but I don't see any heavy calculations in your code.

[Christian Ehrlicher]

@Damian7546 said in Create object in another object:

This project is for learning programming purposes only

Then start with writing clean code without threads. Write a good handler class to handle your incoming data. Once this is done this class can be easily moved into a separate thread if really needed.

[Damian7546]

anyway, can anyone demonstrate why the above code doesn't work when created from the constructor? but works with the main.cpp one?

[jsulm]

@Damian7546 If your application crashes then the first thing to do is: use the debugger.
So, please run your app in debugger and post the stack trace after the crash here...

[Christian Ehrlicher]

Your code crashes in the dtor of RfidSerialWorker (if doWork() would reach it's end), also it's using a blocking approach instead signals and slots to wait for data, the strange while(1) - loop does never exits and at last ít's accessing outFrameQueue from two different threads without proper locking mechanisms (and before you ask - read and understand at least https://doc.qt.io/qt-6/threads-synchronizing.html before fiddling around with threads for no reason).

[Damian7546]

@Christian-Ehrlicher so I will do like you propose.

[Damian7546]

I think that no a problem with thread.

I remove thread and create my object like below:

  QQueue<RfidFrame*> outFrameQueue;
    RfidSerialWorker *rfidSerialWorker = new RfidSerialWorker(&outFrameQueue);
    RfidFrameProcessor *rfidFrameProcessor = new RfidFrameProcessor(&outFrameQueue);

    connect(rfidSerialWorker, SIGNAL(frameReceived(RfidFrame*)), rfidFrameProcessor, SLOT(FrameIncoming(RfidFrame*)));
    connect(rfidFrameProcessor, SIGNAL(frameSend()), rfidSerialWorker, SLOT(frameToSend()));
    rfidSerialWorker->initSerialPort();

    //tests - try send frame cyclically
    QTimer *timer = new QTimer(this);
    QObject::connect(timer, SIGNAL(timeout()), rfidFrameProcessor , SLOT(update_power_ind()));
    timer->start(2000);

Function which one try send frame is below:

void RfidFrameProcessor::update_power_ind()
{
    readSourceRSx(1,9);
}
void RfidFrameProcessor::readSourceRSx(quint8 addr, quint8 source)
{

    QByteArray data;
    data.append(source);
    RfidFrame *frameToSend = new RfidFrame(addr, C_ReadSourceRSx, data);
    qDebug() << "Get lenght from frameToSend: " << frameToSend->GetDataLength();
    m_outFrameQueue->enqueue(frameToSend);
    emit frameSend();

}

and problem is in the same step:
m_outFrameQueue->enqueue(frameToSend); despite the fact that in frameToSend I have data like you can see in below in Application Output.

[jsulm]

@Damian7546 said in Create object in another object:

and problem is in the same step

And again: is m_outFrameQueue a valid pointer?