Update Colore and make IDevice methods asynchronous

[ChrisDurante]

This pull request proposes the following changes: Helping to remove dependencies that are not netstandard 2.0+ or .NET 6/7. Also, WebRequest and WebClient are deprecated in .NET 5+ and should be replaced with HttpClient (warning SYSLIB0014)

• Update Colore to .NET6 version
• IDevice methods made asynchronous
• Remove WebRequest from Razer download and replace with HttpClient
• Remove some .NET Framework only settings
• Fix some warnings due to changes in WPF from .NET Framework to .NET 6
  • e.g. On JsonSerializerSettings the Binder property is deprecated and SerializationBinder should be used

ToDo's

• The only remaining .NET Framework dependencies are:
  • Infagistics.Themes.MetroDark.Wpf
  • SharpDX.RawInput

They have no obvious replacement for .NET 6+, but likely could be forked or merged into the solution and built to target .NET 6 later

• Aurora-Updater uses WebClient and it should use HttpClient - can be done separately

[Aytackydln]

Thanks for the changes. They seem good but private field variable changes will revert most of the naming standardization work I've done. Could you revert those and add some async tags to needed method? Then I will be able to review the changes better, without the clutter.

I've tried to change Colore library earlier but it broke functionality. I will test your changes later.

Infagistics.Themes.MetroDark.Wpf seems abandoned and seems insignificant for .net 6 changes.

For SharpDX.RawInput to be upgraded, classes that depend on it needs to be refactored (lots of effort). Before changing the library those dependencies need to be inverted so that library change could be done easier later

[ChrisDurante]

Hi! I reverted the renames - sorry, it wasn't clear if _ is used because it's different in some places in the solution. It might help to introduce an .editorconfig and set tabs/spaces, style rules for things like using order (System. first), braces optional for 'if' (or not, which I've always preferred), etc.

I don't think I know what you mean by "async tags"? Since I've changed the IDevice interface to return Task, I've updated each implementation to either return Task wrapped results if they are not really async, or used await on async methods. Do you want to always use Lombok.NET Async? I'm not sure how that will work with Interfaces to be honest. Also requires every class to be partial. I've had to use sync and async code together a lot with legacy applications, so I'm kind of used to the boilerplate, even though it seems annoying.

In typical .NET 6 apps, everything is Async first (not always of course) so it seems weird to have both a sync and async method for an API since you usually never want a caller to use the synchronous one.

Let me know, I'll try to get it to be consistent.

[Aytackydln]

Project's been developed since 2016 by many different developers, including people new to coding. So, there isn't many standards here. You can make the .editorconfig with some light rules, if you want.

By async tags I mean making methods "async". Like in below, since method is already overriding async method is it needed to specift return type as Task? I'm not very experienced with C# development so please enlighten me if I'm missing something.

About lombok.net, at first most of the features seemed nice but later I figured they caused false-positive warnings and didn't provide too much flexibility. Async works fine for interfaces, see module classes but generated method has to copy annotated method's accessor tag. I've tried to convince it's developer to add features to fix the issues I had but he shrugged them off without giving solid reasons, so I gave up. I'm okay if you removed lombok library completely.

[ChrisDurante]

By async tags I mean making methods "async". Like in below, since method is already overriding async method is it needed to specift return type as Task?

No worries. If a method can be async, it should always return a Task. There is a pretty special exception for events where you might use async void, but with caution.

On Interfaces where there is no implementation, you omit the async keyword, but have to return Task.

public interface IDoSomething {
  public Task DoSomething();
}

Then in the implementation, you only include the async keyword if the method awaits some other awaitable method.

public class ReallyDoesSomething : IDoSomething {
  public async Task DoSomething() {
    var x = await SomeOtherThing();
  }
}

Implementing that without actually using await, you still return a Task

public class SortaDoesSomething : IDoSomething {
  public Task DoSomething() {
    var x = SomeOtherUnawaitableThing();
    return Task.CompletedTask;
  }
}

I think if you want to remove Lombok, it could be separate :). Maybe the same for .editorconfig - I can try to make a minimal one next contrib.

[Aytackydln]

btw, do you have a razer device and did you try the sdk?

[Aytackydln]

Another thing, clicking Start on device blocks the UI, are you sure they don't block the calling thread? This was my main intention while making them async.

[ChrisDurante]

btw, do you have a razer device and did you try the sdk?

Yes, I have Corsair devices, a Logitech mouse, and a Razer headset. I tested both Chroma layer (with Overwatch 2) and a regular profile with the changes.

[Aytackydln]

Well, I'm getting access violation exception while trying to connect razer on your branch

[ChrisDurante]

Another thing, clicking Start on device blocks the UI, are you sure they don't block the calling thread? This was my main intention while making them async.

The UI isn't blocked - I tested this by adding a fake delay inside EnableDevice I can click all the other start buttons, change tabs, see effect updates on the display, etc. even during that enable time.

The button click events, etc. are fire and forget, so they won't block unless you explicitly call Tasl.Wait or .GetAwaiter().GetResult() or something.

However - what is different is that UpdateControls() is called immediately and the newly added property IsInitializing is not set to true yet, so it doesn't disable the button and show Working... I'll work on that, since that is definitely not the intent and doesn't help the user much.

[ChrisDurante]

Well, I'm getting access violation exception while trying to connect razer on your branch

