Optimizing regular QCustomPlot

lukutis222

@jsulm
Yep you are right. Calling update is not necessary. Calling replot is enough.

I have adjusted my code and fixed this issue but as you have mentioned, this is not going to solve the delay problem.

I will ask QCustomPlot community as you have suggested.

Do you think placing plot_voltage_current in seperate thread would be smart idea? I do not mind that plot_voltage_current take some time to execute, I only care about the fact that it does prevent my readData to capture the samples as the plot is being executed.

jsulm

@lukutis222 said in Optimizing regular QCustomPlot:

Do you think placing plot_voltage_current in seperate thread would be smart idea?

No, accessing UI from other threads than UI/main thread is not supported. If you want to move something to another thread than that should be the data receiving part.

lukutis222

@jsulm
I see. I dont think that would do anything in this particular case since I know my application is more than capable of receiving and parsing samples at 50ms intervals within reasonable accuracy. It is just the plotting that causing the issues. Il post back an update here if I find anything

jsulm

@lukutis222 said in Optimizing regular QCustomPlot:

I dont think that would do anything in this particular case since I know my application is more than capable of receiving and parsing samples at 50ms

It will make a difference, because now plotting blocks receiving because both run in the same thread.
But it will not solve the actual issue: slow rendering. At some point application will become unresposive because it is busy with plotting.

lukutis222

@jsulm

