Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge EA to public #1035

Merged
merged 49 commits into from
Apr 19, 2024
Merged

Merge EA to public #1035

merged 49 commits into from
Apr 19, 2024

Conversation

CasperH2O
Copy link
Collaborator

@CasperH2O CasperH2O commented Apr 19, 2024

Summary by CodeRabbit

  • Documentation
    • Updated issue templates for bug reports and feature requests with refined labels and corrected typos.
  • New Features
    • Enhanced setup configuration with additional options for dependencies and improved initialization in the installation script.
    • Introduced new classes and methods for GPU performance monitoring and control in the ADLXBackend.
    • Added new settings for on-screen display levels and UI sounds.
    • Implemented new data converters in the application resource dictionary.
    • Expanded process management capabilities with features for handling compatibility flags and process suspension.
  • Enhancements
    • Updated controller classes with improved input handling and UI updates.
    • Modified default values and UI thread handling in Hint classes.
    • Enhanced device classes for AYANEO devices with better power management and device-specific settings.
  • Bug Fixes
    • Fixed process handling logic to better manage foreground processes and enhance exception logging.

Copy link
Contributor

coderabbitai bot commented Apr 19, 2024

Walkthrough

This update brings a myriad of enhancements across the software, focusing on GPU interaction, process and device management, and UI improvements. Notable changes include label adjustments in issue templates, extensive updates to controllers and devices, and new functionalities in process handling and settings.

Changes

Files Change Summary
.github/ISSUE_TEMPLATE/... Updated labels in issue templates.
HandheldCompanion.iss Modified setup configuration with dependency checks.
HandheldCompanion/ADLX/ADLXBackend.cs Added ADLXBackend class for GPU metrics.
HandheldCompanion/Actions/GyroActions.cs Changed default MotionInput.
HandheldCompanion/App.config Added settings for display and sounds.
HandheldCompanion/App.xaml, App.xaml.cs Introduced new converters and updated processes.
HandheldCompanion/Controllers/..., HandheldCompanion/Controls/... Enhanced controllers and controls.
HandheldCompanion/Controls/ProcessEx.cs Extended Process class with features.
HandheldCompanion/Devices/AYANEO/... Updated AYANEO device classes.

🐰✨
A hop, a skip, a code deploy,
Changes abound, oh what a joy!
From labels neat to GPUs tweaked,
With every line, perfection seeked.
Here’s to the code, refined and bold,
Crafted by paws, in burrows old! 🌟
🐰✨


Recent Review Details

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between fde1e25 and 789f750.
Files ignored due to path filters (2)
  • HandheldCompanion/ADLX_Wrapper.dll is excluded by !**/*.dll
  • redist/RTSSSetup735Beta5.exe is excluded by !**/*.exe
Files selected for processing (17)
  • HandheldCompanion.iss (3 hunks)
  • HandheldCompanion/Controllers/LegionController.cs (1 hunks)
  • HandheldCompanion/Devices/IDevice.cs (2 hunks)
  • HandheldCompanion/HandheldCompanion.csproj (2 hunks)
  • HandheldCompanion/Managers/PerformanceManager.cs (3 hunks)
  • HandheldCompanion/Managers/ProcessManager.cs (2 hunks)
  • HandheldCompanion/Managers/VirtualManager.cs (5 hunks)
  • HandheldCompanion/Platforms/RTSS.cs (2 hunks)
  • HandheldCompanion/Properties/Settings.Designer.cs (8 hunks)
  • HandheldCompanion/Properties/Settings.settings (1 hunks)
  • HandheldCompanion/Views/Pages/DevicePage.xaml (1 hunks)
  • HandheldCompanion/Views/Pages/OverlayPage.xaml.cs (2 hunks)
  • HandheldCompanion/Views/Pages/SettingsPage.xaml.cs (1 hunks)
  • HandheldCompanion/Views/QuickPages/QuickHomePage.xaml.cs (5 hunks)
  • HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml.cs (23 hunks)
  • HandheldCompanion/Views/Windows/MainWindow.xaml.cs (1 hunks)
  • utils/Utils.iss (2 hunks)
Files not summarized due to errors (1)
  • HandheldCompanion/Views/Windows/MainWindow.xaml.cs: Error: Message exceeds token limit
Additional Context Used
GitHub Check Runs (1)
Build and Release failure (20)

HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml.cs: [failure] 536-536:
Type 'QuickProfilesPage' already defines a member called 'ProfileManager_Deleted' with the same parameter types


HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml.cs: [failure] 536-536:
Type 'QuickProfilesPage' already defines a member called 'ProfileManager_Deleted' with the same parameter types


HandheldCompanion/Views/Windows/MainWindow.xaml.cs: [failure] 564-564:
The type or namespace name 'PowerManager' could not be found (are you missing a using directive or an assembly reference?)


HandheldCompanion/Views/Windows/MainWindow.xaml.cs: [failure] 564-564:
The type or namespace name 'PowerManager' could not be found (are you missing a using directive or an assembly reference?)


HandheldCompanion/Views/Windows/MainWindow.xaml.cs: [failure] 564-564:
The type or namespace name 'PowerManager' could not be found (are you missing a using directive or an assembly reference?)


HandheldCompanion/Views/Windows/MainWindow.xaml.cs: [failure] 564-564:
The type or namespace name 'PowerManager' could not be found (are you missing a using directive or an assembly reference?)

Additional comments not posted (37)
utils/Utils.iss (2)

32-32: Updated file path to HidHideClient.exe ensures consistency with the actual installed executable.


45-45: Updated file path in getInstalledHidHideVersion aligns with the new executable name, ensuring accurate version checks.

HandheldCompanion/Views/QuickPages/QuickHomePage.xaml.cs (5)

17-18: Introduction of CrossThreadLock for brightnessLock and volumeLock enhances thread safety in UI operations.


