Skip to content

fix(tab5): allocate streaming SDIO RX buffers in PSRAM - #46

Merged
vincentkoc merged 2 commits into
mainfrom
fix/tab5-sdio-rx-psram
Sep 12, 2026
Merged

fix(tab5): allocate streaming SDIO RX buffers in PSRAM#46
vincentkoc merged 2 commits into
mainfrom
fix/tab5-sdio-rx-psram

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 9, 2026

Copy link
Copy Markdown
Member

Problem

Stacked on #45; only the streaming RX allocation repair and its build/provenance checks are added here.

The Tab5's streaming RX buffer failed to grow from 3,072 to 6,144 bytes for a 6,024-byte pending read. The post-failure DMA heap sample showed 10,915 bytes free but a largest block of only 3,072 bytes. This supports a contiguous allocation failure, not a claim of a heap leak, malformed SDIO traffic, or provider failure.

Changes

  • Allocate only the two streaming RX buffers through esp_dma_capable_malloc with explicit MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT and 64-byte alignment when PSRAM and SDMMC PSRAM DMA support are enabled. The pinned SDK accounts for cache alignment. Noneligible profiles retain the original allocation.
  • Preserve 512-byte padding, growth/free behavior, assertions, synchronous reads, counters, and buffer ownership. TX, register buffers, packet pools, task stacks, allocation configuration, SDK patches, and dependency versions are unchanged.
  • Extend the strict component helper with a second pinned patch and ordered source hashes. Firmware and symbol manifests identify the final component bytes and both patches under component_compatibility_patch. No component-manager integrity bypass.
  • Keep the existing diagnostic format unchanged. Its DMA heap fields remain DMA-pool samples, not measurements of the PSRAM allocation arena.

At ESP-IDF 362a1776ec212788fda95f75b733bfdde3a0c394, the P4 capability, aligned DMA allocation, and SDMMC cache synchronization support this bounded allocation path. This remains ESP-IDF plus the existing tracked compatibility/diagnostic patches, not a pristine SDK release.

Validation

  • Actual pinned allocator C section: old-source regression demonstrated; repaired-source 4 tests pass, including the observed growth, reuse, alignment, legacy profiles, and allocation-failure assertion. These tests use allocation stubs, not real DMA.
  • Component helper: 13 tests pass with the actual component-manager hashing API.
  • Firmware packaging: 19 tests pass, including profile/capability rejection and final two-patch provenance.
  • Both patches applied and verified idempotently against the actual pinned registry component; only the intended driver source changed.
  • Workflow lint and source whitespace checks pass. The tracked unified patch's context prefixes trigger raw git diff --check warnings; git apply --check --whitespace=error-all passes against its exact input. Independent runtime review and frozen-candidate P2 review completed before publication.

All five native builds passed in CI run 34297512556, attempt 1 for PR head 2310d069b9b59e1bdabecb3524e4dc528d1a4665. The Tab5 artifact compiled test merge 853f2638ca0d50ddaf1e0668982316888d60dc8e; its parents and tree match the stacked base and PR head. The downloaded ZIP matched the official API SHA256, all 11 file checksums and four image mappings passed, and the SDK/configuration/partition table remained unchanged. Both component patches and their final source hashes were verified.

Bounded physical checks on that exact Tab5 image:

  • Four image flash/readback checks passed (4/4).
  • A 141.146-second keyless connectivity check, including one Gateway restart, passed.
  • A 42.921-second Talk attempt showed no SDIO fault, but RTC never became active. This is not a successful Talk/media test.
  • One actual camera.snap dispatch returned UNAVAILABLE from the early camera/V4L2 capture branch, with no SDIO fault and no JPEG. The failing capture stage remains unidentified; no format, timeout, or allocation fix is claimed.

These results do not establish that all memory failures are resolved or that RTC, Talk, camera, or end-to-end media are qualified.

Maintainer Disposition

On September 12, 2026, accept this narrow streaming-RX placement repair with
the pinned DMA/cache/synchronous-read contract, actual-source boundary tests,
five native builds, and bounded physical evidence above. Representative
receive-pressure and PSRAM-exhaustion qualification remain follow-ups, not
claimed results. Eligible profiles intentionally have no silent internal-DMA
fallback; PSRAM exhaustion can still reach the retained allocation assertion.
No global allocation or transport-mode change is included.

