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?