shell: Refactor shell_history to use less RW memory

Shell history module reworked to use ring buffer for storing
commands. Dedicated buffer is used to story all command lineary.
History capacity is in bytes not in number of entries, e.g.
many short commands can be stored or few long (depending on
CONFIG_SHELL_HISTORY_BUFFER).

Removed implicit command null termination from shell_history and
added it to shell after fetching command line from the history.

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

include/shell/shell.h

@@ -30,6 +30,10 @@ extern "C" {
 #define CONFIG_SHELL_PRINTF_BUFF_SIZE 0
 #endif
 
+#ifndef CONFIG_SHELL_HISTORY_BUFFER
+#define CONFIG_SHELL_HISTORY_BUFFER 0
+#endif
+
 #define SHELL_CMD_ROOT_LVL		(0u)
 
 /**
@@ -643,7 +647,7 @@ extern void shell_print_stream(const void *user_ctx, const char *data,
 	SHELL_LOG_BACKEND_DEFINE(_name, _name##_out_buffer,		      \
 				 CONFIG_SHELL_PRINTF_BUFF_SIZE,		      \
 				 _log_queue_size, _log_timeout);	      \
-	SHELL_HISTORY_DEFINE(_name, CONFIG_SHELL_CMD_BUFF_SIZE, 7);	      \
+	SHELL_HISTORY_DEFINE(_name##_history, CONFIG_SHELL_HISTORY_BUFFER);   \
 	SHELL_FPRINTF_DEFINE(_name##_fprintf, &_name, _name##_out_buffer,     \
 			     CONFIG_SHELL_PRINTF_BUFF_SIZE,		      \
 			     true, shell_print_stream);			      \
@@ -655,7 +659,8 @@ extern void shell_print_stream(const void *user_ctx, const char *data,
 		.default_prompt = _prompt,				      \
 		.iface = _transport_iface,				      \
 		.ctx = &UTIL_CAT(_name, _ctx),				      \
-		.history = SHELL_HISTORY_PTR(_name),			      \
+		.history = IS_ENABLED(CONFIG_SHELL_HISTORY) ?		      \
+				&_name##_history : NULL,		      \
 		.shell_flag = _shell_flag,				      \
 		.fprintf_ctx = &_name##_fprintf,			      \
 		.stats = SHELL_STATS_PTR(_name),			      \

include/shell/shell_history.h

@@ -10,6 +10,7 @@
 #include <zephyr.h>
 #include <misc/util.h>
 #include <misc/dlist.h>
+#include <ring_buffer.h>
 #include <stdbool.h>
 
 #ifdef __cplusplus
@@ -18,45 +19,88 @@ extern "C" {
 
 
 struct shell_history {
-	struct k_mem_slab *mem_slab;
+	struct ring_buf *ring_buf;
 	sys_dlist_t list;
 	sys_dnode_t *current;
 };
 
-struct shell_history_item {
+/**
-	sys_dnode_t dnode;
+ * @brief Create shell history instance.
-	u16_t len;
+ *
-	char data[];
+ * @param _name History instance name.
-};
+ * @param _size Memory dedicated for shell history.
-
+ */
-#ifdef CONFIG_SHELL_HISTORY
+#define SHELL_HISTORY_DEFINE(_name, _size) \
-#define SHELL_HISTORY_DEFINE(_name, block_size, block_count)	\
+	static u8_t __noinit __aligned(sizeof(u32_t)) \
-								\
+			_name##_ring_buf_data[_size]; \
-	K_MEM_SLAB_DEFINE(_name##_history_memslab,		\
+	static struct ring_buf _name##_ring_buf = \
-		 ROUND_UP(block_size + sizeof(struct shell_history_item), \
+		{ \
-			  sizeof(void *)), block_count, 4);		\
+			.size = _size, \
-	static struct shell_history _name##_history = {		\
+			.buf =  { .buf8 = _name##_ring_buf_data } \
-		.mem_slab = &_name##_history_memslab		\
+		}; \
+	static struct shell_history _name = { \
+		.ring_buf = &_name##_ring_buf \
 	}
-#define SHELL_HISTORY_PTR(_name) (&_name##_history)
-#else /* CONFIG_SHELL_HISTORY */
-#define SHELL_HISTORY_DEFINE(_name, block_size, block_count) /*empty*/
-#define SHELL_HISTORY_PTR(_name) NULL
-#endif
 
 
+/**
+ * @brief Initialize shell history module.
+ *
+ * @param history Shell history instance.
+ */
 void shell_history_init(struct shell_history *history);
 
+/**
+ * @brief Purge shell history.
+ *
+ * Function clears whole shell command history.
+ *
+ * @param history Shell history instance.
+ *
+ */
 void shell_history_purge(struct shell_history *history);
 
+/**
+ * @brief Exit history browsing mode.
+ *
+ * @param history Shell history instance.
+ */
 void shell_history_mode_exit(struct shell_history *history);
 
-/* returns true if remains in history mode.*/
+/**
+ * @brief Get next entry in shell command history.
+ *
+ * Function returns next (in given direction) stored line.
+ *
+ * @param[in]     history	Shell history instance.
+ * @param[in]     up		Direction.
+ * @param[out]    dst		Buffer where line is copied.
+ * @param[in,out] len		Buffer size (intput), amount of copied
+ *				data (output).
+ * @return True if remains in history mode.
+ */
 bool shell_history_get(struct shell_history *history, bool up,
 		       u8_t *dst, u16_t *len);
 
+/**
+ * @brief Put line into shell command history.
+ *
+ * If history is full, oldest entry (or entries) is removed.
+ *
+ * @param history	Shell history instance.
+ * @param line		Data.
+ * @param len		Data length.
+ *
+ */
 void shell_history_put(struct shell_history *history, u8_t *line, size_t len);
 
+/**
+ * @brief Get state of shell history.
+ *
+ * @param history	Shell history instance.
+ *
+ * @return True if in browsing mode.
+ */
 static inline bool shell_history_active(struct shell_history *history)
 {
 	return (history->current) ? true : false;

subsys/shell/Kconfig

@@ -99,6 +99,7 @@ config SHELL_HELP_ON_WRONG_ARGUMENT_COUNT
 config SHELL_HISTORY
 	bool "Enable history in shell"
 	default y
+	select RING_BUFFER
 	help
 	  Enable commands history. History can be accessed using up and down
 	  arrows.
@@ -107,7 +108,7 @@ if SHELL_HISTORY
 
 config SHELL_HISTORY_BUFFER
 	int "History buffer in bytes"
-	default 1024
+	default 512
 	help
 	  Number of bytes dedicated for storing executed commands.
 

subsys/shell/shell_history.c

@@ -7,6 +7,40 @@
 #include <shell/shell_history.h>
 #include <string.h>
 
+/*
+ * History must store strings (commands) and allow traversing them and adding
+ * new string. When new item is added then first it is compared if it is not
+ * the same as the last one (then it is not stored). If there is no room in the
+ * buffer to store the new item, oldest one is removed until there is a room.
+ *
+ * Items are allocated and stored in the ring buffer. Items then a linked in
+ * the list.
+ *
+ * Because stored strings must be copied and compared, it is more convenient to
+ * store them in the ring buffer in a way that they are not split into two
+ * chunks (when ring buffer wraps). To ensure that item is in a single chunk,
+ * item includes padding. If continues area for new item cannot be allocated
+ * then allocated space is increased by the padding.
+ *
+ * If item does not fit at the end of the ring buffer padding is added: *
+ * +-----------+----------------+-----------------------------------+---------+
+ * | header    | "history item" |                                   | padding |
+ * | padding   |                |                                   |         |
+ * +-----------+----------------+-----------------------------------+---------+
+ *
+ * If item fits in the ring buffer available space then there is no padding:
+ * +-----------------+------------+----------------+--------------------------+
+ * |                 | header     | "history item" |                          |
+ * |                 | no padding |                |                          |
+ * +-----------------+------------+----------------+--------------------------+
+ */
+struct shell_history_item {
+	sys_dnode_t dnode;
+	u16_t len;
+	u16_t padding;
+	char data[0];
+};
+
 void shell_history_mode_exit(struct shell_history *history)
 {
 	history->current = NULL;
@@ -43,7 +77,7 @@ bool shell_history_get(struct shell_history *history, bool up,
 	history->current = l_item;
 	h_item = CONTAINER_OF(l_item, struct shell_history_item, dnode);
 
-	if (h_item) {
+	if (l_item) {
 		memcpy(dst, h_item->data, h_item->len);
 		*len = h_item->len;
 		dst[*len] = '\0';
@@ -56,30 +90,40 @@ bool shell_history_get(struct shell_history *history, bool up,
 
 static void add_to_head(struct shell_history *history,
 			struct shell_history_item *item,
-			u8_t *src, size_t len)
+			u8_t *src, size_t len, u16_t padding)
 {
 	item->len = len;
+	item->padding = padding;
 	memcpy(item->data, src, len);
-	item->data[len] = '\0';
 	sys_dlist_prepend(&history->list, &item->dnode);
 }
 
-static void remove_from_tail(struct shell_history *history)
+/* Returns true if element was removed. */
+static bool remove_from_tail(struct shell_history *history)
 {
 	sys_dnode_t *l_item; /* list item */
 	struct shell_history_item *h_item;
+	u32_t total_len;
+
+	if (sys_dlist_is_empty(&history->list)) {
+		return false;
+	}
 
 	l_item = sys_dlist_peek_tail(&history->list);
 	sys_dlist_remove(l_item);
 
 	h_item = CONTAINER_OF(l_item, struct shell_history_item, dnode);
-	k_mem_slab_free(history->mem_slab, (void **)&l_item);
+
+	total_len = offsetof(struct shell_history_item, data) +
+			h_item->len + h_item->padding;
+	ring_buf_get_finish(history->ring_buf, total_len);
+
+	return true;
 }
 
 void shell_history_purge(struct shell_history *history)
 {
-	while (!sys_dlist_is_empty(&history->list)) {
+	while (remove_from_tail(history)) {
-		remove_from_tail(history);
 	}
 }
 
@@ -87,6 +131,17 @@ void shell_history_put(struct shell_history *history, u8_t *line, size_t len)
 {
 	sys_dnode_t *l_item; /* list item */
 	struct shell_history_item *h_item;
+	u32_t total_len = len + offsetof(struct shell_history_item, data);
+	u32_t claim_len;
+	u32_t claim2_len;
+	u16_t padding = (~total_len + 1) & (sizeof(void *) - 1);
+
+	/* align to word. */
+	total_len += padding;
+
+	if (total_len > ring_buf_capacity_get(history->ring_buf)) {
+		return;
+	}
 
 	shell_history_mode_exit(history);
 
@@ -97,20 +152,50 @@ void shell_history_put(struct shell_history *history, u8_t *line, size_t len)
 	l_item = sys_dlist_peek_head(&history->list);
 	h_item = CONTAINER_OF(l_item, struct shell_history_item, dnode);
 
-	if (h_item &&
+	if (l_item &&
 	   (h_item->len == len) &&
-	   (strncmp(h_item->data, line, CONFIG_SHELL_CMD_BUFF_SIZE) == 0)) {
+	   (memcmp(h_item->data, line, len) == 0)) {
 		/* Same command as before, do not store */
 		return;
 	}
 
-	while (k_mem_slab_alloc(history->mem_slab, (void **)&h_item, K_NO_WAIT)
+	do {
-			!= 0) {
+		claim_len = ring_buf_put_claim(history->ring_buf,
-		/* if no space remove the oldest entry. */
+						(u8_t **)&h_item, total_len);
-		remove_from_tail(history);
+		/* second allocation may succeed if we were at the end of the
-	}
+		 * buffer.
+		 */
+		if (claim_len < total_len) {
+			claim2_len =
+				ring_buf_put_claim(history->ring_buf,
+						   (u8_t **)&h_item, total_len);
+			if (claim2_len == total_len) {
+				ring_buf_put_finish(history->ring_buf,
+						    claim_len);
+				padding += claim_len;
+				claim_len = total_len;
+			}
+		}
 
-	add_to_head(history, h_item, line, len);
+		if (claim_len == total_len) {
+			add_to_head(history, h_item, line, len, padding);
+			ring_buf_put_finish(history->ring_buf, claim_len);
+			break;
+		}
+
+		ring_buf_put_finish(history->ring_buf, 0);
+		if (remove_from_tail(history) == false) {
+			__ASSERT_NO_MSG(ring_buf_is_empty(history->ring_buf));
+			/* if history is empty reset ring buffer. Even when
+			 * ring buffer is empty, it is possible that available
+			 * continues memory in worst case equals half of the
+			 * ring buffer capacity. By reseting ring buffer we
+			 * ensure that it is capable to provide continues memory
+			 * of ring buffer capacity length.
+			 */
+			ring_buf_reset(history->ring_buf);
+		}
+	} while (1);
 }
 
 void shell_history_init(struct shell_history *history)

tests/subsys/shell/shell_history/src/shell_history_test.c

@@ -14,8 +14,8 @@
 
 #include <shell/shell_history.h>
 
-#define MAX_BUF_SIZE 128
+#define HIST_BUF_SIZE 128
-SHELL_HISTORY_DEFINE(history, MAX_BUF_SIZE, 8);
+SHELL_HISTORY_DEFINE(history, HIST_BUF_SIZE);
 
 static void init_test_buf(u8_t *buf, size_t len, u8_t offset)
 {
@@ -27,12 +27,12 @@ static void init_test_buf(u8_t *buf, size_t len, u8_t offset)
 static void test_get(bool ok, bool up, u8_t *exp_buf, u16_t exp_len)
 {
 	bool res;
-	u8_t out_buf[MAX_BUF_SIZE];
+	u8_t out_buf[HIST_BUF_SIZE];
 	u16_t out_len;
 
 	out_len = sizeof(out_buf);
 
-	res = shell_history_get(&history_history, up, out_buf, &out_len);
+	res = shell_history_get(&history, up, out_buf, &out_len);
 
 	if (ok) {
 		zassert_true(res, "history should contain one entry.\n");
@@ -56,32 +56,34 @@ static void test_get(bool ok, bool up, u8_t *exp_buf, u16_t exp_len)
  */
 void test_history_add_get(void)
 {
-	u8_t exp_buf[MAX_BUF_SIZE];
+	u8_t exp_buf[HIST_BUF_SIZE];
 
 	init_test_buf(exp_buf, sizeof(exp_buf), 0);
 
-	shell_history_init(&history_history);
+	shell_history_init(&history);
 
 	test_get(false, true, NULL, 0);
 
-	shell_history_put(&history_history, exp_buf, 20);
+	shell_history_put(&history, exp_buf, 20);
 
 	test_get(true, true, exp_buf, 20);
+
+	shell_history_purge(&history);
 }
 
 /* Test verifies that after purging there is no line in the history. */
 void test_history_purge(void)
 {
-	u8_t exp_buf[MAX_BUF_SIZE];
+	u8_t exp_buf[HIST_BUF_SIZE];
 
 	init_test_buf(exp_buf, sizeof(exp_buf), 0);
 
-	shell_history_init(&history_history);
+	shell_history_init(&history);
 
-	shell_history_put(&history_history, exp_buf, 20);
+	shell_history_put(&history, exp_buf, 20);
-	shell_history_put(&history_history, exp_buf, 20);
+	shell_history_put(&history, exp_buf, 20);
 
-	shell_history_purge(&history_history);
+	shell_history_purge(&history);
 
 	test_get(false, true, NULL, 0);
 }
@@ -103,26 +105,26 @@ void test_history_purge(void)
  */
 void test_history_get_up_and_down(void)
 {
-	u8_t exp1_buf[MAX_BUF_SIZE];
+	u8_t exp1_buf[HIST_BUF_SIZE];
-	u8_t exp2_buf[MAX_BUF_SIZE];
+	u8_t exp2_buf[HIST_BUF_SIZE];
-	u8_t exp3_buf[MAX_BUF_SIZE];
+	u8_t exp3_buf[HIST_BUF_SIZE];
 
 	init_test_buf(exp1_buf, sizeof(exp1_buf), 0);
 	init_test_buf(exp2_buf, sizeof(exp2_buf), 10);
 	init_test_buf(exp3_buf, sizeof(exp3_buf), 20);
 
-	shell_history_init(&history_history);
+	shell_history_init(&history);
 
-	shell_history_put(&history_history, exp1_buf, 20);
+	shell_history_put(&history, exp1_buf, 20);
-	shell_history_put(&history_history, exp2_buf, 20);
+	shell_history_put(&history, exp2_buf, 15);
-	shell_history_put(&history_history, exp3_buf, 20);
+	shell_history_put(&history, exp3_buf, 20);
 
 	test_get(true, true, exp3_buf, 20); /* up - 3*/
-	test_get(true, true, exp2_buf, 20); /* up - 2*/
+	test_get(true, true, exp2_buf, 15); /* up - 2*/
 	test_get(true, true, exp1_buf, 20); /* up - 1*/
-	test_get(true, false, exp2_buf, 20); /* down - 2 */
+	test_get(true, false, exp2_buf, 15); /* down - 2 */
 	test_get(true, true, exp1_buf, 20);  /* up - 1*/
-	test_get(true, false, exp2_buf, 20); /* down - 2 */
+	test_get(true, false, exp2_buf, 15); /* down - 2 */
 	test_get(true, false, exp3_buf, 20); /* down - 3 */
 	test_get(false, false, NULL, 0); /* down - nothing */
 }