Epic/07 stability issues - #7
Merged
Merged
Conversation
The SELECT GPIO was being configured at boot (select_configure) but
no callback was ever registered and no edge-detection routine was
ever polled, so a press did literally nothing. The latched
state-machine select_checkPushReset() in select.c already supported
the short/long press distinction — it just had no caller.
Wires the missing pieces:
- emul.c, after select_configure(): register reset_device as the
short-press callback and reset_deviceAndEraseFlash as the
long-press (≥ SELECT_LONG_RESET ms = 10 s) callback via the
setter pair already provided by select.h.
- emul.c main loop: call select_checkPushReset() each iteration
alongside chandler_loop / usbcdc_drain / term_loop. The 30 ms
debounce window only blocks at press / release transitions —
steady held / steady released paths return immediately, so the
main loop cadence is unaffected.
A first attempt did a verbatim port of md-drives-emulator's
select_coreWaitPush(...) (Core 1 watcher) but that froze the menu
on this app: the main loop's cyw43_arch_wait_for_work_until (Wi-Fi
poll mode, not present in md-drives-emulator) deadlocked once
Core 1 was running. Dropped the Core 1 launch and went with the
foreground-poll path instead — same UX, single core, coexists
cleanly with Wi-Fi poll mode.
Tested on hardware: short tap reboots the device cleanly back into
the firmware; ≥ 10 s hold triggers the factory-reset path
(reset_deviceAndEraseFlash).
Today GEMDRIVE_BLOB lands at screen_base − 8 KB and RUNNER_BLOB at
gemdrive_reloc − 4 KB (= screen_base − 12 KB). 5 KB + 3 KB = 8 KB
of code packed into a 12 KB span with $400 internal slack and zero
growth headroom for either blob. As the m68k firmware grows, the
slot overflows into screen memory — silent corruption.
Bumps the protected region to a contiguous 16 KB:
screen_base − 16 KB ($4000) GEMDRIVE_BLOB (5 KB) ← pinned bottom
screen_base − 11 KB ($2C00) end of GEMDRIVE_BLOB
screen_base − 10 KB ($2800) RUNNER_BLOB (3 KB) ← above gemdrive
screen_base − 7 KB ($1C00) end of RUNNER_BLOB
screen_base − 7 KB .. screen_base — 7 KB safety buffer
Code changes:
- rp/src/include/gemdrive.h — GEMDRIVE_DEFAULT_OFFSET_BYTES from
0x2000 to 0x4000. Comment block rewritten to spell out the
consolidated layout.
- target/atarist/src/runner.s — flips the runner-below-gemdrive
arithmetic: replaced `sub.l #RUNNER_RELOC_OFFSET, d0` with
`add.l #RUNNER_ABOVE_GEMDRIVE_OFFSET, d0` where the new offset
= GEMDRIVE_BLOB_SIZE ($1400) + $400 slack = $1800. RUNNER_BLOB
now sits above GEMDRIVE_BLOB inside the same 16 KB region.
Dropped the redundant runner-side `_memtop` re-patch — gemdrive_init
already lowered _memtop to screen_base − 16 KB which now covers
RUNNER_BLOB too.
- rp/src/emul.c — three user-visible references updated from
"screen-8KB" to "screen-16KB" (menu line, input prompt, comment).
- rp/src/aconfig.c, rp/src/include/aconfig.h — comments refreshed.
- target/atarist/src/main.s, target/atarist/src/devops.ld — the
cartridge-layout / linker-script comment blocks now describe the
new RUNNER_ABOVE_GEMDRIVE arithmetic and the 16 KB protected
region.
- README.md — setup-menu screenshot's [R]eloc addr line updated.
- rp/src/include/target_firmware.h — regenerated m68k binary
(built with the new RUNNER_ABOVE_GEMDRIVE_OFFSET arithmetic).
rp/src/gemdrive.c needs no changes — its `defaultReloc = screenBase −
GEMDRIVE_DEFAULT_OFFSET_BYTES` derivation picks up the bumped constant
automatically, and the protected `_memtop` defaults to the same value
so it covers the entire 16 KB region.
Tested on hardware: device boots with the menu showing
"[R]eloc addr: auto (screen-16KB)"; both [E]/[F] GEMDRIVE-only and
[U] Runner modes come up cleanly; programs Pexec'd from the Runner
see the GEMDRIVE driver as before; no overlap between blobs and no
overlap with the screen framebuffer.
Today gemdrive_init runs unconditionally from pre_auto at TOS
CA_INIT bit-27 phase, which means an unsafe reloc destination
(stack overlap, wrong screen-base, corrupted aconfig) crashes
the m68k before the setup menu can paint — the user has no way
to recover via the [R] reloc-address option without re-flashing.
This story splits gemdrive_init into two routines:
- gemdrive_handshake — Steps 1-3 only: Logbase + send_sync
CMD_GEMDRIVE_HELLO. Runs once at boot from pre_auto so the
setup menu can show the resolved reloc/memtop in shared vars,
but does NOT copy the blob and does NOT install the trap-#1
hook. Cheap and safe to run unconditionally.
- gemdrive_install — Steps 4-6: copy GEMDRIVE_BLOB to the
chosen RAM destination, lower _memtop, jsr into install_entry
(Setexc trap #1 + send CMD_SAVE_VECTORS to the RP). Called
from the mode-commit dispatchers, never from boot.
Dispatcher updates:
- rom_function ([E]/[F]) — now `jsr gemdrive_install` then
`jmp GEMDRIVE_BLOB+4` (diagnostic). The previous "must
NEVER be re-entered after boot" warning was deleted; now
install_entry runs exactly once per cold reset, on first
mode-commit.
- runner_function ([U]) — now `jsr gemdrive_install` then
`jmp RUNNER_BLOB`. The Runner needs the trap-#1 hook
installed because programs it Pexec's depend on the GEMDRIVE
drive being live. Stale "no relocation in v1" claim that
accompanied this dispatcher (incorrect — runner_entry has
self-relocated for many releases) replaced with an accurate
description.
Cross-region calling: rom_function and runner_function live
inside start_rom_code..end_rom_code (the print-loop relocated
chunk), gemdrive_install lives outside (after end_rom_code in
cartridge ROM). The `jsr gemdrive_install` from a relocated
caller resolves to absolute-long, which works regardless of
where the caller is running from — same pattern as the existing
`jmp GEMDRIVE_BLOB+4` and `jmp RUNNER_BLOB`.
Stack & sequencing: the new jsr adds one return-frame; install
saves 14 regs via movem.l, does its work, restores, rts. The
dispatcher's subsequent jmp doesn't push a new frame, so the
stack stays balanced for the GEMDRIVE-only path. On the Runner
path the unused return slot is orphaned harmlessly when Runner
takes over and never returns.
Comment refreshes carried alongside the rename:
- target/atarist/src/devops.ld linker-script doc block
- target/atarist/src/gemdrive.s three doc-block references
- target/atarist/src/runner.s two doc-block references
- target/atarist/src/main.s cartridge-layout block
rp/src/include/target_firmware.h is the regenerated m68k binary
(rebuilt with the deferred-install routing) — picked up
automatically by the next RP-side build.
Tested on hardware: device boots into the menu cleanly; [E]/[F]
GEMDRIVE-only and [U] Runner modes both come up; programs
Pexec'd from the Runner see the GEMDRIVE drive; the deferred
install runs at the expected point in the boot sequence and
TOS continues normally afterwards.
Both relocation sites (GEMDRIVE_BLOB in gemdrive_install,
RUNNER_BLOB in runner_entry) used to copy unconditionally into
[gemdrive_reloc, gemdrive_reloc + 16 KB) without any awareness
of where the live supervisor stack pointer is sitting. An
unsafe [R] reloc value (overlap with stack, weird screen_base
placement) silently corrupted stack frames mid-copy, manifesting
as random crashes / bombs rather than a clean error.
This story adds a single sanity check in gemdrive_install,
before the copy loop:
abort iff gemdrive_reloc <= sp < gemdrive_reloc + 16 KB
- sp < gemdrive_reloc → safe. Stack lives below the blobs;
pushes only lower sp further, away from the danger zone.
This is the common TOS layout — supervisor stack at
CA_INIT bit-27 phase normally sits in low memory
(~0x1000–0x6000), well below the blobs near screen_base.
- sp >= gemdrive_reloc + 16 KB → safe. Stack lives above
the zone; pushes lower sp but not by more than the entire
16 KB region in a single call chain.
- sp inside the 16 KB zone → genuine overlap; abort.
The single check covers both relocation sites: runner_entry
runs immediately after gemdrive_install with an essentially-
identical sp (the jsr+rts is stack-balanced) and lands fully
inside the same checked region (RUNNER_BLOB occupies
gemdrive_reloc + $1800..$2400). Per-site test in runner.s is
redundant.
Note: the FIRST iteration of this check had its direction
reversed — it required sp ABOVE the zone, which fired falsely
on every boot in normal TOS layouts. The corrected in-zone
test passes silently on real hardware.
Abort path (.gd_stack_overlap):
Uses inc/tos.s' `print` macro (Cconws wrapper) to put up
Reloc/stack overlap.
Raise [R] in setup menu, reset.
on the screen, then halts in a bra.s loop. We deliberately
do NOT rts from the abort — the dispatcher would otherwise
jmp into a non-installed blob and crash worse. User
recovers via SELECT (Pico reset → menu) or power-cycle,
then bumps [R].
Constants:
RELOC_DANGER_ZONE_SIZE = $4000 (full 16 KB) — the entire
consolidated protected region from S2. No slack carved out
on top: with the corrected check direction, the slack
rationale (tolerated SP landing in unused buffer) doesn't
apply because pushes only lower SP — if SP starts below
the zone, it stays below.
Cartridge size: 10128 / 10240 bytes (was 10128 / 10240 — net
size unchanged because the diagnostic body roughly cancels
the constant simplification).
Tested on hardware in auto-reloc mode — boots silently, no
banner. The deliberately-bad reloc test (set [R] inside the
stack zone) was not exercised — left to the user if they
need to trigger the abort path.
The "[E]xit (launch)" label was misleading — the verb didn't
actually exit the firmware, it committed GEMDRIVE-only mode
(drop into the emulated drive without activating the Runner
control surface). The "exit" framing made several testers
hesitate to press it because they assumed it would close /
return to Booster (which is what [X] does). New label
"[G]EMDRIVE" is self-explanatory and aligns with the
"[U]nner / [G]EMDRIVE / [F]irmware" trio of mode-commit
shortcuts.
Concrete changes:
rp/src/emul.c
- Command table: {"e", cmdExit} → {"g", cmdGemdrive}.
- cmdExit forward-decl + definition renamed to
cmdGemdrive. Function header comment refreshed.
- Bottom-strip render at line 1121:
"[E]xit (launch) r[U]nner [X] Booster"
→
"[G]EMDRIVE r[U]nner [X] Booster"
(32 chars, well under the 40-column screen width).
- Two countdown-stopped helper-text callsites (612, 1852):
"Press [E], [U] or [X] to continue."
→ "Press [G], [U] or [X] to continue."
- Three in-line comments refreshed (1003, 1157, 1867).
rp/src/include/emul.h
- Doxygen comments at 31 and 304 updated:
[E]/[F] → [G]/[F], [U] / [E] / [F] → [U] / [G] / [F].
m68k side (comments only):
- target/atarist/src/main.s:388 — rom_function dispatcher
comment.
- target/atarist/test/hello-debug/hello-debug.c:15 — the
sample app's firmware-mode-commit comment.
- target/atarist/src/main.s comment also touched as part of
this round; the m68k binary itself is unchanged but
target_firmware.h is regenerated by build.sh, picking up
one byte-pair shift in the embedded RELEASE_VERSION
string due to the build's reproducibility hash.
Documentation:
- README.md (boot-flow keys table, setup-menu screenshot,
bottom-strip + countdown-bar rows, two firmware-mode
references in the Debug traces chapter).
- docs/api.md (firmware-mode commit references in the
Debug traces section).
Compatibility: cartridge sentinel CMD_START is unchanged — the
m68k side only sees a different key binding; the protocol the
RP fires on commit is identical. The [F] alias still works
(separate scope question whether [F] should also be retired —
follow-up if needed).
Out of scope: historical epic docs (03/04/05) still reference
[E] in their narrative. Those describe state at the time the
work shipped — left as-is to keep the historical record
honest.
Tested on hardware (per request).
Every key that resolved to a handler used to be reachable
unconditionally from the setup-menu prompt — including ten
verbs that were not advertised anywhere on screen:
f → cmdFirmware (alias of [G])
m → cmdMenu (redraw)
? → cmdHiddenSettings (legacy debug doorway)
print
save (raw aconfig poke / dump verbs reachable
erase via the ? doorway, callable directly too)
get
put_int
put_bool
put_str
Three failure modes this leaves on the table:
- Terminal noise / ghosted ST keys could silently trigger
cmdSave / cmdErase, mutating or wiping the aconfig flash
sector with no on-screen confirmation.
- Fat-fingering `f` during the boot countdown commits
GEMDRIVE-only mode (the [F] alias) when the user meant
[G] / [U] / [X]. Same hazard the [E] → [G] rename in S5
was meant to address; [F] was the last remaining
unlabelled mode-commit verb.
- The full set of put_*/get/save/erase verbs documented
nowhere user-visible meant the only way for a tester to
know they existed was reading source. So they couldn't
serve as a real diagnostic surface, and they couldn't
serve as a real config UI either — pure footgun.
After this commit the command table is exactly:
{"g", cmdGemdrive} ← bottom-strip [G]EMDRIVE
{"x", cmdBooster} ← bottom-strip [X] Booster
{"o", cmdGemdriveFolder} ← GEMDRIVE row [o]
{"d", cmdGemdriveDrive} ← GEMDRIVE row [D]
{"r", cmdGemdriveRelocAddr} ← GEMDRIVE row [R]
{"t", cmdGemdriveMemtop} ← GEMDRIVE row Mem[t]op
{"v", cmdAdvHookVector} ← Adv [V]ector row
{"u", cmdRunner} ← bottom-strip r[U]nner
Eight entries, exactly matching the eight on-screen labels.
What you see is what you get.
Code shape:
rp/src/emul.c
- Forward decls for the ten retired handlers removed.
- Command-table reduced from 17 to 8 entries.
- Function bodies for cmdFirmware, cmdMenu,
cmdHiddenSettings, cmdPrint, cmdSave, cmdErase, cmdGet,
cmdPutInt, cmdPutBool, cmdPutString deleted.
- Header comment above the table rewritten to record the
retirement rationale.
- Comments referencing [F] updated.
rp/src/include/emul.h
- Doxygen comments narrowed from [U] / [G] / [F] to [U] / [G].
rp/src/gemdrive.c
- VERIFY_MEMTOP DPRINTF "[F]irmware path" → "[G]EMDRIVE path".
m68k side (comments only):
- target/atarist/src/main.s rom_function dispatcher.
- target/atarist/src/gemdrive.s diagnostic_entry block +
entry-table footnote.
- target/atarist/test/hello-debug/hello-debug.c sample app.
Documentation:
- README.md (boot-flow keys table, bottom-strip table row,
two firmware-mode-commit references).
- docs/api.md (two debug-traces references).
m68k binary regenerated (target_firmware.h) — the cartridge
sentinel CMD_START is unchanged so rom_function still fires
on the [G] commit; only the C-side dispatch table shrank.
Tested on hardware (per request).
Sweep over the changes shipped in S1–S6 for stragglers. Four
findings, all directly traceable to earlier stories:
1. README.md:123 — the GEMDRIVE menu-table row still said
"auto = screen_base − 8 KB" inside the parenthetical
description. S2 fixed the setup-menu screenshot block on
line 116 but missed this passing reference one row down.
Updated to "screen_base − 16 KB" so the doc is internally
consistent.
2. rp/src/term.c:119 — `static void cmdExit(const char *arg);`
forward declaration with no static definition anywhere in
the file. Pure compiler-fed orphan from a long-since-
removed earlier surface. Dropped.
3. rp/src/term.c — nine orphan exported term_cmd* functions
deleted (≈210 lines). All were exclusively reachable via
emul.c handlers that S6 retired:
term_cmdSettings (help printer that listed the retired
put_*/get/save/erase verbs)
term_cmdPrint
term_cmdSave
term_cmdErase
term_cmdGet
term_cmdPutInt
term_cmdPutBool
term_cmdPutString
term_cmdExit (was reached from the now-deleted
emul.c cmdExit family)
Plus the two static helpers used only by the put_* trio:
term_parseKeyAndTail
term_parseBoolToken
Replaced with a header-comment paragraph recording why they
were retired and pointing at S6.
Kept: term_cmdClear and term_cmdUnknown. These are generic
terminal-infrastructure helpers (clear screen, fallback for
unknown command) — not part of the settings-poke surface,
reasonable to leave around for future use even if not
currently called.
4. rp/src/include/term.h — eight orphan term_cmd* declarations
dropped to match the term.c body deletions. Two now-orphan
macros also removed:
TERM_PRINT_SETTINGS_BUFFER_SIZE (used only by the
deleted term_cmdPrint)
TERM_BOOL_INPUT_BUFF (used only by the
deleted parseBool helper)
Verification: grep across rp/src/ shows zero external callers
for every removed function or helper. The build path is purely
deletion of dead code; nothing live calls anything that's gone.
Net diff is +13 / −225, mostly the term.c body deletions. Not
staged: rp/version.txt / target/version.txt / version.txt
(held back for the epic-close commit).
The docs/epics/*.md planning files are gitignored — readers
of the committed tree will never see them. References inside
.c / .h / .s comments to "Epic 04", "Epic 06 / S5", "S7's
runner unload", "in v2", "in later stories", etc. therefore
point at documents that don't exist from the committed-tree
perspective. They read as ambient noise to anyone outside
the original development thread.
This commit rewrites every such comment to describe the
PURPOSE of the code without naming the planning artifact.
Examples:
"Epic 06 / S5+S6 — Pexec(3)/(4) load+exec split state."
→ "Pexec(3)/(4) load+exec split state."
"Epic 04 / S4 — active hook vector ID reported by the m68k"
→ "Active hook vector ID reported by the m68k"
"(Epic 06 / S2: switched from GEMDRIVE-only to Runner; ...)"
→ "(switched from GEMDRIVE-only to Runner; ...)"
"S7's runner unload" → "the runner unload"
"in later stories" → dropped
25 source / header / script files touched. Net +153 / -159 —
slightly negative because each removal drops slightly more
than it adds (surrounding prose is preserved verbatim).
Verification:
- `grep` for `Epic NN`, `/ S NN`, `S NN's`, bare `S NN [a-z]`,
`(S NN`, `(later|future|earlier|prior|next) (stor|epic)`
across rp/src/, target/atarist/src/, target/atarist/test/
returns empty (excluding the vendored u8g2/ tree).
- m68k build green; cartridge size unchanged at 10128 / 10240
bytes (comments don't affect the compiled output).
- target_firmware.h regenerated for the new build.
Out of scope (deliberately untouched):
- Commit messages and PR descriptions. Git history is a
fair place to cite the work that introduced a change.
- The docs/epics/*.md planning files themselves —
gitignored already.
Two new read-only rows under Mem[t]op:
Phystop : 0x100000 (or 0x100000 (!) on mismatch)
Screenmem : 0x078000
Phystop is TOS' _phystop ($42E) value. The (!) marker fires
when TOS' phystop disagrees with the silicon's MMU bank-config
register at $FFFF8001 — a sign that a reset-resistant program
lowered phystop and survived warm reset. The user must
power-cycle to recover; the marker exists to make that
conspicuous on the menu instead of failing silently.
Screenmem is _v_bas_ad ($44E), the logical screen base. Same
value XBIOS Logbase returns (which the m68k handshake already
ships as the relocation base), so no extra wire-format byte is
needed for this one — the RP just stashes the screen_base it
was already receiving.
Both values are 6 hex digits (24-bit ST address bus). Both are
read-only — no [?] key, no modification path. Modifying phystop
post-boot doesn't actually resize hardware RAM, it only confuses
TOS' Malloc accounting; the legitimate "reserve high RAM" use
case is already handled via _memtop, which the cartridge
patches for its own relocation region.
Wire format change: CMD_GEMDRIVE_HELLO grew from one longword
(screen_base) to three longwords (screen_base, phystop,
total_ram_kb). m68k decodes the MMU bank nibble at $FFFF8001 to
total_ram_kb (0 if the nibble is unrecognised — RP suppresses
the mismatch check in that case rather than fire spurious
warnings). send_sync size bumped from 4 to 12.
Code shape:
target/atarist/src/main.s — gemdrive_handshake extended:
move.l phystop.w, d4 ; TOS-reported $42E
[decode $FFFF8001 nibble → d5 = total_ram_kb, 0 = unknown]
send_sync CMD_GEMDRIVE_HELLO, 12
rp/src/gemdrive.c — HELLO branch unpacks 3 longwords, caches
phystop / screenmem / mismatch in module-static state.
New getters gemdrive_getPhystop() and gemdrive_getScreenmem()
return false until the first HELLO has landed.
rp/src/emul.c — menu() always paints both rows (with
"(waiting...)" placeholders) so the icon Y-tops, dividers,
and MENU_USBCDC_ROW below stay stable. New
refreshPhystopLine() / refreshScreenmemLine() overdraw the
placeholders once HELLO arrives. Both refreshes are cached
(only redraw on transition), called from emul_onGemdriveHello
for the immediate transition and from the main loop for
safety. Both use a %-16s padded format so the overdraw
fully covers prior content regardless of (!) marker state.
Layout shifts (Phystop + Screenmem add 2 rows = 16 px to the
GEMDRIVE block, every section below moves down):
MENU_ICON_ADV_YTOP 72 → 80 (term row 10)
MENU_ICON_API_YTOP 96 → 104 (term row 13)
MENU_ICON_USB_YTOP 128 → 136 (term row 17)
MENU_USBCDC_ROW 16 → 17
Divider above Adv: 68 → 76
Divider above API: 92 → 100
Divider above USB: 124 → 132
New: MENU_PHYSTOP_ROW = 7
New: MENU_SCREENMEM_ROW = 8
Documentation:
README.md — setup-menu screenshot now shows both rows; the
GEMDRIVE table cell describes the (!) marker and the
power-cycle requirement.
m68k build still 10128 / 10240 bytes; target_firmware.h
regenerated.
Wrap-up commit for the stability-issues epic. Bundles:
- version.txt × 3 → v1.0.1beta (root, rp/, target/).
- CHANGELOG.md gains a v1.0.1beta entry above the existing
v1.0.0beta one. Lists the user-visible deltas in two
sections: Fixes (SELECT button now works, bad reloc no
longer crashes the menu, _phystop tampering surfaces as
a (!) marker, _v_bas_ad now visible) and Setup menu
changes ([E]xit → [G]EMDRIVE rename + key rebind, [F]
alias removed, hidden command-line entries removed,
default Advanced Runner hook flipped to vbl ($70),
relocation region grew from 8 KB to a consolidated 16 KB).
- README.md gains a `### SELECT button` subsection in the
Usage chapter and a `### Stability banners` subsection at
the end of the Setup menu chapter — covers the
`Reloc/stack overlap` halt path and the `Phystop … (!)`
mismatch marker.
The actual fixes shipped earlier in the branch:
e59f035 S1: SELECT button wired
1b9a520 S2: 16 KB consolidated reloc region
8d68ee4 S3: deferred GEMDRIVE relocation
56ae67a S4: stack-overlap safety check
75221bd S5: [E]xit → [G]EMDRIVE rename
6be4cd6 S6: prune unadvertised command verbs (incl. [F])
5c25b9f Epic 07 cleanup (term.c orphans + S2 doc drift)
3911034 S7: strip Epic / S NN refs from in-tree comments
e4e0db0 S8: Phystop + Screenmem rows + (!) marker
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.
No description provided.