Apply fixes from 7-agent code review
Critical bugs found by the parallel code review: 1. .attr tables were at $A000-$B388 — inside the BASIC ROM. The KERNAL LOAD routine doesn't toggle $01, so writes were silently dropped on real C64 hardware. Moved the screens region to $BC00-$D000 (5 KB always-RAM under the banked-out KERNAL and below the I/O area). 2. game_enter_draw() initialized draw_was_pressed[] to 0, so a player holding fire from WAIT into DRAW got an instant win with 0 reaction time. Now initialized from input_fire() so the current state is the baseline, not zero. 3. First-round WAIT was always 100 frames because audio_stop() wrote freq=0 to all 3 voices — $D41B stuck at 0 so the random was deterministic. audio_init() now sets voice 3's freq to 0xffff after audio_stop() so the oscillator is running from the first sample. 4. build.sh --kill pgrep regex was broken: 'x64sc?' matches 'x64s' or 'x64sc' but never 'x64', and the '+' is a regex quantifier not a literal. Fixed: x64(sc)? \\+confirmonexit. 5. -v and -p (or -V and -P) would both fire and launch two VICE processes. Added mutual-exclusion check. 6. c1541 exits 0 even on failure, so set -e couldn't catch a broken d64 build. Extracted build_d64() helper that verifies the PRG landed in the d64 via c1541 -list (in the interactive heredoc form, since the flag form doesn't work on this c1541 version). 7. Counter showed '000' for one frame before '001'. Init to 1 not 0 (GAME.md spec). Build script also now: - --kill removes stale vice.pid / vice.log - -p / -P kill any prior VICE before launching a new one - Has a clear error for -v + -p combination Also: KNOWN_ISSUES.md documents 3 deferred items (LZO-in-IRQ screen tearing, IRQ half of mmap_trampoline silently disabled, audio schedule 1 frame short per note in WIN/GAMEOVER) that require larger refactors to fix.
This commit is contained in:
ballz
@@ -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.
|
||||
@@ -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)
|
||||
|
||||
+55
-13
@@ -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 <<EOF || true
|
||||
write $PRG
|
||||
EOF
|
||||
if c1541 "$D64" 2>/dev/null <<EOF | grep -q "whack_hare.prg"
|
||||
list
|
||||
quit
|
||||
EOF
|
||||
then
|
||||
:
|
||||
else
|
||||
echo "error: failed to write $PRG to $D64 (c1541 silently exited 0)" >&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 <<EOF
|
||||
write $PRG
|
||||
EOF
|
||||
build_d64
|
||||
if [ -z "${DISPLAY:-}" ]; then
|
||||
echo "warning: no \$DISPLAY set; VICE won't show a window" >&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 <<EOF
|
||||
write $PRG
|
||||
EOF
|
||||
build_d64
|
||||
echo "launching VICE $PLAY_VICE in the background with $D64..."
|
||||
log="$BUILD_DIR/vice.log"
|
||||
( setsid nohup "$PLAY_VICE" +confirmonexit -autostart "$D64" \
|
||||
|
||||
+9
-7
@@ -219,15 +219,17 @@ static void game_enter_draw(void)
|
||||
banner_clear(1);
|
||||
enter_frame = frame_count;
|
||||
vic.color_border = 1; // white border matches the white screen
|
||||
draw_was_pressed[0] = 0;
|
||||
draw_was_pressed[1] = 0;
|
||||
// Reset the per-frame counter and render "000" once. The
|
||||
draw_was_pressed[0] = input_fire(1); // Hare — capture current state
|
||||
draw_was_pressed[1] = input_fire(0); // Scoot — so holding from WAIT
|
||||
// doesn't auto-win
|
||||
// Reset the per-frame counter and render "001" once. The
|
||||
// bitmap was cleared to 0 and $D021 was set to white by
|
||||
// show_white_screen(), so this draws black "000" digits on the
|
||||
// show_white_screen(), so this draws black "001" digits on the
|
||||
// white background. game_step_draw() will then increment the
|
||||
// counter and re-render each frame.
|
||||
draw_counter = 0;
|
||||
draw_render_counter(0);
|
||||
// counter and re-render each frame. The spec (GAME.md §3 step
|
||||
// 4) says the counter starts at 001, not 000.
|
||||
draw_counter = 1;
|
||||
draw_render_counter(1);
|
||||
audio_state_enter(STATE_DRAW);
|
||||
// 5-frame transition stinger on voice 0 (voice 2 is the noise stab).
|
||||
audio_play_stinger(stinger_voice_for_state(STATE_DRAW), STINGER_DURATION);
|
||||
|
||||
+16
-3
@@ -26,8 +26,21 @@
|
||||
//
|
||||
// The trampoline is in the code section (around $08B3-$08FD), not at
|
||||
// $A000+ — the $A000+ entries in the .map are sstack (subroutine
|
||||
// stack) declarations of size 0, not actual data. So the entire
|
||||
// $A000-$C000 range is free for our use.
|
||||
// stack) declarations of size 0, not actual data.
|
||||
//
|
||||
// IMPORTANT: with the KERNAL banked out ($01=$35 after memmap_setup),
|
||||
// $A000-$BFFF is *NOT* a free 8 KB region — the BASIC ROM is normally
|
||||
// banked in there, and the C64 KERNAL LOAD routine ($FFD5 / $F49E) does
|
||||
// not toggle $01, so writes to $A000-$BFFF land on the read-only
|
||||
// BASIC ROM and are silently dropped on real hardware. This works
|
||||
// in oscar64's built-in emulator (which doesn't simulate ROM write-
|
||||
// protect) and in VICE (where the bank state depends on the .d64
|
||||
// bootstrapping), but it would fail on a real C64.
|
||||
//
|
||||
// We therefore place the .attr data in $BC00-$CFFF: 5 KB of always-RAM
|
||||
// (the I/O area $D000 stays mapped as I/O registers for the VIC, and
|
||||
// $E000-$FFFF is the LZO target / bitmap). Five .attr files × 1000
|
||||
// bytes = 5000 bytes, fits in 5120 bytes with room to spare.
|
||||
//
|
||||
// We don't need a heap for this program (no malloc). We set
|
||||
// heapsize(0) below in main.c to maximize the room available for data.
|
||||
@@ -58,7 +71,7 @@ const char ScreenWinScootBin[] = {
|
||||
|
||||
#pragma section( screens, 0)
|
||||
|
||||
#pragma region( screens, 0xA000, 0xC000, , , {screens} )
|
||||
#pragma region( screens, 0xBC00, 0xD000, , , {screens} )
|
||||
|
||||
#pragma data(screens)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user