Call BluetoothGatt#close() after disconnecting

device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.device.bluetooth;
 
 import android.annotation.TargetApi;
 import android.bluetooth.BluetoothDevice;
 import android.content.Context;
 import android.os.Build;
@@ skipping 40 matching lines @@
                         ChromeBluetoothRemoteGattCharacteristic>();
         mWrapperToChromeDescriptorsMap = new HashMap<Wrappers.BluetoothGattDescriptorWrapper,
                 ChromeBluetoothRemoteGattDescriptor>();
         Log.v(TAG, "ChromeBluetoothDevice created.");
     }
 
     /**
      * Handles C++ object being destroyed.
      */
     @CalledByNative
     private void onBluetoothDeviceAndroidDestruction() {

[scheib, 2016/01/27 00:30:43]

Should close be called in onBluetoothDeviceAndroidDestruction? You mentioned
issues if close is called right after disconnect, would it be a problem at this
location do you think?

[tommyt, 2016/01/28 12:48:22]

Having spent some time today looking at the Android code that handles close
(bta_gattc_deregister in system/bt/bta/gatt/bta_gattc_act.c), it looks like it
should be enough to call only close. The platform code does look like it first
disconnects properly, before deregistering. Why exactly it doesn't work to
replace disconnect with close in disconnectGatt, I'm not sure. It could be that
it works fine from the platform's point of view, but that some state is not
properly tidied up in one of the numerous layers between
ChromeBluetoothDevice.java and bta_gattc_act.c.

I've tested replacing disconnectGatt with if (mBluetoothGatt != null)
mBluetoothGatt.close() at this location, and that feels correct and seems to
work well enough. I might go for that in my next patchset, but I'll do some more
testing first.

[scheib, 2016/01/29 04:50:39]

Ok, just to be clear, I think we should get that patch set on this review and
land what we think will work best in one shot (just in case you were proposing
we land this first and then follow up with another patch).

         disconnectGatt();
         mNativeBluetoothDeviceAndroid = 0;
     }
 
     // ---------------------------------------------------------------------------------------------
     // BluetoothDeviceAndroid methods implemented in java:
 
     // Implements BluetoothDeviceAndroid::Create.
     // 'Object' type must be used because inner class Wrappers.BluetoothDeviceWrapper reference is
     // not handled by jni_generator.py JavaToJni. http://crbug.com/505554
@@ skipping 42 matching lines @@
         // after a device is connected, discoverServices. crbug.com/508648
         return mUuidsFromScan.toArray(new String[mUuidsFromScan.size()]);
     }
 
     // Implements BluetoothDeviceAndroid::CreateGattConnectionImpl.
     @CalledByNative
     private void createGattConnectionImpl(Context context) {
         Log.i(TAG, "connectGatt");
         // autoConnect set to false as under experimentation using autoConnect failed to complete
         // connections.
         mBluetoothGatt =

[scheib, 2016/01/27 00:30:43]

If we already have a mBluetoothGatt this code will replace it and we won't call
close() on it. Should close be called now, or should mBluetoothGatt.connect() be
called instead of creating a new one?

[tommyt, 2016/01/28 12:48:22]

If we wanted to do mBluetoothGatt.connect() instead of creating a new gatt
object here, we'd have to make sure the existing gatt object is not one that has
already been closed.

I vote for doing mBluetoothGatt.close() and creating a new gatt object.

[ortuno, 2016/01/28 16:19:16]

Could you give an example of when this could happen? We should only be calling
this if no connection exists.
+1 for mBluetoothGatt.close(). Calling mBluetoothGatt.connect() auto-connects,
which could lead to different behavior than the first connection.

                 mDevice.connectGatt(context, false /* autoConnect */, mBluetoothGattCallbackImpl);
     }
 
     // Implements BluetoothDeviceAndroid::DisconnectGatt.
     @CalledByNative
     private void disconnectGatt() {
         Log.i(TAG, "BluetoothGatt.disconnect");
         if (mBluetoothGatt != null) mBluetoothGatt.disconnect();
     }
 
     // Implements BluetoothDeviceAndroid::GetDeviceName.
     @CalledByNative
     private String getDeviceName() {
         return mDevice.getName();
     }
 
     // Implements callbacks related to a GATT connection.
     private class BluetoothGattCallbackImpl extends Wrappers.BluetoothGattCallbackWrapper {
         @Override
         public void onConnectionStateChange(final int status, final int newState) {
             Log.i(TAG, "onConnectionStateChange status:%d newState:%s", status,
                     (newState == android.bluetooth.BluetoothProfile.STATE_CONNECTED)
                             ? "Connected"
                             : "Disconnected");
             if (newState == android.bluetooth.BluetoothProfile.STATE_CONNECTED) {
                 mBluetoothGatt.discoverServices();
+            } else if (newState == android.bluetooth.BluetoothProfile.STATE_DISCONNECTED) {
+                mBluetoothGatt.close();
             }
             ThreadUtils.runOnUiThread(new Runnable() {
                 @Override
                 public void run() {
                     if (mNativeBluetoothDeviceAndroid != 0) {
                         nativeOnConnectionStateChange(mNativeBluetoothDeviceAndroid, status,
                                 newState == android.bluetooth.BluetoothProfile.STATE_CONNECTED);
                     }
                 }
             });
@@ skipping 93 matching lines @@
             long nativeBluetoothDeviceAndroid, int status, boolean connected);
 
     // Binds to BluetoothDeviceAndroid::CreateGattRemoteService.
     // TODO(http://crbug.com/505554): Replace 'Object' with specific type when JNI fixed.
     private native void nativeCreateGattRemoteService(long nativeBluetoothDeviceAndroid,
             String instanceId, Object bluetoothGattServiceWrapper);
 
     // Binds to BluetoothDeviceAndroid::GattServicesDiscovered.
     private native void nativeOnGattServicesDiscovered(long nativeBluetoothDeviceAndroid);
 }