Yeah I guess you are right. I will try to implement my readData in method in seperate thread and see how it goes. Regarding the slow rendering, il ask around in QCustomPlot community`

lukutis222

@jsulm

Hello again. I have been reading and learning a little bit about QThread. I have came up with a potential solution..

I have created a serialworker class which connects to readyRead . This class responsibility is to receive and parse serial data and then append it to the Vectors that are public members of a class which will then later be used by my MainWindow class to replot the graph.

Some code of the serialworker:

SerialWorker::SerialWorker(QObject *parent)
    : QObject{parent}
{
    serial = new QSerialPort(this);
    connect(serial, &QSerialPort::readyRead, this, &SerialWorker::readData);


    automatic_detection.detection_flag = 0;
    automatic_detection.current_device_id = 0;
    automatic_detection.command = "uCurrent?";
    automatic_detection.response = "uCurrent_OK";
    automatic_detection.max_retries = 3;
    automatic_detection.retry_count = 0;
    automatic_detection.timeout = 400;

    available_devices.resize(10);
    //timer_detect = new QTimer(this);
    //connect(timer_detect, &QTimer::timeout, this, &SerialWorker::detect_timeout);




    // set default sampling info
    sampling_info.sampling_time = UNLIMITED;
    sampling_info.sampling_frequency = 1; // 1HZ default // set 100
    sample_counter = 0;
    sample_counter_1_sec = 0;

    current_1_min_avg.resize(60);
    current_30_min_avg.resize(30);
    current_60_min_avg.resize(60);
    current_24_hr_avg.resize(1440);

}


void SerialWorker::readData() {
    QByteArray data = serial->readAll();
    switch(data[0])
    {
        case(0x01):
        {
            uint32_t voltage_data;
            uint32_t current_data;

            voltage_data = (uint8_t)data[2] << 24 | (uint8_t)data[3] << 16 | (uint8_t)data[4] << 8 | (uint8_t)data[5];
            current_data = (uint8_t)data[6] << 24 | (uint8_t)data[7] << 16 | (uint8_t)data[8] << 8 | (uint8_t)data[9];
            float voltage = float(voltage_data/1000.0f);
            float current = float(current_data/1000.0f);


            addPoint_current(double(double(sample_counter) / double(20)), current);
            addPoint_voltage(double(double(sample_counter) / double(20)), voltage);
            sample_counter++;
            sample_counter_1_sec = sample_counter / 20;
            qDebug()<<QDateTime::currentMSecsSinceEpoch();
            return;
        }
    }
}

As you can see, in the readData method, I am receiving serial data and calling addPoint_current and addPoint_voltage methods to append the data to the Vectors that are public variables of serialworker class.

//Append list of qv_x and qv_y which contains all current meaurement points
void SerialWorker::addPoint_current(double x, double y)
{
    qv_x.append(x);
    qv_y.append(y);
}


//Append list of qv_x_voltage and qv_y_voltage which contains all voltage measuremen points
void SerialWorker::addPoint_voltage(double x, double y)
{
    qv_x_voltage.append(x);
    qv_y_voltage.append(y);
}

In my mainwindow.cpp I have the following code to instantiate serialworker class and place it another thread using moveToThread

  serial_worker = new SerialWorker();
    QThread *workerThread = new QThread(this);
    serial_worker->moveToThread(workerThread);
    connect(workerThread, &QThread::started, this, &MainWindow::on_detect_button_clicked);
    connect(this, &MainWindow::finished, serial_worker, &SerialWorker::Serial_disconnect);
    connect(this, &MainWindow::finished, workerThread, &QThread::quit);
    connect(workerThread, &QThread::finished, serial_worker, &QObject::deleteLater);
    connect(this, &MainWindow::finished, serial_worker, &SerialWorker::Serial_disconnect);
    workerThread->start();

And in my Mainwindow.cpp I also have update_graph function (same as before) which is accessing public members of serialworker class to replot the graph:

//This triggers every 1 second
void MainWindow::update_graph()
{
   qDebug("Plot \n");
   ui->customPlot->graph(0)->addData(serial_worker->qv_x, serial_worker->qv_y);
   ui->customPlot->graph(1)->addData(serial_worker->qv_x_voltage, serial_worker->qv_y_voltage);
   ui->customPlot->replot();
}

I have tested this out and I can confirm that this did not solve the issue. Calling update_graph method stops the serialworker class from receiving and parsing data even though it is running in a seperate thread. This is a serial log after about 10 minutes of running the program:

As you can see from the log, when it is time to plot the graph, it prevents serialworker to receive data which results in loosing samples. So even though I have moved data parsing into a seperate thread, the results are exactly the same as my Initial method..
Perhaps I am misunderstanding something? Could you give some insights?

Bob64

@lukutis222 said in Optimizing regular QCustomPlot:

As you can see from the log, when it is time to plot the graph, it prevents serialworker to receive data which results in loosing samples.

I don't understand how this is happening if your data collection work is being done on another thread. You will need to figure out what is really happening here.

Edit: what JonB says below makes a lot of sense. I hadn't read your code carefully enough to see how you were communicating the data updates.

JonB

@lukutis222
I don't know about your behaviour, but it seems to me it ought be even slower, if you write it correctly!

In MainWindow::update_graph() you add data to the graph from/read serial_worker->qv_y/qv_y_voltage etc. At the same time SerialWorker::addPoint_current() may be writing to those (qv_x.append(x) etc.). Where is your "mutex" or similar? So even if this approach were to be faster I think it is "illegal/undefined" and by the time you put in a mutex it would only be slower?

This may not end up being the fastest way but I would start with a basic, legal way:

Worker thread receives data and constructs list of new points to be added in its own variable.
From time to time it sends a signal, which copies the data, to a main thread slot which adds the plots to the graph. Worker clears out its copy of sent points and starts afresh.

I would get that working to make sure the worker never misses a data input and the main ends up with all the points plotted. That ought work. After that work on how you might optimize the speed.

Alternatively, a non-thread possibility might be: get serial data in main thread but make plot_voltage_current() only addData() a chunk of them at a time, on a timer. But you would need to discover how safe a "chunk" size is OK.

BTW, I know nothing about QCustomPlot but my guess is that customPlot->replot()/update() are what is really slow? Do you really need those, does it do this after addData() maybe when it hits the event loop if left to its own devices? Or, maybe, if you do do the addData()s every time but only do the replot/update on a periodic timer does that improve behaviour?

lukutis222

@JonB
You are correct about me not having a mutex where I am trying to access the same data. This is not correct but for the time being I just wanted to test out the multithreading to see how they perform.

The fact that I do not have mutex and my solution is not very refined does not have anything to do with the fact that I now have 2 seperate threads running and somehow the plotting (that runs in the main thread) is stopping the other thread that is responsible for receiving the data.

@JonB said in Optimizing regular QCustomPlot:

This may not end up being the fastest way but I would start with a basic, legal way:

Worker thread receives data and constructs list of new points to be added in its own variable.
From time to time it sends a signal, which copies the data, to a main thread slot which adds the plots to the graph. Worker clears out its copy of sent points and starts afresh.

What you described here is very simillar to my current approach except not so much refined. I am collecting data in my worker thread and appending it to a vector. Then instead of sending a signal from time to time, I have a timer running on a main thread every 1 second to perform Plotting.

There are many methods to achieve this as well as 2 methods that you have suggested, but I am now very curious about the issue that I have with my current solution as this seem very strange. How can update_graph method that runs in the main thread stop the other thread serialwork from receiving serial data

I will try to simplify this even further and instead of actually plotting the data inside update_graph I can use some other operation or function that takes a while to execute to test whether the execution of update_graph slows down the other thread. I do not think this has anything to do with QCustomPlot specifically

lukutis222

UPDATE

As I have mentioned in my earlier post, I was going to simplify update_graph and instead of plotting (lets take QCustomPlot away from the equation), I can use QObject().thread()->usleep in my main thread and it should not affect the serialworker thread right?

I have updated my update_graph function:

void MainWindow::update_graph()
{
    QObject().thread()->usleep(1000*1000*0.5);
   qDebug("Plot \n");
   //qDebug()<<(this->thread());
   //plot_voltage_current(ui->customPlot);
}

Now all it does it is being triggered by timer every 1 second and it sleeps for 0.5 seconds. Even though this thread is sleeping for 0.5 seconds, the other thread should continue running without any issues right?

But I can see that this is not how it actually works:

As you can see from the log above, I am receiving the serial data periodically every 50ms (20Hz frequency), and everytime the update_graph is triggered, the readData is stopped for 0.5 seconds.

Is this expected behaviour or am I not understanding something?

jsulm

@lukutis222 said in Optimizing regular QCustomPlot:

connect(workerThread, &QThread::started, this, &MainWindow::on_detect_button_clicked);

Why are you starting MainWindow::on_detect_button_clicked in your worker thread?!
You need to start a method of your worker class in the thread.
SerialWorker should also allocate everything it needs in that method (not in constructor) to make sure everything lives in the worker thread.

lukutis222

@jsulm
Wow well spotted. This was totally the issue. I did on_detect_button_clicked because as soon as worker thread is started, I call that on_detect_button_clicked method which does the following:

void MainWindow::on_detect_button_clicked()
{
    ui->uCurrent_detected_label->setText("uCurrent being detected");
    ui->uCurrent_detected_label->setStyleSheet("QLabel {color : rgb(255, 165, 00); font : 700 }");
    if (serial_worker->Scan_serial_devices() > 0)
    {
        serial_worker->Automatic_detect(0);
    }
    else
    {
        ui->uCurrent_detected_label->setText("uCurrent not detected");
        ui->uCurrent_detected_label->setStyleSheet("QLabel {color : rgb(255, 0, 00); font : 700 }");
    }
}

It is intended to call serial_worker public methods to automatically find the and connect to the correct serialport.

If I comment out this line:

connect(workerThread, &QThread::started, this, &MainWindow::on_detect_button_clicked);

and I manually connect to the serial port in the worker class constructor (just for testing purposes):

SerialWorker::SerialWorker(QObject *parent)
    : QObject{parent}
{
    serial = new QSerialPort(this);
    connect(serial, &QSerialPort::readyRead, this, &SerialWorker::readData);


    automatic_detection.detection_flag = 0;
    automatic_detection.current_device_id = 0;
    automatic_detection.command = "uCurrent?";
    automatic_detection.response = "uCurrent_OK";
    automatic_detection.max_retries = 3;
    automatic_detection.retry_count = 0;
    automatic_detection.timeout = 400;

    available_devices.resize(10);
    //timer_detect = new QTimer(this);
    //connect(timer_detect, &QTimer::timeout, this, &SerialWorker::detect_timeout);




    // set default sampling info
    sampling_info.sampling_time = UNLIMITED;
    sampling_info.sampling_frequency = 1; // 1HZ default // set 100
    sample_counter = 0;
    sample_counter_1_sec = 0;

    current_1_min_avg.resize(60);
    current_30_min_avg.resize(30);
    current_60_min_avg.resize(60);
    current_24_hr_avg.resize(1440);

    serial->setPortName("COM3");  // Setting the port name to COM3
    serial->setBaudRate(QSerialPort::Baud115200);  // Setting the baud rate to 115200
    serial->setDataBits(QSerialPort::Data8);
    serial->setStopBits(QSerialPort::OneStop);
    serial->setParity(QSerialPort::NoParity);
    serial->setFlowControl(QSerialPort::NoFlowControl);

    Serial_connect();



}

Everything works as expected! My update_graph function no longer stops the readData function.

Thank you very much, although I am not so sure why that happened. Perhaps you can clarify:

Why I cannot do:

connect(workerThread, &QThread::started, this, &MainWindow::on_detect_button_clicked);

in my mainwindow.cpp ? How exactly it affects my serialworker class?

@jsulm said in Optimizing regular QCustomPlot:

SerialWorker should also allocate everything it needs in that method (not in constructor) to make sure everything lives in the worker thread.

Can you clarify a little bit what you mean by that?

jsulm

@lukutis222 said in Optimizing regular QCustomPlot:

How exactly it affects my serialworker class?

You were calling serial_worker->Automatic_detect(0) in main thread, my guess is that then also the signals from the serial port you connected in serial_worker->Automatic_detect(0) were executed in main thread (I mean the slots were executed in main thread).

"Can you clarify a little bit what you mean by that?" - everything created in the constructor of the worker thread will live in the main thread even after moveToThread. Only objects derived from QObject with "this" as parent will also be moved to worker thread when their parent ("this") is moved to worker thread. I think in your case it is fine because you set "this" as parent in QSerialPort.

lukutis222

@jsulm

Thank you very much.

So since I now have a seperate thread serialworker running. I need to be able to update various ui elements based on the serial data received. It has been earlier mentioned that updating the ui via other threads is not currently supported in the QT hence I am looking for another optimal method.

Do you think this method is appropriate:

void SerialWorker::readData() {
    QByteArray data = serial->readAll();
    emit dataReceived(data);
}

And in my mainwindow.cpp

    connect(serial_worker, &SerialWorker::dataReceived, this, &MainWindow::processData);

void MainWindow::processData(const QByteArray &data){
    switch(data[0])
    {
        case(0x01):
        {
            uint32_t voltage_data;
            uint32_t current_data;

            voltage_data = (uint8_t)data[2] << 24 | (uint8_t)data[3] << 16 | (uint8_t)data[4] << 8 | (uint8_t)data[5];
            current_data = (uint8_t)data[6] << 24 | (uint8_t)data[7] << 16 | (uint8_t)data[8] << 8 | (uint8_t)data[9];
            float voltage = float(voltage_data/1000.0f);
            float current = float(current_data/1000.0f);


            addPoint_current(double(double(sample_counter) / double(20)), current);
            addPoint_voltage(double(double(sample_counter) / double(20)), voltage);
            sample_counter++;
            sample_counter_1_sec = sample_counter / 20;
            qDebug()<<QDateTime::currentMSecsSinceEpoch();
            break;
        }
        case(0x02):
        {
                ui->flash_label->setText("Flashing");
                break;
         }
        case(0x03):
        {
                ui->flash_label->setText("Flashing complete");
                break;
        }
    }
}

This way, I can still benefit of receiving serial data in a different thread but I also have an advantage since I can easily access the ui to update various elements in my application because I am on mainwindow class. Is using slot processData correct in this particular example? I am slightly confused whether I can make processData run on a different thread (same thread as serialworkereven though it is not part of serialworker class.

If it will not be running on a different thread, that means that even if my serialworker is able to send a signal at regular intervals the processData will still lag behind due to running on main thread that is being used by plotting the graph.

Pl45m4

@lukutis222 said in Optimizing regular QCustomPlot:

QThread *workerThread = new QThread(this);
serial_worker->moveToThread(workerThread);
connect(workerThread, &QThread::started, this, &MainWindow::on_detect_button_clicked);
connect(this, &MainWindow::finished, serial_worker, &SerialWorker::Serial_disconnect);
connect(this, &MainWindow::finished, workerThread, &QThread::quit);
connect(workerThread, &QThread::finished, serial_worker, &QObject::deleteLater);
connect(this, &MainWindow::finished, serial_worker, &SerialWorker::Serial_disconnect);

Hi, I would also remove the parent-child coupling of your MainWindow and your secondary thread managing QThread object .

QThread *workerThread = new QThread();
// then, delete QThread when done using this connection:
// ICYW: QThread::quit() will emit QThread::finished() before exiting
connect( workerThread, &QThread::finished, workerThread, &QThread::deleteLater );

Or, as the snippet from the documentation even shows, you can put your workerThread as a MainWindow member variable on stack. Then there's no need to worry about its deletion.

Even if it makes no difference in your case, but to maintain consistency, I would change

// this:
connect(workerThread, &QThread::finished, serial_worker, &QObject::deleteLater);
// to this:
connect(workerThread, &QThread::finished, serial_worker, &SerialWorker::deleteLater);

Edit:

@lukutis222 said in Optimizing regular QCustomPlot:

float voltage = float(voltage_data/1000.0f);
float current = float(current_data/1000.0f);
addPoint_current(double(double(sample_counter) / double(20)), current);
addPoint_voltage(double(double(sample_counter) / double(20)), voltage);

Holy mother of C cast .... (╯°□°)╯︵ ┻━┻
:D

lukutis222

@Pl45m4 Thanks

@Pl45m4 said in Optimizing regular QCustomPlot:

Holy mother of C cast .... (╯°□°)╯︵ ┻━┻
:D

Yeah I guess thats a bit extra :D Il get it fixed :D

JonB

@lukutis222
Not that it matters, but you don't need the outer cast, and only one of the inner casts, e.g.

addPoint_current(double(sample_counter) / 20, current);

would deliver the same.

SimonSchroeder

@lukutis222 said in Optimizing regular QCustomPlot:

Why I cannot do:

connect(workerThread, &QThread::started, this, &MainWindow::on_detect_button_clicked);

in my mainwindow.cpp ? How exactly it affects my serialworker class?

You have to keep in mind that the MainWindow (the this pointer in your case) lives inside the main thread and not the worker thread. The third argument to connect is the context object. If the sender and receiver are not inside the same thread, the slot call is automatically queued into the thread of the receiver or context object. To be fair, I assume that the constructor of SerialWorker is most likely also called inside the main thread before it is moved to the worker thread. If that is the case it shouldn't matter which way you are doing things. However, the constructor of SerialWorker is most likely started before the worker thread starts and thus is completely initialized. Whereas things inside the worker thread and on_detect_button_clicked might run in parallel.

@lukutis222 said in Optimizing regular QCustomPlot:

It has been earlier mentioned that updating the ui via other threads is not currently supported in the QT hence I am looking for another optimal method.

The best solution is to use signals and slots. This could be little finer control than what you have already suggested. Instead of sending the full data to the GUI thread for processing, you could do some processing in the worker thread. For example, you could emit a signal carrying a QString which would be connected to ui->flash_label->setText. I hate writing additional signals just for something short and simple like this. Instead I use QMetaObject::invokeMethod(qApp, { ... }); with a lambda to post something to the GUI thread. I have collected a few scenarios of communicating between a worker thread and the GUI thread into a small header-only library: https://github.com/SimonSchroeder/QtThreadHelper