Native CRT video v2: DDR contract, analog video settings, calibration screen#225
Conversation
📝 WalkthroughWalkthroughAdds native CRT path calibration for MiSTer: new CRT fields in config/persist schemas, a ChangesNative CRT Path Calibration
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsScreen
participant MainQML as Main.qml
participant CrtVideo as Browse.CrtVideo (Rust)
participant NativeWriter as zaparoo_native_video_set_offsets
participant Process as execvp (restart)
rect rgba(70, 130, 180, 0.5)
Note over User, Process: CRT Enable Toggle Flow
User->>SettingsScreen: toggle crtEnabled
SettingsScreen->>MainQML: requestAccept("crtEnable"/"crtDisable")
MainQML->>MainQML: stageCrtToggle(enable) → open restartConfirm modal
User->>MainQML: confirm
MainQML->>CrtVideo: write_crt_enable_file(enabled)
MainQML->>Process: Qt.exit(42) → execvp(originalArgv)
end
rect rgba(60, 179, 113, 0.5)
Note over User, NativeWriter: CRT Calibration Flow
User->>SettingsScreen: activate crtCalibration row
SettingsScreen->>MainQML: requestAccept("crtCalibration")
MainQML->>MainQML: openCrtCalibrationModal()
User->>MainQML: directional input
MainQML->>CrtVideo: set_offsets(h, v)
CrtVideo->>NativeWriter: zaparoo_native_video_set_offsets(h, v) [MiSTer FFI]
User->>MainQML: accept/cancel
CrtVideo->>CrtVideo: commit_offsets() → persist to config
MainQML->>MainQML: closeCrtCalibrationModal()
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)src/ui/translations/frontend_ar.tsFile contains syntax errors that prevent linting: Line 1: Expected a type but instead found '?'.; Line 1: expected ... [truncated 171216 characters] ... ne 1551: unterminated regex literal; Line 1552: unterminated regex literal; Line 1553: unterminated regex literal; Line 1555: unterminated regex literal; Line 1557: expected src/ui/translations/frontend_de.tsFile contains syntax errors that prevent linting: Line 1: Expected a type but instead found '?'.; Line 1: expected ... [truncated 160241 characters] ... ne 1551: unterminated regex literal; Line 1552: unterminated regex literal; Line 1553: unterminated regex literal; Line 1555: unterminated regex literal; Line 1557: expected src/ui/translations/frontend_hi.tsFile contains syntax errors that prevent linting: Line 1: Expected a type but instead found '?'.; Line 1: expected ... [truncated 176507 characters] ... ne 1551: unterminated regex literal; Line 1552: unterminated regex literal; Line 1553: unterminated regex literal; Line 1555: unterminated regex literal; Line 1557: expected
Comment |
|
Attached are the menu and main builds that work with this. And this is the overall plan: https://github.com/ZaparooProject/Menu_MiSTer/blob/b0fe65a4de6ba3d0f5390bf6af1dcb9edb405928/docs/native-video-plan.md I've purposely deferred the 480i stuff since I figured this PR will be open for discussion for a while and I don't want merging to be too hard, but the plumbing should be there. In a nutshell:
Looks great for me so far!!! Very nice. It covers my whole screen now and I can adjust it dead centre. |
|
@theypsilon you might also be interested! |
|
Main with fixed PAL mode switch and added CRT mode toggle back to OSD |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/ui/app/MainLayout.qml`:
- Line 95: The CRT calibration modal property crtCalibrationModalRequested does
not have a corresponding help-bar mapping, causing helpEntries to fall through
to the active-screen branch instead of displaying appropriate help actions for
the modal state. Add a help-bar mapping condition that checks
crtCalibrationModalRequested alongside the modal state checks in the helpEntries
logic (around lines 287 and 1459-1470) to ensure that when the calibration modal
is open, the help bar advertises Settings actions instead of falling back to the
active-screen branch behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d9c2b35-ae99-4dbb-a267-fbcd693bd855
📒 Files selected for processing (32)
rust/frontend/build.rsrust/frontend/src/lib.rsrust/frontend/src/mister_runtime.rsrust/frontend/src/models/crt_video.rsrust/frontend/src/models/mod.rsrust/frontend/src/models/settings.rsrust/zaparoo-core/src/config.rsrust/zaparoo-core/src/persist.rssrc/app/main.cppsrc/app/native_video_writer.cppsrc/app/native_video_writer.hsrc/ui/app/Main.qmlsrc/ui/app/MainLayout.qmlsrc/ui/components/CMakeLists.txtsrc/ui/components/CrtCalibrationModal.qmlsrc/ui/screens/SettingsScreen.qmlsrc/ui/translations/frontend_ar.tssrc/ui/translations/frontend_de.tssrc/ui/translations/frontend_el.tssrc/ui/translations/frontend_en.tssrc/ui/translations/frontend_es.tssrc/ui/translations/frontend_eu.tssrc/ui/translations/frontend_he.tssrc/ui/translations/frontend_hi.tssrc/ui/translations/frontend_it.tssrc/ui/translations/frontend_ja.tssrc/ui/translations/frontend_ko.tssrc/ui/translations/frontend_nl.tssrc/ui/translations/frontend_ro.tssrc/ui/translations/frontend_sk.tssrc/ui/translations/frontend_uk.tssrc/ui/translations/frontend_zh_CN.ts
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)
src/ui/translations/frontend_eu.ts (1)
203-205:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix brand typo in welcome string translation.
Line 204 uses “Zaparro Frontend-era”, but the product name is “Zaparoo”. Please correct the translated brand spelling to avoid user-facing inconsistency.
🤖 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 `@src/ui/translations/frontend_eu.ts` around lines 203 - 205, The translation value for the source string "Welcome to Zaparoo Frontend" contains a typo where the product name is misspelled as "Zaparro Frontend-era" instead of "Zaparoo Frontend-era". Correct the translation element to use the proper product name "Zaparoo" in the Basque translation to maintain brand consistency across the user interface.
🤖 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.
Outside diff comments:
In `@src/ui/translations/frontend_eu.ts`:
- Around line 203-205: The translation value for the source string "Welcome to
Zaparoo Frontend" contains a typo where the product name is misspelled as
"Zaparro Frontend-era" instead of "Zaparoo Frontend-era". Correct the
translation element to use the proper product name "Zaparoo" in the Basque
translation to maintain brand consistency across the user interface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63e4c3d6-5667-4e81-ba2f-0729b80fd3f9
📒 Files selected for processing (17)
src/ui/app/MainLayout.qmlsrc/ui/translations/frontend_ar.tssrc/ui/translations/frontend_de.tssrc/ui/translations/frontend_el.tssrc/ui/translations/frontend_en.tssrc/ui/translations/frontend_es.tssrc/ui/translations/frontend_eu.tssrc/ui/translations/frontend_he.tssrc/ui/translations/frontend_hi.tssrc/ui/translations/frontend_it.tssrc/ui/translations/frontend_ja.tssrc/ui/translations/frontend_ko.tssrc/ui/translations/frontend_nl.tssrc/ui/translations/frontend_ro.tssrc/ui/translations/frontend_sk.tssrc/ui/translations/frontend_uk.tssrc/ui/translations/frontend_zh_CN.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/app/MainLayout.qml
Adopt the Menu fork's v2 DDR writer contract and move CRT ownership into the launcher now that Main_MiSTer has no OSD video pages: - Rewrite native_video_writer for the v2 control block (3 MB region, word1 magic/mode/offsets, buffers at +0x1000/+0x180000) with fb0 geometry as the mode selector: 352x240 NTSC, 352x288 PAL, and 720x480i supported in the writer but not yet exposed in the UI. - New Browse.CrtVideo singleton: video standard and centering trims persisted through state.toml + frontend.toml, live word1 pokes for calibration, and the zaparoo_launcher_crt.bin enable flag. - MiSTer-only "Analog video" settings section: confirm-gated CRT toggle (writes the enable flag and exits 42 so Main respawns the frontend under the new mode), NTSC/PAL picker, and a screen-position calibration screen with a test pattern addressing true framebuffer pixels. - Central 5% safe-area inset on the scene in CRT mode; backgrounds and screensaver overscan to the true edge. - Fix the exit-1000 restart dropping --crt: execvp now reuses the unfiltered argv. Verified on hardware: NTSC picture, live offset nudges, exit-42 toggle round trip, and writer self-disable against a stale core.
PAL never took effect: the launcher set the framebuffer mode through vmode, whose fb_cmd path goes through Main's /dev/MiSTer_cmd loop and is not serviced while the alt launcher owns video - and Main hardcoded 352x240 anyway, re-asserting it ~1 s after spawn. - Write the fb mode directly via /sys/module/MiSTer_fb/parameters/mode in the CRT path (the mechanism Main itself uses), skipping the write when the geometry already matches. - Extend zaparoo_launcher_crt.bin to [enabled, mode] so Main programs the per-standard geometry on spawn and on its re-assert; new crt_mode_id() maps standards to DDR word1 ids. - Route video-standard changes through exit 42 (Main-owned respawn) on MiSTer instead of the in-process execvp restart; the desktop preview keeps the execvp restart. - Cover the CRT overscan band with Theme.scrim strips while a modal is open: modal scrims fill the safe-area-inset scene, so the full-bleed background previously glowed undimmed around every dialog. Pairs with Main_MiSTer a0c1bae (per-standard fb geometry + OSD CRT toggle). Verified on hardware: PAL 50 Hz picture, NTSC<->PAL round trips, modal scrims reach the framebuffer edge.
The CRT calibration modal had no helpEntries branch, so opening it fell through to the active-screen branch and advertised the wrong actions. Add a crtCalibrationModalVisible branch with Dpad: Adjust and ButtonA: Save, matching the modal's controls (arrows adjust H/V offset, accept/cancel commit and close). Regenerate translation catalogs for the new strings.
ad63174 to
3c8adb6
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
The crtEnabled field is classified as a toggle control, so keyboard/ gamepad accept enters the toggle block and returns early. That block had no crtEnabled branch, while the handling sat in fieldCommit's deferred path where toggles never reach it. Move the request into the toggle block so accept actually flips CRT mode.
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 (2)
src/ui/translations/frontend_ar.ts (1)
33-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the legal notice faithful to the source.
The Arabic text adds “or redistribution,” which changes the license terms beyond the English source. Please remove that extra restriction in both places.
Suggested fix
- <translation>المصدر متاح بموجب ترخيص PolyForm Noncommercial 1.0.0. مجاني للاستخدام الشخصي وغير التجاري. يتطلب الاستخدام التجاري أو إعادة التوزيع ترخيصًا منفصلًا.</translation> + <translation>المصدر متاح بموجب ترخيص PolyForm Noncommercial 1.0.0. مجاني للاستخدام الشخصي وغير التجاري. يتطلب الاستخدام التجاري ترخيصًا منفصلًا.</translation>Also applies to: 213-215
🤖 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 `@src/ui/translations/frontend_ar.ts` around lines 33 - 35, The Arabic translation in the PolyForm Noncommercial License message in frontend_ar.ts contains an extra phrase "أو إعادة التوزيع" (or redistribution) that does not exist in the English source text. This adds an additional restriction that is not present in the original license terms. Locate the Arabic translation entries for the PolyForm Noncommercial License notice (appearing around lines 33-35 and also at lines 213-215) and remove the phrase "أو إعادة التوزيع" from both occurrences in the translation values to keep them faithful to the English source text.src/ui/translations/frontend_ja.ts (1)
213-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the license notice aligned with the source text.
再配布adds a redistribution restriction that the English source doesn’t mention, so the Japanese notice now states stricter terms than intended.♻️ Proposed fix
- <translation>この無料のソース公開ビルドは個人・非商用利用専用です。商用利用または再配布にはライセンスが必要です。</translation> + <translation>この無料のソース公開ビルドは個人・非商用利用専用です。商用利用には別途ライセンスが必要です。</translation>🤖 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 `@src/ui/translations/frontend_ja.ts` around lines 213 - 214, The Japanese translation in the license notice at line 213-214 includes "または再配布" (or redistribution) which is not present in the corresponding English source text, creating an inconsistency where the Japanese version states stricter terms. Remove the phrase "または再配布" from the translation so that only the commercial use restriction remains, keeping the Japanese translation aligned with the English source text about what requires a license.
🤖 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.
Outside diff comments:
In `@src/ui/translations/frontend_ar.ts`:
- Around line 33-35: The Arabic translation in the PolyForm Noncommercial
License message in frontend_ar.ts contains an extra phrase "أو إعادة التوزيع"
(or redistribution) that does not exist in the English source text. This adds an
additional restriction that is not present in the original license terms. Locate
the Arabic translation entries for the PolyForm Noncommercial License notice
(appearing around lines 33-35 and also at lines 213-215) and remove the phrase
"أو إعادة التوزيع" from both occurrences in the translation values to keep them
faithful to the English source text.
In `@src/ui/translations/frontend_ja.ts`:
- Around line 213-214: The Japanese translation in the license notice at line
213-214 includes "または再配布" (or redistribution) which is not present in the
corresponding English source text, creating an inconsistency where the Japanese
version states stricter terms. Remove the phrase "または再配布" from the translation
so that only the commercial use restriction remains, keeping the Japanese
translation aligned with the English source text about what requires a license.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98af9cc1-3f6d-4996-9211-d3cbf5c10cb1
📒 Files selected for processing (32)
rust/frontend/build.rsrust/frontend/src/lib.rsrust/frontend/src/mister_runtime.rsrust/frontend/src/models/crt_video.rsrust/frontend/src/models/mod.rsrust/frontend/src/models/settings.rsrust/zaparoo-core/src/config.rsrust/zaparoo-core/src/persist.rssrc/app/main.cppsrc/app/native_video_writer.cppsrc/app/native_video_writer.hsrc/ui/app/Main.qmlsrc/ui/app/MainLayout.qmlsrc/ui/components/CMakeLists.txtsrc/ui/components/CrtCalibrationModal.qmlsrc/ui/screens/SettingsScreen.qmlsrc/ui/translations/frontend_ar.tssrc/ui/translations/frontend_de.tssrc/ui/translations/frontend_el.tssrc/ui/translations/frontend_en.tssrc/ui/translations/frontend_es.tssrc/ui/translations/frontend_eu.tssrc/ui/translations/frontend_he.tssrc/ui/translations/frontend_hi.tssrc/ui/translations/frontend_it.tssrc/ui/translations/frontend_ja.tssrc/ui/translations/frontend_ko.tssrc/ui/translations/frontend_nl.tssrc/ui/translations/frontend_ro.tssrc/ui/translations/frontend_sk.tssrc/ui/translations/frontend_uk.tssrc/ui/translations/frontend_zh_CN.ts
💤 Files with no reviewable changes (2)
- src/ui/translations/frontend_zh_CN.ts
- src/ui/translations/frontend_uk.ts
✅ Files skipped from review due to trivial changes (2)
- src/ui/components/CMakeLists.txt
- rust/frontend/build.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- rust/frontend/src/models/mod.rs
- src/ui/components/CrtCalibrationModal.qml
- rust/frontend/src/mister_runtime.rs
- rust/frontend/src/lib.rs
- rust/zaparoo-core/src/persist.rs
- rust/frontend/src/models/settings.rs
- rust/frontend/src/models/crt_video.rs
- src/app/main.cpp
- src/ui/app/MainLayout.qml
- src/app/native_video_writer.h
- src/app/native_video_writer.cpp
- src/ui/screens/SettingsScreen.qml
- src/ui/app/Main.qml
- rust/zaparoo-core/src/config.rs
39 commits since v1.1.1; 7 conflict files, all in the ArtCade-fork surface area: - rust/frontend/src/models/mod.rs (credits/dev_team module decls) - rust/frontend/src/models/settings.rs (5 hide_* qproperties + Initialize wiring) - rust/zaparoo-core/src/config.rs (5 hide_* SettingsConfig fields + RawSettings + settings_config_from_raw helper extension) - src/ui/app/Main.qml (_validStartupScreen → _isStableNavigationScreen helper + ImageOverrides Connections; Credits-family screens added to stable nav) - src/ui/app/MainLayout.qml (help-bar Quit gate + new categoryOptions ButtonX entry) - src/ui/screens/HubScreen.qml (actionEntries redesign: _hubCoverKey, enabled:true, Update tile, requestContextMenu; ArtCade hide_* gates re-applied; Credits tile preserved; Update tile also gated by hide_settings) - tests/ui/tst_navigation.qml (bounds-check the out-of-range cross-row test since action-row size now varies with hide_* + Update + Credits) ArtCade-fork patches verified intact post-merge: - 5 hide_* qproperties + RawSettings + Initialize wiring - HubScreen tile-level hide_* gates (Resume/Favorites/Recents/Settings) - HubScreen Credits tile (always visible — legal-compliance posture) - hide_exit cancel-handler gate + help-bar Quit gate - Credits/Artists/DevTeam/AboutEntries cxx-qt models registered - requestCreditsScreen signal + Main.qml routing Notable upstream features picked up: - ZaparooProject#249 Add hide controls for browse items (show_hidden, hidden_categories, hidden_system_ids) - ZaparooProject#259 launchable systems + Other category surfacing - ZaparooProject#260 hide category index/scrape actions when no indexable systems exist - ZaparooProject#258 user customization for system art, hub icons, names (ImageOverrides model) - ZaparooProject#271 zaparoo-update integration - ZaparooProject#225 Native CRT video v2 settings (crt_video_standard, crt_h_offset, crt_v_offset) - ZaparooProject#251 localized system names + region setting Safety net: tag `artcade-pre-v1.2.1-rebase` pins pre-merge HEAD (1ba850b).
Summary
Adopts the Menu_MiSTer fork's v2 native video contract and moves CRT ownership into the launcher, now that Main_MiSTer has dropped its OSD video pages and
status[9]CRT bit. Pairs with the Menu fork branchfix/native-video-centeringand the corresponding Main_MiSTer changes - all three must be deployed together for the CRT path (HDMI is unaffected; mismatched combos fail obviously, by design).Writer (DDR contract v2)
0x3A000000; word0(frame_counter << 2) | active_buffer, word1magic 0x5A50 / h_offset / v_offset / mode; buffers at+0x1000/+0x180000.static_asserts.Settings + calibration
config/zaparoo_launcher_crt.binand exits with code 42 so Main respawns the frontend under the new mode.Browse.CrtVideosingleton owns standard + offsets, persisted via state.toml with the frontend.toml mirror.Safe area
sceneshrinks 5% per side in CRT mode only; Sizing bindings propagate it everywhere with no per-screen changes. Background and screensaver overscan to the true edge.Fixes
--crton any restart-applied setting change in CRT mode. Restart now reuses the unfiltered argv.Deliberately not in this PR
crt_video_standard = "480i"accepted in frontend.toml for smoke tests) but not offered in the picker - the UI flicker-discipline pass for interlace comes separately.zaparoo_video_offsets.binvalues; offsets start at 0.Testing
just lint,just test(432 tests),just arm32all green; 16 translation catalogs refreshed.menu_zaparoo.rbfinto the FPGA at its own startup - reboot after copying a new core or you'll see the noise pattern.This PR stays in draft while testers run it; it will be rebased onto main periodically.
Summary by CodeRabbit
Release Notes
--crton restart, and refine CRT safe-area/overscan rendering.