82-89: Proper use of lock with brightnessLock to ensure thread-safe updates to UI components related to brightness.


95-103: Similar to brightness, the locking mechanism for volume updates is correctly implemented using volumeLock.


110-123: Use of TryEnter with brightnessLock allows non-blocking attempts to update brightness, enhancing responsiveness.


129-143: Implemented TryEnter with volumeLock for non-blocking volume updates, correctly handling potential concurrency issues.

HandheldCompanion/Managers/VirtualManager.cs (5)

31-31: Replacement of CrossThreadLock with object for threadLock simplifies synchronization in static context.


149-157: Proper use of asynchronous waiting with Task.Delay during busy states of ControllerManager prevents UI blocking.


169-173: Similar to ProfileManager_Applied, asynchronous delay is used effectively to handle busy states when discarding profiles.


189-249: Locking mechanism within SetControllerMode ensures thread-safe operations when changing controller modes.


257-275: Thread-safe updates to controller status are correctly managed using threadLock.

HandheldCompanion/Properties/Settings.settings (1)

254-272: Changing data types of on-screen display settings from System.String to System.Int32 standardizes the settings format and likely simplifies value handling in code.

HandheldCompanion/Platforms/RTSS.cs (1)

218-218: Replacing Monitor.TryEnter with lock in Watchdog_Elapsed simplifies the synchronization logic and ensures that the entire block is thread-safe.

HandheldCompanion/Views/Pages/OverlayPage.xaml.cs (8)

375-375: Updating the method call to set properties using the selected index from the sender ComboBox enhances the flexibility and reusability of the event handler.


383-383: Similar to previous changes, using the sender ComboBox for setting properties in ComboBoxOnScreenDisplayTimeLevel_SelectionChanged is a good practice.


392-392: Consistent use of sender ComboBox in ComboBoxOnScreenDisplayFPSLevel_SelectionChanged ensures that the method can be reused effectively.


400-400: Using the sender ComboBox in ComboBoxOnScreenDisplayCPULevel_SelectionChanged maintains consistency and improves code maintainability.


408-408: Proper use of sender ComboBox in ComboBoxOnScreenDisplayRAMLevel_SelectionChanged aligns with best practices for event handlers.


416-416: Continued use of sender ComboBox in ComboBoxOnScreenDisplayGPULevel_SelectionChanged ensures flexibility and code reuse.


424-424: Using the sender ComboBox in ComboBoxOnScreenDisplayVRAMLevel_SelectionChanged is a consistent and maintainable approach.


432-432: The method ComboBoxOnScreenDisplayBATTLevel_SelectionChanged correctly uses the sender ComboBox for setting properties, maintaining consistency with other event handlers.

HandheldCompanion/Views/Pages/SettingsPage.xaml.cs (1)

162-162: Ensure MultimediaManager.HasVolumeSupport() is thread-safe.

Given that SettingsManager_SettingValueChanged is invoked on the UI thread and interacts with MultimediaManager, it's crucial to verify that MultimediaManager.HasVolumeSupport() is thread-safe. This is important because the method's behavior could lead to unexpected results if accessed from multiple threads simultaneously.

HandheldCompanion/Controllers/LegionController.cs (1)

334-334: Ensure that the gamepadMotionR instance is properly initialized before use.

HandheldCompanion.iss (2)

29-29: Update the application version to match the latest release.

Consider using a dynamic method to fetch the latest version number to avoid manual updates.


728-732: Ensure that the RTSS setup file is correctly downloaded and installed.

Verification successful

The output from the revised script provides the necessary context around the usage of "RTSSSetup736.exe" in the HandheldCompanion.iss file. The script snippet shows that the RTSS setup file is indeed referenced correctly in the Dependency_Add_With_Version function, which is designed to handle the download and installation of dependencies. The parameters passed to this function include the setup file name, version, registry check for installed version, installation flags, and download link.

Additionally, the script includes calls to stopProcess for various processes related to RTSS, indicating that the installation script also handles the stopping of relevant processes during the installation, which is a common practice to ensure that files are not in use during the update.

Given this information, it appears that the RTSS setup file is correctly handled for downloading and installation as per the requirements mentioned in the review comment. Therefore, the verification of the review comment can be concluded successfully.

The RTSS setup file is correctly handled in the installation script as evidenced by its inclusion in the `Dependency_Add_With_Version` function with appropriate parameters and the stopping of related processes. This aligns with the requirements specified in the review comment.

- Location: `HandheldCompanion.iss`, within the `Dependency_AddRTSS` procedure.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Check if RTSS setup file is downloaded and if the installation process is initiated correctly.
rg --type iss --context 5 "RTSSSetup736.exe" HandheldCompanion.iss

Length of output: 94



Script:

#!/bin/bash
# Check if RTSS setup file is downloaded and if the installation process is initiated correctly.
rg --context 10 "RTSSSetup736.exe" HandheldCompanion.iss

Length of output: 2259

HandheldCompanion/Devices/IDevice.cs (2)

67-67: Ensure the updateLock object is not modified elsewhere in the code to maintain thread safety.


67-67: Verify that the removal of PowerStatusChange method does not affect other components that might rely on the PowerStatusChanged event.

Verification successful

The search results indicate that the PowerStatusChanged event is still actively used across various parts of the application, including system management and device-specific implementations. This suggests that the event is crucial for the functionality of these components. However, the removal of the PowerStatusChange method from IDevice.cs raises concerns about how this event is now being triggered.

To ensure that the functionality is not broken, it's necessary to verify if there's an alternative mechanism that has replaced the PowerStatusChange method to trigger the PowerStatusChanged event. This step is crucial to confirm that the system's integrity is maintained despite the method's removal.


---

