Sending a sequence of commands and wait for response
Asked Answered
J

2

12

I have to update firmware and settings on a device connected to a serial port. Since this is done by a sequence of commands, I send a command and wait until I recive an answer. Inside the answere (many lines) I search for a string that indicates if the operation is finished successfully.

Serial->write(“boot”, 1000);
Serial->waitForKeyword(“boot successful”);
Serial->sendFile(“image.dat”);
…

So I’ve created a new Thread for this blocking read/write method. Inside the thread I make use of the waitForX() functions. If I call watiForKeyword() it will call readLines() until it detects the keyword or timesout

bool waitForKeyword(const QString &keyword)
{
    QString str;

    // read all lines
    while(serial->readLines(10000))
    {
        // check each line
        while((str = serial->getLine()) != "")
        {
            // found!
            if(str.contains(keyword))
                return true;
        }
    }
    // timeout
    return false;
}

readLines() reads everything available and separates it into lines , each line is placed inside a QStringList and to get a string I call getLine() which returns the first string in the list and deletes it.

bool SerialPort::readLines(int waitTimeout)
{
if(!waitForReadyRead(waitTimeout))
{
    qDebug() << "Timeout reading" << endl;
    return false;
}

QByteArray data = readAll();
while (waitForReadyRead(100))
    data += readAll();

char* begin = data.data();
char* ptr = strstr(data, "\r\n");

while(ptr != NULL)
{
    ptr+=2;
    buffer.append(begin, ptr - begin);
    emit readyReadLine(buffer);
    lineBuffer.append(QString(buffer)); // store line in Qstringlist
    buffer.clear();

    begin = ptr;
    ptr = strstr(begin, "\r\n");
}
// rest
buffer.append(begin, -1);
return true;
}

The problem is if I send a file via terminal to test the app readLines() will only read a smale part of the file ( 5 Lines or so). Since these lines do not contain the keyword. the function will run once again, but this time it dosnt wait for timeout, readLines just return false immediately. Whats wrong ? Also I'm not shure if this is the right approach... Does anyone know how to send a sequenze of commands and wait for a response each time?

Jaffe answered 9/9, 2015 at 17:50 Comment(1)
Without knowing what the Serial class is, your original question of why it was ignoring the rest of the file cannot be answered. However, note that in linux serial port devices do not behave like sockets with regard to non-blocking IO, so this might be why. (You basically can't use non-blocking I/O with serial ports, that's why the official QSerialPort class, which was added in Qt 5.1, simulates async communication with a thread.)Bacchic
S
23

Let's use QStateMachine to make this simple. Let's recall how you wished such code would look:

Serial->write("boot", 1000);
Serial->waitForKeyword("boot successful");
Serial->sendFile("image.dat");

Let's put it in a class that has explicit state members for each state the programmer could be in. We'll also have action generators send, expect, etc. that attach given actions to states.

// https://github.com/KubaO/stackoverflown/tree/master/questions/comm-commands-32486198
#include <QtWidgets>
#include <private/qringbuffer_p.h>
#include <type_traits>

[...]

class Programmer : public StatefulObject {
   Q_OBJECT
   AppPipe m_port { nullptr, QIODevice::ReadWrite, this };
   State      s_boot   { &m_mach, "s_boot" },
              s_send   { &m_mach, "s_send" };
   FinalState s_ok     { &m_mach, "s_ok" },
              s_failed { &m_mach, "s_failed" };
public:
   Programmer(QObject * parent = 0) : StatefulObject(parent) {
      connectSignals();
      m_mach.setInitialState(&s_boot);
      send  (&s_boot, &m_port, "boot\n");
      expect(&s_boot, &m_port, "boot successful", &s_send, 1000, &s_failed);
      send  (&s_send, &m_port, ":HULLOTHERE\n:00000001FF\n");
      expect(&s_send, &m_port, "load successful", &s_ok, 1000, &s_failed);
   }
   AppPipe & pipe() { return m_port; }
};

