Skip to content

Restore the CPU codename detection a merge resolution discarded - #33

Merged
ManupaKDU merged 3 commits into
masterfrom
fix/restore-cpu-codename-detection
Sep 5, 2026
Merged

Restore the CPU codename detection a merge resolution discarded#33
ManupaKDU merged 3 commits into
masterfrom
fix/restore-cpu-codename-detection

Conversation

@manupawickramasinghe

@manupawickramasinghe manupawickramasinghe commented Sep 5, 2026

Copy link
Copy Markdown
Member

The margin this protects

SensorView's CPU table names six current parts that neither LibreHardwareMonitor nor upstream OpenHardwareMonitor identifies at all: Bartlett Lake, Clearwater Forest, Wildcat Lake, Panther Lake-R, Nova Lake and Diamond Rapids. LHM's IntelCpu.cs switches on only families 0x06 and 0x0F with a highest model of 0xCC, so Nova Lake (12h) and Diamond Rapids (13h) are structurally unreachable for it — the same family-only bug this table fixed.

That margin was silently deleted on master three commits ago. This PR restores it, corrects three CPU mislabels found while reviewing it, and makes the whole class of loss harder to repeat.

What happened

PR #30 replaced codename_for's family-only matching with real family+model tables. PR #31 rewrote the memory-type decoder in the same file, from a branch cut before #30 landed. The merge in 6b59f28 resolved that conflict by taking #31's side of sysinfo.rs wholesale — reverting #30 entirely:

Nothing caught it. Both PRs were green and the merge was clean. The regression is a deletion, so the file still looked healthy, and the tests that would have failed were removed alongside the code.

Status: on origin/master, but unreleased — v2.0.1 (2026-07-31) predates it. No hotfix urgency; this should land before the next tag.

Bugs found and fixed during review

The restored table had a defect of its own, in the same class it exists to prevent. Each of AMD families 17h, 19h and 1Ah ships a Threadripper at model 08h, right beside the server part at model 01h/02h. All three server arms were written as a 0x00..=0x0f range around one sourced model, swallowing all three Threadrippers:

CPUID Real part Reported as
17h/08 Colfax — TR 2990WX "Summit Ridge/Naples (Zen)"
19h/08 Chagall — TR PRO 5995WX "Milan (Zen 3)"
1Ah/08 Shimada Peak — TR PRO 9995WX "Turin (Zen 5)"

Three desktop workstations each claiming to be a server part. The 17h case is worst: Pinnacle Ridge is Zen+, so a Ryzen 2700X (00800F82) got the wrong generation too.

recog_amd.c — already cited in these comments — names model 8 separately in all three families. Models it names individually are now matched individually; ranges are used only where AMD ships a block under one codename. While there, 17h gained Raven Ridge, Picasso, Rome, Renoir and Matisse; 19h gained Genoa and Storm Peak. Intel model 0x27 also moved from Bonnell to Saltwell, per intel-family.h (SALTWELL_MID 0x27; BONNELL 0x1C, BONNELL_MID 0x26).

Commits — read them in order

Commit Diff
1 Pin line endings 6 files, pure EOL churn
2 Restore the codename tables 225+/18− — the core change
3 Pin against upstream PR openhardwaremonitor#1671 tests only
4 Say 12h/13h, not 18h/19h comment only
5 Stop three Threadrippers reporting EPYC the mislabel fix + 5 tests

Commit 1 is line-ending noise and nothing elsegit diff --ignore-cr-at-eol reports zero content change; review it with ?w=1. It comes first deliberately: five of the fifty-five Rust sources were CRLF, which made a 9-line change to firmware.rs render as 941 lines. That noise is precisely what let a 342-line deletion pass as clean. .gitattributes now pins *.rs to LF; the C# tree is marked -text and left byte-for-byte alone (verified: zero .cs files affected).

Commit 4 is small but worth a glance: the doc comment called Nova Lake family "18h" and Diamond Rapids "19h", because intel-family.h writes them in decimal (IFM(18, …), IFM(19, …)) and someone appended an h. 19h is AMD's Zen 3/Zen 4 family, named as such 30 lines above, so the two tables appeared to contradict each other.

On the upstream PRs

Upstream OHM PR openhardwaremonitor#1671 "New Intel Architectures" is the only open upstream PR that overlaps this code. It cannot be merged — it edits the C# tree, which CI never compiles; the shipping Windows path is LibreHardwareMonitorLib 0.9.* via the sidecar. It was used as a second opinion and checked model by model.

Every architecture it adds was already covered. Three of its choices were not followed, each pinned by a test:

  • 0xAB (claimed Meteor Lake) and 0xBC (claimed Lunar Lake) appear in neither intel-family.h nor LHM's IntelCpu.cs. Two independent sources having no such model, and the PR citing none, is not enough to name a part — they report Intel (family 6h).
  • 0xBE is grouped with Raptor Lake by New Intel Architectures openhardwaremonitor/openhardwaremonitor#1671; both sources call it Alder Lake-N, so the PR is the outlier.

Known, deliberately not fixed

sysinfo.rs:49 applies ExtendedModel unconditionally:

let model = (ext_model << 4) | base_model;

Intel SDM and the AMD APM both restrict it to base family 0x6 or 0xF. Benign today — every family this table names has a base family of 6 or 0xF — and it predates this PR, so it stayed out of a restore. Flagged for the architect's call.

Out of scope

