Add hardware register viewer debug widget#2009
Add hardware register viewer debug widget#2009nicolasnoble wants to merge 1 commit intogrumpycoders:mainfrom
Conversation
Move ISTAT/IMASK/DPCR/DICR display out of the CPU Registers widget into a dedicated HW Registers window under Debug > Misc hardware. The new widget reads directly from the hardware register shadow memory and decodes all register groups: IRQ, DMA (global + per-channel), timers, and memory control, with editable bit-field checkboxes for IRQ and DMA control registers. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
📝 WalkthroughWalkthroughA new Hardware Registers debug widget is introduced to display and inspect PSX hardware registers (IRQ, DMA, timers, memory control) in the emulator's GUI. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GUI as GUI Main
participant HWRegs as HWRegs Widget
participant Memory as Memory
User->>GUI: Toggle "Show HW Registers" menu item
GUI->>GUI: Set m_hwRegs.m_show = true
GUI->>HWRegs: Call m_hwRegs.draw(this, m_mem, title)
HWRegs->>Memory: Read IRQ/DMA/Timer/MemCtrl registers
Memory-->>HWRegs: Register values
HWRegs->>HWRegs: Render ImGui window (dropdowns, checkboxes, text fields)
Note over HWRegs: Display IRQ, DMA, Timers, Memory Control sections
User->>HWRegs: Edit register checkbox/field
HWRegs->>Memory: Write modified register value
Memory-->>HWRegs: Acknowledgment
HWRegs-->>GUI: Render complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/gui/widgets/registers.cc (1)
167-170:⚠️ Potential issue | 🟡 Minorrgb1 and rgb2 read from wrong registers.
Both
rgb1andrgb2are reading fromregisters->CP2D.n.rgb0instead of their respective registers (rgb1andrgb2). This is a copy-paste error.Fix
- auto rgb1 = registers->CP2D.n.rgb0; + auto rgb1 = registers->CP2D.n.rgb1; ImGui::Text("rgb1: {%i, %i, %i, %i}", rgb1.r, rgb1.g, rgb1.b, rgb1.c); - auto rgb2 = registers->CP2D.n.rgb0; + auto rgb2 = registers->CP2D.n.rgb2; ImGui::Text("rgb2: {%i, %i, %i, %i}", rgb2.r, rgb2.g, rgb2.b, rgb2.c);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/registers.cc` around lines 167 - 170, The code mistakenly reads registers->CP2D.n.rgb0 for both variables; change the assignments so rgb1 is initialized from registers->CP2D.n.rgb1 and rgb2 from registers->CP2D.n.rgb2, and keep the ImGui::Text calls showing rgb1 and rgb2 values (update the referenced struct fields CP2D.n.rgb1 and CP2D.n.rgb2 to match the displayed labels).
🧹 Nitpick comments (3)
src/gui/widgets/hwregs.cc (2)
119-122: Consider using Memory class template methods for consistency.The local
readHWReg32()function directly accessesmemory->m_hard[]. While this works, the codebase has establishedreadHardwareRegister<>()andwriteHardwareRegister<>()template methods in the Memory class (seesrc/core/psxmem.h:226-240). For writes, you're already usingwriteHardwareRegister<>()(e.g., line 147), but reads use this local helper.Consider using the Memory class methods for reads as well to maintain consistency, though this is optional since the current approach works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/hwregs.cc` around lines 119 - 122, Replace the direct memory access in the local helper readHWReg32 by calling the Memory class template readHardwareRegister<uint32_t>() to match existing usage of writeHardwareRegister<>(); update readHWReg32(PCSX::Memory* memory, uint16_t offset) to return memory->readHardwareRegister<uint32_t>(offset) (or equivalent Memory::readHardwareRegister call) so reads use the same abstraction as writes and remove the manual cast into memory->m_hard.
93-117: Unreachable return statement.Line 116
return "?";is unreachable because(src & 1)can only be 0 or 1, and all three timer cases handle both values in their switch statements.Proposed fix
case 1: return "System clock / 8"; } } - return "?"; + return "System clock"; // Default fallback (should not be reached) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/hwregs.cc` around lines 93 - 117, The function timerClockSource has an unreachable trailing return because each switch on (src & 1) already handles both 0 and 1; update timerClockSource to remove the unreachable return by adding a default case in each switch that returns "?" (or a single default branch) and then delete the final return "?"; specifically modify the switches inside timerClockSource (for timer == 0, timer == 1, and the else branch) to include a default: return "?" so control flow is exhaustive without an unreachable statement.src/gui/widgets/hwregs.h (1)
22-22: Unused include.The
<cstdint>include is not used in this header file. The class only usesbool&and pointer types. Consider removing it or moving it to the implementation file where it may be needed.Proposed fix
-#include <cstdint>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/hwregs.h` at line 22, Remove the unused `#include` <cstdint> from src/gui/widgets/hwregs.h: the header only declares pointer types and bool& parameters and does not use any fixed-width integer types, so delete the include here (or, if any implementation now or later needs cstdint, add it to the corresponding .cpp file). Update hwregs.h to rely only on forward declarations and standard types already available, ensuring no references to std::int32_t/etc. remain in the header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/hwregs.cc`:
- Around line 196-214: The DICR checkboxes (variables derived from dicr and
shown via ImGui::Checkbox in the DICR block and inside the per-channel loop
using dmaName(i)) are currently interactive but not written back; make them
clearly read-only by wrapping the display in
ImGui::BeginDisabled()/ImGui::EndDisabled() (or replacing individual checkboxes
with ImGui::Text showing the bit state) so toggles cannot be changed by the
user, e.g., surround the Bus Error/Master IRQ Enable/Master IRQ Flag checkboxes
and the per-channel Completion/IRQ Enable/Triggered checkboxes inside a disabled
region (keep the same labels/IDs like "###dicr_*" and reuse the existing bool
variables computed from dicr).
In `@vsprojects/gui/gui.vcxproj.filters`:
- Around line 112-114: Update the filter entry for
..\..\src\gui\widgets\hwregs.cc in gui.vcxproj.filters so it matches the other
widget sources: change the <Filter> value from "Source Files" to "Source
Files\widgets" to keep filter paths consistent with files like assembly.cc.
- Around line 216-218: The ClInclude entry for hwregs.h uses an inconsistent
filter; update the ClInclude element referencing
"..\..\src\gui\widgets\hwregs.h" so its <Filter> value matches the other widget
headers (use "Header Files\widgets")—locate the ClInclude with hwregs.h and
change the <Filter> text to "Header Files\widgets" to restore consistency with
files like assembly.h.
---
Outside diff comments:
In `@src/gui/widgets/registers.cc`:
- Around line 167-170: The code mistakenly reads registers->CP2D.n.rgb0 for both
variables; change the assignments so rgb1 is initialized from
registers->CP2D.n.rgb1 and rgb2 from registers->CP2D.n.rgb2, and keep the
ImGui::Text calls showing rgb1 and rgb2 values (update the referenced struct
fields CP2D.n.rgb1 and CP2D.n.rgb2 to match the displayed labels).
---
Nitpick comments:
In `@src/gui/widgets/hwregs.cc`:
- Around line 119-122: Replace the direct memory access in the local helper
readHWReg32 by calling the Memory class template
readHardwareRegister<uint32_t>() to match existing usage of
writeHardwareRegister<>(); update readHWReg32(PCSX::Memory* memory, uint16_t
offset) to return memory->readHardwareRegister<uint32_t>(offset) (or equivalent
Memory::readHardwareRegister call) so reads use the same abstraction as writes
and remove the manual cast into memory->m_hard.
- Around line 93-117: The function timerClockSource has an unreachable trailing
return because each switch on (src & 1) already handles both 0 and 1; update
timerClockSource to remove the unreachable return by adding a default case in
each switch that returns "?" (or a single default branch) and then delete the
final return "?"; specifically modify the switches inside timerClockSource (for
timer == 0, timer == 1, and the else branch) to include a default: return "?" so
control flow is exhaustive without an unreachable statement.
In `@src/gui/widgets/hwregs.h`:
- Line 22: Remove the unused `#include` <cstdint> from src/gui/widgets/hwregs.h:
the header only declares pointer types and bool& parameters and does not use any
fixed-width integer types, so delete the include here (or, if any implementation
now or later needs cstdint, add it to the corresponding .cpp file). Update
hwregs.h to rely only on forward declarations and standard types already
available, ensuring no references to std::int32_t/etc. remain in the header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 917cc846-8ae0-41ea-b519-1e6a09e013a3
📒 Files selected for processing (8)
src/gui/gui.ccsrc/gui/gui.hsrc/gui/widgets/hwregs.ccsrc/gui/widgets/hwregs.hsrc/gui/widgets/registers.ccsrc/gui/widgets/registers.hvsprojects/gui/gui.vcxprojvsprojects/gui/gui.vcxproj.filters
| bool busError = (dicr >> 15) & 1; | ||
| ImGui::Checkbox(_("Bus Error###dicr_buserr"), &busError); | ||
| bool masterEnable = (dicr >> 23) & 1; | ||
| ImGui::Checkbox(_("Master IRQ Enable###dicr_master_en"), &masterEnable); | ||
| bool masterFlag = (dicr >> 31) & 1; | ||
| ImGui::Checkbox(_("Master IRQ Flag###dicr_master_flag"), &masterFlag); | ||
| for (unsigned i = 0; i < 7; i++) { | ||
| std::string nodeStr = fmt::format("{}###dicr_ch{}", dmaName(i), i); | ||
| if (ImGui::TreeNode(nodeStr.c_str())) { | ||
| bool completion = (dicr >> i) & 1; | ||
| ImGui::Checkbox(_("Completion###dicr_comp"), &completion); | ||
| bool mask = (dicr >> (i + 16)) & 1; | ||
| ImGui::Checkbox(_("IRQ Enable###dicr_irq_en"), &mask); | ||
| bool triggered = (dicr >> (i + 24)) & 1; | ||
| ImGui::Checkbox(_("Triggered###dicr_trig"), &triggered); | ||
| ImGui::TreePop(); | ||
| } | ||
| } | ||
| ImGui::Unindent(); |
There was a problem hiding this comment.
DICR checkboxes are display-only but appear interactive.
The DICR section displays checkboxes for Bus Error, Master IRQ Enable, Master IRQ Flag, and per-channel flags, but changes are not written back to the hardware register. Users can toggle these checkboxes, but the changes have no effect. This differs from the ISTAT/IMASK/DPCR sections which do persist changes.
Either add writeback logic to make these checkboxes functional, or use ImGui::Text() or disabled checkboxes to indicate they're read-only.
Option A: Make checkboxes read-only
- bool busError = (dicr >> 15) & 1;
- ImGui::Checkbox(_("Bus Error###dicr_buserr"), &busError);
- bool masterEnable = (dicr >> 23) & 1;
- ImGui::Checkbox(_("Master IRQ Enable###dicr_master_en"), &masterEnable);
- bool masterFlag = (dicr >> 31) & 1;
- ImGui::Checkbox(_("Master IRQ Flag###dicr_master_flag"), &masterFlag);
+ ImGui::Text("Bus Error : %s", (dicr >> 15) & 1 ? "Yes" : "No");
+ ImGui::Text("Master IRQ En : %s", (dicr >> 23) & 1 ? "Yes" : "No");
+ ImGui::Text("Master IRQ Flag : %s", (dicr >> 31) & 1 ? "Yes" : "No");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool busError = (dicr >> 15) & 1; | |
| ImGui::Checkbox(_("Bus Error###dicr_buserr"), &busError); | |
| bool masterEnable = (dicr >> 23) & 1; | |
| ImGui::Checkbox(_("Master IRQ Enable###dicr_master_en"), &masterEnable); | |
| bool masterFlag = (dicr >> 31) & 1; | |
| ImGui::Checkbox(_("Master IRQ Flag###dicr_master_flag"), &masterFlag); | |
| for (unsigned i = 0; i < 7; i++) { | |
| std::string nodeStr = fmt::format("{}###dicr_ch{}", dmaName(i), i); | |
| if (ImGui::TreeNode(nodeStr.c_str())) { | |
| bool completion = (dicr >> i) & 1; | |
| ImGui::Checkbox(_("Completion###dicr_comp"), &completion); | |
| bool mask = (dicr >> (i + 16)) & 1; | |
| ImGui::Checkbox(_("IRQ Enable###dicr_irq_en"), &mask); | |
| bool triggered = (dicr >> (i + 24)) & 1; | |
| ImGui::Checkbox(_("Triggered###dicr_trig"), &triggered); | |
| ImGui::TreePop(); | |
| } | |
| } | |
| ImGui::Unindent(); | |
| ImGui::Text("Bus Error : %s", (dicr >> 15) & 1 ? "Yes" : "No"); | |
| ImGui::Text("Master IRQ En : %s", (dicr >> 23) & 1 ? "Yes" : "No"); | |
| ImGui::Text("Master IRQ Flag : %s", (dicr >> 31) & 1 ? "Yes" : "No"); | |
| for (unsigned i = 0; i < 7; i++) { | |
| std::string nodeStr = fmt::format("{}###dicr_ch{}", dmaName(i), i); | |
| if (ImGui::TreeNode(nodeStr.c_str())) { | |
| bool completion = (dicr >> i) & 1; | |
| ImGui::Checkbox(_("Completion###dicr_comp"), &completion); | |
| bool mask = (dicr >> (i + 16)) & 1; | |
| ImGui::Checkbox(_("IRQ Enable###dicr_irq_en"), &mask); | |
| bool triggered = (dicr >> (i + 24)) & 1; | |
| ImGui::Checkbox(_("Triggered###dicr_trig"), &triggered); | |
| ImGui::TreePop(); | |
| } | |
| } | |
| ImGui::Unindent(); |
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 210-210: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING1024'
(syntaxError)
[error] 197-197: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING256'
(syntaxError)
[error] 206-206: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING512'
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gui/widgets/hwregs.cc` around lines 196 - 214, The DICR checkboxes
(variables derived from dicr and shown via ImGui::Checkbox in the DICR block and
inside the per-channel loop using dmaName(i)) are currently interactive but not
written back; make them clearly read-only by wrapping the display in
ImGui::BeginDisabled()/ImGui::EndDisabled() (or replacing individual checkboxes
with ImGui::Text showing the bit state) so toggles cannot be changed by the
user, e.g., surround the Bus Error/Master IRQ Enable/Master IRQ Flag checkboxes
and the per-channel Completion/IRQ Enable/Triggered checkboxes inside a disabled
region (keep the same labels/IDs like "###dicr_*" and reuse the existing bool
variables computed from dicr).
| <ClCompile Include="..\..\src\gui\widgets\hwregs.cc"> | ||
| <Filter>Source Files</Filter> | ||
| </ClCompile> |
There was a problem hiding this comment.
Inconsistent filter path for hwregs.cc.
Other widget source files use the Source Files\widgets filter (e.g., lines 10-11 for assembly.cc). This file should use the same filter for consistency.
Proposed fix
<ClCompile Include="..\..\src\gui\widgets\hwregs.cc">
- <Filter>Source Files</Filter>
+ <Filter>Source Files\widgets</Filter>
</ClCompile>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ClCompile Include="..\..\src\gui\widgets\hwregs.cc"> | |
| <Filter>Source Files</Filter> | |
| </ClCompile> | |
| <ClCompile Include="..\..\src\gui\widgets\hwregs.cc"> | |
| <Filter>Source Files\widgets</Filter> | |
| </ClCompile> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vsprojects/gui/gui.vcxproj.filters` around lines 112 - 114, Update the filter
entry for ..\..\src\gui\widgets\hwregs.cc in gui.vcxproj.filters so it matches
the other widget sources: change the <Filter> value from "Source Files" to
"Source Files\widgets" to keep filter paths consistent with files like
assembly.cc.
| <ClInclude Include="..\..\src\gui\widgets\hwregs.h"> | ||
| <Filter>Header Files</Filter> | ||
| </ClInclude> |
There was a problem hiding this comment.
Inconsistent filter path for hwregs.h.
Other widget header files use the Header Files\widgets filter (e.g., lines 120-122 for assembly.h). This file should use the same filter for consistency.
Proposed fix
<ClInclude Include="..\..\src\gui\widgets\hwregs.h">
- <Filter>Header Files</Filter>
+ <Filter>Header Files\widgets</Filter>
</ClInclude>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ClInclude Include="..\..\src\gui\widgets\hwregs.h"> | |
| <Filter>Header Files</Filter> | |
| </ClInclude> | |
| <ClInclude Include="..\..\src\gui\widgets\hwregs.h"> | |
| <Filter>Header Files\widgets</Filter> | |
| </ClInclude> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vsprojects/gui/gui.vcxproj.filters` around lines 216 - 218, The ClInclude
entry for hwregs.h uses an inconsistent filter; update the ClInclude element
referencing "..\..\src\gui\widgets\hwregs.h" so its <Filter> value matches the
other widget headers (use "Header Files\widgets")—locate the ClInclude with
hwregs.h and change the <Filter> text to "Header Files\widgets" to restore
consistency with files like assembly.h.
1 participant
Move ISTAT/IMASK/DPCR/DICR display out of the CPU Registers widget into a dedicated HW Registers window under Debug > Misc hardware. The new widget reads directly from the hardware register shadow memory and decodes all register groups: IRQ, DMA (global + per-channel), timers, and memory control, with editable bit-field checkboxes for IRQ and DMA control registers.