shell: Allow calling shell_fprintf from various contexts

Extended shell to allow command to indicate that shell should
halt not accepting any input until termination sequence is
received (CTRL+C) or shell_command_exit() is called. While shell
is in that state it is allowed to print to shell from any thread
context.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

doc/subsystems/shell/shell.rst

@@ -235,13 +235,41 @@ Simple command handler implementation:
 		return 0;
 	}
 
-.. warning::
+Function :cpp:func:`shell_fprintf` or the shell print macros:
-	Do not use function :cpp:func:`shell_fprintf` or shell print macros:
+:c:macro:`shell_print`, :c:macro:`shell_info`, :c:macro:`shell_warn` and
-	:c:macro:`shell_print`, :c:macro:`shell_info`, :c:macro:`shell_warn`,
+:c:macro:`shell_error` can only be used from the command handler, or if the
-	:c:macro:`shell_error` outside of the command handler because this
+command context is forced to stay in the foreground by calling
-	might lead to incorrect text display on the screen. If any text should
+:cpp:func:`shell_command_enter` from within the command handler. In this latter
-	be displayed outside of the command context, then use the
+case, the shell stops reading input and writing to the output (except for the
-	:ref:`logger` or `printk` function.
+logs), allowing a user to print from any thread context. Function
+:cpp:func:`shell_command_exit` or entering a :kbd:`CTRL+C` terminates a
+'foreground' command.
+
+Here is an example foreground command implementation:
+
+.. code-block:: c
+
+	static int cmd_handler(const struct shell *shell, size_t argc,
+				char **argv)
+	{
+		ARG_UNUSED(argc);
+		ARG_UNUSED(argv);
+
+		shell_command_enter(shell);
+
+		foo_shell = shell;
+		foo_signal_thread(); /* Swtich context */
+
+		return 0;
+	}
+
+	static void foo_thread_context(void)
+	{
+		shell_print(foo_shell, "Lorem ipsum");
+
+		/* Terminate foreground command. */
+		shell_command_exit(foo_shell);
+	}
 
 Command help
 ------------

include/shell/shell.h

