From bf3777bdad759b4233fc23e75ccc53b87447bc34 Mon Sep 17 00:00:00 2001 From: Jake Thomas Date: Sat, 18 Jul 2026 21:35:27 -0400 Subject: [PATCH 1/2] feat: harden runtime and add conformance evidence --- .github/workflows/lint.yml | 2 +- .github/workflows/security.yaml | 4 +- .github/workflows/test.yaml | 36 ++- .gitignore | 9 + AGENTS.md | 2 + CHANGELOG.md | 32 ++ CONTEXT.md | 65 ++++ CONTRIBUTING.md | 36 +++ README.md | 164 ++++++++-- SECURITY.md | 13 + broadcast.go | 138 ++++++++- broadcast_test.go | 60 +++- bus.go | 15 +- client.go | 411 ++++++++++++++++++++----- client_test.go | 169 ++++++++++ commandedaddress_test.go | 256 +++++++++++++++ conformance/evidence.example.json | 43 +++ devicename.go | 28 +- docs/adr/0001-bounded-runtime-state.md | 24 ++ docs/adr/0002-wire-fidelity.md | 23 ++ docs/architecture.md | 86 ++++++ docs/conformance.md | 106 +++++++ groupfunction.go | 2 +- internal/adapter/adversarial_test.go | 73 +++++ internal/adapter/canadapter.go | 9 +- internal/adapter/multibuilder.go | 86 +++++- internal/adapter/sequence.go | 81 ++--- internal/canbus/cancellation_test.go | 31 +- internal/canbus/socketcan.go | 26 +- internal/canbus/usbcan.go | 109 +++++-- internal/canbus/usbcan_test.go | 66 +++- internal/claiming/claimer.go | 73 ++++- internal/claiming/claimer_test.go | 176 +++++++++-- internal/framer/canid.go | 2 +- internal/framer/canid_test.go | 2 +- internal/gateway/actisense.go | 15 + internal/gateway/actisense_test.go | 34 ++ internal/transport/bam.go | 11 +- internal/transport/constants.go | 7 + internal/transport/rtscts.go | 37 ++- internal/transport/rtscts_test.go | 139 +++++++++ internal/transport/transport.go | 228 +++++++++++++- internal/transport/transport_test.go | 75 +++++ justfile | 32 +- messagehub.go | 140 +++++++++ messagehub_test.go | 61 ++++ n2k.go | 1 + options.go | 92 +++++- options_test.go | 43 +++ pgn/codec.go | 49 ++- pgn/codec_fuzz_test.go | 24 ++ pgn/messageinfo.go | 18 +- pgn/pgndatastream.go | 25 +- pgn/pgndatastream_additional_test.go | 15 + pgn/pgndatastream_test.go | 2 - pgn/pgndatastream_writer.go | 10 + pgn/pgndatastream_writer_test.go | 10 + pgn/pgninfo.go | 18 +- pgn/pgninfo_additional_test.go | 7 + pgn/preserve_raw_test.go | 28 +- pipeline.go | 20 +- pipeline_test.go | 8 +- registry.go | 30 ++ registry_test.go | 25 +- request.go | 2 +- scanner.go | 62 +++- status.go | 46 +++ status_test.go | 29 ++ system.go | 2 +- units/distance.go | 18 +- units/distance_test.go | 25 +- writeresult.go | 16 + writeresult_test.go | 11 + 73 files changed, 3458 insertions(+), 415 deletions(-) create mode 100644 CONTEXT.md create mode 100644 CONTRIBUTING.md create mode 100644 SECURITY.md create mode 100644 commandedaddress_test.go create mode 100644 conformance/evidence.example.json create mode 100644 docs/adr/0001-bounded-runtime-state.md create mode 100644 docs/adr/0002-wire-fidelity.md create mode 100644 docs/architecture.md create mode 100644 docs/conformance.md create mode 100644 internal/adapter/adversarial_test.go create mode 100644 messagehub.go create mode 100644 messagehub_test.go create mode 100644 pgn/codec_fuzz_test.go create mode 100644 status.go create mode 100644 status_test.go diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index d39308f..d174cb1 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -22,4 +22,4 @@ jobs: - name: golangci-lint uses: golangci/golangci-lint-action@v9 with: - version: latest + version: v2.12.0 diff --git a/.github/workflows/security.yaml b/.github/workflows/security.yaml index e74ca36..aafb860 100644 --- a/.github/workflows/security.yaml +++ b/.github/workflows/security.yaml @@ -23,12 +23,12 @@ jobs: env: GOTOOLCHAIN: go1.25.12 run: | - go install golang.org/x/vuln/cmd/govulncheck@latest + go install golang.org/x/vuln/cmd/govulncheck@v1.5.0 govulncheck ./... - name: Run gosec env: GOTOOLCHAIN: go1.25.12 run: | - go install github.com/securego/gosec/v2/cmd/gosec@latest + go install github.com/securego/gosec/v2/cmd/gosec@v2.27.1 gosec -exclude-generated -exclude=G115 ./... diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index f2378a4..59e6b7e 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -1,12 +1,17 @@ name: Test on: + pull_request: push: - branches: ["**"] + branches: [main] jobs: test: - runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] + runs-on: ${{ matrix.os }} steps: - name: Checkout @@ -22,7 +27,34 @@ jobs: uses: extractions/setup-just@v3 - name: PGN sync check + if: runner.os == 'Linux' run: just pgn-sync-check + - name: Local conformance evidence + if: runner.os == 'Linux' + run: just conformance-local + - name: Test run: go test ./... + + race-and-fuzz: + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 1.23.x + check-latest: true + + - name: Set up just + uses: extractions/setup-just@v3 + + - name: Race detector + run: just test-race + + - name: Fuzz smoke tests + run: just fuzz-smoke diff --git a/.gitignore b/.gitignore index 0494774..2708d3a 100644 --- a/.gitignore +++ b/.gitignore @@ -22,5 +22,14 @@ dist/ # Scratch scratch/* +# Licensed/certification evidence is local to the manufacturer. The tracked +# example under conformance/ documents the metadata shape without test output. +conformance-artifacts/ + # Internal docs docs/* +!docs/architecture.md +!docs/conformance.md +!docs/adr/ +docs/adr/* +!docs/adr/*.md diff --git a/AGENTS.md b/AGENTS.md index c6b6a14..dd9ae0f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,5 +1,7 @@ # Agent Requirements +- Read `CONTEXT.md` before architecture or runtime changes; preserve its + invariants and update it when domain language or module ownership changes. - Before pushing, opening a PR, or marking work complete, run and pass: - `go test ./...` - `just pgn-sync-check` diff --git a/CHANGELOG.md b/CHANGELOG.md index 87f0ef2..222fe05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,37 @@ ## Change Log for open-ships/n2k +### Unreleased — bounded lifecycle, wire fidelity, and hostile-input hardening + +- Commanded Address (PGN 65240) is now a first-class address-claim transition: + only an exact nine-byte broadcast ISO transport transfer for this node's + complete NAME and a claimable address is accepted, followed immediately by + a new Address Claim and contention window. Adversarial tests cover malformed, + mismatched, fast-packet, addressed, and special-address commands. +- Added `just conformance-local`, a current standards/evidence guide, and a + machine-readable certification evidence template. These make local protocol + regression evidence reproducible without misrepresenting it as the licensed + NMEA certification run. +- Live reads now use independent bounded subscriptions; slow consumers receive + `ErrReceiveOverflow` without stalling protocol handling. Writes use a bounded + queue and report `ErrWriteQueueFull`. +- Runtime bus failures propagate to readers, writers, `Client.Err`, and + `Client.Status`. Scanner, bus, scheduler, and write shutdown paths are + cancellation-safe and race-tested. +- ISO transport and fast-packet assembly validate lengths, packet ranges, + session ownership, timeouts, and resource bounds. Missing/out-of-order + packets cannot be accepted as a complete transfer. +- Unmodified decoded messages preserve their original payload exactly; field + changes encode current values. String sentinel and fixed-padding edge cases + now round-trip consistently. +- USB-CAN startup configures 250 kbit/s extended-frame operation before the + bus becomes writable, and stream parsers resynchronize after corrupt input. +- NMEA 2000 source-address claiming now uses the valid 0–251 range. New + `WithPreferredAddress` lets applications restore a persisted prior address + while retaining automatic contention handling. +- Added health/configuration hooks, adversarial and fuzz tests, cross-platform + and race CI, pinned lint/security tools, architecture context, ADRs, + contribution guidance, and private security-reporting instructions. + ### Unreleased — panics in a misbehaving Bus no longer crash the process A `Bus` implementation that panics (rather than returning an error) from `WriteFrame`, `Run`, or a diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..f950c9d --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,65 @@ +# n2k Context + +This is the short entry point for maintainers and coding agents. The README +explains the public product; [docs/architecture.md](docs/architecture.md) +explains the runtime design. + +## Domain language + +- **Frame**: one classical CAN 2.0 frame, with a 29-bit identifier and at most + eight data bytes. +- **PGN**: an NMEA 2000 message type identified by its Parameter Group Number. +- **Message**: a typed `pgn.Message`; a message may occupy one frame, a fast + packet, or an ISO transport transfer. +- **Source / destination**: dynamic 8-bit bus addresses. Addresses 0–251 are + claimable; 252–253 are reserved, 254 means unable to claim, and 255 is + broadcast/global. +- **NAME**: stable 64-bit ISO 11783 device identity used for address claiming. +- **Commanded Address**: PGN 65240, a nine-byte broadcast ISO transport + transfer that assigns a claimable address to the node with an exact NAME. +- **Pipeline**: frame metadata, fast-packet assembly, decode, filtering, and + delivery. +- **System router**: protocol-only decode path for claiming, requests, group + functions, correlation, and the device registry. User filters never affect it. +- **Bus**: the extension seam for physical or virtual read/write transports. +- **Adapter**: converts a transport representation into owned CAN frames. + +## Non-negotiable invariants + +1. Protocol handling runs before user delivery and cannot be stalled by an + unread subscriber. +2. Every queue and reassembly table is bounded. Overflow is explicit; it is + never silent backpressure or unbounded memory growth. +3. Each active transport receive session is unambiguous by source and + destination because TP.DT packets do not carry the transported PGN. +4. Unmodified decoded PGNs re-encode to their original bytes. A field mutation + invalidates that replay path and encodes the current fields. +5. Runtime bus failure reaches readers, writers, `Client.Err`, and + `Client.Status`; a client never remains half-alive after `Bus.Run` exits. +6. Caller-owned frames, payloads, and registry snapshots are copied at + ownership boundaries. +7. Generated PGN files and metadata change only through `just pgn-sync`. +8. Commanded Address changes state only after an exact nine-byte BAM transfer, + an exact 64-bit NAME match, and a requested address in 0–251; the node then + immediately reclaims that address before application writes resume. + +## Where to make changes + +- Public lifecycle and writes: `client.go`, `status.go`, `writeresult.go` +- Read pipeline and fan-out: `pipeline.go`, `scanner.go`, `messagehub.go` +- Hardware/network seams: `bus.go`, `source*.go`, `internal/canbus/`, + `internal/gateway/` +- Fast-packet assembly: `internal/adapter/` +- ISO transport protocol: `internal/transport/` +- Wire codec and generated messages: `pgn/` +- Address claiming and Commanded Address: `internal/claiming/`, `client.go` +- Device discovery: `registry.go` + +## Required verification + +Run `go test ./...`, `just pgn-sync-check`, `just lint`, and `just secure` +before handoff. Run `just test-race` for concurrency changes and `just +fuzz-smoke` for parser, codec, or framing changes. `just conformance-local` +produces the reproducible protocol evidence described in +[docs/conformance.md](docs/conformance.md); it is not a substitute for the +licensed NMEA certification run. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..6587e4d --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,36 @@ +# Contributing + +Issues and focused pull requests are welcome. For protocol changes, include a +capture, standard reference, or minimal frame sequence that demonstrates the +behavior. Keep generated PGN output separate from hand-written runtime changes +when practical. + +## Development + +Go 1.23 or newer and `just` are required. Run `just setup` once to install the +pinned development tools. + +Before submitting a change, run: + +```sh +go test ./... +just pgn-sync-check +just lint +just secure +``` + +Use `just test-race` for lifecycle/concurrency work and `just fuzz-smoke` for +parsers, codecs, framing, or transport protocols. + +For network-management or transport changes, also run `just +conformance-local` and update the test/evidence mapping in +[docs/conformance.md](docs/conformance.md). This local gate is engineering +evidence, not a claim of NMEA certification. Licensed review and official-tool +records belong in the ignored `conformance-artifacts/` directory; commit only +non-confidential requirement identifiers and hashes. + +PGN files and metadata are generated. Modify the generator or its inputs and +run `just pgn-sync`; do not hand-edit generated category files. + +Architecture vocabulary and invariants live in [CONTEXT.md](CONTEXT.md). +Design rationale lives in [docs/adr](docs/adr). diff --git a/README.md b/README.md index d50d18a..84cb9bd 100644 --- a/README.md +++ b/README.md @@ -30,13 +30,15 @@ any Go program that needs to understand or participate on an N2K bus. database for open NMEA 2000 decoding. Every numeric field with a physical interpretation gets a generated SI-unit accessor (`heading.HeadingValue()` → radians) over raw wire ticks. -- **Byte-perfect re-encode.** Decode → re-encode round trips preserve the - original payload bytes, verified against real captures — because decoded - numeric fields keep their raw wire ticks underneath. +- **Wire-faithful re-encode.** An unchanged decoded message retains its exact + original payload, including reserved and trailing bytes. Mutating a field + switches encoding to the current struct values, so edits are never silently + discarded. - **A real bus node, not just a decoder.** `NewClient` claims an address per ISO 11783, heartbeats, answers product/configuration info and ISO requests, - and handles NMEA group functions (transmit / retime / pause) — the protocol - behavior NMEA 2000 requires of a transmitting device, handled for you. + accepts Commanded Address assignments, and handles NMEA group functions + (transmit / retime / pause) — the protocol behavior NMEA 2000 requires of a + transmitting device, handled for you. - **Pure Go, CGO-free**, cross-compiles to Linux, macOS, and Windows. ## Quick Start — No Boat Required @@ -139,8 +141,8 @@ Everything the library does, mapped to its API: |------|---------------|---------------| | Read decoded traffic | Decode live or recorded NMEA 2000 frames into typed PGN structs. | `Receive`, `NewScanner`, `n2k sniff` | | Write PGNs | Encode PGN structs back to byte-preserving CAN frames or gateway messages. | `NewClient`, `Client.Write` | -| Act as a bus node | Claim an address, heartbeat, answer product/configuration info and ISO requests, and handle group functions. | `NewClient`, `WithName`, `WithProductInfo`, `WithConfigInfo` | -| Schedule transmissions | Broadcast PGNs periodically and let other devices retime or pause them through group functions. | `Client.Broadcast` | +| Act as a bus node | Claim or accept a commanded address, heartbeat, answer product/configuration info and ISO requests, and handle group functions. | `NewClient`, `WithName`, `WithProductInfo`, `WithConfigInfo` | +| Schedule transmissions | Broadcast PGNs periodically and let other devices retime or pause them through group functions. | `Client.BroadcastPGN`, `Client.Broadcast` | | Request data | Send typed ISO requests and await typed replies. | `Request[T]` | | Discover devices | Track observed devices by stable 64-bit NAME and current source address. | `Client.Devices`, `Client.DeviceAt` | | Read many sources | Use SocketCAN, USB-CAN serial adapters, TCP/UDP gateways, candump logs, or in-memory frames. | `CAN`, `USB`, `TCP`, `UDP`, `File`, `Replay` | @@ -149,6 +151,8 @@ Everything the library does, mapped to its API: | Filter traffic | Filter by PGN metadata or decoded fields with CEL; metadata-only filters avoid decode work. | `Filter` | | Preserve unknowns | Drop undecoded PGNs by default or surface them for logging/research. | `IncludeUnknown`, `*pgn.UnknownPGN` | | Test without hardware | Replay bundled or custom captures and inspect written frames in tests. | `File`, `OriginalTiming`, `Replay`, `WrittenFrames` | +| Observe health | Export lifecycle errors, queue depth, address state, and structured logs. | `Client.Status`, `Client.Err`, `WithLogger` | +| Extend transports | Provide custom frame buses, readiness, or assembled-message writers. | `Bus`, `ReadyBus`, `MessageWriter`, `WithBus` | ### Reading and Writing @@ -159,8 +163,7 @@ ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) defer stop() client, err := n2k.NewClient(ctx, - n2k.CAN("can0"), - n2k.WithSourceAddress(42), + n2k.CAN("can0"), // auto-claims an available address ) if err != nil { panic(err) @@ -172,16 +175,25 @@ defer client.Close() heading := &pgn.VesselHeading{} heading.SetHeadingValue(1.5708) // radians; stored as raw wire ticks result := client.Write(heading) -if err := result.Wait(); err != nil { +if err := result.WaitContext(ctx); err != nil { log.Printf("write failed: %v", err) } -// Explicitly set priority and destination +// Explicitly set priority. VesselHeading is a broadcast (PDU2) PGN, so it +// cannot carry a destination address. heading2 := &pgn.VesselHeading{ - Info: pgn.MessageInfo{Priority: pgn.Priority(2), TargetId: pgn.Target(42)}, + Info: pgn.MessageInfo{Priority: pgn.Priority(2)}, } heading2.SetHeadingValue(1.5708) -client.Write(heading2) +if err := client.Write(heading2).WaitContext(ctx); err != nil { ... } + +// Addressed PGNs use TargetId. ISO Request is a PDU1 PGN. +requested := uint64(126996) +request := &pgn.IsoRequest{ + Info: pgn.MessageInfo{TargetId: pgn.Target(42)}, + Pgn: &requested, +} +if err := client.Write(request).WaitContext(ctx); err != nil { ... } // Read messages (same as top-level API) for msg, err := range client.Receive() { @@ -194,12 +206,12 @@ for msg, err := range client.Receive() { ### Address Claiming -Every device that transmits on NMEA 2000 must claim a unique bus address (1–253) using the ISO 11783 address claim protocol (PGN 60928). `NewClient` handles this automatically — it broadcasts an address claim, waits for contention, and only returns once a valid address is secured. +Every device that transmits on NMEA 2000 must claim a unique bus address (0–251) using the ISO 11783 address claim protocol (PGN 60928). `NewClient` handles this automatically — it broadcasts an address claim, waits for contention, and only returns once a valid address is secured. **How contention works:** Each device has a 64-bit NAME. When two devices claim the same address, the lower NAME wins and keeps the address; the loser must yield. The client supports two modes: ```go -// Auto mode (default) — starts at address 253 and negotiates downward on +// Auto mode (default) — starts at address 251 and negotiates downward on // contention. If all addresses are exhausted, NewClient returns an error. client, err := n2k.NewClient(ctx, n2k.CAN("can0")) @@ -218,7 +230,7 @@ client, err := n2k.NewClient(ctx, n2k.CAN("can0"), n2k.WithName(n2k.DeviceName{ IndustryGroup: 4, // 3 bits: 4 = Marine - ManufacturerCode: 2000, // 11 bits: unassigned/experimental range + ManufacturerCode: 123, // 11 bits: replace with your assigned code DeviceClass: 25, // 7 bits: 25 = Internetwork Device DeviceFunction: 130, // 8 bits: 130 = PC Gateway DeviceInstance: 0, // 8 bits @@ -228,7 +240,28 @@ client, err := n2k.NewClient(ctx, ) ``` -When `WithName` is not set, `DefaultDeviceName()` is used — it randomizes the identity number so multiple clients from the same binary can coexist on one bus. +When `WithName` is not set, `DefaultDeviceName()` is used. That development default randomizes the identity number so multiple local processes can coexist on one bus. Production devices must provide their assigned manufacturer code and a stable, persisted identity with `WithName`; a random identity is not a provisioned product identity. + +Persist the last claimed address and offer it on the next start; the client +will try it first and move if it is occupied: + +```go +client, err := n2k.NewClient(ctx, + n2k.CAN("can0"), + n2k.WithPreferredAddress(loadLastAddress()), +) +// Save client.Status().Address after claiming or during orderly shutdown. +``` + +The client also handles Commanded Address (PGN 65240) without application +code. It accepts only an exact nine-byte broadcast ISO transport (BAM) +transfer whose full 64-bit NAME matches the client and whose requested address +is 0–251. A valid change immediately emits a new Address Claim and places +application writes behind a fresh contention window. Mismatched NAMEs, +fast-packet or addressed lookalikes, malformed lengths, special addresses, and +commands for the current address have no effect. Observe moves through +`client.Status().Address`; user filters and unread subscriptions cannot disable +this protocol behavior. **Claim timeout:** `NewClient` blocks for up to 1500ms (the default) to allow the network to respond to the initial claim. On heavily contested buses, increase it: @@ -259,6 +292,7 @@ for msg, err := range n2k.Receive(ctx, n2k.CAN("can0")) { ```go s := n2k.NewScanner(ctx, n2k.CAN("can0")) +defer s.Close() for s.Next() { fmt.Printf("Msg: %v\n", s.Message()) } @@ -364,7 +398,7 @@ for msg, err := range n2k.Receive(ctx, | Variable | Type | Description | |----------|------|-------------| | `pgn` | `int` | Parameter Group Number | -| `source` | `int` | Source address (1-253) | +| `source` | `int` | Source address (0-251) | | `priority` | `int` | Message priority (0-7) | | `destination` | `int` | Destination address (255 = broadcast) | | `msg.` | varies | Decoded struct field (case-insensitive), in raw wire ticks | @@ -385,13 +419,49 @@ Repeating-group slice fields (`Repeating1`/`Repeating2`) are not addressable in | `n2k.IncludeUnknown()` | Include undecodable messages as `*pgn.UnknownPGN` | | `n2k.WithLogger(l)` | Override default `slog.Logger` | | `n2k.WithSourceAddress(addr)` | Explicit source address for writes (contention is fatal) | +| `n2k.WithPreferredAddress(addr)` | Starting address for automatic claiming; use a persisted prior address | +| `n2k.WithClaimTimeout(d)` | Initial address-claim deadline; default 1500ms | | `n2k.WithName(name)` | ISO 11783 device NAME for address claiming | | `n2k.WithProductInfo(p)` | Product identity reported via PGN 126996 | | `n2k.WithConfigInfo(ci)` | Installation description reported via PGN 126998 | | `n2k.WithHeartbeatInterval(d)` | Heartbeat (PGN 126993) cadence; default 60s, 0 disables | +| `n2k.WithReceiveBuffer(n)` | Per-subscription live receive buffer; default 64 | +| `n2k.WithWriteQueue(n)` | Pending asynchronous write capacity; default 64 | | `n2k.WithReconnect(policy)` | Auto-reconnect dropped TCP gateway connections with exponential backoff (`ReconnectPolicy{InitialBackoff, MaxBackoff}`; zero values default to 500ms/30s) | | `n2k.WithBus(bus)` | Inject a pre-constructed `n2k.Bus` (custom transport or test fake) instead of CAN/USB sources | +### Errors, backpressure, and health + +Runtime failure is surfaced through the API. A bus disconnect ends live +iterators/scanners with that error, causes later writes to fail, and appears in +`Client.Err()` and `Client.Status()`. + +Queues are deliberately bounded: + +- `Write` never blocks on admission. When its queue is full, the returned + result is already complete with `ErrWriteQueueFull`. +- Each live `Receive` or `Scanner` call has an independent subscription. A + slow subscription ends with `ErrReceiveOverflow`; it cannot stall address + claiming, transport handling, or another reader. + +Use `WaitContext` when a caller has a deadline, tune bounds with +`WithWriteQueue` / `WithReceiveBuffer`, and expose `Status()` from health or +metrics endpoints: + +```go +status := client.Status() +fmt.Println(status.Address, status.AddressClaimed, + status.WriteQueueDepth, status.ReceiveSubscribers, status.TerminalError) +``` + +### Custom transports + +`WithBus` is the extension seam for hardware and gateways not included here. +Implement `Bus` for frame-level read/write access. Optionally implement +`ReadyBus` when opening is asynchronous, and `MessageWriter` when the gateway +accepts assembled PGN payloads instead of CAN frames. The client still owns +claiming, protocol routing, scheduling, and shutdown. + ### A Complete Bus Node Beyond claiming an address, a bus client implements the protocol behavior @@ -406,6 +476,9 @@ NMEA 2000 requires of a transmitting device: - **ISO requests (PGN 59904)** — requests for supported PGNs are answered; requests addressed to us for anything else are refused with an ISO acknowledgement NAK, per ISO 11783-3. +- **Commanded Address (PGN 65240)** — a matching nine-byte BAM assignment + moves the node to a claimable address, immediately reclaims it, and opens a + fresh contention window. Malformed and non-target commands are ignored. - **Group functions (PGN 126208)** — request group functions can transmit, retime, pause (interval 0), or restore (interval `0xFFFFFFFE`) any PGN the client transmits, including scheduled broadcasts. Unsupported group @@ -426,10 +499,11 @@ if err == nil { ``` **Periodic broadcasts** — transmit a PGN on a schedule; the provider runs on -every tick (return nil to skip a tick): +every tick (return nil to skip a tick). Prefer `BroadcastPGN`, which registers +the PGN immediately for deterministic replacement and group-function retiming: ```go -stop := client.Broadcast(time.Second, func() pgn.Message { +stop := client.BroadcastPGN(127250, time.Second, func() pgn.Message { heading := &pgn.VesselHeading{} heading.SetHeadingValue(currentHeadingRadians()) return heading @@ -438,7 +512,9 @@ defer stop() ``` Other devices can retime or pause the broadcast with a request group function -naming its PGN. +naming its PGN. Providers run outside the scheduler goroutine; panic is +contained and `Close` is not held hostage by a blocked provider. Providers +should still return promptly because Go cannot forcibly terminate a callback. ### Device Registry @@ -513,8 +589,9 @@ info := pgn.MessageInfo{ ## Physical Values -Numeric struct fields hold **raw wire ticks** (`*uint64`/`*int64`), which is -what makes byte-perfect re-encoding possible. Every numeric field with a +Numeric struct fields hold **raw wire ticks** (`*uint64`/`*int64`). Exact +round trips are preserved by an owned original-payload snapshot; raw ticks +make field edits precise and predictable. Every numeric field with a physical interpretation also gets **generated typed accessors** that do the unit math — SI units in, SI units out, raw ticks underneath: @@ -559,9 +636,28 @@ decoding would require changing generated PGN struct fields from raw-tick `*uint64`/`*int64` values to quantity types, which is deliberately deferred (see the `units` package doc comment). +## Conformance Status + +The public, reproducible protocol evidence is run with `just +conformance-local` and in Linux CI. Its controlled baseline is +[ISO 11783-3:2026, edition 5](https://www.iso.org/standard/89949.html) for +transport, [ISO 11783-5:2019, edition 3](https://www.iso.org/standard/74366.html) +for address management, and [NMEA 2000 Version 3.000 with +amendments](https://www.nmea.org/nmea-2000.html) for the marine application and +certification process. The test-to-behavior matrix is in [the conformance +guide](docs/conformance.md). + +This evidence is not formal NMEA certification. The official licensed tool has +not been run against this repository alone: certification requires the actual +product hardware and firmware, assigned manufacturer and product codes, the +licensed tool, its encrypted result, and NMEA validation. The tracked +[evidence template](conformance/evidence.example.json) reports those external +steps as `not-run` until real records are supplied; the project does not infer +or simulate a pass. + ## Comparison -As of July 2026, these are relevant projects that document NMEA 2000 support. +Reviewed 2026-07-18, these are relevant projects that document NMEA 2000 support. Go projects come first because `n2k` is for Go applications; non-Go projects follow to show the broader ecosystem. NMEA 0183-only parsers are excluded. @@ -580,13 +676,17 @@ present or not applicable. | [tomer-w/nmea2000](https://github.com/tomer-w/nmea2000) | Python | ❌ not Go | 🟡 generated Python fields | ✅ generated encode/decode | ❓ not documented | ❓ not documented | Python automation and Home Assistant | | [fard-draf/korri-n2k](https://github.com/fard-draf/korri-n2k) | Rust | ❌ not Go | ✅ generated structs | 🟡 generated serialization | ✅ address claiming + fast packet | ❌ roadmap gap | Embedded Rust with selectable PGN sets | -For a Go application, `open-ships/n2k` is the only library here with first-class -support across the whole surface — typed decoding, byte-preserving writes, real -bus-node behavior, group functions, gateway/file sources, and CLI workflows. +For a Go application, `open-ships/n2k` is designed to cover the full path in +one API: typed decoding, byte-preserving writes, bus-node behavior, group +functions, gateway/file sources, and CLI workflows. Evaluate the details that +matter to your deployment rather than relying on the summary table alone. ## Known Limitations - Cross-field validation is not yet implemented. +- Formal NMEA certification has not been run. Products that transmit on a real + vessel network must follow the hardware/tool workflow in [Protocol + conformance](docs/conformance.md) before making a certification claim. - One physical bus per client. - Address claiming uses a 1500ms default timeout; on heavily contested buses, increase via `WithClaimTimeout`. - `File` and `UDP` sources are read-only; writing requires `CAN`, `USB`, `TCP`, or `Replay`. @@ -598,11 +698,21 @@ bus-node behavior, group functions, gateway/file sources, and CLI workflows. connections with backoff. A transparent transport reconnect does not re-run NMEA 2000 address claiming, so a gateway that itself rebooted may still need a fresh client. +- Live delivery is bounded by design. A reader that cannot keep up receives + `ErrReceiveOverflow`; increase `WithReceiveBuffer` or consume faster. ## License MIT — see LICENSE. +## Development and security + +See [CONTEXT.md](CONTEXT.md) for architecture vocabulary and invariants, +[the architecture guide](docs/architecture.md) for runtime ownership, +[the conformance guide](docs/conformance.md) for standards evidence, +[CONTRIBUTING.md](CONTRIBUTING.md) for required checks, and +[SECURITY.md](SECURITY.md) for private vulnerability reporting. + ## Acknowledgments - [canboat](https://github.com/canboat/canboat) — maintains the open PGN diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..cb07139 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,13 @@ +# Security Policy + +Please report vulnerabilities privately through GitHub's **Report a +vulnerability** flow in the repository Security tab. Do not open a public issue +until a fix is available. + +Include affected versions, a minimal reproducer or capture when possible, +impact, and any suggested mitigation. Reports involving parser panics, +unbounded memory growth, deadlocks, unsafe bus writes, or device-identity +spoofing are in scope. + +Security fixes target the latest released minor version. Older `v0` minor +lines may require upgrading because the public API is still pre-1.0. diff --git a/broadcast.go b/broadcast.go index 1a84c11..a367349 100644 --- a/broadcast.go +++ b/broadcast.go @@ -2,7 +2,11 @@ package n2k import ( "context" + "fmt" + "log/slog" + "runtime/debug" "sync" + "sync/atomic" "time" "github.com/open-ships/n2k/pgn" @@ -13,27 +17,57 @@ import ( // available yet). The first transmission happens as soon as the client's // address claim completes. // -// provide is called once immediately to learn the PGN it produces; if it -// returns nil there, the PGN is learned from its first non-nil message -// instead. Scheduling the same PGN again replaces the earlier schedule. The -// returned stop function halts transmission and is safe to call repeatedly; -// Close also stops all broadcasters. +// The PGN is learned asynchronously from the first non-nil message. Until +// then, group-function lookup cannot find this schedule; BroadcastPGN avoids +// that delay. Scheduling the same PGN again replaces the earlier schedule. +// The returned stop function is safe to call repeatedly; Close also stops all +// broadcasters. // // Another device can retime or pause a broadcast at runtime by sending a // request group function (PGN 126208) naming the broadcast PGN. func (c *Client) Broadcast(interval time.Duration, provide func() pgn.Message) (stop func()) { + return c.newBroadcast(0, false, interval, provide) +} + +// BroadcastPGN is Broadcast with an explicit PGN identity. Prefer it when a +// group-function peer must be able to retime the schedule immediately, before +// provide returns its first non-nil message. Declaring the PGN also makes +// replacement of an existing schedule deterministic without probing provide. +func (c *Client) BroadcastPGN(pgnNum uint32, interval time.Duration, provide func() pgn.Message) (stop func()) { + return c.newBroadcast(pgnNum, true, interval, provide) +} + +func (c *Client) newBroadcast(pgnNum uint32, known bool, interval time.Duration, provide func() pgn.Message) func() { + c.mu.Lock() + closed := c.closed + c.mu.Unlock() + if closed { + return func() {} + } + if known && pgnNum > 0x3FFFF { + c.log.Error("cannot schedule broadcast with invalid PGN", "pgn", pgnNum) + return func() {} + } + if provide == nil { + c.log.Error("cannot schedule broadcast with nil provider", "pgn", pgnNum) + return func() {} + } b := &broadcaster{ write: c.Write, provide: provide, + log: c.log, onPGNKnown: c.registerBroadcaster, + onStop: c.unregisterBroadcaster, interval: interval, defaultInterval: interval, changed: make(chan struct{}, 1), quit: make(chan struct{}), done: make(chan struct{}), + pgn: pgnNum, + known: known, + order: c.broadcastSeq.Add(1), } - if first := provide(); first != nil { - b.pgn = first.PGNNumber() + if known { c.registerBroadcaster(b) } go b.run(c.ctx, c.addrReady) @@ -49,10 +83,15 @@ func (c *Client) Broadcast(interval time.Duration, provide func() pgn.Message) ( func (c *Client) registerBroadcaster(b *broadcaster) { pgnNum := b.pgnNumber() c.bMu.Lock() - prev := c.broadcasters[pgnNum] if c.broadcasters == nil { c.broadcasters = make(map[uint32]*broadcaster) } + prev := c.broadcasters[pgnNum] + if prev != nil && prev.order > b.order { + c.bMu.Unlock() + b.stop() + return + } c.broadcasters[pgnNum] = b c.bMu.Unlock() if prev != nil && prev != b { @@ -89,6 +128,9 @@ func (c *Client) stopBroadcasters() { for _, b := range all { b.stop() } + for _, b := range all { + b.wait() + } } // broadcaster periodically transmits one PGN. Its run loop mirrors the @@ -97,12 +139,16 @@ func (c *Client) stopBroadcasters() { type broadcaster struct { write func(pgn.Message) *WriteResult provide func() pgn.Message + log *slog.Logger // onPGNKnown registers the broadcaster for group-function lookup once // its PGN is known (deferred when provide returned nil at registration). onPGNKnown func(*broadcaster) + onStop func(*broadcaster) mu sync.Mutex - pgn uint32 // 0 until the first non-nil message + pgn uint32 // valid once known is true + known bool + order uint64 // creation order makes deferred registration deterministic interval time.Duration // current cadence; <= 0 means paused defaultInterval time.Duration // cadence given to Broadcast, for restore @@ -110,10 +156,16 @@ type broadcaster struct { quit chan struct{} done chan struct{} stopOnce sync.Once + sending atomic.Bool } func (b *broadcaster) run(ctx context.Context, ready <-chan struct{}) { defer close(b.done) + defer func() { + if b.onStop != nil { + b.onStop(b) + } + }() select { case <-ready: @@ -136,7 +188,9 @@ func (b *broadcaster) run(ctx context.Context, ready <-chan struct{}) { } } - b.sendNow() + if !b.sendNow(ctx) { + return + } timer := time.NewTimer(iv) select { @@ -155,21 +209,68 @@ func (b *broadcaster) run(ctx context.Context, ready <-chan struct{}) { // sendNow transmits one message immediately (nil from provide skips), // completing deferred PGN registration on the first non-nil message. -func (b *broadcaster) sendNow() { - msg := b.provide() +func (b *broadcaster) sendNow(ctx context.Context) bool { + if !b.sending.CompareAndSwap(false, true) { + return true + } + defer b.sending.Store(false) + + type provided struct { + msg pgn.Message + err error + } + result := make(chan provided, 1) + go func() { + defer func() { + if r := recover(); r != nil { + result <- provided{err: fmt.Errorf("provider panic: %v", r)} + b.log.Error("recovered panic in broadcast provider", "panic", r, "stack", string(debug.Stack())) + } + }() + result <- provided{msg: b.provide()} + }() + var msg pgn.Message + select { + case got := <-result: + if got.err != nil { + return true + } + msg = got.msg + case <-ctx.Done(): + return false + case <-b.quit: + return false + } if msg == nil { - return + return true } b.mu.Lock() - learned := b.pgn == 0 + learned := !b.known if learned { b.pgn = msg.PGNNumber() + b.known = true } b.mu.Unlock() + if !learned && msg.PGNNumber() != b.pgnNumber() { + b.log.Warn("broadcast provider returned a different PGN", "want", b.pgnNumber(), "got", msg.PGNNumber()) + return true + } if learned && b.onPGNKnown != nil { b.onPGNKnown(b) + select { + case <-b.quit: + return false + default: + } + } + if err := b.write(msg).WaitContext(ctx); err != nil && ctx.Err() == nil { + b.log.Warn("scheduled broadcast failed", "pgn", msg.PGNNumber(), "error", err) } - b.write(msg) + return true +} + +func (b *broadcaster) trigger(ctx context.Context) { + go func() { _ = b.sendNow(ctx) }() } // setInterval retimes the broadcast. Zero (or negative) pauses it; a positive @@ -204,9 +305,12 @@ func (b *broadcaster) pgnNumber() uint32 { return b.pgn } -// stop terminates the run loop and waits for it to exit. Safe to call more -// than once. +// stop requests termination without waiting, so it is safe to invoke from a +// provider callback. Client.Close waits separately for broadcaster exit. func (b *broadcaster) stop() { b.stopOnce.Do(func() { close(b.quit) }) +} + +func (b *broadcaster) wait() { <-b.done } diff --git a/broadcast_test.go b/broadcast_test.go index 344afc8..df70177 100644 --- a/broadcast_test.go +++ b/broadcast_test.go @@ -89,11 +89,11 @@ func TestBroadcast_CloseStopsAll(t *testing.T) { func TestBroadcast_SamePGNReplacesEarlier(t *testing.T) { c, _, _ := newCitizenClient(t) - stop1 := c.Broadcast(time.Hour, headingProvider) + stop1 := c.BroadcastPGN(127250, time.Hour, headingProvider) b1 := c.broadcasterFor(127250) require.NotNil(t, b1) - stop2 := c.Broadcast(time.Hour, headingProvider) + stop2 := c.BroadcastPGN(127250, time.Hour, headingProvider) b2 := c.broadcasterFor(127250) require.NotNil(t, b2) assert.NotSame(t, b1, b2, "re-registering a PGN should install a new broadcaster") @@ -103,3 +103,59 @@ func TestBroadcast_SamePGNReplacesEarlier(t *testing.T) { stop2() assert.Nil(t, c.broadcasterFor(127250)) } + +func TestBroadcast_BlockingProviderDoesNotBlockStopOrClose(t *testing.T) { + c, _, _ := newCitizenClient(t) + blocked := make(chan struct{}) + stop := c.BroadcastPGN(127250, time.Hour, func() pgn.Message { + <-blocked + return headingProvider() + }) + + require.Eventually(t, func() bool { + return c.broadcasterFor(127250) != nil + }, time.Second, time.Millisecond) + stopDone := make(chan struct{}) + go func() { stop(); close(stopDone) }() + select { + case <-stopDone: + case <-time.After(time.Second): + t.Fatal("stop blocked on provider") + } + closeDone := make(chan error, 1) + go func() { closeDone <- c.Close() }() + select { + case err := <-closeDone: + require.NoError(t, err) + case <-time.After(time.Second): + t.Fatal("Close blocked on provider") + } + close(blocked) +} + +func TestBroadcast_ProviderPanicIsContained(t *testing.T) { + c, _, _ := newCitizenClient(t) + var calls atomic.Int32 + stop := c.BroadcastPGN(127250, 10*time.Millisecond, func() pgn.Message { + if calls.Add(1) == 1 { + panic("bad sensor callback") + } + return headingProvider() + }) + defer stop() + + require.Eventually(t, func() bool { return calls.Load() >= 2 }, time.Second, time.Millisecond) +} + +func TestBroadcastPGN_SkipsMismatchedProviderMessage(t *testing.T) { + c, mb, _ := newCitizenClient(t) + var calls atomic.Int32 + stop := c.BroadcastPGN(127250, 10*time.Millisecond, func() pgn.Message { + calls.Add(1) + return &pgn.WaterDepth{} + }) + defer stop() + + require.Eventually(t, func() bool { return calls.Load() >= 2 }, time.Second, time.Millisecond) + assert.Empty(t, framesWithPGN(mb.getWritten(), 128267), "a provider must not silently change its declared PGN") +} diff --git a/bus.go b/bus.go index c6f7fc2..c5db115 100644 --- a/bus.go +++ b/bus.go @@ -11,14 +11,25 @@ import ( // does not ship. Pass an implementation via WithBus. type Bus interface { // Run opens the bus and delivers every incoming frame to handler until - // ctx is cancelled or an unrecoverable error occurs. + // ctx is cancelled or an unrecoverable error occurs. Client serializes + // concurrent handler calls, though Bus implementations should normally + // preserve wire order. Run(ctx context.Context, handler func(can.Frame)) error // WriteFrame sends one frame. WriteFrame(frame can.Frame) error - // Close releases resources. Safe to call even if Run was never called. + // Close releases resources. It must be safe if Run was never called and + // safe concurrently with Run or WriteFrame so cancellation can release + // blocked device I/O. Close() error } +// ReadyBus is optionally implemented by buses that are not writable until +// Run has opened their underlying device. Client waits for Ready before +// starting address claiming, removing scheduler-dependent startup races. +type ReadyBus interface { + Ready() <-chan struct{} +} + // MessageWriter is optionally implemented by Bus implementations that // transmit whole assembled PGN messages rather than raw CAN frames — // message-oriented gateways such as Actisense-format streams, where the diff --git a/client.go b/client.go index 5ac8133..ddf922e 100644 --- a/client.go +++ b/client.go @@ -7,6 +7,7 @@ import ( "fmt" "iter" "log/slog" + "reflect" "runtime/debug" "sync" "sync/atomic" @@ -24,6 +25,18 @@ import ( // claiming to complete. const defaultClaimTimeout = 1500 * time.Millisecond +const defaultWriteQueue = 64 + +const pgnISOCommandedAddress uint32 = 65240 + +var ( + // ErrWriteQueueFull reports that an asynchronous write could not be admitted + // without blocking the caller. Callers may retry or apply their own policy. + ErrWriteQueueFull = errors.New("n2k: write queue full") + // ErrClientClosed reports an operation attempted after Client.Close. + ErrClientClosed = errors.New("n2k: client closed") +) + // Client is a read/write NMEA 2000 bus node. It composes address claiming, // transport protocol, encoding, and framing into a single type that can both // receive and transmit PGN messages. @@ -55,26 +68,26 @@ type Client struct { writeCh chan writeJob writeWg sync.WaitGroup - // writers counts Write calls that have passed the closed check and may - // still send on writeCh. Close waits for it to drain before closing the - // channel, so a send can never race the close. Registered under mu. - writers sync.WaitGroup - // mu guards writtenFrames, closed state, and sourceAddr. mu sync.Mutex writtenFrames []can.Frame // captured frames (replay/testing) closed bool + terminalErr error + claimed bool + claimEpoch uint64 + txReady chan struct{} bus Bus + busDone chan struct{} claimer *claiming.Claimer addrErr error // set by OnFatalError during address claiming // addrReady is closed once address claiming completes (or immediately for replay). addrReady chan struct{} - // msgCh delivers decoded messages from the internal read loop to the read API. - // nil for replay clients. - msgCh chan pgn.Message + // msgHub delivers decoded live messages without allowing application + // backpressure to stall protocol processing. It is nil for replay clients. + msgHub *messageHub // pipeline is the persistent read pipeline (pre-filter -> assembly -> // decode -> unknown-PGN policy -> post-filter -> msgCh) for the internal @@ -101,6 +114,7 @@ type Client struct { // bMu guards broadcasters, the active periodic transmissions by PGN. bMu sync.Mutex broadcasters map[uint32]*broadcaster + broadcastSeq atomic.Uint64 // registry tracks devices observed on the bus, keyed by NAME. Only set // for bus clients. @@ -112,12 +126,65 @@ type writeJob struct { result *WriteResult } +func validateClientConfig(cfg config) error { + if cfg.bus != nil && len(cfg.sources) > 0 { + return errors.New("n2k: WithBus cannot be combined with source options") + } + busSources := 0 + for _, src := range cfg.sources { + if _, ok := src.(busBacked); ok { + busSources++ + } + } + if busSources > 1 || (busSources == 1 && len(cfg.sources) != 1) { + return errors.New("n2k: Client requires exactly one writable bus source") + } + if cfg.bus == nil && busSources == 0 { + for _, src := range cfg.sources { + if _, ok := src.(*replaySource); !ok { + return errors.New("n2k: File and UDP are read-only; use Receive or NewScanner") + } + } + } + if cfg.reconnect != nil && busSources == 0 { + return errors.New("n2k: WithReconnect requires a TCP source") + } + if cfg.productInfo != nil { + fields := map[string]string{ + "model ID": cfg.productInfo.ModelID, + "software version": cfg.productInfo.SoftwareVersion, + "model version": cfg.productInfo.ModelVersion, + "serial number": cfg.productInfo.SerialNumber, + } + for name, value := range fields { + if len([]byte(value)) > 32 { + return fmt.Errorf("n2k: product %s exceeds 32 bytes", name) + } + } + } + if cfg.configInfo != nil { + for name, value := range map[string]string{ + "installation description 1": cfg.configInfo.InstallationDescription1, + "installation description 2": cfg.configInfo.InstallationDescription2, + "manufacturer information": cfg.configInfo.ManufacturerInformation, + } { + if len([]byte(value)) > 253 { + return fmt.Errorf("n2k: configuration %s exceeds 253 bytes", name) + } + } + } + return nil +} + // NewClient creates a Client that can read and write NMEA 2000 messages. // Provide CAN, USB, TCP, Replay, or WithBus for writable clients; File and UDP // are read-only sources for Receive/NewScanner. func NewClient(ctx context.Context, opts ...Option) (*Client, error) { cfg := config{} for _, o := range opts { + if o == nil { + return nil, errors.New("n2k: nil Option") + } o.apply(&cfg) } cfg.applyReconnect() @@ -127,6 +194,9 @@ func NewClient(ctx context.Context, opts ...Option) (*Client, error) { if cfg.logger == nil { cfg.logger = slog.Default() } + if err := validateClientConfig(cfg); err != nil { + return nil, err + } // Validate the device NAME eagerly so a malformed NAME fails fast. if cfg.deviceName != nil { @@ -150,10 +220,11 @@ func NewClient(ctx context.Context, opts ...Option) (*Client, error) { // Determine device NAME. var deviceName uint64 + arbitraryAddressCapable := cfg.sourceAddress == nil if cfg.deviceName != nil { - deviceName = cfg.deviceName.Pack(true) + deviceName = cfg.deviceName.Pack(arbitraryAddressCapable) } else { - deviceName = DefaultDeviceName().Pack(true) + deviceName = DefaultDeviceName().Pack(arbitraryAddressCapable) } // Determine if we have a hardware bus or replay-only. @@ -171,8 +242,10 @@ func NewClient(ctx context.Context, opts ...Option) (*Client, error) { var sourceAddr uint8 if cfg.sourceAddress != nil { sourceAddr = *cfg.sourceAddress + } else if cfg.preferredAddress != nil { + sourceAddr = *cfg.preferredAddress } else if hasBus { - sourceAddr = 253 + sourceAddr = 251 } // else: replay default is 0 @@ -184,7 +257,9 @@ func NewClient(ctx context.Context, opts ...Option) (*Client, error) { sourceAddr: sourceAddr, deviceName: deviceName, addrReady: make(chan struct{}), + busDone: make(chan struct{}), } + c.txReady = c.addrReady c.productInfo = defaultProductInfo(UnpackDeviceName(deviceName).IdentityNumber) if cfg.productInfo != nil { @@ -198,7 +273,11 @@ func NewClient(ctx context.Context, opts ...Option) (*Client, error) { // Start the single writer goroutine for FIFO ordering. It must exist // before initBus so the client's own protocol goroutines (heartbeat, // info responses) can write as soon as the address claim completes. - c.writeCh = make(chan writeJob, 64) + writeQueue := defaultWriteQueue + if cfg.writeQueue != nil { + writeQueue = *cfg.writeQueue + } + c.writeCh = make(chan writeJob, writeQueue) c.writeWg.Add(1) go c.writeLoop() @@ -213,17 +292,20 @@ func NewClient(ctx context.Context, opts ...Option) (*Client, error) { cancel() c.mu.Lock() c.closed = true + c.terminalErr = err c.mu.Unlock() if c.bus != nil { _ = c.bus.Close() } - c.writers.Wait() - close(c.writeCh) + if c.tp != nil { + c.tp.Close() + } c.writeWg.Wait() return nil, err } } else { // Replay path: capture frames in memory. + c.claimed = true close(c.addrReady) c.writeFrame = func(f can.Frame) error { @@ -280,22 +362,34 @@ func (c *Client) initBus(cfg config) error { // through the read pipeline. c.tp = transport.NewManager(transport.ManagerConfig{ WriteFrame: c.writeFrame, + LocalAddress: func() uint8 { + c.mu.Lock() + defer c.mu.Unlock() + return c.sourceAddr + }, OnComplete: func(tpPGN uint32, source uint8, destination uint8, data []byte) { priority := uint8(6) info := pgn.MessageInfo{Timestamp: time.Now(), PGN: tpPGN, SourceId: source, Priority: &priority} if destination != framer.BroadcastAddr { info.TargetId = &destination } - c.pipeline.InjectAssembled(info, data) + if tpPGN == pgnISOCommandedAddress { + c.handleCommandedAddressTransfer(destination, data) + } c.system.handleAssembled(info, data) + c.pipeline.InjectAssembled(info, data) }, Logger: c.log, }) - // Set up the persistent read pipeline (pre-filter -> assembly -> decode - // -> unknown-PGN policy -> post-filter -> msgCh). - c.msgCh = make(chan pgn.Message, 64) - p, err := newReadPipeline(c.ctx, cfg, c.msgCh) + // Set up the persistent read pipeline (pre-filter -> assembly -> decode -> + // unknown-PGN policy -> post-filter -> non-blocking subscription hub). + receiveBuffer := defaultReceiveBuffer + if cfg.receiveBuffer != nil { + receiveBuffer = *cfg.receiveBuffer + } + c.msgHub = newMessageHub(receiveBuffer) + p, err := newReadPipeline(c.ctx, cfg, c.msgHub.publish) if err != nil { return err } @@ -339,15 +433,17 @@ func (c *Client) initBus(cfg config) error { Name: c.deviceName, WriteFrame: c.writeFrame, OnAddressChange: func(newAddr uint8) { - c.mu.Lock() - c.sourceAddr = newAddr - c.mu.Unlock() + c.handleAddressChange(newAddr) }, OnFatalError: func(err error) { c.log.Error("address claiming fatal error", "error", err) c.mu.Lock() c.addrErr = err + active := c.claimed c.mu.Unlock() + if active { + c.fail(fmt.Errorf("n2k: address claiming failed: %w", err)) + } }, Logger: c.log, }) @@ -359,6 +455,21 @@ func (c *Client) initBus(cfg config) error { if cfg.claimTimeout != nil { claimTimeout = *cfg.claimTimeout } + if readyBus, ok := c.bus.(ReadyBus); ok { + readyTimer := time.NewTimer(claimTimeout) + select { + case <-readyBus.Ready(): + readyTimer.Stop() + case <-c.busDone: + readyTimer.Stop() + return c.currentTerminalError("n2k: bus stopped before becoming ready") + case <-readyTimer.C: + return errors.New("n2k: bus did not become ready within claim timeout") + case <-c.ctx.Done(): + readyTimer.Stop() + return c.currentTerminalError(c.ctx.Err().Error()) + } + } timer := time.NewTimer(claimTimeout) defer timer.Stop() @@ -391,14 +502,14 @@ func (c *Client) initBus(cfg config) error { case <-timer.C: return errors.New("n2k: gateway unreachable — initial connection not established within claim timeout") case <-c.ctx.Done(): - return c.ctx.Err() + return c.currentTerminalError(c.ctx.Err().Error()) } // Wait for the remainder of the claim window to allow the network to respond. select { case <-timer.C: case <-c.ctx.Done(): - return c.ctx.Err() + return c.currentTerminalError(c.ctx.Err().Error()) } // Check if a fatal error occurred during the claim window. @@ -419,6 +530,7 @@ func (c *Client) initBus(cfg config) error { // Update sourceAddr with the final claimed address. c.mu.Lock() c.sourceAddr = addr + c.claimed = true c.mu.Unlock() close(c.addrReady) @@ -476,13 +588,13 @@ func (c *Client) handleBusFrame(frame can.Frame) { c.tp.HandleFrame(frame) } - // Decode for the read API using the persistent pipeline (pre-filter moved - // inside HandleFrame). - c.pipeline.HandleFrame(frame) - // Decode protocol PGNs for the client's own use, independent of the user // filter. c.system.handleFrame(frame, info.PGN) + + // Decode for the read API after all synchronous protocol routing. User + // delivery itself is non-blocking through msgHub. + c.pipeline.HandleFrame(frame) } // busReadLoop runs the bus and closes the message channel when done. @@ -499,20 +611,112 @@ func (c *Client) recoverGoroutine(name string) { } func (c *Client) busReadLoop() { - defer close(c.msgCh) - defer c.recoverGoroutine("bus read loop") + defer close(c.busDone) + defer func() { + if r := recover(); r != nil { + err := fmt.Errorf("n2k: panic in bus read loop: %v", r) + c.log.Error("recovered panic in bus read loop", "panic", r, "stack", string(debug.Stack())) + c.fail(err) + } + }() // Handling an inbound frame can write synchronously on this goroutine — the // claimer defends its address by re-broadcasting a claim in response to a // contender or ISO request — so a panicking Bus surfaces here, not only on // the write loop. Recover per frame so one bad frame is logged and skipped // rather than tearing down the read loop (and with it the whole process). + var handleMu sync.Mutex handle := func(f can.Frame) { + handleMu.Lock() + defer handleMu.Unlock() defer c.recoverGoroutine("bus frame handler") c.handleBusFrame(f) } - if err := c.bus.Run(c.ctx, handle); err != nil && c.ctx.Err() == nil { + err := c.bus.Run(c.ctx, handle) + if c.ctx.Err() == nil { + if err == nil { + err = errors.New("bus stopped unexpectedly") + } + wrapped := fmt.Errorf("n2k: bus read loop: %w", err) c.log.Error("bus read loop error", "error", err) + c.fail(wrapped) + return + } + if c.msgHub != nil { + c.msgHub.close(nil) + } +} + +func (c *Client) currentTerminalError(fallback string) error { + c.mu.Lock() + defer c.mu.Unlock() + if c.terminalErr != nil { + return c.terminalErr + } + return errors.New(fallback) +} + +func (c *Client) fail(err error) { + if err == nil { + return + } + c.mu.Lock() + if c.terminalErr == nil { + c.terminalErr = err + } + c.mu.Unlock() + if c.msgHub != nil { + c.msgHub.close(err) + } + c.cancel() +} + +// handleAddressChange moves application writes behind a fresh contention +// window whenever an active auto-claiming client changes address. +func (c *Client) handleAddressChange(newAddr uint8) { + c.mu.Lock() + c.sourceAddr = newAddr + if !c.claimed { + c.mu.Unlock() + return + } + if newAddr == 254 { + c.mu.Unlock() + c.fail(errors.New("n2k: address claiming failed: no address available")) + return + } + c.claimEpoch++ + epoch := c.claimEpoch + ready := make(chan struct{}) + c.txReady = ready + c.mu.Unlock() + + time.AfterFunc(250*time.Millisecond, func() { + c.mu.Lock() + defer c.mu.Unlock() + if !c.closed && c.claimEpoch == epoch && c.txReady == ready { + close(ready) + } + }) +} + +// handleCommandedAddressTransfer validates the transfer-level requirements +// that are lost after generic PGN decoding, then delegates the address state +// transition to the claiming module. PGN 65240 is acted on only when it arrives +// as a broadcast ISO transport transfer with its exact nine-byte payload. +func (c *Client) handleCommandedAddressTransfer(destination uint8, data []byte) { + if destination != framer.BroadcastAddr || len(data) != 9 { + return + } + + commandedName := binary.LittleEndian.Uint64(data[:8]) + changed, err := c.claimer.HandleCommandedAddress(commandedName, data[8]) + if err != nil { + c.fail(fmt.Errorf("n2k: commanded address failed: %w", err)) + return + } + if changed { + c.log.Info("address changed by PGN 65240", "address", data[8]) } } @@ -526,24 +730,41 @@ func (c *Client) Write(msg pgn.Message) *WriteResult { c.mu.Lock() if c.closed { c.mu.Unlock() - wr.complete(errors.New("n2k: client closed")) + wr.complete(ErrClientClosed) return wr } - // Register as an in-flight sender while still holding mu, so Close (which - // sets closed under the same lock) cannot start closing writeCh until this - // send has completed. - c.writers.Add(1) + if c.terminalErr != nil { + err := c.terminalErr + c.mu.Unlock() + wr.complete(err) + return wr + } + select { + case c.writeCh <- writeJob{msg: msg, result: wr}: + default: + wr.complete(ErrWriteQueueFull) + } c.mu.Unlock() - - c.writeCh <- writeJob{msg: msg, result: wr} - c.writers.Done() return wr } func (c *Client) writeLoop() { defer c.writeWg.Done() - for job := range c.writeCh { - c.runWriteJob(job) + for { + select { + case job := <-c.writeCh: + c.runWriteJob(job) + case <-c.ctx.Done(): + err := c.currentTerminalError(c.ctx.Err().Error()) + for { + select { + case job := <-c.writeCh: + job.result.complete(err) + default: + return + } + } + } } } @@ -564,11 +785,36 @@ func (c *Client) runWriteJob(job writeJob) { // doWrite performs the synchronous work of encoding and framing a PGN message. func (c *Client) doWrite(msg pgn.Message) error { + if msg == nil { + return errors.New("n2k: cannot write a nil message") + } + v := reflect.ValueOf(msg) + if v.Kind() == reflect.Ptr && v.IsNil() { + return fmt.Errorf("n2k: cannot write a nil %T", msg) + } + // Wait for address readiness before sending. + c.mu.Lock() + ready := c.txReady + terminalErr := c.terminalErr + c.mu.Unlock() + if terminalErr != nil { + return terminalErr + } select { - case <-c.addrReady: + case <-ready: case <-c.ctx.Done(): - return c.ctx.Err() + return c.currentTerminalError(c.ctx.Err().Error()) + } + c.mu.Lock() + closed := c.closed + terminalErr = c.terminalErr + c.mu.Unlock() + if terminalErr != nil { + return terminalErr + } + if closed { + return ErrClientClosed } pgnNum := msg.PGNNumber() @@ -578,6 +824,16 @@ func (c *Client) doWrite(msg pgn.Message) error { if !ok { return fmt.Errorf("n2k: %T does not implement pgn.PGN", msg) } + if pgnNum > 0x3FFFF { + return fmt.Errorf("n2k: PGN %d exceeds the 18-bit range", pgnNum) + } + if pgnNum&0x20000 != 0 { + return fmt.Errorf("n2k: PGN %d sets the reserved CAN-ID bit", pgnNum) + } + pduFormat := uint8((pgnNum >> 8) & 0xFF) + if pduFormat < 240 && pgnNum&0xFF != 0 { + return fmt.Errorf("n2k: PDU1 PGN %d must have a zero group-extension byte", pgnNum) + } payload, err := pgn.EncodeMessage(msg) if err != nil { @@ -590,11 +846,17 @@ func (c *Client) doWrite(msg pgn.Message) error { if info.Priority != nil { priority = *info.Priority } + if priority > 7 { + return fmt.Errorf("n2k: priority %d is outside 0-7", priority) + } var destination uint8 = 255 if info.TargetId != nil { destination = *info.TargetId } + if info.TargetId != nil && destination != framer.BroadcastAddr && pduFormat >= 240 { + return fmt.Errorf("n2k: PDU2 PGN %d cannot target address %d", pgnNum, destination) + } // Read sourceAddr under lock. c.mu.Lock() @@ -631,6 +893,9 @@ func (c *Client) doWrite(msg pgn.Message) error { return nil } + if destination != framer.BroadcastAddr { + return c.tp.SendRTSCTS(pgnNum, srcAddr, destination, payload) + } return c.tp.SendBAM(pgnNum, srcAddr, payload) } @@ -638,17 +903,23 @@ func (c *Client) doWrite(msg pgn.Message) error { // it reads from the internal message channel; for replay clients it builds a // fresh Scanner over the client's config so each call gets a full replay. func (c *Client) Receive() iter.Seq2[pgn.Message, error] { - if c.msgCh != nil { + if c.msgHub != nil { return func(yield func(pgn.Message, error) bool) { - for msg := range c.msgCh { + sub := c.msgHub.subscribe() + defer sub.unsubscribe() + for msg := range sub.ch { if !yield(msg, nil) { return } } + if err := sub.terminalError(); err != nil { + yield(nil, err) + } } } return func(yield func(pgn.Message, error) bool) { s := c.newReplayScanner() + defer func() { _ = s.Close() }() for s.Next() { if !yield(s.Message(), nil) { return @@ -664,17 +935,8 @@ func (c *Client) Receive() iter.Seq2[pgn.Message, error] { // it reads from the internal message channel; for replay clients it builds a // fresh Scanner over the client's config. func (c *Client) Scanner() *Scanner { - if c.msgCh != nil { - s := &Scanner{ - ctx: c.ctx, - cfg: c.cfg, - ch: c.msgCh, - } - // ch is already live (fed by the client's own read pipeline), so - // suppress Next()'s lazy-start goroutine: firing once.Do here means - // the real closure passed to Next()'s once.Do never runs. - s.once.Do(func() {}) - return s + if c.msgHub != nil { + return &Scanner{ctx: c.ctx, cfg: c.cfg, sub: c.msgHub.subscribe()} } return c.newReplayScanner() } @@ -682,7 +944,12 @@ func (c *Client) Scanner() *Scanner { // newReplayScanner builds a Scanner over the client's already-parsed config, // so replay clients share the exact pipeline code without re-parsing options. func (c *Client) newReplayScanner() *Scanner { - return &Scanner{ctx: c.ctx, cfg: c.cfg, ch: make(chan pgn.Message, 64)} + ctx, cancel := context.WithCancel(c.ctx) + receiveBuffer := defaultReceiveBuffer + if c.cfg.receiveBuffer != nil { + receiveBuffer = *c.cfg.receiveBuffer + } + return &Scanner{ctx: ctx, cancel: cancel, cfg: c.cfg, ch: make(chan pgn.Message, receiveBuffer)} } // WrittenFrames returns a copy of all CAN frames written through this client. @@ -706,31 +973,25 @@ func (c *Client) Close() error { c.closed = true c.mu.Unlock() - // Stop protocol writers before closing the write channel so their final - // writes cannot race the shutdown. - if c.heartbeat != nil { - c.heartbeat.stop() - } - c.stopBroadcasters() - - // Wait for any Write that already passed the closed check to finish - // sending, then close the channel — this ordering makes a send-on-closed - // channel panic impossible. - c.writers.Wait() - close(c.writeCh) - - // Cancel the context and close the bus BEFORE waiting on the write loop. - // With auto-reconnect a write can be parked inside the bus waiting for the - // connection to come back; closing the bus releases it so writeLoop drains - // and exits instead of deadlocking this wait. + // Cancel first and close the bus so an active hardware write is released. + // Write never blocks on queue admission and the write channel is not closed, + // so concurrent callers cannot race a send-on-closed-channel panic. c.cancel() var busErr error if c.bus != nil { busErr = c.bus.Close() } - c.writeWg.Wait() + + if c.heartbeat != nil { + c.heartbeat.stop() + } + c.stopBroadcasters() if c.tp != nil { c.tp.Close() } + c.writeWg.Wait() + if c.msgHub != nil { + c.msgHub.close(nil) + } return busErr } diff --git a/client_test.go b/client_test.go index bd34289..2c6ea78 100644 --- a/client_test.go +++ b/client_test.go @@ -49,6 +49,14 @@ func TestClient_NewClient_Validation(t *testing.T) { assert.Equal(t, name.Pack(true), c.deviceName) assert.NoError(t, c.Close()) }) + + t.Run("explicit address clears arbitrary-address flag", func(t *testing.T) { + name := DeviceName{IndustryGroup: 4, IdentityNumber: 42} + c, err := NewClient(context.Background(), Replay(nil), WithName(name), WithSourceAddress(42)) + require.NoError(t, err) + assert.Equal(t, name.Pack(false), c.deviceName) + assert.NoError(t, c.Close()) + }) } func TestNewClient_Replay_BadFilterFailsEagerly(t *testing.T) { @@ -154,6 +162,31 @@ func TestClient_Write_AfterClose(t *testing.T) { assert.Contains(t, err.Error(), "closed") } +func TestClient_WriteRejectsNilMessages(t *testing.T) { + c, err := NewClient(context.Background(), Replay(nil)) + require.NoError(t, err) + defer func() { _ = c.Close() }() + + assert.Error(t, c.Write(nil).Wait()) + var typedNil *pgn.VesselHeading + assert.Error(t, c.Write(typedNil).Wait()) +} + +func TestClient_WriteQueueFullIsImmediate(t *testing.T) { + c := &Client{writeCh: make(chan writeJob, 1)} + first := c.Write(&pgn.VesselHeading{}) + second := c.Write(&pgn.VesselHeading{}) + + select { + case <-second.Done(): + assert.ErrorIs(t, second.Wait(), ErrWriteQueueFull) + case <-time.After(time.Second): + t.Fatal("saturated Write blocked instead of returning ErrWriteQueueFull") + } + job := <-c.writeCh + assert.Same(t, first, job.result) +} + func TestClient_Replay_Receive(t *testing.T) { // Build a pre-encoded VesselHeading frame for replay. heading := uint64(15000) @@ -546,6 +579,96 @@ func (m *mockBus) getWritten() []can.Frame { // Compile-time proof that Bus is implementable with only public types. var _ Bus = (*mockBus)(nil) +// terminatingBus lets tests end Bus.Run independently of context cancellation. +// It implements ReadyBus so startup does not depend on scheduler timing. +type terminatingBus struct { + ready chan struct{} + terminate chan error + closed chan struct{} + once sync.Once + mu sync.Mutex + written []can.Frame +} + +func newTerminatingBus() *terminatingBus { + return &terminatingBus{ + ready: make(chan struct{}), + terminate: make(chan error, 1), + closed: make(chan struct{}), + } +} + +func (b *terminatingBus) Ready() <-chan struct{} { return b.ready } + +func (b *terminatingBus) Run(ctx context.Context, _ func(can.Frame)) error { + b.once.Do(func() { close(b.ready) }) + select { + case err := <-b.terminate: + return err + case <-b.closed: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + +func (b *terminatingBus) WriteFrame(f can.Frame) error { + b.mu.Lock() + b.written = append(b.written, f) + b.mu.Unlock() + return nil +} + +func (b *terminatingBus) Close() error { + select { + case <-b.closed: + default: + close(b.closed) + } + return nil +} + +var _ Bus = (*terminatingBus)(nil) +var _ ReadyBus = (*terminatingBus)(nil) + +func TestClient_BusFailurePropagatesToReadersAndWriters(t *testing.T) { + b := newTerminatingBus() + c, err := NewClient(context.Background(), + WithBus(b), + WithClaimTimeout(20*time.Millisecond), + WithHeartbeatInterval(0), + ) + require.NoError(t, err) + defer func() { _ = c.Close() }() + scanner := c.Scanner() + defer func() { _ = scanner.Close() }() + + want := errors.New("adapter disconnected") + b.terminate <- want + require.Eventually(t, func() bool { return c.ctx.Err() != nil }, time.Second, time.Millisecond) + + assert.False(t, scanner.Next()) + assert.ErrorIs(t, scanner.Err(), want) + assert.ErrorIs(t, c.Write(&pgn.VesselHeading{}).Wait(), want) +} + +func TestClient_UnexpectedCleanBusStopIsTerminal(t *testing.T) { + b := newTerminatingBus() + c, err := NewClient(context.Background(), + WithBus(b), + WithClaimTimeout(20*time.Millisecond), + WithHeartbeatInterval(0), + ) + require.NoError(t, err) + defer func() { _ = c.Close() }() + + b.terminate <- nil + require.Eventually(t, func() bool { return c.ctx.Err() != nil }, time.Second, time.Millisecond) + err = c.Write(&pgn.VesselHeading{}).Wait() + require.Error(t, err) + assert.Contains(t, err.Error(), "stopped unexpectedly") +} + // blockingBus simulates a reconnecting transport whose writes park during an // outage — mirroring tcpLink.await(), which blocks a write while the gateway // connection is down and releases it only on reconnect or Close. Writes @@ -1017,6 +1140,52 @@ func TestClient_Write_NonPGNMessageErrors(t *testing.T) { } } +type invalidPGNMessage struct { + info pgn.MessageInfo + pgn uint32 +} + +func (m *invalidPGNMessage) PGNNumber() uint32 { return m.pgn } +func (m *invalidPGNMessage) MessageInfo() pgn.MessageInfo { return m.info } +func (m *invalidPGNMessage) SetMessageInfo(i pgn.MessageInfo) { m.info = i } +func (m *invalidPGNMessage) DecodePayload([]uint8) error { return nil } +func (m *invalidPGNMessage) EncodePayload() ([]uint8, error) { return []uint8{0}, nil } + +func TestClient_Write_RejectsNonCanonicalPGNs(t *testing.T) { + c, err := NewClient(context.Background(), Replay(nil)) + require.NoError(t, err) + defer func() { _ = c.Close() }() + + tests := []struct { + name string + pgn uint32 + }{ + {name: "outside 18-bit range", pgn: 0x40000}, + {name: "reserved CAN-ID bit", pgn: 0x20000}, + {name: "PDU1 group extension", pgn: 0xEA01}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := c.Write(&invalidPGNMessage{pgn: tc.pgn}).Wait() + require.Error(t, err) + }) + } +} + +func TestClient_Write_RejectsAddressedPDU2(t *testing.T) { + c, err := NewClient(context.Background(), Replay(nil)) + require.NoError(t, err) + defer func() { _ = c.Close() }() + + msg := &invalidPGNMessage{ + pgn: 127250, + info: pgn.MessageInfo{TargetId: pgn.Target(42)}, + } + err = c.Write(msg).Wait() + require.Error(t, err) + assert.Contains(t, err.Error(), "PDU2") +} + // --- MessageWriter bus path --- // mockMessageBus is a mockBus that also implements MessageWriter, like the diff --git a/commandedaddress_test.go b/commandedaddress_test.go new file mode 100644 index 0000000..dbc66ed --- /dev/null +++ b/commandedaddress_test.go @@ -0,0 +1,256 @@ +package n2k + +import ( + "context" + "encoding/binary" + "errors" + "strconv" + "sync" + "testing" + "time" + + "github.com/brutella/can" + "github.com/open-ships/n2k/internal/framer" + "github.com/open-ships/n2k/internal/transport" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func commandedAddressPayload(name uint64, address uint8) []byte { + payload := make([]byte, 9) + binary.LittleEndian.PutUint64(payload[:8], name) + payload[8] = address + return payload +} + +func transportReceiveFrames(pgnNum uint32, source, destination uint8, payload []byte, bam bool) []can.Frame { + packetCount := (len(payload) + transport.MaxDTDataBytes - 1) / transport.MaxDTDataBytes + var announcement [8]byte + if bam { + announcement[0] = transport.ControlBAM + announcement[4] = 0xFF + destination = framer.BroadcastAddr + } else { + announcement[0] = transport.ControlRTS + announcement[4] = uint8(packetCount) + } + binary.LittleEndian.PutUint16(announcement[1:3], uint16(len(payload))) + announcement[3] = uint8(packetCount) + announcement[5] = byte(pgnNum) + announcement[6] = byte(pgnNum >> 8) + announcement[7] = byte(pgnNum >> 16) + + frames := []can.Frame{{ + ID: framer.BuildCANID(transport.PGNCM, transport.TPPriority, source, destination), + Length: 8, + Data: announcement, + }} + for packet := 0; packet < packetCount; packet++ { + data := [8]byte{0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF} + data[0] = uint8(packet + 1) + start := packet * transport.MaxDTDataBytes + end := min(start+transport.MaxDTDataBytes, len(payload)) + copy(data[1:], payload[start:end]) + frames = append(frames, can.Frame{ + ID: framer.BuildCANID(transport.PGNDT, transport.TPPriority, source, destination), + Length: 8, + Data: data, + }) + } + return frames +} + +func deliverFrames(c *Client, frames []can.Frame) { + for _, frame := range frames { + c.handleBusFrame(frame) + } +} + +type commandedAddressFailBus struct { + *mockBus + mu sync.Mutex + failAddress uint8 + fail bool + err error +} + +func (b *commandedAddressFailBus) failClaimAt(address uint8, err error) { + b.mu.Lock() + b.failAddress = address + b.fail = true + b.err = err + b.mu.Unlock() +} + +func (b *commandedAddressFailBus) WriteFrame(frame can.Frame) error { + b.mu.Lock() + fail := b.fail + failAddress := b.failAddress + err := b.err + b.mu.Unlock() + info := framer.ParseCANID(frame.ID) + if fail && info.PGN == framer.PGNISOAddressClaim && info.Source == failAddress { + return err + } + return b.mockBus.WriteFrame(frame) +} + +func TestConformanceCommandedAddressMatchingBAMReclaimsAddress(t *testing.T) { + c, bus, oldAddress := newCitizenClient(t) + newAddress := uint8(42) + require.NotEqual(t, oldAddress, newAddress) + initialClaims := len(framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim)) + + deliverFrames(c, transportReceiveFrames( + pgnISOCommandedAddress, + 0x33, + framer.BroadcastAddr, + commandedAddressPayload(c.deviceName, newAddress), + true, + )) + + status := c.Status() + assert.Equal(t, newAddress, status.Address) + assert.True(t, status.AddressClaimed) + claims := framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim) + require.Len(t, claims, initialClaims+1) + lastClaim := claims[len(claims)-1] + assert.Equal(t, newAddress, framer.ParseCANID(lastClaim.ID).Source) + assert.Equal(t, c.deviceName, binary.LittleEndian.Uint64(lastClaim.Data[:8])) + + // Application traffic remains behind a fresh contention window, while the + // protocol claim above is emitted immediately. + c.mu.Lock() + ready := c.txReady + c.mu.Unlock() + select { + case <-ready: + t.Fatal("application writes became ready before the commanded-address contention window") + default: + } + require.Eventually(t, func() bool { + select { + case <-ready: + return true + default: + return false + } + }, time.Second, 5*time.Millisecond) +} + +func TestConformanceCommandedAddressRequiresExactTransferAndTarget(t *testing.T) { + tests := []struct { + name string + destination uint8 + payload func(*Client, uint8) []byte + }{ + { + name: "addressed transfer", + destination: 251, + payload: func(c *Client, address uint8) []byte { + return commandedAddressPayload(c.deviceName, address) + }, + }, + { + name: "short payload", + destination: framer.BroadcastAddr, + payload: func(c *Client, address uint8) []byte { + return commandedAddressPayload(c.deviceName, address)[:8] + }, + }, + { + name: "long payload", + destination: framer.BroadcastAddr, + payload: func(c *Client, address uint8) []byte { + return append(commandedAddressPayload(c.deviceName, address), 0xFF) + }, + }, + { + name: "NAME differs only in high bit", + destination: framer.BroadcastAddr, + payload: func(c *Client, address uint8) []byte { + return commandedAddressPayload(c.deviceName^(uint64(1)<<63), address) + }, + }, + } + for _, address := range []uint8{252, 253, 254, 255} { + commandedAddress := address + tests = append(tests, struct { + name string + destination uint8 + payload func(*Client, uint8) []byte + }{ + name: "special address " + strconv.Itoa(int(commandedAddress)), + destination: framer.BroadcastAddr, + payload: func(c *Client, _ uint8) []byte { + return commandedAddressPayload(c.deviceName, commandedAddress) + }, + }) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c, bus, oldAddress := newCitizenClient(t) + initialClaims := len(framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim)) + c.handleCommandedAddressTransfer(tt.destination, tt.payload(c, 42)) + + assert.Equal(t, oldAddress, c.Status().Address) + assert.Len(t, framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim), initialClaims) + }) + } +} + +func TestConformanceCommandedAddressRejectsFastPacketAndAddressedTP(t *testing.T) { + t.Run("fast packet", func(t *testing.T) { + c, bus, oldAddress := newCitizenClient(t) + initialClaims := len(framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim)) + frames := framer.FrameFastPacket( + framer.BuildCANID(pgnISOCommandedAddress, 6, 0x33, framer.BroadcastAddr), + commandedAddressPayload(c.deviceName, 42), + 0, + ) + deliverFrames(c, frames) + + assert.Equal(t, oldAddress, c.Status().Address) + assert.Len(t, framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim), initialClaims) + }) + + t.Run("addressed transport", func(t *testing.T) { + c, bus, oldAddress := newCitizenClient(t) + initialClaims := len(framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim)) + deliverFrames(c, transportReceiveFrames( + pgnISOCommandedAddress, + 0x33, + oldAddress, + commandedAddressPayload(c.deviceName, 42), + false, + )) + + assert.Equal(t, oldAddress, c.Status().Address) + assert.Len(t, framesWithPGN(bus.getWritten(), framer.PGNISOAddressClaim), initialClaims) + }) +} + +func TestConformanceCommandedAddressClaimFailureTerminatesClient(t *testing.T) { + bus := &commandedAddressFailBus{mockBus: newMockBus()} + c, err := NewClient(context.Background(), + WithBus(bus), + WithClaimTimeout(50*time.Millisecond), + WithHeartbeatInterval(0), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = c.Close() }) + + writeErr := errors.New("injected commanded-address claim failure") + bus.failClaimAt(42, writeErr) + deliverFrames(c, transportReceiveFrames( + pgnISOCommandedAddress, + 0x33, + framer.BroadcastAddr, + commandedAddressPayload(c.deviceName, 42), + true, + )) + + require.ErrorIs(t, c.Err(), writeErr) + assert.Equal(t, uint8(42), c.Status().Address) +} diff --git a/conformance/evidence.example.json b/conformance/evidence.example.json new file mode 100644 index 0000000..e8c05ec --- /dev/null +++ b/conformance/evidence.example.json @@ -0,0 +1,43 @@ +{ + "schemaVersion": 1, + "status": "not-run", + "implementation": { + "commit": "", + "version": "", + "goVersion": "" + }, + "baselines": { + "iso11783Part3": "ISO 11783-3:2026 edition 5", + "iso11783Part5": "ISO 11783-5:2019 edition 3", + "nmea2000": "Version 3.000 with amendments" + }, + "localEvidence": { + "command": "just conformance-local", + "runAt": "", + "result": "not-run" + }, + "licensedReview": { + "reviewer": "", + "reviewedAt": "", + "result": "not-run", + "privateRequirementMatrixSha256": "" + }, + "nmeaCertification": { + "toolVersion": "", + "runAt": "", + "manufacturerCode": "", + "productCode": "", + "result": "not-run", + "encryptedResultSha256": "", + "validatedByNMEA": false, + "validatedAt": "" + }, + "hardware": { + "productModel": "", + "firmwareVersion": "", + "canInterface": "", + "transceiver": "", + "bitRate": 250000 + }, + "notes": [] +} diff --git a/devicename.go b/devicename.go index 5195a98..4085962 100644 --- a/devicename.go +++ b/devicename.go @@ -91,19 +91,18 @@ func UnpackDeviceName(name uint64) DeviceName { } } -// DefaultDeviceName returns a DeviceName pre-configured for a PC-based software -// gateway operating on an NMEA 2000 marine network. The identity number is -// randomized so that multiple processes using the same binary can coexist on -// the same bus without NAME collisions during address claiming. +// DefaultDeviceName returns a development-oriented DeviceName for a PC-based +// software gateway. Production devices should use WithName with the vendor's +// assigned manufacturer code and a stable, persisted identity number. The +// default identity is randomized so multiple development processes can coexist +// on one bus, but it is not a substitute for a provisioned product identity. func DefaultDeviceName() DeviceName { return DeviceName{ // Marine industry group per NMEA 2000 specification. IndustryGroup: 4, - // Manufacturer code 2000 is in the unassigned/experimental range (1864-2045). - // Codes 2046-2047 are reserved sentinels in the NMEA 2000 spec. Using an - // unassigned code avoids conflicts with real manufacturers while being clearly - // identifiable as a software gateway rather than a hardware device. + // Development placeholder only. Production applications must replace it + // with their assigned manufacturer code via WithName. ManufacturerCode: 2000, // Device Class 25 = "Internetwork Device" per NMEA 2000 device class table. @@ -131,6 +130,15 @@ func DefaultDeviceName() DeviceName { // ISO 11783 NAME identity number. func randomIdentityNumber() uint32 { var buf [4]byte - _, _ = rand.Read(buf[:]) - return binary.LittleEndian.Uint32(buf[:]) & 0x1FFFFF + if _, err := rand.Read(buf[:]); err != nil { + // crypto/rand failures are exceptionally rare and DefaultDeviceName + // cannot return an error without breaking its API. Zero is a valid but + // collision-prone identity, so reserve 1 as the explicit fallback. + return 1 + } + identity := binary.LittleEndian.Uint32(buf[:]) & 0x1FFFFF + if identity == 0 { + return 1 + } + return identity } diff --git a/docs/adr/0001-bounded-runtime-state.md b/docs/adr/0001-bounded-runtime-state.md new file mode 100644 index 0000000..d6e80c5 --- /dev/null +++ b/docs/adr/0001-bounded-runtime-state.md @@ -0,0 +1,24 @@ +# ADR 0001: Bound runtime queues and reassembly state + +Status: accepted + +## Context + +Marine buses can run indefinitely and can contain malformed, duplicated, or +bursty traffic. User consumers and callback providers may also stop making +progress. Unbounded queues turn either condition into process-wide memory +growth; blocking queues allow application behavior to stall bus citizenship. + +## Decision + +Write admission, live subscriptions, fast-packet sessions, gateway parser +bodies, and ISO transport sessions are bounded. Slow live subscribers fail +with `ErrReceiveOverflow`; saturated writes fail with `ErrWriteQueueFull`. +Fast-packet partial state has both a TTL and a maximum entry count. Protocol +processing never waits for user message consumption. + +## Consequences + +Applications must choose retry/drop policy for write saturation and restart a +subscription after inspecting overflow. Buffer sizes are configurable. Memory +and shutdown behavior remain predictable under hostile traffic. diff --git a/docs/adr/0002-wire-fidelity.md b/docs/adr/0002-wire-fidelity.md new file mode 100644 index 0000000..2e6b8e4 --- /dev/null +++ b/docs/adr/0002-wire-fidelity.md @@ -0,0 +1,23 @@ +# ADR 0002: Preserve raw payloads until fields change + +Status: accepted + +## Context + +Generated schemas do not always describe every reserved bit, trailing byte, or +proprietary variation. Encoding only decoded fields can therefore change an +otherwise untouched message. Always replaying the original bytes, however, +silently discards caller mutations. + +## Decision + +After decode, retain an owned original payload and a canonical encoding of the +decoded fields. On encode, compare the current field encoding with that +canonical value. Return the original bytes only when fields are unchanged; +otherwise return the newly encoded fields. + +## Consequences + +Passive decode/encode forwarding is wire-faithful, including unknown reserved +content. Normal struct mutation behaves as Go callers expect. Code that wants +to alter only unknown bits must work at the raw frame layer. diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..0806d19 --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,86 @@ +# Architecture + +`n2k` is split into deep modules with narrow seams. The public package owns +lifecycle and policy; internal packages own wire protocols. + +```text +CAN / USB / TCP / UDP / capture + | + v + Bus or read-only source + | + v + frame ownership boundary + | + +-------+------------------+ + | | + v v +system protocol path user read pipeline +claim / ISO request metadata filter +ISO transport fast-packet assembly +group functions typed PGN decode +registry / correlator field filter + | | + +-----------+--------------+ + v + bounded subscriptions +``` + +## Runtime ownership + +One `Client` owns one writable `Bus`, one address claimer, one ISO transport +manager, one system router, one serialized write queue, and zero or more live +read subscriptions. `Receive` and `Scanner` subscriptions are independent. +When one subscriber exceeds its configured buffer, only that subscriber ends +with `ErrReceiveOverflow`. + +The system path receives frames first. It handles protocol traffic regardless +of a user's CEL filter or whether the application is currently reading. This +ordering prevents user code from breaking address defense, request correlation, +transport reassembly, or device discovery. + +Commanded Address (PGN 65240) crosses a narrow transport-to-claiming seam. The +transport manager must first complete a broadcast ISO transport transfer. The +client retains the transfer metadata and exact payload long enough to require +nine bytes, compare the complete 64-bit NAME, and reject addressed or +fast-packet lookalikes. The claiming module owns the actual state transition, +contention-history reset, notification, and immediate Address Claim response. +Application writes wait through a fresh contention window after the move. + +## Write selection + +`Client.Write` admits work to a bounded FIFO queue. Encoding then chooses: + +| Payload and PGN | Wire mechanism | +|---|---| +| Non-fast, at most 8 bytes | One CAN frame | +| Fast PGN, at most 223 bytes | NMEA 2000 fast packet | +| Larger broadcast, at most 1785 bytes | ISO TP BAM | +| Larger addressed, at most 1785 bytes | ISO TP RTS/CTS | + +Message-oriented gateways may implement `MessageWriter` and accept assembled +payloads up to 223 bytes. Buses that open asynchronously may implement +`ReadyBus`; the client will not begin address claiming until `Ready` closes. + +## Failure model + +Bus termination, address-claim failure, and receive overflow are data, not log +side effects. They propagate through iterators/scanners and write results. +`Client.Status` supplies a polling seam for health checks and metrics. `slog` +supplies structured diagnostics. `Close` cancels owned work, closes the bus to +release blocked I/O, stops scheduled protocol work, fails queued writes, and +waits for owned goroutines. + +## Codec fidelity + +Decode stores both the original payload and a canonical encoding of decoded +fields. Encode compares current fields with the canonical bytes. If unchanged, +it returns a copy of the original payload, retaining reserved bits and trailing +bytes. If changed, it encodes the current fields. See +[ADR 0002](adr/0002-wire-fidelity.md). + +## Generated boundary + +Category files under `pgn/`, `pgn/dispatch.go`, metadata, and the manifest are +generated from the checked-in schema snapshot. Change generator inputs or the +generator, then run `just pgn-sync`; never repair generated output by hand. diff --git a/docs/conformance.md b/docs/conformance.md new file mode 100644 index 0000000..c342f9e --- /dev/null +++ b/docs/conformance.md @@ -0,0 +1,106 @@ +# Protocol conformance + +This repository keeps two claims deliberately separate: + +1. **Local protocol evidence** is public, deterministic, and runs in CI with + `just conformance-local` plus the normal test, race, lint, and security + gates. +2. **Formal product certification** requires licensed standards, the official + NMEA certification hardware/software, assigned manufacturer and product + codes, representative product hardware, and NMEA validation of the tool's + encrypted result. The public test suite does not claim or imply that status. + +## Controlled baseline + +Baseline metadata was checked against the publishers on 2026-07-18: + +| Scope | Controlled reference | Why it matters | +|---|---|---| +| CAN-frame mapping and transport/network protocol | [ISO 11783-3:2026, edition 5](https://www.iso.org/standard/89949.html), published 2026-03 | Current ISO transport baseline, including the transport layer used to carry PGN 65240. | +| Source-address and NAME management | [ISO 11783-5:2019, edition 3](https://www.iso.org/standard/74366.html), confirmed current in 2024 | Network-management baseline for address changes and subsequent claims. | +| Marine application and certification | [NMEA 2000 Version 3.000 with amendments](https://www.nmea.org/nmea-2000.html) | Product-facing NMEA requirements and the official certification process. | + +The ISO and NMEA documents and official test vectors are licensed material. +They are not copied into this repository. A licensed reviewer must maintain a +private requirement crosswalk and record only non-confidential requirement +identifiers, results, and artifact hashes here. + +## Reproducible local gate + +Run: + +```sh +just conformance-local +go test ./... +just test-race +just pgn-sync-check +just lint +just secure +``` + +`conformance-local` gives the externally visible Commanded Address cases a +stable, verbose test run and reruns the deep protocol modules for CAN framing, +fast-packet discrimination, ISO transport, and address claiming. The full test +suite remains the authoritative local regression gate. + +### Commanded Address evidence map + +| Evidence ID | Expected behavior | Executable evidence | +|---|---|---| +| CA-01 | Accept PGN 65240 only after a complete broadcast ISO transport transfer. | `TestConformanceCommandedAddressMatchingBAMReclaimsAddress`; `TestConformanceCommandedAddressRejectsFastPacketAndAddressedTP` | +| CA-02 | Require an exact nine-byte payload. | `TestConformanceCommandedAddressRequiresExactTransferAndTarget` | +| CA-03 | Compare all 64 NAME bits exactly. | `TestConformanceCommandedAddressRequiresExactTransferAndTarget` (the negative vector differs only at bit 63) | +| CA-04 | Accept only claimable destination addresses 0–251. | `TestConformanceCommandedAddressRequiresExactTransferAndTarget`; `TestClaimer_CommandedAddressRejectsNonTargetsAndSpecialAddresses` | +| CA-05 | Ignore a command for the current address. | `TestClaimer_CommandedAddressRejectsNonTargetsAndSpecialAddresses` | +| CA-06 | Change the runtime source and immediately broadcast Address Claim with the same NAME. | `TestConformanceCommandedAddressMatchingBAMReclaimsAddress` | +| CA-07 | Start a fresh contention window before application traffic resumes. | `TestConformanceCommandedAddressMatchingBAMReclaimsAddress` | +| CA-08 | Preserve automatic versus explicit contention policy after the command. | `TestClaimer_CommandedAddressReclaimsImmediately`; `TestClaimer_CommandedAddressPreservesExplicitContentionPolicy` | +| CA-09 | Surface failure to transmit the required new claim as a terminal protocol failure. | `TestClaimer_CommandedAddressClaimFailureIsReturned`; `TestConformanceCommandedAddressClaimFailureTerminatesClient` | + +These IDs are repository evidence identifiers, not ISO or NMEA clause numbers. +An authorized reviewer must map them to the licensed editions above and add or +change cases wherever the normative requirements differ. + +## Licensed ISO review + +For each release candidate that changes framing, transport, claiming, timing, +or protocol responses: + +1. Pin the exact implementation commit and Go toolchain. +2. Have an authorized reviewer cross-check the implementation and tests against + ISO 11783-3:2026 and the current network-management reference. Record the + reviewer, edition, review date, result, and private matrix SHA-256. +3. Run the complete suite on classical 250 kbit/s extended CAN hardware under + nominal, saturated, reordered, duplicated, malformed, timeout, contention, + disconnect, and restart conditions relevant to the product. +4. Store licensed matrices and raw lab output outside git. Put hashes and the + non-confidential result metadata in an evidence record. + +Until that licensed review is recorded, describe the result as “local protocol +regression passed,” never “ISO conformant.” + +## Official NMEA certification run + +NMEA states that transmitting/receiving products must be certified through its +official process. The certification tool produces an encrypted result for NMEA +validation, and product certification requires assigned manufacturer and +product codes. The tool, license, product identity, and physical unit under +test are not present in this repository, so this step cannot run in public CI. + +For a release candidate: + +1. Obtain the current standard/tool directly from NMEA and record their exact + versions and amendments. +2. Configure the real product with its assigned stable NAME, manufacturer code, + product code, production firmware, transceiver, and power behavior. +3. Run every mandatory official test and the product's declared transmit and + receive PGN set. Do not substitute mocks or a community test suite. +4. Hash the encrypted output, submit it to NMEA, and record NMEA's validation + result and date. Keep licensed tool output in controlled storage. +5. Copy [the evidence example](../conformance/evidence.example.json) to the + ignored `conformance-artifacts/` directory and fill it without embedding + licensed content, credentials, serial secrets, or raw test vectors. + +Formal release evidence is complete only when local gates pass at the recorded +commit, the licensed ISO crosswalk is signed, the official tool run passes, and +NMEA has validated its encrypted result. diff --git a/groupfunction.go b/groupfunction.go index ab5767b..9a8ca5f 100644 --- a/groupfunction.go +++ b/groupfunction.go @@ -131,7 +131,7 @@ func (c *Client) applyRequestedInterval(pgnNum uint32, intervalTicks *uint64) { return // schedule raced away since the transmitsPGN check } if intervalTicks == nil { - b.sendNow() + b.trigger(c.ctx) return } switch *intervalTicks { diff --git a/internal/adapter/adversarial_test.go b/internal/adapter/adversarial_test.go new file mode 100644 index 0000000..90cd94b --- /dev/null +++ b/internal/adapter/adversarial_test.go @@ -0,0 +1,73 @@ +package adapter + +import ( + "testing" + "time" + + "github.com/brutella/can" + "github.com/open-ships/n2k/internal/decoder" + "github.com/open-ships/n2k/internal/framer" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFastPacketOwnsFrameBytes(t *testing.T) { + m := NewMultiBuilder() + info := NewPacketInfo(&can.Frame{ID: framer.BuildCANID(130820, 1, 10, 255), Length: 8}) + frameZero := []byte{0x20, 8, 1, 2, 3, 4, 5, 6} + p := decoder.NewPacket(info, frameZero) + m.Add(p) + + for i := range frameZero { + frameZero[i] = 0xEE + } + last := decoder.NewPacket(info, []byte{0x21, 7, 8}) + m.Add(last) + require.True(t, last.Complete) + assert.Equal(t, []byte{1, 2, 3, 4, 5, 6, 7, 8}, last.Data) +} + +func TestMultiBuilderEvictsExpiredAndCapsState(t *testing.T) { + m := NewMultiBuilder() + now := time.Unix(100, 0) + m.now = func() time.Time { return now } + m.ttl = time.Second + m.maxActive = 2 + addContinuation := func(source uint8) { + info := NewPacketInfo(&can.Frame{ID: framer.BuildCANID(130820, 1, source, 255), Length: 8}) + m.Add(decoder.NewPacket(info, []byte{1, 1, 2, 3, 4, 5, 6, 7})) + } + + addContinuation(1) + addContinuation(2) + addContinuation(3) + assert.LessOrEqual(t, activeFastPacketSequences(m), 2) + + now = now.Add(time.Second) + addContinuation(4) + assert.Equal(t, 1, activeFastPacketSequences(m)) +} + +func activeFastPacketSequences(m *MultiBuilder) int { + total := 0 + for _, byPGN := range m.sequences { + for _, bySeq := range byPGN { + total += len(bySeq) + } + } + return total +} + +func FuzzCANAdapter(f *testing.F) { + f.Add(uint32(0x09F20183), byte(8), []byte{0x60, 0x20}) + f.Add(uint32(0x09F20D00), byte(8), []byte{}) + f.Fuzz(func(t *testing.T, id uint32, length byte, input []byte) { + a := NewCANAdapter() + a.SetOutput(&mockHandler{}) + var data [8]byte + copy(data[:], input) + frame := can.Frame{ID: id, Length: length, Data: data} + require.NotPanics(t, func() { a.HandleMessage(&frame) }) + assert.LessOrEqual(t, activeFastPacketSequences(a.multi), defaultMaxFastPacketState) + }) +} diff --git a/internal/adapter/canadapter.go b/internal/adapter/canadapter.go index fa4a29d..1848087 100644 --- a/internal/adapter/canadapter.go +++ b/internal/adapter/canadapter.go @@ -76,12 +76,19 @@ func (c *CANAdapter) SetOutput(ph PacketHandler) { // Parameters: // - f: A *can.Frame from the brutella/can library. func (c *CANAdapter) HandleMessage(f *can.Frame) { + if f == nil || f.Length > 8 { + return + } // Extract NMEA 2000 message metadata (PGN, source, priority, destination) from // the 29-bit extended CAN ID. pInfo := NewPacketInfo(f) // Create a Packet and look up candidate metadata. - packet := decoder.NewPacket(pInfo, f.Data[:]) + // Own the bytes beyond this call. Many CAN drivers reuse a frame buffer; + // fast-packet state and downstream handlers must not observe later writes + // through an alias to that buffer. + data := append([]byte(nil), f.Data[:f.Length]...) + packet := decoder.NewPacket(pInfo, data) // Reference: https://endige.com/2050/nmea-2000-pgns-deciphered/ diff --git a/internal/adapter/multibuilder.go b/internal/adapter/multibuilder.go index 88c9063..e869a5d 100644 --- a/internal/adapter/multibuilder.go +++ b/internal/adapter/multibuilder.go @@ -1,9 +1,17 @@ package adapter import ( + "fmt" + "time" + "github.com/open-ships/n2k/internal/decoder" ) +const ( + defaultFastPacketTTL = 2 * time.Second + defaultMaxFastPacketState = 2048 +) + // MultiBuilder manages the assembly of multi-frame NMEA 2000 fast packets into complete // single Packets. NMEA 2000 messages with payloads larger than 8 bytes are transmitted // as a series of CAN frames that must be reassembled. @@ -24,6 +32,9 @@ type MultiBuilder struct { // This hierarchy allows efficient lookup and supports multiple simultaneous transmissions // from different sources, different PGNs, and multiple sequence IDs per source/PGN pair. sequences map[uint8]map[uint32]map[uint8]*sequence + now func() time.Time + ttl time.Duration + maxActive int } // NewMultiBuilder creates and returns a new MultiBuilder with an initialized (empty) @@ -31,6 +42,9 @@ type MultiBuilder struct { func NewMultiBuilder() *MultiBuilder { mBuilder := MultiBuilder{ sequences: make(map[uint8]map[uint32]map[uint8]*sequence), + now: time.Now, + ttl: defaultFastPacketTTL, + maxActive: defaultMaxFastPacketState, } return &mBuilder } @@ -46,16 +60,27 @@ func NewMultiBuilder() *MultiBuilder { // sequence is complete, p.Data will contain the fully assembled payload and // p.Complete will be true. func (m *MultiBuilder) Add(p *decoder.Packet) { + if p == nil || len(p.Data) == 0 { + if p != nil { + p.ParseErrors = append(p.ParseErrors, fmt.Errorf("fast-packet frame has no header byte")) + } + return + } + now := m.now() + m.evictExpired(now) // Extract the 3-bit sequence ID and 5-bit frame number from the first data byte. p.GetSeqFrame() // Find or create the sequence for this source/PGN/seqId combination. - seq := m.SeqFor(p) + seq := m.seqFor(p, now) // Add this frame's data to the sequence. - seq.add(p) + if !seq.add(p, now) { + m.deleteSequence(p.Info.SourceId, p.Info.PGN, p.SeqId) + return + } // Check if the sequence now has all expected data and finalize if so. if seq.complete(p) { // Sequence is done -- delete it from the map to free memory and prevent stale data. - delete(m.sequences[p.Info.SourceId][p.Info.PGN], p.SeqId) + m.deleteSequence(p.Info.SourceId, p.Info.PGN, p.SeqId) } } @@ -68,6 +93,13 @@ func (m *MultiBuilder) Add(p *decoder.Packet) { // // Returns a pointer to the sequence for this packet's source/PGN/seqId combination. func (m *MultiBuilder) SeqFor(p *decoder.Packet) *sequence { + return m.seqFor(p, m.now()) +} + +func (m *MultiBuilder) seqFor(p *decoder.Packet, now time.Time) *sequence { + if m.sequences[p.Info.SourceId] == nil || m.sequences[p.Info.SourceId][p.Info.PGN] == nil || m.sequences[p.Info.SourceId][p.Info.PGN][p.SeqId] == nil { + m.evictOldestIfFull() + } // Lazily initialize the source-level map if this is the first packet from this source. if _, t := m.sequences[p.Info.SourceId]; !t { m.sequences[p.Info.SourceId] = make(map[uint32]map[uint8]*sequence) @@ -80,7 +112,55 @@ func (m *MultiBuilder) SeqFor(p *decoder.Packet) *sequence { seq := m.sequences[p.Info.SourceId][p.Info.PGN][p.SeqId] if seq == nil { seq = &sequence{} + seq.updated = now m.sequences[p.Info.SourceId][p.Info.PGN][p.SeqId] = seq } return seq } + +func (m *MultiBuilder) deleteSequence(source uint8, pgn uint32, seqID uint8) { + byPGN := m.sequences[source] + if byPGN == nil { + return + } + bySeq := byPGN[pgn] + delete(bySeq, seqID) + if len(bySeq) == 0 { + delete(byPGN, pgn) + } + if len(byPGN) == 0 { + delete(m.sequences, source) + } +} + +func (m *MultiBuilder) evictExpired(now time.Time) { + for source, byPGN := range m.sequences { + for pgn, bySeq := range byPGN { + for seqID, seq := range bySeq { + if !seq.updated.IsZero() && now.Sub(seq.updated) >= m.ttl { + m.deleteSequence(source, pgn, seqID) + } + } + } + } +} + +func (m *MultiBuilder) evictOldestIfFull() { + count := 0 + var oldest *sequence + var oldestSource, oldestSeq uint8 + var oldestPGN uint32 + for source, byPGN := range m.sequences { + for pgn, bySeq := range byPGN { + for seqID, seq := range bySeq { + count++ + if oldest == nil || seq.updated.Before(oldest.updated) { + oldest, oldestSource, oldestPGN, oldestSeq = seq, source, pgn, seqID + } + } + } + } + if count >= m.maxActive && oldest != nil { + m.deleteSequence(oldestSource, oldestPGN, oldestSeq) + } +} diff --git a/internal/adapter/sequence.go b/internal/adapter/sequence.go index 396e9d5..5263a55 100644 --- a/internal/adapter/sequence.go +++ b/internal/adapter/sequence.go @@ -3,6 +3,7 @@ package adapter import ( "fmt" "log/slog" + "time" "github.com/open-ships/n2k/internal/decoder" ) @@ -38,15 +39,12 @@ type sequence struct { // to trim padding bytes from the final frame. expected uint8 - // received tracks the running total of payload bytes accumulated so far across all frames. - // Frame 0 contributes 6 bytes; each continuation frame contributes 7 bytes. - received uint8 - // contents stores the data bytes from each frame, indexed by frame number. This array // allows frames to be received out of order (except frame 0 which must come first). // A nil entry means that frame has not been received yet. The array size is MaxFrameNum+1 // (32 slots) to accommodate frame numbers 0-31. contents [MaxFrameNum + 1][]uint8 // need arrays since packets can be received out of order + updated time.Time } // add copies the payload data from a CAN frame into the appropriate slot in the sequence. @@ -67,7 +65,8 @@ type sequence struct { // // Parameters: // - p: The Packet containing the raw CAN frame data with sequence/frame header in byte 0. -func (s *sequence) add(p *decoder.Packet) { +func (s *sequence) add(p *decoder.Packet, now time.Time) bool { + s.updated = now if p.FrameNum == 0 { if s.zero != nil { // we've received frame zero for a new sequence before completing the previous one. slog.Debug("Fast sequence duplicate frame zero detected. Resetting") @@ -76,15 +75,24 @@ func (s *sequence) add(p *decoder.Packet) { s.zero = p // Byte 1 of frame 0 contains the total expected payload length (not counting // the 2 header bytes of frame 0 or the 1 header byte of continuation frames). + if len(p.Data) < 2 { + p.ParseErrors = append(p.ParseErrors, fmt.Errorf("fast-packet frame zero is shorter than 2 bytes")) + return false + } s.expected = p.Data[1] + if s.expected == 0 || int(s.expected) > 223 { + p.ParseErrors = append(p.ParseErrors, fmt.Errorf("invalid fast-packet payload length %d", s.expected)) + return false + } // Bytes 2-7 of frame 0 contain the first 6 bytes of the actual payload. - s.contents[p.FrameNum] = p.Data[2:] - s.received += 6 + s.contents[p.FrameNum] = append([]uint8(nil), p.Data[2:]...) } else { if s.zero == nil { // we've received a subsequent frame before getting the first one slog.Debug("fast sequence received subsequent frame before zero frame, resetting", "source", p.Info.SourceId, "pgn", p.Info.PGN, "seqId", p.SeqId, "frameNum", p.FrameNum) s.reset() + s.updated = now + return true } else if s.contents[p.FrameNum] != nil { // uh-oh, we've already seen this frame // Duplicate frame detected -- likely a new sequence has started with the same // sequence ID before the old one completed. Reset to avoid mixing data from @@ -92,13 +100,15 @@ func (s *sequence) add(p *decoder.Packet) { slog.Debug("fast sequence received duplicate frame, resetting sequence", "source", p.Info.SourceId, "pgn", p.Info.PGN, "seqId", p.SeqId, "frameNum", p.FrameNum) s.reset() + s.updated = now + return true } else { // Normal continuation frame: copy bytes 1-7 (7 data bytes, skipping the // sequence/frame header in byte 0). - s.contents[p.FrameNum] = p.Data[1:] - s.received += 7 + s.contents[p.FrameNum] = append([]uint8(nil), p.Data[1:]...) } } + return true } // complete checks whether all expected payload bytes have been received and, if so, @@ -121,37 +131,30 @@ func (s *sequence) add(p *decoder.Packet) { // Returns true if the sequence is complete (either successfully or with errors), false // if more frames are still needed. func (s *sequence) complete(p *decoder.Packet) bool { - if s.zero != nil { - if s.received >= s.expected { - // All expected data has been received. Consolidate the per-frame data arrays - // into a single contiguous buffer. - results := make([]uint8, 0) - for i, d := range s.contents { - if d == nil { // don't allow sparse nodes - // A nil entry before we've collected enough bytes means frames arrived - // out of order with gaps -- this is a malformed sequence. - p.ParseErrors = append(p.ParseErrors, fmt.Errorf("sparse Data in multi")) - return true - } else { - results = append(results, s.contents[i]...) - // Once we've collected at least as many bytes as expected, stop - // processing further frames. - if len(results) >= int(s.expected) { - break - } - } - } - // Trim to exactly the expected length to remove padding bytes from the final - // CAN frame (which always carries a full 8 bytes even if not all are needed). - results = results[:s.expected] - p.Data = results - p.Complete = true - return true + if s.zero == nil { + p.Complete = false + return false + } + remaining := int(s.expected) - 6 + lastFrame := 0 + if remaining > 0 { + lastFrame = (remaining + 6) / 7 + } + results := make([]uint8, 0, s.expected) + for i := 0; i <= lastFrame; i++ { + if s.contents[i] == nil { + p.Complete = false + return false } + results = append(results, s.contents[i]...) + } + if len(results) < int(s.expected) { + p.ParseErrors = append(p.ParseErrors, fmt.Errorf("fast-packet frames contain %d bytes, expected %d", len(results), s.expected)) + return true } - // Not yet complete -- still waiting for more frames. - p.Complete = false - return false + p.Data = append([]uint8(nil), results[:s.expected]...) + p.Complete = true + return true } // reset clears all sequence state to allow reuse of the sequence slot for a new @@ -161,7 +164,7 @@ func (s *sequence) complete(p *decoder.Packet) bool { func (s *sequence) reset() { s.zero = nil s.expected = 0 - s.received = 0 + s.updated = time.Time{} // Clear all stored frame data to prevent stale data from mixing with new frames. for i := range s.contents { s.contents[i] = nil diff --git a/internal/canbus/cancellation_test.go b/internal/canbus/cancellation_test.go index 7c9ba1d..bf8488e 100644 --- a/internal/canbus/cancellation_test.go +++ b/internal/canbus/cancellation_test.go @@ -93,6 +93,8 @@ func (r *blockingCANReadWriteCloser) Close() error { type blockingSerialPort struct { closed chan struct{} once sync.Once + mu sync.Mutex + writes [][]byte } func newBlockingSerialPort() *blockingSerialPort { @@ -104,13 +106,28 @@ func (p *blockingSerialPort) Read([]byte) (int, error) { return 0, io.EOF } -func (p *blockingSerialPort) Write(buf []byte) (int, error) { return len(buf), nil } -func (p *blockingSerialPort) Drain() error { return nil } -func (p *blockingSerialPort) ResetInputBuffer() error { return nil } -func (p *blockingSerialPort) ResetOutputBuffer() error { return nil } -func (p *blockingSerialPort) SetDTR(bool) error { return nil } -func (p *blockingSerialPort) SetRTS(bool) error { return nil } -func (p *blockingSerialPort) SetMode(*serial.Mode) error { return nil } +func (p *blockingSerialPort) Write(buf []byte) (int, error) { + p.mu.Lock() + p.writes = append(p.writes, append([]byte(nil), buf...)) + p.mu.Unlock() + return len(buf), nil +} + +func (p *blockingSerialPort) writtenFrames() [][]byte { + p.mu.Lock() + defer p.mu.Unlock() + frames := make([][]byte, len(p.writes)) + for i := range p.writes { + frames[i] = append([]byte(nil), p.writes[i]...) + } + return frames +} +func (p *blockingSerialPort) Drain() error { return nil } +func (p *blockingSerialPort) ResetInputBuffer() error { return nil } +func (p *blockingSerialPort) ResetOutputBuffer() error { return nil } +func (p *blockingSerialPort) SetDTR(bool) error { return nil } +func (p *blockingSerialPort) SetRTS(bool) error { return nil } +func (p *blockingSerialPort) SetMode(*serial.Mode) error { return nil } func (p *blockingSerialPort) SetReadTimeout(time.Duration) error { return nil } diff --git a/internal/canbus/socketcan.go b/internal/canbus/socketcan.go index ea73ed2..905b552 100644 --- a/internal/canbus/socketcan.go +++ b/internal/canbus/socketcan.go @@ -30,8 +30,10 @@ type socketCANChannel struct { // bus is the underlying brutella/can bus object that manages the CAN socket connection. // It handles subscribing to incoming frames and publishing outgoing frames. - bus *can.Bus - mu sync.Mutex + bus *can.Bus + mu sync.Mutex + ready chan struct{} + readyOnce sync.Once // log is the structured logger for diagnostic output about interface state changes and errors. log *slog.Logger @@ -53,6 +55,7 @@ func newSocketCANChannel(log *slog.Logger, options socketCANChannelOptions) *soc c := socketCANChannel{ options: options, log: log, + ready: make(chan struct{}), } return &c @@ -78,6 +81,17 @@ func (c *socketCANChannel) Run(ctx context.Context, handler func(can.Frame)) err c.mu.Lock() c.bus = bus c.mu.Unlock() + defer func() { + c.mu.Lock() + if c.bus == bus { + c.bus = nil + c.mu.Unlock() + _ = bus.Disconnect() + return + } + c.mu.Unlock() + }() + c.readyOnce.Do(func() { close(c.ready) }) // Wrap the caller's handler so a nil handler becomes a no-op, then // subscribe it to receive all incoming CAN frames. @@ -86,7 +100,7 @@ func (c *socketCANChannel) Run(ctx context.Context, handler func(can.Frame)) err handler(frame) } } - c.bus.Subscribe(can.NewHandler(frameHandler)) + bus.Subscribe(can.NewHandler(frameHandler)) watchDone := make(chan struct{}) defer close(watchDone) @@ -132,6 +146,9 @@ func (c *socketCANChannel) Close() error { // The brutella/can library handles encoding the frame into the Linux SocketCAN wire format // and writing it to the raw CAN socket. Returns an error if the bus is not yet open. func (c *socketCANChannel) WriteFrame(frame can.Frame) error { + if frame.Length > 8 { + return errors.Errorf("socketCAN: invalid classical CAN payload length %d", frame.Length) + } c.mu.Lock() bus := c.bus c.mu.Unlock() @@ -141,6 +158,9 @@ func (c *socketCANChannel) WriteFrame(frame can.Frame) error { return bus.Publish(frame) } +// Ready closes once Run has opened the SocketCAN device for writes. +func (c *socketCANChannel) Ready() <-chan struct{} { return c.ready } + // NewSocketCAN creates a SocketCAN channel for the given Linux CAN interface name. // The channel is not opened, and no handler is registered, until Run() is called. func NewSocketCAN(log *slog.Logger, iface string) *socketCANChannel { diff --git a/internal/canbus/usbcan.go b/internal/canbus/usbcan.go index 00b01aa..11f87ae 100644 --- a/internal/canbus/usbcan.go +++ b/internal/canbus/usbcan.go @@ -62,8 +62,10 @@ type usbCANChannel struct { // mu guards the port reference. Reads and writes must not hold it because // Close needs to close the port concurrently to unblock them. - mu sync.Mutex - writeMu sync.Mutex + mu sync.Mutex + writeMu sync.Mutex + ready chan struct{} + readyOnce sync.Once // log is the structured logger for debug/info messages about frame parsing and errors. log *slog.Logger @@ -86,6 +88,7 @@ func newUSBCANChannel(log *slog.Logger, options usbCANChannelOptions) *usbCANCha c := usbCANChannel{ options: options, log: log, + ready: make(chan struct{}), } return &c @@ -118,6 +121,16 @@ func (c *usbCANChannel) Run(ctx context.Context, handler func(can.Frame)) error c.mu.Lock() c.port = port c.mu.Unlock() + defer func() { + c.mu.Lock() + if c.port == port { + c.port = nil + c.mu.Unlock() + _ = port.Close() + return + } + c.mu.Unlock() + }() watchDone := make(chan struct{}) defer close(watchDone) @@ -129,6 +142,21 @@ func (c *usbCANChannel) Run(ctx context.Context, handler func(can.Frame)) error } }() + // Put the adapter in normal, extended-frame mode at the NMEA 2000 CAN + // bitrate before making it available to writers. The serial baud rate above + // configures only the host-to-adapter link. + settings := usbCANSettingsFrame() + c.writeMu.Lock() + written, writeErr := port.Write(settings[:]) + c.writeMu.Unlock() + if writeErr != nil { + return fmt.Errorf("configure USB-CAN adapter: %w", writeErr) + } + if written != len(settings) { + return fmt.Errorf("configure USB-CAN adapter: wrote %d of %d bytes", written, len(settings)) + } + c.readyOnce.Do(func() { close(c.ready) }) + // Wrap the caller's handler so a nil handler becomes a no-op. c.handler = func(frame can.Frame) { if handler != nil { @@ -253,6 +281,11 @@ func (c *usbCANChannel) parseFrames(bufAddr *[]byte) error { // Bits [3:0] of the info byte encode the number of data bytes (0-8). dataLen := buf[1] & 0xf + if dataLen > 8 { + c.log.Warn("discarding USB-CAN frame with invalid payload length", "length", dataLen) + *bufAddr = buf[1:] + continue + } // Total frame length = header (2) + ID bytes (2 or 4) + data bytes + end byte (0x55) frameLen += dataLen + 1 @@ -279,17 +312,17 @@ func (c *usbCANChannel) parseFrames(bufAddr *[]byte) error { // Extract the data payload bytes (located between the ID and the end byte). dataBytes := buf[frameLen-1-dataLen : frameLen-1] if endByte != 0x55 { - // If the end byte is wrong, the frame is corrupt. Log it and discard the entire - // buffer since we can't reliably determine where the next valid frame starts. + // If the end byte is wrong, discard only this candidate start byte. + // The outer resynchronizer can then recover a valid frame already + // buffered behind the corrupt candidate. c.log.Debug("Data frame with bad end byte", "extended", extendedFrame, "remote", remoteFrame, "frameID", fmt.Sprintf("%X", frameID), "data", fmt.Sprintf("%+v", dataBytes), "endByte", fmt.Sprintf("%X", endByte)) - *bufAddr = []byte{} - return nil + *bufAddr = buf[1:] + continue } - // Build a can.Frame struct with the parsed data. The can.Frame.Data field is a // fixed [8]byte array, so we copy only the actual data bytes into it. fData := [8]byte{} @@ -312,11 +345,31 @@ func (c *usbCANChannel) parseFrames(bufAddr *[]byte) error { // ----- Unknown Frame Type ----- // If we get here, the byte after 0xAA is neither 0x55 (command) nor has bits[7:6]=0b11 (data). - // This is an unrecognized frame type -- log it and discard the buffer. + // Discard only this false start so a later valid frame can still be found. c.log.Debug("Unknown frame", "data", fmt.Sprintf("%+v", buf)) - *bufAddr = []byte{} - return nil + *bufAddr = buf[1:] + continue + } +} + +// usbCANSettingsFrame selects 250 kbit/s CAN, 29-bit extended frames, and +// normal (non-loopback, non-silent) operation. The final byte is the protocol +// checksum: the low byte of the sum of bytes 2 through 18. +func usbCANSettingsFrame() [20]byte { + settings := [20]byte{ + 0xAA, 0x55, 0x12, + 0x05, // 250 kbit/s CAN bitrate + 0x02, // extended CAN frames + 0x00, 0x00, 0x00, 0x00, // acceptance code + 0x00, 0x00, 0x00, 0x00, // acceptance mask + 0x00, // normal mode + 0x01, // apply settings + 0x00, 0x00, 0x00, 0x00, + } + for _, b := range settings[2:19] { + settings[19] += b } + return settings } // Close shuts down the USB-CAN channel by closing the underlying serial port. @@ -337,30 +390,25 @@ func (c *usbCANChannel) Close() error { // // Outgoing data frame format: // - Byte 0: 0xAA (start-of-frame marker) -// - Byte 1: 0xC0 | dataLen (info byte: bits[7:6]=0b11 for data frame, bits[3:0]=length) -// If the CAN ID requires more than 16 bits, bit 5 is set for extended frame. -// - Bytes 2-3 (standard) or 2-5 (extended): CAN ID in little-endian byte order +// - Byte 1: 0xE0 | dataLen (data frame + 29-bit extended-ID flag + length) +// - Bytes 2-5: CAN ID in little-endian byte order // - Next N bytes: data payload (N = frame.Length) // - Final byte: 0x55 (end-of-frame marker) -// -// Note: This currently defaults to standard frames and switches to extended only if the -// frame ID exceeds 0xFFFF. For NMEA 2000, which always uses 29-bit extended IDs, the -// frame ID will always exceed 0xFFFF, so extended mode is used automatically. func (c *usbCANChannel) WriteFrame(frame can.Frame) error { - // Build the frame header: start byte + info byte + 2-byte little-endian ID (standard). + if frame.Length > 8 { + return fmt.Errorf("WriteFrame: invalid classical CAN payload length %d", frame.Length) + } + if frame.ID > 0x1FFFFFFF { + return fmt.Errorf("WriteFrame: invalid 29-bit CAN ID 0x%X", frame.ID) + } + // NMEA 2000 always uses a four-byte, 29-bit extended CAN identifier. buf := []byte{ 0xaa, - 0xC0 | frame.Length, // 0xC0 = bits[7:6]=0b11 (data frame), OR'd with data length in bits[3:0] - byte(frame.ID), // CAN ID low byte - byte(frame.ID >> 8), // CAN ID high byte (for standard frames) - } - // Not sure if this is the right way to do it, but for now give it a shot - // (we're only sending standard frames over calex, so need to test this on n2k someday if we care...) - if frame.ID > 0xffff { - // The CAN ID needs more than 16 bits, so switch to extended frame format. - // Set bit 5 of the info byte to indicate extended frame, and append the upper 2 ID bytes. - buf[1] |= 0x20 - buf = append(buf, byte(frame.ID>>16), byte(frame.ID>>24)) + 0xE0 | frame.Length, + byte(frame.ID), + byte(frame.ID >> 8), + byte(frame.ID >> 16), + byte(frame.ID >> 24), } // Append the data payload bytes and the end-of-frame marker. buf = append(buf, frame.Data[0:frame.Length]...) @@ -385,6 +433,9 @@ func (c *usbCANChannel) WriteFrame(frame can.Frame) error { return nil } +// Ready closes once Run has opened the serial adapter for writes. +func (c *usbCANChannel) Ready() <-chan struct{} { return c.ready } + // NewUSBCAN creates a USB-CAN channel for the given serial port. // The channel is not opened, and no handler is registered, until Run() is called. func NewUSBCAN(log *slog.Logger, port string) *usbCANChannel { diff --git a/internal/canbus/usbcan_test.go b/internal/canbus/usbcan_test.go index 4b56079..4e5653c 100644 --- a/internal/canbus/usbcan_test.go +++ b/internal/canbus/usbcan_test.go @@ -1,11 +1,15 @@ package canbus import ( + "context" "log/slog" "testing" + "time" "github.com/brutella/can" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.bug.st/serial" ) // newTestChannel creates a usbCANChannel for testing with a no-op logger and a handler @@ -181,8 +185,7 @@ func TestParseFrames_ErrorRecovery_NoAA(t *testing.T) { } // TestParseFrames_BadEndByte verifies that a data frame with an incorrect end byte -// (anything other than 0x55) is rejected and the buffer is cleared. The parser cannot -// reliably determine frame boundaries after a bad end byte, so it discards everything. +// (anything other than 0x55) is rejected without losing a valid frame that follows it. func TestParseFrames_BadEndByte(t *testing.T) { usbChan, received := newTestChannel() @@ -194,11 +197,43 @@ func TestParseFrames_BadEndByte(t *testing.T) { 0x10, 0x00, // ID 0xAA, 0xBB, // data 0x99, // bad end byte (should be 0x55) + 0xAA, 0xE1, 0x83, 0x01, 0xF2, 0x09, 0x42, 0x55, } err := usbChan.parseFrames(&buf) assert.NoError(t, err) - assert.Equal(t, 0, len(*received), "Bad end byte should not produce a frame") - assert.Equal(t, 0, len(buf), "Buffer should be cleared on bad end byte") + require.Len(t, *received, 1) + assert.Equal(t, uint8(0x42), (*received)[0].Data[0]) + assert.Empty(t, buf) +} + +func TestUSBCANRunConfiguresNMEA2000BeforeReady(t *testing.T) { + port := newBlockingSerialPort() + ch := newUSBCANChannel(testLogger(), usbCANChannelOptions{ + SerialPortName: "test", + SerialBaudRate: 2000000, + openPort: func(string, *serial.Mode) (serial.Port, error) { + return port, nil + }, + }) + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- ch.Run(ctx, nil) }() + + select { + case <-ch.Ready(): + case <-time.After(time.Second): + t.Fatal("USB-CAN channel did not become ready") + } + require.Equal(t, [][]byte{{ + 0xAA, 0x55, 0x12, 0x05, 0x02, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x1A, + }}, port.writtenFrames()) + + cancel() + require.ErrorIs(t, <-done, context.Canceled) } // TestParseFrames_IncompleteFrame_TooShort verifies that when only the start byte (0xAA) @@ -329,3 +364,26 @@ func TestParseFrames_ZeroLengthDataFrame(t *testing.T) { assert.Equal(t, byte(0), (*received)[0].Length) assert.Equal(t, 0, len(buf)) } + +func TestParseFrames_InvalidDLCResynchronizes(t *testing.T) { + usbChan, received := newTestChannel() + buf := []byte{ + 0xAA, 0xEF, 0x01, 0x02, 0x03, // invalid DLC 15 + 0xAA, 0xE1, 0x83, 0x01, 0xF2, 0x09, 0x42, 0x55, + } + require.NoError(t, usbChan.parseFrames(&buf)) + require.Len(t, *received, 1) + assert.Equal(t, uint8(1), (*received)[0].Length) + assert.Equal(t, uint8(0x42), (*received)[0].Data[0]) +} + +func FuzzUSBCANParseFrames(f *testing.F) { + f.Add([]byte{0xAA, 0xE1, 0x83, 0x01, 0xF2, 0x09, 0x42, 0x55}) + f.Add([]byte{0xAA, 0xEF, 0xAA, 0x55}) + f.Fuzz(func(t *testing.T, input []byte) { + ch, _ := newTestChannel() + buf := append([]byte(nil), input...) + require.NotPanics(t, func() { _ = ch.parseFrames(&buf) }) + assert.LessOrEqual(t, len(buf), max(len(input), 20)) + }) +} diff --git a/internal/claiming/claimer.go b/internal/claiming/claimer.go index 0e78c01..238ca0f 100644 --- a/internal/claiming/claimer.go +++ b/internal/claiming/claimer.go @@ -28,16 +28,18 @@ const globalDestination = 255 const cannotClaimAddress = 254 // minAddress is the lowest valid claimable address on an NMEA 2000 network. -const minAddress = 1 +const minAddress = 0 // maxAddress is the highest valid claimable address on an NMEA 2000 network. -const maxAddress = 253 +const maxAddress = 251 + +const claimableAddressCount = int(maxAddress-minAddress) + 1 // Mode controls how the claimer responds to address contention. type Mode int const ( - // ModeAuto starts at the configured address (default 253) and works + // ModeAuto starts at the configured address (default 251) and works // downward on contention until all addresses are exhausted, at which // point it falls back to address 254 (cannot claim). ModeAuto Mode = iota @@ -53,7 +55,7 @@ type Config struct { Mode Mode // Address is the starting address in auto mode or the fixed address in - // explicit mode. Valid range is 1-253. + // explicit mode. Valid range is 0-251. Address uint8 // Name is our device's 64-bit ISO 11783 NAME used for arbitration. @@ -63,8 +65,9 @@ type Config struct { // WriteFrame sends a CAN frame onto the bus. WriteFrame func(can.Frame) error - // OnAddressChange is called when the claimed address changes (auto mode). - // It may be nil if the caller does not need notifications. + // OnAddressChange is called when the claimed address changes, either while + // resolving contention or after a matching Commanded Address transfer. It + // may be nil if the caller does not need notifications. OnAddressChange func(newAddr uint8) // OnFatalError is called when an unrecoverable error occurs, such as @@ -175,8 +178,41 @@ func (c *Claimer) HandleISORequest() { } } +// HandleCommandedAddress applies an ISO Commanded Address (PGN 65240) to this +// node. The caller is responsible for accepting the command only from a fully +// reassembled, nine-byte broadcast transport transfer. +// +// A command is ignored unless its complete 64-bit NAME matches this node and +// the requested address is claimable (0-251). A real change resets automatic +// contention history, notifies the owner, and immediately broadcasts a fresh +// Address Claim from the commanded address. The returned bool reports whether +// a change was applied. +func (c *Claimer) HandleCommandedAddress(commandedName uint64, newAddress uint8) (bool, error) { + c.mu.Lock() + defer c.mu.Unlock() + + if commandedName != c.cfg.Name || newAddress > maxAddress || newAddress == c.address { + return false, nil + } + + oldAddress := c.address + c.address = newAddress + c.tried = map[uint8]bool{newAddress: true} + c.cfg.Logger.Info("applying commanded address", + "oldAddress", oldAddress, + "newAddress", newAddress, + ) + if c.cfg.OnAddressChange != nil { + c.cfg.OnAddressChange(newAddress) + } + if err := c.sendClaim(); err != nil { + return true, fmt.Errorf("claiming commanded address %d: %w", newAddress, err) + } + return true, nil +} + // Address returns the currently claimed address. This value may change over -// time in auto mode as contention is resolved. +// time as contention is resolved or a matching Commanded Address is applied. func (c *Claimer) Address() uint8 { c.mu.Lock() defer c.mu.Unlock() @@ -193,21 +229,24 @@ func (c *Claimer) sendClaim() error { } // tryNextAddress searches downward from the current address for an untried -// address. If all addresses (1-253) have been tried, it falls back to 254. +// address. If all addresses (0-251) have been tried, it falls back to 254. // The caller must hold c.mu. func (c *Claimer) tryNextAddress() { oldAddr := c.address - // Scan downward from current address-1, wrapping around from minAddress - // back to maxAddress, covering all 253 valid addresses. - for candidate := oldAddr - 1; ; candidate-- { - if candidate < minAddress { - candidate = maxAddress - } - if candidate == oldAddr { - // We have wrapped all the way around — no addresses available. - break + // Scan downward and wrap without uint8 underflow. The final iteration + // revisits oldAddr, which is already marked tried, so every one of the 252 + // valid NMEA 2000 source addresses is considered exactly once. + start := int(oldAddr) + if oldAddr > maxAddress { + start = int(maxAddress) + 1 + } + for offset := 1; offset <= claimableAddressCount; offset++ { + candidateInt := start - offset + for candidateInt < int(minAddress) { + candidateInt += claimableAddressCount } + candidate := uint8(candidateInt) if !c.tried[candidate] { c.address = candidate c.tried[candidate] = true diff --git a/internal/claiming/claimer_test.go b/internal/claiming/claimer_test.go index 77a80ed..7b1d8d3 100644 --- a/internal/claiming/claimer_test.go +++ b/internal/claiming/claimer_test.go @@ -68,7 +68,7 @@ func TestClaimer_AutoMode_HappyPath(t *testing.T) { c := claiming.New(claiming.Config{ Mode: claiming.ModeAuto, - Address: 253, + Address: 251, Name: ourName, WriteFrame: sink.write, Logger: discardLogger(), @@ -81,9 +81,9 @@ func TestClaimer_AutoMode_HappyPath(t *testing.T) { require.Equal(t, 1, sink.count()) f := sink.lastFrame() - assert.Equal(t, expectedCANID(253), f.ID, "CAN ID should target address 253") + assert.Equal(t, expectedCANID(251), f.ID, "CAN ID should use address 251") assert.Equal(t, ourName, extractClaimName(f), "payload should contain our NAME") - assert.Equal(t, uint8(253), c.Address(), "claimed address should be 253") + assert.Equal(t, uint8(251), c.Address(), "claimed address should be 251") } func TestClaimer_AutoMode_Contention(t *testing.T) { @@ -94,7 +94,7 @@ func TestClaimer_AutoMode_Contention(t *testing.T) { var newAddr uint8 c := claiming.New(claiming.Config{ Mode: claiming.ModeAuto, - Address: 253, + Address: 251, Name: ourName, WriteFrame: sink.write, OnAddressChange: func(addr uint8) { @@ -106,16 +106,16 @@ func TestClaimer_AutoMode_Contention(t *testing.T) { err := c.Start() require.NoError(t, err) - // Competitor claims address 253 with a lower NAME — we lose. - c.HandleAddressClaim(253, competitorName) + // Competitor claims address 251 with a lower NAME — we lose. + c.HandleAddressClaim(251, competitorName) - // We should have retried on 252. - assert.Equal(t, uint8(252), c.Address(), "should have moved to 252") - assert.Equal(t, uint8(252), newAddr, "OnAddressChange should have been called with 252") + // We should have retried on 250. + assert.Equal(t, uint8(250), c.Address(), "should have moved to 250") + assert.Equal(t, uint8(250), newAddr, "OnAddressChange should have been called with 250") - // Two frames total: initial claim at 253, retry claim at 252. + // Two frames total: initial claim at 251, retry claim at 250. require.Equal(t, 2, sink.count()) - assert.Equal(t, uint8(252), extractClaimAddress(sink.lastFrame())) + assert.Equal(t, uint8(250), extractClaimAddress(sink.lastFrame())) } func TestClaimer_AutoMode_MultipleRetries(t *testing.T) { @@ -126,7 +126,7 @@ func TestClaimer_AutoMode_MultipleRetries(t *testing.T) { var addressHistory []uint8 c := claiming.New(claiming.Config{ Mode: claiming.ModeAuto, - Address: 253, + Address: 251, Name: ourName, WriteFrame: sink.write, OnAddressChange: func(addr uint8) { @@ -139,16 +139,16 @@ func TestClaimer_AutoMode_MultipleRetries(t *testing.T) { require.NoError(t, err) // Lose contention three times in a row. - c.HandleAddressClaim(253, competitorName) - assert.Equal(t, uint8(252), c.Address()) - - c.HandleAddressClaim(252, competitorName) - assert.Equal(t, uint8(251), c.Address()) - c.HandleAddressClaim(251, competitorName) assert.Equal(t, uint8(250), c.Address()) - assert.Equal(t, []uint8{252, 251, 250}, addressHistory) + c.HandleAddressClaim(250, competitorName) + assert.Equal(t, uint8(249), c.Address()) + + c.HandleAddressClaim(249, competitorName) + assert.Equal(t, uint8(248), c.Address()) + + assert.Equal(t, []uint8{250, 249, 248}, addressHistory) // 1 initial + 3 retries = 4 frames. assert.Equal(t, 4, sink.count()) @@ -182,16 +182,20 @@ func TestClaimer_AutoMode_Exhaustion(t *testing.T) { c.HandleAddressClaim(2, competitorName) assert.Equal(t, uint8(1), c.Address()) - // Contest address 1, should wrap to 253. + // Contest address 1, should try 0. c.HandleAddressClaim(1, competitorName) - assert.Equal(t, uint8(253), c.Address()) + assert.Equal(t, uint8(0), c.Address()) + + // Contest address 0, which wraps to the highest valid address. + c.HandleAddressClaim(0, competitorName) + assert.Equal(t, uint8(251), c.Address()) - // Now contest every remaining address down from 253 to 4. - for addr := uint8(253); addr >= 4; addr-- { + // Now contest every remaining address down from 251 to 4. + for addr := uint8(251); addr >= 4; addr-- { c.HandleAddressClaim(addr, competitorName) } - // All 253 addresses exhausted — should fall back to 254. + // All 252 addresses exhausted — should fall back to 254. assert.Equal(t, uint8(254), c.Address()) assert.Equal(t, uint8(254), lastAddr) } @@ -364,6 +368,122 @@ func TestClaimer_WriteFrameError(t *testing.T) { assert.ErrorIs(t, err, writeErr, "Start should propagate WriteFrame errors") } +func TestClaimer_CommandedAddressReclaimsImmediately(t *testing.T) { + sink := &frameSink{} + ourName := uint64(0x9123456789ABCDEF) + var changedTo uint8 + c := claiming.New(claiming.Config{ + Mode: claiming.ModeAuto, + Address: 100, + Name: ourName, + WriteFrame: sink.write, + OnAddressChange: func(address uint8) { + changedTo = address + }, + Logger: discardLogger(), + }) + require.NoError(t, c.Start()) + + changed, err := c.HandleCommandedAddress(ourName, 42) + require.NoError(t, err) + require.True(t, changed) + assert.Equal(t, uint8(42), c.Address()) + assert.Equal(t, uint8(42), changedTo) + require.Equal(t, 2, sink.count()) + assert.Equal(t, expectedCANID(42), sink.lastFrame().ID) + assert.Equal(t, ourName, extractClaimName(sink.lastFrame())) + + // A command establishes a new automatic-claim starting point. If that + // address then loses contention, negotiation continues from the new value. + c.HandleAddressClaim(42, 1) + assert.Equal(t, uint8(41), c.Address()) + assert.Equal(t, expectedCANID(41), sink.lastFrame().ID) +} + +func TestClaimer_CommandedAddressRejectsNonTargetsAndSpecialAddresses(t *testing.T) { + tests := []struct { + name string + commandedName uint64 + commandedAddr uint8 + }{ + {name: "different NAME", commandedName: 0x22, commandedAddr: 42}, + {name: "current address", commandedName: 0x11, commandedAddr: 100}, + {name: "reserved 252", commandedName: 0x11, commandedAddr: 252}, + {name: "reserved 253", commandedName: 0x11, commandedAddr: 253}, + {name: "cannot claim 254", commandedName: 0x11, commandedAddr: 254}, + {name: "broadcast 255", commandedName: 0x11, commandedAddr: 255}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sink := &frameSink{} + c := claiming.New(claiming.Config{ + Mode: claiming.ModeAuto, + Address: 100, + Name: 0x11, + WriteFrame: sink.write, + Logger: discardLogger(), + }) + require.NoError(t, c.Start()) + + changed, err := c.HandleCommandedAddress(tt.commandedName, tt.commandedAddr) + require.NoError(t, err) + assert.False(t, changed) + assert.Equal(t, uint8(100), c.Address()) + assert.Equal(t, 1, sink.count()) + }) + } +} + +func TestClaimer_CommandedAddressPreservesExplicitContentionPolicy(t *testing.T) { + sink := &frameSink{} + var fatalErr error + c := claiming.New(claiming.Config{ + Mode: claiming.ModeExplicit, + Address: 100, + Name: 5000, + WriteFrame: sink.write, + OnFatalError: func(err error) { + fatalErr = err + }, + Logger: discardLogger(), + }) + require.NoError(t, c.Start()) + + changed, err := c.HandleCommandedAddress(5000, 42) + require.NoError(t, err) + require.True(t, changed) + c.HandleAddressClaim(42, 1000) + + require.Error(t, fatalErr) + assert.Contains(t, fatalErr.Error(), "address 42 contested in explicit mode") + assert.Equal(t, uint8(42), c.Address()) +} + +func TestClaimer_CommandedAddressClaimFailureIsReturned(t *testing.T) { + writeErr := fmt.Errorf("bus offline") + writes := 0 + c := claiming.New(claiming.Config{ + Mode: claiming.ModeAuto, + Address: 100, + Name: 0x11, + WriteFrame: func(can.Frame) error { + writes++ + if writes == 2 { + return writeErr + } + return nil + }, + Logger: discardLogger(), + }) + require.NoError(t, c.Start()) + + changed, err := c.HandleCommandedAddress(0x11, 42) + assert.True(t, changed) + assert.ErrorIs(t, err, writeErr) + assert.Equal(t, uint8(42), c.Address()) +} + func TestClaimer_AutoMode_WrapAround(t *testing.T) { sink := &frameSink{} ourName := uint64(5000) @@ -371,7 +491,7 @@ func TestClaimer_AutoMode_WrapAround(t *testing.T) { c := claiming.New(claiming.Config{ Mode: claiming.ModeAuto, - Address: 1, // start at lowest valid address + Address: 0, // start at lowest valid address Name: ourName, WriteFrame: sink.write, Logger: discardLogger(), @@ -380,9 +500,9 @@ func TestClaimer_AutoMode_WrapAround(t *testing.T) { err := c.Start() require.NoError(t, err) - // Lose contention at address 1 — should wrap to 253. - c.HandleAddressClaim(1, competitorName) - assert.Equal(t, uint8(253), c.Address(), "should wrap from 1 to 253") + // Lose contention at address 0 — should wrap to 251. + c.HandleAddressClaim(0, competitorName) + assert.Equal(t, uint8(251), c.Address(), "should wrap from 0 to 251") } func TestClaimer_PayloadMatchesLittleEndianName(t *testing.T) { diff --git a/internal/framer/canid.go b/internal/framer/canid.go index 5d24ee0..a6bd73f 100644 --- a/internal/framer/canid.go +++ b/internal/framer/canid.go @@ -21,7 +21,7 @@ package framer // Parameters: // - pgn: The Parameter Group Number (18-bit). For PDU1 PGNs, the low byte must be 0. // - priority: Message priority (0-7). Only the lower 3 bits are used. -// - source: Source address of the transmitting device (0-253, 254-255 reserved). +// - source: Source address of the transmitting device (0-251; 252-255 are reserved/special in NMEA 2000). // - destination: Destination address for PDU1 (addressed) PGNs. Ignored for PDU2 (broadcast) PGNs. // // Returns the constructed 29-bit CAN ID. diff --git a/internal/framer/canid_test.go b/internal/framer/canid_test.go index c97b2db..0b982ce 100644 --- a/internal/framer/canid_test.go +++ b/internal/framer/canid_test.go @@ -106,7 +106,7 @@ func TestParseCANIDInverseOfBuild(t *testing.T) { }{ {"pdu2 broadcast", 127250, 2, 42, 255, false}, {"pdu1 addressed", 59904, 6, 10, 42, true}, - {"pdu1 broadcast dest", 60928, 6, 253, 255, true}, + {"pdu1 broadcast dest", 60928, 6, 251, 255, true}, {"high pgn", 130824, 7, 1, 255, false}, } for _, tc := range cases { diff --git a/internal/gateway/actisense.go b/internal/gateway/actisense.go index 409bdfa..b0e9dfb 100644 --- a/internal/gateway/actisense.go +++ b/internal/gateway/actisense.go @@ -50,6 +50,9 @@ type ActisenseReader struct { body []byte } +// Maximum unescaped body: command + one-byte length + 255 payload bytes + checksum. +const maxActisenseBody = 258 + // NewActisenseReader returns a reader ready to consume stream bytes. func NewActisenseReader() *ActisenseReader { return &ActisenseReader{} @@ -78,6 +81,9 @@ func (r *ActisenseReader) Feed(buf []byte, emit func(N2KMessage)) { case dle: // Escaped DLE: a literal 0x10 body byte. r.body = append(r.body, dle) + if len(r.body) > maxActisenseBody { + r.reset() + } case etx: r.finish(emit) case stx: @@ -95,9 +101,18 @@ func (r *ActisenseReader) Feed(buf []byte, emit func(N2KMessage)) { continue } r.body = append(r.body, b) + if len(r.body) > maxActisenseBody { + r.reset() + } } } +func (r *ActisenseReader) reset() { + r.inMessage = false + r.escaped = false + r.body = r.body[:0] +} + // finish validates and emits the accumulated message body, then resets. func (r *ActisenseReader) finish(emit func(N2KMessage)) { defer func() { diff --git a/internal/gateway/actisense_test.go b/internal/gateway/actisense_test.go index 393ac73..a39288b 100644 --- a/internal/gateway/actisense_test.go +++ b/internal/gateway/actisense_test.go @@ -134,6 +134,40 @@ func TestActisenseReader_TruncatedPayloadDropped(t *testing.T) { assert.Zero(t, count) } +func TestActisenseReader_OversizedBodyRecovers(t *testing.T) { + oversized := append([]byte{dle, stx}, make([]byte, maxActisenseBody+1)...) + valid := wrapActisense(cmdN2KReceived, n2kPayload(2, 127250, 255, 0x23, []byte{1})) + stream := append(oversized, valid...) + + var got []N2KMessage + r := NewActisenseReader() + r.Feed(stream, func(m N2KMessage) { got = append(got, m) }) + require.Len(t, got, 1) + assert.Equal(t, []byte{1}, got[0].Data) +} + +func FuzzActisenseReader(f *testing.F) { + f.Add(wrapActisense(cmdN2KReceived, n2kPayload(2, 127250, 255, 0x23, []byte{1, 2, 3}))) + f.Add([]byte{dle, stx, 0x93, 0xFF}) + f.Fuzz(func(t *testing.T, input []byte) { + r := NewActisenseReader() + emitted := 0 + for start := 0; start < len(input); { + end := start + 7 + if end > len(input) { + end = len(input) + } + r.Feed(input[start:end], func(m N2KMessage) { + emitted++ + assert.LessOrEqual(t, len(m.Data), 244) + }) + start = end + } + assert.LessOrEqual(t, len(r.body), maxActisenseBody) + assert.LessOrEqual(t, emitted, len(input)/3+1) + }) +} + func TestReframe_SingleFrame(t *testing.T) { // PGN 127250 (VesselHeading) is a single-frame PGN. data := []byte{0x01, 0x5C, 0x3D, 0xFF, 0x7F, 0xFF, 0x7F, 0xFC} diff --git a/internal/transport/bam.go b/internal/transport/bam.go index 8109f0f..fb89cf9 100644 --- a/internal/transport/bam.go +++ b/internal/transport/bam.go @@ -11,8 +11,12 @@ func (m *Manager) handleBAMReceive(frame can.Frame, source uint8) { numFrames := frame.Data[3] pgn := extractPGN(frame.Data) - if totalSize == 0 || numFrames == 0 { - m.logger.Warn("BAM with zero size or frames", "source", source, "pgn", pgn) + if err := validateAnnouncement(totalSize, numFrames); err != nil { + m.logger.Warn("invalid BAM announcement", "source", source, "pgn", pgn, "error", err) + return + } + if err := validateTransportPGN(pgn); err != nil { + m.logger.Warn("invalid BAM PGN", "source", source, "pgn", pgn, "error", err) return } @@ -25,8 +29,7 @@ func (m *Manager) handleBAMReceive(frame can.Frame, source uint8) { m.mu.Lock() defer m.mu.Unlock() - // If there's already a session for this key, clean it up first. - m.removeSession(key) + m.removeReceiveSessions(source, BroadcastAddr) sess := &session{ key: key, diff --git a/internal/transport/constants.go b/internal/transport/constants.go index 8138776..6232f82 100644 --- a/internal/transport/constants.go +++ b/internal/transport/constants.go @@ -52,3 +52,10 @@ const BAMInterFrameDelay = 50 * time.Millisecond // MaxDTDataBytes is the number of data bytes carried per DT frame (bytes 1-7). const MaxDTDataBytes = 7 + +// MaxPayloadBytes is the largest payload representable by the transport +// protocol's one-byte DT packet count (255 packets of seven data bytes). +const MaxPayloadBytes = 255 * MaxDTDataBytes + +// MaxPGN is the largest value representable by an NMEA 2000 18-bit PGN. +const MaxPGN = 0x3FFFF diff --git a/internal/transport/rtscts.go b/internal/transport/rtscts.go index b4bc79f..c78b747 100644 --- a/internal/transport/rtscts.go +++ b/internal/transport/rtscts.go @@ -13,8 +13,16 @@ func (m *Manager) handleRTSReceive(frame can.Frame, source uint8, destination ui maxPerCTS := frame.Data[4] pgn := extractPGN(frame.Data) - if totalSize == 0 || numFrames == 0 { - m.logger.Warn("RTS with zero size or frames", "source", source, "pgn", pgn) + if err := validateAnnouncement(totalSize, numFrames); err != nil { + m.logger.Warn("invalid RTS announcement", "source", source, "pgn", pgn, "error", err) + return + } + if err := validateTransportPGN(pgn); err != nil { + m.logger.Warn("invalid RTS PGN", "source", source, "pgn", pgn, "error", err) + return + } + if maxPerCTS == 0 { + m.logger.Warn("invalid RTS maximum packets per CTS", "source", source, "pgn", pgn) return } @@ -25,10 +33,8 @@ func (m *Manager) handleRTSReceive(frame can.Frame, source uint8, destination ui } m.mu.Lock() - defer m.mu.Unlock() - // If there's already a session for this key, clean it up first. - m.removeSession(key) + m.removeReceiveSessions(source, destination) sess := &session{ key: key, @@ -50,17 +56,27 @@ func (m *Manager) handleRTSReceive(frame can.Frame, source uint8, destination ui }) m.sessions[key] = sess + m.mu.Unlock() m.logger.Debug("RTS/CTS receive session started", "source", source, "destination", destination, "pgn", pgn, "totalSize", totalSize, "numFrames", numFrames) // Send CTS: request all frames starting from 1. - m.sendCTS(numFrames, 1, pgn, destination, source) + requested := numFrames + if maxPerCTS < requested { + requested = maxPerCTS + } + if err := m.config.WriteFrame(buildCTSFrame(requested, 1, pgn, destination, source)); err != nil { + m.mu.Lock() + m.removeSession(key) + m.mu.Unlock() + m.logger.Warn("failed to send CTS", "error", err, "pgn", pgn) + } } -// sendCTS sends a CTS (Clear To Send) frame. -func (m *Manager) sendCTS(numFrames uint8, nextSeqNum uint8, pgn uint32, source uint8, destination uint8) { +// buildCTSFrame constructs a CTS (Clear To Send) frame. +func buildCTSFrame(numFrames uint8, nextSeqNum uint8, pgn uint32, source uint8, destination uint8) can.Frame { var data [8]uint8 data[0] = ControlCTS data[1] = numFrames @@ -70,12 +86,9 @@ func (m *Manager) sendCTS(numFrames uint8, nextSeqNum uint8, pgn uint32, source encodePGN(data[5:8], pgn) canID := framer.BuildCANID(PGNCM, TPPriority, source, destination) - f := can.Frame{ + return can.Frame{ ID: canID, Length: 8, Data: data, } - if err := m.config.WriteFrame(f); err != nil { - m.logger.Warn("failed to send CTS", "error", err, "pgn", pgn) - } } diff --git a/internal/transport/rtscts_test.go b/internal/transport/rtscts_test.go index 0d4ed13..db54e2b 100644 --- a/internal/transport/rtscts_test.go +++ b/internal/transport/rtscts_test.go @@ -84,6 +84,56 @@ func TestRTSCTSReceive_HappyPath(t *testing.T) { assert.Equal(t, payload, msg.data) } +func TestRTSCTSReceive_RequestsEachCTSWindow(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{WriteFrame: h.writeFrame, OnComplete: h.onComplete}) + defer mgr.Close() + + const ( + source = uint8(42) + destination = uint8(10) + messagePGN = uint32(126998) + ) + payload := make([]byte, 29) // five DT packets + for i := range payload { + payload[i] = byte(i + 1) + } + rts := buildTestCMFrame(ControlRTS, uint16(len(payload)), 5, messagePGN, source, destination) + rts.Data[4] = 2 + mgr.HandleFrame(rts) + + assertCTS := func(index int, count, first uint8) { + t.Helper() + frames := h.getSentFrames() + require.Greater(t, len(frames), index) + assert.Equal(t, ControlCTS, frames[index].Data[0]) + assert.Equal(t, count, frames[index].Data[1]) + assert.Equal(t, first, frames[index].Data[2]) + } + assertCTS(0, 2, 1) + + for seq := uint8(1); seq <= 5; seq++ { + var data [7]byte + start := int(seq-1) * MaxDTDataBytes + if start < len(payload) { + copy(data[:], payload[start:]) + } + mgr.HandleFrame(buildTestDTFrame(seq, data, source, destination)) + if seq == 2 { + assertCTS(1, 2, 3) + } + if seq == 4 { + assertCTS(2, 1, 5) + } + } + + msg := h.waitComplete(t, time.Second) + assert.Equal(t, payload, msg.data) + frames := h.getSentFrames() + require.Len(t, frames, 4) + assert.Equal(t, ControlEndOfMsgAck, frames[3].Data[0]) +} + func TestRTSCTSTransmit_HappyPath(t *testing.T) { h := newTestHelper() mgr := NewManager(ManagerConfig{ @@ -162,6 +212,95 @@ func TestRTSCTSTransmit_HappyPath(t *testing.T) { } } +func TestRTSCTSTransmit_RejectsInvalidCTSRange(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{WriteFrame: h.writeFrame}) + defer mgr.Close() + + errCh := make(chan error, 1) + go func() { + errCh <- mgr.SendRTSCTS(126998, 10, 42, make([]byte, 16)) + }() + require.Eventually(t, func() bool { return len(h.getSentFrames()) == 1 }, time.Second, time.Millisecond) + + // Three packets exist, so a request for packets 2 through 4 is invalid. + mgr.HandleFrame(buildTestCTSFrame(3, 2, 126998, 42, 10)) + select { + case err := <-errCh: + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid CTS range") + case <-time.After(time.Second): + t.Fatal("SendRTSCTS did not reject invalid CTS") + } + assert.Len(t, h.getSentFrames(), 1, "invalid CTS must not emit DT packets") +} + +func TestRTSCTSTransmit_RejectsMismatchedEndAck(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{WriteFrame: h.writeFrame}) + defer mgr.Close() + + errCh := make(chan error, 1) + go func() { + errCh <- mgr.SendRTSCTS(126998, 10, 42, make([]byte, 16)) + }() + require.Eventually(t, func() bool { return len(h.getSentFrames()) == 1 }, time.Second, time.Millisecond) + mgr.HandleFrame(buildTestCTSFrame(3, 1, 126998, 42, 10)) + require.Eventually(t, func() bool { return len(h.getSentFrames()) == 4 }, time.Second, time.Millisecond) + + mgr.HandleFrame(buildTestEndOfMsgAckFrame(15, 3, 126998, 42, 10)) + select { + case err := <-errCh: + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid EndOfMsgAck") + case <-time.After(time.Second): + t.Fatal("SendRTSCTS did not reject mismatched EndOfMsgAck") + } +} + +func TestRTSCTSTransmit_EndAckRequiresEveryPacket(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{WriteFrame: h.writeFrame}) + defer mgr.Close() + + errCh := make(chan error, 1) + go func() { + errCh <- mgr.SendRTSCTS(126998, 10, 42, make([]byte, 16)) + }() + require.Eventually(t, func() bool { return len(h.getSentFrames()) == 1 }, time.Second, time.Millisecond) + + // A hostile peer requests only the last of three packets, then claims the + // transfer is complete. Seeing sequence 3 must not imply 1 and 2 were sent. + mgr.HandleFrame(buildTestCTSFrame(1, 3, 126998, 42, 10)) + require.Eventually(t, func() bool { return len(h.getSentFrames()) == 2 }, time.Second, time.Millisecond) + mgr.HandleFrame(buildTestEndOfMsgAckFrame(16, 3, 126998, 42, 10)) + + select { + case err := <-errCh: + require.Error(t, err) + assert.Contains(t, err.Error(), "packets sent=1") + case <-time.After(time.Second): + t.Fatal("SendRTSCTS accepted an acknowledgement with missing packets") + } +} + +func TestRTSCTSTransmit_RejectsAmbiguousConcurrentSession(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{WriteFrame: h.writeFrame}) + + first := make(chan error, 1) + go func() { first <- mgr.SendRTSCTS(126998, 10, 42, make([]byte, 16)) }() + require.Eventually(t, func() bool { return len(h.getSentFrames()) == 1 }, time.Second, time.Millisecond) + + err := mgr.SendRTSCTS(126996, 10, 42, make([]byte, 16)) + require.Error(t, err) + assert.Contains(t, err.Error(), "session already active") + assert.Len(t, h.getSentFrames(), 1, "a second RTS would make incoming CTS ownership ambiguous") + + mgr.Close() + require.Error(t, <-first) +} + // buildTestCTSFrame creates a CTS frame for testing. func buildTestCTSFrame(numFrames uint8, nextSeqNum uint8, pgn uint32, source uint8, destination uint8) can.Frame { var data [8]uint8 diff --git a/internal/transport/transport.go b/internal/transport/transport.go index 59f53f4..f7c1f2f 100644 --- a/internal/transport/transport.go +++ b/internal/transport/transport.go @@ -15,6 +15,11 @@ type ManagerConfig struct { // WriteFrame sends a CAN frame onto the bus. WriteFrame func(can.Frame) error + // LocalAddress returns the client's currently claimed address. When set, + // addressed TP traffic for other nodes is ignored. A function is used + // because address claiming may move the client at runtime. + LocalAddress func() uint8 + // OnComplete is called when a multi-frame message has been fully reassembled. // It receives the transported PGN, source address, destination address, and // the assembled payload. @@ -66,11 +71,11 @@ type session struct { timer Timer // Transmit-side fields for RTS/CTS - txPayload []byte // full payload to transmit - txOffset int // current offset into txPayload - txSeqNum uint8 // next DT sequence number to send (1-based) - txDone chan struct{} // closed when transmit completes - txErr error // set if transmit fails + txPayload []byte // full payload to transmit + txSent [256]bool // successfully sent DT sequence numbers + txSentCount int // number of unique DT packets sent + txDone chan struct{} // closed when transmit completes + txErr error // set if transmit fails } // Manager orchestrates ISO 11783 transport protocol sessions, handling both @@ -87,6 +92,11 @@ type Manager struct { // NewManager creates a new transport protocol Manager. func NewManager(cfg ManagerConfig) *Manager { + if cfg.WriteFrame == nil { + cfg.WriteFrame = func(can.Frame) error { + return fmt.Errorf("transport WriteFrame is nil") + } + } logger := cfg.Logger if logger == nil { logger = slog.Default() @@ -122,21 +132,36 @@ func (m *Manager) HandleFrame(frame can.Frame) { // handleCM dispatches a Connection Management frame based on the control byte. func (m *Manager) handleCM(frame can.Frame, source uint8, destination uint8) { - if frame.Length < 8 { + if frame.Length != 8 { return } controlByte := frame.Data[0] switch controlByte { case ControlBAM: + if destination != BroadcastAddr { + return + } m.handleBAMReceive(frame, source) case ControlRTS: + if !m.isLocalDestination(destination) { + return + } m.handleRTSReceive(frame, source, destination) case ControlCTS: + if !m.isLocalDestination(destination) { + return + } m.handleCTSReceive(frame, source, destination) case ControlEndOfMsgAck: + if !m.isLocalDestination(destination) { + return + } m.handleEndOfMsgAckReceive(frame, source, destination) case ControlAbort: + if !m.isLocalDestination(destination) { + return + } m.handleAbortReceive(frame, source, destination) default: m.logger.Warn("unknown TP.CM control byte", "controlByte", controlByte, "source", source) @@ -145,7 +170,10 @@ func (m *Manager) handleCM(frame can.Frame, source uint8, destination uint8) { // handleDT delivers a Data Transfer frame to the matching active session. func (m *Manager) handleDT(frame can.Frame, source uint8, destination uint8) { - if frame.Length < 8 { + if frame.Length != 8 { + return + } + if destination != BroadcastAddr && !m.isLocalDestination(destination) { return } @@ -172,6 +200,12 @@ func (m *Manager) handleDT(frame can.Frame, source uint8, destination uint8) { // Copy data bytes into the reassembly buffer. offset := int(seqNum-1) * MaxDTDataBytes remaining := int(sess.totalSize) - offset + if remaining <= 0 || offset < 0 || offset >= len(sess.data) { + m.logger.Warn("DT frame exceeds announced payload", "sequence", seqNum, "size", sess.totalSize) + m.removeSession(sess.key) + m.mu.Unlock() + return + } n := MaxDTDataBytes if remaining < n { n = remaining @@ -185,6 +219,26 @@ func (m *Manager) handleDT(frame can.Frame, source uint8, destination uint8) { } if sess.received < int(sess.numFrames) { + // Addressed transfers are flow-controlled in blocks. Once the block + // granted by the previous CTS has arrived, grant the next one. BAM has + // no CTS exchanges and continues directly to the timeout re-arm below. + var nextCTS *can.Frame + if sess.key.destination != BroadcastAddr && sess.received%int(sess.maxPerCTS) == 0 { + remainingFrames := int(sess.numFrames) - sess.received + requested := int(sess.maxPerCTS) + if remainingFrames < requested { + requested = remainingFrames + } + f := buildCTSFrame( + uint8(requested), + uint8(sess.received+1), + sess.key.pgn, + sess.key.destination, + sess.key.source, + ) + nextCTS = &f + } + // More frames expected; set a DT timeout. sess.timer = m.afterFunc(DTTimeout, func() { m.mu.Lock() @@ -193,7 +247,16 @@ func (m *Manager) handleDT(frame can.Frame, source uint8, destination uint8) { "received", sess.received, "expected", sess.numFrames) m.removeSession(sess.key) }) + key := sess.key m.mu.Unlock() + if nextCTS != nil { + if err := m.config.WriteFrame(*nextCTS); err != nil { + m.mu.Lock() + m.removeSession(key) + m.mu.Unlock() + m.logger.Warn("failed to send next CTS", "error", err, "pgn", key.pgn) + } + } return } @@ -230,7 +293,7 @@ func (m *Manager) handleDT(frame can.Frame, source uint8, destination uint8) { func (m *Manager) findSession(source uint8, destination uint8) *session { // Try each PGN — we iterate sessions since PGN is part of the key. for k, s := range m.sessions { - if k.source == source && k.destination == destination { + if k.source == source && k.destination == destination && s.state == stateReceivingDT { return s } } @@ -245,6 +308,58 @@ func (m *Manager) findSession(source uint8, destination uint8) *session { return nil } +func (m *Manager) isLocalDestination(destination uint8) bool { + return m.config.LocalAddress == nil || destination == m.config.LocalAddress() +} + +func validateAnnouncement(totalSize uint16, numFrames uint8) error { + if totalSize == 0 || numFrames == 0 { + return fmt.Errorf("zero size or frame count") + } + if totalSize > MaxPayloadBytes { + return fmt.Errorf("payload size %d exceeds maximum %d", totalSize, MaxPayloadBytes) + } + want := (int(totalSize) + MaxDTDataBytes - 1) / MaxDTDataBytes + if int(numFrames) != want { + return fmt.Errorf("frame count %d does not match size %d (want %d)", numFrames, totalSize, want) + } + return nil +} + +func validatePayload(payload []byte) error { + if len(payload) == 0 { + return fmt.Errorf("empty transport payload") + } + if len(payload) > MaxPayloadBytes { + return fmt.Errorf("payload size %d exceeds maximum %d", len(payload), MaxPayloadBytes) + } + return nil +} + +func validateTransportPGN(pgn uint32) error { + if pgn > MaxPGN { + return fmt.Errorf("PGN %d exceeds the 18-bit range", pgn) + } + if pgn&0x20000 != 0 { + return fmt.Errorf("PGN %d sets the reserved CAN-ID bit", pgn) + } + if ((pgn>>8)&0xFF) < 240 && pgn&0xFF != 0 { + return fmt.Errorf("PDU1 PGN %d must have a zero group-extension byte", pgn) + } + return nil +} + +// removeReceiveSessions removes the one active receive transfer for a source +// and destination. DT packets carry no PGN, so allowing multiple such +// sessions would make packet ownership ambiguous. +func (m *Manager) removeReceiveSessions(source, destination uint8) { + for key, sess := range m.sessions { + if key.source == source && key.destination == destination && sess.state == stateReceivingDT { + m.removeSession(key) + } + } +} + // removeSession stops timers and deletes a session from the map. func (m *Manager) removeSession(key sessionKey) { if s, ok := m.sessions[key]; ok { @@ -259,6 +374,19 @@ func (m *Manager) removeSession(key sessionKey) { // This blocks the caller for the duration of the transmission due to the required // inter-frame delays. func (m *Manager) SendBAM(pgn uint32, source uint8, payload []byte) error { + if err := validateTransportPGN(pgn); err != nil { + return fmt.Errorf("send BAM: %w", err) + } + if err := validatePayload(payload); err != nil { + return fmt.Errorf("send BAM: %w", err) + } + m.mu.Lock() + closed := m.closed + m.mu.Unlock() + if closed { + return fmt.Errorf("send BAM: manager closed") + } + payload = append([]byte(nil), payload...) totalSize := uint16(len(payload)) numFrames := uint8((len(payload) + MaxDTDataBytes - 1) / MaxDTDataBytes) @@ -274,6 +402,12 @@ func (m *Manager) SendBAM(pgn uint32, source uint8, payload []byte) error { if i > 0 { m.sleep(BAMInterFrameDelay) } + m.mu.Lock() + closed := m.closed + m.mu.Unlock() + if closed { + return fmt.Errorf("send BAM: manager closed") + } if err := m.config.WriteFrame(f); err != nil { return fmt.Errorf("send DT frame %d: %w", i+1, err) } @@ -285,6 +419,15 @@ func (m *Manager) SendBAM(pgn uint32, source uint8, payload []byte) error { // SendRTSCTS transmits a multi-frame message using RTS/CTS flow control. // This blocks until the transfer completes or a timeout/error occurs. func (m *Manager) SendRTSCTS(pgn uint32, source uint8, destination uint8, payload []byte) error { + if destination == BroadcastAddr { + return fmt.Errorf("send RTS/CTS: broadcast destination is invalid") + } + if err := validatePayload(payload); err != nil { + return fmt.Errorf("send RTS/CTS: %w", err) + } + if err := validateTransportPGN(pgn); err != nil { + return fmt.Errorf("send RTS/CTS: %w", err) + } totalSize := uint16(len(payload)) numFrames := uint8((len(payload) + MaxDTDataBytes - 1) / MaxDTDataBytes) @@ -295,9 +438,7 @@ func (m *Manager) SendRTSCTS(pgn uint32, source uint8, destination uint8, payloa state: stateWaitingForCTS, totalSize: totalSize, numFrames: numFrames, - txPayload: payload, - txOffset: 0, - txSeqNum: 1, + txPayload: append([]byte(nil), payload...), txDone: make(chan struct{}), } @@ -306,6 +447,12 @@ func (m *Manager) SendRTSCTS(pgn uint32, source uint8, destination uint8, payloa m.mu.Unlock() return fmt.Errorf("manager closed") } + for activeKey, active := range m.sessions { + if active.txDone != nil && activeKey.source == source && activeKey.destination == destination { + m.mu.Unlock() + return fmt.Errorf("transport session already active for source %d destination %d", source, destination) + } + } m.sessions[key] = sess // Set CTS timeout. @@ -355,10 +502,41 @@ func (m *Manager) handleCTSReceive(frame can.Frame, source uint8, destination ui m.mu.Lock() sess, ok := m.sessions[key] - if !ok { + if !ok || sess.state != stateWaitingForCTS { m.mu.Unlock() return } + if numFrames == 0 { + // A zero-packet CTS asks the transmitter to keep waiting. Re-arm the + // timeout so a peer can apply backpressure without corrupting state. + if sess.timer != nil { + sess.timer.Stop() + } + sess.timer = m.afterFunc(CTSTimeout, func() { + m.mu.Lock() + defer m.mu.Unlock() + if _, active := m.sessions[key]; active { + sess.txErr = fmt.Errorf("CTS timeout while receiver paused transfer") + m.removeSession(key) + close(sess.txDone) + } + }) + m.mu.Unlock() + return + } + lastSeqNum := int(nextSeqNum) + int(numFrames) - 1 + if nextSeqNum == 0 || int(nextSeqNum) > int(sess.numFrames) || lastSeqNum > int(sess.numFrames) { + sess.txErr = fmt.Errorf( + "invalid CTS range: first=%d count=%d total=%d", + nextSeqNum, + numFrames, + sess.numFrames, + ) + m.removeSession(key) + m.mu.Unlock() + close(sess.txDone) + return + } // Stop the CTS timeout timer. if sess.timer != nil { @@ -366,7 +544,6 @@ func (m *Manager) handleCTSReceive(frame can.Frame, source uint8, destination ui } sess.state = stateSendingDT - sess.txSeqNum = nextSeqNum payload := sess.txPayload // immutable after session creation; safe to read unlocked m.mu.Unlock() @@ -387,6 +564,13 @@ func (m *Manager) handleCTSReceive(frame can.Frame, source uint8, destination ui } return } + m.mu.Lock() + seq := dtFrame.Data[0] + if !sess.txSent[seq] { + sess.txSent[seq] = true + sess.txSentCount++ + } + m.mu.Unlock() } // After sending DT frames, set a CTS timeout for the next CTS or EndOfMsgAck. @@ -417,8 +601,24 @@ func (m *Manager) handleEndOfMsgAckReceive(frame can.Frame, source uint8, destin m.mu.Lock() sess, ok := m.sessions[key] - if !ok { + if !ok || sess.state != stateWaitingForCTS { + m.mu.Unlock() + return + } + totalSize := uint16(frame.Data[1]) | uint16(frame.Data[2])<<8 + numFrames := frame.Data[3] + if totalSize != sess.totalSize || numFrames != sess.numFrames || sess.txSentCount != int(sess.numFrames) { + sess.txErr = fmt.Errorf( + "invalid EndOfMsgAck: size=%d frames=%d packets sent=%d; want size=%d frames=%d", + totalSize, + numFrames, + sess.txSentCount, + sess.totalSize, + sess.numFrames, + ) + m.removeSession(key) m.mu.Unlock() + close(sess.txDone) return } diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 785e472..a2107b6 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -8,6 +8,7 @@ import ( "github.com/brutella/can" "github.com/open-ships/n2k/internal/framer" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // fakeTimer is a no-op Timer used by fakeClock. @@ -144,6 +145,80 @@ func TestManager_RoutesCMToCorrectHandler(t *testing.T) { assert.Equal(t, ControlCTS, sentFrames[0].Data[0], "response should be a CTS") } +func TestManager_IgnoresAddressedTrafficForAnotherNode(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{ + WriteFrame: h.writeFrame, + OnComplete: h.onComplete, + LocalAddress: func() uint8 { return 10 }, + }) + defer mgr.Close() + + rts := buildTestCMFrame(ControlRTS, 14, 2, 126998, 42, 11) + rts.Data[4] = 2 + mgr.HandleFrame(rts) + mgr.HandleFrame(buildTestDTFrame(1, [7]byte{}, 42, 11)) + + assert.Zero(t, activeSessionCount(mgr)) + assert.Empty(t, h.getSentFrames()) + assert.Empty(t, h.getCompleted()) +} + +func TestManager_RejectsMalformedAnnouncements(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{WriteFrame: h.writeFrame}) + defer mgr.Close() + + // Sixteen bytes require three packets, not two. + mgr.HandleFrame(buildTestCMFrame(ControlBAM, 16, 2, 126998, 42, BroadcastAddr)) + rts := buildTestCMFrame(ControlRTS, 16, 2, 126998, 42, 10) + rts.Data[4] = 2 + mgr.HandleFrame(rts) + + assert.Zero(t, activeSessionCount(mgr)) + assert.Empty(t, h.getSentFrames()) +} + +func TestManager_IgnoresNonClassicalTPFrames(t *testing.T) { + h := newTestHelper() + mgr := NewManager(ManagerConfig{WriteFrame: h.writeFrame}) + defer mgr.Close() + + cm := buildTestCMFrame(ControlBAM, 7, 1, 126998, 42, BroadcastAddr) + cm.Length = 9 + mgr.HandleFrame(cm) + dt := buildTestDTFrame(1, [7]byte{}, 42, BroadcastAddr) + dt.Length = 7 + mgr.HandleFrame(dt) + + assert.Zero(t, activeSessionCount(mgr)) + assert.Empty(t, h.getSentFrames()) +} + +func TestTransportSendRejectsOutOfRangePGN(t *testing.T) { + mgr := NewManager(ManagerConfig{WriteFrame: func(can.Frame) error { return nil }}) + defer mgr.Close() + + assert.Error(t, mgr.SendBAM(MaxPGN+1, 10, make([]byte, 9))) + assert.Error(t, mgr.SendRTSCTS(MaxPGN+1, 10, 42, make([]byte, 9))) + assert.Error(t, mgr.SendBAM(0x20000, 10, make([]byte, 9))) + assert.Error(t, mgr.SendRTSCTS(0xEA01, 10, 42, make([]byte, 9))) +} + +func TestManager_ReplacesAmbiguousReceiveSession(t *testing.T) { + mgr := NewManager(ManagerConfig{WriteFrame: func(can.Frame) error { return nil }}) + defer mgr.Close() + + mgr.HandleFrame(buildTestCMFrame(ControlBAM, 7, 1, 130816, 42, BroadcastAddr)) + mgr.HandleFrame(buildTestCMFrame(ControlBAM, 7, 1, 130817, 42, BroadcastAddr)) + + mgr.mu.Lock() + defer mgr.mu.Unlock() + require.Len(t, mgr.sessions, 1) + _, ok := mgr.sessions[sessionKey{source: 42, destination: BroadcastAddr, pgn: 130817}] + assert.True(t, ok) +} + func TestManager_IgnoresNonTPFrames(t *testing.T) { h := newTestHelper() mgr := NewManager(ManagerConfig{ diff --git a/justfile b/justfile index c3d1486..2ee4468 100644 --- a/justfile +++ b/justfile @@ -1,5 +1,7 @@ golangci_lint_version := "v2.12.0" secure_go_toolchain := "go1.25.12" +govulncheck_version := "v1.5.0" +gosec_version := "v2.27.1" # list available recipes default: @@ -13,23 +15,28 @@ setup: echo "Installing golangci-lint {{golangci_lint_version}}..."; \ curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b "$(go env GOPATH)/bin" {{golangci_lint_version}}; \ fi - @if command -v govulncheck >/dev/null; then \ - echo "govulncheck already installed"; \ + @if command -v govulncheck >/dev/null && govulncheck -version 2>&1 | grep -q "govulncheck@{{govulncheck_version}}"; then \ + echo "govulncheck {{govulncheck_version}} already installed"; \ else \ - echo "Installing govulncheck..."; \ - go install golang.org/x/vuln/cmd/govulncheck@latest; \ + echo "Installing govulncheck {{govulncheck_version}}..."; \ + go install golang.org/x/vuln/cmd/govulncheck@{{govulncheck_version}}; \ fi - @if command -v gosec >/dev/null; then \ - echo "gosec already installed"; \ + @if command -v gosec >/dev/null && go version -m "$(command -v gosec)" 2>&1 | grep -q "github.com/securego/gosec/v2[[:space:]]*{{gosec_version}}"; then \ + echo "gosec {{gosec_version}} already installed"; \ else \ - echo "Installing gosec..."; \ - go install github.com/securego/gosec/v2/cmd/gosec@latest; \ + echo "Installing gosec {{gosec_version}}..."; \ + go install github.com/securego/gosec/v2/cmd/gosec@{{gosec_version}}; \ fi # run all tests test: go test ./... +# run the public, reproducible protocol evidence suite (not NMEA certification) +conformance-local: + go test -count=1 -v . -run '^TestConformance' + go test -count=1 ./internal/adapter ./internal/claiming ./internal/framer ./internal/transport + # compare runtime PGN support against the current upstream schema upstream-parity: UPSTREAM_PARITY=1 go test ./pgn -run TestUpstreamParity -count=1 -v @@ -46,6 +53,13 @@ test-v: test-race: go test -race ./... +# short, deterministic smoke run of every fuzz harness +fuzz-smoke: + go test ./internal/adapter -run=^$ -fuzz=FuzzCANAdapter -fuzztime=2s + go test ./internal/canbus -run=^$ -fuzz=FuzzUSBCANParseFrames -fuzztime=2s + go test ./internal/gateway -run=^$ -fuzz=FuzzActisenseReader -fuzztime=2s + go test ./pgn -run=^$ -fuzz=FuzzDecodeEncodeMessage -fuzztime=2s + # run tests with coverage test-cover: go test -coverpkg=./... -coverprofile=c.out ./... @@ -79,7 +93,7 @@ lint: # run security review (vulnerability scan + static analysis) secure: GOTOOLCHAIN={{secure_go_toolchain}} govulncheck ./... - GOTOOLCHAIN={{secure_go_toolchain}} gosec -exclude-generated -exclude=G115 ./... + GOTOOLCHAIN={{secure_go_toolchain}} gosec -exclude-dir=.claude -exclude-generated -exclude=G115 ./... # tidy go modules tidy: diff --git a/messagehub.go b/messagehub.go new file mode 100644 index 0000000..5fd0e21 --- /dev/null +++ b/messagehub.go @@ -0,0 +1,140 @@ +package n2k + +import ( + "errors" + "sync" + + "github.com/open-ships/n2k/pgn" +) + +// ErrReceiveOverflow reports that a live Client subscriber could not keep up +// with bus traffic. The subscriber is closed rather than allowing application +// backpressure to stall address claiming and other protocol processing. +var ErrReceiveOverflow = errors.New("n2k: receive buffer overflow") + +const defaultReceiveBuffer = 64 + +// messageHub fans live messages out to independent subscriptions. Publishing +// never blocks: a slow subscription is failed explicitly while other +// subscribers and the bus protocol engine continue making progress. +type messageHub struct { + mu sync.Mutex + buffer int + subscribers map[*messageSubscription]struct{} + backlog []pgn.Message + missed bool + closed bool + terminalErr error +} + +type messageSubscription struct { + hub *messageHub + ch chan pgn.Message + once sync.Once + + mu sync.Mutex + err error +} + +func newMessageHub(buffer int) *messageHub { + if buffer <= 0 { + buffer = defaultReceiveBuffer + } + return &messageHub{ + buffer: buffer, + subscribers: make(map[*messageSubscription]struct{}), + } +} + +func (h *messageHub) publish(msg pgn.Message) { + if msg == nil { + return + } + h.mu.Lock() + defer h.mu.Unlock() + if h.closed { + return + } + if len(h.subscribers) == 0 { + if len(h.backlog) == h.buffer { + copy(h.backlog, h.backlog[1:]) + h.backlog[len(h.backlog)-1] = msg + h.missed = true + return + } + h.backlog = append(h.backlog, msg) + return + } + for sub := range h.subscribers { + select { + case sub.ch <- msg: + default: + sub.failLocked(ErrReceiveOverflow) + delete(h.subscribers, sub) + } + } +} + +func (h *messageHub) subscribe() *messageSubscription { + h.mu.Lock() + defer h.mu.Unlock() + sub := &messageSubscription{hub: h, ch: make(chan pgn.Message, h.buffer)} + for _, msg := range h.backlog { + sub.ch <- msg + } + h.backlog = nil + if h.missed { + h.missed = false + sub.failLocked(ErrReceiveOverflow) + return sub + } + if h.closed { + sub.failLocked(h.terminalErr) + return sub + } + h.subscribers[sub] = struct{}{} + return sub +} + +func (h *messageHub) close(err error) { + h.mu.Lock() + defer h.mu.Unlock() + if h.closed { + return + } + h.closed = true + h.terminalErr = err + for sub := range h.subscribers { + sub.failLocked(err) + delete(h.subscribers, sub) + } +} + +func (h *messageHub) subscriberCount() int { + h.mu.Lock() + defer h.mu.Unlock() + return len(h.subscribers) +} + +func (s *messageSubscription) failLocked(err error) { + s.mu.Lock() + s.err = err + s.mu.Unlock() + s.once.Do(func() { close(s.ch) }) +} + +func (s *messageSubscription) unsubscribe() { + if s == nil || s.hub == nil { + return + } + s.hub.mu.Lock() + delete(s.hub.subscribers, s) + s.once.Do(func() { close(s.ch) }) + s.hub.mu.Unlock() +} + +func (s *messageSubscription) terminalError() error { + s.mu.Lock() + defer s.mu.Unlock() + return s.err +} diff --git a/messagehub_test.go b/messagehub_test.go new file mode 100644 index 0000000..a341ae6 --- /dev/null +++ b/messagehub_test.go @@ -0,0 +1,61 @@ +package n2k + +import ( + "errors" + "testing" + + "github.com/open-ships/n2k/pgn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMessageHubSubscriptionsAreIndependent(t *testing.T) { + hub := newMessageHub(1) + fast := hub.subscribe() + slow := hub.subscribe() + t.Cleanup(fast.unsubscribe) + t.Cleanup(slow.unsubscribe) + + first := &pgn.VesselHeading{} + second := &pgn.RateOfTurn{} + hub.publish(first) + require.Same(t, first, <-fast.ch) + + hub.publish(second) + require.Same(t, second, <-fast.ch) + require.Same(t, first, <-slow.ch) + _, open := <-slow.ch + assert.False(t, open) + assert.ErrorIs(t, slow.terminalError(), ErrReceiveOverflow) + assert.NoError(t, fast.terminalError()) +} + +func TestMessageHubReportsBacklogLoss(t *testing.T) { + hub := newMessageHub(1) + hub.publish(&pgn.VesselHeading{}) + last := &pgn.RateOfTurn{} + hub.publish(last) + + sub := hub.subscribe() + t.Cleanup(sub.unsubscribe) + require.Same(t, last, <-sub.ch) + _, open := <-sub.ch + assert.False(t, open) + assert.ErrorIs(t, sub.terminalError(), ErrReceiveOverflow) +} + +func TestMessageHubPropagatesTerminalError(t *testing.T) { + hub := newMessageHub(1) + sub := hub.subscribe() + want := errors.New("bus failed") + hub.close(want) + + _, open := <-sub.ch + assert.False(t, open) + assert.ErrorIs(t, sub.terminalError(), want) + + late := hub.subscribe() + _, open = <-late.ch + assert.False(t, open) + assert.ErrorIs(t, late.terminalError(), want) +} diff --git a/n2k.go b/n2k.go index f1f7eac..128773e 100644 --- a/n2k.go +++ b/n2k.go @@ -17,6 +17,7 @@ import ( func Receive(ctx context.Context, opts ...Option) iter.Seq2[pgn.Message, error] { return func(yield func(pgn.Message, error) bool) { s := NewScanner(ctx, opts...) + defer func() { _ = s.Close() }() for s.Next() { if !yield(s.Message(), nil) { return diff --git a/options.go b/options.go index 35bd206..26481dd 100644 --- a/options.go +++ b/options.go @@ -2,6 +2,7 @@ package n2k import ( "errors" + "fmt" "log/slog" "time" @@ -14,9 +15,12 @@ type config struct { includeUnknown bool logger *slog.Logger sourceAddress *uint8 // nil = auto mode + preferredAddress *uint8 // nil = default auto-mode starting address deviceName *DeviceName // nil = use default claimTimeout *time.Duration // nil = use default (1500ms) heartbeatInterval *time.Duration // nil = default (60s), 0 = disabled + receiveBuffer *int // nil = default (64) + writeQueue *int // nil = default (64) productInfo *ProductInfo // nil = defaults configInfo *ConfigInfo // nil = defaults bus Bus // pre-constructed bus @@ -27,6 +31,51 @@ func (c *config) validate() error { if len(c.sources) == 0 && c.bus == nil { return errors.New("n2k: at least one source or WithBus is required") } + if c.sourceAddress != nil && *c.sourceAddress > 251 { + return fmt.Errorf("n2k: source address %d is outside 0-251", *c.sourceAddress) + } + if c.preferredAddress != nil && *c.preferredAddress > 251 { + return fmt.Errorf("n2k: preferred address %d is outside 0-251", *c.preferredAddress) + } + if c.sourceAddress != nil && c.preferredAddress != nil { + return errors.New("n2k: WithSourceAddress and WithPreferredAddress are mutually exclusive") + } + if c.claimTimeout != nil && *c.claimTimeout <= 0 { + return errors.New("n2k: claim timeout must be positive") + } + if c.heartbeatInterval != nil && *c.heartbeatInterval < 0 { + return errors.New("n2k: heartbeat interval cannot be negative") + } + if c.receiveBuffer != nil && *c.receiveBuffer <= 0 { + return errors.New("n2k: receive buffer must be positive") + } + if c.writeQueue != nil && *c.writeQueue <= 0 { + return errors.New("n2k: write queue must be positive") + } + for _, src := range c.sources { + switch s := src.(type) { + case *socketCANSource: + if s.iface == "" { + return errors.New("n2k: CAN interface name cannot be empty") + } + case *usbCANSource: + if s.port == "" { + return errors.New("n2k: USB serial port cannot be empty") + } + case *tcpSource: + if s.addr == "" || !s.format.valid() { + return fmt.Errorf("n2k: invalid TCP source address or stream format %d", s.format) + } + case *udpSource: + if s.addr == "" || !s.format.valid() { + return fmt.Errorf("n2k: invalid UDP source address or stream format %d", s.format) + } + case *fileSource: + if s.path == "" { + return errors.New("n2k: file path cannot be empty") + } + } + } return nil } @@ -61,7 +110,9 @@ func File(path string, opts ...FileOption) Option { return optionFunc(func(c *config) { src := &fileSource{path: path} for _, o := range opts { - o.applyFile(src) + if o != nil { + o.applyFile(src) + } } c.sources = append(c.sources, src) }) @@ -90,7 +141,8 @@ func UDP(listenAddr string, format StreamFormat) Option { // Replay adds a source that replays the given CAN frames. Useful for testing. func Replay(frames []can.Frame) Option { return optionFunc(func(c *config) { - c.sources = append(c.sources, &replaySource{frames: frames}) + copied := append([]can.Frame(nil), frames...) + c.sources = append(c.sources, &replaySource{frames: copied}) }) } @@ -119,7 +171,7 @@ func WithLogger(l *slog.Logger) Option { // WithSourceAddress sets an explicit NMEA 2000 source address for the client. // When set, the client uses this address and treats contention as a fatal error. -// When not set (default), the client uses auto mode — starting at address 253 +// When not set (default), the client uses auto mode — starting at address 251 // and working downward if contention occurs. func WithSourceAddress(addr uint8) Option { return optionFunc(func(c *config) { @@ -127,6 +179,17 @@ func WithSourceAddress(addr uint8) Option { }) } +// WithPreferredAddress sets the starting address for automatic address +// claiming while retaining arbitrary-address capability. Persist the last +// Client.Status().Address and pass it here on the next start to reclaim the +// device's prior address when available. Valid addresses are 0 through 251. +// Unlike WithSourceAddress, contention moves the client to another address. +func WithPreferredAddress(addr uint8) Option { + return optionFunc(func(c *config) { + c.preferredAddress = &addr + }) +} + // WithClaimTimeout sets how long NewClient blocks waiting for address claiming // to complete on a real CAN bus. Default is 1500ms. This allows time for the // initial 250ms claim window plus several rounds of contention renegotiation. @@ -138,8 +201,8 @@ func WithClaimTimeout(d time.Duration) Option { // WithProductInfo sets the product identity (PGN 126996) this client reports // when another device requests it. Without it, a generic software-gateway -// identity is reported. String fields longer than 32 bytes are truncated on -// the wire. +// identity is reported. String fields longer than 32 bytes are rejected when +// the client is created. func WithProductInfo(p ProductInfo) Option { return optionFunc(func(c *config) { c.productInfo = &p @@ -164,6 +227,25 @@ func WithHeartbeatInterval(d time.Duration) Option { }) } +// WithReceiveBuffer sets the number of decoded messages retained per live +// Client subscription. The default is 64. A subscriber that falls behind +// this bound is closed with ErrReceiveOverflow so it cannot stall protocol +// processing or other subscribers. +func WithReceiveBuffer(size int) Option { + return optionFunc(func(c *config) { + c.receiveBuffer = &size + }) +} + +// WithWriteQueue sets the number of asynchronous writes that can wait behind +// the active write. The default is 64. Once full, Write completes immediately +// with ErrWriteQueueFull rather than blocking an application goroutine. +func WithWriteQueue(size int) Option { + return optionFunc(func(c *config) { + c.writeQueue = &size + }) +} + // WithName sets the ISO 11783 device NAME used for address claiming. // The NAME is a 64-bit identifier that uniquely identifies this device on the // NMEA 2000 network. In address contention, the device with the lower NAME wins. diff --git a/options_test.go b/options_test.go index b6296da..00a4381 100644 --- a/options_test.go +++ b/options_test.go @@ -60,3 +60,46 @@ func TestNewClient_InvalidDeviceNameRejected(t *testing.T) { t.Fatal("expected invalid DeviceName error") } } + +func TestConfigValidationRejectsInvalidOperationalValues(t *testing.T) { + for _, opt := range []Option{ + WithSourceAddress(252), + WithSourceAddress(254), + WithPreferredAddress(252), + WithClaimTimeout(0), + WithHeartbeatInterval(-time.Second), + WithReceiveBuffer(0), + WithWriteQueue(0), + } { + _, err := NewClient(context.Background(), Replay(nil), opt) + assert.Error(t, err) + } +} + +func TestAddressOptions(t *testing.T) { + for _, opt := range []Option{WithSourceAddress(0), WithSourceAddress(251), WithPreferredAddress(0), WithPreferredAddress(251)} { + c, err := NewClient(context.Background(), Replay(nil), opt) + require.NoError(t, err) + require.NoError(t, c.Close()) + } + + _, err := NewClient(context.Background(), Replay(nil), WithSourceAddress(10), WithPreferredAddress(10)) + require.Error(t, err) +} + +func TestBufferOptions(t *testing.T) { + c, err := NewClient(context.Background(), Replay(nil), WithReceiveBuffer(3), WithWriteQueue(5)) + require.NoError(t, err) + defer func() { _ = c.Close() }() + status := c.Status() + assert.Equal(t, 5, status.WriteQueueCapacity) + assert.True(t, status.AddressClaimed) +} + +func TestNewClientRejectsAmbiguousSources(t *testing.T) { + _, err := NewClient(context.Background(), Replay(nil), Replay(nil)) + assert.NoError(t, err, "multiple replay sources are valid") + + _, err = NewClient(context.Background(), CAN("can0"), Replay(nil)) + assert.Error(t, err) +} diff --git a/pgn/codec.go b/pgn/codec.go index 4844613..722cc34 100644 --- a/pgn/codec.go +++ b/pgn/codec.go @@ -1,6 +1,8 @@ package pgn import ( + "bytes" + "errors" "fmt" "reflect" "sort" @@ -132,7 +134,7 @@ var codecPlanCache sync.Map // reflect.Type -> *codecPlan // dispatch can try the next candidate), reads numeric fields with null-sentinel // detection, expands repeating field sets into slice fields, and resolves // variable-length binary fields via their length-field references. -func decodeFields(m PGN, payload []uint8) error { +func decodeFields(m PGN, payload []uint8) (err error) { // Stash the wire payload so a later encodeFields call on this same // struct can reproduce it byte-for-bit in RESERVED/STRING_FIX // positions instead of always applying the default fill. This must @@ -140,8 +142,21 @@ func decodeFields(m PGN, payload []uint8) error { // this decode's info fields (PGN, source, etc.), which dispatch's // decodePGNCandidates already set on the candidate before calling in. info := m.MessageInfo() - info.rawPayload = payload + info.rawPayload = append([]uint8(nil), payload...) + info.rawCanonical = nil m.SetMessageInfo(info) + defer func() { + if err != nil { + return + } + canonical, encodeErr := encodeCurrentFields(m) + if encodeErr != nil { + return + } + decodedInfo := m.MessageInfo() + decodedInfo.rawCanonical = append([]uint8(nil), canonical...) + m.SetMessageInfo(decodedInfo) + }() plan, err := codecPlanForMessage(m) if err != nil { @@ -193,19 +208,22 @@ func decodeFields(m PGN, payload []uint8) error { // lengths, and deriving variable-length binary length fields from the data // slice length. // -// When m was produced by decodeFields, its stashed wire payload is returned -// verbatim, unconditionally -- this is the only supported way to encode a -// decoded message, and it reproduces bytes the schema has no field for at -// all (e.g. trailing filler some devices pad single-frame messages with) -// that no amount of per-field reconstruction could cover. Any field changes -// made after decode have no effect on the encoded output. A message built -// from scratch (never decoded) has no stashed payload and always goes -// through the normal per-field encode below. +// When m was produced by decodeFields, untouched fields return the original +// wire payload exactly, including reserved bits and trailing filler. If any +// decoded field changes, the current field values are encoded instead. func encodeFields(m PGN) ([]uint8, error) { - if raw := m.MessageInfo().rawPayload; len(raw) > 0 { - return append([]uint8(nil), raw...), nil + info := m.MessageInfo() + current, err := encodeCurrentFields(m) + if err != nil { + return nil, err + } + if len(info.rawPayload) > 0 && len(info.rawCanonical) > 0 && bytes.Equal(current, info.rawCanonical) { + return append([]uint8(nil), info.rawPayload...), nil } + return current, nil +} +func encodeCurrentFields(m PGN) ([]uint8, error) { plan, err := codecPlanForMessage(m) if err != nil { return nil, err @@ -579,8 +597,11 @@ func decodeGroup(stream *PGNDataStream, group *planGroup, target reflect.Value, complete := true for f := range group.fields { if err := decodeField(stream, &group.fields[f], element, raws); err != nil { - // The payload ended (or turned unreadable) inside this - // element: discard the partial element and stop cleanly. + if !errors.Is(err, ErrUnexpectedPayloadEnd) { + return false, fmt.Errorf("decode repeating group field %d: %w", group.fields[f].order, err) + } + // Truncated payloads may legitimately end inside a final + // repeating element; discard only that partial element. complete = false break } diff --git a/pgn/codec_fuzz_test.go b/pgn/codec_fuzz_test.go new file mode 100644 index 0000000..e6afe4a --- /dev/null +++ b/pgn/codec_fuzz_test.go @@ -0,0 +1,24 @@ +package pgn + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func FuzzDecodeEncodeMessage(f *testing.F) { + f.Add(uint32(127250), []byte{0, 0, 0, 0xFF, 0x7F, 0xFF, 0x7F, 0xFC}) + f.Add(uint32(126998), []byte{2, 1, 'a', 2, 1, 'b', 2, 1, 'c'}) + known := []uint32{59904, 60928, 126208, 126996, 126998, 127250, 129029, 130306} + f.Fuzz(func(t *testing.T, selector uint32, payload []byte) { + pgnNum := known[int(selector%uint32(len(known)))] + var decoded PGN + require.NotPanics(t, func() { + var err error + decoded, err = DecodeMessage(MessageInfo{PGN: pgnNum}, payload) + if err == nil { + _, _ = EncodeMessage(decoded) + } + }) + }) +} diff --git a/pgn/messageinfo.go b/pgn/messageinfo.go index 6adf04a..c4ebe87 100644 --- a/pgn/messageinfo.go +++ b/pgn/messageinfo.go @@ -7,7 +7,7 @@ import ( // MessageInfo carries the CAN bus header metadata that accompanies every NMEA 2000 message. // It is extracted from the CAN frame's 29-bit identifier and timestamp before the payload // is passed to a PGN decoder. Every PGN struct embeds a MessageInfo as its "info" -// field, with an exported Info() accessor. +// field, with an exported MessageInfo() accessor. type MessageInfo struct { Timestamp time.Time `json:"timestamp"` Priority *uint8 `json:"priority"` @@ -15,17 +15,11 @@ type MessageInfo struct { SourceId uint8 `json:"sourceId"` TargetId *uint8 `json:"targetId"` - // rawPayload is the exact wire payload the owning message was decoded - // from, stashed by decodeFields and returned verbatim by encodeFields -- - // a decoded message always re-encodes to the bytes it came from, - // regardless of any field changes made in between. Unexported: this is - // codec bookkeeping, not part of MessageInfo's public contract, and is - // cleared whenever a caller replaces Info wholesale (e.g. via - // SetMessageInfo with a fresh value), which conveniently also - // invalidates it whenever a caller is about to repurpose the message - // for something else (the only supported way back to the normal, - // build-from-scratch encode path for a message that was once decoded). - rawPayload []uint8 + // rawPayload and rawCanonical are codec bookkeeping. An untouched decoded + // message returns rawPayload exactly; once fields differ from rawCanonical, + // encoding uses the current field values. Replacing MessageInfo clears both. + rawPayload []uint8 + rawCanonical []uint8 } // Priority returns a pointer to v, for use in MessageInfo literal construction: diff --git a/pgn/pgndatastream.go b/pgn/pgndatastream.go index e9bf4d0..ee0e138 100644 --- a/pgn/pgndatastream.go +++ b/pgn/pgndatastream.go @@ -1,11 +1,16 @@ package pgn import ( + "errors" "fmt" "math" "strings" ) +// ErrUnexpectedPayloadEnd identifies a field read that extends beyond the +// available PGN payload. +var ErrUnexpectedPayloadEnd = errors.New("unexpected end of PGN payload") + // PGNDataStream provides a bit-level sequential reader over raw NMEA 2000 message data. // It is the core decoding primitive: generated PGN decoder functions create a PGNDataStream // from a packet's payload bytes and then call its typed read methods to extract each field @@ -364,6 +369,9 @@ func (s *PGNDataStream) readStringWithLengthAndControl() (string, error) { if err != nil { return "", err } + if lc[0] < 2 { + return "", fmt.Errorf("invalid STRING_LAU length %d", lc[0]) + } // Subtract 2 from the length to exclude the length and control bytes themselves, // then convert to bits for readBinaryData. dataLen := (uint16(lc[0]) - 2) * 8 @@ -409,9 +417,8 @@ func (s *PGNDataStream) readStringWithLength() (string, error) { // - 0xFF: "data not available" fill byte // - '@': legacy padding character used by some older devices // -// The string is truncated at the first occurrence of any of these pad characters. -// This means a string cannot legitimately contain '@' or 0xFF as content -- an -// acceptable limitation given typical marine device naming conventions. +// NUL and 0xFF are terminators. '@' is removed only when trailing, because it +// is also a legitimate character inside device names and descriptions. func (s *PGNDataStream) readFixedString(bitLength uint16) (string, error) { arr, err := s.readBinaryData(bitLength) if err != nil { @@ -419,7 +426,8 @@ func (s *PGNDataStream) readFixedString(bitLength uint16) (string, error) { } str := string(arr) - // Truncate at the first padding character found (NUL, 0xFF, or '@'). + // Truncate at the first byte-valued terminator, then remove legacy trailing + // '@' padding without damaging an embedded '@'. i := strings.IndexByte(str, 0) if i != -1 { str = str[:i] @@ -428,12 +436,7 @@ func (s *PGNDataStream) readFixedString(bitLength uint16) (string, error) { if i != -1 { str = str[:i] } - i = strings.IndexRune(str, '@') - if i != -1 { - str = str[:i] - } - - return str, nil + return strings.TrimRight(str, "@"), nil } // getNumberRaw is the lowest-level read primitive. It extracts up to 64 bits from the @@ -458,7 +461,7 @@ func (s *PGNDataStream) getNumberRaw(bitLength uint16) (uint64, error) { for bitLength > 0 { if int(s.byteOffset) >= len(s.data) { - return 0, fmt.Errorf("reading byte(%d) off end of pgn (len:%d)", s.byteOffset, len(s.data)) + return 0, fmt.Errorf("%w: reading byte %d from %d-byte payload", ErrUnexpectedPayloadEnd, s.byteOffset, len(s.data)) } // How many bits remain in the current source byte from the current bit position. diff --git a/pgn/pgndatastream_additional_test.go b/pgn/pgndatastream_additional_test.go index 448f3a0..0a5ea41 100644 --- a/pgn/pgndatastream_additional_test.go +++ b/pgn/pgndatastream_additional_test.go @@ -47,6 +47,13 @@ func TestReadFixedString_PaddedWithAt(t *testing.T) { assert.Equal(t, "Hi", str) } +func TestReadFixedString_PreservesEmbeddedAt(t *testing.T) { + s := NewPgnDataStream([]byte("A@B@@")) + str, err := s.readFixedString(40) + assert.NoError(t, err) + assert.Equal(t, "A@B", str) +} + // TestReadFixedString_Empty verifies that a string consisting entirely of NUL padding // returns an empty string (the padding is the first byte, so everything is stripped). func TestReadFixedString_Empty(t *testing.T) { @@ -79,6 +86,14 @@ func TestReadStringWithLength_NullLength(t *testing.T) { assert.Contains(t, err.Error(), "null length") } +func TestReadStringWithLengthAndControl_RejectsShortLength(t *testing.T) { + for _, length := range []byte{0, 1} { + s := NewPgnDataStream([]uint8{length, 1}) + _, err := s.readStringWithLengthAndControl() + assert.Error(t, err) + } +} + // TestReadSignedResolution_Positive verifies that a positive signed integer is correctly // read and scaled. Value 100 with resolution 0.1 should produce 10.0. func TestReadSignedResolution_Positive(t *testing.T) { diff --git a/pgn/pgndatastream_test.go b/pgn/pgndatastream_test.go index 6d67f03..7b43a36 100644 --- a/pgn/pgndatastream_test.go +++ b/pgn/pgndatastream_test.go @@ -146,5 +146,3 @@ func TestNumerics(t *testing.T) { assert.Equal(t, tst.exp, v) } } - -// TODO: Tests for strings once we get more confidence diff --git a/pgn/pgndatastream_writer.go b/pgn/pgndatastream_writer.go index 4abccec..ede3c80 100644 --- a/pgn/pgndatastream_writer.go +++ b/pgn/pgndatastream_writer.go @@ -333,6 +333,12 @@ func (w *PGNDataStreamWriter) writeFixedString(s string, bitLength uint16) { func (w *PGNDataStreamWriter) writeStringWithLength(s string) { sBytes := []uint8(s) + // 0xFF is the protocol's null-length sentinel, so 254 is the largest + // representable non-null string. + if len(sBytes) > 254 { + w.setErr(fmt.Errorf("STRING_LZ is %d bytes; maximum is 254", len(sBytes))) + return + } length := uint8(len(sBytes)) w.setErr(w.putNumberRaw(uint64(length), 8)) w.writeBinaryData(sBytes, uint16(length)*8) @@ -340,6 +346,10 @@ func (w *PGNDataStreamWriter) writeStringWithLength(s string) { func (w *PGNDataStreamWriter) writeStringWithLengthAndControl(s string) { sBytes := []uint8(s) + if len(sBytes) > 253 { + w.setErr(fmt.Errorf("STRING_LAU is %d bytes; maximum is 253", len(sBytes))) + return + } // totalLength counts itself, the control byte, and the string bytes -- there is no // trailing NUL, matching readStringWithLengthAndControl's inverse expectations. totalLength := uint8(len(sBytes) + 2) diff --git a/pgn/pgndatastream_writer_test.go b/pgn/pgndatastream_writer_test.go index a35bf0f..f3179ac 100644 --- a/pgn/pgndatastream_writer_test.go +++ b/pgn/pgndatastream_writer_test.go @@ -351,6 +351,16 @@ func TestRoundTrip_StringWithLengthAndControl(t *testing.T) { assert.Equal(t, "AB", v) } +func TestLengthPrefixedStringsRejectOverflow(t *testing.T) { + w := NewPGNDataStreamWriter() + w.writeStringWithLength(string(make([]byte, 255))) + assert.Error(t, w.Err()) + + w = NewPGNDataStreamWriter() + w.writeStringWithLengthAndControl(string(make([]byte, 254))) + assert.Error(t, w.Err()) +} + // TestRoundTrip_UInt32 verifies uint32 write and read round-trip. func TestRoundTrip_UInt32(t *testing.T) { w := NewPGNDataStreamWriter() diff --git a/pgn/pgninfo.go b/pgn/pgninfo.go index a75bd90..0886fe7 100644 --- a/pgn/pgninfo.go +++ b/pgn/pgninfo.go @@ -182,21 +182,23 @@ func IsProprietaryPGN(pgn uint32) bool { // If called on a non-proprietary PGN, the returned values will be meaningless since // those bytes have a different field layout. func GetProprietaryInfo(data []uint8) (ManufacturerCodeConst, IndustryCodeConst, error) { + if len(data) < 2 { + return 0, 0, fmt.Errorf("proprietary payload header requires 2 bytes, got %d", len(data)) + } stream := NewPgnDataStream(data) - var man ManufacturerCodeConst - var ind IndustryCodeConst - var err error // Read the 11-bit manufacturer code from bits 0-10. - if v, err := stream.readLookupField(11); err == nil { - man = ManufacturerCodeConst(v) + manufacturer, err := stream.readLookupField(11) + if err != nil { + return 0, 0, fmt.Errorf("read manufacturer code: %w", err) } // Skip the 2 reserved bits (bits 11-12). stream.skipBits(2) // Read the 3-bit industry code from bits 13-15. - if v, err := stream.readLookupField(3); err == nil { - ind = IndustryCodeConst(v) + industry, err := stream.readLookupField(3) + if err != nil { + return 0, 0, fmt.Errorf("read industry code: %w", err) } - return man, ind, err + return ManufacturerCodeConst(manufacturer), IndustryCodeConst(industry), nil } // GetFieldDescriptor looks up the FieldDescriptor for a specific field within a PGN. diff --git a/pgn/pgninfo_additional_test.go b/pgn/pgninfo_additional_test.go index d1fc994..f60d878 100644 --- a/pgn/pgninfo_additional_test.go +++ b/pgn/pgninfo_additional_test.go @@ -99,6 +99,13 @@ func TestGetProprietaryInfo_DifferentManufacturer(t *testing.T) { assert.Equal(t, IndustryCodeConst(4), ind) } +func TestGetProprietaryInfo_RejectsShortHeader(t *testing.T) { + for _, payload := range [][]uint8{nil, {0x01}} { + _, _, err := GetProprietaryInfo(payload) + assert.Error(t, err) + } +} + // TestGetFieldDescriptor_KnownPGN verifies that field descriptors can be retrieved for // a well-known PGN (127250 Vessel Heading). Checks both field 1 (SID, 8-bit) and // field 2 (Heading) to ensure the field map is correctly populated by the generated code. diff --git a/pgn/preserve_raw_test.go b/pgn/preserve_raw_test.go index d7a0ba2..6c88c5d 100644 --- a/pgn/preserve_raw_test.go +++ b/pgn/preserve_raw_test.go @@ -29,12 +29,9 @@ func TestEncodeFieldsPassthroughForUndeclaredTrailingBytes(t *testing.T) { } } -// TestEncodeFieldsIgnoresFieldMutationAfterDecode proves the two-use-case -// contract: a decoded message always re-encodes to the bytes it came from, -// full stop. Mutating a field afterward does not change the encoded output -// -- decode-then-modify-then-encode is not a supported path, so the mutation -// is simply inert rather than partially honored. -func TestEncodeFieldsIgnoresFieldMutationAfterDecode(t *testing.T) { +// TestEncodeFieldsHonorsFieldMutationAfterDecode proves decoded messages stay +// byte-exact while untouched but switch to field encoding after an edit. +func TestEncodeFieldsHonorsFieldMutationAfterDecode(t *testing.T) { msg := &IsoRequest{} if err := msg.DecodePayload(isoRequestPayload); err != nil { t.Fatalf("decode: %v", err) @@ -46,8 +43,25 @@ func TestEncodeFieldsIgnoresFieldMutationAfterDecode(t *testing.T) { if err != nil { t.Fatalf("encode: %v", err) } + want := []uint8{0x14, 0xF0, 0x01} + if !bytesEqual(encoded, want) { + t.Fatalf("expected mutation to be encoded:\n want: % x\n encoded: % x", want, encoded) + } +} + +func TestDecodePayloadOwnsWireBuffer(t *testing.T) { + payload := append([]uint8(nil), isoRequestPayload...) + msg := &IsoRequest{} + if err := msg.DecodePayload(payload); err != nil { + t.Fatalf("decode: %v", err) + } + payload[0] = 0 + encoded, err := msg.EncodePayload() + if err != nil { + t.Fatalf("encode: %v", err) + } if !bytesEqual(encoded, isoRequestPayload) { - t.Fatalf("expected mutation to be ignored and original wire bytes returned:\n wire: % x\n encoded: % x", isoRequestPayload, encoded) + t.Fatalf("decoded message retained caller-owned payload: % x", encoded) } } diff --git a/pipeline.go b/pipeline.go index b2cb714..ce21bd9 100644 --- a/pipeline.go +++ b/pipeline.go @@ -27,13 +27,13 @@ type readPipeline struct { filter *filter adapter *adapter.CANAdapter decoder *decoder.Decoder - out chan<- pgn.Message + emit func(pgn.Message) } // newReadPipeline compiles the filter eagerly and wires adapter -> decoder -> output stage. // The returned pipeline delivers messages on out. It never closes out; the // caller that owns the frame source closes it when the source ends. -func newReadPipeline(ctx context.Context, cfg config, out chan pgn.Message) (*readPipeline, error) { +func newReadPipeline(ctx context.Context, cfg config, emit func(pgn.Message)) (*readPipeline, error) { log := cfg.logger if log == nil { log = slog.Default() @@ -53,7 +53,7 @@ func newReadPipeline(ctx context.Context, cfg config, out chan pgn.Message) (*re filter: f, adapter: adapter.NewCANAdapter(), decoder: decoder.New(), - out: out, + emit: emit, } p.decoder.SetOutput(p) p.adapter.SetOutput(p.decoder) @@ -103,8 +103,16 @@ func (p *readPipeline) HandleStruct(msg pgn.Message) { return } } - select { - case p.out <- msg: - case <-p.ctx.Done(): + if p.emit != nil && p.ctx.Err() == nil { + p.emit(msg) + } +} + +func channelEmitter(ctx context.Context, out chan<- pgn.Message) func(pgn.Message) { + return func(msg pgn.Message) { + select { + case out <- msg: + case <-ctx.Done(): + } } } diff --git a/pipeline_test.go b/pipeline_test.go index be5d933..588ae8a 100644 --- a/pipeline_test.go +++ b/pipeline_test.go @@ -32,7 +32,7 @@ func vesselHeadingFrame(t *testing.T) can.Frame { func TestReadPipeline_DecodesFrame(t *testing.T) { ctx := context.Background() out := make(chan pgn.Message, 1) - p, err := newReadPipeline(ctx, config{logger: testLogger()}, out) + p, err := newReadPipeline(ctx, config{logger: testLogger()}, channelEmitter(ctx, out)) if err != nil { t.Fatalf("newReadPipeline: %v", err) } @@ -49,7 +49,7 @@ func TestReadPipeline_DecodesFrame(t *testing.T) { func TestReadPipeline_FilterCompileErrorIsEager(t *testing.T) { out := make(chan pgn.Message, 1) - _, err := newReadPipeline(context.Background(), config{logger: testLogger(), filterExpr: "((("}, out) + _, err := newReadPipeline(context.Background(), config{logger: testLogger(), filterExpr: "((("}, channelEmitter(context.Background(), out)) if err == nil { t.Fatal("expected filter compile error") } @@ -58,7 +58,7 @@ func TestReadPipeline_FilterCompileErrorIsEager(t *testing.T) { func TestReadPipeline_DropsUnknownByDefault(t *testing.T) { ctx := context.Background() out := make(chan pgn.Message, 1) - p, err := newReadPipeline(ctx, config{logger: testLogger()}, out) + p, err := newReadPipeline(ctx, config{logger: testLogger()}, channelEmitter(ctx, out)) if err != nil { t.Fatalf("newReadPipeline: %v", err) } @@ -74,7 +74,7 @@ func TestReadPipeline_DropsUnknownByDefault(t *testing.T) { func TestReadPipeline_InjectAssembledAppliesPreFilter(t *testing.T) { ctx := context.Background() out := make(chan pgn.Message, 2) - p, err := newReadPipeline(ctx, config{logger: testLogger(), filterExpr: "pgn == 127250"}, out) + p, err := newReadPipeline(ctx, config{logger: testLogger(), filterExpr: "pgn == 127250"}, channelEmitter(ctx, out)) if err != nil { t.Fatalf("newReadPipeline: %v", err) } diff --git a/registry.go b/registry.go index 7f318cb..ca3a7fe 100644 --- a/registry.go +++ b/registry.go @@ -97,6 +97,12 @@ func (r *registry) handleClaim(source uint8, rawName uint64, now time.Time) (fir if known && r.byAddr[entry.addr] == entry { delete(r.byAddr, entry.addr) } + if displaced := r.byAddr[source]; displaced != nil && displaced != entry { + // Two active NAMEs cannot own the same address. The new claim is the + // authoritative binding; remove the stale entry so snapshots never + // report an impossible duplicate-address topology. + delete(r.byName, displaced.rawName) + } r.byAddr[source] = entry entry.addr = source entry.lastSeen = now @@ -175,11 +181,35 @@ func (e *deviceEntry) device() Device { } if e.product != nil { productCopy := *e.product + productCopy.Info = cloneMessageInfo(e.product.Info) + productCopy.Nmea2000Version = cloneUint64(e.product.Nmea2000Version) + productCopy.ProductCode = cloneUint64(e.product.ProductCode) + productCopy.CertificationLevel = cloneUint64(e.product.CertificationLevel) + productCopy.LoadEquivalency = cloneUint64(e.product.LoadEquivalency) d.ProductInfo = &productCopy } if e.config != nil { configCopy := *e.config + configCopy.Info = cloneMessageInfo(e.config.Info) d.ConfigInfo = &configCopy } return d } + +func cloneUint64(v *uint64) *uint64 { + if v == nil { + return nil + } + copy := *v + return © +} + +func cloneMessageInfo(info pgn.MessageInfo) pgn.MessageInfo { + if info.Priority != nil { + info.Priority = pgn.Priority(*info.Priority) + } + if info.TargetId != nil { + info.TargetId = pgn.Target(*info.TargetId) + } + return info +} diff --git a/registry_test.go b/registry_test.go index a87bcac..fe28b38 100644 --- a/registry_test.go +++ b/registry_test.go @@ -170,11 +170,32 @@ func TestRegistry_SnapshotsAreCopies(t *testing.T) { mb.inbound <- claimFrame(testDeviceName, 0x42) waitFor(t, 2*time.Second, func() bool { return len(c.Devices()) == 1 }) - for _, f := range fastPacketFrames(t, &pgn.ProductInformation{ModelId: "sonar"}, 6, 0x42, 255) { + productCode := uint64(42) + for _, f := range fastPacketFrames(t, &pgn.ProductInformation{ModelId: "sonar", ProductCode: &productCode}, 6, 0x42, 255) { mb.inbound <- f } waitFor(t, 2*time.Second, func() bool { return c.Devices()[0].ProductInfo != nil }) - c.Devices()[0].ProductInfo.ModelId = "tampered" + snapshot := c.Devices()[0] + snapshot.ProductInfo.ModelId = "tampered" + *snapshot.ProductInfo.ProductCode = 99 assert.Equal(t, "sonar", c.Devices()[0].ProductInfo.ModelId, "snapshot mutations must not leak into the registry") + assert.Equal(t, uint64(42), *c.Devices()[0].ProductInfo.ProductCode) +} + +func TestRegistry_AddressCollisionEvictsStaleName(t *testing.T) { + r := newRegistry() + first := testDeviceName.Pack(true) + secondName := testDeviceName + secondName.IdentityNumber++ + second := secondName.Pack(true) + now := time.Now() + + r.handleClaim(0x42, first, now) + r.handleClaim(0x42, second, now.Add(time.Second)) + + devices := r.snapshot() + require.Len(t, devices, 1) + assert.Equal(t, second, devices[0].RawName) + assert.Equal(t, uint8(0x42), devices[0].Address) } diff --git a/request.go b/request.go index 0d3b93d..3908084 100644 --- a/request.go +++ b/request.go @@ -58,7 +58,7 @@ func Request[T pgn.Message](ctx context.Context, c *Client, target uint8) (T, er Info: pgn.MessageInfo{TargetId: pgn.Target(target)}, Pgn: &requested, } - if err := c.Write(req).Wait(); err != nil { + if err := c.Write(req).WaitContext(ctx); err != nil { return zero, fmt.Errorf("n2k: sending ISO request for PGN %d: %w", pgnNum, err) } diff --git a/scanner.go b/scanner.go index 52c96cb..ca17d8a 100644 --- a/scanner.go +++ b/scanner.go @@ -2,6 +2,7 @@ package n2k import ( "context" + "errors" "log/slog" "sync" @@ -11,18 +12,25 @@ import ( // Scanner reads decoded NMEA 2000 messages one at a time. // Call Next() to advance, Message() to get the current message, and Err() for errors. type Scanner struct { - ctx context.Context - cfg config - msg pgn.Message - err error - ch chan pgn.Message - once sync.Once + ctx context.Context + cancel context.CancelFunc + cfg config + msg pgn.Message + err error + ch chan pgn.Message + once sync.Once + sub *messageSubscription } // NewScanner creates a Scanner that reads from the configured sources. func NewScanner(ctx context.Context, opts ...Option) *Scanner { cfg := config{} + var optionErr error for _, o := range opts { + if o == nil { + optionErr = errors.New("n2k: nil Option") + break + } o.apply(&cfg) } cfg.applyReconnect() @@ -30,20 +38,38 @@ func NewScanner(ctx context.Context, opts ...Option) *Scanner { cfg.logger = slog.Default() } + scannerCtx, cancel := context.WithCancel(ctx) + receiveBuffer := defaultReceiveBuffer + if cfg.receiveBuffer != nil && *cfg.receiveBuffer > 0 { + receiveBuffer = *cfg.receiveBuffer + } s := &Scanner{ - ctx: ctx, - cfg: cfg, - ch: make(chan pgn.Message, 64), + ctx: scannerCtx, + cancel: cancel, + cfg: cfg, + ch: make(chan pgn.Message, receiveBuffer), } // Validate eagerly so a misconfigured Scanner (e.g. no sources) fails on // the very first Next() call rather than only after the goroutine starts. - s.err = cfg.validate() + s.err = optionErr + if s.err == nil { + s.err = cfg.validate() + } return s } // Next advances the scanner to the next message. Returns false when no more messages // are available (source exhausted or error occurred). Check Err() after Next returns false. func (s *Scanner) Next() bool { + if s.sub != nil { + msg, ok := <-s.sub.ch + if !ok { + s.err = s.sub.terminalError() + return false + } + s.msg = msg + return true + } s.once.Do(func() { if s.err != nil { close(s.ch) @@ -70,14 +96,26 @@ func (s *Scanner) Err() error { return s.err } +// Close stops the scanner and releases its source or live-client +// subscription. It is safe to call more than once. +func (s *Scanner) Close() error { + if s.cancel != nil { + s.cancel() + } + if s.sub != nil { + s.sub.unsubscribe() + } + return nil +} + func (s *Scanner) run() { defer close(s.ch) - p, err := newReadPipeline(s.ctx, s.cfg, s.ch) + p, err := newReadPipeline(s.ctx, s.cfg, channelEmitter(s.ctx, s.ch)) if err != nil { s.err = err return } - if err := runSources(s.ctx, s.cfg.logger, s.cfg.sources, p.HandleFrame); err != nil { + if err := runSources(s.ctx, s.cfg.logger, s.cfg.sources, p.HandleFrame); err != nil && s.ctx.Err() == nil { s.err = err } } diff --git a/status.go b/status.go new file mode 100644 index 0000000..f9c6c47 --- /dev/null +++ b/status.go @@ -0,0 +1,46 @@ +package n2k + +// ClientStatus is a point-in-time operational snapshot suitable for health +// endpoints and metrics collectors. +type ClientStatus struct { + Address uint8 + AddressClaimed bool + Closed bool + TerminalError error + WriteQueueDepth int + WriteQueueCapacity int + ReceiveSubscribers int +} + +// Status returns a concurrency-safe snapshot of the client's lifecycle and +// bounded queues. It does not perform I/O. +func (c *Client) Status() ClientStatus { + if c == nil { + return ClientStatus{Closed: true, TerminalError: ErrClientClosed} + } + c.mu.Lock() + status := ClientStatus{ + Address: c.sourceAddr, + AddressClaimed: c.claimed, + Closed: c.closed, + TerminalError: c.terminalErr, + WriteQueueDepth: len(c.writeCh), + WriteQueueCapacity: cap(c.writeCh), + } + c.mu.Unlock() + if c.msgHub != nil { + status.ReceiveSubscribers = c.msgHub.subscriberCount() + } + return status +} + +// Err returns the terminal runtime error, if the bus or address-claiming +// lifecycle failed. A normal Close does not set an error. +func (c *Client) Err() error { + if c == nil { + return ErrClientClosed + } + c.mu.Lock() + defer c.mu.Unlock() + return c.terminalErr +} diff --git a/status_test.go b/status_test.go new file mode 100644 index 0000000..bbfc6b0 --- /dev/null +++ b/status_test.go @@ -0,0 +1,29 @@ +package n2k + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestClientStatusTracksLifecycle(t *testing.T) { + c, err := NewClient(context.Background(), Replay(nil)) + require.NoError(t, err) + status := c.Status() + assert.True(t, status.AddressClaimed) + assert.False(t, status.Closed) + assert.NoError(t, status.TerminalError) + assert.Equal(t, defaultWriteQueue, status.WriteQueueCapacity) + + require.NoError(t, c.Close()) + assert.True(t, c.Status().Closed) + assert.NoError(t, c.Err()) +} + +func TestNilClientStatus(t *testing.T) { + var c *Client + assert.True(t, c.Status().Closed) + assert.ErrorIs(t, c.Err(), ErrClientClosed) +} diff --git a/system.go b/system.go index a4726de..28a3246 100644 --- a/system.go +++ b/system.go @@ -40,7 +40,7 @@ func newSystemRouter(ctx context.Context, cfg config) (*systemRouter, error) { sysCfg.includeUnknown = false ch := make(chan pgn.Message, 64) - p, err := newReadPipeline(ctx, sysCfg, ch) + p, err := newReadPipeline(ctx, sysCfg, channelEmitter(ctx, ch)) if err != nil { return nil, err } diff --git a/units/distance.go b/units/distance.go index 765502d..a03fa9a 100644 --- a/units/distance.go +++ b/units/distance.go @@ -20,19 +20,15 @@ type DistanceUnit int // Conversion math: value_in_new_unit = value_in_old_unit * (newConv / oldConv) // // Examples: -// - 1 Mile -> Meter: 1 * (1609.34 / 1) = 1609.34 meters -// - 1 Foot -> Meter: 1 * (1609.34 / 5280) = 0.3048 meters +// - 1 Mile -> Meter: 1 * (1609.344 / 1) = 1609.344 meters +// - 1 Foot -> Meter: 1 * (1609.344 / 5280) = 0.3048 meters // - 1 Mile -> Fathom: 1 * (880 / 1) = 880 fathoms -// -// Note on NauticalMile: The value 1.15078 means "1 mile = 1.15078 in the NauticalMile column". -// Since 1 NM = 1.15078 statute miles, this means converting 1 mile gives 1.15078 "NM units" -// in this table's math. The table encodes ratios, not physical equivalences directly. var distanceConversions = map[DistanceUnit]float32{ - Meter: 1609.34, // 1 mile = 1609.34 meters - Foot: 5280, // 1 mile = 5280 feet - Mile: 1, // Reference unit: miles - NauticalMile: 1.15078, // Ratio relative to miles (1 NM = 1.15078 statute miles) - Fathom: 880, // 1 mile = 880 fathoms (1 fathom = 6 feet) + Meter: 1609.344, // international statute mile, exact + Foot: 5280, // 1 mile = 5280 feet + Mile: 1, // Reference unit: miles + NauticalMile: 1609.344 / 1852.0, // 1 nautical mile = 1852 meters, exact + Fathom: 880, // 1 mile = 880 fathoms (1 fathom = 6 feet) } // Distance is a type-safe unit structure that represents a distance/length measurement. diff --git a/units/distance_test.go b/units/distance_test.go index 6d624ec..207b9b6 100644 --- a/units/distance_test.go +++ b/units/distance_test.go @@ -24,13 +24,14 @@ func TestNewDistance(t *testing.T) { // expressed as "how many of unit X per mile". // // The conversion formula is: value * (newConv / oldConv) -// Table values: Mile=1, Meter=1609.34, Foot=5280, NauticalMile=1.15078, Fathom=880 +// Table values are independently anchored by the exact definitions +// 1 international mile = 1609.344 m and 1 nautical mile = 1852 m. func TestDistanceConvert(t *testing.T) { // Mile -> Meter: 1 * (1609.34 / 1) = 1609.34 t.Run("Mile to Meter", func(t *testing.T) { d := NewDistance(Mile, 1) result := d.Convert(Meter) - assert.InDelta(t, 1609.34, float64(result.Value), 0.1) + assert.InDelta(t, 1609.344, float64(result.Value), 0.001) assert.Equal(t, Meter, result.Unit) }) @@ -41,15 +42,23 @@ func TestDistanceConvert(t *testing.T) { assert.InDelta(t, 5280.0, float64(result.Value), 0.1) }) - // Mile -> NauticalMile: 1 * (1.15078 / 1) = 1.15078 - // Note: The table stores NauticalMile=1.15078 which is the ratio of statute miles - // per nautical mile (1 NM = 1.15078 statute miles). The conversion math produces - // 1.15078 for 1 mile -> NM, which is the table's ratio value. See the extensive - // comments in the original test for the mathematical reasoning. + // Mile -> NauticalMile: 1609.344 / 1852 = 0.868976... t.Run("Mile to NauticalMile", func(t *testing.T) { d := NewDistance(Mile, 1) result := d.Convert(NauticalMile) - assert.InDelta(t, 1.15078, float64(result.Value), 0.01) + assert.InDelta(t, 0.86897624, float64(result.Value), 0.000001) + }) + + t.Run("NauticalMile to Meter", func(t *testing.T) { + d := NewDistance(NauticalMile, 1) + result := d.Convert(Meter) + assert.InDelta(t, 1852.0, float64(result.Value), 0.001) + }) + + t.Run("1852 Meter to NauticalMile", func(t *testing.T) { + d := NewDistance(Meter, 1852) + result := d.Convert(NauticalMile) + assert.InDelta(t, 1.0, float64(result.Value), 0.000001) }) // Mile -> Fathom: 1 * (880 / 1) = 880 (1 mile = 880 fathoms, since 1 fathom = 6 feet) diff --git a/writeresult.go b/writeresult.go index 6c840e3..fd1b97b 100644 --- a/writeresult.go +++ b/writeresult.go @@ -1,5 +1,7 @@ package n2k +import "context" + // WriteResult represents the outcome of an asynchronous write operation. // The zero value is safe to use and behaves as a completed, successful write. type WriteResult struct { @@ -30,6 +32,20 @@ func (r *WriteResult) Wait() error { return r.err } +// WaitContext waits for completion or returns when ctx is done. Cancelling +// ctx does not cancel a write that has already reached the bus. +func (r *WriteResult) WaitContext(ctx context.Context) error { + if r.done == nil { + return nil + } + select { + case <-r.done: + return r.err + case <-ctx.Done(): + return ctx.Err() + } +} + // Done returns a channel that is closed when the write is complete. // On a zero-value WriteResult, Done returns an already-closed channel. func (r *WriteResult) Done() <-chan struct{} { diff --git a/writeresult_test.go b/writeresult_test.go index acaaf02..c9a6330 100644 --- a/writeresult_test.go +++ b/writeresult_test.go @@ -1,6 +1,7 @@ package n2k import ( + "context" "fmt" "testing" "time" @@ -8,6 +9,16 @@ import ( "github.com/stretchr/testify/assert" ) +func TestWriteResult_WaitContext(t *testing.T) { + r := newWriteResult() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + assert.ErrorIs(t, r.WaitContext(ctx), context.Canceled) + + r.complete(nil) + assert.NoError(t, r.WaitContext(context.Background())) +} + func TestWriteResult_Wait(t *testing.T) { r := newWriteResult() go func() { From f8210799c3b138287d8aea3574cbb76119ed68f5 Mon Sep 17 00:00:00 2001 From: Jake Thomas Date: Sat, 18 Jul 2026 21:48:15 -0400 Subject: [PATCH 2/2] ci: gate and prepare v0.3.0 release --- .github/workflows/lint.yml | 25 ------------ .github/workflows/release.yaml | 44 +++++++++++++-------- .github/workflows/security.yaml | 34 ----------------- .github/workflows/test.yaml | 65 +++++++++++++++++++++++++++++++- CHANGELOG.md | 24 +++++++----- CONTRIBUTING.md | 10 +++++ README.md | 14 +++---- VERSION | 1 + cmd/n2k/main_signal_unix_test.go | 49 ++++++++++++++++++++++++ cmd/n2k/main_test.go | 40 -------------------- 10 files changed, 174 insertions(+), 132 deletions(-) delete mode 100644 .github/workflows/lint.yml delete mode 100644 .github/workflows/security.yaml create mode 100644 VERSION create mode 100644 cmd/n2k/main_signal_unix_test.go diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml deleted file mode 100644 index d174cb1..0000000 --- a/.github/workflows/lint.yml +++ /dev/null @@ -1,25 +0,0 @@ -name: Lint - -on: - pull_request: - push: - branches: [main] - -jobs: - lint: - runs-on: ubuntu-latest - - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Set up Go - uses: actions/setup-go@v5 - with: - go-version: 1.23.x - check-latest: true - - - name: golangci-lint - uses: golangci/golangci-lint-action@v9 - with: - version: v2.12.0 diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 56db904..f70f450 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -1,51 +1,65 @@ name: Release -# Two ways in: -# - Every green Test run on main auto-tags the next v0.x.y patch release. -# - Manually pushing a v*.*.* tag (e.g. a minor bump: git tag v0.2.0 && -# git push origin v0.2.0) releases that tag directly. +# A successful CI run on the current main commit is the only release path. +# VERSION sets the next deliberate baseline; later releases increment patch. on: workflow_run: - workflows: [Test] + workflows: [CI] branches: [main] types: [completed] - push: - tags: ["v*.*.*"] permissions: contents: write +concurrency: + group: release + cancel-in-progress: false + jobs: release: runs-on: ubuntu-latest - if: ${{ github.event_name == 'push' || github.event.workflow_run.conclusion == 'success' }} + if: ${{ github.event.workflow_run.conclusion == 'success' }} steps: - name: Checkout uses: actions/checkout@v4 with: fetch-depth: 0 - ref: ${{ github.event.workflow_run.head_sha || github.ref }} + ref: ${{ github.event.workflow_run.head_sha }} - - name: Compute next patch version + - name: Compute release version id: version - if: ${{ github.event_name == 'workflow_run' }} run: | + git fetch origin main --tags + if [ "$(git rev-parse HEAD)" != "$(git rev-parse origin/main)" ]; then + echo "A newer main commit superseded this CI run; nothing to release." + echo "skip=true" >> "$GITHUB_OUTPUT" + exit 0 + fi if git describe --tags --exact-match --match 'v*.*.*' HEAD >/dev/null 2>&1; then echo "HEAD is already tagged; nothing to release." echo "skip=true" >> "$GITHUB_OUTPUT" exit 0 fi + + BASE_VERSION=$(tr -d '[:space:]' < VERSION) + if ! echo "$BASE_VERSION" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+$'; then + echo "VERSION must contain a semantic version without a leading v." >&2 + exit 1 + fi + BASE_TAG="v$BASE_VERSION" LATEST=$(git tag --list 'v*.*.*' | sort -V | tail -n 1) if [ -z "$LATEST" ]; then - TAG="v0.1.0" + TAG="$BASE_TAG" + elif [ "$LATEST" != "$BASE_TAG" ] && [ "$(printf '%s\n%s\n' "$LATEST" "$BASE_TAG" | sort -V | tail -n 1)" = "$BASE_TAG" ]; then + TAG="$BASE_TAG" else TAG=$(echo "$LATEST" | awk -F '[v.]' '{printf "v%d.%d.%d", $2, $3, $4 + 1}') fi echo "tag=$TAG" >> "$GITHUB_OUTPUT" - name: Create and push tag - if: ${{ github.event_name == 'workflow_run' && steps.version.outputs.skip != 'true' }} + if: ${{ steps.version.outputs.skip != 'true' }} run: | git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" @@ -53,14 +67,14 @@ jobs: git push origin "${{ steps.version.outputs.tag }}" - name: Set up Go - if: ${{ github.event_name == 'push' || steps.version.outputs.skip != 'true' }} + if: ${{ steps.version.outputs.skip != 'true' }} uses: actions/setup-go@v5 with: go-version: 1.23.x check-latest: true - name: Run goreleaser - if: ${{ github.event_name == 'push' || steps.version.outputs.skip != 'true' }} + if: ${{ steps.version.outputs.skip != 'true' }} uses: goreleaser/goreleaser-action@v6 with: distribution: goreleaser diff --git a/.github/workflows/security.yaml b/.github/workflows/security.yaml deleted file mode 100644 index aafb860..0000000 --- a/.github/workflows/security.yaml +++ /dev/null @@ -1,34 +0,0 @@ -name: Secure - -on: - pull_request: - push: - branches: [main] - -jobs: - secure: - runs-on: ubuntu-latest - - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Set up Go - uses: actions/setup-go@v5 - with: - go-version: 1.23.x - check-latest: true - - - name: Run govulncheck - env: - GOTOOLCHAIN: go1.25.12 - run: | - go install golang.org/x/vuln/cmd/govulncheck@v1.5.0 - govulncheck ./... - - - name: Run gosec - env: - GOTOOLCHAIN: go1.25.12 - run: | - go install github.com/securego/gosec/v2/cmd/gosec@v2.27.1 - gosec -exclude-generated -exclude=G115 ./... diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 59e6b7e..8ec7b8e 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -1,4 +1,4 @@ -name: Test +name: CI on: pull_request: @@ -58,3 +58,66 @@ jobs: - name: Fuzz smoke tests run: just fuzz-smoke + + lint: + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 1.23.x + check-latest: true + + - name: golangci-lint + uses: golangci/golangci-lint-action@v9 + with: + version: v2.12.0 + + secure: + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 1.23.x + check-latest: true + + - name: Run govulncheck + env: + GOTOOLCHAIN: go1.25.12 + run: | + go install golang.org/x/vuln/cmd/govulncheck@v1.5.0 + govulncheck ./... + + - name: Run gosec + env: + GOTOOLCHAIN: go1.25.12 + run: | + go install github.com/securego/gosec/v2/cmd/gosec@v2.27.1 + gosec -exclude-generated -exclude-dir=.claude -exclude=G115 ./... + + release-gate: + if: ${{ always() }} + needs: [test, race-and-fuzz, lint, secure] + runs-on: ubuntu-latest + env: + TEST_RESULT: ${{ needs.test.result }} + RACE_FUZZ_RESULT: ${{ needs.race-and-fuzz.result }} + LINT_RESULT: ${{ needs.lint.result }} + SECURE_RESULT: ${{ needs.secure.result }} + + steps: + - name: Require every release gate + run: | + test "$TEST_RESULT" = success + test "$RACE_FUZZ_RESULT" = success + test "$LINT_RESULT" = success + test "$SECURE_RESULT" = success diff --git a/CHANGELOG.md b/CHANGELOG.md index 222fe05..84d7ba3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## Change Log for open-ships/n2k -### Unreleased — bounded lifecycle, wire fidelity, and hostile-input hardening +### v0.3.0 — 2026-07-18 — bounded lifecycle, wire fidelity, and hostile-input hardening - Commanded Address (PGN 65240) is now a first-class address-claim transition: only an exact nine-byte broadcast ISO transport transfer for this node's @@ -32,7 +32,12 @@ and race CI, pinned lint/security tools, architecture context, ADRs, contribution guidance, and private security-reporting instructions. -### Unreleased — panics in a misbehaving Bus no longer crash the process +### v0.2.2 — 2026-07-18 — clean CLI shutdown + +`n2k sniff` now handles termination signals through context cancellation and +exits successfully after its input pipeline shuts down. + +### v0.2.1 — 2026-07-16 — panics in a misbehaving Bus no longer crash the process A `Bus` implementation that panics (rather than returning an error) from `WriteFrame`, `Run`, or a message handler previously crashed the whole host process, because n2k runs those on its own @@ -48,7 +53,7 @@ recovers panics, logs them with a stack trace, and degrades locally instead: skipped without tearing down the reader. - The heartbeat and system-router dispatch goroutines are guarded the same way. -### Unreleased — writable TCP gateway buses +### v0.2.0 — 2026-07-15 — writable TCP gateway buses `n2k.TCP(addr, format)` now works with `NewClient`, not just `Receive`/`NewScanner` — the full bus stack over the boat's WiFi gateway: @@ -76,7 +81,7 @@ stack over the boat's WiFi gateway: `just test-race` works again. - `UDP` and `File` sources remain read-only. -### Unreleased — semver releases, installable CLI, typed physical accessors +### v0.1.0 — 2026-07-14 — semver releases, installable CLI, typed physical accessors **Typed physical-value accessors** — every numeric PGN struct field with a physical interpretation (a unit, non-unity resolution, or offset) now has generated accessors, on @@ -93,9 +98,8 @@ tick). Raw-tick fields are unchanged underneath, so round-trip fidelity is prese with `-f` CEL filters and `-unknown`. - `n2k version` reports the release version (set by goreleaser). -**Semver releases with prebuilt binaries** — releases are now tagged `v0.x.y`: every green Test -run on `main` auto-tags the next patch and goreleaser publishes archives for Linux/macOS/Windows -(amd64/arm64); pushing a `v0.X.0` tag manually cuts a minor. Release names carry the CalVer date. +**Semver releases with prebuilt binaries** — releases are tagged `v0.x.y`, and goreleaser +publishes archives for Linux/macOS/Windows (amd64/arm64). Release names carry the CalVer date. A commented Homebrew-tap config is in `.goreleaser.yaml`, pending an `open-ships/homebrew-tap` repo and token. @@ -103,7 +107,7 @@ repo and token. and identity PGNs removed; see `testdata/README.md`), used by new runnable `Example*` tests and the README quickstart, with an animated sniffer demo at `.github/demo.svg`. -### Unreleased — bus citizenship, device registry, and new sources +### 2026.07.13-2 — bus citizenship, device registry, and new sources **Bus citizenship** — bus clients now meet the protocol obligations of a real NMEA 2000 device: @@ -125,11 +129,11 @@ the README quickstart, with an animated sniffer demo at `.github/demo.svg`. - `TCP(addr, format)` / `UDP(listenAddr, format)` — network gateway streams: `FormatYDRaw` (Yacht Devices RAW ASCII) and `FormatActisense` (Actisense binary framing; assembled messages are re-framed through the normal decode pipeline). - candump parsing moved from the roundtrip tool into `internal/candump` (now with timestamp extraction). -### Unreleased — candump round-trip tool +### 2026.07.12 — candump round-trip tool - `cmd/roundtrip` — replays a candump `-L` log through fast-packet assembly, decodes each message, re-encodes it, and compares against the wire bytes. Prints a before/after hex view per message (with markers under differing bytes) and a per-PGN summary; `-report ` collects every issue plus the summary into a file. Always processes the whole log and exits 0. Reads a log file or stdin (`candump -L vcan0 | roundtrip -`). -### Unreleased — Architecture deepening +### 2026.07.08 — Architecture deepening Seven refactors from an architecture review: one read pipeline, one CAN-ID codec, a public bus seam, a testable transport state machine, and a metadata-driven PGN codec with round-trip coverage for every PGN. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6587e4d..d045cda 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -32,5 +32,15 @@ non-confidential requirement identifiers and hashes. PGN files and metadata are generated. Modify the generator or its inputs and run `just pgn-sync`; do not hand-edit generated category files. +## Releases + +[`VERSION`](VERSION) is the minimum version the next successful `main` build +will release. Raise it in the same reviewed pull request as a breaking or +minor-level API change. After that version has been published, subsequent +fully green CI runs increment the patch component automatically. Do not push +release tags manually: the release workflow tags the exact current `main` +commit only after its test, conformance, race/fuzz, lint, and security jobs all +pass. + Architecture vocabulary and invariants live in [CONTEXT.md](CONTEXT.md). Design rationale lives in [docs/adr](docs/adr). diff --git a/README.md b/README.md index 84cb9bd..3d11c06 100644 --- a/README.md +++ b/README.md @@ -1,8 +1,6 @@ # n2k -[![Test](https://github.com/open-ships/n2k/actions/workflows/test.yaml/badge.svg)](https://github.com/open-ships/n2k/actions/workflows/test.yaml) -[![Lint](https://github.com/open-ships/n2k/actions/workflows/lint.yml/badge.svg)](https://github.com/open-ships/n2k/actions/workflows/lint.yml) -[![Secure](https://github.com/open-ships/n2k/actions/workflows/security.yaml/badge.svg)](https://github.com/open-ships/n2k/actions/workflows/security.yaml) +[![CI](https://github.com/open-ships/n2k/actions/workflows/test.yaml/badge.svg)](https://github.com/open-ships/n2k/actions/workflows/test.yaml) [![Go Reference](https://pkg.go.dev/badge/github.com/open-ships/n2k.svg)](https://pkg.go.dev/github.com/open-ships/n2k) [![Release](https://img.shields.io/github/v/release/open-ships/n2k)](https://github.com/open-ships/n2k/releases) @@ -107,10 +105,12 @@ go install github.com/open-ships/n2k/cmd/n2k@latest # CLI Prebuilt CLI binaries for Linux, macOS, and Windows are on the [releases page](https://github.com/open-ships/n2k/releases). -Releases follow semver with a `v0` major (`v0.x.y`). Every green build on -`main` automatically cuts a patch release (with prebuilt CLI binaries); minor -bumps are tagged manually when the API moves. While the major version is 0, -minor releases may contain breaking API changes — pin accordingly. +Releases follow semver with a `v0` major (`v0.x.y`). [`VERSION`](VERSION) +declares the next deliberate release baseline. Once that tag exists, every +fully green CI run on the current `main` commit increments the patch version +and publishes prebuilt CLI binaries. Raise `VERSION` in a reviewed PR when the +API warrants a minor bump. While the major version is 0, minor releases may +contain breaking API changes — pin accordingly. ## The `n2k` CLI diff --git a/VERSION b/VERSION new file mode 100644 index 0000000..0d91a54 --- /dev/null +++ b/VERSION @@ -0,0 +1 @@ +0.3.0 diff --git a/cmd/n2k/main_signal_unix_test.go b/cmd/n2k/main_signal_unix_test.go new file mode 100644 index 0000000..f2feacf --- /dev/null +++ b/cmd/n2k/main_signal_unix_test.go @@ -0,0 +1,49 @@ +//go:build aix || android || darwin || dragonfly || freebsd || illumos || ios || linux || netbsd || openbsd || solaris + +package main + +import ( + "bufio" + "io" + "os" + "os/exec" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestSniffTerminatesCleanlyOnSIGTERM(t *testing.T) { + if os.Getenv("N2K_SIGTERM_HELPER") == "1" { + err := sniff([]string{"-file", "../../testdata/sample.log", "-timing", "-unknown"}) + if err != nil { + os.Exit(1) + } + os.Exit(0) + } + + cmd := exec.Command(os.Args[0], "-test.run=^TestSniffTerminatesCleanlyOnSIGTERM$") + cmd.Env = append(os.Environ(), "N2K_SIGTERM_HELPER=1") + cmd.Stderr = io.Discard + stdout, err := cmd.StdoutPipe() + require.NoError(t, err) + require.NoError(t, cmd.Start()) + + ready := make(chan error, 1) + go func() { + _, err := bufio.NewReader(stdout).ReadBytes('\n') + ready <- err + }() + select { + case err := <-ready: + require.NoError(t, err) + case <-time.After(2 * time.Second): + _ = cmd.Process.Kill() + _ = cmd.Wait() + t.Fatal("sniff subprocess did not become ready") + } + + require.NoError(t, cmd.Process.Signal(syscall.SIGTERM)) + require.NoError(t, cmd.Wait()) +} diff --git a/cmd/n2k/main_test.go b/cmd/n2k/main_test.go index b2c3e1e..0c721c3 100644 --- a/cmd/n2k/main_test.go +++ b/cmd/n2k/main_test.go @@ -1,14 +1,8 @@ package main import ( - "bufio" "context" - "io" - "os" - "os/exec" - "syscall" "testing" - "time" "github.com/stretchr/testify/require" ) @@ -20,37 +14,3 @@ func TestSniffContextCancellationIsClean(t *testing.T) { err := sniffContext(ctx, []string{"-file", "../../testdata/sample.log", "-timing"}) require.NoError(t, err) } - -func TestSniffTerminatesCleanlyOnSIGTERM(t *testing.T) { - if os.Getenv("N2K_SIGTERM_HELPER") == "1" { - err := sniff([]string{"-file", "../../testdata/sample.log", "-timing", "-unknown"}) - if err != nil { - os.Exit(1) - } - os.Exit(0) - } - - cmd := exec.Command(os.Args[0], "-test.run=^TestSniffTerminatesCleanlyOnSIGTERM$") - cmd.Env = append(os.Environ(), "N2K_SIGTERM_HELPER=1") - cmd.Stderr = io.Discard - stdout, err := cmd.StdoutPipe() - require.NoError(t, err) - require.NoError(t, cmd.Start()) - - ready := make(chan error, 1) - go func() { - _, err := bufio.NewReader(stdout).ReadBytes('\n') - ready <- err - }() - select { - case err := <-ready: - require.NoError(t, err) - case <-time.After(2 * time.Second): - _ = cmd.Process.Kill() - _ = cmd.Wait() - t.Fatal("sniff subprocess did not become ready") - } - - require.NoError(t, cmd.Process.Signal(syscall.SIGTERM)) - require.NoError(t, cmd.Wait()) -}