refactor(workspace): consolidate dmi-id, rog-scsi, and rog-slash crates into rog-platform - #240
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR consolidates DMI, SCSI, and Slash functionality into ChangesPlatform consolidation
CLI modularization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Asusctl
participant CLIHandler
participant RogDbus
participant Asusd
Asusctl->>CLIHandler: dispatch platform, anime, fan-curve, or SCSI command
CLIHandler->>RogDbus: discover D-Bus interfaces
RogDbus->>Asusd: query managed objects
Asusd-->>RogDbus: return matching object paths
RogDbus-->>CLIHandler: return proxies
CLIHandler->>Asusd: read or apply device settings
Asusd-->>CLIHandler: return operation result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
05435e6 to
544d2d8
Compare
5ca4f23 to
cba1b91
Compare
…ync into rog-dbus
cba1b91 to
6df2a90
Compare
coderabbitai
Bot
added
asusctl
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
asusctl/src/slash_cli.rs (1)
70-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReturn after the missing-argument warning.
Line 82 warns and then falls through. Lines 85-87 open a D-Bus connection and create the proxy, and every following block is skipped because all options are unset.
handle_animeat line 166 andhandle_fan_curveat line 61 return in the equivalent branch.🐛 Proposed fix
&& !cmd.disable { warn!("Missing arg; run 'asusctl slash set --help' for usage"); + return Ok(()); }🤖 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/slash_cli.rs` around lines 70 - 83, Update handle_slash_set so the missing-argument branch returns immediately after issuing the existing usage warning, preventing the subsequent D-Bus connection and proxy setup from running. Match the early-return behavior already used by handle_anime and handle_fan_curve.asusctl/src/main.rs (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTake
connby reference instead of suppressing the lint.Line 102 adds
#[allow(clippy::needless_pass_by_value)].do_parsedusesconnonly as&connat lines 116, 117, and 127. Change the parameter to&Connectionand remove the attribute. Update the single call site accordingly.♻️ Proposed change
-#[allow(clippy::needless_pass_by_value)] fn do_parsed( parsed: &CliStart, supported_interfaces: &[String], supported_properties: &[Properties], - conn: Connection, + conn: &Connection, ) -> Result<(), Box<dyn std::error::Error>> { @@ - CliCommand::Profile(cmd) => handle_throttle_profile(&conn, supported_properties, cmd)?, - CliCommand::FanCurve(cmd) => fan_curve_cli::handle_fan_curve(&conn, cmd)?, + CliCommand::Profile(cmd) => handle_throttle_profile(conn, supported_properties, cmd)?, + CliCommand::FanCurve(cmd) => fan_curve_cli::handle_fan_curve(conn, cmd)?, @@ - CliCommand::Battery(cmd) => handle_battery(cmd, &conn)?, + CliCommand::Battery(cmd) => handle_battery(cmd, conn)?,🤖 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/main.rs` around lines 102 - 108, Update do_parsed to accept conn as &Connection, remove the #[allow(clippy::needless_pass_by_value)] attribute, and adjust its sole call site to pass a reference while preserving the existing connection usage.rog-platform/src/slash/usb.rs (1)
40-61: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate board-name matching in the same crate.
This chain is identical to
SlashType::from_dmi()inrog-platform/src/slash/data.rslines 52-76. Both readDMIID, upper-caseboard_name, and apply the same orderedcontainschecks with the same model strings. Two copies drift when a new board is added. Keep one implementation and make the other delegate to 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 `@rog-platform/src/slash/usb.rs` around lines 40 - 61, Remove the duplicated board-name matching chain from the USB slash detection logic and delegate to the existing SlashType::from_dmi() implementation in data.rs. Ensure the USB path continues using the same uppercase DMI board name and preserves the existing ordered model matching and Unsupported fallback.
🤖 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/fan_curve_cli.rs`:
- Around line 97-103: Update the warning in the enable_fan_curve branch of the
mod_profile handling to report the missing --fan argument, replacing the
misleading REQ_MOD_PROFILE_MSG reference while preserving the existing control
flow.
In `@asusctl/src/platform_cli.rs`:
- Around line 514-526: Update the ArmourySubCommand::List arm in
handle_armoury_command to propagate the error from find_iface_blocking, matching
the error mapping used by the Get and Set arms so daemon discovery failures
return a nonzero error. Also verify the function no longer triggers
clippy::manual_is_multiple_of or clippy::nonminimal_bool and remove the stale
allow attribute if neither lint is needed.
- Line 72: Remove the find_iface_blocking re-export from platform_cli, then
import find_iface_blocking directly from rog_dbus in anime_cli and scsi_cli and
update their calls to use the direct symbol instead of
crate::platform_cli::find_iface_blocking.
- Around line 341-386: Consolidate the redundant power.command handling in the
aura power flow: after the existing missing-command return, use a single if-let
binding to process the command and remove the nested Option match around the
SetAuraZoneEnabled arms. Also remove the misleading “Commands available” print,
or replace it with an actual command listing if one already exists.
In `@asusctl/src/scsi_cli.rs`:
- Around line 38-40: Move the `--list` handling using `AuraMode::list()` ahead
of the `find_iface_blocking` call in the scsi CLI flow, returning the listed
modes without requiring interface discovery. Remove the now-unreachable trailing
`--list` block while preserving device-dependent behavior for other commands.
- Around line 33-36: The missing-argument guards must stop execution instead of
falling through to device access. In asusctl/src/scsi_cli.rs lines 33-36, update
handle_scsi’s condition to also require cmd.speed.is_none() and
cmd.direction.is_none(), then return Ok(()) after warning; in
asusctl/src/slash_cli.rs lines 70-83, update handle_slash_set to return Ok(())
immediately after its warning.
In `@rog-dbus/src/lib.rs`:
- Around line 99-144: Remove the duplicate find_iface_blocking definitions from
rog-control-center/src/zbus_proxies.rs, asusctl/src/anime_cli.rs, and
asusctl/src/scsi_cli.rs. Re-export rog_dbus::find_iface_blocking from the
consumer crates or update platform_cli::find_iface_blocking to delegate to
rog_dbus::find_iface_blocking, preserving existing call-site behavior and
leaving rog-dbus’s implementation as the single source of truth.
In `@rog-platform/src/scsi/builtin_modes.rs`:
- Around line 351-463: Keep four-colour support in ModeData: update
rog-platform/src/scsi/builtin_modes.rs lines 351-463 in ModeData::to_tasks to
emit an rgb_task for each populated colour zone, and update lines 300-308 in its
Display implementation to include colour3 and colour4 alongside the existing
colour fields; preserve the existing mode-specific task ordering and apply/save
tasks.
- Around line 300-308: Update the Display implementation for ModeData::fmt to
include self.colour3 and self.colour4 in the formatted output alongside colour1
and colour2, preserving the existing labels and formatting style so all four
colours assigned by the SCSI CLI are visible.
- Around line 353-459: Collapse the identical arms in the AuraMode match by
using or-patterns: merge Breathe with Flashing, RainbowCycle with
RainbowCycleBreathe,
RainbowCycleChaseFade/RainbowCycleChase/RainbowCycleWave/RainbowPulseChase, and
ChaseFade/Chase/DoubleFade. Preserve each group’s existing task sequence and
leave the distinct mode arms unchanged.
In `@rog-platform/src/scsi/sg.rs`:
- Around line 247-255: Update the request declaration in the ioctl call near
SG_IO to use the platform-specific libc request type rather than hard-coded
i32/u64 widths. Preserve the musl and non-musl target handling while ensuring
the type matches libc::ioctl on both 32-bit and 64-bit glibc targets; verify the
repository’s supported targets before choosing the appropriate alias.
- Around line 244-250: Set a non-zero default timeout for every task before
issuing SG_IO in SCSI task execution, covering tasks created by rgb_task,
mode_task, speed_task, dir_task, apply_task, and save_task. Update the
initialization path around Task::new or perform so SgIoHdr.timeout is populated
in milliseconds while preserving any explicitly configured timeout.
- Around line 93-111: Replace the derived Clone implementation for Task with a
manual implementation that clones inner, cmd, data, and sense, then
re-synchronizes the cloned SgIoHdr raw pointers to the clone’s own buffers
before returning it. Reuse the existing pointer-update mechanism, such as
sync_pointers, so cloned Tasks preserve the invariant required by the unsafe
Send and Sync implementations.
In `@rog-platform/src/slash/usb.rs`:
- Around line 35-39: Update get_slash_type so DMIID::new errors are handled
explicitly instead of converted with the discarded map_err and unwrap_or_default
chain. Log the DMI read failure, then preserve the existing fallback behavior
for determining the slash type without pretending SlashError::NoSlashDevice is
returned.
- Line 150: Update slash_pkt_set_mode to assign pkt2[9] based on slash_type:
retain 0x19 for existing models and use the documented 0x10 value for GA605
devices. Ensure each model receives its confirmed packet byte without changing
other packet fields.
---
Outside diff comments:
In `@asusctl/src/main.rs`:
- Around line 102-108: Update do_parsed to accept conn as &Connection, remove
the #[allow(clippy::needless_pass_by_value)] attribute, and adjust its sole call
site to pass a reference while preserving the existing connection usage.
In `@asusctl/src/slash_cli.rs`:
- Around line 70-83: Update handle_slash_set so the missing-argument branch
returns immediately after issuing the existing usage warning, preventing the
subsequent D-Bus connection and proxy setup from running. Match the early-return
behavior already used by handle_anime and handle_fan_curve.
In `@rog-platform/src/slash/usb.rs`:
- Around line 40-61: Remove the duplicated board-name matching chain from the
USB slash detection logic and delegate to the existing SlashType::from_dmi()
implementation in data.rs. Ensure the USB path continues using the same
uppercase DMI board name and preserves the existing ordered model matching and
Unsupported fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27c1fd9e-87c2-4c28-9539-a8f293dcf775
📒 Files selected for processing (50)
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_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/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/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/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)
- Cargo.toml
- rog-control-center/Cargo.toml
- dmi-id/Cargo.toml
- rog-scsi/Cargo.toml
- asusctl/Cargo.toml
- rog-scsi/src/error.rs
- rog-slash/Cargo.toml
- rog-slash/src/error.rs
📜 Review details
🔇 Additional comments (72)
asusd/src/daemon.rs (3)
33-33: LGTM!
35-43: LGTM!
160-165: LGTM!rog-profiles/src/fan_curve_set.rs (1)
29-36: LGTM!asusd/Cargo.toml (1)
20-20: LGTM!asusd/src/aura_manager.rs (1)
14-14: LGTM!asusd/src/aura_scsi/config.rs (1)
5-5: LGTM!asusd/src/aura_scsi/mod.rs (1)
4-4: LGTM!asusd/src/aura_scsi/trait_impls.rs (1)
6-6: LGTM!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 (2)
11-13: LGTM!
84-84: 🗄️ Data Integrity & IntegrationKeep the Slash wrapper.
rog_platform::slash::SlashErroris a public alias forrog_platform::error::PlatformError. This construction already uses the sharedPlatformErrortype, so no mapping change is needed.> Likely an incorrect or invalid review comment.asusd/src/error.rs (2)
4-5: LGTM!
112-112: 🗄️ Data Integrity & IntegrationRetain the current error mapping.
SlashErroraliasesPlatformError, so?maps toRogError::Platform; explicitRogError::Slashconstruction remains valid. A separateFrom<SlashError>implementation would overlap withFrom<PlatformError>.> Likely an incorrect or invalid review comment.asusd/src/lib.rs (1)
26-26: LGTM!rog-anime/Cargo.toml (1)
18-18: LGTM!Also applies to: 36-36
rog-anime/src/data.rs (1)
6-7: LGTM!Also applies to: 320-320
rog-anime/src/usb.rs (1)
13-13: LGTM!rog-aura/Cargo.toml (1)
21-21: LGTM!rog-aura/src/aura_detection.rs (1)
4-4: LGTM!rog-control-center/src/main.rs (1)
8-9: LGTM!Also applies to: 23-23, 86-91
rog-control-center/src/ui/setup_slash.rs (1)
7-7: 🗄️ Data Integrity & IntegrationNo change required.
SlashProxyusesrog_platform::slash::SlashMode, andSlashMode::list()preserves the previous ordering and strings.> Likely an incorrect or invalid review comment.asusctl/src/anime_cli.rs (5)
2-3: LGTM!
296-326: LGTM!
328-336: LGTM!
338-376: LGTM!
200-204: 🎯 Functional CorrectnessConfirm the AniMe Matrix clear-buffer value. The
--clearpath writes255u8; determine whether the protocol requires0u8before changing this code.asusctl/src/fan_curve_cli.rs (3)
2-2: LGTM!
85-96: LGTM!Also applies to: 104-113
47-69: LGTM!asusctl/src/platform_cli.rs (9)
1-19: LGTM!Also applies to: 21-61
74-109: LGTM!
111-135: LGTM!
137-172: LGTM!
174-214: LGTM!
216-273: LGTM!
303-330: LGTM!
392-436: LGTM!Also applies to: 438-512
277-301: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the inverted device-type guard in
handle_led_power1.Line 279 warns "This option applies only to keyboards 2021+" when the device is old or TUF. Line 293 then applies the setting for exactly those devices. The warning fires on every supported device and stays silent on unsupported devices.
Compare
handle_led_power2at lines 336-339, which uses the correct shape: test the unsupported case, warn, thencontinue.Line 294-295 also returns after the first matching device. Remaining proxies are skipped.
🐛 Proposed fix
for aura in aura { let dev_type = aura.device_type()?; - if !dev_type.is_old_laptop() && !dev_type.is_tuf_laptop() { - warn!("This option applies only to keyboards 2021+"); - } + if !dev_type.is_old_laptop() && !dev_type.is_tuf_laptop() { + warn!("These options are for keyboards of product ID 0x1866 or TUF only"); + continue; + } if power.awake.is_none() && power.sleep.is_none() && power.boot.is_none() && !power.keyboard && !power.lightbar { warn!("Missing arg or command; run 'asusctl aura power-tuf --help' for usage"); return Ok(()); } - if dev_type.is_old_laptop() || dev_type.is_tuf_laptop() { - handle_led_power_1_do_1866(&aura, power)?; - return Ok(()); - } + handle_led_power_1_do_1866(&aura, power)?; } - warn!("These options are for keyboards of product ID 0x1866 or TUF only"); Ok(())> Likely an incorrect or invalid review comment.asusctl/src/scsi_cli.rs (2)
42-87: LGTM!
2-3: 🩺 Stability & AvailabilityNo change required.
rog_platform::scsiis public, re-exports all four types, and each type implementsFromStrwith aDisplay-able error. No feature gate is required.> Likely an incorrect or invalid review comment.asusctl/src/slash_cli.rs (2)
2-4: LGTM!
142-149: LGTM!asusctl/src/main.rs (3)
109-135: LGTM!
89-100: LGTM!
15-21: 🩺 Stability & AvailabilityNo change needed.
xgm_led_cliremains declared at line 22, andasusctl/src/xgm_led_cli.rsexists.> Likely an incorrect or invalid review comment.rog-platform/Cargo.toml (2)
11-13: LGTM!
24-25: LGTM!rog-platform/src/dmi.rs (1)
94-94: LGTM!rog-platform/src/error.rs (2)
60-89: LGTM!
104-115: LGTM!rog-platform/src/lib.rs (1)
8-21: LGTM!rog-platform/src/scsi/mod.rs (1)
2-5: LGTM!rog-platform/src/slash/mod.rs (1)
6-7: LGTM!rog-dbus/src/zbus_slash.rs (1)
1-1: LGTM!rog-control-center/src/zbus_proxies.rs (1)
71-91: LGTM!rog-platform/src/scsi/protocol.rs (1)
1-1: LGTM!rog-platform/src/scsi/builtin_modes.rs (3)
8-10: LGTM!
211-256: LGTM!
466-472: LGTM!rog-platform/src/scsi/sg.rs (4)
9-43: 🩺 Stability & AvailabilityVerify the hand-written
sg_io_hdr_tABI.
SgIoHdrreproduces the kernelsg_io_hdr_tlayout by hand. The field order, the field widths, andSG_IO = 0x2285must match<scsi/sg.h>exactly on every supported target. A mismatch causes the kernel to read the wrong offsets and to issue the wrong SCSI command.Run the following script to compare the declaration against the system header:
#!/bin/bash # Description: Compare the Rust SgIoHdr declaration with the kernel scsi/sg.h definition. set -euo pipefail for h in /usr/include/scsi/sg.h /usr/include/linux/../scsi/sg.h; do [ -f "$h" ] || continue echo "== $h ==" sed -n '/typedef struct sg_io_hdr/,/} sg_io_hdr_t/p' "$h" rg -n 'define\s+SG_IO\b|SG_DXFER_|SG_INFO_OK' "$h" done echo "== Rust declaration ==" fd -t f 'sg.rs' | xargs rg -n -A 30 'pub struct SgIoHdr'
123-148: LGTM!
228-239: LGTM!Also applies to: 264-268
223-225: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ok()reports only the SGinfofield and hides SCSI command failures.
ok()inspectsinner.infoalone. A command that reaches the device and returns CHECK CONDITION setsstatus,host_status, ordriver_status, andinfocan still report OK. A caller that trustsok()treats a failed LED write as a success.Include the remaining status fields, or rename the method so the narrow meaning is explicit.
🛡️ Proposed fix
pub fn ok(&self) -> bool { - (self.inner.info & SG_INFO_OK_MASK) == SG_INFO_OK + (self.inner.info & SG_INFO_OK_MASK) == SG_INFO_OK + && self.inner.status == 0 + && self.inner.host_status == 0 + && self.inner.driver_status == 0 }> Likely an incorrect or invalid review comment.rog-platform/src/slash/data.rs (1)
6-10: 🩺 Stability & AvailabilityConfirm that the
zbusimport is gated when thedbusfeature is off.Line 6 imports
zbus::zvariantwith no visiblecfgattribute.rog-dbus/Cargo.tomlenablesrog_platformwithfeatures = ["dbus"], which indicates thatzbusis optional inrog-platform. Ifzbusis optional and this import is unconditional, the default-feature build ofrog-platformfails to resolve the crate. The preceding line is outside the provided range, so the gating cannot be confirmed here.Run the following script to check the feature declaration and the gating:
#!/bin/bash # Description: Check whether zbus is optional in rog-platform and whether its imports are gated. set -euo pipefail echo "== rog-platform manifest ==" fd -t f 'Cargo.toml' rog-platform | xargs cat echo "== zbus imports in rog-platform with preceding context ==" rg -nP -B2 '^\s*use\s+zbus' rog-platform/src echo "== dbus feature gates ==" rg -nP 'feature\s*=\s*"dbus"' rog-platform/srcrog-platform/src/slash/usb.rs (2)
11-14: LGTM!
140-157: 🗄️ Data Integrity & IntegrationVerify the changed Slash mode packet against hardware.
0xd2is now inserted atpkt2[1], every following field moved one index right, and the previous trailing byte is gone. The payload is an opaque vendor wire format.asusd/src/aura_slash/trait_impls.rsline 151 andasusd/src/aura_slash/mod.rsline 65 send these bytes directly to the device. If the new layout is wrong, Slash mode setting fails on every supported board and the source gives no way to detect it.Confirm the layout on a real device, and record the reason for the change in a comment or in the commit message. Add a unit test that pins the expected byte array so a future edit cannot shift the fields unnoticed.
Run the following script to find any existing coverage of the packet builders:
#!/bin/bash # Description: Look for tests that pin the Slash USB packet layout. set -euo pipefail rg -nP -C5 'slash_pkt_set_mode|slash_pkt_' -g '*.rs' rg -nP -C3 '#\[test\]' -g '*slash*.rs'rog-dbus/Cargo.toml (1)
17-18: LGTM!rog-dbus/src/lib.rs (1)
62-97: LGTM!rog-dbus/src/scsi_aura.rs (1)
23-23: 🗄️ Data Integrity & IntegrationThe
AuraEffectD-Bus signature changed with the addedzonefield.
AuraEffectis now an alias ofModeDatainrog-platform/src/scsi/builtin_modes.rsline 466, andModeDataaddszone: u32. The marshalled struct signature forLedModeDataandSetLedModeDatatherefore gains a member. A client built against the previous signature fails to decode the property. Confirm that no external consumer depends on the old signature, and record the change in the release notes if the interface is public.Run the following script to find every consumer of the SCSI Aura interface and any published introspection XML:
#!/bin/bash # Description: Find ScsiAura consumers and any checked-in D-Bus introspection data. set -euo pipefail rg -nP -C3 'ScsiAura|xyz\.ljones\.ScsiAura' -g '*.rs' fd -e xml | xargs rg -l 'ScsiAura' 2>/dev/null || echo "no introspection xml with ScsiAura" rg -nP -C3 'led_mode_data|LedModeData' -g '*.rs'
| 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}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report the missing --fan argument instead of --mod-profile.
Line 101 prints REQ_MOD_PROFILE_MSG. This branch runs inside if let Some(profile) = cmd.mod_profile, so --mod-profile is already supplied. The missing argument is --fan. The current message misleads 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");
}
}📝 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(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"); | |
| } | |
| } |
🤖 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 - 103, Update the warning in
the enable_fan_curve branch of the mod_profile handling to report the missing
--fan argument, replacing the misleading REQ_MOD_PROFILE_MSG reference while
preserving the existing control flow.
| println!(" Board name: {}", board_name.trim()); | ||
| } | ||
|
|
||
| pub use rog_dbus::find_iface_blocking; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the re-export and import find_iface_blocking directly.
Line 72 re-exports rog_dbus::find_iface_blocking from platform_cli. asusctl/src/anime_cli.rs line 169 and asusctl/src/scsi_cli.rs line 38 call it as crate::platform_cli::find_iface_blocking. Those modules then depend on platform_cli for a helper that platform_cli does not own. Import the function from rog_dbus in each module instead.
♻️ Proposed change
In asusctl/src/platform_cli.rs:
-pub use rog_dbus::find_iface_blocking; use rog_dbus::asus_armoury::AsusArmouryProxyBlocking;
+use rog_dbus::find_iface_blocking;
use rog_dbus::zbus_aura::AuraProxyBlocking;In asusctl/src/anime_cli.rs and asusctl/src/scsi_cli.rs, add use rog_dbus::find_iface_blocking; and drop the crate::platform_cli:: prefix.
🤖 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` at line 72, Remove the find_iface_blocking
re-export from platform_cli, then import find_iface_blocking directly from
rog_dbus in anime_cli and scsi_cli and update their calls to use the direct
symbol instead of crate::platform_cli::find_iface_blocking.
| if power.command.is_none() { | ||
| warn!("Missing arg or command; run 'asusctl aura power --help' for usage"); | ||
| println!("Commands available"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some(_pow) = power.command.as_ref() { | ||
| let mut states = aura.led_power()?; | ||
| let mut set = | ||
| |zone: PowerZones, boot_v: bool, awake_v: bool, sleep_v: bool, shutdown_v: bool| { | ||
| for state in states.states.iter_mut() { | ||
| if state.zone == zone { | ||
| state.boot = boot_v; | ||
| state.awake = awake_v; | ||
| state.sleep = sleep_v; | ||
| state.shutdown = shutdown_v; | ||
| break; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| if let Some(cmd) = &power.command { | ||
| match cmd { | ||
| crate::aura_cli::SetAuraZoneEnabled::Keyboard(k) => { | ||
| set(PowerZones::Keyboard, k.boot, k.awake, k.sleep, k.shutdown) | ||
| } | ||
| crate::aura_cli::SetAuraZoneEnabled::Logo(l) => { | ||
| set(PowerZones::Logo, l.boot, l.awake, l.sleep, l.shutdown) | ||
| } | ||
| crate::aura_cli::SetAuraZoneEnabled::Lightbar(l) => { | ||
| set(PowerZones::Lightbar, l.boot, l.awake, l.sleep, l.shutdown) | ||
| } | ||
| crate::aura_cli::SetAuraZoneEnabled::Lid(l) => { | ||
| set(PowerZones::Lid, l.boot, l.awake, l.sleep, l.shutdown) | ||
| } | ||
| crate::aura_cli::SetAuraZoneEnabled::RearGlow(r) => { | ||
| set(PowerZones::RearGlow, r.boot, r.awake, r.sleep, r.shutdown) | ||
| } | ||
| crate::aura_cli::SetAuraZoneEnabled::Ally(r) => { | ||
| set(PowerZones::Ally, r.boot, r.awake, r.sleep, r.shutdown) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| aura.set_led_power(states)?; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the redundant Option match on power.command.
Line 347 binds Some(_pow) and never uses it. Line 362 matches the same Option again. Line 341 already returns when power.command is None, so both checks are unconditional at that point. Collapse them into one if let.
Line 343 also prints "Commands available" and then returns without listing any command. Either list the commands or remove the line.
♻️ Proposed change
if power.command.is_none() {
warn!("Missing arg or command; run 'asusctl aura power --help' for usage");
- println!("Commands available");
return Ok(());
}
- if let Some(_pow) = power.command.as_ref() {
+ if let Some(cmd) = power.command.as_ref() {
let mut states = aura.led_power()?;
let mut set =
|zone: PowerZones, boot_v: bool, awake_v: bool, sleep_v: bool, shutdown_v: bool| {
@@
};
- if let Some(cmd) = &power.command {
- match cmd {
+ match cmd {
crate::aura_cli::SetAuraZoneEnabled::Keyboard(k) => {
set(PowerZones::Keyboard, k.boot, k.awake, k.sleep, k.shutdown)
}
@@
crate::aura_cli::SetAuraZoneEnabled::Ally(r) => {
set(PowerZones::Ally, r.boot, r.awake, r.sleep, r.shutdown)
}
- }
}
aura.set_led_power(states)?;📝 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 power.command.is_none() { | |
| warn!("Missing arg or command; run 'asusctl aura power --help' for usage"); | |
| println!("Commands available"); | |
| return Ok(()); | |
| } | |
| if let Some(_pow) = power.command.as_ref() { | |
| let mut states = aura.led_power()?; | |
| let mut set = | |
| |zone: PowerZones, boot_v: bool, awake_v: bool, sleep_v: bool, shutdown_v: bool| { | |
| for state in states.states.iter_mut() { | |
| if state.zone == zone { | |
| state.boot = boot_v; | |
| state.awake = awake_v; | |
| state.sleep = sleep_v; | |
| state.shutdown = shutdown_v; | |
| break; | |
| } | |
| } | |
| }; | |
| if let Some(cmd) = &power.command { | |
| match cmd { | |
| crate::aura_cli::SetAuraZoneEnabled::Keyboard(k) => { | |
| set(PowerZones::Keyboard, k.boot, k.awake, k.sleep, k.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Logo(l) => { | |
| set(PowerZones::Logo, l.boot, l.awake, l.sleep, l.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Lightbar(l) => { | |
| set(PowerZones::Lightbar, l.boot, l.awake, l.sleep, l.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Lid(l) => { | |
| set(PowerZones::Lid, l.boot, l.awake, l.sleep, l.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::RearGlow(r) => { | |
| set(PowerZones::RearGlow, r.boot, r.awake, r.sleep, r.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Ally(r) => { | |
| set(PowerZones::Ally, r.boot, r.awake, r.sleep, r.shutdown) | |
| } | |
| } | |
| } | |
| aura.set_led_power(states)?; | |
| } | |
| if power.command.is_none() { | |
| warn!("Missing arg or command; run 'asusctl aura power --help' for usage"); | |
| return Ok(()); | |
| } | |
| if let Some(cmd) = power.command.as_ref() { | |
| let mut states = aura.led_power()?; | |
| let mut set = | |
| |zone: PowerZones, boot_v: bool, awake_v: bool, sleep_v: bool, shutdown_v: bool| { | |
| for state in states.states.iter_mut() { | |
| if state.zone == zone { | |
| state.boot = boot_v; | |
| state.awake = awake_v; | |
| state.sleep = sleep_v; | |
| state.shutdown = shutdown_v; | |
| break; | |
| } | |
| } | |
| }; | |
| match cmd { | |
| crate::aura_cli::SetAuraZoneEnabled::Keyboard(k) => { | |
| set(PowerZones::Keyboard, k.boot, k.awake, k.sleep, k.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Logo(l) => { | |
| set(PowerZones::Logo, l.boot, l.awake, l.sleep, l.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Lightbar(l) => { | |
| set(PowerZones::Lightbar, l.boot, l.awake, l.sleep, l.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Lid(l) => { | |
| set(PowerZones::Lid, l.boot, l.awake, l.sleep, l.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::RearGlow(r) => { | |
| set(PowerZones::RearGlow, r.boot, r.awake, r.sleep, r.shutdown) | |
| } | |
| crate::aura_cli::SetAuraZoneEnabled::Ally(r) => { | |
| set(PowerZones::Ally, r.boot, r.awake, r.sleep, r.shutdown) | |
| } | |
| } | |
| aura.set_led_power(states)?; | |
| } |
🤖 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 341 - 386, Consolidate the
redundant power.command handling in the aura power flow: after the existing
missing-command return, use a single if-let binding to process the command and
remove the nested Option match around the SetAuraZoneEnabled arms. Also remove
the misleading “Commands available” print, or replace it with an actual command
listing if one already exists.
| pub fn handle_scsi(cmd: &ScsiCommand) -> Result<(), Box<dyn std::error::Error>> { | ||
| if !cmd.list && cmd.enable.is_none() && cmd.mode.is_none() && cmd.colours.is_empty() { | ||
| warn!("Missing arg or command; run 'asusctl scsi --help' for usage"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing-argument guards warn and then continue in handle_scsi and handle_slash_set. Both extracted handlers log "Missing arg" and fall through to D-Bus discovery and device access. handle_anime at asusctl/src/anime_cli.rs line 166 and handle_fan_curve at asusctl/src/fan_curve_cli.rs line 61 return in the equivalent branch. Make the two handlers consistent.
asusctl/src/scsi_cli.rs#L33-L36: addreturn Ok(())after the warning, and addcmd.speed.is_none()andcmd.direction.is_none()to the condition. Without those two fields,asusctl scsi --speed fastwarns and then still applies the speed at lines 71-74.asusctl/src/slash_cli.rs#L70-L83: addreturn Ok(())after the warning at line 82, so the handler does not open a D-Bus connection at line 85 for a no-op invocation.
📍 Affects 2 files
asusctl/src/scsi_cli.rs#L33-L36(this comment)asusctl/src/slash_cli.rs#L70-L83
🤖 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/scsi_cli.rs` around lines 33 - 36, The missing-argument guards
must stop execution instead of falling through to device access. In
asusctl/src/scsi_cli.rs lines 33-36, update handle_scsi’s condition to also
require cmd.speed.is_none() and cmd.direction.is_none(), then return Ok(())
after warning; in asusctl/src/slash_cli.rs lines 70-83, update handle_slash_set
to return Ok(()) immediately after its warning.
| 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
Include colour3 and colour4 in the ModeData display output.
ModeData carries four colours. The formatter prints only colour1 and colour2. The SCSI CLI in asusctl/src/scsi_cli.rs assigns up to four colours and then prints this value, so the user cannot confirm colour3 and colour4.
🐛 Proposed fix
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::fmt to include self.colour3 and
self.colour4 in the formatted output alongside colour1 and colour2, preserving
the existing labels and formatting style so all four colours assigned by the
SCSI CLI are visible.
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
ModeData declares four colours and the crate consumes only one. colour2, colour3, and colour4 are written by asusctl/src/scsi_cli.rs, transported by rog-dbus/src/scsi_aura.rs, and then dropped. Neither the task generator nor the formatter reads them. Decide once whether the type supports four colours, then apply that decision at both sites.
rog-platform/src/scsi/builtin_modes.rs#L351-L463: emit onergb_taskper populated colour zone into_tasks, or removecolour2..colour4fromModeDataand from the CLI.rog-platform/src/scsi/builtin_modes.rs#L300-L308: addcolour3andcolour4to theDisplayoutput if the fields stay, or dropcolour2from the output if the fields are removed.
📍 Affects 1 file
rog-platform/src/scsi/builtin_modes.rs#L351-L463(this comment)rog-platform/src/scsi/builtin_modes.rs#L300-L308
🤖 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, Keep
four-colour support in ModeData: update rog-platform/src/scsi/builtin_modes.rs
lines 351-463 in ModeData::to_tasks to emit an rgb_task for each populated
colour zone, and update lines 300-308 in its Display implementation to include
colour3 and colour4 alongside the existing colour fields; preserve the existing
mode-specific task ordering and apply/save tasks.
| 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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Collapse the duplicated match arms.
Several arms are byte-identical. Breathe and Flashing are the same. RainbowCycle and RainbowCycleBreathe are the same. RainbowCycleChaseFade, RainbowCycleChase, RainbowCycleWave, and RainbowPulseChase are the same. ChaseFade, Chase, and DoubleFade are the same. Merge them with or-patterns to reduce the arm count and to make the per-mode differences visible.
♻️ Example of the merge
- 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 => {
+ AuraMode::Breathe | AuraMode::Flashing | 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));
}🤖 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 353 - 459, Collapse the
identical arms in the AuraMode match by using or-patterns: merge Breathe with
Flashing, RainbowCycle with RainbowCycleBreathe,
RainbowCycleChaseFade/RainbowCycleChase/RainbowCycleWave/RainbowPulseChase, and
ChaseFade/Chase/DoubleFade. Preserve each group’s existing task sequence and
leave the distinct mode arms unchanged.
| pub fn get_slash_type() -> SlashType { | ||
| let dmi = DMIID::new() | ||
| .map_err(|_| SlashError::NoDevice) | ||
| .map_err(|_| SlashError::NoSlashDevice) | ||
| .unwrap_or_default(); | ||
| let board_name = dmi.board_name.to_uppercase(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The map_err value is discarded and the DMI failure is silent.
unwrap_or_default() consumes the Result immediately, so SlashError::NoSlashDevice is never returned and never observed. The change from NoDevice to NoSlashDevice has no runtime effect. A DMI read failure also produces an empty board name, and the function then returns SlashType::Unsupported with no diagnostic. Remove the dead mapping and log the failure.
🐛 Proposed fix
pub fn get_slash_type() -> SlashType {
- let dmi = DMIID::new()
- .map_err(|_| SlashError::NoSlashDevice)
- .unwrap_or_default();
+ let dmi = DMIID::new().unwrap_or_else(|e| {
+ log::warn!("Slash: DMI read failed, treating device as unsupported: {e}");
+ DMIID::default()
+ });
let board_name = dmi.board_name.to_uppercase();📝 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 get_slash_type() -> SlashType { | |
| let dmi = DMIID::new() | |
| .map_err(|_| SlashError::NoDevice) | |
| .map_err(|_| SlashError::NoSlashDevice) | |
| .unwrap_or_default(); | |
| let board_name = dmi.board_name.to_uppercase(); | |
| pub fn get_slash_type() -> SlashType { | |
| let dmi = DMIID::new().unwrap_or_else(|e| { | |
| log::warn!("Slash: DMI read failed, treating device as unsupported: {e}"); | |
| DMIID::default() | |
| }); | |
| let board_name = dmi.board_name.to_uppercase(); |
🤖 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` around lines 35 - 39, Update get_slash_type so
DMIID::new errors are handled explicitly instead of converted with the discarded
map_err and unwrap_or_default chain. Log the DMI read failure, then preserve the
existing fallback behavior for determining the slash type without pretending
SlashError::NoSlashDevice is returned.
| 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
The GA605 difference is documented but not implemented.
The comment states that GA605 needs 0x10 at this position. The code writes 0x19 for every model. slash_type is already a parameter of slash_pkt_set_mode, so the branch is available. GA605 devices receive the wrong byte.
🐛 Proposed fix
- pkt2[9] = 0x19; // difference, GA605 = 0x10
+ pkt2[9] = match slash_type {
+ SlashType::GA605_2025 | SlashType::GA605_2024 => 0x10,
+ _ => 0x19,
+ };Confirm the correct value for each model before you apply this change. Do you want me to open an issue to track the GA605 packet difference?
📝 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_2025 | SlashType::GA605_2024 => 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 slash_pkt_set_mode to
assign pkt2[9] based on slash_type: retain 0x19 for existing models and use the
documented 0x10 value for GA605 devices. Ensure each model receives its
confirmed packet byte without changing other packet fields.
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
asusctl/src/slash_cli.rs (1)
70-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReturn after the missing-argument warning.
Line 82 warns and then falls through. Lines 85-87 open a D-Bus connection and create the proxy, and every following block is skipped because all options are unset.
handle_animeat line 166 andhandle_fan_curveat line 61 return in the equivalent branch.🐛 Proposed fix
&& !cmd.disable { warn!("Missing arg; run 'asusctl slash set --help' for usage"); + return Ok(()); }🤖 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/slash_cli.rs` around lines 70 - 83, Update handle_slash_set so the missing-argument branch returns immediately after issuing the existing usage warning, preventing the subsequent D-Bus connection and proxy setup from running. Match the early-return behavior already used by handle_anime and handle_fan_curve.asusctl/src/main.rs (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTake
connby reference instead of suppressing the lint.Line 102 adds
#[allow(clippy::needless_pass_by_value)].do_parsedusesconnonly as&connat lines 116, 117, and 127. Change the parameter to&Connectionand remove the attribute. Update the single call site accordingly.♻️ Proposed change
-#[allow(clippy::needless_pass_by_value)] fn do_parsed( parsed: &CliStart, supported_interfaces: &[String], supported_properties: &[Properties], - conn: Connection, + conn: &Connection, ) -> Result<(), Box<dyn std::error::Error>> { @@ - CliCommand::Profile(cmd) => handle_throttle_profile(&conn, supported_properties, cmd)?, - CliCommand::FanCurve(cmd) => fan_curve_cli::handle_fan_curve(&conn, cmd)?, + CliCommand::Profile(cmd) => handle_throttle_profile(conn, supported_properties, cmd)?, + CliCommand::FanCurve(cmd) => fan_curve_cli::handle_fan_curve(conn, cmd)?, @@ - CliCommand::Battery(cmd) => handle_battery(cmd, &conn)?, + CliCommand::Battery(cmd) => handle_battery(cmd, conn)?,🤖 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/main.rs` around lines 102 - 108, Update do_parsed to accept conn as &Connection, remove the #[allow(clippy::needless_pass_by_value)] attribute, and adjust its sole call site to pass a reference while preserving the existing connection usage.rog-platform/src/slash/usb.rs (1)
40-61: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate board-name matching in the same crate.
This chain is identical to
SlashType::from_dmi()inrog-platform/src/slash/data.rslines 52-76. Both readDMIID, upper-caseboard_name, and apply the same orderedcontainschecks with the same model strings. Two copies drift when a new board is added. Keep one implementation and make the other delegate to 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 `@rog-platform/src/slash/usb.rs` around lines 40 - 61, Remove the duplicated board-name matching chain from the USB slash detection logic and delegate to the existing SlashType::from_dmi() implementation in data.rs. Ensure the USB path continues using the same uppercase DMI board name and preserves the existing ordered model matching and Unsupported fallback.
🤖 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/fan_curve_cli.rs`:
- Around line 97-103: Update the warning in the enable_fan_curve branch of the
mod_profile handling to report the missing --fan argument, replacing the
misleading REQ_MOD_PROFILE_MSG reference while preserving the existing control
flow.
In `@asusctl/src/platform_cli.rs`:
- Around line 514-526: Update the ArmourySubCommand::List arm in
handle_armoury_command to propagate the error from find_iface_blocking, matching
the error mapping used by the Get and Set arms so daemon discovery failures
return a nonzero error. Also verify the function no longer triggers
clippy::manual_is_multiple_of or clippy::nonminimal_bool and remove the stale
allow attribute if neither lint is needed.
- Line 72: Remove the find_iface_blocking re-export from platform_cli, then
import find_iface_blocking directly from rog_dbus in anime_cli and scsi_cli and
update their calls to use the direct symbol instead of
crate::platform_cli::find_iface_blocking.
- Around line 341-386: Consolidate the redundant power.command handling in the
aura power flow: after the existing missing-command return, use a single if-let
binding to process the command and remove the nested Option match around the
SetAuraZoneEnabled arms. Also remove the misleading “Commands available” print,
or replace it with an actual command listing if one already exists.
In `@asusctl/src/scsi_cli.rs`:
- Around line 38-40: Move the `--list` handling using `AuraMode::list()` ahead
of the `find_iface_blocking` call in the scsi CLI flow, returning the listed
modes without requiring interface discovery. Remove the now-unreachable trailing
`--list` block while preserving device-dependent behavior for other commands.
- Around line 33-36: The missing-argument guards must stop execution instead of
falling through to device access. In asusctl/src/scsi_cli.rs lines 33-36, update
handle_scsi’s condition to also require cmd.speed.is_none() and
cmd.direction.is_none(), then return Ok(()) after warning; in
asusctl/src/slash_cli.rs lines 70-83, update handle_slash_set to return Ok(())
immediately after its warning.
In `@rog-dbus/src/lib.rs`:
- Around line 99-144: Remove the duplicate find_iface_blocking definitions from
rog-control-center/src/zbus_proxies.rs, asusctl/src/anime_cli.rs, and
asusctl/src/scsi_cli.rs. Re-export rog_dbus::find_iface_blocking from the
consumer crates or update platform_cli::find_iface_blocking to delegate to
rog_dbus::find_iface_blocking, preserving existing call-site behavior and
leaving rog-dbus’s implementation as the single source of truth.
In `@rog-platform/src/scsi/builtin_modes.rs`:
- Around line 351-463: Keep four-colour support in ModeData: update
rog-platform/src/scsi/builtin_modes.rs lines 351-463 in ModeData::to_tasks to
emit an rgb_task for each populated colour zone, and update lines 300-308 in its
Display implementation to include colour3 and colour4 alongside the existing
colour fields; preserve the existing mode-specific task ordering and apply/save
tasks.
- Around line 300-308: Update the Display implementation for ModeData::fmt to
include self.colour3 and self.colour4 in the formatted output alongside colour1
and colour2, preserving the existing labels and formatting style so all four
colours assigned by the SCSI CLI are visible.
- Around line 353-459: Collapse the identical arms in the AuraMode match by
using or-patterns: merge Breathe with Flashing, RainbowCycle with
RainbowCycleBreathe,
RainbowCycleChaseFade/RainbowCycleChase/RainbowCycleWave/RainbowPulseChase, and
ChaseFade/Chase/DoubleFade. Preserve each group’s existing task sequence and
leave the distinct mode arms unchanged.
In `@rog-platform/src/scsi/sg.rs`:
- Around line 247-255: Update the request declaration in the ioctl call near
SG_IO to use the platform-specific libc request type rather than hard-coded
i32/u64 widths. Preserve the musl and non-musl target handling while ensuring
the type matches libc::ioctl on both 32-bit and 64-bit glibc targets; verify the
repository’s supported targets before choosing the appropriate alias.
- Around line 244-250: Set a non-zero default timeout for every task before
issuing SG_IO in SCSI task execution, covering tasks created by rgb_task,
mode_task, speed_task, dir_task, apply_task, and save_task. Update the
initialization path around Task::new or perform so SgIoHdr.timeout is populated
in milliseconds while preserving any explicitly configured timeout.
- Around line 93-111: Replace the derived Clone implementation for Task with a
manual implementation that clones inner, cmd, data, and sense, then
re-synchronizes the cloned SgIoHdr raw pointers to the clone’s own buffers
before returning it. Reuse the existing pointer-update mechanism, such as
sync_pointers, so cloned Tasks preserve the invariant required by the unsafe
Send and Sync implementations.
In `@rog-platform/src/slash/usb.rs`:
- Around line 35-39: Update get_slash_type so DMIID::new errors are handled
explicitly instead of converted with the discarded map_err and unwrap_or_default
chain. Log the DMI read failure, then preserve the existing fallback behavior
for determining the slash type without pretending SlashError::NoSlashDevice is
returned.
- Line 150: Update slash_pkt_set_mode to assign pkt2[9] based on slash_type:
retain 0x19 for existing models and use the documented 0x10 value for GA605
devices. Ensure each model receives its confirmed packet byte without changing
other packet fields.
---
Outside diff comments:
In `@asusctl/src/main.rs`:
- Around line 102-108: Update do_parsed to accept conn as &Connection, remove
the #[allow(clippy::needless_pass_by_value)] attribute, and adjust its sole call
site to pass a reference while preserving the existing connection usage.
In `@asusctl/src/slash_cli.rs`:
- Around line 70-83: Update handle_slash_set so the missing-argument branch
returns immediately after issuing the existing usage warning, preventing the
subsequent D-Bus connection and proxy setup from running. Match the early-return
behavior already used by handle_anime and handle_fan_curve.
In `@rog-platform/src/slash/usb.rs`:
- Around line 40-61: Remove the duplicated board-name matching chain from the
USB slash detection logic and delegate to the existing SlashType::from_dmi()
implementation in data.rs. Ensure the USB path continues using the same
uppercase DMI board name and preserves the existing ordered model matching and
Unsupported fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27c1fd9e-87c2-4c28-9539-a8f293dcf775
📒 Files selected for processing (50)
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_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/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/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/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)
- Cargo.toml
- rog-control-center/Cargo.toml
- dmi-id/Cargo.toml
- rog-scsi/Cargo.toml
- asusctl/Cargo.toml
- rog-scsi/src/error.rs
- rog-slash/Cargo.toml
- rog-slash/src/error.rs
📜 Review details
🔇 Additional comments (72)
asusd/src/daemon.rs (3)
33-33: LGTM!
35-43: LGTM!
160-165: LGTM!rog-profiles/src/fan_curve_set.rs (1)
29-36: LGTM!asusd/Cargo.toml (1)
20-20: LGTM!asusd/src/aura_manager.rs (1)
14-14: LGTM!asusd/src/aura_scsi/config.rs (1)
5-5: LGTM!asusd/src/aura_scsi/mod.rs (1)
4-4: LGTM!asusd/src/aura_scsi/trait_impls.rs (1)
6-6: LGTM!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 (2)
11-13: LGTM!
84-84: 🗄️ Data Integrity & IntegrationKeep the Slash wrapper.
rog_platform::slash::SlashErroris a public alias forrog_platform::error::PlatformError. This construction already uses the sharedPlatformErrortype, so no mapping change is needed.> Likely an incorrect or invalid review comment.asusd/src/error.rs (2)
4-5: LGTM!
112-112: 🗄️ Data Integrity & IntegrationRetain the current error mapping.
SlashErroraliasesPlatformError, so?maps toRogError::Platform; explicitRogError::Slashconstruction remains valid. A separateFrom<SlashError>implementation would overlap withFrom<PlatformError>.> Likely an incorrect or invalid review comment.asusd/src/lib.rs (1)
26-26: LGTM!rog-anime/Cargo.toml (1)
18-18: LGTM!Also applies to: 36-36
rog-anime/src/data.rs (1)
6-7: LGTM!Also applies to: 320-320
rog-anime/src/usb.rs (1)
13-13: LGTM!rog-aura/Cargo.toml (1)
21-21: LGTM!rog-aura/src/aura_detection.rs (1)
4-4: LGTM!rog-control-center/src/main.rs (1)
8-9: LGTM!Also applies to: 23-23, 86-91
rog-control-center/src/ui/setup_slash.rs (1)
7-7: 🗄️ Data Integrity & IntegrationNo change required.
SlashProxyusesrog_platform::slash::SlashMode, andSlashMode::list()preserves the previous ordering and strings.> Likely an incorrect or invalid review comment.asusctl/src/anime_cli.rs (5)
2-3: LGTM!
296-326: LGTM!
328-336: LGTM!
338-376: LGTM!
200-204: 🎯 Functional CorrectnessConfirm the AniMe Matrix clear-buffer value. The
--clearpath writes255u8; determine whether the protocol requires0u8before changing this code.asusctl/src/fan_curve_cli.rs (3)
2-2: LGTM!
85-96: LGTM!Also applies to: 104-113
47-69: LGTM!asusctl/src/platform_cli.rs (9)
1-19: LGTM!Also applies to: 21-61
74-109: LGTM!
111-135: LGTM!
137-172: LGTM!
174-214: LGTM!
216-273: LGTM!
303-330: LGTM!
392-436: LGTM!Also applies to: 438-512
277-301: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the inverted device-type guard in
handle_led_power1.Line 279 warns "This option applies only to keyboards 2021+" when the device is old or TUF. Line 293 then applies the setting for exactly those devices. The warning fires on every supported device and stays silent on unsupported devices.
Compare
handle_led_power2at lines 336-339, which uses the correct shape: test the unsupported case, warn, thencontinue.Line 294-295 also returns after the first matching device. Remaining proxies are skipped.
🐛 Proposed fix
for aura in aura { let dev_type = aura.device_type()?; - if !dev_type.is_old_laptop() && !dev_type.is_tuf_laptop() { - warn!("This option applies only to keyboards 2021+"); - } + if !dev_type.is_old_laptop() && !dev_type.is_tuf_laptop() { + warn!("These options are for keyboards of product ID 0x1866 or TUF only"); + continue; + } if power.awake.is_none() && power.sleep.is_none() && power.boot.is_none() && !power.keyboard && !power.lightbar { warn!("Missing arg or command; run 'asusctl aura power-tuf --help' for usage"); return Ok(()); } - if dev_type.is_old_laptop() || dev_type.is_tuf_laptop() { - handle_led_power_1_do_1866(&aura, power)?; - return Ok(()); - } + handle_led_power_1_do_1866(&aura, power)?; } - warn!("These options are for keyboards of product ID 0x1866 or TUF only"); Ok(())> Likely an incorrect or invalid review comment.asusctl/src/scsi_cli.rs (2)
42-87: LGTM!
2-3: 🩺 Stability & AvailabilityNo change required.
rog_platform::scsiis public, re-exports all four types, and each type implementsFromStrwith aDisplay-able error. No feature gate is required.> Likely an incorrect or invalid review comment.asusctl/src/slash_cli.rs (2)
2-4: LGTM!
142-149: LGTM!asusctl/src/main.rs (3)
109-135: LGTM!
89-100: LGTM!
15-21: 🩺 Stability & AvailabilityNo change needed.
xgm_led_cliremains declared at line 22, andasusctl/src/xgm_led_cli.rsexists.> Likely an incorrect or invalid review comment.rog-platform/Cargo.toml (2)
11-13: LGTM!
24-25: LGTM!rog-platform/src/dmi.rs (1)
94-94: LGTM!rog-platform/src/error.rs (2)
60-89: LGTM!
104-115: LGTM!rog-platform/src/lib.rs (1)
8-21: LGTM!rog-platform/src/scsi/mod.rs (1)
2-5: LGTM!rog-platform/src/slash/mod.rs (1)
6-7: LGTM!rog-dbus/src/zbus_slash.rs (1)
1-1: LGTM!rog-control-center/src/zbus_proxies.rs (1)
71-91: LGTM!rog-platform/src/scsi/protocol.rs (1)
1-1: LGTM!rog-platform/src/scsi/builtin_modes.rs (3)
8-10: LGTM!
211-256: LGTM!
466-472: LGTM!rog-platform/src/scsi/sg.rs (4)
9-43: 🩺 Stability & AvailabilityVerify the hand-written
sg_io_hdr_tABI.
SgIoHdrreproduces the kernelsg_io_hdr_tlayout by hand. The field order, the field widths, andSG_IO = 0x2285must match<scsi/sg.h>exactly on every supported target. A mismatch causes the kernel to read the wrong offsets and to issue the wrong SCSI command.Run the following script to compare the declaration against the system header:
#!/bin/bash # Description: Compare the Rust SgIoHdr declaration with the kernel scsi/sg.h definition. set -euo pipefail for h in /usr/include/scsi/sg.h /usr/include/linux/../scsi/sg.h; do [ -f "$h" ] || continue echo "== $h ==" sed -n '/typedef struct sg_io_hdr/,/} sg_io_hdr_t/p' "$h" rg -n 'define\s+SG_IO\b|SG_DXFER_|SG_INFO_OK' "$h" done echo "== Rust declaration ==" fd -t f 'sg.rs' | xargs rg -n -A 30 'pub struct SgIoHdr'
123-148: LGTM!
228-239: LGTM!Also applies to: 264-268
223-225: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ok()reports only the SGinfofield and hides SCSI command failures.
ok()inspectsinner.infoalone. A command that reaches the device and returns CHECK CONDITION setsstatus,host_status, ordriver_status, andinfocan still report OK. A caller that trustsok()treats a failed LED write as a success.Include the remaining status fields, or rename the method so the narrow meaning is explicit.
🛡️ Proposed fix
pub fn ok(&self) -> bool { - (self.inner.info & SG_INFO_OK_MASK) == SG_INFO_OK + (self.inner.info & SG_INFO_OK_MASK) == SG_INFO_OK + && self.inner.status == 0 + && self.inner.host_status == 0 + && self.inner.driver_status == 0 }> Likely an incorrect or invalid review comment.rog-platform/src/slash/data.rs (1)
6-10: 🩺 Stability & AvailabilityConfirm that the
zbusimport is gated when thedbusfeature is off.Line 6 imports
zbus::zvariantwith no visiblecfgattribute.rog-dbus/Cargo.tomlenablesrog_platformwithfeatures = ["dbus"], which indicates thatzbusis optional inrog-platform. Ifzbusis optional and this import is unconditional, the default-feature build ofrog-platformfails to resolve the crate. The preceding line is outside the provided range, so the gating cannot be confirmed here.Run the following script to check the feature declaration and the gating:
#!/bin/bash # Description: Check whether zbus is optional in rog-platform and whether its imports are gated. set -euo pipefail echo "== rog-platform manifest ==" fd -t f 'Cargo.toml' rog-platform | xargs cat echo "== zbus imports in rog-platform with preceding context ==" rg -nP -B2 '^\s*use\s+zbus' rog-platform/src echo "== dbus feature gates ==" rg -nP 'feature\s*=\s*"dbus"' rog-platform/srcrog-platform/src/slash/usb.rs (2)
11-14: LGTM!
140-157: 🗄️ Data Integrity & IntegrationVerify the changed Slash mode packet against hardware.
0xd2is now inserted atpkt2[1], every following field moved one index right, and the previous trailing byte is gone. The payload is an opaque vendor wire format.asusd/src/aura_slash/trait_impls.rsline 151 andasusd/src/aura_slash/mod.rsline 65 send these bytes directly to the device. If the new layout is wrong, Slash mode setting fails on every supported board and the source gives no way to detect it.Confirm the layout on a real device, and record the reason for the change in a comment or in the commit message. Add a unit test that pins the expected byte array so a future edit cannot shift the fields unnoticed.
Run the following script to find any existing coverage of the packet builders:
#!/bin/bash # Description: Look for tests that pin the Slash USB packet layout. set -euo pipefail rg -nP -C5 'slash_pkt_set_mode|slash_pkt_' -g '*.rs' rg -nP -C3 '#\[test\]' -g '*slash*.rs'rog-dbus/Cargo.toml (1)
17-18: LGTM!rog-dbus/src/lib.rs (1)
62-97: LGTM!rog-dbus/src/scsi_aura.rs (1)
23-23: 🗄️ Data Integrity & IntegrationThe
AuraEffectD-Bus signature changed with the addedzonefield.
AuraEffectis now an alias ofModeDatainrog-platform/src/scsi/builtin_modes.rsline 466, andModeDataaddszone: u32. The marshalled struct signature forLedModeDataandSetLedModeDatatherefore gains a member. A client built against the previous signature fails to decode the property. Confirm that no external consumer depends on the old signature, and record the change in the release notes if the interface is public.Run the following script to find every consumer of the SCSI Aura interface and any published introspection XML:
#!/bin/bash # Description: Find ScsiAura consumers and any checked-in D-Bus introspection data. set -euo pipefail rg -nP -C3 'ScsiAura|xyz\.ljones\.ScsiAura' -g '*.rs' fd -e xml | xargs rg -l 'ScsiAura' 2>/dev/null || echo "no introspection xml with ScsiAura" rg -nP -C3 'led_mode_data|LedModeData' -g '*.rs'
🛑 Comments failed to post (3)
rog-platform/src/scsi/sg.rs (3)
93-111: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Derived
Clonecopies stale raw pointers and invalidates theSend/Syncsafety argument.
#[derive(Clone)]copiesinnerfield by field.cmdp,dxferp, andsbpare copied verbatim, butcmd,data, andsenseare cloned into new allocations. The clonedTasktherefore holds raw pointers into the buffers of the originalTask. If the original is dropped, or if its vectors reallocate, the clone points to freed memory. Safe code can produce this state with nounsafeblock.The safety comment states that the pointers are updated prior to any ioctl.
Device::performdoes callsync_pointers, but it needs&mut Task, so a shared or stale clone is never repaired. The stated invariant does not hold for clones.Implement
Clonemanually and re-point the buffers in the new value.🔒️ Proposed fix
-#[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 { + let mut task = Task { + inner: self.inner, + cmd: self.cmd.clone(), + data: self.data.clone(), + sense: self.sense.clone(), + }; + // Re-point the header at this value's own buffers. + 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 owns cmd, data and sense. Every SgIoHdr raw pointer refers only into those +// owned buffers, and `sync_pointers` re-establishes that on construction, on every setter +// and on clone. No pointer refers to memory owned by another value or thread. 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 { let mut task = Task { inner: self.inner, cmd: self.cmd.clone(), data: self.data.clone(), sense: self.sense.clone(), }; // Re-point the header at this value's own buffers. task.sync_pointers(); task } } impl Default for Task { fn default() -> Self { Self::new() } } // SAFETY: Task owns cmd, data and sense. Every SgIoHdr raw pointer refers only into those // owned buffers, and `sync_pointers` re-establishes that on construction, on every setter // and on clone. No pointer refers to memory owned by another value or thread. unsafe impl Send for Task {} unsafe impl Sync 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 implementation that clones inner, cmd, data, and sense, then re-synchronizes the cloned SgIoHdr raw pointers to the clone’s own buffers before returning it. Reuse the existing pointer-update mechanism, such as sync_pointers, so cloned Tasks preserve the invariant required by the unsafe Send and Sync implementations.
244-250: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Every task reaches
performwithtimeoutset to 0.
SgIoHdr::defaultsetstimeout: 0, and the builders inrog-platform/src/scsi/protocol.rs(rgb_task,mode_task,speed_task,dir_task,apply_task,save_task) call onlyTask::new,set_cdb, andset_data. None of them callsset_timeout. The SG driver readstimeoutas milliseconds, so the value 0 leaves the command bound with no meaningful deadline. Apply a default timeout so a stalled device cannot block the calling thread indefinitely.🛡️ Proposed fix
+/// Default SG_IO command timeout in milliseconds. +pub const DEFAULT_TIMEOUT_MS: u32 = 5_000; + impl Default for SgIoHdr { fn default() -> Self { Self { interface_id: b'S' as std::os::raw::c_int, dxfer_direction: SG_DXFER_NONE, @@ - timeout: 0, + timeout: DEFAULT_TIMEOUT_MS,🤖 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 - 250, Set a non-zero default timeout for every task before issuing SG_IO in SCSI task execution, covering tasks created by rgb_task, mode_task, speed_task, dir_task, apply_task, and save_task. Update the initialization path around Task::new or perform so SgIoHdr.timeout is populated in milliseconds while preserving any explicitly configured timeout.
247-255: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Verify the
ioctlrequest type on every supported target.The code selects
i32for musl andu64otherwise.libc::ioctldeclares the request parameter asc_inton musl and asc_ulongon glibc.c_ulongis 32-bit on 32-bit glibc targets, so the hard-codedu64fails to compile there. Confirm the supported target list, then prefer the platform alias over a fixed width.♻️ Suggested alternative
#[cfg(target_env = "musl")] - let request = SG_IO as i32; + let request = SG_IO as libc::c_int; #[cfg(not(target_env = "musl"))] - let request: u64 = SG_IO; + let request = SG_IO as libc::c_ulong;Run the following script to check the declared target list:
#!/bin/bash # Description: Look for target declarations and cross-build configuration. set -euo pipefail fd -t f -H '^(Cargo.toml|rust-toolchain(.toml)?|.cargo)$' -d 3 | sort [ -f .cargo/config.toml ] && cat .cargo/config.toml [ -f rust-toolchain.toml ] && cat rust-toolchain.toml rg -n 'target\s*=|targets\s*=|--target' -g '!target/**' -g '*.toml' -g '*.yml' -g '*.yaml' || true🤖 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 247 - 255, Update the request declaration in the ioctl call near SG_IO to use the platform-specific libc request type rather than hard-coded i32/u64 widths. Preserve the musl and non-musl target handling while ensuring the type matches libc::ioctl on both 32-bit and 64-bit glibc targets; verify the repository’s supported targets before choosing the appropriate alias.
1 participant
Description
This PR simplifies the workspace architecture by consolidating three single-purpose hardware micro-crates (
dmi-id,rog-scsi, androg-slash) directly intorog-platformas internal modules (dmi,scsi,slash).Key Changes
Crate Consolidation into
rog-platform:dmi-id➔rog_platform::dmirog-scsi➔rog_platform::scsirog-slash➔rog_platform::slashrog_platform::error::PlatformError.Workspace & Dependency Cleanup:
dmi-id/,rog-scsi/,rog-slash/) and theirCargo.tomlmanifests.Cargo.tomldependencies in root workspace,asusd,asusctl,rog-anime,rog-aura,rog-control-center, androg-dbus.rog_platformimports.Utility & Profile Refactorings:
find_iface_blockingandfind_iface_async) directly intorog-dbus.CurveDatastring parsing and formatting inrog-profilesusing iterator combinators.asusdlog messages and code comments.