getsenic / nuimo-windows

Library for Windows 10 platforms to connect and communicate with Nuimo controllers made by Senic
MIT License
23 stars 8 forks source link

NuimoSDK cannot be used for headless apps due to DispatchOnMainAsync #2

Closed hansmbakker closed 8 years ago

hansmbakker commented 8 years ago

I want to create a smart home application and I'm doing that using Windows 10 IoT Core.

I'm developing a Background app. Such apps are headless (so they do not have a CoreApplication.MainView).

When calling INuimoController.ConnectAsync(), the NuimoSDK uses DispatchOnMainAsync() to perform some tasks. However, headless apps do not have a MainView and the DispatchOnMainAsync() function causes a COMException when ConnectionState is set to Connecting.

This means that I cannot use the NuimoSDK for my app because after discovering an INuimoController I cannot connect to it.

private async void DispatchOnMainAsync(DispatchedHandler dispatchedHandler)
{
    await CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, dispatchedHandler);
}

System.Runtime.InteropServices.COMException

A method was called at an unexpected time.

Could not create a new view because the main window has not yet been created

Stacktrace:

   at Windows.ApplicationModel.Core.CoreApplication.get_MainView()
   at NuimoSDK.NuimoBluetoothController.<DispatchOnMainAsync>d__43.MoveNext()
hansmbakker commented 8 years ago

Also I'm not sure why the Dispatcher is used at all. If I understand it correctly, the Dispatcher is used to run code on the UI thread.

That would be something that UI applications consuming the NuGet package should do, not the NuGet package itself?.

ghost commented 8 years ago

Hi wind-rider, thanks for your feedback. We decided to use DispatchOnMainAsync() because as you have said it is very helpful for UI applications. We integrated it into the SDK to make the SDK more easy to use and to not encumber the user of the SDK with this. However, your point is reasonable. We are working on it. Do you have any suggestions on how to dispatch on a variable thread instead of dispatching on the main thread? Regards

hansmbakker commented 8 years ago

to make the SDK more easy to use

That is a good goal, but here I think educating the user with good examples and reading material is a better solution, rather than making the SDK limited in use.

Anybody who deals with background processes (for example: downloading some data in a background process and updating some chart in the UI) needs to know how to use a dispatcher. That's why I expect users to be able to implement this in their own application. The users of this NuGet package are developers, not end users.

Also, the dispatcher is a property of the UI (since you derive it from MainView) because it is used to perform tasks upon the UI. It belongs to the presentation layer, not the business layer or data layer (I would say NuimoSDK belongs to the data layer) of an application.

However, your point is reasonable. We are working on it.

That sounds nice :) but what do you mean with that?

Here is a nice example of somebody elses Nuimo UWP library: he also puts the Dispatcher code in his UI app, not in his Nuimo library.

Do you have any suggestions on how to dispatch on a variable thread instead of dispatching on the main thread?

Maybe this is an idea https://msdn.microsoft.com/en-us/library/system.windows.threading.dispatcher.currentdispatcher(v=vs.110).aspx but I have not tested it for your purpose. Also I'm in favor of keeping UI code in the UI apps and background code in libraries. If one starts mixing things, then you create dependencies that cause new kinds of trouble (like in this case - not being able to use the NuGet package for headless apps because you created a dependency on the MainView).

larsblumberg commented 8 years ago

Thanks for raising this issue about where to dispatch the event callbacks. While I understand that the SDK shouldn't always dispatch on the UI thread, I like @voelpade's intention to hide Windows Bluetooth API details behind the Nuimo SDK. The SDK should make the integration into any Windows universal application as easy as possible, for both beginners and advanced developers.

The fact that Windows Bluetooth API does not dispatch Bluetooth callbacks onto the thread where initial calls where made from is not necessarily expected by the programmer. That's why the Nuimo SDK needs provide a simple abstraction that hides the programmer from that knowledge. Apps with a UI thread already don't need to care about the fact that Bluetooth callbacks happen on an extra threads since the Nuimo SDK dispatches event handlers on the UI thread by default.

To avoid repetitive code to be written by the SDK user (this is: dispatching to the preferred thread) I suggest to provide a simple facility that allows the programmer to choose to which thread Nuimo callbacks should dispatch to. By default the SDK should continue to dispatch to the UI thread. Programmers of a UI less app (such as @wind-rider) should be equipped with an easy facility to choose another thread.

I suggest to let NuimoController have a property that points to the Dispatcher to dispatch to. It should default to the UI thread. UI less applications can set it to another dispatcher. But there might be better alternatives?

PS: @voelpade has pointed me to this StackOverflow question: http://stackoverflow.com/questions/27851073/why-would-i-bother-to-use-task-configureawaitcontinueoncapturedcontext-false. It however doesn't seem to work for callback handlers since these Tasks are not started directly by the SDK user.

larsblumberg commented 8 years ago

