Incorrect message frame detection

[tjrmarques]

The current implementations of both read_v1_msg and read_v2_msg check the CRC after reading what is perceived as a complete MAVLink frame (starting with the MAV_STX and MAV_STX_V2 start-of-frame markers respectively), and discards the full message (and the corresponding bytes) if the CRC fails.

The issue is that the byte detected as the start-of-frame marker byte may actually just be a regular byte in the middle of a message and the true start-of-frame marker byte can be a part of the discarded bytes from the discarded frame, which prevents frame re-synchronisation on the next available message (meaning message loss) and actually provides no guarantee of eventual synchronisation. This situation can happen both when initially connecting to a MAVLink data stream, or when reading from an unreliable channel, where some bytes may have been lost.

Moreover, the CRC is a part of how MAVLink actually defines a frame and the only way it can distinguish between a true start-of-frame marker and other bytes with the same value in the middle of a message for proper framing. For this reason, checking the checksum should be a part of the MAVLink frame extraction step (read_v1_raw_message / read_v2_raw_message) and not a yes/no check on the message decoding step.

[tjrmarques]

As an example, the process_file::get_file test parses 1374 valid MAVLink messages, although there are actually 1426 valid MAVLink messages present in the file (that's a 3.6% loss). In this specific test, this happens because the used tests/log.tlog includes an additional 8 bytes (timestamp?) between each MAVLink message/frame and every time there's a MAV_STX_V2 (0xFD) byte among those 8-bytes, at least one immediately subsequent message is lost (often more).

[patrickelectric]

Hi @tjrmarques, right now we don´t have a buffer to deal with such conditions on the parser, it would be nice to add such thing on the project.

[tjrmarques]

Hi @patrickelectric, I experimented with using a buffered reader (e.g. from the bufered_reader crate) or pushback buffer (loosely based on the pushback_iter crate), and they both seem to work. I think this needs to break the API, as the read_v1_msg, read_v2_msg and their raw versions would need to take that "buffered/pushback reader" instead of a impl Read. Additionally, MavConnections implementations would need to construct it from their underlying streams (impl Read).

I can give it a go and create a PR, if you can provide some guidance on the preferred solution. (it may also take some time, I'll need to remove some non-relevant code - custom dialect, and rebase against the latest fairly comprehensive restructuring commit...)