Skip to content

diag(tab5): capture SDIO allocation failures - #45

Merged
vincentkoc merged 3 commits into
mainfrom
diag/tab5-sdio-allocation-failure
Sep 12, 2026
Merged

diag(tab5): capture SDIO allocation failures#45
vincentkoc merged 3 commits into
mainfrom
diag/tab5-sdio-allocation-failure

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 8, 2026

Copy link
Copy Markdown
Member

Problem

A Tab5 streaming RX assertion in sdio_rx_get_buffer does not preserve the
allocation 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

  • Add one pinned esp_hosted 1.4.0 source patch. On the first failed streaming
    buffer 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.
  • Keep the prior read's logical pending length separate from its actual
    uint16_t transfer argument after the existing padding/conversion. Neither
    is a bytes-consumed measurement; later successful reads retain the last error.
  • Apply explicitly after IDF configuration in the isolated Tab5 CI container.
    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.
  • Record modified-component provenance separately from the existing two SDK
    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

  • 10 helper tests and 18 packaging tests passed with the actual component-manager
    2.5.2 hash API, including tampering, identity, opt-in, and idempotence cases.
  • The helper now requires the canonical lock key registry_url, as emitted by
    manager 2.5.2 and present in a previously verified firmware artifact. Regression
    tests failed before the correction and passed after it; service_url-only
    input is rejected. Pin, URL value, hash, and path checks are unchanged.
  • The real pinned archive was patched and verified twice; only sdio_drv.c
    changed. It also verifies using the previously verified artifact's unchanged
    dependency lock. These checks use a synthetic compile-input fixture, not a
    native build.
  • Independent runtime-source preservation review passed. The matching parser
    passed 6 SDIO cases and 4 NVS sibling cases.
  • Initial full-diff P2 review passed with no findings. Focused correction review
    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.
  • Actionlint and code/docs git diff --check passed. The generated patch passes
    git apply --check --whitespace=error-all; raw staged diff checking flags its
    normal 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 7e6747a93d95c8905aa072cb8f86cad7f5720e4e
has the expected base/head parents and matches PR head
405bb8b81c756983b4e412453ed20479590923d6's tree. The actual SDK remains
v5.5.5-648-g362a1776ec2-dirty with 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 7e6747a image was subsequently flashed and the first failed
streaming-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 1ef33d50adb85c0f0ae55d94966bd3d21696631f preserves the
original signed head and merged main. Its complete tree
fb5121e2073a2958f4946d29f5ae02c9c9a6974e equals the frozen original PR delta
plus 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.

@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. 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: needs changes before merge. Reviewed September 12, 2026, 11:15 AM ET / 15:15 UTC (Revision 5).

ClawSweeper review

What this changes

Adds 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
Reviewed head: 1ef33d50adb85c0f0ae55d94966bd3d21696631f

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused diagnostic with careful provenance checks and no identified blocking defect; runtime qualification remains outside its demonstrated coverage.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from the external-contributor proof gate. Reported native builds and artifact checks cover integration and provenance, but do not demonstrate the driver's first-allocation-failure output; that limitation is explicitly disclosed.
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 external-contributor proof gate. Reported native builds and artifact checks cover integration and provenance, but do not demonstrate the driver's first-allocation-failure output; that limitation is explicitly disclosed.
Evidence reviewed 8 items Introduced scope and current-main comparison: The verified base-to-head delta contains seven files. Main retains the existing NVS diagnostic workflow but does not apply the SDIO probe; the qualified container image is already on main and is not introduced by this PR.
Dependency boundary and patch safeguards: The helper directly patches espressif/esp_hosted and imports the IDF component-manager hash API, establishing both dependency contracts as relevant. It checks the registry identity, manifest, source, patch, component bytes, and configured compilation path; packaging requires explicit diagnostic selection.
Pinned upstream receive path: The upstream streaming allocator asserts after allocation. Its receive task holds the driver mutex across allocation and block reads, then advances the original logical counter. The proposed instrumentation observes these values without changing those operations.
Findings None None.
Security None None.

How this fits together

Tab5 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]
Loading

Before merge

  • Complete next step (P2) - Mark this draft ready for review when its diagnostic scope is ready to land.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test growth Production/tooling +205 lines; tests +270 lines Growth comprises 44 upstream C probe lines and 161 net Python tooling lines, justified by failure capture and strict artifact provenance.

Technical review

Best 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.

Labels

Label justifications:

  • P2: This is bounded diagnostic work for an observed Tab5 receive-buffer failure, without claiming a runtime repair.
  • 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 external-contributor proof gate. Reported native builds and artifact checks cover integration and provenance, but do not demonstrate the driver's first-allocation-failure output; that limitation is explicitly disclosed.

Evidence

What I checked:

  • Introduced scope and current-main comparison: The verified base-to-head delta contains seven files. Main retains the existing NVS diagnostic workflow but does not apply the SDIO probe; the qualified container image is already on main and is not introduced by this PR. (.github/workflows/ci.yml:85, 72d37ec9dce6)
  • Dependency boundary and patch safeguards: The helper directly patches espressif/esp_hosted and imports the IDF component-manager hash API, establishing both dependency contracts as relevant. It checks the registry identity, manifest, source, patch, component bytes, and configured compilation path; packaging requires explicit diagnostic selection. (scripts/tab5_sdio_diagnostics.py:29, 1ef33d50adb8)
  • Pinned upstream receive path: The upstream streaming allocator asserts after allocation. Its receive task holds the driver mutex across allocation and block reads, then advances the original logical counter. The proposed instrumentation observes these values without changing those operations. (host/drivers/transport/sdio/sdio_drv.c:653, 6040085eefe9)
  • Actual hashing contract: Version 2.5.2 separately validates the original hash marker and recomputed manifest-aware directory hash. This supports retaining registry metadata while independently verifying the explicitly patched bytes. (idf_component_tools/hash_tools/validate.py:31, 1608b415ee77)
  • Review continuity: The previous completed review contained no findings or published rank-up moves. The diagnostic patch, helper, packaging implementation, and associated tests are unchanged from the previously reviewed head; the live PR remains a draft. (1ef33d50adb8)
  • Validation scope and related work: The captured body reports successful native builds and artifact verification for head 405bb8b, explicitly excluding runtime fault injection and hardware qualification. diag(tab5): capture first NVS and session-load failures #44 is merged NVS instrumentation; fix(tab5): allocate streaming SDIO RX buffers in PSRAM #46 remains a stacked allocation repair with separate bounded hardware observations. Neither establishes runtime coverage for every field of this probe. (405bb8b81c75)

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 (4 earlier review cycles)
  • reviewed 2026-09-08T16:42:17.294Z sha a51867f :: blocked before merge. :: none
  • reviewed 2026-09-08T17:13:21.934Z sha 405bb8b :: needs maintainer review before merge. :: none
  • reviewed 2026-09-08T17:25:30.410Z sha 405bb8b :: blocked before merge. :: none
  • reviewed 2026-09-12T06:02:28.022Z sha 405bb8b :: needs changes before merge. :: none

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

Labels

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