diff --git a/include/zephyr/shell/shell_history.h b/include/zephyr/shell/shell_history.h index 1ddf69b79383a..e638d394684a6 100644 --- a/include/zephyr/shell/shell_history.h +++ b/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,28 +28,15 @@ 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, \ + .list = SYS_DLIST_STATIC_INIT(&_name.list), \ } - -/** - * @brief Initialize shell history module. - * - * @param history Shell history instance. - */ -void z_shell_history_init(struct shell_history *history); - /** * @brief Purge shell history. * diff --git a/subsys/shell/Kconfig b/subsys/shell/Kconfig index 54066fa2a32c1..c35b0193f6b4b 100644 --- a/subsys/shell/Kconfig +++ b/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. diff --git a/subsys/shell/shell.c b/subsys/shell/shell.c index 565a23c93efda..da203ddc62c2f 100644 --- a/subsys/shell/shell.c +++ b/subsys/shell/shell.c @@ -158,15 +158,6 @@ static void tab_item_print(const struct shell *sh, const char *option, z_shell_op_cursor_horiz_move(sh, diff); } -static void history_init(const struct shell *sh) -{ - if (!IS_ENABLED(CONFIG_SHELL_HISTORY)) { - return; - } - - z_shell_history_init(sh->history); -} - static void history_purge(const struct shell *sh) { if (!IS_ENABLED(CONFIG_SHELL_HISTORY)) { @@ -1233,8 +1224,6 @@ static int instance_init(const struct shell *sh, sh->ctx->selected_cmd = root_cmd_find(CONFIG_SHELL_CMD_ROOT); } - history_init(sh); - k_event_init(&sh->ctx->signal_event); k_sem_init(&sh->ctx->lock_sem, 1, 1); diff --git a/subsys/shell/shell_history.c b/subsys/shell/shell_history.c index 9e370f51a472b..dda7df062923c 100644 --- a/subsys/shell/shell_history.c +++ b/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,159 +31,84 @@ 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); } 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; + if (l_item == NULL) { + /* Reached the end of history. */ + *len = 0U; + return false; } - *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); + h_item = CONTAINER_OF(l_item, struct shell_history_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; } -void z_shell_history_purge(struct shell_history *history) -{ - while (remove_from_tail(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) +void z_shell_history_purge(struct shell_history *history) { - sys_dlist_init(&history->list); + while (remove_from_tail(history)) { + } history->current = NULL; } diff --git a/tests/subsys/shell/shell_history/prj.conf b/tests/subsys/shell/shell_history/prj.conf index dd6a985d9bcbb..397aff2f8fd22 100644 --- a/tests/subsys/shell/shell_history/prj.conf +++ b/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 diff --git a/tests/subsys/shell/shell_history/src/shell_history_test.c b/tests/subsys/shell/shell_history/src/shell_history_test.c index 5a9fff6b0b8a3..7d98e74bf1eff 100644 --- a/tests/subsys/shell/shell_history/src/shell_history_test.c +++ b/tests/subsys/shell/shell_history/src/shell_history_test.c @@ -14,7 +14,7 @@ #include -#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) @@ -24,6 +24,10 @@ static void init_test_buf(uint8_t *buf, size_t len, uint8_t offset) } } +static void reset_history(void *fixture) +{ + z_shell_history_purge(&history); +} /** * Function tests getting line from history and compares it against expected * result. @@ -54,7 +58,6 @@ static void test_get(bool ok, bool up, uint8_t *exp_buf, uint16_t exp_len) /* Test put line to history and get it. * * Test steps: - * - initialize history. * - put line to the history. * - read line and verify that it is the one that was put. */ @@ -64,15 +67,11 @@ ZTEST(shell_test, test_history_add_get) init_test_buf(exp_buf, sizeof(exp_buf), 0); - z_shell_history_init(&history); - test_get(false, true, NULL, 0); z_shell_history_put(&history, exp_buf, 20); test_get(true, true, exp_buf, 20); - - z_shell_history_purge(&history); } /* Test verifies that after purging there is no line in the history. */ @@ -82,20 +81,16 @@ ZTEST(shell_test, test_history_purge) init_test_buf(exp_buf, sizeof(exp_buf), 0); - z_shell_history_init(&history); - z_shell_history_put(&history, exp_buf, 20); z_shell_history_put(&history, exp_buf, 20); z_shell_history_purge(&history); - test_get(false, true, NULL, 0); } /* Test browsing history. * * Test steps: - * - initialize history. * - put lines 1,2,3 to history. * - get in up direction a line and verify that it's the last one added (3). * - get next line in up direction and verify that it's line 2. @@ -117,8 +112,6 @@ ZTEST(shell_test, test_history_get_up_and_down) init_test_buf(exp2_buf, sizeof(exp2_buf), 10); init_test_buf(exp3_buf, sizeof(exp3_buf), 20); - z_shell_history_init(&history); - z_shell_history_put(&history, exp1_buf, 20); z_shell_history_put(&history, exp2_buf, 15); z_shell_history_put(&history, exp3_buf, 20); @@ -131,8 +124,6 @@ ZTEST(shell_test, test_history_get_up_and_down) 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 */ - - z_shell_history_purge(&history); } /* Function for getting maximal buffer size that can be stored in the history */ @@ -143,8 +134,6 @@ static int get_max_buffer_len(void) int len = sizeof(buf); uint16_t out_len; - z_shell_history_init(&history); - do { z_shell_history_put(&history, buf, len); out_len = sizeof(out_buf); @@ -160,7 +149,6 @@ static int get_max_buffer_len(void) /* Test verifies that line that cannot fit into history buffer is not stored. * * Test steps: - * - initialize history. * - put buffer that is bigger than history overall capacity. * - verify that history is empty. * - put short line followed by line that is close to max. @@ -172,7 +160,6 @@ ZTEST(shell_test, test_too_long_line_not_stored) int max_len = get_max_buffer_len(); init_test_buf(exp1_buf, sizeof(exp1_buf), 0); - z_shell_history_init(&history); z_shell_history_put(&history, exp1_buf, max_len + 1); @@ -185,15 +172,12 @@ ZTEST(shell_test, test_too_long_line_not_stored) /* Test that long entry evicts older entry. */ test_get(true, true, exp1_buf, max_len - 10); test_get(false, true, NULL, 0); /* only one entry */ - - z_shell_history_purge(&history); } /* Test verifies that same line as the previous one is not stored in the * history. * * Test steps: - * - initialize history. * - put same line twice. * - verify that only one line is in the history. */ @@ -202,7 +186,6 @@ ZTEST(shell_test, test_no_duplicates_in_a_row) uint8_t exp1_buf[HIST_BUF_SIZE]; init_test_buf(exp1_buf, sizeof(exp1_buf), 0); - z_shell_history_init(&history); z_shell_history_put(&history, exp1_buf, 20); z_shell_history_put(&history, exp1_buf, 20); @@ -210,14 +193,11 @@ ZTEST(shell_test, test_no_duplicates_in_a_row) test_get(true, true, exp1_buf, 20); /* only one line stored. */ test_get(false, true, NULL, 0); - - z_shell_history_purge(&history); } /* Test storing long lines in the history. * * * Test steps: - * - initialize history. * - Put max length line 1 in history. * - Verify that it is present. * - Put max length line 2 in history. @@ -236,8 +216,6 @@ ZTEST(shell_test, test_storing_long_buffers) init_test_buf(exp2_buf, sizeof(exp2_buf), 10); init_test_buf(exp3_buf, sizeof(exp3_buf), 20); - z_shell_history_init(&history); - z_shell_history_put(&history, exp1_buf, max_len); test_get(true, true, exp1_buf, max_len); test_get(false, true, NULL, 0); /* only one entry */ @@ -249,8 +227,29 @@ ZTEST(shell_test, test_storing_long_buffers) z_shell_history_put(&history, exp3_buf, max_len); test_get(true, true, exp3_buf, max_len); test_get(false, true, NULL, 0); /* only one entry */ +} - z_shell_history_purge(&history); +ZTEST(shell_test, test_circle_through_history) +{ + uint8_t exp1_buf[HIST_BUF_SIZE]; + uint8_t exp2_buf[HIST_BUF_SIZE]; + uint8_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); + + z_shell_history_put(&history, exp1_buf, 20); + z_shell_history_put(&history, exp2_buf, 15); + z_shell_history_put(&history, exp3_buf, 20); + + test_get(true, true, exp3_buf, 20); /* up - 3*/ + test_get(true, true, exp2_buf, 15); /* up - 2*/ + test_get(true, true, exp1_buf, 20); /* up - 1*/ + test_get(false, true, NULL, 0); /* up - nothing */ + test_get(true, true, exp3_buf, 20); /* up - 3*/ + test_get(true, true, exp2_buf, 15); /* up - 2*/ + test_get(true, true, exp1_buf, 20); /* up - 1*/ } -ZTEST_SUITE(shell_test, NULL, NULL, NULL, NULL, NULL); +ZTEST_SUITE(shell_test, NULL, NULL, NULL, reset_history, NULL);