I asked a question on StackOverflow to better understand: http://stackoverflow.com/questions/38758827/does-every-winrt-windows-core-thread-have-a-dispatcher

Inrego commented 8 years ago

This issue is also very relevant for me, since I plan to use the SDK in a Windows Service that will act as a proxy between the Nuimo and other programming languages that are unable to tap into Windows' BLE api's (such as Python). Sort of like the one you've already made for OSX I guess.

larsblumberg commented 8 years ago

@inrego – Happy to apply a change! Do you think NuimoBluetoothController can keep a reference to a SynchronizationContext and then post events into that context?

I imagine that ConnectAsync() gets an optional parameter for the context to post to. If this optional parameter will not be set, we will set it to SynchronizationContext.Current and store inside the NuimoBluetoothController instance. This way Nuimo will always post events to the thread where ConnectAsync() was called – if not specified.

This would fix the problem that Nuimo doesn't post events onto the same thread where the connection was initiated. Thus the SDK hides that detail that BLE events are posted to a different thread. We would transparently deliver them on the specified context.

Does this make sense to you @Inrego, @wind-rider, @voelpade?

Inrego commented 8 years ago

I looked at the changes wind-rider had made, and they looked like how I would've done it. Simply just fire the event without the SDK worrying about which thread/context it's being fired to.

EDIT: Say for example (theoretically) that I have 2 windows. One for current battery %, and one for other events. Some of the events should be handled on 1 thread, and others on another thread. In my opinion, such thread handling should be done by the developers who implement the SDK in their application and not by the SDK itself.

larsblumberg commented 8 years ago

If you have 2 windows, shouldn't they share the same UI thread?

Inrego commented 8 years ago

I wouldn't think so if they're 2 different UI's.

larsblumberg commented 8 years ago

Asked two question on StackOverflow to clarify:

http://stackoverflow.com/questions/38794664/does-every-thread-have-a-synchronizationcontext

http://stackoverflow.com/questions/38794748/does-every-windows-come-with-its-own-dispatcher-or-do-they-share-one-thread

Inrego commented 8 years ago

Well I do believe that .NET Framework 4.5 handles synchronization differently. So if you lock in to the ones used in UWP, you are locking in your SDK to only work with applications using the synchronization method you implemented. I do appreciate the concern to make the SDK as easy to use as possible, you are also locking in the SDK to only work with it in the context you had in mind while developing the SDK.

hansmbakker commented 8 years ago

I really appreciate that you do effort to make the SDK simple to use.

However - first of all I'm not sure whether there actually is a problem that needs to be fixed. Second of all - I hope that fixing this problem does not limit the SDK to usage in a specific way or environment. Third - I hope that it does not introduce new dependencies or other trouble.

The first time I made an UWP app that displayed data from the internet I already walked into needing the Dispatcher. When I googled for information about that there was a LOT of information that explained how to do that. So I do expect any other developers to do the same as apparently it's such a basic skill for UWP programmers.

Your competitor / contributor @Erebos3D gives a clear example in https://github.com/Erebos3D/nuimo-windows-unofficial/blob/master/NuimoTestApp/MainPage.xaml.cs#L168. With a rather clean sample he shows that it is needed to use this technique. When people start working from such a sample they already have the necessary code in place. A notice in the Readme would also be nice.

Therefore I still believe in educating people if Microsoft designed that this is the way how UWP apps should be built (needing the concept of dispatching) rather than oversimplifying this SDK. If not in this SDK then they will encounter it somewhere else.

I prefer to separate software in layers

In my opinion - the Nuimo SDK would belong to the data layer in this model. Trying to solve an UI problem in your layer feels like mixing responsibilities and feels like introducing code in your library that should be in the client app.

Windows desktop programming is new for me (I'm a web developer) - I don't know what a SynchronizationContext is and when starting programming for UWP with background tasks I didn't encounter it yet (but I did encounter Dispatcher - it seems more often used?).

If developers using NuimoSDK need to know what a SynchronizationContext is, for the purpose of preventing to know what a Dispatcher then it means that the problem is only shifted... (forgive me - I don't completely understand what you proposed so I cannot assess whether it is an improvement).

For me, using no dispatching for headless apps and using dispatching for apps with an UI is something that is clear. It might be new for people, but at least there is a lot of information to find about it and there is a clear way of how to implement it. If you ask my opinion then let's follow that pattern, let's keep the library clean and let the end developer implement it in case he needs it.

larsblumberg commented 8 years ago

While I do not recognize the problem to solve as being a UI-only problem I do recognize that the .Net framework is not providing simple facilities to dispatch Bluetooth events to a specific thread. Given that it's the most easiest solution for now to simply move the burden for thread synchronization to the SDK user, we'll remove the entire dispatching from the SDK. This will allow non-UI applications to make use of Nuimo SDK as well.

Thanks a lot to everybody for taking part in this insightful discussion!