Fix timer accuracy#2001
Conversation
- Implement IRQ toggle mode (bit 7): bit 10 now XORs on each IRQ instead of unconditionally setting, matching hardware behavior - Compute dotclock rate dynamically from GPU horizontal resolution and video standard instead of hardcoding rate=5 (only correct for 320px mode) - Implement gate modes 1 and 3 for Counter 0 (Hblank) and Counter 1 (Vblank): reset-at-blank and pause-until-first-blank - Properly decode Timer 2 sync mode bits (modes 0,3 stop; modes 1,2 free run) - Add 18-test hardware-verified timer test suite - Pull common.mk from fuckit branch for test infrastructure All behaviors verified on physical SCPH-5501 hardware via serial. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Hardware-verified dots/scanline values on SCPH-5501 for every GPU horizontal resolution in both NTSC and PAL modes. Confirms psx-spx documentation: 320px PAL is the only mode where the fractional dot count rounds up (425.75 -> 426) rather than truncating. All other modes truncate as expected. Tests use CESTER_MAYBE_TEST (skipped on emulator, run on hardware). Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Fix BUSY_WAIT to use compiler barrier instead of volatile local, move timer mode constants to counters.h API, replace broken cester equality assertions with typed variants, add icache warmup passes for cycle-sensitive tests, disable interrupts during test execution, and fix dotclock measurement overflow at high resolutions. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded gate-synced behavior for Counters 0 (HBlank) and 1 (VBlank), introduced rate recalculation and IRQ toggle semantics, persisted Changes
Sequence Diagram(s)sequenceDiagram
participant GPU as GPU (HBlank / VBlank)
participant Counters as Counters
participant IRQ as IRQ Controller
participant Save as SaveState
GPU->>Counters: HBlank/VBlank event
Counters->>Counters: check gateStarted and gate mode
Counters->>Counters: recalculateRate(index) if needed
Counters->>Counters: apply gate behavior (reset, pause, free-run) and update counter
alt target or overflow reached
Counters->>Counters: set hit-target/overflow bits
Counters->>IRQ: set or toggle IRQ request flag (based on RcIrqToggle)
end
Counters->>Save: serialize/deserialize gateStarted and rates during save/restore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/core/psxcounters.cc`:
- Around line 231-247: Add handling for gate mode 0 for counter 1: inside the
switch on gateSyncMode(m_rcnts[1].mode) add a case 0 that implements "pause
during VBlank" the same way counter 0 does — update m_rcnts[1].gateStarted
appropriately, call recalculateRate(1) to refresh the counter rate, and ensure
the counter is paused (adjust writes via writeCounterInternal(1, ...) or leave
value unchanged per counter 0 logic) and call set() if the counter start/stop
state changes; use the same sequence of operations and flag updates as in the
counter 0 implementation to keep behavior consistent.
- Around line 194-210: The switch in the Counter 0 gate handling is missing case
0 (pause during HBlank); add a case 0 branch so that when
gateSyncMode(m_rcnts[0].mode) == 0 you reset/pause the counter (e.g., call
writeCounterInternal(0, 0)) just like modes 1 and 2 do; update the switch inside
the isGateEnabled(0, m_rcnts[0].mode) block so Counter 0 pauses during HBlank
consistent with Timer 2 behavior, keeping existing calls to
recalculateRate(0)/set()/m_rcnts[0].gateStarted only where appropriate.
In `@src/mips/tests/timers/timers.c`:
- Around line 296-355: The test timerC0GateMode0PauseDuringHblank asserts gate
mode 0 pauses during HBlank but psxcounters.cc's update() HBlank handling
doesn't implement TM_SYNC_MODE(0); either implement the missing gate-mode-0
behavior inside the psxcounters.cc update() HBlank branch (detect TM_SYNC_EN |
TM_SYNC_MODE(0) and pause counting during HBlank) or change the test to be
conditional/optional by replacing CESTER_TEST with CESTER_MAYBE_TEST (or
otherwise guard it when PCS X_TESTS=1) so it is skipped when the emulator lacks
that feature.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa019cc6-9b5e-4572-b34d-491cf7f8e095
📒 Files selected for processing (8)
src/core/psxcounters.ccsrc/core/psxcounters.hsrc/core/sstate.ccsrc/core/sstate.hsrc/mips/common/hardware/counters.hsrc/mips/tests/common.mksrc/mips/tests/timers/Makefilesrc/mips/tests/timers/timers.c
| // Counter 0 gate: triggered by Hblank | ||
| if (isGateEnabled(0, m_rcnts[0].mode)) { | ||
| switch (gateSyncMode(m_rcnts[0].mode)) { | ||
| case 1: // Reset at Hblank | ||
| case 2: // Reset at Hblank + pause outside | ||
| writeCounterInternal(0, 0); | ||
| break; | ||
| case 3: // Pause until first Hblank, then free run | ||
| if (!m_rcnts[0].gateStarted) { | ||
| m_rcnts[0].gateStarted = 1; | ||
| recalculateRate(0); | ||
| writeCounterInternal(0, 0); | ||
| set(); | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if gate mode 0 is expected to pause during Hblank based on test expectations
rg -n "GateMode0" src/mips/tests/timers/timers.c -A 15Repository: grumpycoders/pcsx-redux
Length of output: 631
🏁 Script executed:
# Get more context around the code lines mentioned
head -220 src/core/psxcounters.cc | tail -50Repository: grumpycoders/pcsx-redux
Length of output: 1577
🏁 Script executed:
# Search for mode 0 handling elsewhere in the file
rg -n "mode.*0|case 0" src/core/psxcounters.cc | head -30Repository: grumpycoders/pcsx-redux
Length of output: 600
🏁 Script executed:
# Check what happens when gate is enabled but switch doesn't match any case
rg -n "isGateEnabled|gateSyncMode" src/core/psxcounters.cc -B 2 -A 10 | head -50Repository: grumpycoders/pcsx-redux
Length of output: 2308
🏁 Script executed:
# Look for the function signature to understand the full context
rg -n "void.*Hblank|onHblank" src/core/psxcounters.cc -B 2 -A 30Repository: grumpycoders/pcsx-redux
Length of output: 49
🏁 Script executed:
# Find gateSyncMode function definition
rg -n "gateSyncMode" src/core/psxcounters.cc -B 2 -A 5Repository: grumpycoders/pcsx-redux
Length of output: 1684
🏁 Script executed:
# Find isGateEnabled function definition
rg -n "isGateEnabled" src/core/psxcounters.cc -B 2 -A 5Repository: grumpycoders/pcsx-redux
Length of output: 1187
🏁 Script executed:
# Search for any references to mode 0 behavior in comments or code
rg -n "mode 0|sync.*0|SYNC_MODE" src/core/psxcounters.cc | head -20Repository: grumpycoders/pcsx-redux
Length of output: 198
🏁 Script executed:
# Find header files and search for function definitions
fd -e "\.h" -e "\.hpp" src/core/ | head -20Repository: grumpycoders/pcsx-redux
Length of output: 408
🏁 Script executed:
# Search for function definitions in header files
rg -n "gateSyncMode|isGateEnabled" src/core/ -g "*.h" -g "*.hpp" -B 2 -A 3Repository: grumpycoders/pcsx-redux
Length of output: 575
🏁 Script executed:
# Check the complete writeCounterInternal and recalculateRate to understand gate behavior
rg -n "writeCounterInternal|recalculateRate" src/core/psxcounters.cc -B 2 -A 8 | head -80Repository: grumpycoders/pcsx-redux
Length of output: 3007
Add missing gate mode 0 handling for Counter 0 (pause during HBlank).
The switch statement at lines 196-209 handles gate modes 1, 2, and 3, but mode 0 is missing. When gateSyncMode() returns 0, the switch falls through without any action, causing the counter to run at full speed instead of pausing during HBlank. The test timerC0GateMode0PauseDuringHblank expects mode 0 to produce a lower counter value than free-running, confirming the pause behavior should be implemented. This is inconsistent with Timer 2, which explicitly pauses on mode 0 (line 303-305).
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 210-210: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING1024'
(syntaxError)
[error] 197-197: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING256'
(syntaxError)
[error] 206-206: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING512'
(syntaxError)
🪛 GitHub Check: CodeScene Code Health Review (main)
[warning] 194-247: ❌ Getting worse: Complex Method
PCSX::Counters::update increases in cyclomatic complexity from 11 to 21, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 194-247: ❌ New issue: Bumpy Road Ahead
PCSX::Counters::update has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 194-247: ❌ New issue: Deep, Nested Complexity
PCSX::Counters::update has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/psxcounters.cc` around lines 194 - 210, The switch in the Counter 0
gate handling is missing case 0 (pause during HBlank); add a case 0 branch so
that when gateSyncMode(m_rcnts[0].mode) == 0 you reset/pause the counter (e.g.,
call writeCounterInternal(0, 0)) just like modes 1 and 2 do; update the switch
inside the isGateEnabled(0, m_rcnts[0].mode) block so Counter 0 pauses during
HBlank consistent with Timer 2 behavior, keeping existing calls to
recalculateRate(0)/set()/m_rcnts[0].gateStarted only where appropriate.
| // Counter 1 gate: triggered by VBlank start | ||
| if (isGateEnabled(1, m_rcnts[1].mode)) { | ||
| switch (gateSyncMode(m_rcnts[1].mode)) { | ||
| case 1: // Reset at VBlank | ||
| case 2: // Reset at VBlank + pause outside | ||
| writeCounterInternal(1, 0); | ||
| break; | ||
| case 3: // Pause until first VBlank, then free run | ||
| if (!m_rcnts[1].gateStarted) { | ||
| m_rcnts[1].gateStarted = 1; | ||
| recalculateRate(1); | ||
| writeCounterInternal(1, 0); | ||
| set(); | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Gate mode 0 (pause during VBlank) also missing for counter 1.
Similar to counter 0, gate mode 0 for counter 1 (pause during VBlank) is not implemented. The switch only handles modes 1, 2, and 3.
🧰 Tools
🪛 GitHub Check: CodeScene Code Health Review (main)
[warning] 194-247: ❌ Getting worse: Complex Method
PCSX::Counters::update increases in cyclomatic complexity from 11 to 21, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 194-247: ❌ New issue: Bumpy Road Ahead
PCSX::Counters::update has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 194-247: ❌ New issue: Deep, Nested Complexity
PCSX::Counters::update has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/psxcounters.cc` around lines 231 - 247, Add handling for gate mode 0
for counter 1: inside the switch on gateSyncMode(m_rcnts[1].mode) add a case 0
that implements "pause during VBlank" the same way counter 0 does — update
m_rcnts[1].gateStarted appropriately, call recalculateRate(1) to refresh the
counter rate, and ensure the counter is paused (adjust writes via
writeCounterInternal(1, ...) or leave value unchanged per counter 0 logic) and
call set() if the counter start/stop state changes; use the same sequence of
operations and flag updates as in the counter 0 implementation to keep behavior
consistent.
nicolasnoble
force-pushed
the
fix-timer-accuracy
branch
from
f55863a to
89ee7fb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/pcsxrunner/timers.cc (1)
23-35: Factor shared runner arguments to avoid drift.Both tests duplicate the same invocation setup except one mode flag. A shared helper (or parameterized test) will be easier to maintain.
♻️ Suggested refactor
+namespace { +void runTimers(const char* cpuMode) { + MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", cpuMode, + "-luacov", "-loadexe", "src/mips/tests/timers/timers.ps-exe"); + EXPECT_EQ(invoker.invoke(), 0); +} +} // namespace + TEST(Timers, Interpreter) { - MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", "-interpreter", - "-luacov", "-loadexe", "src/mips/tests/timers/timers.ps-exe"); - int ret = invoker.invoke(); - EXPECT_EQ(ret, 0); + runTimers("-interpreter"); } TEST(Timers, Dynarec) { - MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", "-dynarec", - "-luacov", "-loadexe", "src/mips/tests/timers/timers.ps-exe"); - int ret = invoker.invoke(); - EXPECT_EQ(ret, 0); + runTimers("-dynarec"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pcsxrunner/timers.cc` around lines 23 - 35, Extract the duplicated MainInvoker setup into a shared helper or a parameterized test so the only difference (the "-interpreter" vs "-dynarec" flag) is supplied per-case; for example, add a helper function (e.g., CreateTimersInvoker or RunTimersWithMode) that constructs MainInvoker with the common args ("-no-ui","-run","-bios","src/mips/openbios/openbios.bin","-testmode","-luacov","-loadexe","src/mips/tests/timers/timers.ps-exe") and accepts the mode flag, then call that helper from the tests (TEST(Timers, Interpreter) and TEST(Timers, Dynarec)) and assert the invoke() return as before to remove duplicated argument lists and reduce drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/pcsxrunner/timers.cc`:
- Around line 23-35: Extract the duplicated MainInvoker setup into a shared
helper or a parameterized test so the only difference (the "-interpreter" vs
"-dynarec" flag) is supplied per-case; for example, add a helper function (e.g.,
CreateTimersInvoker or RunTimersWithMode) that constructs MainInvoker with the
common args
("-no-ui","-run","-bios","src/mips/openbios/openbios.bin","-testmode","-luacov","-loadexe","src/mips/tests/timers/timers.ps-exe")
and accepts the mode flag, then call that helper from the tests (TEST(Timers,
Interpreter) and TEST(Timers, Dynarec)) and assert the invoke() return as before
to remove duplicated argument lists and reduce drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79dd5474-b257-46b5-8941-7375a186897d
📒 Files selected for processing (1)
tests/pcsxrunner/timers.cc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/pcsxrunner/timers.cc (1)
23-35: Consolidate duplicated backend tests into a parameterized test.Line 24 and Line 31 only differ by CPU flag. Converting this into one parameterized test reduces drift when launch arguments evolve.
Refactor sketch
+namespace { +int runTimers(const char* cpuMode) { + MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", cpuMode, + "-luacov", "-loadexe", "src/mips/tests/timers/timers.ps-exe"); + return invoker.invoke(); +} +} // namespace + -TEST(Timers, Interpreter) { - MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", "-interpreter", - "-luacov", "-loadexe", "src/mips/tests/timers/timers.ps-exe"); - int ret = invoker.invoke(); - EXPECT_EQ(ret, 0); -} - -TEST(Timers, Dynarec) { - MainInvoker invoker("-no-ui", "-run", "-bios", "src/mips/openbios/openbios.bin", "-testmode", "-dynarec", - "-luacov", "-loadexe", "src/mips/tests/timers/timers.ps-exe"); - int ret = invoker.invoke(); - EXPECT_EQ(ret, 0); -} +class TimersBackendTest : public ::testing::TestWithParam<const char*> {}; + +TEST_P(TimersBackendTest, ExecutesTimersProgram) { EXPECT_EQ(runTimers(GetParam()), 0); } + +INSTANTIATE_TEST_SUITE_P(Timers, TimersBackendTest, ::testing::Values("-interpreter", "-dynarec"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pcsxrunner/timers.cc` around lines 23 - 35, Replace the two near-duplicate tests TEST(Timers, Interpreter) and TEST(Timers, Dynarec) with a single parameterized test using gtest TEST_P/INSTANTIATE_TEST_SUITE_P: create a fixture (e.g., TimersTest) that takes a CPU-mode string parameter, call MainInvoker with that parameter instead of hardcoding "-interpreter"/"-dynarec", invoke MainInvoker.invoke() and EXPECT_EQ(ret, 0) inside the TEST_P body, then instantiate the suite with the two CPU flag values ("-interpreter", "-dynarec") so behavior of MainInvoker and invoke() remains identical but the launch-argument list is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/pcsxrunner/timers.cc`:
- Around line 20-21: The tests fail to compile because the googletest submodule
is not initialized so the include "gtest/gtest.h" in tests/pcsxrunner/timers.cc
(and other tests) cannot be found; fix this by initializing and populating the
submodule (run git submodule update --init --recursive or reclone with
--recursive), confirm third_party/googletest contains googletest/ include files,
then reload/regen your build (CMake/Visual Studio) so the existing include path
to third_party/googletest/googletest/include is recognized and the gtest headers
can be resolved.
---
Nitpick comments:
In `@tests/pcsxrunner/timers.cc`:
- Around line 23-35: Replace the two near-duplicate tests TEST(Timers,
Interpreter) and TEST(Timers, Dynarec) with a single parameterized test using
gtest TEST_P/INSTANTIATE_TEST_SUITE_P: create a fixture (e.g., TimersTest) that
takes a CPU-mode string parameter, call MainInvoker with that parameter instead
of hardcoding "-interpreter"/"-dynarec", invoke MainInvoker.invoke() and
EXPECT_EQ(ret, 0) inside the TEST_P body, then instantiate the suite with the
two CPU flag values ("-interpreter", "-dynarec") so behavior of MainInvoker and
invoke() remains identical but the launch-argument list is centralized.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e1bcec5-6597-474a-ab42-b4221a014ee2
📒 Files selected for processing (3)
tests/pcsxrunner/timers.ccvsprojects/tests/pcsxrunner/pcsxrunner.vcxprojvsprojects/tests/pcsxrunner/pcsxrunner.vcxproj.filters
✅ Files skipped from review due to trivial changes (2)
- vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj.filters
- vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
| #include "gtest/gtest.h" | ||
| #include "main/main.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== GoogleTest include usage in pcsxrunner tests =="
rg -nP --type=cc --type=cpp '^\s*#include\s*[<"]gtest/gtest\.h[>"]' tests/pcsxrunner || true
echo
echo "== timers.cc wiring in build files (CMake + Visual Studio) =="
rg -n --iglob '*CMakeLists.txt' --iglob '*.cmake' --iglob '*.vcxproj' --iglob '*.vcxproj.filters' \
'timers\.cc|tests/pcsxrunner/timers\.cc|tests\\pcsxrunner\\timers\.cc'
echo
echo "== pcsxrunner/gtest linkage hints in CMake files =="
rg -n --iglob '*CMakeLists.txt' --iglob '*.cmake' 'pcsxrunner|gtest|googletest'Repository: grumpycoders/pcsx-redux
Length of output: 512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Check if timers.cc exists and show the includes ==="
if [ -f tests/pcsxrunner/timers.cc ]; then
head -n 25 tests/pcsxrunner/timers.cc
else
echo "ERROR: tests/pcsxrunner/timers.cc not found"
fi
echo
echo "=== 2. Find all CMakeLists.txt files and check for pcsxrunner/tests/gtest ==="
find . -name "CMakeLists.txt" -type f | head -20
echo
echo "=== 3. Check main CMakeLists.txt for test configuration ==="
if [ -f CMakeLists.txt ]; then
grep -n -i "gtest\|googletest\|pcsxrunner" CMakeLists.txt || echo "No gtest/pcsxrunner in main CMakeLists.txt"
fi
echo
echo "=== 4. Check if there's a tests/ CMakeLists.txt ==="
if [ -f tests/CMakeLists.txt ]; then
cat -n tests/CMakeLists.txt
fi
echo
echo "=== 5. Check if there's a tests/pcsxrunner/ CMakeLists.txt ==="
if [ -f tests/pcsxrunner/CMakeLists.txt ]; then
cat -n tests/pcsxrunner/CMakeLists.txt
fiRepository: grumpycoders/pcsx-redux
Length of output: 2432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Check vcxproj file for include directories and gtest ==="
if [ -f "vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj" ]; then
grep -n -i "include\|gtest" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj | head -30
fi
echo
echo "=== 2. Search for GoogleTest in the repository ==="
find . -name "*gtest*" -o -name "*googletest*" 2>/dev/null | grep -v ".git" | head -20
echo
echo "=== 3. Check if there's a gtest or googletest directory anywhere ==="
find . -type d \( -name "gtest" -o -name "googletest" \) 2>/dev/null
echo
echo "=== 4. Look for other test files in tests/pcsxrunner ==="
ls -la tests/pcsxrunner/ 2>/dev/null || echo "tests/pcsxrunner/ does not exist or cannot be listed"
echo
echo "=== 5. Search repo for `#include` \"gtest/gtest.h\" pattern (without file type restriction) ==="
rg '#include.*gtest/gtest' --max-count 10
echo
echo "=== 6. Check main vcxproj directory structure ==="
ls -la vsprojects/tests/pcsxrunner/ 2>/dev/null | head -20Repository: grumpycoders/pcsx-redux
Length of output: 4623
🏁 Script executed:
#!/bin/bash
set -euo unpipefail
echo "=== 1. Check gtest.vcxproj for include directory configuration ==="
if [ -f "vsprojects/gtest/gtest.vcxproj" ]; then
grep -n -A 5 "AdditionalIncludeDirectories\|IncludePath" vsprojects/gtest/gtest.vcxproj | head -40
fi
echo
echo "=== 2. Check pcsxrunner.vcxproj for include directories ==="
if [ -f "vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj" ]; then
grep -n "AdditionalIncludeDirectories\|IncludePath" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj | head -20
fi
echo
echo "=== 3. Look for third_party googletest structure ==="
ls -la third_party/googletest/
echo
echo "=== 4. Check if there's any CMake build system alongside VS ==="
find . -name "CMakeLists.txt" -type f 2>/dev/null | wc -l
echo "Total CMakeLists.txt files found"
echo
echo "=== 5. Confirm that pcsxrunner project is VS-only (check for CMake) ==="
grep -r "add_executable.*pcsxrunner\|add_test.*timers" . 2>/dev/null || echo "No CMake references to pcsxrunner tests found"Repository: grumpycoders/pcsx-redux
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Check gtest.vcxproj for include directory configuration ==="
if [ -f "vsprojects/gtest/gtest.vcxproj" ]; then
grep -n -A 5 "AdditionalIncludeDirectories\|IncludePath" vsprojects/gtest/gtest.vcxproj | head -40
fi
echo
echo "=== 2. Check pcsxrunner.vcxproj for include directories ==="
if [ -f "vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj" ]; then
grep -n "AdditionalIncludeDirectories\|IncludePath" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj | head -20
fi
echo
echo "=== 3. Look for third_party googletest structure ==="
ls -la third_party/googletest/
echo
echo "=== 4. Check googletest include directory ==="
find third_party/googletest -name "gtest.h" 2>/dev/null
echo
echo "=== 5. Check if there's any CMake build system alongside VS ==="
find . -name "CMakeLists.txt" -type f 2>/dev/null | wc -l
echo "Total CMakeLists.txt files found"
echo
echo "=== 6. Search for any CMake references to pcsxrunner or tests ==="
rg "add_executable.*pcsxrunner|add_test.*timers|tests/pcsxrunner" --max-count 20 || echo "No CMake references to pcsxrunner tests found"Repository: grumpycoders/pcsx-redux
Length of output: 1550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Get full pcsxrunner.vcxproj file to check all configurations ==="
wc -l vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
echo
echo "=== 2. Extract all PropertyGroup sections from pcsxrunner.vcxproj ==="
grep -n "PropertyGroup\|AdditionalIncludeDirectories" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj | head -50
echo
echo "=== 3. Check if gtest include dirs should be inherited or explicitly set ==="
head -150 vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj | tail -50
echo
echo "=== 4. Look for how other test projects (e.g., support tests) handle includes ==="
find vsprojects -name "*.vcxproj" -path "*/tests/*" -type f | head -5
echo
echo "=== 5. Check one of the other test vcxproj files ==="
if [ -f "vsprojects/tests/support/support.vcxproj" ]; then
grep -n "AdditionalIncludeDirectories\|gtest" vsprojects/tests/support/support.vcxproj | head -20
fiRepository: grumpycoders/pcsx-redux
Length of output: 4246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Check testsupport.vcxproj for AdditionalIncludeDirectories ==="
grep -n "AdditionalIncludeDirectories" vsprojects/tests/support/testsupport.vcxproj | head -20
echo
echo "=== 2. Read full compile settings from pcsxrunner.vcxproj (lines 60-150) ==="
sed -n '60,150p' vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
echo
echo "=== 3. Search for any gtest include path configuration in pcsxrunner ==="
grep -i "third_party\|googletest\|gtest" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
echo
echo "=== 4. Check if there's a shared property sheet that includes paths ==="
find vsprojects -name "*.props" | head -10
echo
echo "=== 5. Check if pcsxrunner imports any property sheets ==="
grep -n "Import\|props" vsprojects/tests/pcsxrunner/pcsxrunner.vcxprojRepository: grumpycoders/pcsx-redux
Length of output: 136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Check if testsupport.vcxproj exists ==="
ls -la vsprojects/tests/support/
echo
echo "=== 2. Read all ClCompile settings from pcsxrunner.vcxproj ==="
sed -n '55,120p' vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
echo
echo "=== 3. Search for Import statements in pcsxrunner.vcxproj ==="
grep -n "Import" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
echo
echo "=== 4. Check main vsprojects directory for property sheets ==="
ls -la vsprojects/*.props 2>/dev/null || echo "No .props files in root vsprojects"
echo
echo "=== 5. Look at how gtest is built - check if it's a static lib ==="
grep -n "LibraryOutput\|StaticLibrary\|DynamicLibrary" vsprojects/gtest/gtest.vcxproj | head -10
echo
echo "=== 6. Check what the pcsxrunner project outputs ==="
grep -n "OutDir\|IntDir\|TargetName\|ConfigurationType" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj | head -20Repository: grumpycoders/pcsx-redux
Length of output: 7313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Read common.props file ==="
cat -n vsprojects/common.props
echo
echo "=== 2. Check if gtest include is provided through project reference inheritance ==="
grep -B 5 -A 5 "gtest" vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
echo
echo "=== 3. Verify third_party/googletest/googletest contains gtest.h ==="
find third_party/googletest/googletest -name "gtest.h" -o -name "include" -type dRepository: grumpycoders/pcsx-redux
Length of output: 3895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Check what actually exists in third_party/googletest ==="
ls -la third_party/googletest/ 2>/dev/null || echo "third_party/googletest/ does not exist"
echo
echo "=== 2. Search for gtest.h in third_party ==="
find third_party -name "gtest.h" 2>/dev/null | head -10
echo
echo "=== 3. List directory structure of third_party ==="
ls -la third_party/ | head -30
echo
echo "=== 4. Check if googletest is a git submodule ==="
cat .gitmodules 2>/dev/null | grep -i google || echo "No googletest submodule found in .gitmodules"
echo
echo "=== 5. Check git status for third_party/googletest ==="
git ls-files third_party/googletest 2>/dev/null | head -5 || echo "googletest not in git tracking"
echo
echo "=== 6. Look at what the common.props path actually resolves to ==="
echo "common.props references: \$(SolutionDir)..\\third_party\\googletest\\googletest\\include"
echo "This would be: <repo-root>/third_party/googletest/googletest/include"
ls -la third_party/googletest* 2>/dev/null || echo "No googletest* found"Repository: grumpycoders/pcsx-redux
Length of output: 2698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Confirm all pcsxrunner test files have same gtest include ==="
grep -l '#include "gtest/gtest.h"' tests/pcsxrunner/*.cc | wc -l
echo "test files in pcsxrunner with gtest include"
echo
echo "=== Check test/support files as well ==="
grep -l '#include "gtest/gtest.h"' tests/support/*.cc | wc -l
echo "test files in support with gtest include"
echo
echo "=== Verify submodule status ==="
git config --file .gitmodules --name-only --get-regexp path | wc -l
echo "total submodules defined"
echo
echo "=== Check if submodule init is documented anywhere ==="
grep -r "submodule\|git clone.*recursive" README.md CONTRIBUTING.md 2>/dev/null || echo "No submodule init docs found in standard files"Repository: grumpycoders/pcsx-redux
Length of output: 521
GoogleTest submodule is not initialized—this blocks all test compilation.
The 'gtest/gtest.h' file not found error at Line 20 is confirmed. The googletest submodule is defined but not checked out (empty directory at third_party/googletest). The Visual Studio project wiring is correct (common.props properly includes $(SolutionDir)..\third_party\googletest\googletest\include), but the header cannot be found because the dependency is missing.
This affects 18 test files across tests/pcsxrunner/ (11 files) and tests/support/ (7 files). Per README.md, clone the repository recursively:
git clone --recursive https://github.com/grumpycoders/pcsx-redux.git
🧰 Tools
🪛 Clang (14.0.6)
[error] 20-20: 'gtest/gtest.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/pcsxrunner/timers.cc` around lines 20 - 21, The tests fail to compile
because the googletest submodule is not initialized so the include
"gtest/gtest.h" in tests/pcsxrunner/timers.cc (and other tests) cannot be found;
fix this by initializing and populating the submodule (run git submodule update
--init --recursive or reclone with --recursive), confirm third_party/googletest
contains googletest/ include files, then reload/regen your build (CMake/Visual
Studio) so the existing include path to
third_party/googletest/googletest/include is recognized and the gtest headers
can be resolved.
89ee7fb to
4c860eb
Compare
Gate modes 0 (pause during blank) and 2 (pause outside blank) are not implemented in the emulator. Mark those tests as CESTER_MAYBE_TEST so they're skipped when PCSX_TESTS=1 (CI) but still run on hardware. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mips/tests/timers/timers.c`:
- Line 403: The two busy-wait loops that poll COUNTERS[1].value (the loop at
"while (COUNTERS[1].value < 10) {}" and the loop at "while (COUNTERS[1].value <
scanlines) {}") need timeout guards so the test cannot hang if the external
hsync clock stops; replace each infinite poll with a bounded wait that tracks
elapsed iterations or time (e.g., max_iterations or deadline) and returns an
error/timeout code on expiry, and update the callers/tests to assert the
function’s return value rather than assuming success. Locate the loops in
timers.c, add a timeout variable and break/return path on timeout, and ensure
the calling tests check the returned status to fail fast with a helpful
diagnostic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ce0a300-7585-463e-a2bd-4f13719139b8
📒 Files selected for processing (1)
src/mips/tests/timers/timers.c
| COUNTERS[0].mode = TM_CLK_EXTERNAL; | ||
|
|
||
| /* Wait for 10 scanlines */ | ||
| while (COUNTERS[1].value < 10) {} |
There was a problem hiding this comment.
Consider adding timeout guards for external clock polling loops.
The while loops at lines 403 and 445 poll external clock sources (hsync) without any timeout:
while (COUNTERS[1].value < 10) {} // Line 403
while (COUNTERS[1].value < scanlines) {} // Line 445If the external clock source is not functioning (e.g., GPU misconfiguration, emulator issue), these loops will hang indefinitely, causing the test suite to freeze without diagnostics.
🛡️ Suggested improvement with timeout
- while (COUNTERS[1].value < scanlines) {}
+ int timeout = 10000000;
+ while (COUNTERS[1].value < scanlines && --timeout > 0) {}
+ if (timeout <= 0) {
+ /* Clock source not responding */
+ return -1;
+ }Then assert on the return value in calling tests.
Also applies to: 445-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mips/tests/timers/timers.c` at line 403, The two busy-wait loops that
poll COUNTERS[1].value (the loop at "while (COUNTERS[1].value < 10) {}" and the
loop at "while (COUNTERS[1].value < scanlines) {}") need timeout guards so the
test cannot hang if the external hsync clock stops; replace each infinite poll
with a bounded wait that tracks elapsed iterations or time (e.g., max_iterations
or deadline) and returns an error/timeout code on expiry, and update the
callers/tests to assert the function’s return value rather than assuming
success. Locate the loops in timers.c, add a timeout variable and break/return
path on timeout, and ensure the calling tests check the returned status to fail
fast with a helpful diagnostic.
When target is 0xFFFF, the CountToTarget and CountToOverflow events coincide at the same counter value. Skip the redundant target phase and schedule CountToOverflow directly, so the overflow flag is visible on the first mode register read without requiring two update() calls. Restore small target values for tests that only check flag presence, keeping the larger target only for the back-to-back read test. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
nicolasnoble
force-pushed
the
fix-timer-accuracy
branch
from
a639859 to
1a96364
Compare
Hardware sets bit 10 (IRQ request) to 1 when the mode register is written. The emulator was storing the raw written value, leaving bit 10 at 0. This caused the toggle mode test to fail because the XOR toggle started from 0 instead of 1. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
1 participant
No description provided.