Skip to content

Resolve advertising reports and connection complete #13630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Sep 22, 2020
Merged

Resolve advertising reports and connection complete #13630

merged 6 commits into from
Sep 22, 2020

Conversation

@paul-szczepanek-arm
Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm commented Sep 17, 2020

Summary of changes

Add host resolution of RPAs for advertising and connecting.

Impact of changes

Migration actions required

Documentation

none

Pull request type 

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@pan-

@ciarmcom ciarmcom requested review from a team and pan- September 17, 2020 14:30
@ciarmcom
Copy link
Member

ciarmcom commented Sep 17, 2020

@paul-szczepanek-arm, thank you for your changes.
@pan- @ARMmbed/mbed-os-maintainers please review.

pan-
pan- reviewed Sep 18, 2020
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @paul-szczepanek-arm I left a few comments for improvements.

connectivity/FEATURE_BLE/source/generic/GapImpl.h
private:
/** Last in First Out list*/
template<typename T>
class EventList {
Copy link
Member

@pan- pan- Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: we would really benefit from a true list available to all Mbed OS.

connectivity/FEATURE_BLE/source/generic/GapImpl.cpp Outdated
event.setLocalResolvablePrivateAddress(_address_registry.get_resolvable_private_address());

event.setPeerAddress(*identity_address);
event.setPeerAddressType((peer_address_type_t&)identity_address_type);
Copy link
Member

@pan- pan- Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peer_address_type_t and target_peer_address_type_t are not exact match.
peer_address_type_t has values that represents public identity address and random static identity address.

Copy link
Member Author

@paul-szczepanek-arm paul-szczepanek-arm Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, replaced with translation

connectivity/FEATURE_BLE/source/generic/GapImpl.cpp
#if BLE_ROLE_PERIPHERAL
if (!identity_address) {
if (_peripheral_privacy_configuration.resolution_strategy ==
peripheral_privacy_configuration_t::REJECT_NON_RESOLVED_ADDRESS) {
Copy link
Member

@pan- pan- Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation we are supposed to disconnect if a bond is not present in the security db.
I'm not certain of what we should do if there's no bond though.

connectivity/FEATURE_BLE/source/generic/GapImpl.cpp Outdated
if (pending_event && pending_event->event) {
allocation_successful = _reports_pending_address_resolution.push(pending_event);
}
if (allocation_successful) {
Copy link
Member

@pan- pan- Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cleanup memory if allocation_successful is false.

connectivity/FEATURE_BLE/include/ble/gap/Events.h Outdated
* @param advertisingData Advertising payload.
*/
AdvertisingReportEvent(
ble_error_t status,
Copy link
Member

@pan- pan- Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change the semantic of AdvertisingReportEvent. I don't think we need to report allocation failures to the user in the AdvertisingReportEvent as advertising reports are not reliable. The cordio stack will silent drop events if it doesn't have memory capacity, same for the controller.

Copy link
Member Author

@paul-szczepanek-arm paul-szczepanek-arm Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped

@paul-szczepanek-arm
Copy link
Member Author

paul-szczepanek-arm commented Sep 18, 2020

Now I can copy events I'll make events part of the entries so that should deal with all the memory comments

@paul-szczepanek-arm paul-szczepanek-arm force-pushed the host-privacy-feature branch 3 times, most recently from 5cf3b08 to 1b9b050 Compare September 18, 2020 16:01
@pan- pan- force-pushed the feature-ble-host-privacy branch from cab0b0c to de531fd Compare September 21, 2020 09:17
@mergify
Copy link

mergify bot commented Sep 21, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

pan-
pan- approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Paul, I merge this into the feature branch.
Next stop, next PR.

@pan- pan- merged commit 3e2db59 into ARMmbed:feature-ble-host-privacy Sep 22, 2020
@pan- pan- mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pan- pan- pan- approved these changes

Assignees

No one assigned

Labels

needs: review release-version: feature branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@paul-szczepanek-arm @ciarmcom @pan- @0xc0170