From 5c6d0140c5c0bb06555e54d13ae8b534ee77d8f7 Mon Sep 17 00:00:00 2001 From: Alex Hope-O'Connor Date: Mon, 24 Aug 2026 22:36:39 +1000 Subject: [PATCH] Fix iterator cleanup and configuration guidance --- README.md | 46 ++++++---------- examples/AsyncDashboardDemo/platformio.ini | 2 +- examples/AsyncDashboardDemo/src/main.cpp | 12 +++- examples/NestedLayouts/platformio.ini | 2 +- examples/NestedLayouts/src/main.cpp | 26 ++++++--- include/DeviceFrameworkTemplateContext.h | 1 + include/TemplateEngine.h | 3 +- library.json | 2 +- src/DeviceFrameworkTemplateContext.cpp | 12 ++++ src/DeviceFrameworkTemplateRenderer.cpp | 13 +++++ test/test_template_engine/test_main.cpp | 1 + test/test_template_engine/test_main.h | 1 + .../tests/test_edge_cases.cpp | 14 ++--- .../tests/test_template_renderer.cpp | 55 +++++++++++++++---- .../test_template_engine/utils/test_utils.cpp | 2 +- 15 files changed, 124 insertions(+), 68 deletions(-) diff --git a/README.md b/README.md index 0cdc952..ec38530 100644 --- a/README.md +++ b/README.md @@ -232,39 +232,25 @@ Refer to the Unity tests in `test/test_template_engine/tests` for exhaustive com ### Memory & Configuration Knobs -Tune DFTE by defining these macros **before** including `TemplateEngine.h` (or via PlatformIO `build_flags = -DNAME=value`). Larger values increase RAM or flash use, so bump them only when necessary. +DFTE has two kinds of limits. Set build-wide limits with PlatformIO `build_flags`, so the library and every consuming translation unit use the exact same object layout. Do not set these only in a sketch header. -- `DFTE_BUFFER_SIZE_DEFAULT` (512 bytes) – streaming buffer inside `TemplateContext`. -- `DFTE_MAX_STACK_DEPTH_DEFAULT` (16) – maximum nested placeholder/template depth. -- `DFTE_PLACEHOLDER_NAME_SIZE_DEFAULT` (24) – length limit for placeholder tokens. -- `DFTE_MAX_PLACEHOLDERS_DEFAULT` (16) – default capacity when constructing `PlaceholderRegistry`. -- `DFTE_PROGMEM_CHUNK_SIZE_DEFAULT` (512) – copy window when reading PROGMEM data. -- `DFTE_RAM_CHUNK_SIZE_DEFAULT` (128) – chunk size for RAM-based getters. -- `DFTE_MAX_ITERATIONS_DEFAULT` (50) – safety cap for iterator placeholders. +- `DFTE_BUFFER_SIZE` (default 512) – streaming buffer inside `TemplateContext`. +- `DFTE_MAX_STACK_DEPTH` (default 16) – rendering stack frames. A nested template generally uses two frames, so set this to at least twice the intended nesting plus one. +- `DFTE_PLACEHOLDER_NAME_SIZE` (default 24) – maximum placeholder-token length including `%` characters. +- `DFTE_PROGMEM_CHUNK_SIZE` (default 512) and `DFTE_RAM_CHUNK_SIZE` (default 128) – source copy windows. +- `DFTE_MAX_ITERATIONS` (default 50) – safety cap for an individual render call. +- `DFTE_MAX_PLACEHOLDERS_DEFAULT` (default 16) – constructor default only; alternatively pass an explicit capacity to `PlaceholderRegistry`. -``` -// Increase iterator cap to 100 and expand streaming buffer -#define DFTE_MAX_ITERATIONS_DEFAULT 100 -#define DFTE_BUFFER_SIZE_DEFAULT 768 -#include +```ini +; platformio.ini +build_flags = + -DDFTE_BUFFER_SIZE=768 + -DDFTE_MAX_STACK_DEPTH=24 + -DDFTE_PLACEHOLDER_NAME_SIZE=32 + -DDFTE_MAX_ITERATIONS=80 ``` -**Using DFTE inside DeviceFramework** -DeviceFramework projects generate a `DeviceFrameworkTemplateConfig.h` that is re-exported by `DeviceFrameworkConfig.h`. Define your defaults there and make sure `DeviceFrameworkConfig.h` is included before `TemplateEngine.h`; DFTE detects the `CONFIG_template*` symbols and swaps them in automatically. - -``` -// DeviceFrameworkTemplateConfig.h -#pragma once - -#define CONFIG_templateBufferSize_default 768 -#define CONFIG_templateStackDepth_default 24 -#define CONFIG_templateMaxTemplatePlaceholders_default 24 -#define CONFIG_templateProgmemChunkSize_default 1024 -#define CONFIG_templateRamChunkSize_default 256 -#define CONFIG_templateMaxIterations_default 80 -``` - -When the core pulls in `DeviceFrameworkConfig.h`, all templates compiled in that project will inherit these values without further changes. +DFTE is deliberately independent of DeviceFramework's runtime `CONFIG_template*` values. DeviceFramework may choose a registry capacity at runtime, but any fixed DFTE layout must be configured by the shared `DFTE_*` build flags above. This prevents an application header and the compiled library from disagreeing about object size. ## PlatformIO Usage @@ -276,7 +262,7 @@ lib_deps = https://github.com/alexhopeoconnor/DFTE ``` -Pin to a specific release tag if you need reproducible builds (for example `https://github.com/alexhopeoconnor/DFTE#v1.0.0`), or keep the `lib_deps` entry as-is to track the latest main branch during development. +Pin to a specific release tag if you need reproducible builds (for example `https://github.com/alexhopeoconnor/DFTE#v1.0.2`), or keep the `lib_deps` entry as-is to track the latest main branch during development. ### Local Development & Testing ``` diff --git a/examples/AsyncDashboardDemo/platformio.ini b/examples/AsyncDashboardDemo/platformio.ini index fb86f7d..748f08c 100644 --- a/examples/AsyncDashboardDemo/platformio.ini +++ b/examples/AsyncDashboardDemo/platformio.ini @@ -24,7 +24,7 @@ build_flags = -I$PROJECT_LIBDEPS_DIR/$PIOENV/ESPAsyncTCP/src [env:dashboard_esp32] -platform = espressif32 +platform = espressif32@6.13.0 board = esp32dev lib_deps = ${env.lib_deps} diff --git a/examples/AsyncDashboardDemo/src/main.cpp b/examples/AsyncDashboardDemo/src/main.cpp index db649fc..907f5eb 100644 --- a/examples/AsyncDashboardDemo/src/main.cpp +++ b/examples/AsyncDashboardDemo/src/main.cpp @@ -125,8 +125,13 @@ struct DeviceIteratorState { DeviceIteratorState gIteratorState; static PlaceholderEntry gDeviceOverrides[kDeviceCount][4]; +static DynamicDataDescriptor gDeviceOverrideData[kDeviceCount][4]; static bool gOverridesInitialized = false; +const char* getStaticRamValue(void* userData) { + return static_cast(userData); +} + void initializeDeviceOverrides() { if (gOverridesInitialized) { return; @@ -148,9 +153,10 @@ void initializeDeviceOverrides() { PlaceholderEntry& entry = gDeviceOverrides[i][field]; memset(entry.name, 0, sizeof(entry.name)); strncpy(entry.name, names[field], sizeof(entry.name) - 1); - entry.type = PlaceholderType::PROGMEM_DATA; - entry.data = values[field]; - entry.getLength = DeviceFrameworkPlaceholderRegistry::getProgmemLength; + gDeviceOverrideData[i][field] = {getStaticRamValue, nullptr, const_cast(values[field])}; + entry.type = PlaceholderType::DYNAMIC_DATA; + entry.data = &gDeviceOverrideData[i][field]; + entry.getLength = nullptr; } } diff --git a/examples/NestedLayouts/platformio.ini b/examples/NestedLayouts/platformio.ini index 5c7e4a3..f289a4c 100644 --- a/examples/NestedLayouts/platformio.ini +++ b/examples/NestedLayouts/platformio.ini @@ -15,6 +15,6 @@ platform_packages = platformio/framework-arduinoespressif8266 @ https://github.com/esp8266/Arduino.git#521ae60a89e64bb0d1eb7a0b7addf620ced5cad3 [env:example_esp32] -platform = espressif32 +platform = espressif32@6.13.0 board = esp32dev diff --git a/examples/NestedLayouts/src/main.cpp b/examples/NestedLayouts/src/main.cpp index d8b3653..0c2c012 100644 --- a/examples/NestedLayouts/src/main.cpp +++ b/examples/NestedLayouts/src/main.cpp @@ -103,6 +103,11 @@ struct SubsystemIteratorState { SubsystemIteratorState iteratorState; PlaceholderEntry subsystemOverrides[SUBSYSTEM_COUNT][3]; +DynamicDataDescriptor subsystemOverrideData[SUBSYSTEM_COUNT][3]; + +const char* getStaticRamValue(void* userData) { + return static_cast(userData); +} void initialiseOverrides() { for (size_t i = 0; i < SUBSYSTEM_COUNT; ++i) { @@ -110,21 +115,24 @@ void initialiseOverrides() { PlaceholderEntry& name = subsystemOverrides[i][0]; strncpy(name.name, "%NAME%", sizeof(name.name) - 1); - name.type = PlaceholderType::RAM_DATA; - name.data = status.name; - name.getLength = DeviceFrameworkPlaceholderRegistry::getRamLength; + subsystemOverrideData[i][0] = {getStaticRamValue, nullptr, const_cast(status.name)}; + name.type = PlaceholderType::DYNAMIC_DATA; + name.data = &subsystemOverrideData[i][0]; + name.getLength = nullptr; PlaceholderEntry& detail = subsystemOverrides[i][1]; strncpy(detail.name, "%DETAIL%", sizeof(detail.name) - 1); - detail.type = PlaceholderType::RAM_DATA; - detail.data = status.detail; - detail.getLength = DeviceFrameworkPlaceholderRegistry::getRamLength; + subsystemOverrideData[i][1] = {getStaticRamValue, nullptr, const_cast(status.detail)}; + detail.type = PlaceholderType::DYNAMIC_DATA; + detail.data = &subsystemOverrideData[i][1]; + detail.getLength = nullptr; PlaceholderEntry& severity = subsystemOverrides[i][2]; strncpy(severity.name, "%SEVERITY%", sizeof(severity.name) - 1); - severity.type = PlaceholderType::RAM_DATA; - severity.data = status.severityClass; - severity.getLength = DeviceFrameworkPlaceholderRegistry::getRamLength; + subsystemOverrideData[i][2] = {getStaticRamValue, nullptr, const_cast(status.severityClass)}; + severity.type = PlaceholderType::DYNAMIC_DATA; + severity.data = &subsystemOverrideData[i][2]; + severity.getLength = nullptr; } } diff --git a/include/DeviceFrameworkTemplateContext.h b/include/DeviceFrameworkTemplateContext.h index 31ca80c..0d5cdc2 100644 --- a/include/DeviceFrameworkTemplateContext.h +++ b/include/DeviceFrameworkTemplateContext.h @@ -69,6 +69,7 @@ public: unsigned long startTime; DeviceFrameworkTemplateContext(); + ~DeviceFrameworkTemplateContext(); void reset(); // Unified stack management methods diff --git a/include/TemplateEngine.h b/include/TemplateEngine.h index c4f9572..e1c651b 100644 --- a/include/TemplateEngine.h +++ b/include/TemplateEngine.h @@ -24,7 +24,8 @@ * DeviceFrameworkTemplateRenderer::initializeContext(ctx, my_template); * * uint8_t buffer[512]; - * while (!DeviceFrameworkTemplateRenderer::isComplete(ctx)) { + * while (!DeviceFrameworkTemplateRenderer::isComplete(ctx) && + * !DeviceFrameworkTemplateRenderer::hasError(ctx)) { * size_t written = DeviceFrameworkTemplateRenderer::renderNextChunk(ctx, buffer, sizeof(buffer)); * server->sendContent((const char*)buffer, written); * } diff --git a/library.json b/library.json index cfacc5f..084c1a0 100644 --- a/library.json +++ b/library.json @@ -1,6 +1,6 @@ { "name": "DeviceFrameworkTemplateEngine", - "version": "1.0.1", + "version": "1.0.2", "description": "Memory-efficient streaming template engine for ESP8266/ESP32 with chunked rendering support. Designed for embedded web interfaces with PROGMEM template support.", "keywords": [ "template", diff --git a/src/DeviceFrameworkTemplateContext.cpp b/src/DeviceFrameworkTemplateContext.cpp index 3fe4ffc..fd89368 100644 --- a/src/DeviceFrameworkTemplateContext.cpp +++ b/src/DeviceFrameworkTemplateContext.cpp @@ -12,7 +12,19 @@ DeviceFrameworkTemplateContext::DeviceFrameworkTemplateContext() } } +DeviceFrameworkTemplateContext::~DeviceFrameworkTemplateContext() { + while (renderingDepth > 0) { + popContext(); + } +} + void DeviceFrameworkTemplateContext::reset() { + // An interrupted render may own an iterator handle. Pop through the stack + // before overwriting it so every descriptor receives its close callback. + while (renderingDepth > 0) { + popContext(); + } + state = TemplateRenderState::TEXT; renderingDepth = 0; placeholderPos = 0; diff --git a/src/DeviceFrameworkTemplateRenderer.cpp b/src/DeviceFrameworkTemplateRenderer.cpp index 95d12a8..38f9cc7 100644 --- a/src/DeviceFrameworkTemplateRenderer.cpp +++ b/src/DeviceFrameworkTemplateRenderer.cpp @@ -354,6 +354,9 @@ DeviceFrameworkTemplateRenderer::RenderOutcome DeviceFrameworkTemplateRenderer:: if (!applyStackCommands(ctx, outcome)) { ctx.state = TemplateRenderState::ERROR; + while (ctx.renderingDepth > 0) { + ctx.popContext(); + } return makeError(); } @@ -369,6 +372,13 @@ DeviceFrameworkTemplateRenderer::RenderOutcome DeviceFrameworkTemplateRenderer:: outcome.finished = (ctx.state == TemplateRenderState::COMPLETE); outcome.errored = (ctx.state == TemplateRenderState::ERROR); + if (outcome.errored) { + // Error paths can leave a live iterator below the active context. + // Pop all contexts so its close handler is called exactly once. + while (ctx.renderingDepth > 0) { + ctx.popContext(); + } + } return outcome; } @@ -875,6 +885,9 @@ size_t DeviceFrameworkTemplateRenderer::renderNextChunk(DeviceFrameworkTemplateC DFTE_LOG_WARN("Maximum iterations reached in renderNextChunk"); if (consecutiveNoProgressIterations >= 3 && !ctx.isComplete() && !ctx.hasError()) { ctx.state = TemplateRenderState::ERROR; + while (ctx.renderingDepth > 0) { + ctx.popContext(); + } } } diff --git a/test/test_template_engine/test_main.cpp b/test/test_template_engine/test_main.cpp index 5bec169..f821ad2 100644 --- a/test/test_template_engine/test_main.cpp +++ b/test/test_template_engine/test_main.cpp @@ -74,6 +74,7 @@ TestCase tests[] = { TEST_ENTRY(test_template_renderer_iterator_dynamic_items), TEST_ENTRY(test_template_renderer_iterator_error_cleanup), TEST_ENTRY(test_template_renderer_iterator_stall_guard), + TEST_ENTRY(test_template_renderer_iterator_reset_and_destruction_cleanup), // Group 4: Integration Tests TEST_ENTRY(test_integration_full_rendering), diff --git a/test/test_template_engine/test_main.h b/test/test_template_engine/test_main.h index b591f63..2214425 100644 --- a/test/test_template_engine/test_main.h +++ b/test/test_template_engine/test_main.h @@ -45,6 +45,7 @@ void test_template_renderer_iterator_empty(); void test_template_renderer_iterator_dynamic_items(); void test_template_renderer_iterator_error_cleanup(); void test_template_renderer_iterator_stall_guard(); +void test_template_renderer_iterator_reset_and_destruction_cleanup(); // Group 4: Integration Tests void test_integration_full_rendering(); diff --git a/test/test_template_engine/tests/test_edge_cases.cpp b/test/test_template_engine/tests/test_edge_cases.cpp index 0eae9b5..e127d85 100644 --- a/test/test_template_engine/tests/test_edge_cases.cpp +++ b/test/test_template_engine/tests/test_edge_cases.cpp @@ -285,28 +285,22 @@ void test_edge_cases_stress() { // Test stack depth near limit registry.clear(); + // A nested template consumes a placeholder frame plus a template frame. + // Seven nested templates fit within the default 16-frame rendering stack. registry.registerProgmemTemplate("%L2%", deep_nest_level2); registry.registerProgmemTemplate("%L3%", deep_nest_level3); registry.registerProgmemTemplate("%L4%", deep_nest_level4); registry.registerProgmemTemplate("%L5%", deep_nest_level5); registry.registerProgmemTemplate("%L6%", deep_nest_level6); registry.registerProgmemTemplate("%L7%", deep_nest_level7); - registry.registerProgmemTemplate("%L8%", deep_nest_level8); - registry.registerProgmemTemplate("%L9%", deep_nest_level9); - registry.registerProgmemTemplate("%L10%", deep_nest_level10); - registry.registerProgmemTemplate("%L11%", deep_nest_level11); - registry.registerProgmemTemplate("%L12%", deep_nest_level12); - registry.registerProgmemTemplate("%L13%", deep_nest_level13); - registry.registerProgmemTemplate("%L14%", deep_nest_level14); - registry.registerProgmemTemplate("%L15%", deep_nest_level15); - registry.registerProgmemTemplate("%L16%", deep_nest_level16); - + registry.registerProgmemTemplate("%L8%", deep_nest_level16); ctx.reset(); ctx.setRegistry(®istry); TemplateRenderer::initializeContext(ctx, deep_nest_level1); String result3 = captureRenderedOutput(ctx); TEST_ASSERT_TRUE_MESSAGE(result3.length() > 0, "Should render deep nested template"); + TEST_ASSERT_FALSE_MESSAGE(ctx.hasError(), "Deep nesting within the frame limit must not error"); TEST_ASSERT_TRUE_MESSAGE(TemplateRenderer::isComplete(ctx), "Should complete deep nested template"); TEST_ASSERT_LESS_OR_EQUAL_MESSAGE(TemplateContext::MAX_RENDERING_DEPTH, ctx.renderingDepth, diff --git a/test/test_template_engine/tests/test_template_renderer.cpp b/test/test_template_engine/tests/test_template_renderer.cpp index e26894d..4ae6cf5 100644 --- a/test/test_template_engine/tests/test_template_renderer.cpp +++ b/test/test_template_engine/tests/test_template_renderer.cpp @@ -422,6 +422,7 @@ static const char PROGMEM stalledIteratorWrapperTemplate[] = "Start:%STALLING_LI struct StalledIteratorState { size_t calls; + bool closeCalled; }; static StalledIteratorState stalledIteratorState; @@ -430,6 +431,7 @@ static void* stalledIteratorOpen(void* userData) { StalledIteratorState* state = static_cast(userData); if (state) { state->calls = 0; + state->closeCalled = false; } return state; } @@ -449,7 +451,10 @@ static IteratorStepResult stalledIteratorNext(void* handle, IteratorItemView& vi } static void stalledIteratorClose(void* handle) { - (void)handle; + StalledIteratorState* state = static_cast(handle); + if (state) { + state->closeCalled = true; + } } static IteratorDescriptor stalledIteratorDescriptor = { @@ -852,27 +857,21 @@ void test_template_renderer_nested() { "Should contain placeholder value"); // Test deep nesting (up to MAX_RENDERING_DEPTH) + // A nested template consumes a placeholder frame plus a template frame. + // Seven nested templates fit within the default 16-frame rendering stack. registry.registerProgmemTemplate("%L2%", deep_nest_level2); registry.registerProgmemTemplate("%L3%", deep_nest_level3); registry.registerProgmemTemplate("%L4%", deep_nest_level4); registry.registerProgmemTemplate("%L5%", deep_nest_level5); registry.registerProgmemTemplate("%L6%", deep_nest_level6); registry.registerProgmemTemplate("%L7%", deep_nest_level7); - registry.registerProgmemTemplate("%L8%", deep_nest_level8); - registry.registerProgmemTemplate("%L9%", deep_nest_level9); - registry.registerProgmemTemplate("%L10%", deep_nest_level10); - registry.registerProgmemTemplate("%L11%", deep_nest_level11); - registry.registerProgmemTemplate("%L12%", deep_nest_level12); - registry.registerProgmemTemplate("%L13%", deep_nest_level13); - registry.registerProgmemTemplate("%L14%", deep_nest_level14); - registry.registerProgmemTemplate("%L15%", deep_nest_level15); - registry.registerProgmemTemplate("%L16%", deep_nest_level16); - + registry.registerProgmemTemplate("%L8%", deep_nest_level16); ctx.reset(); ctx.setRegistry(®istry); TemplateRenderer::initializeContext(ctx, deep_nest_level1); String result5 = captureRenderedOutput(ctx); TEST_ASSERT_TRUE_MESSAGE(result5.length() > 0, "Should render deep nested template"); + TEST_ASSERT_FALSE_MESSAGE(ctx.hasError(), "Deep nesting within the frame limit must not error"); TEST_ASSERT_TRUE_MESSAGE(TemplateRenderer::isComplete(ctx), "Should complete deep nested template"); @@ -1364,6 +1363,7 @@ void test_template_renderer_iterator_error_cleanup() { void test_template_renderer_iterator_stall_guard() { PlaceholderRegistry registry(4); stalledIteratorState.calls = 0; + stalledIteratorState.closeCalled = false; TEST_ASSERT_TRUE_MESSAGE(registry.registerIterator("%STALLING_LIST%", &stalledIteratorDescriptor), "Stalling iterator placeholder should register"); TemplateContext ctx; @@ -1376,5 +1376,38 @@ void test_template_renderer_iterator_stall_guard() { TEST_ASSERT_EQUAL_MEMORY_MESSAGE("Start:", buffer, 6, "Stalling iterator output should include expected prefix"); TEST_ASSERT_TRUE_MESSAGE(ctx.hasError(), "Renderer should enter error state after exhausting iteration guard"); TEST_ASSERT_TRUE_MESSAGE(stalledIteratorState.calls > 0, "Stalling iterator should be advanced at least once"); + TEST_ASSERT_TRUE_MESSAGE(stalledIteratorState.closeCalled, "Stall cleanup should close the live iterator"); +} + +void test_template_renderer_iterator_reset_and_destruction_cleanup() { + PlaceholderRegistry registry(4); + TEST_ASSERT_TRUE_MESSAGE(registry.registerIterator("%WIFI_LIST%", &wifiIteratorDescriptor), "Iterator should register"); + + resetWifiIteratorState(2); + { + TemplateContext ctx; + ctx.setRegistry(®istry); + TemplateRenderer::initializeContext(ctx, iteratorWrapperTemplate); + uint8_t buffer[1]; + while (wifiIteratorState.index == 0 && !ctx.hasError()) { + TemplateRenderer::renderNextChunk(ctx, buffer, sizeof(buffer)); + } + TEST_ASSERT_FALSE_MESSAGE(wifiIteratorState.closeCalled, "Iterator should remain open before reset"); + ctx.reset(); + TEST_ASSERT_TRUE_MESSAGE(wifiIteratorState.closeCalled, "reset should close an interrupted iterator"); + } + + resetWifiIteratorState(2); + { + TemplateContext ctx; + ctx.setRegistry(®istry); + TemplateRenderer::initializeContext(ctx, iteratorWrapperTemplate); + uint8_t buffer[1]; + while (wifiIteratorState.index == 0 && !ctx.hasError()) { + TemplateRenderer::renderNextChunk(ctx, buffer, sizeof(buffer)); + } + TEST_ASSERT_FALSE_MESSAGE(wifiIteratorState.closeCalled, "Iterator should remain open before destruction"); + } + TEST_ASSERT_TRUE_MESSAGE(wifiIteratorState.closeCalled, "destruction should close an interrupted iterator"); } diff --git a/test/test_template_engine/utils/test_utils.cpp b/test/test_template_engine/utils/test_utils.cpp index 1c8531f..c8d435e 100644 --- a/test/test_template_engine/utils/test_utils.cpp +++ b/test/test_template_engine/utils/test_utils.cpp @@ -11,7 +11,7 @@ String captureRenderedOutput(TemplateContext& ctx, size_t bufferSize) { return output; } - while (!TemplateRenderer::isComplete(ctx)) { + while (!TemplateRenderer::isComplete(ctx) && !TemplateRenderer::hasError(ctx)) { size_t written = TemplateRenderer::renderNextChunk(ctx, buffer, bufferSize); if (written > 0) { // Arduino String doesn't have (const char*, size_t) constructor