Bluetooth: Host: add unregister connection callback function

[zhjseu]

[Description]
tests: shell: Restart bt will register the same connection callback twice.
Callback next node point to itself, when link established callback function
loop infinitely.
[Fix]
Unregister the previous callback to avoid register repeatedly.
[Test]
After bt init/disable times, create connection successfully.

[Thalley]

Functionality looks good.

Some documentation should be updated, and a code style comment.

Finally, since this is adding a new function in the public API, we should test that it works as intended in CI.

[Thalley]

I think we should expand at least one of our tests (possibly babblesim (bsim) tests, or a unit test) to run and test this functionality.

[jori-nordic]

They added a test, could you re-review?

[Thalley]

Changes LGTM to me, but I do still think we should add a test of this before merging

[zhjseu]

Hello @Thalley , what should I do next?

[theob-pro]

Hello @Thalley , what should I do next?

Hi @zhjseu, I think the next thing to do would be to add a test (bsim or unit test) to ensure the new API work as intended, as @Thalley suggested it.

[zhjseu]

Hello @Thalley , what should I do next?

Hi @zhjseu, I think the next thing to do would be to add a test (bsim or unit test) to ensure the new API work as intended, as @Thalley suggested it.

HI @theob-pro ,
I don't know how to add a bsim or unit test, but to make sure this API work or not, can do the follow test,
bt init -> bt disable -> bt init -> connect remote device,
-> If connect remote device success, this API work.
-> If connect fail , this API not work.
thanks.

[Thalley]

Hello @Thalley , what should I do next?

Hi @zhjseu, I think the next thing to do would be to add a test (bsim or unit test) to ensure the new API work as intended, as @Thalley suggested it.

HI @theob-pro , I don't know how to add a bsim or unit test, but to make sure this API work or not, can do the follow test, bt init -> bt disable -> bt init -> connect remote device, -> If connect remote device success, this API work. -> If connect fail , this API not work. thanks.

The thing we are requesting is that is it being properly tested by CI, so that we can be sure it works for more than just the shell and that it continues to work if there are other changes.

It isn't too difficult to get started on babblesim (or even unit tests), so be don't afraid to look into it :)

[dleach02]

@Thalley, @theob-pro can you point/guide @zhjseu to where you expect the test cases to exist in the test tree?

[Thalley]

@Thalley, @theob-pro can you point/guide @zhjseu to where you expect the test cases to exist in the test tree?

I would create a new test under https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/bsim/bluetooth/host/misc that does the following:

1. Register callbacks
2. Create a connection and verify that callback is called
3. Disconnect the connection
4. Unregister callbacks
5. Create a new connection and verify that the callback is not called

[zhjseu]

@Thalley, @theob-pro can you point/guide @zhjseu to where you expect the test cases to exist in the test tree?

I would create a new test under https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/bsim/bluetooth/host/misc that does the following:

1. Register callbacks
2. Create a connection and verify that callback is called
3. Disconnect the connection
4. Unregister callbacks
5. Create a new connection and verify that the callback is not called

@Thalley, Thanks for your reply,
Are you going to add this test process or shall I do it ？
I never add bsim test case before, is that call the register conn callback and unregister conn callback function in the follow file?
/tests/bsim/bluetooth/host/misc/conn_stress/central/src/main.c
To verify code added no compile error, could you tell me which script to run to build the bsim image?
Thanks!

[Thalley]

@Thalley, @theob-pro can you point/guide @zhjseu to where you expect the test cases to exist in the test tree?

I would create a new test under https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/bsim/bluetooth/host/misc that does the following:

1. Register callbacks
2. Create a connection and verify that callback is called
3. Disconnect the connection
4. Unregister callbacks
5. Create a new connection and verify that the callback is not called

@Thalley, Thanks for your reply, Are you going to add this test process or shall I do it ？

I think you should do it if you are up for it :)

I never add bsim test case before, is that call the register conn callback and unregister conn callback function in the follow file? /tests/bsim/bluetooth/host/misc/conn_stress/central/src/main.c

I would create a new directory in https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/bsim/bluetooth/host/misc and create a new test from scratch (using e.g. https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/bsim/bluetooth/host/misc/disable as inspiration). That way we can have a specific test for this.

To verify code added no compile error, could you tell me which script to run to build the bsim image? Thanks!

