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

tests: add nimble_l2cap test and benchmark applications #11235

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

haukepetersen
Copy link
Contributor

@haukepetersen haukepetersen commented Mar 21, 2019

Contribution description

I continuously ran into buffering trouble with nimble connected to the buffer handling for connection oriented L2CAP channels. As I got tired debugging this, I decided its time for an automated and easy to use test application that can be used to stress-test these L2CAP COC connections. So this PR provides just that.

This test is still missing a fitting pexpect script for its make test target - this will be added soon in a follow up PR.

Testing procedure

Take two NimBLE capable boards (e.g. nrf52dk or nrf52840dk) and flash the client and server application on them. Once they are both started, they should automatically connect and you can run the flood and inctest commands with any parameters... They should never crash :-)

Issues/PRs references

This PR depends (is reabsed on) #11232 and #11233

@haukepetersen haukepetersen added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT 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 21, 2019
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 for the testing for now (I used this to test #11233).

@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 26, 2019
@miri64
Copy link
Member

miri64 commented Mar 26, 2019

Please rebase.

@haukepetersen
Copy link
Contributor Author

rebased

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.

Code-wise it looks good (can't say much about the functionality though, but obviously it works). Some nit-picks about Makefiles and style though.

size_t step = 10;
size_t limit = APP_MTU;

if ((argc == 2) && strncmp(argv[1], "help", 4) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

parens missing

static uint32_t _rxbuf[APP_MTU / 4];
static uint32_t _txbuf[APP_MTU / 4];


Copy link
Member

Choose a reason for hiding this comment

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

Extra newline

size_t pktsize = PKTSIZE_DEFAULT;
unsigned limit = FLOOD_DEFAULT;

if ((argc == 2) && strncmp(argv[1], "help", 4) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

parens missing

USEMODULE += bluetil_ad

# Get the shared NimBLE and test configuration from the backend server
include $(CURDIR)/../nimble_l2cap_server/nimble.inc.mk
Copy link
Member

Choose a reason for hiding this comment

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

Better use $(RIOTBASE) here. I had problems with this construct in #11256.

USEMODULE += bluetil_ad

# Get the shared NimBLE and test configuration from the backend server
include $(CURDIR)/../nimble_l2cap_server/nimble.inc.mk
Copy link
Member

Choose a reason for hiding this comment

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

Dito

static bluetil_ad_t _ad;
static struct ble_gap_adv_params _adv_params = { 0 };


Copy link
Member

Choose a reason for hiding this comment

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

Extra new-line

@haukepetersen
Copy link
Contributor Author

addressed comments and cppcheck findings.

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

@miri64 miri64 merged commit 2c012a2 into RIOT-OS:master Mar 26, 2019
@haukepetersen haukepetersen deleted the add_tests_nimblel2cap branch March 26, 2019 20:07
@danpetry danpetry added this to the Release 2019.04 milestone Apr 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 Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants