Update FileIO.cpp - Optimization of doBuffer() to avoid shifting the entire buffer

[bobh66]

Optimization of doBuffer() - instead of moving all of the data one byte to the left in the buffer, increment readPos to skip over the error code byte.

[cmaglie]

The fix seems good, nice catch!
Maybe you can set readPos to 1 and completely remove the "if (buffered>0)"?

[matthijskooijman]

@cmaglie, your suggestion looks good to me. the "if (buffered>0)" doesn't really add anything (in the old code, the loop would already exexcute 0 times when buffered was 0, and readPos is never actually used when buffered is 0.

However, looking more closely, it actually seems this code is missing a bit of error handling. Bridge::transfer can return TRANSFER_TIMEOUT (0xFFFF), which is not handled here. In addition, the exceptional case that transfer returns 0 should also be handled (I don't think this should occur in practice, but better handle it anway). However, the current "buffered >0" if does not handle either of the exception cases I described above, so it can just be removed.

[bobh66]

I can update the error handling, but I'm not sure what (if anything) to do in the case where transfer returns 0 bytes.

if (TRANSFER_TIMEOUT == buffered) {
    // transfer tried max_tries times and still failed to retrieve any data
    buffered = 0;
} else if (buffered > 0) {
    // transfer retrieved at least one byte of data, so skip the error code in the buffer
    readPos++;
} else if (buffered == 0) {
    // transfer retrieved 0 bytes - is there anything to do here?
}

return;

[matthijskooijman]

The most important thing to do is to not crash / do out-of-bounds reads, really.

Note that in the code, "buffered" is set to the number of bytes
transfered - 1, so your above code will not work as expected.

How about this (which delays the -1 for buffered until it actually
determines something has been transfered).

buffered = bridge.transfer(cmd, 3, buffer, BUFFER_SIZE);

if (TRANSFER_TIMEOUT == buffered || 0 == buffered) {
    // transfer failed to retrieve any data
    buffered = 0;
} else {
    // transfer retrieved at least one byte of data, so skip the error code in the buffer
    readPos++;
    buffered--;
}

One more note: your commit message isn't very useful. "Update FileIO.cpp" doesn't really say anything about what is changed, nor does it indicate the change is in the bridge library. The second line explains all this, but the first line should contain a useful summary already. Better would be something like:

Optimize FileIO::doBuffer in the Bridge library

Instead of moving all of the data one byte to the left in the buffer, increment readPos to skip over the error code byte.

[bobh66]

I made the changes suggested by Matthijs under a new commit. Should
they go into a separate pull request or be squashed into the same commit?

Thanks

Bob

[matthijskooijman]

Just squash them in the same commit and do a force push, that will update this pullrequest.

[bobh66]

The pull request has been updated. Please let me know if you have any
other comments. Thanks!

Bob

[bobh66]

I found a problem and need to do one more change - please hold off on the merge until I can update it again.

Thanks

Bob

[cmaglie]

@bobh66
your latest commit looks good, what problems are you facing?

[bobh66]

I wanted to do some more testing just to be safe but my Yun is having "AVR device not responding" problems.

If you are happy with the changes and/or have tested them, that's good enough for me. Hopefully I can recover this board.

[bobh66]

I tested these changes and they work fine.