diff --git a/README.md b/README.md index 423a369..d4f03cb 100644 --- a/README.md +++ b/README.md @@ -14,8 +14,8 @@ automation systems like openHAB). Runs as a systemd service. active `cpufreq` scaling governor - **Load average** - 1/5/15 minute values shown as gauges, scaled to CPU core count -- **CPU temperature** - auto-detected thermal zone, with an optional - `vcgencmd`-sourced GPU temperature if available +- **CPU temperature** - auto-detected thermal zone, with optional + `vcgencmd`-sourced GPU and PMIC (Pi 4/5) temperatures if available - **Memory & swap** usage - **Filesystem usage** - per mounted filesystem, pseudo filesystems (tmpfs, proc, overlay, ...) excluded by default diff --git a/docs/API.md b/docs/API.md index b0c44da..619f021 100644 --- a/docs/API.md +++ b/docs/API.md @@ -128,6 +128,7 @@ extract the fields you need (e.g. via JSONPath in openHAB's HTTP binding). "cpu_count": 4, "temperature": { "zone": "cpu-thermal", "celsius": 48.6 }, "gpu_temperature": { "celsius": 47.8 }, + "pmic_temperature": { "celsius": 52.1 }, "throttled": { "under_voltage_now": false, "frequency_capped_now": false, @@ -261,7 +262,16 @@ Notes: and a core that is offline or whose driver doesn't expose both files is simply left out rather than failing the whole reading. - `gpu_temperature` is only present if `vcgencmd` is installed and - responded successfully; otherwise the field is omitted. + responded successfully; otherwise the field is omitted. Note that it is + not a second physical sensor: CPU and GPU share the SoC die, so this + reads the same sensor as `temperature`, by a different route. +- `pmic_temperature` is the Power-Management IC's own sensor — genuinely + separate silicon from the SoC, and useful for spotting power-delivery or + board-level heat distinct from CPU load. It exists only on the Raspberry + Pi 4 and 5, is read via `vcgencmd measure_temp pmic`, and is omitted + (exactly like `gpu_temperature`) whenever `vcgencmd` is unavailable or + the board has no PMIC sensor. It is not exposed through sysfs hwmon on + Raspberry Pi OS, so it never appears in `sensors` either. - `throttled` decodes the Raspberry Pi `vcgencmd get_throttled` bitmask. The `*_now` flags reflect the current state; the `*_since_boot` flags latch whether the condition has occurred at any point since boot. A set @@ -317,7 +327,8 @@ Notes: - These are additive to `v1`: `GET /api/v1/metrics` is unchanged, and keeps returning every field, including the ones with no endpoint of their own (`timestamp`, `uptime_seconds`, `load_average`, `cpu_count`, - `cpu_frequency`, `swap`, `gpu_temperature`, `throttled`, `system`, + `cpu_frequency`, `swap`, `gpu_temperature`, `pmic_temperature`, + `throttled`, `system`, `disk_io`, `wireless`, `sensors`). Poll the full snapshot if you need several metrics at once — six narrow requests cost more than one full one. @@ -718,6 +729,7 @@ Metrics exposed (all gauges, prefixed `pimonitor_`): | `cpu_core_usage_percent` | `core` (0-based index) | Omitted entirely on platforms without per-core data | | `temperature_celsius` | `zone` | Omitted entirely — the whole family is skipped — whenever the most recent temperature collection failed (e.g. no readable thermal zone) or hasn't completed yet; a `0` reading is never fabricated for a missing sensor | | `gpu_temperature_celsius` | — | Only present when `vcgencmd` responded, like `gpu_temperature` in `GET /api/v1/metrics` | +| `pmic_temperature_celsius` | — | Only present when `vcgencmd measure_temp pmic` responded (Raspberry Pi 4/5), like `pmic_temperature` in `GET /api/v1/metrics` | | `memory_total_bytes`, `memory_available_bytes`, `memory_used_percent` | — | | | `swap_total_bytes`, `swap_used_bytes`, `swap_used_percent` | — | | | `disk_total_bytes`, `disk_used_bytes`, `disk_used_percent` | `mount` | One series per mounted filesystem, same set as `disks` in `GET /api/v1/metrics` (pseudo-filesystems and network filesystems already excluded) | diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0e47a69..10ccc81 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -337,8 +337,9 @@ Left at its default (`healthz_max_staleness_seconds: 0`), that bound is `3 * poll_interval_seconds + 2 * collector.WorstCaseTickOverhead`, not just the poll interval alone: `Collector.fastTick` (`collector.go`) publishes `latest.Timestamp` only when a tick *completes*, sequentially running collectors that themselves -degrade via timeout rather than fail fast — `TemperatureCollector`/`ThrottledCollector` -each bound a hung `vcgencmd` call at `vcgencmdTimeout`, `DiskCollector` bounds a +degrade via timeout rather than fail fast — `TemperatureCollector` (twice, for +`measure_temp` and `measure_temp pmic`) and `ThrottledCollector` each bound a hung +`vcgencmd` call at `vcgencmdTimeout`, `DiskCollector` bounds a stalled `statfs` at `defaultStatfsTimeout` — so a single legitimately slow tick can already take `collector.WorstCaseTickOverhead`, and the timestamp visible right before the *next* tick publishes can lag by up to twice that. Ignoring this would @@ -536,7 +537,7 @@ apt package cache (`apt-get update`) requires root while reading its result world-readable files under `/proc`, `/sys/class/thermal`, `/sys/devices/system/cpu/*/cpufreq`, `/sys/class/hwmon`, `/etc/os-release`, and the existing apt cache, plus the read-only `apt list --upgradable` command and the optional - `vcgencmd measure_temp` / `vcgencmd get_throttled` commands — all invoked with fixed + `vcgencmd measure_temp` / `vcgencmd measure_temp pmic` / `vcgencmd get_throttled` commands — all invoked with fixed argument lists (never user input interpolated into a shell command), and further sandboxed via systemd unit hardening directives. - **`pimonitor-apt-update.timer`** runs as root, on a schedule (every 6h), and performs diff --git a/internal/collector/collector.go b/internal/collector/collector.go index 2223ec5..df5985d 100644 --- a/internal/collector/collector.go +++ b/internal/collector/collector.go @@ -149,9 +149,13 @@ func dropPrefix(points []HistoryPoint, keep func(HistoryPoint) bool) []HistoryPo // WorstCaseTickOverhead is the most a single fastTick may legitimately run // over instant /proc-style reads before c.latest is updated. Within -// collectFastTickSamples, TemperatureCollector and ThrottledCollector each -// shell out to vcgencmd (bounded by vcgencmdTimeout), and these run -// sequentially, not concurrently, so their worst case is additive. +// collectFastTickSamples, TemperatureCollector shells out to vcgencmd twice +// (`measure_temp` and `measure_temp pmic`) and ThrottledCollector once +// (`get_throttled`), each bounded by vcgencmdTimeout, and these run +// sequentially, not concurrently, so their worst case is additive — hence +// the factor of three below, which must be kept in step with the number of +// vcgencmd invocations a tick makes (see +// TestWorstCaseTickOverhead_CoversEveryVcgencmdInvocation). // DiskCollector bounds a stalled statfs at defaultStatfsTimeout — counted // once here, which is the common case: a single dying device or // unresponsive network mount. Several mounts stalling at the same time cost @@ -162,7 +166,7 @@ func dropPrefix(points []HistoryPoint, keep func(HistoryPoint) bool) []HistoryPo // firmware call or an unresponsive mount, both of which the collector // deliberately degrades rather than dies on — isn't mistaken for a stalled // collector. -const WorstCaseTickOverhead = 2*vcgencmdTimeout + defaultStatfsTimeout +const WorstCaseTickOverhead = 3*vcgencmdTimeout + defaultStatfsTimeout // Collector periodically samples every metric source and keeps the latest // snapshot plus a bounded in-memory history per metric. @@ -454,6 +458,7 @@ type fastTickSamples struct { load LoadAverage temp Temperature gpuTemp *GPUTemperature + pmicTemp *PMICTemperature tempErr error throttled *Throttled mem Memory @@ -491,7 +496,7 @@ func (c *Collector) collectFastTickSamples(ctx context.Context) fastTickSamples if err != nil { c.log.Warn("load average collection failed", "error", err) } - s.temp, s.gpuTemp, s.tempErr = c.temp.Collect(ctx) + s.temp, s.gpuTemp, s.pmicTemp, s.tempErr = c.temp.Collect(ctx) if s.tempErr != nil { c.log.Warn("temperature collection failed", "error", s.tempErr) } @@ -548,6 +553,7 @@ func (c *Collector) fastTick(ctx context.Context) { c.latest.Temperature = s.temp c.latest.TemperatureValid = s.tempErr == nil c.latest.GPUTemperature = s.gpuTemp + c.latest.PMICTemperature = s.pmicTemp c.latest.Throttled = s.throttled c.latest.Memory = s.mem c.latest.Swap = s.swap diff --git a/internal/collector/collector_test.go b/internal/collector/collector_test.go index 73e8774..88609c6 100644 --- a/internal/collector/collector_test.go +++ b/internal/collector/collector_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "time" @@ -337,6 +338,78 @@ func TestCollector_FastTick_Hwmon(t *testing.T) { } } +// fakeVcgencmdCollector points a Collector's two vcgencmd-backed +// collectors at one fake binary that logs every invocation to countFile, +// so a test can assert both what a tick reads and how many firmware calls +// it costs. The fake answers get_throttled, measure_temp and measure_temp +// pmic the way a Pi 4/5 would. +func fakeVcgencmdCollector(t *testing.T, c *Collector) (countFile string) { + t.Helper() + dir := t.TempDir() + countFile = filepath.Join(dir, "invocations") + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", `echo "$@" >> `+countFile+` +if [ "$1" = "get_throttled" ]; then + echo "throttled=0x0" +elif [ "$2" = "pmic" ]; then + echo "temp=52.1'C" +else + echo "temp=42.8'C" +fi`) + vcg := &vcgencmdRunner{detected: true, path: path} + + zoneRoot := t.TempDir() + writeThermalZone(t, zoneRoot, "thermal_zone0", "cpu-thermal", "50000") + c.temp = &TemperatureCollector{ + zonePath: filepath.Join(zoneRoot, "thermal_zone0"), + zoneType: "cpu-thermal", + vcg: vcg, + } + c.throttled = &ThrottledCollector{vcg: vcg} + return countFile +} + +// TestCollector_FastTick_PMICTemperature covers issue #56's wiring: the +// PMIC reading collected alongside the GPU/SoC one must reach the +// published snapshot. +func TestCollector_FastTick_PMICTemperature(t *testing.T) { + c := newTestCollector() + fakeVcgencmdCollector(t, c) + + c.fastTick(context.Background()) + + snap := c.Snapshot() + if snap.GPUTemperature == nil || snap.GPUTemperature.Celsius != 42.8 { + t.Fatalf("GPUTemperature = %+v, want Celsius=42.8", snap.GPUTemperature) + } + if snap.PMICTemperature == nil || snap.PMICTemperature.Celsius != 52.1 { + t.Fatalf("PMICTemperature = %+v, want Celsius=52.1", snap.PMICTemperature) + } +} + +// TestWorstCaseTickOverhead_CoversEveryVcgencmdInvocation keeps the +// /healthz staleness budget honest: every vcgencmd call a fast tick makes +// runs sequentially and can each stall for up to vcgencmdTimeout, so +// WorstCaseTickOverhead must budget for all of them. Adding a firmware +// call (issue #56 added `measure_temp pmic`) without widening the constant +// would make /healthz flap on a Pi whose only problem is slow firmware, so +// this counts the invocations rather than trusting the constant's comment. +func TestWorstCaseTickOverhead_CoversEveryVcgencmdInvocation(t *testing.T) { + c := newTestCollector() + countFile := fakeVcgencmdCollector(t, c) + + c.fastTick(context.Background()) + + logged, err := os.ReadFile(countFile) + if err != nil { + t.Fatalf("read vcgencmd invocation log: %v", err) + } + invocations := len(strings.Split(strings.TrimSpace(string(logged)), "\n")) + if want := time.Duration(invocations)*vcgencmdTimeout + defaultStatfsTimeout; WorstCaseTickOverhead < want { + t.Fatalf("WorstCaseTickOverhead = %v, want at least %v: a fast tick makes %d vcgencmd invocations (%s), each bounded by %v\ninvocations:\n%s", + WorstCaseTickOverhead, want, invocations, countFile, vcgencmdTimeout, logged) + } +} + // TestCollector_FastTick_HwmonDisabled verifies that HwmonEnabled: false // skips hwmon collection entirely, mirroring the network disabled test // above. diff --git a/internal/collector/temperature.go b/internal/collector/temperature.go index e2fe191..0355793 100644 --- a/internal/collector/temperature.go +++ b/internal/collector/temperature.go @@ -73,8 +73,8 @@ func readThermalZoneMilliC(zonePath string) (float64, error) { return float64(milliC) / 1000, nil } -// TemperatureCollector reads CPU temperature from sysfs, with an optional -// vcgencmd-sourced GPU/SoC reading on Raspberry Pi OS. +// TemperatureCollector reads CPU temperature from sysfs, with optional +// vcgencmd-sourced GPU/SoC and PMIC readings on Raspberry Pi OS. // // The thermal zone is resolved lazily and re-resolved (throttled) when it // is still missing, so a sensor or driver that appears after the process @@ -90,7 +90,18 @@ type TemperatureCollector struct { zonePath string zoneType string lastZoneDetect time.Time - vcg *vcgencmdRunner // nil disables the GPU/SoC reading + vcg *vcgencmdRunner // nil disables the GPU/SoC and PMIC readings + + // pmicUnsupported latches true once vcgencmd has run successfully but + // answered `measure_temp pmic` with something other than a temperature + // reading (see errVcgencmdUnsupportedOutput) — in practice, the board + // has no PMIC sensor, a Pi 3 and earlier. Unlike a failed exec or a + // timeout, which are transient and worth retrying, that outcome can + // never become false at runtime, so latching it avoids paying a + // pointless vcgencmd invocation on every fast tick for the rest of the + // process's life. A real exec/timeout failure does not set this flag + // and keeps retrying, same as the GPU/SoC reading. + pmicUnsupported bool } // NewTemperatureCollector auto-detects the CPU thermal zone. Detection @@ -99,7 +110,8 @@ type TemperatureCollector struct { // development off-Pi). If the zone is missing at construction, Collect // re-attempts detection at most once every detectRetryInterval, so a sensor // that shows up later is used automatically. vcg is the vcgencmd runner -// shared with ThrottledCollector; pass nil to disable the GPU/SoC reading. +// shared with ThrottledCollector; pass nil to disable the GPU/SoC and PMIC +// readings. func NewTemperatureCollector(vcg *vcgencmdRunner) *TemperatureCollector { c := &TemperatureCollector{zoneGlob: thermalZoneGlob, now: time.Now, vcg: vcg} c.redetectZoneLocked() @@ -125,8 +137,8 @@ func (c *TemperatureCollector) redetectZoneLocked() { } // Collect returns the current CPU temperature and, if vcgencmd is -// available, the GPU/SoC temperature as a secondary reading. -func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTemperature, error) { +// available, the GPU/SoC and PMIC temperatures as secondary readings. +func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTemperature, *PMICTemperature, error) { c.mu.Lock() defer c.mu.Unlock() @@ -137,7 +149,7 @@ func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTe c.redetectZoneLocked() if c.zonePath == "" { - return Temperature{}, nil, fmt.Errorf("no CPU thermal zone detected") + return Temperature{}, nil, nil, fmt.Errorf("no CPU thermal zone detected") } celsius, err := readThermalZoneMilliC(c.zonePath) if err != nil { @@ -151,44 +163,75 @@ func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTe } } if err != nil { - return Temperature{}, nil, err + return Temperature{}, nil, nil, err } } temp := Temperature{Zone: c.zoneType, Celsius: celsius} - gpuTemp, err := c.readVcgencmdTemp(ctx) - if err != nil { - // vcgencmd is an optional extra data point; its unavailability or - // failure should not fail the whole collection. - return temp, nil, nil + // The vcgencmd readings are optional extra data points: unavailability + // or failure must fail neither the whole collection nor each other. The + // PMIC sensor in particular exists only on the Pi 4/5, so on older + // boards `measure_temp` succeeds while `measure_temp pmic` does not. + var gpuTemp *GPUTemperature + if gpuC, err := c.readVcgencmdTemp(ctx); err == nil { + gpuTemp = &GPUTemperature{Celsius: gpuC} + } + var pmicTemp *PMICTemperature + if !c.pmicUnsupported { + pmicC, err := c.readVcgencmdTemp(ctx, "pmic") + switch { + case err == nil: + pmicTemp = &PMICTemperature{Celsius: pmicC} + case errors.Is(err, errVcgencmdUnsupportedOutput): + // vcgencmd ran and answered, just not with a PMIC reading: + // this board has no PMIC sensor, which cannot change at + // runtime. Stop asking. + c.pmicUnsupported = true + } } - return temp, &gpuTemp, nil + return temp, gpuTemp, pmicTemp, nil } -// readVcgencmdTemp runs `vcgencmd measure_temp` (via the shared vcg runner) -// and parses output of the form "temp=42.8'C". -func (c *TemperatureCollector) readVcgencmdTemp(ctx context.Context) (GPUTemperature, error) { +// readVcgencmdTemp runs `vcgencmd measure_temp [args...]` (via the shared +// vcg runner) and parses output of the form "temp=42.8'C". args carries the +// subcommand's own arguments: none for the GPU/SoC die reading, "pmic" for +// the Power-Management IC's own sensor. +func (c *TemperatureCollector) readVcgencmdTemp(ctx context.Context, args ...string) (float64, error) { if c.vcg == nil { - return GPUTemperature{}, errVcgencmdUnavailable + return 0, errVcgencmdUnavailable } - out, err := c.vcg.run(ctx, "measure_temp") + out, err := c.vcg.run(ctx, "measure_temp", args...) if err != nil { - return GPUTemperature{}, err + return 0, err } return parseVcgencmdTemp(out) } -func parseVcgencmdTemp(output string) (GPUTemperature, error) { +// errVcgencmdUnsupportedOutput indicates vcgencmd executed successfully but +// answered with something other than a "temp=NN.N'C" line — its +// "error=1 error_msg=..." response, or any other output parseVcgencmdTemp +// doesn't recognize. For a sensor-specific subcommand (measure_temp pmic) +// this means the requested sensor does not exist on this board: a +// permanent condition for the life of the process, unlike a failed exec or +// a timeout, which are transient and worth retrying. Callers that want to +// tell the two apart (see TemperatureCollector.pmicUnsupported) check for +// this with errors.Is. +var errVcgencmdUnsupportedOutput = errors.New("vcgencmd: output is not a temperature reading") + +// parseVcgencmdTemp decodes vcgencmd's "temp=NN.N'C" output into degrees +// Celsius. The form is identical for every measure_temp variant, so the +// GPU/SoC and PMIC readings share this parser. +func parseVcgencmdTemp(output string) (float64, error) { output = strings.TrimSpace(output) const prefix = "temp=" if !strings.HasPrefix(output, prefix) { - return GPUTemperature{}, fmt.Errorf("unexpected vcgencmd output: %q", output) + return 0, fmt.Errorf("%w: unexpected vcgencmd output: %q", errVcgencmdUnsupportedOutput, output) } rest := strings.TrimPrefix(output, prefix) rest = strings.TrimSuffix(rest, "'C") celsius, err := strconv.ParseFloat(rest, 64) if err != nil { - return GPUTemperature{}, fmt.Errorf("parse vcgencmd temp %q: %w", output, err) + return 0, fmt.Errorf("%w: parse vcgencmd temp %q: %w", errVcgencmdUnsupportedOutput, output, err) } - return GPUTemperature{Celsius: celsius}, nil + return celsius, nil } diff --git a/internal/collector/temperature_test.go b/internal/collector/temperature_test.go index 9392ff1..e6686c1 100644 --- a/internal/collector/temperature_test.go +++ b/internal/collector/temperature_test.go @@ -2,8 +2,10 @@ package collector import ( "context" + "errors" "os" "path/filepath" + "strings" "testing" "time" ) @@ -69,19 +71,46 @@ func TestReadThermalZoneMilliC(t *testing.T) { } } +// TestParseVcgencmdTemp covers every output shape parseVcgencmdTemp must +// handle: the plain GPU/SoC die reading, the PMIC reading (issue #56, +// identical "temp=NN.N'C" form so both share this parser), and the two +// ways firmware reports "not a temperature" — an explicit error line when +// the requested sensor doesn't exist on this board, and output that is +// simply unparseable. Both error cases must be errVcgencmdUnsupportedOutput +// so TemperatureCollector.Collect can tell "no such sensor" (latch, stop +// asking) apart from a transient exec/timeout failure (keep retrying). func TestParseVcgencmdTemp(t *testing.T) { - got, err := parseVcgencmdTemp("temp=42.8'C\n") - if err != nil { - t.Fatalf("parseVcgencmdTemp: %v", err) - } - if diffFloat(got.Celsius, 42.8) > 0.001 { - t.Fatalf("Celsius = %v, want 42.8", got.Celsius) - } -} - -func TestParseVcgencmdTemp_Malformed(t *testing.T) { - if _, err := parseVcgencmdTemp("garbage output"); err == nil { - t.Fatal("expected error for malformed vcgencmd output") + tests := []struct { + name string + output string + wantTemp float64 + wantErr bool + }{ + {name: "GPU/SoC die reading", output: "temp=42.8'C\n", wantTemp: 42.8}, + {name: "PMIC reading, same form as the die reading", output: "temp=52.1'C\n", wantTemp: 52.1}, + {name: "unsupported sensor (Pi 3 asked for pmic)", output: `error=1 error_msg="Invalid arguments"`, wantErr: true}, + {name: "malformed output", output: "garbage output", wantErr: true}, + {name: "temp= prefix with an unparseable number", output: "temp=N/A'C", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseVcgencmdTemp(tt.output) + if tt.wantErr { + if err == nil { + t.Fatal("expected an error, got nil") + } + if !errors.Is(err, errVcgencmdUnsupportedOutput) { + t.Fatalf("error = %v, want errVcgencmdUnsupportedOutput", err) + } + return + } + if err != nil { + t.Fatalf("parseVcgencmdTemp: %v", err) + } + if diffFloat(got, tt.wantTemp) > 0.001 { + t.Fatalf("celsius = %v, want %v", got, tt.wantTemp) + } + }) } } @@ -90,7 +119,7 @@ func TestTemperatureCollector_Collect(t *testing.T) { writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000") c := &TemperatureCollector{zonePath: filepath.Join(root, "thermal_zone0"), zoneType: "cpu-thermal"} - temp, gpuTemp, err := c.Collect(context.Background()) + temp, gpuTemp, pmicTemp, err := c.Collect(context.Background()) if err != nil { t.Fatalf("Collect: %v", err) } @@ -100,6 +129,9 @@ func TestTemperatureCollector_Collect(t *testing.T) { if gpuTemp != nil { t.Fatalf("expected no GPU temp when vcgencmd is not configured, got %+v", gpuTemp) } + if pmicTemp != nil { + t.Fatalf("expected no PMIC temp when vcgencmd is not configured, got %+v", pmicTemp) + } } func TestTemperatureCollector_Collect_WithGPUTemp(t *testing.T) { @@ -113,7 +145,7 @@ func TestTemperatureCollector_Collect_WithGPUTemp(t *testing.T) { zoneType: "cpu-thermal", vcg: &vcgencmdRunner{detected: true, path: path}, } - temp, gpuTemp, err := c.Collect(context.Background()) + temp, gpuTemp, _, err := c.Collect(context.Background()) if err != nil { t.Fatalf("Collect: %v", err) } @@ -125,6 +157,168 @@ func TestTemperatureCollector_Collect_WithGPUTemp(t *testing.T) { } } +// pmicAwareVcgencmd writes a fake vcgencmd whose `measure_temp pmic` +// invocation answers differently from the plain `measure_temp`, mirroring a +// board where both sensors exist (Pi 4/5) or only the die sensor does +// (Pi 3 and earlier). pmicScript is the body run for the pmic variant. +func pmicAwareVcgencmd(t *testing.T, dir, pmicScript string) string { + t.Helper() + return writeFakeVcgencmd(t, dir, "fake-vcgencmd", `if [ "$2" = "pmic" ]; then +`+pmicScript+` +else + echo "temp=42.8'C" +fi`) +} + +// TestTemperatureCollector_Collect_PMIC covers every way `measure_temp +// pmic` can answer: a genuine reading (Pi 4/5), the "sensor not present" +// error line (Pi 3 and earlier), and the invocation simply failing outright +// (a firmware that fails the exec rather than printing an error line). In +// every case the GPU/SoC reading and the overall collection must survive +// undisturbed; only the PMIC field's presence differs. +func TestTemperatureCollector_Collect_PMIC(t *testing.T) { + tests := []struct { + name string + pmicScript string + wantPMIC bool + wantPMICC float64 + }{ + { + name: "PMIC sensor present (Pi 4/5)", + pmicScript: ` echo "temp=52.1'C"`, + wantPMIC: true, + wantPMICC: 52.1, + }, + { + name: "PMIC unsupported: error line (Pi 3 and earlier)", + pmicScript: ` echo 'error=1 error_msg="Invalid arguments"'`, + wantPMIC: false, + }, + { + name: "PMIC invocation exits non-zero", + pmicScript: " exit 1", + wantPMIC: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertPMICCollect(t, tt.pmicScript, tt.wantPMIC, tt.wantPMICC) + }) + } +} + +// assertPMICCollect runs Collect against a fake vcgencmd whose `measure_temp +// pmic` invocation is scripted by pmicScript, and checks the three things +// every TestTemperatureCollector_Collect_PMIC case cares about: the die +// reading is unaffected, the GPU/SoC reading always survives whatever the +// PMIC invocation did, and the PMIC field itself is present with wantPMICC +// (if wantPMIC) or absent (if not). Split out of the table-driven test's +// t.Run closure to keep that function's (and this one's) cognitive +// complexity down — SonarQube counts a closure's branching into its +// enclosing function, and the combined for-loop-plus-closure had crept past +// the default threshold of 15. +func assertPMICCollect(t *testing.T, pmicScript string, wantPMIC bool, wantPMICC float64) { + t.Helper() + root := t.TempDir() + writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000") + path := pmicAwareVcgencmd(t, t.TempDir(), pmicScript) + + c := &TemperatureCollector{ + zonePath: filepath.Join(root, "thermal_zone0"), + zoneType: "cpu-thermal", + vcg: &vcgencmdRunner{detected: true, path: path}, + } + temp, gpuTemp, pmicTemp, err := c.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if diffFloat(temp.Celsius, 50.0) > 0.001 { + t.Fatalf("Celsius = %v, want 50.0", temp.Celsius) + } + if gpuTemp == nil || diffFloat(gpuTemp.Celsius, 42.8) > 0.001 { + t.Fatalf("gpuTemp = %+v, want Celsius=42.8 (the die reading must survive whatever the PMIC invocation did)", gpuTemp) + } + if !wantPMIC { + if pmicTemp != nil { + t.Fatalf("pmicTemp = %+v, want nil", pmicTemp) + } + return + } + if pmicTemp == nil || diffFloat(pmicTemp.Celsius, wantPMICC) > 0.001 { + t.Fatalf("pmicTemp = %+v, want Celsius=%v", pmicTemp, wantPMICC) + } +} + +// TestTemperatureCollector_Collect_PMICUnsupportedLatches guards the "stop +// trying" latch: once vcgencmd has answered `measure_temp pmic` with a +// non-temperature response, a board without a PMIC sensor must not pay +// another such invocation on every subsequent fast tick for the rest of +// the process's life — the condition cannot change at runtime. +func TestTemperatureCollector_Collect_PMICUnsupportedLatches(t *testing.T) { + scriptDir := t.TempDir() + logFile := filepath.Join(scriptDir, "pmic-invocations") + root := t.TempDir() + writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000") + path := pmicAwareVcgencmd(t, scriptDir, ` echo "$@" >> `+logFile+` + echo 'error=1 error_msg="Invalid arguments"'`) + + c := &TemperatureCollector{ + zonePath: filepath.Join(root, "thermal_zone0"), + zoneType: "cpu-thermal", + vcg: &vcgencmdRunner{detected: true, path: path}, + } + + const ticks = 3 + for i := 0; i < ticks; i++ { + _, _, pmicTemp, err := c.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect #%d: %v", i, err) + } + if pmicTemp != nil { + t.Fatalf("Collect #%d: pmicTemp = %+v, want nil", i, pmicTemp) + } + } + + if !c.pmicUnsupported { + t.Fatal("expected pmicUnsupported to latch true after an unsupported-output response") + } + + logged, err := os.ReadFile(logFile) + if err != nil { + t.Fatalf("read pmic invocation log (expected exactly one invocation): %v", err) + } + if invocations := len(strings.Split(strings.TrimSpace(string(logged)), "\n")); invocations != 1 { + t.Fatalf("measure_temp pmic was invoked %d times across %d Collect() calls, want 1 (the latch should suppress the rest):\n%s", + invocations, ticks, logged) + } +} + +// TestTemperatureCollector_Collect_PMICTransientFailureKeepsRetrying is the +// counterpart to the latch test above: a failed exec (as opposed to a +// successful one with unsupported output) must not latch, since it may +// well succeed on the next tick. +func TestTemperatureCollector_Collect_PMICTransientFailureKeepsRetrying(t *testing.T) { + root := t.TempDir() + writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000") + path := pmicAwareVcgencmd(t, t.TempDir(), " exit 1") + + c := &TemperatureCollector{ + zonePath: filepath.Join(root, "thermal_zone0"), + zoneType: "cpu-thermal", + vcg: &vcgencmdRunner{detected: true, path: path}, + } + + for i := 0; i < 2; i++ { + if _, _, _, err := c.Collect(context.Background()); err != nil { + t.Fatalf("Collect #%d: %v", i, err) + } + } + + if c.pmicUnsupported { + t.Fatal("a failed exec must not latch pmicUnsupported: the same board may still have a PMIC sensor") + } +} + func TestTemperatureCollector_Collect_VcgencmdExecFails(t *testing.T) { root := t.TempDir() writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000") @@ -136,7 +330,7 @@ func TestTemperatureCollector_Collect_VcgencmdExecFails(t *testing.T) { zoneType: "cpu-thermal", vcg: &vcgencmdRunner{detected: true, path: path}, } - temp, gpuTemp, err := c.Collect(context.Background()) + temp, gpuTemp, pmicTemp, err := c.Collect(context.Background()) if err != nil { t.Fatalf("Collect: %v", err) } @@ -146,11 +340,14 @@ func TestTemperatureCollector_Collect_VcgencmdExecFails(t *testing.T) { if gpuTemp != nil { t.Fatalf("expected no GPU temp when vcgencmd exec fails, got %+v", gpuTemp) } + if pmicTemp != nil { + t.Fatalf("expected no PMIC temp when vcgencmd exec fails, got %+v", pmicTemp) + } } func TestTemperatureCollector_Collect_NoZoneDetected(t *testing.T) { c := &TemperatureCollector{} - if _, _, err := c.Collect(context.Background()); err == nil { + if _, _, _, err := c.Collect(context.Background()); err == nil { t.Fatal("expected error when no thermal zone was detected") } } @@ -167,7 +364,7 @@ func TestTemperatureCollector_Collect_RedetectsZone(t *testing.T) { } // No zone exists yet: Collect must fail. - if _, _, err := c.Collect(context.Background()); err == nil { + if _, _, _, err := c.Collect(context.Background()); err == nil { t.Fatal("expected error when no thermal zone exists yet") } @@ -176,13 +373,13 @@ func TestTemperatureCollector_Collect_RedetectsZone(t *testing.T) { // Still within the throttle window: re-detection is suppressed. now = now.Add(detectRetryInterval - time.Second) - if _, _, err := c.Collect(context.Background()); err == nil { + if _, _, _, err := c.Collect(context.Background()); err == nil { t.Fatal("expected re-detection to be throttled within detectRetryInterval") } // Past the throttle window: the same collector now picks up the zone. now = now.Add(2 * time.Second) - temp, _, err := c.Collect(context.Background()) + temp, _, _, err := c.Collect(context.Background()) if err != nil { t.Fatalf("Collect after zone appeared: %v", err) } @@ -204,7 +401,7 @@ func TestTemperatureCollector_Collect_RedetectsAfterZoneVanishes(t *testing.T) { // A zone exists at first and is cached by Collect. writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "40000") - if temp, _, err := c.Collect(context.Background()); err != nil { + if temp, _, _, err := c.Collect(context.Background()); err != nil { t.Fatalf("initial Collect: %v", err) } else if temp.Zone != "cpu-thermal" { t.Fatalf("initial zone = %q, want cpu-thermal", temp.Zone) @@ -220,13 +417,13 @@ func TestTemperatureCollector_Collect_RedetectsAfterZoneVanishes(t *testing.T) { // Within the throttle window the collector cannot re-detect yet, so the // stale path keeps failing (documents that throttling covers this path too). now = now.Add(detectRetryInterval - time.Second) - if _, _, err := c.Collect(context.Background()); err == nil { + if _, _, _, err := c.Collect(context.Background()); err == nil { t.Fatal("expected error while re-detection is throttled after the zone vanished") } // Past the window the same collector recovers onto the new zone. now = now.Add(2 * time.Second) - temp, _, err := c.Collect(context.Background()) + temp, _, _, err := c.Collect(context.Background()) if err != nil { t.Fatalf("Collect after zone moved: %v", err) } diff --git a/internal/collector/types.go b/internal/collector/types.go index 323067f..bde5881 100644 --- a/internal/collector/types.go +++ b/internal/collector/types.go @@ -38,6 +38,17 @@ type GPUTemperature struct { Celsius float64 `json:"celsius"` } +// PMICTemperature is the optional vcgencmd-sourced temperature of the +// Power-Management IC. Unlike GPUTemperature — which reads the same +// physical die sensor as Temperature, since CPU and GPU share the SoC die +// — the PMIC is a genuinely separate on-board sensor, and it exists only +// on the Raspberry Pi 4 and 5. Raspberry Pi OS does not expose it through +// sysfs hwmon, so TemperatureSensor below never carries it; `vcgencmd +// measure_temp pmic` is the only way to read it. +type PMICTemperature struct { + Celsius float64 `json:"celsius"` +} + // TemperatureSensor is a single reading enumerated from the kernel's hwmon // subsystem: the SoC sensor itself, or a genuinely separate sensor (a // PoE-HAT fan controller, an NVMe/SSD drive, a user-attached I2C/1-Wire @@ -193,6 +204,7 @@ type Snapshot struct { // already draws for the alert engine. TemperatureValid bool `json:"-"` GPUTemperature *GPUTemperature `json:"gpu_temperature,omitempty"` + PMICTemperature *PMICTemperature `json:"pmic_temperature,omitempty"` Sensors []TemperatureSensor `json:"sensors,omitempty"` Throttled *Throttled `json:"throttled,omitempty"` Memory Memory `json:"memory"` diff --git a/internal/collector/vcgencmd.go b/internal/collector/vcgencmd.go index 6314290..25b040a 100644 --- a/internal/collector/vcgencmd.go +++ b/internal/collector/vcgencmd.go @@ -70,10 +70,12 @@ func (r *vcgencmdRunner) redetectLocked() { } } -// run executes `vcgencmd ` with a bounded timeout and returns -// its trimmed stdout. It returns errVcgencmdUnavailable, without attempting -// an exec, if vcgencmd has not been detected. -func (r *vcgencmdRunner) run(ctx context.Context, subcommand string) (string, error) { +// run executes `vcgencmd [args...]` with a bounded timeout and +// returns its trimmed stdout. args carries a subcommand's own arguments, +// e.g. the "pmic" in `vcgencmd measure_temp pmic`. It returns +// errVcgencmdUnavailable, without attempting an exec, if vcgencmd has not +// been detected. +func (r *vcgencmdRunner) run(ctx context.Context, subcommand string, args ...string) (string, error) { r.mu.Lock() r.redetectLocked() path := r.path @@ -86,7 +88,7 @@ func (r *vcgencmdRunner) run(ctx context.Context, subcommand string) (string, er ctx, cancel := context.WithTimeout(ctx, vcgencmdTimeout) defer cancel() - cmd := exec.CommandContext(ctx, path, subcommand) + cmd := exec.CommandContext(ctx, path, append([]string{subcommand}, args...)...) // Build the child environment explicitly rather than inheriting the // service's: PiMonitor's own environment may carry PIMONITOR_API_KEY, and // there is no reason for that secret to be visible in vcgencmd's @@ -96,7 +98,7 @@ func (r *vcgencmdRunner) run(ctx context.Context, subcommand string) (string, er out, err := cmd.Output() if err != nil { - return "", fmt.Errorf("run vcgencmd %s: %w", subcommand, err) + return "", fmt.Errorf("run vcgencmd %s: %w", strings.Join(append([]string{subcommand}, args...), " "), err) } return strings.TrimSpace(string(out)), nil } diff --git a/internal/collector/vcgencmd_test.go b/internal/collector/vcgencmd_test.go index ccb8a6b..ac801d7 100644 --- a/internal/collector/vcgencmd_test.go +++ b/internal/collector/vcgencmd_test.go @@ -77,6 +77,25 @@ func TestVcgencmdRunner_Run_ExecutesSubcommand(t *testing.T) { } } +// TestVcgencmdRunner_Run_PassesSubcommandArguments pins that a +// subcommand's own arguments reach the binary as separate argv entries: +// `measure_temp pmic` only reads the Power-Management IC's sensor (issue +// #56) if the "pmic" argument is actually passed through. +func TestVcgencmdRunner_Run_PassesSubcommandArguments(t *testing.T) { + dir := t.TempDir() + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", `echo "argv: $1|$2"`) + r := &vcgencmdRunner{detected: true, path: path} + + out, err := r.run(context.Background(), "measure_temp", "pmic") + + if err != nil { + t.Fatalf("run: %v", err) + } + if out != "argv: measure_temp|pmic" { + t.Fatalf("run() output = %q, want %q", out, "argv: measure_temp|pmic") + } +} + func TestVcgencmdRunner_Run_CommandFails(t *testing.T) { dir := t.TempDir() path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", "exit 1") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 16d306d..4882fc2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -95,7 +95,7 @@ func TestHealthzMaxStaleness(t *testing.T) { name: "default adds 2x tick overhead on top of the poll-interval multiple", pollSeconds: 5, staleSeconds: 0, - tickOverhead: 12 * time.Second, // collector.WorstCaseTickOverhead in production + tickOverhead: 12 * time.Second, // a non-zero overhead, as collector.WorstCaseTickOverhead is in production want: 15*time.Second + 24*time.Second, }, { diff --git a/internal/httpapi/prometheus.go b/internal/httpapi/prometheus.go index 939ed50..80b2d64 100644 --- a/internal/httpapi/prometheus.go +++ b/internal/httpapi/prometheus.go @@ -30,6 +30,7 @@ func renderPrometheusMetrics(snap collector.Snapshot) []byte { writeCPUMetrics(&buf, snap.CPU) writeTemperatureMetrics(&buf, snap) writeGPUTemperatureMetrics(&buf, snap.GPUTemperature) + writePMICTemperatureMetrics(&buf, snap.PMICTemperature) writeMemoryMetrics(&buf, snap.Memory) writeSwapMetrics(&buf, snap.Swap) writeDiskMetrics(&buf, snap.Disks) @@ -85,6 +86,20 @@ func writeGPUTemperatureMetrics(buf *bytes.Buffer, gpuTemp *collector.GPUTempera writeMetric(buf, "pimonitor_gpu_temperature_celsius", "", "", gpuTemp.Celsius) } +// writePMICTemperatureMetrics renders the Power-Management IC reading, +// which — unlike the GPU/SoC one, a second view of the same die sensor as +// pimonitor_temperature_celsius — is a physically separate sensor. It is +// present only on a Pi 4/5 whose vcgencmd answered `measure_temp pmic`, and +// like every other optional family the whole family is skipped when there +// is no reading, rather than reporting a fabricated 0. +func writePMICTemperatureMetrics(buf *bytes.Buffer, pmicTemp *collector.PMICTemperature) { + if pmicTemp == nil { + return + } + writeGaugeHeader(buf, "pimonitor_pmic_temperature_celsius", "PMIC temperature in Celsius (vcgencmd, Pi 4/5 only).") + writeMetric(buf, "pimonitor_pmic_temperature_celsius", "", "", pmicTemp.Celsius) +} + func writeMemoryMetrics(buf *bytes.Buffer, mem collector.Memory) { writeGaugeHeader(buf, "pimonitor_memory_total_bytes", "Total RAM in bytes.") writeMetric(buf, "pimonitor_memory_total_bytes", "", "", float64(mem.TotalBytes)) diff --git a/internal/httpapi/prometheus_test.go b/internal/httpapi/prometheus_test.go index 0e23270..7b6d7c5 100644 --- a/internal/httpapi/prometheus_test.go +++ b/internal/httpapi/prometheus_test.go @@ -9,7 +9,7 @@ import ( ) // fullSnapshot exercises every field renderPrometheusMetrics knows about, -// including the optional/per-device ones (GPU temperature, multiple CPU +// including the optional/per-device ones (GPU and PMIC temperature, multiple CPU // cores, disks, network interfaces). func fullSnapshot() collector.Snapshot { return collector.Snapshot{ @@ -20,6 +20,7 @@ func fullSnapshot() collector.Snapshot { Temperature: collector.Temperature{Zone: "cpu-thermal", Celsius: 48.6}, TemperatureValid: true, GPUTemperature: &collector.GPUTemperature{Celsius: 47.8}, + PMICTemperature: &collector.PMICTemperature{Celsius: 52.1}, Memory: collector.Memory{TotalBytes: 4137000000, AvailableBytes: 2900000000, UsedPercent: 29.9}, Swap: collector.Swap{TotalBytes: 104857600, UsedBytes: 0, UsedPercent: 0}, Disks: []collector.Disk{ @@ -44,6 +45,7 @@ func TestRenderPrometheusMetrics_LabelsAndValues(t *testing.T) { {"per-core CPU gauge, core 1", `pimonitor_cpu_core_usage_percent{core="1"} 15`}, {"temperature gauge with zone label", `pimonitor_temperature_celsius{zone="cpu-thermal"} 48.6`}, {"GPU temperature gauge", "pimonitor_gpu_temperature_celsius 47.8"}, + {"PMIC temperature gauge", "pimonitor_pmic_temperature_celsius 52.1"}, {"memory total bytes", "pimonitor_memory_total_bytes 4137000000"}, {"memory available bytes", "pimonitor_memory_available_bytes 2900000000"}, {"memory used percent", "pimonitor_memory_used_percent 29.9"}, @@ -90,7 +92,7 @@ func TestRenderPrometheusMetrics_HelpAndTypeComments(t *testing.T) { // TestRenderPrometheusMetrics_OmitsAbsentOptionalFields guards the same // omit-when-absent behavior GET /api/v1/metrics already documents: no GPU -// temperature without vcgencmd, no network section when monitoring is +// or PMIC temperature without vcgencmd, no network section when monitoring is // disabled (empty slice), no disk section without any mounted filesystem, // and no per-core CPU family without per-core data. func TestRenderPrometheusMetrics_OmitsAbsentOptionalFields(t *testing.T) { @@ -105,6 +107,7 @@ func TestRenderPrometheusMetrics_OmitsAbsentOptionalFields(t *testing.T) { for _, absent := range []string{ "pimonitor_cpu_core_usage_percent", "pimonitor_gpu_temperature_celsius", + "pimonitor_pmic_temperature_celsius", "pimonitor_disk_", "pimonitor_network_", } { diff --git a/internal/web/assets/app.js b/internal/web/assets/app.js index 6234484..7f11b76 100644 --- a/internal/web/assets/app.js +++ b/internal/web/assets/app.js @@ -238,7 +238,12 @@ setText('temp-value', 'n/a'); tempEl.className = 'metric-value'; } - setText('temp-gpu', snap.gpu_temperature ? 'GPU: ' + snap.gpu_temperature.celsius.toFixed(1) + ' °C' : ''); + // Secondary temperature readings, each present only when vcgencmd + // answered for it: the GPU/SoC die sensor, and the Pi 4/5 PMIC. + const tempExtras = []; + if (snap.gpu_temperature) tempExtras.push('GPU: ' + snap.gpu_temperature.celsius.toFixed(1) + ' °C'); + if (snap.pmic_temperature) tempExtras.push('PMIC: ' + snap.pmic_temperature.celsius.toFixed(1) + ' °C'); + setText('temp-gpu', tempExtras.join(' · ')); // Memory & swap (show absolute sizes alongside the percentage, like // the filesystem rows). diff --git a/internal/web/temperature_test.go b/internal/web/temperature_test.go index a6529cf..835c36e 100644 --- a/internal/web/temperature_test.go +++ b/internal/web/temperature_test.go @@ -25,3 +25,27 @@ func TestAppJS_TemperatureNAUsesZoneNotCelsius(t *testing.T) { t.Errorf("expected app.js to key the temperature \"n/a\" fallback off snap.temperature?.zone") } } + +// TestAppJS_RendersPMICTemperature guards issue #56's dashboard surface: +// the Temperature card's sub-line must show the Pi 4/5 PMIC reading +// alongside the GPU/SoC one, and must keep each of them conditional — both +// fields are omitted from the snapshot on hardware (or a host) that has no +// such sensor, and rendering "undefined °C" there would be worse than +// showing nothing. +func TestAppJS_RendersPMICTemperature(t *testing.T) { + data, err := assetsFS.ReadFile("assets/app.js") + if err != nil { + t.Fatalf("read app.js: %v", err) + } + src := string(data) + + if !strings.Contains(src, "snap.pmic_temperature") { + t.Errorf("expected app.js to render snap.pmic_temperature in the temperature card") + } + if !strings.Contains(src, "if (snap.pmic_temperature)") { + t.Errorf("expected app.js to gate the PMIC reading on the field being present") + } + if !strings.Contains(src, "if (snap.gpu_temperature)") { + t.Errorf("expected app.js to keep the GPU reading conditional and independent of the PMIC one") + } +}