Bonding extensions

[midoragh]

This pull request provides bonding extensions for Android (and dummies for Apple and Windows).
IDevice provides property BondState and methods CreateBond() and ForgetBond().
IAdapter provides event DeviceBondStateChanged and property BondedDevices.

[midoragh]

This PR is linked to issue #690.

[midoragh]

I solved my issue that after CreateBond() or ForgetBond() connecting to the device failed because the device no longer had a reference to the adapter which crashed in OnDeviceStateChanged.
For this I had to provide the adapter to BondStatusBroadcastReceiver which uses it now to create a new device for the event handler.
What I do not understand is why this device is not only used for the event but also has side-effects to the connection? I can not see in new Device or DeviceBase that the device is added to a list.

[janusw]

What I do not understand is why this device is not only used for the event but also has side-effects to the connection? I can not see in new Device or DeviceBase that the device is added to a list.

Not sure I understand the question, but in any case you say the problem is solved, right?

[janusw]

LGTM. Looking through the code, I don't see any issues. I assume you have tested this to a certain extent?

Just one minor comment ...

[midoragh]

What I do not understand is why this device is not only used for the event but also has side-effects to the connection? I can not see in new Device or DeviceBase that the device is added to a list.

Not sure I understand the question, but in any case you say the problem is solved, right?

Yes, but I do not really understand why.
The broadcaster creates locally new devices used for the event. The event does not require the the device has an adapter.
But if I do not set the adapter in this local device then a following connection to the device will fail. So it looks like that the device is not used only locally but also later. Overall the device handling in plugin.ble seem to have side.effects which I do not understand.

[midoragh]

And yes, tested the whole functionality with two devices.

[janusw]

What I do not understand is why this device is not only used for the event but also has side-effects to the connection? I can not see in new Device or DeviceBase that the device is added to a list.

Not sure I understand the question, but in any case you say the problem is solved, right?

Yes, but I do not really understand why. The broadcaster creates locally new devices used for the event. The event does not require the the device has an adapter. But if I do not set the adapter in this local device then a following connection to the device will fail. So it looks like that the device is not used only locally but also later. Overall the device handling in plugin.ble seem to have side.effects which I do not understand.

It seems that DeviceBase.Adapter is primarily required for disconnecting from the device when it is disposed. If that disconnect fails (the crash you mentioned is probably a NullReferenceException), that might be the reason why a subsequent connect fails, I guess ...

[janusw]

What I do not understand is why this device is not only used for the event but also has side-effects to the connection? I can not see in new Device or DeviceBase that the device is added to a list.

Not sure I understand the question, but in any case you say the problem is solved, right?

Yes, but I do not really understand why. The broadcaster creates locally new devices used for the event. The event does not require the the device has an adapter. But if I do not set the adapter in this local device then a following connection to the device will fail. So it looks like that the device is not used only locally but also later. Overall the device handling in plugin.ble seem to have side.effects which I do not understand.

It seems that DeviceBase.Adapter is primarily required for disconnecting from the device when it is disposed. If that disconnect fails (the crash you mentioned is probably a NullReferenceException), that might be the reason why a subsequent connect fails, I guess ...

In this context, what I'm actually not completely sure about is whether it is such a good idea for Adapter.DeviceBondStateChanged to return a Device object that is disconnected upon disposal (somewhat depends on its usage in the event handler, which we cannot predict).

Since one can also get another Device object (for the same physical device), e.g. via Adapter.BondedDevices, one can easily run into situations where a double disconnection is attempted, or where ones disconnects prematurely (and without intention).

One step towards solving this might be to keep a permanent list of bonded devices in AdapterBase (analogous to the discovered and connected devices), instead of just providing a getter method that returns a new Device object on every invocation.

@midoragh What do you think about this topic? Do you agree with my ideas?

[midoragh]

What I do not understand is why this device is not only used for the event but also has side-effects to the connection? I can not see in new Device or DeviceBase that the device is added to a list.

Not sure I understand the question, but in any case you say the problem is solved, right?

Yes, but I do not really understand why. The broadcaster creates locally new devices used for the event. The event does not require the the device has an adapter. But if I do not set the adapter in this local device then a following connection to the device will fail. So it looks like that the device is not used only locally but also later. Overall the device handling in plugin.ble seem to have side.effects which I do not understand.

It seems that DeviceBase.Adapter is primarily required for disconnecting from the device when it is disposed. If that disconnect fails (the crash you mentioned is probably a NullReferenceException), that might be the reason why a subsequent connect fails, I guess ...

In this context, what I'm actually not completely sure about is whether it is such a good idea for Adapter.DeviceBondStateChanged to return a Device object that is disconnected upon disposal (somewhat depends on its usage in the event handler, which we cannot predict).

Since one can also get another Device object (for the same physical device), e.g. via Adapter.BondedDevices, one can easily run into situations where a double disconnection is attempted, or where ones disconnects prematurely (and without intention).

One step towards solving this might be to keep a permanent list of bonded devices in AdapterBase (analogous to the discovered and connected devices), instead of just providing a getter method that returns a new Device object on every invocation.

