need ideas for list/table implementation

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.
}

kshegunov

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

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?

You have QApplication for that purpose and you can get it from anywhere with QCoreApplication::instance(). Connect the quit button to the application quit slot as well as to the thread quit slot. Don't forget to wait for the worker thread to finish (call QThread::wait) before exiting the application.

VRonin

emit newMessage(&msg) is (probably) still a race condition, msg.decodeXml might (and almost surely does) get called before the slots connected to the signal had the time to read the last one. Either serialise access inside Message via stuff like QReadWriteLock or emit passing by value

mzimmers

@kshegunov so my exit button now triggers two calls:

    QApplication a(argc, argv);
    Widget widget;
    QThread* thread = new QThread;
  ...
    QObject::connect(&widget, &Widget::quitButtonPushed, thread, &QThread::quit);
    QObject::connect(&widget, &Widget::quitButtonPushed, &a, &QApplication::quit);

Regarding QThread::wait(), according to the docs:

Blocks the thread until either of these conditions is met

(where one of the conditions is the thread finishes)

So, presumably, what documentation means is that the routine blocks the deletion of the thread, not the thread itself, right? So I can call the wait() routine like this?

    widget.show();
    thread->start();
    thread->wait();
    rc = a.exec();

EDIT:

wrong.

OK, so where should the thread wait call go?

VRonin

You can connect &QThread::finished insetad of &Widget::quitButtonPushed to &QApplication::quit for the application to wait for the thread to finish before closing down

mzimmers

Perfect. Thanks for the help on threads.

And speaking of threads, this one has gone afield enough that I think it's best to consider it finished. Thanks to everyone who provided input on this.