feat: Thermal service state-tracking implementation + tests#56
Open
dymk wants to merge 5 commits into
Open
Conversation
dymk
force-pushed
the
dymk/thermal-state-tracking
branch
4 times, most recently
from
0892334 to
47ca4cb
Compare
There was a problem hiding this comment.
Pull request overview
Implements a stateful Thermal service in ec-service-lib that persists threshold/variable data across requests and adds a comprehensive suite of unit tests validating round-trip behavior and several edge cases. Also tightens the repo’s pre-commit checks by treating Clippy warnings as errors and ensuring tests run against the host target.
Changes:
- Add in-memory state to
Thermalfor per-sensor thresholds, cooling policy tracking, and a small UUID→u32 variable store with LRU eviction. - Update Thermal opcode handlers to read/write that state and return richer responses (e.g.,
get_thresholdreturning values). - Add a large unit test module covering round-trips, out-of-bounds handling, unknown UUIDs, and LRU eviction; update pre-commit to run clippy with
-D warningsand run tests on the host target explicitly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ec-service-lib/src/services/thermal.rs | Adds Thermal state, implements stateful opcode behavior, and introduces unit tests for round-trip + edge cases. |
| .husky/pre-commit | Makes clippy warnings fatal and runs tests explicitly for the host target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cargo test without --target fails locally when rust-toolchain.toml installs aarch64-unknown-none-softfloat, because no_std-incompatible transitive deps (futures-timer, slab) are resolved for that target. Using the host target from rustc -vV ensures portability across Linux, macOS, and Windows.
- Add MAX_SENSORS (8) and MAX_VARS (16) constants - Add ThresholdData, CoolingPolicyData state structs - Add ThresholdRsp response struct with DirectMessagePayload conversion - Add CoolingPolicyReq request struct with DirectMessagePayload parsing - Replace empty Thermal struct with state arrays (thresholds, cooling_policies, variables) - Add LRU tracking fields (var_count, var_timestamps, var_clock) - Initialize Thermal::new() with nil UUIDs and zero timestamps
… tests - Add #[cfg(test)] mod tests with 6 helper functions - Add 4 core tests: threshold round-trip, variable round-trip, cooling policy, temperature response - Use to_bytes_le() for UUID serialization (matches from_slice_le deserialization) - Temperature test verifies TempRsp serialization (Yield::exec() untestable in unit tests)
- Add 8 edge case tests: unset defaults, unknown UUID error, threshold/variable upsert, LRU eviction, sensor bounds checking, sensor independence, cooling policy bounds - Remove unused get_temperature_payload helper (temperature tested via TempRsp serialization) - LRU eviction test verifies correct eviction of least-recently-used entry - Total: 12 thermal tests + 50 TPM tests = 62 passing
Addresses code review WR-01: set_variable accepted any len value silently, creating an asymmetry where stored variables could become unretrievable via get_variable (which requires len == 4). Now rejects non-DWORD writes with status -1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dymk
force-pushed
the
dymk/thermal-state-tracking
branch
from
47ca4cb to
8666ead
Compare
philgweber
previously approved these changes
Apr 15, 2026
kurtjd
previously approved these changes
Apr 15, 2026
dymk
dismissed stale reviews from kurtjd and philgweber
The merge-base changed after approval.
philgweber
previously approved these changes
Apr 15, 2026
dymk
dismissed
philgweber’s stale review
The merge-base changed after approval.
kurtjd
previously approved these changes
Apr 15, 2026
This was referenced Apr 16, 2026
rogurr
previously approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implement stateful thermal service with state-tracking across all 6 opcodes
(get_temperature, set/get_threshold, set/get_variable, get_policy). Adds
comprehensive unit tests covering core round-trip behavior and edge cases.
Changes
Files changed
ec-service-lib/src/services/thermal.rs