v1.23.0 "Cadence" — pin-exact master-clock-stepped NEC DSP - #254
Conversation
Fractional accumulator (dsp_accum) on Upd77c25 + Revision::rates() (7725=760000/ 2147727, 96050=1100000/2147727) + tick_master(); Dsp1Board drives it from coprocessor_tick. read_dr/write_dr no longer run_until_rqm synchronously; a transient instrumented hybrid net in read_dr catches up only when RQM is unset. Serialize dsp_accum (FORMAT_VERSION 9->10). In-game validation: Pilotwings flight fires the hybrid ZERO times, floor_variety=55 (non-degenerate). WIP checkpoint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase B: remove the transient hybrid net; read_dr/write_dr are pure (return current DR/SR, DSP advances only via tick_master). Remove hybrid_fires field + plumbing. dsp1_raster_ramp drives the DSP with run_until_rqm (no bus in the standalone). Pure in-game Pilotwings flight hash = 0x5dba62c02af9a44a, IDENTICAL to the v1.22.0 baseline; floor_variety=55. Phase C: NecDspVariantBoard overrides coprocessor_tick (DSP-2/4 7.6MHz, ST010 11MHz auto via Revision). WIP checkpoint (variant before/after validation next). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bring the spec in line with the implemented free-running model (docs-as-spec, same change as the code it describes): - upd77c25.rs module doc "Host synchronization model": rewrite from the old catch-up-on-host-access framing to the master-clock-stepped free-runner (tick_master on Revision::rates via coprocessor_tick; run_until_rqm now only the firmware-load prime + standalone tests). - docs/cart.md "the shared NEC core": same rewrite, plus the GSU cross-reference that pointed at DSP-1's run_until_rqm as "the same economy" — the GSU stays run-to-completion (nothing observable between Go-set and Go-clear), the DSP is now free-running because its RQM handshake is polled mid-run. - CHANGELOG [Unreleased]: the pin-exact rewrite entry + FORMAT_VERSION 9->10. No code change; rustdoc -D warnings + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
diag_dsp_pinexact.rs and diag_necdsp_variants.rs were session-only validation scaffolds (both self-labeled "THROWAWAY ... NOT to be committed") that hardcode corpus-specific framebuffer hashes and require gitignored commercial ROMs; they were accidentally captured in the Phase B+C WIP commit. The committed regression for this work is dsp1_raster_ramp.rs (permissive, self-skipping). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d NEC DSP Release-prep for v1.23.0, the follow-up v1.22.0 scoped out: the shared NEC-DSP host interface (µPD77C25 / µPD96050 — DSP-1/2/4, ST010) now free-runs on the master-clock scheduler instead of resolving its host handshake in zero emulated time. Model-consistency + response-latency change; NOT a rendering change (all four wired NEC chips are byte-identical to the v1.22.0 baseline). Version: - Bump all 17 workspace package versions 1.22.0 -> 1.23.0 (root [workspace.package] + every crate). Internal path-dependency floor pins (version = "1.4.0") are left untouched — they are decoupled from the marching package version by design. - Refresh Cargo.lock to match. Docs (docs-as-spec, same change as the behaviour it describes — the code + module docs already landed in the preceding pin-exact commits on this branch): - CHANGELOG.md: close [Unreleased] as [1.23.0] "Cadence" - 2026-07-29 with a narrative intro + Changed (tick_master / Revision::rates / dsp_accum / pure read_dr/write_dr; FORMAT_VERSION 9->10) + Internal (host-sync spec + GSU cross-reference correction) sections; open a fresh empty [Unreleased]. - README.md: version badge -> v1.23.0. - docs/STATUS.md: current release -> v1.23.0 "Cadence" (v1.22.0 "Horizon" demoted into the ladder); new pin-exact narrative + FORMAT_VERSION 9->10 note. - to-dos/VERSION-PLAN.md: new "v1.23.0 Cadence" RELEASED entry (what/why-safe/ save-state/verification) + back-reference from the v1.22.0 "not included" note. .gitignore: reviewed, no change needed (.codegraph/, tests/captures/, salvaged/, iOS-generated bindings, AI-local files already covered; working tree clean). Gates green: cargo check/build (Cargo.lock refreshed), fmt, clippy (workspace default + per-feature), cargo test --workspace 527/527, no_std thumbv7em-none-eabihf, RUSTDOCFLAGS=-D warnings doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (6)docs/**/*.md📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{rs,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
docs/**⚙️ CodeRabbit configuration file
Files:
**/*.md⚙️ CodeRabbit configuration file
Files:
CHANGELOG.md📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🔇 Additional comments (4)
WalkthroughThe NEC DSP host interface now advances on master-clock ticks instead of host-access catch-up execution. Its fractional clock phase is serialized, save-state format version 10 is required, tests and specifications reflect the new timing model, and the workspace release is updated to 1.23.0. ChangesNEC DSP Cadence timing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MasterScheduler
participant NecDspVariantBoard
participant Upd77c25
participant Host
MasterScheduler->>NecDspVariantBoard: coprocessor_tick()
NecDspVariantBoard->>Upd77c25: tick_master()
Upd77c25->>Upd77c25: advance DSP and RQM state
Host->>Upd77c25: read_dr() or write_dr()
Upd77c25-->>Host: current DR/SR values
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
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: 4
🤖 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 `@CHANGELOG.md`:
- Around line 10-12: Move the user-visible DSP timing and save-state
compatibility notes from the 1.23.0 section into the currently empty
[Unreleased] section in CHANGELOG.md, leaving the 1.23.0 release notes unchanged
and preserving the notes until the release-cut commit.
In `@crates/rustysnes-cart/src/coproc/upd77c25.rs`:
- Line 874: Validate the value read into self.dsp_accum in the save-state
deserialization path before assigning or accepting it: reject phases greater
than or equal to the divisor den with SaveStateError::Invalid, while preserving
valid values below den. Use the existing typed save-state error mechanism and
ensure malformed NDSP payloads cannot reach tick_master().
In `@crates/rustysnes-test-harness/tests/dsp1_raster_ramp.rs`:
- Around line 40-58: Extend the DSP-1 timing regression beyond the direct
Upd77c25 harness by adding a System/Board-level test that issues the same
Pilotwings host-command sequence through the bus. Drive execution via the
master-clock stepping path (Board::coprocessor_tick/tick_master) rather than
direct run_until_rqm calls, and retain the existing expected DSP results.
In `@docs/STATUS.md`:
- Around line 35-39: Correct the release documentation’s scheduling comparisons:
in docs/STATUS.md lines 35-39, remove GSU from the master-clock comparison; in
CHANGELOG.md lines 14-20, replace the “last synchronous coprocessor” wording and
remove GSU from the free-running list; in CHANGELOG.md lines 32-37 and
to-dos/VERSION-PLAN.md lines 1451-1459, compare the NEC DSP only with SPC700 and
ST018, preserving GSU’s run-to-completion distinction.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 8435c7be-139f-4594-bf03-70534880fc64
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included byCargo.lock
📒 Files selected for processing (27)
CHANGELOG.mdCargo.tomlREADME.mdcrates/rustysnes-android/Cargo.tomlcrates/rustysnes-apu/Cargo.tomlcrates/rustysnes-cart/Cargo.tomlcrates/rustysnes-cart/src/coproc/dsp1.rscrates/rustysnes-cart/src/coproc/necdsp_variant.rscrates/rustysnes-cart/src/coproc/upd77c25.rscrates/rustysnes-cheevos/Cargo.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-core/src/scheduler.rscrates/rustysnes-cpu/Cargo.tomlcrates/rustysnes-frontend/Cargo.tomlcrates/rustysnes-gfx-shaders/Cargo.tomlcrates/rustysnes-ios/Cargo.tomlcrates/rustysnes-mobile/Cargo.tomlcrates/rustysnes-monetization/Cargo.tomlcrates/rustysnes-netplay/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-savestate/Cargo.tomlcrates/rustysnes-script/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlcrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rsdocs/STATUS.mddocs/cart.mdto-dos/VERSION-PLAN.md
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: accuracysnes
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: build demo + docs
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (16)
**/Cargo.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Do not configure the
scriptingandscript-wasmbackends as simultaneously enabled features; they are mutually exclusive.
Files:
crates/rustysnes-cheevos/Cargo.tomlcrates/rustysnes-gfx-shaders/Cargo.tomlcrates/rustysnes-monetization/Cargo.tomlcrates/rustysnes-savestate/Cargo.tomlcrates/rustysnes-script/Cargo.tomlcrates/rustysnes-apu/Cargo.tomlcrates/rustysnes-netplay/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlCargo.tomlcrates/rustysnes-cart/Cargo.tomlcrates/rustysnes-ios/Cargo.tomlcrates/rustysnes-cpu/Cargo.tomlcrates/rustysnes-frontend/Cargo.tomlcrates/rustysnes-mobile/Cargo.tomlcrates/rustysnes-android/Cargo.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.toml
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
crates/rustysnes-cheevos/Cargo.tomlcrates/rustysnes-gfx-shaders/Cargo.tomlcrates/rustysnes-monetization/Cargo.tomlcrates/rustysnes-savestate/Cargo.tomlcrates/rustysnes-script/Cargo.tomlcrates/rustysnes-apu/Cargo.tomlcrates/rustysnes-netplay/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlCargo.tomlcrates/rustysnes-cart/Cargo.tomlcrates/rustysnes-ios/Cargo.tomlcrates/rustysnes-cpu/Cargo.tomlcrates/rustysnes-frontend/Cargo.tomlcrates/rustysnes-mobile/Cargo.tomlcrates/rustysnes-android/Cargo.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlREADME.mdcrates/rustysnes-core/src/scheduler.rsCHANGELOG.mdcrates/rustysnes-cart/src/coproc/necdsp_variant.rsdocs/STATUS.mdcrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rsdocs/cart.mdcrates/rustysnes-cart/src/coproc/dsp1.rsto-dos/VERSION-PLAN.mdcrates/rustysnes-cart/src/coproc/upd77c25.rs
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-cheevos/Cargo.tomlcrates/rustysnes-gfx-shaders/Cargo.tomlcrates/rustysnes-monetization/Cargo.tomlcrates/rustysnes-savestate/Cargo.tomlcrates/rustysnes-script/Cargo.tomlcrates/rustysnes-apu/Cargo.tomlcrates/rustysnes-netplay/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlcrates/rustysnes-cart/Cargo.tomlcrates/rustysnes-ios/Cargo.tomlcrates/rustysnes-cpu/Cargo.tomlcrates/rustysnes-frontend/Cargo.tomlcrates/rustysnes-mobile/Cargo.tomlcrates/rustysnes-android/Cargo.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-core/src/scheduler.rscrates/rustysnes-cart/src/coproc/necdsp_variant.rscrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rscrates/rustysnes-cart/src/coproc/dsp1.rscrates/rustysnes-cart/src/coproc/upd77c25.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Additive features must be default-off so shipped/native,no_std, and wasm builds remain byte-identical.
Never use or configure--all-features; validate opt-in feature combinations individually as required by the project recipe.
Files:
crates/rustysnes-cheevos/Cargo.tomlcrates/rustysnes-gfx-shaders/Cargo.tomlcrates/rustysnes-monetization/Cargo.tomlcrates/rustysnes-savestate/Cargo.tomlcrates/rustysnes-script/Cargo.tomlcrates/rustysnes-apu/Cargo.tomlcrates/rustysnes-netplay/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlCargo.tomlcrates/rustysnes-cart/Cargo.tomlcrates/rustysnes-ios/Cargo.tomlcrates/rustysnes-cpu/Cargo.tomlcrates/rustysnes-frontend/Cargo.tomlcrates/rustysnes-mobile/Cargo.tomlcrates/rustysnes-android/Cargo.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-core/src/scheduler.rscrates/rustysnes-cart/src/coproc/necdsp_variant.rscrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rscrates/rustysnes-cart/src/coproc/dsp1.rscrates/rustysnes-cart/src/coproc/upd77c25.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-cheevos/Cargo.tomlcrates/rustysnes-gfx-shaders/Cargo.tomlcrates/rustysnes-monetization/Cargo.tomlcrates/rustysnes-savestate/Cargo.tomlcrates/rustysnes-script/Cargo.tomlcrates/rustysnes-apu/Cargo.tomlcrates/rustysnes-netplay/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlcrates/rustysnes-cart/Cargo.tomlcrates/rustysnes-ios/Cargo.tomlcrates/rustysnes-cpu/Cargo.tomlcrates/rustysnes-frontend/Cargo.tomlcrates/rustysnes-mobile/Cargo.tomlcrates/rustysnes-android/Cargo.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-core/src/scheduler.rscrates/rustysnes-cart/src/coproc/necdsp_variant.rscrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rscrates/rustysnes-cart/src/coproc/dsp1.rscrates/rustysnes-cart/src/coproc/upd77c25.rs
crates/*/Cargo.toml
⚙️ CodeRabbit configuration file
crates/*/Cargo.toml: The frontend pins one compatible GUI tier (currently winit 0.30 / wgpu 29 / egui 0.35 and
itsegui-wgpu/egui-winitsiblings). Flag a bump to any one of those that is not
accompanied by matching bumps to the others. Optional features are default-off by
convention so shipped, no_std and wasm builds stay byte-identical; flag a new feature added
to adefault = [...]list.
Files:
crates/rustysnes-cheevos/Cargo.tomlcrates/rustysnes-gfx-shaders/Cargo.tomlcrates/rustysnes-monetization/Cargo.tomlcrates/rustysnes-savestate/Cargo.tomlcrates/rustysnes-script/Cargo.tomlcrates/rustysnes-apu/Cargo.tomlcrates/rustysnes-netplay/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlcrates/rustysnes-cart/Cargo.tomlcrates/rustysnes-ios/Cargo.tomlcrates/rustysnes-cpu/Cargo.tomlcrates/rustysnes-frontend/Cargo.tomlcrates/rustysnes-mobile/Cargo.tomlcrates/rustysnes-android/Cargo.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.toml
Cargo.toml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Cargo.toml: Keep crate dependencies one-directional; chip crates must not depend on each other.
Additive features must remain default-off so native, no_std, and wasm outputs remain byte-identical in default configurations.
Do not usecargo clippy --all-features; usecargo clippy --workspace --all-targets -- -D warnings, with explicit feature jobs when needed.
Files:
Cargo.toml
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
README.mdcrates/rustysnes-core/src/scheduler.rsCHANGELOG.mdcrates/rustysnes-cart/src/coproc/necdsp_variant.rsdocs/STATUS.mdcrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rsdocs/cart.mdcrates/rustysnes-cart/src/coproc/dsp1.rsto-dos/VERSION-PLAN.mdcrates/rustysnes-cart/src/coproc/upd77c25.rs
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
README.mdCHANGELOG.mddocs/STATUS.mddocs/cart.mdto-dos/VERSION-PLAN.md
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-core/src/scheduler.rscrates/rustysnes-cart/src/coproc/necdsp_variant.rscrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rscrates/rustysnes-cart/src/coproc/dsp1.rscrates/rustysnes-cart/src/coproc/upd77c25.rs
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Files:
crates/rustysnes-core/src/scheduler.rscrates/rustysnes-cart/src/coproc/necdsp_variant.rscrates/rustysnes-cart/src/coproc/dsp1.rscrates/rustysnes-cart/src/coproc/upd77c25.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI runs with-D warnings. Document every public item.
Keepunsafecode restricted to the frontend and FFI, and include a// SAFETY:justification for each use.
Keep hot paths allocation-free.
Treatrustysnes_core::Busas the owner of mutable emulator state; the CPU borrows&mut Bus.
Use the master clock at 21477270 Hz as the timing master; advance the scheduler in lockstep and run other chips on their divisors.
Maintain determinism: seed, ROM, and input must produce bit-identical audio/video; frontend rate control must not alter emulation results.
When implementing hardware behavior, pin and run the failing test ROM first; treat test ROMs as the specification.
Files:
crates/rustysnes-core/src/scheduler.rscrates/rustysnes-cart/src/coproc/necdsp_variant.rscrates/rustysnes-test-harness/tests/dsp1_raster_ramp.rscrates/rustysnes-cart/src/coproc/dsp1.rscrates/rustysnes-cart/src/coproc/upd77c25.rs
crates/rustysnes-core/**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
The core crate must compile for
thumbv7em-none-eabihfwith--no-default-features; the chip stack must remainno_std.
Files:
crates/rustysnes-core/src/scheduler.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/STATUS.mddocs/cart.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/STATUS.mddocs/cart.md
🔇 Additional comments (27)
crates/rustysnes-cart/src/coproc/upd77c25.rs (1)
15-33: LGTM!Also applies to: 104-127, 236-239, 281-281, 308-308, 364-411, 471-488, 829-829, 1036-1050, 1087-1087
crates/rustysnes-cart/src/coproc/dsp1.rs (1)
168-172: LGTM!crates/rustysnes-cart/src/coproc/necdsp_variant.rs (1)
242-247: LGTM!crates/rustysnes-core/src/scheduler.rs (1)
72-79: LGTM!docs/cart.md (1)
102-124: LGTM!Also applies to: 150-153
crates/rustysnes-monetization/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-netplay/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-ppu/Cargo.toml (1)
3-3: LGTM!crates/rustysnes-savestate/Cargo.toml (1)
3-3: LGTM!crates/rustysnes-script/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-test-harness/Cargo.toml (1)
3-3: LGTM!README.md (1)
17-17: LGTM!CHANGELOG.md (1)
48-57: LGTM!docs/STATUS.md (2)
6-6: LGTM!
40-43: LGTM!to-dos/VERSION-PLAN.md (1)
1441-1441: LGTM!Also applies to: 1443-1450, 1462-1476
Cargo.toml (1)
25-25: LGTM!crates/rustysnes-android/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-apu/Cargo.toml (1)
3-3: LGTM!crates/rustysnes-cart/Cargo.toml (1)
3-3: LGTM!crates/rustysnes-cheevos/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-core/Cargo.toml (1)
3-3: LGTM!crates/rustysnes-cpu/Cargo.toml (1)
3-3: LGTM!crates/rustysnes-frontend/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-gfx-shaders/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-ios/Cargo.toml (1)
4-4: LGTM!crates/rustysnes-mobile/Cargo.toml (1)
4-4: LGTM!
| ## [Unreleased] | ||
|
|
||
| ## [1.23.0] "Cadence" - 2026-07-29 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record these user-visible notes under [Unreleased].
The new DSP timing and save-state compatibility behavior is inserted directly under ## [1.23.0] while [Unreleased] remains empty. Move the notes under [Unreleased] until the release-cut commit.
As per coding guidelines, user-visible changes must be recorded under the [Unreleased] section.
🤖 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 `@CHANGELOG.md` around lines 10 - 12, Move the user-visible DSP timing and
save-state compatibility notes from the 1.23.0 section into the currently empty
[Unreleased] section in CHANGELOG.md, leaving the 1.23.0 release notes unchanged
and preserving the notes until the release-cut commit.
Source: Coding guidelines
| // Host command byte (8-bit while parked) then 16-bit LE parameter words. In the pin-exact model | ||
| // read_dr/write_dr no longer advance the DSP (only the master-clock tick does), so this standalone | ||
| // harness — which has no bus — drives the chip explicitly with `run_until_rqm` after each host op, | ||
| // exactly the catch-up the Bus performs continuously in-game. | ||
| let cmd = |dsp: &mut Upd77c25, c: u8, params: &[u16]| { | ||
| dsp.write_dr(c); | ||
| dsp.run_until_rqm(); | ||
| for &p in params { | ||
| dsp.write_dr((p & 0xff) as u8); | ||
| dsp.run_until_rqm(); | ||
| dsp.write_dr((p >> 8) as u8); | ||
| dsp.run_until_rqm(); | ||
| } | ||
| }; | ||
| let read_word = |dsp: &mut Upd77c25| -> u16 { | ||
| let lo = dsp.read_dr(); | ||
| dsp.run_until_rqm(); | ||
| let hi = dsp.read_dr(); | ||
| dsp.run_until_rqm(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '\b(tick_master|coprocessor_tick|dsp_accum)\b' \
crates/rustysnes-test-harness crates/rustysnes-cart crates/rustysnes-coreRepository: doublegate/RustySNES
Length of output: 6531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== test file ==\n'
cat -n crates/rustysnes-test-harness/tests/dsp1_raster_ramp.rs
printf '\n== nearby DSP1 tests ==\n'
rg -n --context 2 'dsp1_raster_ramp|run_until_rqm|tick_master|coprocessor_tick|raster' crates/rustysnes-test-harness/tests crates/rustysnes-core crates/rustysnes-cart
printf '\n== scheduler / board docs around NDSP ==\n'
sed -n '60,95p' crates/rustysnes-core/src/scheduler.rs
sed -n '360,520p' crates/rustysnes-cart/src/coproc/upd77c25.rs
sed -n '1,120p' crates/rustysnes-cart/src/coproc/dsp1.rsRepository: doublegate/RustySNES
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --context 3 'run_until_rqm' cratesRepository: doublegate/RustySNES
Length of output: 10093
Add a bus-level DSP-1 timing regression. This test only drives Upd77c25 directly through run_until_rqm; it never exercises the master-clock path that calls Board::coprocessor_tick/tick_master, so a broken scheduler or clock ratio would still pass. Add the same Pilotwings host-command sequence to a System/Board test that steps the bus.
🤖 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 `@crates/rustysnes-test-harness/tests/dsp1_raster_ramp.rs` around lines 40 -
58, Extend the DSP-1 timing regression beyond the direct Upd77c25 harness by
adding a System/Board-level test that issues the same Pilotwings host-command
sequence through the bus. Drive execution via the master-clock stepping path
(Board::coprocessor_tick/tick_master) rather than direct run_until_rqm calls,
and retain the existing expected DSP results.
Adjudicated bot-review findings on the pin-exact save-state path (PR #254): - CodeRabbit (Major, accepted): `load_state` accepted any `u64` for the master-clock accumulator phase (`dsp_accum`). A crafted `NDSP` payload with a phase >= the divisor would make the next `tick_master` execute up to billions of `exec()` iterations before the accumulator drained — uncontrolled resource consumption on untrusted save-state input (module 60). A valid phase is always reduced below `den`, so reject `>= den` with the typed `SaveStateError::Invalid` at the boundary. Adds `out_of_range_accumulator_phase_is_rejected_not_spun_on` (rejects `den`/`den+1`/`u64::MAX`, accepts `den-1`). - Antigravity (suggestion + nitpick, accepted): evaluate `Revision::rates()` (a `const fn`) directly inside the reduction's `const _` assertion instead of re-stating the reduced literals, so the proof tracks the source; this also drops the `clippy::eq_op` allow. Verified: fmt, clippy -p rustysnes-cart -D warnings, 157 cart unit tests, and the RUSTDOCFLAGS=-D warnings doc build all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bot-review adjudication (commit b3773e0)Thanks both. Decisions: Accepted — fixed in
|
Adjudicated CodeRabbit finding (PR #254): the release docs described the NEC DSP as free-running "like the SPC700, GSU, and ST018", but the GSU's scheduling model is described inconsistently within the codebase itself — gsu.rs's core doc frames it as run-to-completion (`Gsu::run_until_stopped`, no per-master-clock hook) while superfx.rs's board doc frames it as master-clock-stepped (`coprocessor_tick` -> `Gsu::tick`, one master clock at a time). Resolving that pre-existing GSU-doc contradiction is out of scope for a DSP release, so the release docs no longer take a side: the NEC DSP is now compared only to the SPC700 and ST018 ARMv3, both of which are unambiguously master-clock-stepped free-runners. - CHANGELOG.md / docs/STATUS.md / to-dos/VERSION-PLAN.md: remove GSU/Super FX from the "free-run on the master clock" comparison; simplify the Internal note to say the stale DSP-1 `run_until_rqm` cross-reference was retired (not that the GSU was re-characterized). - docs/cart.md: the GSU host-sync paragraph keeps its own (pre-existing) run-to-completion description and simply drops the stale `run_until_rqm` economy cross-reference, pointing at "the shared NEC core" for the DSP's new model. Docs-only; no code change. (Follow-up noted separately: reconcile the gsu.rs vs superfx.rs GSU scheduling-model docs.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up (commit 4b3b0f5) — GSU scheduling comparisonCodeRabbit's re-review flagged that the release docs compared the NEC DSP to "SPC700, GSU, and ST018" as master-clock-stepped free-runners. Good catch — but the fix goes a step further than the literal suggestion, because the GSU's own model is documented inconsistently in the codebase:
Both I've noted the The three earlier re-posted threads (CHANGELOG |
Antigravity review (Gemini via Ultra)This PR refactors the NEC DSP coprocessor core (µPD77C25 / µPD96050) to free-run on a master-clock-stepped fractional divisor ( Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
2 participants
v1.23.0 "Cadence" — pin-exact master-clock-stepped NEC DSP
The follow-up v1.22.0 "Horizon" scoped out. v1.22.0 fixed the visible DSP-1 Mode-7 flat floor value-exact; this PR rewrites the shared NEC-DSP host interface so the DSP joins every other timed subsystem on the master clock instead of resolving its host handshake in zero emulated time. It is a model-consistency + response-latency change, deliberately not a rendering change.
What changed
The µPD77C25 / µPD96050 engine (DSP-1/2/4, ST010; DSP-3/ST011 decoded but unwired) was the last timed subsystem still drained synchronously inside
Upd77c25::read_dr/write_dr(a lazy catch-up on host access model). It now free-runs on the master-clock scheduler like the SPC700, GSU/Super FX, and ST018 ARMv3:Upd77c25::tick_master, driven once per master clock from each NEC-DSP board'sBoard::coprocessor_tick, advances the DSP on its own gcd-reduced fractional divisor (Revision::rates):760_000 / 2_147_727(7.6 MHz / master)1_100_000 / 2_147_727(11 MHz / master)dsp_accum: u64) the SPC700 uses — no floats, fully deterministic (docs/adr/0004), guarded by a compile-time reduction assertion.read_dr/write_drare now pure — they exchange the currentDR/SR; the DSP has already produced the value on its own clock.run_until_rqmsurvives only for the one-time firmware-load prime and standalone tests.Why it is safe (not a picture change)
All four wired NEC chips render byte-identical to the v1.22.0 baseline (before/after framebuffer content-hash):
0x5dba62c02af9a44a, unchangedA continuously-clocked free-runner cannot "stop at the first
RQM=set", so the hazard class behind the v1.22.0 Mode-7 flat floor is eliminated by construction, not by special-casing.Save-state
The DSP sub-clock phase (
dsp_accum) is serialized in theNDSPsection →FORMAT_VERSION 9 → 10; older blobs loud-fail (docs/adr/0006, no silent migration). Battery.srmsaves are unaffected.Commits
b5cd929Phase A — accumulator infra + DSP-1 + transient hybrid netcc2371dPhase B+C — go pure (remove net) + DSP-2/4/ST010e839742docs — master-clock-stepped model inupd77c25.rs+docs/cart.md(+ GSU cross-ref fix)0e631d8drop throwaway pin-exact diagnostic harnessesbdc631brelease-prep — bump 17 crates to 1.23.0, CHANGELOG/README/STATUS/VERSION-PLANChecks run locally
cargo test --workspace— 527 passed / 0 failedcargo fmt --check;cargo clippy --workspace --all-targets -- -D warnings(default + per-feature; never--all-features)cargo build -p rustysnes-core --target thumbv7em-none-eabihf --no-default-features(no_std)RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depsRelease
On squash-merge to
main,release-auto.ymltagsv1.23.0(CHANGELOG[1.23.0]is closed out with an empty[Unreleased]above it) and builds the cross-platform binaries; the release will be enriched with standalonev1.23.0 "Cadence"notes.🤖 Generated with Claude Code
Summary
Changes the NEC shared DSP (µPD77C25/µPD96050) emulation from a host-access “catch-up”/zero-emulation-time handshake model to a deterministic master-clock-stepped, pin-exact free-run model. Observable behavior changes: host
read_dr/write_drno longer synchronously advances the DSP during the access; instead they exchange the DSP’s current DR/SR bytes and update SR transfer state, while RQM/DRC/DRS handshake progression advances only when the scheduler drivesUpd77c25::tick_master(with revision-specific clock-rate stepping). (DSP-3/ST011 is still decoded but remains unwired.)The PR claims:
tick_master-driven handshake scheduling is pin-exact and the resulting rendering is byte-identical to the v1.22.0 baseline.dsp_accum) in theNDSPsave-state section.dsp_accumis out of range—specificallydsp_accum >= den—withSaveStateError::Invalid. Save-state format handling is intentionally incompatible: format version is bumped 9 → 10 and older blobs are expected to fail explicitly (battery saves unaffected).What would make the claims false:
tick_master+ no synchronous DSP run duringread_dr/write_dr” contract yields different observable RQM/DR handshake evolution than expected hardware (or than the v1.22.0-referenced behavior) for DSP-1/DSP-2/DSP-4/ST010.dsp_accum >= den, or incorrectly rejecting valid boundary values).Coverage/verification tied to the above claims:
dsp_accumserialization and the invalid/valid boundary behavior (including the>= denrejection case).tick_master/RQM sequencing.