refactor: add RTTI-free renderer extension core#407
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
|
Memory usage change @ b86a846
Click for full report table
Click for full report CSV |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/LcdMenu.cpp (1)
100-105:⚠️ Potential issue | 🟡 MinorConsider
endFrame()afterpoll()when a polled item redrew.
MenuScreen::pollcallsitem->draw(renderer)for polling items, butLcdMenu::pollnever invokesendFrame(). For buffered renderers, polled updates will accumulate in the frame buffer and never be flushed until the next user event. If this is intentional (relying on the next event to flush), please add a comment; otherwise wireendFrame()here as well, guarded by whether any polled draw actually occurred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LcdMenu.cpp` around lines 100 - 105, MenuScreen::poll can redraw polled items but LcdMenu::poll never flushes those draws; make polled updates call endFrame() when any draw occurred. Update MenuScreen::poll to return a bool (or add a MenuScreen::didPollRedraw() accessor) indicating whether any item->draw(renderer) ran, then in LcdMenu::poll after screen->poll(...) call renderer.endFrame() (or LcdMenu::endFrame()) only when that flag is true; if you intentionally rely on next user event to flush, add a clear comment above LcdMenu::poll explaining that behavior instead.src/MenuScreen.cpp (1)
64-78:⚠️ Potential issue | 🟡 MinorDocument the begin/end-frame pairing contract.
MenuScreen::drawis invoked from many paths (setCursor,up,down,reset, RIGHT/LEFT view-shift insideprocess, empty-items fallback). Each call emitsbeginFrame(), whileLcdMenuonly emits a singleendFrame()per lifecycle event. As a result, within one user actionbeginFramecan fire multiple times beforeendFrame— e.g.,process(RIGHT)→draw()→ laterLcdMenu::process→endFrame().This is fine if implementers treat
beginFrameas "(re)initialize buffer" (which is typical), but the interface doesn't state it. Please document that implementers must tolerate repeatedbeginFramecalls without an interveningendFrame, or adjust the call sites to ensure strict pairing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MenuScreen.cpp` around lines 64 - 78, MenuScreen::draw currently calls FrameLifecycleRenderer::beginFrame() on every invocation which can occur multiple times per user action before a single endFrame() (seen in LcdMenu::process); to fix either (a) document the lifecycle contract on the FrameLifecycleRenderer interface/class (add a comment on beginFrame/endFrame stating beginFrame may be called repeatedly without an intervening endFrame and implementers must tolerate idempotent/reentrant beginFrame behavior) or (b) change call sites to enforce strict pairing by adding a frame-active check (add or use an isFrameActive()/hasBegunFrame() flag on FrameLifecycleRenderer and only call beginFrame() in MenuScreen::draw when a frame is not already active), referencing MenuScreen::draw, FrameLifecycleRenderer::beginFrame, FrameLifecycleRenderer::endFrame and LcdMenu::process so reviewers can find the affected code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/LcdMenu.cpp`:
- Around line 30-45: The current LcdMenu::process calls frame->endFrame()
whenever handled is true, which can call endFrame without a matching beginFrame
and double-flush when screen transitions occur; change the logic so endFrame is
only invoked when a frame was actually begun or drawn: either (A) track a "frame
dirty/active" flag set by FrameLifecycleRenderer::beginFrame (or by
MenuScreen::beginFrame) and clear it in endFrame, and in LcdMenu::process only
call frame->endFrame() if that flag is set, or (B) move the beginFrame/endFrame
pairing into MenuScreen::draw (have MenuScreen::draw call beginFrame then do
drawing then call endFrame) and remove per-event endFrame calls from LcdMenu
methods (setScreen, show, refresh, process) so only draw paths perform the
flush; update references to FrameLifecycleRenderer, LcdMenu::process,
MenuScreen::draw, LcdMenu::setScreen, show, and refresh accordingly.
In `@src/MenuItem.h`:
- Around line 87-95: Add a non-const overload of queryCapability on MenuItem to
mirror MenuRenderer::queryExtension so mutable capability interfaces are
reachable; declare virtual void* queryCapability(uint8_t capabilityId) { /*
default returns NULL */ } and implement it to forward to the const variant (or
vice‑versa) while preserving the existing default behavior (return NULL) so
callers with non-const MenuItem pointers can obtain mutable capability pointers.
---
Outside diff comments:
In `@src/LcdMenu.cpp`:
- Around line 100-105: MenuScreen::poll can redraw polled items but
LcdMenu::poll never flushes those draws; make polled updates call endFrame()
when any draw occurred. Update MenuScreen::poll to return a bool (or add a
MenuScreen::didPollRedraw() accessor) indicating whether any
item->draw(renderer) ran, then in LcdMenu::poll after screen->poll(...) call
renderer.endFrame() (or LcdMenu::endFrame()) only when that flag is true; if you
intentionally rely on next user event to flush, add a clear comment above
LcdMenu::poll explaining that behavior instead.
In `@src/MenuScreen.cpp`:
- Around line 64-78: MenuScreen::draw currently calls
FrameLifecycleRenderer::beginFrame() on every invocation which can occur
multiple times per user action before a single endFrame() (seen in
LcdMenu::process); to fix either (a) document the lifecycle contract on the
FrameLifecycleRenderer interface/class (add a comment on beginFrame/endFrame
stating beginFrame may be called repeatedly without an intervening endFrame and
implementers must tolerate idempotent/reentrant beginFrame behavior) or (b)
change call sites to enforce strict pairing by adding a frame-active check (add
or use an isFrameActive()/hasBegunFrame() flag on FrameLifecycleRenderer and
only call beginFrame() in MenuScreen::draw when a frame is not already active),
referencing MenuScreen::draw, FrameLifecycleRenderer::beginFrame,
FrameLifecycleRenderer::endFrame and LcdMenu::process so reviewers can find the
affected code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53660bf8-9bb7-420f-9f2d-9faf4e3ded31
📒 Files selected for processing (9)
docs/source/overview/rendering/index.rstsrc/LcdMenu.cppsrc/MenuItem.hsrc/MenuScreen.cppsrc/renderer/FrameLifecycleRenderer.hsrc/renderer/GraphicalMenuItem.hsrc/renderer/GraphicalRendererContext.hsrc/renderer/MenuRenderer.htest/LcdMenu.cpp
Contributor
|
Memory usage change @ 7ed981f
Click for full report table
Click for full report CSV |
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MenuRenderer::queryExtension()andMenuItem::queryCapability().FrameLifecycleRenderer,GraphicalRendererContext,GraphicalMenuItem) without adding pixel-specific methods to base classes.MenuScreen::draw()callsbeginFrame()andLcdMenunow callsendFrame()after handled draw-triggering actions (setScreen,show,refresh, handledprocess).refresh()and handledprocess()commands.Verification
bundle exec arduino_ci.rb --skip-examples-compilation), but local Ruby gem install requires privileged system paths on this machine.Summary by CodeRabbit
Release Notes
New Features
Documentation