feat(zaparoo): adopt native CRT video v2 DDR contract#8
Merged
Conversation
The Menu_MiSTer core dropped its status[9] CRT-enable bit and H/V offset status bits; publishing DDR frames is now the mode switch and centering moves into DDR word1 (frontend-owned). Update the Main side to match: - fb mode 320x240 stride 1280 -> 352x240 stride 1408; blank the full 3 MB v2 DDR region. - Remove all status[9] writes and the 500 ms re-assert timer; remove H/V offset handling (status pushes, persistence, getters/setters). - Zero DDR word0/word1 on CRT-path teardown so the core reverts to its noise pattern even after a frontend crash. - Add exit-code-42 respawn: the frontend rewrites zaparoo_launcher_crt.bin itself and exits 42 to be respawned under the new setting. - Remove the OSD CRT/Video pages entirely (launcher owns CRT mode now); this also shrinks the fork's menu.cpp diff against upstream.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR removes Zaparoo launcher/video menu pages and their menu routing, replaces offset/toggle APIs with persisted native-CRT state APIs, updates native CRT geometry/blanking and deterministic teardown, and implements a respawn flow (ALT_LAUNCHER_EXIT_RELOAD) that reloads persisted CRT settings. ChangesZaparoo Launcher Architecture Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/zaparoo/alt_launcher.cpp (1)
138-155: 💤 Low valueConsider extracting the shared DDR address constant.
0x3A000000uis duplicated here and inblank_native_crt_fb()at line 125. Extracting to a shared constant (e.g.,static const uint32_t kNativeCrtDdrAddr = 0x3A000000u;) would reduce the risk of the values diverging if the DDR layout changes again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@support/zaparoo/alt_launcher.cpp` around lines 138 - 155, Extract the duplicated DDR base address literal into a single shared constant (e.g., declare static const uint32_t kNativeCrtDdrAddr = 0x3A000000u) and replace the hard-coded 0x3A000000u occurrences in zero_native_crt_words and blank_native_crt_fb with that constant; keep the existing map_size and behavior, and ensure both functions call shmem_map(kNativeCrtDdrAddr, map_size) / shmem_unmap(...) so the address is maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@support/zaparoo/alt_launcher.cpp`:
- Around line 138-155: Extract the duplicated DDR base address literal into a
single shared constant (e.g., declare static const uint32_t kNativeCrtDdrAddr =
0x3A000000u) and replace the hard-coded 0x3A000000u occurrences in
zero_native_crt_words and blank_native_crt_fb with that constant; keep the
existing map_size and behavior, and ensure both functions call
shmem_map(kNativeCrtDdrAddr, map_size) / shmem_unmap(...) so the address is
maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7dbe0f0-632c-4a2e-b935-ea3cecdec5a2
📒 Files selected for processing (6)
menu.cppsupport/zaparoo/alt_launcher.cppsupport/zaparoo/alt_launcher.hsupport/zaparoo/alt_launcher_menu.cppsupport/zaparoo/launcher_pages.cppsupport/zaparoo/launcher_pages.h
💤 Files with no reviewable changes (4)
- support/zaparoo/launcher_pages.h
- support/zaparoo/launcher_pages.cpp
- support/zaparoo/alt_launcher.h
- menu.cpp
Extend zaparoo_launcher_crt.bin to two bytes [enabled, mode], where mode is the DDR word1 id (0 NTSC 352x240, 1 480i 720x480, 2 PAL 352x288). set_native_crt_fb_mode() programs the matching geometry and enable_native_crt_path() refreshes the mode from the file on every CRT spawn, so a video-standard change in the frontend (rewrite file, exit 42) gets the right framebuffer on respawn - including the T+1s re-assert, which previously stomped any non-NTSC shape back to 352x240. Legacy 1-byte files partial-read as NTSC. Restore a CRT mode toggle to the OSD System Settings menu, under Scripts. It flips byte 0 of the same state file (preserving the standard byte) and respawns the frontend through the same re-entry steps as the exit-42 reload, re-arming after an earlier give-up or escape. Contained in the zaparoo support files; menu.cpp untouched.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/zaparoo/alt_launcher.h (1)
15-16: Confirm removed alt_launcher API call sites have been migrated
- No remaining C/C++ call sites to
alt_launcher_toggle_crt,alt_launcher_h_offset,alt_launcher_v_offset,alt_launcher_set_h_offset, oralt_launcher_set_v_offsetoutsidesupport/zaparoo/alt_launcher.h.- Only remaining references are in
ZAPAROO_FORK.mdas documentation mentioningsupport/zaparoo/alt_launcher.cpp.- Optional: add brief doc comments for
alt_launcher_native_crt_persisted()andalt_launcher_toggle_native_crt()insupport/zaparoo/alt_launcher.h.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@support/zaparoo/alt_launcher.h` around lines 15 - 16, The header declares alt_launcher_native_crt_persisted() and alt_launcher_toggle_native_crt() while older API functions (alt_launcher_toggle_crt, alt_launcher_h_offset, alt_launcher_v_offset, alt_launcher_set_h_offset, alt_launcher_set_v_offset) were removed—confirm there are no remaining C/C++ call sites using those old symbols by searching the repo for those function names and migrating any callers to the new APIs (or updating them to no-op/removal); ensure only documentation (ZAPAROO_FORK.md) references the old alt_launcher.cpp if intended, and optionally add short doc comments above alt_launcher_native_crt_persisted() and alt_launcher_toggle_native_crt() in support/zaparoo/alt_launcher.h describing their purpose and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@support/zaparoo/alt_launcher.h`:
- Around line 15-16: The header declares alt_launcher_native_crt_persisted() and
alt_launcher_toggle_native_crt() while older API functions
(alt_launcher_toggle_crt, alt_launcher_h_offset, alt_launcher_v_offset,
alt_launcher_set_h_offset, alt_launcher_set_v_offset) were removed—confirm there
are no remaining C/C++ call sites using those old symbols by searching the repo
for those function names and migrating any callers to the new APIs (or updating
them to no-op/removal); ensure only documentation (ZAPAROO_FORK.md) references
the old alt_launcher.cpp if intended, and optionally add short doc comments
above alt_launcher_native_crt_persisted() and alt_launcher_toggle_native_crt()
in support/zaparoo/alt_launcher.h describing their purpose and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d692eda8-2194-46a9-a0fb-d7eca3c932ad
📒 Files selected for processing (3)
support/zaparoo/alt_launcher.cppsupport/zaparoo/alt_launcher.hsupport/zaparoo/alt_launcher_menu.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- support/zaparoo/alt_launcher_menu.cpp
- support/zaparoo/alt_launcher.cpp
…-video-v2 # Conflicts: # support/zaparoo/alt_launcher.cpp # support/zaparoo/alt_launcher.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
status[9]CRT-enable writes and the 500 ms re-assert timer, since the new Menu_MiSTer core has no CRT status bit — publishing DDR frames is the mode switch.[13:10]/[17:14], persistence, getters/setters); centering now lives in DDR word1, written by the frontend.zaparoo_launcher_crt.binitself and exits 42 to be respawned under the new CRT setting without restarting Main.menu.cppdiff against upstream.Summary by CodeRabbit