funknetz/docs/code-review.md

Code review: funknetz

Date: 2026-10-07
Scope: Full review of the funknetz checkout (headset station + local SvxReflector lab).
Fixed point: 8e75515 (Record the station glossary and the window and config decisions.)
Compared tip: HEAD = f90b615
Diff: git diff 8e75515...HEAD (4222 insertions across 38 files)
Spec: docs/issues/2026-10-03-funknetz-station/spec.md (workspace issue tracker)
Standards: knowledge/engineering/go/, CONTEXT.md, docs/adr/

Two axes below are independent. Findings are not merged or ranked across axes.

Commits in range (8e75515..HEAD)

Commit	Summary
9c4f662	Join one FM-Funknetz talk group from a headset window
6233292	Pause playback while transmitting and declare microphone use
ec5f660	Append an optional station log and announce the software after login
eda79e6	Add a local SvxReflector Docker lab for headset testing
f90b615	Merge branch lab/svxlink-docker

Working tree (not in HEAD diff)

At review time, uncommitted edits touched docs/svxlink.md, svxlink-docker/ (Dockerfile, compose, entrypoint, conf, .env.example, .gitignore), root .gitignore, and an untracked svxlink-docker/config/svxreflector.link.conf. Protocol and lab notes in this report include that working tree where it clarifies jayReflector Satellite/Trunk; Standards and Spec axes stay on 8e75515...HEAD only.

Tests run

GOWORK=off go test ./station/ -count=1   # ok (~10s)

main, audio, and internal/opus have no package tests. TestConnectsToLabReflector skips when 127.0.0.1:5300 is down (or FUNKNETZ_LAB=0).

---

Standards

Documented standards

Hard — modern.md (new JSON → encoding/json/v2)
station/software.go marshals with v1 encoding/json while station/config.go already uses encoding/json/v2. Same module, new code: prefer v2.

Hard — errors.md (wrap with %w; don’t drop causes)
Underlying failures are replaced with a bare string:

• app.go / config path resolution paths that surface could not be read
• station/config.go loadConfig / parseConfig / rewrite path: same "could not be read" without %w

Hard — testing.md (prefer t.Fatal over panic)
station/config_test.go configJSON / configWithLog: panic(err) on marshal failure.

Judgement — errors.md / modern.md (errors.Is)
station/config.go: os.IsNotExist(err) — prefer errors.Is(err, fs.ErrNotExist).

Judgement — concurrency-context.md + functions-methods.md (synchronous APIs; obvious goroutine lifetime)
audio.Start returns after starting a cleanup goroutine on ctx.Done(). app.go fire-and-forgets Run with _ = (comment present; discard rule satisfied).

Judgement — modern.md (slices.Contains)
station/protocol.go hasOPUS hand-loops instead of slices.Contains.

OK / tooling skip
_ = sendUDP(...) sites carry justifying comments (errors.md). gofmt/layout not reviewed. App pass-through methods are required by ADR 0001-wails-station-window.md (overrides Middle Man).

Baseline smells (all judgement)

Divergent Change — station.Station / session.go: connection, TX, Opus, UDP, UI state, logging in one type.

Data Clumps — onTCP / onServerInfo pass udpCh, udpErr, udpReady, udpRx, lastTCP as pointers together.

Duplicated Code — audio/device.go repeats channel-count logic for capture vs playback; reflector_test.go reimplements append/parse helpers already in protocol.go.

Mysterious Name — loaded, fileShape, up / outage don’t say what they hold.

Primitive Obsession — State.Connection as free strings ("Connecting" / "Connected" / …); session reason as "tcp"/"udp"/"timer".

Middle Man — Fail → stop → endFatal (thin hop).

---

Spec

Tests: GOWORK=off go test ./... — station OK (~10s); main / audio / internal/opus have no tests. TestConnectsToLabReflector skips when 127.0.0.1:5300 is down (or FUNKNETZ_LAB=0).

(a) Missing / partial

• Separate default devices (partial). Spec: “A device adapter behind the station talks to the operating system’s default input and default output” / US 52. audio.Start opens one malgo.Duplex device, not independent default input and default output—fails if duplex is unavailable even when separate devices would work.
• Broken-JSON wording (partial). Spec: “quit and say the file could not be read” (US 10). Implementation exits with could not be read (no “the file”).

