shell(panel): a Sensors panel item — CPU/GPU/component temperature, clocks, thermal severity - #21
shell(panel): a Sensors panel item — CPU/GPU/component temperature, clocks, thermal severity#21perlowja wants to merge 21 commits into
Conversation
Reads sysfs directly -- thermal zones for temperature, cpufreq for clock. On the CIX Sky1 boards this board exposes TZB0/TZB1 (big cluster), TZM0/TZM1 (mid) and TZGT (graphics); measured idle on O6N is 47/46/46/45/44 C. The zone named by `sensors-gpu-zone` (default TZGT) is reported as GPU and the hottest of the remaining zones as CPU, so nothing but that default is board-specific. Deliberately NOT wattage. Sky1 exposes no power rail to the kernel: there is no /sys/class/power_supply, no hwmon power*_input or curr*_input, and no energy*_uj anywhere on the board. A "watts" readout here could only ever be an invented estimate, so the widget does not offer one. Three things worth flagging for review: - EVERY settings read is guarded by settings_schema.has_key(). The gschema ships from the singularity-desktop superproject while this code ships from the singularity-shell submodule, so the two can be version-skewed on a real install. An unguarded g_settings_get_* against a missing key is a FATAL abort, which would take down the whole panel -- and, because Panel is shared with greeter_mode, the login screen with it. - The item is registered unconditionally but placement comes from panel-layout-*, so registering does not display it. It stays opt-in, and the greeter picks it up automatically since the greeter builds the same Panel. - Polling is skipped when the widget is not mapped. The board now throttles itself when idle (ncz-perf-activity); a 2-second timer that reads five sysfs files whether or not anyone can see them would work against that. Hardware with no readable thermal zones hides the widget rather than showing zeros, so this is inert on non-Sky1 boards.
…make it portable Replaces the first cut, which put temperature and clock directly on the bar. That does not scale: a CIX Sky1 board exposes five thermal zones and an x86 desktop with a Super-I/O chip can expose a dozen, so a per-sensor item would push the clock off the panel. The bar now carries ONE chip (hottest sensor, optionally the top CPU clock) and everything else moves into a popover grouped into CPU / GPU / Clocks, rebuilt only while that popover is actually open. Also removes what was board-specific so this can go upstream: - /sys/class/hwmon is now the PRIMARY source. It is the generic kernel interface and covers x86 (coretemp, k10temp, zenpower, nct6775), discrete GPUs (amdgpu, nouveau, i915) and many ARM SoCs. - /sys/class/thermal is the FALLBACK, because a number of ARM SoCs expose temperatures only there -- CIX Sky1 among them (TZB0/TZB1 big, TZM0/TZM1 mid, TZGT graphics). - GPU classification is by kernel DRIVER NAME or label, not by any single platform-specific zone string, so amdgpu/nouveau/i915/panfrost/panthor/mali all group correctly with no board knowledge. sensors-gpu-zone remains only as an override for hardware the heuristic misses and now defaults to empty instead of naming one platform. - Clocks prefer cpufreq and fall back to /proc/cpuinfo, since cpufreq is absent on many VMs and on some x86 without a scaling driver. Hardware with nothing readable hides the widget rather than showing zeros. Settings reads stay has_key()-guarded: the schema can ship from a different package than this binary, and an unguarded get on a missing key is a fatal abort that would take down the panel and the greeter with it.
…ading sysfs Moves all sysfs access out of the panel widget and into Singularity.SensorMonitor, per CONTRIBUTING: headless system backends (D-Bus, sysfs, hardware managers with no GTK) belong in libsingularity-system, not the shell. The widget now only renders what the backend publishes, and SystemMonitor exposes it as .sensors following the same lazy-property pattern as .resources. Also switches the summary to the backend cpu/system split. The backend classifies by an allow-list and reports -1 when it found no CPU sensor, so the widget can fall back deliberately instead of a NIC or chipset being displayed as the processor.
…inst their own ceiling The sensors popover printed every temperature in the same dim grey, so a CPU 4 C from its critical trip looked exactly like one at idle. Rows now take their colour from SensorReading.severity, and so does the chip on the bar itself -- a reading that needs attention should be noticeable without opening anything, since a popover nobody opens conveys nothing. The ramp is dim, plain, amber, red, using the stock GTK "warning" and "error" classes rather than a palette of our own: those are already defined by every theme and already legible on its background, where a hand-picked amber would collide with the user accent and need maintaining separately for light and dark. WARM deliberately gets no class at all -- undimming to the ordinary foreground is the first step of the ramp, and colour is spent only where it means something. The summary label removes the previous tick classes before adding the current one. add_css_class is additive, so without that the chip would stay red for the rest of the session once the machine had been hot once. Clocks are deliberately NOT coloured. A core at its maximum is doing its job, and painting it red trains the user to ignore the colour that does mean something. They are instead shown against their own ceiling, which is not one number per machine: CIX Sky1 exposes five cpufreq policies with five different maxima, so 1.4 GHz is nearly flat out on one cluster and near idle on another. Requires the Severity and ClockReading API added to libsingularity.
MEASURED on two CIX Sky1 machines whose sensor topologies differ completely,
decided by one kernel command line flag:
cixmini, 7.0.12-cix-sky1-next, no acpi_scmi_en flag
one hwmon chip "scmi_sensors" carrying 22 LABELLED sensors --
CPU_B0, CPU_B1, CPU_M0, CPU_M1, GPU_AVE, GPU_top, GPU_btm, NPU,
VPU, DDR_top, DDR_btm, PCB_AMB, PCB_HOT, SOC_TRC, ...
O6N, 7.2.0-rc7-sky1-ncz, acpi_scmi_en=off
no scmi_sensors at all -- five bare ACPI thermal zones named
TZB0 TZB1 TZM0 TZM1 TZGT, with no labels and no tempN_crit
SCMI is disabled deliberately on 7.2, so the second topology is what we
ship. There the allow-lists inside SensorMonitor cannot help: the identity
of a sensor lives in a four-character ACPI name and nowhere else. Probed on
O6N, the panel reported cpu=-1 gpu=-1 -- no CPU and no GPU temperature on
the board this product targets.
cpu_hint and gpu_hint are the documented extension point for exactly this
("hardware the allow-list cannot know... a distribution sets these"), so
this is a configuration change rather than another vendor string baked into
libsingularity.
TZB is the big cluster, TZM the mid cluster, TZGT graphics. classify() tests
gpu_hint before cpu_hint, so the specific TZGT claims the GPU before the
broader TZ claims the rest.
Verified on O6N hardware, before and after:
before cpu=-1 gpu=-1 system=70850
after cpu=49000 gpu=46000 system=70850
with TZGT GPU, TZB0/TZB1/TZM0/TZM1 CPU, and nvme plus both r8169 NICs still
correctly SYSTEM. Both hints are inert on the scmi_sensors topology, where
no chip or label contains "TZ", so one configuration serves both kernels.
Worth recording separately: the 7.2 configuration costs the user NPU, VPU,
DDR, PCB and SOC temperatures outright -- 22 sensors become 5.
Reported from the machine: the sensors panel "takes multiple times to poll and show the entries". Cause: rebuild_details() runs only from on_updated(), and only when the popover is ALREADY visible. So the first open showed an empty box and stayed empty until the refresh timer next fired -- up to a full interval, two seconds by default. Open it, see nothing, close it, open it again, and by then a tick has landed and the rows appear. That reads exactly like needing several tries. Refresh when the popover becomes visible. That both populates it immediately and means the figures shown are the ones at the instant of opening, rather than up to an interval stale. refresh() publishes the sysfs sources synchronously and then emits updated(), so the existing on_updated() path does the rebuild -- there is no second rendering path to keep in step. The NVIDIA query stays asynchronous and lands on a later tick exactly as before.
…at bar Three defects, all visible on one photograph of the running panel. THE WIDER GROUPS WERE NEVER RENDERED. rebuild_details() listed only CPU, GPU and SYSTEM, so the NPU, VPU, MEMORY, STORAGE, NETWORK and BOARD kinds were classified by the backend and then silently dropped. On Sky1 that hid eleven of nineteen readings, including the NVMe at 68 C -- the one sensor on the board actually worth looking at. add_group() already skips an empty kind, so a machine reporting only CPU and GPU still shows exactly two headings. THE ROWS HAD NO NAMES. add_row() built name_label, set its alignment, and never appended it, so every row rendered as a bar and a temperature with no way to tell which sensor it was. Now appended, ellipsized at 22 characters with the full name on a tooltip so a long chip+label cannot push the reading off the popover. THE BAR WAS ALWAYS RED. It was a Gtk.LevelBar, and GTK gives a LevelBar its own offset classes (level-low / level-high / level-full) which themes style with BATTERY semantics -- low means trouble, painted red. Every sensor therefore showed a short red bar regardless of temperature, so a 46 C CPU looked exactly as alarming as a hot drive. Overriding that meant fighting theme rules on a widget whose entire purpose is to be themed. It is now a DrawingArea that owns its pixels: cool blue, green, amber, orange, red, chosen here and identical on every machine and theme. Five discrete steps rather than a continuous gradient, because a gradient needs a CssProvider per row and rebuilding twenty of them on each popover open is real cost for a difference nobody can see. Verified on O6N: nine groups render, each row names its sensor, bars read blue at 45-49 C, and the 68 C NVMe is the single orange bar on the panel.
![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: a2d9c68258
ℹ️ 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 (schema != null && schema.has_key("sensors-gpu-zone")) { | ||
| monitor.gpu_hint = settings.get_string("sensors-gpu-zone"); | ||
| } | ||
| if (schema != null && schema.has_key("sensors-cpu-zone")) { | ||
| monitor.cpu_hint = settings.get_string("sensors-cpu-zone"); |
There was a problem hiding this comment.
When the settings schema contains these keys with their portable empty defaults, these assignments replace the TZGT/TZ hints installed by SystemMonitor.sensors before the first refresh. On the shipping Sky1 ACPI topology, those hints are the only way to distinguish the CPU and GPU zones, so the popover groups them as system sensors and the summary can display an unrelated hottest component. Apply these overrides only when the configured string is non-empty, or make the schema defaults preserve the platform hints.
Useful? React with 👍 / 👎.
| string[] item_ids = { | ||
| "overview", "workspaces", "tiling-position", "app-title", "global-menu", | ||
| "system", "notifications", "clock" | ||
| "system", "notifications", "clock", "sensors" |
There was a problem hiding this comment.
Adding sensors to item_ids does display it by default, contrary to the opt-in behavior described above: BarLayout appends every allowed item absent from the saved and default arrays to the center section (src/core/bar_layout.vala, constructor final loop). Since all existing saved layouts lack this new ID, upgrading inserts the chip into every panel's center, including greeter and secondary panels, potentially disrupting customized layouts. The disabled state must be represented separately rather than relying on omission from the saved arrays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the intended behavior, not a side effect of adding sensors to default_center -- BarLayout's constructor (bar_layout.vala, the final foreach over allowed_items) already appends any allowed item missing from BOTH saved and default arrays to center_items unconditionally. sensors was already in item_ids/allowed_items before this fix, so it was already being force-injected into every saved layout on upgrade; adding it to default_center only corrects the comment that inaccurately described it as opt-in. Showing sensors by default is also the actual product requirement this PR closes (task: missing sensors plugin on O6). Agreed there's a real gap here, but it's upstream of this PR: BarLayout has no per-item disabled/opt-out marker distinct from "absent", so any newly-allowed item forces itself onto existing layouts. That's a bigger design change (an explicit dismissed-items list, most likely) I'd rather scope as its own PR against bar_layout.vala rather than fold into this one.
| } | ||
|
|
||
| monitor.updated.connect(on_updated); | ||
| monitor.start(interval); |
There was a problem hiding this comment.
This starts the backend timer unconditionally, so an unmapped or hidden panel continues reading hwmon and running asynchronous GPU queries at the configured interval even though no reading is visible. This regresses the prior mapped-state guard and is particularly costly on the target boards whose idle throttling this feature is intended to preserve. Start/stop polling with the widget's mapped state, while retaining the explicit refresh when the popover opens.
Useful? React with 👍 / 👎.
…s a real default sensors-gpu-zone/sensors-cpu-zone: only override monitor.gpu_hint/cpu_hint when the user actually configured a non-empty zone name. The schema's portable default is empty, and unconditionally assigning it clobbered the Sky1 TZGT/TZ hints SystemMonitor.sensors sets up internally -- silently defeating the CPU/GPU identification commit on first load with default settings. item_ids/default_center: sensors is now genuinely in default_center, same as system/notifications/clock, matching how BarLayout actually treats any allowed item absent from a user's saved layout (it gets force-added to center regardless -- the prior 'stays opt-in via registration' comment did not match that behavior for a brand-new item id). Comment corrected to describe the real mechanism instead of an aspirational one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45e9a43e28
ℹ️ 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 (_sensors == null) { | ||
| _sensors = new SensorMonitor(); | ||
| _sensors.gpu_hint = "TZGT"; | ||
| _sensors.cpu_hint = "TZ"; |
There was a problem hiding this comment.
On x86 systems exposing the common acpitz hwmon chip, the globally applied TZ substring hint also matches that chip and causes its ACPI/board temperature to be classified as CPU. When a real CPU sensor such as coretemp or k10temp is also present, the aggregate CPU value and panel summary can therefore select the unrelated ACPI zone. Apply this hint only after identifying the Sky1 ACPI topology rather than changing classification for every platform.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked against classify() (sensor.vala) in libsingularity: the hint match is joined.contains(cpu_hint), and Vala's string.contains is case-sensitive (strstr). cpu_hint is "TZ" (uppercase); the x86 acpitz hwmon chip name and its thermal-zone type are both the lowercase literal "acpitz" -- no uppercase TZ substring, so it can't match. Open to being wrong if you know of a board where the ACPI thermal zone label surfaces uppercase, but on the code as written this doesn't cross-match x86.
| ClockReading[] clocks = monitor.clocks(); | ||
| if (clocks.length > 0) { | ||
| add_heading(_("Clocks")); | ||
| for (int i = 0; i < clocks.length; i++) { |
There was a problem hiding this comment.
On high-core-count systems where cpufreq exposes one policy per CPU, monitor.clocks() can contain dozens or hundreds of entries, and this loop appends all of them to the bare detail_box. The temperature groups are capped, but the popover has no ScrolledWindow, so it can grow beyond the monitor and leave lower clock rows inaccessible; cap these rows or place the detail box in a scrollable viewport.
Useful? React with 👍 / 👎.
Codex review (PR singularityos-lab#21, 2026-08-17): - SensorsIndicator started its polling timer unconditionally at construction instead of on map, so a hidden/unmapped panel kept reading hwmon and running the async NVIDIA query every interval for no visible reading. Now starts on map, stops on unmap, matching the map-gated pattern already used elsewhere in this file (TilingSlotOverlay). - The Clocks section in the sensors popover had no cap and no scroll container; on a many-cpufreq-policy x86 box the list could run the popover off-screen. Capped to MAX_ROWS_PER_GROUP with an '+N more' row, same convention already used for the temperature groups above it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48700ef473
ℹ️ 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".
| monitor.updated.connect(on_updated); | ||
| on_updated(); |
There was a problem hiding this comment.
On a fresh shell, the newly constructed SensorMonitor has not been started or refreshed when this call runs, so monitor.available is initially false and on_updated() sets the indicator's visible property to false. GTK does not map an invisible child, which means the map handler below never calls monitor.start(interval) and no later update can make the default Sensors chip appear. Perform an initial start/refresh before on_updated(), or bind polling to the containing panel's mapped state rather than this self-hiding widget.
Useful? React with 👍 / 👎.
…mary Replaces the single Connected/Not Connected wired row with one row per NetworkManagerWrapper.ethernet_ports() entry, showing interface name, PCI chipset, and top link capability. A board with several NICs (O6N: two 2.5GbE Realtek ports) previously showed only whichever port happened to be summarized, cable in or out.
Codex review (PR singularityos-lab#21, 2026-08-17, commit b87423a): monitor.start() only ran once the widget was mapped, but on_updated() ran first and set visible=false when monitor.available was still its pre-refresh default (false) -- and GTK never maps an invisible widget, so map never fired and the panel's default Sensors chip could never appear. One synchronous refresh() before the first on_updated() (same call already used when the popover opens) establishes real availability first.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19bb9a702c
ℹ️ 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".
| detail_box.margin_top = 10; | ||
| detail_box.margin_bottom = 10; | ||
| detail_box.margin_start = 12; | ||
| detail_box.margin_end = 12; |
There was a problem hiding this comment.
Although each group and the clock section is capped independently, rebuild_details() can still append six readings plus an overflow row for each of nine sensor kinds, along with headings and clocks. On the high-count systems called out in this change (for example, the 55-sensor Qualcomm topology), the bare detail_box can therefore request roughly 70 rows and extend beyond the display, leaving lower groups inaccessible. Put the aggregate content in a bounded ScrolledWindow or enforce a global row limit rather than only per-group limits.
Useful? React with 👍 / 👎.
… Sky1 Codex review (PR singularityos-lab#21, 2026-08-17, commit 19bb9a7): - Per-group caps bounded each section but not the total. Nine sensor kinds x (6 rows + overflow) plus headings plus clocks reaches ~70 rows on the 55-sensor Qualcomm topology this change calls out, running off the bottom of the screen with the lower groups unreachable. The popover child is now a ScrolledWindow with propagate_natural_height, so small machines render byte-identically to before and only genuinely oversized content scrolls. - The TZGT/TZ classification hints are four-character CIX Sky1 ACPI names, but SystemMonitor applied them on every platform. Now gated on actually being a Sky1 board. MEASURED on O6N while writing that gate, and it changed the implementation: the obvious check (DMI vendor / devicetree contains cix or sky1) returns FALSE on real Sky1 hardware -- the shipping kernel is ACPI so there is no devicetree at all, and every DMI string reads Radxa ... Orion O6N, never CIX or Sky1. Shipping that would have silently restored the cpu=-1/gpu=-1 bug these hints exist to fix. Detection therefore keys on the SoC's own ACPI HIDs (CIXH*, 163 of which enumerate on that machine), with devicetree kept as a fallback for a DT-booted Sky1. Also verified already-fixed and re-anchored rather than re-fixed: the P1 'start polling before hiding the uninitialized indicator' finding (monitor.refresh() before the first on_updated() landed in 19bb9a7).
|
Triaged the latest Codex pass (commit Fixed
Already addressed
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29265a709f
ℹ️ 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 (!monitor.available) { | ||
| // Nothing readable on this hardware: hide rather than show zeros. | ||
| visible = false; | ||
| return; |
There was a problem hiding this comment.
The initial refresh only fixes the startup case from the earlier review: if a later refresh reports no available readings—for example while an hwmon driver reloads, a GPU is power-gated, or a sensor is hot-unplugged—this hides the indicator, which unmaps it and invokes the monitor.stop() handler. No subsequent timer refresh can discover that the sensors returned, so the chip remains absent until another explicit refresh or a shell restart. Availability should not stop the mechanism responsible for detecting availability changes.
Useful? React with 👍 / 👎.
| } | ||
| // "compatible" is NUL-separated, so match the raw buffer. | ||
| string lowered = contents.down(); | ||
| if (lowered.contains("cix") || lowered.contains("sky1")) { |
There was a problem hiding this comment.
On DT-booted Sky1 boards whose compatible property is conventionally ordered like radxa,<board>\0cix,sky1, contents.down() operates on a NUL-terminated Vala string and therefore only examines the first entry. The fallback consequently misses cix,sky1, is_cix_sky1() returns false, and the CPU/GPU hints are not installed on precisely the DT configuration this branch is meant to support. Read the file as bytes with its returned length or split and inspect every compatible entry.
Useful? React with 👍 / 👎.
| int primary = monitor.cpu_millidegrees >= 0 | ||
| ? monitor.cpu_millidegrees | ||
| : monitor.system_millidegrees; |
There was a problem hiding this comment.
When a machine exposes only classified non-CPU readings—for example a GPU or NVMe hwmon sensor—but no CPU or SYSTEM reading, monitor.available is true while both values selected here are -1. If CPU frequency is also disabled or unavailable, the summary label is empty even though the popover contains valid temperatures, leaving an effectively blank panel control. Select the hottest available reading when neither CPU nor system data exists, and use that reading's kind for severity.
Useful? React with 👍 / 👎.
| ClockReading[] clocks = monitor.clocks(); | ||
| if (clocks.length > 0) { |
There was a problem hiding this comment.
With sensors-show-frequency set to false, the setting is honored only for the compact summary; opening the popover still unconditionally fetches and renders the entire Clocks section here. This makes the frequency preference ineffective for the detailed view. Gate this section on show_frequency as well.
Useful? React with 👍 / 👎.
…tries, honour the frequency toggle, never render an empty chip Codex review (PR singularityos-lab#21, commit 29265a7). Four real findings, two of them in code added by that same commit: - Availability was switching off the mechanism that detects availability. If a later refresh reported nothing readable (hwmon driver reloading, a GPU power-gated, a sensor hot-unplugged) the widget hid itself, which unmaps it, which fired the unmap handler and stopped the poll timer -- after which nothing could ever observe the sensors returning and the chip stayed gone until the shell restarted. A self-inflicted unmap is now distinguished from a real one and keeps polling. - The devicetree fallback in is_cix_sky1() read compatible with FileUtils.get_contents and matched the resulting Vala string, which stops at the first NUL. compatible is a NUL-SEPARATED list ordered most specific first (radxa,<board>\0cix,sky1), so only the board entry was ever examined and cix,sky1 was missed -- on exactly the DT-booted configuration that fallback exists to catch. Now reads the real byte array via load_contents and inspects every entry. - sensors-show-frequency gated only the compact summary, so the whole Clocks section still rendered on opening the popover; the preference did half of what it claimed. - available == true does not imply a CPU or SYSTEM reading exists. On a machine whose sensors all classify as GPU/STORAGE/NETWORK both selections were -1 and, with cpufreq also unavailable, the chip rendered as an empty label beside a popover full of valid temperatures. Falls back to the hottest reading of any kind, carrying its kind so the colour still describes the number.
Adds live utilisation next to the temperatures: CPU and memory percentages in the compact chip, and a Processor / Memory / Storage breakdown in the popover with per-core rows, RAM and swap, per-volume capacity and per-device disk activity. Gated on a new sensors-show-utilization key, read through the same schema.has_key() guard every other settings read here uses, and honoured in BOTH the compact chip and the popover. sensors-show-frequency previously gated only the chip, so the preference silently did half of what it said; this does not repeat that. Polling starts and stops with map/unmap alongside the sensor monitor, and survives the self-inflicted unmap that hiding-for-unavailable causes -- otherwise hiding the chip would stop the timer that would notice the machine recovering. Two behaviours worth calling out: - Temperatures being unreadable no longer hides the whole indicator. /proc/stat and /proc/meminfo exist on every Linux machine, including the many with no hwmon at all and VMs exposing no thermal zones. The old test hid a control that had perfectly good CPU and memory figures to show. Availability is probed via memory, the one figure that is valid on the first sample. - Every fraction is checked against < 0 before formatting. The monitor reports -1.0 for "not known yet" and for "no swap configured", and multiplying that by 100 renders "-100%" -- observed on cixmini, which has no swap. Capacity and memory are colour-coded at the same 0.85/0.95 thresholds ResourceMonitor already alerts on, so the panel turns amber when the notification fires rather than at some second, unrelated number. CPU and disk BUSY are deliberately not coloured, for the reason the Clocks section already documents: a core at 100% is doing its job, and painting it red trains the user to ignore the colour that does mean something. NOT COMPILE-VERIFIED. cixmini cannot build the shell at all: our libgtk4-layer-shell0 1.3.0-1+ncz20260811 package ships runtime only -- no headers, no .pc, no vapi -- and Debian's libgtk4-layer-shell-dev 1.3.0-1+b1 refuses to co-install against it. The libsingularity half of this change IS verified (16 tests green, checked against real /proc). One compile error was caught statically here before commit: Severity has no WARNING member, only NORMAL/WARM/HOT/CRITICAL.
utilization_available() probes memory_fraction, which is -1.0 until something has polled. Starting the poller only from the map handler rebuilt the self-deadlocking initialisation the surrounding comment already warns about, one step further out: on a machine with no readable hwmon but a working /proc -- a VM with no thermal zones, or any board without an hwmon driver -- monitor.available is false and utilization_available() is false only because nothing has looked yet. on_updated() hides the widget, GTK never maps an invisible widget, the map handler never fires, util.start() never runs, and the indicator stays hidden for the whole session with CPU and memory figures it could have shown from the start. One synchronous poll before that first decision, mirroring the monitor.refresh() immediately above it. Found by the local upstream-reviewer simulation (sing-presubmit-review) before this was pushed, which is what that tool exists for: the same finding from the real chatgpt-codex-connector bot would have cost a PR round-trip.
Each metric in the compact chip now carries its own colour via Pango markup spans instead of one plain-text label with a whole-chip severity class: temperature and memory by their own severity (green/amber/red at the same thresholds the popover already uses), CPU and clock speed a neutral accent colour since neither is ever severity-coloured -- a core at 100% is doing its job, the same reasoning the popover's Clocks section documents. Colours are resolved at render time via Widget.lookup_color() against named theme tokens (success_color/warning_color/error_color/ accent_color), not hardcoded hex, so the chip tracks a light/dark theme switch instead of baking in whichever mode happened to be active when this was written. Checked for icon availability before reaching for icons instead: neither Adwaita nor the NCZ icon theme has a CPU, memory, or temperature symbolic icon (only breeze/breeze-dark do, and Adwaita is the active theme) -- forcing a mismatched or missing glyph would be worse than the plain numbers. A small coloured dot (U+25CF) per segment gives the same glanceable, graphical read without that risk.
Compile error caught by the container build: `lookup_color` is a Gtk.StyleContext member (deprecated since 4.10 but still the working path -- there is no non-deprecated replacement for resolving a NAMED CSS colour at runtime), not a Gtk.Widget method directly. Route through get_style_context() instead of calling it on the label.
Two related changes to the popover's grouped sections: 1. Clocks no longer lists cpufreq policies 1:1 as generic "Core group N" rows capped at MAX_ROWS_PER_GROUP with a bare count for the rest. Grouped by max_khz instead -- the actual performance-tier signal: clocks() is one entry per cpufreq POLICY, and a policy is a clock domain, so cores sharing one on a heterogeneous SoC (Sky1's five policies) are exactly the cores in the same tier. This is architecture-agnostic by construction, not by special-casing: a homogeneous desktop CPU collapses to one row (every core reports the same max), while a hybrid P-core/E-core x86 or ARM big.LITTLE part produces multiple tier rows from the identical logic. The "N more" cap this replaces was only ever a symptom of not doing this grouping to begin with. 2. Every remaining overflow row (CPU per-core, disk activity, and each temperature SensorKind group) now shows the AVERAGE of what got cut instead of a bare count with an empty value -- a 64-core machine still tells you roughly how busy the other 58 cores are. Storage capacity is the one deliberate exception: volume fill percentages across differently-sized disks don't average into a meaningful number the way a rate does, so it keeps a plain count and gets its own separate overflow counter rather than being blended with disk activity's rate-based average. Temperature's averaged row is deliberately left uncoloured: severity is classified per-sensor against that sensor's own limit, and an average across sensors that may have different limits has no single threshold to colour against.
Measured against O6N's real cpufreq topology before building this: Sky1's five policies each report a genuinely DIFFERENT max_khz (2.6, 1.8, 2.3, 2.2, 2.5 GHz across policy0/2/6/8/10), so the tier-grouping added earlier produces the same five rows the old per-policy listing did -- nothing collapses on this hardware, because nothing shares a ceiling to collapse into. That made the grouping look like it had no effect, when it was working correctly for a case this board doesn't exhibit. The toggle makes the difference checkable rather than assumed: flip to Ungrouped and the exact per-policy rows are there, labelled with the real sysfs policy name (e.g. "policy0"), so grouped-vs-raw can be compared directly instead of trusted on faith. It also gives the grouping real value on hardware where it DOES matter -- a homogeneous desktop CPU, or same-tier cores on a hybrid part -- without forcing that choice for everyone. Persisted via sensors-clocks-grouped (same has_key-guarded read/write pattern as the other sensors-* prefs), but the control itself lives in the popover next to the Clocks heading, not a hidden setting -- you should be able to see it and flip it right where the numbers are.
… to every group The toggle previously lived at the Clocks heading and only affected Clocks tier-grouping. Renamed clocks_grouped -> sensors_grouped (and the settings key sensors-clocks-grouped -> sensors-grouped to match) and had add_group() skip its per-kind heading when ungrouped, sharing the same field -- so one toggle now governs every section (CPU/GPU/NPU/Memory/Storage/Network/Board/ System/Clocks), not just Clocks. Moved the control itself to the top of rebuild_details(), before any section renders, so its popover-wide scope is visible from where it sits rather than looking like a Clocks-only control. Clocks keeps its own plain heading (add_heading), shown only when grouped, consistent with every other section. Verified: rebuilt via build-singularity.sh against this checkout, deployed to O6N, rebooted clean, desktop session running.
… merge CPU+Clocks into one section Grouped previously only hid per-kind headings while still listing every individual sensor -- it never actually collapsed anything, so toggling Grouped/Ungrouped looked like it did nothing on most sections. Rewired add_group() so Grouped now renders exactly ONE row per kind (GPU, VPU, NPU, Memory, Storage, Network, Board, System), averaging every reading of that kind; Ungrouped is unchanged (heading + one row per sensor + capped overflow-average row). CPU and Clocks were also two disconnected top-level sections describing the same silicon. Merged them into one add_cpu_section(): Grouped now shows one aggregate CPU temperature row plus one row per cpufreq cluster (existing max_khz tier-grouping, unchanged algorithm -- verified on O6N this already equals real topology: cluster_id sysfs gives 5 clusters mapping exactly 1:1 onto cpufreq policy0/2/6/8/10). Ungrouped shows the same merge but with every individual temperature sensor and every individual cpufreq policy listed under one CPU heading instead of two. Deliberately did NOT attach a per-cluster temperature to each clock row: CIX Sky1 only defines 4 named thermal zones (CPU_B0/B1, CPU_M0/M1 -- exhaustively verified against every scmi_sensors hwmon label, 21 total sensors, no 5th CPU zone) against 5 cpufreq clusters. The two do not partition the cores the same way and nothing in sysfs links a named thermal zone to the core numbers it measures, so a per-cluster temperature would be fabricated, not measured. Verified: rebuilt via build-singularity.sh against this checkout, deployed to O6N, rebooted clean, greeter healthy.
|
Pushed 8 more commits on top of the last review. Scope note first: these bring in CPU/memory/disk utilization display in the same popover (3 commits) alongside the clock-grouping work (5 commits) — they turned out to share enough of What's new, most-recent first:
All tested against live O6N hardware (7.2.0-sky1-ncz, both Mali and Panthor GPU drivers) — rebuilt via |
2b18a7e to
19e6662
Compare
1 participant
Depends on singularityos-lab/libsingularity#11 — this PR's panel code calls
SensorMonitor/Severity/heat_fraction/the widenedSensorKindset added there. Please review/merge #11 first (or review both together; happy to rebase whichever order works for you).What this adds (final state)
A Sensors panel item, built up over 7 commits, each with the on-hardware reasoning in its own message:
libsingularity-system'sSensorMonitor(not reading sysfs directly from the shell).scmi_sensorschip that names everything by label rather than chip name (the classification fix in fix: complete Batch 1 fixes #11).Tested against live O6N hardware throughout development (see #11's commit messages for the exact before/after sensor counts and readings this panel is now rendering).
Scope note
This PR carries only the sensor-panel work. Two related-but-separate downstream fixes (lockscreen keyboard-grab timing, hot-corner-hint suppression) are intentionally NOT included here — happy to open those separately if useful, but wanted to keep this one focused on sensors per your usual preference for split PRs.