From 4898541bce4c8cc41be08f189cda09343a42fc39 Mon Sep 17 00:00:00 2001 From: Kpa-clawbot Date: Tue, 31 Mar 2026 17:53:04 -0700 Subject: [PATCH] fix(ingestor): observer metadata nested stats + SNR/RSSI case fallback (#336) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Two data integrity bugs in the Go ingestor cause observer metadata and signal quality data to be missing for all Go-backend users. ### #320 — Observer metadata never populated `extractObserverMeta()` reads `battery_mv`, `uptime_secs`, and `noise_floor` from the **top level** of the MQTT status message. However, the actual MQTT payload nests these under a `stats` object: ```json { "status": "online", "origin": "ObserverName", "model": "Heltec V3", "firmware_version": "v1.14.0-9f1a3ea", "stats": { "battery_mv": 4174, "uptime_secs": 80277, "noise_floor": -110 } } ``` Result: battery, uptime, and noise floor are always NULL in the database. ### #321 — SNR and RSSI always missing on raw packets The raw packet handler reads `msg["SNR"]` and `msg["RSSI"]` (uppercase only). Some MQTT bridges send these as lowercase `snr`/`rssi`. The companion BLE handler already has a case-insensitive fallback — the raw packet path did not. Result: SNR/RSSI are NULL for all raw packet observations from bridges that use lowercase keys. ## Fix ### #320 — Nested stats with top-level fallback - Added `nestedOrTopLevel()` helper that checks `msg["stats"][key]` first, then `msg[key]` - `extractObserverMeta` now uses this helper for `battery_mv`, `uptime_secs`, `noise_floor` - Top-level fallback preserved for backward compatibility with bridges that flatten the structure - Safe type assertion: `stats, _ := msg["stats"].(map[string]interface{})` — no crash if stats is missing or wrong type ### #321 — Lowercase SNR/RSSI fallback - Raw packet handler now uses `else if` to check lowercase `snr`/`rssi` when uppercase keys are absent - Matches the pattern already used in the companion channel and direct message handlers ## Tests 10 new test cases added: | Test | What it verifies | |------|-----------------| | `TestExtractObserverMetaNestedStats` | All 5 fields populated from nested stats object | | `TestExtractObserverMetaNestedStatsPrecedence` | Nested stats wins over top-level when both present | | `TestExtractObserverMetaFlatFallback` | Flat structure still works (backward compat) | | `TestExtractObserverMetaEmptyStats` | Empty stats object — no crash, model still works | | `TestExtractObserverMetaStatsNotAMap` | stats is a string — no crash, falls back to top-level | | `TestExtractObserverMetaNoiseFloorFloat` | Float precision preserved (noise_floor REAL migration) | | `TestHandleMessageWithLowercaseSNRRSSI` | Lowercase snr/rssi both stored correctly | | `TestHandleMessageSNRRSSIUppercaseWins` | When both cases present, uppercase takes precedence | | `TestHandleMessageNoSNRRSSI` | Neither key present — nil, no crash | | Existing `TestExtractObserverMeta` | Still passes (flat structure backward compat) | All tests pass: `go test ./... -count=1` and `go vet ./...` clean. Closes #320 Closes #321 --------- Co-authored-by: Kpa-clawbot <259247574+Kpa-clawbot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/ingestor/db_test.go | 144 ++++++++++++++++++++++++++++++++++++++ cmd/ingestor/main.go | 31 +++++++- cmd/ingestor/main_test.go | 63 +++++++++++++++++ 3 files changed, 235 insertions(+), 3 deletions(-) diff --git a/cmd/ingestor/db_test.go b/cmd/ingestor/db_test.go index 158ea820..3debc706 100644 --- a/cmd/ingestor/db_test.go +++ b/cmd/ingestor/db_test.go @@ -1313,3 +1313,147 @@ func TestTelemetryMigrationAddsColumns(t *testing.T) { t.Errorf("migration node_telemetry_v1 should be recorded, count=%d", count) } } + +// --- Bug #320: Observer metadata nested stats --- + +func TestExtractObserverMetaNestedStats(t *testing.T) { + // Real-world MQTT status payload: stats fields nested under "stats" + msg := map[string]interface{}{ + "status": "online", + "origin": "ObserverName", + "model": "Heltec V3", + "firmware_version": "v1.14.0-9f1a3ea", + "stats": map[string]interface{}{ + "battery_mv": 4174.0, + "uptime_secs": 80277.0, + "noise_floor": -110.0, + }, + } + meta := extractObserverMeta(msg) + if meta == nil { + t.Fatal("expected non-nil meta") + } + if meta.Model == nil || *meta.Model != "Heltec V3" { + t.Errorf("Model=%v, want Heltec V3", meta.Model) + } + if meta.Firmware == nil || *meta.Firmware != "v1.14.0-9f1a3ea" { + t.Errorf("Firmware=%v, want v1.14.0-9f1a3ea", meta.Firmware) + } + if meta.BatteryMv == nil || *meta.BatteryMv != 4174 { + t.Errorf("BatteryMv=%v, want 4174", meta.BatteryMv) + } + if meta.UptimeSecs == nil || *meta.UptimeSecs != 80277 { + t.Errorf("UptimeSecs=%v, want 80277", meta.UptimeSecs) + } + if meta.NoiseFloor == nil || *meta.NoiseFloor != -110.0 { + t.Errorf("NoiseFloor=%v, want -110", meta.NoiseFloor) + } +} + +func TestExtractObserverMetaNestedStatsPrecedence(t *testing.T) { + // If stats has a value AND top-level has a value, nested wins + msg := map[string]interface{}{ + "battery_mv": 9999.0, // top-level (stale/wrong) + "noise_floor": -120.0, // top-level (stale/wrong) + "stats": map[string]interface{}{ + "battery_mv": 4174.0, // nested (correct) + "noise_floor": -110.5, // nested (correct) + }, + } + meta := extractObserverMeta(msg) + if meta == nil { + t.Fatal("expected non-nil meta") + } + if meta.BatteryMv == nil || *meta.BatteryMv != 4174 { + t.Errorf("BatteryMv=%v, want 4174 (nested should win over top-level)", meta.BatteryMv) + } + if meta.NoiseFloor == nil || *meta.NoiseFloor != -110.5 { + t.Errorf("NoiseFloor=%v, want -110.5 (nested should win over top-level)", meta.NoiseFloor) + } +} + +func TestExtractObserverMetaFlatFallback(t *testing.T) { + // Backward compatibility: flat structure (no stats object) still works + msg := map[string]interface{}{ + "battery_mv": 3500.0, + "uptime_secs": 86400.0, + "noise_floor": -115.5, + } + meta := extractObserverMeta(msg) + if meta == nil { + t.Fatal("expected non-nil meta for flat structure") + } + if meta.BatteryMv == nil || *meta.BatteryMv != 3500 { + t.Errorf("BatteryMv=%v, want 3500", meta.BatteryMv) + } + if meta.UptimeSecs == nil || *meta.UptimeSecs != 86400 { + t.Errorf("UptimeSecs=%v, want 86400", meta.UptimeSecs) + } + if meta.NoiseFloor == nil || *meta.NoiseFloor != -115.5 { + t.Errorf("NoiseFloor=%v, want -115.5", meta.NoiseFloor) + } +} + +func TestExtractObserverMetaEmptyStats(t *testing.T) { + // Empty stats object should not crash, top-level fallback still applies + msg := map[string]interface{}{ + "model": "T-Beam", + "stats": map[string]interface{}{}, + } + meta := extractObserverMeta(msg) + if meta == nil { + t.Fatal("expected non-nil meta (model is present)") + } + if meta.Model == nil || *meta.Model != "T-Beam" { + t.Errorf("Model=%v, want T-Beam", meta.Model) + } + if meta.BatteryMv != nil { + t.Errorf("BatteryMv should be nil, got %v", *meta.BatteryMv) + } +} + +func TestExtractObserverMetaStatsNotAMap(t *testing.T) { + // stats field is not a map (e.g., string) — should not crash, fall back to top-level + msg := map[string]interface{}{ + "stats": "invalid", + "battery_mv": 3700.0, + } + meta := extractObserverMeta(msg) + if meta == nil { + t.Fatal("expected non-nil meta") + } + if meta.BatteryMv == nil || *meta.BatteryMv != 3700 { + t.Errorf("BatteryMv=%v, want 3700 (top-level fallback when stats is not a map)", meta.BatteryMv) + } +} + +func TestExtractObserverMetaNoiseFloorFloat(t *testing.T) { + // noise_floor migrated to REAL — verify float precision preserved + msg := map[string]interface{}{ + "stats": map[string]interface{}{ + "noise_floor": -108.75, + }, + } + meta := extractObserverMeta(msg) + if meta == nil { + t.Fatal("expected non-nil meta") + } + if meta.NoiseFloor == nil || *meta.NoiseFloor != -108.75 { + t.Errorf("NoiseFloor=%v, want -108.75", meta.NoiseFloor) + } +} + +func TestExtractObserverMetaNestedNilSkipsTopLevel(t *testing.T) { + // JSON {"stats": {"battery_mv": null}} decodes to nil value in the map. + // Nested nil should suppress top-level fallback (nested wins semantics). + msg := map[string]interface{}{ + "battery_mv": 3700.0, + "stats": map[string]interface{}{ + "battery_mv": nil, + }, + } + meta := extractObserverMeta(msg) + if meta != nil && meta.BatteryMv != nil { + t.Error("nested nil should suppress top-level fallback") + } +} diff --git a/cmd/ingestor/main.go b/cmd/ingestor/main.go index bbeb502e..64204a6c 100644 --- a/cmd/ingestor/main.go +++ b/cmd/ingestor/main.go @@ -241,11 +241,19 @@ func handleMessage(store *Store, tag string, source MQTTSource, m mqtt.Message, if f, ok := toFloat64(v); ok { mqttMsg.SNR = &f } + } else if v, ok := msg["snr"]; ok { + if f, ok := toFloat64(v); ok { + mqttMsg.SNR = &f + } } if v, ok := msg["RSSI"]; ok { if f, ok := toFloat64(v); ok { mqttMsg.RSSI = &f } + } else if v, ok := msg["rssi"]; ok { + if f, ok := toFloat64(v); ok { + mqttMsg.RSSI = &f + } } if v, ok := msg["origin"].(string); ok { mqttMsg.Origin = v @@ -511,21 +519,25 @@ func extractObserverMeta(msg map[string]interface{}) *ObserverMeta { hasData = true } - if v, ok := msg["battery_mv"]; ok { + // Stats fields may be nested under a "stats" object or at top level. + // Try nested first, fall back to top-level for backward compatibility. + stats, _ := msg["stats"].(map[string]interface{}) + + if v := nestedOrTopLevel(stats, msg, "battery_mv"); v != nil { if f, ok := toFloat64(v); ok { iv := int(math.Round(f)) meta.BatteryMv = &iv hasData = true } } - if v, ok := msg["uptime_secs"]; ok { + if v := nestedOrTopLevel(stats, msg, "uptime_secs"); v != nil { if f, ok := toFloat64(v); ok { iv := int64(math.Round(f)) meta.UptimeSecs = &iv hasData = true } } - if v, ok := msg["noise_floor"]; ok { + if v := nestedOrTopLevel(stats, msg, "noise_floor"); v != nil { if f, ok := toFloat64(v); ok { meta.NoiseFloor = &f hasData = true @@ -538,6 +550,19 @@ func extractObserverMeta(msg map[string]interface{}) *ObserverMeta { return meta } +// nestedOrTopLevel looks up a key in the nested map first, then the top-level map. +func nestedOrTopLevel(nested, toplevel map[string]interface{}, key string) interface{} { + if nested != nil { + if v, ok := nested[key]; ok { + return v + } + } + if v, ok := toplevel[key]; ok { + return v + } + return nil +} + func firstNonEmpty(vals ...string) string { for _, v := range vals { if v != "" { diff --git a/cmd/ingestor/main_test.go b/cmd/ingestor/main_test.go index 92fc1790..08b07efc 100644 --- a/cmd/ingestor/main_test.go +++ b/cmd/ingestor/main_test.go @@ -623,3 +623,66 @@ func TestLoadChannelKeysSkipExplicit(t *testing.T) { t.Errorf("#General = %q, want my_explicit_key", keys["#General"]) } } + +// --- Bug #321: SNR/RSSI case-insensitive fallback --- + +func TestHandleMessageWithLowercaseSNRRSSI(t *testing.T) { + store := newTestStore(t) + source := MQTTSource{Name: "test"} + + rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976" + payload := []byte(`{"raw":"` + rawHex + `","snr":5.5,"rssi":-102}`) + msg := &mockMessage{topic: "meshcore/SJC/obs1/packets", payload: payload} + + handleMessage(store, "test", source, msg, nil, nil) + + var snr, rssi *float64 + store.db.QueryRow("SELECT snr, rssi FROM observations LIMIT 1").Scan(&snr, &rssi) + if snr == nil || *snr != 5.5 { + t.Errorf("snr=%v, want 5.5 (lowercase key)", snr) + } + if rssi == nil || *rssi != -102 { + t.Errorf("rssi=%v, want -102 (lowercase key)", rssi) + } +} + +func TestHandleMessageSNRRSSIUppercaseWins(t *testing.T) { + store := newTestStore(t) + source := MQTTSource{Name: "test"} + + // Both uppercase and lowercase present — uppercase should take precedence + rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976" + payload := []byte(`{"raw":"` + rawHex + `","SNR":7.2,"snr":1.0,"RSSI":-95,"rssi":-50}`) + msg := &mockMessage{topic: "meshcore/SJC/obs1/packets", payload: payload} + + handleMessage(store, "test", source, msg, nil, nil) + + var snr, rssi *float64 + store.db.QueryRow("SELECT snr, rssi FROM observations LIMIT 1").Scan(&snr, &rssi) + if snr == nil || *snr != 7.2 { + t.Errorf("snr=%v, want 7.2 (uppercase should take precedence)", snr) + } + if rssi == nil || *rssi != -95 { + t.Errorf("rssi=%v, want -95 (uppercase should take precedence)", rssi) + } +} + +func TestHandleMessageNoSNRRSSI(t *testing.T) { + store := newTestStore(t) + source := MQTTSource{Name: "test"} + + rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976" + payload := []byte(`{"raw":"` + rawHex + `"}`) + msg := &mockMessage{topic: "meshcore/SJC/obs1/packets", payload: payload} + + handleMessage(store, "test", source, msg, nil, nil) + + var snr, rssi *float64 + store.db.QueryRow("SELECT snr, rssi FROM observations LIMIT 1").Scan(&snr, &rssi) + if snr != nil { + t.Errorf("snr should be nil when not present, got %v", *snr) + } + if rssi != nil { + t.Errorf("rssi should be nil when not present, got %v", *rssi) + } +}