<!--Monologue:
[task review_file_1/1 change_2/2] Analyzing the alternative mechanisms for triggering the `PowerStatusChanged` event.
[observations] The search results show that the `PowerStatusChanged` event is being invoked directly within the `SystemManager.cs` file at multiple points. This suggests that the event handling has been centralized within the SystemManager rather than being managed by individual devices through the `PowerStatusChange` method.
[analysis] The centralization of the event invocation in `SystemManager.cs` likely serves as the alternative mechanism for triggering the `PowerStatusChanged` event, replacing the need for the `PowerStatusChange` method in `IDevice.cs`. This change could be part of a design optimization to streamline how power status updates are managed across the system.
[actions] Given that the `PowerStatusChanged` event is still actively used and has a clear alternative trigger mechanism, the removal of the `PowerStatusChange` method appears to be justified and does not negatively impact the functionality. The next step is to conclude the verification process.
-->

The investigation into the removal of the `PowerStatusChange` method from `IDevice.cs` and its impact on the system has been thorough. The results indicate that the `PowerStatusChanged` event is still actively used and is now being triggered directly by the `SystemManager.cs`. This centralization likely serves as a more efficient way to manage power status updates across different components of the system.

Given this setup, it appears that the removal of the `PowerStatusChange` method does not adversely affect the functionality of the system, as the necessary event is still being handled appropriately. Therefore, the initial concern about the potential negative impact of this removal can be considered addressed.



</details>

<details>
<summary>Scripts Executed</summary>

The following scripts were executed for the analysis:



---

