Skip to content

shell(panel): a Sensors panel item — CPU/GPU/component temperature, clocks, thermal severity - #21

Open
perlowja wants to merge 13 commits into
singularityos-lab:mainfrom
perlowja:pr/sensors-panel
Open

shell(panel): a Sensors panel item — CPU/GPU/component temperature, clocks, thermal severity#21
perlowja wants to merge 13 commits into
singularityos-lab:mainfrom
perlowja:pr/sensors-panel

Conversation

@perlowja

Copy link
Copy Markdown
Contributor

Depends on singularityos-lab/libsingularity#11 — this PR's panel code calls SensorMonitor/Severity/heat_fraction/the widened SensorKind set 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:

  1. The panel item itself — CPU/GPU temperature and clock, sourced from libsingularity-system's SensorMonitor (not reading sysfs directly from the shell).
  2. Grouped into one chip with a detail popover instead of sprawling across the panel — portable across boards with wildly different sensor counts (3 on a typical x86 box, 25 on Sky1).
  3. Populated on open, not eagerly, so an idle desktop isn't polling hwmon it isn't showing.
  4. Coloured by thermal severity (from fix: complete Batch 1 fixes #11's margin-to-limit banding) and clocks shown against their own per-policy ceiling, not a single machine-wide guess.
  5. Sky1 topology identification — CPU/GPU labels resolved correctly on the scmi_sensors chip that names everything by label rather than chip name (the classification fix in fix: complete Batch 1 fixes #11).
  6. Every group gets a name and a row, each with its own heat bar — the earlier version capped the list and silently dropped whatever didn't fit; Sky1's 20-post-dedup readings all get a row now.

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.

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/components/panel/panel.vala Outdated
Comment on lines +90 to +94
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");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve the platform sensor hints for empty overrides

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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep the new sensor item out of layouts until enabled

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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/components/panel/panel.vala Outdated
}

monitor.updated.connect(on_updated);
monitor.start(interval);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Suspend sensor polling while the indicator is not mapped

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>
@perlowja

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/core/system_monitor.vala Outdated
if (_sensors == null) {
_sensors = new SensorMonitor();
_sensors.gpu_hint = "TZGT";
_sensors.cpu_hint = "TZ";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict the Sky1 CPU hint to Sky1 hardware

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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/components/panel/panel.vala Outdated
ClockReading[] clocks = monitor.clocks();
if (clocks.length > 0) {
add_heading(_("Clocks"));
for (int i = 0; i < clocks.length; i++) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bound or scroll the clock rows

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.
@perlowja

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +106 to +107
monitor.updated.connect(on_updated);
on_updated();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Start polling before hiding the uninitialized indicator

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.
@perlowja

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scroll the aggregate sensor list

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).
@perlowja

Copy link
Copy Markdown
Contributor Author

Triaged the latest Codex pass (commit 19bb9a7) — two real findings fixed in 29265a7, two already addressed.

Fixed

  • Scroll the aggregate sensor list — correct: the per-group cap bounds each section but not the total, and nine kinds x (6 rows + overflow) plus headings plus clocks reaches roughly 70 rows on the 55-sensor topology this change explicitly 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 ordinary machines render exactly as before and only genuinely oversized content scrolls.

  • Restrict the Sky1 CPU hint to Sky1 hardware — agreed on the principle, though not for the stated reason: string.contains is case-sensitive, so uppercase TZ does not actually match the lowercase x86 acpitz chip. But relying on that is a coincidence rather than a design, and these are four-character CIX Sky1 ACPI names that had no business being applied on every platform, so they are now gated on the board.

    Worth recording how that gate was written, because measuring it changed the implementation. The obvious check — DMI vendor or devicetree containing cix/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 Computer (Shenzhen) Co., Ltd. / Radxa Orion O6N, never CIX or Sky1. Shipping that would have silently reintroduced the cpu=-1 gpu=-1 bug these hints were added to fix. Detection therefore keys on the SoC own ACPI HIDs (CIXH*, 163 of which enumerate on that machine), with devicetree kept as a fallback for a DT-booted Sky1.

Already addressed

  • Start polling before hiding the uninitialized indicator (P1) — fixed in 19bb9a7, the commit this pass reviewed: monitor.refresh() now runs before the first on_updated(), so availability is established before the visibility decision and the widget cannot self-hide out of ever being mapped. Re-anchored from the previous revision.

  • Keep the new sensor item out of layouts until enabled — replied inline earlier. BarLayout already force-appends any allowed id missing from both the saved and default arrays (constructor final loop), so sensors was being injected into existing layouts before this PR touched default_right; adding it there only makes the placement deliberate instead of incidental, and showing it by default is the requirement this change implements. The real gap — that BarLayout has no way to express "allowed but deliberately off" — is upstream of this PR and worth its own change against bar_layout.vala.

@perlowja

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +164 to +167
if (!monitor.available) {
// Nothing readable on this hardware: hide rather than show zeros.
visible = false;
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep polling after sensors temporarily disappear

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 👍 / 👎.

Comment on lines +109 to +112
}
// "compatible" is NUL-separated, so match the raw buffer.
string lowered = contents.down();
if (lowered.contains("cix") || lowered.contains("sky1")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Inspect every NUL-separated DT compatible entry

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 👍 / 👎.

Comment on lines +175 to +177
int primary = monitor.cpu_millidegrees >= 0
? monitor.cpu_millidegrees
: monitor.system_millidegrees;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fall back to any available temperature for the summary

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 👍 / 👎.

Comment thread src/components/panel/panel.vala Outdated
Comment on lines +419 to +420
ClockReading[] clocks = monitor.clocks();
if (clocks.length > 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor the frequency toggle in the details

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant