shell: history rewrite to use k_heap instead of ring_buffer

The ring_buffer API was not ideal for this use case, as it is designed for
concurrent access, which is not needed here. The new implementation using
k_heap is more readable and easier to maintain.

There is a slight penalty in memory usage & effectiveness due to the
overhead of k_heap, but since this isn't a production-critical path, the
trade-off is acceptable.

Signed-off-by: Måns Ansgariusson <Mansgariusson@gmail.com>

Author: Mattemagikern

include/zephyr/shell/shell_history.h

@@ -19,7 +19,7 @@ extern "C" {
 
 
 struct shell_history {
-	struct ring_buf *ring_buf;
+	struct k_heap *heap;
 	sys_dlist_t list;
 	sys_dnode_t *current;
 };
@@ -28,18 +28,12 @@ struct shell_history {
  * @brief Create shell history instance.
  *
  * @param _name History instance name.
- * @param _size Memory dedicated for shell history.
+ * @param _size Memory size dedicated for shell history.
  */
-#define Z_SHELL_HISTORY_DEFINE(_name, _size)			  \
-	static uint8_t __noinit __aligned(sizeof(void *))	  \
-			_name##_ring_buf_data[_size];		  \
-	static struct ring_buf _name##_ring_buf =		  \
-		{						  \
-			.size = _size,				  \
-			.buffer = _name##_ring_buf_data		  \
-		};						  \
-	static struct shell_history _name = {			  \
-		.ring_buf = &_name##_ring_buf			  \
+#define Z_SHELL_HISTORY_DEFINE(_name, _size)			\
+	K_HEAP_DEFINE(_name##_heap, _size);			\
+	static struct shell_history _name = {			\
+		.heap = &_name##_heap,				\
 	}
 
 

subsys/shell/Kconfig

@@ -229,7 +229,6 @@ config SHELL_HELP_ON_WRONG_ARGUMENT_COUNT
 config SHELL_HISTORY
 	bool "History in shell"
 	default y if !SHELL_MINIMAL
-	select RING_BUFFER
 	help
 	  Enable commands history. History can be accessed using up and down
 	  arrows or Ctrl+n and Ctrl+p meta keys.

subsys/shell/shell_history.c

@@ -12,36 +12,11 @@
  * 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 |                |                          |
- * +-----------------+------------+----------------+--------------------------+
- *
- * As an optimization, the added padding is attributed to the preceding item
- * instead of the current item. This way the padding will be freed one item
- * sooner.
  */
+
 struct shell_history_item {
 	sys_dnode_t dnode;
 	uint16_t len;
-	uint16_t padding;
 	char data[];
 };
 
@@ -56,72 +31,40 @@ bool z_shell_history_get(struct shell_history *history, bool up,
 	struct shell_history_item *h_item; /* history item */
 	sys_dnode_t *l_item; /* list item */
 
-	if (sys_dlist_is_empty(&history->list)) {
-		*len = 0U;
-		return false;
-	}
-
-	if (!up) { /* button down */
-		if (history->current == NULL) {
-			/* Not in history mode. It is started by up button. */
-			*len = 0U;
-
-			return false;
-		}
-
-		l_item = sys_dlist_peek_prev_no_check(&history->list,
-						      history->current);
-	} else { /* button up */
+	if (up) { /* button up */
 		l_item = (history->current == NULL) ?
-		sys_dlist_peek_head_not_empty(&history->list) :
+		sys_dlist_peek_head(&history->list) :
 		sys_dlist_peek_next_no_check(&history->list, history->current);
+	} else { /* button down */
+		l_item = sys_dlist_peek_prev(&history->list, history->current);
+	}
 
+	if (l_item == NULL) {
+		/* Reached the end of history. */
+		*len = 0U;
+		return false;
 	}
 
 	history->current = l_item;
 	h_item = CONTAINER_OF(l_item, struct shell_history_item, dnode);
-
-	if (l_item) {
-		memcpy(dst, h_item->data, h_item->len);
-		*len = h_item->len;
-		dst[*len] = '\0';
-		return true;
-	}
-
-	*len = 0U;
-	return false;
-}
-
-static void add_to_head(struct shell_history *history,
-			struct shell_history_item *item,
-			uint8_t *src, size_t len, uint16_t padding)
-{
-	item->len = len;
-	item->padding = padding;
-	memcpy(item->data, src, len);
-	sys_dlist_prepend(&history->list, &item->dnode);
+	memcpy(dst, h_item->data, h_item->len);
+	*len = h_item->len;
+	dst[*len] = '\0';
+	return true;
 }
 
 /* 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;
-	uint32_t total_len;
+	sys_dnode_t *node;
 
 	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);
-
-	total_len = offsetof(struct shell_history_item, data) +
-			h_item->len + h_item->padding;
-	ring_buf_get(history->ring_buf, NULL, total_len);
-
+	node = sys_dlist_peek_tail(&history->list);
+	sys_dlist_remove(node);
+	k_heap_free(history->heap, CONTAINER_OF(node, struct shell_history_item, dnode));
 	return true;
 }
 
@@ -134,77 +77,39 @@ void z_shell_history_purge(struct shell_history *history)
 void z_shell_history_put(struct shell_history *history, uint8_t *line,
 			 size_t len)
 {
-	sys_dnode_t *l_item; /* list item */
-	struct shell_history_item *h_item, *h_prev_item;
+	sys_dnode_t *node;
+	struct shell_history_item *new, *h_prev_item;
 	uint32_t total_len = len + offsetof(struct shell_history_item, data);
-	uint32_t claim_len;
-	uint32_t claim2_len;
-	uint16_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;
-	}
 
 	z_shell_history_mode_exit(history);
-
 	if (len == 0) {
 		return;
 	}
 
-	l_item = sys_dlist_peek_head(&history->list);
-	h_prev_item = CONTAINER_OF(l_item, struct shell_history_item, dnode);
+	node = sys_dlist_peek_head(&history->list);
+	h_prev_item = CONTAINER_OF(node, struct shell_history_item, dnode);
 
-	if (l_item &&
+	if (node &&
 	   (h_prev_item->len == len) &&
 	   (memcmp(h_prev_item->data, line, len) == 0)) {
 		/* Same command as before, do not store */
 		return;
 	}
 
-	do {
-		if (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 resetting ring buffer we
-			 * ensure that it is capable to provide continues memory
-			 * of ring buffer capacity length.
-			 */
-			ring_buf_reset(history->ring_buf);
-		}
-
-		claim_len = ring_buf_put_claim(history->ring_buf,
-						(uint8_t **)&h_item, total_len);
-		/* 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,
-						   (uint8_t **)&h_item, total_len);
-			if (claim2_len == total_len) {
-				/*
-				 * We may get here only if a previous entry
-				 * exists. Stick the excess padding to it.
-				 */
-				h_prev_item->padding += claim_len;
-				total_len += claim_len;
-				claim_len = total_len;
-			}
-		}
-
-		if (claim_len == total_len) {
-			add_to_head(history, h_item, line, len, padding);
-			ring_buf_put_finish(history->ring_buf, claim_len);
+	 for (;;) {
+		new = k_heap_alloc(history->heap, total_len, K_NO_WAIT);
+		if (new) {
+			/* Got memory, add new item */
 			break;
+		} else if (!remove_from_tail(history)) {
+			/* Nothing to remove, cannot allocate memory. */
+			return;
 		}
+	}
 
-		ring_buf_put_finish(history->ring_buf, 0);
-		remove_from_tail(history);
-	} while (1);
+	new->len = len;
+	memcpy(new->data, line, len);
+	sys_dlist_prepend(&history->list, &new->dnode);
 }
 
 void z_shell_history_init(struct shell_history *history)

tests/subsys/shell/shell_history/prj.conf

@@ -7,3 +7,4 @@ CONFIG_SHELL_METAKEYS=n
 CONFIG_SHELL_HISTORY=y
 CONFIG_LOG=n
 CONFIG_ZTEST=y
+CONFIG_TEST_EXTRA_STACK_SIZE=1024

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

@@ -14,7 +14,7 @@
 
 #include <zephyr/shell/shell_history.h>
 
-#define HIST_BUF_SIZE 160
+#define HIST_BUF_SIZE 256
 Z_SHELL_HISTORY_DEFINE(history, HIST_BUF_SIZE);
 
 static void init_test_buf(uint8_t *buf, size_t len, uint8_t offset)