2

I am working on a Qt application which involves serial communication with one or multiple devices. There are different procedures that can be executed simulteanously and each procedure may send one or unknown number of commands to a device and may receive data in response. To make it more clear, here is a graphical illustration of the scenario: enter image description here

Clicking on a button triggers the execution of the corresponding procedure. So two or more different procedures may be running at the same time when the user clicks on two or more buttons in a short interval. Actually the only thing that may be shared between them is the serial communication with a single device; otherwise they are mostly independent of one another. And here are two pseudo-code examples of what a procedure may look like:

Procedure A:

begin
write command a1 on serial port
wait for one second
perform some computations
write command a2 on serial port
wait for one second
end

Procedure B:

begin
while true:
    write command b1 on serial port
    read the response from serial port
    perform some computations
    if a condition holds return, otherwise continue
end

My solution and its issue:

To simplify the situation consider that there is only one device which we need to communicate with. Since procedures can be executed simulteanously (and only one of them can communicate with the device through serial port at a time) I have created one thread and one worker class for each of the procedures and have moved the workers to their corresponding threads. To synchronize procedures when accessing the serial port I have created one mutex:

MainWindow.h

class MainWindow : public QMainWindow {

public:
    //...
    QSerialPort*    serial_;
    QMutex      serial_mutex_;

private:
    //...
    ProcAWorker*    proca_worker;
    ProcBWorker*    procb_worker;
    ProcCWorker*    procc_worker;
    ProcDWorker*    procd_worker;

    QThread     proca_thread;
    QThread     procb_thread;
    QThread     procc_thread;
    QThread     procd_thread;

}

MainWindow.cpp

void MainWindow::onConnectButtonClicked()
{
    serial_ = new QSerialPort();
    // configure serial port settings

    serial_->open(QIODevice::ReadWrite);
}

void MainWindow::onButtonAClicked()
{
    proca_worker = new ProcAWorker(0, this);   // pass a pointer to this class to be able to access its methods and members
    proca_worker->moveToThread(&proca_thread);

    // setup worker-thread connections: started, quit, finished, etc.

    proca_thread.start();    // triggers `proccess` slot in proca_worker
}

// same thing for other buttons and procedures

ProcAWorker.cpp

void ProcAWorker::ProcAWorker(QObject *parent, QMainWindow *wnd) :
    QObject(parent), wnd_(wnd)
{

}

void ProcAWorker::process()
{
    wnd_->serial_mutex_->lock();
    wnd_->serial_->write('Command a1');   // Warning occurs in this line
    bool write_ok = client_->serial_->waitForBytesWritten(SERIAL_WRITE_TIMEOUT);
    wnd_->serial_mutex_->unlock();

    QThread::sleep(1);
    // perform some computations

    wnd_->serial_mutex_->lock();
    wnd_->serial_->write('Command a2');
    bool write_ok = client_->serial_->waitForBytesWritten(SERIAL_WRITE_TIMEOUT);
    wnd_->serial_mutex_->unlock();

    if (write_ok) {
        // signal successful to main window
        emit success();
    }
}

However, when the write operation is performed on the serial port (i.e. wnd_->serial_->write('Command a1');) the following warning is shown:

QObject: Cannot create children for a parent that is in a different thread. (Parent is QSerialPort(0x18907d0), parent's thread is QThread(0x13cbc50), current thread is QThread(0x17d8d08)

My questions:

1) I have already looked at other questions on Stackoverflow regarding this warning, but their answers have only mentioned that signal/slot should be used. I am familiar with using signal/slot to communicate with worker threads. However, I can't figure out how to implement my specific scenario (simultaneous running procedures with shared resources like serial port) using signal/slot or how can I modify my current solution to resolve this issue? Note that the procedures should be allowed to run in parallel (unless in those moments when they want to communicate with the device). Obviously one can run the procedures sequentially (i.e. one after another) but I am not looking for such solutions.

2) Actually there is also a "Halt" button that stops all the running procedures and sends a halt command to the device. But I could not figure out to implement this functionality as well (set a flag, send a quit signal, etc.). Could you please give me some hints in this regards as well?

