refactor: Not meant for direct merge (Read note) - #211
Conversation
b216791 to
8c1e173
Compare
279cf5f to
434be53
Compare
…dware feature detection
…erator combinators
…ync into rog-dbus
434be53 to
4775aab
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change consolidates platform, DMI, SCSI, Slash, DBus, daemon, and CLI functionality into shared modules. It adds command handlers, capability detection, SG_IO support, task supervision, blocking DBus discovery, and removes legacy crate dependencies. ChangesPlatform consolidation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…to return JoinSet and JoinHandle
scardracs
changed the title
scardracs
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rog-dbus/src/lib.rs (1)
57-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate
find_iface_async/find_iface_blockingpath-filtering logic.Both functions repeat the identical loop that filters
ManagedObjectsby interface name, warns on multiple matches, and sorts the paths. Only the connection setup and.awaitdiffer. Extracting a small sync helper (e.g.fn matching_paths(interfaces: &zbus::fdo::ManagedObjects, iface_name: &str) -> Vec<OwnedObjectPath>) used by both would remove the duplication the PR is otherwise aiming to reduce, and prevents the two copies drifting further (the blocking version already dropped the redundant.clone()that the async version still has).♻️ Suggested helper extraction
+fn matching_paths( + interfaces: &std::collections::HashMap< + zbus::zvariant::OwnedObjectPath, + std::collections::HashMap<zbus::names::OwnedInterfaceName, std::collections::HashMap<String, zbus::zvariant::OwnedValue>>, + >, + iface_name: &str, +) -> Vec<zbus::zvariant::OwnedObjectPath> { + let mut paths = Vec::new(); + for v in interfaces.iter() { + for k in v.1.keys() { + if k.as_str() == iface_name { + paths.push(v.0.clone()); + } + } + } + if paths.len() > 1 { + log::warn!("Multiple asusd interface devices found"); + } + paths.sort(); + paths +}Both
find_iface_asyncandfind_iface_blockingcan then call this instead of repeating the loop.Also applies to: 95-129
🤖 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 `@rog-dbus/src/lib.rs` around lines 57 - 94, Extract the shared interface-path filtering, multiple-match warning, and sorting logic from find_iface_async and find_iface_blocking into a synchronous matching_paths helper returning the appropriate OwnedObjectPath collection. Replace both duplicated loops with calls to this helper, preserving each function’s existing connection setup and async/blocking proxy construction; also remove the async-only redundant path clone when consuming the helper result.
🤖 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 `@asusctl/src/anime_cli.rs`:
- Around line 193-198: Update the anime type initialization around
get_anime_type and cmd.override_type so that when the result remains
AnimeType::Unsupported, emit a warning directing the user to --override-type
before proceeding. Hoist this anime_type resolution and warning outside the for
proxy loop because it is loop-invariant, while preserving the override behavior.
- Around line 154-204: The handle_anime function accepts off_with_his_head in
its argument guard but never applies it; wire cmd.off_with_his_head to the
appropriate AnimeProxyBlocking operation, or remove the flag and its guard
condition if no corresponding proxy API exists. Ensure the option cannot be
accepted while having no effect.
In `@asusctl/src/fan_curve_cli.rs`:
- Around line 46-96: Move FanCurvesProxyBlocking::new construction in
handle_fan_curve below both argument-validation guards, so missing or
incompatible arguments return their existing usage warnings without any D-Bus
calls. Construct the fan proxy only after validation, and keep
PlatformProxyBlocking::new after the guards as well.
- Around line 97-109: Update the fan-curve CLI handling around the
`cmd.enable_fan_curve` and `cmd.data` branches to require `cmd.fan` whenever
either operation is requested. Replace the misleading `REQ_MOD_PROFILE_MSG`
warning with the existing or appropriate missing-`--fan` message, ensure the
`enable_fan_curve` path can reach this validation, and remove the
`FanCurvePU::default()` fallback so `set_fan_curve` is never called without an
explicitly selected fan.
In `@asusctl/src/platform_cli.rs`:
- Line 514: Remove the stale #[allow] attribute associated with
handle_armoury_command, since neither clippy::manual_is_multiple_of nor
clippy::nonminimal_bool applies there; only retain or relocate those allowances
if the actual triggering function requires them.
- Around line 21-37: Update check_service so its return value clearly represents
service health, preferably returning true only when the service is enabled and
active; adjust the failure branches and the main.rs caller to use the renamed or
inverted result so “Is asusd.service running?” reports the correct state.
- Around line 275-301: Validate CLI argument combinations before any D-Bus proxy
creation or device iteration. In asusctl/src/platform_cli.rs:275-301, move the
power.awake/sleep/boot/keyboard/lightbar emptiness guard before
find_iface_blocking in handle_led_power1. In asusctl/src/fan_curve_cli.rs:46-96,
move the existing argument guards before FanCurvesProxyBlocking::new; preserve
the current usage responses while ensuring invalid input cannot produce device
or transport errors.
- Around line 137-172: Update handle_backlight to call
find_iface_blocking::<BacklightProxyBlocking> only once before the
read-versus-update branch. Reuse the resulting backlights collection for
displaying current settings when no setter options are supplied and for applying
setters otherwise, preserving both existing behaviors.
- Line 72: Remove the find_iface_blocking re-export from platform_cli and import
rog_dbus::find_iface_blocking directly in callers such as anime_cli. In the
aura-interface lookup handling around lines 90-105, eliminate the duplicate “No
aura interface found” warning and log the underlying error in the Err arm while
retaining the existing behavior for successful and unavailable-interface cases.
- Around line 517-526: Update the ArmourySubCommand::List arm to propagate
errors from find_iface_blocking::<AsusArmouryProxyBlocking> instead of silently
ignoring the Err result. Preserve the existing attribute iteration and
print_firmware_attr behavior for successful lookups, matching the error
propagation used by the Get and Set arms.
- Around line 341-386: Refactor the power-command handling to destructure
power.command once, eliminating the redundant outer and inner Option checks.
Update the set closure used by the command match to explicitly handle a
requested PowerZones value that is absent from states.states, either by adding a
corresponding AuraPowerState or warning that the zone is unsupported; do not
silently write unchanged state. Preserve the existing per-zone field updates for
entries that are present.
In `@asusctl/src/scsi_cli.rs`:
- Around line 53-69: Update the colour assignment loop in the command handling
flow to use a match on the enumerated index for the four supported colour
fields, and explicitly warn or report when additional colours exceed the fourth
value instead of silently ignoring them. Preserve the existing assignments to
mode.colour1 through mode.colour4 and do_update behavior.
- Around line 33-36: Update the initial guard in handle_scsi to include
cmd.speed and cmd.direction when determining whether any actionable argument was
provided. Preserve the warning and return immediately when all ScsiCommand
options are empty, while allowing valid speed or direction invocations to
proceed without warning.
In `@asusd/src/aura_anime/trait_impls.rs`:
- Around line 449-455: The repeated JoinSet supervisor loops should be
centralized in a shared helper on CtrlTask. In asusd/src/lib.rs, add
spawn_task_supervisor accepting the log label and JoinSet, spawning the existing
drain-and-warn behavior; replace the inline loops in
asusd/src/aura_anime/trait_impls.rs:449-455,
asusd/src/aura_laptop/trait_impls.rs:295-301, and
asusd/src/ctrl_platform.rs:1118-1124 with calls using “AniMeZbus”, “AuraZbus”,
and “CtrlPlatform” respectively.
In `@asusd/src/ctrl_backlight.rs`:
- Around line 153-155: Consume the optional JoinHandle returned by
CtrlBacklight::start_watch_primary at its daemon.rs call site instead of
discarding it. Supervise the handle asynchronously and report JoinError when the
watcher task terminates, preserving the existing initialization flow; apply the
same handling to the related start-watch paths noted in the comment, or remove
the returned handle if supervision is intentionally not supported.
In `@asusd/src/ctrl_platform.rs`:
- Around line 943-950: Update the set_charge_control_end_threshold call in the
!sleeping branch to log failures before discarding the Result, matching the
warn!/error! behavior used by sibling closures in the same function. Preserve
the existing threshold-setting flow and only replace the silent .ok() handling
with contextual error logging.
- Around line 1066-1073: Preserve the watcher’s JoinError for JoinSet
supervision by spawning the JoinHandle returned from
watch_charge_control_end_threshold directly, rather than wrapping h in a task
that ignores h.await. Update the tasks.spawn call in this charge-threshold
watcher block so join_next can report watcher panics or aborts to the existing
warning path.
In `@asusd/src/daemon.rs`:
- Around line 35-45: Update the IS_SERVICE environment parsing so an unset
variable produces false rather than true. Preserve the existing "1" value as
true, allowing the !is_service warning and early return path to run when the
daemon is launched outside the systemd service.
In `@asusd/src/lib.rs`:
- Line 230: Update create_sys_event_tasks so the callback future types Fut1–Fut4
include 'static alongside their existing Send bounds, ensuring every future
passed to JoinSet::spawn satisfies its required lifetime constraint.
In `@rog-platform/src/capabilities.rs`:
- Around line 40-41: Update the has_fan_curves capability check to remove the
fan_boost_mode probe and rely only on the actual writable fan-curve attribute,
such as pwm1_auto_point1_pwm. Preserve the existing capability behavior for
systems exposing the curve points.
In `@rog-platform/src/cpu.rs`:
- Line 90: Update the remaining stale no-CPU error messages in get_available_epp
and the other affected CPU getter branches to return “No CPUs found,” matching
the existing PlatformError::CPU contract used by the nearby branches.
In `@rog-platform/src/scsi/builtin_modes.rs`:
- Around line 300-308: Update the Display implementation for ModeData in fmt to
include colour3 and colour4 alongside the existing colour fields, so
confirmation output from handle_scsi visibly reports all four applied colours.
- Around line 351-463: Replace the repetitive match in AuraMode::to_tasks with
per-mode capability flags describing whether each mode emits rgb, dir, speed,
and mode tasks. Build tasks from those flags in the existing ordering, then
retain apply_task and save_task for every mode; preserve the documented
RandomFlicker omission of dir_task and make adding modes require only updating
the capability mapping.
- Around line 448-458: Update mode_task() used by AuraMode::DoubleFade
serialization so its upper clamp allows value 14, or derive the limit from the
enum’s maximum variant. Ensure to_tasks() preserves AuraMode::DoubleFade as mode
14 rather than serializing it as RandomFlicker.
In `@rog-platform/src/scsi/sg.rs`:
- Around line 114-121: Update Task::new() to initialize the inner SgIoHdr
timeout field to a sensible non-zero default instead of relying on
SgIoHdr::default(). Preserve the existing empty command, data, and sense
buffers, and ensure task builders inherit this timeout unless explicitly
overridden.
- Around line 93-111: Replace the derived Clone implementation for Task with a
manual clone that duplicates the owned cmd, data, and sense buffers, then
rebuilds inner’s raw pointers to reference the clone’s buffers rather than
copying the original pointers. Remove the unsafe Sync implementation and update
the safety comment to describe only the remaining Send guarantee and pointer
invariant.
- Around line 244-255: Update perform’s SG_IO request declaration to use
libc::Ioctl on all targets instead of the target_env-based u64/i32 split, then
pass that libc::Ioctl value directly to libc::ioctl. Preserve the existing
synchronous ioctl call and safety handling.
In `@rog-platform/src/slash/usb.rs`:
- Line 150: Update the packet construction around pkt2[9] to write the
GA605-specific byte value 0x10 for both GA605_2024 and GA605_2025, while
preserving the existing 0x19 value for other device variants.
---
Outside diff comments:
In `@rog-dbus/src/lib.rs`:
- Around line 57-94: Extract the shared interface-path filtering, multiple-match
warning, and sorting logic from find_iface_async and find_iface_blocking into a
synchronous matching_paths helper returning the appropriate OwnedObjectPath
collection. Replace both duplicated loops with calls to this helper, preserving
each function’s existing connection setup and async/blocking proxy construction;
also remove the async-only redundant path clone when consuming the helper
result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d77d82a-4c7a-4b38-83e1-6bc44ed3751b
📒 Files selected for processing (57)
Cargo.tomlasusctl/Cargo.tomlasusctl/src/anime_cli.rsasusctl/src/fan_curve_cli.rsasusctl/src/main.rsasusctl/src/platform_cli.rsasusctl/src/scsi_cli.rsasusctl/src/slash_cli.rsasusd/Cargo.tomlasusd/src/aura_anime/trait_impls.rsasusd/src/aura_laptop/trait_impls.rsasusd/src/aura_manager.rsasusd/src/aura_scsi/config.rsasusd/src/aura_scsi/mod.rsasusd/src/aura_scsi/trait_impls.rsasusd/src/aura_slash/config.rsasusd/src/aura_slash/mod.rsasusd/src/aura_slash/trait_impls.rsasusd/src/aura_types.rsasusd/src/ctrl_backlight.rsasusd/src/ctrl_platform.rsasusd/src/daemon.rsasusd/src/error.rsasusd/src/lib.rsdmi-id/Cargo.tomlrog-anime/Cargo.tomlrog-anime/src/data.rsrog-anime/src/usb.rsrog-aura/Cargo.tomlrog-aura/src/aura_detection.rsrog-control-center/Cargo.tomlrog-control-center/src/main.rsrog-control-center/src/mocking.rsrog-control-center/src/ui/setup_slash.rsrog-control-center/src/zbus_proxies.rsrog-dbus/Cargo.tomlrog-dbus/src/lib.rsrog-dbus/src/scsi_aura.rsrog-dbus/src/zbus_slash.rsrog-platform/Cargo.tomlrog-platform/src/capabilities.rsrog-platform/src/cpu.rsrog-platform/src/dmi.rsrog-platform/src/error.rsrog-platform/src/lib.rsrog-platform/src/scsi/builtin_modes.rsrog-platform/src/scsi/mod.rsrog-platform/src/scsi/protocol.rsrog-platform/src/scsi/sg.rsrog-platform/src/slash/data.rsrog-platform/src/slash/mod.rsrog-platform/src/slash/usb.rsrog-profiles/src/fan_curve_set.rsrog-scsi/Cargo.tomlrog-scsi/src/error.rsrog-slash/Cargo.tomlrog-slash/src/error.rs
💤 Files with no reviewable changes (8)
- asusctl/Cargo.toml
- dmi-id/Cargo.toml
- rog-slash/src/error.rs
- rog-scsi/Cargo.toml
- rog-slash/Cargo.toml
- Cargo.toml
- rog-scsi/src/error.rs
- rog-control-center/Cargo.toml
📜 Review details
🔇 Additional comments (41)
rog-profiles/src/fan_curve_set.rs (1)
29-36: LGTM!rog-platform/src/slash/data.rs (1)
6-10: LGTM!rog-platform/src/slash/mod.rs (1)
6-7: LGTM!rog-platform/src/slash/usb.rs (1)
11-14: LGTM!Also applies to: 37-37
asusd/src/aura_slash/config.rs (1)
2-2: LGTM!asusd/src/aura_slash/mod.rs (1)
5-7: LGTM!asusd/src/aura_slash/trait_impls.rs (1)
3-8: LGTM!asusd/src/aura_types.rs (1)
11-12: LGTM!Also applies to: 84-84
rog-dbus/src/zbus_slash.rs (1)
1-1: LGTM!rog-control-center/src/ui/setup_slash.rs (1)
7-7: LGTM!asusctl/src/slash_cli.rs (1)
2-4: LGTM!Also applies to: 82-82, 142-149
rog-aura/Cargo.toml (1)
21-21: LGTM!rog-platform/Cargo.toml (1)
11-12: LGTM!Also applies to: 24-25
rog-platform/src/capabilities.rs (1)
1-39: LGTM!Also applies to: 42-58
rog-platform/src/dmi.rs (1)
94-94: LGTM!rog-platform/src/error.rs (1)
60-89: LGTM!Also applies to: 104-115
rog-platform/src/lib.rs (1)
6-22: LGTM!rog-anime/src/data.rs (1)
6-7: LGTM!Also applies to: 320-320
asusctl/src/anime_cli.rs (2)
296-336: LGTM!Also applies to: 338-376
200-204: 🎯 Functional CorrectnessKeep
0xFFfor--clear. This is the fill value used to blank the AniMe buffer; switching to0u8would invert the behavior.> Likely an incorrect or invalid review comment.asusctl/src/main.rs (1)
1-19: LGTM!Also applies to: 95-102, 117-135
rog-platform/src/scsi/builtin_modes.rs (1)
8-10: LGTM!Also applies to: 209-230, 232-256, 281-285
rog-platform/src/scsi/mod.rs (1)
2-5: LGTM!rog-platform/src/scsi/protocol.rs (1)
1-1: LGTM!rog-platform/src/scsi/sg.rs (2)
9-16: LGTM!Also applies to: 18-72, 74-91
124-148: LGTM!Also applies to: 264-268
asusd/src/aura_scsi/config.rs (1)
5-5: LGTM!asusd/src/aura_scsi/trait_impls.rs (1)
6-6: LGTM! SourcingAuraMode/AuraEffectfromrog_platform::scsihere matchesrog-dbus/src/scsi_aura.rs, so the D-Bus signatures on both sides now derive from one definition.asusctl/src/scsi_cli.rs (3)
2-3: LGTM!
71-96: LGTM!
47-51: 🗄️ Data Integrity & IntegrationProperty signatures already match
AuraModeuseszvariant(signature = "u"), so the getter and setter share the same D-Bus type; drop this comment.> Likely an incorrect or invalid review comment.asusd/src/aura_scsi/mod.rs (1)
4-4: 🩺 Stability & Availability
DeviceandTaskare re-exported atrog_platform::scsiroot, so this import is valid.> Likely an incorrect or invalid review comment.rog-dbus/src/scsi_aura.rs (1)
23-23: 🗄️ Data Integrity & Integration
rog-dbusalready enablesrog-platformwith thedbusfeature.> Likely an incorrect or invalid review comment.asusd/src/aura_anime/trait_impls.rs (1)
319-448: LGTM!asusd/src/aura_laptop/trait_impls.rs (1)
242-293: LGTM!asusd/src/ctrl_platform.rs (1)
1075-1081: LGTM!asusd/src/daemon.rs (1)
22-24: LGTM!Also applies to: 59-59, 160-165
rog-dbus/Cargo.toml (1)
15-18: LGTM!rog-control-center/src/mocking.rs (1)
5-6: LGTM!Also applies to: 98-125, 137-137, 152-154, 165-171, 198-202
rog-control-center/src/main.rs (1)
8-23: LGTM!Also applies to: 86-91
rog-control-center/src/zbus_proxies.rs (1)
71-91: 📐 Maintainability & Code QualityNo change needed
| pub fn handle_anime(cmd: &AnimeCommand) -> Result<(), Box<dyn std::error::Error>> { | ||
| if cmd.command.is_none() | ||
| && cmd.enable_display.is_none() | ||
| && cmd.enable_powersave_anim.is_none() | ||
| && cmd.brightness.is_none() | ||
| && cmd.off_when_lid_closed.is_none() | ||
| && cmd.off_when_suspended.is_none() | ||
| && cmd.off_when_unplugged.is_none() | ||
| && cmd.off_with_his_head.is_none() | ||
| && !cmd.clear | ||
| { | ||
| warn!("Missing arg or command; run 'asusctl anime --help' for usage"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let animes = crate::platform_cli::find_iface_blocking::< | ||
| rog_dbus::zbus_anime::AnimeProxyBlocking, | ||
| >("xyz.ljones.Anime")?; | ||
|
|
||
| for proxy in animes { | ||
| if let Some(enable) = cmd.enable_display { | ||
| proxy.set_enable_display(enable)?; | ||
| } | ||
| if let Some(enable) = cmd.enable_powersave_anim { | ||
| proxy.set_builtins_enabled(enable)?; | ||
| } | ||
| if let Some(bright) = cmd.brightness { | ||
| proxy.set_brightness(bright)?; | ||
| } | ||
| if let Some(enable) = cmd.off_when_lid_closed { | ||
| proxy.set_off_when_lid_closed(enable)?; | ||
| } | ||
| if let Some(enable) = cmd.off_when_suspended { | ||
| proxy.set_off_when_suspended(enable)?; | ||
| } | ||
| if let Some(enable) = cmd.off_when_unplugged { | ||
| proxy.set_off_when_unplugged(enable)?; | ||
| } | ||
|
|
||
| let mut anime_type = get_anime_type(); | ||
| if let AnimeType::Unsupported = anime_type { | ||
| if let Some(model) = cmd.override_type { | ||
| anime_type = model; | ||
| } | ||
| } | ||
|
|
||
| if cmd.clear { | ||
| let data = vec![255u8; anime_type.data_length()]; | ||
| let tmp = rog_anime::AnimeDataBuffer::from_vec(anime_type, data)?; | ||
| proxy.write(tmp)?; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any proxy setter/property for off_with_his_head and its usages
rg -nP -C3 'off_with_his_head' --type=rustRepository: OpenGamingCollective/asusctl
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the CLI definition and any proxy API around the relevant option/property.
rg -n -C 3 'off_with_his_head|off-with-his-head|off_with_his' asusctl/src rog_dbus -g '*.rs'
# Show the anime CLI handling area with line numbers for context.
sed -n '140,230p' asusctl/src/anime_cli.rsRepository: OpenGamingCollective/asusctl
Length of output: 1190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find anime proxy definitions and all mentions of the option/property.
fd -a 'anime' .
rg -n -C 3 'AnimeProxyBlocking|off_with_his_head|off-with-his-head|set_off_with_his_head|builtins_enabled|enable_display|off_when_lid_closed|off_when_suspended|off_when_unplugged' . -g '*.rs'
# Show candidate files if found.
fd -a 'anime.*\.rs$|anime.*\.toml$|anime.*\.json$|anime.*\.yaml$' .Repository: OpenGamingCollective/asusctl
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for anime proxy definitions and any setter/property related to the option.
fd -a 'anime' .
rg -n -C 3 'AnimeProxyBlocking|off_with_his_head|off-with-his-head|set_off_with_his_head|builtins_enabled|enable_display|off_when_lid_closed|off_when_suspended|off_when_unplugged' . -g '*.rs'
# Show any matching files that look relevant.
fd -a 'anime.*\.rs$|anime.*\.toml$|anime.*\.json$|anime.*\.yaml$' .Repository: OpenGamingCollective/asusctl
Length of output: 50384
Wire or remove --off-with-his-head The flag is accepted and satisfies the empty-argument guard, but handle_anime never reads cmd.off_with_his_head, so it currently does nothing.
🤖 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 `@asusctl/src/anime_cli.rs` around lines 154 - 204, The handle_anime function
accepts off_with_his_head in its argument guard but never applies it; wire
cmd.off_with_his_head to the appropriate AnimeProxyBlocking operation, or remove
the flag and its guard condition if no corresponding proxy API exists. Ensure
the option cannot be accepted while having no effect.
| let mut anime_type = get_anime_type(); | ||
| if let AnimeType::Unsupported = anime_type { | ||
| if let Some(model) = cmd.override_type { | ||
| anime_type = model; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Warn when the anime type stays Unsupported.
If get_anime_type() returns Unsupported and no --override-type is given, the code proceeds with a guessed data_length(), so writes fail later with an opaque buffer-length error. A warning pointing at --override-type here would make the failure actionable. Also, this is loop-invariant and can be hoisted above the for proxy loop.
🤖 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 `@asusctl/src/anime_cli.rs` around lines 193 - 198, Update the anime type
initialization around get_anime_type and cmd.override_type so that when the
result remains AnimeType::Unsupported, emit a warning directing the user to
--override-type before proceeding. Hoist this anime_type resolution and warning
outside the for proxy loop because it is loop-invariant, while preserving the
override behavior.
|
|
||
| const REQ_MOD_PROFILE_MSG: &str = | ||
| "--enable-fan-curves, --enable-fan-curve, --fan, and --data options require --mod-profile"; | ||
|
|
||
| pub fn handle_fan_curve( | ||
| conn: &zbus::blocking::Connection, | ||
| cmd: &FanCurveCommand, | ||
| ) -> Result<(), Box<dyn std::error::Error>> { | ||
| let fan_proxy = rog_dbus::zbus_fan_curves::FanCurvesProxyBlocking::new(conn).map_err(|e| { | ||
| warn!("Fan curves unavailable: {e}"); | ||
| rog_profiles::error::ProfileError::NotSupported | ||
| })?; | ||
|
|
||
| if !cmd.get_enabled && !cmd.default && cmd.mod_profile.is_none() { | ||
| warn!("Missing arg or command; run 'asusctl fan-curve --help' for usage"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if (cmd.enable_fan_curves.is_some() || cmd.fan.is_some() || cmd.data.is_some()) | ||
| && cmd.mod_profile.is_none() | ||
| { | ||
| warn!("{REQ_MOD_PROFILE_MSG}"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let plat_proxy = rog_dbus::zbus_platform::PlatformProxyBlocking::new(conn)?; | ||
| if cmd.get_enabled { | ||
| let profile = plat_proxy.platform_profile()?; | ||
| let curves = fan_proxy.fan_curve_data(profile)?; | ||
| for curve in curves.iter() { | ||
| println!("{}", String::from(curve)); | ||
| } | ||
| } | ||
|
|
||
| if cmd.default { | ||
| let active = plat_proxy.platform_profile()?; | ||
| fan_proxy.set_curves_to_defaults(active)?; | ||
| } | ||
|
|
||
| if let Some(profile) = cmd.mod_profile { | ||
| if cmd.enable_fan_curves.is_none() && cmd.data.is_none() { | ||
| let data = fan_proxy.fan_curve_data(profile)?; | ||
| let ron = | ||
| ron::ser::to_string_pretty(&data, ron::ser::PrettyConfig::new().depth_limit(4))?; | ||
| println!("\nFan curves for {:?}\n\n{}", profile, ron); | ||
| } | ||
|
|
||
| if let Some(enabled) = cmd.enable_fan_curves { | ||
| fan_proxy.set_fan_curves_enabled(profile, enabled)?; | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Move proxy construction after argument validation.
FanCurvesProxyBlocking::new at line 54 runs before the guards at lines 59-69, so the pure-usage-error paths pay a D-Bus round trip and can fail with NotSupported instead of printing the usage warning. Constructing both proxies after the guards keeps the argument feedback deterministic.
🤖 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 `@asusctl/src/fan_curve_cli.rs` around lines 46 - 96, Move
FanCurvesProxyBlocking::new construction in handle_fan_curve below both
argument-validation guards, so missing or incompatible arguments return their
existing usage warnings without any D-Bus calls. Construct the fan proxy only
after validation, and keep PlatformProxyBlocking::new after the guards as well.
| if let Some(enabled) = cmd.enable_fan_curve { | ||
| if let Some(fan) = cmd.fan { | ||
| fan_proxy.set_profile_fan_curve_enabled(profile, fan, enabled)?; | ||
| } else { | ||
| warn!("{REQ_MOD_PROFILE_MSG}"); | ||
| } | ||
| } | ||
|
|
||
| if let Some(mut curve) = cmd.data.clone() { | ||
| let fan = cmd.fan.unwrap_or_default(); | ||
| curve.set_fan(fan); | ||
| fan_proxy.set_fan_curve(profile, curve)?; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrong warning text, and a missing --fan is silently defaulted.
At line 101 --mod-profile is present by construction (we are inside if let Some(profile) = cmd.mod_profile), so REQ_MOD_PROFILE_MSG is misleading — the actual missing option is --fan. Also note cmd.enable_fan_curve is absent from the guard at line 64, so it never reaches that message path anyway.
Separately, line 106 falls back to FanCurvePU::default() when --fan is omitted, so --data without --fan writes the curve to an arbitrary fan without telling the user.
🐛 Proposed fix
if let Some(enabled) = cmd.enable_fan_curve {
if let Some(fan) = cmd.fan {
fan_proxy.set_profile_fan_curve_enabled(profile, fan, enabled)?;
} else {
- warn!("{REQ_MOD_PROFILE_MSG}");
+ warn!("--enable-fan-curve requires --fan <cpu/gpu/mid>");
}
}
if let Some(mut curve) = cmd.data.clone() {
- let fan = cmd.fan.unwrap_or_default();
+ let fan = cmd.fan.unwrap_or_else(|| {
+ let fan = FanCurvePU::default();
+ warn!("--fan not specified, applying curve to {fan:?}");
+ fan
+ });
curve.set_fan(fan);📝 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.
| if let Some(enabled) = cmd.enable_fan_curve { | |
| if let Some(fan) = cmd.fan { | |
| fan_proxy.set_profile_fan_curve_enabled(profile, fan, enabled)?; | |
| } else { | |
| warn!("{REQ_MOD_PROFILE_MSG}"); | |
| } | |
| } | |
| if let Some(mut curve) = cmd.data.clone() { | |
| let fan = cmd.fan.unwrap_or_default(); | |
| curve.set_fan(fan); | |
| fan_proxy.set_fan_curve(profile, curve)?; | |
| } | |
| if let Some(enabled) = cmd.enable_fan_curve { | |
| if let Some(fan) = cmd.fan { | |
| fan_proxy.set_profile_fan_curve_enabled(profile, fan, enabled)?; | |
| } else { | |
| warn!("--enable-fan-curve requires --fan <cpu/gpu/mid>"); | |
| } | |
| } | |
| if let Some(mut curve) = cmd.data.clone() { | |
| let fan = cmd.fan.unwrap_or_else(|| { | |
| let fan = FanCurvePU::default(); | |
| warn!("--fan not specified, applying curve to {fan:?}"); | |
| fan | |
| }); | |
| curve.set_fan(fan); | |
| fan_proxy.set_fan_curve(profile, curve)?; | |
| } |
🤖 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 `@asusctl/src/fan_curve_cli.rs` around lines 97 - 109, Update the fan-curve CLI
handling around the `cmd.enable_fan_curve` and `cmd.data` branches to require
`cmd.fan` whenever either operation is requested. Replace the misleading
`REQ_MOD_PROFILE_MSG` warning with the existing or appropriate missing-`--fan`
message, ensure the `enable_fan_curve` path can reach this validation, and
remove the `FanCurvePU::default()` fallback so `set_fan_curve` is never called
without an explicitly selected fan.
| pub fn check_service(name: &str) -> bool { | ||
| if name != "asusd" && !check_systemd_unit_enabled(name) { | ||
| warn!( | ||
| "{} is not enabled, enable it with `systemctl enable {}`", | ||
| name, name | ||
| ); | ||
| true | ||
| } else if !check_systemd_unit_active(name) { | ||
| warn!( | ||
| "{} is not running, start it with `systemctl start {}`", | ||
| name, name | ||
| ); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
check_service returns true on failure, which reads backwards at the call site.
main.rs line 56 prints "Is asusd.service running? {}" with this return value, so a stopped service prints true. Rename to something like service_has_problem (or invert to return "healthy") so callers cannot misread it.
🤖 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 `@asusctl/src/platform_cli.rs` around lines 21 - 37, Update check_service so
its return value clearly represents service health, preferably returning true
only when the service is enabled and active; adjust the failure branches and the
main.rs caller to use the renamed or inverted result so “Is asusd.service
running?” reports the correct state.
| // TODO: check cpu are sync | ||
| } else { | ||
| Err(PlatformError::CPU("No CPU's?".to_string())) | ||
| Err(PlatformError::CPU("No CPUs found".to_string())) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the remaining stale no-CPU error message.
get_available_epp still returns "No CPU's?" at Line [135], while these branches now return "No CPUs found". Update that branch so all CPU getters expose the same error contract.
Proposed fix
- Err(PlatformError::CPU("No CPU's?".to_string()))
+ Err(PlatformError::CPU("No CPUs found".to_string()))Also applies to: 99-99, 119-119
🤖 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 `@rog-platform/src/cpu.rs` at line 90, Update the remaining stale no-CPU error
messages in get_available_epp and the other affected CPU getter branches to
return “No CPUs found,” matching the existing PlatformError::CPU contract used
by the nearby branches.
| impl Display for ModeData { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!( | ||
| f, | ||
| "Mode: {}, Zone: {}, Colour1: {:?}, Colour2: {:?}, Speed: {:?}, Direction: {:?}", | ||
| self.mode, self.zone, self.colour1, self.colour2, self.speed, self.direction | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Display drops colour3/colour4.
asusctl scsi --colours accepts up to four colours and handle_scsi prints this Display output as the confirmation of what was applied (asusctl/src/scsi_cli.rs Line 85), so zones 3 and 4 are silently invisible to the user.
🔧 Include all four colours
write!(
f,
- "Mode: {}, Zone: {}, Colour1: {:?}, Colour2: {:?}, Speed: {:?}, Direction: {:?}",
- self.mode, self.zone, self.colour1, self.colour2, self.speed, self.direction
+ "Mode: {}, Zone: {}, Colour1: {:?}, Colour2: {:?}, Colour3: {:?}, Colour4: {:?}, \
+ Speed: {:?}, Direction: {:?}",
+ self.mode,
+ self.zone,
+ self.colour1,
+ self.colour2,
+ self.colour3,
+ self.colour4,
+ self.speed,
+ self.direction
)📝 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.
| impl Display for ModeData { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| write!( | |
| f, | |
| "Mode: {}, Zone: {}, Colour1: {:?}, Colour2: {:?}, Speed: {:?}, Direction: {:?}", | |
| self.mode, self.zone, self.colour1, self.colour2, self.speed, self.direction | |
| ) | |
| } | |
| } | |
| impl Display for ModeData { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| write!( | |
| f, | |
| "Mode: {}, Zone: {}, Colour1: {:?}, Colour2: {:?}, Colour3: {:?}, Colour4: {:?}, \ | |
| Speed: {:?}, Direction: {:?}", | |
| self.mode, | |
| self.zone, | |
| self.colour1, | |
| self.colour2, | |
| self.colour3, | |
| self.colour4, | |
| self.speed, | |
| self.direction | |
| ) | |
| } | |
| } |
🤖 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 `@rog-platform/src/scsi/builtin_modes.rs` around lines 300 - 308, Update the
Display implementation for ModeData in fmt to include colour3 and colour4
alongside the existing colour fields, so confirmation output from handle_scsi
visibly reports all four applied colours.
| pub fn to_tasks(&self) -> Vec<Task> { | ||
| let mut tasks = Vec::new(); | ||
|
|
||
| tasks.append(&mut vec![ | ||
| mode_task(effect.mode as u8), | ||
| rgb_task(0, &effect.colour1.into()), | ||
| rgb_task(1, &effect.colour2.into()), | ||
| rgb_task(2, &effect.colour3.into()), | ||
| rgb_task(3, &effect.colour4.into()), | ||
| ]); | ||
|
|
||
| if !matches!(effect.mode, AuraMode::Static | AuraMode::Off) { | ||
| tasks.push(speed_task(effect.speed as u8)); | ||
| match self.mode { | ||
| AuraMode::Off | AuraMode::Static => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::Breathe => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::Flashing => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RainbowCycle => { | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RainbowWave => { | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RainbowCycleBreathe => { | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::ChaseFade => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RainbowCycleChaseFade => { | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::Chase => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RainbowCycleChase => { | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RainbowCycleWave => { | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RainbowPulseChase => { | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::RandomFlicker => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| AuraMode::DoubleFade => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } | ||
| } | ||
| if matches!( | ||
| effect.mode, | ||
| AuraMode::RainbowWave | ||
| | AuraMode::ChaseFade | ||
| | AuraMode::RainbowCycleChaseFade | ||
| | AuraMode::Chase | ||
| | AuraMode::RainbowCycleChase | ||
| | AuraMode::RainbowCycleWave | ||
| | AuraMode::RainbowPulseChase | ||
| ) { | ||
| tasks.push(dir_task(effect.direction as u8)); | ||
| } | ||
|
|
||
| tasks.append(&mut vec![ | ||
| apply_task(), | ||
| save_task(), | ||
| ]); | ||
| tasks.push(apply_task()); | ||
| tasks.push(save_task()); | ||
| tasks | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
to_tasks has 14 near-identical arms; collapse into per-mode capability flags.
Every arm is a subset of {rgb, dir, speed, mode} and several are byte-for-byte duplicates (Breathe/Flashing, RainbowCycle/RainbowCycleBreathe, RainbowCycleChase/RainbowCycleWave/RainbowPulseChase/RainbowCycleChaseFade). Adding a mode currently requires touching this match plus the clamp in protocol.rs.
♻️ Table-driven construction
pub fn to_tasks(&self) -> Vec<Task> {
- let mut tasks = Vec::new();
- match self.mode {
- AuraMode::Off | AuraMode::Static => {
- tasks.push(rgb_task(
- self.zone,
- &[
- self.colour1.r, self.colour1.g, self.colour1.b,
- ],
- ));
- tasks.push(mode_task(self.mode as u8));
- }
- // ... 13 further arms
- }
+ // (uses_rgb, uses_dir, uses_speed)
+ let (rgb, dir, speed) = match self.mode {
+ AuraMode::Off | AuraMode::Static => (true, false, false),
+ AuraMode::Breathe | AuraMode::Flashing | AuraMode::RandomFlicker => (true, false, true),
+ AuraMode::RainbowCycle | AuraMode::RainbowCycleBreathe => (false, false, true),
+ AuraMode::RainbowWave
+ | AuraMode::RainbowCycleChaseFade
+ | AuraMode::RainbowCycleChase
+ | AuraMode::RainbowCycleWave
+ | AuraMode::RainbowPulseChase => (false, true, true),
+ AuraMode::ChaseFade | AuraMode::Chase | AuraMode::DoubleFade => (true, true, true),
+ };
+
+ let mut tasks = Vec::new();
+ if rgb {
+ tasks.push(rgb_task(self.zone, &[
+ self.colour1.r, self.colour1.g, self.colour1.b,
+ ]));
+ }
+ if dir {
+ tasks.push(dir_task(self.direction as u8));
+ }
+ if speed {
+ tasks.push(speed_task(self.speed as u8));
+ }
+ tasks.push(mode_task(self.mode as u8));
tasks.push(apply_task());
tasks.push(save_task());
tasks
}Note this preserves ordering except for RandomFlicker, which already differs from its neighbours only by omitting dir_task.
📝 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.
| pub fn to_tasks(&self) -> Vec<Task> { | |
| let mut tasks = Vec::new(); | |
| tasks.append(&mut vec![ | |
| mode_task(effect.mode as u8), | |
| rgb_task(0, &effect.colour1.into()), | |
| rgb_task(1, &effect.colour2.into()), | |
| rgb_task(2, &effect.colour3.into()), | |
| rgb_task(3, &effect.colour4.into()), | |
| ]); | |
| if !matches!(effect.mode, AuraMode::Static | AuraMode::Off) { | |
| tasks.push(speed_task(effect.speed as u8)); | |
| match self.mode { | |
| AuraMode::Off | AuraMode::Static => { | |
| tasks.push(rgb_task( | |
| self.zone, | |
| &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ], | |
| )); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::Breathe => { | |
| tasks.push(rgb_task( | |
| self.zone, | |
| &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ], | |
| )); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::Flashing => { | |
| tasks.push(rgb_task( | |
| self.zone, | |
| &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ], | |
| )); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RainbowCycle => { | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RainbowWave => { | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RainbowCycleBreathe => { | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::ChaseFade => { | |
| tasks.push(rgb_task( | |
| self.zone, | |
| &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ], | |
| )); | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RainbowCycleChaseFade => { | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::Chase => { | |
| tasks.push(rgb_task( | |
| self.zone, | |
| &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ], | |
| )); | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RainbowCycleChase => { | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RainbowCycleWave => { | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RainbowPulseChase => { | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::RandomFlicker => { | |
| tasks.push(rgb_task( | |
| self.zone, | |
| &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ], | |
| )); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| AuraMode::DoubleFade => { | |
| tasks.push(rgb_task( | |
| self.zone, | |
| &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ], | |
| )); | |
| tasks.push(dir_task(self.direction as u8)); | |
| tasks.push(speed_task(self.speed as u8)); | |
| tasks.push(mode_task(self.mode as u8)); | |
| } | |
| } | |
| if matches!( | |
| effect.mode, | |
| AuraMode::RainbowWave | |
| | AuraMode::ChaseFade | |
| | AuraMode::RainbowCycleChaseFade | |
| | AuraMode::Chase | |
| | AuraMode::RainbowCycleChase | |
| | AuraMode::RainbowCycleWave | |
| | AuraMode::RainbowPulseChase | |
| ) { | |
| tasks.push(dir_task(effect.direction as u8)); | |
| } | |
| tasks.append(&mut vec![ | |
| apply_task(), | |
| save_task(), | |
| ]); | |
| tasks.push(apply_task()); | |
| tasks.push(save_task()); | |
| tasks | |
| } | |
| pub fn to_tasks(&self) -> Vec<Task> { | |
| // (uses_rgb, uses_dir, uses_speed) | |
| let (rgb, dir, speed) = match self.mode { | |
| AuraMode::Off | AuraMode::Static => (true, false, false), | |
| AuraMode::Breathe | AuraMode::Flashing | AuraMode::RandomFlicker => (true, false, true), | |
| AuraMode::RainbowCycle | AuraMode::RainbowCycleBreathe => (false, false, true), | |
| AuraMode::RainbowWave | |
| | AuraMode::RainbowCycleChaseFade | |
| | AuraMode::RainbowCycleChase | |
| | AuraMode::RainbowCycleWave | |
| | AuraMode::RainbowPulseChase => (false, true, true), | |
| AuraMode::ChaseFade | AuraMode::Chase | AuraMode::DoubleFade => (true, true, true), | |
| }; | |
| let mut tasks = Vec::new(); | |
| if rgb { | |
| tasks.push(rgb_task(self.zone, &[ | |
| self.colour1.r, self.colour1.g, self.colour1.b, | |
| ])); | |
| } | |
| if dir { | |
| tasks.push(dir_task(self.direction as u8)); | |
| } | |
| if speed { | |
| tasks.push(speed_task(self.speed as u8)); | |
| } | |
| tasks.push(mode_task(self.mode as u8)); | |
| tasks.push(apply_task()); | |
| tasks.push(save_task()); | |
| tasks | |
| } |
🤖 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 `@rog-platform/src/scsi/builtin_modes.rs` around lines 351 - 463, Replace the
repetitive match in AuraMode::to_tasks with per-mode capability flags describing
whether each mode emits rgb, dir, speed, and mode tasks. Build tasks from those
flags in the existing ordering, then retain apply_task and save_task for every
mode; preserve the documented RandomFlicker omission of dir_task and make adding
modes require only updating the capability mapping.
| AuraMode::DoubleFade => { | ||
| tasks.push(rgb_task( | ||
| self.zone, | ||
| &[ | ||
| self.colour1.r, self.colour1.g, self.colour1.b, | ||
| ], | ||
| )); | ||
| tasks.push(dir_task(self.direction as u8)); | ||
| tasks.push(speed_task(self.speed as u8)); | ||
| tasks.push(mode_task(self.mode as u8)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the clamp and the enum discriminants
fd -t f 'protocol.rs' -p rog-platform | xargs -r rg -n 'min\(|ENE_REG_MODE'
fd -t f 'builtin_modes.rs' -p rog-platform | xargs -r rg -n -A20 'enum AuraMode'Repository: OpenGamingCollective/asusctl
Length of output: 982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant protocol and enum context with line numbers.
sed -n '1,120p' rog-platform/src/scsi/protocol.rs
printf '\n----\n'
sed -n '150,210p' rog-platform/src/scsi/builtin_modes.rs
printf '\n----\n'
sed -n '430,480p' rog-platform/src/scsi/builtin_modes.rsRepository: OpenGamingCollective/asusctl
Length of output: 5532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any other mode clamping/mapping that might justify the limit.
rg -n 'min\(13\)|mode_task\(|ENE_REG_MODE|DoubleFade|RandomFlicker' rog-platform/srcRepository: OpenGamingCollective/asusctl
Length of output: 2944
DoubleFade is still serialized as RandomFlicker. mode_task() clamps at 13, but AuraMode::DoubleFade is 14, so to_tasks() writes the wrong mode value. Raise the clamp to 14 or derive it from the enum’s max variant.
🤖 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 `@rog-platform/src/scsi/builtin_modes.rs` around lines 448 - 458, Update
mode_task() used by AuraMode::DoubleFade serialization so its upper clamp allows
value 14, or derive the limit from the enum’s maximum variant. Ensure to_tasks()
preserves AuraMode::DoubleFade as mode 14 rather than serializing it as
RandomFlicker.
| pkt2[6] = 0x01; | ||
| pkt2[7] = mode as u8; | ||
| pkt2[8] = 0x02; | ||
| pkt2[9] = 0x19; // difference, GA605 = 0x10 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the GA605-specific packet byte.
The inline protocol note says GA605 requires 0x10, but this always writes 0x19; both GA605_2024 and GA605_2025 therefore receive an invalid mode packet.
Proposed fix
- pkt2[9] = 0x19; // difference, GA605 = 0x10
+ pkt2[9] = match slash_type {
+ SlashType::GA605_2024 | SlashType::GA605_2025 => 0x10,
+ _ => 0x19,
+ };📝 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.
| pkt2[9] = 0x19; // difference, GA605 = 0x10 | |
| pkt2[9] = match slash_type { | |
| SlashType::GA605_2024 | SlashType::GA605_2025 => 0x10, | |
| _ => 0x19, | |
| }; |
🤖 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 `@rog-platform/src/slash/usb.rs` at line 150, Update the packet construction
around pkt2[9] to write the GA605-specific byte value 0x10 for both GA605_2024
and GA605_2025, while preserving the existing 0x19 value for other device
variants.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rog-dbus/src/lib.rs (1)
57-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate
find_iface_async/find_iface_blockingpath-filtering logic.Both functions repeat the identical loop that filters
ManagedObjectsby interface name, warns on multiple matches, and sorts the paths. Only the connection setup and.awaitdiffer. Extracting a small sync helper (e.g.fn matching_paths(interfaces: &zbus::fdo::ManagedObjects, iface_name: &str) -> Vec<OwnedObjectPath>) used by both would remove the duplication the PR is otherwise aiming to reduce, and prevents the two copies drifting further (the blocking version already dropped the redundant.clone()that the async version still has).♻️ Suggested helper extraction
+fn matching_paths( + interfaces: &std::collections::HashMap< + zbus::zvariant::OwnedObjectPath, + std::collections::HashMap<zbus::names::OwnedInterfaceName, std::collections::HashMap<String, zbus::zvariant::OwnedValue>>, + >, + iface_name: &str, +) -> Vec<zbus::zvariant::OwnedObjectPath> { + let mut paths = Vec::new(); + for v in interfaces.iter() { + for k in v.1.keys() { + if k.as_str() == iface_name { + paths.push(v.0.clone()); + } + } + } + if paths.len() > 1 { + log::warn!("Multiple asusd interface devices found"); + } + paths.sort(); + paths +}Both
find_iface_asyncandfind_iface_blockingcan then call this instead of repeating the loop.Also applies to: 95-129
🤖 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 `@rog-dbus/src/lib.rs` around lines 57 - 94, Extract the shared interface-path filtering, multiple-match warning, and sorting logic from find_iface_async and find_iface_blocking into a synchronous matching_paths helper returning the appropriate OwnedObjectPath collection. Replace both duplicated loops with calls to this helper, preserving each function’s existing connection setup and async/blocking proxy construction; also remove the async-only redundant path clone when consuming the helper result.
🤖 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 `@asusctl/src/anime_cli.rs`:
- Around line 193-198: Update the anime type initialization around
get_anime_type and cmd.override_type so that when the result remains
AnimeType::Unsupported, emit a warning directing the user to --override-type
before proceeding. Hoist this anime_type resolution and warning outside the for
proxy loop because it is loop-invariant, while preserving the override behavior.
- Around line 154-204: The handle_anime function accepts off_with_his_head in
its argument guard but never applies it; wire cmd.off_with_his_head to the
appropriate AnimeProxyBlocking operation, or remove the flag and its guard
condition if no corresponding proxy API exists. Ensure the option cannot be
accepted while having no effect.
In `@asusctl/src/fan_curve_cli.rs`:
- Around line 46-96: Move FanCurvesProxyBlocking::new construction in
handle_fan_curve below both argument-validation guards, so missing or
incompatible arguments return their existing usage warnings without any D-Bus
calls. Construct the fan proxy only after validation, and keep
PlatformProxyBlocking::new after the guards as well.
- Around line 97-109: Update the fan-curve CLI handling around the
`cmd.enable_fan_curve` and `cmd.data` branches to require `cmd.fan` whenever
either operation is requested. Replace the misleading `REQ_MOD_PROFILE_MSG`
warning with the existing or appropriate missing-`--fan` message, ensure the
`enable_fan_curve` path can reach this validation, and remove the
`FanCurvePU::default()` fallback so `set_fan_curve` is never called without an
explicitly selected fan.
In `@asusctl/src/platform_cli.rs`:
- Line 514: Remove the stale #[allow] attribute associated with
handle_armoury_command, since neither clippy::manual_is_multiple_of nor
clippy::nonminimal_bool applies there; only retain or relocate those allowances
if the actual triggering function requires them.
- Around line 21-37: Update check_service so its return value clearly represents
service health, preferably returning true only when the service is enabled and
active; adjust the failure branches and the main.rs caller to use the renamed or
inverted result so “Is asusd.service running?” reports the correct state.
- Around line 275-301: Validate CLI argument combinations before any D-Bus proxy
creation or device iteration. In asusctl/src/platform_cli.rs:275-301, move the
power.awake/sleep/boot/keyboard/lightbar emptiness guard before
find_iface_blocking in handle_led_power1. In asusctl/src/fan_curve_cli.rs:46-96,
move the existing argument guards before FanCurvesProxyBlocking::new; preserve
the current usage responses while ensuring invalid input cannot produce device
or transport errors.
- Around line 137-172: Update handle_backlight to call
find_iface_blocking::<BacklightProxyBlocking> only once before the
read-versus-update branch. Reuse the resulting backlights collection for
displaying current settings when no setter options are supplied and for applying
setters otherwise, preserving both existing behaviors.
- Line 72: Remove the find_iface_blocking re-export from platform_cli and import
rog_dbus::find_iface_blocking directly in callers such as anime_cli. In the
aura-interface lookup handling around lines 90-105, eliminate the duplicate “No
aura interface found” warning and log the underlying error in the Err arm while
retaining the existing behavior for successful and unavailable-interface cases.
- Around line 517-526: Update the ArmourySubCommand::List arm to propagate
errors from find_iface_blocking::<AsusArmouryProxyBlocking> instead of silently
ignoring the Err result. Preserve the existing attribute iteration and
print_firmware_attr behavior for successful lookups, matching the error
propagation used by the Get and Set arms.
- Around line 341-386: Refactor the power-command handling to destructure
power.command once, eliminating the redundant outer and inner Option checks.
Update the set closure used by the command match to explicitly handle a
requested PowerZones value that is absent from states.states, either by adding a
corresponding AuraPowerState or warning that the zone is unsupported; do not
silently write unchanged state. Preserve the existing per-zone field updates for
entries that are present.
In `@asusctl/src/scsi_cli.rs`:
- Around line 53-69: Update the colour assignment loop in the command handling
flow to use a match on the enumerated index for the four supported colour
fields, and explicitly warn or report when additional colours exceed the fourth
value instead of silently ignoring them. Preserve the existing assignments to
mode.colour1 through mode.colour4 and do_update behavior.
- Around line 33-36: Update the initial guard in handle_scsi to include
cmd.speed and cmd.direction when determining whether any actionable argument was
provided. Preserve the warning and return immediately when all ScsiCommand
options are empty, while allowing valid speed or direction invocations to
proceed without warning.
In `@asusd/src/aura_anime/trait_impls.rs`:
- Around line 449-455: The repeated JoinSet supervisor loops should be
centralized in a shared helper on CtrlTask. In asusd/src/lib.rs, add
spawn_task_supervisor accepting the log label and JoinSet, spawning the existing
drain-and-warn behavior; replace the inline loops in
asusd/src/aura_anime/trait_impls.rs:449-455,
asusd/src/aura_laptop/trait_impls.rs:295-301, and
asusd/src/ctrl_platform.rs:1118-1124 with calls using “AniMeZbus”, “AuraZbus”,
and “CtrlPlatform” respectively.
In `@asusd/src/ctrl_backlight.rs`:
- Around line 153-155: Consume the optional JoinHandle returned by
CtrlBacklight::start_watch_primary at its daemon.rs call site instead of
discarding it. Supervise the handle asynchronously and report JoinError when the
watcher task terminates, preserving the existing initialization flow; apply the
same handling to the related start-watch paths noted in the comment, or remove
the returned handle if supervision is intentionally not supported.
In `@asusd/src/ctrl_platform.rs`:
- Around line 943-950: Update the set_charge_control_end_threshold call in the
!sleeping branch to log failures before discarding the Result, matching the
warn!/error! behavior used by sibling closures in the same function. Preserve
the existing threshold-setting flow and only replace the silent .ok() handling
with contextual error logging.
- Around line 1066-1073: Preserve the watcher’s JoinError for JoinSet
supervision by spawning the JoinHandle returned from
watch_charge_control_end_threshold directly, rather than wrapping h in a task
that ignores h.await. Update the tasks.spawn call in this charge-threshold
watcher block so join_next can report watcher panics or aborts to the existing
warning path.
In `@asusd/src/daemon.rs`:
- Around line 35-45: Update the IS_SERVICE environment parsing so an unset
variable produces false rather than true. Preserve the existing "1" value as
true, allowing the !is_service warning and early return path to run when the
daemon is launched outside the systemd service.
In `@asusd/src/lib.rs`:
- Line 230: Update create_sys_event_tasks so the callback future types Fut1–Fut4
include 'static alongside their existing Send bounds, ensuring every future
passed to JoinSet::spawn satisfies its required lifetime constraint.
In `@rog-platform/src/capabilities.rs`:
- Around line 40-41: Update the has_fan_curves capability check to remove the
fan_boost_mode probe and rely only on the actual writable fan-curve attribute,
such as pwm1_auto_point1_pwm. Preserve the existing capability behavior for
systems exposing the curve points.
In `@rog-platform/src/cpu.rs`:
- Line 90: Update the remaining stale no-CPU error messages in get_available_epp
and the other affected CPU getter branches to return “No CPUs found,” matching
the existing PlatformError::CPU contract used by the nearby branches.
In `@rog-platform/src/scsi/builtin_modes.rs`:
- Around line 300-308: Update the Display implementation for ModeData in fmt to
include colour3 and colour4 alongside the existing colour fields, so
confirmation output from handle_scsi visibly reports all four applied colours.
- Around line 351-463: Replace the repetitive match in AuraMode::to_tasks with
per-mode capability flags describing whether each mode emits rgb, dir, speed,
and mode tasks. Build tasks from those flags in the existing ordering, then
retain apply_task and save_task for every mode; preserve the documented
RandomFlicker omission of dir_task and make adding modes require only updating
the capability mapping.
- Around line 448-458: Update mode_task() used by AuraMode::DoubleFade
serialization so its upper clamp allows value 14, or derive the limit from the
enum’s maximum variant. Ensure to_tasks() preserves AuraMode::DoubleFade as mode
14 rather than serializing it as RandomFlicker.
In `@rog-platform/src/scsi/sg.rs`:
- Around line 114-121: Update Task::new() to initialize the inner SgIoHdr
timeout field to a sensible non-zero default instead of relying on
SgIoHdr::default(). Preserve the existing empty command, data, and sense
buffers, and ensure task builders inherit this timeout unless explicitly
overridden.
- Around line 93-111: Replace the derived Clone implementation for Task with a
manual clone that duplicates the owned cmd, data, and sense buffers, then
rebuilds inner’s raw pointers to reference the clone’s buffers rather than
copying the original pointers. Remove the unsafe Sync implementation and update
the safety comment to describe only the remaining Send guarantee and pointer
invariant.
- Around line 244-255: Update perform’s SG_IO request declaration to use
libc::Ioctl on all targets instead of the target_env-based u64/i32 split, then
pass that libc::Ioctl value directly to libc::ioctl. Preserve the existing
synchronous ioctl call and safety handling.
In `@rog-platform/src/slash/usb.rs`:
- Line 150: Update the packet construction around pkt2[9] to write the
GA605-specific byte value 0x10 for both GA605_2024 and GA605_2025, while
preserving the existing 0x19 value for other device variants.
---
Outside diff comments:
In `@rog-dbus/src/lib.rs`:
- Around line 57-94: Extract the shared interface-path filtering, multiple-match
warning, and sorting logic from find_iface_async and find_iface_blocking into a
synchronous matching_paths helper returning the appropriate OwnedObjectPath
collection. Replace both duplicated loops with calls to this helper, preserving
each function’s existing connection setup and async/blocking proxy construction;
also remove the async-only redundant path clone when consuming the helper
result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d77d82a-4c7a-4b38-83e1-6bc44ed3751b
📒 Files selected for processing (57)
Cargo.tomlasusctl/Cargo.tomlasusctl/src/anime_cli.rsasusctl/src/fan_curve_cli.rsasusctl/src/main.rsasusctl/src/platform_cli.rsasusctl/src/scsi_cli.rsasusctl/src/slash_cli.rsasusd/Cargo.tomlasusd/src/aura_anime/trait_impls.rsasusd/src/aura_laptop/trait_impls.rsasusd/src/aura_manager.rsasusd/src/aura_scsi/config.rsasusd/src/aura_scsi/mod.rsasusd/src/aura_scsi/trait_impls.rsasusd/src/aura_slash/config.rsasusd/src/aura_slash/mod.rsasusd/src/aura_slash/trait_impls.rsasusd/src/aura_types.rsasusd/src/ctrl_backlight.rsasusd/src/ctrl_platform.rsasusd/src/daemon.rsasusd/src/error.rsasusd/src/lib.rsdmi-id/Cargo.tomlrog-anime/Cargo.tomlrog-anime/src/data.rsrog-anime/src/usb.rsrog-aura/Cargo.tomlrog-aura/src/aura_detection.rsrog-control-center/Cargo.tomlrog-control-center/src/main.rsrog-control-center/src/mocking.rsrog-control-center/src/ui/setup_slash.rsrog-control-center/src/zbus_proxies.rsrog-dbus/Cargo.tomlrog-dbus/src/lib.rsrog-dbus/src/scsi_aura.rsrog-dbus/src/zbus_slash.rsrog-platform/Cargo.tomlrog-platform/src/capabilities.rsrog-platform/src/cpu.rsrog-platform/src/dmi.rsrog-platform/src/error.rsrog-platform/src/lib.rsrog-platform/src/scsi/builtin_modes.rsrog-platform/src/scsi/mod.rsrog-platform/src/scsi/protocol.rsrog-platform/src/scsi/sg.rsrog-platform/src/slash/data.rsrog-platform/src/slash/mod.rsrog-platform/src/slash/usb.rsrog-profiles/src/fan_curve_set.rsrog-scsi/Cargo.tomlrog-scsi/src/error.rsrog-slash/Cargo.tomlrog-slash/src/error.rs
💤 Files with no reviewable changes (8)
- asusctl/Cargo.toml
- dmi-id/Cargo.toml
- rog-slash/src/error.rs
- rog-scsi/Cargo.toml
- rog-slash/Cargo.toml
- Cargo.toml
- rog-scsi/src/error.rs
- rog-control-center/Cargo.toml
📜 Review details
🔇 Additional comments (41)
rog-profiles/src/fan_curve_set.rs (1)
29-36: LGTM!rog-platform/src/slash/data.rs (1)
6-10: LGTM!rog-platform/src/slash/mod.rs (1)
6-7: LGTM!rog-platform/src/slash/usb.rs (1)
11-14: LGTM!Also applies to: 37-37
asusd/src/aura_slash/config.rs (1)
2-2: LGTM!asusd/src/aura_slash/mod.rs (1)
5-7: LGTM!asusd/src/aura_slash/trait_impls.rs (1)
3-8: LGTM!asusd/src/aura_types.rs (1)
11-12: LGTM!Also applies to: 84-84
rog-dbus/src/zbus_slash.rs (1)
1-1: LGTM!rog-control-center/src/ui/setup_slash.rs (1)
7-7: LGTM!asusctl/src/slash_cli.rs (1)
2-4: LGTM!Also applies to: 82-82, 142-149
rog-aura/Cargo.toml (1)
21-21: LGTM!rog-platform/Cargo.toml (1)
11-12: LGTM!Also applies to: 24-25
rog-platform/src/capabilities.rs (1)
1-39: LGTM!Also applies to: 42-58
rog-platform/src/dmi.rs (1)
94-94: LGTM!rog-platform/src/error.rs (1)
60-89: LGTM!Also applies to: 104-115
rog-platform/src/lib.rs (1)
6-22: LGTM!rog-anime/src/data.rs (1)
6-7: LGTM!Also applies to: 320-320
asusctl/src/anime_cli.rs (2)
296-336: LGTM!Also applies to: 338-376
200-204: 🎯 Functional CorrectnessKeep
0xFFfor--clear. This is the fill value used to blank the AniMe buffer; switching to0u8would invert the behavior.> Likely an incorrect or invalid review comment.asusctl/src/main.rs (1)
1-19: LGTM!Also applies to: 95-102, 117-135
rog-platform/src/scsi/builtin_modes.rs (1)
8-10: LGTM!Also applies to: 209-230, 232-256, 281-285
rog-platform/src/scsi/mod.rs (1)
2-5: LGTM!rog-platform/src/scsi/protocol.rs (1)
1-1: LGTM!rog-platform/src/scsi/sg.rs (2)
9-16: LGTM!Also applies to: 18-72, 74-91
124-148: LGTM!Also applies to: 264-268
asusd/src/aura_scsi/config.rs (1)
5-5: LGTM!asusd/src/aura_scsi/trait_impls.rs (1)
6-6: LGTM! SourcingAuraMode/AuraEffectfromrog_platform::scsihere matchesrog-dbus/src/scsi_aura.rs, so the D-Bus signatures on both sides now derive from one definition.asusctl/src/scsi_cli.rs (3)
2-3: LGTM!
71-96: LGTM!
47-51: 🗄️ Data Integrity & IntegrationProperty signatures already match
AuraModeuseszvariant(signature = "u"), so the getter and setter share the same D-Bus type; drop this comment.> Likely an incorrect or invalid review comment.asusd/src/aura_scsi/mod.rs (1)
4-4: 🩺 Stability & Availability
DeviceandTaskare re-exported atrog_platform::scsiroot, so this import is valid.> Likely an incorrect or invalid review comment.rog-dbus/src/scsi_aura.rs (1)
23-23: 🗄️ Data Integrity & Integration
rog-dbusalready enablesrog-platformwith thedbusfeature.> Likely an incorrect or invalid review comment.asusd/src/aura_anime/trait_impls.rs (1)
319-448: LGTM!asusd/src/aura_laptop/trait_impls.rs (1)
242-293: LGTM!asusd/src/ctrl_platform.rs (1)
1075-1081: LGTM!asusd/src/daemon.rs (1)
22-24: LGTM!Also applies to: 59-59, 160-165
rog-dbus/Cargo.toml (1)
15-18: LGTM!rog-control-center/src/mocking.rs (1)
5-6: LGTM!Also applies to: 98-125, 137-137, 152-154, 165-171, 198-202
rog-control-center/src/main.rs (1)
8-23: LGTM!Also applies to: 86-91
rog-control-center/src/zbus_proxies.rs (1)
71-91: 📐 Maintainability & Code QualityNo change needed
🛑 Comments failed to post (3)
rog-platform/src/scsi/sg.rs (3)
93-111: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
CloneonTaskyields danglingSgIoHdrpointers, andSyncisn't needed.
#[derive(Clone)]copiesinnerverbatim, so the clone'scmdp/dxferp/sbppoint into the original's vectors. Todayperformcallssync_pointers()first so nothing dereferences them, but the invariant is invisible to future callers and the safety comment claims pointers "are updated prior to any ioctl execution" without enforcing it. Alsounsafe impl Syncgrants nothing useful here (&Taskcan't mutate or exposeinner) and only widens the unsafe surface.🔒 Hand-write `Clone` and drop `Sync`
-#[derive(Clone, Debug)] +#[derive(Debug)] pub struct Task { inner: SgIoHdr, cmd: Vec<u8>, data: Vec<u8>, sense: Vec<u8>, } + +impl Clone for Task { + fn clone(&self) -> Self { + // Re-sync so the clone's raw pointers reference its own buffers. + let mut task = Task { + inner: self.inner, + cmd: self.cmd.clone(), + data: self.data.clone(), + sense: self.sense.clone(), + }; + task.sync_pointers(); + task + } +} impl Default for Task { fn default() -> Self { Self::new() } } -// SAFETY: Task manages its internal buffers and SgIoHdr raw pointers. The raw pointers -// are updated prior to any ioctl execution to point directly to owned heap vectors, making -// Send and Sync safe across thread boundaries. +// SAFETY: Task exclusively owns the buffers its SgIoHdr pointers reference, and those +// pointers are re-synced (`sync_pointers`) on mutation and before every ioctl, so moving +// a Task between threads cannot observe a pointer into another thread's memory. unsafe impl Send for Task {} -unsafe impl Sync for Task {}📝 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.#[derive(Debug)] pub struct Task { inner: SgIoHdr, cmd: Vec<u8>, data: Vec<u8>, sense: Vec<u8>, } impl Clone for Task { fn clone(&self) -> Self { // Re-sync so the clone's raw pointers reference its own buffers. let mut task = Task { inner: self.inner, cmd: self.cmd.clone(), data: self.data.clone(), sense: self.sense.clone(), }; task.sync_pointers(); task } } impl Default for Task { fn default() -> Self { Self::new() } } // SAFETY: Task exclusively owns the buffers its SgIoHdr pointers reference, and those // pointers are re-synced (`sync_pointers`) on mutation and before every ioctl, so moving // a Task between threads cannot observe a pointer into another thread's memory. unsafe impl Send for Task {}🤖 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 `@rog-platform/src/scsi/sg.rs` around lines 93 - 111, Replace the derived Clone implementation for Task with a manual clone that duplicates the owned cmd, data, and sense buffers, then rebuilds inner’s raw pointers to reference the clone’s buffers rather than copying the original pointers. Remove the unsafe Sync implementation and update the safety comment to describe only the remaining Send guarantee and pointer invariant.
114-121: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Linux sg_io_hdr timeout field 0 value SG_IO ioctl behaviour💡 Result:
In the Linux sg_io_hdr structure, the timeout field defines the time in milliseconds that the kernel will wait for a SCSI command to complete [1][2][3]. The behavior of a 0 value in the timeout field depends on the context of the ioctl call: 1. SG Driver Behavior: When using the SCSI generic (sg) driver, a timeout value of 0 is interpreted literally as 0 milliseconds [1][4]. 2. Block Layer Behavior: When the SG_IO ioctl is used on block devices (e.g., /dev/sda), the kernel's block layer typically applies a default timeout if the user-provided value is 0 [1][4]. Historically and in many kernel versions, this default is 60 seconds [1][4]. Because of this inconsistency, it is standard practice for applications to explicitly set a meaningful timeout value rather than relying on a 0-value default [1][5]. Users should also be aware that if a command times out, the kernel may attempt error recovery, which can include resetting the device or the entire SCSI bus, potentially impacting other devices on the same interconnect [1][5]. For detailed documentation, the sg_io_hdr structure is defined in the Linux kernel header include/scsi/sg.h [3][6]. Applications often use the sg3_utils library for standardized SCSI command handling and error processing to avoid common pitfalls associated with manual ioctl management [1].
Citations:
- 1: http://sg.danny.cz/sg/sg_io.html
- 2: https://sg.danny.cz/sg/p/scsi-generic_v3.txt
- 3: https://github.com/torvalds/linux/blob/master/include/scsi/sg.h
- 4: https://sg.danny.cz/sg/sg_io.html
- 5: https://tldp.org/HOWTO/pdf/SCSI-Generic-HOWTO.pdf
- 6: https://gbmc.googlesource.com/linux/+/33f36147be36c5fa0919d32d20d501ca97981d9a/include/scsi/sg.h
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== File list ==\n' git ls-files 'rog-platform/src/scsi/*' 'asusd/src/aura_scsi/*' || true printf '\n== sg.rs outline ==\n' ast-grep outline rog-platform/src/scsi/sg.rs --view expanded || true printf '\n== protocol.rs outline ==\n' ast-grep outline rog-platform/src/scsi/protocol.rs --view expanded || true printf '\n== Search for timeout setters/usages ==\n' rg -n 'set_timeout|timeout|Task::new|SgIoHdr::default' rog-platform/src/scsi asusd/src/aura_scsi || true printf '\n== Relevant slices ==\n' sed -n '1,220p' rog-platform/src/scsi/sg.rs printf '\n--- protocol.rs ---\n' sed -n '1,260p' rog-platform/src/scsi/protocol.rs printf '\n--- aura_scsi mod.rs ---\n' sed -n '1,220p' asusd/src/aura_scsi/mod.rsRepository: OpenGamingCollective/asusctl
Length of output: 13546
Set a non-zero SG_IO timeout by default
Task::new()leavessg_io_hdr.timeoutat0, and these task builders never override it. The sg driver treats that as a 0 ms deadline, so the command can fail immediately andwrite_effect()drops the error with.ok(). InitializeTaskwith a sensible non-zero timeout.🤖 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 `@rog-platform/src/scsi/sg.rs` around lines 114 - 121, Update Task::new() to initialize the inner SgIoHdr timeout field to a sensible non-zero default instead of relying on SgIoHdr::default(). Preserve the existing empty command, data, and sense buffers, and ensure task builders inherit this timeout unless explicitly overridden.
244-255: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
libc crate ioctl signature request c_ulong glibc musl c_int💡 Result:
The libc crate's ioctl function signature is platform-dependent, specifically regarding the type of the request argument [1][2][3]. While the function is generally defined as: pub unsafe extern "C" fn ioctl(fd: c_int, request: libc::Ioctl,...) -> c_int The type libc::Ioctl is not a fixed primitive but is defined per-platform [4][2]. On many glibc-based platforms (e.g., x86_64), it is typically defined as c_ulong [5][6][2]. Conversely, on musl-based platforms and some others (e.g., Android aarch64), it is defined as c_int [7][2][3]. This mismatch often causes compilation errors when constants defined as one type (e.g., a u32 or c_ulong) are passed to an ioctl expecting another (e.g., c_int) [1][2][8]. To ensure cross-platform compatibility, the idiomatic approach is to cast the request constant to libc::Ioctl [2][8]: const MY_IOCTL: libc::Ioctl = CONSTANT_VALUE as libc::Ioctl; Alternatively, many Rust developers avoid using the libc::ioctl function directly and instead use the nix crate, which provides high-level macros (such as ioctl_write_int! and ioctl_write_ptr!) to handle the underlying ioctl number generation and type safety, abstracting away these platform-specific differences [9].
Citations:
- 1: rust-lang/libc#1094
- 2: sched-ext/scx#1860
- 3: rust-lang/libc#1036
- 4: https://docs.rs/libc/latest/src/libc/unix/linux_like/linux/gnu/mod.rs.html
- 5: https://docs.rs/libc/latest/libc/fn.ioctl.html
- 6: https://rust-lang.github.io/hashbrown/libc/fn.ioctl.html
- 7: https://docs.rs/libc/latest/x86_64-unknown-linux-musl/libc/fn.ioctl.html
- 8: socketcan-rs/socketcan-rs#101
- 9: https://docs.rs/nix/latest/nix/sys/ioctl/index.html
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant file and search for SG_IO / ioctl typing. git ls-files rog-platform/src/scsi/sg.rs wc -l rog-platform/src/scsi/sg.rs cat -n rog-platform/src/scsi/sg.rs | sed -n '1,320p' printf '\n--- search for SG_IO and ioctl-related typing ---\n' rg -n "SG_IO|ioctl|c_ulong|c_int|Ioctl" rog-platform/src -SRepository: OpenGamingCollective/asusctl
Length of output: 10051
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read libc's ioctl type definitions from the installed crate sources if available. python3 - <<'PY' import glob, os, sys candidates = glob.glob(os.path.expanduser('~/.cargo/registry/src/*/libc-*/src/lib.rs')) print("\n".join(candidates[:20])) PY # If source is available, inspect ioctl signature and type aliases. LIBC_SRC=$(python3 - <<'PY' import glob, os cands = glob.glob(os.path.expanduser('~/.cargo/registry/src/*/libc-*/src/lib.rs')) print(cands[0] if cands else "") PY ) if [ -n "$LIBC_SRC" ]; then echo "--- libc source: $LIBC_SRC ---" rg -n "pub type Ioctl|pub unsafe.*ioctl|type Ioctl" "$LIBC_SRC" -n -S fiRepository: OpenGamingCollective/asusctl
Length of output: 371
🏁 Script executed:
#!/bin/bash set -euo pipefail LIBC_SRC=/home/jailuser/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/libc-0.2.189/src/lib.rs # Inspect the ioctl signature and related aliases in the libc crate source. rg -n "pub type Ioctl|pub unsafe extern \"C\" fn ioctl|type Ioctl|ioctl\\(" "$LIBC_SRC" -n -S printf '\n--- nearby context ---\n' sed -n '1,260p' "$LIBC_SRC" | cat -n | sed -n '1,260p'Repository: OpenGamingCollective/asusctl
Length of output: 166
🏁 Script executed:
#!/bin/bash set -euo pipefail ROOT=/home/jailuser/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/libc-0.2.189/src printf '%s\n' "--- find Ioctl declarations ---" rg -n --hidden "pub type Ioctl|type Ioctl|extern \"C\" fn ioctl|fn ioctl\\(" "$ROOT" -S printf '\n%s\n' "--- candidate files containing ioctl ---" rg -n --hidden "ioctl" "$ROOT/unix" "$ROOT/linux" "$ROOT/generic" -S | head -n 200Repository: OpenGamingCollective/asusctl
Length of output: 39724
Use
libc::IoctlforSG_IO
libc::ioctltakeslibc::Ioctlon Linux (c_ulongon glibc,c_inton musl), so the currentu64/i32split breaks 32-bit glibc builds. MakeSG_IOuselibc::Ioctland pass it through directly.🔧 Possible fix
-pub const SG_IO: u64 = 0x2285; +pub const SG_IO: libc::Ioctl = 0x2285;- #[cfg(target_env = "musl")] - let request = SG_IO as i32; - #[cfg(not(target_env = "musl"))] - let request: u64 = SG_IO; + let request = SG_IO;📝 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.pub fn perform(&self, task: &mut Task) -> io::Result<()> { task.sync_pointers(); let request = SG_IO; // SAFETY: The raw file descriptor is open and valid, and task has valid synced // pointers into its own buffers, which stay alive for the duration of the // synchronous ioctl. let ret = unsafe { libc::ioctl(self.0.as_raw_fd(), request, &mut task.inner) };🤖 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 `@rog-platform/src/scsi/sg.rs` around lines 244 - 255, Update perform’s SG_IO request declaration to use libc::Ioctl on all targets instead of the target_env-based u64/i32 split, then pass that libc::Ioctl value directly to libc::ioctl. Preserve the existing synchronous ioctl call and safety handling.
…ght watcher, and charge threshold logging
1 participant
Note
I've opened this PR to discuss about future cleanups and simplification of asusctl. This Pull Request is not meant for direct merge itself.
1. Crate Consolidation & Workspace Simplification (Phase 1)
dmi-id,rog-scsi, androg-slashinto submodules withinrog-platform(dmi,scsi,slash).Cargo.toml.SlashErrorand SCSI domain errors intorog_platform::PlatformError.asusd,asusctl,rog-dbus, androg-control-center.2. Architecture & Utility Cleanup (Phase 2)
DeviceCapabilitiesmatrix inrog-platformfor centralized feature detection.find_iface_blockingandfind_iface_asynchelper functions intorog-dbus.asusctlCLI Refactoring: Modularized CLI handlers into dedicated files (platform_cli.rs,scsi_cli.rs,slash_cli.rs).rog-profiles: Replaced manual parsing loops inCurveDatawith functional iterator combinators.3. Hardware Event Handling, Error Robustness & Tokio Task Management (Phase 3)
create_sys_event_tasks: Removed.expect(...)calls during D-Bus system connection andlogindproxy initialization, replacing them withRogErrorpropagation.tokio::task::JoinSet):task_watch_item!andtask_watch_item_notify!macros to returnResult<Option<tokio::task::JoinHandle<()>>, RogError>.create_sys_event_tasksto returnResult<tokio::task::JoinSet<()>, RogError>.CtrlPlatform,CtrlBacklight,AniMeZbus, andAuraZbusto manage task execution and log task completion/failures.