Repository navigation
Support read-only 2 MiB and 4 MiB EasyFlash CRTs - #65
Conversation
SensoriumEmbedded
left a comment
There was a problem hiding this comment.
Thank you for the 2 and 4 MB EasyFlash support! Looking forward to new content utilizing this expanded capability. :)
Apologies for the delay reviewing, there were several PRs in front of it. One conflicts, thus the rebase request below.
I haven't done any HW testing yet, sounds like you haven't either, so more could come after the requested changes.
I reviewed this against its base (5b61f3e), since it no longer merges cleanly with current main, and built both targets. As always, feel free to push back on any that are inappropriate or would take significant memory/code to resolve.
Memory: on TR+, RAM1 free for local variables drops 32 B in both the main image and MinimalBoot, matching your numbers. ITCM grows 640 B in the main image and 576 B in both MinimalBoot and the extension. RAM2 free for malloc drops 8,192 B in the main image and MinimalBoot, for the erased page.
There are four things I'd like you to look at before merging.
1. Keep the 2/4 MB support in MinimalBoot, and leave the main image's CHIP table as it is
A 2 or 4 MB image never runs in the main image. It loads CHIPs until RAM2 runs out, then hands the file to MinimalBoot, as it already does for files over about 650 KB. The shared changes still move every banked cart's CHIP table, in every image, from DTCM to the RAM2 heap. In the main image that brings:
- Less RAM2 for the REU. The REU loses 8 KB (the erased page), and 1.5 KB more (the table) when a CRT is loaded alongside it. A 512 KB REU takes the free part of the 128 KB RAM1 buffer, then 8 KB blocks from RAM2, rounded down to a power of two. With a 64 KB freezer loaded alongside (Super Snapshot V5, Final Cartridge III 1.01 or Retro Replay with REU), it needs 448 KB of RAM2. The spare drops from about 47 KB to 37 KB before any runtime allocations, and falling short halves the REU to 256 KB.
- USB launches near the limit stop working. A CRT just under the main image's limit now goes to MinimalBoot, which reads SD only.
- The table can be freed while a cart is running, so its handler has to be retired before each
FreeCrtChips. Those are theTeensy.inoandRemoteControl.inoedits, and the second one conflicts with main (see 2). - A longer Super Snapshot V5 IO1 read (
IOH_SuperSnapshotV5.c:177). Main reads the table entry with one load from DTCM. The read now loads the table pointer first, then the entry from RAM2, before the data reaches the bus. Constraints.md's "An IO1 read has to put its data on the bus early" has the background.
MinimalBoot never takes down a running cart; it reboots to the main image to get back to the menu. So the heap table there needs none of the retirement changes.
Suggested fix:
- In the main image, keep the static 128-entry table, 64 decode banks and the existing teardown order.
#ifdef MinimumBuildaroundMAX_CRT_CHIPS,NumDecodeBanks, the erased page and the table declarations covers it. - Keep your new count check in the main image's
ParseChipHeader. It closes the unbounded write described in Known-Issues.md. - In the main image, send an EasyFlash with any bank ≥ 64 to MinimalBoot, the same way as a file that outgrows RAM. Dense 2/4 MB images get there anyway; this covers a sparse high-bank file small enough to fit.
- The extension's ITCM has a hard 64 KB ceiling, and this PR takes its padding from 1,368 to 792 B at your base, about 536 B on current main. Keep the new EasyFlash-only code out of it with
VM_HOST_PROFILEwhere you can, as you already did for the bank count.
2. Rebase onto current main
With 1, the RemoteControl.ino conflict goes away. The one left is in Min_DriveDirLoad.ino: keep main's #include "Common/C128Crt.h". The harness slices this file between uint16_t NumCrtChips and File myFile, which now takes in that include, so re-run npm test after rebasing.
3. Match the Ultimate's CHIP handling (FileParsers.ino:154, Min_DriveDirLoad.ino:325)
ParseChipHeader now rejects an EasyFlash CHIP whose packet length isn't $2010, whose type isn't 0 or 2, or that repeats a bank half. The current firmware loads all of these: it reads ROMSize bytes, and a later half replaces an earlier one. The Ultimate's CRT loader accepts them too. It ignores the packet length, skips chip types other than 0 and 2 instead of failing, and lets the last duplicate win. It refuses a file only when the first header isn't a CHIP, a bank lies past the cartridge's size, or a read comes up short.
I don't know whether files like these exist, but accepting them is harmless and matches the Ultimate, so it seems better than refusing one that's potentially out there.
The rejections are also silent on the C64. None of the new checks print anything, and neither do the 128-CHIP limit for other types or the calloc failure. MinimalBoot used to say "More than 128 CRT Chips found" for these; that message now only fires at 512, for EasyFlash.
Suggested fix:
- Drop the packet-length and duplicate-half checks, letting a later half replace the earlier one.
- Skip other chip types instead of refusing the file.
- Keep the bank < 256, 8 KB size and load-address checks. The bank decoding depends on them, and files that fail them don't run correctly today (the Ultimate repeats smaller chips to fill 8 KB, but TeensyROM never has).
- Keep refusing a short final CHIP, as the Ultimate does, with a message saying the file is truncated.
- Print a reason for each refusal (on screen in the main image, on serial in MinimalBoot).
4. In MinimalBoot, allocate the erased page with the EasyFlash CHIP table (IOH_EasyFlash.c:28)
The erased page is a static DMAMEM buffer, so it takes 8 KB of RAM2 in MinimalBoot whatever cart is running, though only EasyFlash reads it. For Magic Desk 2 and other carts there, that can mean one fewer CHIP held in RAM, so more loads from SD. (With 1, the main image doesn't carry the page at all.)
Suggested fix: allocate it together with the EasyFlash CHIP table instead, which gives those carts the 8 KB back. The ISR only reads the page while the EasyFlash handler is current, and allocating it with the table puts it in RAM2 before the CHIP payloads fill the heap.
Hardware testing still to do: for one of us to get to before release
The native harness covers the loaders and malformed inputs well.
- Could you share the 2 and 4 MB images you've been testing with? I'll also generate synthetic ones (each bank identifies itself; sparse and out-of-order variants).
- 2 and 4 MB images from SD on TR and TR+, on PAL and NTSC, with enough play to exercise the SD swaps
- an existing EasyFlash small enough to run in the main image (under about 650 KB, for example Pool of Radiance), and one over 850 KB that swaps in MinimalBoot (for example A Pig Quest)
- a sparse image in MinimalBoot (missing halves read $FF), and one with its CHIPs out of order
- a 2 MB Magic Desk 2 image, which shares the bank reader, such as the current SNK vs Capcom "Stronger" releases: 128 banks of 16 KB, exactly the 128-CHIP limit for non-EasyFlash types
- a large EasyFlash launched from USB returns cleanly to the menu
- crafted bad CRTs are refused with a message and leave the menu working, in both images
Thank you!
SensoriumEmbedded
left a comment
There was a problem hiding this comment.
A few additions to the review above, continuing the numbering.
5. The EasyFlash test harness needs reworking with 1
easyflash.test.js doesn't compile the firmware files as they are: it cuts pieces out of seven of them by matching exact text and edits them with regexes. Two of those pieces are the retirement code that 1 removes, the if (CurrentIOHandler != IOH_TeensyROM) block in RemoteControl.ino and the retirement at the top of SetUpMainMenuROM. The harness also runs the main image's loader against the heap-allocated CHIP table, which 1 takes away. So npm test fails until the harness changes; in 2 I only mentioned the rebase side of this.
Suggested fix: with the 2/4 MB support only in MinimalBoot, the harness can shrink to the MinimalBoot loader and the EasyFlash handler. That also drops most of its text anchors in DriveDirLoad.ino, FileParsers.ino, Teensy.ino and RemoteControl.ino, where an unrelated edit near one would otherwise break the test.
6. Update the docs to match the fixes
Constraints.mddescribes a 512-entry CHIP table on the RAM2 heap in every image and a staticDMAMEMerased page. After 1 and 4, the main image keeps its static 128-entry table, and the page is allocated with the EasyFlash table.CRT_Implementation.mdsays duplicate halves and unsupported CHIP layouts are rejected. After 3, they're accepted, as on the Ultimate.
7. A failed SD read in MinimalBoot leaves the user at BASIC with no explanation (IOH_EasyFlash.c:108)
When a bank read fails, EasyFlashSwapFailed() detaches the cart and sets doReset. That happens either at startup, when bank 0 is loaded before the C64 starts, or during play. At startup the flag also skips the existing "CRT not loaded, Abort!" path, which returns to the menu. Either way the C64 resets to BASIC with no cart, the only output is "Resetting C64" on serial, and the menu comes back only when someone presses the button. The message request in 3 covers files refused while loading, not reads that fail afterwards.
It takes a file that loads cleanly and then fails a read (an SD card pulled mid-game, say), so this one is low priority.
Suggested fix: print the reason and call runMainTRApp_FromMin() instead of resetting.
Thanks!
b5cb2d5 to
50388d1
Compare
|
Thanks for the detailed review, Travis. I've rebased onto current main and pushed the changes. The main image is back to its static 128-entry table, 64-bank decoder and original teardown order. Sparse high-bank files take the existing SD handoff, and native MinimalBoot allocates the erased page with its EasyFlash metadata before loading payloads. The extension host keeps its original loader path and code footprint. CHIP handling now ignores packet-length discrepancies, skips unsupported types and lets the last duplicate half win. Duplicates reuse their table slot, including an override after all 512 halves are present. Bank, size, address and truncation checks have messages. Failed SD reads return to the menu. Review also caught a long duplicate/skipped-record stream exceeding the swap marker's 24-bit offset range; that now fails cleanly. The harness directly includes the MinimalBoot loader and handlers, with focused main-image routing/bounds tests. The C128 changes and documentation are retained and updated. Validation passes: 169 npm tests with seven Windows platform/tool skips, 32/64-bit native regressions, extension verification, 79 Linux bench tests, and complete TR/TR+ builds. Compared with current main, free RAM is unchanged in both TR+ images. The extension remains at 64,424 bytes of ITCM code with 1,112 bytes padding. I've also prepared KQ2 as a genuine 2 MB EasyFlash CRT and Another World as a 4 MB CRT to provide privately. KQ2 passed fresh PAL/NTSC startup and walking checks and has real data through bank 70. Both exact CRTs passed the revised native loader/bank readback tests. The public PR includes a generator for original dense/sparse/reversed bank-identification fixtures, without game data. |
|
Thank you for the quick and thorough rework, and for the KQ2, SQ2 and Another World images, this fits really well now! I re-reviewed 50388d1 merged onto current main and built both targets. Everything from the earlier review is addressed:
Memory: on TR+ nothing changes in RAM1 or RAM2 in any image, matching your table, and the extension stays at 1,112 B of padding. On plain TR, the new message strings push DTCM over an alignment boundary: free for local variables drops 1,024 B in MinimalBoot (still about 5 KB above its floor) and 4,096 B in the main image. No change needed for that. Hardware testingI ran a full pass on a TR+ with a PAL C64, and everything passed. Besides your images, I used some runnable bank-test carts: each checks every byte of every bank, then the bank register wrap, then switches to random banks for as long as it runs. Happy to share them and their generator if you're interested.
Still to do: for one of us to get to before release
Thank you very much!! |
…chitecture PR #65 put its loader reference (bank mask, CHIP table, CHIP handling, 24-bit swap offsets) and the test-fixture commands in CRT_Implementation.md, between "until..." and the SD bank-swap section it leads into. That doc tells how CRT support grew, so it now ends with a short "2 and 4 MB EasyFlash" note crediting John, and the detail moves to a new "EasyFlash beyond 1 MB" subsection under Teensy-Firmware.md's "MinimalBoot vs full firmware", with nothing dropped. The user docs say MB/KB rather than MiB/KiB and no longer mention PSRAM. Constraints.md notes that an EasyFlash now spends about 14 KB of MinimalBoot's RAM2 on its CHIP table and erased page, so the measured resident-bank figure is one or two banks high for EasyFlash.
Adds read-only 2 MiB and 4 MiB EasyFlash CRT support through native MinimalBoot's existing SD bank-swap path. Ordinary 1 MiB images retain six-bit bank wrapping; extended images use 128 or 256 banks. EAPI writes remain unsupported.
The normal launcher retains its static 128-entry DTCM CHIP table, 64-bank decoder and existing teardown order. A sparse EasyFlash containing bank 64 or above takes the existing SD-only MinimalBoot handoff even if its file fits the normal launcher's RAM. USB launches requiring that handoff are refused with a message.
Native MinimalBoot allocates up to 512 unique EasyFlash halves and the 8 KiB erased page together, before ROM payload allocation. Other cartridges retain the 128-CHIP limit and allocate no erased page. Omitted halves read $FF, and unordered bank-zero halves are loaded before startup. The extension host retains its original loader and 64-bank client mapping.
CHIP packet-length fields are ignored; unsupported EasyFlash CHIP types are skipped; a later duplicate half wins without consuming another metadata slot. Accepted halves require bank <256, exactly 8 KiB and address $8000/$A000/$E000. Short headers/payloads and allocation failures report a reason. Swapped payloads must fit the 24-bit offset marker, including their final byte. Failed SD bank reads return to the menu rather than resetting to BASIC.
Rebased onto current main, retaining its shared C128 cartridge support. The regression harness directly includes the production MinimalBoot loader/handlers and shared CRT/C128 headers; focused main-image tests cover routing and its static-table boundary. An original bank-identification fixture generator is included, with dense/sparse/reversed 2/4 MiB examples described in docs/CRT_Implementation.md. Game data is excluded from Git.
Validation:
Fresh TR+ comparison against unchanged upstream e0a2532 with the same shipping configuration and toolchain:
The extension retains 1,112 bytes of ITCM padding. EasyFlash's table/erased page use heap only when a native EasyFlash is loaded; the free-RAM figures above are fixed-image figures.