feat(sensor): thresholds, severity, and broader classification for Arm SoCs - #11
feat(sensor): thresholds, severity, and broader classification for Arm SoCs#11perlowja wants to merge 10 commits into
Conversation
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.
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.
…licon
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56dffa3579
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public enum SensorKind { | ||
| CPU, | ||
| GPU, | ||
| NPU, |
There was a problem hiding this comment.
Inserting the new variants before SYSTEM renumbers this public enum from 2 to 8. Because the system library retains the same SONAME, a precompiled client upgraded to this version will interpret a returned NPU value (2) as SYSTEM and will no longer recognize actual SYSTEM readings (8). Keep SYSTEM in its original position and append the new variants after it to preserve the existing ABI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked against the current head (3658ead): SensorKind is CPU, GPU, SYSTEM, ... -- SYSTEM is still enum value 2, unchanged from before this PR. This looks re-anchored from an earlier revision: 4f249f7 already restored SYSTEM's original position and split the constructor to avoid the codegen ABI break (same class of finding your prior review caught, already applied). Let me know if I'm misreading the current file.
| public SensorReading(string label, int millidegrees, SensorKind kind, | ||
| int limit_millidegrees = 0, bool is_labelled = false) { |
There was a problem hiding this comment.
Default arguments provide source compatibility only; adding these parameters changes the generated C constructor signature while retaining the same exported symbol. A client compiled against the previous three-argument constructor can therefore still link against this shared library but pass no defined values for the two new parameters, causing arbitrary reported limits and label state. Preserve the old constructor signature and expose the extended form through a separate named constructor or setter.
Useful? React with 👍 / 👎.
| if (upgrade_index >= 0) { | ||
| found[upgrade_index] = zone; |
There was a problem hiding this comment.
When several hwmon channels share a normalized label but thermal exposes only a subset, this replaces the first limit-less name match without checking that its temperature corresponds to the zone. For example, hwmon acpitz readings at 40°C and 50°C plus one 40°C thermal zone can overwrite the 50°C reading depending on directory order, leaving duplicate 40°C readings and hiding the hottest sensor. Select the corresponding channel using additional identity such as its temperature rather than replacing an arbitrary name match.
Useful? React with 👍 / 👎.
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) <noreply@anthropic.com>
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.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…oximity Codex review (PR singularityos-lab#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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ility) 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.
…ision
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 197f73cc7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (other.is_labelled | ||
| && other.kind == candidate.kind | ||
| && other.millidegrees == candidate.millidegrees) { |
There was a problem hiding this comment.
When a labelled hwmon reading and a differently named unlabelled thermal zone have the same kind and temperature, this drops the thermal reading even if it carries the only reported critical trip. This is possible in the documented Sky1 flow (CPU_B0 versus TZB0): the surviving labelled reading then uses a fallback limit, producing an inaccurate margin and severity. Transfer the reported limit to the labelled reading or retain the unlabelled reading when it has better limit provenance.
Useful? React with 👍 / 👎.
| var port = new EthernetPortInfo(ed.get_iface(), ed.state == NM.DeviceState.ACTIVATED); | ||
| ethernet_ports_list.add(port); | ||
| probe_ethernet_port.begin(port); |
There was a problem hiding this comment.
When a USB Ethernet adapter or dock is attached or removed after init_client() completes, this list remains unchanged: find_wifi_device() performs only the initial client.get_devices() enumeration, and no NetworkManager device-added/device-removed handlers update ethernet_devices, ethernet_ports_list, or has_ethernet. Consequently ethernet_ports() can keep returning unplugged ports or omit newly attached ones despite the new signal's contract covering additions and removals.
Useful? React with 👍 / 👎.
5ddd4b3 to
197f73c
Compare
…tplug Codex review (PR singularityos-lab#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.
|
Triaged the latest Codex pass (commit Fixed
Not changed
Also note this branch previously carried a merge of #10 — that was only ever a local device to get the shell to compile (shell |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c827e091e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (twin.kind == found[i].kind | ||
| && twin.millidegrees == found[i].millidegrees) { | ||
| found[i] = new SensorReading.with_limit(found[i].label, |
There was a problem hiding this comment.
When multiple labelled sensors of the same kind happen to report the same temperature as one unlabelled sensor, this loop reuses that single twin for every labelled reading, so one zone's critical trip can be assigned to unrelated sensors and produce incorrect margins and severity. Fresh evidence in the updated code is that a matched twin is never marked as consumed or otherwise tied to a specific sensor; pair limits one-to-one using sensor identity rather than kind and temperature alone.
Useful? React with 👍 / 👎.
Codex review (PR singularityos-lab#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.
1 participant
Follow-on to the
SensorMonitorbackend merged in #9. Three changes, each measured on real Sky1 hardware (CIX/O6N) with SCMI sensors enabled, where the generic sensor set first showed its edges.What's in this PR (final state)
Arm SoC classification + thermal limits.
classify()previously matched only chip names; a singlescmi_sensorschip carrying 22 differently-labelled sensors (CPU_B0, GPU_AVE, NPU, VPU, DDR, PCB…) reportedcpu=-1 gpu=-1— no temperature at all. Labels are now consulted for GPU too, with VRM labels excluded so a hot regulator can't be mistaken for the die it feeds.SensorReadinggainslimit_millidegrees+ aSeveritybanded by margin-to-limit (not fraction-of-limit, so a drive with an 84°C trip isn't "warm" at idle), with a per-kind fallback for the common case wherek10temp/scmi/cpu_thermalpublish no limit at all. Readings are now bounded above as well as below (one host reported 65,261°C from a sentinel value).Nine
SensorKinds instead of three. Enabling SCMI took Sky1 from 5 readings to 25, with 17 dumped intoSYSTEM. Added NPU, VPU, MEMORY, STORAGE, NETWORK, BOARD — matched against chip and label together since a Sky1 board names components in the label and a PC in the chip. Ordering is asserted (NPU before VPU before GPU) so the neural accelerator and the GPU can't swap.heat_fraction(0..1) spans ambient (20°C) to limit rather than 0°C to limit — measured on O6N, a 0°C floor put every idle sensor near mid-bar and stopped distinguishing anything.Dedup labelled vs. unlabelled duplicates. Sky1 reports CPU/GPU twice — once via
scmi_sensorswith real labels, once as bare ACPI thermal zones at the same temperature — so the panel drew 8 CPU rows for 4 sensors. An unlabelled reading is now dropped only when a labelled reading of the same kind reports the same temperature; genuinely independent unlabelled sensors (both onboard NICs) survive untouched. 25 readings become 20 on O6N; both NICs remain; the NVMe that actually needs attention is still the one row that stands out.Each commit has the full hardware evidence in its message (exact readings, before/after counts). 24 fixture tests, all passing — including a
monitor_for_sky1_fixture()that models the shipping ACPI-zone config, since the dedup rule has nothing to compare unless those zones classify correctly first.Standalone and useful without a consumer, same as #9 — a shell-side sensors panel PR building on this will follow.