You simply compile it as you would with any other application, but using the nrf52_bsim board. You can then a script similar to https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/bsim/bluetooth/host/misc/disable/tests_scripts/disable_with_gatt.sh to run and verify the test.

[zhaynxp]

Hi @Thalley , reviewers,

After setup BabbleSim environment in my local PC, compile (west build -p always -b nrf52_bsim -d xxx xxx) still failed says:
CMake Error at C:/home/nxf57794/Zephyr_projects/zephyr/arch/posix/CMakeLists.txt:4 (message):
The POSIX architecture only works on Linux. If on Windows or macOS
consider using a virtual machine to run a Linux guest.

Can't Windows PC be used to compile and run the app target to nrf52_bsim ? Currently we only have Zephyr env in Windows PC to facilitate development.
Could you help to guide here?
Thanks a lot!

[jori-nordic]

@zhaynxp yes bsim only works on linux. You can set up a virtual machine w/ ubuntu 22.04 and it should work in that environment.

[zhaynxp]

@zhaynxp yes bsim only works on linux. You can set up a virtual machine w/ ubuntu 22.04 and it should work in that environment.

Hi @jori-nordic , thanks for the info. More one question is what if the bsim test pass on our local linux setup? any material or evidence needed to submit to this ticket for clarification? Could you please share next step？

[jori-nordic]

If the new test passes on your local setup, the next step is to add it to the pull request introducing the change.
E.g. like I did here: I identified a bug, made a test that reproduces it by failing, and then added the fix that makes the test pass again: https://github.com/zephyrproject-rtos/zephyr/pull/67531/files

From this PR you can see what you need to add a new test.

[jori-nordic]

since we're only creating LE connections, you could move this to L105.

[zhaynxp]

noted, resolved this, thanks a lot!

Comments have been resolved

[zhaynxp]

Dear Reviewers,

There are still 2 checks failed to this PR, one is bsim-test failure as below:

the other is compliance check failure as below:

Is there anything we need to do on above failures? Please kindly let us know if anything pending on our side, really appreciated and many thanks!

[jori-nordic]

@zhaynxp seems the permissions are not set properly on the test you just made.
A chmod +x unregister_conn_cb.sh should do the trick.

As for the other error, the workflow used by zephyr PRs is to always force-push, and rebase on origin/main if necessary. Basically never do a merge or small edition commits in response to comments.

You can probably resolve that by doing a git rebase -i origin/main. In the interactive rebase, you can then remove the merge commit (if it shows up at all).

[zhaynxp]

@zhaynxp seems the permissions are not set properly on the test you just made. A chmod +x unregister_conn_cb.sh should do the trick.

As for the other error, the workflow used by zephyr PRs is to always force-push, and rebase on origin/main if necessary. Basically never do a merge or small edition commits in response to comments.

You can probably resolve that by doing a git rebase -i origin/main. In the interactive rebase, you can then remove the merge commit (if it shows up at all).

Hello @jori-nordic , all, thanks for the suggestions. After above 2 problems solved, a new error occured:

also found the target_prj_conf was not being built/compiled before running test, we've shared the _compile.sh in this PR, but don't understand how and when the bsim-test check will compile the target_prj_conf, could you please guide? Many thanks!

[jori-nordic]

you should add it below here:

zephyr/tests/bsim/bluetooth/host/compile.sh

Line 77 in 8534767

app=tests/bsim/bluetooth/host/misc/conn_stress/peripheral compile

edit: you can test that locally by running this from the zephyrproject/zephyr/ dir:

$(west topdir)/zephyr/tests/bsim/bluetooth/host/compile.sh && SEARCH_PATH=$(west topdir)/zephyr/tests/bsim/bluetooth/host/misc/unregister_conn_cb $(west topdir)/zephyr/tests/bsim/run_parallel.sh

[zhaynxp]

Hi @jori-nordic @Thalley , would you please approve this PR if no more changes are needed?
All CI checks passed now.
Many thanks here!

[jhedberg]

I'm approving since considering the current callback struct definition, this is the only way to do it and the PR has been open for long enough without me pointing the following out: would it perhaps have made more sense to just convert the callback list to sys_slist_t and use sys_snode_t in the callback struct instead of a custom linked list pointer? Then we don't have to do this reimplementation of the linked list manipulation code. Anyway, can be done as a follow-up effort as far as I'm concerned.