fix(hdd): un-poison the APA module-load latch + surface silent probe failures (Vapor) - #241
Conversation
…failures (Vapor) Vapor: APA internal HDD lists ZERO games in RiptOPL under both Auto and Manual, while the same drive lists fine in wOPL and official OPL; another tester on the SAME build has APA working. Root-caused from code (adversarially verified); two confirmed mechanisms, all fixed here except the one that is a legitimate user setting: 1. THE ONE-SHOT LATCH (upstream-verbatim code, fork-widened exposure). hddLoadModules consumes its load count on the FIRST call; if that first ATAD load fails, the failure was permanent for the session: every later call returned BUSYLOADING(2), which all `>= 0` caller tests read as SUCCESS while nothing was loaded -- then hddDetectNonSonyFileSystem devctl'd a nonexistent xhdd0:, got -1, and bailed WITH NO ERROR MESSAGE. Net effect: an empty APA page all session, Auto and Manual alike, indistinguishable from a dead drive. Upstream's earliest callers run at HDD-tab entry when the drive is ready; OUR fork added the earliest, least-gated caller -- bdmResolveBootDir's ATA escalation on a mass: boot, seconds after power-on with the platter still spinning up -- which is exactly how a USB-booting APA user (Vapor boots MMCE/USB) consumes the latch and poisons the page. FIX: a failed load resets the count (retryable -- the HDD tab gets a real second attempt); HDD_LOADMODULES_OK() replaces the four `>= 0` tests (bdmsupport.c x2, opl.c x2) so BUSYLOADING is no longer success; the silent -1 probe-failure bail now raises the HDD-not-detected error box. The genuine MBR/GPT/exFAT result (1) stays deliberately silent -- that is the normal coexistence case for BDM-HDD users and must not toast every boot. 2. THE DEFAULT-GAME-VIEW LOCK (mechanism confirmed; the value itself is a legitimate setting). default_game_view=VCD makes every VCD-capable page serve its PS1 list; on a pure-PS2 APA drive that page is EMPTY with the L3 toggle inert, its hint hidden, no rescan on SELECT, and the normal "HDD Games" title -- unlabeled and unrecoverable on-console. This commit only CLAMPS an out-of-enum config value to BOTH (the coverflow ints beside it were always clamped; this one was not). Whether a deliberate VCD lock should stay L3-escapable is a design call left to the maintainer -- flagged in the PR, not changed here. Builds clean (make clean + make opl.elf, exit 0). HW-pending: Vapor's 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 (3)
📜 Recent review details🔇 Additional comments (3)
📝 WalkthroughWalkthroughHDD module loading now distinguishes completed success from busy states, resets failed-load state for retries, refines support-module detection errors, updates HDD checks to use the new predicate, and clamps invalid saved game-view settings. ChangesHDD loading and configuration behavior
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 |
![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 improves HDD module loading reliability and configuration handling by introducing strict status checks, making module loading failures retryable, surfacing filesystem probe failures, and clamping the default game view configuration. However, the newly introduced HDD_LOADMODULES_OK macro evaluates its argument twice. When called as HDD_LOADMODULES_OK(hddLoadModules()), this causes hddLoadModules() to execute twice, leading to side effects such as double-incrementing the module load count and redundant logging. It is highly recommended to replace this macro with a static inline function to avoid double evaluation.
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.
| // after a FAILED first load consumed the one-shot count -- so "nothing is loaded" read as success and | ||
| // the APA page sat silently empty under both Auto and Manual (Vapor). Pair with the retryable-failure | ||
| // change in hddLoadModules. | ||
| #define HDD_LOADMODULES_OK(r) ((r) == HDD_LOADMODULES_STATUS_NOERROR || (r) == HDD_LOADMODULES_STATUS_ALREADYLOADED) |
There was a problem hiding this comment.
Double Evaluation of Macro Argument with Side Effects
The macro HDD_LOADMODULES_OK(r) evaluates its argument r twice when the first check (r) == HDD_LOADMODULES_STATUS_NOERROR is false (e.g., when it returns ALREADYLOADED, ERROR, or BUSYLOADING).
Because callers invoke this macro as HDD_LOADMODULES_OK(hddLoadModules()), hddLoadModules() will be executed twice per check under those conditions. Since hddLoadModules() has side effects (it increments hddModulesLoadCount and logs output), this leads to several issues:
- Reference Count Leak:
hddModulesLoadCountis incremented twice instead of once when already loaded. This prevents the count from ever reaching0during shutdown, meaninghddSetIdleImmediate()is never called, potentially causing emergency head parking and hardware wear on the drive. - Redundant Execution & Logging: Duplicate log entries and unnecessary CPU cycles are spent re-running the load checks.
Converting this macro into a static inline function resolves the double-evaluation issue safely and cleanly.
static inline int HDD_LOADMODULES_OK(int r)
{
return (r == HDD_LOADMODULES_STATUS_NOERROR || r == HDD_LOADMODULES_STATUS_ALREADYLOADED);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@include/hddsupport.h`:
- Line 74: Change include/hddsupport.h:74 to make HDD_LOADMODULES_OK evaluate
its argument exactly once via an inline helper or equivalent. In
src/bdmsupport.c:356-357 and src/bdmsupport.c:1663-1664, store each
hddLoadModules() result before evaluating the predicate; likewise update the
single checks at src/opl.c:1316 and src/opl.c:1904 so each HDD load attempt
executes once and its returned status drives detection.
In `@src/hddsupport.c`:
- Around line 325-335: Update hddDetectNonSonyFileSystem() to return distinct
status values for probe failures versus successfully detected but unrecognized
partition data. Adjust the caller in LoadSupportModules so
setErrorMessageWithCode is invoked only for the probe-failure status, while
unknown layouts retain the existing silent behavior.
- Around line 227-235: Update the module-load success logic around retLoadModule
so both the ATAD and xhdd_irx load results must succeed before setting
hddModulesLoaded or returning success/ALREADYLOADED. Preserve the retry reset
path for any required-module failure, ensuring an XHDD failure leaves
hddModulesLoadCount retryable instead of marking the HDD stack loaded.
🪄 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: 17751909-f212-486d-8741-c70196f192c6
📒 Files selected for processing (4)
include/hddsupport.hsrc/bdmsupport.csrc/hddsupport.csrc/opl.c
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: merge-variants-ps2dev-latest
- GitHub Check: check-format
- GitHub Check: build-lang-ps2dev-latest
- GitHub Check: build-ps2dev-latest
- GitHub Check: build-variants (EXTRA_FEATURES=1, PADEMU=0)
🔇 Additional comments (1)
src/opl.c (1)
1582-1588: LGTM!
| // Make the failure RETRYABLE. Leaving the count consumed turned one bad first probe into a | ||
| // whole-session poison: every later call took the else branch and returned BUSYLOADING(2), | ||
| // which the >= 0 caller tests read as SUCCESS while nothing was loaded -- so the APA page | ||
| // sat silently empty under BOTH Auto and Manual (Vapor's report; the drive lists fine in | ||
| // wOPL/upstream because their earliest callers run at tab entry, when the drive is ready -- | ||
| // our fork adds boot-time callers like bdmResolveBootDir's ATA escalation, seconds after | ||
| // power-on). sysInitDev9/sysLoadModuleBuffer are safe to re-run; a later caller (e.g. the | ||
| // HDD tab) now gets a real second attempt instead of a poisoned latch. | ||
| hddModulesLoadCount = 0; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include XHDD failures in the retry decision.
The reset is reached only when the ATAD result in retLoadModule fails, but the xhdd_irx load result is discarded. If ATAD succeeds and XHDD fails, the success branch sets hddModulesLoaded = 1, so later callers return success/ALREADYLOADED and never retry even though xhdd0: is unusable. Check every required module load before marking the stack loaded.
🤖 Prompt for 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.
In `@src/hddsupport.c` around lines 227 - 235, Update the module-load success
logic around retLoadModule so both the ATAD and xhdd_irx load results must
succeed before setting hddModulesLoaded or returning success/ALREADYLOADED.
Preserve the retry reset path for any required-module failure, ensuring an XHDD
failure leaves hddModulesLoadCount retryable instead of marking the HDD stack
loaded.
| int nonSony = hddDetectNonSonyFileSystem(); | ||
| if (nonSony != 0) { | ||
| // Drive is MBR/GPT style, or unknown, bail out or risk corrupting the drive. | ||
| LOG("HDDSUPPORT LoadSupportModules bailing out early...\n"); | ||
| LOG("HDDSUPPORT LoadSupportModules bailing out early (%d)...\n", nonSony); | ||
| // A PROBE FAILURE (-1: the xhdd0: partition-sector devctl errored -- drive not ready, module | ||
| // missing, transient bus fault) was completely SILENT: no error box, no list, an APA page that | ||
| // just looks like a dead drive for the whole session. Surface it. The 1 (genuine MBR/GPT/exFAT) | ||
| // branch stays silent on purpose -- that is the NORMAL coexistence case for BDM-HDD users and | ||
| // must not toast at every boot. | ||
| if (nonSony < 0) | ||
| setErrorMessageWithCode(_STR_HDD_NOT_CONNECTED_ERROR, ERROR_HDD_NOT_DETECTED); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish probe errors from unknown partition data.
hddDetectNonSonyFileSystem() returns -1 both for allocation/devctl failures and for merely unrecognized partition data (Lines [284-288]). This new < 0 check therefore reports _STR_HDD_NOT_CONNECTED_ERROR for connected disks with an unknown layout, despite the comment describing only probe failures. Return distinct statuses and surface the error only for actual probe failures.
🤖 Prompt for 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.
In `@src/hddsupport.c` around lines 325 - 335, Update hddDetectNonSonyFileSystem()
to return distinct status values for probe failures versus successfully detected
but unrecognized partition data. Adjust the caller in LoadSupportModules so
setErrorMessageWithCode is invoked only for the probe-failure status, while
unknown layouts retain the existing silent behavior.
#241 -- vetted, real) HDD_LOADMODULES_OK(hddLoadModules()) double-evaluated its argument: any non-NOERROR first result ran hddLoadModules() a SECOND time inside the same condition. Combined with this PR's retryable-failure change, a failed load would immediately re-run the entire load -- second dev9 init, second module attempt, second error toast -- per test site. Replaced the macro with static inline hddLoadModulesReady(), which calls once and tests the captured result. All four call sites updated. Builds clean (make clean + make opl.elf, exit 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Design question resolved (maintainer, 2026-07-20): Default Game View defaults to Both with L3 switching — that is the goal and always has been. An explicit ISO/VCD choice is the user's own deliberate setting and stays a lock (L3 inert by design). Verified the shipped code matches exactly: reset default BOTH (opl.c:2877), out-of-enum values clamp to BOTH (this PR's fix, opl.c:1587), L3 free under BOTH, and the only writer besides the default/clamp is the user's Display Settings pick. No further change needed — the lock is doctrine, not a bug. |
1 participant
Root-caused from code alone (adversarially verified), two confirmed mechanisms for Vapor's "APA lists zero games in RiptOPL, fine in wOPL/official OPL, Auto and Manual both fail":
1. The one-shot load latch (fixed). A failed FIRST
hddLoadModuleswas permanent: every later call returnedBUSYLOADING(2), which all four>= 0caller tests read as success while nothing was loaded; the follow-upxhdd0:probe then failed silently (no error box). Upstream's earliest callers run at tab entry when the drive is ready — our fork added the earliest, least-gated caller (bdmResolveBootDir's ATA escalation on amass:boot, seconds after power-on, platter still spinning up), which is exactly how a USB-booting APA user consumes the latch. Fix: failure resets the count (retryable),HDD_LOADMODULES_OK()replaces the four>= 0tests, and the silent −1 probe-failure bail now raises the HDD-not-detected error. The genuine MBR/GPT/exFAT result stays silent on purpose (normal BDM-HDD coexistence — must not toast every boot).2. The Default-Game-View VCD lock (clamped; design call flagged).
default_game_view=VCDmakes every VCD-capable page serve its PS1 list — on a pure-PS2 APA drive that's an empty, unlabeled, unrecoverable page: L3 inert, hint hidden, no SELECT rescan, normal "HDD Games" title. Upstream has no VCD view at all, which is precisely why the same drive lists fine there. This PR only clamps out-of-enum config values to BOTH (the coverflow ints beside it were always clamped).Design question for you (not changed here): should a global ISO/VCD lock stay L3-escapable for the session? Today the lock makes L3 fully inert with no on-screen explanation — that's the invisibility that made this look like a dead drive. Cheap options if you want it: show the VCD hint under the lock, or let L3 override for the session. Say the word and I'll implement your pick.
Builds clean (
make clean+make opl.elf, exit 0). HW-pending: Vapor's rig.🤖 Generated with Claude Code
Summary by CodeRabbit