Skip to content

pkg/nimble: ver bump + config improvement#12775

Merged
haukepetersen merged 3 commits intoRIOT-OS:masterfrom
haukepetersen:opt_nimble_vernov19andlldatalenfornetif
Dec 17, 2019
Merged

pkg/nimble: ver bump + config improvement#12775
haukepetersen merged 3 commits intoRIOT-OS:masterfrom
haukepetersen:opt_nimble_vernov19andlldatalenfornetif

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Nov 21, 2019

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
Copy Markdown
Contributor Author

haukepetersen commented Nov 22, 2019

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
Copy Markdown
Contributor Author

haukepetersen commented Dec 16, 2019

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
Copy Markdown
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
Copy Markdown
Contributor

benpicco commented Dec 16, 2019

Murdock still found some issues.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 16, 2019

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
bluetil_ad_t ad;

bluetil_ad_init(&ad, disc->data,
bluetil_ad_init(&ad, (uint8_t *)disc->data,
Copy link
Copy Markdown
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
Copy Markdown
Contributor

benpicco commented Dec 16, 2019

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
Copy Markdown
Contributor Author

haukepetersen commented Dec 16, 2019

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
Copy Markdown
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
Copy Markdown
Contributor Author

haukepetersen commented Dec 17, 2019

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