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

[REFERENCE] hotfix for new Legion Space update v1.0.2.5: Controller READY-byte an… #892

Merged
merged 4 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion HandheldCompanion.iss
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ end;

#define MyAppSetupName 'Handheld Companion'
#define MyBuildId 'HandheldCompanion'
#define MyAppVersion '0.19.1.6'
#define MyAppVersion '0.19.1.7'
#define MyAppPublisher 'BenjaminLSR'
#define MyAppCopyright 'Copyright @ BenjaminLSR'
#define MyAppURL 'https://github.com/Valkirie/HandheldCompanion'
Expand Down
27 changes: 12 additions & 15 deletions HandheldCompanion/Controllers/LegionController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ private enum BackEnum
private const byte BACK_IDX = 19;
private const byte STATUS_IDX = 0;
private const byte PING_IDX = 40;
private HashSet<int> READY_STATES = new HashSet<int>() {25, 60};
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the previous comment about making READY_STATES readonly and the absence of any code modifying it after initialization, it should be marked as readonly to prevent accidental modifications and clarify the intent.

- private HashSet<int> READY_STATES = new HashSet<int>() {25, 60};
+ private readonly HashSet<int> READY_STATES = new HashSet<int>() {25, 60};

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 HashSet<int> READY_STATES = new HashSet<int>() {25, 60};
private readonly HashSet<int> READY_STATES = new HashSet<int>() {25, 60};


private const byte MIN_WIRELESS_STATUS = 40;
private const byte MAX_WIRELESS_STATUS = 50;

private Thread dataThread;
private bool dataThreadRunning;
Expand All @@ -52,16 +56,16 @@ public override bool IsReady
get
{
byte status = GetStatus(STATUS_IDX);
return status == 25;
return READY_STATES.Contains(status);
}
}

Valkirie marked this conversation as resolved.
Show resolved Hide resolved
public bool IsWireless
{
get
Comment on lines 63 to 65
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate property definition.

- public bool IsWireless
- {
-     get

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
public bool IsWireless
{
get

{
byte status = GetStatus(PING_IDX);
return (status >= 40 && status <= 50);
byte status = GetStatus(PING_IDX);
return (status >= MIN_WIRELESS_STATUS && status <= MAX_WIRELESS_STATUS);
}
}

Expand Down Expand Up @@ -235,11 +239,12 @@ private async void dataThreadLoop(object? obj)
if (hidDevice is null)
continue;

HidReport report = hidDevice.ReadReport();
HidReport report = hidDevice.ReadReport();

if (report is not null)
Valkirie marked this conversation as resolved.
Show resolved Hide resolved
{
// check if packet is safe
if (report.Data[STATUS_IDX] == 25)
if (READY_STATES.Contains(report.Data[STATUS_IDX]))
Data = report.Data;
}
Comment on lines +242 to 249
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 NOTE
Codebase verification is a beta feature.

The search results show multiple accesses to the Data array in the LegionController.cs file. The Data array is being accessed in the IsReady getter, the GetStatus method, and within the UpdateInputs method. These accesses are outside of the dataThreadLoop method, which suggests that Data is indeed accessed from multiple threads.

Given that the Data array is being written to in the dataThreadLoop method and read from in other methods, thread safety is a concern. There is no evidence in the provided context of any synchronization mechanisms like locks, mutexes, or other thread-safe patterns being used when accessing the Data array.

This could potentially lead to race conditions where one thread is writing to the Data array while another thread is reading from it, which can cause undefined behavior or incorrect data being processed.

Based on the information provided, it is reasonable to conclude that the Data array is not being accessed in a thread-safe manner.

  • The Data array in LegionController.cs is accessed from multiple methods without apparent thread synchronization, which could lead to race conditions.
Analysis chain

The dataThreadLoop method has been updated to use the READY_STATES set for comparison. Ensure that the Data array is thread-safe since it's being accessed from multiple threads.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Check for any other occurrences of 'Data' being accessed without proper synchronization.
rg --type cs "Data" --context 5 HandheldCompanion/Controllers/LegionController.cs

Length of output: 3585

}
Expand Down Expand Up @@ -370,17 +375,9 @@ public void HandleTouchpadInput(bool touched, ushort x, ushort y)

internal void SetPassthrough(bool enabled)
{
switch(enabled)
{
case true:
SetTouchPadStatus(1);
break;
case false:
SetTouchPadStatus(0);
break;
}

SetTouchPadStatus(enabled ? 1 : 0);
IsPassthrough = enabled;
}

}
}
107 changes: 107 additions & 0 deletions HandheldCompanion/Controls/Hints/Hint_LegionGoServices.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
using HandheldCompanion.Devices;
using HandheldCompanion.Utils;
using HandheldCompanion.Views;
using System;
using System.Collections.Generic;
using System.Linq;
using System.ServiceProcess;
using System.Threading.Tasks;
using System.Timers;
using System.Windows;

