Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WinRT method throws RaiseFailFastException #110

Open
azimuthdeveloper opened this issue May 29, 2022 · 9 comments
Open

WinRT method throws RaiseFailFastException #110

azimuthdeveloper opened this issue May 29, 2022 · 9 comments

Comments

@azimuthdeveloper
Copy link

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
Copy link

@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
Copy link

There is a second way it can fail in the same method, which I wrote up here: #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
Copy link
Collaborator

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

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

"writeValue" -> {
val deviceId = call.argument<String>("deviceId")!!
val service = call.argument<String>("service")!!
val characteristic = call.argument<String>("characteristic")!!
val value = call.argument<ByteArray>("value")!!
val gatt = knownGatts.find { it.device.address == deviceId }
?: return result.error("IllegalArgument", "Unknown deviceId: $deviceId", null)
val writeResult = gatt.getCharacteristic(service to characteristic)?.let {
it.value = value
gatt.writeCharacteristic(it)
}
if (writeResult == true)
result.success(null)
else
result.error("Characteristic unavailable", null, null)
}

@kinyoklion
Copy link

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

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

"writeValue" -> {
val deviceId = call.argument<String>("deviceId")!!
val service = call.argument<String>("service")!!
val characteristic = call.argument<String>("characteristic")!!
val value = call.argument<ByteArray>("value")!!
val gatt = knownGatts.find { it.device.address == deviceId }
?: return result.error("IllegalArgument", "Unknown deviceId: $deviceId", null)
val writeResult = gatt.getCharacteristic(service to characteristic)?.let {
it.value = value
gatt.writeCharacteristic(it)
}
if (writeResult == true)
result.success(null)
else
result.error("Characteristic unavailable", null, null)
}

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
Copy link
Collaborator

Sunbreak commented May 31, 2022

Feell free to take advantage of messageConnector/message_connector_ channel:

  • Android:

messageConnector = BasicMessageChannel(flutterPluginBinding.binaryMessenger, "quick_blue/message.connector", StandardMessageCodec.INSTANCE)

  • iOS:

let messageConnector = FlutterBasicMessageChannel(name: "quick_blue/message.connector", binaryMessenger: registrar.messenger())

  • macOS:

let messageConnector = FlutterBasicMessageChannel(name: "quick_blue/message.connector", binaryMessenger: registrar.messenger)

  • Windows:

auto message_connector_ =
std::make_unique<flutter::BasicMessageChannel<EncodableValue>>(
registrar->messenger(), "quick_blue/message.connector",
&flutter::StandardMessageCodec::GetInstance());

@azimuthdeveloper
Copy link
Author

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
Copy link

@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: f6d26f1

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
Copy link
Collaborator

Sunbreak commented Jun 6, 2022

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

result is

  • a Java object on Android
  • a ObjC block on iOS
  • a Swift lambda object on macOS
  • a unique_ptr to MethodResult C++ object

Impl on Web/Linux are pure Dart

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

@Sunbreak
Copy link
Collaborator

Sunbreak commented Jun 6, 2022

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) -- microsoft/cppwinrt#676

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants