feat(tnc): HdlcCodec — replace libmodem bitstream_state with pure-C++ HDLC framer#3475
Conversation
Add HdlcCodec: a pure C++ HDLC framing engine (no Qt, no libmodem, no Direwolf derivation) that replaces lm::ax25::bitstream_state, bitstreamBuffer, and candidateFrameBytes in every decode lane. Key design points: - Shift-register flag/stuff/abort detection identical to Direwolf - CRC-16-CCITT lookup table verified against both Direwolf and libmodem - 8-bit InPreamble confirmation window mirrors libmodem bitstream_size >= 8 - Byte-alignment check at frame close mirrors Direwolf olen == 0 - frameData()/frameSize() replace candidateFrameBytes — no fixed 1000-byte array Phase 1b: integrate into AetherAx25LibmodemShim. DecodeLane now holds a HdlcCodec per lane; processBit() calls hdlcCodec.processBit() then lm::ax25::try_decode_frame_no_fcs() for AX.25 structure validation. libmodem kept for HF demod and AX.25 address/frame parsing only. All 27 hdlc_codec_test cases pass; existing ax25_libmodem_shim_test continues to pass unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mments Addresses non-blocking review notes 1 and 2 from aethersdr#3473: 1. ax25DemodDescription() now appends ", N lane(s)" via a new ax25DemodLaneCount() free function computed from config — restores the suffix that was lost when the free function was extracted from the member demodDescription() during the threading refactor. 2. Restore two explanatory comments dropped during ax25BuildTransmitAudioFromFrame extraction: the minimum-frame-size invariant (14 addr + 1 ctrl = 15 bytes, no FCS) and the "host omits FCS; TNC appends it" note. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Nice work, @K5PTB. The codec is a clean, self-contained replacement for bitstream_state, and lifting the responsibility out of libmodem makes the framing path much more testable. The CRC table + algorithm cross-check against both libmodem and Direwolf is a good belt-and-braces touch. Scope is tight — the two restored comments in ax25BuildTransmitAudioFromFrame and the new ax25DemodLaneCount() helper are reasonable carryovers from #3473 review.
One non-blocking observation on a subtle edge case:
HdlcCodec::closeFrame() does not reset byte-assembly state for the next frame.
At the close call, m_currentByte == 0x7E (the 7 accumulated ones of the closing flag), m_bitIndex == 7, and m_frameByteCount == N. State is set to InPreamble, but beginFrame() is not called. The current InPreamble "not a flag" branch then calls accumulateBit(decoded) and increments m_bitsAfterFlag without clearing those carry-overs.
This is safe as long as there is at least one further flag (0x7E) before any data bit — the InPreamble flag branch calls beginFrame(), which clears the state. That is the path your testTwoConsecutiveFrames exercises (preamble = 5, postamble = 2 → many inter-frame flags), and the on-air APRS path with normal txdelay. But for back-to-back frames separated by a single shared closing/opening flag (allowed by HDLC, not tested), the next frame's first byte would be assembled on top of m_currentByte = 0x7E at bitIndex = 7, writing a garbage byte into m_frameBuffer[N] and continuing to append past the closed frame's bytes — FCS for that next frame would then never validate.
Suggested fix in closeFrame() after the FCS work:
// Reset byte-assembly state so a single shared closing/opening flag
// (no extra inter-frame preamble) doesn't leak m_currentByte / m_bitIndex
// into the next frame. Preserve m_frameSizeBits / m_fcsValid / actualFcs /
// expectedFcs / preambleCount for callers to read after this returns.
m_currentByte = 0;
m_bitIndex = 0;
m_frameByteCount = 0;
m_rawBitsInFrame = 0;
m_bitsAfterFlag = 0;(Equivalent to inlining the parts of beginFrame() that don't clobber the just-published frame metadata.) A unit test that hands a single shared flag between two encoded frames would lock it in.
Also a smaller note: ax25DemodDescription() now derives the lane count from ax25DemodLaneCount(cfg) rather than from m_impl->lanes.size(). They agree today because both follow the same formula, but the two are no longer cross-checked — if the lane construction ever gains a runtime branch (a tighter Hf300 pruning, a low-CPU profile, etc.) the description will silently drift. Not worth fixing now, but worth a // Keep in sync with lane construction in Impl::reconfigure style note on ax25DemodLaneCount().
Otherwise this looks good — the shim refactor reads cleanly, the FCS cross-check tests give good coverage, and your on-air validation across all three platforms plus the explicit "300-baud HF unchanged" risk note is exactly the right level of disclosure. Thanks for the well-structured PR and for splitting #3466 into reviewable pieces.
🤖 aethersdr-agent · cost: $15.9635 · model: claude-opus-4-7
… HDLC framer (aethersdr#3475) ## Summary Second of three focused PRs extracted from aethersdr#3466 per @jensenpat's review guidance, implementing part of the design laid out in aethersdr#2701 (Native Soft-TNC + APRS Integration). Introduces `HdlcCodec`, a self-contained pure-C++ HDLC framer with CRC-16-CCITT, and wires it into the VHF 1200-baud decode lane in place of libmodem's `bitstream_state`. libmodem is retained for HF demodulation and AX.25 frame parsing. Also addresses non-blocking review notes 1 and 2 from aethersdr#3473: lane count restored in `ax25DemodDescription()` via `ax25DemodLaneCount()`, and two explanatory comments restored in `ax25BuildTransmitAudioFromFrame`. ## Motivation libmodem's `bitstream_state` is an opaque internal struct — not unit-testable in isolation and not replaceable without forking libmodem. Extracting HDLC framing into `HdlcCodec` makes the state machine independently testable, and establishes the boundary where the Direwolf demodulator (PR 3) will plug in. ## Changes - **New:** `src/core/tnc/HdlcCodec.h` / `HdlcCodec.cpp` — pure C++ HDLC bit-unstuffing and CRC-16-CCITT framer - **New:** `tests/hdlc_codec_test.cpp` — 27 tests covering state transitions, FCS validation cross-checked against libmodem, two-frame decode, long payload, and reset - `CMakeLists.txt`: added `HdlcCodec.cpp` to main build and shim test; added `hdlc_codec_test` target - `AetherAx25LibmodemShim`: `DecodeLane` replaces `bitstreamState` with `HdlcCodec hdlcCodec` - `ax25DemodLaneCount()` free function added; `ax25DemodDescription()` now includes lane count suffix - Two explanatory comments restored in `ax25BuildTransmitAudioFromFrame` (minimum-frame-size invariant; host-omits-FCS note) Squashed-from: aethersdr#3475 Co-authored-by: Paul <274291579+K5PTB@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jensenpat <84298137+jensenpat@users.noreply.github.com>
Summary
Second of three focused PRs extracted from #3466 per @jensenpat's review guidance, implementing part of the design laid out in #2701 (Native Soft-TNC + APRS Integration). Introduces
HdlcCodec, a self-contained pure-C++ HDLC framer with CRC-16-CCITT, and wires it into the VHF 1200-baud decode lane in place of libmodem'sbitstream_state. libmodem is retained for HF demodulation and AX.25 frame parsing.Also addresses non-blocking review notes 1 and 2 from #3473: lane count restored in
ax25DemodDescription()viaax25DemodLaneCount(), and two explanatory comments restored inax25BuildTransmitAudioFromFrame.Motivation
libmodem's
bitstream_stateis an opaque internal struct — not unit-testable in isolation and not replaceable without forking libmodem. Extracting HDLC framing intoHdlcCodecmakes the state machine independently testable, and establishes the boundary where the Direwolf demodulator (PR 3) will plug in.Changes
src/core/tnc/HdlcCodec.h/HdlcCodec.cpp— pure C++ HDLC bit-unstuffing and CRC-16-CCITT framertests/hdlc_codec_test.cpp— 27 tests covering state transitions, FCS validation cross-checked against libmodem, two-frame decode, long payload, and resetCMakeLists.txt: addedHdlcCodec.cppto main build and shim test; addedhdlc_codec_testtargetAetherAx25LibmodemShim:DecodeLanereplacesbitstreamStatewithHdlcCodec hdlcCodecax25DemodLaneCount()free function added;ax25DemodDescription()now includes lane count suffixax25BuildTransmitAudioFromFrame(minimum-frame-size invariant; host-omits-FCS note)Test plan
Tested on all three target platforms.
Unit tests —
hdlc_codec_test,ax25_libmodem_shim_test,ax25_frame_formatter_test,tnc_terminal_testall pass on all three platforms (these also retroactively cover the PR 1 shim refactor):Basic VHF decode on-air — frames decoded on 144.390 MHz APRS on Mac M2, RPi 5, and Win 11. Decode rate consistent with PR 1 baseline; no regression from codec change.
TX path — UI frame transmitted; confirmed key, send, and unkey without crash or hang. Hardware note: transverter attenuated to prevent RF output — full software TX path exercised but no RF generated.
Clean shutdown — dialog closed while modem enabled; no hang or crash on all three platforms.
RX sensitivity note: Consistent decode requires a strong local signal (HT at close range). This is a known pre-existing libmodem limitation addressed in PR 3 (Direwolf demodulator), not a regression.
300-baud HF decode — not tested on-air; HF path uses libmodem's HDLC unchanged;
HdlcCodeconly affects the VHF 1200-baud lane. Risk is low but noted.Extracted from #3466 (closed). Depends on #3473 (merged).
Note: The follow-on PR adding the Direwolf demodulator (feat/ax25-direwolf-demod) is ready but will require extended comparison testing before it can be submitted. It will be opened against
mainafter this PR merges and testing is complete.