namespace HandheldCompanion.Controls.Hints
{
public class Hint_LegionGoServices : IHint
{
private List<string> serviceNames = new()
{
"DAService",
};
Comment on lines +16 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider externalizing the service names to a configuration file or resource for easier maintenance.


private List<ServiceController> serviceControllers = new();
private Timer serviceTimer;

public Hint_LegionGoServices() : base()
{
if (MainWindow.CurrentDevice is not LegionGo)
return;

// Get all the services installed on the local computer
ServiceController[] services = ServiceController.GetServices();
foreach (string serviceName in serviceNames)
{
if (services.Any(s => serviceNames.Contains(s.ServiceName)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Logical error in service existence check.

- if (services.Any(s => serviceNames.Contains(s.ServiceName)))
+ if (services.Any(s => s.ServiceName == serviceName))

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
if (services.Any(s => serviceNames.Contains(s.ServiceName)))
if (services.Any(s => s.ServiceName == serviceName))

{
// Create a service controller object for the specified service
ServiceController serviceController = new ServiceController(serviceName);
serviceControllers.Add(serviceController);
}
}

// Check if any of the services in the list exist
if (!serviceControllers.Any())
return;
Comment on lines +42 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider providing user feedback or logging when no services are found on the system.


serviceTimer = new Timer(4000);
serviceTimer.Elapsed += ServiceTimer_Elapsed;
serviceTimer.Start();

// default state
this.HintActionButton.Visibility = Visibility.Visible;

this.HintTitle.Text = Properties.Resources.Hint_LegionGoServices;
this.HintDescription.Text = Properties.Resources.Hint_LegionGoServicesDesc;
this.HintReadMe.Text = Properties.Resources.Hint_LegionGoServicesReadme;

this.HintActionButton.Content = Properties.Resources.Hint_LegionGoServicesAction;
}

private void ServiceTimer_Elapsed(object? sender, ElapsedEventArgs e)
{
if(!serviceControllers.Any())
return;

// Check if any of the services in the list exist and are running
bool anyRunning = false;

foreach (ServiceController serviceController in serviceControllers)
{
serviceController.Refresh();
if (serviceController.Status == ServiceControllerStatus.Running)
{
anyRunning = true;
break;
}
}

// UI thread (async)
Application.Current.Dispatcher.BeginInvoke(() =>
{
this.Visibility = anyRunning ? Visibility.Visible : Visibility.Collapsed;
});
}

protected override void HintActionButton_Click(object sender, RoutedEventArgs e)
{
if (!serviceControllers.Any())
return;

Task.Run(async () =>
{
foreach (ServiceController serviceController in serviceControllers)
{
if (serviceController.Status == ServiceControllerStatus.Running)
serviceController.Stop();
serviceController.WaitForStatus(ServiceControllerStatus.Stopped);
ServiceUtils.ChangeStartMode(serviceController, ServiceStartMode.Disabled, out _);
}
Valkirie marked this conversation as resolved.
Show resolved Hide resolved
});
}

public override void Stop()
{
serviceTimer.Stop();
base.Stop();
}
}
}
2 changes: 1 addition & 1 deletion HandheldCompanion/HandheldCompanion.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<StartupObject>HandheldCompanion.App</StartupObject>
<OutputPath>$(SolutionDir)bin\$(Configuration)</OutputPath>
<ApplicationIcon>Resources\icon.ico</ApplicationIcon>
<Version>0.19.1.6</Version>
<Version>0.19.1.7</Version>
<ApplicationManifest>app.manifest</ApplicationManifest>
<Platforms>AnyCPU;x64;x86</Platforms>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
Expand Down
36 changes: 36 additions & 0 deletions HandheldCompanion/Properties/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions HandheldCompanion/Properties/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -2673,4 +2673,16 @@ with motion input toggle, press selected button(s) to switch from enabled to dis
<data name="DevicePage_FullFanSpeedText" xml:space="preserve">
<value>Fan max speed override</value>
</data>
<data name="Hint_LegionGoServices" xml:space="preserve">
<value>Legion Space Daemon is active</value>
</data>
<data name="Hint_LegionGoServicesAction" xml:space="preserve">
<value>Disable Legion Space Daemon</value>
</data>
<data name="Hint_LegionGoServicesDesc" xml:space="preserve">
<value>Legion Space Daemon is already active on your console. This may cause compatibility issues with the application.</value>
</data>
<data name="Hint_LegionGoServicesReadme" xml:space="preserve">
<value>You should disable Legion Space Daemon, so that the application can run properly and without any errors</value>
</data>
</root>
1 change: 1 addition & 0 deletions HandheldCompanion/Views/Pages/NotificationsPage.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
<ui:SimpleStackPanel Name="Notifications" Spacing="6">

<hints:Hint_LegionGoDaemon d:Visibility="Visible" />
<hints:Hint_LegionGoServices d:Visibility="Visible" />
<hints:Hint_RogAllyServiceCheck d:Visibility="Visible" />
<hints:Hint_SteamNeptuneDesktop d:Visibility="Visible" />
<hints:Hint_SteamXboxDrivers d:Visibility="Visible" />
Expand Down
Loading