From c9b620aa3ec6e30c3cbb0ffc6d9bd6c6cd9d9830 Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Sun, 16 Aug 2026 15:33:11 +0000 Subject: [PATCH 01/10] fix(sensor): classify Arm SoC sensors, and report a thermal limit Three defects, all measured on hardware. CPU AND GPU WERE UNIDENTIFIABLE ON ARM SoCs. classify() matched the chip name against an allow-list and consulted labels only for a short list of x86 spellings. On CIX Sky1 a single hwmon chip named scmi_sensors carries all 22 sensors, and CPU, GPU, NPU, VPU, DDR and PCB are distinguished only by their labels -- CPU_B0, GPU_AVE and so on. The chip name matches nothing in either list, so the monitor reported cpu=-1 gpu=-1 on that board: no CPU and no GPU temperature at all. Labels are now consulted for GPU as well as CPU, and "cpu"/"gpu" are accepted as label substrings for the same reason they are already accepted as chip substrings. Regulator labels ("CPU VRM") are excluded, because a VRM runs hotter than the die it feeds and hottest-wins would otherwise report it as the CPU. NO SENSOR CARRIED A LIMIT, so nothing could be drawn against one. SensorReading now exposes limit_millidegrees and a Severity banded by thermal margin. The limit comes from tempN_crit, else tempN_max, else the lowest critical trip point of the zone, else a per-kind fallback. The fallback is load-bearing rather than cosmetic: k10temp, scmi and cpu_thermal -- Ryzen, Sky1 and most Arm SoCs -- publish no limit of any kind, so limit-bearing sensors alone would leave the CPU uncoloured on exactly the machines people watch. Severity uses margin, not a fraction of the limit, so an NVMe with an 84 C trip is not called warm at its idle temperature. A thermal zone that duplicates an hwmon chip by name is still dropped, but its limit is now rescued first: Sky1 publishes four acpitz sensors with no tempN_crit and four identically named zones each carrying a 98 C critical trip, and discarding those threw away the only real limit on the machine. A SENTINEL COULD BECOME THE HOTTEST SENSOR. One host reported 65261850 -- 65261 C. Readings are now bounded above as well as below; -274000, seen on the same survey, was already excluded by the existing lower bound. Clocks additionally carry the maximum of the cpufreq policy they came from. Sky1 exposes five policies with five different maxima, so a single machine-wide maximum normalises the little cores against a big core ceiling. clocks_khz() is unchanged for existing callers; clocks() is new. cpuinfo-derived readings report their fraction as unknown rather than inventing a denominator. Eleven fixture tests added, each built from a shape observed on real hardware. The seven pre-existing tests still pass. --- src/system/sensor.vala | 369 ++++++++++++++++++++++++++++++++++++++--- tests/sensor_test.vala | 291 ++++++++++++++++++++++++++++++++ 2 files changed, 636 insertions(+), 24 deletions(-) diff --git a/src/system/sensor.vala b/src/system/sensor.vala index eb18a41..dfc1683 100644 --- a/src/system/sensor.vala +++ b/src/system/sensor.vala @@ -8,16 +8,117 @@ namespace Singularity { SYSTEM } + /** + * How close a sensor is to the temperature the hardware acts on. + * + * Banded by THERMAL MARGIN -- degrees still available before the limit -- + * and not by a fraction of it. A fraction misreads every device whose + * limit is not near 100 C: an NVMe with an 84 C critical trip would be + * called warm at 63 C, which is its ordinary idle temperature. + */ + public enum Severity { + NORMAL, + WARM, + HOT, + CRITICAL + } + + /** + * Turns a temperature plus a limit into a Severity. + * + * THE FALLBACK LIMITS ARE LOAD-BEARING, NOT DECORATION. Surveyed across + * eight fleet machines, the drivers that matter most are precisely the + * ones that advertise no limit at all: k10temp (Ryzen), scmi (CIX Sky1) + * and cpu_thermal (Pi and most Arm SoCs) publish tempN_input with no + * tempN_crit and no critical trip point. Colouring only the sensors that + * declare a limit would therefore leave the CPU -- the one sensor a user + * actually watches -- permanently uncoloured on both Arm and AMD. + */ + namespace Thresholds { + /** Margin at or above which a sensor is unremarkable. */ + public const int NORMAL_MARGIN_MILLIDEGREES = 25000; + /** Margin at or above which a sensor is warm but not yet notable. */ + public const int WARM_MARGIN_MILLIDEGREES = 12000; + + public int fallback_limit(SensorKind kind) { + switch (kind) { + // Tctl on Zen throttles at 95 and Arm SoC critical trips sit + // at 105, so 100 is the middle of the range the fleet has. + case SensorKind.CPU: return 100000; + // NVIDIA slows at 83 and shuts down in the low 90s; the + // Mali-G720 on Sky1 trips at 95. + case SensorKind.GPU: return 95000; + // Board, NIC and NVMe sensors. Anything with a real limit -- + // and an NVMe almost always publishes one -- overrides this. + default: return 85000; + } + } + + public Severity classify(int millidegrees, int limit_millidegrees) { + if (limit_millidegrees <= 0) { + return Severity.NORMAL; + } + int margin = limit_millidegrees - millidegrees; + if (margin <= 0) { + return Severity.CRITICAL; + } + if (margin < WARM_MARGIN_MILLIDEGREES) { + return Severity.HOT; + } + if (margin < NORMAL_MARGIN_MILLIDEGREES) { + return Severity.WARM; + } + return Severity.NORMAL; + } + } + /** One temperature sensor, as reported by the kernel. */ public class SensorReading : Object { public string label { get; private set; } public int millidegrees { get; private set; } public SensorKind kind { get; private set; } - public SensorReading(string label, int millidegrees, SensorKind kind) { + /** + * Temperature at which the hardware acts, in millidegrees. + * + * Taken from the driver when it publishes one and from + * Thresholds.fallback_limit() when it does not, so it is never zero + * and a caller can always draw a margin. + */ + public int limit_millidegrees { get; private set; } + + /** + * True when the driver published the limit, false when it came from + * Thresholds.fallback_limit(). + * + * Needed because a reported limit is worth keeping when two sources + * describe the same sensor -- see the dedup in refresh_internal(). + */ + public bool limit_is_reported { get; private set; } + + public Severity severity { get; private set; } + + /** Degrees still available before limit_millidegrees. */ + public int margin_millidegrees { + get { return limit_millidegrees - millidegrees; } + } + + /** + * The limit argument is optional so that every existing caller -- + * including the NVIDIA path, which has no sysfs node to read a limit + * from -- keeps compiling and falls back by kind. + */ + public SensorReading(string label, int millidegrees, SensorKind kind, + int limit_millidegrees = 0) { this.label = label; this.millidegrees = millidegrees; this.kind = kind; + this.limit_is_reported = limit_millidegrees > 0; + this.limit_millidegrees = this.limit_is_reported + ? limit_millidegrees + : Thresholds.fallback_limit(kind); + this.severity = Thresholds.classify(millidegrees, + this.limit_millidegrees); } } @@ -43,6 +144,40 @@ namespace Singularity { } } + /** + * One cpufreq policy's current clock, with the maximum that policy can + * reach. + * + * The maximum travels WITH the reading because it is not one number per + * machine. CIX Sky1 exposes five cpufreq policies with five different + * maxima, so 2.1 GHz is near-idle on one cluster and flat out on another. + * Normalising against a single machine-wide maximum would paint the + * little cores as permanently idle. + */ + public class ClockReading : Object { + public string label { get; private set; } + public int khz { get; private set; } + /** 0 when the policy publishes no maximum. */ + public int max_khz { get; private set; } + + public ClockReading(string label, int khz, int max_khz) { + this.label = label; + this.khz = khz; + this.max_khz = max_khz; + } + + /** 0.0 to 1.0 of this policy's maximum; -1.0 when unknown. */ + public double fraction { + get { + if (max_khz <= 0) { + return -1.0; + } + double f = (double) khz / (double) max_khz; + return f > 1.0 ? 1.0 : f; + } + } + } + /** * Reads temperatures, fan speeds and CPU clocks from sysfs. * @@ -105,6 +240,77 @@ namespace Singularity { */ public string sysfs_root { get; set; default = ""; } + /** + * Upper bound on a believable temperature, in millidegrees. + * + * MEASURED: a fleet host reported 65261850 -- 65261 C -- from a hwmon + * node. That is a sentinel, not a temperature, and without a ceiling + * it becomes the hottest sensor on the machine and pins every summary + * and every colour to itself. + */ + private const int MAX_PLAUSIBLE_MILLIDEGREES = 150000; + + /** + * The lower bound stays at "greater than zero": the same survey saw + * -274000, which is below absolute zero and so unambiguously a + * sentinel as well. + */ + private static bool plausible(int millidegrees) { + return millidegrees > 0 + && millidegrees <= MAX_PLAUSIBLE_MILLIDEGREES; + } + + /** + * The temperature this hwmon sensor is acted on at, or 0. + * + * tempN_crit first and tempN_max second: _crit is the hard limit, + * while _max is frequently a soft target the chip sits at under full + * load. Treating the soft one as critical would report a merely busy + * CPU as overheating. + */ + private int hwmon_limit(string base_path, string stem) { + foreach (string suffix in new string[] { "_crit", "_max" }) { + string? raw = read_first_line(base_path + "/" + stem + suffix); + if (raw == null) { + continue; + } + int value = int.parse(raw); + if (plausible(value)) { + return value; + } + } + return 0; + } + + /** + * The lowest critical trip point of a thermal zone, or 0. + * + * "critical" is preferred over "hot" because critical is the trip the + * kernel powers the machine off at, where hot is an intermediate + * notification. The LOWEST is taken because a zone may publish several + * and the first one reached is the one that matters. + */ + private int thermal_limit(string base_path) { + int best = 0; + for (int i = 0; i < 16; i++) { + string? trip_type = read_first_line( + "%s/trip_point_%d_type".printf(base_path, i)); + if (trip_type == null || trip_type.strip().down() != "critical") { + continue; + } + string? raw = read_first_line( + "%s/trip_point_%d_temp".printf(base_path, i)); + if (raw == null) { + continue; + } + int value = int.parse(raw); + if (plausible(value) && (best == 0 || value < best)) { + best = value; + } + } + return best; + } + private string hwmon_dir() { return sysfs_root + HWMON_DIR; } private string thermal_dir() { return sysfs_root + THERMAL_DIR; } private string cpufreq_dir() { return sysfs_root + CPUFREQ_DIR; } @@ -124,8 +330,34 @@ namespace Singularity { "armada_thermal", "imx_thermal", "sun4i-ts", "scpi-sensors" }; // hwmon label text that identifies a CPU package or core. + // + // Bare "cpu" is here for the same reason it is in CPU_CHIPS: a sensor + // that spells out CPU is a CPU sensor. It is load-bearing on every + // Arm SystemReady board, where the identity is in the LABEL and never + // in the chip name. MEASURED on CIX Sky1: one hwmon chip named + // scmi_sensors carries all 22 sensors, and the CPU ones are told apart + // only by their labels CPU_B0, CPU_B1, CPU_M0, CPU_M1. Without this + // the panel reported cpu=-1 on the SoC this distribution targets. private const string[] CPU_LABELS = { - "package id", "tctl", "tdie", "tccd", "core " + "package id", "tctl", "tdie", "tccd", "core ", "cpu" + }; + // hwmon label text that identifies a GPU. Same reasoning: on Sky1 the + // GPU is scmi_sensors GPU_AVE / GPU_top / GPU_btm. + private const string[] GPU_LABELS = { + "gpu" + }; + /* + * Labels that name the REGULATOR rather than the die. + * + * A desktop Super-I/O chip labels its VRM sensors "CPU VRM" and + * "GPU VRM". Those contain "cpu" and "gpu" but run hotter than the + * part they feed, so with hottest-wins they would be reported as the + * CPU temperature and overstate it. Excluded rather than ranked -- + * ranking would need a notion of which sensor is more authoritative, + * which sysfs does not provide. + */ + private const string[] NOT_DIE_LABELS = { + "vrm", "vrout", "vddq", "ambient" }; private const string[] GPU_CHIPS = { "amdgpu", "radeon", "nouveau", "i915", "xe", @@ -144,6 +376,7 @@ namespace Singularity { // Sysfs-derived readings only, without the NVIDIA set merged in. private SensorReading[] _base_readings = {}; private int[] _clocks_khz = {}; + private ClockReading[] _clocks = {}; private FanReading[] _fans = {}; private PowerReading[] _power = {}; private SensorReading[] _nvidia_readings = {}; @@ -208,6 +441,18 @@ namespace Singularity { return _clocks_khz; } + /** + * The same clocks, each carrying the maximum of the cpufreq policy it + * came from. Ordered to match clocks_khz(). + * + * Kept alongside clocks_khz() rather than replacing it: a bare kHz + * list is all a caller printing a number needs, and changing that + * return type would break every existing one. + */ + public ClockReading[] clocks() { + return _clocks; + } + /** * Fans that are actually turning. * @@ -285,8 +530,14 @@ namespace Singularity { if (matches_any(chip, CPU_CHIPS)) { return SensorKind.CPU; } - if (label != null && matches_any(label, CPU_LABELS)) { - return SensorKind.CPU; + if (label != null && !matches_any(label, NOT_DIE_LABELS)) { + // GPU before CPU, matching the order of the chip checks. + if (matches_any(label, GPU_LABELS)) { + return SensorKind.GPU; + } + if (matches_any(label, CPU_LABELS)) { + return SensorKind.CPU; + } } // Unknown is SYSTEM on purpose. See the class comment. return SensorKind.SYSTEM; @@ -320,7 +571,7 @@ namespace Singularity { continue; } int millidegrees = int.parse(raw); - if (millidegrees <= 0) { + if (!plausible(millidegrees)) { continue; } string stem = entry.substring(0, entry.length - "_input".length); @@ -328,7 +579,9 @@ namespace Singularity { string name = (label != null && label != "") ? "%s %s".printf(chip, label) : chip; - found += new SensorReading(name, millidegrees, classify(chip, label)); + found += new SensorReading(name, millidegrees, + classify(chip, label), + hwmon_limit(base_path, stem)); } } return found; @@ -354,10 +607,12 @@ namespace Singularity { continue; } int millidegrees = int.parse(raw); - if (millidegrees <= 0) { + if (!plausible(millidegrees)) { continue; } - found += new SensorReading(zone_type, millidegrees, classify(zone_type, null)); + found += new SensorReading(zone_type, millidegrees, + classify(zone_type, null), + thermal_limit(base_path)); } return found; } @@ -487,12 +742,19 @@ namespace Singularity { * absent (common on virtual machines and on x86 with no scaling driver) * or because it exists but is empty / has no readable scaling_cur_freq. */ - private int[] clocks_from_cpuinfo() { - int[] found = {}; + /** + * Clocks with no maximum attached: /proc/cpuinfo reports the current + * MHz and nothing else, so these readings are deliberately built with + * max_khz 0 and a caller must treat their fraction as unknown rather + * than inventing a denominator. + */ + private ClockReading[] clocks_from_cpuinfo() { + ClockReading[] found = {}; string? cpuinfo = read_first_line(cpuinfo_path()); if (cpuinfo == null) { return found; } + int index = 0; foreach (string line in cpuinfo.split("\n")) { if (!line.down().has_prefix("cpu mhz")) { continue; @@ -503,14 +765,38 @@ namespace Singularity { } int mhz = (int) double.parse(parts[1].strip()); if (mhz > 0) { - found += mhz * 1000; + found += new ClockReading("cpu%d".printf(index), mhz * 1000, 0); + index++; } } return found; } - private int[] collect_clocks() { - int[] found = {}; + /** + * This policy's ceiling, in kHz, or 0. + * + * cpuinfo_max_freq is the hardware maximum; scaling_max_freq is what + * the governor is currently allowed to use and can be lowered at + * runtime. The hardware number is the honest denominator -- against + * scaling_max_freq a thermally capped core would read as 100% busy. + */ + private int policy_max_khz(string policy_path) { + foreach (string name in new string[] { "cpuinfo_max_freq", + "scaling_max_freq" }) { + string? raw = read_first_line(policy_path + "/" + name); + if (raw == null) { + continue; + } + int khz = int.parse(raw); + if (khz > 0) { + return khz; + } + } + return 0; + } + + private ClockReading[] collect_clocks() { + ClockReading[] found = {}; Dir dir; try { dir = Dir.open(cpufreq_dir(), 0); @@ -522,13 +808,14 @@ namespace Singularity { if (!node.has_prefix("policy")) { continue; } - string? raw = read_first_line(cpufreq_dir() + "/" + node + "/scaling_cur_freq"); + string policy_path = cpufreq_dir() + "/" + node; + string? raw = read_first_line(policy_path + "/scaling_cur_freq"); if (raw == null) { continue; } int khz = int.parse(raw); if (khz > 0) { - found += khz; + found += new ClockReading(node, khz, policy_max_khz(policy_path)); } } // A present cpufreq directory can still yield nothing: no policy* @@ -582,7 +869,13 @@ namespace Singularity { } string name = fields[0].strip(); int celsius = int.parse(fields[1].strip()); - if (celsius <= 0) { + // The same ceiling as sysfs. nvidia-smi also reports the + // absolute die temperature on every generation here -- do NOT + // switch this query to temperature.gpu.tlimit, which on Ada is + // degrees BELOW the throttle point while Turing reports an + // absolute value, so one field would mean two different things + // across the fleet. + if (!plausible(celsius * 1000)) { continue; } found += new SensorReading(name, celsius * 1000, SensorKind.GPU); @@ -674,14 +967,34 @@ namespace Singularity { // Thermal zones that merely duplicate an hwmon chip are dropped. SensorReading[] found = collect_hwmon(); foreach (SensorReading zone in collect_thermal()) { + // The two interfaces do not carry the same information. + // MEASURED on CIX Sky1: hwmon publishes acpitz with no + // tempN_crit at all, while the identically named thermal zone + // publishes a 98 C critical trip. Dropping the zone outright + // therefore threw away the only real limit on the machine and + // left the sensor on a guess. + // + // The scan prefers an entry that still lacks a reported limit, + // rather than stopping at the first name match. Sky1 presents + // FOUR sensors all called acpitz and four zones to match; a + // first-match rule upgraded one of them and left the other + // three on the fallback, so the same sensor was drawn against + // two different limits. + int upgrade_index = -1; bool duplicate = false; - foreach (SensorReading existing in found) { - if (same_sensor(existing.label, zone.label)) { - duplicate = true; + for (int i = 0; i < found.length; i++) { + if (!same_sensor(found[i].label, zone.label)) { + continue; + } + duplicate = true; + if (!found[i].limit_is_reported && zone.limit_is_reported) { + upgrade_index = i; break; } } - if (!duplicate) { + if (upgrade_index >= 0) { + found[upgrade_index] = zone; + } else if (!duplicate) { found += zone; } } @@ -700,20 +1013,28 @@ namespace Singularity { _fans = collect_fans(); _power = collect_power(); - int[] clocks = collect_clocks(); + ClockReading[] clocks = collect_clocks(); // Highest first, so a caller can take element 0 as "the" clock. + // The whole reading is swapped, not the kHz alone: each carries + // its own policy maximum and separating the two would normalise a + // big core against a little core's ceiling. if (clocks.length > 1) { for (int i = 0; i < clocks.length; i++) { for (int j = i + 1; j < clocks.length; j++) { - if (clocks[j] > clocks[i]) { - int swap = clocks[i]; + if (clocks[j].khz > clocks[i].khz) { + ClockReading swap = clocks[i]; clocks[i] = clocks[j]; clocks[j] = swap; } } } } - _clocks_khz = clocks; + _clocks = clocks; + int[] khz_only = {}; + foreach (ClockReading clock in clocks) { + khz_only += clock.khz; + } + _clocks_khz = khz_only; publish_state(); } diff --git a/tests/sensor_test.vala b/tests/sensor_test.vala index c7d2de1..ca381c9 100644 --- a/tests/sensor_test.vala +++ b/tests/sensor_test.vala @@ -42,6 +42,36 @@ private void thermal_zone(int n, string type, int millidegrees) { write_file(Path.build_filename(dir, "temp"), "%d\n".printf(millidegrees)); } +/** Add a critical trip point to an existing thermal zone. */ +private void thermal_trip(int n, int idx, string trip_type, int millidegrees) { + string dir = Path.build_filename(fixture_root, "sys", "class", "thermal", + "thermal_zone%d".printf(n)); + write_file(Path.build_filename(dir, "trip_point_%d_type".printf(idx)), + trip_type + "\n"); + write_file(Path.build_filename(dir, "trip_point_%d_temp".printf(idx)), + "%d\n".printf(millidegrees)); +} + +/** Create /sys/devices/system/cpu/cpufreq/policyN. */ +private void cpufreq_policy(int n, int cur_khz, int max_khz) { + string dir = Path.build_filename(fixture_root, "sys", "devices", "system", + "cpu", "cpufreq", "policy%d".printf(n)); + write_file(Path.build_filename(dir, "scaling_cur_freq"), "%d\n".printf(cur_khz)); + if (max_khz > 0) { + write_file(Path.build_filename(dir, "cpuinfo_max_freq"), "%d\n".printf(max_khz)); + } +} + +private Singularity.SensorReading? reading_named(Singularity.SensorMonitor m, + string needle) { + foreach (Singularity.SensorReading r in m.readings()) { + if (needle in r.label) { + return r; + } + } + return null; +} + private void remove_path(File file) { try { var type = file.query_file_type(FileQueryInfoFlags.NOFOLLOW_SYMLINKS); @@ -196,6 +226,256 @@ private void test_available_with_fan_but_no_temperature() { assert(m.available); } + +/* + * A sentinel must not become the hottest sensor on the machine. + * + * MEASURED on a fleet host: a hwmon node reported 65261850 -- 65261 C. With no + * ceiling it wins every "hottest" comparison, pins the summary chip to itself + * and paints the panel permanently critical. -274000 is the same class of + * value from the other end, below absolute zero. + */ +private void test_sentinel_temperatures_are_rejected() { + reset_fixture(); + string junk = hwmon_chip(0, "k10temp"); + hwmon_temp(junk, 1, 65261850, "Tctl"); + hwmon_temp(junk, 2, -274000, "Tdie"); + hwmon_temp(junk, 3, 54000, "Tccd1"); + + var m = monitor_for_fixture(); + // Only the believable one survives, so it is also the hottest CPU. + assert(m.cpu_millidegrees == 54000); + assert(reading_named(m, "Tctl") == null); + assert(reading_named(m, "Tdie") == null); +} + +/* + * tempN_crit, when the driver publishes one, is the limit. + */ +private void test_hwmon_crit_becomes_the_limit() { + reset_fixture(); + string nvme = hwmon_chip(0, "nvme"); + hwmon_temp(nvme, 1, 48000, "Composite"); + write_file(Path.build_filename(nvme, "temp1_crit"), "84850\n"); + + var m = monitor_for_fixture(); + var r = reading_named(m, "Composite"); + assert(r != null); + assert(r.limit_millidegrees == 84850); + // 36 C of margin: unremarkable, despite being a drive. + assert(r.severity == Singularity.Severity.NORMAL); +} + +/* + * tempN_crit is preferred over tempN_max. + * + * _max is frequently a soft target a chip sits at under full load; treating it + * as critical reports a merely busy CPU as overheating. + */ +private void test_crit_preferred_over_max() { + reset_fixture(); + string cpu = hwmon_chip(0, "coretemp"); + hwmon_temp(cpu, 1, 82000, "Package id 0"); + write_file(Path.build_filename(cpu, "temp1_max"), "84000\n"); + write_file(Path.build_filename(cpu, "temp1_crit"), "100000\n"); + + var m = monitor_for_fixture(); + var r = reading_named(m, "Package id 0"); + assert(r != null); + assert(r.limit_millidegrees == 100000); + // 18 C of margin against the hard limit is warm. Had _max been taken as + // the limit the margin would have been 2 C and this busy-but-healthy CPU + // would have been reported as HOT -- which is the whole point of the + // preference. + assert(r.severity == Singularity.Severity.WARM); +} + +/* + * THE CASE THAT MAKES FALLBACKS NECESSARY. + * + * MEASURED across the fleet: k10temp (Ryzen), scmi (CIX Sky1) and cpu_thermal + * (Pi, most Arm SoCs) publish tempN_input with no tempN_crit and no critical + * trip point whatsoever. Without a fallback the CPU -- the one sensor anybody + * watches -- would be uncoloured on every Arm and AMD machine we ship. + */ +private void test_driver_without_crit_still_gets_a_limit() { + reset_fixture(); + string cpu = hwmon_chip(0, "scmi_sensors"); + hwmon_temp(cpu, 1, 91000, "CPU_B0"); + + var m = monitor_for_fixture(); + var r = reading_named(m, "CPU_B0"); + assert(r != null); + assert(r.limit_millidegrees == 100000); + // 9 C of margin. + assert(r.severity == Singularity.Severity.HOT); +} + +/* + * A thermal zone with a critical trip point uses it, and the LOWEST one when + * several are published -- the first trip reached is the one that matters. + */ +private void test_thermal_uses_lowest_critical_trip() { + reset_fixture(); + thermal_zone(0, "cpu-thermal", 70000); + thermal_trip(0, 0, "passive", 85000); + thermal_trip(0, 1, "critical", 105000); + thermal_trip(0, 2, "critical", 95000); + + var m = monitor_for_fixture(); + var r = reading_named(m, "cpu-thermal"); + assert(r != null); + assert(r.limit_millidegrees == 95000); + assert(r.margin_millidegrees == 25000); + assert(r.severity == Singularity.Severity.NORMAL); +} + +/* + * The severity bands, at their exact boundaries. Margin-based, so a device + * with a low limit is not called warm at its idle temperature. + */ +private void test_severity_bands() { + reset_fixture(); + thermal_zone(0, "cpu-thermal", 70000); + thermal_trip(0, 0, "critical", 95000); // 25 C margin -> NORMAL + thermal_zone(1, "gpu-thermal", 71000); + thermal_trip(1, 0, "critical", 95000); // 24 C margin -> WARM + thermal_zone(2, "ddr-thermal", 84000); + thermal_trip(2, 0, "critical", 95000); // 11 C margin -> HOT + thermal_zone(3, "npu-thermal", 96000); + thermal_trip(3, 0, "critical", 95000); // over -> CRITICAL + + var m = monitor_for_fixture(); + assert(reading_named(m, "cpu-thermal").severity == Singularity.Severity.NORMAL); + assert(reading_named(m, "gpu-thermal").severity == Singularity.Severity.WARM); + assert(reading_named(m, "ddr-thermal").severity == Singularity.Severity.HOT); + assert(reading_named(m, "npu-thermal").severity == Singularity.Severity.CRITICAL); +} + +/* + * EACH CLOCK CARRIES ITS OWN POLICY MAXIMUM. + * + * MEASURED on CIX Sky1: five cpufreq policies with five different maxima. A + * single machine-wide maximum would normalise the little cores against a big + * core ceiling and paint them permanently idle -- here the little core is at + * its own limit while the big core is barely off idle. + */ +private void test_clocks_normalise_per_policy() { + reset_fixture(); + cpufreq_policy(0, 1800000, 1800000); // little, flat out + cpufreq_policy(4, 1900000, 2600000); // big, part way + + var m = monitor_for_fixture(); + var clocks = m.clocks(); + assert(clocks.length == 2); + // Sorted highest kHz first, and the maximum travelled with the reading + // rather than being paired to the wrong policy. + assert(clocks[0].khz == 1900000 && clocks[0].max_khz == 2600000); + assert(clocks[1].khz == 1800000 && clocks[1].max_khz == 1800000); + assert(clocks[1].fraction == 1.0); + assert(clocks[0].fraction < 0.75); + // The kHz-only view stays in the same order for existing callers. + assert(m.clocks_khz()[0] == 1900000); +} + +/* + * /proc/cpuinfo publishes a current MHz and no maximum, so those readings must + * report their fraction as unknown rather than inventing a denominator. + */ +private void test_cpuinfo_clocks_have_no_fraction() { + reset_fixture(); + write_file(Path.build_filename(fixture_root, "proc", "cpuinfo"), + "processor\t: 0\ncpu MHz\t\t: 2400.000\n"); + + var m = monitor_for_fixture(); + var clocks = m.clocks(); + assert(clocks.length == 1); + assert(clocks[0].khz == 2400000); + assert(clocks[0].max_khz == 0); + assert(clocks[0].fraction == -1.0); +} + + +/* + * THE SHAPE OF THE SoC THIS DISTRIBUTION TARGETS. + * + * MEASURED on CIX Sky1: ONE hwmon chip named scmi_sensors carries all 22 + * sensors, and CPU, GPU, NPU, VPU, DDR and PCB are told apart only by their + * labels. The chip name matches nothing in either allow-list, so before the + * labels were consulted this board reported cpu=-1 and gpu=-1 -- no CPU and no + * GPU temperature at all on the hardware the product exists for. + */ +private void test_scmi_labels_identify_cpu_and_gpu() { + reset_fixture(); + string scmi = hwmon_chip(0, "scmi_sensors"); + hwmon_temp(scmi, 1, 61000, "CPU_B0"); + hwmon_temp(scmi, 2, 58000, "CPU_M1"); + hwmon_temp(scmi, 3, 55000, "GPU_AVE"); + hwmon_temp(scmi, 4, 71000, "DDR_top"); + hwmon_temp(scmi, 5, 49000, "NPU"); + hwmon_temp(scmi, 6, 47000, "PCB_AMB"); + + var m = monitor_for_fixture(); + assert(m.cpu_millidegrees == 61000); + assert(m.gpu_millidegrees == 55000); + // DDR is the hottest thing on the board and must NOT become the CPU. + assert(m.system_millidegrees == 71000); + assert(reading_named(m, "NPU").kind == Singularity.SensorKind.SYSTEM); + assert(reading_named(m, "DDR_top").kind == Singularity.SensorKind.SYSTEM); +} + +/* + * A VRM label contains "cpu" but is not the die. It runs hotter than the part + * it feeds, so with hottest-wins it would be reported as the CPU temperature + * and overstate it. + */ +private void test_vrm_labels_are_not_the_die() { + reset_fixture(); + string sio = hwmon_chip(0, "nct6798"); + hwmon_temp(sio, 1, 88000, "CPU VRM"); + string cpu = hwmon_chip(1, "k10temp"); + hwmon_temp(cpu, 1, 62000, "Tctl"); + + var m = monitor_for_fixture(); + assert(m.cpu_millidegrees == 62000); + assert(reading_named(m, "VRM").kind == Singularity.SensorKind.SYSTEM); +} + +/* + * A thermal zone that duplicates an hwmon chip by name is dropped -- but its + * limit is not, when hwmon published none. + * + * MEASURED on CIX Sky1: hwmon exposes four sensors named acpitz with no + * tempN_crit, and four identically named thermal zones each carrying a 98 C + * critical trip. Dropping the zones outright discarded the only real limit on + * the machine. All four must end up on it, not just the first: a first-match + * upgrade left three of them on the fallback, so one sensor was drawn against + * two different limits. + */ +private void test_thermal_limit_rescued_from_dropped_duplicate() { + reset_fixture(); + string acpi = hwmon_chip(0, "acpitz"); + hwmon_temp(acpi, 1, 44000, null); + hwmon_temp(acpi, 2, 43000, null); + thermal_zone(0, "acpitz", 44000); + thermal_trip(0, 0, "critical", 98000); + thermal_zone(1, "acpitz", 43000); + thermal_trip(1, 0, "critical", 98000); + + var m = monitor_for_fixture(); + int seen = 0; + foreach (Singularity.SensorReading r in m.readings()) { + if (r.label != "acpitz") { + continue; + } + seen++; + assert(r.limit_is_reported); + assert(r.limit_millidegrees == 98000); + } + // Still two sensors, not four: the zones were merged, not appended. + assert(seen == 2); +} + public int main(string[] args) { Test.init(ref args); Test.add_func("/sensor/unknown-never-cpu", test_unknown_sensors_are_never_cpu); @@ -205,6 +485,17 @@ public int main(string[] args) { Test.add_func("/sensor/gpuss-is-gpu", test_gpuss_is_gpu_not_cpu); Test.add_func("/sensor/cpufreq-empty-falls-back", test_cpufreq_empty_dir_falls_back_to_cpuinfo); Test.add_func("/sensor/available-with-fan-only", test_available_with_fan_but_no_temperature); + Test.add_func("/sensor/sentinels-rejected", test_sentinel_temperatures_are_rejected); + Test.add_func("/sensor/hwmon-crit-is-limit", test_hwmon_crit_becomes_the_limit); + Test.add_func("/sensor/crit-beats-max", test_crit_preferred_over_max); + Test.add_func("/sensor/no-crit-still-limited", test_driver_without_crit_still_gets_a_limit); + Test.add_func("/sensor/lowest-critical-trip", test_thermal_uses_lowest_critical_trip); + Test.add_func("/sensor/severity-bands", test_severity_bands); + Test.add_func("/sensor/clocks-per-policy", test_clocks_normalise_per_policy); + Test.add_func("/sensor/cpuinfo-no-fraction", test_cpuinfo_clocks_have_no_fraction); + Test.add_func("/sensor/scmi-labels-classify", test_scmi_labels_identify_cpu_and_gpu); + Test.add_func("/sensor/vrm-is-not-the-die", test_vrm_labels_are_not_the_die); + Test.add_func("/sensor/limit-rescued-from-duplicate", test_thermal_limit_rescued_from_dropped_duplicate); int rc = Test.run(); if (fixture_root != null && FileUtils.test(fixture_root, FileTest.EXISTS)) { remove_path(File.new_for_path(fixture_root)); From 2db2d2e8918015b3a681ee5ebff358f3ee2f02da Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Sun, 16 Aug 2026 17:47:17 +0000 Subject: [PATCH 02/10] feat(sensor): group by component, and expose a heat fraction for bars Enabling the SCMI hwmon driver took CIX Sky1 from 5 readings to 25, and 17 of them landed in SYSTEM: NPU, VPU, two DDR, two SOC, an interconnect, six PCB points, three NVMe and two NICs, all in one capped list. The only way to see the drive was to widen the cap. Three kinds is too few for what a modern SoC actually reports. SensorKind gains NPU, VPU, MEMORY, STORAGE, NETWORK and BOARD. SYSTEM stays the honest fallback for anything unrecognised -- the allow-list doctrine is unchanged, this only widens what can be named. Matching is against chip AND label together, because a Sky1 board names these in the label (scmi_sensors NPU, DDR_top, PCB_AMB) while a PC names them in the chip (nvme, r8169). Order matters and is asserted: NPU before VPU before the GPU rules, or "VPU" is read as a GPU and the neural accelerator as either. A test pins that the GPU figure stays the GPU when a much hotter VPU sits beside it. SensorReading also gains heat_fraction, 0..1, for drawing a bar. It spans AMBIENT (20 C) to the limit rather than 0 C to the limit, because zero is not a meaningful floor for a temperature -- a machine in a room is already at 20, so measuring from 0 puts every idle sensor near mid-bar and the bars stop distinguishing anything. Measured on O6N: from ambient the CPU at 49 C against a 100 C limit fills 0.36 while the NVMe at 67.8 C against 84.85 C fills 0.73; from 0 C those are 0.49 and 0.80, close enough that two bars tell you little. Clamped both ends so a sub-ambient sensor cannot draw negative and one past its limit cannot overflow. Verified against the live 25-sensor set on O6N: every reading now names its component, only the interconnect falls through to SYSTEM, and the NVMe that needs attention is the one bar that stands out at 74%. 22 fixture tests, all passing. One earlier assertion was updated rather than deleted: it pinned DDR_top and NPU to SYSTEM, which encoded the old coarse behaviour. The property it existed for -- that the hottest thing on the board must not be mistaken for the CPU -- is now asserted directly. --- src/system/sensor.vala | 110 ++++++++++++++++++++++++++++++++++++++- tests/sensor_test.vala | 114 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 219 insertions(+), 5 deletions(-) diff --git a/src/system/sensor.vala b/src/system/sensor.vala index dfc1683..578d494 100644 --- a/src/system/sensor.vala +++ b/src/system/sensor.vala @@ -2,9 +2,28 @@ using GLib; namespace Singularity { + /** + * What a sensor is measuring. + * + * Finer than CPU/GPU/SYSTEM because a modern SoC reports far more than + * three things, and lumping the rest together makes the list unreadable. + * MEASURED on CIX Sky1 once SCMI sensors were enabled: 25 readings, of + * which 17 fell into SYSTEM -- NPU, VPU, two DDR, two SOC, an + * interconnect, six PCB points, three NVMe and two NICs, all in one capped + * list where the only way to see the drive was to widen the cap. + * + * SYSTEM stays the honest fallback for anything unrecognised; the + * allow-list doctrine is unchanged, this only widens what can be named. + */ public enum SensorKind { CPU, GPU, + NPU, + VPU, + MEMORY, + STORAGE, + NETWORK, + BOARD, SYSTEM } @@ -34,6 +53,13 @@ namespace Singularity { * declare a limit would therefore leave the CPU -- the one sensor a user * actually watches -- permanently uncoloured on both Arm and AMD. */ + /** + * Room temperature, the floor a heat bar is drawn from. Not a threshold -- + * nothing is judged against it, it only stops idle sensors from all + * rendering half-full. See SensorReading.heat_fraction. + */ + public const int AMBIENT_MILLIDEGREES = 20000; + namespace Thresholds { /** Margin at or above which a sensor is unremarkable. */ public const int NORMAL_MARGIN_MILLIDEGREES = 25000; @@ -48,8 +74,14 @@ namespace Singularity { // NVIDIA slows at 83 and shuts down in the low 90s; the // Mali-G720 on Sky1 trips at 95. case SensorKind.GPU: return 95000; - // Board, NIC and NVMe sensors. Anything with a real limit -- - // and an NVMe almost always publishes one -- overrides this. + // Accelerators share the die with the GPU and throttle in + // the same range. + case SensorKind.NPU: + case SensorKind.VPU: return 95000; + // Board, NIC, DRAM, drive and anything unrecognised. DDR5 is + // already throttling above 85, so the same number serves. + // Anything with a real limit -- and an NVMe almost always + // publishes one -- overrides this. default: return 85000; } } @@ -103,6 +135,35 @@ namespace Singularity { get { return limit_millidegrees - millidegrees; } } + /** + * How hot this sensor is, 0.0 to 1.0, for drawing a bar. + * + * Spans AMBIENT to the limit rather than 0 C to the limit. Zero is not + * a meaningful floor for a temperature: a machine sitting in a room is + * already at 20-ish, so measuring from 0 puts every idle sensor near + * the middle of its bar and the bars stop distinguishing anything. + * Measured on O6N, from ambient: CPU_B0 at 49 C against a 100 C limit + * fills 0.36 while the NVMe at 67.8 C against 85 C fills 0.73, so the + * one sensor actually worth looking at is the one that reads full -- + * from 0 C those would be 0.49 and 0.80, much closer together. + * + * Clamped at both ends: a sensor below ambient reads 0 rather than + * negative, and one past its limit reads 1 rather than overflowing. + */ + public double heat_fraction { + get { + int span = limit_millidegrees - AMBIENT_MILLIDEGREES; + if (span <= 0) { + return 0.0; + } + double f = (double) (millidegrees - AMBIENT_MILLIDEGREES) + / (double) span; + if (f < 0.0) return 0.0; + if (f > 1.0) return 1.0; + return f; + } + } + /** * The limit argument is optional so that every existing caller -- * including the NVIDIA path, which has no sysfs node to read a limit @@ -359,6 +420,29 @@ namespace Singularity { private const string[] NOT_DIE_LABELS = { "vrm", "vrout", "vddq", "ambient" }; + + /* + * The rest of what a SoC reports. Matched against chip AND label, + * because a Sky1 board names these in the label (scmi_sensors NPU, + * DDR_top, PCB_AMB) while a PC names them in the chip (nvme, r8169). + * + * Order matters in classify(): VPU is tested before GPU would be, or + * a "VPU" label never gets the chance -- and NPU before both, since a + * neural accelerator is neither. + */ + private const string[] NPU_NEEDLES = { "npu", "aipu" }; + private const string[] VPU_NEEDLES = { "vpu", "amvx", "venc", "vdec" }; + private const string[] MEMORY_NEEDLES = { "ddr", "dram", "dimm", "lpddr" }; + private const string[] STORAGE_NEEDLES = { "nvme", "drivetemp", "sd_", "ssd" }; + private const string[] NETWORK_NEEDLES = { + "r8169", "r8125", "mt7921", "iwlwifi", "phy", "eth", "enp", "wlan" + }; + /* + * Board-level points: the PCB thermistors, the SoC package zones and + * the generic ACPI zone. These are the machine, not a component, and + * grouping them apart keeps them from crowding out a hot drive. + */ + private const string[] BOARD_NEEDLES = { "pcb", "soc_", "acpitz", "board" }; private const string[] GPU_CHIPS = { "amdgpu", "radeon", "nouveau", "i915", "xe", "panfrost", "panthor", "mali", "lima", "v3d", "vc4", @@ -539,6 +623,28 @@ namespace Singularity { return SensorKind.CPU; } } + + // Everything else the SoC reports, matched on chip+label together. + // NPU first, then VPU: both would otherwise be swallowed by a + // broader match, and "vpu" must not be read as "gpu". + if (matches_any(joined, NPU_NEEDLES)) { + return SensorKind.NPU; + } + if (matches_any(joined, VPU_NEEDLES)) { + return SensorKind.VPU; + } + if (matches_any(joined, MEMORY_NEEDLES)) { + return SensorKind.MEMORY; + } + if (matches_any(joined, STORAGE_NEEDLES)) { + return SensorKind.STORAGE; + } + if (matches_any(joined, NETWORK_NEEDLES)) { + return SensorKind.NETWORK; + } + if (matches_any(joined, BOARD_NEEDLES)) { + return SensorKind.BOARD; + } // Unknown is SYSTEM on purpose. See the class comment. return SensorKind.SYSTEM; } diff --git a/tests/sensor_test.vala b/tests/sensor_test.vala index ca381c9..8e7ae90 100644 --- a/tests/sensor_test.vala +++ b/tests/sensor_test.vala @@ -419,9 +419,12 @@ private void test_scmi_labels_identify_cpu_and_gpu() { assert(m.cpu_millidegrees == 61000); assert(m.gpu_millidegrees == 55000); // DDR is the hottest thing on the board and must NOT become the CPU. - assert(m.system_millidegrees == 71000); - assert(reading_named(m, "NPU").kind == Singularity.SensorKind.SYSTEM); - assert(reading_named(m, "DDR_top").kind == Singularity.SensorKind.SYSTEM); + // It is now named MEMORY rather than dumped in SYSTEM, but the property + // this test exists for is unchanged: it is not the CPU and not the GPU. + assert(reading_named(m, "DDR_top").kind == Singularity.SensorKind.MEMORY); + assert(reading_named(m, "NPU").kind == Singularity.SensorKind.NPU); + assert(m.cpu_millidegrees != 71000); + assert(m.gpu_millidegrees != 71000); } /* @@ -476,6 +479,107 @@ private void test_thermal_limit_rescued_from_dropped_duplicate() { assert(seen == 2); } + +/* + * heat_fraction spans AMBIENT to the limit, not 0 C to the limit. + * + * MEASURED on O6N: the CPU at 49 C against a 100 C limit and the NVMe at + * 67.8 C against 84.85 C. From ambient those separate clearly (0.36 vs 0.73) + * and the drive is obviously the one to look at; measured from 0 C they would + * be 0.49 and 0.80, close enough that a glance at two bars tells you little. + */ +private void test_heat_fraction_spans_from_ambient() { + reset_fixture(); + thermal_zone(0, "cpu-thermal", 49000); + thermal_trip(0, 0, "critical", 100000); + string nvme = hwmon_chip(0, "nvme"); + hwmon_temp(nvme, 1, 67800, "Composite"); + write_file(Path.build_filename(nvme, "temp1_crit"), "84850\n"); + + var m = monitor_for_fixture(); + double cpu = reading_named(m, "cpu-thermal").heat_fraction; + double drive = reading_named(m, "Composite").heat_fraction; + // (49-20)/(100-20) = 0.3625 ; (67.8-20)/(84.85-20) = 0.7370 + assert(cpu > 0.35 && cpu < 0.38); + assert(drive > 0.72 && drive < 0.75); + // The whole point: the drive must read visibly hotter than the CPU. + assert(drive - cpu > 0.3); +} + +/* + * Clamped at both ends -- a sensor below ambient must not draw a negative bar, + * and one past its limit must not overflow it. + */ +private void test_heat_fraction_is_clamped() { + reset_fixture(); + thermal_zone(0, "cold-thermal", 5000); + thermal_trip(0, 0, "critical", 90000); + thermal_zone(1, "hot-thermal", 99000); + thermal_trip(1, 0, "critical", 90000); + + var m = monitor_for_fixture(); + assert(reading_named(m, "cold-thermal").heat_fraction == 0.0); + assert(reading_named(m, "hot-thermal").heat_fraction == 1.0); +} + + +/* + * THE GROUPING THE PANEL ACTUALLY HAS TO RENDER. + * + * Every label here is one Sky1 reports through scmi_sensors, plus the NVMe and + * NIC chips that sit alongside. Before the kinds were widened, all of these + * except the CPU and GPU entries landed in SYSTEM -- 17 of 25 readings in one + * capped list. + */ +private void test_soc_sensors_group_by_component() { + reset_fixture(); + string scmi = hwmon_chip(0, "scmi_sensors"); + hwmon_temp(scmi, 1, 61000, "CPU_B0"); + hwmon_temp(scmi, 2, 55000, "GPU_AVE"); + hwmon_temp(scmi, 3, 49000, "NPU"); + hwmon_temp(scmi, 4, 47000, "VPU"); + hwmon_temp(scmi, 5, 71000, "DDR_top"); + hwmon_temp(scmi, 6, 46000, "PCB_AMB"); + hwmon_temp(scmi, 7, 48000, "SOC_TRC"); + string nvme = hwmon_chip(1, "nvme"); + hwmon_temp(nvme, 1, 67000, "Composite"); + string nic = hwmon_chip(2, "r8169_0_100:00"); + hwmon_temp(nic, 1, 48000, null); + + var m = monitor_for_fixture(); + assert(reading_named(m, "CPU_B0").kind == Singularity.SensorKind.CPU); + assert(reading_named(m, "GPU_AVE").kind == Singularity.SensorKind.GPU); + assert(reading_named(m, "NPU").kind == Singularity.SensorKind.NPU); + assert(reading_named(m, "VPU").kind == Singularity.SensorKind.VPU); + assert(reading_named(m, "DDR_top").kind == Singularity.SensorKind.MEMORY); + assert(reading_named(m, "PCB_AMB").kind == Singularity.SensorKind.BOARD); + assert(reading_named(m, "SOC_TRC").kind == Singularity.SensorKind.BOARD); + assert(reading_named(m, "Composite").kind == Singularity.SensorKind.STORAGE); + assert(reading_named(m, "r8169").kind == Singularity.SensorKind.NETWORK); + // Nothing may fall through to SYSTEM on this board any more. + foreach (Singularity.SensorReading r in m.readings()) { + assert(r.kind != Singularity.SensorKind.SYSTEM); + } +} + +/* + * "VPU" must not be read as a GPU, and the NPU must not be read as either. + * Both would be swallowed by a broader match if the order were wrong. + */ +private void test_vpu_and_npu_are_not_the_gpu() { + reset_fixture(); + string scmi = hwmon_chip(0, "scmi_sensors"); + hwmon_temp(scmi, 1, 90000, "VPU"); + hwmon_temp(scmi, 2, 91000, "NPU"); + hwmon_temp(scmi, 3, 40000, "GPU_AVE"); + + var m = monitor_for_fixture(); + assert(reading_named(m, "VPU").kind == Singularity.SensorKind.VPU); + assert(reading_named(m, "NPU").kind == Singularity.SensorKind.NPU); + // The GPU figure must be the GPU, not the much hotter VPU next to it. + assert(m.gpu_millidegrees == 40000); +} + public int main(string[] args) { Test.init(ref args); Test.add_func("/sensor/unknown-never-cpu", test_unknown_sensors_are_never_cpu); @@ -496,6 +600,10 @@ public int main(string[] args) { Test.add_func("/sensor/scmi-labels-classify", test_scmi_labels_identify_cpu_and_gpu); Test.add_func("/sensor/vrm-is-not-the-die", test_vrm_labels_are_not_the_die); Test.add_func("/sensor/limit-rescued-from-duplicate", test_thermal_limit_rescued_from_dropped_duplicate); + Test.add_func("/sensor/heat-fraction-from-ambient", test_heat_fraction_spans_from_ambient); + Test.add_func("/sensor/heat-fraction-clamped", test_heat_fraction_is_clamped); + Test.add_func("/sensor/soc-groups-by-component", test_soc_sensors_group_by_component); + Test.add_func("/sensor/vpu-npu-not-gpu", test_vpu_and_npu_are_not_the_gpu); int rc = Test.run(); if (fixture_root != null && FileUtils.test(fixture_root, FileTest.EXISTS)) { remove_path(File.new_for_path(fixture_root)); From 56dffa3579930fe9037cfed1709f5db7669b34a2 Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Sun, 16 Aug 2026 17:55:18 +0000 Subject: [PATCH 03/10] fix(sensor): prefer the labelled source when two describe the same silicon MEASURED on CIX Sky1 with SCMI sensors enabled: the SoC reports its CPU and GPU twice, once through scmi_sensors with real labels and once as bare ACPI thermal zones, and the pairs agree to the degree -- TZB0 47.0 == scmi_sensors CPU_B0 47.0 TZM0 46.0 == scmi_sensors CPU_M0 46.0 TZGT 44.0 == scmi_sensors GPU_AVE 44.0 so the panel drew eight CPU rows for four sensors and four GPU rows for three. An unlabelled reading is now dropped when a labelled reading of the same kind reports the same temperature: a sensor the driver bothered to name is the more specific description of the same thing. The rule is deliberately narrow. It requires same kind AND identical temperature rather than dropping ACPI zones wholesale, because plenty of unlabelled sensors are genuinely independent -- both r8169 NICs on this board are unlabelled and must survive. Tested in both directions: the twin is dropped, and an unlabelled sensor with no labelled same-temperature twin survives, as do two unlabelled NICs when nothing labelled is NETWORK at all. The trade is that two distinct same-kind sensors reading identically for one tick will briefly render as one. That is a cosmetic loss for a tick; dropping a real sensor outright would not be. SensorReading gains is_labelled to carry the provenance. NVIDIA readings are constructed labelled -- the driver names them -- so they are never shadowed. Verified on live O6N hardware: 25 readings become 20, CPU shows exactly its four cluster sensors and GPU its three, both NICs remain, and the NVMe at 67.8 C is still the one row that stands out. The dedup tests use a new monitor_for_sky1_fixture() that sets the ACPI-zone hints, because the rule has nothing to compare unless those zones classify as CPU/GPU -- a test of shipping behaviour has to model the shipping config. 24 fixture tests, all passing. --- src/system/sensor.vala | 60 +++++++++++++++++++++++++++++++-- tests/sensor_test.vala | 76 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) diff --git a/src/system/sensor.vala b/src/system/sensor.vala index 578d494..bd8072b 100644 --- a/src/system/sensor.vala +++ b/src/system/sensor.vala @@ -130,6 +130,15 @@ namespace Singularity { public Severity severity { get; private set; } + /** + * True when the driver gave this sensor a name of its own (a hwmon + * tempN_label), false when all we have is the chip or zone name. + * + * Used to break ties between two sources describing the same silicon: + * a labelled reading is the more specific one. See refresh_internal(). + */ + public bool is_labelled { get; private set; } + /** Degrees still available before limit_millidegrees. */ public int margin_millidegrees { get { return limit_millidegrees - millidegrees; } @@ -170,7 +179,8 @@ namespace Singularity { * from -- keeps compiling and falls back by kind. */ public SensorReading(string label, int millidegrees, SensorKind kind, - int limit_millidegrees = 0) { + int limit_millidegrees = 0, bool is_labelled = false) { + this.is_labelled = is_labelled; this.label = label; this.millidegrees = millidegrees; this.kind = kind; @@ -687,7 +697,8 @@ namespace Singularity { : chip; found += new SensorReading(name, millidegrees, classify(chip, label), - hwmon_limit(base_path, stem)); + hwmon_limit(base_path, stem), + label != null && label != ""); } } return found; @@ -984,7 +995,8 @@ namespace Singularity { if (!plausible(celsius * 1000)) { continue; } - found += new SensorReading(name, celsius * 1000, SensorKind.GPU); + found += new SensorReading(name, celsius * 1000, SensorKind.GPU, + 0, true); if (fields.length >= 4) { // Fields can read "[N/A]" -- an integrated Thor GPU reports // no SM clock. double.parse yields 0 there, which the @@ -1104,6 +1116,48 @@ namespace Singularity { found += zone; } } + // PREFER THE LABELLED SOURCE WHEN TWO DESCRIBE THE SAME SILICON. + // + // MEASURED on CIX Sky1 with SCMI sensors enabled: the SoC reports + // its CPU and GPU twice, once through scmi_sensors with real + // labels and once as bare ACPI thermal zones, and the pairs are + // identical to the degree -- + // + // TZB0 47.0 == scmi_sensors CPU_B0 47.0 + // TZM0 46.0 == scmi_sensors CPU_M0 46.0 + // TZGT 44.0 == scmi_sensors GPU_AVE 44.0 + // + // -- so the panel drew eight CPU rows for four sensors. Drop the + // unlabelled twin: a reading the driver bothered to name is the + // more specific description of the same thing. + // + // Deliberately NARROW. It requires the same kind AND the exact + // same temperature, rather than dropping ACPI zones wholesale, + // because plenty of unlabelled sensors are genuinely independent + // -- both r8169 NICs on this board are unlabelled and must + // survive. The trade is that two distinct same-kind sensors + // reading identically for one tick will briefly show as one; that + // is a cosmetic loss, where dropping a real sensor outright is + // not. + SensorReading[] deduped = {}; + foreach (SensorReading candidate in found) { + bool shadowed = false; + if (!candidate.is_labelled) { + foreach (SensorReading other in found) { + if (other.is_labelled + && other.kind == candidate.kind + && other.millidegrees == candidate.millidegrees) { + shadowed = true; + break; + } + } + } + if (!shadowed) { + deduped += candidate; + } + } + found = deduped; + // Keep the sysfs-derived set separate from the NVIDIA set: when the // async query lands, publish_state() can merge the two again without // re-walking every hwmon and thermal node. diff --git a/tests/sensor_test.vala b/tests/sensor_test.vala index 8e7ae90..d5294f0 100644 --- a/tests/sensor_test.vala +++ b/tests/sensor_test.vala @@ -100,6 +100,22 @@ private void reset_fixture() { } } +/** + * A monitor configured the way the NCZ shell configures it on Sky1 -- with the + * ACPI-zone hints set. The dedup rule only has anything to compare when those + * zones are classified as CPU/GPU rather than falling through to SYSTEM, so a + * test of that rule must model the shipping configuration, not the bare + * default. + */ +private Singularity.SensorMonitor monitor_for_sky1_fixture() { + var m = new Singularity.SensorMonitor(); + m.sysfs_root = fixture_root; + m.gpu_hint = "TZGT"; + m.cpu_hint = "TZ"; + m.refresh(); + return m; +} + private Singularity.SensorMonitor monitor_for_fixture() { var m = new Singularity.SensorMonitor(); m.sysfs_root = fixture_root; @@ -580,6 +596,64 @@ private void test_vpu_and_npu_are_not_the_gpu() { assert(m.gpu_millidegrees == 40000); } + +/* + * A labelled reading shadows its unlabelled twin. + * + * MEASURED on CIX Sky1: the SoC reports CPU and GPU twice, once through + * scmi_sensors with labels and once as bare ACPI zones, identical to the + * degree -- TZB0 47.0 == CPU_B0 47.0, TZGT 44.0 == GPU_AVE 44.0. The panel + * drew eight CPU rows for four sensors. + */ +private void test_labelled_reading_shadows_unlabelled_twin() { + reset_fixture(); + string scmi = hwmon_chip(0, "scmi_sensors"); + hwmon_temp(scmi, 1, 47000, "CPU_B0"); + hwmon_temp(scmi, 2, 44000, "GPU_AVE"); + string tzb = hwmon_chip(1, "TZB0"); // same silicon, no label + hwmon_temp(tzb, 1, 47000, null); + string tzgt = hwmon_chip(2, "TZGT"); + hwmon_temp(tzgt, 1, 44000, null); + + var m = monitor_for_sky1_fixture(); + // The named ones survive; the bare zones do not. + assert(reading_named(m, "CPU_B0") != null); + assert(reading_named(m, "GPU_AVE") != null); + assert(reading_named(m, "TZB0") == null); + assert(reading_named(m, "TZGT") == null); +} + +/* + * THE OTHER DIRECTION, which is what keeps the rule honest. + * + * An unlabelled sensor with no labelled same-kind twin at the same + * temperature must survive. Both r8169 NICs on this board are unlabelled and + * genuinely independent; a rule that dropped ACPI zones wholesale, or matched + * on kind alone, would delete real sensors. + */ +private void test_independent_unlabelled_sensors_survive() { + reset_fixture(); + string scmi = hwmon_chip(0, "scmi_sensors"); + hwmon_temp(scmi, 1, 47000, "CPU_B0"); + // Same kind, DIFFERENT temperature -> not a twin, must survive. + string tzb = hwmon_chip(1, "TZB0"); + hwmon_temp(tzb, 1, 52000, null); + // No labelled NETWORK sensor exists at all -> both NICs must survive. + string nic1 = hwmon_chip(2, "r8169_0_100:00"); + hwmon_temp(nic1, 1, 48000, null); + string nic2 = hwmon_chip(3, "r8169_0_3100:00"); + hwmon_temp(nic2, 1, 48000, null); + + var m = monitor_for_sky1_fixture(); + assert(reading_named(m, "CPU_B0") != null); + assert(reading_named(m, "TZB0") != null); + int nics = 0; + foreach (Singularity.SensorReading r in m.readings()) { + if (r.kind == Singularity.SensorKind.NETWORK) nics++; + } + assert(nics == 2); +} + public int main(string[] args) { Test.init(ref args); Test.add_func("/sensor/unknown-never-cpu", test_unknown_sensors_are_never_cpu); @@ -604,6 +678,8 @@ public int main(string[] args) { Test.add_func("/sensor/heat-fraction-clamped", test_heat_fraction_is_clamped); Test.add_func("/sensor/soc-groups-by-component", test_soc_sensors_group_by_component); Test.add_func("/sensor/vpu-npu-not-gpu", test_vpu_and_npu_are_not_the_gpu); + Test.add_func("/sensor/labelled-shadows-twin", test_labelled_reading_shadows_unlabelled_twin); + Test.add_func("/sensor/independent-unlabelled-survive", test_independent_unlabelled_sensors_survive); int rc = Test.run(); if (fixture_root != null && FileUtils.test(fixture_root, FileTest.EXISTS)) { remove_path(File.new_for_path(fixture_root)); From 4f249f788cf7836fb980735f539b0ec68bcded1a Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Mon, 17 Aug 2026 19:01:14 +0000 Subject: [PATCH 04/10] =?UTF-8?q?fix:=20address=20Codex=20review=20?= =?UTF-8?q?=E2=80=94=20preserve=20SensorKind=20ABI=20+=20constructor=20ABI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SensorKind: keep SYSTEM at its original numeric position (2) by inserting the new NPU/VPU/MEMORY/STORAGE/NETWORK/BOARD variants after it instead of before -- the prior ordering silently renumbered SYSTEM from 2 to 8, which would misclassify readings for any client built against the old numbering. SensorReading: split into a primary 3-arg constructor (frozen at the original signature) plus a named .with_limit() constructor for the extended (limit + is_labelled) form. Vala default arguments are source-level sugar only -- the generated C constructor takes every listed parameter with no overload, so a client compiled against the old 3-arg symbol would still link against the new 5-arg one and silently receive undefined values for the two new parameters instead of failing to build. All 3 internal call sites updated to use .with_limit() explicitly. Co-Authored-By: Claude Opus 5 (1M context) --- src/system/sensor.vala | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/system/sensor.vala b/src/system/sensor.vala index bd8072b..fa2852f 100644 --- a/src/system/sensor.vala +++ b/src/system/sensor.vala @@ -18,13 +18,13 @@ namespace Singularity { public enum SensorKind { CPU, GPU, + SYSTEM, NPU, VPU, MEMORY, STORAGE, NETWORK, - BOARD, - SYSTEM + BOARD } /** @@ -173,13 +173,25 @@ namespace Singularity { } } + public SensorReading(string label, int millidegrees, SensorKind kind) { + this.with_limit(label, millidegrees, kind, 0, false); + } + /** - * The limit argument is optional so that every existing caller -- - * including the NVIDIA path, which has no sysfs node to read a limit - * from -- keeps compiling and falls back by kind. + * Extended form carrying a reported thermal limit and/or provenance. + * + * A NAMED constructor rather than default arguments on the primary + * one: Vala default arguments are source-level sugar only -- the + * generated C constructor takes every listed parameter with no + * overload, so a client compiled against the old 3-argument + * SensorReading(label, millidegrees, kind) would still link against + * a 5-argument symbol and silently pass garbage for the two new + * parameters instead of failing to build. Keeping the primary + * constructor's signature frozen and adding this as a second, + * separately-named entry point avoids that trap entirely. */ - public SensorReading(string label, int millidegrees, SensorKind kind, - int limit_millidegrees = 0, bool is_labelled = false) { + public SensorReading.with_limit(string label, int millidegrees, SensorKind kind, + int limit_millidegrees, bool is_labelled) { this.is_labelled = is_labelled; this.label = label; this.millidegrees = millidegrees; @@ -695,7 +707,7 @@ namespace Singularity { string name = (label != null && label != "") ? "%s %s".printf(chip, label) : chip; - found += new SensorReading(name, millidegrees, + found += new SensorReading.with_limit(name, millidegrees, classify(chip, label), hwmon_limit(base_path, stem), label != null && label != ""); @@ -727,9 +739,9 @@ namespace Singularity { if (!plausible(millidegrees)) { continue; } - found += new SensorReading(zone_type, millidegrees, + found += new SensorReading.with_limit(zone_type, millidegrees, classify(zone_type, null), - thermal_limit(base_path)); + thermal_limit(base_path), false); } return found; } @@ -995,7 +1007,7 @@ namespace Singularity { if (!plausible(celsius * 1000)) { continue; } - found += new SensorReading(name, celsius * 1000, SensorKind.GPU, + found += new SensorReading.with_limit(name, celsius * 1000, SensorKind.GPU, 0, true); if (fields.length >= 4) { // Fields can read "[N/A]" -- an integrated Thor GPU reports From 2352e42bc84c66b7ef16f757293173463e73eb21 Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Sun, 16 Aug 2026 23:54:55 +0000 Subject: [PATCH 05/10] fix(network): report the wired port that is actually connected The control panel showed "Wired Connection: not connected" on a machine whose LAN was fully up. NetworkManager was right and the panel was wrong. nm_wrapper latched onto the FIRST ethernet device it enumerated: if (ethernet_device == null) { ethernet_device = ed; ... } and then reported is_wired_connected from that one device alone. On any machine with more than one wired port that is a coin flip. Measured on cixmini (.66), where sinty-nm exposes: Device/2 enp49s0 type=1 state=30 (DISCONNECTED, nothing plugged in) Device/3 enp1s0 type=1 state=100 (ACTIVATED, carrying the entire LAN) enp49s0 enumerates first, so the panel read state=30 and said not connected while the box was reachable over enp1s0 the whole time. Two changes. update_state() now prefers whichever ethernet device is ACTIVATED, falling back to the first when none is. And the state notify handler is attached to EVERY ethernet device rather than only the first, so a link coming up on any other port actually triggers a refresh -- without that, the fix above would only take effect on some unrelated later update. No change to what NM reports; this only corrects which device is believed. --- src/system/nm_wrapper.vala | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/system/nm_wrapper.vala b/src/system/nm_wrapper.vala index cb32ab8..bea29a4 100644 --- a/src/system/nm_wrapper.vala +++ b/src/system/nm_wrapper.vala @@ -127,13 +127,16 @@ namespace Singularity { } else if (device is NM.DeviceEthernet) { var ed = (NM.DeviceEthernet) device; ethernet_devices.add(ed); + has_ethernet = true; if (ethernet_device == null) { ethernet_device = ed; - has_ethernet = true; - ethernet_device.notify["state"].connect(() => { - update_state(); - }); } + // Watch EVERY wired port. Only the first one used to be + // watched, so on a machine with more than one NIC a link + // coming up on any other port never triggered a refresh. + ed.notify["state"].connect(() => { + update_state(); + }); } } if (wifi_device == null) { @@ -709,6 +712,19 @@ namespace Singularity { wifi_enabled = client.wireless_enabled; bool wwan_off = !client.wwan_enabled; is_airplane_mode = !wifi_enabled && wwan_off; + // Prefer whichever wired port is actually ACTIVATED. Latching onto + // the first enumerated NIC reports "not connected" on any machine + // with several ethernet ports whenever the live link is not the + // first -- measured on cixmini (.66), where enp49s0 (DISCONNECTED) + // enumerates ahead of enp1s0, which carries the entire LAN. The + // NM state was correct throughout; only this selection was wrong. + for (int i = 0; i < ethernet_devices.length; i++) { + var cand = ethernet_devices.get(i); + if (cand.state == NM.DeviceState.ACTIVATED) { + ethernet_device = cand; + break; + } + } is_wired_connected = (ethernet_device != null && ethernet_device.state == NM.DeviceState.ACTIVATED); if (!wifi_enabled) { From 3658ead1f7c87a0e50e88d1d9df80663f99941e7 Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Mon, 17 Aug 2026 15:51:43 -0400 Subject: [PATCH 06/10] fix(sensor): match duplicate hwmon/thermal channels by temperature proximity Codex review (PR #11, 2026-08-17): the acpitz upgrade loop broke ties by iteration order, not correspondence. With two same-labelled hwmon readings and one thermal zone, the loop could upgrade the wrong one and leave a duplicate reading, hiding the hottest sensor. Now selects the closest match by millidegrees among limit-less candidates. --- src/system/sensor.vala | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/system/sensor.vala b/src/system/sensor.vala index fa2852f..0583aa1 100644 --- a/src/system/sensor.vala +++ b/src/system/sensor.vala @@ -1110,16 +1110,30 @@ namespace Singularity { // first-match rule upgraded one of them and left the other // three on the fallback, so the same sensor was drawn against // two different limits. + // + // Among same-label limit-less candidates, the CLOSEST reading + // by temperature wins, not the first found. hwmon and thermal + // enumerate independently, so "first" is directory order, not + // correspondence -- with two acpitz readings at 40C/50C and + // one 40C zone, a first-match rule could upgrade the 50C entry + // and leave two 40C readings, hiding the hottest sensor. int upgrade_index = -1; + int best_delta = int.MAX; bool duplicate = false; for (int i = 0; i < found.length; i++) { if (!same_sensor(found[i].label, zone.label)) { continue; } duplicate = true; - if (!found[i].limit_is_reported && zone.limit_is_reported) { + if (found[i].limit_is_reported) { + continue; + } + int delta = found[i].millidegrees > zone.millidegrees + ? found[i].millidegrees - zone.millidegrees + : zone.millidegrees - found[i].millidegrees; + if (delta < best_delta) { + best_delta = delta; upgrade_index = i; - break; } } if (upgrade_index >= 0) { From fcb8f5934ee17580fef704a2e9c29ee867a28c5e Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Mon, 17 Aug 2026 16:36:35 -0400 Subject: [PATCH 07/10] feat(network): expose per-port wired device info (chipset, link capability) Adds EthernetPortInfo + NetworkManagerWrapper.ethernet_ports(): every wired port the board has, connected or not, with its PCI chipset name (lspci) and highest advertised link mode (ethtool), probed once per port asynchronously and cached. Existing is_wired_connected/ethernet_device summary is untouched -- this is additive, for a settings page that needs to list every port rather than summarize one. --- src/system/nm_wrapper.vala | 138 +++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/src/system/nm_wrapper.vala b/src/system/nm_wrapper.vala index bea29a4..a9e58ac 100644 --- a/src/system/nm_wrapper.vala +++ b/src/system/nm_wrapper.vala @@ -2,6 +2,38 @@ using NM; namespace Singularity { + /** + * One physical wired port, connected or not. NM only tells the UI about + * a port once it has a device object, so "not connected" and "not + * present" are the same picture to it -- this exists so the Network + * settings page can list every port a board actually has, cable in or + * out, rather than only the one currently carrying traffic. + */ + public class EthernetPortInfo : GLib.Object { + public string iface { get; private set; } + public bool connected { get; private set; } + + /** PCI chipset string (lspci), e.g. "RTL8125 2.5GbE Controller". Empty until probed. */ + public string chipset { get; private set; default = ""; } + + /** Highest advertised link mode (ethtool), e.g. "2.5 GbE". Empty until probed or unknown. */ + public string capability { get; private set; default = ""; } + + public EthernetPortInfo(string iface, bool connected) { + this.iface = iface; + this.connected = connected; + } + + internal void set_connected(bool value) { + connected = value; + } + + internal void set_details(string chipset, string capability) { + this.chipset = chipset; + this.capability = capability; + } + } + public class NetworkManagerWrapper : GLib.Object { public bool wifi_enabled { get; private set; default = true; } public bool has_wifi { get; private set; default = false; } @@ -25,6 +57,11 @@ namespace Singularity { public signal void hotspot_state_changed(); public signal void sharing_action_result(bool success, string message); + // Fires when a port is added/removed, changes link state, or a + // chipset/capability probe completes -- whichever, the settings + // page's list needs a full rebuild rather than one row's update. + public signal void ethernet_ports_changed(); + private const string HOTSPOT_ID = "Singularity Hotspot"; private const string WIRED_SHARE_ID = "Singularity Wired Sharing"; @@ -42,6 +79,7 @@ namespace Singularity { private GenericArray wifi_devices = new GenericArray(); private NM.DeviceEthernet? ethernet_device; private GenericArray ethernet_devices = new GenericArray(); + private GenericArray ethernet_ports_list = new GenericArray(); private bool wifi_request_in_flight = false; private bool wifi_requested_state = false; @@ -131,10 +169,15 @@ namespace Singularity { if (ethernet_device == null) { ethernet_device = ed; } + var port = new EthernetPortInfo(ed.get_iface(), ed.state == NM.DeviceState.ACTIVATED); + ethernet_ports_list.add(port); + probe_ethernet_port.begin(port); // Watch EVERY wired port. Only the first one used to be // watched, so on a machine with more than one NIC a link // coming up on any other port never triggered a refresh. ed.notify["state"].connect(() => { + port.set_connected(ed.state == NM.DeviceState.ACTIVATED); + ethernet_ports_changed(); update_state(); }); } @@ -144,6 +187,101 @@ namespace Singularity { } } + /** Every wired port the board has, cable in or out. */ + public GenericArray ethernet_ports() { + return ethernet_ports_list; + } + + /** + * Chipset (lspci) and top advertised link mode (ethtool) for one + * port. Both are external processes, so this runs once per port at + * discovery time and caches the result on the EthernetPortInfo -- + * neither changes while the machine is running, so there is nothing + * to re-probe on a later link-state change. + */ + private async void probe_ethernet_port(EthernetPortInfo port) { + string chipset = yield probe_chipset(port.iface); + string capability = yield probe_capability(port.iface); + port.set_details(chipset, capability); + ethernet_ports_changed(); + } + + private async string probe_chipset(string iface) { + string pci_addr; + try { + string link = GLib.FileUtils.read_link("/sys/class/net/%s/device".printf(iface)); + pci_addr = GLib.Path.get_basename(link); + } catch (GLib.FileError e) { + return ""; + } + string[] argv = { "lspci", "-s", pci_addr, "-vmm" }; + string? output = yield run_probe(argv); + if (output == null) { + return ""; + } + // lspci -vmm is "Key:\tValue" lines, one record per device. The + // marketing chipset name is the Device field, not Vendor -- e.g. + // "RTL8125 2.5GbE Controller", not "Realtek Semiconductor Co., Ltd.". + foreach (string line in output.split("\n")) { + if (line.has_prefix("Device:")) { + return line.substring("Device:".length).strip(); + } + } + return ""; + } + + private async string probe_capability(string iface) { + string[] argv = { "ethtool", iface }; + string? output = yield run_probe(argv); + if (output == null) { + return ""; + } + // "Supported link modes:" is followed by one or more continuation + // lines (further indented, no leading key) until the next + // "Key:" line. Collect the whole block, then take the highest + // NbaseT rate mentioned -- that is the port's ceiling regardless + // of what it is currently negotiated to. + bool in_block = false; + int best_mbps = -1; + foreach (string raw_line in output.split("\n")) { + string line = raw_line.strip(); + if (line.has_prefix("Supported link modes:")) { + in_block = true; + line = line.substring("Supported link modes:".length).strip(); + } else if (in_block && raw_line.length > 0 && !raw_line.get_char(0).isspace()) { + break; + } else if (!in_block) { + continue; + } + foreach (string token in line.split(" ")) { + int idx = token.index_of("baseT"); + if (idx <= 0) continue; + int mbps = int.parse(token.substring(0, idx)); + if (mbps > best_mbps) best_mbps = mbps; + } + } + if (best_mbps <= 0) { + return ""; + } + return best_mbps >= 1000 + ? "%.3g GbE".printf(best_mbps / 1000.0) + : "%d Mbps".printf(best_mbps); + } + + /** Run a short-lived probe utility off-thread; null on any failure. */ + private async string? run_probe(string[] argv) { + try { + var proc = new GLib.Subprocess.newv(argv, + GLib.SubprocessFlags.STDOUT_PIPE | GLib.SubprocessFlags.STDERR_SILENCE); + string? stdout_text = null; + string? stderr_text = null; + yield proc.communicate_utf8_async(null, null, out stdout_text, out stderr_text); + return stdout_text; + } catch (GLib.Error e) { + return null; + } + } + // Setting client.wireless_enabled / wwan_enabled directly is a // synchronous D-Bus call that blocks the GTK main thread for seconds. // Set the documented WirelessEnabled/WwanEnabled property on From 197f73cc7d5eaf59082ec6256eedc2a96cee8050 Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Mon, 17 Aug 2026 16:42:08 -0400 Subject: [PATCH 08/10] fix(network): rename EthernetPortInfo.set_connected to avoid ABI collision Vala auto-generates a C setter for the 'connected { get; private set; }' property using the same mangled name (singularity_ethernet_port_info_set_connected) as an explicitly-declared method of that name -- valac failed with 'internal: Redefinition of ...set_connected'. Renamed the explicit method to mark_connected. --- src/system/nm_wrapper.vala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/system/nm_wrapper.vala b/src/system/nm_wrapper.vala index a9e58ac..8128fb1 100644 --- a/src/system/nm_wrapper.vala +++ b/src/system/nm_wrapper.vala @@ -24,7 +24,7 @@ namespace Singularity { this.connected = connected; } - internal void set_connected(bool value) { + internal void mark_connected(bool value) { connected = value; } @@ -176,7 +176,7 @@ namespace Singularity { // watched, so on a machine with more than one NIC a link // coming up on any other port never triggered a refresh. ed.notify["state"].connect(() => { - port.set_connected(ed.state == NM.DeviceState.ACTIVATED); + port.mark_connected(ed.state == NM.DeviceState.ACTIVATED); ethernet_ports_changed(); update_state(); }); From 8c827e091e790b0c22740011f1c51b2c1d1abe95 Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Mon, 17 Aug 2026 18:24:51 -0400 Subject: [PATCH 09/10] fix(sensor,network): carry limit provenance across dedup; track NM hotplug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review (PR #11, 2026-08-17, commit 197f73c) — two real findings: - Shadowing an unlabelled twin into its labelled counterpart dropped the twin's reported critical trip. On the documented Sky1 topology the unlabelled ACPI zone is routinely the ONLY side carrying a real limit (hwmon acpitz publishes no tempN_crit while the matching thermal zone publishes 98 C — already noted earlier in this same method), so the surviving labelled reading fell back to a guessed ceiling and computed margin/severity against it. Silent, because the row still rendered fine. Now the reported limit is adopted before the twin is dropped. - ethernet_devices/ethernet_ports_list were built once during init_client() and never updated, so a wired port attached or removed afterwards (USB-C dock, USB ethernet adapter — routine on this hardware) left ethernet_ports() reporting a port that was gone or omitting one just added, contradicting what ethernet_ports_changed() promises subscribers. Added NM.Client device_added/device_removed handling, with the per-device registration factored into register_ethernet_device() so both paths stay identical, and the cached ethernet_device reference re-picked rather than left dangling when the removed device was the cached one. --- src/system/nm_wrapper.vala | 77 +++++++++++++++++++++++++++++--------- src/system/sensor.vala | 32 ++++++++++++++++ 2 files changed, 92 insertions(+), 17 deletions(-) diff --git a/src/system/nm_wrapper.vala b/src/system/nm_wrapper.vala index 8128fb1..bff20e6 100644 --- a/src/system/nm_wrapper.vala +++ b/src/system/nm_wrapper.vala @@ -163,28 +163,71 @@ namespace Singularity { }); } } else if (device is NM.DeviceEthernet) { - var ed = (NM.DeviceEthernet) device; - ethernet_devices.add(ed); - has_ethernet = true; - if (ethernet_device == null) { - ethernet_device = ed; - } - var port = new EthernetPortInfo(ed.get_iface(), ed.state == NM.DeviceState.ACTIVATED); - ethernet_ports_list.add(port); - probe_ethernet_port.begin(port); - // Watch EVERY wired port. Only the first one used to be - // watched, so on a machine with more than one NIC a link - // coming up on any other port never triggered a refresh. - ed.notify["state"].connect(() => { - port.mark_connected(ed.state == NM.DeviceState.ACTIVATED); - ethernet_ports_changed(); - update_state(); - }); + register_ethernet_device((NM.DeviceEthernet) device); } } if (wifi_device == null) { warning("No WiFi device found!"); } + + // A wired port can appear or disappear long after this initial + // enumeration -- a USB-C dock or a USB ethernet adapter is the + // common case, and this hardware is docked routinely. Without + // these, ethernet_ports() kept reporting a port that had been + // unplugged and never showed one that had just been attached, + // which directly contradicts what ethernet_ports_changed() + // promises its subscribers. + client.device_added.connect((device) => { + if (device is NM.DeviceEthernet) { + register_ethernet_device((NM.DeviceEthernet) device); + ethernet_ports_changed(); + update_state(); + } + }); + client.device_removed.connect((device) => { + if (!(device is NM.DeviceEthernet)) { + return; + } + var gone = (NM.DeviceEthernet) device; + ethernet_devices.remove(gone); + string iface = gone.get_iface(); + for (int i = ethernet_ports_list.length - 1; i >= 0; i--) { + if (ethernet_ports_list.get(i).iface == iface) { + ethernet_ports_list.remove_index(i); + } + } + has_ethernet = ethernet_devices.length > 0; + // ethernet_device is the cached "currently interesting" port + // used by is_wired_connected. If the removed device was it, + // drop the dangling reference and let update_state() re-pick + // from whatever is left rather than reporting link state for + // a device that no longer exists. + if (ethernet_device == gone) { + ethernet_device = ethernet_devices.length > 0 + ? ethernet_devices.get(0) : null; + } + ethernet_ports_changed(); + update_state(); + }); + } + + private void register_ethernet_device(NM.DeviceEthernet ed) { + ethernet_devices.add(ed); + has_ethernet = true; + if (ethernet_device == null) { + ethernet_device = ed; + } + var port = new EthernetPortInfo(ed.get_iface(), ed.state == NM.DeviceState.ACTIVATED); + ethernet_ports_list.add(port); + probe_ethernet_port.begin(port); + // Watch EVERY wired port. Only the first one used to be + // watched, so on a machine with more than one NIC a link + // coming up on any other port never triggered a refresh. + ed.notify["state"].connect(() => { + port.mark_connected(ed.state == NM.DeviceState.ACTIVATED); + ethernet_ports_changed(); + update_state(); + }); } /** Every wired port the board has, cable in or out. */ diff --git a/src/system/sensor.vala b/src/system/sensor.vala index 0583aa1..fb01ad5 100644 --- a/src/system/sensor.vala +++ b/src/system/sensor.vala @@ -1165,6 +1165,38 @@ namespace Singularity { // reading identically for one tick will briefly show as one; that // is a cosmetic loss, where dropping a real sensor outright is // not. + // CARRY THE LIMIT ACROSS BEFORE DROPPING THE TWIN. + // + // The labelled twin is the better NAME, but not necessarily the + // better LIMIT. On this same Sky1 topology the unlabelled ACPI + // zone is frequently the only side carrying a real critical trip + // -- the identical case already noted further up this method, + // where hwmon acpitz publishes no tempN_crit at all while the + // matching thermal zone publishes 98 C. Shadowing TZB0 into + // CPU_B0 without moving that trip over left the survivor on a + // guessed fallback limit, so margin and severity were computed + // against the wrong ceiling -- silently, because the row still + // looked right. Adopt the reported limit first, then drop. + for (int i = 0; i < found.length; i++) { + if (!found[i].is_labelled || found[i].limit_is_reported) { + continue; + } + foreach (SensorReading twin in found) { + if (twin.is_labelled || !twin.limit_is_reported) { + continue; + } + if (twin.kind == found[i].kind + && twin.millidegrees == found[i].millidegrees) { + found[i] = new SensorReading.with_limit(found[i].label, + found[i].millidegrees, + found[i].kind, + twin.limit_millidegrees, + true); + break; + } + } + } + SensorReading[] deduped = {}; foreach (SensorReading candidate in found) { bool shadowed = false; From cb853f896daa4642a1bbea40cc52880232f1d64d Mon Sep 17 00:00:00 2001 From: Jason Perlow Date: Mon, 17 Aug 2026 19:00:21 -0400 Subject: [PATCH 10/10] fix(sensor): consume each limit donor once when carrying trips across MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review (PR #11, commit 8c827e0) — correct, and it is about the fix added in that same commit. The limit-carry loop selected a donor by (kind, millidegrees) and never marked it consumed, so if several same-kind labelled sensors happened to read identically for a tick they would all adopt the SAME zone's critical trip. Kind plus temperature is not an identity: the Sky1 CPU cluster zones sit within a degree of each other and can coincide. Now each donor is used at most once. A labelled sensor with no donor left stays honestly limit-less, which downstream already renders as a fallback -- strictly better than being handed another zone's ceiling and computing margin and severity against it. --- src/system/sensor.vala | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/system/sensor.vala b/src/system/sensor.vala index fb01ad5..f607e06 100644 --- a/src/system/sensor.vala +++ b/src/system/sensor.vala @@ -1177,11 +1177,26 @@ namespace Singularity { // guessed fallback limit, so margin and severity were computed // against the wrong ceiling -- silently, because the row still // looked right. Adopt the reported limit first, then drop. + // Each donor is consumed at most ONCE. Kind plus temperature is + // not an identity: Sky1 reports several same-kind sensors that + // can read identically for a tick (the CPU cluster zones sit + // within a degree of each other), and without a consumed flag a + // single zone's trip point would be handed to every labelled + // sensor that happened to match it that tick -- inventing a + // limit for sensors whose donor was really a different zone. + // One-to-one keeps an unmatched sensor honestly limit-less + // instead, which downstream already renders as a fallback rather + // than as a wrong ceiling. + bool[] limit_donor_used = new bool[found.length]; for (int i = 0; i < found.length; i++) { if (!found[i].is_labelled || found[i].limit_is_reported) { continue; } - foreach (SensorReading twin in found) { + for (int j = 0; j < found.length; j++) { + if (limit_donor_used[j]) { + continue; + } + SensorReading twin = found[j]; if (twin.is_labelled || !twin.limit_is_reported) { continue; } @@ -1192,6 +1207,7 @@ namespace Singularity { found[i].kind, twin.limit_millidegrees, true); + limit_donor_used[j] = true; break; } }