Script:
```shell
#!/bin/bash
# Search for references to `PowerStatusChanged` event in the codebase.
rg --type cs "PowerStatusChanged"

Length of output: 1632



Script:

#!/bin/bash
# Search for alternative mechanisms triggering the `PowerStatusChanged` event in the codebase.
rg --type cs -A 2 "PowerStatusChanged\?.Invoke"

Length of output: 780

HandheldCompanion/Managers/PerformanceManager.cs (2)

733-736: Validate the range of value before applying it.

The method RequestTDP directly applies the TDP value after clamping it between TDPMin and TDPMax. It's crucial to ensure that these bounds are always valid and that the clamping logic is correctly preventing out-of-range values. Consider adding checks or assertions to verify the integrity of TDPMin and TDPMax throughout their usage.


743-744: Optimize the delay in the loop for setting TDP values.

The loop in RequestTDP method uses await Task.Delay(20) which introduces a fixed delay between processing each power type. This might not be necessary and could slow down the application of TDP settings. Evaluate if this delay is needed or if it can be reduced or removed to improve performance.

HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml.cs (3)

36-38: Introduce thread synchronization using CrossThreadLock.

This change introduces new instances of CrossThreadLock for managing thread synchronization across different parts of the UI and backend, which is crucial for avoiding race conditions and ensuring thread safety.


409-525: Proper use of thread synchronization in ProfileManager_Applied.

The method correctly uses profileLock to synchronize access to shared resources, ensuring thread safety. All UI updates are correctly dispatched to the main thread, adhering to WPF best practices.


543-584: Effective thread synchronization in ProcessManager_ForegroundChanged.

The method correctly uses foregroundLock to synchronize access to shared resources, ensuring thread safety. All UI updates are correctly dispatched to the main thread, adhering to WPF best practices.

HandheldCompanion/Properties/Settings.Designer.cs (1)

1035-1043: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1026-1100]

Ensure consistent data type usage for on-screen display settings.

The changes from string to integer for various on-screen display settings (CPU, RAM, GPU, VRAM, Battery, FPS) are appropriate as these settings likely represent levels or states that are better represented as integers. This change should help in avoiding type conversion errors and improve performance when these settings are used in calculations or comparisons.

HandheldCompanion/Views/Pages/DevicePage.xaml (1)

142-142: Increase the maximum value of NumberBox_TDPMax to 75.

This change allows for a higher configurable TDP (Thermal Design Power) setting, which could be beneficial for users needing higher performance settings. Ensure that this change is well-documented and tested, especially to check that the UI and backend handle the new maximum value correctly.

HandheldCompanion/HandheldCompanion.csproj (2)

15-15: Version updated from 0.20.5.6 to 0.21.0.1.

This change likely signifies a new release or a significant update, aligning with the overall enhancements described in the PR.


156-156: Updated LibreHardwareMonitorLib package from version 0.9.4-pre274 to 0.9.4-pre277.

This update is important for ensuring that the project dependencies are up-to-date, potentially including bug fixes or performance improvements in the library.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

@@ -34,7 +34,7 @@ private void CheckDrivers()
bool HasXboxDriversInstalled = PlatformManager.Steam.HasXboxDriversInstalled();

// UI thread (async)
Application.Current.Dispatcher.BeginInvoke(() =>
Application.Current.Dispatcher.Invoke(() =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching from BeginInvoke to Invoke may block the UI thread.

Consider using BeginInvoke if responsiveness is critical, or ensure that Invoke does not lead to UI freezes by testing with scenarios of high computational load.

Comment on lines 37 to 50
protected override byte[] MapColorValues(int zone, Color color)
{
if (!DynamicLightingCapabilities.HasFlag(level))
return false;

color = MainColor;

switch (level)
switch (zone)
{
case LEDLevel.SolidColor:
SetLEDGroupColor(LEDGroup.AYAButton, AYA_ZONES, color);
SetLEDGroupColor(LEDGroup.JoystickBoth, STICK_ZONES, color);
break;
case 1:
return [color.G, color.R, color.B];
case 2:
return [color.G, color.B, color.R];
case 3:
return [color.B, color.R, color.G];
case 4:
return [color.B, color.G, color.R];
default:
return [color.R, color.G, color.B];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New MapColorValues method for LED color mapping.

The method returns an array with incorrect syntax. Use curly braces {} instead of square brackets [] for array initialization.

- return [color.G, color.R, color.B];
+ return new byte[] {color.G, color.R, color.B};

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.

Suggested change
protected override byte[] MapColorValues(int zone, Color color)
{
if (!DynamicLightingCapabilities.HasFlag(level))
return false;
color = MainColor;
switch (level)
switch (zone)
{
case LEDLevel.SolidColor:
SetLEDGroupColor(LEDGroup.AYAButton, AYA_ZONES, color);
SetLEDGroupColor(LEDGroup.JoystickBoth, STICK_ZONES, color);
break;
case 1:
return [color.G, color.R, color.B];
case 2:
return [color.G, color.B, color.R];
case 3:
return [color.B, color.R, color.G];
case 4:
return [color.B, color.G, color.R];
default:
return [color.R, color.G, color.B];
protected override byte[] MapColorValues(int zone, Color color)
{
switch (zone)
{
case 1:
return new byte[] {color.G, color.R, color.B};
case 2:
return new byte[] {color.G, color.B, color.R};
case 3:
return new byte[] {color.B, color.R, color.G};
case 4:
return new byte[] {color.B, color.G, color.R};
default:
return new byte[] {color.R, color.G, color.B};

@@ -28,7 +28,7 @@ private void Steam_Updated(PlatformStatus status)
bool DesktopProfileApplied = PlatformManager.Steam.HasDesktopProfileApplied();

// UI thread (async)
Application.Current.Dispatcher.BeginInvoke(() =>
Application.Current.Dispatcher.Invoke(() =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching from BeginInvoke to Invoke may block the UI thread.

Consider using BeginInvoke if responsiveness is critical, or ensure that Invoke does not lead to UI freezes by testing with scenarios of high computational load.

Comment on lines 125 to 156
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBetterBattery, Properties.Resources.PowerProfileROGAllyBetterBatteryDesc)
{
Default = true,
DeviceDefault = true,
OEMPowerMode = (int)AsusMode.Silent,
OSPowerMode = OSPowerMode.BetterBattery,
Guid = new("961cc777-2547-4f9d-8174-7d86181b8a7a")
CPUBoostLevel = CPUBoostLevel.Disabled,
Guid = new("961cc777-2547-4f9d-8174-7d86181b8a7a"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 10.0d, 10.0d, 10.0d }
});

DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyPerformanceName, Properties.Resources.PowerProfileROGAllyPerformanceDescription)
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBetterPerformance, Properties.Resources.PowerProfileROGAllyBetterPerformanceDesc)
{
Default = true,
DeviceDefault = true,
OEMPowerMode = (int)AsusMode.Performance,
OSPowerMode = OSPowerMode.BetterPerformance,
Guid = new("3af9B8d9-7c97-431d-ad78-34a8bfea439f")
Guid = new("3af9B8d9-7c97-431d-ad78-34a8bfea439f"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 15.0d, 15.0d, 15.0d }
});

DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyTurboName, Properties.Resources.PowerProfileROGAllyTurboDescription)
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBestPerformance, Properties.Resources.PowerProfileROGAllyBestPerformanceDesc)
{
Default = true,
DeviceDefault = true,
OEMPowerMode = (int)AsusMode.Turbo,
OSPowerMode = OSPowerMode.BestPerformance,
Guid = new("ded574b5-45a0-4f42-8737-46345c09c238")
Guid = new("ded574b5-45a0-4f42-8737-46345c09c238"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 25.0d, 25.0d, 25.0d }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refactoring the power profiles to avoid duplication of default values across profiles, enhancing maintainability.

-            Default = true,
-            DeviceDefault = true,
+            Default = this.IsFirstProfile(),
+            DeviceDefault = this.IsFirstProfile(),

Add a method IsFirstProfile() to determine if a profile is the first one being added, thus maintaining the intent but reducing redundancy.


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.

Suggested change
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBetterBattery, Properties.Resources.PowerProfileROGAllyBetterBatteryDesc)
{
Default = true,
DeviceDefault = true,
OEMPowerMode = (int)AsusMode.Silent,
OSPowerMode = OSPowerMode.BetterBattery,
Guid = new("961cc777-2547-4f9d-8174-7d86181b8a7a")
CPUBoostLevel = CPUBoostLevel.Disabled,
Guid = new("961cc777-2547-4f9d-8174-7d86181b8a7a"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 10.0d, 10.0d, 10.0d }
});
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyPerformanceName, Properties.Resources.PowerProfileROGAllyPerformanceDescription)
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBetterPerformance, Properties.Resources.PowerProfileROGAllyBetterPerformanceDesc)
{
Default = true,
DeviceDefault = true,
OEMPowerMode = (int)AsusMode.Performance,
OSPowerMode = OSPowerMode.BetterPerformance,
Guid = new("3af9B8d9-7c97-431d-ad78-34a8bfea439f")
Guid = new("3af9B8d9-7c97-431d-ad78-34a8bfea439f"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 15.0d, 15.0d, 15.0d }
});
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyTurboName, Properties.Resources.PowerProfileROGAllyTurboDescription)
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBestPerformance, Properties.Resources.PowerProfileROGAllyBestPerformanceDesc)
{
Default = true,
DeviceDefault = true,
OEMPowerMode = (int)AsusMode.Turbo,
OSPowerMode = OSPowerMode.BestPerformance,
Guid = new("ded574b5-45a0-4f42-8737-46345c09c238")
Guid = new("ded574b5-45a0-4f42-8737-46345c09c238"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 25.0d, 25.0d, 25.0d }
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBetterBattery, Properties.Resources.PowerProfileROGAllyBetterBatteryDesc)
{
Default = this.IsFirstProfile(),
DeviceDefault = this.IsFirstProfile(),
OEMPowerMode = (int)AsusMode.Silent,
OSPowerMode = OSPowerMode.BetterBattery,
CPUBoostLevel = CPUBoostLevel.Disabled,
Guid = new("961cc777-2547-4f9d-8174-7d86181b8a7a"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 10.0d, 10.0d, 10.0d }
});
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBetterPerformance, Properties.Resources.PowerProfileROGAllyBetterPerformanceDesc)
{
Default = this.IsFirstProfile(),
DeviceDefault = this.IsFirstProfile(),
OEMPowerMode = (int)AsusMode.Performance,
OSPowerMode = OSPowerMode.BetterPerformance,
Guid = new("3af9B8d9-7c97-431d-ad78-34a8bfea439f"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 15.0d, 15.0d, 15.0d }
});
DevicePowerProfiles.Add(new(Properties.Resources.PowerProfileROGAllyBestPerformance, Properties.Resources.PowerProfileROGAllyBestPerformanceDesc)
{
Default = this.IsFirstProfile(),
DeviceDefault = this.IsFirstProfile(),
OEMPowerMode = (int)AsusMode.Turbo,
OSPowerMode = OSPowerMode.BestPerformance,
Guid = new("ded574b5-45a0-4f42-8737-46345c09c238"),
TDPOverrideEnabled = true,
TDPOverrideValues = new[] { 25.0d, 25.0d, 25.0d }

Comment on lines 566 to 583
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].X), 0, outputData, outIdx, 4);
outIdx += 4;
// accelYG
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer.Y), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].Y), 0, outputData, outIdx, 4);
outIdx += 4;
// accelZG
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Accelerometer.Z), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].Z), 0, outputData, outIdx, 4);
outIdx += 4;

// Gyroscope
// angVelPitch
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Gyroscope.X), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].X), 0, outputData, outIdx, 4);
outIdx += 4;
// angVelYaw
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Gyroscope.Y), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].Y), 0, outputData, outIdx, 4);
outIdx += 4;
// angVelRoll
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope.Z), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].Z), 0, outputData, outIdx, 4);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to use a method for accessing Accelerometer and Gyroscope data to reduce duplication and improve maintainability.

- Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].X), 0, outputData, outIdx, 4);
- Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].Y), 0, outputData, outIdx, 4);
- Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].Z), 0, outputData, outIdx, 4);
- Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].X), 0, outputData, outIdx, 4);
- Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].Y), 0, outputData, outIdx, 4);
- Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].Z), 0, outputData, outIdx, 4);
+ private void CopySensorDataToOutput(byte[] outputData, ref int outIdx, SensorData sensorData)
+ {
+     Array.Copy(BitConverter.GetBytes(sensorData.X), 0, outputData, outIdx, 4);
+     outIdx += 4;
+     Array.Copy(BitConverter.GetBytes(sensorData.Y), 0, outputData, outIdx, 4);
+     outIdx += 4;
+     Array.Copy(BitConverter.GetBytes(sensorData.Z), 0, outputData, outIdx, 4);
+     outIdx += 4;
+ }
+ CopySensorDataToOutput(outputData, ref outIdx, Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU]);
+ CopySensorDataToOutput(outputData, ref outIdx, Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU]);

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.

Suggested change
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].X), 0, outputData, outIdx, 4);
outIdx += 4;
// accelYG
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer.Y), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].Y), 0, outputData, outIdx, 4);
outIdx += 4;
// accelZG
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Accelerometer.Z), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU].Z), 0, outputData, outIdx, 4);
outIdx += 4;
// Gyroscope
// angVelPitch
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Gyroscope.X), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].X), 0, outputData, outIdx, 4);
outIdx += 4;
// angVelYaw
Array.Copy(BitConverter.GetBytes(Inputs.GyroState.Gyroscope.Y), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].Y), 0, outputData, outIdx, 4);
outIdx += 4;
// angVelRoll
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope.Z), 0, outputData, outIdx, 4);
Array.Copy(BitConverter.GetBytes(-Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU].Z), 0, outputData, outIdx, 4);
private void CopySensorDataToOutput(byte[] outputData, ref int outIdx, SensorData sensorData)
{
Array.Copy(BitConverter.GetBytes(sensorData.X), 0, outputData, outIdx, 4);
outIdx += 4;
Array.Copy(BitConverter.GetBytes(sensorData.Y), 0, outputData, outIdx, 4);
outIdx += 4;
Array.Copy(BitConverter.GetBytes(sensorData.Z), 0, outputData, outIdx, 4);
outIdx += 4;
}
CopySensorDataToOutput(outputData, ref outIdx, Inputs.GyroState.Accelerometer[GyroState.SensorState.DSU]);
CopySensorDataToOutput(outputData, ref outIdx, Inputs.GyroState.Gyroscope[GyroState.SensorState.DSU]);

Comment on lines 195 to 196
//TODO - Add firewall entry
end;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the necessary firewall entry as indicated by the TODO comment.

Would you like assistance in implementing the firewall entry code? If so, I can help draft the necessary script or configuration.

@@ -664,7 +671,7 @@ 19. Triggers
SendHidControlWrite(flushBufferWriteChanges); // 16

// Choose the appropriate mapping based on the 'Remap' flag
SendHidControlWrite(Remap ? M1F17M2F18 : M1M2Default); // Step 17
SendHidControlWrite(Remap ? M1F18M2F17 : M1M2Default); // Step 17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The 'Remap' flag is minimally documented in the codebase, as indicated by a single comment describing its use in HID control writes. However, there is no evidence of dedicated testing for this flag, which is a concern for ensuring its correct implementation and functionality. It is recommended to add comprehensive tests to verify the behavior of the 'Remap' flag under various conditions.

  • Documentation found: HandheldCompanion/Devices/ASUS/ROGAlly.cs (Line 674)
  • Testing: No test cases involving the 'Remap' flag were found.
Analysis chain

Ensure the 'Remap' flag is well-documented and tested to confirm its impact on device configuration.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for usage of 'Remap' flag in the codebase to ensure it's handled correctly.
rg --type cs 'bool Remap' --context 5

Length of output: 691



Script:

#!/bin/bash
# Search for comments related to the 'Remap' flag in the codebase
rg --type cs '//.*Remap.*' --context 2

# Search for test cases involving the 'Remap' flag
rg --type cs 'Remap' --files-with-matches | grep 'Tests'

Length of output: 517

Comment on lines 71 to 72
protected FontFamily GlyphFontFamily = new("PromptFont");
public static readonly string defaultGlyph = "\u2753";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Several files in the codebase reference the font "PromptFont," but it does not appear to be included in the project resources based on the search results in XML and JSON files. This could potentially lead to runtime errors or fallback to default fonts as the font might not be available at runtime.

  • HandheldCompanion/Managers/Hotkeys/InputsHotkey.cs
  • HandheldCompanion/Devices/IDevice.cs
  • HandheldCompanion/Controllers/IController.xaml.cs
Analysis chain

Ensure that the font "PromptFont" is included in the project resources or is a system font. Missing font resources can lead to runtime errors or fallback to default fonts.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Check if the font "PromptFont" is included in the project resources
rg --type cs "PromptFont"

Length of output: 402



Script:

#!/bin/bash
# Search for resource files or configurations that might include font definitions
rg --type xml --type json "PromptFont"

Length of output: 38

Comment on lines 85 to 87
protected double VibrationStrength = 1.0d;
private byte _UserIndex = 255;
private readonly int MaxUserIndex = 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The review comment raises a concern about the handling of _UserIndex initialized to 255, which is used as a sentinel value. The script output confirms that _UserIndex is accessed and modified within its class but does not show any special handling or conditions for the sentinel value of 255. This could potentially lead to unintended behavior if other parts of the code rely on this value without proper checks.

  • HandheldCompanion/Controllers/IController.xaml.cs: The _UserIndex is used in getter and setter methods without conditions checking for the sentinel value.
Analysis chain

The _UserIndex is initialized to 255, which seems to be used as a sentinel value. Ensure that this value is handled appropriately in all relevant parts of the code to avoid unintended behavior.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for usages of `_UserIndex` to ensure it's handled properly
rg --type cs "_UserIndex"

Length of output: 285

Comment on lines 117 to 131
ProgressBarWarning.Visibility = value ? Visibility.Visible : Visibility.Collapsed;
});

switch (value)
{
case false:
{
// kill working thread
if (workingThread is not null)
{
workingThreadRunning = false;
// Ensure the thread has finished execution
if (workingThread.IsAlive)
workingThread.Join();
workingThread = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thread management logic here is risky. Specifically, the use of Thread.Join without a timeout can lead to a deadlock if the thread does not terminate as expected. Consider using Thread.Join(TimeSpan) with a reasonable timeout.

- workingThread.Join();
+ workingThread.Join(TimeSpan.FromSeconds(5)); // Timeout after 5 seconds

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.

Suggested change
ProgressBarWarning.Visibility = value ? Visibility.Visible : Visibility.Collapsed;
});
switch (value)
{
case false:
{
// kill working thread
if (workingThread is not null)
{
workingThreadRunning = false;
// Ensure the thread has finished execution
if (workingThread.IsAlive)
workingThread.Join();
workingThread = null;
ProgressBarWarning.Visibility = value ? Visibility.Visible : Visibility.Collapsed;
});
switch (value)
{
case false:
{
// kill working thread
if (workingThread is not null)
{
workingThreadRunning = false;
// Ensure the thread has finished execution
if (workingThread.IsAlive)
workingThread.Join(TimeSpan.FromSeconds(5)); // Timeout after 5 seconds
workingThread = null

Valkirie and others added 28 commits April 19, 2024 14:03
* Disable the RealTimeStylus for WPF Applications

* Update MainWindow.xaml.cs

* test ?
…#158)

* Start implementing IGCL (wip)

* More work

- Implemented GPUManager
- Implemented GPU-specific classes (AMDGPU, IntelGPU)
- Implemented IGCLBackend (wip)
- SystemManager renamed to MultimediaManager
- PowerManager renamed to SystemManager

* more work on IGCL

* prevent crash on null MainThread

* prevent useless SetResolution() calls

* more work on IGCL

* add missing sharpness check

* implement ctl_device_adapter_properties_t  (wip)

* what if the issue was deviceIdx all along...

* Update IGCL_Wrapper.dll

* fix remaining implementations

* implement IntegerScalingType (Intel only)

* make sure to use defaultGPU (idx: 0)

We need to find a proper way to guess which one is used for 3D rendering I guess or linked to main screen..

* fix ctl_device_adapter_properties_t Marshalling

* implemented some form of logic to pick the first available external GPU (if any)

* improve GPUManager

- add support for Manufacturer: "Advanced Micro Devices, Inc."
- improve GPUManager and GPU Start() and Stop() logics
- prevent Task Execution within Tasks on AMDGPU

* fix a crash when UpdateTimer is null
* Implement new UI classes

- UISounds to manage UI sounds on interaction.
- UIGamepad to manage gamepad interactions.
- Audio files from https://kenney.nl/assets/ui-audio.
- Add support for TextBox and RepeatButton selection via gamepad.

* Update HandheldCompanion/UI/UISounds.cs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* fix PlayOggFile refs

* removed unused audio files

* Add UI Sounds toggle on SettingsPage (default Off)

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Migrate everything ADLX related to ADLX_Wrapper()

* update IGCL logic

- Implemented Terminate() and Initialize() as well as GetTelemetryData()

* debug functions on both IGCL and ADLX backends

* Update ADLX_Wrapper and fix Initialize() calls on GPU classes

* add Telemetry Timer as part of GPU class

* Implement GPU GetLoad() and GetPower()
Fixing the rare case of soft-brick issue on uninstallation caused by a race condition
…e#186)

* early wip

* more work

* improve DesktopScreen GetResolution() logic

* implement GPUManager Hooked event

* prevent crash on null gpu variable

* improve performance manager locks logic

* more work on ADLX

- Implemented displayIdx logic
- Renamed a few ADLXBackend arguments for clarity
- Leveraging WindowsDisplayAPI library to link a display DisplayName with its FriendlyName (which is reported by ADLX
- Now storing FriendlyName and DevicePath on DesktopScreen

* add new functions to MultimediaManager

- GetDisplayFriendlyName()
- GetDisplayPath()
- GetDisplayTarget()
* move a few calls away from MainWindow

* improve main window loading experience

* halt and resume GPU manager on sleep/resume

* suspend/resume LibreHardwareMonitor with system

* check IGCL/ADLX status before trying to terminate()

* migrate GPU wait to GPUManager

* mark GPU as Halting on system stop/sleep

Prevents any further ADLX/IGCL calls while GPU is halting

* Fixes Valkirie#990

* misc edit to VirtualManager and vTargets

* Improved management of controller-specific parameters

- Implemented IController UpdateSettings()
- Implemented UpdateSettings() supports over LegionController and SteamController

* improve lock logic on GPU

* revert GPU halting logic

I need to learn about Semaphore here to lock and release from different thread.

* implement CrossThreadLock class

- used by GPU

* delay system until we're done !

* fix usage of CrossThreadLock within GPU

* fix DesktopScreen GetResolution()

That's dirty :'(

* Support for Ayaneo Slide

Support for Ayaneo Slide

---------

Co-authored-by: DevL0rd <[email protected]>
* MVVM Rework - Batch 1

* Fix merge mistake.

* update IController

- make TargetButtons, TargetAxis, SourceAxis, SourceButtons non static

* feed SourceButtons, SourceAxis on IController creation

* Corrected inconsistencies in the use of Environment.ProcessorCount and MotherboardInfo.NumberOfCores

---------

Co-authored-by: Matthias Seys <[email protected]>
Co-authored-by: Lesueur Benjamin <[email protected]>
* Testing left gyro

* start implementing per-controller gyro support

* improve the automatic sensor switching logic

- when a controller is targeted, use its sensors if available
- when unplugged, if controller had sensors, pick next available
- if a controller is plugged and doesn't have sensors, pick next available

* implement LegionControllerGyroIndex on DevicePage

* fix type on gZ
* Testing left gyro

* start implementing per-controller gyro support

* improve the automatic sensor switching logic

- when a controller is targeted, use its sensors if available
- when unplugged, if controller had sensors, pick next available
- if a controller is plugged and doesn't have sensors, pick next available

* implement LegionControllerGyroIndex on DevicePage

* fix type on gZ

* migrate sensor filtering

* improve filtering logic

* Tentative

* it's working

* remove deprecated filtering

* sorting all axis

* more work on axis, UI and calibration

- still getting awkward results when testing PadTest (Cemuhook)

* remove deprecated FilterMotion() function

* more work on gyro aiming

- implemented local space, player space and world space

* add missing glyph for MotionInput.LocalSpace

* fix inclination

* use GetGravity() instead of GetProcessedAcceleration()

* restore normal inclination now that we're using GetGravity instead of GetProcessedAcceleration

* fix legion controller gyro/accelero computation

* improve overall motion experience

- remove hardcoded motion delta, use TimerManager event to pass double delta value.
- implement SensorsManager ProcessReport() function

* remove useless argument from SensorsManager UpdateReport()

* Make TimerManager delta float

- One cast instead of several.

* remove MadgwickAHRS

- migrate ToEulerAngles to InputUtils

* compile GamepadMotionHelper in Release

* rename Legion Controller

* implement IMUCalibration

- SensorsManager can store and retrieve calibration offset data from GamepadMotion objects

* fix incorrect GamepadMotion variable when using Internal IMU

* Implement device IMU calibration

* set calibration button style to accent

* prevent crash on null gamepadMotion

* move motion processing to Motion Manager

* remove redundant switch

* more work on motion manager

- fixed motion invert (horizontal, vertical)
- fixed steering
- now properly swapping (Roll, Yaw) on LocalSpace
* InnoSetup Update

Update InnoSetup with the following:
- Install dependencies only when not installed or newer version comes with installer
- Show optional reboot screen only when required
- After installing HidHide, remove desktop icon
- Option to open the application after install

* update HidHide and RTSS redist

Files were already pushed to mainline

* update NewHidHideVersion

---------

Co-authored-by: Lesueur Benjamin <[email protected]>
* Support for MSI Claw

Adds support for Claw and Quick Settings buttons
Adds orientation for IMU
Adds TDP and clock information
Updated PromptFont with MSI Claw glyphs

* fix gyro axis

* force device to XInput mode on startup (poc)

* fix CPU power reading on Intel CPUs

* fix incorrect accelerometer axis

* add default power profiles

* implementing hint (WIP)

* implement hint

* prevent crash if ContentFrame can't go back on power profile delete

* fix MSI power profiles naming

* rename OEM power presets variable names

* change profile CPUBoostLevel default value to 1:Enabled

---------

Co-authored-by: romracer <romracer@localhost>
Co-authored-by: Lesueur Benjamin <[email protected]>
- if main sensor is internal/external, swap axis to Yaw.
- if main sensor is controller, compare accelZ and accelY to swap axis.
Valkirie and others added 21 commits April 19, 2024 14:26
* prevent GPU Execute() overlapping

* prevent WinEventProc (FOREGROUND) from locking UI thread
This is the same code as the previous PR (Valkirie#236) but I noticed the filesize was bigger.
Inspecting it revealed it was a DLL (GUI) (whatever that is) versus a DLL (console).
I was also getting access violation exceptions that I would not expect.

I re-compiled it from the VS Developer Command Prompt instead of the GUI.
The filesize now more closely matches the previous file and it no longer reports as DLL (GUI).
I have also not been able to reproduce the access violation exceptions..
* Revert "prevent crash on null MotherboardInfo collections"

This reverts commit 3cd59dd.

* Revert "prevent crash on null motherboard details"

This reverts commit 288a641.

* Prevent looping null collections
This syncs the DLL with Valkirie/ADLX-SDK-Wrapper#6
Specifically, it reverts the strncpy change from Valkirie/ADLX-SDK-Wrapper#5
* Actuall set  CurrentTDP

* Actually  set CurrentTDP at some point
* Use correct type for custom osd level settings

* fix Settings.Designer.cs

We need to make sure we're using our custom SettingsProvider

---------

Co-authored-by: Lesueur Benjamin <[email protected]>
* Hold RGB control while running

* Remove Ayaneo PowerStatusChanged, control makes it obsolete

---------

Co-authored-by: CasperH2O <[email protected]>
* Cache properties

Only ProcessorMaxTurboSpeed is not cached now, but its queried from IDevice anyway.

* Use separate caches for easier deserialization

* Additional MotherboardInfo changes

* Fix duplicate cache keys

* simplify MotherboardInfo caching logic

* prevent crash on null processor

---------

Co-authored-by: Lesueur Benjamin <[email protected]>
* Update path and check for newer HidHide versions
* Update the version string for RTSS and flag it as reboot required
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Comment on lines +92 to +93
// avoid locking UI thread
new Thread(() => { ForegroundCallback(); }).Start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using Task.Run instead of new Thread for better resource management.

Using Task.Run instead of new Thread can lead to better thread pool management and potentially improved performance. Task.Run uses the .NET thread pool, which is optimized for short-running compute-bound operations and can efficiently handle a large number of small tasks by maintaining a pool of threads that are reused.

- new Thread(() => { ForegroundCallback(); }).Start();
+ Task.Run(() => { ForegroundCallback(); });

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.

Suggested change
// avoid locking UI thread
new Thread(() => { ForegroundCallback(); }).Start();
// avoid locking UI thread
Task.Run(() => { ForegroundCallback(); });

Comment on lines +573 to +576
{
processor.SetTDPLimit(type, TDP);
CurrentTDP[idx] = TDP;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure thread safety when updating CurrentTDP array.

The code modifies the CurrentTDP array within a loop that potentially runs on multiple threads due to the asynchronous nature of cpuWatchdog_Elapsed. This could lead to race conditions. Consider using thread-safe operations or synchronization mechanisms when updating shared data.

Comment on lines +870 to +878
if (processor is not null && processor.IsInitialized)
{
processor.StatusChanged += Processor_StatusChanged;
processor.Initialize();
}

// deprecated
/*
processor.ValueChanged += Processor_ValueChanged;
processor.LimitChanged += Processor_LimitChanged;
processor.MiscChanged += Processor_MiscChanged;
*/
else
{
ProcessorStatusChanged?.Invoke(false, false);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle potential null reference in processor initialization.

The method Start checks if processor is not null and is initialized before attaching event handlers and calling Initialize. However, there's a potential for processor to be null or uninitialized when ProcessorStatusChanged is invoked immediately after. This could lead to a null reference exception. Ensure that processor is properly checked before invoking any methods or accessing its properties.

Comment on lines +536 to +541
private void ProfileManager_Deleted(Profile profile)
{
// update current process
currentProcess = processEx;

// update real profile
realProfile = ProfileManager.GetProfileFromPath(processEx.Path, true);
// this shouldn't happen, someone removed the currently applied profile
if (selectedProfile == profile)
ProcessManager_ForegroundChanged(currentProcess, null);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove duplicate method definition for ProfileManager_Deleted.

-    private void ProfileManager_Deleted(Profile profile)
-    {
-        // this shouldn't happen, someone removed the currently applied profile
-        if (selectedProfile == profile)
-            ProcessManager_ForegroundChanged(currentProcess, null);
-    }

This method is defined twice in the file, which will cause a compilation error. The duplicate definition should be removed to resolve this issue.


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.

Suggested change
private void ProfileManager_Deleted(Profile profile)
{
// update current process
currentProcess = processEx;
// update real profile
realProfile = ProfileManager.GetProfileFromPath(processEx.Path, true);
// this shouldn't happen, someone removed the currently applied profile
if (selectedProfile == profile)
ProcessManager_ForegroundChanged(currentProcess, null);
}

}

// no code from the cases inside this function will be called on program start
private async void OnSystemStatusChanged(PowerManager.SystemStatus status, PowerManager.SystemStatus prevStatus)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the necessary using directive or reference for PowerManager to resolve the compilation error.

@@ -49,7 +49,8 @@
#define NewDirectXVersion "9.29.1974"
#define NewViGemVersion "1.22.0.0"
#define NewHidHideVersion "1.5.212"
#define NewRtssVersion "7.3.6"
; RTSS 7.3.6
#define NewRtssVersion "7.3.5.28010"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RTSS version number is inconsistent with the defined constant.

- #define NewRtssVersion "7.3.5.28010"
+ #define NewRtssVersion "7.3.6"

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.

Suggested change
#define NewRtssVersion "7.3.5.28010"
#define NewRtssVersion "7.3.6"

@CasperH2O CasperH2O merged commit cf38ec9 into Valkirie:main Apr 19, 2024
1 check failed
@CasperH2O CasperH2O deleted the MergeEAtoPublic branch April 25, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants