Optimize the reading of a client

Dooham

Hello,
I have the following issue: I have to create two program, a server that can read data from the serial port and a client that can receive this data and send response. I made the code and worked, but the issue that I have is the reading speed of the client: the device that is connected to my serial port is a telemetry device connected to a Pixhawk. The Pixhawk is a device that send constantly messages. There are other programs that can read this kind of device (thought a server). When I connect my client I see that the reading speed is lower than the reading speed of these programs. I would like to know how can I improve my client to reach a better reading speed. This is the code where I configure the TCPSocket of the client:

socket = new QTcpSocket(this);
       socket->setSocketOption(QAbstractSocket::LowDelayOption, 1);
        data="";
        connect(socket, SIGNAL(connected()),this, SLOT(connected()));
        connect(socket, SIGNAL(disconnected()),this, SLOT(disconnected()));
        connect(socket, SIGNAL(bytesWritten(qint64)),this, SLOT(bytesWritten(qint64)));
        connect(socket, SIGNAL(readyRead()),this, SLOT(readyRead()));

        Conexiones();
        qDebug() << "connecting...";

        // this is not blocking call

        socket->connectToHost("localhost", 9999);

        // we need to wait...
        if(!socket->waitForConnected(5000))
        {
            qDebug() << "Error: " << socket->errorString();
        }

    }else{
        qDebug()<<"Dispositivo conectado";
    }

JonB

@Dooham
Show us your slots bytesWritten() & readyRead().

Also, since you have chosen to use QAbstractSocket::LowDelayOption, wherever your code writes to the socket do you call QAbstractSocket::flush() somewhere? If not, your low-delay probably won't have any effect anyway (and that's if it has any effect on a serial port anyway, which I don't know)....

Dooham

@JonB Here are these slots:

readyRead:

void MyTcpSocket::readyRead()
{

    data = socket->readAll();


    mensajeRec.getData(data);
    mensajeRec.descifrarMsg2();
}

mensajeRec is another class that I wrote to decode the messages from the Pixhawk.

bytesWritten:

void MyTcpSocket::bytesWritten(qint64 bytes)
{
    qDebug() << bytes << " bytes written...";
}

This function was in the original code that I saw and I included it to check errors when I write messages from the client.

I have seen that I didnt include the flush function in the parts where I write messages to the servers.

JonB

@Dooham
My two observations:

I do think you need to call flush() after each write from your client. Else you don't know what buffering is going on, and if there is that would affect your overall exchange timings.
Obviously only you know how long the extra code in your readyRead() takes. For the purposes of timings, you might like to comment those out? Meanwhile, I don't know how you have written the two functions you call: have you allowed for the fact/possibility that the data = socket->readAll() may not actually have the whole of the data read from your device, it could be as little as 1 byte, and not any kind of "complete message" if that is what you are expecting? Though I don't suppose that will affect the speed.

If it's outside of the above and is somehow to do with how serial devices can be accessed optimally, that's beyond my pay-grade....

Dooham

@JonB First of sorry for the delay in my response, I didnt have the device until now.
1- I implemented a flush() function every time I write and that didnt work.
2- The function that I call when I read the messages could be the problem. Pixhawk can read and write two different types of messages (called Mavlink 1.0 and 2.0), and the problem with the delay just occur when I am using the version 2.0 not 1.0. These ones are the functions:

void MensajeRecibido::getData(QByteArray dataRec)
{
   
    data=dataRec;
    corregirMagigSum();
}