Four upstream PRs cover hardware LHM 0.9.6 genuinely lacks — IT8669E (openhardwaremonitor#1384), IT8260E (openhardwaremonitor#1032), Nvidia Tesla (openhardwaremonitor#1618), Framework Chromium EC (openhardwaremonitor#1551). On Windows that layer is LibreHardwareMonitor's, so the fix belongs upstream there and arrives on the next 0.9.* bump.

Also skipped: the .NET-upgrade PRs (openhardwaremonitor#1601, openhardwaremonitor#1532, openhardwaremonitor#1352, openhardwaremonitor#1389) — the sidecar is already net8.0 — and openhardwaremonitor#1341's Stopwatch optimization, which has no Rust analogue; poll.rs already uses Instant with deadline scheduling.

Verification

  • cargo test172 passed, 0 failed (25 in this module: 9 restored, 7 memory, 9 added)
  • cargo clippy --all-targets -- -D warnings — clean
  • End-to-end on a Ryzen 7 7700 (00A60F12 → family 19h, model 61h): sensorview info reports Raphael (Zen 4). The unit tests pass family/model as arguments so they run on every CI leg including aarch64; this run covers the CPUID bit-extraction path into codename_for.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SrSpbo4TKd7ZrsDZWMvjKN

manupawickramasinghe and others added 3 commits September 5, 2026 13:52
Five of the fifty-five Rust sources were committed with CRLF and the other
fifty with LF. Touching one of the five from a checkout with different
`core.autocrlf` settings produced a whole-file rewrite in the diff: 941 lines
on `firmware.rs`, 349 on `dvfs.rs`, 790 on `summary_window.rs`. Under
`--ignore-cr-at-eol` those same diffs are 9, 7 and 2 real lines.

That is not cosmetic, and this commit comes first for a reason. The next
commit restores a 342-line deletion that reached master unnoticed, and the
reason it was reviewable as "clean" is that the line-ending noise in the
files beside it looked exactly the same in a diff.

`text eol=lf` stores and checks out LF regardless of a contributor's
`core.autocrlf`, so from here a diff means a change.

Scoped to the tree CI builds. The C# sources are marked `-text` and left
byte-for-byte alone — they are reference material the build never compiles,
and normalising them would bury every later commit under thousands of
untouched lines.

The .rs changes here are pure `git add --renormalize` output;
`git diff --ignore-cr-at-eol` reports no content difference.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SrSpbo4TKd7ZrsDZWMvjKN
PR #30 (5275a22, 47096c0) replaced `codename_for`'s family-only matching with
real family+model tables for both vendors. PR #31 (dd4ee23, fccf627) rewrote
the memory-type decoder in the same file, from a branch cut before #30 landed.
The merge in 6b59f28 resolved that conflict by taking #31's side of
`sysinfo.rs` wholesale, so #30 was reverted in its entirety: `intel_codename`
and `amd_codename` gone, `codename_for` back to `0x6 => "Intel Core"`, and all
nine of its tests deleted with it.

Nothing caught this. Both PRs were green, the merge was clean once resolved,
and the regression is a *deletion* — the memory-type work #31 was actually for
survived intact, so the file still looked healthy. The tests that would have
failed were removed by the same resolution that caused the failure.

This re-applies #30's tables onto the current file rather than reverting
6b59f28, which would have taken the memory-type decoder down with it.

The acceptance condition is that both features coexist, because restoring one
by re-truncating the other is the failure mode here:

  - `codename_for` dispatches to `intel_codename` / `amd_codename`
  - `smbios_memory_type` still decodes the full 0x01..=0x24 range
  - `summary_window` still calls `memory_type_label`
  - 16 tests in the module — #30's nine and #31's seven — all pass

163 tests pass; `clippy --all-targets -D warnings` is clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SrSpbo4TKd7ZrsDZWMvjKN
…del numbers

Upstream OpenHardwareMonitor PR openhardwaremonitor#1671 "New Intel Architectures" is the only
open upstream PR that overlaps this code. It is not mergeable — it edits the
C# tree, which is reference material the build never compiles — so it was
used as a second opinion on the model numbers and checked model by model.

Result: every architecture it adds was already covered. The table needs no
change. What it needed was a record of the check, so the next person to find
openhardwaremonitor#1671 does not redo it.

Three of its choices were deliberately not followed, and those are the tests
worth having:

  - 0xAB (Meteor Lake) and 0xBC (Lunar Lake) appear in neither the Linux
    kernel's intel-family.h nor LibreHardwareMonitor's IntelCpu.cs — the
    engine that actually reads sensors on Windows in this app. Two
    independent sources having no such model, and the PR citing none, is
    not enough to name a part. They fall to "Intel (family 6h)".

  - 0xBE is grouped with Raptor Lake by openhardwaremonitor#1671. Both sources call it
    Alder Lake-N, so the PR is the outlier.

The last test records the other direction: Bartlett Lake, Clearwater Forest,
Wildcat Lake, Panther Lake-R, Nova Lake and Diamond Rapids are in this table
and in neither upstream OHM nor LibreHardwareMonitor. Those six are the
current margin over both, and a test is what keeps it from being lost the
way the whole table just was.

167 tests pass; clippy --all-targets -D warnings is clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SrSpbo4TKd7ZrsDZWMvjKN
Copilot AI lite review requested due to automatic review settings September 5, 2026 08:25

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ManupaKDU
ManupaKDU merged commit 34eff81 into master Sep 5, 2026
4 checks passed
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.

3 participants