need ideas for list/table implementation

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

So, regarding cross-thread communication, if the worker thread's infinite loop is blocking the event loop, does that imply that the worker won't respond to any signals at all?

Yes, exactly. Blocking the event loop means no event processing, means no queued signal-slot calls, means slots won't be called until the event processing resumes. Any signals that are connected to slots in the worker through Qt::AutoConnection, Qt::QueuedConnection, or Qt::BlockingQueuedConnection are not called. This doesn't apply to Qt::DirectConnection, because direct connections don't require an event loop and event processing - the same as a direct calls.

And if so, what's the recommended method for killing another thread?

This is the correct way of stopping the thread when you're using the worker object, you just have to rework a bit your code so it doesn't block the event loop. For example substituting the while (true) for a timer with some timeout (or zero timeout). :)

[mzimmers]

I do have a sleep in my loop:

void Worker::process()
{
    int len;
    while (true)
    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            Message msg(nullptr, buffIn);
            emit(newMessage(&msg));

            // if message is a heartbeat, send an ack.
            if (msg.getType() == MSG_HEARTBEAT)
            {
                sendHeartbeatAck();
                updateDeviceTable(msg);
            }
        }
        else
        {
            SocketState ss = sm.getSocketState();
            if (ss == SOCKET_DISCONNECTED)
            {
                //sm.init();
            }
        }
        Sleep(10);
    }
    //emit reachedEndOfThread();
}

(I renamed my worker routine from run() to process() to agree with the example I'm trying to follow.)

But this doesn't work. Are you saying it needs an actual timer (sleep() isn't any good)?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

But this doesn't work. Are you saying it needs an actual timer (sleep() isn't any good)?

sleep() makes me cringe every time. Yes, I am saying that sleep() puts the thread to sleep and it does nothing, no event processing, literally nothing. You can't even interrupt that! In your case, looking at the code, the very simple solution would be to have something like this:

class Worker : public QObject
{
public:
    Worker();

public slots:
    void start();
    void process();

private:
    QTimer pollTimer;
};

Worker::Worker()
    : pollTimer(this) //< Important so the timer is moved to the new thread along with the worker
{
    pollTimer.setInterval(10000); // 10 seconds or so
}

void Worker::start()
{
    pollTimer.start();
}

void Worker::process()
{
    int len = sm.recv(buffIn, sizeof(buffIn));
    // And more of whatever it is you do INSIDE the `while (true)`
}

Also you seem to be using the native sockets, why?

PS.
Connect the QThread::started to Worker::start and it should be working out of the box.

[mzimmers]

Thanks for the example. Don't I need to connect the timer to my connect() routine somewhere, though?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Don't I need to connect the timer to my connect() routine somewhere, though?

Yeah, I missed it, sorry about that. You should have one such line:

QObject::connect(&pollTimer, &QTimer::timeout, this, &Worker::process);

in Worker's constructor.

[mzimmers]

OK, I think I'm almost there. Now, what's the recommended behavior of the slot that receives the quit button signal? exit()? quit()? Something else?

[kshegunov]

The quit button signal you should connect to QThread::quit, just as you had shown above. That's the way to do it.

[mzimmers]

Hmm...it's not working. I read this in the QThread page:

This function [quit] does nothing if the thread does not have an event loop.

Do I need to explicitly declare/start an event loop?

[VRonin]

If you don't subclass QThread the native QThread::run starts an event loop. Can you show us your current Worker::process implementation?

[mzimmers]

Sure:

void Worker::process()
{
    int len;
//    running.ref(); // set to value of 1
//    while (true)
//    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            Message msg(nullptr, buffIn);
            emit(newMessage(&msg));

            // if message is a heartbeat, send an ack.
            if (msg.getType() == MSG_HEARTBEAT)
            {
                sendHeartbeatAck();
                updateDeviceTable(msg);
            }
        }
        else
        {
            SocketState ss = sm.getSocketState();
            if (ss == SOCKET_DISCONNECTED)
            {
                //sm.init();
            }
        }
//        Sleep(10);
//    }
    //emit reachedEndOfThread();
}

[VRonin]

emit newMessage(&msg) is a race condition, msg might (and almost surely does) go out of scope before the slots connected to the signal can use the argument

[mzimmers]

Noted, but I don't think that's causing my quit button to be ignored, is it?

I just realized my process() routine is only called once. Is there something wrong with my pollTimer?

Worker::Worker(QObject *parent) : QObject (parent), devices(parent), pollTimer(this)
{
    QObject::connect(&pollTimer, &QTimer::timeout, this, &Worker::process);
    pollTimer.setInterval(100);
}
void Worker::start()
{
    pollTimer.start();
}

[VRonin]

This is weird... can you post the entire worker.h and worker.cpp?

Hmm...it's not working.

How are you testing if it's working or not?

P.S.
No need to give parents to stack-allocated QObjects like devices or pollTimer
edit: wrong in this case, without a parent the objects will belong to the thread calling Worker constructor and not be moved with it

[mzimmers]

worker.h:

#ifndef WORKER_H
#define WORKER_H

#include "winsock2.h"
#include "ws2tcpip.h"

#include <QAtomicInt>
#include <QObject>
//#include <QThread>
#include <QTimer>

#include "constants.h"
#include "devices.h"
#include "message.h"
#include "socket.h"

namespace Ui
{
class Widget;
}

class Worker : public QObject //QThread
{
    Q_OBJECT

private:
    SOCKET sock;
    SocketMC sm;
    int myErrno;
    Message msg;
    char msgOut[1024];
    char buffIn[1024];
    Devices devices;
    QTimer pollTimer;

