Suspending UART Async using PM_DEVICE ACTION_SUSPEND causes crash.

[Crisvin_K]

Hello, 

I am currently using the BLE NUS example from the Nordic Dev Academy course for BLE (https://github.com/NordicDeveloperAcademy/bt-fund/blob/main/lesson4/blefund_less4_exer3_solution/src/main.c).

However, the current consumption was around 3.5 mA, which is too high for my application.

Thus, I used the commands "pm_device_action_run(uart, PM_DEVICE_ACTION_SUSPEND);" on the ble connection callback and "pm_action_run(uart, PM_DEVICE_ACTION_RESUME);" to suspend the UART driver and hopefully reduce current consumption. After the device is suspended, the current consumption does go down to around 300 uA. The idea is that when the device is not connected on bluetooth, UART would be suspended to save power, and UART could resume as soon as the device is reconnected on bluetooth. I implemented the code as shown below:

static void connected(struct bt_conn *conn, uint8_t err)
{
	char addr[BT_ADDR_LE_STR_LEN];

	if (err) {
		LOG_ERR("Connection failed (err %u)", err);
		return;
	}

	bt_addr_le_to_str(bt_conn_get_dst(conn), addr, sizeof(addr));
	LOG_INF("Connected %s", addr);

	current_conn = bt_conn_ref(conn);

	dk_set_led_on(CON_STATUS_LED);
	
	pm_device_action_run(uart, PM_DEVICE_ACTION_RESUME);
	struct uart_data_t *buf;
	uart_rx_enable(uart, buf->data, sizeof(buf->data), UART_WAIT_FOR_RX);
}

static void disconnected(struct bt_conn *conn, uint8_t reason)
{
	char addr[BT_ADDR_LE_STR_LEN];

	bt_addr_le_to_str(bt_conn_get_dst(conn), addr, sizeof(addr));

	LOG_INF("Disconnected: %s (reason %u)", addr, reason);

	if (auth_conn) {
		bt_conn_unref(auth_conn);
		auth_conn = NULL;
	}

	if (current_conn) {
		bt_conn_unref(current_conn);
		current_conn = NULL;
		dk_set_led_off(CON_STATUS_LED);
	}
	
	uart_rx_disable(uart);
	pm_device_action_run(uart, PM_DEVICE_ACTION_SUSPEND);
}

However, whenever PM_DEVICE_ACTION_SUSPEND is executed, the device crashes and the RGB LED starts to flicker. The same occurs if I put the "pm_device_action_run(uart, PM_DEVICE_ACTION_SUSPEND);" statement in the main function after all initializations.

For context, I am using the EVK-ANNA-B112 development board that uses nrf52832. When I flash the device, the UART TX and RX pins are connected to a separate device (EmStat Pico Development Board) for communication. 

Is there anything I am getting wrong about suspending UART Async? 

[tesc]

Hi,

struct uart_data_t *buf;
uart_rx_enable(uart, buf->data, sizeof(buf->data), UART_WAIT_FOR_RX);

Here, you create an uninitialized pointer to uart_data_t, which you send to uart_rx_enable(). There is no guarantee what buf will point to.

In the SDK there are a couple of samples using uart_rx_enable(), and the API call is also used in several drivers. Common for all of them is the struct uart_data_t buf exist in a global (or static) context, typically throuigh use of k_malloc to allocate space for the buffer. Then when uart_rx_enable is called, a pointer is given to the buffer within that structure.

Regards,
Terje

[Crisvin_K]

Hello,

I tried removing the uart_rx_enable() and the program doesn't crash anymore. However, I still have issues as follows.

1. When the program first starts up there has no call to suspend PM_DEVICE_ACTION_SUSPEND yet, so when i connect to bluetooth, the EVK-ANNA-B112 can communicate through UART to the with no problem.

2. After I disconnect from bluetooth, UART gets suspended in the disconnect callback with PM_DEVICE_ACTION_SUSPEND. This causes the current consumption to reduce from around 3.3 mA to 300 uA.

3. When I reconnect to bluetooth, UART should resume in the connect callback with PM_DEVICE_ACTION_RESUME, but UART doesn't seem to start working again. Reconnecting to bluetooth also increases current consumption back up to 3.3 mA.

I guess my problem is that I don't know if there is something more I need to do after calling PM_DEVICE_ACTION_RESUME to actually get UART to start working again. 

There is another thing I did which was to remove uart_rx_disable() in the disconnect callback. However, if I remove uart_rx_disable() I don't see the current draw going down after disconnects. 

What I really want to achieve is lower current consumption with the NUS example (https://github.com/NordicDeveloperAcademy/bt-fund/blob/main/lesson4/blefund_less4_exer3_solution/src/main.c), so if you have anymore recommendations about that it would also be helpful

For context, this is what I have tried so far for reducing power consumption. These have not seemed to reduce current consumption much down from around 3.5-3.3 mA which I have been seeing. I was expecting current consumption much lower in the 100s of uA range.

- disabling devices not in use, like spi, in the overlay file.

- enabling CONFIG_PM, CONFIG_PM_DEVICE, and CONFIG_PM_DEVICE_RUNTIME.

[Vidar Berg]

Hello,

Sorry for the delayed response. I have now had a chance to test this with the peripheral_uart sample from nRF Connect version 2.6.1.

The changes I made to suspend UART while not connected

diff --git a/prj.conf b/prj.conf
index 25deae6..c267e7e 100644
--- a/prj.conf
+++ b/prj.conf
@@ -7,6 +7,8 @@
 # Enable the UART driver
 CONFIG_UART_ASYNC_API=y
 CONFIG_NRFX_UARTE0=y
+CONFIG_UART_0_NRF_HW_ASYNC=y
+CONFIG_UART_0_NRF_HW_ASYNC_TIMER=1
 CONFIG_SERIAL=y
 
 CONFIG_GPIO=y
@@ -49,3 +51,7 @@ CONFIG_LOG_BACKEND_UART=n
 CONFIG_LOG_PRINTK=n
 
 CONFIG_ASSERT=y
+
+CONFIG_PM_DEVICE=y
+
+
diff --git a/src/main.c b/src/main.c
index 7526fd6..fe5bc0b 100644
--- a/src/main.c
+++ b/src/main.c
@@ -13,6 +13,10 @@
 #include <zephyr/kernel.h>
 #include <zephyr/drivers/uart.h>
 #include <zephyr/usb/usb_device.h>
+#include <zephyr/pm/device.h>
+
+#include <zephyr/drivers/clock_control.h>
+#include <zephyr/drivers/clock_control/nrf_clock_control.h>
 
 #include <zephyr/device.h>
 #include <zephyr/devicetree.h>
@@ -54,6 +58,10 @@ LOG_MODULE_REGISTER(LOG_MODULE_NAME);
 #define UART_WAIT_FOR_BUF_DELAY K_MSEC(50)
 #define UART_WAIT_FOR_RX CONFIG_BT_NUS_UART_RX_WAIT_TIME
 
+#define UART_SUSPENDED	1
+
+static ATOMIC_DEFINE(uart_suspended, 1);
+
 static K_SEM_DEFINE(ble_init_ok, 0, 1);
 
 static struct bt_conn *current_conn;
@@ -61,6 +69,8 @@ static struct bt_conn *auth_conn;
 
 static const struct device *uart = DEVICE_DT_GET(DT_CHOSEN(nordic_nus_uart));
 static struct k_work_delayable uart_work;
+struct onoff_client uart_clk_request;
+
 
 struct uart_data_t {
 	void *fifo_reserved;
@@ -86,6 +96,98 @@ UART_ASYNC_ADAPTER_INST_DEFINE(async_adapter);
 static const struct device *const async_adapter;
 #endif
 
+static void rx_hfclk_callback(struct onoff_manager *mgr,
+			      struct onoff_client *cli,
+			      uint32_t state, int res)
+{
+	int err;
+	struct uart_data_t *rx;
+
+	if (res) {
+		LOG_ERR("HFCLK request error %d", err);
+	} else {
+		LOG_INF("HFCLK started");
+	}
+
+	rx = k_malloc(sizeof(*rx));
+	if (rx) {
+		rx->len = 0;
+	} else {
+		LOG_ERR("Failed to allocate UART RX buffer");
+		return;
+	}
+
+	err = uart_rx_enable(uart, rx->data, sizeof(rx->data), UART_WAIT_FOR_RX);
+	if (err) {
+		LOG_ERR("Cannot enable uart reception (err: %d)", err);
+		/* Free the rx buffer only because the tx buffer will be handled in the callback */
+		k_free(rx);
+	}
+
+	atomic_clear_bit(uart_suspended, UART_SUSPENDED);
+
+}
+
+static void rx_hfclk_request(const struct device *dev)
+{
+	struct onoff_manager *mgr =
+		z_nrf_clock_control_get_onoff(CLOCK_CONTROL_NRF_SUBSYS_HF);
+	int err;
+
+	sys_notify_init_callback(&uart_clk_request.notify, rx_hfclk_callback);
+	err = onoff_request(mgr, &uart_clk_request);
+	__ASSERT_NO_MSG(err >= 0);
+}
+
+static void rx_hfclk_release(void)
+{
+	int err;
+	struct onoff_manager *mgr =
+	     z_nrf_clock_control_get_onoff(CLOCK_CONTROL_NRF_SUBSYS_HF);
+	
+	(void)onoff_cancel_or_release(mgr, &uart_clk_request);
+		
+}
+
+static void uart_suspend(void)
+{
+	int err;
+
+	atomic_set_bit(uart_suspended, UART_SUSPENDED);
+
+	(void)uart_rx_disable(uart);
+	(void)uart_tx_abort(uart);
+
+	/* Delay to allow pending UART events to be processed */
+	k_msleep(100);
+
+	err = pm_device_action_run(uart, PM_DEVICE_ACTION_SUSPEND);
+	if (err) {
+		LOG_ERR("Failed to suspend uart (err: %d)", err);
+		return;
+	}
+	/* Release request for HF crystal oscillator to reduce idle current*/
+	rx_hfclk_release();
+
+	LOG_INF("UART suspended..");
+}
+
+static void uart_resume(void)
+{
+	int err;
+
+	err = pm_device_action_run(uart, PM_DEVICE_ACTION_RESUME);
+	if (err) {
+		LOG_ERR("Failed to re-enable uart (err: %d)", err);
+	}
+
+	/*	
+	 * 	Request HF crystal oscillator before enabling UART to ensure
+	 *	sufficient clock accuracy for stable communication.
+	 */
+	rx_hfclk_request(uart);
+}
+
 static void uart_cb(const struct device *dev, struct uart_event *evt, void *user_data)
 {
 	ARG_UNUSED(dev);
@@ -147,6 +249,10 @@ static void uart_cb(const struct device *dev, struct uart_event *evt, void *user
 		LOG_DBG("UART_RX_DISABLED");
 		disable_req = false;
 
+		if (atomic_test_bit(uart_suspended, UART_SUSPENDED)) {
+			return;
+		}
+
 		buf = k_malloc(sizeof(*buf));
 		if (buf) {
 			buf->len = 0;
@@ -233,9 +339,6 @@ static bool uart_test_async_api(const struct device *dev)
 static int uart_init(void)
 {
 	int err;
-	int pos;
-	struct uart_data_t *rx;
-	struct uart_data_t *tx;
 
 	if (!device_is_ready(uart)) {
 		return -ENODEV;
@@ -249,16 +352,8 @@ static int uart_init(void)
 		}
 	}
 
-	rx = k_malloc(sizeof(*rx));
-	if (rx) {
-		rx->len = 0;
-	} else {
-		return -ENOMEM;
-	}
-
 	k_work_init_delayable(&uart_work, uart_work_handler);
 
-
 	if (IS_ENABLED(CONFIG_BT_NUS_UART_ASYNC_ADAPTER) && !uart_test_async_api(uart)) {
 		/* Implement API adapter */
 		uart_async_adapter_init(async_adapter, uart);
@@ -267,7 +362,6 @@ static int uart_init(void)
 
 	err = uart_callback_set(uart, uart_cb, NULL);
 	if (err) {
-		k_free(rx);
 		LOG_ERR("Cannot initialize UART callback");
 		return err;
 	}
@@ -295,40 +389,6 @@ static int uart_init(void)
 		}
 	}
 
-	tx = k_malloc(sizeof(*tx));
-
-	if (tx) {
-		pos = snprintf(tx->data, sizeof(tx->data),
-			       "Starting Nordic UART service example\r\n");
-
-		if ((pos < 0) || (pos >= sizeof(tx->data))) {
-			k_free(rx);
-			k_free(tx);
-			LOG_ERR("snprintf returned %d", pos);
-			return -ENOMEM;
-		}
-
-		tx->len = pos;
-	} else {
-		k_free(rx);
-		return -ENOMEM;
-	}
-
-	err = uart_tx(uart, tx->data, tx->len, SYS_FOREVER_MS);
-	if (err) {
-		k_free(rx);
-		k_free(tx);
-		LOG_ERR("Cannot display welcome message (err: %d)", err);
-		return err;
-	}
-
-	err = uart_rx_enable(uart, rx->data, sizeof(rx->data), UART_WAIT_FOR_RX);
-	if (err) {
-		LOG_ERR("Cannot enable uart reception (err: %d)", err);
-		/* Free the rx buffer only because the tx buffer will be handled in the callback */
-		k_free(rx);
-	}
-
 	return err;
 }
 
@@ -347,6 +407,7 @@ static void connected(struct bt_conn *conn, uint8_t err)
 	current_conn = bt_conn_ref(conn);
 
 	dk_set_led_on(CON_STATUS_LED);
+	uart_resume();
 }
 
 static void disconnected(struct bt_conn *conn, uint8_t reason)
@@ -367,6 +428,8 @@ static void disconnected(struct bt_conn *conn, uint8_t reason)
 		current_conn = NULL;
 		dk_set_led_off(CON_STATUS_LED);
 	}
+
+	uart_suspend();
 }
 
 #ifdef CONFIG_BT_NUS_SECURITY_ENABLED
@@ -581,6 +644,8 @@ int main(void)
 	if (err) {
 		error();
 	}
+	
+	uart_suspend();
 
 	if (IS_ENABLED(CONFIG_BT_NUS_SECURITY_ENABLED)) {
 		err = bt_conn_auth_cb_register(&conn_auth_callbacks);

Measurement results 

Sample project

 3513.peripheral_uart_lp.zip

To further reduce the current consumption, you may consider extending the connection and advertising interval. We have an online power profiler at  https://devzone.nordicsemi.com/power/w/opp/2/online-power-profiler-for-bluetooth-le, which can be used to estimate the expected current consumption based on paramaters such as the advertising interval.

Please note that I have not done any extensive testing with this modification apart from verifying the basic functionality.

Best regards,

Vidar 

[Crisvin_K]

Hello Vidar, 

I really appreciate this example code you provided for how to suspend the UART device. Before I was trying to do all these steps within other functions, but it seems to make more sense to have separate uart_suspend() and uart_resume() functions. I was able to adapt this code for our application and replicate the reduced current draw!

Thank you so much!

Crisvin K.