feat(hdd): APA and BDM-ATA coexist -- remove the exclusivity policy + real APA retry (HW batch S1/S2) - #249
Merged
Merged
Conversation
… give the APA page real retry (HW batch S1+S2) Maintainer directive (2026-07-21): "APA and ATA should be able to coexist. POPSLoader has them coexisting and populating - we have no excuse." Root-caused by the triage workflow (verified): the mutual block was 100% OUR EE-side policy, not a driver conflict -- OPL's "ps2atad" is the SDK's ata_bd build, which registers the drive with BDM while exporting the atad library ps2hdd links against; one shared stack already serves both, exactly like wLaunchELF-R3Z3N, NHDDL and POPSLoader. REMOVED (S1): the Device-Settings interlock (gui.c seed lines + the guiDeviceUpdater live re-grey), the #120-audit F-12 load-time reconcile that force-zeroed one flag (opl.c, + its showHddReconcilePopup plumbing; the _STR_HDD_BACKEND_RECONCILED label STAYS in _base.yml -- labels are positional and append-only), and the forced APA-disable on an ATA boot (the boot-device auto-enable stays). Per-DRIVE arbitration is untouched and remains the doctrine: hddDetectNonSonyFileSystem keeps APA off MBR/GPT/exFAT drives (corruption guard), BDM publishes massN: only where a FAT/exFAT partition exists. ADDED (S2, the empty-APA-page session): the APA list was ONE-SHOT at boot -- an hddInitModules first touch that raced drive spin-up (MC boot touches ATA seconds after power-on) left the page empty with NOTHING retrying for the whole session (and no massN: for BDM-ATA either, which reads on HW as "they block each other"). Now: - hddUpdateGameList self-heals: if the support stack is not loaded, re-run hddLoadModulesReady() + hddLoadSupportModules() before scanning (both idempotent; the failed-load count is retryable post-#241) -- wLaunchELF-R3Z3N parity: latch only on success, retry per use. One toast per failure streak (hddSupportErrToasted), reset on recovery. - xhdd's READ_PARTITION_SECTOR devctl (the EE side's readiness probe) re-probes via sceAtaInit before the transfer instead of DMA-ing against a dead devinfo -- a latched no-op on healthy paths. KNOWN RESIDUAL (documented, next PR if HW still shows it): the SDK ata_bd latches its device-info init even on a failed probe, so an in-flight spin-up race at _start can still need a reboot to clear -- fully fixing that needs a fork-vendored/patched atad (same .patch mechanism as the mmceman fixes). The retries above cover every post-probe failure; ship and see what HW says. Concurrency note carried from the review: with both stacks live, pfs and bdmfs reads can interleave ATA commands (the three reference implementations never issue concurrent IO) -- if hybrid-drive instability appears, the fix is a semaphore in the atad patch, NOT re-adding the exclusivity policy. Do NOT revert #241 (BUSYLOADING-as-failure verified correct; reverting recreates the Vapor poison). Builds clean (make opl.elf, exit 0). HW-pending: Nathan's dual-HDD rig. 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 (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (20)
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe changes remove APA/BDM-HDD interlock and reconciliation notifications, rate-limit HDD support errors, retry failed support-module initialization, and verify ATA device presence before partition-sector DMA reads. ChangesHDD coexistence and notification updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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 removes the mutual exclusivity between APA and BDM-ATA HDD modes, allowing them to coexist, and implements self-healing retry logic for support-module loading to handle boot-time drive spin-up races. The review feedback highlights two main improvements: guarding the support-module loading with a check on the base module readiness to prevent redundant error toasts, and refactoring nested conditional assignments in xhdd.c to improve code readability.
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.
Comment on lines
+591
to
+594
| if (!hddSupportModulesLoaded) { | ||
| hddLoadModulesReady(); | ||
| hddLoadSupportModules(); | ||
| } |
There was a problem hiding this comment.
The return value of hddLoadModulesReady() is ignored here. If the base modules fail to load, calling hddLoadSupportModules() unconditionally will trigger a second, redundant error toast (ERROR_HDD_NOT_DETECTED on top of ERROR_HDD_IF_NOT_DETECTED). You should guard the call to hddLoadSupportModules() with a check on hddLoadModulesReady().
Suggested change
| if (!hddSupportModulesLoaded) { | |
| hddLoadModulesReady(); | |
| hddLoadSupportModules(); | |
| } | |
| if (!hddSupportModulesLoaded) { | |
| if (hddLoadModulesReady()) { | |
| hddLoadSupportModules(); | |
| } | |
| } |
Comment on lines
+60
to
+62
| if ((devinfo = sceAtaInit(fd->unit)) == NULL || !devinfo->exists) | ||
| if ((devinfo = sceAtaInit(fd->unit)) == NULL || !devinfo->exists) | ||
| return -ENODEV; |
There was a problem hiding this comment.
The nested if statements without braces and with assignments inside the conditions are hard to read and prone to errors. It is highly recommended to separate the assignment from the condition and use explicit braces for nested blocks to improve readability and maintainability.
devinfo = sceAtaInit(fd->unit);
if (devinfo == NULL || !devinfo->exists) {
devinfo = sceAtaInit(fd->unit);
if (devinfo == NULL || !devinfo->exists) {
return -ENODEV;
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hddsupport.c`:
- Around line 338-341: Reset hddSupportErrToasted when the non-Sony probe
succeeds in the nonSony > 0 path, so a later APA failure can display a new error
toast. Preserve the existing failure handling for nonSony < 0 and only clear the
latch on a valid successful probe.
- Around line 585-595: Update the retry block around hddLoadModulesReady() so
hddLoadSupportModules() is called only when hddLoadModulesReady() reports
readiness. Preserve the existing hddSupportModulesLoaded guard and retry
behavior, but do not probe xhdd0: while the lower-level modules are still
loading or have failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b16d3332-40e3-4523-bcca-76cf167d054e
📒 Files selected for processing (5)
include/opl.hmodules/hdd/xhdd/xhdd.csrc/gui.csrc/hddsupport.csrc/opl.c
💤 Files with no reviewable changes (1)
- include/opl.h
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: build-debug-ps2dev-latest (iopcore_ppctty_debug)
- GitHub Check: build-debug-ps2dev-latest (ingame_ppctty_debug)
- GitHub Check: build-debug-ps2dev-latest (ingame_debug)
- GitHub Check: build-debug-ps2dev-latest (eesio_debug)
- GitHub Check: build-variants (EXTRA_FEATURES=0, PADEMU=1)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=1, PADEMU=1)
- GitHub Check: build-variants (EXTRA_FEATURES=0, PADEMU=0)
- GitHub Check: build
- GitHub Check: build-lang
- GitHub Check: build-ps2dev-latest
🔇 Additional comments (9)
src/opl.c (3)
198-198: LGTM!
1706-1713: LGTM!
1805-1807: LGTM!src/gui.c (3)
305-358: LGTM!
1227-1228: LGTM!
1282-1283: LGTM!src/hddsupport.c (2)
38-41: LGTM!
350-379: LGTM!modules/hdd/xhdd/xhdd.c (1)
55-63: LGTM!
… definitive probe + xhdd retry braces (bot sweep of #249 -- all vetted) - hddUpdateGameList retry now gates hddLoadSupportModules on hddLoadModulesReady() -- a failed base load no longer double-toasts on its first pass (Gemini high + CodeRabbit major, same finding). - A SUCCESSFUL non-Sony probe (genuine MBR/GPT) clears hddSupportErrToasted -- a later, different failure is new news and deserves its own toast (CodeRabbit minor). - xhdd re-probe restructured with explicit braces, assignment out of the condition (Gemini style). Builds clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The triage proved it (verified): the mutual block was 100% our own EE-side policy — there is no driver conflict. Our "ps2atad" is the SDK's
ata_bdbuild: it registers the drive with BDM while exporting the atad library ps2hdd links against. One shared stack already serves both — exactly how wLaunchELF-R3Z3N, NHDDL, and POPSLoader ship it.Removed (S1): the Device-Settings interlock (seed + live re-grey), the F-12 load-time reconcile that force-zeroed one flag (+ its popup plumbing; the lang label stays — positional/append-only), and the forced APA-disable on ATA boot (the boot-device auto-enable stays). Kept, doctrine:
hddDetectNonSonyFileSystem— the per-drive corruption guard that keeps APA off MBR/GPT/exFAT drives — and BDM's own "publish massN: only where a FAT/exFAT partition exists".Added (S2 — the empty APA page): the APA list was one-shot at boot; a first touch racing drive spin-up (MC boot hits ATA seconds after power-on) left the page empty all session with nothing retrying — and no massN: for BDM-ATA either, which reads on hardware as "they block each other". Now
hddUpdateGameListself-heals (idempotent module/support reload before scanning — wLE-R3Z3N parity: latch on success only, retry per use; one toast per failure streak), and xhdd's partition-sector devctl re-probes viasceAtaInitinstead of DMA-ing against a dead devinfo.Known residual (documented): the SDK
ata_bdlatches its init even on a failed probe; a mid-_startspin-up race can still need a reboot — fully clearing that needs a fork-patched/vendored atad (next PR if HW still shows it). Concurrency caveat carried in the commit: if hybrid-drive instability appears with both stacks live, the fix is a semaphore in that atad patch — never re-adding the exclusivity policy. #241 stays (verified correct; reverting recreates the Vapor poison).Companion PRs from the same batch: #247 (S4), #248 (S3/S5/S6/S7).
Builds clean. HW-pending: the dual-HDD rig.
🤖 Generated with Claude Code
Summary by CodeRabbit