Skip to content

applications: nrf_desktop: Disable advertising when connected via USB #23006

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.

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ CONFIG_DESKTOP_USB_ENABLE=y
# Enable the only USB stack that supports USBHS
CONFIG_DESKTOP_USB_STACK_NEXT=y

# Suspend Bluetooth advertising when USB is connected to improve
Copy link
Contributor

Choose a reason for hiding this comment

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

Suspend Bluetooth while USB is connected to improve ... (we not only disable BLE advertising, we also terminate existing Bluetooth connections)

# USBHS report rate.
CONFIG_DESKTOP_BLE_ADV_CTRL_ENABLE=y
CONFIG_DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB=y

CONFIG_DESKTOP_DFU_MCUMGR_ENABLE=y
CONFIG_CAF_INIT_LOG_BLE_SMP_TRANSFER_EVENTS=n

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ CONFIG_DESKTOP_USB_ENABLE=y
# Enable the only USB stack that supports USBHS
CONFIG_DESKTOP_USB_STACK_NEXT=y

# Suspend Bluetooth advertising when USB is connected to improve
# USBHS report rate.
CONFIG_DESKTOP_BLE_ADV_CTRL_ENABLE=y
CONFIG_DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB=y

CONFIG_DESKTOP_DFU_MCUMGR_ENABLE=y

# The LLPM must be explicitly enabled, as the Bluetooth Controller
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ CONFIG_DESKTOP_USB_ENABLE=y
# Enable the only USB stack that supports USBHS
CONFIG_DESKTOP_USB_STACK_NEXT=y

# Suspend Bluetooth advertising when USB is connected to improve
# USBHS report rate.
CONFIG_DESKTOP_BLE_ADV_CTRL_ENABLE=y
CONFIG_DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB=y

CONFIG_DESKTOP_BLE_USE_DEFAULT_ID=y

CONFIG_DESKTOP_BLE_PEER_CONTROL=y
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ CONFIG_DESKTOP_USB_ENABLE=y
# Enable the only USB stack that supports USBHS
CONFIG_DESKTOP_USB_STACK_NEXT=y

# Suspend Bluetooth advertising when USB is connected to improve
# USBHS report rate.
CONFIG_DESKTOP_BLE_ADV_CTRL_ENABLE=y
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to also align llvm configuration after #23000 goes in

CONFIG_DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB=y

CONFIG_DESKTOP_BLE_PEER_CONTROL=y
CONFIG_DESKTOP_BLE_PEER_CONTROL_BUTTON=0x0000
CONFIG_DESKTOP_BLE_PEER_ERASE_ON_START=y
Expand Down
3 changes: 3 additions & 0 deletions applications/nrf_desktop/src/modules/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

target_sources_ifdef(CONFIG_DESKTOP_BLE_ADV_CTRL_ENABLE
app PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/ble_adv_ctrl.c)

target_sources_ifdef(CONFIG_DESKTOP_BLE_BOND_ENABLE
app PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/ble_bond.c)

Expand Down
1 change: 1 addition & 0 deletions applications/nrf_desktop/src/modules/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ rsource "Kconfig.hid_state"
rsource "Kconfig.led_state"
rsource "Kconfig.led_stream"
rsource "Kconfig.usb_state"
rsource "Kconfig.ble_adv_ctrl"
rsource "Kconfig.ble_bond"
rsource "Kconfig.ble_conn_params"
rsource "Kconfig.ble_latency"
Expand Down
27 changes: 27 additions & 0 deletions applications/nrf_desktop/src/modules/Kconfig.ble_adv_ctrl
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#
# Copyright (c) 2024 Nordic Semiconductor ASA
#
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

menuconfig DESKTOP_BLE_ADV_CTRL_ENABLE
bool "Bluetooth LE advertising control module"
depends on CAF_BLE_ADV
select CAF_MODULE_SUSPEND_EVENTS
help
The module controls suspend/resume of CAF Bluetooth LE advertising
module.

if DESKTOP_BLE_ADV_CTRL_ENABLE

config DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB
bool "Suspend Bluetooth LE adverising when USB is connected"
depends on DESKTOP_USB_ENABLE
help
Suspend CAF Bluetooth LE advertising module while HID USB is active.