Stories 1–9, 11–50, 53–62, 64–74 and the protocol / log Implementation Decisions look covered at the station seam (config, login, TG select, OPUS, PTT, heartbeats, retries, fatalities, log).

(b) Scope creep

• Local reflector lab. Spec Solution / US 1–74 never ask for Docker or SvxReflector; Testing Decisions: “An in-process reflector double is the stand-in for TCP and UDP.” Diff adds svxlink-docker/, docs/svxlink.md, and station/lab_reflector_test.go (real TCP to :5300).
• Audio-open fatal. Spec never defines device-open failure copy. app.go calls Fail("could not open the audio device"), inventing an English sentence and stopping retries outside US 57–59.

(c) Looks implemented, looks wrong

• Duplex vs defaults (same as (a)). Spec wants default input and default output; duplex is a different contract and can reject a valid headset/speakers pair.
• No other clear behavioral mismatches found against US 1–74 / protocol / log rules at HEAD (8e75515...HEAD). Wails labels, callsign/TG, PTT OR of button+Space, flush-on-release, busy-room inert PTT, and log line shapes match the quoted decisions.

---

Protocol deep dive: SvxLink vs funknetz

There is no separate RFC. The contract is SvxLink / jayReflector message types (ReflectorMsg.h and client logics). Funknetz targets protocol 2.0 (ReflectorV2 / AUTH_KEY), matching FM-Funknetz’s published guidance. Background: docs/research/2026-10-03-fm-funknetz-go-client.md, lab notes in docs/svxlink.md.

1. What funknetz implements

Framing. TCP: 4-byte BE length + uint16 type + body (station/protocol.go). UDP V2 header: type, client id, sequence (station/session.go). Strings/blobs: uint16 length + bytes. Max TCP frame 32768.

TCP types handled or sent (protocol.go, session.go, software.go):

Type	Role in funknetz
5	Send ProtoVer 2.0 first
10 / 11 / 12	HMAC-SHA1 challenge (20 B) → callsign + digest; AuthOK is a no-op
13	Fatal string → stop retries
100	Parse reserved, client id, nodes, codecs; require "OPUS"
111	Send NodeInfoV2 JSON {"sw","swVer"} only
106	Send configured TG once after 111
104 / 105	Talker start/stop: honor only matching TG; own callsign start ignored
1	TCP heartbeat

Unknown TCP types are ignored.

UDP: send/recv type 1 heartbeat; 101 audio (uint16 len + Opus); 102 flush on PTT release (after padding a partial frame). Drop datagrams whose client id ≠ ours; no sequence check on RX. While transmitting, discard RX audio.

Auth / codecs / Opus. Auth key never on the wire. Opus-only: 16 kHz mono, 320 samples / 20 ms, 20 kbit/s VBR, voice, medium-band, DTX off (internal/opus/opus.go).

Heartbeats / talker rules. Match ReflectorV2 counters: TCP TX 10 s / RX 15 s; UDP TX 15 s / RX 60 s. Room busy from talker-start (matching TG) or any accepted RX audio; PTT inert while busy; mic never looped to playback.

Tests assert: login ProtoVer 2.0, HMAC digest, NodeInfo keys, SelectTG, initial UDP HB seq 0; heartbeats, retries, MsgError / no-OPUS stop retry, TCP/UDP timeouts, TG-filtered talker, busy blocks TX, PTT + flush 102, seq wrap, wrong-client-id audio dropped.

2. Parity with SvxLink ReflectorV2

Capability	Upstream (ReflectorV2 / research)	funknetz
Proto 2.0 + AUTH_KEY HMAC-SHA1	ReflectorV2Logic	Yes
Framed TCP + UDP same host/port	AsyncFramedTcp / V2 UDP	Yes
MsgServerInfo → client id + codecs	type 100	Yes (OPUS required)
MsgSelectTG	106	Yes
MsgNodeInfoV2	111 JSON	Yes (minimal)
TalkerStart/Stop with TG	104 / 105 (≥ proto 2)	Yes
UDP HB / Audio / FlushSamples	1 / 101 / 102	Yes
One talker/TG; client mute when busy	Reflector + client	Client-side busy
Heartbeat timers 10 / 15 / 15 / 60	ReflectorV2Logic.h	Yes
Opus defaults 16 kHz / 20 ms / 20 kbit	AsyncAudioEncoderOpus	Yes

3. Differences

