diff --git a/src/KNOWN_ISSUES.md b/src/KNOWN_ISSUES.md new file mode 100644 index 0000000..dd14661 --- /dev/null +++ b/src/KNOWN_ISSUES.md @@ -0,0 +1,127 @@ +# Known Issues + +Issues found during the 7-agent code review that are deferred (not +fixed in the current build) because they require a more substantial +refactor or only matter on real hardware. + +## 1. LZO decompression runs inside the raster IRQ (Review 4) + +**Symptom:** On every state transition that loads an LZO-compressed +screen (TITLE, READY→WAIT, WAIT→DRAW is white, DRAW→WIN, WIN→READY, +GAMEOVER), the user sees a ~170 ms screen freeze with partial-update +tearing. About 8-9 frames worth of raster time. + +**Cause:** `show_screen()` (in `screens.c`) calls +`oscar_expand_lzo((char *)0xE000, …)` for 8 000 output bytes, which +takes ~170 000 cycles at 1 MHz. The raster IRQ handler at line 311 +(`src/tick.c:78-82`) calls `game_step()` which dispatches to +`game_enter_X()` which calls `show_screen()`. The VIC is mid-scanout +and the bitmap is being overwritten live. + +**Workarounds considered:** +- Pre-decompress during the long READY/WAIT dwells (would require + predicting the next state) +- Atomic memcpy in VBlank (8000 bytes at 1 byte/9 cycles = 72 000 + cycles; doesn't fit in 1575-cycle VBlank) +- Pending-flag pattern with main loop (would introduce a 1-frame + visual lag between state change and screen update) +- Move the entire state machine to main loop, keep only frame_count + in IRQ (loses audio cue precision) + +**Decision:** Accepted for now. The visible tearing is annoying but +not a playability blocker; the game still works correctly. The +deferred-swap refactor is a self-contained follow-up commit. + +## 2. IRQ half of `mmap_trampoline()` is silently disabled (Review 1) + +**Symptom:** The mmap_trampoline writes `IRQTrampoline` to +`$FFFE/$FFFF` and `NMITrampoline` to `$FFFA/$FFFB`. But +`rirq_init(false)` in `src/tick.c:100` overwrites `$FFFE/$FFFF` +with `rirq_isr_ram_io` (per `oscar64/include/c64/rasterirq.c:572-578`). +So the IRQ half of the trampoline is dead code; only the NMI half +remains. + +**Why it works anyway:** `src/tick.c:92-95` masks both CIAs +(`cia1.icr = 0x7f; cia2.icr = 0x7f;`), so the only IRQ source is +the raster. `rirq_isr_ram_io` correctly acks the raster and runs +`frame_tick_handler`. The trampoline is never needed. + +**Latent risk:** If anyone ever un-masks a CIA source (e.g. to use +CIA 1 Timer A for the jiffy clock), a stray CIA IRQ will land in +`rirq_isr_ram_io` which only acks `$D019` and returns — it will +spin re-entering on the next CIA edge. The fix would be to +re-install the trampoline after `rirq_init()`. + +**Decision:** Documented. No code change. + +## 3. Audio schedule is 1 frame short per note (Reviews 2, audio review) + +**Symptom:** WIN cue plays 4 notes × 9 frames = 36 frames = 0.72 s +(spec says 4 × 10 = 0.80 s). GAMEOVER plays 5 notes × 29 frames = +2.90 s (spec says 5 × 30 = 3.00 s). About 10% shorter per note. + +**Cause:** `audio_state_step()` does `audio_step++` at the *start* of +the function, so the first call to step (in the same IRQ as the state +enter) has `audio_step == 1`. The schedule uses `if (audio_step == +10) …` to trigger the next note, which fires on the 10th call (frame +11 of the state), not the 10th frame. + +**Verified unaffected:** READY (8 frames per note — the schedule +checks `== 8` and `== 16` which line up correctly with 8+8=16 +total), DRAW noise (4 frames — checks `>= 4` which also lines up +correctly), WAIT retrigger cadence (the `mod 16` checks produce the +right 16-frame cycles). + +**Decision:** Cosmetic. Not worth the schedule risk of changing +`audio_step++` to the end of the function (would need to verify +all 5 state schedules still work). + +## 4. `docs/c64/vic/graphics_modes.md` has wrong color mapping (Review 7) + +**Symptom:** The local doc (line 52) says multicolor bitmap mode +"01" pixels use `$D022` and "10" pixels use `$D023`. The C64 +hardware (per cebix-vic-article §3.7.3.4) actually uses the high / +low nibble of the cell's screen memory byte for "01" / "10" +respectively. `$D022` and `$D023` are not used at all in multicolor +bitmap mode. + +**Impact:** None on runtime — the code in `src/screens.c` and the +Python in `tools/convert_screens.py` both follow the correct +hardware mapping. Only future developers reading the local doc +would be misled. The same doc also has the wrong mapping for +multicolor CHARACTER mode on line 40 (not used by this project). + +**Decision:** Doc-only fix. Deferred to a doc-cleanup commit. + +## 5. First-round WAIT was always 100 frames (Reviews 2, fixed) + +Fixed in the same review-driven commit: `audio_init()` now sets +`sid.voices[2].freq = 0xffff` after `audio_stop()` so the SID +oscillator-3 (used as a random number source via `$D41B`) is +running from the very first sample. + +## 6. DRAW state "instant win" on held fire (Reviews 2, 5, fixed) + +Fixed in the same review-driven commit: `game_enter_draw()` now +initializes `draw_was_pressed[]` from the current input state +(via `input_fire()`), not from zero, so a player holding fire +from WAIT into DRAW does NOT auto-win on the first frame. + +## 7. `.attr` tables were in BASIC ROM space (Review 1, fixed) + +Fixed in the same review-driven commit: the `screens` region moved +from `$A000-$C000` (overlapping the banked-in BASIC ROM) to +`$BC00-$D000` (always-RAM under the banked-out KERNAL and below +the I/O area). + +## 8. Build script bugs (Review 6, fixed) + +Fixed in the same review-driven commit: +- `--kill` pgrep regex was broken (unescaped `+`, missing group). +- `-v -p` (or `-V -P`) would launch two VICE processes — now + rejected with a clear error. +- `c1541` exits 0 even on failure, so `set -e` couldn't catch + a failed `.d64` build — now verified with a `c1541 -list` check. +- `--kill` now also removes the stale `vice.pid` and `vice.log`. +- `-p` / `-P` now kill any prior VICE process before launching + a new one. diff --git a/src/audio.c b/src/audio.c index eb4a8d5..758f869 100644 --- a/src/audio.c +++ b/src/audio.c @@ -94,6 +94,15 @@ void audio_init(void) // Silence all 3 voices to clear any leftover SID state. audio_stop(); + + // The game uses the SID's voice-3 oscillator as its random + // number source (read via $D41B in game.c). audio_stop() above + // wrote freq=0 to all 3 voices, which leaves the oscillator + // stopped and $D41B stuck at 0 — so the first round of every + // session would always get the same (deterministic) WAIT + // duration. Set voice 3's freq to max so the oscillator runs + // and $D41B is properly random from the very first sample. + sid.voices[2].freq = 0xffff; } void audio_stop(void) diff --git a/src/build.sh b/src/build.sh index 0c3e4dc..8c310b9 100755 --- a/src/build.sh +++ b/src/build.sh @@ -55,7 +55,10 @@ mkdir -p "$BUILD_DIR" # --- kill any running VICE ------------------------------------------------ if [ "${1:-}" = "--kill" ]; then - pids=$(pgrep -f "x64sc? +confirmonexit.*whack\.d64" 2>/dev/null || true) + # pgrep -f uses ERE: + is a quantifier, so the regex must escape it. + # x64(sc)? matches both "x64" and "x64sc". The process is launched + # with the literal argument string `+confirmonexit`. + pids=$(pgrep -f "x64(sc)? \+confirmonexit.*whack\.d64" 2>/dev/null || true) if [ -n "$pids" ]; then echo "killing VICE process(es): $pids" kill $pids 2>/dev/null || true @@ -64,6 +67,7 @@ if [ "${1:-}" = "--kill" ]; then else echo "no VICE process running (autostart of whack.d64)" fi + rm -f "$BUILD_DIR/vice.pid" "$BUILD_DIR/vice.log" exit 0 fi @@ -103,10 +107,47 @@ for arg in "$@"; do esac done +# Reject conflicting flag combinations: -v/-V (foreground VICE) and +# -p/-P (background VICE) would both fire, leaving two emulator windows. +if [ -n "$EMU_CMD" ] && [ -n "$PLAY_VICE" ]; then + echo "error: cannot combine a foreground VICE flag (-v/-V) with a play flag (-p/-P)" >&2 + echo " use -v to run in the foreground, or -p to detach and continue" >&2 + exit 1 +fi + # --- compile -------------------------------------------------------------- echo "compiling $SRC with $OSCAR64_BIN -> $BUILD_DIR/" "$OSCAR64_BIN" -i="$OSCAR64_DIR/include" -o="$PRG" $OPT_FLAGS "$SRC" +# build_d64() — wrap $PRG in $D64. c1541's exit code is unreliable +# (the interactive form always exits 0 even on failure), so the only +# reliable success signal is listing the disk afterwards and checking +# the PRG is there. The flag-form `c1541 -list image` doesn't work +# (c1541 treats it as a unit number), so we use the interactive +# heredoc form. All c1541 calls are wrapped in `|| true` so set -e +# doesn't trip on transient non-zero exits from the format / write +# steps; the final list-and-grep is the authoritative check. +build_d64() { + if ! command -v c1541 >/dev/null 2>&1; then + echo "error: c1541 not found (needed to build the .d64 wrapper)" >&2 + return 1 + fi + c1541 -format wh,of d64 "$D64" >/dev/null 2>&1 || true + c1541 "$D64" >/dev/null 2>&1 </dev/null <&2 + return 1 + fi +} + # --- optional actions ----------------------------------------------------- case "$EMU_CMD" in "oscar64") @@ -115,14 +156,7 @@ case "$EMU_CMD" in ;; "x64"|"x64sc") echo "wrapping $PRG in $D64 (VICE autostart needs a disk image)..." - if ! command -v c1541 >/dev/null 2>&1; then - echo "error: c1541 not found (needed to build the .d64 wrapper)" >&2 - exit 1 - fi - c1541 -format wh,of d64 "$D64" >/dev/null - c1541 "$D64" >/dev/null <&2 fi @@ -140,11 +174,19 @@ if [ -n "$PLAY_VICE" ]; then echo "error: $PLAY_VICE not found in PATH" >&2 exit 1 fi + # If a previous VICE is still running, kill it before launching a + # new one — otherwise the two windows will fight for the same + # display and the old one's whack_hare will keep running in the + # background. + existing=$(pgrep -f "x64(sc)? \+confirmonexit.*whack\.d64" 2>/dev/null || true) + if [ -n "$existing" ]; then + echo "killing previous VICE process(es): $existing" + kill $existing 2>/dev/null || true + sleep 1 + kill -9 $existing 2>/dev/null || true + fi echo "wrapping $PRG in $D64 (VICE autostart needs a disk image)..." - c1541 -format wh,of d64 "$D64" >/dev/null - c1541 "$D64" >/dev/null <