module = DESKTOP_BLE_ADV_CTRL
module-str = BLE advertising control
source "subsys/logging/Kconfig.template.log_config"

endif # DESKTOP_BLE_ADV_CTRL_ENABLE
12 changes: 12 additions & 0 deletions applications/nrf_desktop/src/modules/Kconfig.usb_state
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ config DESKTOP_USB_PM_REQ_NO_PM_LATENCY
latency and ensures high performance. While USB is active, application
power down is blocked anyway.

config DESKTOP_USB_PM_RESTRICT_REMOVE_DELAY_MS
int "Delay in milliseconds before removing power manager level restriction"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int "Delay in milliseconds before removing power manager level restriction"
int "Delay removing power manager restrictions after USB disconnection [ms]"

range 0 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
range 0 10000
range 0 60000

Consider allowing bigger values here to avoid artificial limitations

default 3000 if DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could always use 3000 (or even 4000) as the default here?

default 0
depends on DESKTOP_USB_PM_ENABLE
help
Delay in milliseconds before removing power manager level restriction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Delay in milliseconds before removing power manager level restriction.
Delay in milliseconds before removing power manager level restriction after USB is disconnected.

This is used to allow the USB to be disconnected without immediate system off.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is used to allow the USB to be disconnected without immediate system off.
This is used to delay the system off after USB disconnection.

It allows user to take actions, like e.g. restart advertising, after disconnecting
Copy link
Contributor

Choose a reason for hiding this comment

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

restart BLE advertising?

the USB cable without going through reboot.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider also mentioning that:

While disconnecting an USB cable, USB suspend might be triggered.
The USB suspend triggers force power down sequence which makes CAF power manager instantly suspend the application.

This fact might not be obvious and is quite relevant here


config DESKTOP_USB_HID_REPORT_SENT_ON_SOF
bool "Submit HID report sent event on USB Start of Frame (SOF) [EXPERIMENTAL]"
default y if UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT
Expand Down
71 changes: 71 additions & 0 deletions applications/nrf_desktop/src/modules/ble_adv_ctrl.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
Copy link
Contributor

Choose a reason for hiding this comment

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

date

*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

#define MODULE ble_adv_ctrl
#include <caf/events/module_state_event.h>
#include <caf/events/module_suspend_event.h>
#include "usb_event.h"

#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(MODULE, CONFIG_DESKTOP_BLE_ADV_CTRL_LOG_LEVEL);

static bool ble_adv_suspended;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider limiting scope to handle_usb_state_event function. Alternatively we could keep here as bitmask (that might be easily expanded in the future)



static void broadcast_ble_adv_req(bool suspend)
{
if (suspend) {
struct module_suspend_req_event *event = new_module_suspend_req_event();

event->module_id = MODULE_ID(ble_adv);
APP_EVENT_SUBMIT(event);
} else {
struct module_resume_req_event *event = new_module_resume_req_event();

event->module_id = MODULE_ID(ble_adv);
APP_EVENT_SUBMIT(event);
}
}

static bool handle_module_state_event(const struct module_state_event *event)
{
return false;
}

