fix(kiss): decode frames sharing a single FEND delimiter#444
Open
chrissnell wants to merge 1 commit into
Open
Conversation
The KISS decoder ran its 'skip until FEND' resync on every Next() call. FEND is a frame delimiter, not a per-frame wrapper: the byte right after a frame's closing FEND begins the next frame. TNCs that share one FEND between back-to-back frames, or emit only a trailing FEND per frame, had their packets silently discarded (every-other frame, or effectively all frames when packets arrive with idle gaps) — the 'connects fine but receives no packets' reports on the tcp-client path. The server and serial paths share the same decoder and had the same latent bug. Sync on the first FEND once per stream, then never discard frame bytes again; the read loop's empty-frame skip already absorbs leading/shared delimiters. Only the first pre-sync frame in trailing-FEND-only framing is (unavoidably) lost.
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
1 participant
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.
Problem
A user running Graywolf as a KISS TNC client (tcp-client), dialing a real hardware TNC over the network, reported receiving no packets — while other apps connected to the same TNC saw traffic fine. This is a recurring "connects fine but no packets" report.
Root cause
kiss.Decoder.Next()ran a "skip bytes until the next FEND" resync at the top of every call. But FEND (0xC0) is a frame delimiter, not a per-frame wrapper — the byte immediately after a frame's closing FEND is the start of the next frame.The decoder only worked for TNCs that wrap every frame
C0 <frame> C0(a leading FEND per frame). TNCs that instead:C0 f1 C0 f2 C0), orhad their frames thrown away: after emitting one frame (and consuming its trailing FEND), the next
Next()discarded the following frame's bytes while hunting for a leading FEND that never comes. Result: every-other frame lost, or effectively all frames when packets arrive with idle gaps between them.The server-listen and serial paths share the same decoder and had the same latent bug; it just surfaced most visibly on the newer tcp-client-to-real-TNC path.
Fix
Sync on the first FEND once per stream (still discards a TNC text banner or a mid-stream-connect partial frame), then never discard frame bytes again. The read loop's existing empty-frame skip (
len(buf)==0 → continue) already absorbs leading FENDs, empty frames, and shared delimiters. Only the first pre-sync frame in trailing-FEND-only framing is unavoidably lost (standard KISS sync behavior).Tests
TestDecodeSharedDelimiter— three frames sharing one FEND each → all decode.TestDecodeTrailingFendOnly— trailing-FEND-only stream → first (pre-sync) frame lost, rest decode.TestDecodeMultipleFrames,TestDecodeSkipsLeadingFends, round-trip and escape tests still pass.go test ./pkg/kiss/...and./pkg/app/...green;go build ./...andgo vet ./pkg/kiss/...clean.Wiki: added invariant #58 documenting the FEND-delimiter contract so this isn't reintroduced.