I tried uninstalling chroma, re-installing chroma, hitting that "Disable Device Control" button and then re-intalling Chroma and I'm not able to repro. Do you have a stack trace or log that might give me a hint? I thought maybe it had to do with switching to/from RGB.NET, but I couldn't provoke it.

[ChrisDurante]

Another thing, clicking Start on device blocks the UI, are you sure they don't block the calling thread? This was my main intention while making them async. ... I'll work on that, since that is definitely not the intent and doesn't help the user much.

OK - The buttons on the Device Manager are disabled when the operation starts and subsequent timer ticks will re-enable the button when the device state changes.

[Aytackydln]

There is another problem, with OpenRGB. When I restarted my pc, Aurora didn't have any control of OpenRGB. Device manager shows "Initialized". Clicking Disable gets stuck.

Also happens when I click "Start" while openrgb is closed and opening OpenRGB later.

There doesn't seem to be OpenRGB specific change, so other devices might be affected. Can you fix this without touching openrgb code?

[ChrisDurante]

There is another problem, with OpenRGB. When I restarted my pc, Aurora didn't have any control of OpenRGB. Device manager shows "Initialized". Clicking Disable gets stuck.

Also happens when I click "Start" while openrgb is closed and opening OpenRGB later.

There doesn't seem to be OpenRGB specific change, so other devices might be affected. Can you fix this without touching openrgb code?

One issue is the _openRgb?.Dispose(); call in OpenRGBDevice.cs. This always throws an exception.

The code in the OpenRGB.NET.dll included in the project tries to check the socket state after it's already been disposed. That exception leaves the semaphore locked - "fixed" using try-finally. It really seems like we can't Dispose that OpenRgbClient with the code in that assembly.

Is there a reason the project doesn't use this dependency? OpenRGB.NET nuget package v 2.1.0

[Aytackydln]

Reason was (until a few weeks ago) I changed packet reading of library to support DeviceListChanged event. This solved many things but since it changed a very important mechanism of library, it's developer held the PR for a while so I included my own fork.

My changes are merged now and you can use 2.1.0 version of it. But doing so library will use higher version of OpenRGB protocol, which I'm not sure if we should use it.

[ChrisDurante]

Reason was (until a few weeks ago) I changed packet reading of library to support DeviceListChanged event. This solved many things but since it changed a very important mechanism of library, it's developer held the PR for a while so I included my own fork.

My changes are merged now and you can use 2.1.0 version of it. But doing so library will use higher version of OpenRGB protocol, which I'm not sure if we should use it.

OK - I fixed the locking issues due to use of SemaphoreSlim instead of simple lock in the OpenRGB device and other related device startup and shutdown behaviors. I tested this with OpenRGB locally and it looks like it behaves as expected - let me know if you still see issues.

I won't update the OpenRGB.Net dependency in this PR. I did do that locally and tested after fixing the two small changes and it appeared to work, but it could just be done separately.

[Aytackydln]

If I start Aurora with Yeelight enabled no lighting effects happen, UI does not respond. I assume device intialization get stuck on deadlock.

[ChrisDurante]

If I start Aurora with Yeelight enabled no lighting effects happen, UI does not respond. I assume device intialization get stuck on deadlock.

I bought a couple Yeelight bulbs and I'm testing. I didn't get this behavior because the Razer, Corsair, Logitech and OpenRGB devices I had enabled take a very short amount of time to initialize and/or await is not hit so no thread context switch happens.

I bought these Yeelight bulbs: Smart LED Bulb 1S (color).

In testing, this SetMusicMode call always throws with these lights, and after that call, IsConnected is false, so they are not added.

If I don't try to enable MusicMode, they connect - have you seen this issue? Should we try to enable it, catch and reconnect if there was an exception?

[Aytackydln]

I guess waiting for music mode was the breaking thing.

Btw, I won't be able to maintain Aurora for a while (I don't know if it will be weeks or months).

Btw, I had to make some conflicts (sorry) but you can make some other progress assuming this branch will be merged as soon as I can confirm Yeelight thing is fixed

[ChrisDurante]

I guess waiting for music mode was the breaking thing.

Btw, I won't be able to maintain Aurora for a while (I don't know if it will be weeks or months).

Btw, I had to make some conflicts (sorry) but you can make some other progress assuming this branch will be merged as soon as I can confirm Yeelight thing is fixed

No worries. The startup behavior should be fixed for Yeelight. I can resolve the conflicts in a couple minutes.

[ChrisDurante]

@Aytackydln - I merged instead of rebasing, let me know if this is fine. I did a lot of testing around enabling/disabling, startup and shutdown with Corsair, Razer, Yeelight, Logitech and did also some with OpenRGB. Hopefully this is mergeable and thanks much!

[Aytackydln]

Sorry for the long delay! I've moved to Germany and just rebuilt my pc (barely). I only have RGB mouse and few motherboard lights now. If you apply the last review I will merge this.

I think merging is fine, I don't have too much experience about it. Do what you think is best

[ChrisDurante]

Sorry for the long delay! I've moved to Germany and just rebuilt my pc (barely). I only have RGB mouse and few motherboard lights now. If you apply the last review I will merge this.

I think merging is fine, I don't have too much experience about it. Do what you think is best

Hope your move was safe! I marked that last comment as resolved because if we wait for initialization, the app window doesn't respond, which I think is a behavior you had previously fixed by making device initialization asynchronous.

The additional changes I pushed should keep any synchronous calls to the device init/shutdown methods from deadlocking.