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](../../docs/issues/2026-10-03-funknetz-station/spec.md) (workspace issue tracker)  
**Standards:** [knowledge/engineering/go/](../../knowledge/engineering/go/), [CONTEXT.md](../CONTEXT.md), [docs/adr/](./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

```sh
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](../../docs/research/2026-10-03-fm-funknetz-go-client.md), lab notes in [docs/svxlink.md](./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 |