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.

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

tests: Bluetooth: Add example bsim test #71053

Merged
merged 3 commits into from
Apr 10, 2024
Merged

tests: Bluetooth: Add example bsim test #71053

merged 3 commits into from
Apr 10, 2024

Conversation

jori-nordic
Copy link
Collaborator

This test is intended to teach the conventions and best practices to a
potential contributor that wants to add a test, for say a new feature.

It is liberally commented on purpose, so new contributors don't have to
spend two weeks understanding the intricacies of the simulator, the bsim
test framework, the native builds, backchannels, etc..

@zephyrbot zephyrbot added area: Documentation area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim labels Apr 3, 2024
@jori-nordic
Copy link
Collaborator Author

image

image

@jfischer-no Am I doing something wrong with the app Kconfig? It builds properly, and this is how most apps add custom options.

@aescolar aescolar assigned aescolar and unassigned jhedberg Apr 4, 2024
@aescolar
Copy link
Member

aescolar commented Apr 4, 2024

Am I doing something wrong with the app Kconfig? It builds properly, and this is how most apps add custom options.

Those 2 options are not defined in any Kconfig file

@jori-nordic
Copy link
Collaborator Author

jori-nordic commented Apr 4, 2024 •
edited
Loading

Those 2 options are not defined in any Kconfig file

But they are:
https://github.com/zephyrproject-rtos/zephyr/pull/71053/files#diff-6773bd5381942dbcabbb337e43aeeb37e6348a65fdc30453576c734bf1b54c1bR14

edit: also like I said, they're used in two source files and the test compiles without issue.

aescolar
aescolar requested changes Apr 4, 2024
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

Thanks @jori-nordic
A few requests

Thalley
Thalley requested changes Apr 4, 2024
View reviewed changes
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Great job!

Given the nature of purpose of this, I've been extra pedantic in my review :)

tests/bsim/bluetooth/host/misc/sample_test/src/peer.c Outdated
LOG_DBG("");

err = bt_testlib_gatt_discover_primary(&svc_handle, &svc_end_handle, conn, svc, 1, 0xffff);
if (err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto with using boolean values for boolean expressions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cmon, we have to ditch the whole of zephyr then. This is pretty much the idiomatic error handing in zephyr.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. The coding guidelines do state this though, but it also states that we are not yet at a level where this should be enforced, so it's not a blocking comment, but the plan is actually to modify all if statements in Zephyr to comply with that rule (at some point).

My personal opinion is that we might as well follow the to-be-followed rules now for new code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is a good opinion, I agree. I did make the change :).
Do we have a task-force for MISRA-ing the tree or is the plan to only enforce the rule on new PRs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a task-force for MISRA-ing the tree or is the plan to only enforce the rule on new PRs?

I think it's just been a low-effort thing from since like 2020 or something...

@jori-nordic
Copy link
Collaborator Author

I've been extra pedantic in my review

Damn I thought I had -fno-emil, not -extra-pedantic 😅

Thalley
Thalley requested changes Apr 5, 2024
View reviewed changes
@jori-nordic
Copy link
Collaborator Author

I s/TEST_APP_/APP_ for the log level to fool the CI. (It already has an exception for this)

@jori-nordic
Copy link
Collaborator Author

I s/TEST_APP_/APP_ for the log level to fool the CI. (It already has an exception for this)

seems compliance script is more broken than I thought...

@jori-nordic jori-nordic requested review from aescolar and Thalley April 5, 2024 09:39
Thalley
Thalley previously approved these changes Apr 5, 2024
View reviewed changes
jhedberg
jhedberg previously approved these changes Apr 5, 2024
View reviewed changes
@aescolar aescolar requested a review from tejlmand April 8, 2024 08:09
aescolar
aescolar reviewed Apr 8, 2024
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

Mostly minors at this point

tests/bsim/babblekit/CMakeLists.txt

# Helpers that can be used on bsim targets but don't have other
# dependencies (e.g. on Bluetooth, etc).
add_library(babblekit)
Copy link
Member

