Bluetooth: Controller: Update Zephyr PM policy function call parameter. #17543
Conversation
github-actions
bot
added
the
changelog-entry-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: d4214b56d427daabc733c399763d6e28f26dea60 more detailssdk-nrf:
Github labels
List of changed files detected by CI (2)Outputs:ToolchainVersion: 9583beca34 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
KyraLengfeld
force-pushed
the
fix_pm_nrf_implementation
branch
2 times, most recently
from
7f694ad to
de0b665
Compare
This commit changes the input parameter of event time from relative to absolute time according to recent Zephyr RTOS changes. As an upmerge is imminent and we don't have any system tests in place, this can be merged as is. Signed-off-by: Kyra Lengfeld <[email protected]>
KyraLengfeld
force-pushed
the
fix_pm_nrf_implementation
branch
from
de0b665 to
d4214b5
Compare
| /* In case we missed a state and are in zero-latency, set low-latency.*/ | ||
| m_update_latency_request(PM_MAX_LATENCY_HCI_COMMANDS_US); | ||
|
|
||
| if (event_time_us > UINT32_MAX) { | ||
| /* Event scheduled */ | ||
| if (params.event_time_abs_us > UINT32_MAX) { |
There was a problem hiding this comment.
This will always be true after 2**32 microseconds. The check was meant to verify that the relative time did not overflow, but does not make sense now.
There was a problem hiding this comment.
Isnt the parameter to pm_policy_event_update in cycles anyway so does checking if µs overflow make sense?
There was a problem hiding this comment.
Yes, this check does not make sense now. The old API used relative time in microseconds, hence the check to see if we needed to reschedule registering the event closer to the event time.
There was a problem hiding this comment.
@knutel-nordic is it ok to shift this to a follow up PR? missed that with the MPSL changes now in nrfxlib those UT will fail on nrf main, and are blocking the upmerge. ( merging this PR would fix it)
There was a problem hiding this comment.
@KyraLengfeld For the followup PR:
Note that CONFIG_NRF_GRTC_TIMER also selects TIMER_HAS_64BIT_CYCLE_COUNTER, SYS_CLOCK_HW_CYCLES_PER_SEC will be 1000000 and SYS_CLOCK_TICKS_PER_SEC will be 10000. CONFIG_TIMEOUT_64BIT is probably also "y".
This means that in this particular case, 1 cycle is 1 us. The time parameter to pm_policy_event_register etc is 32 bits and will therefore overflow if the event is too far into the future ( close to overflowing, which happens every ~4294 seconds), so we still need to reschedule ourselves closer to the event time and try again.
CONFIG_TIMEOUT_64BIT=y will affect the bit width of the timeout given to mpsl_work_schedule, which will in this case be 64 bits, too. I think we can schedule the retry only once, since (event_time_abs_us - now_us) converted to ticks will not overflow.
Also note that we need to handle running on bsim, and possibly other socs, which has other values for these constants.
|
Discussed offline to remove the overflow check after upmerge. |
This commit changes the input parameter of event time from relative to absolute time according to recent Zephyr RTOS changes. As an upmerge is imminent and we don't have any system tests in place, this can be merged as is.