Uh oh!
There was an error while loading. Please reload this page.
perf(mem): free ~16.5 KB internal DRAM on S3 (PSRAM relocation + String-free LAN logging) - #142
Merged
Conversation
Arduino does not pass IDF's buffer profile to esp_wifi_init(). With _wifiUseStaticBuffers false (its default) wifiLowLevelInit() overwrites WIFI_INIT_CONFIG_DEFAULT() with a demand-driven one: static_tx 0 / dynamic_tx 32 / cache_tx 4 / static_rx 4 / dynamic_rx 32 so TX memory is malloc'd per frame from internal DRAM with no ceiling below 32 x ~1.6 KB. Setting the flag skips the override and restores the sdkconfig profile: static_tx 8, dynamic_tx 0, cache_tx 0, static_rx 8, dynamic_rx 32. Net: ~13 KB more resident internal DRAM, ~38 KB off the peak. TX is a fixed 12.8 KB pool that frames queue behind instead of an allocator that consumes whatever is left. That is the trade this part needs -- it dies on transients, not on baseline. A coredump taken 2026-08-03 (esp32-s3-N16R8-extuart-debug, reTerminal E1003) caught the tcpip task with a NULL return from the mDNS packet allocator during an inbound query burst ~5 s after the responder began announcing; the resulting OOM error log aborted in newlib lock_init_generic, which is the PANIC (4) reset seen in the field logs. RX is untouched: dynamic_rx_buf_num is 32 in both profiles and Arduino exposes no knob for it. cache_tx 0 pairs correctly with static TX -- cache buffers only exist to copy dynamic TX buffers out of PSRAM into DMA-capable memory. The call must precede every WiFi API call; wifiLowLevelInit() reads the flag once and latches lowLevelInitDone. Nothing in this repo touches WiFi before initWiFi() -- the WiFi.status()/localIP() sites in main.cpp and config_parser.cpp all run after setup(). NOT verified on hardware: that esp_wifi_init() still returns ESP_OK with static TX buffers and PSRAM enabled, and the resulting min-heap figure. Built: esp32-s3-N16R8-extuart-debug, esp32-s3-E1004, esp32-wrover-e-N4R8.
lanLog(const String&) existed only to avoid reflowing ~67 call sites into format strings -- its own comment said so. Reflowing them removes the adapter and everything it cost. Arduino String on ESP32 has SSO up to 14 chars (WString.h SSOSIZE), so short literals were already free; but essentially every line here is longer, so all 67 sites heap-allocated at least once and the 29 concatenating ones allocated and realloc'd several times each. On the LAN path that ran on the same task the 2026-08-03 coredump caught failing an allocation. Three wins beyond removing the allocations: - OD_LOG_LEVEL becomes a real compile-time gate. The String was built by the caller BEFORE control reached od_log_*'s level test, so a gated-out line still paid for its formatting. od_log_debug(...) now compiles away, arguments and all. - Per-site level choice instead of one INFO bucket with an ERROR:/WARNING: prefix sniff (String::startsWith) on every line. - Level is chosen at the call site from a literal, not inferred at runtime from message text that a runtime value could in principle land at position 0 of. Message text and log level are UNCHANGED at all 67 sites, including the redundant-looking "ERROR: "/"WARNING: " prefixes -- output already reads "E: ERROR: ..." in the field, so old and new logs stay directly comparable. Retagging chatty INFO lines to DEBUG is a diagnostics decision and belongs in its own commit where the disappearing lines are visible. Also in this change, all forced by removing String from these paths: - tlsFailNote() returns into a caller buffer instead of a String; its 5 call sites get a char[80]. Failure paths should not need the allocator, least of all when they may be failing because of it. - lanFormatBssid() replaces WiFi.BSSIDstr() (returns String by value), matching its output including the empty string when the driver has no BSSID. - WiFi.localIP()/remoteIP().toString() -> IPAddress octets via %u.%u.%u.%u. Verified: rebuilt with -Wformat -Wformat-security (NOT on by default here -- the project sets no -Wall, so the format(printf) attribute on _od_log was not being checked). Zero format warnings from the 67 converted sites. The two pre-existing %u/uint32_t warnings in main.cpp:150,159 and the -Wconversion-null in wifi_service.cpp are untouched by this change and left alone. Not converted, deliberately: getChipIdHex() still returns String, so deviceName/mac in restartLanService() stay Strings. That signature touches 6 files and belongs in its own commit. Built: esp32-s3-N16R8-extuart-debug, esp32-s3-N16R8, esp32-s3-E1004, esp32-wrover-e-N4R8, esp32-c3-N4, esp32-N4.
…true)" This reverts commit b0060d1. It bounded the wrong transient. The change traded ~12.8 KB of permanently resident internal DRAM for a lower peak: static_rx 4->8, static_tx 0->8, cache_tx 4->0, so preallocation goes 12.8 KB -> 25.6 KB while the TX ceiling drops from 32 x ~1.6 KB on demand to a fixed 12.8 KB pool. On a TX-heavy device that is a good trade. This is not one. Three reasons it is wrong here: - The failure it was meant to help is RX-side. The 2026-08-03 coredump caught the tcpip task with a NULL from the mDNS packet allocator during an inbound query burst. dynamic_rx_buf_num is 32 in BOTH profiles and static mode also doubles static_rx_buf_num, so it leaves the RX side 6.4 KB WORSE resident and 6.4 KB higher at the ceiling -- the opposite of what was needed. - The workload is RX-dominated. A LAN push sends ~493 KB to the device; the device replies with TCP ACKs and a few protocol frames. Dynamic TX demand is a handful of buffers, never near 32, so the 51.2 KB TX ceiling is not approached and bounding it buys nothing measurable. - Lowering a ceiling of ~115 KB is moot when free internal heap after setup is ~66 KB. Neither profile can reach its ceiling; the heap runs out first. What is left is the baseline cost, and this made the baseline worse. Confirmed on hardware before reverting: min free dropped by roughly the predicted 13 KB. The remaining levers are all baseline reductions rather than ceiling bounds -- relocating fixed-size buffers to PSRAM (mbedTLS record slots 34.8 KB, chunkedWriteState + configScratch 8.2 KB, NimBLE host ~14 KB) -- and none of them carry this trade. Keeps 194bd7b (lanLog -> od_log_*), which is unaffected.
configScratch and chunkedWriteState.buffer were .bss arrays: 8,212 B of internal DRAM held for the entire uptime to serve two paths that are idle almost all of it. Both are now pointers reserved once at boot from PSRAM. esp32-s3-N16R8-extuart-debug .bss 95,352 -> 87,168 (-8,184 B) esp32-N4, nrf52840custom unchanged (still 4,096 / 4,116 arrays) Gated on OPENDISPLAY_ENABLE_WIFI *and* BOARD_HAS_PSRAM -- the seven S3 envs, and NOT esp32-wrover-e-N4R8, which has PSRAM but no WiFi. Both halves matter: PSRAM because there is nowhere else to put them, WiFi because only that surface creates the internal-DRAM shortage this relieves (esp32-N4 uses 77 KB of 327 KB). The gate is written against the raw -D flags rather than OPENDISPLAY_HAS_WIFI deliberately: it changes the LAYOUT of chunked_write_state_t, so every TU seeing config_parser.h must evaluate it identically. Command-line -D is visible everywhere; a macro from another header depends on include order, and one TU disagreeing about a struct layout links cleanly and corrupts memory. Why reserve at boot and never free, rather than allocate per use: both buffers must be live across whole operations (the scratch on every config read, the chunk buffer across a multi-command upload), an on-demand 4 KB alloc/free cycle is exactly the fragmentation the TLS reservation comment warns about, and it would fail precisely when someone is trying to reconfigure a device that is already short of memory. Reserving at boot decides WHERE, not WHEN. No internal-DRAM fallback, deliberately: this code is only reached on -DBOARD_HAS_PSRAM envs, and a board whose PSRAM is dead cannot run this firmware anyway (FastEPD's framebuffer is a single 2.6 MB allocation). A fallback would spend 4 KB of the scarcest memory on a case that cannot occur. On failure the pointers stay null and the consumers fail closed -- loadGlobalConfig and hasValidStoredConfig return false, handleReadConfig and handleWriteConfig NACK -- rather than dereferencing null. Guarding handleWriteConfig covers the chunked path too, since handleWriteConfigChunk only runs after it sets .active. Also documents in platformio.ini why -DCONFIG_BT_NIMBLE_MEM_ALLOC_MODE_EXTERNAL=1 must NOT be added: it is inert on this framework. exp_nimble_mem.c tests #ifdef ..._INTERNAL before #elif ..._EXTERNAL, and Arduino's prebuilt sdkconfig.h:589 defines ..._INTERNAL unconditionally before nimconfig.h's guard runs. Verified by disassembling nimble_platform_mem_calloc with and without the flag: identical, both computing MALLOC_CAP_INTERNAL|MALLOC_CAP_8BIT (0x804). Built: all 16 environments.
pipeReorder was 33 slots x 252 B = 8,316 B of .bss -- the largest app-owned static after tinfl's ROM-fixed tables -- and it is dead memory except during a PIPE transfer. Now a pointer reserved once at boot from PSRAM. esp32-s3-N16R8-extuart-debug .bss 87,168 -> 78,856 (-8,312 B) esp32-N4, nrf52840custom unchanged (still 4,284 / 17-slot arrays) Combined with 78ccddb, .bss on the S3 debug env is 95,352 -> 78,856 (-16,496 B), all of it returned to the internal heap the 2026-08-03 coredump showed exhausted. Same gate as the config buffers (OPENDISPLAY_ENABLE_WIFI && BOARD_HAS_PSRAM, raw -D flags), so the seven S3 envs change and nothing else does. Safe to relocate: touched only on the loop task via BLE command dispatch, never from an ISR, never handed to DMA -- pipeConsumePayload() copies out of it into the decompressor or the panel sink. Access is at BLE frame rate, orders of magnitude below PSRAM bandwidth. Simpler than the config buffers in one respect: PipeReorderSlot itself is unchanged and the array is a file static, so there is no cross-TU layout question -- only this file sees the difference. A failed reservation is logged and not otherwise handled. It means defective PSRAM, and a board that cannot allocate 8 KB cannot hold FastEPD's 2.6 MB framebuffer either, so the panel is dead regardless -- there is no degraded mode worth carrying code for. Built: all 16 environments.
Follow-up to 78ccddb, which shipped fail-closed guards on the two config buffers: loadGlobalConfig and hasValidStoredConfig returning false, handleReadConfig and handleWriteConfig NACKing. Those are removed. Only the boot-time error log stays. A failed reservation is not a runtime condition, it is a defective part: - the allocation runs at boot with a pristine heap, before WiFi, BLE or the panel have taken anything, so there is no pressure for it to lose to; - it is only compiled on -DBOARD_HAS_PSRAM envs, so the memory is meant to exist; - a board that cannot allocate 4 KB of PSRAM cannot hold FastEPD's 2.6 MB framebuffer either, so the display is dead whatever the config path does. Carrying a degraded mode for that bought nothing real and spread null checks across four call sites in two files. It also left 752eb69 (the PIPE reorder queue, same reservation, same gate) inconsistent with it -- that one never had guards, and having one buffer fail closed while another does not is worse than either policy applied uniformly. reserveConfigBuffer() still logs od_log_error on failure so the fault is named at boot rather than diagnosed from a crash. Built: all 16 environments.
davelee98 pushed a commit
to davelee98/Firmware
that referenced
this pull request
Aug 7, 2026
Move the completed PLAN/FINDINGS/IMPLEMENTATION/inventory docs to ../old_docs (outside the repo) to declutter docs/; delete the superseded IT8951 bb_epaper integration plan outright. No code changes. The PSRAM relocation and String-free LAN logging this branch previously carried already landed on main as OpenDisplay#142, so those commits are dropped as redundant; the config-buffer null guards are dropped as out of scope for this PR and need to be raised separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmiW5Kpby5ZJSp4CX3JncT
jonasniesner pushed a commit
that referenced
this pull request
Aug 7, 2026
* docs: archive completed plan/findings docs out of docs/ Move the completed PLAN/FINDINGS/IMPLEMENTATION/inventory docs to ../old_docs (outside the repo) to declutter docs/; delete the superseded IT8951 bb_epaper integration plan outright. No code changes. The PSRAM relocation and String-free LAN logging this branch previously carried already landed on main as #142, so those commits are dropped as redundant; the config-buffer null guards are dropped as out of scope for this PR and need to be raised separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmiW5Kpby5ZJSp4CX3JncT * chore: ignore coredumps, compile_commands.json and .mcp.json All three are local artifacts that showed up as untracked noise in git status: - core.* -- coredumps pulled off-device for post-mortem analysis (the 2026-08-03 mDNS crash investigation left one in the tree) - compile_commands.json -- generated by the build for clangd/IDE indexing, machine-specific paths - .mcp.json -- local MCP server config None belong in the repo, and leaving them untracked meant they were one stray `git add -A` away from being committed. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reduces resident internal DRAM on ESP32-S3 WiFi targets and removes
Stringallocation from theLAN logging path. Motivated by a coredump taken 2026-08-03 (
esp32-s3-N16R8-extuart-debug,reTerminal E1003): the tcpip task got a NULL from the mDNS packet allocator during an inbound
query burst, causing a PANIC (4) reset from internal heap exhaustion.
Net on the seven S3 envs:
.bss95,352 → 78,856 B (−16,496 B), returned to the internalheap. Other targets unchanged.
Contents
194bd7blanLog(const String&)withod_log_*printf logging across 67 call sites78ccddbconfigScratch,chunkedWriteState.buffer) in PSRAM on S3752eb692fd268478ccddb, for consistency with752eb69b0060d1setsWiFi.useStaticBuffers(true)andc12be6ereverts it. They cancel out, keptunsquashed because the revert message documents why the approach was wrong: it bounds the TX
ceiling, but the observed failure is RX-side (
dynamic_rx_buf_numis 32 in both profiles, andstatic mode also doubles
static_rx_buf_num), leaving RX ~6.4 KB worse resident. Confirmed onhardware before reverting — min free dropped by roughly the predicted 13 KB.
Notes
Both relocated buffers are touched only on the loop task via BLE command dispatch — never from an
ISR, never handed to DMA — and accessed at BLE frame rate, far below PSRAM bandwidth. Gated on
OPENDISPLAY_ENABLE_WIFI && BOARD_HAS_PSRAM, so only the seven S3 envs change.A failed reservation is logged at boot and not otherwise handled: it means defective PSRAM, and a
board that cannot allocate 4–8 KB cannot hold FastEPD's 2.6 MB framebuffer either.
Testing
194bd7brebuilt with-Wformat -Wformat-securityexplicitly enabled (the project sets no-Wall, so theformat(printf)attribute on_od_logwas not being checked). Zero formatwarnings from the 67 converted sites; two pre-existing
%u/uint32_twarnings inmain.cpp:150,159and a-Wconversion-nullinwifi_service.cppare untouched.directly comparable.