Ble extended advertising #8738
Ble extended advertising #8738
Conversation
| } | ||
|
|
||
| if (is_extended_advertising_available()) { | ||
| return _pal_gap.extended_scan_enable( |
There was a problem hiding this comment.
we allow the user to set scanning parameters that are then ignored
for example the user may set coded phy scanning but because it is not supported this will run a normal scan but return success
we can't reject it in the settings either because we use chaining in the sets that are incapable of returning error codes
this needs to check the settings and report an error if any of the extended scan params have been set
There was a problem hiding this comment.
Parameters are rejected on legacy controllers if they include phy parameters:
However we need to check scan and connection parameter; please do not resolve the conversation.
|
@pan- All comments addressed? And ready for another round of review (helps reviewers if you add a comment after rebase/adding more commits - "whats new"). |
|
@donatieng Mind re-reviewing? Looks like your comments were addressed. |
|
And @paul-szczepanek-arm, although it seems like you're involved in the PR itself 😄 |
|
@cmonr That's a collective effort 😉 . |
There was a problem hiding this comment.
AdvDataBuilder review:
It would be good if we add new helpers functions:
setFlags(adv_flags_t);
setName(const char*, bool complete);
setTxPowerLevel(int8_t);
setAppearance(adv_appearance_t);
setManufacturerSpecificData(Span<const uint8_t>);
setLocalServiceList(Span<const UUID>, bool complete);
setConnectionIntervalPreference(conn_interval_t min, conn_interval_t max);
setServiceData(UUID serviceUUID, Span<const uint8_t> serviceData);
setAdvertisingInterval(adv_interval_t); // note: serialized to a uint16_t!
setRequestedServices(Span<const UUID>);
|
what's new AdvertisingDataBuilder updated with helper functions (but not complete, will post when done) |
|
@pan- : this needs a rebase |
features/FEATURE_BLE/ble/Gap.h
Outdated
| * @note The maximum size of data depends on the controller and its support for | ||
| * extended advertising however even if the controller supports larger data lengths if | ||
| * you wish to be compatible with older devices you may wish to use legacy | ||
| * advertising and should not use payloads larger than LEGACY_ADVERTISING_MAX_SIZE. | ||
| * |
There was a problem hiding this comment.
Might be worth mentioning that this feature was introduced in Bluetooth 5 and that 5 support from the controller's standpoint is necessary but not sufficient.
There was a problem hiding this comment.
@donatieng I thin I addressed your comment.
features/FEATURE_BLE/ble/Gap.h
Outdated
| * Subsequently you may disable extended advertising and the periodic advertising | ||
| * will continue. If you start periodic advertising while extended advertising is | ||
| * inactive, periodic advertising will not start until you start extended advertising | ||
| * at a later time. |
There was a problem hiding this comment.
Might be worth adding a quick word on why that's required (background from spec)
There was a problem hiding this comment.
@donatieng I thin I addressed your comment.
|
gap/ConnectionParameters.h fixed (as in needs re-review) |
|
Me thinks this needs a rebase. |
pan-
force-pushed
the
ble-extended-advertising
branch
from
2107d69 to
524b9a7
Compare
|
@ARMmbed/mbed-os-pan When is this completed ? It still needs work and review |
|
While we are reviewing, I'll run this through CI |
Test run: SUCCESSSummary: 4 of 4 test jobs passed |
Edit file for active voice and comma use.
Edit file, mostly to fix typos.
Edit file.
| /** | ||
| * Construct a Duration from an integer value. | ||
| * | ||
| * @param v The value of the duration in TN units. |
There was a problem hiding this comment.
Query: What are TN units?
There was a problem hiding this comment.
That is a typo, it should be TB or TIME_BASE.
| /** | ||
| * Construct a Duration from another Duration. | ||
| * | ||
| * @note The operation fail at compile time of there is a loss of precision. |
There was a problem hiding this comment.
Query: What does this mean? The operation fails at compile time if there's a loss of precision? Or something else?
| } | ||
|
|
||
| /** | ||
| * Return the duration in milliseconds. |
There was a problem hiding this comment.
Global note throughout the file: Sometimes we use "milliseconds", and sometimes we abbreviate. Please standardize.
| }; | ||
|
|
||
| /** | ||
| * Type that represents micro seconds. |
There was a problem hiding this comment.
Global note throughout the file: Sometimes we use "us", and sometimes we spell it out. Please standardize. (Please do not include a space in "microseconds" if we choose to spell it out.)
| * @tparam RangeIn The range of duration. | ||
| * @tparam FIn The Forever value of duration. | ||
| * @param duration The duration to convert. | ||
| * @return The converted duration. It is rounded up if precision is loss. |
There was a problem hiding this comment.
Query: Should this be "... if precision is lost."?
| UNKNOWN = 0, | ||
|
|
||
| /** | ||
| * Generic Phone. |
There was a problem hiding this comment.
Most of these descriptions repeat the name. Is there a way to provide more information?
There was a problem hiding this comment.
The specification doesn't offer more precision for these values. We document the entities to make sure they get rendered in Doxygen but the name is already self descriptive.
|
CI started! |
|
I can deal with my queries being addressed in a patch release, but please be sure they are addressed. I don't want them to fall through the cracks. |
|
@ARMmbed/mbed-os-maintainers Please be sure that @AnotherButler comments get ack'd in some way before merging this PR. |
The new PR would be the best to address them (as this is already in the CI). |
Test run: SUCCESSSummary: 4 of 4 test jobs passed |
|
Exporters failed (the rest of CI pipeline looks good), investigating |
|
Restarted exporter (IAR failed due to multiple writes to a file, will be investigated). |
|
Reviewed again, found the issue , two Gap.cpp files (IAR can't handle files if you run with compile multiple files - I experienced this before). |
|
@0xc0170 I pushed a fix. |
|
New Ci job started |
Test run: SUCCESSSummary: 4 of 4 test jobs passed |
|
Only exporters failed, There's colorama import failure @OPpuolitaival Please review this job |
|
Exporters restarted |
|
@0xc0170 that is CI internal problem, not related to this change |
|
Restarting exporter CI |
Description
This PR introduce extended and periodic advertising management in mbed-os BLE API. These two features have been introduced in Bluetooth 5 and greatly improve advertisement capabilities of compatible devices.
Extended Advertising
With extended advertising, devices are not limited to a single payload advertised and a single scan response. Devices can manage multiple concurrent advertisement data called advertising sets as they contain their own data and operate according to their own parameters.
Other noticeable improvement of extended advertising are:
Periodic Advertising
Periodic advertising is a new kind of advertising that allows a device to stream data at a fixed rate. Peers can sync on the data stream and listen to it. Internally it is very similar to a unidirectional connection.
Integration
On many aspects it wasn't possible to add this new feature on the top of the previous GAP advertisement API. Defects such as (incorrect) global address type, global filter policies, state bookkeeping and global scanning/advertisement parameters mutable in a non atomic way would have make it memory heavy and not very usable as it would have been full of corner cases (it already is!) due to the global state held in it if we wanted to preserve backward compatibility.
The new API introduced is basically a version two of the GAP Advertisement/Scanning/Connection API. It is not compatible with the old subsystem. You have to use one or the other but it is forbidden to use both concurrently. That way we preserve complete backward compatibility and prepare a clean ground to build upcoming Bluetooth features.
TODO
Pull request type