Add integration with bt-hci crate

[lulf]

Implementing traits from bt-hci allows the BleConnector to be used with the Trouble BLE stack.

See #1933

The bt-hci crate is not yet released on crates.io, hence marking this as draft until that's done. I've created an example here that use the TrouBLE stack. I'm not sure if it makes sense to add an example to esp-hal or not, there's no trouble release yet so it would have to be a git dependency.

[Dominaezzz]

I'm not sure if it makes sense to add an example to esp-hal or not, there's no trouble release yet so it would have to be a git dependency.

I think that's what this PR should be. Which can land now, rather than wait for a release.

Ideally esp-wifi should expose an API that allows users to implement traits of any 3rd party library, rather than require esp-wifi to provide an implementation for every bluetooth library that exists out there.

This PR can just be to add an example trouble.rs that just shows how to integrate esp-hal and trouble yourself.

(For pragmatic reasons, once trouble is released esp-wifi will probably provide an implementation 🙂)

I strongly suggest using a commit hash rather than the main branch. The examples are built in CI and breaking changes in trouble would break CI here.

[lulf]

I'm not sure if it makes sense to add an example to esp-hal or not, there's no trouble release yet so it would have to be a git dependency.

I think that's what this PR should be. Which can land now, rather than wait for a release.

We're in the process of moving the bt-hci crate to the embassy org, so will wait for that before updating this PR.

Ideally esp-wifi should expose an API that allows users to implement traits of any 3rd party library, rather than require esp-wifi to provide an implementation for every bluetooth library that exists out there.

Just to clear things up, there are two crates at play:

