Add central memory-class ladder (MemClass.h) with fail-safe-small defaults - #10901
Merged
Conversation
Field reports on 2.8.0 show nRF52840 devices at 99% heap (114/115 KB) within minutes of boot; operator new asserts on OOM, so these devices are one allocation from a reboot. The 2.8.0 cache sizing ladders gave nRF52 the largest non-PSRAM tiers on the assumption that a BLE-only part has a roomy heap - the arena is actually ~125 KB shared with the FreeRTOS task stacks. Per-target retiers (nRF52840 unless noted): - Traffic Management cache 1000 -> 250 entries (10 KB -> 2.5 KB); the unclassified fallthrough drops 1000 -> 400 to match the classic-ESP32 tier (also affects RP2040/RP2350) - Warm node store 200 -> 100 entries (8 KB -> 4 KB); the non-XXAA fallthrough drops 320 -> 100 so an unclassified RAM-constrained part can't boot-allocate 12.8 KB - MESSAGE_HISTORY_LIMIT 20 -> 10 (text pool 4.4 KB -> 2.2 KB), the tier classic ESP32 already ships - MAX_RX_TOPHONE 32 -> 16, shrinking the static packet pool 70 -> 54 slots (~6.6 KB of .bss returned to the heap arena) - PacketHistory hash index off arch-wide (1 KB); O(n) over 240 records is negligible at LoRa packet rates - OLEDDISPLAY_REDUCE_MEMORY arch-wide (~1 KB OLED back buffer); the five TFT variants -U it because TFTDisplay.cpp needs buffer_back for dirty-window diffing - Drop the stale "for testing" 1024-entry TMM override on T1000-E Measured on rak4631: heap arena grows 124,572 -> 131,180 B and boot allocations drop ~15.7 KB, roughly +22 KB free heap on the field-report device class. Migration: the nRF52840 warm flash ring replays through place() (LRU), so the newest 100 identities survive the shrink; the file backend rejects oversized snapshots cleanly (new test covers this). Native suites pass (536/536 Docker, 13/13 native-macos warm store); rak4631, heltec-mesh-node-t114 (TFT) and tracker-t1000-e build green.
…aults The 2.8.0 nRF52840 heap exhaustion happened because each RAM-sized cache picked its per-platform tier from its own chip #ifdef ladder, and every ladder's fallthrough default was its largest non-PSRAM tier - nRF52 was never named, so it silently got 1000-entry caches on a ~115 KB arena. This introduces src/memory/MemClass.h: a single MESHTASTIC_MEM_CLASS (TINY / SMALL / MEDIUM / LARGE) ranked by usable app heap after platform overheads, with the deliberate property that an unclassified chip lands in SMALL - a new target boots with small caches until someone opts it up in one visible place. The TMM cache, warm store, MAX_RX_TOPHONE and MAX_SATELLITE_NODES ladders in mesh-pb-constants.h now key off the class; branches pinned by something other than RAM stay explicit and say why (nRF52840's SoftDevice arena, RP2040's warm.dat watchdog bound). MAX_NUM_NODES intentionally stays separate - it is flash-shaped (nodes.proto vs LittleFS), not heap-shaped. A per-class MESHTASTIC_BOOT_CACHE_BUDGET static_assert now covers the three big boot-allocated caches, so the next cache-adding PR that would blow a small platform's budget fails to compile instead of exhausting heap in the field. No values change for any existing target: rak4631, tbeam, rak11310 and wio-e5 build byte-identical before/after; all ladders remain #ifndef-guarded so variant overrides keep working.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces a memory-class tier abstraction, drives mesh cache sizing from that tier, adds compile-time size checks for packet history and cache budgets, and updates two nRF52840 variant build flags. ChangesMemory class tiering and cache budget enforcement
nRF52840 OLED display memory build flag
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a centralized memory-class ladder (MESHTASTIC_MEM_CLASS) to drive RAM-shaped cache sizing decisions from a single, fail-safe-small policy point, and adds a compile-time guardrail (MESHTASTIC_BOOT_CACHE_BUDGET) to prevent future boot-time cache growth from silently exhausting heap on constrained targets.
Changes:
- Add
src/memory/MemClass.hdefiningMEM_CLASS_{TINY,SMALL,MEDIUM,LARGE}with an explicit “unknown chips default to SMALL” policy and per-class boot-cache budgets. - Refactor several cache/limit ladders in
src/mesh/mesh-pb-constants.hto key offMESHTASTIC_MEM_CLASS(with documented exceptions). - Add a
static_assertbudget check over the three major boot-allocated mesh caches (TMM cache, warm store, packet history) whereMAX_NUM_NODESis compile-time constant.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/mesh/mesh-pb-constants.h | Switch cache tier ladders to use MESHTASTIC_MEM_CLASS and add a compile-time boot-cache budget assertion. |
| src/memory/MemClass.h | New central classification header defining memory classes and per-class boot-cache budgets with fail-safe-small defaulting. |
# Conflicts: # src/mesh/mesh-pb-constants.h
…ssert - MemClass.h's class table claimed RP2350 was MEDIUM while the mapping ladder classifies it SMALL (with RP2040) - the table now matches the ladder, with a note that RP2350 is a MEDIUM candidate whenever someone wants to tune it up (kept SMALL here so this header stays a behavioral no-op). - The boot-cache budget comment referenced a static_assert pinning PacketHistory::PacketRecord at 20 B that did not exist (only a layout comment). Add the real static_assert so the budget math in mesh-pb-constants.h fails to compile if the record layout changes. Also merges develop (the base #10898 landed there as a squash, which is what made this stacked branch conflict); develop's mesh-pb-constants.h is byte-identical to this branch's base, so the resolution keeps the MemClass ladder unchanged. rak4631 and wio-e5 build green; test_packet_history 47/47.
Contributor
Author
|
Both review findings addressed in 85a3c2f, and the merge conflict is resolved (the base #10898 landed on develop as a squash, which is what tripped this stacked branch — develop was merged in; its
Verified: rak4631 and wio-e5 build green post-merge; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/mesh/mesh-pb-constants.h (1)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment block exceeds repo guideline of 1-2 lines.
As per coding guidelines,
**/*.{c,cc,cpp,h,hpp}should "use at most one or two lines, only when the reason is not obvious; do not add multi-paragraph explanatory comments." This 5-line policy comment could be trimmed to a one-liner referencingmemory/MemClass.h.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/mesh-pb-constants.h` around lines 10 - 14, The comment block in mesh-pb-constants.h is too verbose for the repository’s header comment style. Trim the existing multi-line policy note near the mesh options constants down to a single short line, keeping only the essential reference to memory/MemClass.h and the safe-small fallback rationale. Use the surrounding constants and mesh.options context to preserve meaning while removing the extra explanatory lines.Source: Coding guidelines
src/memory/MemClass.h (1)
3-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the header comment block to comply with repo comment-length guideline.
This block is ~36 lines of prose. As per coding guidelines,
**/*.{c,cc,cpp,cxx,h,hpp,hh}requires "one or two lines mმაximum, only when the reason is not obvious, and do not restate the next line." Consider moving the rationale/history into the PR description or a design doc, and keeping only a short pointer comment in the header (e.g., a one-liner plus a link to docs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/MemClass.h` around lines 3 - 38, The top-of-file comment in MemClass.h is far too long for the header comment guideline; trim it down to a one- or two-line pointer that keeps only the essential note about MESHTASTIC_MEM_CLASS and where the fuller rationale lives. Move the detailed history, platform ranking, and override explanation out of the header and into the PR description or a design doc, keeping the remaining comment near the MemClass.h definitions brief and easy to scan.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mesh/mesh-pb-constants.h`:
- Around line 209-225: The boot-cache budget check in mesh-pb-constants.h is
duplicating the PACKETHISTORY_MAX arithmetic from PacketHistory.cpp, so the
assert and the runtime clamp can diverge. Move the packet-history capacity bound
into a shared constexpr or header symbol (for example alongside
PacketHistory/PacketHistory::PacketRecord definitions) and update both the
static_assert in mesh-pb-constants.h and the constructor clamp in
PacketHistory.cpp to use that single shared value.
---
Nitpick comments:
In `@src/memory/MemClass.h`:
- Around line 3-38: The top-of-file comment in MemClass.h is far too long for
the header comment guideline; trim it down to a one- or two-line pointer that
keeps only the essential note about MESHTASTIC_MEM_CLASS and where the fuller
rationale lives. Move the detailed history, platform ranking, and override
explanation out of the header and into the PR description or a design doc,
keeping the remaining comment near the MemClass.h definitions brief and easy to
scan.
In `@src/mesh/mesh-pb-constants.h`:
- Around line 10-14: The comment block in mesh-pb-constants.h is too verbose for
the repository’s header comment style. Trim the existing multi-line policy note
near the mesh options constants down to a single short line, keeping only the
essential reference to memory/MemClass.h and the safe-small fallback rationale.
Use the surrounding constants and mesh.options context to preserve meaning while
removing the extra explanatory lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c4c4b9bc-11b9-4a07-a0de-1b9ee9cfc95c
📒 Files selected for processing (5)
src/memory/MemClass.hsrc/mesh/PacketHistory.hsrc/mesh/mesh-pb-constants.hvariants/nrf52840/heltec_mesh_node_t114/platformio.inivariants/nrf52840/heltec_mesh_solar/platformio.ini
- Hoist PACKETHISTORY_MAX from PacketHistory.cpp into mesh-pb-constants.h (next to the MAX_NUM_NODES it derives from) so the constructor clamp and the boot-cache budget static_assert use one definition instead of hand-mirrored arithmetic that could drift. The expression stays valid where MAX_NUM_NODES resolves at runtime (ESP32-S3, portduino); the pointless 2.0 double math becomes integer. - Trim the MemClass.h header (36 -> 16 comment lines) and the budget / sizing-policy comments per the repo comment-length guideline, keeping the class table, the fail-safe-small rule, the override mechanism, and the include-order constraint. rak4631 (compile-time MAX_NUM_NODES) and heltec-v3 (runtime) build green; test_packet_history 47/47.
Contributor
Author
|
CodeRabbit feedback addressed in 08476c2:
Verified: rak4631 + heltec-v3 build green, |
Itzdavid01
pushed a commit
to Itzdavid01/firmware
that referenced
this pull request
Sep 5, 2026
…aults (meshtastic#10901) * Right-size nRF52 heap tiers after 2.8.0 heap-exhaustion field reports Field reports on 2.8.0 show nRF52840 devices at 99% heap (114/115 KB) within minutes of boot; operator new asserts on OOM, so these devices are one allocation from a reboot. The 2.8.0 cache sizing ladders gave nRF52 the largest non-PSRAM tiers on the assumption that a BLE-only part has a roomy heap - the arena is actually ~125 KB shared with the FreeRTOS task stacks. Per-target retiers (nRF52840 unless noted): - Traffic Management cache 1000 -> 250 entries (10 KB -> 2.5 KB); the unclassified fallthrough drops 1000 -> 400 to match the classic-ESP32 tier (also affects RP2040/RP2350) - Warm node store 200 -> 100 entries (8 KB -> 4 KB); the non-XXAA fallthrough drops 320 -> 100 so an unclassified RAM-constrained part can't boot-allocate 12.8 KB - MESSAGE_HISTORY_LIMIT 20 -> 10 (text pool 4.4 KB -> 2.2 KB), the tier classic ESP32 already ships - MAX_RX_TOPHONE 32 -> 16, shrinking the static packet pool 70 -> 54 slots (~6.6 KB of .bss returned to the heap arena) - PacketHistory hash index off arch-wide (1 KB); O(n) over 240 records is negligible at LoRa packet rates - OLEDDISPLAY_REDUCE_MEMORY arch-wide (~1 KB OLED back buffer); the five TFT variants -U it because TFTDisplay.cpp needs buffer_back for dirty-window diffing - Drop the stale "for testing" 1024-entry TMM override on T1000-E Measured on rak4631: heap arena grows 124,572 -> 131,180 B and boot allocations drop ~15.7 KB, roughly +22 KB free heap on the field-report device class. Migration: the nRF52840 warm flash ring replays through place() (LRU), so the newest 100 identities survive the shrink; the file backend rejects oversized snapshots cleanly (new test covers this). Native suites pass (536/536 Docker, 13/13 native-macos warm store); rak4631, heltec-mesh-node-t114 (TFT) and tracker-t1000-e build green. * Add central memory-class ladder (MemClass.h) with fail-safe-small defaults The 2.8.0 nRF52840 heap exhaustion happened because each RAM-sized cache picked its per-platform tier from its own chip #ifdef ladder, and every ladder's fallthrough default was its largest non-PSRAM tier - nRF52 was never named, so it silently got 1000-entry caches on a ~115 KB arena. This introduces src/memory/MemClass.h: a single MESHTASTIC_MEM_CLASS (TINY / SMALL / MEDIUM / LARGE) ranked by usable app heap after platform overheads, with the deliberate property that an unclassified chip lands in SMALL - a new target boots with small caches until someone opts it up in one visible place. The TMM cache, warm store, MAX_RX_TOPHONE and MAX_SATELLITE_NODES ladders in mesh-pb-constants.h now key off the class; branches pinned by something other than RAM stay explicit and say why (nRF52840's SoftDevice arena, RP2040's warm.dat watchdog bound). MAX_NUM_NODES intentionally stays separate - it is flash-shaped (nodes.proto vs LittleFS), not heap-shaped. A per-class MESHTASTIC_BOOT_CACHE_BUDGET static_assert now covers the three big boot-allocated caches, so the next cache-adding PR that would blow a small platform's budget fails to compile instead of exhausting heap in the field. No values change for any existing target: rak4631, tbeam, rak11310 and wio-e5 build byte-identical before/after; all ladders remain #ifndef-guarded so variant overrides keep working. * Address review: fix RP2350 class-table doc, add PacketRecord static_assert - MemClass.h's class table claimed RP2350 was MEDIUM while the mapping ladder classifies it SMALL (with RP2040) - the table now matches the ladder, with a note that RP2350 is a MEDIUM candidate whenever someone wants to tune it up (kept SMALL here so this header stays a behavioral no-op). - The boot-cache budget comment referenced a static_assert pinning PacketHistory::PacketRecord at 20 B that did not exist (only a layout comment). Add the real static_assert so the budget math in mesh-pb-constants.h fails to compile if the record layout changes. Also merges develop (the base meshtastic#10898 landed there as a squash, which is what made this stacked branch conflict); develop's mesh-pb-constants.h is byte-identical to this branch's base, so the resolution keeps the MemClass ladder unchanged. rak4631 and wio-e5 build green; test_packet_history 47/47. * Address review: share PACKETHISTORY_MAX, trim policy comments - Hoist PACKETHISTORY_MAX from PacketHistory.cpp into mesh-pb-constants.h (next to the MAX_NUM_NODES it derives from) so the constructor clamp and the boot-cache budget static_assert use one definition instead of hand-mirrored arithmetic that could drift. The expression stays valid where MAX_NUM_NODES resolves at runtime (ESP32-S3, portduino); the pointless 2.0 double math becomes integer. - Trim the MemClass.h header (36 -> 16 comment lines) and the budget / sizing-policy comments per the repo comment-length guideline, keeping the class table, the fail-safe-small rule, the override mechanism, and the include-order constraint. rak4631 (compile-time MAX_NUM_NODES) and heltec-v3 (runtime) build green; test_packet_history 47/47.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
The 2.8.0 nRF52840 heap exhaustion (#10898) had a structural cause beyond the numbers: every RAM-sized cache picks its per-platform tier from its own chip
#ifdefladder, and every ladder's fallthrough default was its largest non-PSRAM tier. nRF52 was never explicitly named in the TMM ladder, so it silently inherited 1000-entry caches on a ~115 KB arena. Nothing stops the next feature from repeating this.Change
New
src/memory/MemClass.h— a singleMESHTASTIC_MEM_CLASS(TINY/SMALL/MEDIUM/LARGE) ranked by usable app heap after platform overheads (SoftDevice, WiFi+BLE stacks), which is why classic ESP32 shares a class with nRF52840. Two deliberate properties:MESHTASTIC_BOOT_CACHE_BUDGETisstatic_asserted inmesh-pb-constants.hover the three big boot-allocated caches (TMM cache + warm store + packet history), so a future cache-adding PR that would blow a small platform's budget fails to compile instead of exhausting heap in the field. (Skipped whereMAX_NUM_NODESis runtime-resolved: ESP32-S3 and portduino.)The TMM cache, warm store,
MAX_RX_TOPHONE, andMAX_SATELLITE_NODESladders now key off the class. Branches pinned by something other than RAM stay explicit and say why (nRF52840's SoftDevice-shared arena → TMM 250; RP2040's warm.dat watchdog bound → warm 150).MAX_NUM_NODESintentionally stays separate — it's flash-shaped (nodes.proto vs LittleFS), not heap-shaped. All ladders remain#ifndef-guarded, so existing variant overrides keep working.No-op proof
Rebuilt one env per class on this branch vs its base and compared
sizeoutput:MEDIUM/LARGE (S3/C6/P4, PSRAM, portduino) are covered by the value-by-value mapping in the diff (each class value equals the branch it replaced) and by CI.
Verification
Follow-up candidates deliberately not changed here (kept historical values, flagged inline):
MESSAGE_HISTORY_LIMITnormalization, ESP32-C3'sMAX_RX_TOPHONE=32.Summary by CodeRabbit
New Features
Bug Fixes