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: Update Zephyr PM policy function call parameter. #17543

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Bluetooth: Controller: Update Zephyr PM policy function call parameter. #17543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 7 additions & 9 deletions subsys/mpsl/pm/mpsl_pm_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ LOG_MODULE_REGISTER(mpsl_pm_utils, CONFIG_MPSL_LOG_LEVEL);
* absolute time instead of relative time. This would remove the need for safety
* margins and allow optimal power savings.
*/
#define MAX_DELAY_SINCE_READING_PARAMS_US 50
#define TIME_TO_REGISTER_EVENT_IN_ZEPHYR_US 1000
#define PM_MAX_LATENCY_HCI_COMMANDS_US 4999999
#define PM_MAX_LATENCY_HCI_COMMANDS_US 499999

static void m_work_handler(struct k_work *work);
static K_WORK_DELAYABLE_DEFINE(pm_work, m_work_handler);
Expand Down Expand Up @@ -71,22 +70,21 @@ void mpsl_pm_utils_work_handler(void)
}
case MPSL_PM_EVENT_STATE_BEFORE_EVENT:
{
/* Event scheduled */
uint64_t event_time_us = params.event_time_rel_us -
MAX_DELAY_SINCE_READING_PARAMS_US;

/* 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) {
Copy link
Contributor

@knutel-nordic knutel-nordic Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Isnt the parameter to pm_policy_event_update in cycles anyway so does checking if µs overflow make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@KyraLengfeld KyraLengfeld Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor

@knutel-nordic knutel-nordic Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

mpsl_work_schedule(&pm_work, K_USEC(RETRY_TIME_MAX_US));
return;
}

if (m_pm_event_is_registered) {
pm_policy_event_update(&m_evt, event_time_us);
pm_policy_event_update(&m_evt,
k_us_to_cyc_floor32(params.event_time_abs_us));
} else {
pm_policy_event_register(&m_evt, event_time_us);
pm_policy_event_register(&m_evt,
k_us_to_cyc_floor32(params.event_time_abs_us));
m_pm_event_is_registered = true;
}
break;
Expand Down
17 changes: 8 additions & 9 deletions tests/subsys/mpsl/pm/pm_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
#include <zephyr/kernel.h>


#define PM_MAX_LATENCY_HCI_COMMANDS_US 4999999
#define MAX_DELAY_SINCE_READING_PARAMS_US 50
#define PM_MAX_LATENCY_HCI_COMMANDS_US 499999

#define TIME_TO_REGISTER_EVENT_IN_ZEPHYR_US 1000
#define RETRY_TIME_MAX_US (UINT32_MAX - TIME_TO_REGISTER_EVENT_IN_ZEPHYR_US)
Expand Down Expand Up @@ -185,7 +184,7 @@ void test_register_and_derigster_event(void)
LATENCY_FUNC_REGISTER, PM_MAX_LATENCY_HCI_COMMANDS_US},
/* Register event. */
{false, true, {10000, MPSL_PM_EVENT_STATE_BEFORE_EVENT, 1},
EVENT_FUNC_REGISTER, 10000 - MAX_DELAY_SINCE_READING_PARAMS_US,
EVENT_FUNC_REGISTER, 10000,
LATENCY_FUNC_NONE, 0},
/* Deregister event. */
{false, true, {0, MPSL_PM_EVENT_STATE_NO_EVENTS_LEFT, 2},
Expand All @@ -204,7 +203,7 @@ void test_register_enter_and_derigster_event(void)
LATENCY_FUNC_REGISTER, PM_MAX_LATENCY_HCI_COMMANDS_US},
/* Register event. */
{false, true, {10000, MPSL_PM_EVENT_STATE_BEFORE_EVENT, 1},
EVENT_FUNC_REGISTER, 10000 - MAX_DELAY_SINCE_READING_PARAMS_US,
EVENT_FUNC_REGISTER, 10000,
LATENCY_FUNC_NONE, 0},
/* Pretend to be in event */
{false, true, {0, MPSL_PM_EVENT_STATE_IN_EVENT, 2},
Expand All @@ -227,11 +226,11 @@ void test_register_update_enter_and_deregister_event(void)
LATENCY_FUNC_REGISTER, PM_MAX_LATENCY_HCI_COMMANDS_US},
/* Register event. */
{false, true, {10000, MPSL_PM_EVENT_STATE_BEFORE_EVENT, 1},
EVENT_FUNC_REGISTER, 10000 - MAX_DELAY_SINCE_READING_PARAMS_US,
EVENT_FUNC_REGISTER, 10000,
LATENCY_FUNC_NONE, 0},
/* Update event. */
{false, true, {15000, MPSL_PM_EVENT_STATE_BEFORE_EVENT, 2},
EVENT_FUNC_UPDATE, 15000 - MAX_DELAY_SINCE_READING_PARAMS_US,
EVENT_FUNC_UPDATE, 15000,
LATENCY_FUNC_NONE, 0},
/* Pretend to be in event */
{false, true, {0, MPSL_PM_EVENT_STATE_IN_EVENT, 3},
Expand All @@ -254,15 +253,15 @@ void test_register_enter_and_update_event(void)
LATENCY_FUNC_REGISTER, PM_MAX_LATENCY_HCI_COMMANDS_US},
/* Register event. */
{false, true, {10000, MPSL_PM_EVENT_STATE_BEFORE_EVENT, 1},
EVENT_FUNC_REGISTER, 10000 - MAX_DELAY_SINCE_READING_PARAMS_US,
EVENT_FUNC_REGISTER, 10000,
LATENCY_FUNC_NONE, 0},
/* Pretend to be in event */
{false, true, {0, MPSL_PM_EVENT_STATE_IN_EVENT, 2},
EVENT_FUNC_NONE, 0,
LATENCY_FUNC_UPDATE, 0},
/* Update event (before we get the state no events left). */
{false, true, {15000, MPSL_PM_EVENT_STATE_BEFORE_EVENT, 3},
EVENT_FUNC_UPDATE, 15000 - MAX_DELAY_SINCE_READING_PARAMS_US,
EVENT_FUNC_UPDATE, 15000,
LATENCY_FUNC_UPDATE, PM_MAX_LATENCY_HCI_COMMANDS_US},
};
run_test(&test_vectors[0], ARRAY_SIZE(test_vectors));
Expand All @@ -289,7 +288,7 @@ void test_event_delayed_work(void)
{false, true, {retry_evt_time - RETRY_TIME_MAX_US - RETRY_TIME_MAX_US,
MPSL_PM_EVENT_STATE_BEFORE_EVENT, 3},
EVENT_FUNC_REGISTER, retry_evt_time - RETRY_TIME_MAX_US -
RETRY_TIME_MAX_US - MAX_DELAY_SINCE_READING_PARAMS_US,
RETRY_TIME_MAX_US,
LATENCY_FUNC_NONE, 0},
/* Pretend to be in event */
{false, true, {0, MPSL_PM_EVENT_STATE_IN_EVENT, 4},
Expand Down
Loading