woodemi / quick_blue

A cross-platform (Android/iOS/macOS/Windows/Linux) BluetoothLE plugin for Flutter
134 stars 69 forks source link

WinRT method throws RaiseFailFastException #110

Open azimuthdeveloper opened 2 years ago

azimuthdeveloper commented 2 years ago

Sometimes, quick_blue will throw an error in the WinRT code. This causes the app to crash to desktop.

I've managed to get logging working with my app, so I've got a really good stacktrace of the error here: https://sentry.io/share/issue/07424e3a35e147db9ec78707d2e54678/

Unfortunately, I lack the C++ knowledge to work out why this is happening, but I thought maybe @Sunbreak or @kinyoklion would be able to use this stacktrace to possibly debug and fix the issue, and make quick_blue more stable for all 😊

kinyoklion commented 2 years ago

@azimuthdeveloper To me this looks like one of the exceptions I am trying to prevent in the PR I have up. (I am not sure, what the contribution process is.)

auto writeDescriptorStatus = co_await gattCharacteristic.WriteClientCharacteristicConfigurationDescriptorAsync(descriptorValue);

Basically at this co_await we are suspending some code to resume once the underlying windows operation completes. Then that operation does complete it checks the result of the underlying call. If that call failed, then it will throw an exception. That exception isn't handled and then the application will terminate. The stowed exception type is a wrapper from the fire_and_forget declaration of those async methods.

My understanding is that the only real way to handle it is a try/catch like in that PR. If the state of the system changes before that call returns, like a device loses connectivity, then it can throw.

kinyoklion commented 2 years ago

There is a second way it can fail in the same method, which I wrote up here: https://github.com/woodemi/quick_blue/issues/104 I also address that in my PR. It makes it more stable, but then there is a new problem. Currently you cannot really tell from the flutter side if an operation was successful or not. So if it does fail to write to the characteristic, then you don't actually know if you need to retry. (But at least it won't crash).

Sunbreak commented 2 years ago

Any proposal on notifying Dart side ASYNChronously that gattCharacteristic.WriteClientCharacteristicConfigurationDescriptorAsync failed?

Now we only notify SYNChronously Android's failure on writeValue:

https://github.com/woodemi/quick_blue/blob/973784ed097f61952e652a2afe093f83f7f45c40/quick_blue/android/src/main/kotlin/com/example/quick_blue/QuickBluePlugin.kt#L140-L155

kinyoklion commented 2 years ago

Any proposal on notifying Dart side ASYNChronously that gattCharacteristic.WriteClientCharacteristicConfigurationDescriptorAsync failed?

Now we only notify SYNChronously Android's failure on writeValue:

https://github.com/woodemi/quick_blue/blob/973784ed097f61952e652a2afe093f83f7f45c40/quick_blue/android/src/main/kotlin/com/example/quick_blue/QuickBluePlugin.kt#L140-L155

I do not have a proposal yet. I can look into it next weekend though and see if I can come up with something. I am new to Flutter, so I need to do some more learning about method channels. It looks like they support async messaging, but I need to learn more about it and do some experimentation.

Sunbreak commented 2 years ago

Feell free to take advantage of messageConnector/message_connector_ channel:

https://github.com/woodemi/quick_blue/blob/973784ed097f61952e652a2afe093f83f7f45c40/quick_blue/android/src/main/kotlin/com/example/quick_blue/QuickBluePlugin.kt#L37

https://github.com/woodemi/quick_blue/blob/973784ed097f61952e652a2afe093f83f7f45c40/quick_blue/ios/Classes/SwiftQuickBluePlugin.swift#L44

https://github.com/woodemi/quick_blue/blob/973784ed097f61952e652a2afe093f83f7f45c40/quick_blue_macos/macos/Classes/QuickBlueMacosPlugin.swift#L44

https://github.com/woodemi/quick_blue/blob/973784ed097f61952e652a2afe093f83f7f45c40/quick_blue_windows/windows/quick_blue_windows_plugin.cpp#L176-L179

azimuthdeveloper commented 2 years ago

That's okay @kinyoklion , I'm new to C++, only because of Windows integration for Flutter am I getting into it haha.

I think with me having a production-ish app using this package and good crash reporting set up, we can hopefully work towards the stability of quick_blue overall, and over time.

kinyoklion commented 2 years ago

@Sunbreak

It seems like the ideal for a user would be if they could call a method and handle errors from that method. The way the method channels work it should be possible to provide a return later in the process from within an async method. For the windows implementation, because it uses coroutines (stays on the same thread), this should just be a matter of moving the result to the async method and then calling Success/Error once we know the result.

Then a user can use chaining .then/.catchError or they could use try/catch with await. (We could also change the signatures from Future<void> to a Future<bool> or something)

I put a quick example commit together: https://github.com/woodemi/quick_blue/commit/f6d26f1e2fc1a7c1b98fb5c53b9a70fefa64b242

Output for the code in the example

flutter: _handleConnectionChange 234701352743926, connected
flutter: Got an error setting notifiable PlatformException(Could not access characteristic: 57444e02-ba5e-f4ee-5ca1-eb1e5e4b1ce0, null, null, null)
flutter: _handleConnectionChange 234701352743926, disconnected

I've not looked at the mac/ios code yet, but typically callbacks for results are on the main thread, so maintaining a reference to the result object and then calling the result later should work similarly.

Anyway, I want thoughts before I work through it too much more.

Sunbreak commented 2 years ago

Yep. The main concern is multi-thread handling of result object

result is

Impl on Web/Linux are pure Dart

If you're sure about the multi-thread handling, I'm OK with that PR

Sunbreak commented 2 years ago

because it uses coroutines (stays on the same thread)

I'm not sure about C++ coroutines, especially experimental coroutine support of C++/WinRT patched onto C++ 17

Using the /await command line (which is forced in various places by the C++/WinRT project files) -- https://github.com/microsoft/cppwinrt/issues/676