today
  • 32,602
  • 8
  • 95
  • 115
  • Regarding the error message `"QObject: Cannot create children for..."`, you can use [`qInstallMessageHandler`](http://doc.qt.io/qt-5/qtglobal.html#qInstallMessageHandler) to install your own message handler and find out exactly where the message is coming from. – G.M. Aug 14 '18 at 10:31
  • @G.M. Thanks for mentioning that. But do you think it would help since currently I know it is raised by the `wnd_->serial_->write('Command a1');` line (or am I mistaken?!)? – today Aug 14 '18 at 10:35
  • Instead of creating a new thread for every button click you could have a thread that manages a list of function pointers (or functors in general). When the list is not empty it could remove a function from the list and then call it, This way your `onButtonAClicked()` function just needs to push the relevant function pointer onto that list whenever a key is pressed. – Galik Aug 14 '18 at 10:42
  • @today True, but the message probably isn't coming directly from the `write` call. It was really just to provide a bit more info on the underlying cause of the message. – G.M. Aug 14 '18 at 10:44
  • Note that most `QIODevice` classes are designed in such a way that you really shouldn't need to use threads. Just connect to their various [signals](http://doc.qt.io/qt-5/qiodevice.html#signals) and handle them in whatever way you require. – G.M. Aug 14 '18 at 10:47
  • @Galik Thanks. Yes, that's an improvement. So only 4 threads are created for the four procedures. But I think it still does not resolve the main issue I am facing with. – today Aug 14 '18 at 10:52
  • Well if you have only one thread rather than four you could instantiate your `QSerial` in that one thread. Would that solve the problem? – Galik Aug 14 '18 at 10:54
  • @Galik That's true, but here I have four procedures and they are allowed to be executed at the same time and they use/share the same `QSerialPort` instance. Then I need that four threads, right? – today Aug 14 '18 at 10:58
  • But I thought your four procedures must be executed in serial - one after the other? Isn't the only part that needs to happen simultaneously is placing the relevant function pointer on the queue to be processed? – Galik Aug 14 '18 at 11:00
  • It is just that this seems like a classic "producer - consumer" problem from what I can see. The main thread pushes items onto a queue and a second thread processes them in turn. – Galik Aug 14 '18 at 11:01
  • @G.M. Well, that's right but how can I handle the execution of four (possibly) simultaneous procedures without using threads? These procedures are almost independent of one another (can run in parallel) and only communicate with an external device through a shared resource (i.e. `QSerialPort`)? – today Aug 14 '18 at 11:05
  • @Galik see my last comment in response to G.M. – today Aug 14 '18 at 11:07
  • Ah I mistook. I thought all the commands for each button needed to use the serial port before allowing another. – Galik Aug 14 '18 at 11:11
  • Sorry, I've also misunderstood your intent somewhat. So, regarding the error message, it appears that the call to `QSerialPort::write` is causing another `QObject` to be instantiated with the `QSerialPort` as its parent. But because `write` is being called from a thread *other* than that returned by `wnd_->serial_->thread()` that's not allowed -- hence the message. So `QSerialPort::write` can only be called from the thread associated with the `QSerialPort` instance itself. – G.M. Aug 14 '18 at 12:24
  • @G.M. No problem :) Yes, I know that; but I can't find any other way to call QSerialPort in its own thread... unless I create a serial worker and move that to a new thread; then create signals and slots for communication between the serial worker and the procedure workers (like `write_data`, `read_data`, `write_finished`, `read_finished`, etc.). But it makes it fairly complicated. I need to convert the procedures to a kind of state machine: +state1: write `command a1` to serial port using the `write_data` signal, +state2: (waiting for) `write_finished` signal from serial worker, etc.??!! – today Aug 14 '18 at 12:39

1 Answers1

1

First of all, you don't need explicit multithreading (it's optional), second of all you don't need any manually managed synchronization primitives.

Then, model each procedure using a state machine. Hopefully the communication protocol allows each procedure recognize the responses to its own commands, so that even though you'd be replicating the incoming data to all of the procedures, they'd ignore the data irrelevant to them.

This answer has a sketch of a solution that does exactly what you want, sans multiplexing. Multiplexing a QIODevice is trivial when you expose it via local pipes: everything incoming from the port is written to one end of one or more local pipes. Everything incoming from the pipes is written to the port. The pipes will maintain the integrity of the packets as long as you open their procedure end in Unbuffered mode. That way each write will arrive at the serial port as a contiguous block of bytes, and will be written to the port in the same manner.

How would you multiplex? Like so:

