New CD-Rom code. #1129
New CD-Rom code. #1129
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1129 +/- ##
===========================================
+ Coverage 9.94% 20.46% +10.52%
===========================================
Files 448 449 +1
Lines 132699 132249 -450
===========================================
+ Hits 13200 27069 +13869
+ Misses 119499 105180 -14319 ☔ View full report in Codecov by Sentry. |
nicolasnoble
force-pushed
the
fuckit
branch
2 times, most recently
from
f3c0ecc to
94444c4
Compare
# Conflicts: # .github/workflows/linux-build.yml
# Conflicts: # .github/workflows/linux-build.yml # src/core/DynaRec_aa64/recompiler.cc # src/core/DynaRec_x64/recompiler.cc # src/core/cdrom.cc # src/core/r3000a.cc # src/core/r3000a.h # src/core/sio.h # src/core/sstate.h # src/gui/widgets/registers.cc
# Conflicts: # src/core/cdrom.cc # src/core/luaiso.cc # src/core/psxinterpreter.cc # src/core/r3000a.h
WalkthroughThis pull request makes extensive changes across build workflows, core modules, GUI components, and test suites. New steps in GitHub Actions now download and extract a UNIROM boot disc for building and testing. The CD-ROM handling has been enhanced with new methods, scheduling callbacks, and operator overloads for time (MSF) operations. Several interrupt and scheduling routines have been refactored to use a unified Schedule enum. The GUI now calls the updated parseIso() for ISO processing, and numerous new tests and Makefile simplifications for MIPS targets have been added. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant CDROM
participant GUI
Main->>CDROM: setIso(file)
CDROM-->>Main: parseIso()
Main->>GUI: Update display (show scheduleMask)
sequenceDiagram
participant CPU
participant Peripheral
CPU->>Peripheral: schedule(Schedule::XYZ, cycle)
Peripheral-->>CPU: scheduledCallback() triggered
CPU->>CPU: unschedule(Schedule::XYZ) after handling
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (50)
third_party/cueparser/cueparser.h (1)
91-91: Consider documenting the purpose of the 'cutting' field.Added a new uint32_t field to the CueParser struct, but the purpose isn't immediately clear from the name alone. Consider adding a brief comment to explain this field's intended use for sector number adjustments during cue sheet parsing.
- uint32_t cutting; + uint32_t cutting; /* Stores the sector number used to adjust calculations when processing cue sheet indices */third_party/cueparser/cueparser.c (1)
494-500: Simplified track index handling.The refactored code improves the handling of implicit indices and the first index of a track, making the calculation of file offsets clearer.
Consider breaking down the large
parsefunction into smaller, more manageable functions as noted by the static analyzer. The complexity (cyclomatic complexity of 119, threshold is 9) makes it difficult to maintain and understand.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 488-500: ❌ Getting worse: Complex Method
parse increases in cyclomatic complexity from 117 to 119, 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.
[notice] 488-500: ✅ Getting better: Bumpy Road Ahead
parse decreases from 11 to 10 logical blocks with deeply nested code, threshold is one single block 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.src/core/luaiso.cc (1)
40-42: Consider adding comments for the MSF structureThe MSF (Minute, Second, Frame) structure represents CD time format as defined in the IEC-60908B standard. Adding a brief comment would help developers understand this structure's purpose.
struct MSF { + // Represents CD time in Minutes, Seconds, Frames format per IEC-60908B standard uint8_t m, s, f; };src/mips/tests/cdrom/playing.c (1)
219-225: Good debugging output for test diagnosticsThe detailed logging of responses helps with debugging and understanding test failures. Consider adding a summary message at the end that indicates the test passed successfully.
ramsyscall_printf("Simple Playing until end with reports:\n"); for (unsigned i = 0; i < count; i++) { ramsyscall_printf(" - Response[%i] = %02x %02x %02x %02x %02x %02x %02x %02x\n", i, responses[i * 16 + 0], responses[i * 16 + 1], responses[i * 16 + 2], responses[i * 16 + 3], responses[i * 16 + 4], responses[i * 16 + 5], responses[i * 16 + 6], responses[i * 16 + 7]); } + ramsyscall_printf("Test completed successfully with %u report responses\n", count);src/mips/tests/cdrom/cdlgettn.c (1)
64-100: Good error case test for getTN with argumentsThis test validates the error handling when the getTN command receives too many arguments, checking both response codes and timing constraints.
Consider extracting common setup code (reset and timing initialization) into a helper function to reduce duplication between the two test cases.
src/cdrom/cdriso.cc (2)
425-454: New getLocP method for CD position informationThe method correctly retrieves location information for a given MSF position, handling track boundaries and pregap detection appropriately.
Consider refactoring to reduce nested conditionals as flagged by static analysis. This could be achieved by extracting the track identification logic to a separate helper method (similar to the getTrack method below), improving readability and maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 425-454: ❌ New issue: Bumpy Road Ahead
PCSX::CDRIso::getLocP has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block 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.
456-470: New getTrack method for MSF to track conversionThis method properly identifies which track contains a given MSF position, handling track boundaries correctly.
To reduce the nested conditional complexity flagged by static analysis, consider restructuring the code. For example:
unsigned PCSX::CDRIso::getTrack(const PCSX::IEC60908b::MSF msf) { auto length = getTD(0) + IEC60908b::MSF{0, 2, 0}; if (msf >= length) return 0; unsigned track; for (track = m_numtracks; track != 1; track--) { if (m_ti[track].start <= msf) break; } - IEC60908b::MSF nextPregapStart; - if (track != m_numtracks) { - if (msf >= (m_ti[track + 1].start - m_ti[track + 1].pregap)) { - track++; - } - } + // Check if we're in the pregap of the next track + if (track != m_numtracks && msf >= (m_ti[track + 1].start - m_ti[track + 1].pregap)) { + track++; + } return track; }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 456-470: ❌ New issue: Bumpy Road Ahead
PCSX::CDRIso::getTrack has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block 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.src/mips/tests/cdrom/create-test-iso.lua (2)
62-75: Consider adding error handling for file operations.While the code copies sectors from the input file to the output and fills additional sectors with data, it lacks error handling for file operations that could fail.
Consider adding error checking:
while not uniromFile:eof() do - uniromFile:read(b) + local bytesRead = uniromFile:read(b) + if not bytesRead or bytesRead == 0 then + error("Failed to read from uniromFile") + end iso:writeSector(b) count = count + 1 end
111-146: Consider documenting the purpose of different audio tracks.The code generates multiple audio tracks with similar parameters but doesn't document their specific purposes or how they differ.
Consider adding comments explaining the purpose of each audio track and why they have different lengths or settings.
src/core/r3000a.cc (1)
345-375: Consider replacing macro with a static inline function
While the macro-based logic incheckAndUpdatereduces repeated code, debuggability and maintainability can often improve by using a static inline function instead of a macro. This would also keep type checking and error reporting more consistent.src/mips/tests/cdrom/README.md (7)
1-1: Consider using “CD-ROM”
“CDRom” is less common. For consistency and clarity, consider changing it to “CD-ROM.”-# CDRom tests +# CD-ROM tests🧰 Tools
🪛 LanguageTool
[style] ~1-~1: Consider using “CD-ROM”.
Context: # CDRom tests This directory contains tests fo...(CDROM)
3-3: Use consistent “CD-ROM” formatting
For consistency, consider changing “CDRom controller” to “CD-ROM controller.”-This directory contains tests for the CDRom controller of the PS1. +This directory contains tests for the CD-ROM controller of the PS1.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Consider using “CD-ROM”.
Context: ... This directory contains tests for the CDRom controller of the PS1. They are assumin...(CDROM)
15-15: Simplify wording
Consider removing the phrase "In order to" for brevity and directness.-One important caveat of the test framework is it'll allocate memory for failed tests, and the memory allocator is very rudimentary. This means that if too many tests fail, the system will run out of memory and crash. Check the file `cdrom.c` to disable portions of tests and work incrementally towards a working emulator. In order to further disable some tests, replace the corresponding `CESTER_TEST` keyword with `CESTER_SKIP_TEST`. +One important caveat of the test framework is it'll allocate memory for failed tests, and the memory allocator is very rudimentary. This means that if too many tests fail, the system will run out of memory and crash. Check the file `cdrom.c` to disable portions of tests and work incrementally towards a working emulator. To further disable some tests, replace the corresponding `CESTER_TEST` keyword with `CESTER_SKIP_TEST`.🧰 Tools
🪛 LanguageTool
[style] ~15-~15: Consider a shorter alternative to avoid wordiness.
Context: ...crementally towards a working emulator. In order to further disable some tests, replace the...(IN_ORDER_TO_PREMIUM)
17-17: Use “CD-ROM” consistently
Again, replace “CDRom” with “CD-ROM” for clarity and consistency.-The tests are checking two things: proper results from the CDRom controller, and approximate timings. +The tests are checking two things: proper results from the CD-ROM controller, and approximate timings.🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Consider using “CD-ROM”.
Context: ...ing two things: proper results from the CDRom controller, and approximate timings. Th...(CDROM)
21-21: Remove the comma for clarity
No comma is typically used before “so that” in this context.-The code is commented to explain what is being tested, and why. The tests are also written to be as self-contained as possible, so that they can be easily copied and modified... +The code is commented to explain what is being tested, and why. The tests are also written to be as self-contained as possible so that they can be easily copied and modified...🧰 Tools
🪛 LanguageTool
[typographical] ~21-~21: The conjunction “so that” does not have a comma in front.
Context: ...tten to be as self-contained as possible, so that they can be easily copied and modified ...(SO_THAT_UNNECESSARY_COMMA)
23-23: Avoid uncertainty
Removing “somewhat” will strengthen clarity.-...as long as the main CPU timings are somewhat accurate... +...as long as the main CPU timings are accurate...🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...ng, as long as the main CPU timings are somewhat accurate. If any of these tests fail, i...(SOMEWHAT)
25-25: Add missing article
Use “a busy wait loop” or “the busy wait loop” to be grammatically correct.-However, many games will have busy wait loop when talking to the controller... +However, many games will have a busy wait loop when talking to the controller...🧰 Tools
🪛 LanguageTool
[uncategorized] ~25-~25: Possible missing article found.
Context: ...accuracy. However, many games will have busy wait loop when talking to the controlle...(AI_HYDRA_LEO_MISSING_A)
src/mips/tests/cdrom/cdlnop.c (2)
29-58: Well-structured test case with appropriate assertionsThe
cdlNoptest properly verifies CD-ROM command behavior with clear assertions and timing validation. The test initializes, executes the command, and verifies all expected register values and response data.One suggestion to improve maintainability:
- Consider adding comments for the magic values (0xe0, 0x38, 0x18) to explain what these register bits represent
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
60-95: Complete error case testing with proper validationThe
cdlNopTooManyArgstest effectively validates error handling when too many arguments are provided. The test structure is good and the assertions correctly verify the error response.The timing validation range (500-7000μs) is quite broad. While this accommodates normal variations, it might miss subtle performance regressions. Consider narrowing the range if consistent timing is important for your emulation.
src/mips/tests/cdrom/cdlsetmode.c (1)
29-61: Well-implemented command test with proper validationThe
cdlSetModetest properly validates the basic functionality of the setMode command. The test structure follows the established pattern of setup, execution, and verification with appropriate assertions.As with other tests, consider adding comments for magic values like 0x80 on line 39 to explain what mode is being set.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
src/core/sstate.cc (1)
251-273: Consider adding a TODO comment for the commented-out code.The CD-ROM loading code has been commented out, but there's no explanation why. If this is temporary or part of a larger refactoring, adding a TODO comment would help explain the intent.
#if 0 +// TODO: CD-ROM loading temporarily disabled during CD-ROM subsystem refactoring g_emulator->m_cdrom->load(); #endifsrc/mips/tests/cdrom/cdlinit.c (1)
97-98: Consider adding a timeout handler.While the wait loop works correctly, it would be better to include a timeout message for diagnostic purposes in case the test fails.
do { delayedTime = updateTime(); + if (delayedTime > 500000) { + ramsyscall_printf("Warning: Timeout waiting for IRQ\n"); + } } while (((IREG & IRQ_CDROM) == 0) && (delayedTime <= 500000));🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 98-98: syntax error
(syntaxError)
src/mips/tests/cdrom/cester-hw.c (2)
35-56: Consider refactoringhexdumpto reduce nesting and complexity.
The static analysis indicates a high cyclomatic complexity (11) and multiple nested blocks in this function.You could split out the ASCII assembly logic from the printing logic or otherwise restructure the conditional checks to make it more maintainable and reduce nesting.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 35-56: ❌ New issue: Complex Method
hexdump has a cyclomatic complexity of 11, 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] 35-56: ❌ New issue: Bumpy Road Ahead
hexdump has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block 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] 35-56: ❌ New issue: Deep, Nested Complexity
hexdump has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
214-238: Reduce duplication betweenseekPTo()andseekLTo().
Both functions share a very similar flow: setting up location, acknowledging the interrupt, and checking the cause.Consider unifying them into a single helper, for example passing the command (
CDL_SEEKPvs.CDL_SEEKL) as a parameter:-static int seekPTo(uint8_t minute, uint8_t second, uint8_t frame) { - ... - CDROM_REG1 = CDL_SEEKP; - ... -} - -static int seekLTo(uint8_t minute, uint8_t second, uint8_t frame) { - ... - CDROM_REG1 = CDL_SEEKL; - ... -} +static int seekTo(uint8_t minute, uint8_t second, uint8_t frame, uint8_t command) { + uint8_t cause; + // setLoc + ... + CDROM_REG1 = command; + ... +}Also applies to: 240-265
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 214-238: ❌ New issue: Code Duplication
The module contains 2 functions with similar structure: seekLTo,seekPTo. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.src/mips/tests/cdrom/cdlreads.c (5)
29-136: Consider adding a timeout forwaitCDRomIRQ
In testcdlReadS1x, if the CD-ROM interrupt never arrives, the loop could block indefinitely. A timeout can help prevent test hangs in hardware failure or emulator misconfiguration cases.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
255-372: Validate speed-setting success before reading
In testcdlReadS2x, we rely on successful mode setting for 2x read speed. Adding an explicit check aftersetMode(0x80)ensures we detect failures earlier, improving test reliability.
501-618: Warn about indefinite waiting
In testcdlReadS2xRunaway, there's another loop waiting on CD-ROM status before reading. Introduce a fail-safe condition or timeout in case the device never signals readiness.
673-725: Extend error checks on out-of-range sector
In testcdlReadSTooFar, we verify an error is raised. It might be beneficial to confirm that subsequent commands (e.g., re-seek or pause) behave correctly after this out-of-range error.
726-843: Maintain consistent naming for test outputs
In testcdlReadS2xWithNop, the print statement references “full sector readS @2x with DMA” in other tests but not here. Using consistent wording helps quickly compare logs across tests.src/mips/tests/cdrom/cdlsetloc.c (6)
29-62: Add additional assertions on valid sector range
In testcdlSetLoc, you successfully set the sector to 00:02:00. Consider validating that the location is within a known disc range and is truly accessible.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
64-93: Clarify expected error for missing arguments
In testcdlSetLocNoArgs, an error is expected. Adding a single comment clarifying which error code is expected can improve maintainability if new error codes appear.
158-190: Additional validations for invalid param
IncdlSetLocInvalid1, the test verifies a0x2aminute segment fails. Consider also confirming that further read commands remain blocked or produce the correct error after setting an invalid location.
192-224: Ensure coverage of all erroneous track boundaries
IncdlSetLocInvalid2, the location00:02:79is invalid. Expanding coverage to various invalid frames or second boundaries can catch additional edge cases.
226-259: Differentiate between too many vs. invalid arguments
IncdlSetLocTooManyArgs, the code merges the logic for invalid argument counts. If more distinct validations are introduced in the firmware, a more granular check might be beneficial.
261-295: Combine repeated “too many + invalid” scenarios
IncdlSetLocTooManyArgsAndInvalid, you test a scenario with combined error triggers. If these are expected to produce the same code path, referencing that in the test name or logs clarifies the test’s focus.src/mips/tests/cdrom/reading.c (7)
29-111: Use a fail-safe for indefinite loops
InsimpleReading, theforloop reading 100 sectors might stall if the hardware never signals cause=1. Consider adding a maximum iteration check or a configurable timeout.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
300-396: Recommended negative path testing
simpleReadingWithoutAckNorResponseReadThenInittests an eventual init after ignoring responses. A negative test for partial sector reads or multiple ignoring cycles might provide additional coverage and catch corner cases.
397-495: Validate correct re-initialization
InsimpleReadingWithoutAckThenInit, the code checks successful re-init. Consider verifying that the CD-ROM state truly resets any leftover partial reads or queue data to avoid lingering state issues.
496-573: Count partial read interrupts
InsimpleReadingPauseWithoutAck, you track how manycause == 1interrupts occur before a final break. Logging or verifying the number of partial read interrupts ensures the drive transitions as expected.
574-672: Check for concurrency overlap
simpleReadingNopQueryuses NOP commands mid-read to probe status. If these tests were ever run in parallel with other read processes, concurrency checks or locks might be needed to ensure stable results.
673-792: Guard against indefinite looping
InsimpleReadingNopSeriesQuery, the teardown loop runs untilgotInt1is set. If hardware or emulation is unresponsive, it may hang tests. A short time-based fallback is advisable.
793-870: Exercise partial read or error scenario
InsimpleReadingNoSeekNopQueries, the test checks reading without a prior seek. Consider verifying how the code behaves if the drive partially reads an invalid sector or signals an error mid-sequence.src/supportpsx/iec-60908b.h (1)
164-173: Investigate efficiency of operator+= and operator-=.
These rely on creating and assigning a new MSF viaMSF(lba). If these operators are frequently called, consider an in-place arithmetic approach that avoids repeated conversions for performance-critical paths (if any).src/core/r3000a.h (2)
166-178: Enhance documentation for newly introduced Schedule enum.
The new enum is clear, but consider clarifying usage in a comment so future maintainers understand each enum constant’s role in the scheduling system.
189-189: Rename scheduleMask for clarity.
scheduleMaskis more accurate than a generic interrupt mask, but a more semantically explicit name, likeactiveSchedules, could aid readability.src/core/cdrom.h (5)
65-65: Check parseIso error handling or logging.
Unclear how parseIso deals with invalid ISO data. If it can fail, ensure we have meaningful logs or user feedback.
82-85: Highly cohesive pure virtual callbacks.
The approach of separate scheduled callbacks is clear but check if these calls can unify under one method with an enum. Lifting code duplication might be beneficial.
103-103: dataFIFOHasData naming.
Method name is fine but might read better ashasDataFIFO(). Not critical—just style preference.
105-109: Lid state booleans synergy.
m_lidOpen,m_wasLidOpened,m_lidCloseScheduledcan be combined into a small state machine or single enum. This might reduce confusion about transitions among multiple booleans.
146-153: Expand doc for newly introduced Cause enum.
Consider clarifying the meaning of each cause. For example, how does “Error” differ from “End”? Proper documentation helps prevent confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
.github/workflows/linux-build.yml(2 hunks).github/workflows/linux-coverage.yml(1 hunks).gitignore(1 hunks).vscode/c_cpp_properties.json(1 hunks)src/cdrom/cdriso.cc(1 hunks)src/cdrom/cdriso.h(2 hunks)src/core/DynaRec_aa64/recompiler.cc(3 hunks)src/core/DynaRec_x64/recompiler.cc(4 hunks)src/core/cdrom.h(3 hunks)src/core/gdb-server.cc(1 hunks)src/core/gte.cc(1 hunks)src/core/isoffi.lua(3 hunks)src/core/luaiso.cc(4 hunks)src/core/mdec.cc(3 hunks)src/core/mdec.h(1 hunks)src/core/psxdma.h(1 hunks)src/core/psxemulator.h(3 hunks)src/core/psxinterpreter.cc(32 hunks)src/core/r3000a.cc(1 hunks)src/core/r3000a.h(5 hunks)src/core/sio.cc(2 hunks)src/core/sio.h(2 hunks)src/core/sio1.cc(2 hunks)src/core/sio1.h(3 hunks)src/core/sstate.cc(4 hunks)src/core/sstate.h(5 hunks)src/gui/gui.cc(5 hunks)src/gui/widgets/isobrowser.cc(2 hunks)src/gui/widgets/log.cc(1 hunks)src/gui/widgets/registers.cc(1 hunks)src/lua/fileffimeta.lua(1 hunks)src/main/main.cc(1 hunks)src/mips/common.mk(1 hunks)src/mips/tests/Makefile(2 hunks)src/mips/tests/basic/Makefile(1 hunks)src/mips/tests/cdrom/Makefile(1 hunks)src/mips/tests/cdrom/README.md(1 hunks)src/mips/tests/cdrom/cdlgetlocl.c(1 hunks)src/mips/tests/cdrom/cdlgetlocp.c(1 hunks)src/mips/tests/cdrom/cdlgettd.c(1 hunks)src/mips/tests/cdrom/cdlgettn.c(1 hunks)src/mips/tests/cdrom/cdlid.c(1 hunks)src/mips/tests/cdrom/cdlinit.c(1 hunks)src/mips/tests/cdrom/cdlnop.c(1 hunks)src/mips/tests/cdrom/cdlreadn.c(1 hunks)src/mips/tests/cdrom/cdlreads.c(1 hunks)src/mips/tests/cdrom/cdlseekl.c(1 hunks)src/mips/tests/cdrom/cdlseekp.c(1 hunks)src/mips/tests/cdrom/cdlsetloc.c(1 hunks)src/mips/tests/cdrom/cdlsetmode.c(1 hunks)src/mips/tests/cdrom/cdltest.c(1 hunks)src/mips/tests/cdrom/cdrom.c(1 hunks)src/mips/tests/cdrom/cester-hw.c(1 hunks)src/mips/tests/cdrom/create-test-iso.lua(1 hunks)src/mips/tests/cdrom/invalid.c(1 hunks)src/mips/tests/cdrom/misc.c(1 hunks)src/mips/tests/cdrom/playing.c(1 hunks)src/mips/tests/cdrom/race.c(1 hunks)src/mips/tests/cdrom/reading.c(1 hunks)src/mips/tests/common.mk(1 hunks)src/mips/tests/cop0/Makefile(1 hunks)src/mips/tests/cpu/Makefile(1 hunks)src/mips/tests/libc/Makefile(1 hunks)src/mips/tests/pcdrv/Makefile(1 hunks)src/supportpsx/iec-60908b.h(2 hunks)src/supportpsx/iso9660-builder.cc(1 hunks)tests/pcsxrunner/cdrom.cc(1 hunks)third_party/cueparser/cueparser.c(2 hunks)third_party/cueparser/cueparser.h(1 hunks)vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj(1 hunks)vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj.filters(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/gui/widgets/log.cc (1)
src/gui/gui.h (1) (1)
MenuItem(341-341)
src/core/psxinterpreter.cc (3)
src/core/r3000a.h (4) (4)
delayedLoadRef(343-350)delayedLoadRef(343-343)addr(269-269)addr(270-270)src/core/psxmem.h (3) (3)
addr(92-92)addr(136-136)addr(136-136)src/core/disr3000a.cc (5) (5)
addr(84-92)addr(84-84)addr(93-93)addr(93-93)addr(94-94)
src/core/r3000a.h (3)
src/core/sio.h (3) (3)
SIO(80-80)eCycle(236-243)eCycle(236-236)src/core/cdrom.cc (8) (8)
delay(256-256)delay(256-256)delay(259-259)delay(259-259)delay(262-262)delay(262-262)delay(265-265)delay(265-265)src/core/psxinterpreter.cc (1) (1)
target(119-119)
🪛 Cppcheck (2.10-2)
src/mips/tests/cdrom/invalid.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/playing.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/misc.c
[error] 33-33: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlgettn.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlsetmode.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlgetlocl.c
[error] 31-31: syntax error
(syntaxError)
src/core/gte.cc
[error] 449-449: Signed integer overflow for expression '1<<31'.
(integerOverflow)
[error] 451-451: Signed integer overflow for expression '1<<31'.
(integerOverflow)
src/mips/tests/cdrom/cester-hw.c
[error] 293-293: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlnop.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdltest.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlinit.c
[error] 98-98: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlid.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlseekl.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlgetlocp.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/reading.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlsetloc.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlreadn.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlreads.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlseekp.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlgettd.c
[error] 31-31: syntax error
(syntaxError)
src/mips/tests/cdrom/race.c
[error] 31-31: syntax error
(syntaxError)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
third_party/cueparser/cueparser.c
[warning] 488-500: ❌ Getting worse: Complex Method
parse increases in cyclomatic complexity from 117 to 119, 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.
[notice] 488-500: ✅ Getting better: Bumpy Road Ahead
parse decreases from 11 to 10 logical blocks with deeply nested code, threshold is one single block 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.
src/core/r3000a.cc
[notice] 331-374: ✅ Getting better: Complex Method
PCSX::R3000Acpu::branchTest decreases in cyclomatic complexity from 14 to 11, 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.
[notice] 331-374: ✅ Getting better: Bumpy Road Ahead
PCSX::R3000Acpu::branchTest decreases from 3 to 2 logical blocks with deeply nested code, threshold is one single block 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.
src/cdrom/cdriso.cc
[warning] 425-454: ❌ New issue: Bumpy Road Ahead
PCSX::CDRIso::getLocP has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block 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] 456-470: ❌ New issue: Bumpy Road Ahead
PCSX::CDRIso::getTrack has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block 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.
src/core/sstate.cc
[notice] 34-117: ✅ Getting better: Large Method
PCSX::SaveStates::constructSaveState decreases from 138 to 86 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
src/mips/tests/cdrom/cester-hw.c
[warning] 214-238: ❌ New issue: Code Duplication
The module contains 2 functions with similar structure: seekLTo,seekPTo. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
[warning] 35-56: ❌ New issue: Complex Method
hexdump has a cyclomatic complexity of 11, 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] 35-56: ❌ New issue: Bumpy Road Ahead
hexdump has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block 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] 35-56: ❌ New issue: Deep, Nested Complexity
hexdump has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🪛 LanguageTool
src/mips/tests/cdrom/README.md
[style] ~1-~1: Consider using “CD-ROM”.
Context: # CDRom tests This directory contains tests fo...
(CDROM)
[style] ~3-~3: Consider using “CD-ROM”.
Context: ... This directory contains tests for the CDRom controller of the PS1. They are assumin...
(CDROM)
[style] ~15-~15: Consider a shorter alternative to avoid wordiness.
Context: ...crementally towards a working emulator. In order to further disable some tests, replace the...
(IN_ORDER_TO_PREMIUM)
[style] ~17-~17: Consider using “CD-ROM”.
Context: ...ing two things: proper results from the CDRom controller, and approximate timings. Th...
(CDROM)
[typographical] ~21-~21: The conjunction “so that” does not have a comma in front.
Context: ...tten to be as self-contained as possible, so that they can be easily copied and modified ...
(SO_THAT_UNNECESSARY_COMMA)
[style] ~23-~23: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...ng, as long as the main CPU timings are somewhat accurate. If any of these tests fail, i...
(SOMEWHAT)
[uncategorized] ~25-~25: Possible missing article found.
Context: ...accuracy. However, many games will have busy wait loop when talking to the controlle...
(AI_HYDRA_LEO_MISSING_A)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: cross-arm64
- GitHub Check: coverage
- GitHub Check: build
🔇 Additional comments (224)
.gitignore (3)
113-114: Updated comment to match configuration file purpose.The comment was updated from "# NOPS leftovers" to "# nops' configuration file", providing clearer context about the purpose of comport.txt.
116-117: Added cue sheet files to gitignore.Appropriate addition for a CD-ROM implementation. Cue sheets (.cue) are metadata files that describe CD tracks and typically shouldn't be committed to the repository.
119-120: Added editor backup files to gitignore.Good practice to exclude editor backup files (*.bak) to prevent accidental commits of temporary files.
src/mips/tests/Makefile (2)
3-3: Added build target for CD-ROM tests.Properly integrated the new CD-ROM test directory into the main build process, following the established pattern.
14-14: Added clean target for CD-ROM tests.Ensures that CD-ROM test artifacts are properly cleaned up when running the clean target.
src/mips/tests/cdrom/Makefile (1)
1-8: Well-structured Makefile for CD-ROM tests.The new Makefile follows the project's established pattern for test directories:
- Sets the target name
- Includes common build configurations
- Adds specific source files
- Includes the parent common makefile
The consistent structure maintains the codebase's build system integrity.
vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj.filters (1)
33-35: Appropriate addition of the new cdrom.cc test file to the filter.The inclusion of the cdrom.cc test file in the Source Files filter aligns with the existing organization pattern of other test files in the project.
vsprojects/tests/pcsxrunner/pcsxrunner.vcxproj (1)
254-254: Appropriate addition of cdrom.cc to the project.The inclusion of the cdrom.cc test file in the project is consistent with the changes made to the .filters file, ensuring the new CD-ROM test file will be properly compiled as part of the test suite.
src/supportpsx/iso9660-builder.cc (1)
79-79: Important enhancement to data integrity.Adding the
compute_edcecc(ptr)call here properly computes error detection and correction codes for the M2_RAW sector mode, ensuring that the generated ISO data conforms to standard specifications and has proper error correction capabilities.This change aligns with CD-ROM specifications where sector data requires error detection and correction codes to ensure data integrity during reading operations.
src/main/main.cc (1)
337-337: Method name change from check() to parseIso()Replacing
check()withparseIso()reflects a shift from basic validation to comprehensive parsing of ISO files. This change is part of a broader refactoring across the codebase to enhance CD-ROM functionality.This modification is consistent with similar changes in other files mentioned in the PR, indicating a deliberate architectural decision to improve ISO file handling throughout the emulator.
src/gui/widgets/isobrowser.cc (2)
70-70: Method replacement: check() -> parseIso()This change replaces the call to
check()withparseIso(), which seems to be part of a broader refactoring across the codebase for ISO handling. The new method name better describes its purpose.
90-90: Method replacement: check() -> parseIso()Similar to the previous change, this replaces
check()withparseIso()after opening a new disk image. This maintains consistency in how ISO files are processed throughout the application..vscode/c_cpp_properties.json (1)
13-13: Added Makefile Tools configuration providerAdding the Makefile Tools configuration provider improves the development experience in VS Code by helping IntelliSense understand the project structure defined in Makefiles, enabling better code navigation and completion.
src/core/gdb-server.cc (1)
758-758: Method replacement: check() -> parseIso()Consistent with other files in this PR,
check()has been replaced withparseIso(). This change maintains consistency across the codebase in how ISO files are processed after mounting a CD image via the GDB monitor command.src/lua/fileffimeta.lua (1)
74-77: Added buffer casting functionalityThe addition of a
castfunction to thebufferMetatable enhances the Lua FFI interface by providing the ability to cast buffer data to specific C types. This is useful for working with complex data structures and binary formats like MSF timestamps used in CD-ROM operations.tests/pcsxrunner/cdrom.cc (3)
23-28: Well-structured test for CD-ROM interpreter mode.The test is properly configured to test CD-ROM functionality using the interpreter mode. It loads the necessary components (BIOS, ISO, executable) and verifies successful execution with the EXPECT_EQ assertion.
30-35: Well-structured test for CD-ROM dynarec mode.This test mirrors the interpreter test but runs with the dynamic recompilation mode enabled. This ensures CD-ROM functionality works correctly across different execution methods, which is good testing practice.
1-18: LGTM: Appropriate license header.The file includes the proper GPL license header as used throughout the project.
src/mips/tests/libc/Makefile (2)
3-3: Good simplification by including common build definitions.Including the common.mk file centralizes build configurations and reduces duplication across makefiles, which is a good practice for maintainability.
5-7: LGTM: Streamlined source file list.The makefile has been simplified to include only the essential source file, which should improve build clarity and reduce maintenance overhead.
src/gui/widgets/registers.cc (1)
348-348: Change from interrupt to scheduleMask display reflects architectural updates.This change aligns the GUI with the broader refactoring effort in the codebase, where interrupt handling is transitioning to a scheduling mechanism. The display now shows the schedule mask instead of the interrupt register, providing more relevant information to the user.
src/mips/tests/cdrom/cdrom.c (4)
27-32: Good inclusion of necessary hardware and system headers.The file includes all required headers for CD-ROM testing, covering hardware access to CD-ROM, DMA, hardware registers, interrupts, and system calls.
34-39: Proper test framework configuration.The code correctly configures the Cester testing framework by disabling signals and timing, which is appropriate for hardware testing where timing can be unpredictable.
43-63: Comprehensive inclusion of CD-ROM test cases.The conditional inclusion (#if 1) includes a complete set of test files covering all major CD-ROM operations, providing thorough coverage of the CD-ROM functionality. The alternative path (#else) with only misc.c suggests a possible debugging configuration.
1-25: LGTM: Proper MIT license header.The file includes the correct MIT license header as used for test files in this project.
src/mips/common.mk (2)
50-51: Enhancement: New optimization flags for faster builds.These new flags enable O3 optimization level for ReleaseFast builds, providing maximum performance optimizations compared to the more conservative -Os optimization used in regular Release builds.
56-57: Enhancement: New fast LTO build configuration.Adding LTO (Link Time Optimization) with -O3 provides maximum performance optimization by allowing the compiler to perform optimizations across translation units at link time. This complements the existing LTO configuration that uses -Os (optimize for size).
src/core/mdec.cc (5)
458-459: Method renamed for consistency with scheduler architecture.Changed from direct call to
mdec0Interrupt()toscheduledCallback0()to align with the broader refactoring of the codebase toward a scheduling paradigm.
461-465: Method renamed: Refactored interrupt handling to callback pattern.The functionality remains identical - clearing DMA busy state and triggering DMA interrupt. The renaming better reflects the scheduled callback architecture being adopted throughout the codebase.
560-599: Method renamed: Refactored interrupt handling for consistency.Changed
mdec1Interrupt()toscheduledCallback1()while preserving the same functionality. This is part of the codebase-wide transition to scheduled callbacks instead of direct interrupt handling.
587-588: Internal method call updated for consistency.Updated to call
scheduledCallback0()instead of the oldmdec0Interrupt()for consistent method naming.
592-593: Internal method call updated for consistency.Updated to call
scheduledCallback0()instead of the oldmdec0Interrupt()for consistent method naming.src/core/mdec.h (1)
40-41: Method renamed for consistency.Renamed
mdec0Interrupt()andmdec1Interrupt()toscheduledCallback0()andscheduledCallback1()respectively to better reflect their role in the scheduling architecture. This is part of a broader refactoring to standardize interrupt handling across the codebase.src/gui/widgets/log.cc (1)
111-112: New logging feature: CD-ROM hardware access logging.Added a menu item to toggle logging of CD-ROM hardware access, which complements the existing CD-ROM commands logging. This will be useful for debugging CD-ROM related issues at a lower hardware level.
.github/workflows/linux-build.yml (3)
24-27: UNIROM boot disc integration looks goodThe addition of steps to download and extract the UNIROM boot disc enhances the build process by ensuring a consistent testing environment.
66-70: OpenBIOS rebuilding sequence is well-structuredThe added steps correctly clean, rebuild, and position the OpenBIOS binary for subsequent testing. This ensures that the tests run against the latest version of the bootloader.
71-71: Comprehensive CD-ROM test executionThe test execution command properly utilizes the downloaded UNIROM boot disc and executes the CD-ROM test script within the emulator, with appropriate flags for test mode and output redirection.
src/core/DynaRec_x64/recompiler.cc (3)
337-337: New ROM instruction tracking variable added correctlyThe addition of the
extravariable to track ROM instructions is well-placed within the function scope.
399-409: ROM instruction counter implementation looks goodThe modified lambda now correctly captures and increments the
extracounter when instructions are in ROM address space.
458-459:✅ Verification successful
Cycle counting adjustment for ROM instructions
The change to include the ROM-specific instruction count with a bias factor in the cycle calculation provides more accurate timing for ROM code execution.
Let's verify the meaning and value of the ROM_EXTRA_BIAS constant:
🏁 Script executed:
#!/bin/bash # Look for the ROM_EXTRA_BIAS definition to understand its purpose and value rg -A 2 "ROM_EXTRA_BIAS" src/core/psxemulator.hLength of output: 137
Verification Complete: Cycle Counting Adjustment Confirmed
The cycle counting adjustment now clearly uses the
ROM_EXTRA_BIASconstant, which our search confirms is defined as10insrc/core/psxemulator.h. This value is applied to the extra cycle count, ensuring more precise timing for ROM code execution. No further changes are needed.src/mips/tests/common.mk (3)
1-33: Well-structured Makefile for MIPS testingThe Makefile correctly defines build configuration and source files for the MIPS tests. The hierarchical structure with relative paths to the required components (uC-sdk, system libraries, and common code) is well organized.
34-40: Comprehensive include paths for test compilationThe CPPFLAGS setup includes all necessary paths for headers from the main project and third-party dependencies, which ensures proper compilation of the test code.
41-43: Clean conditional compilation for PCSX testsThe conditional addition of the PCSX_TESTS definition when the flag is set provides a flexible way to enable specific test code only when needed.
src/mips/tests/cdrom/invalid.c (4)
1-28: License and formatting setup is correctThe MIT license header and clang-format directive follow project conventions and are properly included.
29-35: Proper test initialization with error handlingThe test correctly begins with CD-ROM reset and includes appropriate error handling for failed resets, ensuring the test doesn't proceed when prerequisites aren't met.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
36-49: CD-ROM command sequence is well-structuredThe sequence of operations (initialize time, set registers, wait for interrupt, acknowledge cause, and read response) follows a logical flow for testing CD-ROM behavior.
50-61:✅ Verification successful
Comprehensive assertions for error validation
The test includes thorough assertions checking multiple aspects of the error response, including cause codes, response data, control register states, and timing constraints.
The test includes timing assertions with specific ranges (500-7000μs). Let's verify if these ranges are consistent with other tests:
🏁 Script executed:
#!/bin/bash # Look for other timing assertions in CD-ROM tests rg -B 1 -A 1 "cester_assert_uint_(ge|lt)" src/mips/tests/cdrom/*.cLength of output: 44062
Consistent Timing Ranges Confirmed
After reviewing similar timing assertions across the CD-ROM test suite (e.g., in race.c, cdltest.c, cdlsetmode.c, etc.), the 500–7000 μs range for errorTime in
src/mips/tests/cdrom/invalid.cis consistent with the values used elsewhere. The assertions in this file match the expected boundaries found throughout the tests, so no adjustments are needed. Please continue monitoring these values to ensure they remain representative of the hardware’s behavior under all conditions.src/core/isoffi.lua (3)
31-31: Addition of MSF structure looks appropriate.This structure provides a standardized way to represent the Minutes-Seconds-Frames format commonly used in CD-ROM time addressing.
46-47: Well-defined function declarations for CD-ROM track information.These new functions enhance the ISO functionality by providing access to track number and track time data, which are essential for proper CD-ROM handling.
84-89: Clean implementation of the track information wrapper methods.The added methods properly expose the new C functions to Lua, keeping the API consistent with the rest of the codebase. The implementation is straightforward and follows the established pattern.
.github/workflows/linux-coverage.yml (2)
35-38: Properly set up UNIROM bootdisc download for testing.The added step correctly downloads and extracts the bootdisc needed for the CD-ROM tests.
48-48: Test execution with bootdisc looks good.This command properly runs tests with the downloaded bootdisc, using appropriate flags and the Lua script for creating a test ISO.
src/core/gte.cc (1)
449-449: Improved clarity for integer literal representation.Replaced the
S64macro with explicit long long (LL) suffixes, which makes the code more maintainable and clearer regarding the intended data types.Note that the static analyzer flags potential signed integer overflow for the bit-shift expressions
1<<31, but this is unrelated to your changes and would need a separate fix if it's an actual issue.Also applies to: 451-451
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 449-449: Signed integer overflow for expression '1<<31'.
(integerOverflow)
third_party/cueparser/cueparser.c (2)
138-138: Added initialization for the new cutting variable.The cutting variable is properly initialized to zero in the constructor.
488-492: Improved index calculation logic for CD tracks.The modified logic now properly handles sector number adjustments by introducing the cutting variable to account for the sector offset at the start of a track.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 488-500: ❌ Getting worse: Complex Method
parse increases in cyclomatic complexity from 117 to 119, 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.
[notice] 488-500: ✅ Getting better: Bumpy Road Ahead
parse decreases from 11 to 10 logical blocks with deeply nested code, threshold is one single block 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.src/mips/tests/cpu/Makefile (1)
3-3: Build system refactoring looks goodThe inclusion of
../common.mkhelps centralize common build configurations, which will improve maintainability by reducing duplication across makefiles.src/mips/tests/basic/Makefile (1)
3-3: Build system simplification is a good approachSimilar to the CPU makefile, this refactoring centralizes build configurations in a common file, which reduces duplication and makes maintenance easier.
src/core/luaiso.cc (3)
30-30: Good addition of IEC-60908B CD standard headerThis header inclusion supports the new CD-ROM time representation features being added.
64-68: Good implementation of ISO track data access functionsThese functions provide a clean interface for retrieving track information from ISO images. The conversion to the MSF structure is implemented correctly.
105-106: Good registration of new functions with Lua interfaceThe new functions are properly registered, making them accessible from Lua scripts.
src/mips/tests/cdrom/playing.c (3)
29-80: Good test for basic CD-ROM playback functionalityThis test verifies playback from a specific position until the end, checking appropriate interrupts, control register values, and timing. The time assertions have appropriate ranges to account for timing variations.
Note: The static analyzer reported a syntax error here, but this appears to be a false positive due to the Cester test framework's macro syntax.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
81-131: Good double-speed playback testThis test properly verifies that setting mode 0x80 causes playback to complete in approximately half the time (820-980ms vs 1640-1960ms in the normal speed test). The assertions properly validate all relevant aspects of the playback process.
133-218: Comprehensive playback test with detailed subcode data validationThis test effectively validates the report mode (mode 0x04) which provides detailed subcode data during playback. The test correctly:
- Collects responses during playback
- Validates the expected number of responses
- Verifies detailed content of each response including track, index, and timing information
- Handles potential timing variations by using appropriate range checks
The validation logic is thorough and accounts for the specific timing patterns expected in the CD-ROM report data.
src/core/sio.cc (2)
310-310: Refactoring interrupt handling to use scheduling approachThis change is part of a larger refactor that moves away from direct interrupt register manipulation to a more structured scheduling system. The new approach provides better encapsulation of interrupt management.
351-351: Method rename clarifies callback purposeRenaming
interrupt()toscheduledCallback()better reflects the function's purpose as a callback for scheduled events rather than a direct interrupt handler.src/mips/tests/cdrom/cdlgettn.c (1)
29-62: Well-structured CD-ROM getTN command testThe test thoroughly verifies the CD-ROM's getTN command behavior after reset, including register values, response data, and timing constraints.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
src/core/sio1.h (3)
73-73: Method rename for consistency with scheduling patternRenaming
interrupt()toscheduledCallback()aligns with the refactoring across the codebase to transition from direct interrupt handling to a more structured callback approach.
96-96: Improved interrupt handling in reset methodUsing
unschedule(Schedule::SIO1)instead of directly manipulating interrupt registers provides better encapsulation and follows the new scheduling pattern.
250-250: Updated interrupt scheduling methodThe implementation now uses the CPU's scheduling system with an enum for better type safety and consistency.
src/mips/tests/cdrom/misc.c (4)
1-25: License header looks good.The MIT license header is properly formatted and includes all necessary information.
27-30: Clear documentation about test limitations.The comment clearly indicates that these tests cannot be easily automated, which is helpful for understanding why they're marked as SKIP_TEST.
31-62: Well-structured test case for door waiting functionality.The test follows a good pattern with proper error handling:
- It resets the CD-ROM and checks for success before proceeding
- It verifies seeking works correctly
- It waits for and validates the interrupt and register values
The assertions thoroughly check expected values across multiple registers and response data.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 33-33: syntax error
(syntaxError)
64-90: Comprehensive test for door opened/missing disc scenario.This test correctly:
- Validates the reset operation works before proceeding
- Sets up the appropriate registers for testing
- Properly captures and validates interrupt timing, causes, control flags, and response data
The assertions are clear and specific, making the expected behavior easy to understand.
src/core/sio.h (2)
96-96: Good method renaming for clarity.Renaming
interrupt()toscheduledCallback()improves code clarity by better describing the function's purpose in the scheduling system.
236-238: Refactored interrupt scheduling to use unified Schedule enum.The change from a specific interrupt scheduling mechanism to using
schedule(Schedule::SIO, eCycle)improves consistency with the broader scheduling system refactoring throughout the codebase.This change is part of a systematic refactoring of interrupt handling across multiple components, including SIO1.
src/mips/tests/cdrom/create-test-iso.lua (6)
1-17: License header is properly formatted.The license header contains all required information for GPL compliance.
18-27: Good initialization and file preparation.The script properly initializes libraries and prepares the necessary files for ISO creation. The comment on line 18 clearly states the purpose of the script.
29-61: Well-structured ISO sector creation.The code properly sets up the primary volume descriptor (PVD), primary track (PT), and root directory structure for the ISO file. The buffer management is handled appropriately.
77-109: Audio generation is well-implemented.The tone sample generation and XA encoding functions are well-structured. The code handles audio data creation correctly with appropriate buffer management.
148-171: Well-formatted cue sheet creation.The cue sheet is properly formatted with all necessary track information and index points.
173-182: Good loop for creating additional tracks.The loop efficiently creates multiple additional audio tracks and updates the cue file accordingly.
src/core/sio1.cc (2)
129-143: Method renamed for consistency with broader refactoring.Renaming
interrupt()toscheduledCallback()improves clarity and aligns with the scheduling system refactoring seen in other components like SIO.
284-284: Improved interrupt handling using the Schedule enum.Replacing direct interrupt register manipulation with
unschedule(Schedule::SIO1)enhances code consistency and maintainability.This change is part of a systematic transition from direct interrupt flags to a unified scheduling system across the codebase.
src/mips/tests/cop0/Makefile (1)
3-3: Include statement looks good
No concerns with addinginclude ../common.mk. This simplifies the build logic and centralizes common definitions.src/core/r3000a.cc (1)
338-344: New scheduling mask usage
IntroducingscheduleMaskand direct comparisons tom_regs.lowestTargetis a clear and organized way to handle scheduling conditions. This approach improves readability over the previous interrupt-based logic.src/mips/tests/cdrom/README.md (5)
5-5: No issues with instructions
Clear guidance on running the script. Looks good.
7-9: Script snippet is clear
The example usage instructions are straightforward and helpful.
11-11: No issues
This explanation of the output files is informative and well-structured.
13-13: Build instructions look good
Thanks for clarifying the toolchain requirements.
19-19: No concerns
Clear note on the test environment and hardware coverage.src/mips/tests/cdrom/cdltest.c (5)
1-25: License is properly appliedThe file correctly includes the MIT license header, which matches the project's licensing requirements.
29-60: Well-structured CD-ROM test for basic command with no argumentsThis test case effectively validates the CD-ROM's behavior when the Test command is issued without arguments. The test follows a clear pattern of:
- Resetting the CD-ROM
- Initializing timing
- Setting registers
- Waiting for and validating the IRQ
- Verifying error responses and timing constraints
The error time assertions (between 500μs and 7000μs) provide reasonable bounds for the expected timing behavior.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
62-92: Well-structured CD-ROM test with 0x20 argumentThis test properly validates the CD-ROM's behavior when the Test command is issued with the 0x20 argument. The expected response size and control register values are properly validated.
94-127: Test case validates error handling with multiple argumentsThis test effectively checks the CD-ROM's behavior when trying to send multiple arguments (0x20 followed by 0x00) to the Test command, correctly verifying that it results in an error response.
129-161: Test case for invalid 0xFF argumentThis test correctly validates that using 0xFF as an argument for the Test command results in an error with the expected error code (0x10). The assertions properly verify the error conditions and timing constraints.
src/core/DynaRec_aa64/recompiler.cc (3)
251-252: Changed counter types to unsigned for cycle calculationThe variables
countand the newextravariable are now properly typed as unsigned, which is more appropriate for counters that shouldn't be negative.
295-295: Added tracking for ROM instructionsThis additional check identifies when instructions are located in ROM (at address range 0xbfc00000) and increments the
extracounter, allowing different cycle calculation for ROM instructions.
313-313: Improved cycle accounting for ROM instructionsThe cycle calculation now properly accounts for ROM instructions by applying the
ROM_EXTRA_BIASmultiplier to theextracounter. This creates more accurate emulation timing by differentiating between ROM and RAM instruction execution speeds.src/cdrom/cdriso.h (4)
51-51: Fixed unsigned comparison in getTN()Changed from
1to1Uto ensure proper unsigned integer comparison withm_numtracks, avoiding potential sign-related issues.
55-57: Added new CD positioning methodsTwo new methods have been added:
getLocP()- Retrieves position information for a given MSF (Minutes/Seconds/Frames) timegetTrack()- Returns the track number corresponding to a given MSF timeThese additions enhance the CD-ROM handling capabilities of the class.
58-58: Re-added error state check methodThe
failed()method has been reintroduced, providing a way to check if the ISO handling has encountered errors.
152-152: Changed track count to unsigned typeModified
m_numtracksfrominttounsigned, which is more appropriate for a count that should never be negative. This change aligns with the unsigned parameter types used in related methods.src/mips/tests/pcdrv/Makefile (2)
3-3: Simplified build configuration with common MakefileIncluding
../common.mkcentralizes build configurations, reducing duplication across test Makefiles.
5-7: Streamlined source file listThe source file list has been simplified to include only the essential
pcdrv.cfile. Common dependencies are likely now handled by the included common Makefile.src/mips/tests/cdrom/cdlnop.c (1)
97-128: Detailed timing analysis in the busy flag testThe
cdlNopBusyTimetest adds value by measuring both the busy flag timing and the overall processing time. This provides important timing information for accurate emulation.The debug output is particularly useful, showing busy flag time and actual processing time separately.
src/mips/tests/cdrom/cdlgetlocl.c (2)
29-63: Good documentation of edge case behaviorThe test includes excellent comments explaining the controller behavior, particularly noting the "bug" where GetLocL works right after reset even though it's only supposed to work during data transfer. These kinds of documentation details are valuable for accurate emulation.
The assertions and timing validations are appropriately set up.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
65-104: Well-structured error case verificationThe test appropriately checks the error condition when GetLocL is called after seekP without a subsequent read. The assertions correctly verify error codes and response values.
The comments clearly explain the expected behavior and why the error occurs.
src/gui/gui.cc (3)
1213-1213: Consistent replacement of check() with parseIso()All instances of
PCSX::g_emulator->m_cdrom->check()have been consistently replaced withparseIso(). This suggests a more robust ISO handling mechanism that parses ISO data rather than just checking it.This change appears in multiple places where ISO files are loaded, reloaded, or closed, ensuring consistent behavior throughout the application.
Also applies to: 1218-1218, 1523-1523, 2674-2674
1287-1294: New CD-ROM lid operations addedAdded explicit methods for CD-ROM lid operations:
openLid()closeLid()scheduleCloseLid()These provide more granular control over the CD-ROM drive emulation, allowing for simulation of the physical CD handling process.
2346-2358: Updated scheduler terminology and organizationThe change from "Interrupt Scaler" to "Scheduler Scaler" reflects a conceptual shift from interrupt-based to scheduler-based timing control. The corresponding array elements have been updated to use more specific CD-ROM related terminology: "CDR FIFO", "CDR Command", "CDR Reads".
The variable reference has also been updated from
m_interruptScalestom_scheduleScales, maintaining consistency with the new naming scheme.src/mips/tests/cdrom/cdlsetmode.c (2)
63-95: Good negative test case for missing argumentsThe test effectively verifies error handling when required arguments are missing. The assertions correctly check for the expected error response (cause 5) and error code (0x20).
The timing assertions and debug output provide useful information for understanding the command behavior.
97-133: Thorough error handling test for excessive argumentsThis test effectively verifies that providing too many arguments to the setMode command generates the appropriate error. It correctly validates the error response and codes.
The repeated setting of the same value (0x80) to CDROM_REG2 on lines 107-110 is an effective way to simulate providing too many arguments.
src/mips/tests/cdrom/cdlid.c (5)
29-76: The cdlId test looks well-structured and comprehensive.This test thoroughly validates the basic CD-ROM ID command by checking acknowledgment, response data, control register values, and timing constraints. The timing assertions (500-7000μs) provide good bounds while allowing for hardware variability.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
78-113: Good test for edge case handling with too many arguments.This test properly verifies error handling when the cdlId command is provided with too many arguments. It checks for the expected error code (0x20) and ensures proper response formats.
115-178: Well-designed test for data buffer reading behavior.This test checks an important edge case - reading exactly 12 bytes from the response buffer and then issuing a new command. This helps verify the controller's behavior when not all data is read before a subsequent command.
180-243: Good coverage of alternative read buffer sizes.Testing with 17 bytes (which exceeds the 8-byte response size) is an excellent boundary test that validates the controller's behavior when reading past the actual response data.
245-338: Comprehensive test for excessive data reading.This test properly validates that reading way too much data (32 bytes) doesn't cause issues. The verification of repeating patterns in the data (lines 299-306) is particularly valuable to confirm the hardware's buffering behavior.
src/core/psxemulator.h (3)
112-112: Good addition of hardware-level CD-ROM logging capability.Adding a separate logging setting for hardware-level CD-ROM operations allows for more granular debugging control.
147-152: Proper integration of the new logging setting.The LoggingHWCDROM setting has been correctly added to the Settings typedef, maintaining the formatting pattern.
233-235: Good conversion to static constexpr for timing constants.Making these timing constants static constexpr is beneficial as it:
- Enforces compile-time evaluation
- Makes it clear these values are constants
- May allow for compiler optimizations
The addition of ROM_EXTRA_BIAS with a value of 10 appears to be for finer timing adjustments for ROM operations.
src/core/sstate.cc (2)
34-117: Good restructuring of the SaveState construction.The code has been reformatted to use a cleaner, more consistent structure with proper indentation. The static analysis correctly notes this reduced the function from 138 to 86 lines. The addition of the ScheduleMask field and replacement of IntTargetsField with ScheduleTargetsField suggests a move to a more unified scheduling system.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[notice] 34-117: ✅ Getting better: Large Method
PCSX::SaveStates::constructSaveState decreases from 138 to 86 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
114-116: Simplified CDRom structure in the save state.The CDRom structure has been significantly simplified to only maintain the CDDataFIFO. This suggests a redesign of how CD-ROM state is handled in the emulator, likely moving to a more encapsulated approach.
src/mips/tests/cdrom/cdlinit.c (3)
29-77: Well-structured CD-ROM initialization test.This test thoroughly validates the basic CD initialization command, checking both acknowledgment and completion phases with appropriate timing assertions. The comments about expected timing values (2ms for ack, 120ms for completion) are particularly helpful.
79-131: Good test for delayed acknowledgment behavior.This test verifies an important aspect of the CD-ROM controller - that it continues initialization in the background even if the acknowledgment is delayed. The thorough comments explain the expected behavior and timing constraints.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 98-98: syntax error
(syntaxError)
133-188: Thorough test for error handling with incorrect arguments.This test properly validates the controller's behavior when initialization is called with invalid arguments, ensuring it returns the expected error code (0x20) and that subsequent commands still work correctly.
src/mips/tests/cdrom/cdlseekl.c (5)
29-86: Well-structured test case for basic seekL functionality.This test case thoroughly verifies the basic CD-ROM seekL command, with clear assertions for expected values and helpful comments explaining timing variations. The test properly checks both acknowledgment and completion phases of the operation.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
88-132: Good error case handling for seekL with too many arguments.This test appropriately verifies the error response when providing too many arguments to the seekL command. It correctly checks for error code 0x20, which indicates an invalid parameter.
134-197: Comprehensive test for seekL between specific tracks.The test thoroughly validates seeking from track 2 to track 4, with appropriate timing checks and acknowledgment verification. The comment at lines 193-195 helpfully explains why precise timing intervals aren't enforced in the assertions.
199-264: Good error case handling for seeking past the end of data track.This test correctly validates behavior when seeking to a sector beyond the data track (0x71:00:00), verifying the proper error code (6, 4) is returned. The comment at lines 259-262 provides excellent context about the expected timing behavior.
266-332: Thorough test for seeking beyond the physical end of disc.This test provides important coverage for error handling when attempting to seek beyond the physical disc boundary (0x85:00:00). The verification of error code (6, 16) ensures proper detection of this condition. The comments at lines 326-330 clearly explain the expected timing differences compared to the previous test.
src/core/psxdma.h (5)
25-27: Updated DMA interrupt scheduling to use new Schedule enum.The changes to the
scheduleGPUDMAIRQfunction reflect a systematic shift from using specific interrupt constants to a more structured enumeration approach usingPCSX::Schedule::GPUDMA.
29-31: Consistent update to SPU DMA scheduling.The changes to the
scheduleSPUDMAIRQfunction maintain consistency with the new scheduling approach using the enumeration system.
33-35: Aligned MDEC output DMA scheduling with new system.The consistent pattern of updates continues with the MDEC output DMA scheduling function.
37-39: Updated MDEC input DMA scheduling.The
scheduleMDECINDMAIRQfunction has been updated to use theSchedule::MDECINDMAenum value, maintaining the same parameter structure but using the new scheduling system.
41-43: Completed transition to enum-based scheduling for GPU OTC DMA.The update to the
scheduleGPUOTCDMAIRQfunction completes the consistent application of the scheduling system changes across all DMA-related interrupt scheduling functions.src/core/psxinterpreter.cc (26)
372-385: Simplified ADDIU implementation by removing unnecessary _u32() casting.The code now directly uses register values without the _u32() wrapper, making the code cleaner and likely more efficient. This change preserves the same behavior while improving code readability.
386-394: Removed unnecessary type casting in ANDI operation.Similar to the ADDIU change, the ANDI operation has been simplified by directly using register values without additional type casting.
395-403: Simplified ORI implementation for better readability.The ORI operation now directly uses register values without the _u32() wrapper, maintaining consistency with other similar changes in the file.
404-412: Streamlined XORI implementation.The XORI operation has been simplified by removing unnecessary type casting, consistent with other bitwise operations in this PR.
413-421: Improved signed comparison in SLTI implementation.The update to SLTI now uses an explicit int32_t cast for the signed comparison instead of the previous _i32() function, making the code's intent clearer.
422-430: Simplified SLTIU implementation.The SLTIU operation now directly uses register values with a more straightforward comparison syntax.
519-527: Streamlined AND register operation.The AND operation between registers has been simplified by removing unnecessary type casting, consistent with the approach taken for immediate operations.
528-536: Simplified OR register operation.The OR operation between registers follows the same pattern of removing unnecessary type casting for cleaner code.
537-545: Streamlined XOR register operation.The XOR operation has been simplified by directly using register values without additional type casting.
546-554: Simplified NOR register operation.The NOR operation now uses a cleaner syntax without unnecessary type conversions.
555-563: Improved signed comparison in SLT register operation.The SLT operation now uses explicit int32_t casts for both operands, making the signed comparison more explicit and clearer.
564-572: Simplified SLTU register operation.The SLTU operation now directly compares register values without unnecessary type casting.
854-855: Simplified base address calculation in oB macro.The oB macro has been simplified to directly add registers and immediate values without the _u32() wrapper.
856-864: Updated LB memory access to use simpler expression.The LB instruction now uses the simplified oB macro and has removed unnecessary type casting in the delayed load reference.
865-873: Streamlined LBU memory access implementation.The LBU instruction implementation has been simplified by removing unnecessary type casting.
874-890: Updated LH memory access to use simpler expressions.The LH instruction implementation has been simplified while maintaining the same alignment check and error handling logic.
891-907: Streamlined LHU memory access implementation.The LHU instruction now uses the simplified approach consistent with other load operations.
931-932: Simplified LW delayed load implementation.The LW instruction's delayed load reference now uses a cleaner syntax without unnecessary type casting.
942-943: Updated LWL memory access implementation.The LWL instruction now uses a cleaner approach to delayed loading with explicit mask parameter.
960-961: Streamlined LWR memory access implementation.The LWR instruction's delayed load follows the same pattern of simplification as other load operations.
1009-1009: Simplified SWL memory write implementation.The SWL instruction now directly uses the register value without unnecessary type casting.
1031-1031: Streamlined SWR memory write implementation.The SWR instruction now directly uses the register value without unnecessary type casting, consistent with the changes to SWL.
1049-1049: Updated MFC0 to use explicit int32_t cast.The MFC0 instruction now uses an explicit int32_t cast, improving code clarity for the signed value transfer.
1055-1055: Updated CFC0 to use explicit int32_t cast.The CFC0 instruction now uses an explicit int32_t cast, similar to MFC0.
1087-1088: Simplified MTC0 and CTC0 function implementations.Both functions have been updated to directly use register values without unnecessary type casting.
1632-1632: Added cycle penalty for BIOS code execution.This change adds an additional bias to the cycle count when executing code in the BIOS range, likely to better simulate the real hardware timing characteristics.
src/mips/tests/cdrom/cdlgetlocp.c (5)
29-89: Well-structured test for basic GetLocP functionality.This test thoroughly verifies the CD-ROM's GetLocP command after reset, with comprehensive checks for track, index, timing information, and register states. The detailed comments explain the expected behavior, including the calculation of time values and the meaning of pregap states.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
91-151: Comprehensive test for GetLocP after seekP operation.This test case effectively verifies the GetLocP command's behavior after a physical seek operation. The assertions properly validate that the position is near the requested seek location with appropriate tolerances, and the detailed comments explain the expected position variations.
153-216: Thorough validation of GetLocP after seekL operation.This test provides important validation of the more precise logical seek operation, with appropriate assertions for the tighter tolerance expected compared to the physical seek. The comments clearly explain the differences between seekL and seekP in terms of precision.
219-277: Good coverage for GetLocP in a specific track.This test provides important validation of the GetLocP command when positioned within track 5. The assertions properly verify the track number, index, and relative/absolute time values, with detailed comments explaining the expected behavior.
279-336: Thorough test for GetLocP in pregap region.This test effectively verifies the GetLocP command's behavior when positioned in the pregap area of track 5. The assertions correctly validate the track number, index value (0 for pregap), and the countdown nature of the relative time in the pregap area.
src/mips/tests/cdrom/cester-hw.c (1)
280-294:❓ Verification inconclusive
Verify potential false-positive syntax error.
Static analysis reported a syntax error around line 293, but the code seems syntactically valid.Please confirm by running a build or quickly checking this region:
If nothing is flagged, it is likely a false positive.
🏁 Script executed:
#!/bin/bash # Attempt to locate potential syntax issues around CESTER_AFTER_ALL declarations. ast-grep --pattern $'CESTER_AFTER_ALL($_,\n $$$\n)'Length of output: 51
Action Required: Confirm Manual Verification for CESTER_AFTER_ALL Macro Syntax
The automated check using the provided ast-grep command did not output any issues, suggesting that the reported syntax error around the CESTER_AFTER_ALL declaration is likely a false positive. However, since the output was inconclusive (i.e., no explicit confirmation was produced), please verify manually—ideally by running a build or additional static analysis on this region—to ensure there are no hidden problems.
- File:
src/mips/tests/cdrom/cester-hw.c- Region: Lines 280–294 (covers both CESTER_BEFORE_ALL and CESTER_AFTER_ALL blocks)
- Findings: The ast-grep check did not flag any issues with the CESTER_AFTER_ALL macro syntax.
Once verified, you can safely ignore the static analysis warning if the build and further inspection confirm the code is valid.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 293-293: syntax error
(syntaxError)
src/mips/tests/cdrom/cdlseekp.c (7)
29-86: Thorough test forcdlSeekP.
The sequence of resetting, setting location, waiting for interrupts, and checking response codes is clear and well-structured.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
88-132: Validation for argument overflow withcdlSeekP.
Verifies that sending too many parameters triggers an error. Good coverage of edge cases.
134-197: Incremental location testcdlSeekP2to4.
You correctly handle the time checks and response codes. The test logic is clear; no issues observed.
199-261: Long distance seek testcdlSeekP2to71.
Good check for ensuring the seek operation takes a substantial amount of time. Implementation looks solid.
263-329: Seek exceeding disc bounds incdlSeekP2to85.
You appropriately check error conditions, making sure the cause is5and the response code is6. This is solid negative-case testing.
331-407:cdlSeekP2to85AndNop: verifying mid-operation commands.
Tests how NOP interacts during a long seek, ensuring consistent interrupts and responses. Good concurrency approach.
409-482:cdlSeekP2to85AndDelayedNop: validating delayed command.
Great job testing a delayed NOP scenario to ensure the drive processes the current seek first. Implementation looks correct.src/mips/tests/cdrom/cdlreadn.c (8)
29-137: Basic 1x read testcdlReadN1x.
Test covers reset, location setup, waiting for read readiness, partial sector check, and pause handling. No immediate issues found.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
138-253: DMA read test at 1x speedcdlReadN1xwithDMA.
Verifies the DMA transfer boundaries, interrupt flow, and correct data. Straightforward, well-implemented.
255-373: 2x read testcdlReadN2x.
Good checks for speed switching overhead (650 ms) and partial data read. Thorough timing assertions.
374-499: DMA read test at 2x speedcdlReadN2xwithDMA.
Similar to the above, but verifying full DMA sector retrieval. All logic is consistent.
501-639: Testing 2x read with mode 0xA0cdlReadN2xwithDMAMode02.
Checks reading in a 2340-byte mode. This extends coverage for different CD sector layouts. Well done.
640-758: Runaway read test at 2x speedcdlReadN2xRunaway.
Ensures partial read occurs before the pause command. The multi-ack approach is well tested. No issues spotted.
759-811: Reading in audio track testcdlReadNInAudio.
Correctly expects an error for attempting to read a purely audio region. Excellent negative-case scenario.
812-864: Read beyond the disc limit testcdlReadNTooFar.
Validates the error cause is5and checks the response code. Good coverage for out-of-range reads.src/mips/tests/cdrom/cdlreads.c (1)
620-671: Check for edge cases with audio tracks
TestcdlReadSInAudiois good for negative scenarios. Consider adding boundary checks for tracks near the edges of the disc to ensure behavior is correct for start or end-of-audio track reads.src/mips/tests/cdrom/reading.c (1)
113-199: Invoke checks around intermediatesetLoccalls
InsetLocDuringSimpleReading, asetLocis done mid-read at line 178. Ensure that ifsetLocfails (e.g., returns 0), further reading or the final checks handle the incomplete transition gracefully.src/supportpsx/iec-60908b.h (4)
131-148: Validate boundary handling for negative MSF values.
The pre-decrement operator wraps around from 00:00:00 to 99:59:74, but there's a risk of unintended behavior if MSF values are expected never to exceed real-world CD durations. Confirm that the wrap logic aligns with the intended CD-Rom specification and that no negative overflow occurs from external usage.
149-153: Confirm intended usage of pre- vs post-decrement.
The post-decrement operator creates a temporary copy, but the code reuses the same wrap logic from pre-decrement. Check whether client code relies on the old value before wrap or after wrap to avoid subtle bugs.
154-158: Assess potential overflow in operator+.
When adding two MSF instances, the resulting LBA computation might overflow a 32-bit integer if the values are extreme. Check whether these additions exist in realistic CD domain or can exceed 99:59:74.
203-248: Ensure correctness of SubHeaders flag handling.
All flags are set/cleared via bitwise operations. This looks good, but carefully confirm the correct bits (0x01, 0x02, etc.) match intended subMode usage, and there’s no overlap or missing definitions.src/core/r3000a.h (8)
24-24: Confirm need for include.
This addition is fine as we’re using std::chrono. Just ensure no leftover references or partial usage remain if no longer needed.
191-191: Validate array bounds for scheduleTargets.
We see 32 elements for each possible enum. Ensure new entries never exceed 31 or cause out-of-range access, as the code currently uses(1 << s)withscast to unsigned.
195-201: Check time conversion correctness.
durationToCyclesdepends onEmulator::m_psxClockSpeed. Ensure the integer multiplication and division order cannot lose precision for large durations.
309-313: Consider unschedule side effects or concurrency.
Unscheduling sets the bit to 0, but does not clear thescheduleTargets[s]. If code checks the array after unscheduling, it might see stale data. Confirm this is intentional or safe.
315-318: Potential race in isScheduled?
If other threads schedule or unschedule concurrently, this check may be out of date. If concurrency is a factor, adding atomic operations or locks could be required.
321-323: Confirm array size sufficiency for scheduleScales.
We have 15 floats, but the Schedule enum has 11 entries. Four spares remain. Ensure that expansions won’t index out-of-bounds if new entries are added to Schedule in the future.
327-327: Reset logic looks correct.
Resettingm_regs.scheduleMaskis consistent with the new scheduling approach. Ensure any scheduled callbacks are re-registered as needed upon reset.
342-343: Validate template usage in delayedLoadRef.
Allowing various types is flexible. Confirm that reinterpreting stored data as T is valid for all usage contexts (e.g., storing 64-bit, then reading as 32-bit).src/core/cdrom.h (9)
24-24: Confirm usage of <string_view>.
If only partial usage is needed, confirm that the overhead of including<string_view>is justified. Otherwise, looks good if we’re usingstd::string_viewin pushPayloadData or parseIso.
44-44: Clearer alias for MSF.
UsingIEC60908b::MSFdirectly is fine, but this alias is neat. Good choice to keep code more readable.
48-48: Check difference from old isLidOpened().
Renamed toisLidOpen. Confirm no external references to the old name remain.
49-52: Ensure correct behavior after closeLid.
Settingm_lidOpen = falsemight need additional state resets or transitions. Verify that scheduling tasks (e.g. reading from disc) re-initialize correctly once the lid is closed.
58-64: Potential race condition in scheduleCloseLid.
m_lidCloseScheduled= true until we reachm_lidCloseAtCycles. If an external call closes the lid before that point, consider clearing or adjusting the schedule.
111-145: Re-check defaults for new members.
Be certain each new boolean by default is set to the correct initial state. For instance,m_motorOn = falsemight conflict if some flows expect the motor to auto-start upon disc insertion.
155-159: Validate currentPosition vs. seekPosition usage.
If these are updated in different scheduling callbacks, concurrency or ordering issues might arise. Confirm usage is well synchronized, or if single-threaded, clarify in comments.
160-203: QueueElement approach seems flexible.
The design handles separate payload and single-value storage. Be mindful ofpayloadIndexwrap-around at line 194; ensure it cannot loop infinitely ifpayloadIndexgrows accidentally.
207-207: responseFifoFull correctness.
A response FIFO with two entries might become full. If there's future expansion to multiple response entries, be mindful of indexing. The current approach is consistent if we always keep two slots.src/mips/tests/cdrom/race.c (9)
1-25: License information looks good.The MIT license is properly included with copyright information for PCSX-Redux authors.
29-72: Test case properly validates GetLocP and Nop race condition.The test correctly resets the CD-ROM, seeks to a specific position, and tests the interaction between the
CDL_GETLOCPandCDL_NOPcommands. The assertions comprehensively verify the expected behavior including response cause, control register states, and timing constraints.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
74-118: Good test coverage for the reverse command order.This test case verifies the opposite command sequence (Nop followed by GetLocP) and properly validates the different expected response values. The comparison with the previous test case helps ensure command ordering is properly handled by the CD-ROM implementation.
120-161: Tests for Nop and GetTD commands interaction are comprehensive.Both test cases (NopAndGetTD1 and GetTD1AndNop) are well structured and validate important aspects of the CD-ROM behavior:
- Different response values depending on command order
- Appropriate control register states during command execution
- Proper timing constraints for acknowledgments
The assertions provide good coverage of the expected behavior.
Also applies to: 163-203
205-291: Thorough test for complex seek operations with acknowledgments.This test case checks a more complex scenario with SeekP followed by a Nop command. It properly:
- Verifies initial seek operation from 00:02:00 to 85:00:00
- Captures and validates multiple acknowledgment signals
- Checks error conditions that occur after the operations
- Validates control register state transitions throughout the process
The comprehensive assertions ensure the CD-ROM behavior is correctly implemented.
293-375: Detailed testing of seek operation with timing constraints.This test case focuses on SeekL operation with timing validation:
- Uses a 2-second wait to test long operations
- Properly stalls with a Nop command
- Verifies the control register values at different stages
- Includes assertions for both acknowledgment events
The specific timing assertions help ensure the CD-ROM implementation meets performance requirements.
377-431: Good coverage of sequential Nop commands.This test properly verifies how consecutive Nop commands are handled:
- Tests control register states between commands
- Verifies acknowledgment timing characteristics
- Checks response values for each command
The comment on lines 426-427 shows good awareness of timing behavior limitations.
433-460: Command queuing tests provide good coverage.These two test cases validate command queuing behavior:
nopQueuetests the number of commands that can be queuednopQueueBusychecks the busy flag transitions during queuingTogether they ensure the CD-ROM implementation handles command queuing properly.
Also applies to: 462-487
489-533: Comprehensive testing of response and acknowledgment handling.These four test cases thoroughly test the interaction between reading responses and acknowledging interrupts:
- Reading response before acknowledgment
- Acknowledging before reading response
- Mixed order of operations for two different commands
This set of tests ensures the CD-ROM implementation correctly handles all permutations of response and acknowledgment ordering, which is critical for proper emulation.
Also applies to: 535-579, 581-625, 627-670
src/mips/tests/cdrom/cdlgettd.c (5)
1-25: License information looks good.The MIT license is properly included with copyright information for PCSX-Redux authors.
29-62: Test for GetTD track 0 is well structured.This test properly validates:
- CD-ROM reset functionality
- Correct setting of registers for GetTD command
- Response validation including status codes and data values
- Control register state transitions
- Timing constraints for the operation
The test provides a good baseline for the GetTD command testing.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: syntax error
(syntaxError)
64-97: Good coverage of various track numbers.These tests comprehensively validate the GetTD command for multiple valid track numbers (1-6, 12, 25). Each test:
- Verifies appropriate response values for each track
- Ensures consistent timing behavior
- Validates control register state transitions
The wide range of track numbers provides good coverage of the CD-ROM's track data functionality.
Also applies to: 100-133, 136-169, 171-204, 206-239, 241-274, 276-309, 311-344
346-378: Error case handling is well tested.These tests verify the CD-ROM's behavior when requested to provide data for:
- Invalid track numbers (26, 99)
- Non-numeric track IDs (0xaa)
- Malformed track IDs (0x0a, 0x1a)
The tests properly verify error codes, response formats, and timing behavior for error conditions.
Also applies to: 380-412, 414-446, 448-480, 482-514
516-547: Edge cases for argument handling are properly tested.The final two tests verify how the CD-ROM handles:
- Missing arguments for the GetTD command
- Too many arguments for the GetTD command
Both tests properly verify the expected error codes and response formats, ensuring robust error handling in the CD-ROM implementation.
Also applies to: 549-582
src/core/sstate.h (5)
59-59: Refactored Interrupt to ScheduleMask for better semantics.This change from Interrupt to ScheduleMask likely reflects a conceptual shift in how scheduling is managed in the emulator. The more specific naming improves code readability and maintainability.
78-78: Renamed IntTargetsField to ScheduleTargetsField for consistency.This renaming aligns with the previous change from Interrupt to ScheduleMask, maintaining naming consistency within the scheduling system.
80-83: Updated Registers message with new schedule-related field names.The Registers message definition now uses the renamed ScheduleMask and ScheduleTargetsField fields, completing the refactoring of the scheduling-related fields.
245-258: Added CDRom structure with essential fields.A new CDRom message structure has been defined with fields for:
- Data, parameter, and response FIFOs
- FIFO indices and sizes
- Register index and state tracking
- Command status and busy indicators
This provides a comprehensive save state representation for the CD-ROM subsystem.
260-263: Updated SaveState message to include CDRom field.The CDRomField has been added to the SaveState message, ensuring that CD-ROM state is properly saved and restored as part of the emulator's save state functionality.
| CESTER_TEST(cdlGetLocLafterSeekL, test_instances, | ||
| int resetDone = resetCDRom(); | ||
| if (!resetDone) { | ||
| cester_assert_true(resetDone); | ||
| return; | ||
| } | ||
|
|
||
| int seekDone = seekPTo(0x50, 0, 0); | ||
| if (!seekDone) { | ||
| cester_assert_true(seekDone); | ||
| return; | ||
| } | ||
|
|
||
| initializeTime(); | ||
| CDROM_REG0 = 0; | ||
| CDROM_REG1 = CDL_GETLOCL; | ||
| uint32_t ackTime = waitCDRomIRQ(); | ||
| uint8_t cause1 = ackCDRomCause(); | ||
| uint8_t ctrl1 = CDROM_REG0 & ~3; | ||
| uint8_t response[16]; | ||
| uint8_t responseSize = readResponse(response); | ||
| uint8_t ctrl2 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause1b = CDROM_REG3_UC; | ||
|
|
||
| cester_assert_uint_eq(5, cause1); | ||
| cester_assert_uint_eq(0xe0, cause1b); | ||
| cester_assert_uint_eq(0x38, ctrl1); | ||
| cester_assert_uint_eq(0x18, ctrl2); | ||
| // Just like with the seekP, we will error out here, as | ||
| // there was no valid read performed. | ||
| cester_assert_uint_eq(2, responseSize); | ||
| cester_assert_uint_eq(3, response[0]); | ||
| cester_assert_uint_eq(0x80, response[1]); | ||
| // Typical value seems to be around 750us, but has | ||
| // been seen to spike high from time to time. | ||
| cester_assert_uint_ge(ackTime, 500); | ||
| cester_assert_uint_lt(ackTime, 7000); | ||
| ramsyscall_printf("Basic getlocL after seekL, ack in %ius\n", ackTime); | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential inconsistency between test name and implementation
This test is named cdlGetLocLafterSeekL but uses seekPTo() on line 114 just like the previous test. This appears to be inconsistent with the test name, which suggests it should be testing after a seekL operation.
If both tests are supposed to use seekP, consider renaming this test to better reflect its purpose, or modify it to use seekL if that was the intention.
🏁 Script executed:
#!/bin/bash
# Check if there's a seekLTo function defined elsewhere in the codebase
rg -A 1 "seekLTo" --type c --type cppLength of output: 1321
Please run the above script to gather information about seekPTo.
🏁 Script executed:
#!/bin/bash
# Search for the definition or usage of seekPTo in the repository
rg -A 1 "seekPTo" --type c --type cppLength of output: 3310
Action Required: Address Mismatch in Seek Function Usage in Test
It appears that the test named cdlGetLocLafterSeekL (in src/mips/tests/cdrom/cdlgetlocl.c) is expected to verify behavior after a "seekL" operation. However, the test currently calls seekPTo(0x50, 0, 0) instead of a seekLTo function. Notably, a proper seekLTo implementation exists (e.g., in src/mips/tests/cdrom/cester-hw.c) and is used in tests like the one in src/mips/tests/cdrom/cdlgetlocp.c.
- Option 1: Change the function call in this test from
seekPTotoseekLToif the intent is to test a "seekL" operation. - Option 2: If the test is actually meant to validate behavior following a seekP operation, then rename the test to better reflect its purpose.
Please confirm the intended behavior and update the test accordingly.
| uint32_t ackTime1 = waitCDRomIRQ(); | ||
| uint8_t cause1 = ackCDRomCause(); | ||
| uint8_t ctrl1 = CDROM_REG0 & ~3; | ||
| uint8_t response1[16]; | ||
| uint8_t responseSize1 = readResponse(response1); | ||
| uint8_t ctrl2 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause1b = CDROM_REG3_UC; | ||
|
|
||
| uint32_t readyTime = waitCDRomIRQ() - ackTime1; | ||
| uint8_t cause2 = ackCDRomCause(); | ||
| uint8_t ctrl3 = CDROM_REG0 & ~3; | ||
| uint8_t response2[16]; | ||
| uint8_t responseSize2 = readResponse(response2); | ||
| uint8_t ctrl4 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause2b = CDROM_REG3_UC; | ||
|
|
||
| initializeTime(); | ||
| uint8_t sector[2048]; | ||
|
|
||
| CDROM_REG0 = 0; | ||
| CDROM_REG3 = 0x80; | ||
|
|
||
| uint32_t dicr = DICR; | ||
| dicr &= 0x00ffffff; | ||
| dicr |= 0x00880000; | ||
| DICR = dicr; | ||
| DPCR |= 0x8000; | ||
| DMA_CTRL[DMA_CDROM].MADR = (uintptr_t)sector; | ||
| DMA_CTRL[DMA_CDROM].BCR = (2048 >> 2) | 0x10000; | ||
| DMA_CTRL[DMA_CDROM].CHCR = 0x11000000; | ||
|
|
||
| while ((DMA_CTRL[DMA_CDROM].CHCR & 0x01000000) != 0); | ||
| dicr = DICR; | ||
| dicr &= 0x00ffffff; | ||
| dicr |= 0x88000000; | ||
| DICR = dicr; | ||
| CDROM_REG3 = 0; | ||
| uint32_t readTime = updateTime(); | ||
|
|
||
| initializeTime(); | ||
| CDROM_REG0 = 0; | ||
| CDROM_REG1 = CDL_PAUSE; | ||
| uint32_t ackTime2 = waitCDRomIRQ(); | ||
| uint8_t cause3 = ackCDRomCause(); | ||
| uint8_t ctrl5 = CDROM_REG0 & ~3; | ||
| uint8_t response3[16]; | ||
| uint8_t responseSize3 = readResponse(response3); | ||
| uint8_t ctrl6 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause3b = CDROM_REG3_UC; | ||
|
|
||
| uint32_t completeTime = waitCDRomIRQ() - ackTime2; | ||
| uint8_t cause4 = ackCDRomCause(); | ||
| uint8_t ctrl7 = CDROM_REG0 & ~3; | ||
| uint8_t response4[16]; | ||
| uint8_t responseSize4 = readResponse(response4); | ||
| uint8_t ctrl8 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause4b = CDROM_REG3_UC; | ||
|
|
||
| cester_assert_uint_eq(3, cause1); | ||
| cester_assert_uint_eq(0xe0, cause1b); | ||
| cester_assert_uint_eq(1, cause2); | ||
| cester_assert_uint_eq(0xe0, cause2b); | ||
| cester_assert_uint_eq(3, cause3); | ||
| cester_assert_uint_eq(0xe0, cause3b); | ||
| cester_assert_uint_eq(2, cause4); | ||
| cester_assert_uint_eq(0xe0, cause4b); | ||
| cester_assert_uint_eq(0x38, ctrl1); | ||
| cester_assert_uint_eq(0x18, ctrl2); | ||
| cester_assert_uint_eq(0x38, ctrl3); | ||
| cester_assert_uint_eq(0x18, ctrl4); | ||
| cester_assert_uint_eq(0x38, ctrl5); | ||
| cester_assert_uint_eq(0x18, ctrl6); | ||
| cester_assert_uint_eq(0x38, ctrl7); | ||
| cester_assert_uint_eq(0x18, ctrl8); | ||
| cester_assert_uint_eq(2, response1[0]); | ||
| cester_assert_uint_eq(1, responseSize1); | ||
| cester_assert_uint_eq(0x22, response2[0]); | ||
| cester_assert_uint_eq(1, responseSize2); | ||
| cester_assert_uint_eq(0x22, response3[0]); | ||
| cester_assert_uint_eq(1, responseSize3); | ||
| cester_assert_uint_eq(2, response4[0]); | ||
| cester_assert_uint_eq(1, responseSize4); | ||
| cester_assert_uint_ge(ackTime1, 500); | ||
| cester_assert_uint_lt(ackTime1, 7000); | ||
| cester_assert_uint_ge(ackTime2, 500); | ||
| cester_assert_uint_lt(ackTime2, 7000); | ||
| // Pausing at 1x is ~70ms | ||
| cester_assert_uint_ge(completeTime, 65000); | ||
| cester_assert_uint_eq(1, sector[0]); | ||
| cester_assert_uint_eq('C', sector[1]); | ||
| cester_assert_uint_eq('D', sector[2]); | ||
| cester_assert_uint_eq('0', sector[3]); | ||
| cester_assert_uint_eq('0', sector[4]); | ||
| cester_assert_uint_eq('1', sector[5]); | ||
| ramsyscall_printf("Basic single full sector readS @1x with DMA at 00:02:16, ack1 in %ius, ready in %ius, read in %ius, ack2 in %ius, complete in %ius\n", ackTime1, readyTime, readTime, ackTime2, completeTime); | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive DMA setup
In test cdlReadS1xwithDMA, the DMA configuration logic is repeated. Consider extracting a helper function to ensure consistent DMA setup and simplify future maintenance.
| uint8_t responseSize1 = readResponse(response1); | ||
| uint8_t ctrl2 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause1b = CDROM_REG3_UC; | ||
|
|
||
| uint32_t readyTime = waitCDRomIRQ() - ackTime1; | ||
| uint8_t cause2 = ackCDRomCause(); | ||
| uint8_t ctrl3 = CDROM_REG0 & ~3; | ||
| uint8_t response2[16]; | ||
| uint8_t responseSize2 = readResponse(response2); | ||
| uint8_t ctrl4 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause2b = CDROM_REG3_UC; | ||
|
|
||
| initializeTime(); | ||
| uint8_t sector[2048]; | ||
|
|
||
| CDROM_REG0 = 0; | ||
| CDROM_REG3 = 0x80; | ||
|
|
||
| uint32_t dicr = DICR; | ||
| dicr &= 0x00ffffff; | ||
| dicr |= 0x00880000; | ||
| DICR = dicr; | ||
| DPCR |= 0x8000; | ||
| DMA_CTRL[DMA_CDROM].MADR = (uintptr_t)sector; | ||
| DMA_CTRL[DMA_CDROM].BCR = (2048 >> 2) | 0x10000; | ||
| DMA_CTRL[DMA_CDROM].CHCR = 0x11000000; | ||
|
|
||
| while ((DMA_CTRL[DMA_CDROM].CHCR & 0x01000000) != 0); | ||
| dicr = DICR; | ||
| dicr &= 0x00ffffff; | ||
| dicr |= 0x88000000; | ||
| DICR = dicr; | ||
| CDROM_REG3 = 0; | ||
| uint32_t readTime = updateTime(); | ||
|
|
||
| initializeTime(); | ||
| CDROM_REG0 = 0; | ||
| CDROM_REG1 = CDL_PAUSE; | ||
| uint32_t ackTime2 = waitCDRomIRQ(); | ||
| uint8_t cause3 = ackCDRomCause(); | ||
| uint8_t ctrl5 = CDROM_REG0 & ~3; | ||
| uint8_t response3[16]; | ||
| uint8_t responseSize3 = readResponse(response3); | ||
| uint8_t ctrl6 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause3b = CDROM_REG3_UC; | ||
|
|
||
| uint32_t completeTime = waitCDRomIRQ() - ackTime2; | ||
| uint8_t cause4 = ackCDRomCause(); | ||
| uint8_t ctrl7 = CDROM_REG0 & ~3; | ||
| uint8_t response4[16]; | ||
| uint8_t responseSize4 = readResponse(response4); | ||
| uint8_t ctrl8 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause4b = CDROM_REG3_UC; | ||
|
|
||
| cester_assert_uint_eq(3, cause1); | ||
| cester_assert_uint_eq(0xe0, cause1b); | ||
| cester_assert_uint_eq(1, cause2); | ||
| cester_assert_uint_eq(0xe0, cause2b); | ||
| cester_assert_uint_eq(3, cause3); | ||
| cester_assert_uint_eq(0xe0, cause3b); | ||
| cester_assert_uint_eq(2, cause4); | ||
| cester_assert_uint_eq(0xe0, cause4b); | ||
| cester_assert_uint_eq(0x38, ctrl1); | ||
| cester_assert_uint_eq(0x18, ctrl2); | ||
| cester_assert_uint_eq(0x38, ctrl3); | ||
| cester_assert_uint_eq(0x18, ctrl4); | ||
| cester_assert_uint_eq(0x38, ctrl5); | ||
| cester_assert_uint_eq(0x18, ctrl6); | ||
| cester_assert_uint_eq(0x38, ctrl7); | ||
| cester_assert_uint_eq(0x18, ctrl8); | ||
| cester_assert_uint_eq(2, response1[0]); | ||
| cester_assert_uint_eq(1, responseSize1); | ||
| cester_assert_uint_eq(0x22, response2[0]); | ||
| cester_assert_uint_eq(1, responseSize2); | ||
| cester_assert_uint_eq(0x22, response3[0]); | ||
| cester_assert_uint_eq(1, responseSize3); | ||
| cester_assert_uint_eq(2, response4[0]); | ||
| cester_assert_uint_eq(1, responseSize4); | ||
| cester_assert_uint_ge(ackTime1, 500); | ||
| cester_assert_uint_lt(ackTime1, 7000); | ||
| cester_assert_uint_ge(ackTime2, 500); | ||
| cester_assert_uint_lt(ackTime2, 7000); | ||
| // Switching speed is roughly 650ms for the speed up, | ||
| // and then this also contains the seeking time, and | ||
| // the time to read the first sector, which is 6.66ms | ||
| cester_assert_uint_ge(readyTime, 500000); | ||
| // Pausing at 2x is ~35ms | ||
| cester_assert_uint_ge(completeTime, 32500); | ||
| cester_assert_uint_eq(1, sector[0]); | ||
| cester_assert_uint_eq('C', sector[1]); | ||
| cester_assert_uint_eq('D', sector[2]); | ||
| cester_assert_uint_eq('0', sector[3]); | ||
| cester_assert_uint_eq('0', sector[4]); | ||
| cester_assert_uint_eq('1', sector[5]); | ||
| ramsyscall_printf("Basic single full sector readS @2x with DMA at 00:02:16, ack1 in %ius, ready in %ius, read in %ius, ack2 in %ius, complete in %ius\n", ackTime1, readyTime, readTime, ackTime2, completeTime); | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm DMA status at completion
In test cdlReadS2xwithDMA, consider asserting the final DMA status register to ensure that the transfer has truly completed without errors before proceeding to the next steps.
| CESTER_TEST(cdlSetLocMultiple, test_instances, | ||
| int resetDone = resetCDRom(); | ||
| uint8_t cause; | ||
| if (!resetDone) { | ||
| cester_assert_true(resetDone); | ||
| return; | ||
| } | ||
|
|
||
| initializeTime(); | ||
| CDROM_REG0 = 0; | ||
| CDROM_REG2 = 0; | ||
| CDROM_REG2 = 2; | ||
| CDROM_REG2 = 0; | ||
| CDROM_REG1 = CDL_SETLOC; | ||
| uint32_t time1 = waitCDRomIRQ(); | ||
| cause = ackCDRomCause(); | ||
| CDROM_REG1; | ||
| if (cause != 3) { | ||
| cester_assert_uint_eq(3, cause); | ||
| return; | ||
| } | ||
|
|
||
| initializeTime(); | ||
| CDROM_REG0 = 0; | ||
| CDROM_REG2 = 0x99; | ||
| CDROM_REG2 = 0x59; | ||
| CDROM_REG2 = 0x74; | ||
| CDROM_REG1 = CDL_SETLOC; | ||
| uint32_t time2 = waitCDRomIRQ(); | ||
| cause = ackCDRomCause(); | ||
| CDROM_REG1; | ||
| if (cause != 3) { | ||
| cester_assert_uint_eq(3, cause); | ||
| return; | ||
| } | ||
|
|
||
| initializeTime(); | ||
| CDROM_REG0 = 0; | ||
| CDROM_REG2 = 0x50; | ||
| CDROM_REG2 = 0; | ||
| CDROM_REG2 = 0; | ||
| CDROM_REG1 = CDL_SETLOC; | ||
| uint32_t time3 = waitCDRomIRQ(); | ||
| cause = ackCDRomCause(); | ||
| CDROM_REG1; | ||
| if (cause != 3) { | ||
| cester_assert_uint_eq(3, cause); | ||
| return; | ||
| } | ||
|
|
||
| // Setloc is only changing an internal state. | ||
| // Its response time is very fast, and won't | ||
| // vary regardless of the location, but can | ||
| // still spike to 6ms from time to time. | ||
| cester_assert_uint_ge(time1, 500); | ||
| cester_assert_uint_lt(time1, 7000); | ||
| cester_assert_uint_ge(time2, 500); | ||
| cester_assert_uint_lt(time2, 7000); | ||
| cester_assert_uint_ge(time3, 500); | ||
| cester_assert_uint_lt(time3, 7000); | ||
| ramsyscall_printf("Multiple setloc to 00:02:00, complete in %ius, %ius, %ius\n", time1, time2, time3); | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate repeated code
Test cdlSetLocMultiple uses repeated reset and initialization patterns. Factor these into shared helper functions to make each test more concise and readable.
| CESTER_TEST(simpleReadingWithoutAck, test_instances, | ||
| int resetDone = resetCDRom(); | ||
| if (!resetDone) { | ||
| cester_assert_true(resetDone); | ||
| return; | ||
| } | ||
|
|
||
| int setModeDone = setMode(0x80); | ||
| if (!setModeDone) { | ||
| cester_assert_true(setModeDone); | ||
| return; | ||
| } | ||
|
|
||
| int seekDone = seekLTo(0x20, 0, 0); | ||
| if (!seekDone) { | ||
| cester_assert_true(seekDone); | ||
| return; | ||
| } | ||
|
|
||
| CDROM_REG0 = 0; | ||
| CDROM_REG1 = CDL_READN; | ||
| waitCDRomIRQ(); | ||
| ackCDRomCause(); | ||
| uint8_t response[16]; | ||
| readResponse(response); | ||
|
|
||
| initializeTime(); | ||
| while (updateTime() <= 500000); | ||
|
|
||
| initializeTime(); | ||
| uint32_t time1 = waitCDRomIRQ(); | ||
| uint8_t cause1 = ackCDRomCause(); | ||
| uint8_t ctrl1 = CDROM_REG0 & ~3; | ||
| uint8_t response1[16]; | ||
| uint8_t responseSize1 = readResponse(response1); | ||
| uint8_t ctrl2 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause1b = CDROM_REG3_UC; | ||
|
|
||
| initializeTime(); | ||
| uint32_t time2 = waitCDRomIRQ(); | ||
| uint8_t cause2 = ackCDRomCause(); | ||
| uint8_t ctrl3 = CDROM_REG0 & ~3; | ||
| uint8_t response2[16]; | ||
| uint8_t responseSize2 = readResponse(response2); | ||
| uint8_t ctrl4 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause2b = CDROM_REG3_UC; | ||
|
|
||
| CDROM_REG0 = 0; | ||
| CDROM_REG1 = CDL_PAUSE; | ||
|
|
||
| initializeTime(); | ||
| uint32_t time3 = waitCDRomIRQ(); | ||
| uint8_t cause3 = ackCDRomCause(); | ||
| uint8_t ctrl5 = CDROM_REG0 & ~3; | ||
| uint8_t response3[16]; | ||
| uint8_t responseSize3 = readResponse(response3); | ||
| uint8_t ctrl6 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause3b = CDROM_REG3_UC; | ||
|
|
||
| initializeTime(); | ||
| uint32_t time4 = waitCDRomIRQ(); | ||
| uint8_t cause4 = ackCDRomCause(); | ||
| uint8_t ctrl7 = CDROM_REG0 & ~3; | ||
| uint8_t response4[16]; | ||
| uint8_t responseSize4 = readResponse(response4); | ||
| uint8_t ctrl8 = CDROM_REG0 & ~3; | ||
| CDROM_REG0 = 1; | ||
| uint8_t cause4b = CDROM_REG3_UC; | ||
|
|
||
| cester_assert_uint_eq(1, cause1); | ||
| cester_assert_uint_eq(0xe0, cause1b); | ||
| cester_assert_uint_eq(1, cause2); | ||
| cester_assert_uint_eq(0xe0, cause2b); | ||
| cester_assert_uint_eq(3, cause3); | ||
| cester_assert_uint_eq(0xe0, cause3b); | ||
| cester_assert_uint_eq(2, cause4); | ||
| cester_assert_uint_eq(0xe0, cause4b); | ||
| cester_assert_uint_eq(0x38, ctrl1); | ||
| cester_assert_uint_eq(0x18, ctrl2); | ||
| cester_assert_uint_eq(0x38, ctrl3); | ||
| cester_assert_uint_eq(0x18, ctrl4); | ||
| cester_assert_uint_eq(0x38, ctrl5); | ||
| cester_assert_uint_eq(0x18, ctrl6); | ||
| cester_assert_uint_eq(0x38, ctrl7); | ||
| cester_assert_uint_eq(0x18, ctrl8); | ||
| cester_assert_uint_eq(0x22, response1[0]); | ||
| cester_assert_uint_eq(1, responseSize1); | ||
| cester_assert_uint_eq(0x22, response2[0]); | ||
| cester_assert_uint_eq(1, responseSize2); | ||
| cester_assert_uint_eq(0x22, response3[0]); | ||
| cester_assert_uint_eq(1, responseSize3); | ||
| cester_assert_uint_eq(2, response4[0]); | ||
| cester_assert_uint_eq(1, responseSize4); | ||
| ramsyscall_printf("Long read, ack then pause, ready1 in %ius, ready2 in %ius, ack in %ius, complete in %ius\n", time1, time2, time3, time4); | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate repeated asserts
In simpleReadingWithoutAck, the tests repeatedly assert on cause and control registers. Consider unifying these into a small helper function to reduce duplication.
| MSF operator-(const MSF &other) const { | ||
| MSF tmp = *this; | ||
| tmp -= other; | ||
| return tmp; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify edge behavior for operator-.
Similar to operator+, ensure subtracting larger MSF from smaller MSF is properly validated, or specify wrap-around vs. negative LBA behavior. Consider returning a clamped 00:00:00 if underflow is not desired.
| void schedule(Schedule s_, uint32_t eCycle) { | ||
| unsigned s = static_cast<unsigned>(s_); | ||
| PSXIRQ_LOG("Scheduling callback %08x at %08x\n", s, eCycle); | ||
| const uint64_t cycle = m_regs.cycle; | ||
| uint64_t target = cycle + uint64_t(eCycle * m_interruptScales[interrupt]); | ||
| m_regs.interrupt |= (1 << interrupt); | ||
| m_regs.intTargets[interrupt] = target; | ||
| if (target < m_regs.lowestTarget) m_regs.lowestTarget = target; | ||
| uint64_t target = uint64_t(cycle + eCycle * m_scheduleScales[s]); | ||
| m_regs.scheduleMask |= (1 << s); | ||
| m_regs.scheduleTargets[s] = target; | ||
| int64_t lowest = m_regs.lowestTarget - cycle; | ||
| int64_t maybeNewLowest = target - cycle; | ||
| if (maybeNewLowest < lowest) m_regs.lowestTarget = target; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent float-int casting surprises in schedule().
m_scheduleScales[s] is a float factor. Multiplying the eCycle by a float may introduce rounding. If scheduling events precisely is needed, consider using integer scaling or double for precision.
| void openLid() { | ||
| m_lidOpen = true; | ||
| m_wasLidOpened = true; | ||
| m_lidCloseScheduled = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Open-lid session logic.
When opening the lid and setting m_wasLidOpened, confirm if any partial read or cdda playback must be canceled. The existing code sets no specialized logic for aborting ongoing activities.
No description provided.