• bt-hci which is just HCI type definitions following the bluetooth specification (no BLE stack implementation), it's not bound to any BLE stack and is generally a small stable dependency that doesn't change. This is not part of trouble at all.
• trouble which implements the BLE Host stack, similar to bleps. I agree an integration with that is outside the scope of esp-hal, so it should only be put as an example (which I've pushed now).

Whether or not to depend on bt-hci in esp-hal is up to you. It would reduce the boilerplate needed for users, so my recommendation would be to do that, as I think whatever API would be in esp-hal instead would end up pretty similar (since HCI is a spec). It's another dependency though, so I can understand why you'd want to keep it out as well.

This PR can just be to add an example trouble.rs that just shows how to integrate esp-hal and trouble yourself.

I agree that the trouble crate should only be depended on in the example. I've pushed an example just now for that.

(For pragmatic reasons, once trouble is released esp-wifi will probably provide an implementation 🙂)

Just to clarify, there isn't any need for esp-wifi to integrate with trouble, it's kind of at the layer above, using the esp-wifi implementations of the bt-hci traits.

I strongly suggest using a commit hash rather than the main branch. The examples are built in CI and breaking changes in trouble would break CI here.

[bjoernQ]

I think it's totally fine (and something I want) to offer two HCI interfaces - embedded-io (what we already have) since it's not too uncommon to have a serial interface and bt-hci. I agree esp-wifi shouldn't depend on any BLE-stack directly (which it doesn't and this PR doesn't change that)

In theory we could have just one interface and use / offer adapters but in the end, I think this is a more convenient solution.

[Dominaezzz]

Ah yeah sorry, I used "trouble" and "bt-hci" interchangeably. (I understand the difference)
To be more precise, I always referring to bt-hci.

I was just saying that the bt-hci integration can live in the example code until the crate is released, that way the PR can land now.

[jessebraham]

Thank you for this, LGTM!

[lulf]

Allright its ready for a proper review now, bt-hci is released

Edit: let me know if you want me to squash commits.

[bjoernQ]

Thanks a lot!

I tried this on some targets but unfortunately on ESP32-C3 and ESP32-S3 it advertises but I cannot connect. On ESP32-C6 I don't see it advertising.

Here is a pcap from ESP32-C3 (using Windows BLE Explorer, but also NRF Connect on Android shows same behavior)
esp32c3_trouble.zip

[lulf]

@bjoernQ Oh, that's not good, thanks for testing. I only have esp32c3 to test on, but I'm able to connect, read attribute and get notifications. On iphone (nrf connect 2.7.8), so it might be some differences with Android that triggers that. I'll look more into it.

[bjoernQ]

You probably already know this but you can use https://github.com/esp-rs/esp-hal/tree/main/extras/esp-wifishark to capture HCI data in Wireshark - at least for me that was quite helpful before. I remember at least e.g. Windows and Android perform very different things during service discovery

[lulf]

@bjoernQ Well, that was a good find. The BleConnector integration was using read_hci, which ended up giving 2 hci frames during read, and the second one was skipped. I switched it to use read_next which just reads a single HCI packet rather than a stream, and now it's working on both Android and iOS for me. My guess I was just 'lucky' that it worked on iOS before.

The HCI capture thing helped identify that there was a frame in the internal buffer that was 'skipped' in the read(), thanks

[bjoernQ]

Thanks!

The BleConnector integration was using read_hci, which ended up giving 2 hci frames during read, and the second one was skipped. I switched it to use read_next which just reads a single HCI packet rather than a stream, and now it's working on both Android and iOS for me.

That makes a lot of sense - thanks for fixing this. I am now able to connect from Android to most chips. Unfortunately, Windows LE Explorer still locks-up on connecting.

Also, for some chips (ESP32-C2, ESP32-C6 and ESP32-H2) it seems that it never gets to start the advertising. The example stops at

I (23) boot: ESP-IDF v5.1-beta1-378-gea5e0ff298-dirt 2nd stage bootloader
I (24) boot: compile time Jun  7 2023 08:03:28
I (25) boot: chip revision: v0.1
I (29) boot.esp32h2: SPI Speed      : 32MHz
I (33) boot.esp32h2: SPI Mode       : DIO
I (38) boot.esp32h2: SPI Flash Size : 4MB
I (43) boot: Enabling RNG early entropy source...
I (48) boot: Partition Table:
I (52) boot: ## Label            Usage          Type ST Offset   Length
I (59) boot:  0 nvs              WiFi data        01 02 00009000 00006000
I (67) boot:  1 phy_init         RF data          01 01 0000f000 00001000
I (74) boot:  2 factory          factory app      00 00 00010000 003f0000
I (82) boot: End of partition table
I (86) esp_image: segment 0: paddr=00010020 vaddr=40800000 size=04ba4h ( 19364) load
I (100) esp_image: segment 1: paddr=00014bcc vaddr=40824958 size=00030h (    48) load
I (103) esp_image: segment 2: paddr=00014c04 vaddr=00000000 size=0b3f4h ( 46068)
I (124) esp_image: segment 3: paddr=00020000 vaddr=42000000 size=4f258h (324184) map
I (212) esp_image: segment 4: paddr=0006f260 vaddr=00000000 size=03ae8h ( 15080)
I (217) esp_image: segment 5: paddr=00072d50 vaddr=42052d50 size=0adc0h ( 44480) map
I (231) boot: Loaded app from partition at offset 0x10000
I (231) boot: Disabling RNG early entropy source...
INFO - esp-wifi configuration Config { rx_queue_size: 5, tx_queue_size: 3, static_rx_buf_num: 10, dynamic_rx_buf_num: 32, static_tx_buf_num: 0, dynamic_tx_buf_num: 32, ampdu_rx_enable: 0, ampdu_tx_enable: 0, amsdu_tx_enable: 0, rx_ba_win: 6, max_burst_size: 1, country_code: "CN", country_code_operating_class: 0, mtu: 1492, heap_size: 65536, tick_rate_hz: 100, listen_interval: 3, beacon_timeout: 6, ap_beacon_timeout: 300, failure_retry_cnt: 1, scan_method: 0 }
INFO - Starting advertising and GATT service

What all these chips have in common is that they are using the newer (Nimble based) driver under the hood - so might be something on our side. Haven't looked into this yet

[lulf]

Thanks!

The BleConnector integration was using read_hci, which ended up giving 2 hci frames during read, and the second one was skipped. I switched it to use read_next which just reads a single HCI packet rather than a stream, and now it's working on both Android and iOS for me.

That makes a lot of sense - thanks for fixing this. I am now able to connect from Android to most chips. Unfortunately, Windows LE Explorer still locks-up on connecting.

I don't have a windows system, so I'm not sure how to debug that. If you some kind of a packet trace, that would be helpful.

Also, for some chips (ESP32-C2, ESP32-C6 and ESP32-H2) it seems that it never gets to start the advertising. The example stops at

I (23) boot: ESP-IDF v5.1-beta1-378-gea5e0ff298-dirt 2nd stage bootloader
I (24) boot: compile time Jun  7 2023 08:03:28
I (25) boot: chip revision: v0.1
I (29) boot.esp32h2: SPI Speed      : 32MHz
I (33) boot.esp32h2: SPI Mode       : DIO
I (38) boot.esp32h2: SPI Flash Size : 4MB
I (43) boot: Enabling RNG early entropy source...
I (48) boot: Partition Table:
I (52) boot: ## Label            Usage          Type ST Offset   Length
I (59) boot:  0 nvs              WiFi data        01 02 00009000 00006000
I (67) boot:  1 phy_init         RF data          01 01 0000f000 00001000
I (74) boot:  2 factory          factory app      00 00 00010000 003f0000
I (82) boot: End of partition table
I (86) esp_image: segment 0: paddr=00010020 vaddr=40800000 size=04ba4h ( 19364) load
I (100) esp_image: segment 1: paddr=00014bcc vaddr=40824958 size=00030h (    48) load
I (103) esp_image: segment 2: paddr=00014c04 vaddr=00000000 size=0b3f4h ( 46068)
I (124) esp_image: segment 3: paddr=00020000 vaddr=42000000 size=4f258h (324184) map
I (212) esp_image: segment 4: paddr=0006f260 vaddr=00000000 size=03ae8h ( 15080)
I (217) esp_image: segment 5: paddr=00072d50 vaddr=42052d50 size=0adc0h ( 44480) map
I (231) boot: Loaded app from partition at offset 0x10000
I (231) boot: Disabling RNG early entropy source...
INFO - esp-wifi configuration Config { rx_queue_size: 5, tx_queue_size: 3, static_rx_buf_num: 10, dynamic_rx_buf_num: 32, static_tx_buf_num: 0, dynamic_tx_buf_num: 32, ampdu_rx_enable: 0, ampdu_tx_enable: 0, amsdu_tx_enable: 0, rx_ba_win: 6, max_burst_size: 1, country_code: "CN", country_code_operating_class: 0, mtu: 1492, heap_size: 65536, tick_rate_hz: 100, listen_interval: 3, beacon_timeout: 6, ap_beacon_timeout: 300, failure_retry_cnt: 1, scan_method: 0 }
INFO - Starting advertising and GATT service

What all these chips have in common is that they are using the newer (Nimble based) driver under the hood - so might be something on our side. Haven't looked into this yet

I don't have any other esp32 to test on, so I'm not sure what that could be. I tested the apache-nimble controller integration on nrf, which does work, but that controller is perhaps different to the one used in esp32?

[bjoernQ]

Here are captures for ESP32-C3 with Windows BLE Explorer and for ESP32-C6

captures.zip

The first three packets on ESP32-C6 look the same for Trouble and Bleps - but then there is silence in the Trouble example

Compared to ESP32-C3 there is an initial CommandComplete which is ignored by Bleps but might cause trouble for Trouble:

[lulf]

@bjoernQ You're right, it's the initial command complete that causes an error when bt-hci attempts to parse it. It's an interesting edge case, because all valid BLE command complete events corresponding to a command have an additional 'status' byte set, whereas this one is one byte short (you can see the Reset command for instance is 7 bytes), and not in response to a command.

An error like this will cause the ble.run() in the example to exit, which is probably why everything hangs.

Here is the alternatives I think:

1. Change bt-hci event parser to not require the status byte to be present. It feels a bit wrong to deal with it in the bt-hci crate though, since this is specific to a controller and I'm not sure if it's really allowed or not.
2. Change trouble to ignore parsing errors. I don't like this one, because it masks potential real bugs.
3. Change the esp-hal bt-hci integration to check for this case when parsing the HCI command.

I've gone with 3. and pushed a fix that should ignore this event,, it would be great if you could try it out.

[bjoernQ]

Thanks! I agree option 3 is the best way here

I can confirm it's now working with ESP32-C6 and nrf-Connect on Android!

I got the example to crash when closing nrf-Connect - not an issue / just mentioning:

[lulf]

Thanks! I agree option 3 is the best way here

I can confirm it's now working with ESP32-C6 and nrf-Connect on Android!

Great, thanks for testing!

I got the example to crash when closing nrf-Connect - not an issue / just mentioning:

Ah yes, it could happen if trying to notify a host that was disconnected. I've modified the example to just ignore that for now.

[bjoernQ]

I think this looks good and works fine for me with an Android central - would love to get this in - do you know if using Windows as central is a general known problem in Trouble currently? (I don't think it's an issue with the integration if Android and iOS are working fine)

[lulf]

Regarding that trouble PR, I'll see if I can get hold on some windows pc one day to reproduce, but it might not be very soon. It should probably be looked into, I don't think the intersection of 'windows BLE connector' and trouble users are that big yet for anyone to notice. Whether or not it should block the PR I dunno.

[MabezDev]

Whether or not it should block the PR I dunno.

I don't think it should, this is really nice to have, so let's get this in now. Would you mind rebasing?

[lulf]

@MabezDev Done. I'll raise an issue in trouble repo regarding the Windows LE explorer so it's not forgotten.

[MabezDev]

Thank you! ❤️

[jessebraham]

Feel free to bump the MSRV to 1.77 in order to get this green. Or if you'd prefer I can do so in a separate PR.

[lulf]

@jessebraham I wasn't sure where to bump that, I set it on esp-wifi, is that ok?

[MabezDev]

@lulf You'll have to bump the version in CI too, I suppose for simplicity we should also just bump esp-hal MSRV so we don't do the check on two different toolchains

[lulf]

Finally green 🎉 😂

[jessebraham]

Thanks again for your work on this!