class IODeviceMux : public QObject {
  Q_OBJECT
  QVector<QPointer<AppPipe>> m_portPipes;
  QVector<QPointer<AppPipe>> m_userPipes;
  QPointer<QSerialPort> m_port;
public:
  IODeviceMux(QObject *parent = {}) : QObject(parent) {}
  void setPort(QIODevice *port) {
    if (m_port) {
      disconnect(m_port.get(), 0, this, 0);
      m_userPipes.removeAll({});
      for (auto pipe : qAsConst(m_userPipes))
        disconnect(m_port.get(), 0, pipe.get(), 0);
    }
    m_port = port;
    connect(m_port.get(), &QIODevice::readyRead, this, &IODeviceMux::onPortRead);
  }
  AppPipe *getPipe() {
    QScopedPointer<AppPipe> user(new AppPipe(QIODevice::ReadWrite | QIODevice::Unbuffered));
    auto *port = new AppPipe(QIODevice::ReadWrite | QIODevice::Unbuffered, this);
    user->addOther(port);
    connect(port, &QIODevice::readyRead, this, &IODeviceMux::onPipeRead);
    connect(m_port.get(), &QIODevice::bytesWritten, user.get(), &QIODevice::bytesWritten);
    connect(user, &QObject::destroyed, port, &QObject::deleteLater);
    m_userPipes.push_back(user.get());
    m_portPipes.push_back(port);
    return user.take();
  } 
private:
  void onPortRead() {
    if (!m_port) return;
    auto data = m_port->readAll();
    m_portPipes.removeAll({});
    for (auto pipe : qAsConst(m_portPipes))
      pipe->write(data);
  }
  void onPipeRead() {
    auto *pipe = qobject_cast<AppPipe*>(sender());
    QByteArray data;
    if (pipe) data = pipe->readAll();
    if (m_port) m_port->write(data);
  }
};

The procedures would each getPipe() and treat the pipe as if it was a serial port device. Each write into a pipe gets faithfully executed on the port. Each readyRead on the port is faithfully forwarded, with same data amounts available immediately to read. Even the port's bytesWritten is forwarded. But bytesToWrite doesn't work - it always returns zero. This could be fixed by adding an option to AppPipe to query this value.

That's about all you need to get it to work, I'd think.

Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • Thanks. I considered implementing procedures as state machines but I was not sure and thought there must be a better solution (it seems there isn't). So there is no problem with that part. However, for the serial communication I have another design in my mind which I am not sure about: create a serial worker class and move that to a new thread. Then create signals/slots like `write_data` and `read_data` which serial worker and procedure workers use to communicate with each other. This way it is guaranteed that only one procedure has access to serial port at a time (since the event loop >>> – today Aug 15 '18 at 06:52
  • >>> of serial worker processes the events one by one). So the problem of shared resource is solved. Only one problem remains which you mentioned: the communication protocol does not distinguish the response of different procedures. To resolve this I thought we could pass an additional argument, like a unique procedure id, to the write signal emitted by each procedure. This way the response which is sent back by the serial worker as another signal, could have this id as one of its argument as well and it would be ignored by the irrelevant procedures. What do you think about this design? – today Aug 15 '18 at 06:58
  • Moving the serial communication to a worker class has the benefit of keeping GUI responsive. – today Aug 15 '18 at 08:26
  • 1
    @today When the serial communication is fully asynchronously handled, then it will have minimal gui impact. Of course you want it out of the UI thread, but it can be in another thread that handles communications in general, and definitely you don't need a thread per procedure or per port or anything like that. – Kuba hasn't forgotten Monica Aug 15 '18 at 21:29
  • Any protocol handling overlapped request execution should accept a request id, and that request id must be included with each response to that request. IP protocols like UDP and TCP/IP handle this via port numbers :) Whether those request identifiers are procedure ids or something else is up to you - don't tie your protocol to it. It should be an opaque value that the responses copy from the request. Its length is up to you - whether 8, 16 or 32 bytes. – Kuba hasn't forgotten Monica Aug 15 '18 at 21:35
  • Thanks again for the comments. So you think that the solution I mentioned (i.e. signals/slots between serial worker and procedure workers) can be used instead of pipe solution you provided, right? And just one more thing: why do you think that there is no need for a thread per procedure? If procedures need to be (possibly) executed simultaneously then how we can achieve it without threads? Considering that this (i.e. possibility of simultaneous execution) is something the problem statement somehow impose on and ask from us. – today Aug 15 '18 at 22:04
  • 1
    Think of it like this: a UI has a number of procedures (reponses) to every event that might happen - a user pressing a button, time passing in an animation, etc. Your procedures do the same thing: they respond to events, i.e. data becoming available, or time passing, etc. They do the chunk of work and return control to the event loop. They never block. – Kuba hasn't forgotten Monica Aug 15 '18 at 22:44
  • I see. As you mentioned they are almost non-blocking; specially that makes sense with the new design. However, what if the procedures perform some (long) computations based on the events and responses they get from serial communication? Doesn't that make them eligible for a new exclusive thread? – today Aug 16 '18 at 06:14
  • 1
    Long computations should be done concurrently using `QtConcurrent::run`, so that you won't be doing thread management manually for what amounts to batch compute jobs. Machines are better at it :) After a computation is done, a future watcher's signal gets the procedure moving ahead again. It makes sense to have a dedicated thread for all of the communications, but having many of them just because you occasionally have long-running computations is wasteful - those threads will be mostly sitting idle, and that's just wasting RAM for no good reason. – Kuba hasn't forgotten Monica Aug 16 '18 at 06:57