Shell history use k_heap instead of ring_buffer #96862
Conversation
zephyrbot
added
area: Tests
Mattemagikern
force-pushed
the
shell_history
branch
2 times, most recently
from
66f46ef to
fd2394f
Compare
Mattemagikern
force-pushed
the
shell_history
branch
from
fd2394f to
eb93215
Compare
There was a problem hiding this comment.
Thank you very much for this PR — it’s a great idea to use a heap instead of a ring buffer!
Unfortunately, in its current form, this change introduces a regression in the command history functionality.
That part of the logic is quite delicate and has many corner cases. While your approach improves performance, it also causes some unexpected behavior.
Test case
Enter command:
clear→ press <Enter>
Enter command:
date get→ press <Enter>
(Now there are two commands in history: clear and date get)
Press the Up arrow repeatedly.
✅ Expected behavior (main branch)
Up → date get
Up → clear
Up → [empty prompt]
Up → date get
Up → clear
...
(Cycles correctly through history.)
❌ Observed behavior (this PR)
Up → date get
Up → clear
Up → [empty prompt]
Up → [empty prompt]
Up/Down → still empty prompt (no access to history)
Screencast.from.2025-10-07.10-18-19.webm |
Mattemagikern
force-pushed
the
shell_history
branch
from
eb93215 to
84c9f3a
Compare
|
Hi @jakub-uC, thanks for the review and the excellent bug-report! I'm fairly certain I've addressed the issue you outlined and added a test case for it. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the shell history implementation to use k_heap instead of ring_buffer for memory management. This change is a prerequisite for PR #96861 and modernizes the memory allocation strategy for shell history.
Key changes include:
- Replaced ring buffer-based memory management with heap allocation
- Simplified memory allocation logic by removing complex padding calculations
- Updated test infrastructure to properly reset history state between tests
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
subsys/shell/shell_history.c |
Core implementation change from ring buffer to k_heap with simplified allocation logic |
include/zephyr/shell/shell_history.h |
Updated API and macro definitions to use k_heap instead of ring_buffer |
subsys/shell/shell.c |
Removed history initialization call since k_heap doesn't require explicit init |
subsys/shell/Kconfig |
Removed RING_BUFFER dependency from shell history configuration |
tests/subsys/shell/shell_history/src/shell_history_test.c |
Updated tests to use reset fixture and added new test case |
tests/subsys/shell/shell_history/prj.conf |
Added extra stack size configuration for tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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 <[email protected]>
The shell history initialization function doesn't work in if the shell instance hasn't been created through the shell macro. This removes the function to avoid confusion. Signed-off-by: Måns Ansgariusson <[email protected]>
Mattemagikern
force-pushed
the
shell_history
branch
from
84c9f3a to
d47a700
Compare
|
3 participants
pre-requisite for: #96861