Refactor cleanup logic and improve argument validation in AsusFanControl - #6
Conversation
Co-authored-by: beenycool <129210955+beenycool@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @beenycool, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and reliability of the Asus fan control application by implementing proper resource cleanup mechanisms and improving argument validation. It ensures that fan settings are consistently reset to default upon application termination, preventing unintended fan behavior, and makes the command-line interface more robust against invalid user input. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThis pull request improves resource management by implementing the IDisposable pattern in AsusControl, introducing a ResetToDefault() method to safely reset all fans, and updating all three entry points to use a consistent cleanup sequence with error handling and disposal guarantees. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of refactoring the cleanup logic by implementing IDisposable and adding handlers for process exit. The argument validation is also improved. My review includes a few suggestions to make the resource management even more robust by correctly implementing the full IDisposable pattern and ensuring cleanup happens on all exit paths. I've also pointed out a couple of places where argument parsing can be made safer to prevent crashes from invalid input.
| public void Dispose() | ||
| { | ||
| AsusWinIO64.ShutdownWinIo(); | ||
| GC.SuppressFinalize(this); | ||
| } |
There was a problem hiding this comment.
Implementing IDisposable is a good step. However, the current implementation has removed the finalizer (~AsusControl), which acted as a safety net for resource cleanup. If Dispose() isn't called, ShutdownWinIo() will not be called, leading to a resource leak. Also, GC.SuppressFinalize(this) is ineffective without a finalizer.
It's best practice to implement the full dispose pattern for classes that wrap unmanaged resources. This makes your class more robust by making Dispose() idempotent (safe to call multiple times) and ensuring cleanup even if Dispose() is not called explicitly.
Here's an example of how you could implement it:
public class AsusControl : IDisposable
{
private bool _disposed = false;
public AsusControl()
{
AsusWinIO64.InitializeWinIo();
}
~AsusControl()
{
Dispose(false);
}
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
protected virtual void Dispose(bool disposing)
{
if (_disposed) return;
// Unmanaged resources are cleaned up regardless of whether this was called
// from Dispose() or the finalizer.
AsusWinIO64.ShutdownWinIo();
_disposed = true;
}
// ... rest of the class
}| if (!skipResetOnExit) | ||
| { | ||
| var fanSpeeds = asusControl.GetFanSpeeds(); | ||
| Console.WriteLine($"Current fan speeds: {string.Join(" ", fanSpeeds)} RPM"); | ||
| asusControl.ResetToDefault(); | ||
| } |
There was a problem hiding this comment.
The ProcessExit handler is a good addition for cleanup on abnormal termination (e.g., Ctrl-C). However, it currently only calls ResetToDefault() and not Dispose(). This will lead to a resource leak because AsusWinIO64.ShutdownWinIo() won't be called. You should also call Dispose() within this handler.
To safely call Dispose() from both the finally block and the ProcessExit handler, the Dispose() method in AsusControl should be made idempotent (safe to call more than once), as suggested in my other comment.
if (!skipResetOnExit)
{
asusControl.ResetToDefault();
asusControl.Dispose();
}| var newSpeed = int.Parse(newSpeedStr); | ||
| asusControl.SetFanSpeeds(newSpeed); | ||
|
|
||
| if (arg.StartsWith("--get-fan-speed=")) | ||
| { | ||
| var fanIds = arg.Split('=')[1].Split(','); | ||
| foreach (var fanIdStr in fanIds) | ||
| if (newSpeed == 0) | ||
| Console.WriteLine("Test mode turned off"); | ||
| else | ||
| Console.WriteLine($"New fan speeds: {newSpeed}%"); |
There was a problem hiding this comment.
For robustness and consistency with other argument parsing improvements in this PR, you should use int.TryParse() here instead of int.Parse(). Additionally, it's a good idea to validate that the parsed speed is within the expected percentage range (0-100) to prevent unexpected behavior.
if (int.TryParse(newSpeedStr, out int newSpeed) && newSpeed >= 0 && newSpeed <= 100)
{
asusControl.SetFanSpeeds(newSpeed);
if (newSpeed == 0)
Console.WriteLine("Test mode turned off");
else
Console.WriteLine($"New fan speeds: {newSpeed}%");
}
else
{
Console.WriteLine($"Error: Invalid fan speed value. Must be a percentage between 0 and 100. Value: '{newSpeedStr}'");
}| var fanSpeed = int.Parse(settingParts[1]); | ||
| asusControl.SetFanSpeed(fanSpeed, (byte)fanId); | ||
|
|
||
| if (fanSpeed == 0) | ||
| Console.WriteLine($"Test mode turned off for fan {fanId}"); | ||
| else | ||
| Console.WriteLine($"New fan speed for fan {fanId}: {fanSpeed}%"); | ||
| if (fanSpeed == 0) | ||
| Console.WriteLine($"Test mode turned off for fan {fanId}"); | ||
| else | ||
| Console.WriteLine($"New fan speed for fan {fanId}: {fanSpeed}%"); |
There was a problem hiding this comment.
For consistency and robustness, you should use int.TryParse() here instead of int.Parse(). This will prevent the application from crashing if a non-integer value is provided for the fan speed. You should also validate the fan speed is within the expected 0-100 percent range.
if (int.TryParse(settingParts[1], out int fanSpeed) && fanSpeed >= 0 && fanSpeed <= 100)
{
asusControl.SetFanSpeed(fanSpeed, (byte)fanId);
if (fanSpeed == 0)
Console.WriteLine($"Test mode turned off for fan {fanId}");
else
Console.WriteLine($"New fan speed for fan {fanId}: {fanSpeed}%");
}
else
{
Console.WriteLine($"Error: Invalid fan speed value for fan {fanId}. Must be a percentage between 0 and 100. Value: '{settingParts[1]}'");
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@AsusFanControl/AsusControl.cs`:
- Around line 23-30: Change the loop counters from byte to int in
ResetToDefault, SetFanSpeeds(byte value), and GetFanSpeeds to avoid byte
overflow; iterate with "int fanIndex = 0; fanIndex < fanCount; fanIndex++" and
when calling SetFanSpeed or any API that expects a byte, cast the int index to
byte (e.g., (byte)fanIndex) so loop bounds use the int fanCount safely while
preserving the original API calls.
In `@AsusFanControl/Program.cs`:
- Around line 117-122: The finally block currently calls
asusControl.ResetToDefault() then asusControl.Dispose(), which will skip Dispose
if ResetToDefault throws; change it so Dispose always runs by wrapping
ResetToDefault() in its own try/catch or a nested try/finally and call
asusControl.Dispose() in the outer/final finally; preserve setting
skipResetOnExit = true and log or swallow any exception from ResetToDefault so
hardware I/O is always cleaned up by asusControl.Dispose().
- Around line 58-107: The argument parsing silently skips invalid fan IDs and
will throw on non-numeric speeds; update the logic around the "--get-fan-speed="
and "--set-fan-speed=" branches to validate and report errors: for each fanId
string in the "--get-fan-speed=" loop (where GetFanSpeed is called) emit a clear
error when int.TryParse fails and keep the existing 0–255 range check; in the
"--set-fan-speed=" branch (around parsing settingParts and where SetFanSpeed is
called) replace int.Parse with int.TryParse, validate that settingParts has
exactly two elements, report parsing errors for fanId or speed, enforce fanId in
0–255 and speed in 0–100 before calling asusControl.SetFanSpeed, and emit
descriptive Console.WriteLine messages for each validation failure (referencing
GetFanSpeed, HealthyTable_FanCounts, and SetFanSpeed to locate the code).
- Around line 46-56: Validate and guard parsing of the "--set-fan-speeds"
argument: ensure the arg contains '=' before splitting, use int.TryParse to
parse the RHS into newSpeed, and if parsing fails or the value is outside 0–100
write a clear error message and skip calling asusControl.SetFanSpeeds; also
harden SetFanSpeeds (or the percent→PWM conversion path) to clamp the percent
input to 0–100 (e.g., via Math.Clamp or explicit min/max) before converting to
byte PWM so values >100 or <0 cannot wrap into unexpected PWM bytes.
In `@AsusFanControlGUI/Form1.cs`:
- Around line 62-80: The current shutdown block only disposes resources when
Properties.Settings.Default.turnOffControlOnExit is true, so ensure that
AsusWinIO64.ShutdownWinIo() and asusControl.Dispose()/nulling always run; keep
only asusControl.ResetToDefault() inside the conditional. Update the block
around asusControl.ResetToDefault(), asusControl.Dispose() and
AsusWinIO64.ShutdownWinIo() so that ResetToDefault() is invoked only when
turnOffControlOnExit is true, but the null-checked Dispose() (call
asusControl.Dispose() and set asusControl = null) and a call to
AsusWinIO64.ShutdownWinIo() always execute (use try/finally as needed) to
guarantee driver cleanup after InitializeWinIo().
🧹 Nitpick comments (1)
AsusFanControl/AsusControl.cs (1)
10-21: MakeDispose()idempotent to avoid double shutdown.
Dispose()can be called from multiple exit paths; guarding against repeatedShutdownWinIo()calls keeps cleanup safe and predictable.♻️ Proposed fix
public class AsusControl : IDisposable { + private bool _disposed; + public void Dispose() { - AsusWinIO64.ShutdownWinIo(); - GC.SuppressFinalize(this); + if (_disposed) return; + _disposed = true; + AsusWinIO64.ShutdownWinIo(); + GC.SuppressFinalize(this); }
| public void ResetToDefault() | ||
| { | ||
| var fanCount = AsusWinIO64.HealthyTable_FanCounts(); | ||
| for (byte fanIndex = 0; fanIndex < fanCount; fanIndex++) | ||
| { | ||
| SetFanSpeed(0, fanIndex); | ||
| System.Threading.Thread.Sleep(20); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the AsusControl.cs file
find . -name "AsusControl.cs" -type fRepository: beenycool/AsusFanControl
Length of output: 99
🏁 Script executed:
# Search for the HealthyTable_FanCounts method definition
rg "HealthyTable_FanCounts" -A 5 -B 2Repository: beenycool/AsusFanControl
Length of output: 3445
🏁 Script executed:
# Search for the SetFanSpeed method signature
rg "SetFanSpeed" -B 2 -A 10Repository: beenycool/AsusFanControl
Length of output: 6332
🏁 Script executed:
# Check for any other uses of byte loop counters in the codebase
rg "for\s*\(\s*byte\s+" -A 3Repository: beenycool/AsusFanControl
Length of output: 953
Use an int loop counter to prevent byte wrap.
fanCount is int; a byte loop counter will wrap at 255. While unlikely for systems to exceed 255 fans, changing the loop counter to int eliminates the infinite-loop risk and keeps the loop bounds consistent. This pattern appears in at least three methods in this class: ResetToDefault(), SetFanSpeeds(byte value), and GetFanSpeeds().
🛠️ Suggested change
public void ResetToDefault()
{
var fanCount = AsusWinIO64.HealthyTable_FanCounts();
- for (byte fanIndex = 0; fanIndex < fanCount; fanIndex++)
+ for (int fanIndex = 0; fanIndex < fanCount; fanIndex++)
{
- SetFanSpeed(0, fanIndex);
+ SetFanSpeed(0, (byte)fanIndex);
System.Threading.Thread.Sleep(20);
}
}🤖 Prompt for AI Agents
In `@AsusFanControl/AsusControl.cs` around lines 23 - 30, Change the loop counters
from byte to int in ResetToDefault, SetFanSpeeds(byte value), and GetFanSpeeds
to avoid byte overflow; iterate with "int fanIndex = 0; fanIndex < fanCount;
fanIndex++" and when calling SetFanSpeed or any API that expects a byte, cast
the int index to byte (e.g., (byte)fanIndex) so loop bounds use the int fanCount
safely while preserving the original API calls.
| if (arg.StartsWith("--set-fan-speeds")) | ||
| { | ||
| var newSpeedStr = arg.Split('=')[1]; | ||
| var newSpeed = int.Parse(newSpeedStr); | ||
| asusControl.SetFanSpeeds(newSpeed); | ||
|
|
||
| if (arg.StartsWith("--get-fan-speed=")) | ||
| { | ||
| var fanIds = arg.Split('=')[1].Split(','); | ||
| foreach (var fanIdStr in fanIds) | ||
| if (newSpeed == 0) | ||
| Console.WriteLine("Test mode turned off"); | ||
| else | ||
| Console.WriteLine($"New fan speeds: {newSpeed}%"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Program.cs" -type f | head -20Repository: beenycool/AsusFanControl
Length of output: 126
🏁 Script executed:
wc -l ./AsusFanControl/Program.csRepository: beenycool/AsusFanControl
Length of output: 99
🏁 Script executed:
cat -n ./AsusFanControl/Program.csRepository: beenycool/AsusFanControl
Length of output: 6086
🏁 Script executed:
find . -name "*.cs" -type f | xargs grep -l "SetFanSpeeds" | head -10Repository: beenycool/AsusFanControl
Length of output: 156
🏁 Script executed:
grep -n "SetFanSpeeds" ./AsusFanControl/AsusControl.csRepository: beenycool/AsusFanControl
Length of output: 206
🏁 Script executed:
sed -n '46,65p' ./AsusFanControl/AsusControl.csRepository: beenycool/AsusFanControl
Length of output: 722
🏁 Script executed:
# Verify the wrapping behavior with a quick test
cat > /tmp/test_cast.cs << 'EOF'
using System;
class Program {
static void Main() {
// Test wrapping behavior
int[] testPercents = { -1, 0, 50, 100, 101, 200, 256, 300, 1000 };
foreach (var percent in testPercents) {
var value = (byte)(percent / 100.0f * 255);
Console.WriteLine($"percent={percent} -> (int)formula={(int)(percent / 100.0f * 255)} -> byte={value}");
}
}
}
EOF
csc /tmp/test_cast.cs && mono /tmp/test_cast.exe 2>/dev/null || dotnet /tmp/test_cast.cs 2>/dev/null || echo "C# compiler not available, but cast behavior is standard"Repository: beenycool/AsusFanControl
Length of output: 184
Harden --set-fan-speeds parsing and range checks.
The code has two vulnerabilities: arg.Split('=')[1] throws IndexOutOfRangeException if = is missing, and int.Parse() throws FormatException for non-numeric input. Additionally, SetFanSpeeds() converts percent to PWM via (byte)(percent / 100.0f * 255), which silently wraps values >100 (e.g., 101% becomes PWM 1, 300% becomes PWM 44) and negative values become high PWM values. Validate the argument format, use int.TryParse(), and enforce 0–100 range before applying.
🐛 Proposed fix
- if (arg.StartsWith("--set-fan-speeds"))
+ if (arg.StartsWith("--set-fan-speeds"))
{
- var newSpeedStr = arg.Split('=')[1];
- var newSpeed = int.Parse(newSpeedStr);
+ var parts = arg.Split('=', 2);
+ if (parts.Length != 2 || !int.TryParse(parts[1], out var newSpeed) || newSpeed < 0 || newSpeed > 100)
+ {
+ Console.WriteLine("Error: --set-fan-speeds expects a value between 0 and 100");
+ continue;
+ }
asusControl.SetFanSpeeds(newSpeed);
if (newSpeed == 0)
Console.WriteLine("Test mode turned off");
else
Console.WriteLine($"New fan speeds: {newSpeed}%");
}📝 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 (arg.StartsWith("--set-fan-speeds")) | |
| { | |
| var newSpeedStr = arg.Split('=')[1]; | |
| var newSpeed = int.Parse(newSpeedStr); | |
| asusControl.SetFanSpeeds(newSpeed); | |
| if (arg.StartsWith("--get-fan-speed=")) | |
| { | |
| var fanIds = arg.Split('=')[1].Split(','); | |
| foreach (var fanIdStr in fanIds) | |
| if (newSpeed == 0) | |
| Console.WriteLine("Test mode turned off"); | |
| else | |
| Console.WriteLine($"New fan speeds: {newSpeed}%"); | |
| } | |
| if (arg.StartsWith("--set-fan-speeds")) | |
| { | |
| var parts = arg.Split('=', 2); | |
| if (parts.Length != 2 || !int.TryParse(parts[1], out var newSpeed) || newSpeed < 0 || newSpeed > 100) | |
| { | |
| Console.WriteLine("Error: --set-fan-speeds expects a value between 0 and 100"); | |
| continue; | |
| } | |
| asusControl.SetFanSpeeds(newSpeed); | |
| if (newSpeed == 0) | |
| Console.WriteLine("Test mode turned off"); | |
| else | |
| Console.WriteLine($"New fan speeds: {newSpeed}%"); | |
| } |
🤖 Prompt for AI Agents
In `@AsusFanControl/Program.cs` around lines 46 - 56, Validate and guard parsing
of the "--set-fan-speeds" argument: ensure the arg contains '=' before
splitting, use int.TryParse to parse the RHS into newSpeed, and if parsing fails
or the value is outside 0–100 write a clear error message and skip calling
asusControl.SetFanSpeeds; also harden SetFanSpeeds (or the percent→PWM
conversion path) to clamp the percent input to 0–100 (e.g., via Math.Clamp or
explicit min/max) before converting to byte PWM so values >100 or <0 cannot wrap
into unexpected PWM bytes.
| if (arg.StartsWith("--get-fan-speed=")) | ||
| { | ||
| var fanId = int.Parse(fanIdStr); | ||
| var fanSpeed = asusControl.GetFanSpeed((byte)fanId); | ||
| Console.WriteLine($"Current fan speed for fan {fanId}: {fanSpeed} RPM"); | ||
| var fanIds = arg.Split('=')[1].Split(','); | ||
| foreach (var fanIdStr in fanIds) | ||
| { | ||
| if (int.TryParse(fanIdStr, out int fanId)) | ||
| { | ||
| if (fanId >= 0 && fanId <= 255) | ||
| { | ||
| var fanSpeed = asusControl.GetFanSpeed((byte)fanId); | ||
| Console.WriteLine($"Current fan speed for fan {fanId}: {fanSpeed} RPM"); | ||
| } | ||
| else | ||
| { | ||
| Console.WriteLine($"Error: fan id must be between 0 and 255: {fanId}"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (arg.StartsWith("--get-fan-count")) | ||
| { | ||
| var fanCount = asusControl.HealthyTable_FanCounts(); | ||
| Console.WriteLine($"Fan count: {fanCount}"); | ||
| } | ||
| if (arg.StartsWith("--get-fan-count")) | ||
| { | ||
| var fanCount = asusControl.HealthyTable_FanCounts(); | ||
| Console.WriteLine($"Fan count: {fanCount}"); | ||
| } | ||
|
|
||
| if (arg.StartsWith("--set-fan-speed=")) | ||
| { | ||
| var fanSettings = arg.Split('=')[1].Split(','); | ||
| foreach (var fanSetting in fanSettings) | ||
| if (arg.StartsWith("--set-fan-speed=")) | ||
| { | ||
| var fanId = int.Parse(fanSetting.Split(':')[0]); | ||
| var fanSpeed = int.Parse(fanSetting.Split(':')[1]); | ||
| asusControl.SetFanSpeed(fanSpeed, (byte)fanId); | ||
| var fanSettings = arg.Split('=')[1].Split(','); | ||
| foreach (var fanSetting in fanSettings) | ||
| { | ||
| var settingParts = fanSetting.Split(':'); | ||
| if (settingParts.Length == 2 && int.TryParse(settingParts[0], out int fanId)) | ||
| { | ||
| if (fanId >= 0 && fanId <= 255) | ||
| { | ||
| var fanSpeed = int.Parse(settingParts[1]); | ||
| asusControl.SetFanSpeed(fanSpeed, (byte)fanId); | ||
|
|
||
| if (fanSpeed == 0) | ||
| Console.WriteLine($"Test mode turned off for fan {fanId}"); | ||
| else | ||
| Console.WriteLine($"New fan speed for fan {fanId}: {fanSpeed}%"); | ||
| if (fanSpeed == 0) | ||
| Console.WriteLine($"Test mode turned off for fan {fanId}"); | ||
| else | ||
| Console.WriteLine($"New fan speed for fan {fanId}: {fanSpeed}%"); | ||
| } | ||
| else | ||
| { | ||
| Console.WriteLine($"Error: fan id must be between 0 and 255: {fanId}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "AsusFanControl/Program.cs"Repository: beenycool/AsusFanControl
Length of output: 93
🏁 Script executed:
cat -n AsusFanControl/Program.cs | head -150Repository: beenycool/AsusFanControl
Length of output: 6086
🏁 Script executed:
sed -n '50,115p' AsusFanControl/Program.csRepository: beenycool/AsusFanControl
Length of output: 3195
🏁 Script executed:
grep -n "255\|100\|fan.*speed\|fan.*count" AsusFanControl/Program.cs | head -20Repository: beenycool/AsusFanControl
Length of output: 1592
🏁 Script executed:
sed -n '40,120p' AsusFanControl/Program.csRepository: beenycool/AsusFanControl
Length of output: 3809
Add explicit error handling and validation for per-fan IDs and speeds.
The code has three critical issues:
-
Lines 63–74: Invalid fan IDs (non-numeric) are silently skipped without error feedback. When
int.TryParse()fails, the loop just continues. -
Line 94: Fan speed parsing uses
int.Parse()without error handling and will throw an unhandledFormatExceptionif the input is non-numeric. Although wrapped in an outer try-finally, this crashes the program instead of providing a graceful error message. -
Lines 84–107: No range validation for fan speed values (should be 0–100 per the help text at line 20). Invalid speeds could be passed to
SetFanSpeed()and also silently accepted if format is invalid.
Add explicit error handling with informative messages and validate both fan IDs (0–255) and speeds (0–100) before proceeding.
Proposed fix
if (arg.StartsWith("--get-fan-speed="))
{
var fanIds = arg.Split('=')[1].Split(',');
foreach (var fanIdStr in fanIds)
{
- if (int.TryParse(fanIdStr, out int fanId))
- {
- if (fanId >= 0 && fanId <= 255)
- {
- var fanSpeed = asusControl.GetFanSpeed((byte)fanId);
- Console.WriteLine($"Current fan speed for fan {fanId}: {fanSpeed} RPM");
- }
- else
- {
- Console.WriteLine($"Error: fan id must be between 0 and 255: {fanId}");
- }
- }
+ if (!int.TryParse(fanIdStr, out int fanId))
+ {
+ Console.WriteLine($"Error: invalid fan id '{fanIdStr}'");
+ continue;
+ }
+ if (fanId < 0 || fanId > 255)
+ {
+ Console.WriteLine($"Error: fan id must be between 0 and 255: {fanId}");
+ continue;
+ }
+ var fanSpeed = asusControl.GetFanSpeed((byte)fanId);
+ Console.WriteLine($"Current fan speed for fan {fanId}: {fanSpeed} RPM");
}
}
if (arg.StartsWith("--set-fan-speed="))
{
var fanSettings = arg.Split('=')[1].Split(',');
foreach (var fanSetting in fanSettings)
{
var settingParts = fanSetting.Split(':');
- if (settingParts.Length == 2 && int.TryParse(settingParts[0], out int fanId))
+ if (settingParts.Length == 2 &&
+ int.TryParse(settingParts[0], out int fanId) &&
+ int.TryParse(settingParts[1], out int fanSpeed))
{
if (fanId >= 0 && fanId <= 255)
{
- var fanSpeed = int.Parse(settingParts[1]);
- asusControl.SetFanSpeed(fanSpeed, (byte)fanId);
+ if (fanSpeed < 0 || fanSpeed > 100)
+ {
+ Console.WriteLine($"Error: fan speed must be between 0 and 100: {fanSpeed}");
+ continue;
+ }
+ asusControl.SetFanSpeed(fanSpeed, (byte)fanId);
if (fanSpeed == 0)
Console.WriteLine($"Test mode turned off for fan {fanId}");
else
Console.WriteLine($"New fan speed for fan {fanId}: {fanSpeed}%");
}
else
{
Console.WriteLine($"Error: fan id must be between 0 and 255: {fanId}");
}
}
+ else
+ {
+ Console.WriteLine($"Error: invalid fan setting '{fanSetting}' (expected fanId:percent)");
+ }
}
}🤖 Prompt for AI Agents
In `@AsusFanControl/Program.cs` around lines 58 - 107, The argument parsing
silently skips invalid fan IDs and will throw on non-numeric speeds; update the
logic around the "--get-fan-speed=" and "--set-fan-speed=" branches to validate
and report errors: for each fanId string in the "--get-fan-speed=" loop (where
GetFanSpeed is called) emit a clear error when int.TryParse fails and keep the
existing 0–255 range check; in the "--set-fan-speed=" branch (around parsing
settingParts and where SetFanSpeed is called) replace int.Parse with
int.TryParse, validate that settingParts has exactly two elements, report
parsing errors for fanId or speed, enforce fanId in 0–255 and speed in 0–100
before calling asusControl.SetFanSpeed, and emit descriptive Console.WriteLine
messages for each validation failure (referencing GetFanSpeed,
HealthyTable_FanCounts, and SetFanSpeed to locate the code).
| finally | ||
| { | ||
| skipResetOnExit = true; | ||
| asusControl.ResetToDefault(); | ||
| asusControl.Dispose(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n AsusFanControl/Program.cs | sed -n '100,140p'Repository: beenycool/AsusFanControl
Length of output: 1200
🏁 Script executed:
cat -n AsusFanControl/Program.cs | sed -n '1,130p'Repository: beenycool/AsusFanControl
Length of output: 6086
🏁 Script executed:
find . -name "AsusControl.cs" -type fRepository: beenycool/AsusFanControl
Length of output: 99
🏁 Script executed:
cat -n AsusFanControl/AsusControl.csRepository: beenycool/AsusFanControl
Length of output: 3302
Ensure Dispose() runs even if ResetToDefault() fails.
If ResetToDefault() throws, Dispose() is skipped, leaving hardware I/O resources unclean. A nested try/finally guarantees cleanup always executes.
🛠️ Suggested change
finally
{
skipResetOnExit = true;
- asusControl.ResetToDefault();
- asusControl.Dispose();
+ try
+ {
+ asusControl.ResetToDefault();
+ }
+ finally
+ {
+ asusControl.Dispose();
+ }
}📝 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.
| finally | |
| { | |
| skipResetOnExit = true; | |
| asusControl.ResetToDefault(); | |
| asusControl.Dispose(); | |
| } | |
| finally | |
| { | |
| skipResetOnExit = true; | |
| try | |
| { | |
| asusControl.ResetToDefault(); | |
| } | |
| finally | |
| { | |
| asusControl.Dispose(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@AsusFanControl/Program.cs` around lines 117 - 122, The finally block
currently calls asusControl.ResetToDefault() then asusControl.Dispose(), which
will skip Dispose if ResetToDefault throws; change it so Dispose always runs by
wrapping ResetToDefault() in its own try/catch or a nested try/finally and call
asusControl.Dispose() in the outer/final finally; preserve setting
skipResetOnExit = true and log or swallow any exception from ResetToDefault so
hardware I/O is always cleaned up by asusControl.Dispose().
| if (Properties.Settings.Default.turnOffControlOnExit) | ||
| asusControl.SetFanSpeeds(0); | ||
| { | ||
| try | ||
| { | ||
| asusControl.ResetToDefault(); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore exceptions during shutdown | ||
| } | ||
| finally | ||
| { | ||
| if (asusControl != null) | ||
| { | ||
| asusControl.Dispose(); | ||
| asusControl = null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd AsusFanControlGUI && head -150 Form1.cs | tail -100Repository: beenycool/AsusFanControl
Length of output: 3120
🏁 Script executed:
find . -name "Form1.cs" -type fRepository: beenycool/AsusFanControl
Length of output: 96
🏁 Script executed:
sed -n '55,85p' AsusFanControlGUI/Form1.csRepository: beenycool/AsusFanControl
Length of output: 939
🏁 Script executed:
rg "OnProcessExit" -A 30 AsusFanControlGUI/Form1.csRepository: beenycool/AsusFanControl
Length of output: 2455
🏁 Script executed:
rg "asusControl" -B 2 -A 2 AsusFanControlGUI/Form1.cs | head -100Repository: beenycool/AsusFanControl
Length of output: 1419
🏁 Script executed:
find . -name "AsusControl.cs" -type fRepository: beenycool/AsusFanControl
Length of output: 99
🏁 Script executed:
rg "class AsusControl" -A 50 AsusFanControlGUI/AsusControl.cs | head -100Repository: beenycool/AsusFanControl
Length of output: 140
🏁 Script executed:
rg "Dispose" -B 2 -A 10 AsusFanControlGUI/AsusControl.csRepository: beenycool/AsusFanControl
Length of output: 140
🏁 Script executed:
rg "override.*Dispose|Dispose\(" AsusFanControlGUI/Form1.csRepository: beenycool/AsusFanControl
Length of output: 114
🏁 Script executed:
cat AsusFanControl/AsusControl.csRepository: beenycool/AsusFanControl
Length of output: 2651
🏁 Script executed:
rg "Dispose|IDisposable|WinIo" AsusFanControl/AsusControl.cs -B 2 -A 8Repository: beenycool/AsusFanControl
Length of output: 562
Ensure Dispose() runs even when "turn off on exit" is disabled.
Disposal is inside the turnOffControlOnExit conditional, so AsusWinIO64.ShutdownWinIo() is skipped when the user disables that setting. Since InitializeWinIo() is called in the constructor, cleanup must always run to properly release driver resources. Only the ResetToDefault() call should be conditional.
🐛 Proposed fix
private void OnProcessExit(object sender, EventArgs e)
{
- if (Properties.Settings.Default.turnOffControlOnExit)
- {
- try
- {
- asusControl.ResetToDefault();
- }
- catch
- {
- // Ignore exceptions during shutdown
- }
- finally
- {
- if (asusControl != null)
- {
- asusControl.Dispose();
- asusControl = null;
- }
- }
- }
+ try
+ {
+ if (Properties.Settings.Default.turnOffControlOnExit)
+ {
+ asusControl?.ResetToDefault();
+ }
+ }
+ catch
+ {
+ // Ignore exceptions during shutdown
+ }
+ finally
+ {
+ asusControl?.Dispose();
+ asusControl = null;
+ }
}📝 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 (Properties.Settings.Default.turnOffControlOnExit) | |
| asusControl.SetFanSpeeds(0); | |
| { | |
| try | |
| { | |
| asusControl.ResetToDefault(); | |
| } | |
| catch | |
| { | |
| // Ignore exceptions during shutdown | |
| } | |
| finally | |
| { | |
| if (asusControl != null) | |
| { | |
| asusControl.Dispose(); | |
| asusControl = null; | |
| } | |
| } | |
| } | |
| private void OnProcessExit(object sender, EventArgs e) | |
| { | |
| try | |
| { | |
| if (Properties.Settings.Default.turnOffControlOnExit) | |
| { | |
| asusControl?.ResetToDefault(); | |
| } | |
| } | |
| catch | |
| { | |
| // Ignore exceptions during shutdown | |
| } | |
| finally | |
| { | |
| asusControl?.Dispose(); | |
| asusControl = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@AsusFanControlGUI/Form1.cs` around lines 62 - 80, The current shutdown block
only disposes resources when Properties.Settings.Default.turnOffControlOnExit is
true, so ensure that AsusWinIO64.ShutdownWinIo() and
asusControl.Dispose()/nulling always run; keep only asusControl.ResetToDefault()
inside the conditional. Update the block around asusControl.ResetToDefault(),
asusControl.Dispose() and AsusWinIO64.ShutdownWinIo() so that ResetToDefault()
is invoked only when turnOffControlOnExit is true, but the null-checked
Dispose() (call asusControl.Dispose() and set asusControl = null) and a call to
AsusWinIO64.ShutdownWinIo() always execute (use try/finally as needed) to
guarantee driver cleanup after InitializeWinIo().
Co-authored-by: beenycool <129210955+beenycool@users.noreply.github.com>
1 participant
IDisposableandResetToDefaultinAsusControlto ensure proper resource cleanup.Program.csto useint.TryParseand validate fan ID range (0-255).ProcessExithandler andtry-finallyblock inProgram.csto ensure fan reset on exit.Form1.csto wrapResetToDefaultintry-catchand ensureDisposeis called infinallyblock during process exit.PR created automatically by Jules for task 2926545181562135474 started by @beenycool
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.