Fix intermittent BLE connection failures #23598
Merged
Conversation
This file contains 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
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kpschoedel,
lazarkov,
LuDuda,
mrjerryjohns,
msandstedt,
robszewczyk and
saurabhst
pullapprove
bot
requested review from
tcarmelveilleux,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
Damian-Nordic
commented
Nov 14, 2022
1. Rename two BTP constants to match BTP_CONN_RSP_TIMEOUT and BTP_ACK_TIMEOUT from the spec. 2. Move the constants to BleConfig.h 3. Use BTP_ACK_TIMEOUT in both secure and unauthenticated session for ACK timeout calculation. Signed-off-by: Damian Krolik <[email protected]>
Damian-Nordic
force-pushed
the
ble-timeout
branch
from
c716f14 to
7d3f5bb
Compare
|
PR #23598: Size comparison from e072098 to 7d3f5bb Increases (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (7 builds for cc13x2_26x2, esp32)
Full report (40 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #23598: Size comparison from e072098 to 326cbe1 Increases (49 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (12 builds for cc13x2_26x2, esp32, linux, psoc6)
Full report (51 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
approved these changes
Nov 15, 2022
LuDuda
approved these changes
Nov 18, 2022
|
I also found |
|
@Emill The value in the spec seems to be a default if you have no better idea of what to set the value to, not a requirement. |
|
I don't know why the column says "Default" rather than something like "Value". Allowing implementations to choose custom timeouts would break the protocol: "The timer’s duration timer SHALL be defined as any value less than one-half the acknowledgement timeout interval. This ensures that on a healthy BLE connection, a peer will always receive acknowledgements for sent packets before its acknowledgement-received timer expires." If the peers select their own ack timeout, it could happen that one-half ack interval of one peer is longer than the full ack interval of the other peer. In that case the connection would be torn down when idling. Another section also hints that the timeouts on the two devices are "the same": "When a client sends a handshake request, it SHALL start a timer with a globally-defined duration of BTP_CONN_RSP_TIMEOUT seconds. If this timer expires before the client receives a handshake response from the server, the client SHALL close the BTP session and report an error to the application. Likewise, a server SHALL start a timer with the same duration when it receives a handshake request from a client." |
|
That seems like a problem that should get fixed in the spec, then... @robszewczyk |
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.
BTP_CONN_RSP_TIMEOUTandBTP_ACK_TIMEOUTfrom the spec.BTP_ACK_TIMEOUTin both secure and unauthenticated session for ACK timeout calculation.Fixes #23438