Adding command buffer and test command #1911
Conversation
WalkthroughThis pull request introduces a new header defining the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CDRomDevice
participant TestAction
participant CommandFifo
Client->>CDRomDevice: test(commandBuffer, callback)
CDRomDevice->>TestAction: initiate test action
TestAction->>CommandFifo: send(CDL command, commandBuffer)
CommandFifo-->>TestAction: command processed
TestAction-->>CDRomDevice: complete test (callback invoked)
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 0
🧹 Nitpick comments (1)
src/mips/psyqo/src/cdrom-device-test.cpp (1)
42-62: Add error handling to response processing.The
completeandacknowledgemethods always set success to true without examining the response. Consider implementing proper error detection based on the response content.- bool complete(const psyqo::CDRomDevice::Response &) override { + bool complete(const psyqo::CDRomDevice::Response &response) override { + // Check response for potential errors + if (!response.empty() && (response[0] & 0x01)) { + setSuccess(false); + return true; + } setSuccess(true); return true; } - bool acknowledge(const psyqo::CDRomDevice::Response &response) override { + bool acknowledge(const psyqo::CDRomDevice::Response &response) override { + // Check response for potential errors + if (!response.empty() && (response[0] & 0x01)) { + setSuccess(false); + return true; + } setSuccess(true); return true; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mips/psyqo/cdrom-commandbuffer.hh(1 hunks)src/mips/psyqo/cdrom-device.hh(3 hunks)src/mips/psyqo/hardware/cdrom.hh(3 hunks)src/mips/psyqo/src/cdrom-device-test.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: macos-arm-build-and-test
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
🔇 Additional comments (16)
src/mips/psyqo/hardware/cdrom.hh (3)
5-5: Copyright year updated to 2025.The copyright year has been updated from 2023 to 2025.
29-34: LGTM! New header includes support implementation requirements.The added headers provide necessary support for the new functionality:
<stdint.h>for integer type definitions<concepts>and<type_traits>for concept-based constraints"psyqo/cdrom-commandbuffer.hh"to use the new command buffer structure
87-93: LGTM! Well-structured implementation for command buffer sending.The new method correctly extends the existing functionality to support sending commands with variable-length command buffers:
- Initializes Ctrl register to 0
- Populates the Fifo register with each byte from the command buffer
- Sets the Response register with the command value
This maintains consistency with the existing send method pattern.
src/mips/psyqo/cdrom-commandbuffer.hh (5)
1-25: LGTM! Standard MIT license header.The license header follows the project's standard format.
27-32: LGTM! Appropriate header includes.The file includes the necessary headers for the implementation:
<stdint.h>for integer type definitions<concepts>and<type_traits>for concept-based constraint definition
34-35: LGTM! Well-designed concept for command arguments.The concept
CDRomArgumentTypecorrectly constrains command arguments to integral types, ensuring type safety for the command buffer.
37-46: LGTM! Well-structured command buffer implementation.The
CDRomCommandBufferstructure implements a clean interface for setting variable-length command arguments:
- Uses template metaprogramming with concept constraints
- Efficiently calculates buffer size from argument count
- Initializes buffer contents through recursive calls
47-54: LGTM! Clean implementation of recursive buffer initialization.The private implementation uses an elegant recursive pattern:
- Base case for a single argument
- Recursive case for multiple arguments
- Correctly increments position for each recursive call
src/mips/psyqo/cdrom-device.hh (3)
5-5: Copyright year updated to 2025.The copyright year has been updated from 2022 to 2025.
38-38: LGTM! Added include for command buffer support.The include for the new
cdrom-commandbuffer.hhheader is appropriately placed among the other includes.
416-424: LGTM! Well-documented CD-ROM test functionality.The new methods for testing the CD-ROM follow the established pattern in this class:
test: Async version with callbackscheduleTest: Task-queue version for scheduled executiontestBlocking: Synchronous versionThe documentation clearly explains the purpose of these methods.
src/mips/psyqo/src/cdrom-device-test.cpp (5)
1-34: LGTM! Standard headers and includes.The file includes:
- Standard MIT license header
- Necessary headers for implementation
- EASTL atomic for synchronization
35-41: LGTM! TestActionState enum follows existing patterns.The
TestActionStateenum correctly defines the necessary states for the test action:
IDLEat value 0 (meeting theCDRomDeviceStateEnumconcept requirements)TESTstate for ongoing operations
68-71: LGTM! Test method implementation follows established patterns.The
testmethod correctly:
- Asserts that no other action is pending
- Starts the test action with the provided command buffer and callback
73-76: LGTM! Well-implemented scheduled test method.The
scheduleTestmethod follows the established pattern for creating task-queue tasks, capturing the command buffer and converting the completion callback.
78-85: LGTM! Blocking test method implementation.The
testBlockingmethod correctly:
- Asserts no pending actions
- Uses the
BlockingActionRAII pattern for synchronization- Captures the success result from the callback
|
|
||
| #include <concepts> | ||
| #include <type_traits> | ||
|
|
There was a problem hiding this comment.
We're missing a namespace here. Probably psyqo::Hardware::CDRom
src/mips/psyqo/cdrom-device.hh
Outdated
| * | ||
| * @param callback The callback to call when the command operation is complete. | ||
| */ | ||
| void test(CDRomCommandBuffer commandBuffer, eastl::function<void(bool)> &&callback); |
There was a problem hiding this comment.
This probably should be a const reference, for all 3.
| public: | ||
| TestAction() : Action("TestAction") {} | ||
|
|
||
| void start(psyqo::CDRomDevice *device, CDRomCommandBuffer commandBuffer, eastl::function<void(bool)> &&callback) { |
There was a problem hiding this comment.
This CDRomCommandBuffer should be a const reference.
|
|
||
| psyqo::TaskQueue::Task psyqo::CDRomDevice::scheduleTest(CDRomCommandBuffer commandBuffer) { | ||
| return TaskQueue::Task( | ||
| [this, commandBuffer](auto task) { test(commandBuffer, [task](bool success) { task->complete(success); }); }); |
There was a problem hiding this comment.
So, this one will allocate memory implicitly, which isn't something we want. What we should be doing instead here is the following:
- Create a CDRomCommandBuffer inside the s_testAction.
- Copy the buffer to it.
- Add a new
startoverride without the buffer as a parameter, which should just call into the normalstartversion. - Call
startfrom within the task's lambda instead.
This way there's no need to capture the command buffer inside the task's lambda, and the action will hold it statically.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mips/psyqo/src/cdrom-device-test.cpp (1)
5-5: Copyright year should be consistent with other files.The copyright year is set to 2025, which differs from the 2022 used in other files (like cdrom-device.hh).
-Copyright (c) 2025 PCSX-Redux authors +Copyright (c) 2022 PCSX-Redux authors
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mips/psyqo/cdrom-commandbuffer.hh(1 hunks)src/mips/psyqo/cdrom-device.hh(2 hunks)src/mips/psyqo/examples/cdrom-demo/cdrom-demo.cpp(2 hunks)src/mips/psyqo/hardware/cdrom.hh(2 hunks)src/mips/psyqo/src/cdrom-device-test.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mips/psyqo/hardware/cdrom.hh
- src/mips/psyqo/cdrom-commandbuffer.hh
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/mips/psyqo/src/cdrom-device-test.cpp (1)
src/core/system.h (1)
atomic_signal_fence(165-168)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: build-openbios
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: toolchain
🔇 Additional comments (11)
src/mips/psyqo/examples/cdrom-demo/cdrom-demo.cpp (2)
30-30: Appropriate header inclusion.The inclusion of the new header file for
CDRomCommandBufferis correctly placed with the other CD-ROM related headers.
81-83: LGTM! Demonstrates new test functionality.The code correctly demonstrates the use of the new CDRom test functionality by:
- Creating a command buffer
- Setting it with test values
- Invoking the new
testBlockingmethodThis provides a good example of how to use the new feature.
src/mips/psyqo/cdrom-device.hh (2)
38-38: Appropriate header inclusion.The inclusion of the new header file for
CDRomCommandBufferis correctly placed before it's used in the class declaration.
416-424: Well-documented function declarations with proper parameter types.The three new test methods are well-documented with Doxygen-style comments and correctly take the command buffer as a const reference, as previously suggested in the code review.
The function declarations follow the established pattern in this class:
- A method that takes a callback
- A method that returns a task
- A blocking method variant
src/mips/psyqo/src/cdrom-device-test.cpp (7)
37-40: Well-structured enum class definition.The
TestActionStateenum class correctly follows the pattern established for other action states, withIDLEas the first state with value 0, which satisfies theIsCDRomDeviceStateEnumconcept requirement.
46-54: Properly implemented start method with const reference parameter.The implementation correctly takes the command buffer as a const reference, as previously suggested in the code review.
The method follows the established pattern for starting actions:
- Asserting that the device is idle
- Registering the action
- Setting up the callback
- Setting the appropriate state
- Using an atomic fence for memory ordering
- Sending the command
55-57: Efficient overloaded start method.This overload correctly reuses the other start method, passing the internal command buffer, which avoids code duplication.
67-67: Appropriate member variable for command buffer.The class includes a member variable to store the command buffer, which addresses the previous review comment about memory allocation in the task lambda.
74-78: Correctly implemented test method.The method properly asserts that there's no pending action and delegates to the
TestActionclass.
80-86: Memory-efficient scheduleTest implementation.The implementation addresses the previous review comment by:
- Copying the command buffer to the static action's member variable
- Creating a task that uses the overloaded start method without capturing the command buffer
This avoids implicit memory allocation in the lambda capture.
88-95: Proper implementation of the blocking test method.The method correctly:
- Asserts that there's no pending action
- Creates a BlockingAction to handle synchronization
- Starts the test action with a callback that captures the result
Following the established pattern for blocking methods in this class.
|
|
||
| psyqo::Hardware::CDRom::CDRomCommandBuffer commandBuffer; | ||
| commandBuffer.set(0x50, 0xf2, 0xde, 0xad); | ||
| m_cdrom.testBlocking(gpu(), commandBuffer); |
There was a problem hiding this comment.
I didn't realize I committed this change to the repo - is there a clean way to roll back this change or prevent it from getting merged in? Otherwise I can just revert the changes.
There was a problem hiding this comment.
Eh, no need to rewrite history for this. Just push a commit to remove it if you want it gone.
No description provided.