Magic Desk Plus cartridge support (#727) — simulated only, never run - #844
enver-haase wants to merge 5 commits into
Conversation
Magic Desk Plus is a Magic Desk with three things bolted on: one more bank bit, so DE00 selects 128 banks of 8K instead of 64, a page register at DE01, and a control register at DE03 that switches a 256 byte window at DF00 between 128K of battery-backed SRAM and an 8K or 32K EEPROM. The window is readable and writable, and stays served when the ROM is switched off -- the file system the format ships with depends on that. The SRAM and the EEPROM go in the memory the REU and GeoRAM already share rather than in an area of their own. That region exists on every target, which an area of its own would not: above the 64K of cartridge RAM there is a free megabyte on U64, U64-II and U2+L, but on the U2 the cartridge ROM starts right there. The cost is that this cart and the REU cannot both be on, which is what GeoRAM already does and what the prohibit mechanism already handles. tb_magic_desk_plus drives the registers the way the machine does and checks the address the logic produces. Against this change all 20 checks pass; against the unchanged file 17 fail. It also pins down something the file does not say anywhere: cart_variant is sampled only while the cartridge is in reset, so the EEPROM size cannot be changed without one. For GideonZ#727.
ece738c to
15aaf09
Compare
|
Corrected the EEPROM handling: VICE does say which part is fitted, and I had claimed it does not.
if (size != 8192 && size != 32768) {
log_message(LOG_DEFAULT,
"MAGICDESK: Invalid EEPROM image size (must be 8K or 32K).");
return -1;
}and the page mask follows from it, in both io2 handlers: It now does the same. The store travels as chunks at DF00 and the size says which is which — 8K or 32K is the EEPROM, 128K is the SRAM, and the three cannot be confused. A size that is none of them is refused rather than loaded. The variant, and with it the mask, follows the EEPROM image size instead of being fixed. One consequence worth naming: a cart that brings only SRAM now gets the 8K mask, not the 32K one I had picked. That is what VICE creates when it has to make an EEPROM image from nothing, so it is the more faithful default. Rebuilt: Everything in the opening paragraph still stands: none of this has run on hardware, and Murder on the Mississippi Remastered has not been tried. |
Magic Desk and Magic Desk Plus are the same CRT hardware type. The upstream implementation confirms it: the VICE patch that comes with the format extends magicdesk.c rather than adding a cartridge, and its attach path derives nothing from the file but a bank mask, taken from the highest bank present. What turns the SRAM and the EEPROM on there is the user supplying an image for them. So nothing in the header can tell the two apart, and this uses the same thing VICE does: a cart that brought its non-volatile memory with it is a Plus. It travels in the CRT as chunks at DF00, the address of the window they are reached through, and the bank field says which piece each chunk is. It has to: the size field of a CHIP header is 16 bits, so the 128K of SRAM cannot be one chunk and is carried in four quarters of 32K. Bank 0 is the EEPROM, banks 1 to 4 are the SRAM in address order. A chunk that is none of those shapes is refused rather than loaded. The EEPROM size chooses the page mask, again as in VICE: 8K masks the page register to 0x1F and 32K to 0x7F, which is what its io2 handlers do, and it accepts an EEPROM image only at those two sizes. A cart carrying only SRAM gets the 8K mask, the size VICE itself creates when it has to make an EEPROM image from nothing. The cart prohibits the whole of IO rather than only DEXX. Its registers are at DE00..DE03 and its window is the entire DF00 page, so the UCI at DF1C, the sampler, an ACIA at either address and the REU whose memory this borrows all have to give way. A Magic Desk without any of this keeps the mapping it has always had. For GideonZ#727.
15aaf09 to
331f1fc
Compare
|
Adversarial pass over my own branch. Three findings, one of them fatal. Two are fixed; the third turned out not to be mine to fix. Fatal: a 128K chunk cannot exist in a CRTThe size field of a CHIP header is 16 bits — Neither the simulation nor the build could have caught this: the testbench exercises the cartridge logic, not the file parser, and a constant that never matches compiles happily. Fixed by using the bank field, which a DF00 chunk does not otherwise need: bank 0 is the EEPROM at 8K or 32K, banks 1 to 4 are the SRAM in quarters of 32K, in address order. That also removes an ambiguity I had built in, where 32K would have meant both "EEPROM" and "a quarter of the SRAM". A chunk of any other shape is now refused instead of loaded. Real: the prohibition missed the DF00 pageI had #define CART_PROHIBIT_DFXX (CART_ACIA_DF | CART_REU | CART_MAXREU | CART_UCI | CART_SAMPLER)so the UCI at DF1C — inside the window — the sampler, an ACIA at DF00 and CART_MAXREU were all left enabled. This cart owns DE00..DE03 and the whole DF00 page. Fixed: Not a defect: saving is explicit here, by designI was going to add an automatic write-back, on the grounds that "battery-backed" means the player should not lose their notebook. Checking first was the right call: nothing on this device persists cartridge state by itself. So this is a house convention, not a gap, and Magic Desk Plus follows it rather than becoming the one cart that saves on its own. The consequence should be said plainly, because this cart is unusual in how much it depends on it: the reason Murder on the Mississippi Remastered uses this format at all is that it writes notes and progress continuously. On real hardware a battery keeps them. Here they live in the shared memory until the player picks Save Cartridge, and a power cycle without that loses them. If that trade should come out differently for this cart, it is a UX call rather than a technical one, and it is yours. UnchangedThe cartridge logic and its testbench are untouched by all three: the address map, the register decode and the 0x1F/0x7F masks were right from the start. One minor note carried over from #822: the store is filled with a plain Rebuilt after both fixes: Still true, and still the most important line here: none of this has run. No bitstream, no device, and the game has not been tried. |
Adding the Magic Desk Plus cartridge to the slot logic changes what Synplify emits for the U2+L, and that is enough to move i_riscv/i_core/N_268 onto a primary clock resource. A primary net inside the RiscV stops the FPGA from booting, so the check in target/fpga/u2plus_ecp5 aborts the build. Spell the net the way par reports it. The failing build also shows map disabling a PROHIBIT for "i_riscv/i_core/N_268_i" as not matching any net in the design; that spelling is not in this tree. The abort says three signals because par prints the same warning block three times and the check counts lines. There is one signal.
Gideon added both spellings of the net in 8806a43, so the LPF line this branch added is now upstream's. Keep upstream's version of the file, which carries N_268_i alongside N_268.
chrisgleissner
left a comment
chrisgleissner
left a comment
There was a problem hiding this comment.
PR Review: Changes Requested
Thank you for the detailed implementation and GHDL simulation testbench (tb_magic_desk_plus.vhd), @enver-haase.
This PR is not yet ready for merge. As noted in the description, this implementation has only been run in VHDL simulation and has not been verified on target firmware/hardware or against real cartridges/games.
Before this PR can be considered for merge, it must be enriched with an automated end-to-end Python test suite in tests/e2e/io/c64/ (and registered in run-tests and tests/README.md), following the established pattern of other recent cartridge fixes in this repository.
Recent Cartridge E2E Test References
Please consult these recent cartridge E2E tests for reference on how synthetic CRT generation and host-side 6502 test routines are structured:
tests/e2e/io/c64/c64gs_cartridge_test.py(commit7d9bf4bd):
Generates a synthetic 64-bank Type 15 CRT, uploads it viadevice.runners.upload("run_crt", ...), copies a 6502 test routine to $C000, and verifies bank switching via IO1 accesses with marker validation in screen RAM ($0400).tests/e2e/io/c64/comal80_cartridge_test.py(PR #899 / commitc7464da2):
Builds a synthetic Comal 80 CRT to test bank selection and cartridge-off bit handling.tests/e2e/io/c64/ocean_cartridge_test.py(commit2db13b9f):
Tests Ocean 16K bank switching using synthetic CRT generation.tests/e2e/io/c64/ultimax_cartridge_test.py(PR #882 / commitfd756470):
Verifies Ultimax CRT loading and VIC stream output.
Required Changes Before Merge
-
Add E2E Test Suite (
tests/e2e/io/c64/magicdesk_plus_cartridge_test.py):- Construct a synthetic Type 19 Magic Desk Plus CRT containing ROM bank chunks, an EEPROM chunk (bank 0 at
$DF00), and SRAM chunks (banks 1..4 at$DF00). - Run a 6502 test routine from RAM ($C000) to validate:
- DE00: Bank selection across bits 0..6 (128 banks) and bit 7 ROM disable (
exrom_n). - DE01 & DF00 window: Page selection via DE01 and window read/write data correctness at
$DF00..$DFFF. - DE03: EEPROM select (bit 5 = 0) vs SRAM select (bit 5 = 1) and SRAM portion select (bit 0).
- Window persistence: Confirming that
$DF00..$DFFFremains served and writable even when the ROM is switched off (DE00bit 7 set).
- DE00: Bank selection across bits 0..6 (128 banks) and bit 7 ROM disable (
- Register the new test suite in
run-testsandtests/README.md.
- Construct a synthetic Type 19 Magic Desk Plus CRT containing ROM bank chunks, an EEPROM chunk (bank 0 at
-
Verify FPGA Generics:
- Double-check that top-level FPGA generics on target platforms set
g_max_cart_bits >= 21so that all 128 banks of 8K are addressable.
- Double-check that top-level FPGA generics on target platforms set
Please update the PR with the requested E2E tests once verified on target hardware/firmware.
Is this not for @GideonZ to provide the (testable) gateware? |
|
In the latest update, the Vice Team introduced support for the Magic Desk Plus but strangely decided to assign it a new, additional ID—different from 19. The new ID is 87. |
|
I assigned TwoMegabyter to 87.. hehe |
|
I saw the FPGA changes. They are quite significant. And, I am wondering why the cartridge RAM area was not used. I don't think cartridges should use the REU ram. Did you have a special reason for using REU ram? @enver-haase |
|
@enver-haase I was referring to your initial statement: “This has never run. Not on hardware, not as a bitstream, not against the game.” We normally require tests before merging a PR, ideally E2E tests. If testing depends on Gideon providing a bitstream, could you make that dependency explicit in the PR and clarify which tests remain outstanding and who would run them? It would also help to distinguish what the existing simulation tests establish from what still needs hardware verification. Keeping the PR in draft until that verification is complete would make its status clearer. By the way, were you able to get your C64U repaired? You opened several other PRs a few weeks ago with similar notes about missing hardware testing. Once your setup is working and the necessary builds are available, could you revisit those PRs, test them on your hardware, and update them with the results and any fixes needed? It's always a good idea for us firmware devs to test on our own rig first and publish red/green results. |
|
@GideonZ — fair question, and the reasoning belongs here in the thread rather than Magic Desk Plus carries 128K of battery-backed SRAM plus an 8K or 32K EEPROM, so
So the SRAM does not fit in the cartridge RAM area, not even at half its size. The That is what sent me to the memory the REU and GeoRAM share: it is the only region of I share your instinct that cartridges should not be reaching into REU memory. Two ways
Tell me which and I will move the branch to it. One more thing worth deciding while this is open: @crystalct points out VICE gave Magic @chrisgleissner — yes, the C64U is working again as of today, and I have set this PR to Verified, and only this: Not verified, and not verifiable by me: anything on real hardware. There is no synthesis |
|
@enver-haase Congrats on fixing the C64U! Did you end up soldering your own JTAG adapter based on an FT232H and did you use the unbrick tool in this repo? |
# Conflicts: # software/io/c64/c64_crt.cc
|
Ultimately, it is Team Vice that defines the CRT IDs, and everyone else using that format falls into line; otherwise, they would create ambiguities and CRTs that are incompatible depending on the platform used. When the TwoMegabyter type becomes "official," it will get its own ID—specifically, the first one available at that time. |
|
@chrisgleissner — bought, not built: an FT232H breakout of the kind And yes, the recovery tooling in The machine has been in steady use since: everything measured in #843 and #915 today was measured on it. |
4 participants
Implements Magic Desk Plus, requested in #727 for Murder on the Mississippi Remastered.
What the format actually is
The cartridge is a Magic Desk with three things added: DE00 gains bit 6 as a bank bit, so it selects 128 banks of 8K instead of 64; DE01 becomes a 256-byte page register; DE03 becomes a control register whose bit 0 picks the SRAM half and bit 5 switches between 128K of battery-backed SRAM and an 8K or 32K EEPROM. Both are reached through a 256-byte window at DF00, readable and writable, which stays served when the ROM is switched off — the file system the format ships with relies on that.
Magic Desk and Magic Desk Plus are the same CRT hardware type, 19. The VICE patch that comes with the format extends
magicdesk.crather than adding a cartridge, andmagicdesk_crt_attach()derives nothing from the file but a bank mask taken from the highest bank present. What turns the SRAM and EEPROM on there is the user supplying an image for them. So there is nothing in the header to detect, and this follows the same idea: a cart that brought its non-volatile memory with it is a Plus.Where the memory lives
In the region the REU and GeoRAM already share, not in an area of its own. That was the second attempt: above the 64K of cartridge RAM there is a free megabyte on U64, U64-II and U2+L, but on the U2 the cartridge ROM starts exactly there, so an area of its own would have made this a feature one platform could not have. The shared region exists everywhere. The cost is that this cart and the REU cannot both be on, which is what GeoRAM already does and what
def->prohibitalready handles.The store travels in the CRT as a chunk at DF00 — the address of the window — holding the EEPROM first and the SRAM behind it, in the order the logic addresses them. This layout is a proposal. The format defines no CRT representation for its non-volatile memory, so if @crystalct has one in mind, say so and I will follow it instead.
Red, then green
fpga/cart_slot/vhdl_sim/tb_magic_desk_plus.vhddrives the registers as the machine does and checks the address the logic produces, under GHDL:all_carts_v5.vhdThe three that pass either way are the power-up defaults, where the expected value is zero and an unimplemented cart is zero anyway.
It also pinned down something the sources do not state:
cart_variantis sampled only while the cartridge is in reset, together withcart_logic_d. My first testbench changed the variant without one and got the old value. That is now a documented step in the test.For @GideonZ, one thing I cannot answer
rom_addr(g_max_cart_bits-1 downto 13)bounds the bank width, and the software lays banks at a 16K stride (bank_multiplier = 16 * 1024), so 128 banks needg_max_cart_bits >= 21. The U2+ top levels pass 22; the default inslot_server_v4.vhdis 20. What the U64 and C64U pass I cannot see —fpga/fpga_top/holds onlyultimate_fpga,cyclone4_test,ecp5_testandvideo_fpga, so those top levels are not in this repository. At 20, this cart is limited to 64 banks and a 1 MB image will not work. Worth checking before anyone builds a bitstream.What is verified, and it is not much
ghdl -aon the changedall_carts_v5.vhd: clean, exactly as the unchanged file is.make u64ii_no_espinghcr.io/gideonz/riscvfrom a tree with everyoutput/andresult/undertarget/removed first: exit 0, no error, no new warning fromc64_crt.cc.What is not verified
Everything else. No bitstream was built, so the cartridge logic has never been synthesised, let alone run. No device has seen this. The game has not been tried. The 10 ms an EEPROM write physically takes is not modelled — a write through the window completes immediately, which is more permissive than the real part, so software that respects the delay will work and software that tests for it will not see it. The EEPROM size is fixed at the 32K variant because the file does not say which part was fitted.