Skip to content

fix(tab5): prefer PSRAM for larger ordinary allocations - #43

Merged
vincentkoc merged 1 commit into
mainfrom
fix/tab5-psram-allocation-policy
Sep 12, 2026
Merged

fix(tab5): prefer PSRAM for larger ordinary allocations#43
vincentkoc merged 1 commit into
mainfrom
fix/tab5-psram-allocation-policy

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 8, 2026

Copy link
Copy Markdown
Member

Problem

Tab5 showed WebSocket task creation failures after successful session persistence. A subsequent boot hit the esp_hosted 1.4.0 streaming receive-buffer allocation assertion. The allocation sizes and contemporaneous heap state were not captured, so a shared underlying cause is not established.

The existing generated configuration prefers internal memory for ordinary allocations up to 16 KiB. Each WebSocket client allocates separate 2 KiB receive and transmit buffers with ordinary malloc.

Change

  • Set CONFIG_SPIRAM_MALLOC_ALWAYSINTERNAL=1024 only in the Tab5 example defaults.
  • Document the allocation preference, fallback behavior, and qualification limits.

Ordinary allocations above 1 KiB will prefer PSRAM. This does not change the 32 KiB internal reserve, task stack sizes or placement, TLS policy, SDIO DMA requirements, memory pools, assertions, NVS behavior, or existing SDK compatibility patch.

This is a bounded allocation-policy qualification candidate, not a claim that 1 KiB is optimal or that the SDIO failure is fixed.

Validation

  • Configuration check: exactly one cutoff assignment; removing that new line reproduces the previous defaults byte-for-byte.
  • Exact two-file scope and git diff --check passed.
  • Fresh independent review: scoped-clean through P2.
  • Exact-head CI run 34214631278, attempt 1: all five jobs passed, including Tab5 and Waveshare builds.
  • Downloaded only the Tab5 firmware artifact 10051460342: API SHA256, all 11 file checksums, manifest sizes/hashes, and all four image offsets verified. The partition table is unchanged, and the flash erase ranges exclude NVS.
  • Generated configuration compared with the previous patched Tab5 build: the only difference is CONFIG_SPIRAM_MALLOC_ALWAYSINTERNAL, from 16384 to 1024; the internal reserve remains 32768.
  • The compiled source is test merge fd1a46a189009e4b379c12aa4e27bffec1c00c31, with parents PR42 head c8e63f6b5510156ba1b12dc4e916031823a39056 and this PR head d4aae3f2f07f7d2f8a59b9df1b73c63849df6edd. Its tree matches this PR head.
  • SDK remains 362a1776ec212788fda95f75b733bfdde3a0c394, built as v5.5.5-648-g362a1776ec2-dirty, with the same verified SPM compatibility patch. This is not pristine ESP-IDF 5.5.5.
  • Symbols were retained by CI but not downloaded or matched to the embedded ELF hash in the original artifact verification. No synthetic runtime test was added for the static setting. Subsequent bounded physical evidence is recorded below; it does not establish that the allocation failures are fixed.

Source And Stack

Stacked on #42 (fix/tab5-spm-flash-compat). Its firmware/symbol packaging and exact SDK-plus-SPM-patch provenance remain unchanged.

Source inspected:

Maintainer Disposition

On September 12, 2026, accept this bounded Tab5 default using the reviewed
allocator contract, exact generated-configuration comparison, native builds,
and subsequent bounded campaign evidence. The matched fd1a46a image was
flashed with four readbacks and completed a 60-second node/operator saved-session
check without an allocation or SDK fault. Later transport/media failures were
not resolved by this setting; the cutoff is not claimed optimal or a complete
memory repair.

Existing retained sdkconfig and explicit overrides were not comprehensively
requalified by this PR. No configuration is reset during landing, and no new
claim is made that changing example defaults rewrites an existing configuration.
The original source, artifact, and SDK limits above remain applicable.
Land with a merge commit, retain the signed source objects, and keep voice and
broader hardware qualification separate.

@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 8, 2026
@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 12, 2026, 1:46 AM ET / 05:46 UTC (Revision 5).

ClawSweeper review

What this changes

The Tab5 example makes ordinary allocations above 1 KiB prefer external PSRAM and documents fallback behavior and qualification limits.

Merge readiness

Blocked before merge - 1 item remains

The change remains absent from main, has no actionable correctness finding, and has explicit maintainer acceptance of its qualification limits. The previous review’s accepted uncertainty does not require another approval cycle.

Priority: P2
Reviewed head: d4aae3f2f07f7d2f8a59b9df1b73c63849df6edd

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused configuration change with verified allocator semantics, reported native validation, and explicitly accepted qualification limits.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor proof gate and changes no authority boundary; its reported matched-image saved-session run supports bounded operation, not a demonstrated fix for allocation failures.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor proof gate and changes no authority boundary; its reported matched-image saved-session run supports bounded operation, not a demonstrated fix for allocation failures.
Evidence reviewed 8 items Verified introduced scope: The merge-base comparison contains exactly one configuration assignment and ten README lines; no runtime, dependency, workflow, storage, or authorization code changes.
Current main and merge preservation: Main lacks the cutoff assignment. The raw test-merge parents are exactly the pinned main followed by the PR head; its result changes only the two intended files. The merged SDK compatibility predecessor does not implement this allocation policy.
Authoritative allocator contract: The PR directly configures ESP-IDF’s allocator and cites this exact SDK revision. heap_caps_malloc_default selects internal memory at or below the cutoff, PSRAM above it, then retries with unrestricted default capabilities.
Findings None None.
Security None None.