void MensajeRecibido::descifrarMsg2()
{


    estado=0;
    contPayload=0;
    for (int i=0; i<=data.size(); i++){
        caracter=static_cast<uint8_t>(data[i]);
        switch (estado) {
        case 0:
            if(caracter==253){
                msg= new mavlink_message_t;
                msg->magic=caracter;
                mavlink_start_checksum(msg);
                estado=1;
                versionMavlink = 2;
            } else if (caracter==254) {
                msg= new mavlink_message_t;
                msg->magic=caracter;
                mavlink_start_checksum(msg);
                estado=1;
                versionMavlink = 1;
            }

            break;
        case 1:
            msg->len=caracter;
            mavlink_update_checksum(msg, msg->len);
            if(versionMavlink==2){
                estado=2;
            }else if (versionMavlink==1) {
                msg->incompat_flags=0;
                msg->compat_flags=0;
                estado=4;

            }
            break;
        case 2:
            msg->incompat_flags=caracter;
            mavlink_update_checksum(msg, msg->incompat_flags);
            estado=3;
            break;
        case 3:
            msg->compat_flags=caracter;
            mavlink_update_checksum(msg, msg->compat_flags);
            estado=4;
            break;
        case 4:
            msg->seq=caracter;
            mavlink_update_checksum(msg, msg->seq);
            estado=5;
            break;
        case 5:
            msg->sysid=caracter;
            mavlink_update_checksum(msg, msg->sysid);
            estado=6;
            break;
        case 6:
            msg->compid=caracter;
            mavlink_update_checksum(msg, msg->compid);
            estado=7;
            break;
        case 7:
            msgid1=caracter;
            mavlink_update_checksum(msg, msgid1);
            if(versionMavlink==2){
                estado=8;
            }else if (versionMavlink==1) {
                msg->msgid=msgid1;
                estado=10;

            }
            break;
        case 8:
            msgid2=caracter;
            mavlink_update_checksum(msg, msgid2);
            estado=9;
            break;
        case 9:
            msgid3=caracter;
            mavlink_update_checksum(msg, msgid3);
            msg->msgid = msgid1;
            msg->msgid |= msgid2<<8;
            msg->msgid |= ((uint32_t)msgid3)<<16; 
            estado=10;
            break;
        case 10:
            msg->payload64[contPayload]=caracter;
            mavlink_update_checksum(msg, caracter);
            contPayload++;
            if(contPayload==msg->len){
                estado=11;
                if(msg->msgid<256){
                    mavlink_update_checksum(msg, magicSum[msg->msgid]);
                }else {

                    uint8_t crc_extra = 205;


                    mavlink_update_checksum(msg, crc_extra);
                }

            }

            break;
        case 11:
            msg->ck[0]=caracter;

            estado=12;
            break;
        case 12:
            msg->ck[1]=caracter;
            uint16_t compr=msg->ck[0]| msg->ck[1]<<8;

            menTot++;
            if(msg->checksum==compr){
                menCor+=1;
                qDebug()<<msg->seq;
                identificarMensaje();

                qDebug()<<menCor/menTot;
                qDebug()<<menCor<<" de "<<menTot;
              

            }else {
                menInc+=1;

               
                qDebug()<<"MENSAJE ERRONEO***********";

                

            }
            estado=0;
            contPayload=0;

            //msg->~__mavlink_message();
            delete msg;
            break;
        }

    }

}

As I said the issue just appear with the version 2.0,the main differences is that in the version 2.0 I use the cases 2,3,8 and 9.
Thanks for your help.

JonB

@Dooham
I don't know why that should cause what you describe as "slow reading speed". (I am assuming mavlink_update_checksum() is a very cheap operation.)

One thing you could do is debug out just how many bytes your data = socket->readAll(); is receiving each time. You should not assume it will be all the bytes received from the device in one blob, you may receive sub-blobs over separate calls. If that is the case you will call descifrarMsg2() for each "sub-blob", and that resets estado=0; so your code will not act the way you would expect (it will go all over the place). Hence your code actually relies on the readAll() getting one whole complete message in a single go. Maybe that's how it happens for you in practice, but it's not right and is just waiting to fall over....

(Hint: re-initialising estado & contPayload to 0 each time upon entry to descifrarMsg2() is wrong approach given that code uses estando to be "stateless". That should be done prior to this method, and only reset to 0 as you have in case 12:, i.e. at the end of a full message received.)

Whether this will make a blind bit of difference to your speed I don't know, but I think you should reconsider your code in the light of not necessarily expecting readAll() to return a complete message....

Dooham

@JonB You were right, the issue was that the code was spliting some message. I have tried another solution and now the reading speed is higher but in some moment the code can execute an order and it is unexpectedly finished. This is my new code:

