fix(tab5): allocate streaming SDIO RX buffers in PSRAM - #46
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 12, 2026, 11:47 AM ET / 15:47 UTC (Revision 4). ClawSweeper reviewWhat this changesMoves 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 Review scores
Verification
How this fits togetherTab5 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
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest 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. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
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
esp_dma_capable_mallocwith explicitMALLOC_CAP_SPIRAM | MALLOC_CAP_8BITand 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.component_compatibility_patch. No component-manager integrity bypass.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
git diff --checkwarnings;git apply --check --whitespace=error-allpasses 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 merge853f2638ca0d50ddaf1e0668982316888d60dc8e; 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:
camera.snapdispatch returnedUNAVAILABLEfrom 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
d8b220b22a437c73a7fd6264c84e78153c454c45retains theoriginal signed head and qualified main. Its full tree
c0a2478df592d60e5ac1be77f175000e869be78dequals the frozen RX-placement deltaon 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.