Skip to content
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

BLE: Enforce advertising data payload limits #9096

Merged
merged 9 commits into from Jan 17, 2019

Conversation

Projects
None yet
6 participants
@paul-szczepanek-arm
Copy link
Member

commented Dec 13, 2018

Description

The complete advertising data length may be different based on whether it's connectable advertising. A new call has been added to the API.
Data set when advertising is active is also limited. New function added to advise of the limit.
There is an illegal adv parameters combinations that is now checked.

On Pal side:
The HCI length of packets may be variable and dependent on the controller. This fix queries the PAL to use the correct fragmentation of set data.

Pull request type

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

@paul-szczepanek-arm paul-szczepanek-arm requested a review from donatieng Dec 13, 2018

@cmonr cmonr added the needs: review label Dec 13, 2018

@cmonr cmonr requested a review from pan- Dec 20, 2018

*
* @return Maximum advertising data length you may set if advertising set is active.
*/
virtual uint8_t getMaxActiveSetAdvertisingDataLength();

This comment has been minimized.

Copy link
@pan-

pan- Jan 2, 2019

Member

I would use a uint16_t as the return type here. The Bluetooth SIG may lift that limitation in a next release.

This comment has been minimized.

Copy link
@paul-szczepanek-arm

paul-szczepanek-arm Jan 2, 2019

Author Member

the update needs to happen in a single HCI command which is limited to 255 bytes (less headers)

This comment has been minimized.

Copy link
@pan-

pan- Jan 2, 2019

Member

It is understood that for now the update must happen in a single hci packet when advertising is on.
However that may change in the future as the controller can easily identify when the update is ready to be commited when the host sends a last fragment operation.

That's why I suggest using uint16_t as the return type. It virtually cost nothing and it can avoid integer size issues if the Bluetooth specification is updated.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jan 2, 2019

Contributor

On ARM, any narrow types are normally more expensive than 32-bit types, if they're automatic variables, arguments or return values. The compiler needs to add extra narrowing instructions.

Unless the narrow width is semantically important, it's best to stick with int, int32_t or size_t most of the time. (I used to avoid size_t when still dealing with 8-bit CPUs where that forced 32-bit rather than 16-bit arithmetic, but no longer.)

IIRC, standalone static variables are also normally word-aligned anyway, so I don't think narrow types buy you anything there either. Automatic variables are are also word-aligned on the stack.

So narrow types are only really useful in structures and arrays, unless you require narrow unsigned modulo arithmetic.

tl;dr size_t would probably generate smaller code than either uint8_t or uint16_t.

@pan-

pan- approved these changes Jan 2, 2019

Copy link
Member

left a comment

Thanks for the changes @paul-szczepanek-arm; they looks good.

@pan-

pan- approved these changes Jan 2, 2019

) {
const size_t hci_length = _pal_gap.get_maximum_hci_advertising_data_length();

for (size_t i = 0, end = payload.size(); (i < end) || (i == 0 && end == 0); i += hci_length) {

This comment has been minimized.

Copy link
@cmonr

cmonr Jan 3, 2019

Contributor

Is there a particular reason why if (end == 0) doesn't come before this weird for loop header?

This comment has been minimized.

Copy link
@paul-szczepanek-arm

paul-szczepanek-arm Jan 3, 2019

Author Member

not my code (incidental style change when updating hci_length) but looking at it I can't see a way of removing the conditions without having to duplicate the code (or refactor the inside into a func)

This comment has been minimized.

Copy link
@pan-

pan- Jan 3, 2019

Member

We still have to execute one iteration of the loop if end == 0 . Thinking about it now, there is several alternative:

  • Use a do { } while loop. The pattern fits the use case as the first iteration must be executed unconditionally. However it mean extracting iteration variables outside of the iteration scope 😞 .
  • rewrite the for condition into: i == 0 || i < end. The end == 0 is not necessary.
@paul-szczepanek-arm

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

ready for CI

@0xc0170

0xc0170 approved these changes Jan 3, 2019

@0xc0170 0xc0170 requested a review from AnotherButler Jan 3, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

@AnotherButler Please review docs updates

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jan 16, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Moving to CI. @AnotherButler If you can have a chance meanwhile to review docs

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 17, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 17, 2019

@0xc0170 0xc0170 merged commit 284781a into ARMmbed:master Jan 17, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 0 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10373 cycles (+556 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.