fix(bluetoothle): fix typo in BluetoothLeService and migrate to snippets#934
fix(bluetoothle): fix typo in BluetoothLeService and migrate to snippets#934ithinkihaveacat wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Bluetooth Low Energy (BLE) GATT service and activity implementations in both Java and Kotlin. The code reviewer identified several critical issues, including potential GATT connection leaks due to unclosed BluetoothGatt instances, ServiceConnection leaks from missing unbindService calls in onDestroy, control flow bugs where connection attempts proceed after initialization failures, and unsafe null assertions (!!) in the Kotlin activity that could cause runtime crashes.
| public boolean connect(final String address) { | ||
| if (bluetoothAdapter == null || address == null) { | ||
| Log.w(TAG, "BluetoothAdapter not initialized or unspecified address."); | ||
| return false; | ||
| } | ||
| try { | ||
| final BluetoothDevice device = bluetoothAdapter.getRemoteDevice(address); | ||
| // connect to the GATT server on the device | ||
| bluetoothGatt = device.connectGatt(this, false, bluetoothGattCallback); | ||
| return true; | ||
| } catch (IllegalArgumentException exception) { | ||
| Log.w(TAG, "Device not found with provided address. Unable to connect."); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
If connect is called multiple times, the previous BluetoothGatt instance is overwritten without being closed. This leads to a GATT connection leak, which can quickly exhaust the maximum number of simultaneous BLE connections on Android. Call close() before initiating a new connection.
public boolean connect(final String address) {\n if (bluetoothAdapter == null || address == null) {\n Log.w(TAG, "BluetoothAdapter not initialized or unspecified address.");\n return false;\n }\n try {\n final BluetoothDevice device = bluetoothAdapter.getRemoteDevice(address);\n // Close any existing connection to prevent resource leaks\n close();\n // connect to the GATT server on the device\n bluetoothGatt = device.connectGatt(this, false, bluetoothGattCallback);\n return true;\n } catch (IllegalArgumentException exception) {\n Log.w(TAG, "Device not found with provided address. Unable to connect.");\n return false;\n }\n }| fun connect(address: String): Boolean { | ||
| bluetoothAdapter?.let { adapter -> | ||
| try { | ||
| val device = adapter.getRemoteDevice(address) | ||
| // connect to the GATT server on the device | ||
| bluetoothGatt = device.connectGatt(this, false, bluetoothGattCallback) | ||
| return true | ||
| } catch (exception: IllegalArgumentException) { | ||
| Log.w(TAG, "Device not found with provided address. Unable to connect.") | ||
| return false | ||
| } | ||
| } ?: run { | ||
| Log.w(TAG, "BluetoothAdapter not initialized") | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
If connect is called multiple times, the previous BluetoothGatt instance is overwritten without being closed. This leads to a GATT connection leak. Call close() before initiating a new connection.
fun connect(address: String): Boolean {\n bluetoothAdapter?.let { adapter ->\n try {\n val device = adapter.getRemoteDevice(address)\n // Close any existing connection to prevent resource leaks\n close()\n // connect to the GATT server on the device\n bluetoothGatt = device.connectGatt(this, false, bluetoothGattCallback)\n return true\n } catch (exception: IllegalArgumentException) {\n Log.w(TAG, "Device not found with provided address. Unable to connect.")\n return false\n }\n } ?: run {\n Log.w(TAG, "BluetoothAdapter not initialized")\n return false\n }\n }| protected void onPause() { | ||
| super.onPause(); | ||
| unregisterReceiver(gattUpdateReceiver); | ||
| } | ||
| private static IntentFilter makeGattUpdateIntentFilter() { |
There was a problem hiding this comment.
The activity binds to BluetoothLeService in onCreate but never unbinds from it. This causes a ServiceConnection leak when the activity is destroyed. Implement onDestroy and call unbindService.
@Override\n protected void onPause() {\n super.onPause();\n unregisterReceiver(gattUpdateReceiver);\n }\n\n @Override\n protected void onDestroy() {\n super.onDestroy();\n unbindService(serviceConnection);\n bluetoothService = null;\n }|
|
||
| override fun onPause() { | ||
| super.onPause() | ||
| unregisterReceiver(gattUpdateReceiver) |
There was a problem hiding this comment.
The activity binds to BluetoothLeService in onCreate but never unbinds from it. This causes a ServiceConnection leak when the activity is destroyed. Implement onDestroy and call unbindService to clean up resources.
override fun onPause() {\n super.onPause()\n unregisterReceiver(gattUpdateReceiver)\n }\n\n override fun onDestroy() {\n super.onDestroy()\n unbindService(serviceConnection)\n bluetoothService = null\n }| if (!bluetoothService.initialize()) { | ||
| Log.e(TAG, "Unable to initialize Bluetooth"); | ||
| finish(); | ||
| } | ||
| // [END gatt_initialize_activity_java] | ||
| // perform device connection | ||
| // [START gatt_connect_activity_java] | ||
| bluetoothService.connect(deviceAddress); | ||
| // [END gatt_connect_activity_java] |
There was a problem hiding this comment.
If bluetoothService.initialize() fails, finish() is called to destroy the activity, but the method continues executing and attempts to call connect(). Add an early return; statement to prevent further execution.
if (!bluetoothService.initialize()) {\n Log.e(TAG, "Unable to initialize Bluetooth");\n finish();\n return;\n }\n // [END gatt_initialize_activity_java]\n // perform device connection\n // [START gatt_connect_activity_java]\n bluetoothService.connect(deviceAddress);\n // [END gatt_connect_activity_java]| if (!bluetooth.initialize()) { | ||
| Log.e(TAG, "Unable to initialize Bluetooth") | ||
| finish() | ||
| } | ||
| // [END gatt_initialize_activity] | ||
| // perform device connection | ||
| // [START gatt_connect_activity] | ||
| bluetooth.connect(deviceAddress!!) | ||
| // [END gatt_connect_activity] | ||
| } |
There was a problem hiding this comment.
If bluetooth.initialize() fails, finish() is called but the method continues executing and attempts to call connect(). Additionally, using deviceAddress!! is unsafe and can cause a NullPointerException if the activity is started without the expected intent extra. Return early and safely unwrap deviceAddress.
if (!bluetooth.initialize()) {\n Log.e(TAG, "Unable to initialize Bluetooth")\n finish()\n return@let\n }\n // [END gatt_initialize_activity]\n // perform device connection\n // [START gatt_connect_activity]\n deviceAddress?.let { address ->\n bluetooth.connect(address)\n }\n // [END gatt_connect_activity]\n }| if (bluetoothService != null) { | ||
| val result = bluetoothService!!.connect(deviceAddress!!) | ||
| Log.d(TAG, "Connect request result=$result") | ||
| } |
There was a problem hiding this comment.
Avoid using unsafe null assertions (!!) on bluetoothService and deviceAddress. Use safe calls or optional chaining to prevent potential NullPointerException crashes.
bluetoothService?.let { service ->\n deviceAddress?.let { address ->\n val result = service.connect(address)\n Log.d(TAG, "Connect request result=$result")\n }\n }
ithinkihaveacat
force-pushed
the
fix-359501045
branch
2 times, most recently
from
c7852e3 to
a02d6a9
Compare
|
These snippets have been ported directly from https://developer.android.com/develop/connectivity/bluetooth/ble/connect-gatt-server using a tool I'm testing. The comments from gemini-code-assist seem mostly reasonable however I am not a SME and so I'd rather not make any changes if possible. |
ithinkihaveacat
force-pushed
the
fix-359501045
branch
2 times, most recently
from
fc4336f to
439afc5
Compare
- Fix typo 'Return;' -> 'return;' in Java BluetoothLeService. - Migrate inline code samples from connect-gatt-server.md to snippets. - Create BluetoothLeService and DeviceControlActivity in Java and Kotlin. - Add gatt_services_characteristics layout and string resources to enable compilation. Bug: 359501045
ithinkihaveacat
force-pushed
the
fix-359501045
branch
from
439afc5 to
51240a7
Compare
|
Thanks Michael! Would you mind getting a Bluetooth SME to approve first? Then Yacine/I can approve to merge. |
|
Hey @kkuan2011
I checked with our Bluetooth SME and made a small change (0007881). |
2 participants
Bug: 359501045