tests: bluetooth: mesh: remove tinycrypt support from bsim mesh #90045

alxelax · 2025-05-16T07:43:58Z

Commit removes Tinycrypt support from bsim mesh tests as well as optimizes test suite number and features.

PavelVPV

Please explain changes in details (why each config is moved). It will be useful in future and could speed up review.

It would be also useful if each change is done in a separate commit (e.g removal of overlay_psa.conf should be in a separate commit).

I also didn't get change with overlay_legacy.conf and overlay_low_lat.conf. Because overlay_low_lat.conf still present. This should be explained in the commit message.

PavelVPV · 2025-05-19T06:15:32Z

tests/bsim/bluetooth/mesh/src/test_suspend.c

@@ -194,14 +194,9 @@ extern const struct bt_mesh_comp comp;
 * with a 100ms interval.
 */
 static struct bt_mesh_test_gatt gatt_param = {
-#if defined(CONFIG_BT_EXT_ADV)


Why this was removed?

PavelVPV · 2025-05-19T06:18:49Z

tests/bsim/bluetooth/mesh/tests_scripts/advertiser/disable.sh

@@ -34,8 +34,8 @@ source $(dirname "${BASH_SOURCE[0]}")/../../_mesh_test.sh
 RunTest mesh_adv_disable adv_tx_disable adv_rx_disable

 # Low latency overlay uses legacy advertiser
-overlay=overlay_low_lat_conf
-RunTest mesh_adv_disable adv_tx_disable adv_rx_disable
+overlay=overlay_legacy_conf


Why this was changed from overlay_low_lat_conf to overlay_legacy_conf? I don't see any changes related to overlay_low_lat_conf. Such change should be done in a separate commit with the reasoning.

PavelVPV · 2025-05-19T06:21:16Z

tests/bsim/bluetooth/mesh/tests_scripts/friendship/msg_mesh.sh

@@ -26,8 +26,8 @@ RunTest mesh_friendship_msg_mesh_lpn_scan_on \
 	friendship_other_msg \
 	friendship_friend_est

-overlay=overlay_psa_conf
-RunTest mesh_friendship_msg_mesh_psa \
+overlay=overlay_low_lat_conf


Same as above. Separate commit with reasoning.

PavelVPV · 2025-05-19T06:24:06Z

tests/bsim/bluetooth/mesh/tests_scripts/suspend/suspend_disable_resume.sh

-
-overlay=overlay_psa_conf
-RunTest mesh_suspend_disable_resume_psa \
+overlay=overlay_legacy_conf


Same as above. Should go to a separate commit.

PavelVPV · 2025-05-19T06:24:27Z

tests/bsim/bluetooth/mesh/tests_scripts/suspend/suspend_resume.sh

-
-overlay=overlay_psa_conf
-RunTest mesh_suspend_resume_psa \
+overlay=overlay_legacy_conf


Same as above. Should go to a separate commit.

PavelVPV · 2025-05-19T06:24:41Z

tests/bsim/bluetooth/mesh/tests_scripts/transport/loopback_group.sh

@@ -6,5 +6,5 @@ source $(dirname "${BASH_SOURCE[0]}")/../../_mesh_test.sh

 RunTest mesh_transport_loopback_group transport_tx_loopback_group transport_rx_group

-overlay=overlay_psa_conf
-RunTest mesh_transport_loopback_group_psa transport_tx_loopback_group transport_rx_group
+overlay=overlay_low_lat_conf


Same as above. Should go to a separate commit.

PavelVPV · 2025-05-19T06:24:51Z

tests/bsim/bluetooth/mesh/tests_scripts/transport/unicast.sh

@@ -6,5 +6,5 @@ source $(dirname "${BASH_SOURCE[0]}")/../../_mesh_test.sh

 RunTest mesh_transport_unicast transport_tx_unicast transport_rx_unicast

-overlay=overlay_psa_conf
-RunTest mesh_transport_unicast_psa transport_tx_unicast transport_rx_unicast
+overlay=overlay_low_lat_conf


Same as above. Should go to a separate commit.

alxelax · 2025-05-19T08:29:53Z

Please explain changes in details (why each config is moved). It will be useful in future and could speed up review.

It would be also useful if each change is done in a separate commit (e.g removal of overlay_psa.conf should be in a separate commit).

I also didn't get change with overlay_legacy.conf and overlay_low_lat.conf. Because overlay_low_lat.conf still present. This should be explained in the commit message.

Several tests were added only for Tinycrypt library and were never run with PSA Crypto. Especially these are tests related to advertiser or suspending functionality. Mesh has two advertiser implementations. Some legacy advertiser functionality was NEVER run in combination with PSA.
Furthermore, the low latency feature of the legacy advertiser has been added a long time ago to suitable test suites like transport and friendship data transmission.
Later when another tests were added those required legacy advertiser checking the low latency overlay has been used instead of adding pure legacy advertiser.
That caused a situation where actually the low latency advertiser has never been tested without the low latency feature.
And even worse case, we started adapting the mesh tests to the low latency feature: #90045 (comment)
These 2 maintenance mistakes:

Running some tests only with Tinycrypt
Using the legacy advertiser always wtih the low latency feature

caused the situation where it started multiplying misbehavior. Some tests became fully not functional after removing Tinycrypt library.

I'm talking about these two:
gatt_suspend_resume.sh
gatt_suspend_disable_resume.sh

The legacy advertiser does not send data after switching from gatt to adv with low latency feature. This is not a timing issue. There are no advertising packets at all. They appear only after suspending\resuming, that means for Host\Controller initialization\start functionality triggering.
However, when the low latency feature is disabled, the legacy advertiser works as Mesh test scenarios expect.

I have no idea how the low latency feature should behave in case of switching between gatt and adv. I suspect this is correct behavior since the low latency assumes some delay in advertising, but Mesh mandates to send it immediately.
Not only that, but I do not know how PSA impacts on Host\Controller in aspect of this feature.

For my opinion, there should be fixed the cause of the problem. I mean usage exact features that we want to test, instead of usage of some combination of features because overlay exists.

This is the reason I removed the low latency overlay everywhere where the low latency feature is not tested. If we want to test behavior of the legacy advertiser, then the only legacy advertiser should be added. Nothing more.

Splitting it on several commits impossible since it breaks requirement for them to pass CI.

PavelVPV · 2025-05-19T09:00:21Z

@alxelax , this should be explained in the commit message. We should respect reviewers and their time. Currently I need to guess why each thing is done this way or the other.

Splitting it on several commits impossible since it breaks requirement for them to pass CI.

I'm not sure I agree since we don't change implementation.

alxelax · 2025-05-19T09:05:29Z

I'm not sure I agree since we don't change implementation.

Hm. Switching crypto library changes implementation significantly.

PavelVPV · 2025-05-19T09:11:55Z

I'm not sure I agree since we don't change implementation.

Hm. Switching crypto library changes implementation significantly.

Your PR and commit message states:

Commit removes Tinycrypt support from bsim mesh tests as well as optimizes test suite number and features.

From which I make an assumption that this PR should just remove _psa overlays and merge PSA related configuration to the base configs. Thus CI shouldn't brake between changes since we were running with PSA before, right?

Now you say this PR changes implementation significantly...

What is correct?

Commit removes Tinycrypt support from bsim mesh tests as well as optimizes test suite number and features. Several tests were added only for Tinycrypt library and were never run with PSA Crypto. Especially these are tests related to advertiser or suspending functionality. Mesh has two advertiser implementations. Some legacy advertiser functionality was NEVER run in combination with PSA. Furthermore, the low latency feature of the legacy advertiser has been added a long time ago to suitable test suites like transport and friendship data transmission. Later when another tests were added those required legacy advertiser checking the low latency overlay has been used instead of adding pure legacy advertiser. That caused a situation where actually the low latency advertiser has never been tested without the low latency feature. And even worse case, we started adapting the mesh tests to the low latency feature. These 2 maintenance mistakes: Running some tests only with Tinycrypt Using the legacy advertiser always wtih the low latency feature caused the situation where it started multiplying misbehavior. Some tests became fully not functional after removing Tinycrypt library. I'm talking about these two: gatt_suspend_resume.sh gatt_suspend_disable_resume.sh The legacy advertiser does not send data after switching from gatt to adv with low latency feature. The low latency overlay was changed to the legacy overlay everywhere where the low latency feature is not tested. If we want to test behavior of the legacy advertiser, then the only legacy advertiser should be added. Nothing more. Signed-off-by: Aleksandr Khromykh <[email protected]>

sonarqubecloud · 2025-05-19T11:27:47Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

alxelax · 2025-05-19T14:37:50Z

I'm not sure I agree since we don't change implementation.

Hm. Switching crypto library changes implementation significantly.

Your PR and commit message states:

Commit removes Tinycrypt support from bsim mesh tests as well as optimizes test suite number and features.

From which I make an assumption that this PR should just remove _psa overlays and merge PSA related configuration to the base configs. Thus CI shouldn't brake between changes since we were running with PSA before, right?

Now you say this PR changes implementation significantly...

What is correct?

Both are correct. Could it be?

PavelVPV · 2025-05-19T19:10:01Z

Both are correct. Could it be?

If some tests are removed then CI should still work.

If some tests are changed, this should be done in a separate commit.

The removal of overlay_psa.conf and overlay_ss.conf can be done in a separate commit.
Addition of overlay_legacy.conf can be done in a separate commit.
Things like this can be done in a separate commit.

Please follow rules for PR process: https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-requirements
Specifically:

Commits in a pull request should represent clear, logical units of change that are easy to review and maintain bisectability. The following guidelines expand on this principle:

Distinct, Logical Units of Change

Each commit should correspond to a self-contained, meaningful change. For example, adding a feature, fixing a bug, or refactoring existing code should be separate commits. Avoid mixing different types of changes (e.g., feature implementation and unrelated refactoring) in the same commit.

github-actions bot added area: Bluetooth platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: Bluetooth Mesh labels May 16, 2025

github-actions bot requested review from aescolar, akredalen, Andrewpini, HaavardRei, jhedberg, KyraLengfeld, LingaoM, omkar3141 and PavelVPV May 16, 2025 07:44

github-actions bot assigned PavelVPV May 16, 2025

alxelax force-pushed the remove_tc_from_bsim_mesh branch from de97fda to 59a04db Compare May 16, 2025 08:08

PavelVPV requested changes May 19, 2025

View reviewed changes

alxelax requested a review from PavelVPV May 19, 2025 08:30

alxelax force-pushed the remove_tc_from_bsim_mesh branch from 59a04db to 2a89430 Compare May 19, 2025 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: bluetooth: mesh: remove tinycrypt support from bsim mesh #90045

tests: bluetooth: mesh: remove tinycrypt support from bsim mesh #90045

alxelax commented May 16, 2025

PavelVPV left a comment

PavelVPV May 19, 2025

PavelVPV May 19, 2025

PavelVPV May 19, 2025

PavelVPV May 19, 2025

PavelVPV May 19, 2025

PavelVPV May 19, 2025

PavelVPV May 19, 2025

alxelax commented May 19, 2025 •

edited

Loading

PavelVPV commented May 19, 2025

alxelax commented May 19, 2025

PavelVPV commented May 19, 2025

sonarqubecloud bot commented May 19, 2025

alxelax commented May 19, 2025

PavelVPV commented May 19, 2025

tests: bluetooth: mesh: remove tinycrypt support from bsim mesh #90045

Are you sure you want to change the base?

tests: bluetooth: mesh: remove tinycrypt support from bsim mesh #90045

Conversation

alxelax commented May 16, 2025

PavelVPV left a comment

Choose a reason for hiding this comment

PavelVPV May 19, 2025

Choose a reason for hiding this comment

PavelVPV May 19, 2025

Choose a reason for hiding this comment

PavelVPV May 19, 2025

Choose a reason for hiding this comment

PavelVPV May 19, 2025

Choose a reason for hiding this comment

PavelVPV May 19, 2025

Choose a reason for hiding this comment

PavelVPV May 19, 2025

Choose a reason for hiding this comment

PavelVPV May 19, 2025

Choose a reason for hiding this comment

alxelax commented May 19, 2025 • edited Loading

PavelVPV commented May 19, 2025

alxelax commented May 19, 2025

PavelVPV commented May 19, 2025

sonarqubecloud bot commented May 19, 2025

Quality Gate passed

alxelax commented May 19, 2025

PavelVPV commented May 19, 2025

alxelax commented May 19, 2025 •

edited

Loading