Bluetooth: Mesh: Use BT_MESH_SUSPENDED flag in the legacy advertiser thread #80627
Conversation
PavelVPV
force-pushed
the
fix_legacy_adv_stop
branch
from
b4d3998
to
672d849
Compare
zephyrbot
requested review from
akredalen,
alxelax,
Andrewpini,
HaavardRei,
jhedberg,
LingaoM and
omkar3141
|
@valeriosetti, this PR should fix suspend tests in your PR. |
subsys/bluetooth/mesh/adv_legacy.c
Outdated
| /* The advertiser thread relies on BT_MESH_SUSPENDED flag. No point in starting the | ||
| * advertiser thread if the flag is not set. | ||
| */ | ||
| __ASSERT_NO_MSG(ADV_ENABLED); |
There was a problem hiding this comment.
Why assert here (considering asserts can be disabled), may be better to return -EADV?
There was a problem hiding this comment.
Why ever use asserts if they can be disabled?
There was a problem hiding this comment.
I guess the main purpose of assertion in assertion post action that is weak in Zephyr and customer is able to implement it on its own purpose. Generally, assertion is changed on hardware reset in final product as the only way for recovering device workability from unknown and unpredictable situation.
https://docs.zephyrproject.org/latest/kernel/services/other/fatal.html
Is this the real place where device requires hardware reset in "fields"?
It seems @omkar3141 suggestion is more suitable. Since functional prototype returns operation status, it is better to return error code.
subsys/bluetooth/mesh/adv_legacy.c
Outdated
| * thread will exit once the flag is set. The flag is set by the higher layer function. Here | ||
| * we need to check that the flag is dropped and ensure that the thread is stopped. | ||
| */ | ||
| __ASSERT_NO_MSG(!ADV_ENABLED); |
There was a problem hiding this comment.
This 'ensure' works only when asserts are enabled, otherwise this can cause locking, better to return error code to let higher layer handle it.
PavelVPV
force-pushed
the
fix_legacy_adv_stop
branch
from
672d849
to
8706ab0
Compare
subsys/bluetooth/mesh/adv_legacy.c
Outdated
| @@ -35,10 +35,11 @@ LOG_MODULE_REGISTER(bt_mesh_adv_legacy); | |||
| #define ADV_INT_DEFAULT_MS 100 | |||
| #define ADV_INT_FAST_MS 20 | |||
|
|
|||
| #define ADV_ENABLED (!atomic_test_bit(bt_mesh.flags, BT_MESH_SUSPENDED)) | |||
There was a problem hiding this comment.
I'd suggest to add an input parameter to this macro (either bt_mesh or bt_mesh.flags). Without it you get the impression that this macro evaluates to a build-time constant, which is not true.
Instead of checking the `enabled` flag, check if BT_MESH_SUSPENDED is set in the legacy advertiser thread. BT_MESH_SUSPENDED is set earlier than advertiser is stopped and will prevent the advertiser send anything earlier. Signed-off-by: Pavel Vasilyev <[email protected]>
k_sleep may not be enough to let advertiser send the message. Instead we should rely on the bt_mesh_send_cb. Signed-off-by: Pavel Vasilyev <[email protected]>
PavelVPV
force-pushed
the
fix_legacy_adv_stop
branch
from
8706ab0
to
5d9deb6
Compare
Instead of checking the
enabledflag, check if BT_MESH_SUSPENDED is set in the legacy advertiser thread. BT_MESH_SUSPENDED is set earlier than advertiser is stopped and will prevent the advertiser send anything earlier.This PR also replaces
k_sleepwith semaphore before suspending mesh as k_sleep may not be enough to let advertiser send the message. Instead we should rely on the bt_mesh_send_cb.This PR fixes mesh suspend tests in #79931
Fixes #80910