VS Code extension: fix/update templates based on ps1-bare-metal#1970
VS Code extension: fix/update templates based on ps1-bare-metal#1970spicyjpeg wants to merge 4 commits intogrumpycoders:mainfrom
Conversation
📝 WalkthroughWalkthroughTemplates switch from manual CMake executable/post-build steps to toolchain macros (e.g., addPS1Executable/addBinaryFile), and GPU/DMA APIs are renamed and refactored (DMAChain → GPUDMAChain), with accompanying updates to GPU init, packet allocation, texture upload, and font rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant GPUDMA as GPUDMAChain
participant GP0 as GP0 Command Stream
participant VRAM as VRAM/Video HW
App->>GPUDMA: allocateGP0Packet(zIndex, numCommands)
App->>GP0: uploadTexture(image, coords, size)
GP0->>GP0: gp0_flushCache()
App->>GP0: gp0_setPage(...)
App->>GP0: sendGPULinkedList(packet)
GP0->>VRAM: DMA transfer
VRAM-->>GPUDMA: notify DMA done
GPUDMA-->>App: waitForGPUDMADone()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c (1)
195-197: LGTM! Cleaner DMA channel enabling with helper macro.The consolidated approach using
DMA_DPCR_CH_ENABLE()is more maintainable than manual bit shifts. The|= 0pattern is unconventional but harmless—you could simplify toDMA_DPCR |= DMA_DPCR_CH_ENABLE(DMA_GPU) | DMA_DPCR_CH_ENABLE(DMA_OTC);if preferred.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/vscode-extension/templates/bare-metal/cmake-cube/CMakeLists.txt(1 hunks)tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c(2 hunks)tools/vscode-extension/templates/bare-metal/empty-cmake/CMakeLists.txt(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Code Health Review (main)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c
[warning] 195-197: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 87 to 88. 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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
🔇 Additional comments (8)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c (1)
33-33: LGTM! API naming update aligns with ps1-bare-metal.The change from
COP0_SRtoCOP0_STATUSandCOP0_SR_CU2toCOP0_STATUS_CU2reflects updated naming conventions in the ps1-bare-metal library, improving code clarity.tools/vscode-extension/templates/bare-metal/empty-cmake/CMakeLists.txt (3)
28-36: LGTM! Library sources updated for ps1-bare-metal compatibility.The addition of
clz.s,string.s, andcache.salongside the removal of obsolete sources aligns the template with upstream ps1-bare-metal changes.
44-44: LGTM! Default linking simplifies executable configuration.The
link_libraries(common)directive ensures the common library is automatically linked to all executables, reducing boilerplate in subsequent target definitions.
46-50: LGTM! Macro-driven executable creation reduces boilerplate.The
addPS1Executable()macro encapsulates executable creation and PS1 format conversion, eliminating the need for manualadd_custom_commandpost-build steps. This modernizes the build flow and improves maintainability.tools/vscode-extension/templates/bare-metal/cmake-cube/CMakeLists.txt (4)
28-36: LGTM! Library sources synchronized with ps1-bare-metal.Identical to the
empty-cmaketemplate, these source updates maintain compatibility with upstream ps1-bare-metal changes.
44-44: LGTM! Consistent default linking across templates.The
link_libraries(common)directive matches the approach in theempty-cmaketemplate, ensuring consistency.
46-56: LGTM! Multi-source executable with addPS1Executable.The macro correctly handles multiple source files, demonstrating its flexibility beyond the single-file usage in the
empty-cmaketemplate.
58-64: LGTM! Updated documentation reflects macro-based asset embedding.The comments accurately reference the
convertImage()andaddBinaryFile()macros, guiding users on the modernized asset embedding workflow.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.h (1)
7-10:⚠️ Potential issue | 🟡 MinorStale macro name in comment.
The constant was renamed to
GPU_ORDERING_TABLE_SIZE, but the surrounding comment still refers to the oldORDERING_TABLE_SIZE. Update the comment to avoid confusing readers grepping for the macro.📝 Proposed fix
-// In order for Z averaging to work properly, ORDERING_TABLE_SIZE should be set -// to either a relatively high value (1024 or more) or a multiple of 12; see -// setupGTE() for more details. Higher values will take up more memory but are -// required to render more complex scenes with wide depth ranges correctly. +// In order for Z averaging to work properly, GPU_ORDERING_TABLE_SIZE should be +// set to either a relatively high value (1024 or more) or a multiple of 12; +// see setupGTE() for more details. Higher values will take up more memory but +// are required to render more complex scenes with wide depth ranges correctly. `#define` GPU_CHAIN_BUFFER_SIZE 1024 `#define` GPU_ORDERING_TABLE_SIZE 240🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.h` around lines 7 - 10, Update the stale comment to reference the renamed macro GPU_ORDERING_TABLE_SIZE instead of ORDERING_TABLE_SIZE; locate the comment near setupGTE() in gpu.h and change the text so it advises about GPU_ORDERING_TABLE_SIZE needing to be large (e.g., 1024 or multiple of 12) for Z averaging and notes the memory/tradeoffs just as before.
🧹 Nitpick comments (2)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c (1)
309-319: Nit: prefer the canonical PCSX-Redux URL over abit.lyshortlink.Embedding a URL shortener in a template string makes the destination opaque to readers and creates a third-party dependency that can break or be repurposed. Since this is sample code that users will see and learn from, it's worth using the direct project URL.
📝 Proposed change
- "https://bit.ly/pcsx-redux\n" + "https://pcsx-redux.consoledev.net\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c` around lines 309 - 319, Replace the bit.ly shortlink in the string passed to printString with the canonical PCSX-Redux repo URL so the template shows the direct destination; locate the string literal argument in the call to printString (the block using chain, &font, 16, 24, 0) and swap "https://bit.ly/pcsx-redux" for "https://github.com/pcsx-redux/pcsx-redux" (or the correct canonical repo URL) so examples reference the full URL directly.tools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.c (1)
80-91: Confirm caller contract forsendVRAMDatachunking.The slice DMA path requires
length(computed as(width*height + 1)/2) to either be< DMA_MAX_CHUNK_SIZEor be an exact multiple ofDMA_MAX_CHUNK_SIZE— otherwise theassert(!(length % DMA_MAX_CHUNK_SIZE))at line 90 fires. For the cube template's font upload (FONT_WIDTH=96,FONT_HEIGHT=56, 4bpp → image uploadwidth=24,height=56→length = 24*56/2 = 672 = 42*16✅; palette16x1→length = 8✅), the dimensions happen to satisfy this, but it's a sharp edge for users adapting the template.A short comment near the assert (or in
uploadTexture/uploadIndexedTexture) noting the dimension constraint would prevent confusion when learners hit the assert. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.c` around lines 80 - 91, The assert in sendVRAMData enforces that length = (width*height + 1)/2 is either < DMA_MAX_CHUNK_SIZE or an exact multiple of DMA_MAX_CHUNK_SIZE; add a short clarifying comment next to the assert (and/or in uploadTexture/uploadIndexedTexture) stating this caller contract and the dimension constraint (e.g., 4bpp images require width*height/2 to be divisible by DMA_MAX_CHUNK_SIZE or smaller) so users know why the assert exists and what dimensions are required when chunking VRAM uploads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.h`:
- Around line 7-10: Update the stale comment to reference the renamed macro
GPU_ORDERING_TABLE_SIZE instead of ORDERING_TABLE_SIZE; locate the comment near
setupGTE() in gpu.h and change the text so it advises about
GPU_ORDERING_TABLE_SIZE needing to be large (e.g., 1024 or multiple of 12) for Z
averaging and notes the memory/tradeoffs just as before.
---
Nitpick comments:
In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.c`:
- Around line 80-91: The assert in sendVRAMData enforces that length =
(width*height + 1)/2 is either < DMA_MAX_CHUNK_SIZE or an exact multiple of
DMA_MAX_CHUNK_SIZE; add a short clarifying comment next to the assert (and/or in
uploadTexture/uploadIndexedTexture) stating this caller contract and the
dimension constraint (e.g., 4bpp images require width*height/2 to be divisible
by DMA_MAX_CHUNK_SIZE or smaller) so users know why the assert exists and what
dimensions are required when chunking VRAM uploads.
In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c`:
- Around line 309-319: Replace the bit.ly shortlink in the string passed to
printString with the canonical PCSX-Redux repo URL so the template shows the
direct destination; locate the string literal argument in the call to
printString (the block using chain, &font, 16, 24, 0) and swap
"https://bit.ly/pcsx-redux" for "https://github.com/pcsx-redux/pcsx-redux" (or
the correct canonical repo URL) so examples reference the full URL directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13aeb66f-0004-4df4-8e51-f9e181eb5281
📒 Files selected for processing (6)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/font.ctools/vscode-extension/templates/bare-metal/cmake-cube/src/font.htools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.ctools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.htools/vscode-extension/templates/bare-metal/cmake-cube/src/main.ctools/vscode-extension/templates/bare-metal/cmake-cube/src/trig.c
✅ Files skipped from review due to trivial changes (1)
- tools/vscode-extension/templates/bare-metal/cmake-cube/src/trig.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c (1)
182-324: Optional: CodeScene flags growing complexity inmain()— consider a follow-up extraction, not a blocker.Static analysis notes
maingrew from 87 to 97 LoC and remains a high-cyclomatic-complexity function. For an example/template that is meant to be readable top-to-bottom this is a reasonable trade-off, so I'd defer this. If you ever revisit, splitting out the per-frame work into helpers likedrawCubeFaces(chain, frameCounter)andsetupFrame(chain, bufferX, bufferY)would tame the conditional/loop count without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c` around lines 182 - 324, main() has grown in size/complexity; extract per-frame logic into smaller helpers to reduce cyclomatic complexity. Split the loop body into at least two functions such as setupFrame(GPUDMAChain *chain, int bufferX, int bufferY, bool usingSecondFrame) to handle buffer selection, GPUDMA setup, GTE resets/rotation and ordering table init, and drawCubeFaces(GPUDMAChain *chain, int frameCounter) to handle the cube face loop (gte_loadV*/gte_command/GTE_CMD_* processing, culling, zIndex checks, and allocateGP0Packet calls). Ensure each new helper uses existing symbols (GPUDMAChain, allocateGP0Packet, gte_getDataReg, gte_storeDataReg, printString, waitForGP0Ready, sendGPULinkedList) and keep main() as a high-level orchestrator that calls setupFrame(...), drawCubeFaces(...), renders UI (printString), waits for VSync, and sends the linked list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c`:
- Around line 182-324: main() has grown in size/complexity; extract per-frame
logic into smaller helpers to reduce cyclomatic complexity. Split the loop body
into at least two functions such as setupFrame(GPUDMAChain *chain, int bufferX,
int bufferY, bool usingSecondFrame) to handle buffer selection, GPUDMA setup,
GTE resets/rotation and ordering table init, and drawCubeFaces(GPUDMAChain
*chain, int frameCounter) to handle the cube face loop
(gte_loadV*/gte_command/GTE_CMD_* processing, culling, zIndex checks, and
allocateGP0Packet calls). Ensure each new helper uses existing symbols
(GPUDMAChain, allocateGP0Packet, gte_getDataReg, gte_storeDataReg, printString,
waitForGP0Ready, sendGPULinkedList) and keep main() as a high-level orchestrator
that calls setupFrame(...), drawCubeFaces(...), renders UI (printString), waits
for VSync, and sends the linked list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03108e8c-608d-4dc8-a051-b62309fb74e9
📒 Files selected for processing (2)
tools/vscode-extension/templates/bare-metal/cmake-cube/src/gpu.htools/vscode-extension/templates/bare-metal/cmake-cube/src/main.c
1 participant
Quick fixes for the
empty-cmakeandcmake-cubetemplates provided by the PSX.Dev VS Code extension, which no longer compiled due to some minor but breaking changes in the ps1-bare-metal repository they depend on. I haven't bothered to bump the extension's version number as I already did that as part of #1948, which is still unmerged.