Signals/Slots timing issue in an handshake communication protocol

[Andrew23]

My application has to communicate with a custom device through a handshake protocol which provides for a response (ACK or NACK) for every packet/command sent in both directions. Every packets/commands start with 0x02 marker, then two byte for the size, then the type, then data and finally the close marker 0x03. I've created a separate class for the serial communication with QSerialPort and created the signals/slots needed:

connect( myInterface, &MyInterface::SendPacket, serialConnection, &SerialConnection::SendPacket, Qt::DirectConnection );
connect( myInterface, &MyInterface::SendAck, serialConnection, &SerialConnection::SendAck, Qt::DirectConnection );
connect( myInterface, &MyInterface::SendNack, serialConnection, &SerialConnection::SendNack, Qt::DirectConnection );
connect( serialConnection, &SerialConnection::ReceivePacket, myInterface, &MyInterface::ReceivePacket, Qt::DirectConnection );
connect( serialConnection, &SerialConnection::ReceiveAck, myInterface, &MyInterface::ReceiveAck, Qt::DirectConnection );
connect( serialConnection, &SerialConnection::ReceiveNack, myInterface, &MyInterface::ReceiveNack, Qt::DirectConnection );

Following my serial communication class (I've omitted the unnecessary parts of code) :

serialconnection.h

class SerialConnection : public QWidget
{
    Q_OBJECT
public:
    explicit SerialConnection(QWidget *parent = nullptr);
    ~SerialConnection();

signals:
    void ReceiveAck();
    void ReceiveNack();
    void ReceiveString( QByteArray packet );
    void ReceivePacket( QByteArray packet );
    void ReceiveRawData( QByteArray rawData );

public slots:
    void SendAck();
    void SendNack();
    void SendPacket( QByteArray packet );
    void ReadData();

private:
    QSerialPort *serial = nullptr;

};

serialconnection.cpp

SerialConnection::SerialConnection( QWidget *parent ) :
    QWidget( parent ),
    serial( new QSerialPort )
{
    connect( serial, &QSerialPort::readyRead, this, &SerialConnection::ReadData );
}

SerialConnection::~SerialConnection()
{

}

void SerialConnection::SendAck()
{
    QByteArray ack;

    ack.append( ( char ) COMMUNICATION_USB_PACKET_STX );
    ack.append( ( char ) COMMUNICATION_USB_PACKET_ACK_SIZE );
    ack.append( ( char ) 0x00 );
    ack.append( ( char ) COMMUNICATION_USB_PACKET_TYPE_ACK );
    ack.append( ( char ) COMMUNICATION_USB_PACKET_ETX );

    serial->write( ack );
    qDebug() << "-> ack";
}

void SerialConnection::SendNack()
{
    QByteArray nack;

    nack.append( ( char ) COMMUNICATION_USB_PACKET_STX );
    nack.append( ( char ) COMMUNICATION_USB_PACKET_NACK_SIZE );
    nack.append( ( char ) 0x00 );
    nack.append( ( char ) COMMUNICATION_USB_PACKET_TYPE_NACK );
    nack.append( ( char ) COMMUNICATION_USB_PACKET_ETX );

    serial->write( nack );
    qDebug() << "-> nack";
}

void SerialConnection::SendPacket( QByteArray packet )
{
    serial->write( packet );
    qDebug() << "-> " + packet.toHex();
}

void SerialConnection::ReadData()
{
    static QByteArray packet;
    QByteArray rawRead;
    static quint16 packetSize;
    int index;

    rawRead.append( serial->readAll() );
    emit ReceiveRawData( rawRead );

    packet.append( rawRead );

    while ( 1 )
    {
        if ( ( index = packet.indexOf( 0x02 ) ) > 0 )
        {
            packet.remove( 0, index );
        }
        else if ( packet.indexOf( 0x02 ) == -1 )
        {
            packet.clear();
        }

        if ( packet.size() < 3 )
        {
            return;
        }
        else
        {
            packetSize = *( ( qint16 * )( packet.data() + 1 ) );
        }

        if ( packet.size() >= packetSize )
        {
            if ( packet.at( 0 ) == 0x02 && packet.at( packetSize - 1 ) == 0x03 )
            {
                if ( packet.at( 3 ) == COMMUNICATION_USB_PACKET_TYPE_ACK )
                {
                    qDebug() << "<- ack";
                    emit ReceiveAck();
                }
                else if ( packet.at( 3 ) == COMMUNICATION_USB_PACKET_TYPE_NACK )
                {
                    qDebug() << "<- nack";
                    emit ReceiveNack();
                }
                else if ( packet.at( 3 ) == COMMUNICATION_USB_PACKET_TYPE_STRING )
                {
                    qDebug() << "<- string: " + packet.mid(4, packetSize - 5);
                    emit ReceiveString( packet );
                }
                else
                {
                    qDebug() << "<- packet: " + packet.toHex();
                    emit ReceivePacket( packet );
                }

                packet.remove( 0, packetSize );
            }
            else
            {
                SendNack();
            }
        }
        else
        {
            return;
        }
    }
}

and my interface class:

myinterface.h

namespace Ui {
class MyInterface;
}

class MyInterface : public QWidget
{
    Q_OBJECT

public:
    explicit MyInterface( QWidget *parent = nullptr );
    ~MyInterface();

signals:
    void SendPacket( QByteArray packet );
    void SendAck();
    void SendNack();

public slots:
    void ReceiveAck();
    void ReceiveNack();
    void ReceivePacket( QByteArray packet );
    void Test();

private:
    Ui::MyInterface *ui;
    bool ackReceived;
    bool nackReceived;
    bool requestReceived;

    void RequestSerialNumber();
    void RequestType();
};

myinterface.cpp

MyInterface::MyInterface( QWidget *parent ) :
    QWidget( parent ),
    ui( new Ui::MyInterface )
{
    ui->setupUi( this );

    connect( ui->pushButtonTest, &QPushButton::clicked, this, &MyInterface::Test );
}


void Onemytis2Interface::ReceiveAck()
{
    ackReceived = true;
}

void Onemytis2Interface::ReceiveNack()
{
    nackReceived = true;
}

void Onemytis2Interface::ReceivePacket( QByteArray packet )
{
    switch ( packet.at( 4 ) )
    {
        case COMMUNICATION_USB_RX_COMMAND_DEVICE_SERIAL_NUMBER_GET:
        {
            if ( packet.size() == COMMUNICATION_USB_RX_COMMAND_DEVICE_SERIAL_NUMBER_GET_SIZE )
            {
                emit SendAck();
                deviceSerialNumber = packet.mid( 5, 16 );
                ui->labelSNValue->setText( deviceSerialNumber );
                requestReceived = true;
            }
            else
            {
                emit SendNack();
            }

            break;
        }

        case COMMUNICATION_USB_RX_COMMAND_DEVICE_TYPE_GET:
        {
            if ( packet.size() == COMMUNICATION_USB_RX_COMMAND_DEVICE_TYPE_GET_SIZE )
            {
                emit SendAck();
                deviceType = packet.mid( 5, 16 );
                ui->labelModelValue->setText( deviceType );
                requestReceived = true;
            }
            else
            {
                emit SendNack();
            }

            break;
        }

        case COMMUNICATION_USB_RX_COMMAND_TEST:
        {

            break;
        }

        default:
        {
            qDebug() << "ERROR";
            break;
        }
    }
}


void MyInterface::Test()
{
    RequestType();
    RequestSerialNumber();
}


void MyInterface::RequestType()
{
    QByteArray request;
    QByteArray cmdSize;
    uint16_t cmdSizeTmp;
    int retry;

    request.append( ( char ) COMMUNICATION_USB_PACKET_STX );
    request.append( ( char ) 0x00 );
    request.append( ( char ) 0x00 );
    request.append( ( char ) COMMUNICATION_USB_PACKET_TYPE_COMMAND );
    request.append( ( char ) COMMUNICATION_USB_TX_COMMAND_DEVICE_TYPE_GET );
    request.append( ( char ) COMMUNICATION_USB_PACKET_ETX );

    cmdSizeTmp = request.length();
    cmdSize.append( *( char * )&cmdSizeTmp );
    cmdSize.append( *( ( char * )&cmdSizeTmp + 1 ) );
    request.replace( 1, 2, cmdSize );

    ackReceived = false;
    nackReceived = false;
    requestReceived = false;
    emit SendPacket( request );

    retry = 0;

    while ( ackReceived == false && nackReceived == false )
    {
        retry++;
        QThread::msleep( 1 );

        if ( retry > COMMUNICATION_USB_PACKET_WAIT_MAX_MS )
        {
            qDebug() << "timeout type response";
            break;
        }
    }

    if ( ackReceived == true )
    {
        retry = 0;

        while ( requestReceived == false )
        {
            retry++;
            QThread::msleep( 1 );

            if ( retry > COMMUNICATION_USB_PACKET_WAIT_MAX_MS )
            {
                qDebug() << "timeout type request";
                break;
            }
        }
    }
    else
    {
        ui->labelTypeValue->setText( "error" );
    }
}


void MyInterface::RequestSerialNumber()
{
    QByteArray request;
    QByteArray cmdSize;
    uint16_t cmdSizeTmp;
    int retry;

    request.append( ( char ) COMMUNICATION_USB_PACKET_STX );
    request.append( ( char ) 0x00 );
    request.append( ( char ) 0x00 );
    request.append( ( char ) COMMUNICATION_USB_PACKET_TYPE_COMMAND );
    request.append( ( char ) COMMUNICATION_USB_TX_COMMAND_DEVICE_SERIAL_NUMBER_GET );
    request.append( ( char ) COMMUNICATION_USB_PACKET_ETX );

    cmdSizeTmp = request.length();
    cmdSize.append( *( char * )&cmdSizeTmp );
    cmdSize.append( *( ( char * )&cmdSizeTmp + 1 ) );
    request.replace( 1, 2, cmdSize );

    ackReceived = false;
    nackReceived = false;
    requestReceived = false;
    emit SendPacket( request );

    retry = 0;

    while ( ackReceived == false && nackReceived == false )
    {
        retry++;
        QThread::msleep( 1 );

        if ( retry > COMMUNICATION_USB_PACKET_WAIT_MAX_MS )
        {
            qDebug() << "timeout sn response";
            break;
        }
    }

    if ( ackReceived == true )
    {
        retry = 0;

        while ( requestReceived == false )
        {
            retry++;
            QThread::msleep( 1 );

            if ( retry > COMMUNICATION_USB_PACKET_WAIT_MAX_MS )
            {
                qDebug() << "timeout sn response";
                break;
            }
        }
    }
    else
    {
        ui->labelSNValue->setText( "error" );
    }
}

Now, if I click the Test push button, I expect the following transactions:

-> 020600030803 // The interface send the request packet emitting signal in RequestType()
<- ack // The custom device respond with ack, we are waiting it in the RequestType()
<- packet: 02160003034f74686563000000000000000000000003 //The custom device send to interface the requested packet
-> ack // The interface respond with ack
-> 020600030703 // The interface send the request packet emitting signal in RequestSerialNumber()
<- ack // The custom device respond with ack, we are waiting it in the RequestSerialNumber()
<- packet: 02160003034f74686563000000000000000000000003 //The custom device send to interface the requested packet
-> ack // The interface respond with ack

but the interface doesn't work as I expect. Usign the qDebug I get

-> 020600030803 // The interface send the request packet emitting signal in RequestType()
timeout type response // I've a timeout waiting the ack/nack for the RequestType() command
-> 020600030703 // The interface send the request packet emitting signal in RequestSerialNumber()
timeout sn response // I've a timeout waiting the ack/nack for the RequestSerialNumber() command 
<- ack // HERE I HAVE THE FIRST ACK OF THE  RequestType() COMMAND
<- packet: 02160003034f74686563000000000000000000000003
-> ack
<- nack

As you can see, I receive the ack for the first command RequestType() AFTER the Test function terminates and it means after I send two packet, and this cause an error in communication. The state variable ackReceived/nackReceived never change after emit the SendPacket signal if I remain in the call function scope, also if a use a Qt::DirectConnection between signals and slots.

I've not much experience in the Qt signal/slot context and I want to understand what goin on in my code to achieve my target.

Anyone can give me some suggestions and/or clarifications?

[jsulm]

@Andrew23 said in Signals/Slots timing issue in an handshake communication protocol:

while ( 1 )

this is not going to work well with signals/slots...
Do not use such loops, use Qt as it is supposed to be used - as asynchronous framework.

[Andrew23]

@jsulm said in Signals/Slots timing issue in an handshake communication protocol:

@Andrew23 said in Signals/Slots timing issue in an handshake communication protocol:

while ( 1 )

this is not going to work well with signals/slots...
Do not use such loops, use Qt as it is supposed to be used - as asynchronous framework.

Ok @jsulm , I'd have to modify it using some sort of state variable like isPacketEmpty == false?

[SGaist]

Hi,

The usual way is to have a buffer where you cumulate the data your receive from the serial port. You then parse it to see if your have a full data frame, if so extract and process it.

[Andrew23]

@SGaist said in Signals/Slots timing issue in an handshake communication protocol:

Hi,

The usual way is to have a buffer where you cumulate the data your receive from the serial port. You then parse it to see if your have a full data frame, if so extract and process it.

Isn't what I'm doing with the static QByteArray packet in the SerialConnection::ReadData()?

Anyway, is this a possible cause of my behaviour above?

[jsulm]

@Andrew23 said in Signals/Slots timing issue in an handshake communication protocol:

is this a possible cause of my behaviour above?

Yes, as long as you loop is running you're blocking the Qt event loop.