Intentional (spec Out of Scope): protocol 3 / certs / AES-GCM UDP; MsgTgMonitor / QSY / DTMF; DNS SRV / baked-in host; GSM / Speex / RAW; rich node-info; multi-TG; station list / siglev UI.

Thinner than ReflectorV2 client:

Topic	Upstream V2	funknetz
MsgProtoVerDowngrade (6)	Client should retry lower ver	Ignored
RX UDP sequence / peer filter	Drop behind-seq; peer = TCP peer	Client-id only; connected UDP socket
MsgUdpAllSamplesFlushed (103)	Flush handshake	Not sent/handled
TalkerStop vs late audio	Audio may still arrive after 105	Stop clears busy; any later 101 re-sets roomBusy without a named talker (session.go onDatagram) — can strand PTT
Pre-auth size cap	64 B	Always up to 32768
MONITOR_TGS / StateEvent / Signal / TxStatus	Optional client telemetry	Absent
Codecs	First mutually supported	Hard-fail if no OPUS
jayReflector type 121 trunk node list	Extra	Ignored (OK)

4. Currently missing

(A) Intentionally out of scope for v1 headset — Proto 3 / encryption; MsgTgMonitor; MsgRequestQsy; NodeJoined / Left UI; StateEvent / SignalStrength / TxStatus; DNS SRV; non-OPUS codecs; rich NODE_INFO; multi-TG.

(B) Gaps that can still affect ReflectorV2 / FM-Funknetz / jayReflector headset use

1. Late audio after TalkerStop re-busying the room without a named talker (upstream documents post-stop audio).
2. No ProtoVerDowngrade handling (unlikely on FM’s V2 servers; breaks if a server asks to step down).
3. No UDP flush / all-flushed — usually fine for app clients; edge cases if a peer expects 103.
4. No SRV — operators must set host/port (FM docs often use 5308 + DNS_DOMAIN on nodes).
5. No MsgTgMonitor — idle monitoring of other TGs unavailable; single SelectTG is enough for one configured TG.
6. Stricter RX sequence not implemented — more permissive than V2; rarely blocks interop.

Core login + SelectTG + Opus UDP path is aligned with FM-Funknetz’s ReflectorV2 guidance.

5. Lab stack

Stack	Role
funknetz svxlink-docker	Builds jayReflector → svxreflector; TCP+UDP 5300; [USERS] / [PASSWORDS] for proto 2; optional Satellite (outbound 5303) / Trunk (5302); clients stay on local :5300
Stock sm0svx SvxReflector	Same client wire protocol; no reflector peering
jayReflector Satellite / Trunk	Server-side mesh / uplink; Funknetz does not speak Trunk / Satellite

Headset interop testing needs a ReflectorV2-capable svxreflector with OPUS and AUTH_KEY users. Satellite / Trunk only matter when exercising FM uplink, not the client protocol itself.

Message coverage (quick map)

Direction	Implemented	Not implemented (by design unless noted)
TCP send	5, 11, 1, 111, 106	14–21 (proto 3), 107 monitor, 109 QSY, 110–113 telemetry
TCP recv	1, 10, 12, 13, 100, 104, 105	6 downgrade (gap), 101 NodeList (legacy), 102/103 presence, 114 UDP encrypt
UDP send	1, 101, 102	103 all-flushed, 104 signal
UDP recv	1 (timer), 101	sequence validation, 102/103 handshake

---

Summary

Standards: 3 hard findings (json/v2 in software.go; bare could not be read dropping causes; panic in test helpers); several judgement smells — worst hard issue is inconsistent JSON API / dropped error causes on config read.
Spec: 2 partial gaps (duplex vs separate default devices; broken-JSON wording) + 2 scope-creep items (Docker lab; audio-open fatal copy) — worst issue is duplex audio vs the specified separate default input/output devices.

Follow-up (2026-10-07)

Hard Standards and Spec gaps addressed in code where possible:

Finding	Status
software.go → encoding/json/v2	Fixed
Config / path errors wrap with %w; US 10 wording the file could not be read	Fixed
Test helpers use t.Fatal instead of panic	Fixed
Separate default Capture + Playback devices (not Duplex)	Fixed
Docker lab scope creep	Left in place — intentional engineering aid, not a product story
Audio-open Fail("could not open the audio device")	Left — spec has no copy; stopping without audio is still required UX