    void sendHeartbeatAck();
    void updateDeviceTable(Message msg);
public:
    explicit Worker(QObject *parent = nullptr);
    ~Worker();
    void readSocket();
    void doQuit();
signals:
    void newMessage(Message *msg);
    void finished();
//    void reachedEndOfThread(int rc = 0);

public slots:
    void start();
    void process();
    void sendLedPatternChange(LedPattern newState);
    void sendLedBrightnessChange(int value);
    void sendScanRequest();
};

#endif // WORKER_H

worker.cpp:

#include <iostream>
#include <string>
#include <stdio.h>

#include <QDateTime>

#include "constants.h"
#include "message.h"
#include "worker.h"

using namespace std;

Worker::Worker(QObject *parent) : QObject (parent), devices(parent), pollTimer(this)
{
    DeviceDetails dev;

//    dev.macAddr = "macaddr2";
//    dev.devName = "devname22";
//    dev.latestHB = "hbtime222";
//    devices.update(dev);

//    dev.macAddr = "macaddr55";
//    dev.devName = "devname55";
//    dev.latestHB = "hbtime55";
//    devices.update(dev);

    QObject::connect(&pollTimer, &QTimer::timeout, this, &Worker::process);
    pollTimer.setInterval(100);
}
Worker::~Worker()
{
}
void Worker::doQuit()
{
//    running.deref(); // set to value of 0.
    emit finished();
}

void Worker::start()
{
    pollTimer.start();
}

void Worker::sendLedPatternChange(LedPattern newState)
{
    Message msg;

    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_LED_SET_PATTERN]);
    msg.add(msgTags[TAGENUM_NEWPATTERN], ledPatternText[newState]);

    sm.send(msg.encodeXml());
}
void Worker::sendLedBrightnessChange(int value)
{
    Message msg;
    string s;

    s = std::to_string(value);
    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_LED_SET_BRIGHTNESS]);
    msg.add(msgTags[TAGENUM_NEWBRIGHTNESS], s);

    sm.send(msg.encodeXml());
}

void Worker::sendHeartbeatAck()
{
    Message msg;
    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_HEARTBEAT_ACK]);

    sm.send(msg.encodeXml());
}

void Worker::updateDeviceTable(Message msg)
{
    DeviceDetails dev;
    dev.macAddr = QString::fromStdString(msg.getValue(msgTags[TAGENUM_MACADDRESS]));
    dev.devName = QString::fromStdString(msg.getValue(msgTags[TAGENUM_DEVICENAME]));
    dev.latestHB = QDateTime::currentDateTimeUtc().toString(QString::fromStdString(timestampFormat));

    devices.update(dev);
}
void Worker::sendScanRequest(void)
{
    Message msg;
    msg.add(msgTags[TAGENUM_PRODUCT], PRODUCT_NAME);
    msg.add(msgTags[TAGENUM_PACKETTYPE], msgtypeText[MSG_SCAN_REQUEST]);

    sm.send(msg.encodeXml());
}
void Worker::process()
{
    cout << "worker::process() started." << endl;
    int len;
//    running.ref(); // set to value of 1
//    while (true)
//    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            msg.decodeXml(buffIn);
            emit(newMessage(&msg));

            // if message is a heartbeat, send an ack.
            if (msg.getType() == MSG_HEARTBEAT)
            {
                sendHeartbeatAck();
                updateDeviceTable(msg);
            }
        }
        else
        {
            SocketState ss = sm.getSocketState();
            if (ss == SOCKET_DISCONNECTED)
            {
                //sm.init();
            }
        }
//        Sleep(10);
//    }
    //emit reachedEndOfThread();
}

The test for whether the exit works is if the window disappears, and the debugger exits. It doesn't. The test for how often process() is called is the cout at the top of the routine.

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Noted, but I don't think that's causing my quit button to be ignored, is it?

Probably not, although it would eventually cause a segafault somewhere.

I just realized my process() routine is only called once. Is there something wrong with my pollTimer?

Not that I can see. Are you sure this:

len = sm.recv(buffIn, sizeof(buffIn));

isn't blocking? (also the reason for my question why aren't you using Qt's sockets)

[kshegunov]

@VRonin said in need ideas for list/table implementation:

No need to give parents to stack-allocated QObjects like devices or pollTimer

That is incorrect! It's quite needed.

[mzimmers]

The socket call might block, but only for a few seconds. I just stepped through the code, and discovered that worker::start() is never called. In main, I have:

    worker->moveToThread(thread);
     ...
    thread->start();

Am I supposed to explicitly call worker::start()?

No good reason for using native sockets; I'm just re-using code that I implemented on the target device (which sadly doesn't have Qt [yet]). But I'm fairly sure they're not the problem here. Remember this was (sort of) working once, before VRonin MOG'd me and I started making changes.

[kshegunov]

Am I supposed to explicitly call worker::start()?

No, not really. Do you have that:

QObject::connect(thread, &QThread::started, worker, &Worker::start);

before calling thread->start();?

[mzimmers]

Well, I didn't (the slot was process, not start), but I do now...same result.

Is there a way to see whether this slot is actually getting invoked:

    QObject::connect(&widget, &Widget::quitButtonPushed, thread, &QThread::quit);

I have verified the signal in the debugger.

EDIT:

Wait a minute: I just realized I'm doing this a little differently than I have in the past, in that the worker, not the widget is in the new thread. I think maybe I am exiting the worker, but I still have to stop the main/widget thread. Since main() isn't a Qt object, I can't use connect, so what's the recommended technique here?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Is there a way to see whether this slot is actually getting invoked:

Subclass QThread for the testing purposes and override run(). In the run implementation only do:

void MyThread::run()
{
    int ret = QThread::exec();
    qDebug() << "ret"; //< If you get to here with the debugger, then the `quit` was called.
}