Function spiWriteRead() may return incomplete vector, due to bad optimization

[samuelfmlourenco]

An error was introduced in version 2.2.2, that causes the returned vector to only contain the last bytes read from the SPI bus if the number of bytes written is greater than 56. Upon further analysis, it was found out that the vector mentioned above is not being added to, but actually cut in size, with the new values overwriting the previous ones. An excerpt containing the erroneous code follows:

// Writes to the SPI bus while reading back, returning a vector of the same size as the one given
// This is the prefered method of writing and reading, if both endpoint addresses are known
QVector<quint8> CP2130::spiWriteRead(const QVector<quint8> &data, quint8 endpointInAddr, quint8 endpointOutAddr, int &errcnt, QString &errstr)
{
    size_t bytesToWriteRead = static_cast<size_t>(data.size());
    size_t bytesLeft = bytesToWriteRead;
    QVector<quint8> retdata;
    while (bytesLeft > 0) {
        int payload = bytesLeft > 56 ? 56 : bytesLeft;
...
        retdata.resize(bytesRead);  // Optimization implemented in version 2.2.2
        for (int i = 0; i < bytesRead; ++i) {
            retdata[i] = writeReadInputBuffer[i];  // Note that the values are no longer appended to the QVector since version 2.2.2, because it is more efficient to resize the QVector just once (see above), so that the values can be simply assigned
        }
        delete[] writeReadInputBuffer;
        bytesLeft -= payload;
    }
    return retdata;
}

It is possible to see that the for loop will potentially rewrite values of "retdata". Furthermore, the line above will potentially shrink the vector. A correction that fixes the issue, follows:

// Writes to the SPI bus while reading back, returning a vector of the same size as the one given
// This is the prefered method of writing and reading, if both endpoint addresses are known
QVector<quint8> CP2130::spiWriteRead(const QVector<quint8> &data, quint8 endpointInAddr, quint8 endpointOutAddr, int &errcnt, QString &errstr)
{
    size_t bytesToWriteRead = static_cast<size_t>(data.size());
    size_t bytesLeft = bytesToWriteRead;
    size_t bytesReadAcc = 0;  // Accumulator implemented in version 2.2.3
    QVector<quint8> retdata;
    while (bytesLeft > 0) {
        int payload = bytesLeft > 56 ? 56 : bytesLeft;
    size_t bytesToWriteRead = static_cast<size_t>(data.size());
    size_t bytesLeft = bytesToWriteRead;
    size_t bytesReadAcc = 0;  // Accumulator implemented in version 2.2.3
    QVector<quint8> retdata;
    while (bytesLeft > 0) {
        int payload = bytesLeft > 56 ? 56 : bytesLeft;
...
        bytesReadAcc += bytesRead;
        retdata.resize(static_cast<int>(bytesReadAcc));  // Optimization implemented in version 2.2.2, and fixed in version 2.2.3
        for (int i = 0; i < bytesRead; ++i) {
            retdata[bytesReadAcc - bytesRead + i] = writeReadInputBuffer[i];  // Note that the values are no longer appended to the QVector since version 2.2.2, because it is more efficient to resize the QVector only once per iteration (see above), so that the values may be simply assigned (fixed in version 2.2.3)
        }
        delete[] writeReadInputBuffer;
        bytesLeft -= payload;
    }
    return retdata;
}

The implementation of the "bytesReadAcc" variable, which accumulates the number of bytes read during previous iterations of the while loop, is paramount. Thus, the size of "retdata" is always expanded as needed, and none of the previously obtained values are overwritten. Note that the vector is always expanded by the amount of bytes that were actually read (which may differ from the value that "payload" carries).

[samuelfmlourenco]

This is a refinement of spiWriteRead():

// Writes to the SPI bus while reading back, returning a vector of the same size as the one given
// This is the prefered method of writing and reading, if both endpoint addresses are known
QVector<quint8> CP2130::spiWriteRead(const QVector<quint8> &data, quint8 endpointInAddr, quint8 endpointOutAddr, int &errcnt, QString &errstr)
{
    size_t bytesToWriteRead = static_cast<size_t>(data.size());
    size_t bytesProcessed = 0;  // Loop control variable implemented in version 2.2.3, to replace "bytesLeft"
    QVector<quint8> retdata;
    while (bytesProcessed < bytesToWriteRead) {
        size_t bytesRemaining = bytesToWriteRead - bytesProcessed;  // Equivalent to the variable "bytesLeft" found in version 2.2.2, except that it is no longer used for control
        quint32 payload = static_cast<quint32>(bytesRemaining > 56 ? 56 : bytesRemaining);
        int bufSize = payload + 8;
        unsigned char *writeReadCommandBuffer = new unsigned char[bufSize] {
            0x00, 0x00,         // Reserved
            CP2130::WRITEREAD,  // WriteRead command
            0x00,               // Reserved
            static_cast<quint8>(payload),
            static_cast<quint8>(payload >> 8),
            static_cast<quint8>(payload >> 16),
            static_cast<quint8>(payload >> 24)
        };
        for (size_t i = 0; i < payload; ++i) {
            writeReadCommandBuffer[i + 8] = data[bytesProcessed + i];
        }
#if LIBUSB_API_VERSION >= 0x01000105
        bulkTransfer(endpointOutAddr, writeReadCommandBuffer, bufSize, nullptr, errcnt, errstr);
#else
        int bytesWritten;
        bulkTransfer(endpointOutAddr, writeReadCommandBuffer, bufSize, &bytesWritten, errcnt, errstr);
#endif
        delete[] writeReadCommandBuffer;
        unsigned char *writeReadInputBuffer = new unsigned char[payload];
        int bytesRead = 0;  // Important!
        bulkTransfer(endpointInAddr, writeReadInputBuffer, payload, &bytesRead, errcnt, errstr);
        int prevretdataSize = retdata.size();
        retdata.resize(prevretdataSize + bytesRead);  // Optimization implemented in version 2.2.2, and fixed in version 2.2.3
        for (int i = 0; i < bytesRead; ++i) {
            retdata[prevretdataSize + i] = writeReadInputBuffer[i];  // Note that the values are no longer appended to the QVector since version 2.2.2, because it is more efficient to resize the QVector only once per iteration (see above), so that the values may be simply assigned (fixed in version 2.2.3)
        }
        delete[] writeReadInputBuffer;
        bytesProcessed += payload;  // Note that, since version 2.2.3, the loop control variable is added to (it is generaly a bad idea to subtract from a unsigned variable, because it can lead to a overflow that may go unchecked)
    }
    return retdata;
}

The variable "bytesReadAcc" is not needed, because it is possible to obtain the value from the size of the vector. This code was tested against the implementation from version 2.2.1. Seemingly, it works without any issue.

[samuelfmlourenco]

Fixed in version 2.2.3.