Extend scrolling text FX with custom fonts and international UTF-8 character support#5372
Extend scrolling text FX with custom fonts and international UTF-8 character support#5372
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a comprehensive font subsystem for 2D effects, adding a FontManager class with glyph handling and font loading capabilities. It includes UTF-8 utilities for text processing, updates the 2D scrolling text mode to use dynamic font metrics, and transforms the font data format to a new WBF (WLED Bitmap Font) packed structure. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@wled00/FX_2Dfcn.cpp`:
- Around line 969-970: Check that fileHdr.last >= fileHdr.first before computing
numGlyphs to avoid underflow; if not, handle the error (return/fail) instead of
proceeding. Change the numGlyphs calculation to use a wider type (e.g., size_t
or uint16_t) and allocate the width table safely (use std::vector<uint8_t> or
heap allocation) rather than a VLA: compute numGlyphs = (size_t)fileHdr.last -
(size_t)fileHdr.first + 1 after the validation, resize the vector to numGlyphs,
then call file.read(widthTable.data(), numGlyphs). Ensure early exit or error
logging if validation fails to prevent reading an incorrect/huge amount of data.
- Around line 1010-1015: The loop copies widths from widthTable into registry
but widthTable may be partially uninitialized if file.read(widthTable,
numGlyphs) returned fewer bytes; check the return value of file.read and ensure
it equals numGlyphs before using widthTable, or otherwise handle the short read
by adjusting numGlyphs/neededCount, zero-filling the remaining widthTable
entries, or aborting loading; also validate that each `code` from neededCodes is
within the bounds of widthTable before doing `registry[k].width =
widthTable[code]` (and continue to set `registry[k].height = fileHdr.height`),
so update the loading logic that populates widthTable and the loop that uses
`neededCount`, `neededCodes`, `registry`, and `widthTable` to guard against
truncated files.
- Around line 783-795: The cache-clear branch in FX_2Dfcn.cpp (when fontToUse !=
meta->cachedFontNum) reallocates SegmentFontMetadata via _segment->allocateData
but does not reset meta->glyphCount, leaving stale non-zero values that prevent
prepare() from detecting an empty cache; after reassigning meta = getMetadata()
and before setting meta->availableFonts/meta->cachedFontNum/meta->fontsScanned,
explicitly set meta->glyphCount = 0 so prepare()'s if (meta->glyphCount == 0)
correctly triggers a rebuild of the glyph cache.
In `@wled00/FX.cpp`:
- Around line 6455-6485: The centering math is wrong: SEGENV.aux0 was computed
as the left-origin centered offset but drawX is using cols - SEGENV.aux0 which
places text at the far right; change the drawX calculation in FX.cpp so it uses
SEGENV.aux0 as the left-origin base (e.g., baseX = SEGENV.aux0) and compute
drawX = baseX + currentXOffset (still using advance = (rotate == 1 || rotate ==
-1) ? letterHeight : glyphWidth), ensuring this branch (when totalTextWidth <=
cols / non-scrolling case) uses the left-origin base rather than cols - aux0 so
glyphs are centered correctly.
In `@wled00/FX.h`:
- Around line 1145-1149: Update the memory layout comment to match the constant
FONT_HEADER_SIZE by changing "[11-byte font header]" to "[12-byte font header]";
locate the comment near the cached fonts section in FX.h and ensure it exactly
reflects the value of FONT_HEADER_SIZE (defined earlier) so the documentation
and the constant remain consistent.
- Around line 1095-1100: The readUInt32LE function can invoke signed-integer
overflow because readByte() (uint8_t) promotes to signed int before left shifts;
cast each readByte(...) to uint32_t (e.g., (uint32_t)readByte(offset + N))
before shifting and ORing so all shifts operate on unsigned 32-bit values,
ensuring defined behavior in readUInt32LE.
- Around line 814-815: Update the two non-2D drawCharacter stub signatures to
use the same Unicode type as the 2D variant and FontManager: change the first
parameter from "unsigned char chr" to "uint32_t unicode" in both inline stubs
(the two empty drawCharacter overloads currently declared), so their signatures
match the 2D drawCharacter (uint32_t unicode) and FontManager::drawCharacter in
FX_2Dfcn.cpp; keep the rest of the parameter list the same and do not modify the
empty bodies.
In `@wled00/util.cpp`:
- Around line 165-196: Fix the typos in the UTF-8 comment block above
utf8_decode: change "onversion" to "conversion" and remove the stray trailing
'c' so "U+10FFFFc" becomes "U+10FFFF"; these edits live in the comment
immediately preceding the UTF8_LEN macro/utf8_decode function and do not require
code changes.
- Around line 184-188: Replace the expensive strlen(s) call and the lax
continuation handling by iterating up to n bytes from s and validating each
continuation byte prefix instead of scanning the whole string; specifically, in
the branch that currently does "if (strlen(s) < n) { *len = 1; return '?'; }"
check for early NUL by verifying s[i] != '\0' for each i in [0,n-1] and also
validate each continuation byte with (s[i] & 0xC0) == 0x80, and if any check
fails set *len = 1 and return '?' (preserving behavior of n, *len and the
returned default char).
🧹 Nitpick comments (4)
wled00/util.cpp (1)
174-196: Indentation uses 4 spaces; the rest of util.cpp uses 2 spaces.The function body uses 4-space base indentation, which is inconsistent with the file's established 2-space convention. As per coding guidelines,
wled00/**/*.cppshould use 2-space indentation.wled00/FX.cpp (1)
6394-6397: Avoid reloading/preparing fonts every frame.
loadFont()+prepare()are called on every frame (Line 6395-6396). If these hit filesystem or re-parse glyphs, this will tank FPS and I/O. Consider gating onSEGENV.call == 0or whenfontNum/useCustomFont/textchanges (the unusedinitflag could drive this).wled00/FX_2Dfcn.cpp (2)
1024-1040: Repeated O(code) offset recomputation for each glyph.For every needed glyph, lines 1031–1034 re-scan widthTable from index 0 to
code-1to compute the file offset. With up to 64 needed glyphs and codes up to ~255, this is ~16K iterations — tolerable for a one-time cache build but easy to optimize with a single prefix-sum pass.Sketch: pre-compute cumulative offsets
+ // Pre-compute cumulative byte offsets for each glyph + uint32_t cumOffset[numGlyphs + 1]; + cumOffset[0] = 0; + for (uint8_t j = 0; j < numGlyphs; j++) { + uint16_t b = widthTable[j] * fileHdr.height; + cumOffset[j + 1] = cumOffset[j] + (b + 7) / 8; + } + // Write bitmap data in registry order for (uint8_t k = 0; k < neededCount; k++) { uint8_t code = neededCodes[k]; uint16_t bits = widthTable[code] * fileHdr.height; uint16_t bytes = (bits + 7) / 8; // Calculate file offset - uint32_t offset = fileDataStart; - for (uint8_t j = 0; j < code; j++) { - uint16_t b = widthTable[j] * fileHdr.height; - offset += (b + 7) / 8; - } + uint32_t offset = fileDataStart + cumOffset[code]; // Read from file file.seek(offset);
647-701: Pointer returned fromgetGlyphBitmapis only used as an offset base — document this contract.The returned
const uint8_t*is never dereferenced directly;drawCharactercomputes(bitmap - _fontBase) + bytePosand reads through_reader. This works correctly but is a non-obvious contract — a future caller might dereference the pointer directly, which would fail for PROGMEM data. A brief comment would prevent misuse.
|
The Font Factory tool: Font pixel editor mode: 
Built-in international chars support: 
|
|
@coderabbitai please review the fontmanager and make suggesions for code consolidation and maintainability. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 13262 🏁 Script executed: Length of output: 14394 🏁 Script executed: Length of output: 1191 🏁 Script executed: Length of output: 1426
🎯 Major Refactoring Opportunities1. Extract File Path Management into a Helper FunctionFile name generation is duplicated in // Add this private helper to FontManager
static void getFontFileName(uint8_t fontNum, char* buffer, size_t bufferSize) {
strcpy_P(buffer, PSTR("/font"));
if (fontNum > 0) {
snprintf(buffer + 5, bufferSize - 5, "%d", fontNum);
}
strcat_P(buffer, PSTR(".wbf"));
}2. Break Down
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
wled00/FX.h (1)
1162-1251: Consider makinggetMetadata()returnconstor adding null checks to callers.The
getMetadata()helper (line 1236) returns a raw pointer that could be null. While callers likescanAvailableFonts()check for null, the inline helpers likeupdateReader()already handle the null case gracefully. This is acceptable but consider defensive assertions in debug builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.h` around lines 1162 - 1251, getMetadata() can return nullptr but currently returns a mutable pointer; change its signature to return a const SegmentFontMetadata* (i.e. const SegmentFontMetadata* getMetadata()) and update all callers (scanAvailableFonts(), updateReader() uses meta via SegmentFontMetadata* cast — switch to const and handle as const) or alternatively add defensive null checks/assertions where getMetadata() is used (e.g., in scanAvailableFonts(), rebuildCache(), updateReader()) to avoid dereferencing null; also add a debug-only assert inside getMetadata() or callers to catch unexpected nulls in development builds.wled00/FX_2Dfcn.cpp (2)
944-944: VLA (widthTable[numGlyphs]) on stack may cause issues with large glyph ranges.While
numGlyphsis bounded to 255 max (uint8_t difference), the worst case is 256 bytes on the stack. This is generally acceptable on ESP32 but could be problematic in deeply nested call stacks. Consider using a fixed-size array or heap allocation for robustness.Alternative using fixed-size array
- uint8_t widthTable[numGlyphs]; + uint8_t widthTable[256]; // Max possible glyphs (uint8_t range)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_2Dfcn.cpp` at line 944, The VLA declaration uint8_t widthTable[numGlyphs] risks stack overflow for large glyph ranges; replace it with a heap-allocated or fixed-size buffer. For example, allocate a dynamic container (e.g., std::vector<uint8_t> widthTable(numGlyphs)) or use a compile-time fixed array sized to the maximum glyph count and index only up to numGlyphs; update any code that reads/writes widthTable accordingly so it uses the new container. Ensure you include <vector> if using std::vector and free/let go of heap resources where appropriate.
785-821: Minor: Consider extracting the TODO comment into a tracked issue.Line 789 has a TODO comment (
// TOD: use printf) that appears incomplete. Also, the hardcoded"0123456789:. "string could be made configurable if different locales need different pre-cached characters.Would you like me to open an issue to track the TODO and potential configurability of pre-cached characters?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_2Dfcn.cpp` around lines 785 - 821, The TODO comment and hardcoded nums string in FontManager::collectNeededCodes are incomplete; open a tracked issue for the TODO and replace the inline note with a short "see ISSUE-<id>" or issue URL, and make the pre-cached characters configurable instead of the hardcoded const char* nums. Concretely: create an issue describing "make pre-cached characters configurable and revisit TODO", update the TODO comment in collectNeededCodes to reference that issue, and add a configurable member (e.g., _cachedChars or setCachedChars()/getCachedChars()) used by collectNeededCodes when _cacheNumbers is true so callers can change "0123456789:. " without editing the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/FX_2Dfcn.cpp`:
- Around line 981-986: The loop that assigns registry entries uses code =
neededCodes[k] and then reads widthTable[code] without validating that code is
within range; add a defensive bounds check in the loop (for the loop in
FX_2Dfcn.cpp that iterates over neededCount and uses neededCodes, widthTable,
registry, fileHdr.height) to ensure code < numGlyphs before accessing widthTable
or writing registry[k]; if out-of-range, skip the entry or set a safe fallback
(e.g., width=0 and a sentinel code) and optionally log or assert to aid
debugging so undefined behavior is prevented.
In `@wled00/util.cpp`:
- Around line 165-171: Fix the typo in the comment header in util.cpp: change
"UTF-8 to unicode onversion" to "UTF-8 to unicode conversion" in the block above
the UTF-8 byte-length table so the documentation text is correct; the comment
sits near the top of the UTF-8 handling code and is not function-specific, so
update that string only.
---
Nitpick comments:
In `@wled00/FX_2Dfcn.cpp`:
- Line 944: The VLA declaration uint8_t widthTable[numGlyphs] risks stack
overflow for large glyph ranges; replace it with a heap-allocated or fixed-size
buffer. For example, allocate a dynamic container (e.g., std::vector<uint8_t>
widthTable(numGlyphs)) or use a compile-time fixed array sized to the maximum
glyph count and index only up to numGlyphs; update any code that reads/writes
widthTable accordingly so it uses the new container. Ensure you include <vector>
if using std::vector and free/let go of heap resources where appropriate.
- Around line 785-821: The TODO comment and hardcoded nums string in
FontManager::collectNeededCodes are incomplete; open a tracked issue for the
TODO and replace the inline note with a short "see ISSUE-<id>" or issue URL, and
make the pre-cached characters configurable instead of the hardcoded const char*
nums. Concretely: create an issue describing "make pre-cached characters
configurable and revisit TODO", update the TODO comment in collectNeededCodes to
reference that issue, and add a configurable member (e.g., _cachedChars or
setCachedChars()/getCachedChars()) used by collectNeededCodes when _cacheNumbers
is true so callers can change "0123456789:. " without editing the function.
In `@wled00/FX.h`:
- Around line 1162-1251: getMetadata() can return nullptr but currently returns
a mutable pointer; change its signature to return a const SegmentFontMetadata*
(i.e. const SegmentFontMetadata* getMetadata()) and update all callers
(scanAvailableFonts(), updateReader() uses meta via SegmentFontMetadata* cast —
switch to const and handle as const) or alternatively add defensive null
checks/assertions where getMetadata() is used (e.g., in scanAvailableFonts(),
rebuildCache(), updateReader()) to avoid dereferencing null; also add a
debug-only assert inside getMetadata() or callers to catch unexpected nulls in
development builds.
1 participant
still a little rough around the edges and needs some refinement and testing of AI assisted code but basically working.
This is accompanied with a web based tool to generate wbs fonts from true type fonts as well as from bitmap based bdf fonts. It's a very versatile and easy to use tool: load a font, move sliders until it looks like you want it to, choose extended char range or set it to any unicode offset you like. Choose the range of glyphs to be exportet, remove unwanted glyphs and even edit glyph pixels before export. I also made a "showcase" tool that loads a wbf font file and exports the glyphs as a PNG so users can share their fonts with a nice preview on discourse/reddit etc.
Currently the tool is not yet downloadable in the PixelForge but available here: https://github.com/DedeHai/WLED-Tools/tree/main/WLED-FontFactory
Fixes #5101
Summary by CodeRabbit