feat: add graphical renderer foundation with U8g2 adapter#408
Conversation
|
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 |
Pull Request is too largeallow added line is 600, but got 1230 lines. Added files
Ignored files
reported by gigi |
Pull Request is too largeallow added line is 600, but got 1230 lines. Added files
Ignored files
reported by gigi |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/overview/rendering/graphical-display.rst`:
- Around line 33-40: The example currently shows ITEM_FONT and
renderer.setItemFont(mainItems[1], ...) but standard ItemLabel and ItemBool do
not implement GraphicalMenuItem so those calls silently no-op; update the docs:
either remove the ITEM_FONT and renderer.setItemFont lines from the snippet or
add a clarifying sentence after the snippet stating that ItemLabel and ItemBool
(and other non-GraphicalMenuItem types) do not support font customization and
that only items implementing GraphicalMenuItem will respond to
renderer.setItemFont and ITEM_FONT settings, referencing GraphicalMenuItem,
renderer.setItemFont, ITEM_FONT, ItemBool, ItemLabel, and mainItems so readers
can locate the relevant code.
In `@examples/SSD1306_I2C/SSD1306_I2C.ino`:
- Around line 342-377: The dynamic label buffers created by cloneText in
addDynamicItem and insertDynamicItem are never freed when items are removed by
removeDynamicAtHead, removeDynamicLast, or clearDynamicItems, causing memory
leaks; fix by tracking each cloned pointer (e.g., store returned char* in a
parallel container or vector alongside the items) and delete[] the pointer
whenever you remove an item (in removeItemAt/removeLastItem wrappers or inside
removeDynamicAtHead/removeDynamicLast), and iterate the tracking container to
delete[] all pointers in clearDynamicItems (and on teardown), or alternatively
replace cloneText with a fixed/static string pool to avoid allocations. Ensure
references to ITEM_BASIC/dynamicScreen entries remain consistent with the
tracking container so you free the correct char* for each removed item.
In `@examples/ST7920_SPI/ST7920_SPI.ino`:
- Line 17: The constructor call U8G2_ST7920_128X64_F_HW_SPI u8g2(U8G2_R0, 10,
U8X8_PIN_NONE); uses GPIO 10 which is reserved on ESP-WROOM-32 modules; change
the CS pin to a safe GPIO (e.g., 5 or 15 with a boot-mode note) in that
U8G2_ST7920_128X64_F_HW_SPI u8g2(...) call and update any wiring comments
accordingly, and if this example is intended for non-WROOM ESP32 variants
(S2/S3/C3) add a header comment clarifying the target board and why GPIO 10 is
used.
In `@README.md`:
- Line 26: The README contains a Sphinx-style relative link "[display
interfaces](reference/api/display/index)" that won't resolve on GitHub; update
the link in README.md (the line containing that link text) to an absolute HTTPS
URL pointing to the hosted documentation page (e.g. the canonical docs site URL
+ /reference/api/display/index) so clicks from GitHub go to a real page, and
check/replace any other similar relative intra-doc links in README.md with their
corresponding absolute hosted-docs URLs.
In `@src/display/GraphicalDisplayInterface.h`:
- Around line 11-22: The interface currently caps coordinates/sizes at 8 bits;
change the public geometry types in GraphicalDisplayInterface to uint16_t to
avoid 8-bit truncation: update the return types of getDisplayWidth,
getDisplayHeight, getTextWidth, getFontWidth, getFontHeight and the parameter
types x/y/w/h on drawBox, drawFrame, drawXbm from uint8_t to uint16_t, and then
update implementations (e.g., U8g2DisplayAdapter) to use u8g2_uint_t or uint16_t
consistently; if you intentionally want to stay 8-bit instead, add a clear
comment on GraphicalDisplayInterface documenting the 255px ceiling instead of
changing signatures.
In `@src/display/U8g2DisplayAdapter.h`:
- Around line 61-69: These accessors (getDisplayWidth, getDisplayHeight,
getFontWidth, getFontHeight, getTextWidth) currently return uint8_t causing
silent truncation when U8G2_16BIT is enabled; change their return types to
uint16_t to match U8G2::getDisplayWidth/Height/getUTF8Width (u8g2_uint_t) and
the updated GraphicalDisplayInterface.h, and update the method
declarations/definitions for these functions in U8g2DisplayAdapter to return
uint16_t (no other logic changes needed; cast only if the compiler requires an
explicit conversion).
- Around line 39-43: The byte-handling in U8g2DisplayAdapter::draw(uint8_t)
currently wraps a single byte as UTF-8 which only works for 0x00–0x7F; bytes >=
0x80 become invalid UTF-8 continuation bytes, render as replacement glyphs and
return zero width. Fix by either (A) adding a clear contract to
DisplayInterface::draw(uint8_t) documentation that implementations expect 7-bit
ASCII only (so GraphicalDisplayRenderer/U8g2DisplayAdapter are validated), or
(B) making U8g2DisplayAdapter::draw(uint8_t) robust: detect bytes >= 0x80 and
route them to a fallback path (e.g., render a single-byte placeholder glyph or
advance cursor by a fixed glyph width) or expose/forward them to a glyph-indexed
method (e.g., a new drawGlyph(uint8_t) or existing glyph path) so cursorX
remains consistent; update U8g2DisplayAdapter, DisplayInterface::draw(uint8_t),
and GraphicalDisplayRenderer/CharacterDisplayRenderer references accordingly.
In `@src/renderer/GraphicalDisplayRenderer.cpp`:
- Line 9: listGlyph is 7 bytes but drawListIndicator() calls drawXbm with
listGlyphWidth=7 and listGlyphHeight=8 causing an out-of-bounds read; fix by
making the bitmap size consistent: either change the height passed to drawXbm in
drawListIndicator() from 8 to 7 to match listGlyph, or append a zero byte to the
listGlyph array so it contains 8 bytes; apply the same correction for the other
occurrences of listGlyph/drawListIndicator usage in this file (the other
drawListIndicator/drawXbm calls mentioned).
- Around line 186-197: drawItem() relies on the member activeItem to decide
whether to render toggle indicators, but MenuScreen::draw() never calls
setActiveItem() while iterating visible items so non-focused rows render without
their checkboxes; fix by ensuring the currently-rendered item is provided to
drawItem(): either call setActiveItem(item) immediately before each
item->draw(renderer) inside MenuScreen::draw(), or change drawItem()'s signature
to accept a const MenuItem* (or const GraphicalMenuItem*) parameter and use that
instead of the activeItem member—update all drawItem() callers and any
references to activeItem/graphicalToggleState()/hasGraphicalToggle() accordingly
(e.g., GraphicalMenuItem::graphicalToggleState()).
In `@src/renderer/GraphicalDisplayRenderer.h`:
- Around line 30-31: maxRowHeight and maxFontWidth only increase because
captureCurrentFontMetrics() uses "if (h > maxRowHeight) ..." and never reduces
them; reset these maxima when changing fonts so they can shrink: in
setDefaultFont() (and/or at the start of each frame) set maxRowHeight = 0 and
maxFontWidth = 0, then let applyItemFont()/captureCurrentFontMetrics()
re-measure visible items and rebuild the maxima across the currently active
fonts so row height and font width reflect the current frame's fonts rather than
only growing.
In `@src/renderer/GraphicalIndicatorRenderer.h`:
- Line 10: GraphicalIndicatorRenderer::extensionId() currently returns a
hard-coded literal (3) which risks runtime collisions; add a centralized
ExtensionId definition (e.g., an enum class or constexpr values in a shared
header) and replace the literal in GraphicalIndicatorRenderer::extensionId()
with the corresponding shared symbol (e.g., ExtensionId::GRAPHICAL_INDICATOR),
then update other classes that use matching literals (FrameLifecycleRenderer,
GraphicalRendererContext) to reference the same shared ExtensionId symbol to
ensure compile-time consistency and prevent silent ID collisions.
In `@src/renderer/GraphicalMenuItem.h`:
- Around line 28-30: The two new GraphicalMenuItem virtuals
(hasGraphicalListIndicator() and useTightGraphicalSelectionBox()) are never
used; either wire them into the renderer or remove them. To wire them, update
GraphicalDisplayRenderer's item rendering path (e.g., the method that draws list
entries / selection boxes) to call
GraphicalMenuItem::hasGraphicalListIndicator() and render the list indicator
when true, and call GraphicalMenuItem::useTightGraphicalSelectionBox() to choose
a tighter selection box geometry when true (adjust hit/test/outline drawing
accordingly). Alternatively, if you don't need the behavior yet, remove the two
virtual methods from GraphicalMenuItem to avoid dead API surface.
🪄 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: 2f3c4ed9-429b-4519-a4f6-6e5cf62a5f15
📒 Files selected for processing (14)
.github/workflows/compile-arduino.ymlREADME.mddocs/source/overview/rendering/graphical-display.rstdocs/source/overview/rendering/index.rstexamples/SSD1306_I2C/SSD1306_I2C.inoexamples/ST7920_SPI/ST7920_SPI.inoplatformio.inisrc/display/GraphicalDisplayInterface.hsrc/display/U8g2DisplayAdapter.hsrc/renderer/GraphicalDisplayRenderer.cppsrc/renderer/GraphicalDisplayRenderer.hsrc/renderer/GraphicalIndicatorRenderer.hsrc/renderer/GraphicalItemFont.hsrc/renderer/GraphicalMenuItem.h
| ITEM_BASIC("Blink random")); | ||
| // clang-format on | ||
|
|
||
| U8G2_ST7920_128X64_F_HW_SPI u8g2(U8G2_R0, 10, U8X8_PIN_NONE); |
There was a problem hiding this comment.
GPIO 10 is reserved for the ESP32's integrated SPI flash — this sketch will crash the board on typical ESP-WROOM-32 hardware.
The CI matrix pins this example to ESP32 only (via ESP32_ONLY_SKETCH_PATHS in .github/workflows/compile-arduino.yml), but on ESP-WROOM-32 modules GPIO 6–11 are connected to the on-chip SPI flash and cannot be repurposed — driving GPIO 10 as CS for the ST7920 will fight the flash IO line and typically hangs boot or causes a reset loop. Compilation succeeds, so CI won't catch this, but the first person to upload the sketch will hit it.
Pick a CS pin in the "safe to use" range — e.g., GPIO 5 (the default VSPI CS on ESP32 DevKit), or GPIO 15 with a short note about boot-mode. VSPI's fixed SCK/MOSI are GPIO 18 / GPIO 23 by default, which the ST7920 wiring should match.
🐛 Proposed fix
-U8G2_ST7920_128X64_F_HW_SPI u8g2(U8G2_R0, 10, U8X8_PIN_NONE);
+// ESP32 VSPI: SCK=18, MOSI=23 (default). CS is user-selectable — GPIO 5 is a safe choice.
+U8G2_ST7920_128X64_F_HW_SPI u8g2(U8G2_R0, /* CS=*/ 5, /* reset=*/ U8X8_PIN_NONE);Confirm the target board for this example — if it's intentionally meant for a non-WROOM ESP32 variant (S2/S3/C3) where the flash pin map differs, please add a header comment clarifying that; otherwise, GPIO 10 should be replaced.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| U8G2_ST7920_128X64_F_HW_SPI u8g2(U8G2_R0, 10, U8X8_PIN_NONE); | |
| // ESP32 VSPI: SCK=18, MOSI=23 (default). CS is user-selectable — GPIO 5 is a safe choice. | |
| U8G2_ST7920_128X64_F_HW_SPI u8g2(U8G2_R0, /* CS=*/ 5, /* reset=*/ U8X8_PIN_NONE); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/ST7920_SPI/ST7920_SPI.ino` at line 17, The constructor call
U8G2_ST7920_128X64_F_HW_SPI u8g2(U8G2_R0, 10, U8X8_PIN_NONE); uses GPIO 10 which
is reserved on ESP-WROOM-32 modules; change the CS pin to a safe GPIO (e.g., 5
or 15 with a boot-mode note) in that U8G2_ST7920_128X64_F_HW_SPI u8g2(...) call
and update any wiring comments accordingly, and if this example is intended for
non-WROOM ESP32 variants (S2/S3/C3) add a header comment clarifying the target
board and why GPIO 10 is used.
| LcdMenu is an open-source Arduino library for creating menu systems. It is designed to be easy to use and flexible enough to support a wide range of use cases. | ||
|
|
||
| With LcdMenu, you can create a menu system for your Arduino project with minimal effort. The library provides a simple API for creating menus and handling user input. There are also a number of built-in [display interfaces](reference/api/display/index) to choose from, including LCD displays and OLED displays _(coming soon)_. | ||
| With LcdMenu, you can create a menu system for your Arduino project with minimal effort. The library provides a simple API for creating menus and handling user input. There are also a number of built-in [display interfaces](reference/api/display/index) to choose from, including classic character LCDs and graphical displays powered by U8g2 (for example SSD1306 and ST7920 modules). |
There was a problem hiding this comment.
Relative docs link won't resolve on GitHub's README view.
[display interfaces](reference/api/display/index) is a Sphinx-style intra-doc link — GitHub will render it as https://github.com/forntoh/LcdMenu/blob/.../reference/api/display/index, which 404s. Since this README is the primary entry point on GitHub, consider an absolute URL to the hosted docs so readers clicking from GitHub land on a real page.
🔗 Proposed fix
-With LcdMenu, you can create a menu system for your Arduino project with minimal effort. The library provides a simple API for creating menus and handling user input. There are also a number of built-in [display interfaces](reference/api/display/index) to choose from, including classic character LCDs and graphical displays powered by U8g2 (for example SSD1306 and ST7920 modules).
+With LcdMenu, you can create a menu system for your Arduino project with minimal effort. The library provides a simple API for creating menus and handling user input. There are also a number of built-in [display interfaces](https://lcdmenu.forntoh.dev/reference/api/display/index.html) to choose from, including classic character LCDs and graphical displays powered by U8g2 (for example SSD1306 and ST7920 modules).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 26, The README contains a Sphinx-style relative link
"[display interfaces](reference/api/display/index)" that won't resolve on
GitHub; update the link in README.md (the line containing that link text) to an
absolute HTTPS URL pointing to the hosted documentation page (e.g. the canonical
docs site URL + /reference/api/display/index) so clicks from GitHub go to a real
page, and check/replace any other similar relative intra-doc links in README.md
with their corresponding absolute hosted-docs URLs.
| virtual void setFont(const uint8_t* font) = 0; | ||
| virtual uint8_t getDisplayWidth() const = 0; | ||
| virtual uint8_t getDisplayHeight() const = 0; | ||
| virtual uint8_t getFontWidth() const = 0; | ||
| virtual uint8_t getFontHeight() const = 0; | ||
| virtual uint8_t getTextWidth(const char* text) = 0; | ||
| virtual void setDrawColor(uint8_t color) = 0; | ||
| virtual void clearBuffer() = 0; | ||
| virtual void sendBuffer() = 0; | ||
| virtual void drawBox(uint8_t x, uint8_t y, uint8_t w, uint8_t h) = 0; | ||
| virtual void drawFrame(uint8_t x, uint8_t y, uint8_t w, uint8_t h) = 0; | ||
| virtual void drawXbm(uint8_t x, uint8_t y, uint8_t w, uint8_t h, const uint8_t* bitmap) = 0; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider uint16_t for display/coordinate dimensions to avoid truncation on larger panels.
All geometry methods (getDisplayWidth/Height, getTextWidth, and every x/y/w/h parameter on drawBox/drawFrame/drawXbm) are typed as uint8_t, capping the interface at 255 px per axis. That's fine for SSD1306/ST7920 (128×64), but U8g2's native dimension type is u8g2_uint_t, which widens to uint16_t when U8G2_16BIT is defined — common for larger panels (e.g., 240×320 ILI-series). Since this header is the public contract for any pixel-addressable display and the docs already call out a future "TFT renderer", widening to uint16_t now would prevent a breaking API change later and silent 8-bit wraparound in U8g2DisplayAdapter.
♻️ Proposed widening
- virtual uint8_t getDisplayWidth() const = 0;
- virtual uint8_t getDisplayHeight() const = 0;
- virtual uint8_t getFontWidth() const = 0;
- virtual uint8_t getFontHeight() const = 0;
- virtual uint8_t getTextWidth(const char* text) = 0;
+ virtual uint16_t getDisplayWidth() const = 0;
+ virtual uint16_t getDisplayHeight() const = 0;
+ virtual uint8_t getFontWidth() const = 0;
+ virtual uint8_t getFontHeight() const = 0;
+ virtual uint16_t getTextWidth(const char* text) = 0;
virtual void setDrawColor(uint8_t color) = 0;
virtual void clearBuffer() = 0;
virtual void sendBuffer() = 0;
- virtual void drawBox(uint8_t x, uint8_t y, uint8_t w, uint8_t h) = 0;
- virtual void drawFrame(uint8_t x, uint8_t y, uint8_t w, uint8_t h) = 0;
- virtual void drawXbm(uint8_t x, uint8_t y, uint8_t w, uint8_t h, const uint8_t* bitmap) = 0;
+ virtual void drawBox(uint16_t x, uint16_t y, uint16_t w, uint16_t h) = 0;
+ virtual void drawFrame(uint16_t x, uint16_t y, uint16_t w, uint16_t h) = 0;
+ virtual void drawXbm(uint16_t x, uint16_t y, uint16_t w, uint16_t h, const uint8_t* bitmap) = 0;Please confirm whether supporting panels > 255 px per axis is in scope for this interface, or whether the intent is to keep it small-display-only (in which case a comment in the header documenting the 8-bit ceiling would help downstream renderer authors).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/display/GraphicalDisplayInterface.h` around lines 11 - 22, The interface
currently caps coordinates/sizes at 8 bits; change the public geometry types in
GraphicalDisplayInterface to uint16_t to avoid 8-bit truncation: update the
return types of getDisplayWidth, getDisplayHeight, getTextWidth, getFontWidth,
getFontHeight and the parameter types x/y/w/h on drawBox, drawFrame, drawXbm
from uint8_t to uint16_t, and then update implementations (e.g.,
U8g2DisplayAdapter) to use u8g2_uint_t or uint16_t consistently; if you
intentionally want to stay 8-bit instead, add a clear comment on
GraphicalDisplayInterface documenting the 255px ceiling instead of changing
signatures.
| #include <string.h> | ||
|
|
||
| namespace { | ||
| const uint8_t listGlyph[] = {0x08, 0x1C, 0x3E, 0x00, 0x3E, 0x1C, 0x08}; |
There was a problem hiding this comment.
Out-of-bounds read: listGlyph has 7 bytes but is drawn as a 7×8 XBM.
listGlyph is declared as {0x08, 0x1C, 0x3E, 0x00, 0x3E, 0x1C, 0x08} (7 bytes), but drawListIndicator() passes listGlyphWidth=7, listGlyphHeight=8 to drawXbm, which for an XBM with width 7 requires ceil(7/8) * 8 = 8 bytes. U8g2's drawXBM will read past the end of listGlyph, producing undefined pixel data in the last row and an out-of-bounds read. The glyph visually spans 7 rows, so the height constant is what's off.
🐛 Proposed fix (set height to 7 to match the actual glyph data)
- static const uint8_t listGlyphWidth = 7;
- static const uint8_t listGlyphHeight = 8;
+ static const uint8_t listGlyphWidth = 7;
+ static const uint8_t listGlyphHeight = 7;Alternatively, keep listGlyphHeight = 8 and add an 8th byte (e.g. 0x00) to listGlyph so the array length matches the declared height.
Also applies to: 239-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/GraphicalDisplayRenderer.cpp` at line 9, listGlyph is 7 bytes
but drawListIndicator() calls drawXbm with listGlyphWidth=7 and
listGlyphHeight=8 causing an out-of-bounds read; fix by making the bitmap size
consistent: either change the height passed to drawXbm in drawListIndicator()
from 8 to 7 to match listGlyph, or append a zero byte to the listGlyph array so
it contains 8 bytes; apply the same correction for the other occurrences of
listGlyph/drawListIndicator usage in this file (the other
drawListIndicator/drawXbm calls mentioned).
| } else { | ||
| const GraphicalMenuItem* graphicalItem = asGraphical(activeItem); | ||
| if (graphicalItem != NULL && graphicalItem->hasGraphicalToggle()) { | ||
| uint8_t box = toggleIndicatorWidth(); | ||
| uint8_t xBox = contentRight > rightPadding + box ? contentRight - rightPadding - box : x; | ||
| uint8_t yBox = yTop + (h > box ? (h - box) / 2 : 0); | ||
| gDisplay->drawFrame(xBox, yBox, box, box); | ||
| if (graphicalItem->graphicalToggleState() && box > 4) { | ||
| gDisplay->drawBox(xBox + 2, yBox + 2, box - 4, box - 4); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites for setActiveItem and see whether they're per-row or per-focus.
rg -nP '\bsetActiveItem\s*\(' -C5
# Locate MenuScreen's render loop to inspect the sequencing.
rg -nP '\bdrawItem\s*\(' --type=cpp --type=h -C5Repository: forntoh/LcdMenu
Length of output: 14863
🏁 Script executed:
# Find all call sites of setActiveItem to see where it's invoked
rg -nP '->setActiveItem|\.setActiveItem' --type=cpp --type=h -B2 -A2
# Look for MenuScreen or rendering loop that orchestrates drawItem calls
rg -nP 'class.*MenuScreen|void.*render|void.*draw.*(' --type=h -l | head -20Repository: forntoh/LcdMenu
Length of output: 536
🏁 Script executed:
# Find all call sites of setActiveItem - simpler pattern
rg 'setActiveItem' --type=cpp --type=h -B3 -A3
# Find MenuScreen or rendering orchestration classes
rg 'class MenuScreen|void render\(' --type=hRepository: forntoh/LcdMenu
Length of output: 1760
🏁 Script executed:
# Look for MenuScreen implementation and rendering logic
fd -e cpp -e h | xargs grep -l 'MenuScreen' | head -10
# Search for actual invocations of setActiveItem (not the declaration)
rg 'setActiveItem\s*\(' --type=cpp -B5 -A5
# Check MenuScreen.h for the render structure
head -100 src/MenuScreen.hRepository: forntoh/LcdMenu
Length of output: 5144
🏁 Script executed:
# Find MenuScreen.cpp and look at the render logic
cat src/MenuScreen.cpp | head -300
# Also search for where setActiveItem is called (invoke sites, not definitions)
rg 'setActiveItem' src/MenuScreen.cpp --type=cpp -B5 -A5Repository: forntoh/LcdMenu
Length of output: 5957
🏁 Script executed:
# Search more broadly for where setActiveItem is called (invocations)
rg 'setActiveItem\(' --type=cpp --type=h
# Let's check if there's any GraphicalRenderer specific rendering code
rg 'GraphicalRenderer' src/MenuScreen.cpp --type=cpp
# Check GraphicalDisplayRenderer to understand the draw flow better
head -200 src/renderer/GraphicalDisplayRenderer.cpp | tail -100Repository: forntoh/LcdMenu
Length of output: 4216
🏁 Script executed:
# Exhaustive search for where setActiveItem is invoked (not defined)
rg -w 'setActiveItem' --type=cpp --type=h | grep -v 'virtual\|override\|void.*setActiveItem' | head -30
# Check GraphicalMenuItem to understand the toggle interface
fd -name '*GraphicalMenuItem*' --type f
# Check if activeItem is initialized anywhere
rg 'activeItem\s*=' --type=cpp --type=h -B2 -A2Repository: forntoh/LcdMenu
Length of output: 977
🏁 Script executed:
# Find GraphicalMenuItem definition
find . -name '*.h' -o -name '*.cpp' | xargs grep -l 'class GraphicalMenuItem'
# Look at the full GraphicalMenuItem definition
rg 'class GraphicalMenuItem' -A 30 --type=cpp --type=h
# Search for any place where -> or . calls setActiveItem
rg 'context.*setActiveItem|renderer.*setActiveItem|this.*setActiveItem' --type=cpp --type=hRepository: forntoh/LcdMenu
Length of output: 1688
Pass the current item to drawItem() so toggle indicators render correctly for all rows, not just the focused item.
drawItem() uses the activeItem member to query hasGraphicalToggle() and graphicalToggleState(), but activeItem is never set during the render loop. MenuScreen::draw() iterates through all visible items and calls each item->draw(renderer), which invokes drawItem(), but setActiveItem() is never called. This means activeItem remains uninitialized when rendering non-focused toggle items, causing them to skip the checkbox draw path (lines 186-197) and render with an empty value area instead.
Either call setActiveItem(item) immediately before each item->draw(renderer) in MenuScreen::draw(), or modify the signature to pass the item as a parameter to drawItem().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/GraphicalDisplayRenderer.cpp` around lines 186 - 197, drawItem()
relies on the member activeItem to decide whether to render toggle indicators,
but MenuScreen::draw() never calls setActiveItem() while iterating visible items
so non-focused rows render without their checkboxes; fix by ensuring the
currently-rendered item is provided to drawItem(): either call
setActiveItem(item) immediately before each item->draw(renderer) inside
MenuScreen::draw(), or change drawItem()'s signature to accept a const MenuItem*
(or const GraphicalMenuItem*) parameter and use that instead of the activeItem
member—update all drawItem() callers and any references to
activeItem/graphicalToggleState()/hasGraphicalToggle() accordingly (e.g.,
GraphicalMenuItem::graphicalToggleState()).
| uint8_t maxRowHeight = 8; | ||
| uint8_t maxFontWidth = 1; |
There was a problem hiding this comment.
maxRowHeight / maxFontWidth only grow, never shrink.
captureCurrentFontMetrics() updates these with if (h > maxRowHeight) maxRowHeight = h; / if (w > maxFontWidth) maxFontWidth = w;. Once a tall font is ever applied (e.g. a per-item font on a rarely-visited screen, or a larger defaultFont set temporarily), the row height stays inflated for the rest of the session, and setDefaultFont(smallerFont) cannot reduce it. Consider resetting these in setDefaultFont / at the start of each frame and letting applyItemFont rebuild the max over the currently visible items so row density actually reflects the active font set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/GraphicalDisplayRenderer.h` around lines 30 - 31, maxRowHeight
and maxFontWidth only increase because captureCurrentFontMetrics() uses "if (h >
maxRowHeight) ..." and never reduces them; reset these maxima when changing
fonts so they can shrink: in setDefaultFont() (and/or at the start of each
frame) set maxRowHeight = 0 and maxFontWidth = 0, then let
applyItemFont()/captureCurrentFontMetrics() re-measure visible items and rebuild
the maxima across the currently active fonts so row height and font width
reflect the current frame's fonts rather than only growing.
| */ | ||
| class GraphicalIndicatorRenderer { | ||
| public: | ||
| static uint8_t extensionId() { return 3; } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Centralize extension IDs to prevent silent collisions.
Hard-coding return 3; here (and presumably matching literals in FrameLifecycleRenderer / GraphicalRendererContext) means extension ID conflicts become a runtime bug that manifests as queryExtension returning the wrong interface. A single enum class ExtensionId : uint8_t in a shared header (or at minimum a constexpr constant) makes collisions a compile-time error and documents the allocation.
♻️ Example — a shared registry header
// src/renderer/ExtensionId.h
`#pragma` once
`#include` <stdint.h>
namespace LcdMenuExt {
constexpr uint8_t FRAME_LIFECYCLE = 1;
constexpr uint8_t GRAPHICAL_CONTEXT = 2;
constexpr uint8_t GRAPHICAL_INDICATOR = 3;
}- static uint8_t extensionId() { return 3; }
+ static uint8_t extensionId() { return LcdMenuExt::GRAPHICAL_INDICATOR; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/GraphicalIndicatorRenderer.h` at line 10,
GraphicalIndicatorRenderer::extensionId() currently returns a hard-coded literal
(3) which risks runtime collisions; add a centralized ExtensionId definition
(e.g., an enum class or constexpr values in a shared header) and replace the
literal in GraphicalIndicatorRenderer::extensionId() with the corresponding
shared symbol (e.g., ExtensionId::GRAPHICAL_INDICATOR), then update other
classes that use matching literals (FrameLifecycleRenderer,
GraphicalRendererContext) to reference the same shared ExtensionId symbol to
ensure compile-time consistency and prevent silent ID collisions.
| virtual bool hasGraphicalListIndicator() const { return false; } | ||
|
|
||
| virtual bool useTightGraphicalSelectionBox() const { return false; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all call sites for the two new capability queries across the repo.
rg -nP '\b(hasGraphicalListIndicator|useTightGraphicalSelectionBox)\s*\(' -C2Repository: forntoh/LcdMenu
Length of output: 595
Wire the new capabilities into the renderer or drop them.
hasGraphicalListIndicator() and useTightGraphicalSelectionBox() are declared but never called anywhere in the codebase. Either integrate their usage into GraphicalDisplayRenderer to handle list indicators and tight selection boxes, or remove them until actually needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/GraphicalMenuItem.h` around lines 28 - 30, The two new
GraphicalMenuItem virtuals (hasGraphicalListIndicator() and
useTightGraphicalSelectionBox()) are never used; either wire them into the
renderer or remove them. To wire them, update GraphicalDisplayRenderer's item
rendering path (e.g., the method that draws list entries / selection boxes) to
call GraphicalMenuItem::hasGraphicalListIndicator() and render the list
indicator when true, and call GraphicalMenuItem::useTightGraphicalSelectionBox()
to choose a tighter selection box geometry when true (adjust hit/test/outline
drawing accordingly). Alternatively, if you don't need the behavior yet, remove
the two virtual methods from GraphicalMenuItem to avoid dead API surface.
1 participant
Summary
GraphicalDisplayInterface,U8g2DisplayAdapter, andGraphicalDisplayRendererGraphicalIndicatorRenderer,GraphicalValueSelectionRenderer,GraphicalItemFont) and extendGraphicalMenuItemMenuScreento dynamic row/column context (getMaxRows/getMaxCols) so graphical renderers can compute viewport/value layoutSSD1306_I2CandST7920_SPISummary by CodeRabbit
Release Notes
New Features
Documentation