chroma-sdk / Colore

A powerful C# library for Razer Chroma's SDK
https://chroma-sdk.github.io/Colore/
MIT License
146 stars 30 forks source link

CreateNativeAsync needs to be called twice #279

Closed Shadowtail117 closed 3 years ago

Shadowtail117 commented 4 years ago

I'm following the Getting Started tutorial to set up basic key modifications, but whenever I run it, I have to click the button twice to get any changes. The first time it is clicked, my Chroma Studio effects freeze, and then clicking it again will set the colors correctly. As demonstrated in #253, it looks like in order to successfully connect with Chroma, you have to initialize twice (since they had CreateNativeAsync followed by InitializeAsync), which can be a problem. Below is my code for reference, just in case I simply mistyped something (irrelevant usings are omitted). Anyone have a potential solution to this other than running the methods twice?

using Colore;
using ColoreColor = Colore.Data.Color;
using ColoreKey = Colore.Effects.Keyboard.Key;

///

private async void button1_Click(object sender, EventArgs e)
{
    var chroma = await ColoreProvider.CreateNativeAsync();
    await chroma.SetAllAsync(ColoreColor.White);
    await chroma.Keyboard.SetKeysAsync(ColoreColor.Red, ColoreKey.W, ColoreKey.A, ColoreKey.S, ColoreKey.D);
}
Sharparam commented 4 years ago

This is likely related to #274. Some thinking needed on how best to deal with this.

poveden commented 4 years ago

After some testing related with #274, I've found that the call to ColoreProvider.CreateNativeAsync() does return immediately, but the Chroma SDK seems to perform some validation tasks (lasting around 250 ms - 750 ms) before it starts processing commands.

I've discovered that the Chroma SDK will actually signal when this validation has completed by emitting the WM_CHROMA_EVENT event with wParam = 2 (Access to device) and lParam = 1 (Access granted).

Colore provides an event handler for catching that event (IChroma.DeviceAccess) but, in order to get that event triggered, you need to:

  1. Set up a Win32 event pump (available in .NET with WinForms, WPF, or subclassing NativeWindow), and
  2. Override WndProc(ref Message m) and carefully pass the events to IChroma.HandleMessage().

By "carefully", I mean that the current implementation of HandleMessage (as of 6.0.0-rc0005) will throw if you haven't previously called IChroma.Register() or it doesn't like the hWnd handle, or if you close the window and forget to call IChroma.Unregister().

(@Sharparam please change this to just returning false, as throwing exceptions within an event pump it's not really a good idea; if you want, I can set up a PR for you 😜)

As an example, I'm trying something like this (untested):

public sealed class ChromaWindow : NativeWindow // Or System.Windows.Forms.Form, or WPF
{
    private IChroma _chroma;
    private bool _chromaMsgHandlerSet;
    private TaskCompletionSource<bool> _chromaAccessGranted = new TaskCompletionSource<bool>();

    protected override void WndProc(ref Message m)
    {
        if (_chromaMsgHandlerSet && _chroma.HandleMessage(m.HWnd, m.Msg, m.WParam, m.LParam))
        {
            return;
        }

        base.WndProc(ref m);
    }

    private async Task Init()
    {
        var hWnd = this.Handle; // Should be available after the HandleCreated event.

        _chroma = await ColoreProvider.CreateNativeAsync().ConfigureAwait(false);
        _chroma.Register(hWnd);
        _chromaMsgHandlerSet = true;
        _chroma.DeviceAccess += Chroma_DeviceAccess;

        var granted = await _chromaAccessGranted.Task.ConfigureAwait(false);

        if (!granted)
        {
            throw new Exception("Chroma SDK has denied access.");
        }
    }

    private void Chroma_DeviceAccess(object sender, Colore.Events.DeviceAccessEventArgs e)
    {
        _chromaAccessGranted.SetResult(e.Granted);
    }
}

Hope that it helps.

Sharparam commented 4 years ago

@poveden Thanks for the input!

Apologies for the late response, have been absent from here for a while. I can change the behaviour of the handler to just log a warning and return false.

That would enable people to check it if they have access to the Win32 event system. For the cases where they don't have it available though I'm unsure how to best handle it. Got any ideas?

diff --git a/src/Colore/Implementations/ChromaImplementation.cs b/src/Colore/Implementations/ChromaImplementation.cs
index f1ee6b01..5934ea69 100644
--- a/src/Colore/Implementations/ChromaImplementation.cs
+++ b/src/Colore/Implementations/ChromaImplementation.cs
@@ -373,13 +373,20 @@ namespace Colore.Implementations
         public bool HandleMessage(IntPtr handle, int msgId, IntPtr wParam, IntPtr lParam)
         {
             if (!_registered)
-                throw new InvalidOperationException("Register must be called before event handling can be performed.");+            {
+                Log.Warn($"{nameof(HandleMessage)} called without event handling being registered");
+
+                return false;
+            }

             if (handle != _registeredHandle)
             {
-                throw new ArgumentException(
-                    "The specified window handle does not match the currently registered one.",
-                    nameof(handle));
+                Log.Warn(
+                    $"Unexpected handle passed to {nameof(HandleMessage)}. Expected 0x{{0}} but was 0x{{1}}",
+                    _registeredHandle.ToString("X"),
+                    handle.ToString("X"));
+
+                return false;
             }

             if (msgId != Constants.WmChromaEvent)
poveden commented 4 years ago

@Sharparam Sorry for my late response. 😛

I'd actually drop the log calls. You want event pumps to be as fast as possible. By the way Windows works, any delay here will delay all the messages that come after (this is the UI thread, after all). The logic is simple: If the message is not addressed to you, just return false.

That would enable people to check it if they have access to the Win32 event system.

I don't understand what you mean by this. Whoever links to Colore will do so either from a Win32 window (which needs an event pump) or from something else (which doesn't have an event pump). They should know in which context they are running.

This HandleMessage() method will only be called in the context of an event pump. If that pump doesn't receive any messages is probably because the app is not running in a Win32 window context (which would make having a pump useless in the first place).

I'd say that you leave it as is, unless you

Hope that it helps.

Sharparam commented 3 years ago

As noted in #274 this behaviour is now documented in the getting started guide. Users need to create an artifical delay after creating the Chroma instance to allow the internal SDK to finish initializing, or wait for the DeviceAccess event to be generated with Granted set to true.