implement pairing for bluetooth proxy#4475
Conversation
This patch will turn on encryption when making active connections in order to comply with just-works BLE encryption.
|
Hey there @jesserockz, mind taking a look at this pull request as it has been labeled with an integration ( |
|
While this change seems to be working, it does bring in a bunch of error logs from esp-idf now |
|
Interesting, is ESP_GAP_BLE_SEC_REQ_EVT being called? My device does not trigger it and thus encryption is never setup leading to char write returning GATT_INSUFFICIENT_ENCRYPTION. I assume your device is asking for it and thus complain that it is being re-setup. Thanks a bunch for testing so quickly 👍 |
|
No this event is not received on the ESP |
|
Ok, so the I assume your device is unpaired, unencrypted, “Bluetooth Smart” device |
|
relates to esphome/aioesphomeapi#390 |
|
OK! @jesserockz, I've tested the changes with my device and issuing a pair() command through aioesphomeapi works beautifully, e.g.: import aioesphomeapi
import asyncio
async def main():
api = aioesphomeapi.APIClient("10.0.13.37", 6053, password=None)
await api.connect(login=True)
def _on_bluetooth_connection_state(
connected: bool, mtu: int, error: int
) -> None:
print(f"Connect cb! {connected=} {mtu=}")
address = int("MY:DE:VI:CE:HE:RE".replace(":", ""), 16)
await api.bluetooth_device_connect(
address,
_on_bluetooth_connection_state,
timeout=10,
has_cache=False,
version=3,
address_type=0,
)
await api.bluetooth_device_pair(address)
def on_notify(int, data):
print(data)
await api.bluetooth_gatt_start_notify(address, 26, on_notify)
print(await api.bluetooth_gatt_write(address, 29, bytes([3]), True))
await asyncio.sleep(0.5)
print(await api.bluetooth_gatt_write(address, 23, bytes(b'get sy v\r\n'), True))
await asyncio.sleep(5)
await api.bluetooth_device_disconnect(address)
loop = asyncio.get_event_loop()
loop.run_until_complete(main())Without calling bluetooth_device_pair() the device returns GATT_INSUFFICIENT_ENCRYPTION. |
|
We need to bump the Bluetooth proxy version as well so hass can know it's available |
|
I started https://github.com/jagheterfredrik/core/tree/bluetooth-proxy-pair but you're quick @bdraco, thank you! 🙏 I'll bump the version here |
|
There seems to be some issue with the latest version. I'm testing with two pairable devices, and Home Assistant often gets auth errors when trying to communicate with the devices. It seems like the first device Home Assistant talks to works fine, whereas the 2nd device has issues. I've only tested a handful of times though, so I'm not sure if it's always the 2nd device which fails. The error in the Home Assistant log (actually from bleak) say there's a problem with "insufficient authentication". Note: The problem doesn't always occur, it happens maybe 3 out of 4 times when restarting Home Assistant. In the ESPHome logs I see this: Let me know what I can do to aid debugging. |
So you mean that it was working before? |
|
@jagheterfredrik I tested on an earlier commit, before the waiting for ACK after pairing was implemented, that had other issues, probably because there was no wait for pairing to complete? You can contact me on discord if you want to chat, I'm emontnemery#6618 there. |
|
I see. Would you mind bisecting? |
|
I don't mind bisecting, but I think it gets a bit tricky since there's interdependencies between the Home Assistant Core, aioesphomeapi and esphome PRs? If you can outline which combinations commits you'd like me to test or bisect, I'll do that though. |
|
esphome should suffice. Did you try to explicitly disconnect-> connect -> pair -> write? |
That's where I'm confused, if I just bisect the commits in this PR, I'll run into problems because aioesphome and home assistant are not compatible with ESPHome? Or do you have some suggestion for how to test ESPHome in isolation?
No, I did not try that. |
|
Latest aioesphomeapi should be backwards compatible. I’m not sure I follow how you are testing, is it a custom HA component? I’d prefer to move this to a separate issue as it’s working on your other device. Insufficient auth could mean that the device is not happy with just-works pairing and requires a passcode pairing procedure which is not implemented and would require a lot of additional work. |
Is it really backwards compatible considering the protobuf definitions were changed several times, with new messages added and so on?
I'm testing with a core integration which has support for a Bluetooth device which requires pairing: https://next.home-assistant.io/integrations/dormakaba_dkey/
The pairing implemented by this PR seems to be flaky; it sometimes works and it sometimes doesn't, so I think that should be solved before this PR is merged? Based on some more testing:
Could you contact me on discord, I'm emontnemery#6618 there, I think it may be easier to explain how I'm testing etc. if it's still unclear? |
|
I just took a another look at the error logs I posted when I first mentioned the issue, and it seems clear that ESPHome is mixing up the two connections: |
|
Here's a successful log, the difference is that the connections to the two devices do not happen in parallel: |
|
I got some help from @jesserockz, the problem is that the newly added code does not check if BLE events are for the correct connection slot. After adding that, it works 👍 |
|
This needs to be added: diff --git a/esphome/components/bluetooth_proxy/bluetooth_connection.cpp b/esphome/components/bluetooth_proxy/bluetooth_connection.cpp
index f4504983..452f5fd8 100644
--- a/esphome/components/bluetooth_proxy/bluetooth_connection.cpp
+++ b/esphome/components/bluetooth_proxy/bluetooth_connection.cpp
@@ -163,6 +163,8 @@ void BluetoothConnection::gap_event_handler(esp_gap_ble_cb_event_t event, esp_bl
switch (event) {
case ESP_GAP_BLE_AUTH_CMPL_EVT:
+ if (memcmp(param->ble_security.auth_cmpl.bd_addr, this->remote_bda_, 6) != 0)
+ return;
if (param->ble_security.auth_cmpl.success) {
api::global_api_server->send_bluetooth_device_pairing(this->address_, true);
} else {
diff --git a/esphome/components/esp32_ble_client/ble_client_base.cpp b/esphome/components/esp32_ble_client/ble_client_base.cpp
index 9654287f..9008e7bd 100644
--- a/esphome/components/esp32_ble_client/ble_client_base.cpp
+++ b/esphome/components/esp32_ble_client/ble_client_base.cpp
@@ -250,11 +250,15 @@ void BLEClientBase::gap_event_handler(esp_gap_ble_cb_event_t event, esp_ble_gap_
switch (event) {
// This event is sent by the server when it requests security
case ESP_GAP_BLE_SEC_REQ_EVT:
+ if (memcmp(param->ble_security.ble_req.bd_addr, this->remote_bda_, 6) != 0)
+ return;
ESP_LOGV(TAG, "[%d] [%s] ESP_GAP_BLE_SEC_REQ_EVT %x", this->connection_index_, this->address_str_.c_str(), event);
esp_ble_gap_security_rsp(param->ble_security.ble_req.bd_addr, true);
break;
// This event is sent once authentication has completed
case ESP_GAP_BLE_AUTH_CMPL_EVT:
+ if (memcmp(param->ble_security.auth_cmpl.bd_addr, this->remote_bda_, 6) != 0)
+ return;
esp_bd_addr_t bd_addr;
memcpy(bd_addr, param->ble_security.auth_cmpl.bd_addr, sizeof(esp_bd_addr_t));
ESP_LOGI(TAG, "[%d] [%s] auth complete. remote BD_ADDR: %s", this->connection_index_, this->address_str_.c_str(), |
|
Thanks @emontnemery, nice debugging 👍 |
|
@jagheterfredrik do you have some example of BLE devices that require pairing? Is it the igrill device you need it for? |
|
I'm testing on a Wallbox Pulsar Plus (https://github.com/jagheterfredrik/wallbox-ble) |
|
I've tested this a bit more now, with the address guards in place things seem to work fine every time 👍 |
This patch implements bluetooth pairing for bluetooth proxy on ESP32.
What does this implement/fix?
The pairing function was not implemented
Types of changes
Related issue or feature (if applicable):
Test Environment
Example entry for
config.yaml:https://esphome.github.io/bluetooth-proxies/
Checklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: