do not trust received msg length to read forever

[arve0]

The bridge will read up to 2^8 bytes of data, no matter what rxlen is fed to BridgeClass::transfer. This will hang the bridge upon erroneous communication (as when Yun boots #2351). This PR fixes this. It breaks out of the read loop (when trying to further read and discard bytes) beyond what the read buffer can hold.

Might also want to check out #2353, which also adds a check for the bridge, so that one can restart the bridge from the arduino side.

[facchinm]

Hi @arve0 , I'm merging this pull request but not #2353 (comments in the other thread)

[cmaglie]

The l variable contains the expected size of the packet being received. The meaning of the original loop is:

1. Read & copy packet payload until it fits on the transfer buffer size (rxlen).
2. Read & discard all the remaining characters.
   Notice that the CRC is calculated using the full packet (even if some characters are not copied in the transfer buffer)

Now to get back to the issue: if an unlucky error happens (and l gets a randomly high value) the loop may be blocked, but IMHO we should consider to use a timeout here instead of ignoring the remainder of the packet that does not fit in the buffer.

I see that a timeout was already implemented in the loop:

for (uint16_t i = 0; i < l; i++) {
  int c = timedRead(5);        <------- Read with a timeout of 5 ms
  if (c < 0)
    continue;
  // Cut received data if rxbuffer is too small
  if (i < rxlen)
    rxbuff[i] = c;
  crcUpdate(c);
}

I guees that the main issue with this code is the use of continue (that do not leave the loop) instead of break. So my suggestion is to change it with a break a write a proper exit condition.

[arve0]

@cmaglie If I understand you correctly, then the loop will break only if not receiving bytes (negative timedRead)?

[cmaglie]

Exactly.

To say in other words:

• the functions expects l bytes to come
• the for loop read those l bytes and eventually discard some of them if they not fit into the destination buffer

what is missing is the exit condition in case those l bytes for some reason do not arrive so:

• if inside the for loop you can't read a byte within 5 ms consider the packet lost and exit

[arve0]

And if not reading them, the bytes will hang somewhere until they are read? So in other words, they will be read upon next transfer?

[cmaglie]

Yes, if you do not read bytes from serial you will get them in the next round of transfer.

BTW if the time-out happens it means that there are no bytes waiting in the queue, so the next transfer will probably be in sync.

[arve0]

Sounds reasonable to break at c < 0 then, with the minor detail on trusting the remote to decide whether to read 2^16 bytes (I see now that l is two bytes long).

[arve0]

A "solution" might be adding a flush function instead, so that the programmer can control this him self in the main loop.

[facchinm]

Well, both the solutions have their side effects:

• @arve0 's one will make impossible for someone to call a transfer that triggers an n bytes response while he is only interested in the first m bytes (so he allocates rxbuffer with length m ,with m < n) -> in my opinion it's a bad practice that should not be encouraged.
• @cmaglie 's allows a continued message (boot printk from the Linux side, for example) to hold the function for a long time, while it's perfectly clear from the 2nd byte received that the packet will be broken, and the function can return an error almost immediately