This is fully functional, complete code for the programmer! Completely asynchronous, non-blocking, and it handles timeouts, too.

It's possible to have infrastructure that generates the states on-the-fly, so that you don't have to manually create all the states. The code is much smaller and IMHO easier to comperehend if you have explicit states. Only for complex communication protocols with 50-100+ states would it make sense to get rid of explicit named states.

The AppPipe is a simple intra-process bidirectional pipe that can be used as a stand-in for a real serial port:

// See https://mcmap.net/q/1007635/-is-there-an-intra-process-local-pipe-in-qt
/// A simple point-to-point intra-process pipe. The other endpoint can live in any
/// thread.
class AppPipe : public QIODevice {
  [...]
};

The StatefulObject holds a state machine, some basic signals useful for monitoring the state machine's progress, and the connectSignals method used to connect the signals with the states:

class StatefulObject : public QObject {
   Q_OBJECT
   Q_PROPERTY (bool running READ isRunning NOTIFY runningChanged)
protected:
   QStateMachine m_mach  { this };
   StatefulObject(QObject * parent = 0) : QObject(parent) {}
   void connectSignals() {
      connect(&m_mach, &QStateMachine::runningChanged, this, &StatefulObject::runningChanged);
      for (auto state : m_mach.findChildren<QAbstractState*>())
         QObject::connect(state, &QState::entered, this, [this, state]{
            emit stateChanged(state->objectName());
         });
   }
public:
   Q_SLOT void start() { m_mach.start(); }
   Q_SIGNAL void runningChanged(bool);
   Q_SIGNAL void stateChanged(const QString &);
   bool isRunning() const { return m_mach.isRunning(); }
};

The State and FinalState are simple named state wrappers in the style of Qt 3. They allow us to declare the state and give it a name in one go.

template <class S> struct NamedState : S {
   NamedState(QState * parent, const char * name) : S(parent) {
      this->setObjectName(QLatin1String(name));
   }
};
typedef NamedState<QState> State;
typedef NamedState<QFinalState> FinalState;

The action generators are quite simple, too. The meaning of an action generator is "do something when a given state is entered". The state to act on is always given as the first argument. The second and subsequent arguments are specific to the given action. Sometimes, an action might need a target state as well, e.g. if it succeeds or fails.

void send(QAbstractState * src, QIODevice * dev, const QByteArray & data) {
   QObject::connect(src, &QState::entered, dev, [dev, data]{
      dev->write(data);
   });
}

QTimer * delay(QState * src, int ms, QAbstractState * dst) {
   auto timer = new QTimer(src);
   timer->setSingleShot(true);
   timer->setInterval(ms);
   QObject::connect(src, &QState::entered, timer, static_cast<void (QTimer::*)()>(&QTimer::start));
   QObject::connect(src, &QState::exited,  timer, &QTimer::stop);
   src->addTransition(timer, SIGNAL(timeout()), dst);
   return timer;
}

void expect(QState * src, QIODevice * dev, const QByteArray & data, QAbstractState * dst,
            int timeout = 0, QAbstractState * dstTimeout = nullptr)
{
   addTransition(src, dst, dev, SIGNAL(readyRead()), [dev, data]{
      return hasLine(dev, data);
   });
   if (timeout) delay(src, timeout, dstTimeout);
}

The hasLine test simply checks all lines that can be read from the device for a given needle. This works fine for this simple communications protocol. You'd need more complex machinery if your communications were more involved. It is necessary to read all the lines, even if you find your needle. That's because this test is invoked from the readyRead signal, and in that signal you must read all the data that fulfills a chosen criterion. Here, the criterion is that the data forms a full line.

static bool hasLine(QIODevice * dev, const QByteArray & needle) {
   auto result = false;
   while (dev->canReadLine()) {
      auto line = dev->readLine();
      if (line.contains(needle)) result = true;
   }
   return result;
}

Adding guarded transitions to states is a bit cumbersome with the default API, so we will wrap it to make it easier to use, and to keep the action generators above readable:

