Fix heap fragmentation in Snake mode - #800
Conversation
JanPetterMG
changed the title
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueSummary by CodeRabbit
WalkthroughSnakeMode now stores positions as grid indices, uses enum-based stages, separates pathfinding from fallback movement, and updates lifecycle, rendering, cleanup, and target selection for the new representation. ChangesSnakeMode index-based state and movement
Merge Risk: 🟡 Moderate · up to Snake mode can start with an invisible target and may immediately treat the starting cell as collected, causing incorrect gameplay after restart. The initialization must be corrected before this PR is merge-ready. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The refactor directly addresses issue ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed compile/runtime correctness issues (invalid enum class initialization, missing standard includes for used algorithms, and inconsistent DEATH-stage initialization when target placement fails).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request refactors the Snake mode implementation to reduce dynamic allocations (a common source of heap fragmentation on embedded targets) by switching to index-based pixel representation and using fixed-size arrays for pathfinding state.
Changes:
- Replaces the numeric
stagestate machine with a strongly-typedStageenum. - Refactors snake and target representation from
{x,y}pixels to linear frame indices (size_t) and updates rendering calls accordingly. - Replaces the prior pathfinding structures (
std::map/std::queue) with stack-allocated fixed-size arrays to avoid heap churn.
File summaries
| File | Description |
|---|---|
| firmware/src/modes/SnakeMode.cpp | Updates Snake mode logic to use index-based pixels, fixed-size BFS pathfinding buffers, and typed stage transitions. |
| firmware/include/modes/SnakeMode.h | Adjusts SnakeMode’s internal state (stage enum, target/snake types) and updates private API accordingly. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: d2b1c89a-104e-4d1a-a389-91255cb43840
📒 Files selected for processing (2)
firmware/include/modes/SnakeMode.hfirmware/src/modes/SnakeMode.cpp
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: copilot-pull-request-reviewer
🔇 Additional comments (1)
firmware/include/modes/SnakeMode.h (1)
33-33: 🎯 Functional CorrectnessNo change is required. The pinned pioarduino platform uses
gnu++17by default, soStage stage{0U}is valid.
github-project-automation
Bot
moved this from In review/test
to In Progress
in Frekvens roadmap
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
🟡 Changes recommended
idle() bypasses setTarget() and setTarget() can keep an invalid target when enabling the clock, which can break gameplay correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
firmware/src/modes/SnakeMode.cpp:292
setTarget()can leavetargetunchanged if it currently points at an empty pixel, which means when the clock is enabled it may keep a target in the reserved top rows (< offset). This happens because the random selection only runs insidewhile (Display.getPixel(target) != 0U). Force at least one random draw from the valid range sotargetalways ends up >= offset when the clock is enabled.
const size_t offset{static_cast<size_t>(clock == nullptr ? 0U : 5U * GRID_COLUMNS)};
for (size_t idx{offset}; idx < GRID_COLUMNS * GRID_ROWS; ++idx)
{
if (Display.getPixel(idx) == 0U)
{
while (Display.getPixel(target) != 0U)
{
target =
static_cast<size_t>(random(static_cast<long>(offset), static_cast<long>(GRID_COLUMNS * GRID_ROWS)));
}
Display.setPixel(target, static_cast<uint8_t>(random(1L, static_cast<long>(0b1U << 8U))));
return;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| snake = {static_cast<size_t>((random(clock == nullptr ? 0 : 5, GRID_ROWS) * GRID_COLUMNS) + random(GRID_COLUMNS))}; | ||
| Display.setPixel(snake.front(), static_cast<uint8_t>(random(1, 0b1U << 8U))); | ||
| target = static_cast<size_t>(random(static_cast<long>(clock == nullptr ? 0U : 5U * GRID_COLUMNS), | ||
| static_cast<long>(GRID_COLUMNS * GRID_ROWS))); | ||
| stage = Stage::MOVE; |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 816ab063-76ef-43df-a558-d738f78a6782
📒 Files selected for processing (2)
firmware/include/modes/SnakeMode.hfirmware/src/modes/SnakeMode.cpp
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (64)
- GitHub Check: Minimal (esp32-c3-devkitm-1)
- GitHub Check: Minimal (lolin_d32)
- GitHub Check: Minimal (lolin_s3_mini)
- GitHub Check: Minimal (lolin_d32_pro)
- GitHub Check: Minimal (seeed_xiao_esp32c6)
- GitHub Check: Minimal (esp32-c5-devkitc-1)
- GitHub Check: Minimal (seeed_xiao_esp32s3)
- GitHub Check: Minimal (wemos_d1_mini32)
- GitHub Check: Minimal (esp32-s3-devkitc-1)
- GitHub Check: Minimal (seeed_xiao_esp32c3)
- GitHub Check: Default (seeed_xiao_esp32c6)
- GitHub Check: Minimal (esp32dev)
- GitHub Check: Minimal (esp32-c6-devkitm-1)
- GitHub Check: Default (esp32-c6-devkitm-1)
- GitHub Check: Default (lolin_d32)
- GitHub Check: Default (seeed_xiao_esp32s3)
- GitHub Check: Minimal (adafruit_qtpy_esp32s2)
- GitHub Check: Default (esp32-c5-devkitc-1)
- GitHub Check: Default (lolin_s3_mini)
- GitHub Check: Default (adafruit_qtpy_esp32s2)
- GitHub Check: Default (esp32dev)
- GitHub Check: Default (wemos_d1_mini32)
- GitHub Check: Minimal (adafruit_qtpy_esp32s3_n4r2)
- GitHub Check: Default (adafruit_qtpy_esp32s3_n4r2)
- GitHub Check: Minimal (adafruit_qtpy_esp32s3_nopsram)
- GitHub Check: Default (esp32-c3-devkitm-1)
- GitHub Check: Default (esp32-s3-devkitc-1)
- GitHub Check: Default (seeed_xiao_esp32c3)
- GitHub Check: Default (adafruit_qtpy_esp32s3_nopsram)
- GitHub Check: Extensive (adafruit_qtpy_esp32s3_nopsram)
- GitHub Check: Typical (adafruit_qtpy_esp32s3_nopsram)
- GitHub Check: Extensive (seeed_xiao_esp32s3)
- GitHub Check: Minimal (adafruit_qtpy_esp32s2)
- GitHub Check: Default (wemos_d1_mini32)
- GitHub Check: Minimal (lolin_d32)
- GitHub Check: Minimal (esp32-c6-devkitm-1)
- GitHub Check: Minimal (lolin_d32_pro)
- GitHub Check: Minimal (esp32-c3-devkitm-1)
- GitHub Check: Default (seeed_xiao_esp32s3)
- GitHub Check: Default (adafruit_qtpy_esp32s3_nopsram)
- GitHub Check: Minimal (adafruit_qtpy_esp32s3_n4r2)
- GitHub Check: Minimal (lolin_s3_mini)
- GitHub Check: Default (lolin_d32)
- GitHub Check: Default (adafruit_qtpy_esp32s2)
- GitHub Check: Minimal (seeed_xiao_esp32s3)
- GitHub Check: Default (esp32-s3-devkitc-1)
- GitHub Check: Minimal (wemos_d1_mini32)
- GitHub Check: Default (esp32dev)
- GitHub Check: Minimal (seeed_xiao_esp32c3)
- GitHub Check: Default (seeed_xiao_esp32c6)
- GitHub Check: Minimal (esp32-c5-devkitc-1)
- GitHub Check: Default (lolin_s3_mini)
- GitHub Check: Default (seeed_xiao_esp32c3)
- GitHub Check: Minimal (seeed_xiao_esp32c6)
- GitHub Check: Minimal (esp32dev)
- GitHub Check: Minimal (adafruit_qtpy_esp32s3_nopsram)
- GitHub Check: Extensive (seeed_xiao_esp32s3)
- GitHub Check: Default (esp32-c3-devkitm-1)
- GitHub Check: Default (esp32-c5-devkitc-1)
- GitHub Check: Extensive (esp32-s3-devkitc-1)
- GitHub Check: Typical (adafruit_qtpy_esp32s3_nopsram)
- GitHub Check: Tidy (IKEA_OBEGRANSAD, esp32-s3-devkitc-1)
- GitHub Check: Tidy (IKEA_OBEGRANSAD, esp32-c6-devkitm-1)
- GitHub Check: Tidy (IKEA_FREKVENS, esp32-c5-devkitc-1)
🔇 Additional comments (4)
firmware/include/modes/SnakeMode.h (1)
33-46: LGTM!firmware/src/modes/SnakeMode.cpp (3)
12-13: LGTM!
266-271: LGTM!Also applies to: 281-294
103-105: 🩺 Stability & AvailabilityThe configured 16×16 grid requires 2,304 bytes for these arrays with 4-byte
size_t, while theModetask stack is 8 kB. The inspected source does not establish that the remaining stack is insufficient for the function frame and existing task usage.
There was a problem hiding this comment.
🟡 Changes recommended
The PR’s “Fixes #793” linkage appears incorrect and the crash context indicates heap-allocation during snake growth still exists due to continued std::deque usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
firmware/src/modes/SnakeMode.cpp:55
- The linked issue reference
Fixes #793in the PR description appears unrelated (Issue #793 is about playlist auto stop), so it’s unclear whether this change set actually addresses that issue. Please confirm the correct issue/ID and update the reference (or PR description) accordingly so release notes and automation aren’t misleading.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| size_t target{0U}; | ||
|
|
||
| std::deque<Pixel> snake{}; | ||
| std::deque<size_t> snake{}; | ||
|
|
There was a problem hiding this comment.
@copilot It is true that deque still is used, but instead of push_back a object, it is now pushing back a size_t (essentially just an unsigned integer). I'd be surprised if that is worse than the previous implementation.
Co-authored-by: JanPetterMG <11933090+JanPetterMG@users.noreply.github.com>
Refactor the Snake mode implementation to address heap fragmentation issues by changing data structures and optimizing memory usage.
Impact
These changes enhance the efficiency of the Snake mode, reducing the likelihood of heap fragmentation and improving overall performance. The refactor maintains compatibility with existing functionality while optimizing memory usage for better stability and performance.
Fixes #793