void MensajeRecibido::getData(QByteArray dataRec)
{
    // qDebug()<<dataRec.toHex();
    data.clear();
    data.append(dataInc);
    qDebug()<<data.toHex();
    data.append(dataRec);
   // data=dataRec;
}
void MensajeRecibido::descifrarMsg2()
{
    corregirMagigSum();
    estado=0;
    contPayload=0;
    for (int i=0; i<=data.size(); i++){
        caracter=static_cast<uint8_t>(data[i]);
        switch (estado) {
        case 0:
            if(caracter==253){
                msg= new mavlink_message_t;
                msg->magic=caracter;
                mavlink_start_checksum(msg);
                estado=1;
                versionMavlink = 2;
                dataInc.append(static_cast<char>(caracter));
            } else if (caracter==254) {
                msg= new mavlink_message_t;
                msg->magic=caracter;
                mavlink_start_checksum(msg);
                estado=1;
                versionMavlink = 1;
                dataInc.append(static_cast<char>(caracter));
            }


            break;
        case 1:
            msg->len=caracter;
            mavlink_update_checksum(msg, msg->len);
            if(versionMavlink==2){
                estado=2;
                dataInc.append(static_cast<char>(caracter));
            }else if (versionMavlink==1) {
                msg->incompat_flags=0;
                msg->compat_flags=0;
                estado=4;
                dataInc.append(static_cast<char>(caracter));

            }

            break;
        case 2:
            msg->incompat_flags=caracter;
            mavlink_update_checksum(msg, msg->incompat_flags);
            estado=3;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 3:
            msg->compat_flags=caracter;
            mavlink_update_checksum(msg, msg->compat_flags);
            estado=4;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 4:
            msg->seq=caracter;
            mavlink_update_checksum(msg, msg->seq);
            estado=5;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 5:
            msg->sysid=caracter;
            mavlink_update_checksum(msg, msg->sysid);
            estado=6;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 6:
            msg->compid=caracter;
            mavlink_update_checksum(msg, msg->compid);
            estado=7;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 7:
            msgid1=caracter;
            mavlink_update_checksum(msg, msgid1);
            if(versionMavlink==2){
                estado=8;
                dataInc.append(static_cast<char>(caracter));
            }else if (versionMavlink==1) {
                msg->msgid=msgid1;
                estado=10;
                dataInc.append(static_cast<char>(caracter));

            }
            break;
        case 8:
            msgid2=caracter;
            mavlink_update_checksum(msg, msgid2);
            estado=9;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 9:
            msgid3=caracter;
            mavlink_update_checksum(msg, msgid3);
            msg->msgid = msgid1;
            msg->msgid |= msgid2<<8;
            msg->msgid |= ((uint32_t)msgid3)<<16;
            estado=10;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 10:
            msg->payload64[contPayload]=caracter;
            mavlink_update_checksum(msg, caracter);
            contPayload++;
            dataInc.append(static_cast<char>(caracter));
            if(contPayload==msg->len){
                estado=11;

                if(msg->msgid<256){
                    mavlink_update_checksum(msg, magicSum[msg->msgid]);
                }else {

                    uint8_t crc_extra = 205;


                    mavlink_update_checksum(msg, crc_extra);
                }

            }

            break;
        case 11:
            msg->ck[0]=caracter;

            estado=12;
            dataInc.append(static_cast<char>(caracter));
            break;
        case 12:
            msg->ck[1]=caracter;
            uint16_t compr=msg->ck[0]| msg->ck[1]<<8;

            menTot++;
            if(msg->checksum==compr){
                menCor+=1;
                qDebug()<<msg->seq;
                identificarMensaje();

                qDebug()<<menCor/menTot;
                qDebug()<<menCor<<" de "<<menTot;
                //qDebug()<<msg->checksum<<" y "<<compr;
                dataInc.clear();

            }else {
                menInc+=1;

                qDebug()<<msg->seq;
                identificarMensaje();
                qDebug()<<"MENSAJE ERRONEO";

                qDebug()<<menInc/menTot;
                //qDebug()<<msg->checksum<<" y "<<compr;
                dataInc.clear();

            }
            estado=0;
            contPayload=0;

            msg->~__mavlink_message();
            delete msg;
            break;
        }

    }
    if(dataInc.size()>279){

        dataInc.clear();
    }

}

Always it is finished in this line:

case 0:
            if(caracter==253){
                msg= new mavlink_message_t;

However, I have seen that almost all the messages that is cut between two readAll() is wrong.

JonB

@Dooham
Briefly: this looks like a coding/debugging issue, which I leave to you. Don't forget that a message might get split across more than 2 "packets", if your code now relies on it only being 2 that is insufficient. I don't know why you have introduced dataInc. According to my original reading, and the Hint I gave you, all you needed to do was stick with the original but move the initialisation of

    estado=0;
    contPayload=0;

outside of each entry into descifrarMsg2() (since that is stateless) and I think it would have worked, but it's for you to play with.....

Dooham

@JonB Thanks again. I included that variable to accumulate the incomplete part of the message and include it at the beginning. However, I think that, because I am using a telemtry system, I am not going to include the message incomplete because, between two readyRead(), there are parts that are missing.

JonB

@Dooham
Just to say: Qt readyRead/readAll() will not "miss parts", if that is what you are implying. Re-look at your code, maybe as I indicated revert to original and just sort out where you do those two initializations.

Dooham

@JonB No, It is because is a radio communication not a wire communication. The issue would be in the process of sending data from the Pixhawk to the device connected to my serial port. Not in the function

JonB

@Dooham OK, I just about understand :)