feat(modem): built-in TNC terminal — connected-mode AX.25 client for packet BBS#3381
feat(modem): built-in TNC terminal — connected-mode AX.25 client for packet BBS#3381jensenpat wants to merge 6 commits into
Conversation
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks @jensenpat — this is a serious piece of work, and the protocol-bug commentary in Ax25Connection reads as live-fire forensics rather than spec recitation. The split of TncTerminal mirroring PmsMailbox is clean: both are RF-agnostic, both share the same Ax25Connection state machine, both run their bytes through the GUI's one-at-a-time TX queue. HeardList factored out so the planned digipeater can reuse it is the right call. 13 tests on the terminal side (lost-UA adoption on both I and RR-poll, REJ resend, reject-exception suppression, invalid-N(R), VIA path) is a strong harness.
A few items worth a look. Most are minor; flagging them now since the connection/state-machine surface is what other PRs will reuse.
Findings
-
HeardList::record()writes JSON on every frame (src/core/tnc/HeardList.cpp:72,:94). Every decoded AX.25 frame — including channel chatter the operator isn't connected to — triggers a full file rewrite. On a quiet 1200-baud VHF link that's fine; on a shared APRS frequency or HF you'll be churning disk per beacon. AQTimer::singleShotdebounce (single-shot, started on dirty, ~1–5s coalesce) or a periodic flush would be cheap. Related:HeardList::save()ignores thef.open()return — a failed write disappears silently. Worth at least aqCWarningso it's not invisible. -
Ax25Connection::onFrameReceivedlost-UA fallthrough order (src/core/tnc/Ax25Connection.cpp:203-210). The adoption path callsenterConnected(m_remote)which resetsm_vs = m_vr = m_va = 0, then falls through to the switch and handles the I/RR/RNR/REJ normally. That works because (a) the peer's first I-frame is N(S)=0 = freshly-reset V(R), and (b)ackUpTo(0)is a no-op when V(A)=V(S)=0. Both invariants hold today, but they're load-bearing — worth a one-line comment in the adoption block noting "rest of the switch runs against the freshly-reset session state, which is why the I/RR cases work out of the box." Otherwise the next person who touchesenterConnected()'s reset block (or adds non-zero initial state for a future MAXFRAME>1 path) won't see the coupling. -
Vendored library fix in
third_party/libmodem_core/bitstream.h(< 18→< 17). This is a real bug — a 17-byte U-frame (SABM/DISC/UA/DM) is the entire connected-mode handshake and the old gate drops every one. Two notes: (i) please confirm there's a marker in this repo's third_party README or equivalent listing local patches against upstream, so the next libmodem_core sync doesn't silently revert it; (ii) consider sending the same change upstream — it's a single-byte off-by-one that bites any caller running connected mode, not just AetherSDR. -
PR scope is stacked on #3279 + #3290 — declared but worth re-flagging for the merger. As you note in the description, only the terminal-side files (
TncTerminal.{cpp,h}, the client paths inAx25Connection—connectTo, the lost-UA adoption block, the REJ store-and-resend,ackUpToinvalid-N(R) guard — plusHeardListsplit out, the dialog Terminal tab, the title-bar fix, andtnc_terminal_test) are this PR's true delta. Everything else (KISS server, PMS, the bitstream.h fix) needs to land via the parent PRs first or the review of this PR is unnecessarily wide. Strong preference for rebase-on-parents-merging rather than a parallel review of duplicated diff. -
Title-bar Return fix (
src/gui/FramelessWindowTitleBar.cpp) — nice. Worth mentioning in the PR description / release notes since it changes Return-key behavior across every frameless dialog in the app, not just AetherModem. (The blast radius is "good thing" but operators with muscle memory for "Return = minimize accidentally" deserve a heads-up.)
Nits (skip if rebasing)
TncTerminal::cmdMheard()usesQChar(0x2026)for the truncation ellipsis (TncTerminal.cpp:487) — fine, but a one-line// Unicode ellipsiskeepsgit blameunderstanding context.MainWindow::startKissTncOnStartupIfConfigured()does the right pattern (deferred viaQTimer::singleShot(0, ...), constructed hidden, noWA_DeleteOnClose, tracked inm_persistentDialogs), but this is part of the stacked KISS PR scope, not this PR.
What looks good
Ax25Connectioncomments on T2 deferred-ack, the reject-exception phase-lock fix, the REJ store-and-resend reasoning (// NOTE: we must NOT do m_vs = frame.nr;...), and theackUpToinvalid-N(R) guard are exactly the kind of "why" comments that survive refactors. Don't strip them.Frame::decodereturningstd::optionaland theonFrameReceivedalways-guard-then-act flow keeps every entry point safe even when the radio sends garbage.- Parent ownership (
m_link = new Ax25Connection(this),m_t1/m_t2parented to the connection,m_pms/m_terminal/m_heard/m_kissServerparented to the dialog) is consistent — no manual lifecycle management anywhere I looked. - Tests are synchronous + signal-loop-free, which keeps them fast and deterministic. The "simulated air" cross-wire is a clean idiom — worth keeping the pattern as the digipeater work lands.
LGTM modulo (1)/(2)/(3); the scope question (4) is the bigger merge-order issue. 73.
🤖 aethersdr-agent · cost: $28.0174 · model: claude-opus-4-7
|
@jensenpat — heads up regarding the stack you flagged in the PR description:
No urgency from my side. The PMS-side state machine + terminal-side state machine sharing `Ax25Connection` is exactly the kind of reusable design that earns review time — happy to take a thorough look once you've decided whether to keep them as one PR (this one, post-rebase) or split. Apologies for the merge-mechanics noise on #3290. If I'd noticed the stacked-base relationship I would have used a regular merge without `--delete-branch`. |
Add a Kantronics-style Personal Mailbox System to AetherModem. A single remote caller can connect over 1200-baud AX.25 connected mode to read, list, and send messages, list stations heard, and disconnect; a new Mailbox config tab and an hourly beacon are included. Built on aethersdr#3279. This required AX.25 v2.0 connected mode (LAPB), which the modem lacked (it only did connectionless UI/APRS). New reusable, RF-agnostic layers: - src/core/tnc/Ax25.{h,cpp}: Address + Frame parse/build (I, RR/RNR/REJ, SABM, DISC, DM, UA, FRMR, UI), mod-8, command/response C-bits. - src/core/tnc/Ax25Connection.{h,cpp}: single-connection data-link state machine (SABM->UA, V(S)/V(R)/V(A), RR acks, I-frame segmentation, T1 retransmit up to N2, REJ/RNR/DISC). Tuned for 1200-baud FM + PTT. - src/core/pms/PmsMailbox.{h,cpp}: the mailbox service (greeting, command interpreter, JSON store, heard list, beacon) in one file pair. GUI: a Mailbox tab in Ax25HfPacketDecodeDialog (enable, answer SSID, welcome/PTEXT, hourly beacon, last 5 callers, stats); settings persist in AppSettings; outbound frames share the existing PTT/DAX keying queue. Tests: pms_mailbox_test (Qt6::Core) covers frame codec round-trips, the connection handshake, and a full mailbox session; isolated via AETHER_PMS_DIR so it is repeatable and never touches a real mailbox. The connected-mode protocol layer is unit-tested; on-air RF validation at 1200 baud against a real TNC is the top follow-up (see docs/MODEM.md). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tats/callers split Iterate on the Mailbox tab per review: - Replace the answer-SSID spinbox with two free-text callsign fields and no defaults: a full LISTEN CALLSIGN (e.g. KI6BCJ-10) and an optional VANITY ALIAS. AX.25 limits a callsign to 6 characters plus an optional -SSID, so the alias must be <= 6 chars (e.g. AETBBS); the field placeholder/tooltip says so. The mailbox answers on either address; the one the caller dialed is used for the whole session (UA, greeting, replies). - Lay Statistics on the left and Last Callers on the right as separate, evenly-sized panels in one row. - Remove the "A single remote caller..." help paragraph. - Collapse MODEM STATUS / GAIN STAGE / PACKET ACTIVITY into one slim inline status bar with a compact activity strip instead of three tall panels. Model: Ax25Connection gains an optional alias address (setAliasAddress) and matches inbound frames against primary-or-alias while idle, latching onto the dialed address for the session. PmsMailbox swaps base-call+SSID for setListenCallsign()/setAliasCallsign() (parsed ax25::Address) and hasValidAddress(); onAirFrame lets the link do address matching. Settings: AetherModemPmsListenCallsign / AetherModemPmsAliasCallsign replace AetherModemPmsSsid. pms_mailbox_test gains an alias-dial case (UA + greeting sent from the alias; unrelated callsigns ignored) and address-length checks. Verified: full AetherSDR app builds clean (Ninja, macOS) and pms_mailbox_test passes on repeated runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e length gate Found while troubleshooting a live PMS connect failure (a caller's TNC connect frames were never answered). Replaying the operator's "Capture 3m" recording through the decoder, then adding a SABM AFSK loopback test, isolated a real decoder bug — separate from the original no-audio symptom. try_decode_frame() in third_party/libmodem_core/bitstream.h rejected any frame < 18 bytes, but the shortest valid AX.25 frame WITH FCS is 17: 14 address + 1 control + 2 FCS, no PID/info. That is exactly every connected-mode U-frame (SABM, DISC, UA, DM). So the decoder silently dropped all connect/disconnect/ack frames — a PMS/BBS could never be reached in connected mode even with perfect audio. UI/APRS frames carry a PID byte (>= 18), which is why connectionless decode always worked. The sibling try_decode_frame variant already used the correct < 15 (no-FCS) minimum. Fix: gate < 18 -> < 17, and align the shim's reject-classifier threshold to match. Diagnostics added alongside: - tools/ax25_replay.cpp: offline tool that replays a captured mono-float32 WAV through the decoder, sweeping both tone polarities, printing decoded frames and reject counters. Built on demand (EXCLUDE_FROM_ALL). - ax25_libmodem_shim_test: testSabmConnectFrameLoopbackDecodes builds a real SABM, renders it through the AFSK modem, and asserts it decodes back as FrameType::SABM with the right addresses. Regression guard for the gate. - PmsMailbox::onAirFrame logs every decoded frame with the listen/alias address-match decision, so a future connect shows decode-vs-mismatch at a glance on aether.ax25. Verified: ax25_libmodem_shim_test (incl. 10 SABM assertions) passes deterministically over repeated runs; pms_mailbox_test passes; full app and the replay tool build clean on macOS. Replaying the original noise-only capture decodes 0 frames across both polarities (no false-positive regression). Note: the operator's specific capture contained no packet keyups at all (flat ~-23 dBFS hiss), so their immediate issue is RX-audio routing/level — but this bug would have blocked the connect regardless once audio is fixed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…emitted signal ax25_replay connected to AetherAx25LibmodemShim::frameDecoded and counted that signal — but processMonoFloat() (which the tool calls) does not emit it; only feedAudio() does. So the tool always printed 'decoded frames: 0' even when the diagnostics showed accepted frames, which masked the real SABM decodes in a live capture. Count the processMonoFloat() return value instead, and print fcsOk and proper U/S/I frame-type names. Verified against the operator capture: now correctly reports 18 decoded SABM connect frames (KI6BCJ>KI6BCJ-10, fcsOk=1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ll in a T1 loop First live connect worked, but a multi-I-frame reply (the LIST command) stalled: the data displayed correctly on the caller's TNC, yet our side never saw an acknowledgement and retransmitted until N2 link failure. A single-frame reply (INFO) acked fine. Log (aether.ax25) showed the cause: the LIST reply went out as three I-frames back-to-back (NS=1,2,3), each its own PTT keyup, then 8 retransmits with ZERO received frames in between, then link failure. INFO was one frame followed by a clean listen window, and its RR arrived 520 ms later. On a half-duplex radio the back-to-back keyups (and the long 3-frame retransmit bursts) keep us transmitting while the peer's ack arrives, so we are deaf to it every cycle. Fix: default the send window to 1 (MAXFRAME=1) — one unacknowledged I-frame in flight at a time, so each frame is a solo keyup followed by a listen window for its ack before the next goes out. This is exactly the INFO pattern that works, repeated per frame. kWindow (was constexpr 4) becomes a configurable m_window (setWindow(), qBound 1..7) so a future single-keyup multi-frame TX path or a full-duplex transport can raise it. Regression test (pms_mailbox_test): drive Ax25Connection with a 300-byte payload (3 I-frames at paclen 128) and a peer that acks each with a standalone RR; assert only one I-frame is in flight at a time, frames advance one-per-ack, and each N(S) is transmitted exactly once (no duplicate/retransmit storm). Would have caught this. Diagnosis confidence: the half-duplex-deafness root cause is inferred from the logs (multi-frame burst -> zero RX -> loop; single frame -> ack heard) and is consistent, but not yet confirmed on the air. window=1 strictly cannot regress vs the old k=4 here and matches the proven INFO path. On-air retest of LIST is the confirmation. Verified: pms_mailbox_test (incl. 8 new window assertions) and ax25_libmodem_shim_test pass; full app builds clean on macOS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1200-baud packet BBS Adds a "Terminal" tab to AetherModem: a connected-mode AX.25 client that calls a VHF packet BBS, reads/sends messages, and disconnects, sharing the data-link machinery (Ax25Connection) with the PMS mailbox rather than duplicating it. Connection state machine (client role added to the shared Ax25Connection): - Outbound connectTo() with SABM/UA handshake, N2 retries, optional VIA digipeater path. - Lost-UA adoption: an inbound I/RR/RNR/REJ received while still connecting is treated as proof the UA was lost, and the link is adopted. - T2 deferred-ack: on a half-duplex link, defer an unpolled ack so we don't key the radio mid-burst and go deaf to the rest of the peer's window. - Silent reject-exception: send exactly one REJ per sequence gap, then listen — even on polled retransmits — to break the half-duplex REJ phase-lock that stalled multi-frame replies (e.g. a long BBS help menu). - REJ recovery resends the outstanding I-frames from the store (the old path rewound V(S) and called pumpOutbound() on an already-drained buffer, so it resent nothing and the link silently desynced). - ackUpTo() ignores an out-of-range N(R) instead of corrupting the send window. - Poll on the window-filling I-frame so the peer acknowledges promptly. - Per-session telemetry counters (I sent/resent/rcvd/dropped, RR/REJ/RNR in & out, T1 timeouts, T2 acks, FRMR, ignored bad-N(R)). Terminal UI + commands: - CONNECT [VIA digi...], BYE/DISC, CONV, STATUS, MHEARD, MYCALL, LOG, ESCAPE, HELP. - Monospace transcript, Up/Down command history, right-click Clear / Command Mode. - Quick-connect dropdown from a shared HeardList; timestamped session logging; last-called BBS persisted across restarts. - Tunable T1 / N2 / paclen and a tunable TX tail (half-duplex turnaround); live drop/resent readout in the status line. - Auto-enables the modem RX tap when a connect is initiated. Shared / refactor: - New HeardList class backing MHEARD and quick-connect (reusable by PMS and a future digipeater). - FramelessWindowTitleBar: min/max/close buttons are no longer the dialog's default button, so pressing Return in a text field no longer minimizes the window (macOS) in any AetherModem field. Tooling / tests: - tools/ax25_session_analyze: replays a capture WAV through the real decoder AND the real state machine to surface sequencing / timer / retry gaps. - tests/tnc_terminal_test: connect/converse/disconnect, VIA, MHEARD, lost-UA adoption, multi-frame deferred ack, REJ resend, reject-exception storm suppression, invalid-N(R) guard, CONV/STATUS. Stacked on aethersdr#3279 (KISS-over-TCP TNC + AetherModem UX) and aethersdr#3290 (PMS over connected-mode AX.25); this branch includes both until they merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Codex <noreply@openai.com>
ten9876
force-pushed
the
aether/tnc-terminal-tab
branch
from
0dbc08a to
307ebbf
Compare
|
@jensenpat — I went ahead and rebased the branch against current main as a follow-up to the coordination note above. The stack collapsed cleanly to 6 commits (the original 9 minus the squashed-and-merged #3279 work and the two merge-of-PR branches that no longer have content):
So #3381 now represents the PMS + Terminal as one cumulative PR. If you'd prefer to land PMS as its own PR first, the first 5 commits are easily extractable. Conflicts resolvedTwo small conflicts during the rebase — both in `Ax25HfPacketDecodeDialog.cpp` constants/includes:
Build + tests
CI will rerun against the new head (`307ebbf3`). I'm not going to dive into a full review here — you spent serious effort on this connection state machine (the half-duplex reliability work alone is substantial) and it deserves a careful read once you're confident the rebase didn't disturb anything you cared about. Let me know if anything looks off. Apologies again for the merge-mechanics noise on #3290. |
2 participants
Summary
Adds a Terminal tab to AetherModem: a built-in connected-mode AX.25 client for calling a 1200-baud VHF packet BBS — connect, read/send messages, and disconnect — with reliable error correction over a half-duplex link. It reuses the existing data-link state machine (
Ax25Connection) shared with the PMS mailbox rather than duplicating it.Stacked on #3279 and #3290
This branch is built on top of two open PRs and includes their commits until they merge:
Merge order should be #3279 → #3290 → this PR. The diff here will shrink to just the terminal work once the parents land; happy to rebase on request. (#3290 lives on the
jensenpatfork, so an upstream PR can't base on it directly — hence targetingmainwith this note.)What's new
Terminal tab — connected-mode AX.25 client (the calling-side counterpart of the PMS mailbox's answering side):
CONNECT <call> [VIA <digi>,…],BYE/DISC,CONV,STATUS,MHEARD,MYCALL,LOG,ESCAPE,HELP.~) and aCONVcommand to return.Connection / error-correction work (shared
Ax25Connection)The client role and a series of half-duplex reliability fixes, each found by analyzing on-air captures through a new replay harness:
connectTo()with SABM/UA, N2 retries, and an optional VIA digipeater path.I/RR/RNR/REJwhile still connecting is treated as proof the UA was lost; adopt the link (connect went live but no data otherwise).ackUpTo()guard — ignore an out-of-range N(R) instead of corrupting the send window.STATUSand the live readout.Shared / refactor
HeardList— new class backingMHEARDand quick-connect (SSID, last-heard, last beacon; JSON-persisted). Reusable by PMS and a future digipeater.FramelessWindowTitleBar— title-bar min/max/close buttons are no longer the dialog's default button, so pressing Return in a text field no longer minimizes the window on macOS (was happening in every AetherModem field).Tooling & tests
tools/ax25_session_analyze— replays a capture WAV through the real decoder and the real state machine, printing each RX frame, our reaction, V(R) transitions, and flagging out-of-sequence drops / REJ storms. This is how each protocol bug above was pinned down empirically.tests/tnc_terminal_test— connect/converse/disconnect, VIA path, MHEARD, lost-UA adoption, multi-frame deferred ack, REJ resend, reject-exception storm suppression, invalid-N(R) guard, CONV/STATUS. PMS tests unaffected.Testing
tnc_terminal_test,pms_mailbox_test,ax25_libmodem_shim_testall green.💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat