onCentralConnected() is called without a matching call to onCentralDisconnected()

[akwizgran]

Apologies for opening two issues in quick succession, but I've realised that the issue reported in #153 is not specific to connection-oriented channels: it also happens when using plain GATT.

The issue happens when two devices are operating in both peripheral mode and central mode. When device A (acting as central) connects to device B (acting as peripheral), device A receives the following callbacks:

1. onConnectingPeripheral()
2. onCentralConnected()
3. onConnectedPeripheral()

Callbacks 1 and 3 are expected, but 2 is unexpected because device A is the central for this connection.

Meanwhile device B receives a callback to onCentralConnected(), as expected.

When device A disconnects from device B by calling BluetoothPeripheral#cancelConnection(), device A receives the following callbacks:

1. onDisconnectingPeripheral()
2. onDisconnectedPeripheral() with status SUCCESS

Both of these are expected, but there's no call to onCentralDisconnected() to match the earlier call to onCentralConnected().

Meanwhile device B does not receive any callbacks. onCentralDisconnected() is not called on either device, and calling BluetoothPeripheralManager#getConnectedCentrals() on either device still returns the other device.

In contrast, if device A is only operating in central mode and device B is only operating in peripheral mode then everything works as expected. When connecting, device A receives the following callbacks:

1. onConnectingPeripheral()
2. onConnectedPeripheral()

Meanwhile device B receives a callback to onCentralConnected(), as expected.

When disconnecting, device A receives the following callbacks:

1. onDisconnectingPeripheral()
2. onDisconnectedPeripheral() with status SUCCESS

Meanwhile device B receives a callback to onCentralDisconnected(), as expected.

So the issue only seems to occur when both devices are operating in central mode. This causes two specific problems:

• Device A receives an unexpected callback to onCentralConnected() when connecting
• Neither device receives a callback to onCentralDisconnected() when disconnecting

I've created a new branch of my testbed app for reproducing this issue:

https://code.briarproject.org/briar/public-mesh-testbed/-/tree/blessed-gatt-central-callback-without-l2cap

The relevant code is in this class:

https://code.briarproject.org/briar/public-mesh-testbed/-/blob/blessed-gatt-central-callback-without-l2cap/app/src/main/java/org/briarproject/publicmesh/bt/BtServiceImpl.java

To reproduce the expected behaviour, tap "start advertising" on one device and "start scanning" on the other, then tap on the discovered device when it appears in the list. The callbacks can be seen in logcat.

To reproduce the unexpected behaviour, tap "start advertising" and "start scanning" on both devices, then tap on the discovered device when it appears in the first device's list.

[akwizgran]

I've narrowed down the conditions for reproducing this issue:

• Device B doesn't have to be operating as a central, only as a peripheral
• Device A doesn't need to be advertising, it only needs to create a BlueoothPeripheralManager

I've pushed another commit to this branch that adds new buttons to the UI for creating the BluetoothCentralManager and BluetoothPeripheralManager without starting scanning/advertising respectively:

https://code.briarproject.org/briar/public-mesh-testbed/-/tree/blessed-gatt-central-callback-without-l2cap

Updated instructions for reproducing the unexpected behaviour:

• On device A, tap "peripheral" to create the BluetoothPeripheralManager, then "start scanning"
• On device B, tap "start advertising"
• When device B appears in device A's list, optionally tap "stop scanning", then tap on the discovered device

The steps for reproducing the expected behaviour are the same, except don't tap "peripheral" on device A.

[weliem]

I think your issue is caused by this line: https://github.com/weliem/blessed-android/blob/master/blessed/src/main/java/com/welie/blessed/BluetoothPeripheralManager.java#L117

So when a connection to a Peripheral happens, it calls 'connect' back to the Central. Of course that shouldn't be there. The reason it is there is because if a bug in Android which causes disconnecting from the Peripheral side to disfunction. See the link in the comments just above it.

You can try removing that line, see if it works better for you.

[akwizgran]

@weliem thanks for your attention to this. I think there are three parts to this issue:

