diff --git a/.skills/SKILL.md b/.skills/SKILL.md index 8e5e8f8f..b7b59dc7 100644 --- a/.skills/SKILL.md +++ b/.skills/SKILL.md @@ -58,6 +58,7 @@ find src -name "*.cpp" -o -name "*.h" | xargs clang-format -i 6. `constexpr` First: Compile-time constants and lookup tables must be `constexpr`, not just `static const`. This moves computation to compile time, enables dead-branch elimination, and guarantees flash placement. Use `static constexpr` for class-level constants. 7. `std::vector` Pre-allocation: Always call `.reserve(N)` before any `push_back()` loop. Each growth event allocates a new block (2×), copies all elements, then frees the old one — three heap operations that fragment DRAM. When the final size is unknown, estimate conservatively. 8. SPIFFS Write Throttling: Never write a settings file on every user interaction. Guard all writes with a value-change check (`if (newVal == _current) return;`). Progress saves during reading must be debounced — write on activity exit or every N page turns, not on every turn. SPIFFS sectors have a finite erase cycle limit. +9. `new` is not nothrow on ESP32: With `-fno-exceptions`, bare `new` that fails calls `abort()` — it does NOT return `nullptr`. Always use `new (std::nothrow)` and null-check the result, or use `makeUniqueNoThrow()` from `lib/Memory/Memory.h`. Never write bare `new` for any fallible allocation. --- @@ -266,43 +267,78 @@ When a template is necessary, limit instantiations: use explicit template instan **Rules**: NO exceptions, NO abort(), ALWAYS log before error return -### Acceptable malloc/free Patterns +### Heap Buffer Allocation -**Source**: [src/activities/home/HomeActivity.cpp:166](../src/activities/home/HomeActivity.cpp), [lib/GfxRenderer/GfxRenderer.cpp:439-440](../lib/GfxRenderer/GfxRenderer.cpp) +**Prefer `makeUniqueNoThrow` over `malloc`.** Both are nothrow (return `nullptr` on OOM rather than calling `abort()`), but `malloc` requires a manual `free` on every return path — a common source of leaks. `makeUniqueNoThrow(size)` from `lib/Memory/Memory.h` frees automatically when it goes out of scope. -Despite "prefer stack allocation," malloc is acceptable for: -1. **Large temporary buffers** (> 256 bytes, won't fit on stack) -2. **One-time allocations** during activity initialization -3. **Bitmap rendering buffers** (variable size, used briefly) - -**Pattern**: +**Preferred pattern**: ```cpp -// Allocate -auto* buffer = static_cast(malloc(bufferSize)); +#include + +auto buffer = makeUniqueNoThrow(bufferSize); if (!buffer) { - LOG_ERR("MODULE", "malloc failed: %d bytes", bufferSize); - return false; // Handle allocation failure + LOG_ERR("MODULE", "OOM: %d bytes", bufferSize); + return false; } -// Use buffer -processData(buffer, bufferSize); +processData(buffer.get(), bufferSize); +// freed automatically — no manual free needed, no leak on early return +``` -// Free immediately after use -free(buffer); -buffer = nullptr; +**`malloc` or `new (std::nothrow)` are still acceptable** when the buffer must be passed to a C API that takes ownership and frees it itself (e.g., certain SDK callbacks). In that case follow the manual pattern: +```cpp +auto* buffer = static_cast(malloc(bufferSize)); // or new (std::nothrow) uint8_t[bufferSize] +if (!buffer) { + LOG_ERR("MODULE", "OOM: %d bytes", bufferSize); + return false; +} +sdkApiThatTakesOwnership(buffer, bufferSize); // SDK calls free() / delete[] ``` **Rules**: -- **ALWAYS check for nullptr** after malloc -- **Free immediately** after use (don't hold across multiple operations) -- **Set to nullptr** after free (avoid use-after-free) -- **Document size**: Comment why stack allocation was rejected +- **Prefer `makeUniqueNoThrow`** — automatic cleanup eliminates leak risk on error paths +- **ALWAYS check for nullptr** after any allocation and `LOG_ERR` before returning false +- **Raw allocation only** when a C API takes ownership; document why in a comment **Examples in codebase**: +- Memory utilities: [Memory.h](../lib/Memory/Memory.h) (`makeUniqueNoThrow`) - Cover image buffers: [HomeActivity.cpp:166](../src/activities/home/HomeActivity.cpp) -- Text chunk buffers: [TxtReaderActivity.cpp:259](../src/activities/reader/TxtReaderActivity.cpp) - Bitmap rendering: [GfxRenderer.cpp:439-440](../lib/GfxRenderer/GfxRenderer.cpp) -- OTA update buffer: [OtaUpdater.cpp:40](../src/network/OtaUpdater.cpp) + +### Heap Allocation with `new`: Always Use `makeUniqueNoThrow` + +**CRITICAL**: With `-fno-exceptions`, bare `new` on OOM calls `abort()` — it does NOT return `nullptr`. Always use `makeUniqueNoThrow` from `lib/Memory/Memory.h`, which wraps `new (std::nothrow)` and returns a `std::unique_ptr` that is null on OOM and automatically frees on scope exit. + +**Preferred pattern**: +```cpp +#include + +auto obj = makeUniqueNoThrow(args); +if (!obj) { LOG_ERR("MOD", "OOM: MyClass"); return false; } + +auto buf = makeUniqueNoThrow(size); +if (!buf) { LOG_ERR("MOD", "OOM: %d bytes", size); return false; } + +// Pass to C APIs via .get(); unique_ptr frees automatically on return +someApi(buf.get(), size); +``` + +**`new (std::nothrow)` directly is acceptable** when the object must be passed to a C API that takes ownership and calls `delete` itself: +```cpp +auto* obj = new (std::nothrow) MyClass(args); +if (!obj) { LOG_ERR("MOD", "OOM: MyClass"); return false; } +sdkApiThatTakesOwnership(obj); // SDK calls delete +``` + +**Rules**: +- **Prefer `makeUniqueNoThrow`** — automatic cleanup eliminates leak risk on error paths +- **NEVER use bare `new`** — always `makeUniqueNoThrow` or `new (std::nothrow)` +- **ALWAYS `LOG_ERR` before returning false** on OOM +- **Use `.get()`** to pass the raw pointer to C-style APIs; ownership stays with the `unique_ptr` +- **`new (std::nothrow)` directly only** when a C API takes ownership; document why in a comment + +**Examples in codebase**: +- Memory utilities: [Memory.h](../lib/Memory/Memory.h) (`makeUniqueNoThrow`) --- diff --git a/lib/JpegToBmpConverter/JpegToBmpConverter.cpp b/lib/JpegToBmpConverter/JpegToBmpConverter.cpp index 0dd78727..2c9545b2 100644 --- a/lib/JpegToBmpConverter/JpegToBmpConverter.cpp +++ b/lib/JpegToBmpConverter/JpegToBmpConverter.cpp @@ -4,10 +4,10 @@ #include #include #include +#include #include #include -#include #include "BitmapHelpers.h" @@ -211,26 +211,26 @@ struct BmpConvertCtx { // Accumulates one MCU row (up to MAX_MCU_HEIGHT source rows × srcWidth pixels) // Filled column-by-column as JPEGDEC callbacks arrive for the same MCU row - uint8_t* mcuBuf; + std::unique_ptr mcuBuf; // Y-axis area averaging accumulators (needsScaling only) int currentOutY; uint32_t nextOutY_srcStart; // 16.16 fixed-point boundary for the next output row - uint32_t* rowAccum; - uint32_t* rowCount; + std::unique_ptr rowAccum; + std::unique_ptr rowCount; - uint8_t* bmpRow; + std::unique_ptr bmpRow; - AtkinsonDitherer* atkinsonDitherer; - FloydSteinbergDitherer* fsDitherer; - Atkinson1BitDitherer* atkinson1BitDitherer; + std::unique_ptr atkinsonDitherer; + std::unique_ptr fsDitherer; + std::unique_ptr atkinson1BitDitherer; bool error; }; // Write a fully-assembled output row (grayscale bytes, length outWidth) to BMP static void writeOutputRow(BmpConvertCtx* ctx, const uint8_t* srcRow, int outY) { - memset(ctx->bmpRow, 0, ctx->bytesPerRow); + memset(ctx->bmpRow.get(), 0, ctx->bytesPerRow); if (USE_8BIT_OUTPUT && !ctx->oneBit) { for (int x = 0; x < ctx->outWidth; x++) { @@ -262,12 +262,12 @@ static void writeOutputRow(BmpConvertCtx* ctx, const uint8_t* srcRow, int outY) ctx->fsDitherer->nextRow(); } - ctx->bmpOut->write(ctx->bmpRow, ctx->bytesPerRow); + ctx->bmpOut->write(ctx->bmpRow.get(), ctx->bytesPerRow); } // Flush one scaled output row from Y-axis accumulators and advance currentOutY static void flushScaledRow(BmpConvertCtx* ctx) { - memset(ctx->bmpRow, 0, ctx->bytesPerRow); + memset(ctx->bmpRow.get(), 0, ctx->bytesPerRow); if (USE_8BIT_OUTPUT && !ctx->oneBit) { for (int x = 0; x < ctx->outWidth; x++) { @@ -301,7 +301,7 @@ static void flushScaledRow(BmpConvertCtx* ctx) { ctx->fsDitherer->nextRow(); } - ctx->bmpOut->write(ctx->bmpRow, ctx->bytesPerRow); + ctx->bmpOut->write(ctx->bmpRow.get(), ctx->bytesPerRow); ctx->currentOutY++; } @@ -324,7 +324,7 @@ int bmpDrawCallback(JPEGDRAW* pDraw) { for (int r = 0; r < blockH && r < MAX_MCU_HEIGHT; r++) { const int copyW = (blockX + validW <= ctx->srcWidth) ? validW : (ctx->srcWidth - blockX); if (copyW <= 0) continue; - memcpy(ctx->mcuBuf + r * ctx->srcWidth + blockX, pixels + r * stride, copyW); + memcpy(ctx->mcuBuf.get() + r * ctx->srcWidth + blockX, pixels + r * stride, copyW); } // Wait for the last MCU column before processing any rows @@ -334,7 +334,7 @@ int bmpDrawCallback(JPEGDRAW* pDraw) { const int endRow = blockY + blockH; for (int y = blockY; y < endRow && y < ctx->srcHeight; y++) { - const uint8_t* srcRow = ctx->mcuBuf + (y - blockY) * ctx->srcWidth; + const uint8_t* srcRow = ctx->mcuBuf.get() + (y - blockY) * ctx->srcWidth; if (!ctx->needsScaling) { // 1:1 — outWidth == srcWidth, write directly @@ -364,8 +364,8 @@ int bmpDrawCallback(JPEGDRAW* pDraw) { flushScaledRow(ctx); ctx->nextOutY_srcStart = static_cast(ctx->currentOutY + 1) * ctx->scaleY_fp; if (srcY_fp >= ctx->nextOutY_srcStart) continue; - memset(ctx->rowAccum, 0, ctx->outWidth * sizeof(uint32_t)); - memset(ctx->rowCount, 0, ctx->outWidth * sizeof(uint32_t)); + memset(ctx->rowAccum.get(), 0, ctx->outWidth * sizeof(uint32_t)); + memset(ctx->rowCount.get(), 0, ctx->outWidth * sizeof(uint32_t)); } } } @@ -387,19 +387,20 @@ bool JpegToBmpConverter::jpegFileToBmpStreamInternal(FsFile& jpegFile, Print& bm s_jpegFile = &jpegFile; - JPEGDEC* jpeg = new (std::nothrow) JPEGDEC(); + const auto jpeg = makeUniqueNoThrow(); if (!jpeg) { - LOG_ERR("JPG", "Failed to allocate JPEG decoder"); + LOG_ERR("JPG", "OOM: JPEG decoder"); return false; } int rc = jpeg->open("", bmpJpegOpen, bmpJpegClose, bmpJpegRead, bmpJpegSeek, bmpDrawCallback); if (rc != 1) { LOG_ERR("JPG", "JPEG open failed (err=%d)", jpeg->getLastError()); - delete jpeg; return false; } + const ScopedCleanup cleanup{[&jpeg]() { jpeg->close(); }}; + const int srcWidth = jpeg->getWidth(); const int srcHeight = jpeg->getHeight(); @@ -411,8 +412,6 @@ bool JpegToBmpConverter::jpegFileToBmpStreamInternal(FsFile& jpegFile, Print& bm if (srcWidth <= 0 || srcHeight <= 0 || srcWidth > MAX_IMAGE_WIDTH || srcHeight > MAX_IMAGE_HEIGHT) { LOG_DBG("JPG", "Image too large or invalid (%dx%d), max supported: %dx%d", srcWidth, srcHeight, MAX_IMAGE_WIDTH, MAX_IMAGE_HEIGHT); - jpeg->close(); - delete jpeg; return false; } @@ -472,54 +471,49 @@ bool JpegToBmpConverter::jpegFileToBmpStreamInternal(FsFile& jpegFile, Print& bm ctx.scaleY_fp = scaleY_fp; ctx.error = false; - // RAII guard: frees all heap resources on any return path - struct Cleanup { - BmpConvertCtx& ctx; - JPEGDEC* jpeg; - ~Cleanup() { - delete[] ctx.rowAccum; - delete[] ctx.rowCount; - delete ctx.atkinsonDitherer; - delete ctx.fsDitherer; - delete ctx.atkinson1BitDitherer; - free(ctx.mcuBuf); - free(ctx.bmpRow); - jpeg->close(); - delete jpeg; - } - } cleanup{ctx, jpeg}; - // MCU row buffer: MAX_MCU_HEIGHT rows × srcWidth columns of grayscale - ctx.mcuBuf = static_cast(malloc(MAX_MCU_HEIGHT * srcWidth)); + ctx.mcuBuf = makeUniqueNoThrow(MAX_MCU_HEIGHT * srcWidth); if (!ctx.mcuBuf) { - LOG_ERR("JPG", "Failed to allocate MCU buffer (%d bytes)", MAX_MCU_HEIGHT * srcWidth); + LOG_ERR("JPG", "OOM: MCU buffer (%d bytes)", MAX_MCU_HEIGHT * srcWidth); return false; } - memset(ctx.mcuBuf, 0, MAX_MCU_HEIGHT * srcWidth); + memset(ctx.mcuBuf.get(), 0, MAX_MCU_HEIGHT * srcWidth); - ctx.bmpRow = static_cast(malloc(bytesPerRow)); + ctx.bmpRow = makeUniqueNoThrow(bytesPerRow); if (!ctx.bmpRow) { - LOG_ERR("JPG", "Failed to allocate BMP row buffer"); + LOG_ERR("JPG", "OOM: BMP row buffer"); return false; } if (needsScaling) { - ctx.rowAccum = new (std::nothrow) uint32_t[outWidth](); - ctx.rowCount = new (std::nothrow) uint32_t[outWidth](); + ctx.rowAccum = makeUniqueNoThrow(outWidth); + ctx.rowCount = makeUniqueNoThrow(outWidth); if (!ctx.rowAccum || !ctx.rowCount) { - LOG_ERR("JPG", "Failed to allocate scaling buffers"); + LOG_ERR("JPG", "OOM: scaling buffers"); return false; } ctx.nextOutY_srcStart = scaleY_fp; } if (oneBit) { - ctx.atkinson1BitDitherer = new (std::nothrow) Atkinson1BitDitherer(outWidth); + ctx.atkinson1BitDitherer = makeUniqueNoThrow(outWidth); + if (!ctx.atkinson1BitDitherer) { + LOG_ERR("JPG", "OOM: Atkinson1BitDitherer"); + return false; + } } else if (!USE_8BIT_OUTPUT) { if (USE_ATKINSON) { - ctx.atkinsonDitherer = new (std::nothrow) AtkinsonDitherer(outWidth); + ctx.atkinsonDitherer = makeUniqueNoThrow(outWidth); + if (!ctx.atkinsonDitherer) { + LOG_ERR("JPG", "OOM: AtkinsonDitherer"); + return false; + } } else if (USE_FLOYD_STEINBERG) { - ctx.fsDitherer = new (std::nothrow) FloydSteinbergDitherer(outWidth); + ctx.fsDitherer = makeUniqueNoThrow(outWidth); + if (!ctx.fsDitherer) { + LOG_ERR("JPG", "OOM: FloydSteinbergDitherer"); + return false; + } } } diff --git a/lib/Memory/Memory.h b/lib/Memory/Memory.h new file mode 100644 index 00000000..0093a852 --- /dev/null +++ b/lib/Memory/Memory.h @@ -0,0 +1,54 @@ +#pragma once + +#include +#include +#include +#include +#include + +// Nothrow versions of std::make_unique. Return nullptr on allocation failure +// instead of calling abort() (the default when exceptions are disabled on ESP32). +// +// Single object: +// auto obj = makeUniqueNoThrow(); +// if (!obj) { LOG_ERR("TAG", "OOM"); return false; } +// +// Array: +// auto buf = makeUniqueNoThrow(size); +// if (!buf) { LOG_ERR("TAG", "OOM"); return false; } +// buf[0] = 0xFF; +// someApi(buf.get(), size); +// + +template + requires(!std::is_array_v) +std::unique_ptr makeUniqueNoThrow(Args&&... args) { + return std::unique_ptr(new (std::nothrow) T(std::forward(args)...)); +} + +template + requires std::is_unbounded_array_v +std::unique_ptr makeUniqueNoThrow(size_t count) { + using Elem = std::remove_extent_t; + return std::unique_ptr(new (std::nothrow) Elem[count]()); +} + +// Helper struct to call a cleanup function on exit from any scope. +// Use with a lambda to avoid unnecessary allocations from std::function/std::bind: +// Example: +// auto jpeg = makeUniqueNoThrow(); +// ScopedCleanup cleanup{[&jpeg]{ jpeg->close(); }}; +// +template +struct [[nodiscard]] ScopedCleanup final { + const F fn; + explicit ScopedCleanup(F f) : fn{std::move(f)} {} + ScopedCleanup(const ScopedCleanup&) = delete; + ScopedCleanup& operator=(const ScopedCleanup&) = delete; + ScopedCleanup(ScopedCleanup&&) = delete; + ScopedCleanup& operator=(ScopedCleanup&&) = delete; + ~ScopedCleanup() { fn(); } +}; + +template +ScopedCleanup(F) -> ScopedCleanup;