template <typename F>
class GuardedSignalTransition : public QSignalTransition {
   F m_guard;
protected:
   bool eventTest(QEvent * ev) Q_DECL_OVERRIDE {
      return QSignalTransition::eventTest(ev) && m_guard();
   }
public:
   GuardedSignalTransition(const QObject * sender, const char * signal, F && guard) :
      QSignalTransition(sender, signal), m_guard(std::move(guard)) {}
   GuardedSignalTransition(const QObject * sender, const char * signal, const F & guard) :
      QSignalTransition(sender, signal), m_guard(guard) {}
};

template <typename F> static GuardedSignalTransition<F> *
addTransition(QState * src, QAbstractState *target,
              const QObject * sender, const char * signal, F && guard) {
   auto t = new GuardedSignalTransition<typename std::decay<F>::type>
         (sender, signal, std::forward<F>(guard));
   t->setTargetState(target);
   src->addTransition(t);
   return t;
}

That's about it - if you had a real device, that's all you need. Since I don't have your device, I'll create another StatefulObject to emulate the presumed device behavior:

class Device : public StatefulObject {
   Q_OBJECT
   AppPipe m_dev { nullptr, QIODevice::ReadWrite, this };
   State      s_init     { &m_mach, "s_init" },
              s_booting  { &m_mach, "s_booting" },
              s_firmware { &m_mach, "s_firmware" };
   FinalState s_loaded   { &m_mach, "s_loaded" };
public:
   Device(QObject * parent = 0) : StatefulObject(parent) {
      connectSignals();
      m_mach.setInitialState(&s_init);
      expect(&s_init, &m_dev, "boot", &s_booting);
      delay (&s_booting, 500, &s_firmware);
      send  (&s_firmware, &m_dev, "boot successful\n");
      expect(&s_firmware, &m_dev, ":00000001FF", &s_loaded);
      send  (&s_loaded,   &m_dev, "load successful\n");
   }
   Q_SLOT void stop() { m_mach.stop(); }
   AppPipe & pipe() { return m_dev; }
};

Now let's make it all nicely visualized. We'll have a window with a text browser showing the contents of the communications. Below it will be buttons to start/stop the programmer or the device, and labels indicating the state of the emulated device and the programmer:

screenshot

int main(int argc, char ** argv) {
   using Q = QObject;
   QApplication app{argc, argv};
   Device dev;
   Programmer prog;

   QWidget w;
   QGridLayout grid{&w};
   QTextBrowser comms;
   QPushButton devStart{"Start Device"}, devStop{"Stop Device"},
               progStart{"Start Programmer"};
   QLabel devState, progState;
   grid.addWidget(&comms, 0, 0, 1, 3);
   grid.addWidget(&devState, 1, 0, 1, 2);
   grid.addWidget(&progState, 1, 2);
   grid.addWidget(&devStart, 2, 0);
   grid.addWidget(&devStop, 2, 1);
   grid.addWidget(&progStart, 2, 2);
   devStop.setDisabled(true);
   w.show();

We'll connect the device's and programmer's AppPipes. We'll also visualize what the programmer is sending and receiving:

   dev.pipe().addOther(&prog.pipe());
   prog.pipe().addOther(&dev.pipe());
   Q::connect(&prog.pipe(), &AppPipe::hasOutgoing, &comms, [&](const QByteArray & data){
      comms.append(formatData("&gt;", "blue", data));
   });
   Q::connect(&prog.pipe(), &AppPipe::hasIncoming, &comms, [&](const QByteArray & data){
      comms.append(formatData("&lt;", "green", data));
   });

Finally, we'll connect the buttons and labels:

   Q::connect(&devStart, &QPushButton::clicked, &dev, &Device::start);
   Q::connect(&devStop, &QPushButton::clicked, &dev, &Device::stop);
   Q::connect(&dev, &Device::runningChanged, &devStart, &QPushButton::setDisabled);
   Q::connect(&dev, &Device::runningChanged, &devStop, &QPushButton::setEnabled);
   Q::connect(&dev, &Device::stateChanged, &devState, &QLabel::setText);
   Q::connect(&progStart, &QPushButton::clicked, &prog, &Programmer::start);
   Q::connect(&prog, &Programmer::runningChanged, &progStart, &QPushButton::setDisabled);
   Q::connect(&prog, &Programmer::stateChanged, &progState, &QLabel::setText);
   return app.exec();
}

