[A13xB0]

Problem

Repeater adverts don't carry battery, so CoreScope only knows a repeater's battery when the repeater is itself an observer. Observers can ask the repeaters they hear directly: a guest login (blank password works on default repeaters) plus REQ_TYPE_GET_STATUS returns batt_milli_volts. There was nowhere to send those readings.

Change

• Topic: meshcore/<region>/<observer>/battery, one batch per poll cycle:

  {"v":1,"origin":"…","origin_id":"<observer key>","timestamp":"…",
   "readings":[{"node_id":"<64-hex>","battery_mv":3940,"snr":-5.0,"rssi":-119,
                "source":"poll","reply_hops":0,"method":"status","sampled_at":"…"}]}

• Ingestor: validates each batch and stores readings in a new neighbor_battery table (one row per node, observer and sample). Rejects: unknown version, origin_id ≠ topic, 0 or > 64 readings. Skips a single reading for: bad key, self-report, mV outside 0–10000 or not an integer, sampled_at more than 2 h old or 5 min ahead, duplicate node, bad source/method/hops. At most one stored reading per node per observer every minIntervalMinutes (default 10). Handled next to /neighbors, after the blacklist and IATA whitelist.
• API: /api/nodes/{pk}/battery merges the node's own status reports (source: self) with neighbour readings (source: neighbor, with observer_id, observer_name, snr, reply_hops). New sources (per source: latest, count, median SNR) and selected (the source latest_mv and status come from: a self report under 2 h old, else the neighbour observer with the best median SNR over 24 h, else the newest). status gains no_battery for 0 mV, which is what an openHop repeater without a power sensor reports; it was graded critical before.
• UI: the node analytics battery card names the observer behind the value (linked, with route, SNR and age) and lists any others, draws one line per source on a time axis, and lists recent readings with observer, SNR and route.
• The route is now in the OpenAPI spec (removed from the known gaps).

Config

Opt-in, ingestor side:

"neighborBattery": { "enabled": true, "minIntervalMinutes": 10 },
"retention": { "neighborBatteryDays": 30 }

Off, the topic is dropped and nothing changes. The server works with or without the table.

Scale: a reading is one row per node per observer per poll (hourly by default). Reads are by node_id, the primary key prefix.

Tests