static bool handle_usb_state_event(const struct usb_state_event *event)
{
bool new_ble_adv_suspended = (event->state == USB_STATE_ACTIVE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder a bit if we shouldn't keep BLE advertising suspended until USB disconnection after USB was active (to prevent resuming BLE advertising module if USB is temporarily suspended). IIRC USB suspend might also happen if we connect to an USB charger, but then there should be no USB_STATE_ACTIVE detected.


if (new_ble_adv_suspended != ble_adv_suspended) {
ble_adv_suspended = new_ble_adv_suspended;
broadcast_ble_adv_req(ble_adv_suspended);
}

return false;
}

static bool app_event_handler(const struct app_event_header *aeh)
{
if (is_module_state_event(aeh)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could completely skip handling module_state_event here (as we do nothing anyway)

return handle_module_state_event(cast_module_state_event(aeh));
}

if (IS_ENABLED(CONFIG_DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB) &&
is_usb_state_event(aeh)) {
return handle_usb_state_event(cast_usb_state_event(aeh));
}

/* If event is unhandled, unsubscribe. */
__ASSERT_NO_MSG(false);

return false;
}

APP_EVENT_LISTENER(MODULE, app_event_handler);
APP_EVENT_SUBSCRIBE(MODULE, module_state_event);
#ifdef CONFIG_DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB
APP_EVENT_SUBSCRIBE(MODULE, usb_state_event);
#endif /* CONFIG_DESKTOP_BLE_ADV_CTRL_SUSPEND_ON_USB */
38 changes: 35 additions & 3 deletions applications/nrf_desktop/src/modules/usb_state_pm.c
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ LOG_MODULE_REGISTER(MODULE, CONFIG_DESKTOP_USB_STATE_LOG_LEVEL);
#include <caf/events/force_power_down_event.h>


static bool initialized;
static struct k_work_delayable pm_restrict_remove_work;

static void pm_restrict_remove_work_fn(struct k_work *work)
Copy link
Contributor

Choose a reason for hiding this comment

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

ARG_UNUSED(work)?

{
power_manager_restrict(MODULE_IDX(MODULE), POWER_MANAGER_LEVEL_MAX);
}

static void update_pm_policy_latency_req(bool enable)
{
static bool enabled;
Expand All @@ -45,11 +53,17 @@ static bool app_event_handler(const struct app_event_header *aeh)
const struct usb_state_event *event = cast_usb_state_event(aeh);

LOG_DBG("USB state change detected");
__ASSERT_NO_MSG(initialized);

if (IS_ENABLED(CONFIG_DESKTOP_USB_PM_REQ_NO_PM_LATENCY)) {
update_pm_policy_latency_req(event->state == USB_STATE_ACTIVE);
}

if (CONFIG_DESKTOP_USB_PM_RESTRICT_REMOVE_DELAY_MS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider:

Suggested change
if (CONFIG_DESKTOP_USB_PM_RESTRICT_REMOVE_DELAY_MS) {
if (CONFIG_DESKTOP_USB_PM_RESTRICT_REMOVE_DELAY_MS > 0) {

/* Cancel the work if it is scheduled. */
k_work_cancel_delayable(&pm_restrict_remove_work);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add (void) cast to indicate that we ignore the returned value

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we cancel the work only if state != USB_STATE_DISCONNECTED?

}

switch (event->state) {
case USB_STATE_POWERED:
power_manager_restrict(MODULE_IDX(MODULE), POWER_MANAGER_LEVEL_SUSPENDED);
Expand All @@ -58,7 +72,23 @@ static bool app_event_handler(const struct app_event_header *aeh)
power_manager_restrict(MODULE_IDX(MODULE), POWER_MANAGER_LEVEL_ALIVE);
break;
case USB_STATE_DISCONNECTED:
power_manager_restrict(MODULE_IDX(MODULE), POWER_MANAGER_LEVEL_MAX);
if (CONFIG_DESKTOP_USB_PM_RESTRICT_REMOVE_DELAY_MS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the code LGTM but it's overall a bit complex.
I suggest to break it into subfunctions.
Maybe in a separate PR to simplify this review (i.e., avoid adding new functionality and moving code at the same time).

/* Remove the restriction after the delay to allow the user to
* take actions like e.g. restart advertising without going
* through system off.
*/
k_work_schedule(
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing (void) cast

&pm_restrict_remove_work,
K_MSEC(CONFIG_DESKTOP_USB_PM_RESTRICT_REMOVE_DELAY_MS));
} else {
power_manager_restrict(MODULE_IDX(MODULE), POWER_MANAGER_LEVEL_MAX);
}
/* When disconnecting USB cable, the USB_STATE_SUSPENDED event may
* or may not appear before the USB_STATE_DISCONNECTED event. Force power
* down for application to behave consistently even if the
* USB_STATE_SUSPENDED event doesn't appear.
*/
force_power_down();
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about separating 2 small functions (for force power down handling and applying power manager restrictions - both based on USB state)? Similarly to update_pm_policy_latency_req.

That might improve readability here I think (as @pdunaj mentioned, the function is already quite complex)

break;
case USB_STATE_SUSPENDED:
LOG_DBG("USB suspended");
Expand All @@ -76,9 +106,11 @@ static bool app_event_handler(const struct app_event_header *aeh)
const struct module_state_event *event = cast_module_state_event(aeh);

if (check_state(event, MODULE_ID(main), MODULE_STATE_READY)) {
static bool initialized;

__ASSERT_NO_MSG(!initialized);
if (CONFIG_DESKTOP_USB_PM_RESTRICT_REMOVE_DELAY_MS) {
k_work_init_delayable(&pm_restrict_remove_work,
pm_restrict_remove_work_fn);
}
initialized = true;
}
return false;
Expand Down
5 changes: 5 additions & 0 deletions include/caf/events/module_state_event.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ enum module_state {
*/
MODULE_STATE_STANDBY,

/** Module is suspended in reaction to @ref module_suspend_req_event. The module is resumed
* by @ref module_resume_req_event.
*/
MODULE_STATE_SUSPENDED,

/** Module reported fatal error. */
MODULE_STATE_ERROR,

Expand Down
46 changes: 46 additions & 0 deletions include/caf/events/module_suspend_event.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding missing Jira references in commit messages

* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

#ifndef _MODULE_SUSPEND_EVENT_H_
#define _MODULE_SUSPEND_EVENT_H_

/**
* @file
* @defgroup caf_module_suspend_event CAF Module Suspend Event
* @{
* @brief CAF Module Suspend Event.
*/


#include <zephyr/sys/atomic.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is #include <zephyr/sys/atomic.h> needed here?

#include <app_event_manager.h>
#include <app_event_manager_profiler_tracer.h>

#ifdef __cplusplus
extern "C" {
#endif

struct module_suspend_req_event {
/** Event header. */
struct app_event_header header;

/** ID of the module. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider also adding source_id (ID of module that is source of the request). Also for module_resume_req_event. That would allow to support more than one client in the future. It might also simplify debugging now.

const void *module_id;
};

APP_EVENT_TYPE_DECLARE(module_suspend_req_event);

struct module_resume_req_event {
/** Event header. */
struct app_event_header header;

/** ID of the module. */
const void *module_id;
};

APP_EVENT_TYPE_DECLARE(module_resume_req_event);

#endif /* _MODULE_SUSPEND_EVENT_H_ */
4 changes: 4 additions & 0 deletions subsys/caf/events/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ zephyr_sources_ifdef(CONFIG_CAF_MODULE_STATE_EVENTS
module_state_event.c
)

zephyr_sources_ifdef(CONFIG_CAF_MODULE_SUSPEND_EVENTS
module_suspend_event.c
)

zephyr_sources_ifdef(CONFIG_CAF_PM_EVENTS
power_event.c
)
Expand Down
1 change: 1 addition & 0 deletions subsys/caf/events/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ rsource "Kconfig.factory_reset_event"
rsource "Kconfig.force_power_down_event"
rsource "Kconfig.keep_alive_event"
rsource "Kconfig.module_state_event"
rsource "Kconfig.module_suspend_event"
rsource "Kconfig.power_manager_event"
rsource "Kconfig.sensor_event"

Expand Down
17 changes: 17 additions & 0 deletions subsys/caf/events/Kconfig.module_suspend_event
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#
# Copyright (c) 2024 Nordic Semiconductor
#
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

config CAF_MODULE_SUSPEND_EVENTS
bool "Enable module suspend/resume request events"
depends on CAF_MODULE_STATE_EVENTS
help
Enable support for module suspend/resume request events.

config CAF_INIT_LOG_MODULE_SUSPEND_EVENTS
bool "Log module suspend/resume request events"
depends on CAF_MODULE_SUSPEND_EVENTS
depends on LOG
default y
1 change: 1 addition & 0 deletions subsys/caf/events/module_state_event.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ static const char * const state_name[] = {
[MODULE_STATE_READY] = "READY",
[MODULE_STATE_OFF] = "OFF",
[MODULE_STATE_STANDBY] = "STANDBY",
[MODULE_STATE_SUSPENDED] = "SUSPENDED",
[MODULE_STATE_ERROR] = "ERROR",
};

Expand Down
Loading
Loading