Bluetooth: Mesh: fix NO_ACTIONS scheduling #20054
Conversation
github-actions
bot
added
changelog-entry-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: be56440eaa9be9505faeb964d2a2d9f2a783ea6d more detailssdk-nrf:
Github labels
List of changed files detected by CI (1)Outputs:ToolchainVersion: 342151af73 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
| @@ -639,6 +639,12 @@ static int action_set(const struct bt_mesh_model *model, struct bt_mesh_msg_ctx | |||
| run_scheduler(srv); | |||
| } | |||
|
|
|||
| /* If the next scheduled action is set to "NO_ACTIONS", cancel it. */ | |||
| if ((srv->sch_reg[idx].action == BT_MESH_SCHEDULER_NO_ACTIONS) && (srv->idx == idx)) { | |||
There was a problem hiding this comment.
What will happen if BT_MESH_SCHEDULER_NO_ACTIONS came for not ongoing action?
There was a problem hiding this comment.
Good point, changed it to check whether the bitmap is active for the corresponding index, and to only run the scheduler if the canceled task is first in line.
Won't it be cleared here? https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/bluetooth/mesh/scheduler_srv.c#L647 |
HaavardRei
force-pushed
the
mesh_scheduler_srv_cancel_work
branch
from
e5db06a to
f8b36f9
Compare
They're in-the-loop through Jira, I'll ping them once it's merged :) |
| /* If the next scheduled action is set to "NO_ACTIONS", schedule the next in line */ | ||
| if (srv->idx == idx) { | ||
| run_scheduler(srv); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic can cause buggy behavior.
Imagine:
- Only single schedule register was set with valid action. So, active_bitmap has exactly one bit set, and the action is scheduled.
- User immediately issues another action_set, changing the same register index to NO_ACTION but leaves all other fields unchanged.
- Now, flow reaches here, and since there is just one action (which is now NO_ACTION),
run_scheduler()returns early. - Nothing is changed, and the stale scheduled work still continues.
What is needed, I think, is this:
| /* If the next scheduled action is set to "NO_ACTIONS", schedule the next in line */ | |
| if (srv->idx == idx) { | |
| run_scheduler(srv); | |
| } | |
| } | |
| /* If the next scheduled action is set to "NO_ACTIONS", schedule the next in line */ | |
| if (srv->idx == idx) { | |
| cancel_action(srv, idx); | |
| run_scheduler(srv); | |
| } | |
| } |
It will also be useful to extend the existing tests to cover this bugfix.
There was a problem hiding this comment.
I missed the case where there's no other upcoming task. Now the server indices are cleared (set to BT_MESH_SCHEDULER_ACTION_ENTRY_COUNT) and the run_scheduler function will cancel the work in this case.
HaavardRei
force-pushed
the
mesh_scheduler_srv_cancel_work
branch
from
f8b36f9 to
1e05d9e
Compare
If the Scheduler Setup Server receives a Scheduler Action Set message setting the next scheduled task to NO_ACTIONS, it will schedule the next task in line. Previously, the "canceled" task would be executed if it was first in line. Signed-off-by: Håvard Reierstad <[email protected]>
HaavardRei
force-pushed
the
mesh_scheduler_srv_cancel_work
branch
from
1e05d9e to
be56440
Compare
alxelax
removed
the
changelog-entry-required
If the Scheduler Setup Server receives a Scheduler Action Set message setting the next scheduled task to NO_ACTIONS, it will schedule the next task in line.
Previously, the "canceled" task would be executed if it was first in line.