diag(tab5): capture SDIO allocation failures - #45
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: needs changes before merge. Reviewed September 12, 2026, 11:15 AM ET / 15:15 UTC (Revision 5). ClawSweeper reviewWhat this changesAdds first-failure diagnostics for Tab5 Wi-Fi receive-buffer allocations, with pinned component verification, firmware provenance, CI integration, and regression tests. Merge readiness⛔ Needs changes before merge - 1 item remains Keep open: the SDIO diagnostic remains useful and absent from main, and no actionable correctness defect was found. The merged NVS diagnostics and stacked allocation repair do not supersede it. Priority: P2 Review scores
Verification
How this fits togetherTab5 uses Espressif’s hosted Wi-Fi driver to receive data over SDIO. This change records allocation and preceding read failures before the existing assertion, while packaging identifies the instrumented firmware. flowchart LR
A[Wi-Fi coprocessor] --> B[SDIO receive driver]
B --> C[Receive buffer allocation]
C --> D[Normal packet processing]
C --> E[First failure diagnostic]
E --> F[Existing assertion]
G[Pinned component verification] --> B
G --> H[Firmware provenance manifest]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain the bounded diagnostic and exact component provenance until the SDIO cause is qualified, then retire the temporary probe alongside its packaging exception. Do we have a high-confidence way to reproduce the issue? Not applicable as a diagnostic enhancement: the source establishes the observation point, but this review did not execute an allocation failure or reproduce the underlying hardware fault. Is this the best way to solve the issue? Yes, for the stated diagnostic scope: the probe preserves the upstream allocator, assertion, reads, and counters while enforcing explicit, verifiable packaging. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 72d37ec9dce6. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
Problem
A Tab5 streaming RX assertion in
sdio_rx_get_bufferdoes not preserve theallocation request and preceding block-read state needed to distinguish the
failure's cause. This PR adds diagnostic instrumentation, not a root-cause fix.
Originally stacked on #44,
now merged. The landing branch also preserves the qualified Tab5 image from
#57.
Changes
esp_hosted1.4.0 source patch. On the first failed streamingbuffer allocation, emit a fixed numeric ROM record before the original
assertion, including request/padding, buffer capacity, DMA heap samples,
register/counter values, and the last failed block read.
uint16_ttransfer argument after the existing padding/conversion. Neitheris a bytes-consumed measurement; later successful reads retain the last error.
Validate registry identity, whole-component/source/patch hashes and compile
input before application and packaging. Reject unrelated changes and missing
diagnostic opt-in; never rewrite component-manager integrity metadata.
patches. Firmware and retained ELF/map artifacts share the same manifest.
No payloads, addresses, IDs, credentials, or normal-frame output are added.
Allocation mode, task stacks, mempools, assertions, block-read calls, counter
advancement, SDK base/patches, and configuration remain unchanged. Normal Ninja
must succeed; component-manager failures are not bypassed.
Validation
2.5.2 hash API, including tampering, identity, opt-in, and idempotence cases.
registry_url, as emitted bymanager 2.5.2 and present in a previously verified firmware artifact. Regression
tests failed before the correction and passed after it;
service_url-onlyinput is rejected. Pin, URL value, hash, and path checks are unchanged.
sdio_drv.cchanged. It also verifies using the previously verified artifact's unchanged
dependency lock. These checks use a synthetic compile-input fixture, not a
native build.
passed 6 SDIO cases and 4 NVS sibling cases.
completed; its optional-dependency finding was rejected because the existing
class-wide skip guard remains effective, confirmed by all 10 tests skipping
cleanly without the manager.
git diff --checkpassed. The generated patch passesgit apply --check --whitespace=error-all; raw staged diff checking flags itsnormal unified-diff context prefixes, not whitespace added to driver source.
The initial CI run completed four jobs successfully. Tab5 reconfiguration
succeeded, then the patch helper refused before Ninja and artifact upload.
The caught exception was not exposed by the log; the independently reproduced
lock-key bug is not claimed to be the only possible cause of that refusal.
The corrected exact-head CI run
passed all five jobs. Only the Tab5 firmware archive was downloaded: GitHub's
archive digest, all 11 checksum entries, four flash images, and the SDK and
managed-component patch provenance were verified. Configuration and partition
table match the previous verified image; the four image erase ranges exclude
NVS. Compiled test-merge
7e6747a93d95c8905aa072cb8f86cad7f5720e4ehas the expected base/head parents and matches PR head
405bb8b81c756983b4e412453ed20479590923d6's tree. The actual SDK remainsv5.5.5-648-g362a1776ec2-dirtywith the two existing tracked SDK patches,not pristine release 5.5.5. Symbols were retained by CI but not downloaded
or ELF-matched in this verification.
There is no executable SDIO allocation-failure fixture in this repository, and
CI compilation is not runtime fault-injection proof.
Subsequent Bounded Hardware Evidence
The matched
7e6747aimage was subsequently flashed and the first failedstreaming-RX allocation record was retained before the assertion. It reported
a 6024-byte request, 6144-byte padded allocation, 10915 bytes free in the DMA
heap, and a 3072-byte largest DMA block. No prior failed block read was recorded
in that event. This demonstrates one complete failure record on the tested
Tab5, not every field combination, every failure path, or broader qualification.
Maintainer Disposition
On September 12, 2026, accept the unchanged diagnostic source using its reviewed
ownership/format, host provenance checks, five successful native builds, and
that bounded physical capture. Preserve all SDK/component identity guards and
the original assertion. Land with a merge commit and retain signed source
objects. No allocator/counter repair, successful Talk flow, or complete transport
repair is claimed by this PR.
Landing Synchronization
Signed ancestry merge
1ef33d50adb85c0f0ae55d94966bd3d21696631fpreserves theoriginal signed head and merged main. Its complete tree
fb5121e2073a2958f4946d29f5ae02c9c9a6974eequals the frozen original PR deltaplus the three approved image-pin edits. Only workflow conflict context was
resolved; the SDIO source patch, strict guards, and provenance flags are intact.
Actionlint and diff checks passed. The new-head
CI run
passed all five native jobs; CodeQL also passed. Earlier physical evidence above
belongs to its recorded image, not this synchronization build.