Land with a merge commit and retain the signed source objects. The earlier
camera and Talk outcomes are qualification limits, not successes; this decision
does not authorize a release or claim a complete memory or media repair.

Landing Synchronization

Signed ancestry merge d8b220b22a437c73a7fd6264c84e78153c454c45 retains the
original signed head and qualified main. Its full tree
c0a2478df592d60e5ac1be77f175000e869be78d equals the frozen RX-placement delta
on the reviewed cumulative tree, including the CI image pin. No new runtime
choice was made. This refresh was necessary because GitHub retained a
test-merge parent from the former stacked base after retargeting; the exact
parent gate was not relaxed.

New-head CI
passed all five native jobs; CodeQL also passed. The physical evidence above remains tied to its earlier image, not
this synchronization build.

@clawsweeper

clawsweeper Bot commented Sep 9, 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: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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 9, 2026
@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 12, 2026, 11:47 AM ET / 15:47 UTC (Revision 4).

ClawSweeper review

What this changes

Moves eligible Tab5 streaming receive buffers into PSRAM and adds matching build flags, firmware provenance, documentation, and allocator regression coverage.

Merge readiness

Blocked before merge - 1 item remains

The repair remains necessary: main contains allocation diagnostics, not PSRAM placement. No introduced correctness defect was found, and the recorded maintainer disposition accepts the remaining qualification limits.

Likely related people: vincentkoc, supported by prior merged work on the component verification path.

Priority: P2
Reviewed head: d8b220b22a437c73a7fd6264c84e78153c454c45

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with useful boundary coverage and preserved provenance; hardware qualification limits are clearly scoped and accepted.
Proof confidence 🌊 off-meta tidepool Not applicable: The ordinary contributor-proof gate is exempt for this MEMBER-authored PR. The reported earlier-image Tab5 connectivity run exercises SDIO traffic, but does not prove the observed growth scenario, exhaustion handling, or successful media; those limits are explicitly accepted.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The ordinary contributor-proof gate is exempt for this MEMBER-authored PR. The reported earlier-image Tab5 connectivity run exercises SDIO traffic, but does not prove the observed growth scenario, exhaustion handling, or successful media; those limits are explicitly accepted.
Evidence reviewed 11 items Policy and checkout: Repository origin matches the target. No root or nested AGENTS.md or matching maintainer-note files were found. The final working-tree status was clean; no builds or tests were executed.
Main still needs the repair: Main documents that the merged diagnostic patch retains DMA allocation and does not repair the receive-buffer assertion. The ordinary-malloc PSRAM preference explicitly does not change SDIO DMA requirements. The merged prerequisite is #45; it does not supersede this repair.
Introduced allocation boundary: The introduced patch changes only streaming-buffer allocation, selecting explicit SPIRAM/8-bit capabilities with 64-byte alignment behind PSRAM and SDMMC capability guards. Its SHA256 matches the helper's pinned value. Direct SDK API use establishes the ESP-IDF dependency contract relevant to this review.
Findings None None.
Security None None.

How this fits together

Tab5 receives Wi-Fi coprocessor traffic through an SDIO driver that grows two receive buffers before copying packets into processing queues. This change selects their memory arena while preserving the existing read and buffer ownership paths.

flowchart LR
  A[Wi-Fi coprocessor traffic] --> B[SDIO receive driver]
  B --> C[Pad receive buffer size]
  C --> D{PSRAM DMA supported?}
  D -->|Yes| E[Aligned PSRAM allocation]
  D -->|No| F[Existing DMA allocation]
  E --> G[Synchronous read and packet queues]
  F --> G
Loading

Before merge

  • Resolve merge risk (P1) - Eligible firmware now requires a sufficiently large PSRAM allocation for streaming RX; exhaustion can reach the retained assertion even when internal DMA memory remains. The maintainer explicitly accepts this tradeoff, but representative receive-pressure and exhaustion qualification remain unproven.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Net code growth Production/tooling +87 lines; tests +272 lines Production/tooling includes the tracked driver patch; growth supports the bounded allocator change and its explicit provenance checks.

