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

[samuelfmlourenco]

An error was introduced in version 1.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
std::vector<uint8_t> CP2130::spiWriteRead(const std::vector<uint8_t> &data, uint8_t endpointInAddr, uint8_t endpointOutAddr, int &errcnt, std::string &errstr)
{
    size_t bytesToWriteRead = data.size();
    size_t bytesLeft = bytesToWriteRead;
    std::vector<uint8_t> retdata;
    while (bytesLeft > 0) {
        int payload = bytesLeft > 56 ? 56 : bytesLeft;
...
        retdata.resize(static_cast<size_t>(bytesRead));  // Optimization implemented in version 1.2.2
        for (int i = 0; i < bytesRead; ++i) {
            retdata[i] = writeReadInputBuffer[i];  // Note that std::vector::push_back() is no longer used since version 1.2.2, because it is more efficient to resize the vector 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
std::vector<uint8_t> CP2130::spiWriteRead(const std::vector<uint8_t> &data, uint8_t endpointInAddr, uint8_t endpointOutAddr, int &errcnt, std::string &errstr)
{
    size_t bytesToWriteRead = data.size();
    size_t bytesLeft = bytesToWriteRead;
    size_t bytesReadAcc = 0;  // Accumulator implemented in version 1.2.3
    std::vector<uint8_t> retdata;
    while (bytesLeft > 0) {
        int payload = bytesLeft > 56 ? 56 : bytesLeft;
...
        bytesReadAcc += bytesRead;
        retdata.resize(bytesReadAcc);  // Optimization implemented in version 1.2.2, and fixed in version 1.2.3
        for (int i = 0; i < bytesRead; ++i) {
            retdata[bytesReadAcc - bytesRead + i] = writeReadInputBuffer[i];  // Note that std::vector::push_back() is no longer used since version 1.2.2, because it is more efficient to resize the vector only once per iteration (see above), so that the values may be simply assigned (fixed in version 1.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]

Fixed in version 1.2.3.