Extend scrolling text FX with custom fonts and international UTF-8 character support

[DedeHai]

still a little rough around the edges and needs some refinement and testing of AI assisted code but basically working.

• change font format to a tightly bit packed format with a header
• added UTF-8 to unicode conversion functions (2, 3 and 4 byte codes supported)
• added support to read font files from file system (font.wbs, font1.wbs ... font4.wbs)
• added checkmark to use custom fonts - fallback to internal fonts if no fonts are found (backwads compatible)

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

• New Features
  • Custom font selection for 2D Scrolling Text.
  • Unicode/UTF‑8 text support for wider character rendering.
  • Dynamic font metrics for correct sizing, spacing and rotation.
  • Loading of fonts from storage with automatic discovery and fallback.
  • Number/token rendering cache for smoother, faster scrolling text.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

Walkthrough

This 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

Cohort / File(s)	Summary
Font System Infrastructure wled00/FX.h	Added FontManager public class, forward declarations, ByteReader abstraction (FlashByteReader, RAMByteReader), font-related structs (GlyphEntry, SegmentFontMetadata, FontHeader), and new Segment::drawCharacter overload accepting unicode codepoints and font data sources. Defines font format constants and font management API.
Font Implementation wled00/FX_2Dfcn.cpp	Implemented FontManager class with glyph indexing, header parsing, bitmap retrieval, font discovery/loading from storage, cache management, and character rendering. Replaced legacy font drawing with data-driven font system supporting dynamic glyph preparation and gradient coloring.
2D Scrolling Text Mode wled00/FX.cpp	Updated mode_2Dscrollingtext to integrate FontManager, replacing hard-coded letter sizing with dynamic font metrics. Added font selection logic, cache initialization for numeric tokens, and updated mode metadata string to expose Custom Font option.
UTF-8 Support wled00/fcn_declare.h, wled00/util.cpp	Added UTF-8 decoding utilities: utf8_decode() for parsing individual UTF-8 sequences and utf8_strlen() for counting UTF-8 characters. Includes UTF8_LEN macro for sequence length detection.
Font Data Format wled00/src/font/console_font_4x6.h	Transformed console_font_4x6 from legacy CP437 glyph table to WBF (WLED Bitmap Font) packed format with 12-byte header containing magic byte, dimensions, spacing, flags, character range, and unicode offset, followed by width table and bitmap streams.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Suggested reviewers

