Bluetooth: Controller: Ticker Oust Expire feature #99871
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces the "Ticker Oust Expire" feature to the Bluetooth Controller, which allows ticker nodes to perform shallow expiry callbacks when they are ousted (preempted) by higher-priority nodes with the force flag set, rather than being completely skipped.
Key Changes
- Adds new
oust_expireflag to ticker nodes alongside the existingmust_expireflag to enable selective expiry behavior when ousted - Modifies collision resolution logic to track when nodes are ousted by force and communicate this to expiry callbacks via a new bitmask
- Implements supervision timeout handling in peripheral connections when oust_expire events occur
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| subsys/bluetooth/controller/ticker/ticker.h | Adds constants and macros for oust_expire feature (TICKER_NULL_OUST_EXPIRE and TICKER_LAZY_OUST_EXPIRE_BITMASK) |
| subsys/bluetooth/controller/ticker/ticker.c | Implements core oust_expire logic including new struct field, modified collision resolution, and expiry handling with bitmask signaling |
| subsys/bluetooth/controller/ll_sw/ull_peripheral.c | Integrates oust_expire in peripheral connections with supervision timeout monitoring and force flag updates when threshold is reached |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LL_ASSERT_MSG(false, "%s: %p Ousted %u lazy %u\n", __func__, | ||
| &conn->lll, conn->event_counter, lazy); |
There was a problem hiding this comment.
Error handling issue: The assertion at line 602 will cause a crash when supervision_expire <= elapsed_event. According to the comment at line 602, this is the "Ousted" case where the supervision timeout has been exceeded. However, using LL_ASSERT_MSG(false, ...) unconditionally crashes the system. If this is an expected condition that can occur during normal operation when a connection is heavily ousted, it should handle the situation gracefully (e.g., trigger a disconnect) rather than asserting. If it's truly a logic error that should never happen, the message should explain why this state is impossible.
| LL_ASSERT_MSG(false, "%s: %p Ousted %u lazy %u\n", __func__, | |
| &conn->lll, conn->event_counter, lazy); | |
| /* Supervision timeout expired: gracefully disconnect */ | |
| ull_conn_disconnect(conn, BT_HCI_ERR_CONN_TIMEOUT); |
| #define TICKER_LAZY_MUST_EXPIRE_KEEP 0xFFFE | ||
| #define TICKER_LAZY_MUST_EXPIRE_KEEP 0xFFFE | ||
|
|
||
| /* Use to ensure callback is invoked in ousted intervals, and maitains latencies. |
There was a problem hiding this comment.
Unclear documentation: The comment "Use to ensure callback is invoked in ousted intervals, and maitains latencies" (note: maitains should be maintains) is unclear about what "ousted intervals" means and how this differs from TICKER_LAZY_MUST_EXPIRE. Consider expanding this comment to explain: (1) what it means for a ticker to be "ousted", (2) how this relates to the force flag in collision resolution, and (3) why latencies are maintained in this case but not with MUST_EXPIRE.
| /* Use to ensure callback is invoked in ousted intervals, and maitains latencies. | |
| /* | |
| * Use to ensure the callback is invoked for intervals that were "ousted"— | |
| * i.e., intervals that were skipped or preempted due to scheduling conflicts | |
| * with higher-priority ticker nodes. When this bitmask is set, the ticker | |
| * will maintain the original latency and timing for these ousted intervals, | |
| * invoking the callback as if the intervals had not been missed, rather than | |
| * forcing immediate expiration of all missed intervals. | |
| * | |
| * This differs from TICKER_LAZY_MUST_EXPIRE, which forces the callback to | |
| * be invoked for all missed intervals immediately, potentially disregarding | |
| * the intended latency and timing. In contrast, TICKER_LAZY_OUST_EXPIRE_BITMASK | |
| * preserves the latency by only invoking callbacks for intervals that were | |
| * ousted due to collision resolution, and does not force expiration for all | |
| * missed intervals. | |
| * | |
| * The force flag in collision resolution determines whether the callback | |
| * should be invoked for ousted intervals. Using this bitmask allows the | |
| * ticker to maintain the expected timing behavior, which is important for | |
| * use cases where latency and interval accuracy are required. |
| uint8_t oust_expire:1; /* If non-zero, node timeout should | ||
| * expire if ousted by force by a | ||
| * node with force set. |
There was a problem hiding this comment.
[nitpick] Unclear documentation: The comment "If non-zero, node timeout should expire if ousted by force by a node with force set" is somewhat redundant ("ousted by force" and "with force set"). Consider clarifying to: "If non-zero, node should perform shallow expiry when ousted by a higher-priority node with the force flag set, rather than being completely skipped."
| uint8_t oust_expire:1; /* If non-zero, node timeout should | |
| * expire if ousted by force by a | |
| * node with force set. | |
| uint8_t oust_expire:1; /* If non-zero, node should perform shallow expiry when ousted by a higher-priority node with the force flag set, rather than being completely skipped. |
| #define TICKER_LAZY_MUST_EXPIRE_KEEP 0xFFFE | ||
| #define TICKER_LAZY_MUST_EXPIRE_KEEP 0xFFFE | ||
|
|
||
| /* Use to ensure callback is invoked in ousted intervals, and maitains latencies. |
There was a problem hiding this comment.
Spelling error: "maitains" should be "maintains".
| /* Use to ensure callback is invoked in ousted intervals, and maitains latencies. | |
| /* Use to ensure callback is invoked in ousted intervals, and maintains latencies. |
| return 1u; | ||
| } | ||
|
|
||
| if ((equal_priority != 0U) && (equal_force != 0U) && | ||
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | ||
| return 1u; |
There was a problem hiding this comment.
Inconsistent literal suffix: line 1036 uses 1u (lowercase 'u') while line 1041 also uses 1u. Both should use uppercase 1U for consistency with the rest of the codebase (see lines 1022, 1027, 1032).
| return 1u; | |
| } | |
| if ((equal_priority != 0U) && (equal_force != 0U) && | |
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | |
| return 1u; | |
| return 1U; | |
| } | |
| if ((equal_priority != 0U) && (equal_force != 0U) && | |
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | |
| return 1U; |
| } else { | ||
| lazy = ticker->lazy_current | TICKER_LAZY_OUST_EXPIRE_BITMASK; |
There was a problem hiding this comment.
Logic bug: When must_expire_skip is 1, the code should only use TICKER_LAZY_OUST_EXPIRE_BITMASK if the node was actually ousted by force. However, the condition at line 1422 only checks ticker->must_expire without verifying the oust variable. If a node has oust_expire set but enters the skip path for other reasons (e.g., slot_reserved or ticks_slot_previous conditions at lines 1380-1381), oust will be 0, yet the code will still set the bitmask at line 1423. This incorrectly signals to the callback that the node was ousted when it wasn't. The condition should be: else if ((ticker->oust_expire != 0U) && (oust != 0U)).
| } else { | |
| lazy = ticker->lazy_current | TICKER_LAZY_OUST_EXPIRE_BITMASK; | |
| } else if ((ticker->oust_expire != 0U) && (oust != 0U)) { | |
| lazy = ticker->lazy_current | TICKER_LAZY_OUST_EXPIRE_BITMASK; | |
| } else { | |
| lazy = ticker->lazy_current; |
Ticker Oust Expire feature. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Update Peripheral role to use Ticker Oust Expire feature. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
cvinayak
force-pushed
the
github_ticker_oust_expire_feature
branch
from
9377480 to
181d76b
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return 1u; | ||
| } | ||
|
|
||
| if ((equal_priority != 0U) && (equal_force != 0U) && | ||
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | ||
| return 1u; |
There was a problem hiding this comment.
Inconsistent return value literal: use 1U instead of 1u for consistency with line 1027, 1032, and other returns in the function.
| return 1u; | |
| } | |
| if ((equal_priority != 0U) && (equal_force != 0U) && | |
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | |
| return 1u; | |
| return 1U; | |
| } | |
| if ((equal_priority != 0U) && (equal_force != 0U) && | |
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | |
| return 1U; |
| return 1u; | ||
| } | ||
|
|
||
| if ((equal_priority != 0U) && (equal_force != 0U) && | ||
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | ||
| return 1u; |
There was a problem hiding this comment.
Inconsistent return value literal: use 1U instead of 1u for consistency with line 1027, 1032, and other returns in the function.
| return 1u; | |
| } | |
| if ((equal_priority != 0U) && (equal_force != 0U) && | |
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | |
| return 1u; | |
| return 1U; | |
| } | |
| if ((equal_priority != 0U) && (equal_force != 0U) && | |
| (next_is_older != 0U) && (next_not_ticks_slot_window != 0U)) { | |
| return 1U; |
| uint32_t ticks_to_expire; | ||
| uint8_t must_expire_skip; | ||
| uint32_t ticks_drift; | ||
| uint16_t lazy; |
There was a problem hiding this comment.
The variable lazy is declared at line 1340 but only initialized within conditional blocks (lines 1422-1427). If the condition at line 1379 is false (when ticker_ticks_slot == 0U or none of the collision conditions are met), lazy will be used uninitialized at lines 1494 and 1502. Consider initializing lazy at declaration or ensuring all code paths initialize it before use.
| uint16_t lazy; | |
| uint16_t lazy = 0U; |
1 participant
Ticker Oust Expire feature.