Skip to content

ble/nimble: bump version to 9d4bda2#11273

Merged
3 commits merged into
RIOT-OS:masterfrom
haukepetersen:opt_nimble_ver370305a
May 17, 2019
Merged

ble/nimble: bump version to 9d4bda2#11273
3 commits merged into
RIOT-OS:masterfrom
haukepetersen:opt_nimble_ver370305a

Conversation

@haukepetersen

@haukepetersen haukepetersen commented Mar 26, 2019

Copy link
Copy Markdown
Contributor

Contribution description

The current NimBLE version used has an issue when receiving large data packets via L2CAP COC. This was fixed upstream (apache/mynewt-nimble#378). So this PR updates NimBLE to include those fixe(es).

Testing procedure

Once #11235 is merged, use that test application to verify that sending/receiving large data packets at a high rate is working properly.

Issues/PRs references

fixes apache/mynewt-nimble#373
depends on #11235 (for proper testing)

@haukepetersen haukepetersen added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) State: waiting for other PR State: The PR requires another PR to be merged first Area: BLE Area: Bluetooth Low Energy support labels Mar 26, 2019
@haukepetersen

haukepetersen commented Mar 27, 2019

Copy link
Copy Markdown
Contributor Author

I'd like to put this PR on hold until apache/mynewt-nimble#392 is also merged into NimBLE. This way we save one version bump PR...

@haukepetersen haukepetersen mentioned this pull request Mar 27, 2019
Merged
@haukepetersen haukepetersen force-pushed the opt_nimble_ver370305a branch from 793f391 to f849dfd Compare April 29, 2019 08:37
@haukepetersen

haukepetersen commented Apr 29, 2019

Copy link
Copy Markdown
Contributor Author

apache/mynewt-nimble#392 was merged, so I updated the NimBLE version in this PR once more. The PR is now ready to be tested and merged.

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Apr 29, 2019
@haukepetersen haukepetersen mentioned this pull request Apr 29, 2019
Merged
@haukepetersen haukepetersen changed the title ble/nimble: bump version to 370305a ble/nimble: bump version to 8b45120 Apr 29, 2019
@haukepetersen

haukepetersen commented Apr 29, 2019

Copy link
Copy Markdown
Contributor Author

Turns out that NimBLE appears to have new issues since apache/mynewt-nimble#414. When running out tests/nimble_l2cap application, the send fails using this new event. Still looking into the cause...

@ghost

ghost commented May 3, 2019

Copy link
Copy Markdown

I still have failed assertion with packagelength > 2518 on client.
tests/nimble_l2cap/main.c:205 => 0xce9, MTU is 250 now.

Also when flooding the client does not print to pyterm after 23 packets - but it recovers after resetting the server. Did not debug this further.

@haukepetersen

haukepetersen commented May 3, 2019

Copy link
Copy Markdown
Contributor Author

Yes, thats exactly the faulty behavior. The problem ist, that somehow the internal send buffers are not processed properly, so that the BLE_L2CAP_EVENT_COC_TX_UNSTALLED event is triggered in an eternal loop, letting the node to constantly send the same packet over and over...

@haukepetersen haukepetersen mentioned this pull request May 16, 2019
Closed
@haukepetersen haukepetersen force-pushed the opt_nimble_ver370305a branch from b011a28 to 0fa0fb9 Compare May 16, 2019 15:28
@haukepetersen haukepetersen changed the title ble/nimble: bump version to 8b45120 ble/nimble: bump version to 9d4bda2 May 16, 2019
@haukepetersen

haukepetersen commented May 16, 2019

Copy link
Copy Markdown
Contributor Author

Alright, found the issue: the application code around the send function was flawed.

Updated this PR:

  • bumped Nimble version once more: 9d4bda2edcf0e7c2a03aa908321293a53361d258
  • fixed the test/nimble_l2cap test applications to make now proper use of the new BLE_L2CAP_EVENT_COC_TX_UNSTALLED event

This PR should be ready for final review now.

@ghost

ghost commented May 17, 2019

Copy link
Copy Markdown

Great, flood and inctest from /tests/nimble_l2cap to /tests/nimble_l2cap_server go through now.
Anything else I should test?

@haukepetersen haukepetersen force-pushed the opt_nimble_ver370305a branch from 0fa0fb9 to e172c80 Compare May 17, 2019 08:34
@ghost ghost 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: 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 labels May 17, 2019
@haukepetersen

haukepetersen commented May 17, 2019

Copy link
Copy Markdown
Contributor Author

you could just try the examples/nimble_gatt example and connect with a phone or similar. But there weren't any significant changes in that code path of nimble...

@ghost ghost added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label May 17, 2019
ghost
ghost approved these changes May 17, 2019
View reviewed changes

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested with my phone too. All tests work as desired and code looks good.
ACK

@ghost ghost merged commit 5c35ea8 into RIOT-OS:master May 17, 2019
@haukepetersen

haukepetersen commented May 17, 2019

Copy link
Copy Markdown
Contributor Author

Awesome, thanks a lot!

@haukepetersen haukepetersen deleted the opt_nimble_ver370305a branch May 17, 2019 09:26
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
Reviewers whose approvals may not affect merge requirements

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 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: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

L2CAP COC: buffer configuration errors

1 participant

@haukepetersen