Refactor

[bettio]

Reorganizes AtomGL display drivers into one unified driver per
hardware family, each dispatched via a compatible-string lookup
returning a per-controller descriptor. Bytes-on-the-wire are
unchanged for every previously supported compatible.

Before → After

Family	Before	After
DCS LCD	3 per-controller drivers (ili934x, ili948x, st7789) covering 6 compatibles	1 unified driver (dcs_lcd_display_driver.c) + DCSLCDDesc descriptor table
E-paper	2 per-panel drivers (5in65_acep_7c, gdep073e01) covering 2 compatibles	1 unified driver (epaper_display_driver.c) + EPaperDesc descriptor table
OLED (I2C)	1 driver with if/else branching on 3 variants	1 driver (oled_display_driver.c) + OLEDDesc descriptor table
Sharp memory LCD	1 driver with file-static screen state	1 driver with embedded state

What every driver gets for free

• clock_speed_hz Erlang opt: override the descriptor's default
  SPI clock rate.
• width, height, x_offset, y_offset, color_order Erlang
  opts apply uniformly to every DCS LCD / e-paper / memory LCD
  compatible.
• init_list Erlang-side init override now applies to every DCS
  LCD and e-paper compatible (extended with
  {wait_busy_level, 0|1} for e-paper).
• Reset GPIO is optional on every DCS LCD controller.
• A new compatible is ~30 lines of descriptor data, no new C code
  path required.
• Multi-instance prep: zero file-statics in any ESP32 driver; each
  driver's state travels with its CONTAINER_OF-recoverable
  struct.

Hardware validation

Hardware-confirmed on real panels with the demo app and a
regression test scene:

• ilitek,ili9341 (TFT RGB565)
• sitronix,st7789 (TFT RGB565)
• waveshare,5in65-acep-7c (7-color e-ink)
• good-display/gdep073e01 (7-color e-ink)
• sharp,memory-lcd (monochrome SPI)

Build-only validation (no panels in the test rig):

• solomon-systech,ssd1306, solomon-systech,ssd1315,
  sino-wealth,sh1106 (OLED variants)
• ilitek,ili9342c, ilitek,ili9486, ilitek,ili9488,
  sitronix,st7796 (DCS LCD variants)

For the OLED family, byte-for-byte equivalence of the post-refactor
init sequences was verified via _Static_assert size guards on the
init arrays and manual transcription review against the pre-refactor
inline code. Hardware validation on real OLED panels should precede
relying on this driver in production.

Preserved behaviour

• Every previously-supported compatible string still works.
• Existing applications pass their opts exactly as before.
• Bytes-on-the-wire unchanged for every controller.
• Display-list semantics, scanline rendering, dithering, and alpha
  blending are unchanged.

Follow-ups (deferred, not blocking)

• Hardware validation of the OLED variants
  (SSD1306/SSD1315/SH1106).
• Rotations 2/3 validation on ILI9341/ILI9342C/ST7789/ST7796 —
  descriptor madctl[] tables carry 0xFF sentinels where the
  pre-refactor drivers didn't support those rotations.
• Multi-instance hardware test (two panels on one ESP32).
• init_list Erlang override for OLED (parity with e-paper).
• Migrate DCS LCD init sequences to length-based framing (e-paper
  already uses this).
• Bump ILI SPI clock from 27 MHz to 40 MHz after per-board IOMUX
  validation.
• Strip BGR bit from ILI MADCTL tables so color_order: rgb works
  cleanly on ili9341 / ili9342c.
• Graceful failure for unknown commands in all drivers
  (SDL-style).
• Switch ufont rendering surface from 32bpp RGBA to 4bpp alpha-only
  (drops a 921×63 glyph from ~227KB to ~29KB).

Test plan

• idf.py build on the full AtomVM tree.
• Hardware smoke test on each available panel (ILI9341, ST7789,
  Waveshare 5.65", GDEP073E01, Sharp memory LCD) with the demo
  app and the regression test scene.
• Hardware validation of SSD1306/SSD1315/SH1106 — follow-up,
  flagged above.

[petermm]

per AMP:

PR Review — Branch pr/12 (52 commits)

Reviewer: Amp (AI-assisted)
Date: 2026-04-17
Scope: ~4100 lines added, ~4500 removed across 46 files

---

Summary

This PR is a major architectural refactor that:

1. Consolidates duplicated per-driver boilerplate (message queues, mailbox consumption, process_messages loops, send_message helpers) into shared modules: display_task, display_message, display_items.
2. Replaces per-driver if/else init code with data-driven descriptor tables and compact byte-array init sequences for OLED (SSD1306/SSD1315/SH1106), DCS LCD (ILI9341/9342C/9486/9488/ST7789/ST7796), and e-paper (ACeP 5.65", GDEP073E01).
3. Adds uFontLib custom font support to all ESP32 drivers (previously SDL-only).
4. Fixes several bugs: off-by-one in draw_scaled_cropped_img_x, memory leaks in e-paper do_update, ufont crash on allocation failure, inverted IFF validation logic in ufont_parse, ufont text ignoring background color.
5. Removes legacy drivers: ili934x_display_driver.c, ili948x_display_driver.c, st7789_display_driver.c, 5in65_acep_7c_display_driver.c, draw_common.h, monochrome.h, message_helpers.h.

Overall assessment: The refactoring direction is excellent — it significantly reduces code duplication and makes adding new display controllers trivial. The init-sequence table design is sound (with _Static_assert guardrails). Several issues should be addressed before or shortly after merge.

---

Architecture Highlights (Positive)

• CONTAINER_OF pattern for recovering the driver struct from ctx->platform_data is clean and avoids unnecessary casts.
• Init sequence format is well-designed: DCS LCD uses a sentinel 0x00 end marker (safe since NOP is not a real init command), while e-paper and OLED use length-framing (because 0x00 is PSR on Waveshare/GoodDisplay controllers). The _Static_assert on sequence sizes is a strong guardrail against byte-counting errors.
• Descriptor tables (OLEDDesc, DCSLCDDesc, EPaperDesc) capture all controller variation as pure data, eliminating function-pointer complexity.
• display_task.c cleanly abstracts the FreeRTOS task+queue pattern with a function-pointer callback for per-driver message handling, plus intercepts register_font generically.

---

Issues

🔴 P0 — uFont Global State & Lifetime

File: display_task.c:33, display_items.c:31

ufont_manager is a global variable shared across all display drivers. Each call to display_task_process_messages overwrites it (line 106). With multiple display ports, the last task to start wins and earlier registrations are lost.

Additionally, ufont_parse() stores raw pointers into the passed buffer (ufontlib.c), but the font binary comes from an Erlang message that is disposed immediately after try_handle_register_font returns (line 116-118). If the AtomVM runtime reclaims that memory, all registered fonts become use-after-free.

Recommendation: Make the font manager per-task (embed in DisplayTaskArgs), and deep-copy the font binary before parsing:

// display_task.h
struct DisplayTaskArgs
{
    QueueHandle_t messages_queue;
    void (*process_message_fn)(Message *message, Context *ctx);
    Context *ctx;
#ifdef ENABLE_UFONT
    UFontManager *ufont_manager;
#endif
};

// display_task.c — in try_handle_register_font:
size_t sz = term_binary_size(font_bin);
void *owned = malloc(sz);
if (!owned) { /* reply error, return true */ }
memcpy(owned, term_binary_data(font_bin), sz);
EpdFont *loaded_font = ufont_parse(owned, sz);
if (!loaded_font) { free(owned); /* reply error */ }

---

🔴 P0 — Uninitialized Display Items on Error Paths

File: display_items.c

display_items_init_item() sets only owns_data = false at entry (line 69), then has multiple early returns on error (unsupported format: lines 93-96, 128-132; invalid text: 163-165; failed ufont alloc: 201-205; failed ufont draw: 217-219). The caller (do_update) still includes these items in the render loop, which reads uninitialized primitive, width, height, x, y fields.

Suggested fix — initialize sentinel at function entry:

 void display_items_init_item(BaseDisplayItem *item, term req, Context *ctx)
 {
+    item->primitive = PrimitiveInvalid;
+    item->x = -1;
+    item->y = -1;
+    item->width = 0;
+    item->height = 0;
+    item->brcolor = 0;
     item->owns_data = false;
+    memset(&item->data, 0, sizeof(item->data));
+    item->source_x = item->source_y = 0;
+    item->x_scale = item->y_scale = 1;

Also: lines 186-190 leak text on the "unsupported font" path, and line 217-219 leaks surface.buffer on draw failure.

---

🟡 P1 — Silent Message Loss on Queue Overflow

File: display_task.c:42-57

When the queue is full, display_task_consume_mailbox drops the oldest message (disposing it) to make room. For gen_call messages, the caller's Erlang process will block forever waiting for a reply that never comes.

// Current: silently drops oldest message
if (xQueueSend(args->messages_queue, &msg, 0) != pdTRUE) {
    Message *old = NULL;
    if (xQueueReceive(args->messages_queue, &old, 0) == pdTRUE && old) {
        // ... dispose old, no reply sent ...
    }

Recommendation: Either use backpressure (block with a timeout) or send an error reply before disposing:

// Option A: backpressure (simplest, matches old behavior)
xQueueSend(args->messages_queue, &msg, portMAX_DELAY);

---

🟡 P1 — Port Creation Succeeds Even When Init Fails

Files: dcs_lcd_display_driver.c:272-278, epaper_display_driver.c:415-421, oled_display_driver.c (equivalent)

Each *_create_port() function creates a Context, calls display_init(), and returns the context unconditionally. But display_init() has multiple early return paths (invalid compat string, bad GPIOs, unsupported rotation, I2C/SPI failure). On any of these, the caller receives a live port with no worker task, a partial driver struct, and leaked allocations.

Recommendation: Have display_init() return bool and return NULL from *_create_port() on failure, freeing any partial allocations.

---

🟡 P1 — Pixel Write Boundary Check Off-by-One

Files: mono_draw.c:71, epaper_draw.c:37

Both use > instead of >=:

// mono_draw.c:71
if (xpos > screen->w) {   // allows write at xpos == w (out of bounds)

// epaper_draw.c:37
if (xpos > screen->w) {   // same issue

Fix:

-    if (xpos > screen->w) {
+    if (xpos >= screen->w) {

---

🟢 P2 — Typo in Error Message

File: display_driver.c:73

ESP_LOGE(TAG, "No matching display driver for given `comptaible`: `%s`.", compat_string);

Fix:

-    ESP_LOGE(TAG, "No matching display driver for given `comptaible`: `%s`.", compat_string);
+    ESP_LOGE(TAG, "No matching display driver for given `compatible`: `%s`.", compat_string);

---

🟢 P2 — Missing malloc/heap_caps_malloc NULL Checks

Several allocations are unchecked:

File	Line(s)	Allocation
dcs_lcd_display_driver.c	312, 318-319, 321-322	calloc(driver), heap_caps_malloc(pixels/pixels_out), heap_caps_malloc(bytes/bytes_out)
dcs_lcd_commands.c	64, 88	heap_caps_malloc(tmpbuf) for DMA transfer
epaper_display_driver.c	205, 295, 345	heap_caps_malloc(buf), malloc(driver)
oled_display_driver.c	~118	malloc(buf) in do_update — leaks items on early return

On ESP32 with limited heap, heap_caps_malloc(MALLOC_CAP_DMA) can realistically fail.

---

🟢 P2 — oled_display_driver.c Leaks buf and items on I2C Failure

File: oled_display_driver.c (do_update), around line 124

if (i2c_driver_acquire(driver->i2c_host, &i2c_num, ctx->global) != I2CAcquireOk) {
    fprintf(stderr, "Invalid I2C peripheral\n");
    return;  // leaks: buf, items (and items' owned data)
}

Fix:

 if (i2c_driver_acquire(driver->i2c_host, &i2c_num, ctx->global) != I2CAcquireOk) {
     fprintf(stderr, "Invalid I2C peripheral\n");
+    free(buf);
+    display_items_delete(items, len);
     return;
 }

---

🟢 P2 — dcs_lcd_draw.c:140 — Misleading Variable Name

uint16_t *pixmem32 = (uint16_t *) ...;  // named "32" but is actually 16-bit

Carried over from the old st7789_display_driver.c. Trivial rename:

-    uint16_t *pixmem32 = (uint16_t *) (((uint8_t *) screen->pixels) + xpos * sizeof(uint16_t));
+    uint16_t *pixmem16 = (uint16_t *) (((uint8_t *) screen->pixels) + xpos * sizeof(uint16_t));

(And update the two references at lines 162, 164.)

---

🟢 P2 — uFont Parser Minimum Size Guard

File: ufontlib.c:602

ufont_parse() reads at offsets 0 and 8 without checking buf_size >= 12:

 EpdFont *ufont_parse(const void *iff_binary, int buf_size)
 {
+    if (!iff_binary || buf_size < 12) {
+        return NULL;
+    }
     if (!ufont_iff_is_valid_ufl(iff_binary)) {
         return NULL;
     }

---

Design Notes (Non-blocking)

find_max_line_len receives i not items_len — intentional

In all draw modules (dcs_lcd_draw.c:243, epaper_draw.c:308, mono_draw.c:333), the call passes i (current item index) as the length argument:

int max_line_len = below ? 1 : dcs_lcd_find_max_line_len(screen, items, i, xpos, ypos);

This is intentional: it scans only items with lower index (higher z-order / painted first) to determine the maximum contiguous pixel run before hitting an overlapping item. The parameter name items_len in the function signature is somewhat misleading but the logic is correct and matches the SDL reference driver.

Duplicate compatible-string tables

display_driver.c duplicates the family-specific compatible tables (dcs_lcd_compat_table, oled_compat_table, epaper_compat_table) as an if/else chain. This creates a maintenance burden — adding a new controller requires editing two places. A future improvement could be a single registration table, but this is fine for now.

context_make_atom helper

display_items.h:31-34 keeps a // TODO: deprecated helper wrapper. Consider removing it and inlining globalcontext_make_atom at call sites as a follow-up.

---

Summary of Recommendations

Priority	Issue	Effort
🔴 P0	uFont global state / use-after-free	Medium
🔴 P0	Uninitialized display items on error paths	Small
🟡 P1	Silent message loss (queue overflow)	Small
🟡 P1	Port creation succeeds despite init failure	Medium
🟡 P1	Pixel write off-by-one (> → >=)	Trivial
🟢 P2	Typo comptaible	Trivial
🟢 P2	Missing malloc NULL checks	Small
🟢 P2	OLED do_update leaks on I2C failure	Trivial
🟢 P2	Misleading pixmem32 name	Trivial
🟢 P2	uFont parser min-size guard	Trivial