@@ -89,6 +89,15 @@ struct shell_static_args {
 
 /**
  * @brief Shell command handler prototype.
+ *
+ * @param shell Shell instance.
+ * @param argc  Arguments count.
+ * @param argv  Arguments.
+ *
+ * @retval 0 Successful command execution.
+ * @retval 1 Help printed and command not executed.
+ * @retval -EINVAL Argument validation failed.
+ * @retval -ENOEXEC Command not executed.
  */
 typedef int (*shell_cmd_handler)(const struct shell *shell,
 				 size_t argc, char **argv);
@@ -238,6 +247,7 @@ enum shell_state {
 	SHELL_STATE_UNINITIALIZED,
 	SHELL_STATE_INITIALIZED,
 	SHELL_STATE_ACTIVE,
+	SHELL_STATE_COMMAND,
 	SHELL_STATE_PANIC_MODE_ACTIVE,  /*!< Panic activated.*/
 	SHELL_STATE_PANIC_MODE_INACTIVE /*!< Panic requested, not supported.*/
 };
@@ -297,10 +307,10 @@ struct shell_transport_api {
 	/**
 	 * @brief Function for writing data to the transport interface.
 	 *
-	 * @param[in] transport  Pointer to the transfer instance.
+	 * @param[in]  transport  Pointer to the transfer instance.
-	 * @param[in] data       Pointer to the source buffer.
+	 * @param[in]  data       Pointer to the source buffer.
-	 * @param[in] length     Source buffer length.
+	 * @param[in]  length     Source buffer length.
-	 * @param[in] cnt        Pointer to the sent bytes counter.
+	 * @param[out] cnt        Pointer to the sent bytes counter.
 	 *
 	 * @return Standard error code.
 	 */
@@ -310,10 +320,10 @@ struct shell_transport_api {
 	/**
 	 * @brief Function for reading data from the transport interface.
 	 *
-	 * @param[in] p_transport  Pointer to the transfer instance.
+	 * @param[in]  p_transport  Pointer to the transfer instance.
-	 * @param[in] p_data       Pointer to the destination buffer.
+	 * @param[in]  p_data       Pointer to the destination buffer.
-	 * @param[in] length       Destination buffer length.
+	 * @param[in]  length       Destination buffer length.
-	 * @param[in] cnt          Pointer to the received bytes counter.
+	 * @param[out] cnt          Pointer to the received bytes counter.
 	 *
 	 * @return Standard error code.
 	 */
@@ -370,9 +380,10 @@ union shell_internal {
 
 enum shell_signal {
 	SHELL_SIGNAL_RXRDY,
-	SHELL_SIGNAL_TXDONE,
 	SHELL_SIGNAL_LOG_MSG,
 	SHELL_SIGNAL_KILL,
+	SHELL_SIGNAL_COMMAND_EXIT,
+	SHELL_SIGNAL_TXDONE,
 	SHELL_SIGNALS
 };
 
@@ -407,6 +418,9 @@ struct shell_ctx {
 
 	struct k_poll_signal signals[SHELL_SIGNALS];
 	struct k_poll_event events[SHELL_SIGNALS];
+
+	struct k_mutex wr_mtx;
+	k_tid_t tid;
 };
 
 extern const struct log_backend_api log_backend_shell_api;
@@ -561,7 +575,11 @@ int shell_stop(const struct shell *shell);
 
 /**
  * @brief printf-like function which sends formatted data stream to the shell.
- *	  This function shall not be used outside of the shell command context.
+ *
+ * This function shall not be used outside of the shell command context unless
+ * command requested to stay in the foreground (see @ref shell_command_enter).
+ * In that case, function can be called from any thread context until command is
+ * terminated with CTRL+C or @ref shell_command_exit call.
  *
  * @param[in] shell	Pointer to the shell instance.
  * @param[in] color	Printed text color.
@@ -574,7 +592,7 @@ void shell_fprintf(const struct shell *shell, enum shell_vt100_color color,
 /**
  * @brief Print info message to the shell.
  *
- *  This function shall not be used outside of the shell command context.
+ * See @ref shell_fprintf.
  *
  * @param[in] _sh Pointer to the shell instance.
  * @param[in] _ft Format string.
@@ -586,7 +604,7 @@ void shell_fprintf(const struct shell *shell, enum shell_vt100_color color,
 /**
  * @brief Print normal message to the shell.
  *
- *  This function shall not be used outside of the shell command context.
+ * See @ref shell_fprintf.
  *
  * @param[in] _sh Pointer to the shell instance.
  * @param[in] _ft Format string.
@@ -598,7 +616,7 @@ void shell_fprintf(const struct shell *shell, enum shell_vt100_color color,
 /**
  * @brief Print warning message to the shell.
  *
- * This function shall not be used outside of the shell command context.
+ * See @ref shell_fprintf.
  *
  * @param[in] _sh Pointer to the shell instance.
  * @param[in] _ft Format string.
@@ -610,7 +628,8 @@ void shell_fprintf(const struct shell *shell, enum shell_vt100_color color,
 /**
  * @brief Print error message to the shell.
  *
- * This function shall not be used outside of the shell command context.
+ * See @ref shell_fprintf.
+ *
  * @param[in] _sh Pointer to the shell instance.
  * @param[in] _ft Format string.
  * @param[in] ... List of parameters to print.
@@ -626,6 +645,24 @@ void shell_fprintf(const struct shell *shell, enum shell_vt100_color color,
  */
 void shell_process(const struct shell *shell);
 
+/**
+ * @brief Indicate to shell that command stay in foreground, blocking the shell.
+ *
+ * Command in foreground is terminated by @ref shell_command_exit or CTRL+C.
+ *
+ * @param[in] shell	Pointer to the shell instance.
+ */
+void shell_command_enter(const struct shell *shell);
+
+/**
+ * @brief Exit command in foreground state.
+ *
+ * See @ref shell_command_enter.
+ *
+ * @param[in] shell	Pointer to the shell instance.
+ */
+void shell_command_exit(const struct shell *shell);
+
 /**
  * @brief Change displayed shell prompt.
  *

subsys/shell/shell.c

@@ -859,17 +859,22 @@ static void state_collect(const struct shell *shell)
 		switch (shell->ctx->receive_state) {
 		case SHELL_RECEIVE_DEFAULT:
 			if (process_nl(shell, data)) {
+				int err = 0;
+
 				if (!shell->ctx->cmd_buff_len) {
 					history_mode_exit(shell);
 					cursor_next_line_move(shell);
 				} else {
 					/* Command execution */
-					(void)execute(shell);
+					err = execute(shell);
 				}
 				/* Function responsible for printing prompt
 				 * on received NL.
 				 */
-				state_set(shell, SHELL_STATE_ACTIVE);
+				if (err ||
+				    shell->ctx->state != SHELL_STATE_COMMAND) {
+					state_set(shell, SHELL_STATE_ACTIVE);
+				}
 				return;
 			}
 
@@ -1008,6 +1013,8 @@ static void state_collect(const struct shell *shell)
 			break;
 		}
 	}
