Create object in another object

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?

Damian7546

@jsulm In my opinion yes, but I am beginner.... Please explain me.
.h

class RfidFrameProcessor : public QObject
{
    Q_OBJECT
public:
    explicit RfidFrameProcessor(QQueue<RfidFrame*> *outFrameQueue, QObject *parent = nullptr);
private:
    QQueue<RfidFrame*> *m_outFrameQueue;
}

.cpp

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

jsulm

@Damian7546 said in Create object in another object:

m_outFrameQueue = outFrameQueue;

I laready asked that before: is outFrameQueue a valid pointer?

Damian7546

Ok,
I create in this way:

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

But in class

   Q_OBJECT
public:
    explicit RfidFrameProcessor(QQueue<RfidFrame*> *outFrameQueue, QObject *parent = nullptr);
private:
    QQueue<RfidFrame*> *m_outFrameQueue;

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?

Damian7546

Thanks QQueue<RfidFrame*> outFrameQueue i wolud like to pass frame to send beetwen RfidSerialWorker and RfidFrameProcessor.

@JonB said in Create object in another object:

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?

Exactly. But how do this ?

jsulm

@Damian7546 said in Create object in another object:

But how do this ?

Make outFrameQueue member variable instead of local variable.

Damian7546

sure!! Thank you !