Fix cancel connect #13390
Merged
Fix cancel connect #13390
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
|
Tested with new tests in the new ble-tests suite: |
ciarmcom
added
the
release-type: patch
|
@paul-szczepanek-arm, thank you for your changes. |
LDong-Arm
suggested changes
Aug 4, 2020
| } | ||
|
|
||
| if (connection_id < DM_CONN_MAX) { | ||
| connection_id++; |
Comment on lines
344
to
351
| DmConnClose( | ||
| DM_CLIENT_ID_APP, | ||
| /* connection handle */ connection_id, | ||
| /* reason - invalid (use success) */ 0x00 | ||
| ); | ||
| } | ||
|
|
||
| return BLE_ERROR_NONE; |
There was a problem hiding this comment.
The DmConnClose call can be nested into the for loop so we don't need to check DM_CONN_MAX twice. In the loop, return BLE_ERROR_NONE after DmConnClose. If we reach the end, no address matches so we could return some kind of error.
There was a problem hiding this comment.
thanks, changed as requested
Comment on lines
637
to
639
| if (_last_used_peer_address_type != peer_address_type_t::ANONYMOUS) { | ||
| return cancelConnect(_last_used_peer_address_type, _last_used_peer_address); | ||
| } |
There was a problem hiding this comment.
Note that if the previous connection got disconnected or didn't succeed, _last_used_peer_address_type, _last_used_peer_address still have the old values. Don't know if it's worth resetting them somewhere - the legacy cancelConnect() is deprecated and may not be worth complicating.
Anyway the if condition has little value for that reason in my opinion.
There was a problem hiding this comment.
The if checks if it has been set, otherwise I'd be checking random addresses. The cancel connect does nothing if there isn't an ongoing connection attempt for the address.
LDong-Arm
reviewed
Aug 5, 2020
paul-szczepanek-arm
force-pushed
the
fix-cancel-connect
branch
from
effeceb
to
5bf8929
Compare
|
had to force push to rebase on preceding PR |
paul-szczepanek-arm
force-pushed
the
fix-cancel-connect
branch
3 times, most recently
from
df89b69
to
a9fd028
Compare
Merged
paul-szczepanek-arm
force-pushed
the
fix-cancel-connect
branch
2 times, most recently
from
a14d513
to
7a539f5
Compare
|
this is good for CI |
paul-szczepanek-arm
force-pushed
the
fix-cancel-connect
branch
from
7a539f5
to
0a9624d
Compare
|
wait, this is showing up as all wrong, I need to fix the base |
|
This PR cannot be merged due to conflicts. Please rebase to resolve them. |
paul-szczepanek-arm
force-pushed
the
fix-cancel-connect
branch
from
0a9624d
to
1aa6da4
Compare
|
I apologise, this is now rebased correctly and good to go |
LDong-Arm
approved these changes
Aug 10, 2020
paul-szczepanek-arm
force-pushed
the
fix-cancel-connect
branch
from
6021f8c
to
e877eb7
Compare
|
Fixed the brace on newline |
0xc0170
approved these changes
Aug 11, 2020
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
pan-
reviewed
Aug 18, 2020
There was a problem hiding this comment.
Thank you for the fix Paul but I have difficulties to understand why the public facing API and pal API were changed. At the HCI level there is no address asked to cancel the process that creates a connection everything is canceled.
Can you revert the user facing and PAL API change. The only change needed should be in the PAL implementation.
| @@ -1002,10 +1002,30 @@ class Gap { | |||
| /** Cancel the connection attempt. This is not guaranteed to succeed. As a result | |||
| * onConnectionComplete in the event handler will be called. Check the success parameter | |||
| * to see if the connection was created. | |||
| * @depreacted This version has a defective API. You must provide the address of the peer. | |||
|
Cordio requires the ble address to identify the control block. This is a merged PR. You might want to just talk to me over slack. |
mbedmain
removed
release-type: patch
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.
Preceeding PR #13365
Summary of changes
The command in GAP cancelConnect writes to random memory due to incorrect usage of cordio state machine. Additionally cancel command has faulty API as it does not accept the peer address.
This fixes the current cancelConnect to remove the random memory write and also adds an overload that allows to cancel a selected target.
Impact of changes
Migration actions required
Documentation
None. Doxy updated.
Pull request type
Test results
Reviewers
@LDong-Arm