+
+	transport_buffer_flush(shell);
 }
 
 static void transport_evt_handler(enum shell_transport_evt evt_type, void *ctx)
@@ -1024,24 +1031,37 @@ static void transport_evt_handler(enum shell_transport_evt evt_type, void *ctx)
 static void shell_log_process(const struct shell *shell)
 {
 	bool processed;
-	int signaled;
+	int signaled = 0;
 	int result;
 
 	do {
-		shell_cmd_line_erase(shell);
+		if (shell->ctx->state < SHELL_STATE_PANIC_MODE_ACTIVE) {
-		processed = shell_log_backend_process(shell->log_backend);
+			k_mutex_lock(&shell->ctx->wr_mtx, K_FOREVER);
-		cmd_line_print(shell);
-
-		/* Arbitrary delay added to ensure that prompt is readable and
-		 * can be used to enter further commands.
-		 */
-		if (shell->ctx->cmd_buff_len) {
-			k_sleep(K_MSEC(15));
 		}
 
-		k_poll_signal_check(&shell->ctx->signals[SHELL_SIGNAL_RXRDY],
+		shell_cmd_line_erase(shell);
-						    &signaled, &result);
 
+		processed = shell_log_backend_process(shell->log_backend);
+
+		if (shell->ctx->state != SHELL_STATE_COMMAND) {
+			struct k_poll_signal *signal =
+				&shell->ctx->signals[SHELL_SIGNAL_RXRDY];
+
+			cmd_line_print(shell);
+
+			/* Arbitrary delay added to ensure that prompt is readable and
+			 * can be used to enter further commands.
+			 */
+			if (shell->ctx->cmd_buff_len) {
+				k_sleep(K_MSEC(15));
+			}
+
+			k_poll_signal_check( signal, &signaled, &result);
+		}
+
+		if (shell->ctx->state < SHELL_STATE_PANIC_MODE_ACTIVE) {
+			k_mutex_unlock(&shell->ctx->wr_mtx);
+		}
 	} while (processed && !signaled);
 }
 
@@ -1056,14 +1076,15 @@ static int instance_init(const struct shell *shell, const void *p_config,
 	int err = shell->iface->api->init(shell->iface, p_config,
 					  transport_evt_handler,
 					  (void *) shell);
-
 	if (err != 0) {
 		return err;
 	}
 
+	memset(shell->ctx, 0, sizeof(*shell->ctx));
+
 	history_init(shell);
 
-	memset(shell->ctx, 0, sizeof(*shell->ctx));
+	k_mutex_init(&shell->ctx->wr_mtx);
 
 	if (IS_ENABLED(CONFIG_SHELL_STATS)) {
 		shell->stats->log_lost_cnt = 0;
@@ -1110,6 +1131,35 @@ static int instance_uninit(const struct shell *shell)
 	return 0;
 }
 
+typedef void (*shell_signal_handler_t)(const struct shell *shell);
+
+static void shell_signal_handle(const struct shell *shell,
+				enum shell_signal sig_idx,
+				shell_signal_handler_t handler)
+{
+	struct k_poll_signal *signal = &shell->ctx->signals[sig_idx];
+	int set;
+	int res;
+
+	k_poll_signal_check(signal, &set, &res);
+
+	if (set) {
+		k_poll_signal_reset(signal);
+		handler(shell);
+	}
+}
+
+static void kill_handler(const struct shell *shell)
+{
+	(void)instance_uninit(shell);
+	k_thread_abort(k_current_get());
+}
+
+static void command_exit_handler(const struct shell *shell)
+{
+	state_set(shell, SHELL_STATE_ACTIVE);
+}
+
 void shell_thread(void *shell_handle, void *arg_log_backend,
 		  void *arg_log_level)
 {
@@ -1137,40 +1187,29 @@ void shell_thread(void *shell_handle, void *arg_log_backend,
 	}
 
 	while (true) {
-		int signaled;
+		int num_events = (shell->ctx->state != SHELL_STATE_COMMAND) ?
-		int result;
+				SHELL_SIGNALS : SHELL_SIGNAL_TXDONE;
 
-		err = k_poll(shell->ctx->events, SHELL_SIGNALS, K_FOREVER);
+		err = k_poll(shell->ctx->events, num_events, K_FOREVER);
 		if (err != 0) {
 			return;
 		}
 
-		k_poll_signal_check(&shell->ctx->signals[SHELL_SIGNAL_KILL],
+		/* Check for KILL request */
-				    &signaled, &result);
+		shell_signal_handle(shell, SHELL_SIGNAL_KILL, kill_handler);
+		shell_signal_handle(shell, SHELL_SIGNAL_RXRDY, shell_process);
 
-		if (signaled) {
+		if  (shell->ctx->state == SHELL_STATE_COMMAND) {
-			k_poll_signal_reset(
+			shell_signal_handle(shell, SHELL_SIGNAL_COMMAND_EXIT,
-				&shell->ctx->signals[SHELL_SIGNAL_KILL]);
+					    command_exit_handler);
-			(void)instance_uninit(shell);
+		} else {
-
+			shell_signal_handle(shell, SHELL_SIGNAL_TXDONE,
-			k_thread_abort(k_current_get());
+					shell_process);
 		}
 
-		k_poll_signal_check(&shell->ctx->signals[SHELL_SIGNAL_LOG_MSG],
+		if (IS_ENABLED(CONFIG_LOG)) {
-				    &signaled, &result);
+			shell_signal_handle(shell, SHELL_SIGNAL_LOG_MSG,
-
+					    shell_log_process);
-		if (!signaled) {
-			/* Other signals handled together.*/
-			k_poll_signal_reset(
-				&shell->ctx->signals[SHELL_SIGNAL_RXRDY]);
-			k_poll_signal_reset(
-				&shell->ctx->signals[SHELL_SIGNAL_TXDONE]);
-			shell_process(shell);
-		} else if (IS_ENABLED(CONFIG_LOG)) {
-			k_poll_signal_reset(
-				&shell->ctx->signals[SHELL_SIGNAL_LOG_MSG]);
-			/* process log msg */
-			shell_log_process(shell);
 		}
 	}
 }
@@ -1193,6 +1232,7 @@ int shell_init(const struct shell *shell, const void *transport_config,
 			      (void *)init_log_level,
 			      K_LOWEST_APPLICATION_THREAD_PRIO, 0, K_NO_WAIT);
 
+	shell->ctx->tid = tid;
 	k_thread_name_set(tid, shell->thread_name);
 
 	return 0;
@@ -1254,6 +1294,35 @@ int shell_stop(const struct shell *shell)
 	return 0;
 }
 
+void shell_command_enter(const struct shell *shell)
+{
+	state_set(shell, SHELL_STATE_COMMAND);
+}
+
+void shell_command_exit(const struct shell *shell)
+{
+	struct k_poll_signal *signal =
+			&shell->ctx->signals[SHELL_SIGNAL_COMMAND_EXIT];
+
+	(void)k_poll_signal_raise(signal, 0);
+}
+
+static void shell_state_command(const struct shell *shell)
+{
+	size_t count;
+	char data;
+
+	(void)shell->iface->api->read(shell->iface, &data,
+				      sizeof(data), &count);
+	if (count == 0) {
+		return;
+	}
+
+	if (data == SHELL_VT100_ASCII_CTRL_C) {
+		state_set(shell, SHELL_STATE_ACTIVE);
+	}
+}
+
 void shell_process(const struct shell *shell)
 {
 	__ASSERT_NO_MSG(shell);
@@ -1276,13 +1345,14 @@ void shell_process(const struct shell *shell)
 	case SHELL_STATE_ACTIVE:
 		state_collect(shell);
 		break;
+	case SHELL_STATE_COMMAND:
+		shell_state_command(shell);
+		break;
 
 	default:
 		break;
 	}
 
-	transport_buffer_flush(shell);
-
 	internal.value = 0xFFFFFFFF;
 	internal.flags.processing = 0;
 	(void)atomic_and((atomic_t *)&shell->ctx->internal.value,
@@ -1293,14 +1363,22 @@ void shell_fprintf(const struct shell *shell, enum shell_vt100_color color,
 		   const char *p_fmt, ...)
 {
 	__ASSERT_NO_MSG(shell);
+	__ASSERT(!k_is_in_isr(), "Thread context required.");
 	__ASSERT_NO_MSG(shell->ctx);
 	__ASSERT_NO_MSG(shell->fprintf_ctx);
 	__ASSERT_NO_MSG(p_fmt);
 
 	va_list args = { 0 };
 
+	if (k_current_get() != shell->ctx->tid &&
+			shell->ctx->state != SHELL_STATE_COMMAND) {
+		return;
+	}
+
 	va_start(args, p_fmt);
 
+	k_mutex_lock(&shell->ctx->wr_mtx, K_FOREVER);
+
 	if (IS_ENABLED(CONFIG_SHELL_VT100_COLORS) &&
 	    shell->ctx->internal.flags.use_colors &&
 	    (color != shell->ctx->vt100_ctx.col.col)) {
@@ -1317,6 +1395,8 @@ void shell_fprintf(const struct shell *shell, enum shell_vt100_color color,
 	}
 
 	va_end(args);
+
+	k_mutex_unlock(&shell->ctx->wr_mtx);
 }
 
 int shell_prompt_change(const struct shell *shell, char *prompt)

subsys/shell/shell_ops.c

@@ -276,7 +276,8 @@ void shell_cmd_line_erase(const struct shell *shell)
 
 static void shell_pend_on_txdone(const struct shell *shell)
 {
-	if (IS_ENABLED(CONFIG_MULTITHREADING)) {
+	if (IS_ENABLED(CONFIG_MULTITHREADING) &&
+	    (shell->ctx->state < SHELL_STATE_PANIC_MODE_ACTIVE)) {
 		k_poll(&shell->ctx->events[SHELL_SIGNAL_TXDONE], 1, K_FOREVER);
 		k_poll_signal_reset(&shell->ctx->signals[SHELL_SIGNAL_TXDONE]);
 	} else {