feat(vcd): adaptive BDMA source seek -- Custom dir, boot device, then game device (FifthFox FR) - #244
Conversation
…oot device, then the game device (FifthFox FR) FifthFox: "it should be seeked in the order of: Custom Path, CWD, and finally game's device POPS/ folder." Maintainer-approved. This is also the FAST order: the custom dir and the boot (cwd) device live on ALREADY-LOADED stacks (we booted from one and are launching from the other), so a hit there costs a few open() probes and skips the family search's transport force-loads and 2s bounded mount waits entirely -- the second half of the "BDMA operations are overboard" complaint after #243's already-equipped fast path. Seek order in vcdEquipBdma: 1. the CUSTOM POPSTARTER.ELF's own folder (gPopstarterDevice == POPS_DEV_CUSTOM) -- a user who relocated POPSTARTER keeps its drivers beside it ("<elfdir>/usbd.irx.<suffix>"); 2. the BOOT device's POPS/ folder (device root derived from gBootDir, e.g. "mmce0:/POPS/"); 3. the existing game-device family search (unchanged, now gated on !found). Adversarially verified (SHIP): buffer math in-bounds with skip-not-truncate guards; probe path shapes identical across tiers; the found gate airtight (a pre-probe hit force-loads nothing; a miss runs the family search byte-for-byte as before); src0/src1 pair atomicity per candidate; staging/commit machinery untouched; POSIX-only IO rule kept. Verifier's one advisory taken: the NULL-pointer relational compare in the separator pick rewritten to conforming C. Also confirmed for the FR: gBdmaApplyOnLaunch has been default ON since the MX4SIO->OSDSYS fix -- FifthFox's "should default to on" was already the shipped state. DEFERRED, with reasons (not silently dropped): - progress indicator during equip: the launch path blocks the GUI thread, so an honest indicator needs frame-pump machinery; with #243 + this change the slow path now runs only on a genuine one-time variant switch, so speed replaced the spinner. Revisit if HW still feels a pause. - "don't launch when BDMA is needed but missing": OPL cannot reliably know a device NEEDS the exFAT pair (a FAT32 stick launches fine via POPSTARTER's built-in driver) -- gating on equip failure would block working FAT32 launches. Needs filesystem-type detection first; the dumb-handoff contract (toast + launch anyway) stands until then. Builds clean (make opl.elf, exit 0). HW-pending: FifthFox. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (20)
🧰 Additional context used🪛 ast-grep (0.44.1)src/vcdsupport.c[error] 800-800: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf). (dangerous-buffer-functions-c) 🔇 Additional comments (1)
📝 WalkthroughWalkthrough
ChangesBDMA module pre-probe
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an adaptive seek order to locate BDMA variant files, checking the custom POPSTARTER.ELF's directory and the boot device's POPS/ folder before falling back to the standard game device search. This optimization avoids unnecessary transport force-loads and mount waits. The review feedback identifies potential path truncation issues due to small buffer sizes and recommends increasing the capacities of src0, src1, and preBuf to 256 bytes to safely accommodate longer custom POPSTARTER paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| const char *suffix = vcdBdmaSuffix[mode]; | ||
| char src0[96], src1[96]; |
There was a problem hiding this comment.
With the introduction of the adaptive seek order, src0 and src1 can now hold paths derived from gPopstarterPath (which can be up to 256 characters). A limit of 96 bytes is sufficient for root-level device paths, but can cause silent truncation for deeply nested custom POPSTARTER paths. Increasing the buffer size of src0 and src1 to 256 bytes prevents truncation and aligns with other path buffers in this file.
char src0[256], src1[256];| // one and are launching from the other), so a hit here costs a few open() probes and skips the | ||
| // family search's transport force-loads and bounded mount waits entirely. | ||
| { | ||
| char preBuf[2][96]; |
There was a problem hiding this comment.
Similarly to src0 and src1, preBuf should be increased to 256 bytes to accommodate the potentially long custom POPSTARTER paths from gPopstarterPath without skipping the search due to the bounds check n < (int)sizeof(preBuf[0]).
char preBuf[2][256];
1 participant
The FR (FifthFox, maintainer-approved)
What ships
Seek order in
vcdEquipBdma, which is also the fast order — the custom dir and boot device are on already-loaded stacks, so a hit costs a fewopen()probes and skips the family search's transport force-loads + 2s mount waits entirely (the second half of the "BDMA overboard" complaint, after #243's already-equipped fast path):POPS_DEV_CUSTOM) — drivers beside the ELF;POPS/(root derived fromgBootDir);!found).Already true:
gBdmaApplyOnLaunchhas been default ON since the MX4SIO→OSDSYS fix — the "default to on" ask was the shipped state.Adversarially verified (SHIP)
Buffer math in-bounds (skip-not-truncate); identical probe shapes across tiers; the
foundgate airtight (pre-probe hit → zero force-loads; miss → family search unchanged); per-candidate pair atomicity; staging/commit untouched; POSIX-only rule kept. Verifier's one advisory taken (NULL-pointer relational compare → conforming C).Deferred, with reasons (not silently dropped)
Reviewer notes
The pre-probe can also satisfy a Settings-screen equip from the boot/custom location (suffix encodes the variant, so the bytes are the intended pair either way). A Default-device user keeping drivers beside a boot-dir-resolved POPSTARTER falls through to the family search — no regression, just an FR coverage gap noted for the future.
Builds clean. HW-pending: FifthFox.
🤖 Generated with Claude Code
Summary by CodeRabbit