How this fits together

The Tab5 firmware connects the device to OpenClaw through WebSocket clients. ESP-IDF’s allocator places their buffers and other ordinary allocations in internal RAM or external PSRAM according to the example’s configuration.

flowchart TD
 A[Tab5 configuration] --> B[ESP-IDF allocator]
 C[WebSocket buffers and ordinary allocations] --> B
 B --> D{Allocation above 1 KiB?}
 D -->|Yes| E[Prefer PSRAM]
 D -->|No| F[Prefer internal RAM]
 E --> G[Fallback if preferred memory is full]
 F --> G
Loading

Before merge

  • Resolve merge risk (P1) - Accepted qualification limit: builds adopting the default change memory placement for ordinary allocations of 1025–16384 bytes; the bounded saved-session observation does not establish performance or stability across all Tab5 workloads.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Allocation preference 16 KiB → 1 KiB; 32 KiB reserve unchanged The policy affects ordinary allocations across the Tab5 firmware, beyond the two WebSocket buffers.

Merge-risk options

Maintainer options:

  1. Honor the accepted qualification boundary (recommended)
    Proceed under the recorded maintainer acceptance without resetting retained configurations or treating this cutoff as a complete memory repair.

Technical review

Best possible solution:

Retain the accepted Tab5-specific default and existing configuration overrides, while keeping broader memory-failure diagnosis and hardware qualification separate.

Do we have a high-confidence way to reproduce the issue?

No high-confidence reproduction of the reported allocation failures is established: allocation sizes and contemporaneous heap state were not captured. The preference change itself is directly supported by the SDK source.

Is this the best way to solve the issue?

Yes, for the accepted allocation-policy adjustment: using the existing board-specific SDK setting is narrow and maintainable, preserves explicit configurations, and avoids claiming an unproven repair.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 37dedfbae80e.

Labels

Label justifications:

  • P2: This is a bounded Tab5 memory-policy improvement without a demonstrated urgent regression repair.
  • merge-risk: 🚨 compatibility: Firmware adopting the new default changes allocation placement across workloads, with the remaining qualification uncertainty explicitly accepted by the maintainer.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor proof gate and changes no authority boundary; its reported matched-image saved-session run supports bounded operation, not a demonstrated fix for allocation failures.

Evidence

What I checked:

  • Verified introduced scope: The merge-base comparison contains exactly one configuration assignment and ten README lines; no runtime, dependency, workflow, storage, or authorization code changes. (examples/m5stack-tab5-room-node/sdkconfig.defaults:7, d4aae3f2f07f)
  • Current main and merge preservation: Main lacks the cutoff assignment. The raw test-merge parents are exactly the pinned main followed by the PR head; its result changes only the two intended files. The merged SDK compatibility predecessor does not implement this allocation policy. (examples/m5stack-tab5-room-node/sdkconfig.defaults:6, 37dedfbae80e)
  • Authoritative allocator contract: The PR directly configures ESP-IDF’s allocator and cites this exact SDK revision. heap_caps_malloc_default selects internal memory at or below the cutoff, PSRAM above it, then retries with unrestricted default capabilities. (components/heap/heap_caps.c:107, 362a1776ec21)
  • Existing configuration preservation: The SDK build-system documentation states that example defaults initialize new configurations or unset values; they do not replace existing sdkconfig values. The inspected Kconfig separately defaults the cutoff to 16384 and internal reserve to 32768. (docs/en/api-guides/build-system.rst:1103, 362a1776ec21)
  • WebSocket allocation consumer: The target pins this submodule revision and configures a 2048-byte transport buffer. The dependency allocates separate receive and transmit buffers using ordinary malloc, so both are affected by the new preference. (components/esp_websocket_client/esp_websocket_client.c:859, b7f3de500a4f)
  • Maintainer disposition and bounded hardware evidence: The complete supplied PR body records September 12 acceptance, a generated configuration comparison with only 16384→1024 changed, native builds, four flash readbacks, and a 60-second saved-session check on the matched fd1a46a image without an allocation or SDK fault. It explicitly excludes comprehensive workload and retained-configuration qualification. This is reported campaign evidence, not a reviewer-executed run. (d4aae3f2f07f)

Likely related people:

  • Peter Steinberger: Raw commit 3c69d59 adds examples/m5stack-tab5-room-node/sdkconfig.defaults:6 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 3c69d597df20; files: examples/m5stack-tab5-room-node/sdkconfig.defaults)
  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (4 earlier review cycles)
  • reviewed 2026-09-08T10:19:50.173Z sha d4aae3f :: blocked before merge. :: none
  • reviewed 2026-09-08T10:57:25.847Z sha d4aae3f :: blocked before merge. :: none
  • reviewed 2026-09-12T05:25:13.468Z sha d4aae3f :: blocked before merge. :: none
  • reviewed 2026-09-12T05:34:57.937Z sha d4aae3f :: blocked before merge. :: none

@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 05:21
Base automatically changed from fix/tab5-spm-flash-compat to main September 12, 2026 05:31
@vincentkoc
vincentkoc merged commit 9b46a78 into main Sep 12, 2026
16 of 17 checks passed
@vincentkoc
vincentkoc deleted the fix/tab5-psram-allocation-policy branch September 12, 2026 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant