Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluetooth: controller: Fix ticker catch up on ISR latency #23815

Merged
merged 6 commits into from
Apr 1, 2020
Merged

Bluetooth: controller: Fix ticker catch up on ISR latency #23815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
44f3d20
Bluetooth: controller: Fix ticker catch up on ISR latency
cvinayak Mar 26, 2020
b3001e4
Bluetooth: controller: Remove lazy handling in ticker_worker
cvinayak Mar 27, 2020
7e5765b
Bluetooth: controller: Fix ticker state on skipped
cvinayak Mar 27, 2020
6db710e
Bluetooth: controller: Fix ticks_slot_previous calculation
cvinayak Mar 27, 2020
1c63b8e
Bluetooth: controller: Consider must_expire while avoiding catchup
cvinayak Mar 27, 2020
32a77ef
Bluetooth: controller: nRF: Revert use of ticker compat mode as default
cvinayak Mar 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions subsys/bluetooth/controller/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ config BT_LLL_VENDOR_NORDIC
select BT_HAS_HCI_VS
select ENTROPY_NRF5_RNG
select ENTROPY_NRF5_BIAS_CORRECTION

select BT_CTLR_LE_ENC_SUPPORT if !BT_CTLR_DATA_LENGTH_CLEAR && \
!BT_CTLR_PHY_2M_NRF
select BT_CTLR_CONN_PARAM_REQ_SUPPORT
Expand All @@ -137,11 +136,6 @@ config BT_LLL_VENDOR_NORDIC
select BT_CTLR_XTAL_ADVANCED_SUPPORT
select BT_CTLR_SCHED_ADVANCED_SUPPORT
select BT_CTLR_TIFS_HW_SUPPORT

# Until ticker resolve collision is fixed for skipped ticker catch-up
# issue, use the old compatibility mode
select BT_TICKER_COMPATIBILITY_MODE

default y
help
Use Nordic Lower Link Layer implementation.
Expand Down
137 changes: 92 additions & 45 deletions subsys/bluetooth/controller/ticker/ticker.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,23 +638,18 @@ static u32_t ticker_dequeue(struct ticker_instance *instance, u8_t id)
static u8_t ticker_resolve_collision(struct ticker_node *nodes,
struct ticker_node *ticker)
{
u8_t skipped = 0;
s32_t lazy_current = ticker->lazy_current;
if ((ticker->priority != TICKER_PRIORITY_CRITICAL) &&
(ticker->next != TICKER_NULL)) {
s32_t lazy_current = ticker->lazy_current;

if (ticker->lazy_periodic > lazy_current) {
/* Programmed latency must be respected */
skipped = 1;

} else if ((ticker->priority != TICKER_PRIORITY_CRITICAL) &&
(ticker->next != TICKER_NULL)) {
/* Check if this ticker node will starve next node which has
* latency or higher priority
*/
if (lazy_current >= ticker->lazy_periodic) {
lazy_current -= ticker->lazy_periodic;
}
u8_t id_head = ticker->next;
u32_t acc_ticks_to_expire = 0;
u32_t acc_ticks_to_expire = 0U;

/* Age is time since last expiry */
u32_t current_age = ticker->ticks_periodic +
Expand All @@ -670,7 +665,7 @@ static u8_t ticker_resolve_collision(struct ticker_node *nodes,
}

/* We only care about nodes with slot reservation */
if (ticker_next->ticks_slot == 0) {
if (ticker_next->ticks_slot == 0U) {
id_head = ticker_next->next;
continue;
}
Expand Down Expand Up @@ -725,13 +720,13 @@ static u8_t ticker_resolve_collision(struct ticker_node *nodes,
(next_has_priority && !current_is_older) ||
(equal_priority && next_is_older))) {
/* This node must be skipped - check window */
skipped = 1;
break;
return 1U;
}
id_head = ticker_next->next;
}
}
return skipped;

return 0U;
}
#endif /* !CONFIG_BT_TICKER_COMPATIBILITY_MODE */

Expand Down Expand Up @@ -1007,7 +1002,6 @@ static u8_t ticker_remainder_inc(struct ticker_node *ticker)
#endif
}

#if defined(CONFIG_BT_TICKER_COMPATIBILITY_MODE)
/**
* @brief Decrement remainder
*
Expand Down Expand Up @@ -1037,7 +1031,6 @@ static u8_t ticker_remainder_dec(struct ticker_node *ticker)
return 0;
#endif
}
#endif /* CONFIG_BT_TICKER_COMPATIBILITY_MODE */

/**
* @brief Invoke user operation callback
Expand Down Expand Up @@ -1093,7 +1086,6 @@ static inline void ticker_job_node_update(struct ticker_node *ticker,
if ((ticker->ticks_periodic != 0U) &&
(user_op->params.update.lazy != 0U)) {
user_op->params.update.lazy--;
#if defined(CONFIG_BT_TICKER_COMPATIBILITY_MODE)
while ((ticks_to_expire > ticker->ticks_periodic) &&
(ticker->lazy_current > user_op->params.update.lazy)) {
ticks_to_expire -= ticker->ticks_periodic +
Expand All @@ -1106,7 +1098,6 @@ static inline void ticker_job_node_update(struct ticker_node *ticker,
ticker_remainder_inc(ticker);
ticker->lazy_current++;
}
#endif /* CONFIG_BT_TICKER_COMPATIBILITY_MODE */
ticker->lazy_periodic = user_op->params.update.lazy;
}

Expand Down Expand Up @@ -1365,9 +1356,18 @@ static inline void ticker_job_worker_bh(struct ticker_instance *instance,
struct ticker_node *node;
u32_t ticks_expired;

#if !defined(CONFIG_BT_TICKER_COMPATIBILITY_MODE)
u32_t ticks_latency;
u32_t ticks_now;

ticks_now = cntr_cnt_get();
ticks_latency = ticker_ticks_diff_get(ticks_now, ticks_previous);
#endif /* !CONFIG_BT_TICKER_COMPATIBILITY_MODE */

node = &instance->nodes[0];
ticks_expired = 0U;
while (instance->ticker_id_head != TICKER_NULL) {
u8_t is_must_expire_skip = 0U;
struct ticker_node *ticker;
u32_t ticks_to_expire;
u8_t id_expired;
Expand All @@ -1388,32 +1388,29 @@ static inline void ticker_job_worker_bh(struct ticker_instance *instance,
ticks_expired += ticks_to_expire;

#if !defined(CONFIG_BT_TICKER_COMPATIBILITY_MODE)
if (ticker->lazy_current != 0U &&
!TICKER_RESCHEDULE_PENDING(ticker)) {
ticks_latency -= ticks_to_expire;

is_must_expire_skip = (ticker->must_expire &&
(ticker->lazy_current != 0U));
#endif /* !CONFIG_BT_TICKER_COMPATIBILITY_MODE */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep the logic that if lazy_current != 0 and no pending reschedules, then clear the slot_previous info and nothing else. This is needed because must_expire still needs special handling for must_expired nodes that don't go in the air, and catch a lazy count. By checking for this in addition, I think we get the desired functionality for both "worlds". At least my tests pass with this:

#if !defined(CONFIG_BT_TICKER_COMPATIBILITY_MODE)
		ticks_latency -= ticks_to_expire;

		if (ticker->must_expire && ticker->lazy_current != 0U &&
		    !TICKER_RESCHEDULE_PENDING(ticker)) {
			instance->ticker_id_slot_previous = TICKER_NULL;
			instance->ticks_slot_previous = 0U;
		} else
#endif /* !CONFIG_BT_TICKER_COMPATIBILITY_MODE */
		{

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A skipped ticker instance shall not clear the ticks_slot_previous, wouldn't that also apply to a "must expire" tickers? We should have a audio call at the next opportunity to review this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I had a sneaking suspicion after writing that, and went back to the code this morning. I will take a second look, and let's talk about this Monday morning, if you're available.

Copy link
Collaborator

@mtpr-ot mtpr-ot Mar 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I figured it out. The problem is in the updating of slot_previous to the expired ticker id, regardsless of must_expire:

	/* If a reschedule is set pending, we will need to keep
	 * the slot_previous information
	 */
	if ((ticker->ticks_slot != 0U) &&
	    !(ticker->must_expire && ticker->lazy_current != 0U) && // <-- added
	    (((ticker->req - ticker->ack) & 0xff) == 2U) &&
	    !TICKER_RESCHEDULE_PENDING(ticker)) {
		instance->ticker_id_slot_previous =
			id_expired;
		instance->ticks_slot_previous =
			ticker->ticks_slot;
	}

This will overwrite the info from the blocking node, and use the ticks_slot from the must_expire (shallow) expiry instead, which is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested and added your check, PR updated.

Copy link
Collaborator

@mtpr-ot mtpr-ot Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great. Doing final re-test here. However, unfortunately you need to apply conditional compile with !CONFIG_BT_TICKER_COMPATIBILITY_MODE around that added line. Sorry for not seeing that when suggesting the change :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my fault, I was aware, but forgot to add the IS_ENABLE(...). Will handle and push updates soon.


/* decrement ticks_slot_previous */
if (instance->ticks_slot_previous > ticks_to_expire) {
instance->ticks_slot_previous -= ticks_to_expire;
} else {
instance->ticker_id_slot_previous = TICKER_NULL;
instance->ticks_slot_previous = 0U;
} else
#endif /* !CONFIG_BT_TICKER_COMPATIBILITY_MODE */
{
/* decrement ticks_slot_previous */
if (instance->ticks_slot_previous > ticks_to_expire) {
instance->ticks_slot_previous -=
ticks_to_expire;
} else {
instance->ticker_id_slot_previous = TICKER_NULL;
instance->ticks_slot_previous = 0U;
}
}

/* If a reschedule is set pending, we will need to keep
* the slot_previous information
*/
if (!TICKER_RESCHEDULE_PENDING(ticker) &&
ticker->ticks_slot != 0U) {
instance->ticker_id_slot_previous =
id_expired;
instance->ticks_slot_previous =
ticker->ticks_slot;
}
/* If a reschedule is set pending, we will need to keep
* the slot_previous information
*/
if ((ticker->ticks_slot != 0U) &&
(((ticker->req - ticker->ack) & 0xff) == 2U) &&
!is_must_expire_skip &&
!TICKER_RESCHEDULE_PENDING(ticker)) {
instance->ticker_id_slot_previous = id_expired;
instance->ticks_slot_previous = ticker->ticks_slot;
}

/* ticker expired, set ticks_to_expire zero */
Expand All @@ -1432,12 +1429,62 @@ static inline void ticker_job_worker_bh(struct ticker_instance *instance,
* restarted
*/
ticker->ticks_to_expire = ticks_elapsed;

/* Reset ticker state, so that its put
* back in requested state later down
* in the code.
*/
ticker->req = ticker->ack;
} else {
/* Reload ticks_to_expire with one period */
ticker->ticks_to_expire =
ticker->ticks_periodic;
ticker->ticks_to_expire +=
ticker_remainder_inc(ticker);
u16_t lazy_periodic;
u32_t count;
u16_t lazy;

/* If not skipped, apply lazy_periodic */
if (!ticker->lazy_current) {
lazy_periodic = ticker->lazy_periodic;
} else {
lazy_periodic = 0U;

/* Reset ticker state, so that its put
* back in requested state later down
* in the code.
*/
ticker->req = ticker->ack;
}

/* Reload ticks_to_expire with atleast one
* period.
*/
ticks_to_expire = 0U;
count = 1 + lazy_periodic;
while (count--) {
ticks_to_expire +=
ticker->ticks_periodic;
ticks_to_expire +=
ticker_remainder_inc(ticker);
}

/* Skip intervals that have elapsed w.r.t.
* current ticks.
*/
lazy = 0U;
if (!ticker->must_expire) {
while (ticks_to_expire <
ticks_latency) {
ticks_to_expire +=
ticker->ticks_periodic;
ticks_to_expire +=
ticker_remainder_inc(ticker);
lazy++;
}
}
Copy link
Collaborator

@mtpr-ot mtpr-ot Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is a safeguard for when lagging behind, and catching up? The only thing I worry about is that this will violate the "must_expire" contract, even if it most likely will not occur for platforms using must_expire.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will fix this to consider the "must_expire" ticker instances.


/* Use the calculated ticks to expire and
* laziness.
*/
ticker->ticks_to_expire = ticks_to_expire;
ticker->lazy_current += (lazy_periodic + lazy);
}

ticks_to_expire_prep(ticker, instance->ticks_current,
Expand Down