Conversation
WalkthroughThe pull request introduces a modular core library architecture by extracting fan control logic into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
🧪 Generate unit tests (beta)
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 |
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 refactors the application's fan control mechanisms by introducing a dedicated core library and an interface for fan control. It also enhances the robustness of the application by implementing proper resource disposal patterns and comprehensive error handling during shutdown and unexpected crashes. Furthermore, it improves the user experience for the command-line interface with better argument parsing and clearer error messages, and adds null checks in the GUI to prevent runtime issues. 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
|
![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 significantly improves the codebase by refactoring shared logic into a Core library, correctly implementing the IDisposable pattern for resource management, and enhancing the robustness of both the console and GUI applications. The improvements to argument parsing and error handling in the command-line tool are particularly noteworthy. I've identified a few areas for improvement: a newly added menu item in the GUI is not actually displayed, some exceptions are silently swallowed where logging would be beneficial, and there are minor coding style inconsistencies.
| this.toolStripMenuItemForbidUnsafeSettings = new System.Windows.Forms.ToolStripMenuItem(); | ||
| this.toolStripMenuItemMinimizeToTrayOnClose = new System.Windows.Forms.ToolStripMenuItem(); | ||
| this.toolStripMenuItemAutoRefreshStats = new System.Windows.Forms.ToolStripMenuItem(); | ||
| this.toolStripMenuItemStartLogging = new System.Windows.Forms.ToolStripMenuItem(); |
There was a problem hiding this comment.
The toolStripMenuItemStartLogging is instantiated here, but it is not added to any menu's DropDownItems collection in the designer code. As a result, it will not be visible in the UI. Based on the context, it seems it should be added to the toolStripMenuItem1 (the "Advanced" menu).
| { | ||
| if (_disposed) return; | ||
| var fanCount = AsusWinIO64.HealthyTable_FanCounts(); | ||
| for(byte fanIndex = 0; fanIndex < fanCount; fanIndex++) |
There was a problem hiding this comment.
For consistency with C# coding conventions and the rest of the codebase, a space should be added after the for keyword. This improves readability.
for (byte fanIndex = 0; fanIndex < fanCount; fanIndex++)This minor style issue also appears on line 125.
for (byte fanIndex = 0; fanIndex < fanCount; fanIndex++)| // and if we haven't set the flag to skip reset. | ||
| if (!isDisposed && !skipResetOnExit) | ||
| { | ||
| try { asusControl.ResetToDefault(); } catch {} |
There was a problem hiding this comment.
The empty catch {} block silently swallows any exception from ResetToDefault(). While this prevents the shutdown process from crashing, it also hides potential issues that might be important for diagnostics. Since this is a console application and a goal of this PR is to improve logging, it would be better to log the exception to Console.Error. This recommendation also applies to the other empty catch blocks in the exception handlers.
try { asusControl.ResetToDefault(); } catch (Exception ex) { Console.Error.WriteLine($"[ProcessExit] Failed to reset fans: {ex.Message}"); }
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AsusFanControl.Core/AsusControl.cs (1)
65-74:⚠️ Potential issue | 🟡 MinorPotential race condition: disposed state may change during async iteration.
The
_disposedcheck at line 67 occurs only at method entry. IfDispose()is called while the loop is iterating, subsequentSetFanSpeedcalls will silently return (due to their own guards), butAsusWinIO64.HealthyTable_FanCounts()at line 68 could access disposed resources.🛡️ Suggested improvement: capture fan count before the disposed check
private async Task SetFanSpeeds(byte value) { - if (_disposed) return; var fanCount = AsusWinIO64.HealthyTable_FanCounts(); + if (_disposed) return; for(byte fanIndex = 0; fanIndex < fanCount; fanIndex++) { + if (_disposed) return; SetFanSpeed(value, fanIndex); await Task.Delay(20); } }AsusFanControlGUI/Form1.Designer.cs (1)
14-21:⚠️ Potential issue | 🟠 MajorDuplicate Dispose(bool) definition exists in Form1.Designer.cs and Form1.cs.
This designer-generated
Dispose(bool disposing)at lines 14-21 will conflict with the override in Form1.cs (lines 48-83). This typically causes a compilation error due to duplicate member definition.🔧 Recommended fix: Remove the designer-generated Dispose
The designer file should not contain the
Disposemethod when a custom implementation exists in the partial class. Either:
- Remove lines 14-21 from the designer file, or
- Move the custom disposal logic from Form1.cs into the designer's Dispose method (not recommended as it mixes concerns)
- protected override void Dispose(bool disposing) - { - if (disposing && (components != null)) - { - components.Dispose(); - } - base.Dispose(disposing); - }
🤖 Fix all issues with AI agents
In `@AsusFanControl/Program.cs`:
- Around line 65-88: The code sets skipResetOnExit before validating or applying
the requested speed, which can prevent the safety reset on invalid input; move
the assignment of skipResetOnExit so it only runs after a successful change is
applied (i.e., after asusControl.SetFanSpeeds(newSpeed) returns/executes and
int.TryParse(parts[1], out int newSpeed) succeeded). Update the branch handling
"--set-fan-speeds" (variables arg, parts, newSpeed and the call
asusControl.SetFanSpeeds) to set skipResetOnExit = true only inside the
successful-parsing/after-SetFanSpeeds path (and not on parse failure or format
error), and apply the same fix to the analogous block referenced at lines
128-165.
🧹 Nitpick comments (2)
AsusFanControl.Core/AsusControl.cs (2)
76-82: Fire-and-forget async task may silently swallow exceptions.
SetFanSpeeds(int percent)discards theTaskreturned by the async method. Any exceptions inSetFanSpeeds(byte)will be silently lost, and there's no guarantee the operation completes before subsequent calls or disposal.Consider either:
- Making the public method
async Taskand awaiting properly- Using
.ConfigureAwait(false).GetAwaiter().GetResult()if synchronous behavior is intended- Adding exception handling within the async method
♻️ Option 1: Make public method async
-public void SetFanSpeeds(int percent) +public async Task SetFanSpeedsAsync(int percent) { if (_disposed) return; percent = ClampPercentage(percent); var value = (byte)(percent / 100.0f * 255); - _ = SetFanSpeeds(value); + await SetFanSpeeds(value); }Note: This would require updating
IFanControllerinterface and all callers.
32-40: Dispose pattern does not handle managed resources distinctly.The
Dispose(bool disposing)method doesn't differentiate between thedisposingparameter cases. Thedisposingflag indicates whether managed resources should be cleaned up (true when called fromDispose(), false from finalizer). Currently,ShutdownWinIo()is called in both cases, which is correct for unmanaged cleanup, but the pattern could be clearer.♻️ Clearer dispose pattern structure
protected virtual void Dispose(bool disposing) { if (!_disposed) { + if (disposing) + { + // Dispose managed resources here if any are added later + } + // Unmanaged resources AsusWinIO64.ShutdownWinIo(); _disposed = true; } }
| if (arg.StartsWith("--set-fan-speeds")) | ||
| { | ||
| var newSpeedStr = arg.Split('=')[1]; | ||
| var newSpeed = int.Parse(newSpeedStr); | ||
| asusControl.SetFanSpeeds(newSpeed); | ||
| skipResetOnExit = true; // User wants this speed to stick | ||
|
|
||
| var parts = arg.Split('='); | ||
| if (parts.Length < 2 || string.IsNullOrWhiteSpace(parts[1])) | ||
| { | ||
| Console.WriteLine("Error: Invalid format for --set-fan-speeds. Usage: --set-fan-speeds=50"); | ||
| continue; | ||
| } | ||
|
|
||
| if (int.TryParse(parts[1], out int newSpeed)) | ||
| { | ||
| asusControl.SetFanSpeeds(newSpeed); | ||
|
|
||
| if (newSpeed == 0) | ||
| Console.WriteLine("Test mode turned off"); | ||
| if(newSpeed == 0) | ||
| Console.WriteLine("Test mode turned off"); | ||
| else | ||
| Console.WriteLine($"New fan speeds: {newSpeed}%"); | ||
| } | ||
| else | ||
| Console.WriteLine($"New fan speeds: {newSpeed}%"); | ||
| { | ||
| Console.WriteLine($"Error: Invalid number format for speed: {parts[1]}"); | ||
| } |
There was a problem hiding this comment.
Don’t skip reset on invalid set-fan arguments.
skipResetOnExit is set before validating input, so invalid args can suppress the safety reset even though no speed was applied. Move the flag to only set after at least one successful change.
Suggested fix
- if (arg.StartsWith("--set-fan-speeds"))
- {
- skipResetOnExit = true; // User wants this speed to stick
+ if (arg.StartsWith("--set-fan-speeds"))
+ {
var parts = arg.Split('=');
if (parts.Length < 2 || string.IsNullOrWhiteSpace(parts[1]))
{
Console.WriteLine("Error: Invalid format for --set-fan-speeds. Usage: --set-fan-speeds=50");
continue;
}
if (int.TryParse(parts[1], out int newSpeed))
{
asusControl.SetFanSpeeds(newSpeed);
+ skipResetOnExit = true; // only after a successful set
if(newSpeed == 0)
Console.WriteLine("Test mode turned off");
else
Console.WriteLine($"New fan speeds: {newSpeed}%");
}
else
{
Console.WriteLine($"Error: Invalid number format for speed: {parts[1]}");
}
}
@@
- if (arg.StartsWith("--set-fan-speed="))
- {
- skipResetOnExit = true; // User wants this speed to stick
+ if (arg.StartsWith("--set-fan-speed="))
+ {
var parts = arg.Split('=');
if (parts.Length < 2 || string.IsNullOrWhiteSpace(parts[1]))
{
Console.WriteLine("Error: Invalid format. Usage: --set-fan-speed=0:50,1:100");
continue;
}
+ var anyApplied = false;
var fanSettings = parts[1].Split(',');
foreach (var fanSetting in fanSettings)
{
var settingParts = fanSetting.Split(':');
if (settingParts.Length == 2 &&
int.TryParse(settingParts[0], out int fanId) &&
int.TryParse(settingParts[1], out int fanSpeed))
{
if (fanId >= 0 && fanId <= 255)
{
asusControl.SetFanSpeed(fanSpeed, (byte)fanId);
+ anyApplied = true;
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 format: {fanSetting}");
}
}
+
+ if (anyApplied)
+ skipResetOnExit = true; // only if at least one valid setting applied
}Also applies to: 128-165
🤖 Prompt for AI Agents
In `@AsusFanControl/Program.cs` around lines 65 - 88, The code sets
skipResetOnExit before validating or applying the requested speed, which can
prevent the safety reset on invalid input; move the assignment of
skipResetOnExit so it only runs after a successful change is applied (i.e.,
after asusControl.SetFanSpeeds(newSpeed) returns/executes and
int.TryParse(parts[1], out int newSpeed) succeeded). Update the branch handling
"--set-fan-speeds" (variables arg, parts, newSpeed and the call
asusControl.SetFanSpeeds) to set skipResetOnExit = true only inside the
successful-parsing/after-SetFanSpeeds path (and not on parse failure or format
error), and apply the same fix to the analogous block referenced at lines
128-165.
1 participant
Apply automated fixes requested in PR #8: dispose cpuCounter on shutdown; improve logging error handling and user messages; pluralize CSV header; simplify logging cleanup; marshal UI updates when stopping logging; instantiate Start Logging menu item. Changes committed on branch fix/pr8.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.