Merge-risk options

Maintainer options:

  1. Honor the recorded bounded acceptance (recommended)
    Accept PSRAM-only placement for eligible streaming buffers while retaining receive-pressure and exhaustion qualification as follow-up work rather than claiming release readiness.

Technical review

Best possible solution:

Retain the narrow placement repair and strict provenance guards under the recorded maintainer acceptance, with broader hardware qualification kept separate from release approval.

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

Yes, source and the recorded hardware failure identify a 6,144-byte DMA allocation exceeding the sampled 3,072-byte largest block before the retained assertion. This review did not execute a hardware reproduction.

Is this the best way to solve the issue?

Yes, the pinned SDK supports the narrowly selected PSRAM allocation, and the existing ordinary-malloc preference does not affect this DMA path. The maintainer has explicitly accepted its exhaustion behavior.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: This addresses a captured allocation failure in the bounded Tab5 streaming receive profile.
  • merge-risk: 🚨 availability: PSRAM exhaustion can still assert without an internal-DMA fallback, an explicitly accepted but incompletely qualified availability tradeoff.
  • 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 ordinary contributor-proof gate is exempt for this MEMBER-authored PR. The reported earlier-image Tab5 connectivity run exercises SDIO traffic, but does not prove the observed growth scenario, exhaustion handling, or successful media; those limits are explicitly accepted.

Evidence

What I checked:

  • Policy and checkout: Repository origin matches the target. No root or nested AGENTS.md or matching maintainer-note files were found. The final working-tree status was clean; no builds or tests were executed. (d8b220b22a43)
  • Main still needs the repair: Main documents that the merged diagnostic patch retains DMA allocation and does not repair the receive-buffer assertion. The ordinary-malloc PSRAM preference explicitly does not change SDIO DMA requirements. The merged prerequisite is diag(tab5): capture SDIO allocation failures #45; it does not supersede this repair. (examples/m5stack-tab5-room-node/README.md:108, dc5c8917467a)
  • Introduced allocation boundary: The introduced patch changes only streaming-buffer allocation, selecting explicit SPIRAM/8-bit capabilities with 64-byte alignment behind PSRAM and SDMMC capability guards. Its SHA256 matches the helper's pinned value. Direct SDK API use establishes the ESP-IDF dependency contract relevant to this review. (patches/esp-hosted/tab5-sdio-streaming-rx-psram.patch:16, d8b220b22a43)
  • Pinned DMA allocator supports PSRAM: The pinned allocator handles explicit SPIRAM capabilities, combines DMA and cache alignment, rounds allocation size, and reports allocation failure. The P4 capability header declares SDMMC PSRAM DMA support. (components/esp_hw_support/dma/esp_dma_utils.c:162, 362a1776ec21)
  • Synchronous DMA and cache contract: The pinned SDMMC transaction owner checks buffer alignment, synchronizes cache before DMA, waits for transaction completion, and synchronizes received data afterward. These operations support the changed memory placement without adding another cache owner. (components/esp_driver_sdmmc/src/sdmmc_transaction.c:109, 362a1776ec21)
  • Receive owner and unchanged consumers: The pinned component grows the write-side streaming buffer, retains its allocation assertion, performs block reads, and transfers completed buffers to the packet-copy task. Its existing MEM_ALLOC macro requests DMA allocation with no explicit SPIRAM capability, so ordinary malloc preferences do not already implement this change. (host/drivers/transport/sdio/sdio_drv.c:654, 6040085eefe9)

Likely related people:

  • 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 (3 earlier review cycles)
  • reviewed 2026-09-09T01:04:50.055Z sha 2310d06 :: blocked before merge. :: none
  • reviewed 2026-09-09T02:23:59.577Z sha 2310d06 :: blocked before merge. :: none
  • reviewed 2026-09-12T15:30:02.169Z sha 2310d06 :: blocked before merge. :: none

@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 15:23
Base automatically changed from diag/tab5-sdio-allocation-failure to main September 12, 2026 15:25
@vincentkoc
vincentkoc merged commit 6df6c02 into main Sep 12, 2026
12 checks passed
@vincentkoc
vincentkoc deleted the fix/tab5-sdio-rx-psram branch September 12, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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