@midoragh What do you think about this topic? Do you agree with my ideas?

GetBondedDevice is similar to GetSystemConnectedOrPairedDevices (at the end it's just the paired part of this method).
In general I'm not happy with the handling of devices. There are too many places calling new Device().
From my point of view there should always exist just one device for a native device..
The system tracks the advertising or connected devices and remembers them. It knows the current bonding state and handles the pairing after CreateBond by broadcasting a pairing request.

Only the OS knows about the bonded devices. So if you startup you would have to call GetBondedDevices once and than update this list on BondStateChanged. This could be done.

[midoragh]

What I do not understand is why this device is not only used for the event but also has side-effects to the connection? I can not see in new Device or DeviceBase that the device is added to a list.

Not sure I understand the question, but in any case you say the problem is solved, right?

Yes, but I do not really understand why. The broadcaster creates locally new devices used for the event. The event does not require the the device has an adapter. But if I do not set the adapter in this local device then a following connection to the device will fail. So it looks like that the device is not used only locally but also later. Overall the device handling in plugin.ble seem to have side.effects which I do not understand.

It seems that DeviceBase.Adapter is primarily required for disconnecting from the device when it is disposed. If that disconnect fails (the crash you mentioned is probably a NullReferenceException), that might be the reason why a subsequent connect fails, I guess ...

it never crashed on disconnect but in
case ProfileState.Connected
on access to to the adapter.
So I called CreateBond and did the bonding via UI. After that I called Connect and it crashed.
But if I set the adapter in BondingStateChanged to a new device for some reason the adapter is also available in the device used in ConnectionChange and the crash does not happen.

[janusw]

It seems that DeviceBase.Adapter is primarily required for disconnecting from the device when it is disposed. If that disconnect fails (the crash you mentioned is probably a NullReferenceException), that might be the reason why a subsequent connect fails, I guess ...

it never crashed on disconnect but in case ProfileState.Connected on access to to the adapter.

In GattCallback.OnConnectionStateChange you mean? Lines 107 and below?

So I called CreateBond and did the bonding via UI. After that I called Connect and it crashed.

Yeah, that is understandable. For every Device object, a corresponding GattCallback is constructed (and it gets the Adapter passed on). So if Device.Adapter is null, then also GattCallback._adapter is null and will cause a crash. It's simply not a good idea to construct a Device object without Adapter (but that is already fixed by now).

But if I set the adapter in BondingStateChanged to a new device for some reason the adapter is also available in the device used in ConnectionChange and the crash does not happen.

Well, if you re-use the Device object that you got from the bonding-change event for connecting subsequently, then it's not surprising that it avoids the crash.

But I assume further problems arise, if you don't reuse the Device object, but get it from some other place, e.g. from Adapter.DiscoveredDevices or similar. You have two different objects then, and disposing one of them will probably cause a disconnection (even if you did not intend that).

Note: The Device object from the bonding-change event is usually already the second one that is created for the same physical device, since the bonding change was probably already triggered by a call to Device.CreateBond or Device.ForgetBond, which must have happened on a different Device object.

I think some action is required to address this issue. We need to avoid creating new Device objects that are not somehow linked to the Adapter object. Instead of creating a new device, BondStatusBroadcastReceiver.OnReceive should better look it up from the Adapter (probably from a to-be-created BondedDevices list or dictionary).

[janusw]

@midoragh What do you think about this topic? Do you agree with my ideas?

GetBondedDevice is similar to GetSystemConnectedOrPairedDevices (at the end it's just the paired part of this method). In general I'm not happy with the handling of devices. There are too many places calling new Device(). From my point of view there should always exist just one device for a native device..

I agree. For the connected devices there is already AdapterBase.ConnectedDeviceRegistry, which is supposed to address this.

But good that you mention GetSystemConnectedOrPairedDevices: This indeed fails to use the registry and creates new Device objects on every invocation, which is a problem and should be changed.

Only the OS knows about the bonded devices. So if you startup you would have to call GetBondedDevices once and than update this list on BondStateChanged. This could be done.

Yes, probably it would make sense to add a BondedDevicesRegistry to AdapterBase (analogous to DiscoveredDevicesRegistry and ConnectedDeviceRegistry). If possible, new entries in the registry should be re-used from the DiscoveredDevicesRegistry.

@midoragh Would you be willing/interested to include this in this PR? (If not, I could try to handle it in a follow-up PR.)

[midoragh]

Well, if you re-use the Device object that you got from the bonding-change event for connecting subsequently, then it's not surprising that it avoids the crash.
No I used the device from a rescan to connect.
But anyway I guess the Adapter needs a kind of DeviceManager which holds all known NativeDevices from all kind of sources.
In the meantime you proposed to create a BondedDevicesRegistry but I would propose to use just one DevicesRegistry which would be the source for all different kind of device lists.
I guess I could spent some time on this, but I need to figure out first why I can not use the BLE.Client in Debug Mode.

[janusw]

But anyway I guess the Adapter needs a kind of DeviceManager which holds all known NativeDevices from all kind of sources.
In the meantime you proposed to create a BondedDevicesRegistry but I would propose to use just one DevicesRegistry which would be the source for all different kind of device lists.

Yeah, might be an option. I was also thinking about a single device registry, but I couldn't make up my mind yet whether it would be preferable over three distinct ones. We can still think this through some more ...

I guess I could spent some time on this

That would be awesome.

I was actually planning to tag another beta release soon, in order to make the recent API additions available to a larger audience. I guess I'll just do that tonight. This PR can be merged afterwards, then we can work on the device cleanup in a follow-up PR and publish another beta soon after that (in the time frame of about a month or so).

but I need to figure out first why I can not use the BLE.Client in Debug Mode.

Haven't tried that recently, but it certainly used to work at some point.

I think it would be a great idea to create a proper MAUI sample app as well, in addition to the old Xamarin samples we have, but I'm not sure if I'll have any time to deal with that soon ...

[midoragh]

I was actually planning to tag another beta release soon, in order to make the recent API additions available to a larger audience. I guess I'll just do that tonight. This PR can be merged afterwards, then we can work on the device cleanup in a follow-up PR and publish another beta soon after that (in the time frame of about a month or so).

That's fine for me. As long as the bonding extension will be added later ;-). In the moment I'm still doing extensions and fix in 2.1.2 and port them later to 3.0. After bonding has been included I would like to add Pair
/ing Extensions. If someone calls CreateBond then Android will broadcast a PairingRequest and you should be able to handle some stuff like setting the PairingKey. The Android documentation is limited but I guess I should be able to handle it doing some tests.
No idea about Maoi in the moment, but I have to end of the year...

[midoragh]

I did more test regarding CreateBond and there is an issue in the case that the device is not connected and CreateBond is called.
In the not connected case CreateBond connects to the device to handle the pairing but it doesn't disconnect after a successful pairing, which means that the device no longer does advertising. The device state in this situation is Limited, which means the device is connected but there is no gatt callback to talk to the device. I have to switch off/on BLE to disrupt the connection and enter the disconnected state.
Has anyone an idea how to leave this limited state programmatically?

[MDale-RAC]

Any updates to this?

[janusw]

I did more test regarding CreateBond and there is an issue in the case that the device is not connected and CreateBond is called. In the not connected case CreateBond connects to the device to handle the pairing but it doesn't disconnect after a successful pairing, which means that the device no longer does advertising. The device state in this situation is Limited, which means the device is connected but there is no gatt callback to talk to the device. I have to switch off/on BLE to disrupt the connection and enter the disconnected state. Has anyone an idea how to leave this limited state programmatically?

Unfortunately I don't. Is this a blocker, or should we just merge this PR after all, @midoragh?

[midoragh]

Unfortunately I don't. Is this a blocker, or should we just merge this PR after all, @midoragh?

Sorry, I'm really busy with an other project which needs approval (ETA in August).

The binding state works fine in my tests but CreateBond has issues. In the moment I guess there is an issue with the device lists in the plugin.ble. There is something not consistent. I wouldn't use it for a production release of an app.
I need it in one app where I can get the passkey from the app and use the mechanism to bond with out UI. So there is a good chance that I will continue to work on it. Just a question of priorities.

[janusw]

Thanks for the feedback!

The point is: We should really publish a final version 3.0 soon, and we need to decide if this PR should still be part of it. I would like to publish the release no later than end of August, and this PR is one of the last open items. So, yes, it's a question of priority and timing.

Since there are some open issues, I don't think this PR should be included in 3.0 as is. That basically leaves us with to options:

1. Publish 3.0 now, without this PR. After that, work on the remaining issues in this PR and include it in an upcoming release 3.1. Gladly this PR only extends the API, but does not break the existing API. 3.1 could be released by the end of the year or so.
2. Fix the remaining issues in the coming weeks, and publish 3.0 in August with this PR included. I can help with cleaning up the device registries, but I'd certainly need some support from you, @midoragh, if we take this route.

Any opinions about which way we should go? Which one sounds more reasonable to you, @midoragh ?

[midoragh]

I would propose to remove the methods CreateBond and ForgetBond in the moment and merge it to 3.0. Just the bonding state and event helps in some tools. We can later check if we can solve it later.

[janusw]

I would propose to remove the methods CreateBond and ForgetBond in the moment and merge it to 3.0. Just the bonding state and event helps in some tools. We can later check if we can solve it later.

Sounds like a reasonable suggestion. Could you maybe open a new PR that does this? (Or modify this one?) I will then merge it.

[janusw]

Note that there even is a new PR related to bonding now (#724). @midoragh Maybe you can cross-check this to see if there is anything it does better than yours?

It seems there is quite some interest in this features, so maybe it would be a good idea to fully get it into 3.0 after all ...

[midoragh]

I compared a bit this with the other bonding solution. The main difference seams to be that it provides BondAsync as a method which calls CreateBond and waits for the result. I expect similar issues. At the end I like to support more, like the pairing request which would allow to bond without ui if the pass key is available.