Refactor

[bettio]

Address the excessive code duplication across AtomGL display
drivers by collapsing each hardware family into one driver,
dispatched via a compatible-string lookup that returns a
per-controller descriptor.

• DCS LCD: 3 drivers (ili934x, ili948x, st7789) -> 1.
• E-paper: 2 drivers (5in65_acep_7c, gdep073e01) -> 1.
• OLED I2C: if/else branching on 3 variants -> descriptor table.
• Sharp memory LCD: file-static state -> embedded state.

Bytes-on-the-wire and existing compatible strings are
unchanged. A new compatible is now ~30 lines of descriptor data,
no new C code. Zero file-statics in any ESP32 driver, so
multi-instance is unblocked.

Also adds support for the Good Display 7.3" 7-color e-paper
panel; use compatible string good-display/gdep073e01.

[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

[bettio]

per AMP:

PR Review — Branch pr/12 (52 commits)

I fixed all review comments, except:

• Port creation succeeds despite init failure (this was already an existing issue)
• Missing malloc NULL checks, mostly fixed (except parts that intersect "Port creation succeeds despite init failure")

[petermm]

Final PR Review — Branch pr/12

Reviewer: Amp (AI-assisted)
Date: 2026-05-02
Scope: 52 original commits + 7 review-fixup commits, ~4100 lines added / ~4500 removed across 46 files
Verdict: ✅ Approve / merge, with two small follow-up tickets noted at the end.

---

What this PR does

A major architectural refactor of the AtomGL display driver library:

1. Boilerplate consolidation — per-driver mailbox handling, message queues, and process_messages loops moved into shared display_task / display_message / display_items modules.
2. Data-driven init sequences — replaces per-controller if/else ladders with descriptor tables and compact [CMD][FLAGS_LEN][DATA…][DELAY] byte arrays for OLED (SSD1306/SSD1315/SH1106), DCS LCD (ILI9341/9342C/9486/9488/ST7789/ST7796), and e-paper (ACeP 5.65" / GDEP073E01). _Static_assert(sizeof(...)) guards every sequence.
3. uFontLib custom font support added to all ESP32 drivers (previously SDL-only).
4. Bug fixes carried in the original 52 commits: off-by-one in draw_scaled_cropped_img_x, e-paper memory leaks in do_update, ufont allocation-failure crash, inverted IFF magic check (!ufont_iff_is_valid_ufl → ufont_iff_is_valid_ufl), ufont text ignoring background color.
5. Legacy drivers removed: 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.

---

Review fixup commits — what landed after first review

Commit	Issue priority	Summary	Status
a3a61e3	🟡 P1	Off-by-one in draw_pixel_x: > → >= in mono_draw.c and epaper_draw.c	✅ Verified
176acab	🟢 P2	ufont_parse: guard against !iff_binary || buf_size < 12	✅ Verified
0c78709	🟢 P2	Typo: comptaible → compatible in display_driver.c log	✅ Verified
e753bc8	🔴 P0	display_items_init_item: memset(item, 0, sizeof(*item)) on entry → PrimitiveInvalid (= 0) is the safe default; also plugs text and surface.buffer leaks	✅ Verified
6c0e995	🔴 P0	register_font: deep-copy font binary into a heap-owned buffer before ufont_parse. Resolves UAF after the gen_call message is disposed.	✅ Verified
9bd2e1e	🟡 P1	Pre-ack update / draw_buffer at enqueue time so callers never time out when the drop-oldest queue evicts a stale frame. Drivers add return to skip duplicate replies.	✅ Verified
b01a698	🟢 P2	NULL-check per-frame allocations in do_update / clear_screen / dcs_lcd_draw_buffer across all four ESP32 drivers; fixes OLED leak-on-i2c-failure	✅ Verified

Notes on the pre-ack design (9bd2e1e)

The "reply at enqueue" approach is cleaner than the original "use backpressure" suggestion: it preserves the frame-loss-tolerant intent of the drop-oldest queue while ensuring gen_server:call callers never block. Two correctness checks pass:

• Each driver's process_message adds return after the update branch and after draw_buffer so the post-switch OK_ATOM reply tuple is not sent twice.
• try_pre_ack_render_cmd matches only update / draw_buffer. register_font and load_image keep their synchronous reply because they must convey the operation outcome.

---

Architecture highlights (positive)

• CONTAINER_OF pattern for recovering each driver struct from ctx->platform_data is clean and avoids unsafe casts.
• Init sequence framing is well-chosen: DCS LCD uses a 0x00 end sentinel (safe — NOP isn't used as init), e-paper and OLED use length-framing (necessary because 0x00 is PSR / a valid init command on Waveshare/GoodDisplay parts). _Static_assert(sizeof(...)) catches byte-counting mistakes at compile time.
• Descriptor tables capture all controller variation as pure data; no function pointers needed across the current set.
• display_task.c cleanly abstracts the FreeRTOS task+queue pattern with a per-driver function-pointer for process_message, plus generic interception of register_font.

---

Outstanding follow-ups

The contributor explicitly deferred two items. Both are acceptable for merge; both deserve a tracking ticket.

Follow-up #1 — Per-task ufont_manager (was 🔴 P0, partial)

File: display_task.c

ufont_manager is still a process-global that every display task overwrites in display_task_process_messages line 148:

UFontManager *ufont_manager;                          // line 33

void display_task_process_messages(void *arg)
{
    struct DisplayTaskArgs *args = arg;

    ufont_manager = ufont_manager_new();              // line 148 — last task wins
    ...
}

display_items.c reads the same global:

#ifdef ENABLE_UFONT
#include "ufontlib.h"
extern UFontManager *ufont_manager;                   // display_items.c:31

Why this is fine for now: in the field, only one display port is created per VM, so the race is latent. The deep-copy fix already eliminates the use-after-free, which was the user-visible bug.

Why it should still be fixed eventually: any future use case with two displays (e.g. e-paper + small status OLED) will silently lose font registrations from the first-started task.

Suggested fix — embed the manager in DisplayTaskArgs:

--- a/display_task.h
+++ b/display_task.h
@@ -27,11 +27,16 @@
 #include <context.h>
 #include <mailbox.h>

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

 NativeHandlerResult display_task_consume_mailbox(Context *ctx);

--- a/display_task.c
+++ b/display_task.c
@@ -30,8 +30,6 @@
 #include "display_message.h"
 #include "ufontlib.h"

-UFontManager *ufont_manager;
-
 static bool try_pre_ack_render_cmd(Message *message, Context *ctx)
 {
     ...
@@ -127,7 +125,8 @@ static bool try_handle_register_font(Message *message, Context *ctx)
     char *handle = interop_atom_to_string(ctx,
             term_get_tuple_element(req, 1));
     if (loaded_font != NULL && handle != NULL) {
-        ufont_manager_register(ufont_manager, handle, loaded_font);
+        struct DisplayTaskArgs *args = ctx->platform_data;
+        ufont_manager_register(args->ufont_manager, handle, loaded_font);
     }
     free(handle);

@@ -145,7 +144,7 @@ void display_task_process_messages(void *arg)
 {
     struct DisplayTaskArgs *args = arg;

-    ufont_manager = ufont_manager_new();
+    args->ufont_manager = ufont_manager_new();

     while (true) {
         Message *message;

--- a/display_items.c
+++ b/display_items.c
@@ -28,8 +28,7 @@
 #include <interop.h>

 #ifdef ENABLE_UFONT
-#include "ufontlib.h"
-extern UFontManager *ufont_manager;
+#include "display_task.h"

 struct Surface
 ...
@@ -178,8 +177,9 @@ void display_items_init_item(BaseDisplayItem *item, term req, Context *ctx)
         } else {
 #ifdef ENABLE_UFONT
             char *handle = interop_atom_to_string(ctx, font);
+            struct DisplayTaskArgs *args = ctx->platform_data;
             EpdFont *loaded_font = NULL;
             if (handle != NULL) {
-                loaded_font = ufont_manager_find_by_handle(ufont_manager, handle);
+                loaded_font = ufont_manager_find_by_handle(args->ufont_manager, handle);
                 free(handle);
             }

Effort: S (≈1h, one focused commit). No new tests needed — existing single-display path keeps working byte-identically.

---

Follow-up #2 — Context teardown on init failure (was 🟡 P1, deferred)

The contributor flagged this themselves in the b01a698 commit message:

"Init-time allocations in *_create_port / display_init / display_spi_init remain unchecked; context-teardown-on-init-failure is tracked as a separate follow-up."

Symptom: each *_create_port() always returns a live Context *, even when display_init() hit an early return (invalid compatible string, missing GPIO, I2C/SPI failure, unsupported rotation). Callers receive a port with:

• no worker xTaskCreate,
• partial / NULL platform_data,
• leaked screen buffers, queues, and SPI device handles.

Subsequent mailbox traffic to that port silently hangs (the new pre-ack does not run because the worker never started).

Affected files:

• dcs_lcd_display_driver.c — display_init has 4 early returns
• epaper_display_driver.c — display_spi_init has 2 early returns
• oled_display_driver.c — display_init has 2 early returns
• memory_display_driver.c — implicit, only one failure path

Suggested approach — make init return bool, return NULL from *_create_port on failure:

--- a/dcs_lcd_display_driver.c
+++ b/dcs_lcd_display_driver.c
-static void display_init(Context *ctx, term opts);
+static bool display_init(Context *ctx, term opts);

 Context *dcs_lcd_display_create_port(GlobalContext *global, term opts)
 {
     Context *ctx = context_new(global);
     ctx->native_handler = display_task_consume_mailbox;
-    display_init(ctx, opts);
+    if (!display_init(ctx, opts)) {
+        context_destroy(ctx);
+        return NULL;
+    }
     return ctx;
 }

Then convert each return; in display_init() to goto cleanup; (or equivalent), and add a single cleanup tail:

static bool display_init(Context *ctx, term opts)
{
    struct DCSLCDDriver *driver = NULL;
    bool ok = false;

    /* … existing init flow, on any failure: goto cleanup … */

    ok = true;

cleanup:
    if (!ok && driver != NULL) {
        if (driver->display_args.messages_queue) {
            vQueueDelete(driver->display_args.messages_queue);
        }
        free(driver->screen.pixels);
        free(driver->screen.pixels_out);
        free(driver->screen.bytes);
        free(driver->screen.bytes_out);
        /* spi_bus_remove_device(driver->bus.spi_disp.handle) if init'd */
        free(driver);
    }
    return ok;
}

Apply the same shape to the other three drivers. The dispatcher in display_driver.c already treats NULL from a *_create_port as failure (the parent display_create_port returns NULL directly when no compatible matches), so no caller change is needed.

Effort: M (≈half a day, four similar commits). Worth doing because today this is the single class of "silent hang on misconfiguration" most likely to confuse new users wiring up a panel.

---

Items still in scope but explicitly non-blocking

These are nice-to-haves noted in the original review and not addressed in fixups:

• display_driver.c duplicates the family-specific compatible strings as an if/else chain. A single registration table would be cleaner but is fine for the current 11-controller set.
• display_items.h:31-34 keeps a // TODO: deprecated helper context_make_atom wrapper. Can be inlined as a follow-up.
• dcs_lcd_draw.c:140 has a pixmem32 variable that holds 16-bit pixels — a misleading carry-over name; trivial rename:

--- a/dcs_lcd_draw.c
+++ b/dcs_lcd_draw.c
@@ -137,7 +137,7 @@ int dcs_lcd_draw_text_x(const struct DCSLCDScreen *screen,

     int drawn_pixels = 0;

-    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));

     if (width > xpos - x + max_line_len) {
         width = xpos - x + max_line_len;
@@ -159,9 +159,9 @@ int dcs_lcd_draw_text_x(const struct DCSLCDScreen *screen,
         }

         if (opaque) {
-            pixmem32[drawn_pixels] = fgcolor;
+            pixmem16[drawn_pixels] = fgcolor;
         } else if (visible_bg) {
-            pixmem32[drawn_pixels] = bgcolor;
+            pixmem16[drawn_pixels] = bgcolor;
         } else {
             return drawn_pixels;
         }

---

Recommendation

✅ Merge pr/12. All five P0/P1 issues from the first review are resolved or have a clear, low-risk path forward. The two outstanding items (per-task ufont_manager, init-failure teardown) should be filed as follow-up tickets but do not block.

Suggested ticket titles:

1. display_task: per-task ufont_manager (multi-display safety)
2. display drivers: clean teardown on init failure (return NULL from *_create_port)