Espressif BLE fixes: advertising duration, _bleio.adapter.connected#11036
Draft
dhalbert wants to merge 1 commit into
Draft
Espressif BLE fixes: advertising duration, _bleio.adapter.connected#11036dhalbert wants to merge 1 commit into
dhalbert wants to merge 1 commit into
Conversation
1. The advertising duration value for "forever" is different for `ble_gap_adv_start()` and `ble_gap_ext_adv_start()`. It is `BLE_HS_FOREVER` and `0`, respectively. 2. Code was added in adafruit#9289 that required that the MTU value was non-zero when returning true for `_bleio.adapter.connected`. This caused a brief interval, while a central was connecting, for a peripheral with both `ble.connected()` and `ble.advertising()` being `False`. The transition between the two states did not appear atomic. This compares with `nordic` where it was.
2fca313 to
2722f6b
Compare
Member
|
I think we can consider it connected when we are negotiating MTU. It's been a while since I did BLE but I think our MTU negotiation is just to get the most bytes over the air at the time. It is optional and not required. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think this may fix #10007.
Advertising duration
The advertising duration value for "forever" is different for
ble_gap_adv_start()andble_gap_ext_adv_start(). It isBLE_HS_FOREVERand0, respectively. The code was usingBLE_HS_FOREVERIn 6.0.1, this value is actually checked, and gives a parameter range error.connectedstate checkCode was added in #9289 that required that the MTU value be non-zero when returning
truefor_bleio.adapter.connected.circuitpython/ports/espressif/common-hal/_bleio/Adapter.c
Line 666 in 7a27366
circuitpython/ports/espressif/common-hal/_bleio/Adapter.c
Line 742 in d6fd51f
The MTU is zero when a connection has first been made but the MTU is not yet negotiated.
The MTU check above causes a brief interval, for both
ble.connected()andble.advertising()to beFalse. The transition between advertising and connected did not appear atomic. This compares with thenordicimplementation where this glitch is not present.I removed this MTU check and connections still work. I was testing with an HID keyboard example (derived from https://learn.adafruit.com/ble-hid-keyboard-buttons-with-circuitpython) which requires pairing. It is necessary to forget the CircuitPython device on the central side if its firmware is reloaded, at least for an iOS central.
@tannewt Do you remember why you added the MTU check in #9289? It's true that the connection is not complete until the MTU is negotiated. But we have a lot of example code something like this:
So what happens is that when the central has connected initially but the MTU is not yet negotiated, the
ifstatement is brieflyFalse, which makes the program think erroneously that there was a disconnect, and it starts advertising again. On Espressif, this quickly causes "Nimble: out of memory".If there was a
ble.connection_initiatedble.connection_in_progressthat could be used instead. But otherwise there's no state available to know that. And we'd have to change a lot of code.