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

pkg/nimble: optimize package initialization #11131

Merged

Conversation

haukepetersen
Copy link
Contributor

@haukepetersen haukepetersen commented Mar 7, 2019

Contribution description

Some typical initialization steps for nimBLE need to be (re-)written for any application, so it makes sense to put them in the shared initialization code path to prevent (future) code duplication. I have a couple of additional example applications that I am preparing (mostly around ICN and IPv6 over BLE), and all of these will benefit from these proposed changes.

This PR collects some generic initialization artifacts and places them in the global init function. It then removes these lines from the nimble_gatt example.

Testing procedure

Run the examples/nimble_gatt example an verify that all services are still working.

Issues/PRs references

none

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: BLE Area: Bluetooth Low Energy support labels Mar 7, 2019
@miri64 miri64 self-requested a review March 7, 2019 15:34
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Minor nit-pick

bluetil_ad_t ad;
bluetil_ad_init_with_flags(&ad, buf, sizeof(buf), BLUETIL_AD_FLAGS_DEFAULT);
bluetil_ad_add_name(&ad, device_name);
ble_gap_adv_set_data(ad.buf, ad.pos);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a comment to the commit why you unrolled this function.

@miri64 miri64 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Mar 7, 2019
@miri64
Copy link
Member

miri64 commented Mar 7, 2019

Tested ACK. We can merge if you at least acknowledge my comment ;-)

This static function was a leftover from very early stages of this
example and at the current state there is not sense in keeping it.
Having these few lines directly where the advertising is triggered
makes the code more clean and readible.
@haukepetersen
Copy link
Contributor Author

put the justification in the commit message.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Thanks. ACK.

@miri64 miri64 added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Mar 7, 2019
@haukepetersen
Copy link
Contributor Author

all green, lets go.

@haukepetersen haukepetersen merged commit ed5bd8e into RIOT-OS:master Mar 7, 2019
@haukepetersen haukepetersen deleted the opt_nimble_initialization branch March 7, 2019 16:08
@danpetry danpetry added this to the Release 2019.04 milestone Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants