Bluetooth: controller: Fix ticker catch up on ISR latency

[cvinayak]

Fix ticker to avoid catch up of periodic timeouts in case of
large ISR latencies like in case of flash erase scenarios.

Signed-off-by: Vinayak Kariappa Chettimada vich@nordicsemi.no

[mtpr-ot]

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.

[cvinayak]

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

[mtpr-ot]

As I see it, this change will make ticker_job take over the lazy/latency handling, which is fine and more efficient, but then the lazy handling in ticker_worker should be removed (a few lines).

[cvinayak]

As I see it, this change will make ticker_job take over the lazy/latency handling, which is fine and more efficient, but then the lazy handling in ticker_worker should be removed (a few lines).

Thanks for pointing out, will address this.

[cvinayak]

@mtpr-ot I have reworked, some more fixes, and they are in additional separate commits in this PR. Please review.

[mtpr-ot]

@mtpr-ot I have reworked, some more fixes, and they are in additional separate commits in this PR. Please review.

Working on it - I think I need to test it, I'm a bit unsure about impact of the latest change. Feedback later today
My ticker unittests fails with the change, so I need to analyze that.

[mtpr-ot]

Testing with this PR shows problems honoring critical priority events (priority = -128), getting shallow "must_expire" timeouts where they should go in the air. Will continue to investigate.

[mtpr-ot]

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 */
		{

[cvinayak]

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.

[mtpr-ot]

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.

[mtpr-ot]

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.

[cvinayak]

I have tested and added your check, PR updated.

[mtpr-ot]

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 :-/

[cvinayak]

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

[mtpr-ot]

LGTM

[thoh-ot]

Please create a backport pr for this.