• willmmiles

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 43.75% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Merge Conflict Detection	⚠️ Warning	❌ Merge conflicts detected (26 files): ⚔️ package-lock.json (content) ⚔️ package.json (content) ⚔️ usermods/rgb-rotary-encoder/rgb-rotary-encoder.cpp (content) ⚔️ wled00/FX.cpp (content) ⚔️ wled00/FX.h (content) ⚔️ wled00/FX_2Dfcn.cpp (content) ⚔️ wled00/FX_fcn.cpp (content) ⚔️ wled00/bus_manager.cpp (content) ⚔️ wled00/bus_manager.h (content) ⚔️ wled00/bus_wrapper.h (content) ⚔️ wled00/cfg.cpp (content) ⚔️ wled00/const.h (content) ⚔️ wled00/data/settings_leds.htm (content) ⚔️ wled00/fcn_declare.h (content) ⚔️ wled00/file.cpp (content) ⚔️ wled00/network.cpp (content) ⚔️ wled00/set.cpp (content) ⚔️ wled00/src/font/console_font_4x6.h (content) ⚔️ wled00/src/font/console_font_5x12.h (content) ⚔️ wled00/src/font/console_font_5x8.h (content) ⚔️ wled00/src/font/console_font_6x8.h (content) ⚔️ wled00/src/font/console_font_7x9.h (content) ⚔️ wled00/util.cpp (content) ⚔️ wled00/wled.cpp (content) ⚔️ wled00/wled.h (content) ⚔️ wled00/xml.cpp (content) These conflicts must be resolved before merging into main.	Resolve conflicts locally and push changes to this branch.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately describes the main changes: extending the scrolling text FX with custom fonts and UTF-8 character support, which aligns with the substantial additions to font management, UTF-8 utilities, and font subsystem integration across all modified files.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/**/*.cpp should 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 on SEGENV.call == 0 or when fontNum/useCustomFont/text changes (the unused init flag 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-1 to 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 from getGlyphBitmap is only used as an offset base — document this contract.

The returned const uint8_t* is never dereferenced directly; drawCharacter computes (bitmap - _fontBase) + bytePos and 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.

[DedeHai]

The Font Factory tool:

---

Font pixel editor mode:

---

Built-in international chars support:

[DedeHai]

@coderabbitai please review the fontmanager and make suggesions for code consolidation and maintainability.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@DedeHai, I'll review the FontManager implementation and provide suggestions for code consolidation and maintainability.

---

🏁 Script executed:

#!/bin/bash
# Read the FontManager declarations from FX.h
echo "=== FontManager declarations in FX.h ==="
cat wled00/FX.h | grep -A 200 "class FontManager"

Length of output: 13262

---

🏁 Script executed:

#!/bin/bash
# Read the FontManager implementation from FX_2Dfcn.cpp
echo "=== FontManager implementation in FX_2Dfcn.cpp ==="
cat wled00/FX_2Dfcn.cpp | grep -B 5 -A 100 "FontManager::"

Length of output: 14394

---

🏁 Script executed:

#!/bin/bash
# Get the full context of ByteReader and related structures
echo "=== ByteReader and font structures ==="
cat wled00/FX.h | grep -B 5 -A 30 "class ByteReader"

Length of output: 1191

---

🏁 Script executed:

#!/bin/bash
# Check how FontManager is used in the scrolling text effect
echo "=== FontManager usage in mode_2Dscrollingtext ==="
cat wled00/FX.cpp | grep -B 10 -A 30 "FontManager"

Length of output: 1426

---

@DedeHai, I've completed a thorough review of the FontManager implementation. Here are my suggestions for code consolidation and maintainability:

🎯 Major Refactoring Opportunities

1. Extract File Path Management into a Helper Function

File name generation is duplicated in scanAvailableFonts() and rebuildCache():

// 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 rebuildCache() - Violates Single Responsibility

This method is ~100 lines and does too much. Extract into smaller, testable functions:

private:
  // Extract file opening with fallback logic
  File openFontFileWithFallback(uint8_t& fontNum);
  
  // Extract header reading
  bool readFontHeader(File& file, FontHeader& header);
  
  // Extract width table population
  bool populateWidthTable(File& file, const FontHeader& hdr, uint8_t* widthTable);
  
  // Extract bitmap copying
  bool copyGlyphBitmaps(File& file, const FontHeader& hdr, 
                        const uint8_t* widthTable, const uint8_t* neededCodes, 
                        uint8_t count, uint8_t* destPtr);

3. Reduce State Tracking Complexity

Instead of multiple boolean flags, use an enum for font state:

enum class FontState {
  Uninitialized,
  FlashFontReady,
  FileFontReady,
  CacheValid,
  Error
};

private:
  FontState _state = FontState::Uninitialized;

4. Simplify ByteReader Pattern

The current ByteReader abstraction with virtual dispatch may be overkill. Consider using a template-based approach or std::function for better performance:

// Alternative: Use templates to avoid virtual dispatch overhead
template<typename Reader>
bool parseHeaderTemplate(const Reader& reader, FontHeader& hdr) {
  // Same logic, but no vtable lookup
}

Or keep the current design but add a comment explaining why virtual dispatch is acceptable here (readability over micro-optimization).

5. Extract Glyph Registry Management

The registry lookup logic is repeated in getGlyphWidth() and getGlyphBitmap(). Create a helper:

private:
  // Returns nullptr if not found, otherwise returns pointer to entry
  GlyphEntry* findGlyphInRegistry(uint8_t code);

6. Improve Error Handling and Propagation

Many methods return bool but don't provide error details. Consider:

enum class FontError {
  None,
  FileNotFound,
  InvalidHeader,
  OutOfMemory,
  InvalidGlyph
};

private:
  FontError _lastError = FontError::None;

public:
  FontError getLastError() const { return _lastError; }
  const char* getErrorString() const; // Human-readable error

7. Decouple from Segment with Dependency Injection

The friend class declaration creates tight coupling. Instead, pass needed dependencies:

// Instead of accessing _segment->data directly, inject an interface
class IFontCache {
public:
  virtual bool allocateData(size_t size) = 0;
  virtual uint8_t* getData() = 0;
  virtual ~IFontCache() = default;
};

// Segment implements this interface
// FontManager depends on interface, not concrete Segment

8. Extract Constants and Magic Numbers

// In header
static constexpr uint8_t MAX_CACHED_GLYPHS = 64;
static constexpr uint8_t MAX_FONTS = 5;
static constexpr size_t FONT_NAME_BUFFER_SIZE = 16;
static constexpr char NUMBERS_FOR_CACHE[] = "0123456789:. ";

9. Consider Splitting FontManager

FontManager has at least 3 distinct responsibilities:

// Separate classes/namespaces:
class FontLoader;      // Handles loadFont, scanAvailableFonts, file I/O
class FontCache;       // Handles prepare, rebuildCache, cache management  
class FontRenderer;    // Handles drawCharacter, getGlyphBitmap
class GlyphIndexer;    // Handles getGlyphIndex calculations (could be static utility)

10. Add Defensive Checks and Assertions

uint8_t FontManager::getGlyphWidth(uint32_t unicode) {
  if (!_headerValid) {
    `#ifdef` WLED_DEBUG
    DEBUGFX_PRINTLN(F("getGlyphWidth called before header valid"));
    `#endif`
    return 0;
  }
  // ... rest of implementation
}

📊 Complexity Metrics

Current issues:

• Cyclomatic complexity: rebuildCache() is very high (~15+)
• Lines per method: Several methods exceed 50 lines
• Class coupling: High coupling with Segment via friend access
• Cohesion: Low - font loading, caching, and rendering should be separate concerns

🔧 Quick Wins (High Impact, Low Effort)

1. Extract getFontFileName() helper (5 minutes)
2. Replace magic strings/numbers with named constants (10 minutes)
3. Add error code tracking (15 minutes)
4. Extract findGlyphInRegistry() helper (10 minutes)
5. Add debug assertions for preconditions (15 minutes)

💡 Long-term Recommendations

1. Consider moving to a Strategy Pattern for flash vs. file fonts
2. Implement RAII wrappers for File handling to prevent leaks
3. Add unit tests for pure functions like getGlyphIndex(), collectNeededCodes()
4. Profile the virtual dispatch in ByteReader - if hot path, consider template approach
5. Document the font format and cache structure more thoroughly in comments

The code is functional but could benefit from these refactorings to improve testability, reduce duplication, and make future maintenance easier. I recommend tackling the "Quick Wins" first, then considering the larger structural changes as time permits.

Would you like me to elaborate on any specific suggestion or help you implement one of these refactorings?

---

🧠 Learnings used

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.