#include "main.moc"

The Programmer and Device could live in any thread. I've left them in the main thread since there's no reason to move them out, but you could put both into a dedicated thread, or each into its own thread, or into threads shared with other objects, etc. It's completely transparent since AppPipe supports communications across the threads. This would also be the case if QSerialPort was used instead of AppPipe. All that matters is that each instance of a QIODevice is used from one thread only. Everything else happens via signal/slot connections.

E.g. if you wanted the Programmer to live in a dedicated thread, you'd add the following somewhere in main:

  // fix QThread brokenness
  struct Thread : QThread { ~Thread() { quit(); wait(); } };

  Thread progThread;
  prog.moveToThread(&progThread);
  progThread.start();

A little helper formats the data to make it easier to read:

static QString formatData(const char * prefix, const char * color, const QByteArray & data) {
   auto text = QString::fromLatin1(data).toHtmlEscaped();
   if (text.endsWith('\n')) text.truncate(text.size() - 1);
   text.replace(QLatin1Char('\n'), QString::fromLatin1("<br/>%1 ").arg(QLatin1String(prefix)));
   return QString::fromLatin1("<font color=\"%1\">%2 %3</font><br/>")
         .arg(QLatin1String(color)).arg(QLatin1String(prefix)).arg(text);
}
Schott answered 15/9, 2015 at 20:52 Comment(3)
This should be 100+ answer, I couldn't stop myself from commenting :)Meritocracy
@KubaOber Could you explain more about the guarded transitions ?Realgar
@wanyancan, might help : linkGurl
R
1

I'm not sure indeed this is the right approach.

You're polling with waitForReadyRead(). But since the serial port is a QIODevice, it will emit a void QIODevice::readyRead() signal when something will arrive on the serial port. Why not connect this signal to your input parsing code? No need for waitForReadyRead().

Also/on the other hand: "...this time it doesn't wait for timeout, readLines just return false immediately. Whats wrong ?"

Quoting the documentation:

If waitForReadyRead() returns false, the connection has been closed or an error has occurred.

(emphasis mine) From my experience as an embedded developer, it is not impossible that you put the device into kind of a "firmware upgrade" mode, and that by doing so the device rebooted into a special boot mode (not running the firmware you're about to update) and thus closed the connection. No way to tell unless it's documented/you have contact with the device developers. Not so obvious to check using a serial terminal to type your commands and witness that, I use minicom daily connected to my devices and it's pretty resilient across reboot - good for me.

Ravo answered 9/9, 2015 at 19:4 Comment(4)
While testing the program I use two virtual com ports. One connected to the app and the other one to realterm. I send files via realterm to and check what the app does. If I connect the readLine code directly to the readyRead() signal the eventloop has no chance to update events, since I only rotate inside waitForKeyword(). Is there a way to force the eventloop to update? If so how do I implement that in my functions?Jaffe
@Jaffe Do not write pseudo-synchronous code. You should not be using any waitForXxxx methods at all. None. Forget that they exist, and think of how to solve it then. All you can do and need to do then is to connect to the readyRead signal. And it will all work, since you don't block anymore with wait method calls.Caulk
@KubaOber But how do i wait if everything is asynchronous? Thats exactly the the point I’m having trouble with. If I send a command and receive an response, which leads to an slot. Should I check for each possible keyword in a big switch case and decide what to do next? There has to be some kind of design pattern. I need something like a state machine … do step1 then do step 2 then … How do I implement something like that with signals and slots?Jaffe
@Jaffe Bingo. QStateMachine. I wanted you to realize that that's what you need :)Caulk

© 2022 - 2024 — McMap. All rights reserved.