Extend scrolling text FX with custom fonts and international UTF-8 character support#5372
Open
Extend scrolling text FX with custom fonts and international UTF-8 character support#5372
Conversation
Contributor
WalkthroughThis PR introduces a FontManager-based font rendering system for 2D scrolling text, replacing hard-coded per-letter calculations with dynamic font metrics. It adds UTF-8 support, font loading from storage, a new WLED Bitmap Font (WBF) format for font data, and token support for dates/times. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
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.
Collaborator
Author
|
The Font Factory tool: Font pixel editor mode: 
Built-in international chars support: 
|
This was referenced Feb 15, 2026
Collaborator
Author
|
@coderabbitai please review the fontmanager and make suggesions for code consolidation and maintainability. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
6445-6446:⚠️ Potential issue | 🟠 MajorUse left-origin base X for centered non-scrolling text.
Line 6445stores a centered left-origin offset inSEGENV.aux0, butLine 6474treats it as scrolling offset (cols - aux0). This shifts short text to the right and clips it.🔧 Proposed fix
// Draw characters idx = 0; int currentXOffset = 0; // offset of current glyph from text start + const int baseX = (totalTextWidth > cols) + ? int(cols) - int(SEGENV.aux0) // scrolling mode + : int(SEGENV.aux0); // centered non-scrolling mode for (int c = 0; c < numberOfChars; c++) { uint8_t charLen; uint32_t unicode = utf8_decode(&text[idx], &charLen); idx += charLen; uint8_t glyphWidth = fontManager.getGlyphWidth(unicode); - int drawX = int(cols) - int(SEGENV.aux0) + currentXOffset; // AUX0 is scrolling offset + int drawX = baseX + currentXOffset; int advance = isRotated ? glyphHeight + 1 : glyphWidth + letterSpacing; // when rotated use spacing of 1Also applies to: 6474-6475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6445 - 6446, SEGENV.aux0 is being set as a centered left-origin X offset but later treated as a scrolling offset (using cols - aux0) which pushes/ clips short non-scrolling text; change the code paths that use SEGENV.aux0 for non-scrolling/centered text (the logic around where cols - SEGENV.aux0 is used) to use SEGENV.aux0 directly as the left-origin base X for drawing centered text, and stop inverting it (remove the cols - ... usage) so short centered text is placed and not clipped; look for uses of SEGENV.aux0 around the centering block and at the usage site currently doing cols - SEGENV.aux0 and replace that behavior to consume aux0 as the left-origin X.
🧹 Nitpick comments (4)
wled00/src/font/font_6x12.h (2)
40-49: Remove duplicate comment block.Lines 40-44 and 45-49 contain identical content—this appears to be a copy-paste error.
🧹 Proposed fix to remove duplicate
// Font: 6x12 // Height: 12, Max Width: 5, Spacing: 1 // Characters: 32-127 (96 glyphs) // Unicode Offset: 0x00000000 // Variable Width: Yes -// Font: 6x12 -// Height: 12, Max Width: 5, Spacing: 1 -// Characters: 32-127 (96 glyphs) -// Unicode Offset: 0x00000000 -// Variable Width: Yes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/font/font_6x12.h` around lines 40 - 49, Remove the duplicated comment block describing the font (the repeated lines starting with "Font: 6x12", "Height: 12, Max Width: 5, Spacing: 1", "Characters: 32-127 (96 glyphs)", "Unicode Offset: 0x00000000", "Variable Width: Yes") so only a single header comment remains; locate the duplicate by searching for the "Font: 6x12" comment block in font_6x12.h and delete the second occurrence.
14-14: Minor documentation inconsistency: Last Char value.The header layout comment on line 14 says
[6] Last Char: 126, but the actual header byte at line 52 is0x7F(127). The width table correctly includes 96 entries for characters 32-127, with character 127 having width 0. Consider updating line 14 to match the actual value.📝 Proposed fix
- * [6] Last Char: 126 + * [6] Last Char: 127🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/font/font_6x12.h` at line 14, The header comment's "Last Char" value is inconsistent with the actual font data: update the header layout comment (the "[6] Last Char" entry in font_6x12.h) to 127 to match the header byte 0x7F in the file and the width table covering characters 32–127 (96 entries, with char 127 width 0); ensure the comment and the stored last-char byte (0x7F) are consistent.wled00/src/font/old fonts/console_font_6x8.h (1)
41-170: Consider using 2-space indentation for consistency with project guidelines.The array contents use 4-space indentation, but the coding guidelines specify 2-space indentation for C++ header files matching
wled00/**/!(html_*)*.h. While this is data rather than logic, aligning with the project standard improves consistency.As per coding guidelines: "Use 2-space indentation for non-generated C++ header files (.h)".
Example adjustment (first few lines)
static const unsigned char console_font_6x8[] PROGMEM = { - 0x57, 0x08, 0x06, 0x01, 0x00, 0x20, 0x7E, 0x00, 0x00, 0x00, 0x00, 0x00, // Header: Magic, H, W, Spacing, Flags, First, Last, Reserved, UnicodeOffset - - // 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // /* code=0, hex=0x00, ascii="^@" */ + 0x57, 0x08, 0x06, 0x01, 0x00, 0x20, 0x7E, 0x00, 0x00, 0x00, 0x00, 0x00, // Header: Magic, H, W, Spacing, Flags, First, Last, Reserved, UnicodeOffset + + // 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // /* code=0, hex=0x00, ascii="^@" */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/font/old` fonts/console_font_6x8.h around lines 41 - 170, The console_font_6x8 array (static const unsigned char console_font_6x8[] PROGMEM) uses 4-space indentation; update the file to use 2-space indentation throughout (including the array header line, each byte row, and commented code/metadata lines) to match project C++ header style; ensure alignment is preserved so each comment like /* code=32, ... */ and each hex row keeps the same relative spacing but with two spaces per indent level.wled00/FX.h (1)
1144-1146: Consider defensive null checks for font dimension getters.The inline getters
getFontHeight(),getFontWidth(), andgetFontSpacing()directly dereference_fontBasewithout null checks. If called beforeloadFont()succeeds or after a failed load, this would cause undefined behavior.While the current usage pattern in the scrolling text effect appears to guard these calls properly, adding defensive checks would improve robustness.
Proposed fix
// Get dimensions (use cached header) - inline uint8_t getFontHeight() { return reinterpret_cast<FontHeader*>(_fontBase)->height; } - inline uint8_t getFontWidth() { return reinterpret_cast<FontHeader*>(_fontBase)->width; } - inline uint8_t getFontSpacing() { return reinterpret_cast<FontHeader*>(_fontBase)->spacing; } + inline uint8_t getFontHeight() { return _fontBase ? reinterpret_cast<FontHeader*>(_fontBase)->height : 0; } + inline uint8_t getFontWidth() { return _fontBase ? reinterpret_cast<FontHeader*>(_fontBase)->width : 0; } + inline uint8_t getFontSpacing() { return _fontBase ? reinterpret_cast<FontHeader*>(_fontBase)->spacing : 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.h` around lines 1144 - 1146, The getters getFontHeight(), getFontWidth(), and getFontSpacing() dereference _fontBase via reinterpret_cast<FontHeader*> without null checks; update each to defensively check whether _fontBase is non-null (and optionally valid) and return a safe default (e.g., 0) when it is null instead of dereferencing, so calls to getFontHeight/getFontWidth/getFontSpacing do not cause undefined behavior if loadFont() failed or wasn't called; locate these methods in FX.h and guard access to reinterpret_cast<FontHeader*>(_fontBase)->height/width/spacing accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX_2Dfcn.cpp`:
- Around line 812-820: The bitmap read in the font-loading loop doesn't verify
file.read returned the expected byte count, so update the loop where
file.read(dataptr, bytes) is called to capture its return value (e.g., size_t n
= file.read(dataptr, bytes)) and check that n == bytes; on mismatch, handle the
truncated file by zeroing the target buffer (e.g., memset(dataptr, 0, bytes))
and either break/return an error code or mark the font load as failed as
appropriate for the surrounding flow; ensure the same dataptr/offset bookkeeping
is preserved and keep the existing flash path using memcpy_P(flashFont + offset,
...) unchanged.
In `@wled00/FX.cpp`:
- Around line 6407-6419: totalTextWidth is being computed inconsistently with
the actual glyph advance causing double-spacing and mismatches for rotated text;
update the loop that decodes glyphs (utf8_decode) to accumulate per-glyph
advance instead of mixing getGlyphWidth + letterSpacing: for each unicode glyph
compute advance = isRotated ? (glyphHeight + 1) :
fontManager.getGlyphAdvance(unicode) (or if no getGlyphAdvance exists, use
fontManager.getGlyphWidth(unicode) and add letterSpacing only once per-glyph),
add advance to totalTextWidth without adding an extra letterSpacing after the
last character (remove the redundant totalTextWidth -= letterSpacing when you
handle spacing per-glyph), and make the later draw-advance calculation (the code
around the draw advance at lines ~6475) use the same per-glyph advance formula
so both scrolling decision and rendering use identical metrics.
---
Duplicate comments:
In `@wled00/FX.cpp`:
- Around line 6445-6446: SEGENV.aux0 is being set as a centered left-origin X
offset but later treated as a scrolling offset (using cols - aux0) which pushes/
clips short non-scrolling text; change the code paths that use SEGENV.aux0 for
non-scrolling/centered text (the logic around where cols - SEGENV.aux0 is used)
to use SEGENV.aux0 directly as the left-origin base X for drawing centered text,
and stop inverting it (remove the cols - ... usage) so short centered text is
placed and not clipped; look for uses of SEGENV.aux0 around the centering block
and at the usage site currently doing cols - SEGENV.aux0 and replace that
behavior to consume aux0 as the left-origin X.
---
Nitpick comments:
In `@wled00/FX.h`:
- Around line 1144-1146: The getters getFontHeight(), getFontWidth(), and
getFontSpacing() dereference _fontBase via reinterpret_cast<FontHeader*> without
null checks; update each to defensively check whether _fontBase is non-null (and
optionally valid) and return a safe default (e.g., 0) when it is null instead of
dereferencing, so calls to getFontHeight/getFontWidth/getFontSpacing do not
cause undefined behavior if loadFont() failed or wasn't called; locate these
methods in FX.h and guard access to
reinterpret_cast<FontHeader*>(_fontBase)->height/width/spacing accordingly.
In `@wled00/src/font/font_6x12.h`:
- Around line 40-49: Remove the duplicated comment block describing the font
(the repeated lines starting with "Font: 6x12", "Height: 12, Max Width: 5,
Spacing: 1", "Characters: 32-127 (96 glyphs)", "Unicode Offset: 0x00000000",
"Variable Width: Yes") so only a single header comment remains; locate the
duplicate by searching for the "Font: 6x12" comment block in font_6x12.h and
delete the second occurrence.
- Line 14: The header comment's "Last Char" value is inconsistent with the
actual font data: update the header layout comment (the "[6] Last Char" entry
in font_6x12.h) to 127 to match the header byte 0x7F in the file and the width
table covering characters 32–127 (96 entries, with char 127 width 0); ensure the
comment and the stored last-char byte (0x7F) are consistent.
In `@wled00/src/font/old` fonts/console_font_6x8.h:
- Around line 41-170: The console_font_6x8 array (static const unsigned char
console_font_6x8[] PROGMEM) uses 4-space indentation; update the file to use
2-space indentation throughout (including the array header line, each byte row,
and commented code/metadata lines) to match project C++ header style; ensure
alignment is preserved so each comment like /* code=32, ... */ and each hex row
keeps the same relative spacing but with two spaces per indent level.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
wled00/src/font/old fonts/console_font_4x6_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_5x12_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_5x8_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_6x8_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_7x9_showcase.pngis excluded by!**/*.png
📒 Files selected for processing (25)
wled00/FX.cppwled00/FX.hwled00/FX_2Dfcn.cppwled00/src/font/6x12.wbfwled00/src/font/console_font_6x8.hwled00/src/font/console_font_6x8.wbfwled00/src/font/console_font_7x9.hwled00/src/font/console_font_7x9.wbfwled00/src/font/font_6x12.hwled00/src/font/font_TinyUnicode_8px.hwled00/src/font/font_TinyUnicode_8px.wbfwled00/src/font/font_tom-thumb_6px.wbfwled00/src/font/font_tom_thumb_6px.hwled00/src/font/old fonts/console_font_4x6.hwled00/src/font/old fonts/console_font_4x6.wbfwled00/src/font/old fonts/console_font_5x12.hwled00/src/font/old fonts/console_font_5x12.wbfwled00/src/font/old fonts/console_font_5x8.hwled00/src/font/old fonts/console_font_5x8.wbfwled00/src/font/old fonts/console_font_6x8.hwled00/src/font/old fonts/console_font_6x8.wbfwled00/src/font/old fonts/console_font_7x9.hwled00/src/font/old fonts/console_font_7x9.wbfwled00/src/font/old fonts/tom-thumb_6px.wbfwled00/util.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
wled00/FX.cpp (2)
6407-6419:⚠️ Potential issue | 🟠 MajorUnify width calculation with glyph advance logic.
totalTextWidthand per-glyphadvanceare still computed with different spacing rules, so fit/scroll selection can be wrong and clipping still occurs.🔧 Proposed fix
- for (int c = 0; c < numberOfChars; c++) { + const int interGlyphSpacing = isRotated ? 1 : letterSpacing; + for (int c = 0; c < numberOfChars; c++) { uint8_t charLen; uint32_t unicode = utf8_decode(&text[idx], &charLen); idx += charLen; - - if (isRotated) { - totalTextWidth += glyphHeight + 1; // use height when rotated, spacing of 1 - } else { - totalTextWidth += fontManager.getGlyphWidth(unicode) + letterSpacing; - } - if (c < numberOfChars - 1) totalTextWidth += letterSpacing; + const int glyphAdvance = isRotated + ? (glyphHeight + interGlyphSpacing) + : (fontManager.getGlyphWidth(unicode) + interGlyphSpacing); + totalTextWidth += glyphAdvance; } - totalTextWidth -= letterSpacing; // remove spacing after last character + if (numberOfChars > 0) totalTextWidth -= interGlyphSpacing; // remove trailing spacing ... - int advance = isRotated ? glyphHeight + 1 : glyphWidth + letterSpacing; // when rotated use spacing of 1 + int advance = isRotated ? glyphHeight + interGlyphSpacing : glyphWidth + interGlyphSpacing;Also applies to: 6475-6476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6407 - 6419, The width accumulation in the loop computing totalTextWidth uses different spacing rules than the glyph advance calculation, causing mismatched fit/scroll decisions and clipping; modify the totalTextWidth computation to mirror the per-glyph advance logic used elsewhere: for each decoded glyph (unicode, charLen) add the exact advance used for rendering (use glyphHeight when isRotated, otherwise use fontManager.getGlyphWidth(unicode)), include letterSpacing only when the glyph advance logic does, and remove the extra subtraction of letterSpacing at the end—also apply the same unified rule where totalTextWidth is computed in the other occurrence referenced (the block around the second occurrence of the same computation).
6445-6446:⚠️ Potential issue | 🟠 MajorFix centered-text X origin (left-origin vs right-origin mixup).
In non-scrolling mode,
SEGENV.aux0is set as a centered left-origin offset, butdrawXstill usescols - SEGENV.aux0, pushing text rightward.🔧 Proposed fix
- idx = 0; - int currentXOffset = 0; // offset of current glyph from text start + idx = 0; + int currentXOffset = 0; // offset of current glyph from text start + const int baseX = (totalTextWidth > cols) + ? int(cols) - int(SEGENV.aux0) + : int(SEGENV.aux0); for (int c = 0; c < numberOfChars; c++) { ... - int drawX = int(cols) - int(SEGENV.aux0) + currentXOffset; // AUX0 is scrolling offset + int drawX = baseX + currentXOffset;Also applies to: 6474-6475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6445 - 6446, SEGENV.aux0 is computed as a left-origin centered offset (SEGENV.aux0 = (cols - totalTextWidth) / 2) but drawX uses cols - SEGENV.aux0 which treats aux0 as a right-origin offset, causing rightward shift; fix by making drawX use SEGENV.aux0 directly (i.e., drawX = SEGENV.aux0 + currentCharX or equivalent) in the non-scrolling path where SEGENV.aux0 is set, and apply the same change to the duplicate instance around the block referenced by lines ~6474-6475 so both places consistently interpret SEGENV.aux0 as a left-origin X offset.wled00/FX_2Dfcn.cpp (1)
812-820:⚠️ Potential issue | 🟡 MinorMissing return value check for
file.readin bitmap loading loop.The width table read on line 751 correctly validates the return value, but the bitmap read on line 815 does not check if the expected number of bytes were actually read. If the font file is truncated, this could result in partially uninitialized bitmap data being cached.
Proposed fix
// read from file or flash if (file) { file.seek(offset); - file.read(dataptr, bytes); + if (file.read(dataptr, bytes) != bytes) { + file.close(); + meta->glyphCount = 0; // invalidate cache + return; + } } else { memcpy_P(dataptr, flashFont + offset, bytes); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_2Dfcn.cpp` around lines 812 - 820, The bitmap-read branch in the font loading loop (in FX_2Dfcn.cpp) calls file.read(dataptr, bytes) without checking its return value, so if file.read returns fewer than bytes the cached bitmap (dataptr) may be partially uninitialized; update the bitmap read logic to check the return value from file.read and handle short/failed reads (e.g., abort the load and return an error, or zero-fill the remaining bytes and mark the font as invalid) — modify the block that uses file.read/dataptr/bytes/offset to validate the number of bytes read and take a deterministic failure path (same style as the width-table check) instead of assuming a full read.
🧹 Nitpick comments (6)
wled00/src/font/font_6x12.h (1)
5-52: Minor comment inconsistencies.
The header description at line 14 says
Last Char: 126but the actual header byte at line 52 is0x7F(127), and lines 42-43 say "Characters: 32-127 (96 glyphs)". The data is correct; the comment at line 14 should sayLast Char: 127.Lines 40-49 contain duplicate metadata comments ("Font: 6x12... Variable Width: Yes" appears twice).
These are cosmetic issues—the font data itself is correctly structured.
🔧 Suggested comment cleanup
* [5] First Char: 32 -* [6] Last Char: 126 +* [6] Last Char: 127 * [7] reserved: 0x00And remove the duplicate block at lines 45-49.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/font/font_6x12.h` around lines 5 - 52, Update the header comments for font_6x12: change the "Last Char: 126" text to "Last Char: 127" to match the header byte 0x7F in the font_6x12 array and the "Characters: 32-127" lines, and remove the duplicated metadata block that repeats "Font: 6x12 ... Variable Width: Yes" so there is only a single metadata comment block describing the font.wled00/src/font/old fonts/console_font_6x8.h (2)
42-170: Use 2-space indentation in this non-generated header.Current array row indentation is 4 spaces; project guideline for
.hfiles is 2 spaces.As per coding guidelines:
wled00/**/!(html_*)*.h: Use 2-space indentation for non-generated C++ header files (.h).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/font/old` fonts/console_font_6x8.h around lines 42 - 170, The header's font data array uses 4-space indentation for each byte row (e.g., lines starting with "0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* code=32..." and all subsequent glyph lines), but the project guideline requires 2-space indentation for non-generated .h files; update the indentation for the array rows and any commented glyph lines (such as entries for code=32..126 and the header line "0x57, 0x08, 0x06, 0x01, 0x00, 0x20, 0x7E, ...") to use 2 spaces instead of 4 while preserving all byte values and comments.
41-300: Add a compile-time payload-size assertion for the packed font table.This asset is tightly packed; a single accidental byte drift can desync glyph indexing. Add a
static_asserton total size to fail fast at compile time.Proposed hardening diff
+static constexpr uint8_t CONSOLE_FONT_6X8_HEADER_SIZE = 12; +static constexpr uint8_t CONSOLE_FONT_6X8_FIRST_CHAR = 0x20; +static constexpr uint8_t CONSOLE_FONT_6X8_LAST_CHAR = 0x7E; +static constexpr uint8_t CONSOLE_FONT_6X8_BYTES_PER_GLYPH = 6; + static const unsigned char console_font_6x8[] PROGMEM = { @@ }; + +static_assert( + sizeof(console_font_6x8) == + CONSOLE_FONT_6X8_HEADER_SIZE + + ((CONSOLE_FONT_6X8_LAST_CHAR - CONSOLE_FONT_6X8_FIRST_CHAR + 1) * CONSOLE_FONT_6X8_BYTES_PER_GLYPH), + "console_font_6x8 payload size does not match header/glyph layout" +);Based on learnings: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/font/old` fonts/console_font_6x8.h around lines 41 - 300, The packed font array console_font_6x8 can silently shift if a byte is added/removed; add a compile-time check: introduce meaningful constants (e.g., FIRST_GLYPH = 0x20, LAST_GLYPH = 0x7E, GLYPH_BYTES = 6, HEADER_BYTES = 8) and a static_assert that sizeof(console_font_6x8) == (HEADER_BYTES + (LAST_GLYPH - FIRST_GLYPH + 1) * GLYPH_BYTES) with a clear message so compilation fails if the table size drifts.wled00/FX_2Dfcn.cpp (2)
596-642: Redundant_fontNumassignment on line 605.
_fontNumis already set tofontNumon line 598, making the assignment on line 605 redundant.Proposed fix
SegmentFontMetadata* meta = getMetadata(); if (!meta) return false; // ensure segment data exists - _fontNum = fontNum; // store font to be used if (useFile) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_2Dfcn.cpp` around lines 596 - 642, The loadFont function redundantly assigns _fontNum twice; remove the second assignment of _fontNum in FontManager::loadFont (the line that reassigns _fontNum = fontNum just before the useFile block) so the initial _fontNum = fontNum at the top remains the single initialization, keeping the later fallback logic that modifies _fontNum intact and ensuring no behavioral change to cacheID creation or scanAvailableFonts handling.
629-634: Consider defining a named constant for the file-font flag0x80.The magic number
0x80is used here (line 629) and again on line 708 to indicate a file-based font vs. flash font. A named constant would improve readability and maintainability.Proposed fix
Add near the other font constants in FX.h:
static constexpr uint8_t FONT_FILE_FLAG = 0x80; // highest bit indicates file vs flash fontThen update usage:
- uint8_t cacheID = _fontNum | (_useFileFont ? 0x80 : 0x00); + uint8_t cacheID = _fontNum | (_useFileFont ? FONT_FILE_FLAG : 0x00);Based on learnings: In WLED code reviews, look for "magic numbers" and suggest replacing them with defined constants when meaningful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_2Dfcn.cpp` around lines 629 - 634, Replace the magic 0x80 file-font flag with a named constant: add a constexpr uint8_t FONT_FILE_FLAG = 0x80 near the other font constants in FX.h, then use FONT_FILE_FLAG when building cacheID (uint8_t cacheID = _fontNum | (_useFileFont ? FONT_FILE_FLAG : 0x00)) and in the other location that checks/sets the file-font bit so references to the flag are clear; update any occurrences that currently use 0x80 to use FONT_FILE_FLAG and keep meta->cachedFontNum and meta->glyphCount logic unchanged.wled00/FX.h (1)
1144-1146: Inline accessors dereference_fontBasewithout null checks.
getFontHeight(),getFontWidth(), andgetFontSpacing()will crash if called before a successfulloadFont()or if font loading failed. While the function comment notes thatloadFont()must succeed first, consider adding defensive null checks for robustness:Proposed fix
- inline uint8_t getFontHeight() { return reinterpret_cast<FontHeader*>(_fontBase)->height; } - inline uint8_t getFontWidth() { return reinterpret_cast<FontHeader*>(_fontBase)->width; } - inline uint8_t getFontSpacing() { return reinterpret_cast<FontHeader*>(_fontBase)->spacing; } + inline uint8_t getFontHeight() { return _fontBase ? reinterpret_cast<FontHeader*>(_fontBase)->height : 0; } + inline uint8_t getFontWidth() { return _fontBase ? reinterpret_cast<FontHeader*>(_fontBase)->width : 0; } + inline uint8_t getFontSpacing() { return _fontBase ? reinterpret_cast<FontHeader*>(_fontBase)->spacing : 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.h` around lines 1144 - 1146, The three inline accessors getFontHeight(), getFontWidth(), and getFontSpacing() currently dereference _fontBase without checking for null and can crash if loadFont() hasn't succeeded; update each to first check _fontBase (or reinterpret_cast<FontHeader*>(_fontBase)) for nullptr and return a safe default (e.g., 0) when no font is loaded, otherwise return the respective FontHeader field (height, width, spacing) so callers are protected from null dereference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX_2Dfcn.cpp`:
- Around line 854-876: Move null checks for _fontBase and _segment->data before
any dereference: at the top of FontManager::getGlyphBitmap validate _fontBase
(and _segment and _segment->data) and return nullptr if any are null, then
construct FontHeader* hdr = reinterpret_cast<FontHeader*>(_fontBase) and call
getGlyphIndex(unicode, hdr); this prevents dereferencing _fontBase when it may
be invalid. Also ensure that getGlyphIndex is only called after hdr is created
and that subsequent code (accessing _segment->data, SegmentFontMetadata,
GlyphEntry, and _fontBase + FONT_HEADER_SIZE + bitmapOffset) runs after these
checks.
In `@wled00/FX.cpp`:
- Line 6395: Do not early-return when fontManager.loadFont(fontNum, text,
useCustomFont) fails; instead implement a deterministic fallback: on false, set
useCustomFont = false and attempt to load a known-safe default font (e.g., call
fontManager.loadFont(0, text, false) or your project’s default font index), and
if that also fails ensure you clear or reset the pixel/frame state via the
existing frame-clear/reset function so stale pixels aren’t left on screen; keep
the rendering path continuing after the fallback so FX rendering proceeds
predictably.
In `@wled00/src/font/old` fonts/console_font_6x8.h:
- Line 3: The font file console_font_6x8.h currently attributes the source but
lacks license provenance; update the top-of-file comment to include an explicit
license statement or written permission from the original author (e.g., add
"License: [license name] — confirmed by author: [name/email] on [date]" or a
SPDX identifier) or replace the glyph data with a font that has a clear
open-source license; ensure the change is applied in console_font_6x8.h (top
comment) and any README or attribution metadata referencing this font is updated
accordingly.
---
Duplicate comments:
In `@wled00/FX_2Dfcn.cpp`:
- Around line 812-820: The bitmap-read branch in the font loading loop (in
FX_2Dfcn.cpp) calls file.read(dataptr, bytes) without checking its return value,
so if file.read returns fewer than bytes the cached bitmap (dataptr) may be
partially uninitialized; update the bitmap read logic to check the return value
from file.read and handle short/failed reads (e.g., abort the load and return an
error, or zero-fill the remaining bytes and mark the font as invalid) — modify
the block that uses file.read/dataptr/bytes/offset to validate the number of
bytes read and take a deterministic failure path (same style as the width-table
check) instead of assuming a full read.
In `@wled00/FX.cpp`:
- Around line 6407-6419: The width accumulation in the loop computing
totalTextWidth uses different spacing rules than the glyph advance calculation,
causing mismatched fit/scroll decisions and clipping; modify the totalTextWidth
computation to mirror the per-glyph advance logic used elsewhere: for each
decoded glyph (unicode, charLen) add the exact advance used for rendering (use
glyphHeight when isRotated, otherwise use fontManager.getGlyphWidth(unicode)),
include letterSpacing only when the glyph advance logic does, and remove the
extra subtraction of letterSpacing at the end—also apply the same unified rule
where totalTextWidth is computed in the other occurrence referenced (the block
around the second occurrence of the same computation).
- Around line 6445-6446: SEGENV.aux0 is computed as a left-origin centered
offset (SEGENV.aux0 = (cols - totalTextWidth) / 2) but drawX uses cols -
SEGENV.aux0 which treats aux0 as a right-origin offset, causing rightward shift;
fix by making drawX use SEGENV.aux0 directly (i.e., drawX = SEGENV.aux0 +
currentCharX or equivalent) in the non-scrolling path where SEGENV.aux0 is set,
and apply the same change to the duplicate instance around the block referenced
by lines ~6474-6475 so both places consistently interpret SEGENV.aux0 as a
left-origin X offset.
---
Nitpick comments:
In `@wled00/FX_2Dfcn.cpp`:
- Around line 596-642: The loadFont function redundantly assigns _fontNum twice;
remove the second assignment of _fontNum in FontManager::loadFont (the line that
reassigns _fontNum = fontNum just before the useFile block) so the initial
_fontNum = fontNum at the top remains the single initialization, keeping the
later fallback logic that modifies _fontNum intact and ensuring no behavioral
change to cacheID creation or scanAvailableFonts handling.
- Around line 629-634: Replace the magic 0x80 file-font flag with a named
constant: add a constexpr uint8_t FONT_FILE_FLAG = 0x80 near the other font
constants in FX.h, then use FONT_FILE_FLAG when building cacheID (uint8_t
cacheID = _fontNum | (_useFileFont ? FONT_FILE_FLAG : 0x00)) and in the other
location that checks/sets the file-font bit so references to the flag are clear;
update any occurrences that currently use 0x80 to use FONT_FILE_FLAG and keep
meta->cachedFontNum and meta->glyphCount logic unchanged.
In `@wled00/FX.h`:
- Around line 1144-1146: The three inline accessors getFontHeight(),
getFontWidth(), and getFontSpacing() currently dereference _fontBase without
checking for null and can crash if loadFont() hasn't succeeded; update each to
first check _fontBase (or reinterpret_cast<FontHeader*>(_fontBase)) for nullptr
and return a safe default (e.g., 0) when no font is loaded, otherwise return the
respective FontHeader field (height, width, spacing) so callers are protected
from null dereference.
In `@wled00/src/font/font_6x12.h`:
- Around line 5-52: Update the header comments for font_6x12: change the "Last
Char: 126" text to "Last Char: 127" to match the header byte 0x7F in the
font_6x12 array and the "Characters: 32-127" lines, and remove the duplicated
metadata block that repeats "Font: 6x12 ... Variable Width: Yes" so there is
only a single metadata comment block describing the font.
In `@wled00/src/font/old` fonts/console_font_6x8.h:
- Around line 42-170: The header's font data array uses 4-space indentation for
each byte row (e.g., lines starting with "0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
/* code=32..." and all subsequent glyph lines), but the project guideline
requires 2-space indentation for non-generated .h files; update the indentation
for the array rows and any commented glyph lines (such as entries for
code=32..126 and the header line "0x57, 0x08, 0x06, 0x01, 0x00, 0x20, 0x7E,
...") to use 2 spaces instead of 4 while preserving all byte values and
comments.
- Around line 41-300: The packed font array console_font_6x8 can silently shift
if a byte is added/removed; add a compile-time check: introduce meaningful
constants (e.g., FIRST_GLYPH = 0x20, LAST_GLYPH = 0x7E, GLYPH_BYTES = 6,
HEADER_BYTES = 8) and a static_assert that sizeof(console_font_6x8) ==
(HEADER_BYTES + (LAST_GLYPH - FIRST_GLYPH + 1) * GLYPH_BYTES) with a clear
message so compilation fails if the table size drifts.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
wled00/src/font/old fonts/console_font_4x6_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_5x12_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_5x8_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_6x8_showcase.pngis excluded by!**/*.pngwled00/src/font/old fonts/console_font_7x9_showcase.pngis excluded by!**/*.png
📒 Files selected for processing (25)
wled00/FX.cppwled00/FX.hwled00/FX_2Dfcn.cppwled00/src/font/6x12.wbfwled00/src/font/console_font_6x8.hwled00/src/font/console_font_6x8.wbfwled00/src/font/console_font_7x9.hwled00/src/font/console_font_7x9.wbfwled00/src/font/font_6x12.hwled00/src/font/font_TinyUnicode_8px.hwled00/src/font/font_TinyUnicode_8px.wbfwled00/src/font/font_tom-thumb_6px.wbfwled00/src/font/font_tom_thumb_6px.hwled00/src/font/old fonts/console_font_4x6.hwled00/src/font/old fonts/console_font_4x6.wbfwled00/src/font/old fonts/console_font_5x12.hwled00/src/font/old fonts/console_font_5x12.wbfwled00/src/font/old fonts/console_font_5x8.hwled00/src/font/old fonts/console_font_5x8.wbfwled00/src/font/old fonts/console_font_6x8.hwled00/src/font/old fonts/console_font_6x8.wbfwled00/src/font/old fonts/console_font_7x9.hwled00/src/font/old fonts/console_font_7x9.wbfwled00/src/font/old fonts/tom-thumb_6px.wbfwled00/util.cpp
| @@ -0,0 +1,300 @@ | |||
| // code points 0-31 and 127-255 are commented out to save memory, they contain extra characters (CP437), | |||
| // which could be used with an UTF-8 to CP437 conversion | |||
| // font courtesy of https://github.com/idispatch/raster-fonts | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
github idispatch raster-fonts license
💡 Result:
The GitHub repo idispatch/raster-fonts does not appear to include a LICENSE/COPYING file, and GitHub does not show a detected repository license in the file listing we can access. [1]
Practically, that means you should treat the contents as copyrighted / “all rights reserved” (i.e., no permission to reuse/modify/redistribute beyond what copyright law allows) unless you get permission from the author(s) or they add an explicit license. [1]
Source
- idispatch/raster-fonts repository file listing (no license file shown) [1]
🏁 Script executed:
# Check the file contents and any license information
head -50 "wled00/src/font/old fonts/console_font_6x8.h" 2>/dev/null || echo "File path may need adjustment"Repository: wled/WLED
Length of output: 2293
Add explicit license provenance for imported font data.
The source repository https://github.com/idispatch/raster-fonts contains no LICENSE file and no detected license terms. Without explicit permission, redistributing this font data in WLED (an open-source project) is not authorized under copyright law. Either add proper license documentation to Line 3 (e.g., explicit license confirmation from the original author) or replace this asset with a font that has an explicit open-source compatible license.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/src/font/old` fonts/console_font_6x8.h at line 3, The font file
console_font_6x8.h currently attributes the source but lacks license provenance;
update the top-of-file comment to include an explicit license statement or
written permission from the original author (e.g., add "License: [license name]
— confirmed by author: [name/email] on [date]" or a SPDX identifier) or replace
the glyph data with a font that has a clear open-source license; ensure the
change is applied in console_font_6x8.h (top comment) and any README or
attribution metadata referencing this font is updated accordingly.
Collaborator
Author
There was a problem hiding this comment.
this font has been used by WLED for years. but you are right, license is unclear
Contributor
There was a problem hiding this comment.
@DedeHai, thanks for the context! Since this is legacy code in the "old fonts" directory, here are a few paths forward to resolve the license uncertainty:
- Contact the original author – Reach out to the idispatch/raster-fonts maintainer to request explicit permission or ask them to add a license file
- Document existing permission – If WLED obtained permission previously, add that documentation to the file header
- Find/substitute a clearly-licensed alternative – Replace with fonts from sources like GNU Unifont, Terminus, or other explicitly open-licensed bitmap fonts
- Remove if unused – If the new FontManager system makes these "old fonts" obsolete, consider removing them in a future cleanup
No urgency if this has been stable for years, but documenting the license status (even if it's "used with implicit permission since [year]") would help future contributors. Would you like me to help search for suitable open-licensed replacement fonts?
🐰💭
((
( -.-)
o_(")(")
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 5372
File: wled00/FX_2Dfcn.cpp:854-876
Timestamp: 2026-02-26T18:46:21.931Z
Learning: In WLED (repo wled/WLED), file wled00/FX_2Dfcn.cpp, class FontManager: null pointer checks are intentionally omitted in methods like getGlyphBitmap(), getGlyphWidth(), and drawCharacter() because the API contract requires callers to first call loadFont() and verify it returns true before using any other FontManager functions. This precondition is documented in the loadFont() comment.
Learnt from: DedeHai
Repo: wled/WLED PR: 5372
File: wled00/FX.cpp:6395-6395
Timestamp: 2026-02-26T18:45:23.115Z
Learning: In WLED (repo wled/WLED), file wled00/FX.cpp, function mode_2Dscrollingtext(): the early return when fontManager.loadFont(...) fails is intentional to signal that a custom font wasn’t loaded (not that the effect failed). Do not replace this with a visual fallback that clears or renders with a default font.
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Collaborator
Author
There was a problem hiding this comment.
explicit permission was given AFAIK but the font's original sources are unknow
Contributor
There was a problem hiding this comment.
@DedeHai, thanks for confirming! If explicit permission was given, that resolves the core licensing concern. Since the original sources are unknown, further provenance tracking isn't practical.
Optional follow-up: Consider adding a brief note to the file header documenting the permission status, something like:
// Font courtesy of https://github.com/idispatch/raster-fonts
// Used with explicit permission; original sources unknownThis would help future contributors understand the licensing situation without needing to trace the history. But given this is legacy code in the "old fonts" directory with established permission, no urgent action needed.
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 5372
File: wled00/FX_2Dfcn.cpp:854-876
Timestamp: 2026-02-26T18:46:21.931Z
Learning: In WLED (repo wled/WLED), file wled00/FX_2Dfcn.cpp, class FontManager: null pointer checks are intentionally omitted in methods like getGlyphBitmap(), getGlyphWidth(), and drawCharacter() because the API contract requires callers to first call loadFont() and verify it returns true before using any other FontManager functions. This precondition is documented in the loadFont() comment.
Learnt from: DedeHai
Repo: wled/WLED PR: 5372
File: wled00/FX.cpp:6395-6395
Timestamp: 2026-02-26T18:45:23.115Z
Learning: In WLED (repo wled/WLED), file wled00/FX.cpp, function mode_2Dscrollingtext(): the early return when fontManager.loadFont(...) fails is intentional to signal that a custom font wasn’t loaded (not that the effect failed). Do not replace this with a visual fallback that clears or renders with a default font.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/update.htm:2-2
Timestamp: 2026-02-19T13:24:28.983Z
Learning: In WLED, generated embedded header files (html_*.h, js_iro.h, etc.) are NOT stored in the repository and have not been for over 2 years. These files are automatically regenerated during the build process from the source files in wled00/data/. Only the source .htm, .html, .css, and .js files in wled00/data/ should be committed when making web UI changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/dmxmap.htm:2-2
Timestamp: 2026-02-19T13:23:42.152Z
Learning: In WLED, html_*.h files are NOT stored in the repository. They have been excluded for more than two years and are automatically regenerated as part of the build process. Do not request these files to be committed.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.cpp (1)
6339-6341:⚠️ Potential issue | 🟠 MajorCast input to
unsigned charbeforestd::toupper.At line 6340, passing raw
charintostd::toupperis undefined for negative byte values. With UTF-8 text support, this can occur when a#is followed by non-ASCII bytes in the segment name.🔧 Proposed fix
- token[j] = std::toupper(SEGMENT.name[i + j]); + token[j] = char(std::toupper(static_cast<unsigned char>(SEGMENT.name[i + j])));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6339 - 6341, The call to std::toupper uses raw char from SEGMENT.name which is undefined for negative bytes; update the assignment in the loop that sets token[j] to pass an unsigned char cast: call std::toupper(static_cast<unsigned char>(SEGMENT.name[i + j])) (or C-style (unsigned char)) so non-ASCII bytes are handled safely, leaving the rest of the loop and variables (token, SEGMENT.name, i, j) unchanged.
🧹 Nitpick comments (5)
wled00/src/font/font_5x12.h (1)
48-48: Use 2-space indentation per coding guidelines.The array content uses 4-space indentation, but the project guideline for non-generated header files under
wled00/requires 2-space indentation.As per coding guidelines: "
wled00/**/!(html_*)*.h: Use 2-space indentation for non-generated C++ header files (.h)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/font/font_5x12.h` at line 48, The array initializer line in font_5x12.h (the byte header row starting with 0x57, 0x0C, ...) is indented with 4 spaces; change its leading indentation to 2 spaces to comply with the wled00 header-file style rule (non-generated .h files use 2-space indentation). Locate the array/byte header in font_5x12.h and adjust the whitespace at the start of that line (and any adjacent non-generated header lines in the same file) from 4 spaces to 2 spaces so the file matches the project's indentation guideline.wled00/FX.cpp (2)
6331-6331: Cache numeric glyphs once per frame instead of once per token.
fontManager.cacheNumbers(true)is invoked repeatedly inside token parsing. Functionally fine, but unnecessary repeated calls on the hot path.♻️ Suggested refactor
+ bool shouldCacheNumbers = false; ... - fontManager.cacheNumbers(true); // cache all numbers when using clock to avoid frequent re-caching + shouldCacheNumbers = true; ... - fontManager.cacheNumbers(true); // cache all numbers when using clocks to avoid frequent re-caching + shouldCacheNumbers = true; ... + if (shouldCacheNumbers) fontManager.cacheNumbers(true);Also applies to: 6374-6375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` at line 6331, fontManager.cacheNumbers(true) is being called inside the token parsing hot path causing redundant work; move this call to a per-frame location (e.g., at the start of the frame render/update routine that drives token parsing) so numeric glyphs are cached once per frame instead of per token. Locate the token-parsing code paths that call fontManager.cacheNumbers(true) (the current occurrences including the one shown and the other calls around the positions marked 6374-6375), remove those in-token calls, and add a single fontManager.cacheNumbers(true) at the frame entry point (the function that begins each render/update cycle) so caching happens once per frame. Ensure behavior is unchanged by keeping the true argument and that the new placement executes before token rendering.
6388-6392: Consider named constants for font-count and rotation mapping literals.The literals
4(font index max) and31/-2/2(rotation mapping) are meaningful domain constants and would be easier to maintain as named constants.Based on learnings: In WLED reviews, meaningful magic numbers should be replaced with defined constants to improve maintainability and reduce inconsistencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6388 - 6392, Replace the magic numbers used in font and rotation mapping with named constants: introduce a constant for the font index max (used in map(SEGMENT.custom2, 0, 255, 0, 4)) and constants for the rotation mapping input/output bounds (used in map(SEGMENT.custom3, 0, 31, -2, 2)); then update the expressions that compute fontNum, rotate and isRotated (references: fontNum, rotate, isRotated, SEGMENT.custom2, SEGMENT.custom3) to use those constants so the meanings (max font index and rotation mapping ranges) are explicit and maintainable.wled00/FX_2Dfcn.cpp (2)
629-629: Consider replacing magic number0x80with a named constant.The bit flag
0x80is used to distinguish file-based vs flash fonts in the cache ID. A named constant would improve readability.Suggested improvement
+// At file/class scope: +static constexpr uint8_t CACHE_ID_FILE_FLAG = 0x80; + - uint8_t cacheID = _fontNum | (_useFileFont ? 0x80 : 0x00); + uint8_t cacheID = _fontNum | (_useFileFont ? CACHE_ID_FILE_FLAG : 0x00);Based on learnings: "look for 'magic numbers' (hardcoded numeric literals) and suggest replacing them with appropriate defined constants."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_2Dfcn.cpp` at line 629, The cache ID assembly uses a magic literal 0x80; define a named constant (e.g., FONT_CACHE_FILE_FLAG or FONT_FLAG_FILE = 0x80) and replace the literal in the expression "uint8_t cacheID = _fontNum | (_useFileFont ? 0x80 : 0x00);" with that constant (and use 0x00 as a named zero flag if desired); add the constant near other font-related constants or at file scope and update any other occurrences of 0x80 used for this purpose to use the new constant for clarity (refer to _fontNum, _useFileFont, and cacheID to locate the change).
746-746: Consider heap allocation forwidthTableto avoid stack pressure.The VLA
uint8_t widthTable[numGlyphs]can be up to 256 bytes. While typically safe, this is called during effect rendering where stack depth may already be significant. Heap allocation would be safer on memory-constrained ESP devices.Alternative using heap
- uint8_t widthTable[numGlyphs]; + uint8_t* widthTable = new (std::nothrow) uint8_t[numGlyphs]; + if (!widthTable) { if (file) file.close(); return; } // ... use widthTable ... + delete[] widthTable; if (file) file.close();Or use a small fixed-size buffer since
numGlyphsmax is 256.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_2Dfcn.cpp` at line 746, The local VLA "uint8_t widthTable[numGlyphs]" can cause stack pressure; replace it with a heap allocation or a fixed max-size buffer: allocate with "uint8_t *widthTable = (uint8_t*)malloc(numGlyphs);" (check for nullptr and handle error) and free(widthTable) when done, or use a static/fixed buffer "static uint8_t widthTable[256];" and ensure numGlyphs <= 256 before writing; update the code paths that populate and read widthTable to use the pointer or fixed array accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX_2Dfcn.cpp`:
- Around line 788-793: The loop populating registry uses widthTable[code]
without validating code against numGlyphs; add a bounds check (e.g., if (code <
numGlyphs)) before accessing widthTable[code] in the loop that writes to
registry[k], registry[k].width, registry[k].height, and handle the else-case by
skipping the invalid code and adjusting how entries are written (either
decrement k to retry with the next valid code, maintain a separate write index
for registry, or reduce neededCount/track validCount) so you never read past the
VLA bounds and registry contains only valid entries.
In `@wled00/FX.cpp`:
- Around line 6472-6474: The rotated glyph height logic loses per-glyph width
because glyphWidth is forced to fontHeight when isRotated; change the order so
glyphWidth is always obtained from fontManager.getGlyphWidth(unicode) (do not
override it with fontHeight), and then set glyphHeight = isRotated ? glyphWidth
: fontHeight to swap dimensions for 90° rotation; update the same pattern at the
other occurrence (the block around glyphWidth/glyphHeight near the second
mention) referencing glyphWidth, glyphHeight, isRotated,
fontManager.getGlyphWidth(unicode), and fontHeight.
- Line 6489: The metadata string _data_FX_MODE_2DSCROLLTEXT currently labels the
control for custom2 as "Font size" which is outdated; update that token in the
semicolon/comma-separated metadata (the "...,Trail,Font size,Rotate,..." segment
inside the static const char _data_FX_MODE_2DSCROLLTEXT[] PROGMEM) to a clearer
label such as "Font" or "Font selection" to reflect that custom2 now selects a
font (mapped with map(..., 0, 4)); leave the rest of the string intact so UI
parsing continues to work.
---
Outside diff comments:
In `@wled00/FX.cpp`:
- Around line 6339-6341: The call to std::toupper uses raw char from
SEGMENT.name which is undefined for negative bytes; update the assignment in the
loop that sets token[j] to pass an unsigned char cast: call
std::toupper(static_cast<unsigned char>(SEGMENT.name[i + j])) (or C-style
(unsigned char)) so non-ASCII bytes are handled safely, leaving the rest of the
loop and variables (token, SEGMENT.name, i, j) unchanged.
---
Nitpick comments:
In `@wled00/FX_2Dfcn.cpp`:
- Line 629: The cache ID assembly uses a magic literal 0x80; define a named
constant (e.g., FONT_CACHE_FILE_FLAG or FONT_FLAG_FILE = 0x80) and replace the
literal in the expression "uint8_t cacheID = _fontNum | (_useFileFont ? 0x80 :
0x00);" with that constant (and use 0x00 as a named zero flag if desired); add
the constant near other font-related constants or at file scope and update any
other occurrences of 0x80 used for this purpose to use the new constant for
clarity (refer to _fontNum, _useFileFont, and cacheID to locate the change).
- Line 746: The local VLA "uint8_t widthTable[numGlyphs]" can cause stack
pressure; replace it with a heap allocation or a fixed max-size buffer: allocate
with "uint8_t *widthTable = (uint8_t*)malloc(numGlyphs);" (check for nullptr and
handle error) and free(widthTable) when done, or use a static/fixed buffer
"static uint8_t widthTable[256];" and ensure numGlyphs <= 256 before writing;
update the code paths that populate and read widthTable to use the pointer or
fixed array accordingly.
In `@wled00/FX.cpp`:
- Line 6331: fontManager.cacheNumbers(true) is being called inside the token
parsing hot path causing redundant work; move this call to a per-frame location
(e.g., at the start of the frame render/update routine that drives token
parsing) so numeric glyphs are cached once per frame instead of per token.
Locate the token-parsing code paths that call fontManager.cacheNumbers(true)
(the current occurrences including the one shown and the other calls around the
positions marked 6374-6375), remove those in-token calls, and add a single
fontManager.cacheNumbers(true) at the frame entry point (the function that
begins each render/update cycle) so caching happens once per frame. Ensure
behavior is unchanged by keeping the true argument and that the new placement
executes before token rendering.
- Around line 6388-6392: Replace the magic numbers used in font and rotation
mapping with named constants: introduce a constant for the font index max (used
in map(SEGMENT.custom2, 0, 255, 0, 4)) and constants for the rotation mapping
input/output bounds (used in map(SEGMENT.custom3, 0, 31, -2, 2)); then update
the expressions that compute fontNum, rotate and isRotated (references: fontNum,
rotate, isRotated, SEGMENT.custom2, SEGMENT.custom3) to use those constants so
the meanings (max font index and rotation mapping ranges) are explicit and
maintainable.
In `@wled00/src/font/font_5x12.h`:
- Line 48: The array initializer line in font_5x12.h (the byte header row
starting with 0x57, 0x0C, ...) is indented with 4 spaces; change its leading
indentation to 2 spaces to comply with the wled00 header-file style rule
(non-generated .h files use 2-space indentation). Locate the array/byte header
in font_5x12.h and adjust the whitespace at the start of that line (and any
adjacent non-generated header lines in the same file) from 4 spaces to 2 spaces
so the file matches the project's indentation guideline.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.cppwled00/FX_2Dfcn.cppwled00/src/font/font_5x12.hwled00/src/font/font_5x12.wbf
Comment on lines
+788
to
+793
| for (uint8_t k = 0; k < neededCount; k++) { | ||
| uint8_t code = neededCodes[k]; | ||
| registry[k].code = code; | ||
| registry[k].width = widthTable[code]; | ||
| registry[k].height = hdr.height; | ||
| } |
Contributor
There was a problem hiding this comment.
Bounds check missing: widthTable[code] access without validation.
At line 766, there's a bounds check if (code < numGlyphs) when calculating ramFontSize, but the registry population loop at lines 788-793 unconditionally accesses widthTable[code] without the same check. If code >= numGlyphs (possible with extended Unicode ranges or corrupted cache data), this reads beyond the VLA bounds.
Proposed fix
// write registry (GlyphEntry array)
GlyphEntry* registry = (GlyphEntry*)dataptr;
for (uint8_t k = 0; k < neededCount; k++) {
uint8_t code = neededCodes[k];
+ if (code >= numGlyphs) continue; // skip invalid codes
registry[k].code = code;
registry[k].width = widthTable[code];
registry[k].height = hdr.height;
}Note: If codes are skipped, you may also need to adjust neededCount or track valid entries separately to avoid gaps in the registry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX_2Dfcn.cpp` around lines 788 - 793, The loop populating registry
uses widthTable[code] without validating code against numGlyphs; add a bounds
check (e.g., if (code < numGlyphs)) before accessing widthTable[code] in the
loop that writes to registry[k], registry[k].width, registry[k].height, and
handle the else-case by skipping the invalid code and adjusting how entries are
written (either decrement k to retry with the next valid code, maintain a
separate write index for registry, or reduce neededCount/track validCount) so
you never read past the VLA bounds and registry contains only valid entries.
Comment on lines
+6472
to
+6474
| int glyphWidth = isRotated ? fontHeight : fontManager.getGlyphWidth(unicode); | ||
| int glyphHeight = isRotated ? glyphWidth : fontHeight; // use (variable) glyph-width for height if 90° rotated | ||
| int drawX = int(cols) - int(SEGENV.aux0) + currentXOffset; // aux0 is (scrolling) offset, no offset position is right side boarder (cols) |
Contributor
There was a problem hiding this comment.
Fix rotated glyph height calculation for variable-width fonts.
At Line 6473, glyphHeight in rotated mode is derived from glyphWidth, but glyphWidth is already forced to fontHeight. This loses per-glyph width and skews vertical centering/clipping for rotated variable-width fonts.
🔧 Proposed fix
- int glyphWidth = isRotated ? fontHeight : fontManager.getGlyphWidth(unicode);
- int glyphHeight = isRotated ? glyphWidth : fontHeight; // use (variable) glyph-width for height if 90° rotated
+ int unrotatedGlyphWidth = fontManager.getGlyphWidth(unicode);
+ int glyphWidth = isRotated ? fontHeight : unrotatedGlyphWidth;
+ int glyphHeight = isRotated ? unrotatedGlyphWidth : fontHeight; // use variable width as rotated heightAlso applies to: 6483-6483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 6472 - 6474, The rotated glyph height logic loses
per-glyph width because glyphWidth is forced to fontHeight when isRotated;
change the order so glyphWidth is always obtained from
fontManager.getGlyphWidth(unicode) (do not override it with fontHeight), and
then set glyphHeight = isRotated ? glyphWidth : fontHeight to swap dimensions
for 90° rotation; update the same pattern at the other occurrence (the block
around glyphWidth/glyphHeight near the second mention) referencing glyphWidth,
glyphHeight, isRotated, fontManager.getGlyphWidth(unicode), and fontHeight.
Contributor
There was a problem hiding this comment.
@DedeHai Thanks! Here’s a minimal fix that preserves per-glyph width when rotated 90°, while keeping the horizontal advance equal to fontHeight.
Patch (FX.cpp, mode_2Dscrollingtext draw loop; Lines around 6472–6484):
- int glyphWidth = isRotated ? fontHeight : fontManager.getGlyphWidth(unicode);
- int glyphHeight = isRotated ? glyphWidth : fontHeight; // use (variable) glyph-width for height if 90° rotated
+ int unrotatedGlyphWidth = fontManager.getGlyphWidth(unicode);
+ int glyphWidth = isRotated ? fontHeight : unrotatedGlyphWidth; // rotated: advance by fontHeight
+ int glyphHeight = isRotated ? unrotatedGlyphWidth : fontHeight; // rotated: height = original glyph widthThis keeps:
- advance = glyphWidth = fontHeight when rotated (correct), and
- vertical centering/clipping based on each glyph’s original (variable) width.
Let me know if you want me to prep a follow-up commit/PR with this change.
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 5372
File: wled00/FX.cpp:6395-6395
Timestamp: 2026-02-26T18:45:23.115Z
Learning: In WLED (repo wled/WLED), file wled00/FX.cpp, function mode_2Dscrollingtext(): the early return when fontManager.loadFont(...) fails is intentional to signal that a custom font wasn’t loaded (not that the effect failed). Do not replace this with a visual fallback that clears or renders with a default font.
Learnt from: DedeHai
Repo: wled/WLED PR: 5372
File: wled00/FX_2Dfcn.cpp:854-876
Timestamp: 2026-02-26T18:46:21.931Z
Learning: In WLED (repo wled/WLED), file wled00/FX_2Dfcn.cpp, class FontManager: null pointer checks are intentionally omitted in methods like getGlyphBitmap(), getGlyphWidth(), and drawCharacter() because the API contract requires callers to first call loadFont() and verify it returns true before using any other FontManager functions. This precondition is documented in the loadFont() comment.
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:149-151
Timestamp: 2026-01-24T18:51:09.740Z
Learning: In WLED's Spinning Wheel effect (usermods/user_fx/user_fx.cpp), the color scale calculation (SEGENV.aux1) must match the position modulo range. When position uses `pos = (pos_fixed >> 16) % SEGLEN`, the color scale must be `aux1 = (255 << 8) / SEGLEN` to distribute the palette evenly across the full position range. Using vstripLen for color scaling while position uses SEGLEN causes palette colors to cycle too quickly and appear incomplete.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
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.
in the current state, this only uses 1k of extra flash (thanks to optimized font bit packing)
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
Release Notes