lib: sm_at_client: Multiple fixes

- Fix MON_ANY usage.
- Fix monitor matching multiple URCs if they come in the same
  UART RX buffer.
- Fix uninit.
- Fix operations before init is done.

Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>

Author: MarkusLassila

lib/sm_at_client/sm_at_client.c

@@ -31,7 +31,14 @@ LOG_MODULE_REGISTER(sm_at_client, CONFIG_SM_AT_CLIENT_LOG_LEVEL);
 
 static const struct device *uart_dev = DEVICE_DT_GET(DT_CHOSEN(ncs_sm_uart));
 
-static struct k_work_delayable rx_process_work;
+static void rx_process(struct k_work *work);
+static void dtr_uart_enable_work_fn(struct k_work *work);
+static void dtr_uart_disable_work_fn(struct k_work *work);
+
+K_WORK_DELAYABLE_DEFINE(rx_process_work, rx_process);
+K_WORK_DEFINE(dtr_uart_enable_work, dtr_uart_enable_work_fn);
+K_WORK_DELAYABLE_DEFINE(dtr_uart_disable_work, dtr_uart_disable_work_fn);
+
 struct rx_buf_t {
 	atomic_t ref_counter;
 	uint8_t buf[CONFIG_SM_AT_CLIENT_UART_RX_BUF_SIZE];
@@ -73,6 +80,7 @@ static K_SEM_DEFINE(at_rsp, 0, 1);
 
 static sm_data_handler_t data_handler;
 static enum at_cmd_state sm_at_state;
+static bool initialized;
 
 #if defined(CONFIG_SM_AT_CLIENT_SHELL)
 static const struct shell *global_shell;
@@ -97,10 +105,6 @@ struct dtr_config {
 
 	/* Current DTR state. */
 	bool active;
-
-	/* Work items for enabling and disabling DTR UART. */
-	struct k_work dtr_uart_enable_work;
-	struct k_work_delayable dtr_uart_disable_work;
 };
 
 static struct dtr_config dtr_config = {
@@ -446,10 +450,10 @@ static void reschedule_disable(void)
 {
 	if (dtr_config.active && dtr_config.automatic) {
 		/* Restart the inactivity timer. */
-		k_work_reschedule(&dtr_config.dtr_uart_disable_work, dtr_config.inactivity);
+		k_work_reschedule(&dtr_uart_disable_work, dtr_config.inactivity);
 	} else {
 		/* Stop the inactivity timer. */
-		k_work_cancel_delayable(&dtr_config.dtr_uart_disable_work);
+		k_work_cancel_delayable(&dtr_uart_disable_work);
 	}
 }
 
@@ -713,7 +717,7 @@ static void gpio_cb_func(const struct device *dev, struct gpio_callback *gpio_cb
 
 	if (dtr_config.automatic && !dtr_config.active) {
 		/* Wake up the application */
-		k_work_submit(&dtr_config.dtr_uart_enable_work);
+		k_work_submit(&dtr_uart_enable_work);
 	}
 
 	if (ri_handler) {
@@ -787,7 +791,6 @@ static int gpio_init(void)
 int sm_at_client_init(sm_data_handler_t handler, bool automatic, k_timeout_t inactivity)
 {
 	int err;
-	static bool initialized;
 
 	if (initialized) {
 		return -EALREADY;
@@ -810,11 +813,6 @@ int sm_at_client_init(sm_data_handler_t handler, bool automatic, k_timeout_t ina
 		return -EFAULT;
 	}
 
-	k_work_init_delayable(&rx_process_work, rx_process);
-
-	k_work_init(&dtr_config.dtr_uart_enable_work, dtr_uart_enable_work_fn);
-	k_work_init_delayable(&dtr_config.dtr_uart_disable_work, dtr_uart_disable_work_fn);
-
 	/* Initialize shell pointer so it's available for printing in callbacks */
 #if defined(CONFIG_SHELL_BACKEND_SERIAL)
 	global_shell = shell_backend_uart_get_ptr();
@@ -848,6 +846,7 @@ int sm_at_client_uninit(void)
 	data_handler = NULL;
 	ri_handler = NULL;
 	sm_at_state = AT_CMD_OK;
+	initialized = false;
 
 	return 0;
 }
@@ -868,6 +867,11 @@ int sm_at_client_send_cmd(const char *const command, uint32_t timeout)
 {
 	int ret;
 
+	if (!initialized) {
+		LOG_ERR("AT client not initialized");
+		return -EPERM;
+	}
+
 	sm_at_state = AT_CMD_PENDING;
 	ret = tx_write(command, strlen(command), false);
 	if (ret < 0) {
@@ -900,32 +904,52 @@ int sm_at_client_send_cmd(const char *const command, uint32_t timeout)
 
 int sm_at_client_send_data(const uint8_t *const data, size_t datalen)
 {
+	if (!initialized) {
+		LOG_ERR("AT client not initialized");
+		return -EPERM;
+	}
+
 	return tx_write(data, datalen, true);
 }
 
 void sm_at_client_configure_dtr_uart(bool automatic, k_timeout_t inactivity)
 {
+	if (!initialized) {
+		LOG_ERR("AT client not initialized");
+		return;
+	}
+
 	dtr_config.automatic = automatic;
 	dtr_config.inactivity = inactivity;
 
 	if (dtr_config.automatic && !dtr_config.active && !ring_buf_is_empty(&tx_buf)) {
 		/* If automatic DTR UART is enabled and there is data to send, enable DTR UART. */
-		k_work_submit(&dtr_config.dtr_uart_enable_work);
+		k_work_submit(&dtr_uart_enable_work);
 	} else {
 		reschedule_disable();
 	}
 }
 
 void sm_at_client_disable_dtr_uart(void)
 {
+	if (!initialized) {
+		LOG_ERR("AT client not initialized");
+		return;
+	}
+
 	sm_at_client_configure_dtr_uart(false, K_NO_WAIT);
-	k_work_reschedule(&dtr_config.dtr_uart_disable_work, K_NO_WAIT);
+	k_work_reschedule(&dtr_uart_disable_work, K_NO_WAIT);
 }
 
 void sm_at_client_enable_dtr_uart(void)
 {
+	if (!initialized) {
+		LOG_ERR("AT client not initialized");
+		return;
+	}
+
 	sm_at_client_configure_dtr_uart(false, K_NO_WAIT);
-	k_work_submit(&dtr_config.dtr_uart_enable_work);
+	k_work_submit(&dtr_uart_enable_work);
 }
 
 #if defined(CONFIG_SM_AT_CLIENT_SHELL)

lib/sm_at_client/sm_at_client_monitor.c

@@ -39,42 +39,76 @@ void sm_monitor_dispatch(const char *notif, size_t len)
 	struct at_notif_fifo *at_notif;
 	size_t sz_needed;
 	size_t notif_len = 0;
-	char *match = NULL;
+	const char *current = notif;
+	const char *end = notif + len;
+	bool queued_any = false;
 
 	/* TODO:
-	 * To reliably separate AT notifications from AT commands, data and other
-	 * AT notifications would require that the AT notifications are separated with control
-	 * characters or possibly sent with a separate CMUX channel.
+	 * - Not called if URC comes immediately after sending an AT command.
+	 *   ATE1 should be used, and echoed AT-command should be matched to received data to
+	 *   deduct the start of the AT response.
+	 * - Cannot handle the case where URC is split over multiple UART RX buffers.
+	 * - Cannot separate URC from data mode data which may also contain \r\n\ sequences.
 	 */
-	STRUCT_SECTION_FOREACH(sm_monitor_entry, e) {
-		if (!is_paused(e)) {
-			match = strnstr(notif, e->filter, len);
-			if (match) {
-				notif_len = len - (size_t)(match - notif);
+
+	while (current < end) {
+		/* Process each notification in the buffer.
+		 * Notifications are delimited by \r\n\r\n (end of one + start of next).
+		 */
+		const char *next_delim = strnstr(current, "\r\n\r\n", end - current);
+		bool has_monitor_match = false;
+
+		if (next_delim) {
+			/* Include the trailing \r\n in this notification */
+			notif_len = (next_delim - current) + 2;
+		} else {
+			/* Last notification in buffer */
+			notif_len = end - current;
+		}
+
+		if (notif_len == 0) {
+			break;
+		}
+
+		/* Check if any monitor is interested in this notification. */
+		STRUCT_SECTION_FOREACH(sm_monitor_entry, e) {
+			if (!is_paused(e)) {
+				if (e->filter == MON_ANY) {
+					has_monitor_match = true;
+					break;
+				}
+				if (strnstr(current, e->filter, notif_len)) {
+					has_monitor_match = true;
+					break;
+				}
+			}
+		}
+
+		if (has_monitor_match) {
+			sz_needed = sizeof(struct at_notif_fifo) + notif_len + sizeof(char);
+			at_notif = k_heap_alloc(&at_monitor_heap, sz_needed, K_NO_WAIT);
+			if (!at_notif) {
+				LOG_WRN("No heap space for incoming notification");
+				/* Submit work for already queued notifications and stop. */
 				break;
 			}
+			strncpy(at_notif->data, current, notif_len);
+			at_notif->data[notif_len] = '\0';
+			k_fifo_put(&at_monitor_fifo, at_notif);
+			queued_any = true;
 		}
-	}
 
-	if (!match) {
-		/* Only copy monitored notifications to save heap */
-		return;
+		/* Move to next notification (skip the \r\n\ delimiter from the current). */
+		if (next_delim) {
+			current = next_delim + 2;
+		} else {
+			break;
+		}
 	}
 
-	sz_needed = sizeof(struct at_notif_fifo) + notif_len + sizeof(char);
-
-	at_notif = k_heap_alloc(&at_monitor_heap, sz_needed, K_NO_WAIT);
-	if (!at_notif) {
-		LOG_WRN("No heap space for incoming notification: %s",
-			notif);
-		return;
+	if (queued_any) {
+		k_work_submit(&at_monitor_work);
 	}
-
-	strncpy(at_notif->data, match, notif_len);
-	at_notif->data[notif_len] = '\0';
-
-	k_fifo_put(&at_monitor_fifo, at_notif);
-	k_work_submit(&at_monitor_work);
 }
 
 static void sm_monitor_task(struct k_work *work)