1. When a BluetoothPeripheralManager is created, it registers a BluetoothGattServerCallback that receives callbacks to onConnectionStateChange() for remote peripherals as well as centrals. Arguably the platform should only call the BluetoothGattServerCallback for remote centrals, but unfortunately that's not the case.
2. When the BluetoothPeripheralManager receives a callback to onConnectionStateChanged(), it assumes the call refers to a remote central. I can't see any unhorrible way for the BluetoothPeripheralManager to know whether the remote device is actually a central or not, but perhaps you can see one?
3. The BluetoothPeripheralManager calls BluetoothGattServer#connect() at line 117, which as you explained is a necessary workaround when the remote device is really a central. But when it's not, this call creates a redundant connection to the remote peripheral, which isn't cleaned up when we disconnect from the peripheral, leaving both devices thinking that they're still connected to a remote central.

I tried commenting out line 117 as you suggested. This prevented the redundant connection from being created, but it didn't solve the issue that device A thought it was connected to device B as both a peripheral and a central for the duration of the connection. More importantly, as you explained, line 117 is there for a reason and removing it will cause connections to be dropped by the platform.

I found another workaround, which is to ensure that whenever we disconnect from a peripheral, we also check whether the BluetoothPeripheralManager thinks the same device is a connected central, and if so, disconnect from the central. Likewise, whenever we disconnect from a central, check whether the BluetoothCentralManager thinks the same device is a connected peripheral, and if so, disconnect from the peripheral. This workaround has the effect of cleaning up the redundant connection. However, we still need to bear in mind that whenever we call BluetoothPeripheralManager#getConnectedCentrals(), the returned set of devices includes all connected peripherals as well as centrals. And there may be other quirks that I haven't thought of, caused by the fact that BluetoothPeripheralManager is tracking all remote peripherals as if they were centrals.

So although I've found a workaround, I hope you might be able to see a way that we can avoid this issue altogether by ignoring any calls to BluetoothGattServerCallback#onConnectionStateChange() that refer to remote peripherals rather than centrals. Do you have any thoughts on how we might do that?

[weliem]

Thank you for the extended writeup. I understand the issue I think and Android is messing stuff up here. I will think about any other workarounds although I don't see any obvious ones....

[akwizgran]

I wonder if it might be possible to solve this issue through coordination between the BluetoothCentralManager and BluetoothPeripheralManager: when the BluetoothPeripheralManager received a callback to onConnectionStateChanged(), it would query the BluetoothCentralManager to see whether the callback referred to a peripheral that the BluetoothCentralManager was currently connecting or connected to. If so, the BluetoothPeripheralManager would ignore the callback. If not, it would handle the callback like it does now.

(I say "connecting or connected" because I'm not sure if we can guarantee whether onConnectionStateChanged() gets called before or after BluetoothPeripheral.InternalCallback#connected(). If we can be sure of that then maybe we only need to consider peripherals that are connecting, not connected.)

The tricky part here is how the BluetoothPeripheralManager instance gets a reference to the current BluetoothCentralManager instance (if any).

One possible approach would be to convert these classes into singletons: the app would get an instance via a factory method rather than constructing it directly, the factory would keep a reference to the singleton instance, and the instance's close() method would clear the singleton reference. The BluetoothPeripheralManager instance would then be able to look up the current BluetoothCentralManager instance (if any) via the factory when handling the onConnectionStateChanged() callback.

Another possibility would be for the app to pass a nullable BluetoothCentralManager argument to the BluetoothPeripheralManager constructor. This would allow P2P apps to work around this issue while having minimal impact on non-P2P apps that only want to act as centrals or peripherals.

Any thoughts?

[weliem]

Yes, something like that might work. It would need some experimentation to be sure. What still bothers me the most is if we can reliably detect whether a central or peripheral is connecting. Especially when a device takes both the central AND peripheral role....

[weliem]

Just released version 2.4.1. that contains a possible fix for your issue.

You now need to tell the PeripheralManager who the Central is and then I check every connected event to see if it is a peripheral or central.

So do peripheralManager.setCentralManager(central) in your code when you create them.

[akwizgran]

Fantastic, thank you! This fixes the issue for us.