• Ingestor: storing a reading, keeping 0 mV, every message-level reject, every reading-level skip, duplicates, defaults, the min-interval cap per observer, prune, config defaults, and the topic gate through handleMessage (off stores nothing; on stores and doesn't touch observers).
• Server: neighbour samples with observer names, best-SNR selection over the last reporter, a fresh self report winning and a stale one not, no_battery, a missing table, and the fallback to the newest source.
• Frontend: test-node-analytics-battery-observers.js (Reported by, one line per observer, readings table, escaping, self-only).
• go test ./... passes in cmd/ingestor and cmd/server; test-all.sh passes.

A publisher for openHop repeaters is being built alongside this; until one exists, nothing publishes on the topic.

[efiten]

Thanks. I read the whole diff and ran go vet, gofmt -l and the cmd/ingestor and cmd/server suites on 280a3c79 (all pass), plus test-all.sh (passes). The firmware side matches: batt_milli_volts is the first uint16_t of RepeaterStats (examples/simple_repeater/MyMesh.h:44-45), in mV, and 0 mV means no sensor on boards without one (STM32Board.h:21), so no_battery is right. The writes stay in the ingestor and the server only reads.

Three things to change before merge:

1. The observer ranking uses a different window than documented. The PR body and openapi.go:150 say "best median SNR over 24 h". summariseBatterySources (cmd/server/neighbor_battery.go:136-163) takes the median over the whole ?days window (up to 365); the 24 h check in selectBatterySource (:194) only looks at each observer's latest reading. So selected depends on days. A probe test (not committed) showed it: observer A with 20 readings 3-4 days ago at SNR +10 and one reading 1 h ago at -15, observer B with one reading 2 h ago at +5. A is selected (median 10), and A's latest reading sets the status. Computing the median only over readings inside batteryRankWindow would match the docs.
2. Any publisher can set any node's battery. Readings are accepted from whoever may publish on meshcore/+/+/battery, for any node_id. The origin_id check at cmd/ingestor/neighbor_battery.go:64-68 compares the payload with itself; the comment says the broker binds origin_id to the authenticated key, but nothing in the repo enforces or documents that. SNR is accepted up to +64 (:151) and the highest median wins, so one publisher reporting snr: 64 overrides every real neighbour. Could you add a doc page with the broker ACL this needs (as docs/client-rf-samples.md:65 does for the client topics), bound SNR to a realistic range, and only accept readings from observers present in observers?
3. Retention defaults to keep forever. config.go:381-389 falls back to 0 when the feature is on and neighborBatteryDays is absent; only config.example.json sets 30. minIntervalMinutes limits per (node, observer) pair, so a publisher rotating observer keys adds up to 64 rows per message. A non-zero default (30) when the feature is enabled would bound it.

Smaller, can be in the same push:

4. public/node-analytics.js:366 adds BATTERY_LINE_COLORS as hex values, and node-battery.js:11-14 puts hex fallbacks in var(). Colours go through CSS variables in this codebase.
5. node-battery.js:17-26 (route, reporter) repeats batteryRoute and batteryObserverLink from node-analytics.js:380-390.
6. fillObserverNames (cmd/server/neighbor_battery.go:102) runs one query per observer; one IN (...) query does the same.
7. The PR adds 4 map[string]interface{} uses outside tests (parseBatteryReport, parseBatteryReading, handleBatteryReport, one cast in neighbor_battery.go). AGENTS.md asks for that count to go down; a named struct for the payload would do it.

Not done on my side: no browser check, and nothing publishes on this topic yet, so I tested only through the unit tests and the probe above.

[n30nex]

Rechecked the existing review against 92831a19. The ranking-window issue still reproduces in the current server code: the same recent readings select observer B at 4000 mV for days=1, but observer A at 2900 mV for days=7 because old +10 dB readings enter the median. The 24-hour filter is still applied only to each source's latest timestamp. Please exclude older readings from the ranking median and retain them only for chart history. The other six review requests also remain visible in this head; I would keep the existing review open.

[efiten]

Thanks for the new commits. I re-read the diff at 92831a19 and ran go test -run 'Shared|Battery|Claim|Status' in cmd/ingestor and -run 'Battery|SharedGuest|StatusHistory' in cmd/server (both pass). The RepeaterStats field order in sgStatusFields matches examples/simple_repeater/MyMesh.h:44-60, and the RESPONSE layout matches docs/payloads.md:69-78.

The earlier review

None of the seven points has changed yet:

1. Ranking window: summariseBatterySources (cmd/server/neighbor_battery.go:238-267) still takes the median over the whole ?days window, and the 24 h check in selectBatterySource (:299) still looks only at each source's latest reading.
2. Publisher trust: SNR is still accepted up to ±64 (cmd/ingestor/neighbor_battery.go:196), there is no check that the observer exists in observers, the origin_id check (:110-114) still compares the payload with its own topic, and there is no doc page with the broker ACL.
3. Retention: NeighborBatteryDaysOrZero (cmd/ingestor/config.go:384-389) still returns 0 when the setting is absent.
4. Hex colours: still there, and more were added (public/node-analytics.js:373, :488, :529-533, node-battery.js:13-15, node-status-charts.js:41-43, :104).
5. route / observerLink in node-battery.js:19-35 still repeat node-analytics.js:393-405.
6. fillObserverNames (cmd/server/neighbor_battery.go:142-157) still runs one query per observer, and the new fillNodeNames (:130-137) does the same per relay.
7. map[string]interface{} outside tests went from 4 to 14 added lines, including HandleClaim (shared_guest_decode.go:121, :140) and the response in shared_guest_key.go:22.

The shared guest key

1. Question on the trust model. The keypair is committed in cmd/ingestor/shared_guest_decode.go:28-29 and cmd/server/shared_guest_key.go:15-16, and GET /api/shared-guest/key serves the private half with no auth and no config gate. The comment says this is public by design. The consequence: anyone with that seed can derive the ECDH secret with any repeater, build a valid RESPONSE "from" it and inject it on any packets topic or over RF. Handle (shared_guest_decode.go:232-295) stores it as a reading if the 2-byte MAC, the tag window and the mV value check out. The same seed also gives everyone the access level of every repeater that adds this identity to its ACL. Is that acceptable for battery readings that drive the node status? If yes, it needs to be written down for operators, and the readings should be marked as unauthenticated. If no, a per-deployment key from config, not served by the API, would close it.
2. The decoder runs with the feature off. main.go:106 creates it unconditionally, and :1040 (HandleClaim) and :1300 (Handle) call it without cfg.NeighborBatteryEnabled(), unlike the /battery topic at :1030. With the default config, rows land in neighbor_battery and neighbor_battery_heard while pruning never runs (retention 0).
3. Claims can be taken over. Any client that may publish on meshcore/<r>/<X>/polling can claim any node_id. The last writer wins (shared_guest_decode.go:164), and the decoded reading is then credited to X, which does not have to be a known observer.
4. Stored XSS through the relay name. batteryRoute (public/node-analytics.js:396) inserts s.via_name unescaped, and :446 writes the result with innerHTML. via_name is the relay's advert name, which sanitizeName (cmd/ingestor/decoder.go:1195) does not HTML-escape, and via is chosen by the publisher of the claim or the /battery payload. node-battery.js:26 already escapes the same field, so escapeHtml here fixes it.
5. CI is red because of gofmt. "Go Build & Test" fails on cmd/ingestor/shared_guest_decode.go (the struct at :44-48 and the const block at :71-76), and "Build & Publish Docker Image" fails because it depends on that job. The E2E shards passed.

Smaller:

• sgTagTTL (:39) is unused.
• The key constants exist twice, in the ingestor and the server.
• sgParseStatus stores the stats without the range checks statusFields applies.

Not done on my side: I did not build a forged packet end to end (points 1 and 3 come from reading the code), and no browser check.