thijse / Arduino-CmdMessenger

CmdMessenger Communication library for Arduino & .NET
Other
208 stars 87 forks source link

Unity Compatibility #18

Closed Palatis closed 7 years ago

Palatis commented 7 years ago
  1. remove Microsoft.CSharp reference
  2. remove System.Windows.Forms reference
  3. backport SpinWait class
  4. use Thread.Sleep(0) instead of Thread.Yield()
Palatis commented 7 years ago

removing System.Windows.Forms requires the removal of ControlToInvokeOn...

Palatis commented 7 years ago

If you feel like the commit is too big, and you want me to break them into smaller commits, please instruct me how you would like it to be done.

valkuc commented 7 years ago

I'm agree that it's better to remove dependency to WinForms. Such utility library like CmdMessenger should now depend on UI libs. But I'm confused with all other changes to code. Purpose of adding call to ToArray()? Replacing Yield with Sleep? SpinWait class that is not used? Anyway, final word is for @thijse .

Palatis commented 7 years ago

because, we don't have Thread.Yield() and SpinWait class in .Net 3.5. and string.Join() doesn't support List<string> in .Net 3.5, either.

i have no idea how to achieve something like

#if NET_FRAMEWORK >= 4
Thread.Yield();
#else
Thread.Sleep(0);
#endif

so I had to

  1. replace Thread.Yield() with Thread.Sleep(0). however, Thread.Yield() should give better performance on platform those support it.
  2. "backport" the SpinWait class, .Net 4 might have native support on this to achieve better performance, but this is a version that "just work".
  3. ToArray()... I dunno, I had to add that call for this thing to compile (against .Net 3.5).
valkuc commented 7 years ago

I don't see SpinWait usages in you commit. Purpose for adding it?

Palatis commented 7 years ago

its used everywhere... https://github.com/thijse/Arduino-CmdMessenger/search?utf8=%E2%9C%93&q=SpinWait

there's no System.Threading.SpinWait class in .Net 3.5, so we had to have our own so it will use <same_namespace>.SpinWait instead of System.Threading.SpinWait.

valkuc commented 7 years ago

Well, CmdMessender is written to target .NET4 and I would not backport it to 3.5 at lease because your SpinWait implementation != implementation in .NET. This can cause regression. So I definetelly don't want to merge SpinWait and Thread.Yield changes. Changes related to WinForms looks like reasonable. @thijse , what do you say?

thijse commented 7 years ago

I agree that it will be cleaner to remove the Winforms dependency. I added it as a convenience function to keep the samples as simple as possible. We can provide a helper function outside of the CmdMessenger library, something like

using System.ComponentModel;
using System.Windows.Forms;

namespace Utils
{
    public static class InvokeUtils
    {
        public static void InvokeIfRequired(this ISynchronizeInvoke invokableObject,
                                         MethodInvoker action)
        {
            if (invokableObject.InvokeRequired) {
                var args = new object[0];
                invokableObject.Invoke(action, args);
            } else {
                action();
            }
        }
    }
}
Palatis commented 7 years ago

SpinWait impl in .Net 4 has a timeout check, but we're not using it.

i'm looking for ways to guard Thread.Sleep(0) and SpinWait so it use framework impl when it's available. something like

#if NET_FRAMEWORK >= 4
System.Threading.SpinWait.SpinUntil(func, timeout);
#else
while (!func.Invoke() && !timed_out);
#endif

the weird thing is that we have

if (android.os.Build.VERSION.SDK_INT>=11)
  do_something();
else
  do_something_else();

in android but no equivalent stuff in .Net...

thijse commented 7 years ago

Btw, why do we need Unity compatibility? I'm very interested to hear what you're working on!

Palatis commented 7 years ago

I'm working on a unity game project that use arduino as key input and controlling LEDs. :-) instead of regular keyboard an mouse we want to have some joystick and large buttons - all on one board.

I choose CmdMessenger because it's got C# support, Unity also supports MonoBehavior as "Scripting" which you can write C# codes directly.

I'm sending this PR because

  1. I guess some other people might need this, too. (there must be some guy who also wants to control an arduino inside of an Unity app, too, right?)
  2. the codes are done, anyway...
thijse commented 7 years ago

@valkuc I agree, I like the idea of removing the WinForms dependency, but I prefer to not implement NET 3.5 backport. Of course, when have no problem at all with you making a branch for this specific purpose.

Palatis commented 7 years ago

okay, i'll reformat the patch to

  1. remove unnecessary dependencies (like Linq stuff, which is not used by the lib)
  2. remove dep to System.Windows.Forms.

and leave .Net 3.5 backport in a branch.

however we might just not need to have the backport, since Unity 5.6 beta is starting to target .Net (Mono) 4.6, anyway.

Palatis commented 7 years ago

I'm experiencing some other weird problems with Unity, too. the received messages are dropped at a rate for no reason... like 4 out of 5...

may post a fix (or workaround) when I sort it out.