fix reader buffer truncating multi-frame UDP datagrams (#245)#253
Open
evanofficial wants to merge 1 commit into
Open
fix reader buffer truncating multi-frame UDP datagrams (#245)#253evanofficial wants to merge 1 commit into
evanofficial wants to merge 1 commit into
Conversation
Member
|
Hello, i checked the source code of mavp2p (that i wrote myself), ardupilot and MAVproxy, and none of those coalesce multiple Mavlink frames into a single UDP packet. In mavp2p / gomavlib, each Mavlink frame is written into a single UDP packet by calling the socket's Line 239 in b1ab42a The same happens in Ardupilot: and MAVproxy: where are your packets coming from? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #245.
Root cause
The
frame.Readerwraps its underlyingio.Readerin abufio.Readersized to 512 bytes (the maximum length of a single MAVLink V2 frame:len(magic) + len(header) + 255 + len(check) + len(sig)≈ 280, doubled by the original author as a safety margin).That sizing is fine for stream transports (serial, TCP), but it is wrong for UDP. On UDP the underlying
Read()consumes exactly one datagram per call, and any bytes that do not fit into the caller's buffer are silently discarded by the kernel (SOCK_DGRAM semantics). ArduPilot, mavp2p, and mavproxy commonly coalesce several MAVLink frames into a single UDP datagram; once the datagram exceeds 512 bytes the trailing bytes are dropped, so the next read picks up a fresh datagram mid-frame and the parser emits the symptoms reported in the issue:The same stream works over TCP (no datagram boundaries to lose) and in QGroundControl / MAVProxy (they size their receive buffers for the full UDP MTU), which matches the report.
Fix
Raise the internal buffer to 65536 bytes so a single
fill()can hold any plausible UDP datagram (max UDP payload is 65507). The buffer is allocated once perReader, not per packet, so there is no hot-path allocation and no impact on the TCP / serial code paths.Test
Added
TestReaderUDPMultiFrameDatagraminpkg/frame/reader_test.go. It uses a smallio.Readerthat mimics UDP semantics (one datagram perRead, tail discarded if it does not fit) and feeds the Reader a single datagram containing three V2 frames totalling ~590 bytes. With the previous 512-byte buffer the third frame is corrupted; with the fix all three decode cleanly.