refactor(rog-platform, asusd): simplify sysfs attribute parsing and error handling - #266
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR centralizes sysfs parsing, adds typed ChangesPlatform parsing and Armoury handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
asusd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rog-platform/src/asus_armoury.rs`:
- Around line 174-181: Update the value parsing in the possible_values handling
to split delimited input into tokens and create AttrValue::EnumInt only when
every token parses successfully as i32; otherwise preserve all original tokens
in AttrValue::EnumStr, including mixed or wholly non-integer values, and avoid
filter_map silently dropping entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 423eba2c-852c-4f83-8269-b3ef683cd088
📒 Files selected for processing (8)
asusd/src/asus_armoury.rsrog-aura/src/aura_detection.rsrog-platform/src/asus_armoury.rsrog-platform/src/cpu.rsrog-platform/src/gpu_pci.rsrog-platform/src/lib.rsrog-platform/src/platform.rsrog-platform/src/power.rs
💤 Files with no reviewable changes (1)
- rog-platform/src/power.rs
📜 Review details
🔇 Additional comments (8)
rog-platform/src/asus_armoury.rs (1)
16-18: LGTM!Also applies to: 38-69, 93-96
asusd/src/asus_armoury.rs (1)
65-65: LGTM!Also applies to: 283-290, 320-321, 382-383, 401-401, 658-659
rog-platform/src/lib.rs (1)
31-34: LGTM!rog-platform/src/cpu.rs (1)
9-9: LGTM!Also applies to: 320-329, 340-342
rog-platform/src/platform.rs (1)
9-9: LGTM!Also applies to: 336-338
rog-platform/src/gpu_pci.rs (2)
19-19: LGTM!Also applies to: 462-470, 616-618, 633-641, 668-669
689-694: 🎯 Functional CorrectnessNo change is required. The only caller passes
-1.0toSystemPageData, whose UI binding displaysN/Afor values below0.0. No serialization or policy consumer exists.> Likely an incorrect or invalid review comment.rog-aura/src/aura_detection.rs (1)
136-147: LGTM!
43b4018 to
d125693
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
d125693 to
7a5780a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rog-platform/src/asus_armoury.rs`:
- Around line 594-641: Update test_possible_values_parsing to create a unique
per-invocation temporary directory instead of the fixed
test_possible_values_parsing path, and assert that directory creation, each
possible_values write, and cleanup succeed. Preserve the existing parsing cases
and assertions while preventing stale data from concurrent or interrupted runs.
In `@rog-platform/src/gpu_pci.rs`:
- Around line 636-641: Remove the busy_path.exists() guard in the GPU busy
percentage lookup and call read_sysfs_parsed::<f32>(busy_path) directly,
retaining the existing vendor check and None-handling return behavior.
- Around line 462-470: Update find_slot_power to retain the first error from
reading each slot’s address instead of ignoring it; continue scanning for a
readable matching slot, return the matching power path immediately, and if none
matches return the retained PlatformError::Io before falling back to
PlatformError::NotSupported when no address read failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19b0b839-e0e3-4d9d-ad1c-df7b7d9854fa
📒 Files selected for processing (8)
asusd/src/asus_armoury.rsrog-aura/src/aura_detection.rsrog-platform/src/asus_armoury.rsrog-platform/src/cpu.rsrog-platform/src/gpu_pci.rsrog-platform/src/lib.rsrog-platform/src/platform.rsrog-platform/src/power.rs
💤 Files with no reviewable changes (1)
- rog-platform/src/power.rs
📜 Review details
🔇 Additional comments (15)
rog-platform/src/asus_armoury.rs (1)
16-18: LGTM!Also applies to: 38-68, 93-96, 170-189
asusd/src/asus_armoury.rs (1)
65-65: LGTM!Also applies to: 283-290, 320-321, 382-383, 401-401, 658-659
rog-aura/src/aura_detection.rs (1)
136-146: LGTM!rog-platform/src/lib.rs (1)
31-33: LGTM!rog-platform/src/cpu.rs (3)
9-9: LGTM!
320-329: LGTM!
340-342: LGTM!rog-platform/src/platform.rs (2)
9-9: LGTM!
336-338: LGTM!rog-platform/src/gpu_pci.rs (6)
19-19: LGTM!
616-618: LGTM!
633-635: LGTM!
668-669: LGTM!
689-693: LGTM!
694-694: 🎯 Functional CorrectnessNo change needed. Both usage values display
N/Awhen they are negative.
7a5780a to
cc0bc4d
Compare
Ghoul4500
left a comment
Ghoul4500
left a comment
There was a problem hiding this comment.
Commit history needs fixing. Final HEAD is good except some unused code
| } | ||
|
|
||
| /// Find the PCI hotplug slot power control file for a device. | ||
| pub fn find_slot_power(address: &str) -> Result<PathBuf> { |
There was a problem hiding this comment.
this is an unused function. drop any commits that bring any change to this. this together with a lot of other code related to GPU will be dropped in main very soon by me
There was a problem hiding this comment.
I didn't make any change to it though?
…y sysfs reading across modules
…e config deserialization error logging
cc0bc4d to
898e3c7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusd/src/asus_armoury.rs`:
- Around line 287-290: Update the available-attribute condition in the relevant
attribute-reading flow to add “current_value” only when the value can be
converted with value.as_i32().is_some(). Preserve the existing error-state reset
and avoid advertising values that the current_value D-Bus property cannot
return.
In `@rog-platform/src/cpu.rs`:
- Line 329: Handle the unavailable-value sentinel returned by get_cpu_temp and
get_cpu_frequency_mhz so the UI does not display -1 °C or -1 MHz, following the
existing GPU metric handling. Apply the corresponding change in
rog-platform/src/cpu.rs at lines 329-329 and 364-364; use
rog-platform/src/gpu_pci.rs lines 684-684 and 703-703 as the reference
implementation, with no direct changes required there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db51d111-feee-497b-bd09-492d581cabf8
📒 Files selected for processing (8)
asusd/src/asus_armoury.rsrog-aura/src/aura_detection.rsrog-platform/src/asus_armoury.rsrog-platform/src/cpu.rsrog-platform/src/gpu_pci.rsrog-platform/src/lib.rsrog-platform/src/platform.rsrog-platform/src/power.rs
💤 Files with no reviewable changes (1)
- rog-platform/src/power.rs
📜 Review details
🔇 Additional comments (7)
rog-platform/src/asus_armoury.rs (1)
15-18: LGTM!Also applies to: 38-53, 77-77, 150-169, 574-645
asusd/src/asus_armoury.rs (1)
68-68: LGTM!Also applies to: 327-328, 389-390, 410-410, 668-669
rog-aura/src/aura_detection.rs (1)
136-147: LGTM!rog-platform/src/lib.rs (1)
31-33: LGTM!rog-platform/src/cpu.rs (1)
9-9: LGTM!Also applies to: 320-322, 340-342
rog-platform/src/gpu_pci.rs (1)
19-19: LGTM!Also applies to: 627-629, 644-651, 677-680, 698-700
rog-platform/src/platform.rs (1)
9-9: LGTM!Also applies to: 336-338
| Ok(value) if !matches!(value, AttrValue::None) => { | ||
| self.logged_read_error.store(false, Ordering::Relaxed); | ||
| if matches!(value, AttrValue::Integer(_)) { | ||
| attrs.push("current_value".to_string()); | ||
| } | ||
| attrs.push("current_value".to_string()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not advertise a property that this D-Bus interface cannot read.
AttrValue::String passes this condition and adds current_value to available_attrs. The current_value property returns i32 only, so it then returns fdo::Error::Failed for the same value.
Only add current_value when value.as_i32().is_some(), or extend the D-Bus API to expose string values.
Proposed fix
- Ok(value) if !matches!(value, AttrValue::None) => {
+ Ok(value) => {
self.logged_read_error.store(false, Ordering::Relaxed);
- attrs.push("current_value".to_string());
+ if value.as_i32().is_some() {
+ attrs.push("current_value".to_string());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Ok(value) if !matches!(value, AttrValue::None) => { | |
| self.logged_read_error.store(false, Ordering::Relaxed); | |
| if matches!(value, AttrValue::Integer(_)) { | |
| attrs.push("current_value".to_string()); | |
| } | |
| attrs.push("current_value".to_string()); | |
| } | |
| Ok(value) => { | |
| self.logged_read_error.store(false, Ordering::Relaxed); | |
| if value.as_i32().is_some() { | |
| attrs.push("current_value".to_string()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/asus_armoury.rs` around lines 287 - 290, Update the
available-attribute condition in the relevant attribute-reading flow to add
“current_value” only when the value can be converted with
value.as_i32().is_some(). Preserve the existing error-state reset and avoid
advertising values that the current_value D-Bus property cannot return.
898e3c7 to
4971164
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
asusd/src/asus_armoury.rs (1)
287-290:⚠️ Potential issue | 🟠 MajorDo not advertise an unreadable
current_value.
AttrValue::Stringis non-None, so this branch advertises"current_value". The D-Bus property returnsi32, whileAttrValue::as_i32()returnsSomeonly forAttrValue::Integerinrog-platform/src/asus_armoury.rsat Lines 38-45. A string current value is therefore advertised but the property call returnsfdo::Error::Failed. Gate this entry onvalue.as_i32().is_some()or add a string-valued property. This is the same unresolved finding from the previous review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@asusd/src/asus_armoury.rs` around lines 287 - 290, Update the current_value advertisement branch to add "current_value" only when the returned AttrValue can be converted with value.as_i32().is_some(), rather than merely being non-None. Preserve the logged_read_error reset, and keep non-integer values from advertising a property whose D-Bus type is i32.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@asusd/src/asus_armoury.rs`:
- Around line 287-290: Update the current_value advertisement branch to add
"current_value" only when the returned AttrValue can be converted with
value.as_i32().is_some(), rather than merely being non-None. Preserve the
logged_read_error reset, and keep non-integer values from advertising a property
whose D-Bus type is i32.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5ecbebb-8b0e-42a5-b040-44155aa10362
📒 Files selected for processing (8)
asusd/src/asus_armoury.rsrog-aura/src/aura_detection.rsrog-platform/src/asus_armoury.rsrog-platform/src/cpu.rsrog-platform/src/gpu_pci.rsrog-platform/src/lib.rsrog-platform/src/platform.rsrog-platform/src/power.rs
💤 Files with no reviewable changes (1)
- rog-platform/src/power.rs
📜 Review details
🔇 Additional comments (8)
rog-platform/src/asus_armoury.rs (1)
15-18: LGTM!Also applies to: 38-53, 77-77, 150-169, 573-645
asusd/src/asus_armoury.rs (1)
68-68: LGTM!Also applies to: 291-297, 327-328, 389-390, 410-410, 668-669
rog-aura/src/aura_detection.rs (1)
136-147: LGTM!rog-platform/src/cpu.rs (2)
327-329: Re-check the CPU unavailable sentinel in downstream consumers.
get_cpu_temp()andget_cpu_frequency_mhz()now return-1.0when no value is available. The previous review already found that the UI rendered these values as-1 °Cand-1 MHz. Verify that current consumers map-1.0to an unavailable state before merging.#!/usr/bin/env bash set -euo pipefail rg -n -C 6 \ '\b(get_cpu_temp|get_cpu_frequency_mhz)\s*\(|cpu_temp_val|cpu_freq_mhz' \ . --glob '*.rs' --glob '*.slint'Also applies to: 361-364
9-9: LGTM!Also applies to: 320-321, 340-342
rog-platform/src/lib.rs (1)
31-33: LGTM!rog-platform/src/gpu_pci.rs (1)
19-19: LGTM!Also applies to: 627-629, 644-651, 677-684, 698-703
rog-platform/src/platform.rs (1)
9-9: LGTM!Also applies to: 336-338
2 participants
Description
This PR refactors sysfs reading, attribute parsing, and error handling across
rog-platform,asusd, androg-aurato eliminate redundant boilerplate, unnecessary filesystemstatcalls, and nestedif let Ok(...)statements.Rationale & Key Improvements:
Generic Sysfs Parser (
read_sysfs_parsed<T>):Introduced a shared helper in
rog-platformto read, trim, and parse sysfs attributes in a single line. Replaced 4-level deepif let Ok(...)nests incpu.rs,gpu_pci.rs, andplatform.rswith clean functional expressions.AttrValueConversion Helpers:Added
.as_i32()and.as_str()helper methods onAttrValueinrog-platform/src/asus_armoury.rs. Refactoredasusddaemon methods (reset_to_default,current_valueproperty getter, andreload) to use type-safe helper methods instead of manualif let AttrValue::Integer(i)pattern matching.current_value&read_i32Cleanup:Simplified
current_value(&self)by using?for early error propagation and concise match parsing onval.parse::<i32>(). Refactoredread_i32to reuseread_string.Elimination of Redundant Sysfs
exists()Syscalls:Removed
if !path.exists()checks prior tostd::fs::read_to_stringinpower.rs.read_to_stringalready returnsio::ErrorKind::NotFoundif the node is absent, eliminating duplicatestatsyscalls.Improved Error Logging:
Updated user-defined LED support config parsing in
rog-aura/src/aura_detection.rsto match default config handling and log explicit deserialization errors if RON parsing fails.Tested Hardware & Environment
Verification and testing:
cargo fmt --all -- --check)cargo clippy --all -- -D warnings/cargo check --all-targets)cargo test --all)cargo cranky)