Skip to content

Ble extended advertising fixes #8904

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.

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

Merged
merged 16 commits into from
Dec 4, 2018

Conversation

pan-
Copy link
Member

@pan- pan- commented Nov 29, 2018

Description

Updating the examples revealed few defects in the ble extended feature. This PR intend to fix those.
We're waiting for OOB to add more if necessary.

Pull request type

The "breaking changes" are in the new API:

  • 4ed3f56 Fix a type name for consistency.
  • 3ddcdf2 fix a return type that was incorrectly sized.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[X] Breaking change

pan- and others added 10 commits November 27, 2018 15:29
…ion parameter.

Unlike the public address, the RANDOM address is guaranteed to exist.
If privacy is enabled, it means the controller will generates random resolvable addresses or non resolvable addresses depending on the config.
If privacy is not enabled then the device will use the device's random static address that doesn't change between radio processes.
@pan-
Copy link
Member Author

pan- commented Nov 29, 2018

@0xc0170 0xc0170 requested a review from a team November 29, 2018 12:44
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 29, 2018

This are fixes for recent feature (5.11 one), is that correct?

@@ -145,10 +145,10 @@ class ConnectionParameters {
phy_t phy = phy_t::LE_1M,
scan_interval_t scanInterval = scan_interval_t::min(),
scan_window_t scanWindow = scan_window_t::min(),
conn_interval_t minConnectionInterval = conn_interval_t::min(),
conn_interval_t maxConnectionInterval = conn_interval_t::max(),
conn_interval_t minConnectionInterval = conn_interval_t(50),

Choose a reason for hiding this comment

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

any reason for these particular values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they are more balanced with common use cases than the previous one (62 to 124 ms for the connection interval instead of 7.5 to 4 seconds).

@pan-
Copy link
Member Author

pan- commented Nov 29, 2018

@0xc0170 Correct.

@donatieng
Copy link
Contributor

Looks good!

@cmonr
Copy link
Contributor

cmonr commented Dec 3, 2018

More fixes incoming.

@pan-
Copy link
Member Author

pan- commented Dec 3, 2018

@donatieng @paul-szczepanek-arm Could you re-review ?

  • e026bce change the parameter order of the startScan function to not force the user to define the filter and duration if the duration is the only parameter required.
  • dc3ff6a and ed426c5 fix consistency issue in naming of the setter and getter of the ConnectionParameters filter.
  • 5abf2f0 Fix a typedef missing (used for backward compatibility).

@cmonr I think that's all.

Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

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

One tiny Doxygen change required

scan_duration_t duration = scan_duration_t::forever(),
duplicates_filter_t filtering = duplicates_filter_t::DISABLE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs doxygen update

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@cmonr
Copy link
Contributor

cmonr commented Dec 4, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 4, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 1
Build artifacts
Build logs

@0xc0170 0xc0170 merged commit c1c94c8 into ARMmbed:master Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants