Skip to content

pkg/nimble: ver bump + config improvement #12775

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 3 commits into from
Dec 17, 2019
Merged

pkg/nimble: ver bump + config improvement #12775

merged 3 commits into from
Dec 17, 2019

Conversation

haukepetersen
Copy link
Contributor

Contribution description

In the last month some (for us) relevant bugs in NimBLE got fixed (e.g. apache/mynewt-nimble#650), so its time for another version bump.

On top, this PR includes a configuration improvement to enable the link layer data length extension to be used whenever nimble_netif is used -> this improves network performance quite a bit.

Both commits are included in the same PR, as testing is identical and therefore this reduces the workload...

Testing procedure

Verify that examples/nimble_gatt, examples/gnrc_networking, and tests/nimble_l2cap are still working. Any nrf52xxx-based platform does the job to test this.

Issues/PRs references

none

@haukepetersen haukepetersen added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Nov 21, 2019
@haukepetersen
Copy link
Contributor Author

do not merge yet, there is one other bugfix that needs should go in before the version bump: apache/mynewt-nimble#678

@HendrikVE HendrikVE mentioned this pull request Nov 27, 2019
Merged
3 tasks
@haukepetersen haukepetersen force-pushed the opt_nimble_vernov19andlldatalenfornetif branch from c815178 to ffd8fc9 Compare December 16, 2019 12:55
@haukepetersen haukepetersen removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 16, 2019
@haukepetersen
Copy link
Contributor Author

All dependencies are merged and I updated the NimBLE version accordingly. This PR is ready to be merged now.

miri64
miri64 approved these changes Dec 16, 2019
View reviewed changes
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.

ACK. @haukepetersen is doing a lot of experimenting with NimBLE at the moment and confirmed offline that he is using this version for that already (I've seen it), so no need for me to do more testing.

@benpicco
Copy link
Contributor

Murdock still found some issues.

@haukepetersen
Copy link
Contributor Author

In July the NimBLE GAP API changed slightly: the data of an advertising packet is now passed as const uint8_t *. This caused an error when passing this to the bluetil module in tests/nimble_l2cap. I have not noticed this prior, as most of my code is using the nimble_scanner module, which is passing a const pointer per default...

I pushed a fix for this, so now Murdock should be happy.

miri64
miri64 requested changes Dec 16, 2019
View reviewed changes
tests/nimble_l2cap/main.c
@@ -143,7 +143,7 @@ static void _filter_and_connect(struct ble_gap_disc_desc *disc)
int res;
bluetil_ad_t ad;

bluetil_ad_init(&ad, disc->data,
bluetil_ad_init(&ad, (uint8_t *)disc->data,
Copy link
Member

@miri64 miri64 Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Please add a comment why this is possible and won't lead to a crash.

@benpicco
Copy link
Contributor

Wouldn't it be better to change bluetil_ad_init() so it operates on const buffers?

@haukepetersen haukepetersen force-pushed the opt_nimble_vernov19andlldatalenfornetif branch from 02f1b4e to a612002 Compare December 16, 2019 14:58
@haukepetersen
Copy link
Contributor Author

Wouldn't it be better to change bluetil_ad_init() so it operates on const buffers?

Nope, as bluetil_ad can be used for both read and write.

@haukepetersen haukepetersen force-pushed the opt_nimble_vernov19andlldatalenfornetif branch from a612002 to 20af661 Compare December 16, 2019 15:00
miri64
miri64 approved these changes Dec 16, 2019
View reviewed changes
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.

ACK.

@haukepetersen
Copy link
Contributor Author

All green -> go.

@haukepetersen haukepetersen merged commit 5350b79 into RIOT-OS:master Dec 17, 2019
@haukepetersen haukepetersen deleted the opt_nimble_vernov19andlldatalenfornetif branch December 17, 2019 08:32
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

Assignees
No one assigned
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.
4 participants
@haukepetersen @benpicco @miri64 @fjmolinas