Choose a reason for hiding this comment

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

nit: common test_lib was fine enough :), with this new name users will not know what this is (a test for something called babblekit?) (I'm not going to block it due to this name and folder though :) )

@jori-nordic jori-nordic dismissed stale reviews from jhedberg and Thalley via 7c52dff April 8, 2024 11:04
@jori-nordic jori-nordic force-pushed the explain-the-bs branch 2 times, most recently from 7c52dff to e5cf620 Compare April 8, 2024 13:06
@jori-nordic
Copy link
Collaborator Author

@aescolar and @Thalley I removed the timeout callback machinery.
Replaced by a much simpler callback on device teardown.

@aescolar it seems I can't call bs_trace_error from it though (infinite recursion? loop).
So I defined a new helper TEST_PRINT so we can easily make changes when the framework is updated.

@aescolar
Copy link
Member

aescolar commented Apr 8, 2024

@aescolar it seems I can't call bs_trace_error from it though (infinite recursion? loop).

from all exit hooks you need to be careful that they may reenter (specially if you are yourself calling exit from them)

@jori-nordic
Copy link
Collaborator Author

@aescolar it seems I can't call bs_trace_error from it though (infinite recursion? loop).

from all exit hooks you need to be careful that they may reenter (specially if you are yourself calling exit from them)

Ah but then it seems anything with trace level ERROR will re-enter exit then.

I tried calling bs_trace_print(BS_TRACE_ERROR, (ie not the bs_trace_error macro) but failed the same way.
Anyway, the print is functional just doesn't have colors. We can change the macro later.

@jori-nordic
tests: Bluetooth: Add common helpers for bsim
725dba7 
This code can be found (kinda) duplicated all over
`tests/bsim/bluetooth`.

Put it in a common place.

Refactoring the current tests will be done in a future commit.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic
tests: Bluetooth: Add example bsim test
d4d0ead 
This test is intended to teach the conventions and best practices to a
potential contributor that wants to add a test, for say a new feature.

It is liberally commented on purpose, so new contributors don't have to
spend two weeks understanding the intricacies of the simulator, the bsim
test framework, the native builds, backchannels, etc..

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic
scripts: Add APP_LOG_LEVEL_DBG
11e2757 
Someone should really fix this properly..

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
alwa-nordic
alwa-nordic reviewed Apr 9, 2024
View reviewed changes
tests/bsim/babblekit/include/babblekit/testcase.h
#define TEST_START(msg, ...) \
do { \
bst_result = In_progress; \
bs_trace_info_time(2, "Test start: " msg "\n", ##__VA_ARGS__); \
Copy link
Collaborator

Choose a reason for hiding this comment

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

PSA: Explanation of , ##__VA_ARGS__: https://stackoverflow.com/questions/5588855/standard-alternative-to-gccs-va-args-trick

@nashif nashif merged commit 55533bc into zephyrproject-rtos:main Apr 10, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aescolar aescolar aescolar approved these changes

@theob-pro theob-pro theob-pro approved these changes

@jhedberg jhedberg jhedberg left review comments

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@carlescufi carlescufi Awaiting requested review from carlescufi

@hermabe hermabe Awaiting requested review from hermabe

@kartben kartben Awaiting requested review from kartben

@nashif nashif Awaiting requested review from nashif

@sjanc sjanc Awaiting requested review from sjanc

@Vudentz Vudentz Awaiting requested review from Vudentz

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@keith-zephyr keith-zephyr Awaiting requested review from keith-zephyr

@tejlmand tejlmand Awaiting requested review from tejlmand

@alwa-nordic alwa-nordic Awaiting requested review from alwa-nordic

@Thalley Thalley Awaiting requested review from Thalley

Assignees

@aescolar aescolar

Labels
area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth area: Coding Guidelines Coding guidelines and style area: Continuous Integration area: Documentation platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jori-nordic @aescolar @jhedberg @Thalley @alwa-nordic @theob-pro @nashif @zephyrbot