#431 Add support for Magic Desk (Hardware Revision 1) Magic Desk Plus

[ClausS]

Attached i have a patch that adds support for Magic Desk cartridges as a PLUS version

The patch extends the existing Magic Desk cartridge (ID 19) by using Hardware Revision 1 for Magic Desk Plus.
Existing Magic Desk cartridges are not affected.

The patch also contains the required changes for cartconv, so Magic Desk Plus CRT images can be created and extracted using -t md -s 1.

I have tested the patch on both machines, macOS and Windows. So far I did not see any crashes or other problems.
And normal Magic Desk cartridges are still working as expected.

The idea behind this is to keep the complete cartridge, including SRAM and EEPROM, inside a single CRT file. This makes it easy to exchange the same CRT between different emulators and real hardware.

Thanks for taking a look.

[gpz]

Quickly skimmed over this patch.....

• all the hackery related to embedding the sram and/or eprom binary into crt files should get removed for the time being, we don't do this right now (see https://pastebin.com/Ahjiw9aY )
• the format should get implemented as its own CRT ID (Its too different from Magic Desk). Then the different variants (sram and/or eeprom present) can be handled as sub IDs.

Other than that....

• the cartridge API must be extended to deal with trinary image(s)
• sram and eeprom must be hooked into the cartridge api as secondary and trinary images
• GTK and SDL UIs need to be updated accordingly

[ClausS]

Thanks for your feedback.

Your comments sound fair and reasonable to me.
I'll need some time to work out an updated version.

Claus

[ClausS]

I have now reworked the implementation according to your suggestions:

Magic Desk Plus is now implemented as its own cartridge type with a dedicated CRT ID.
SRAM and EEPROM are handled through the cartridge API as secondary and tertiary images.
The GTK3 and SDL2 user interfaces have been updated accordingly.
cartconv has been updated to support the new cartridge format and hardware revisions.

I have also completed functional testing on both macOS and Windows for the GTK3 and SDL2 builds.

Tested features include:
all supported hardware revisions
SRAM
EEPROM
snapshot save/load
cartconv image creation rev. 0 to rev. 2

Please let me know if you would like any further changes or if there are additional aspects that should be reviewed.

Claus

[gpz]

mmmmh, it doesn't build here

/usr/x86_64-pc-linux-gnu/binutils-bin/2.46.0/ld.bfd: /usr/x86_64-pc-linux-gnu/binutils-bin/2.46.0/ld.bfd: DWARF error: mangled line number section (bad file number)
../src/c64/libc64stubs.a(c64-stubs.o): in function `cartridge_flush_tertiary_image':
/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/c64-stubs.c:41: multiple definition of `cartridge_flush_tertiary_image'; ../src/c64/cart/libc64cartsystem.a(c64carthooks.o):/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/cart/c64carthooks.c:3046: first defined here
/usr/x86_64-pc-linux-gnu/binutils-bin/2.46.0/ld.bfd: ../src/c64/libc64stubs.a(c64-stubs.o): in function `cartridge_save_tertiary_image':
/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/c64-stubs.c:46: multiple definition of `cartridge_save_tertiary_image'; ../src/c64/cart/libc64cartsystem.a(c64carthooks.o):/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/cart/c64carthooks.c:3141: first defined here
/usr/x86_64-pc-linux-gnu/binutils-bin/2.46.0/ld.bfd: ../src/c64/libc64stubs.a(c64-stubs.o): in function `cartridge_can_flush_tertiary_image':
/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/c64-stubs.c:51: multiple definition of `cartridge_can_flush_tertiary_image'; ../src/c64/cart/libc64cartsystem.a(c64carthooks.o):/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/cart/c64carthooks.c:2937: first defined here
/usr/x86_64-pc-linux-gnu/binutils-bin/2.46.0/ld.bfd: ../src/c64/libc64stubs.a(c64-stubs.o): in function `cartridge_can_save_tertiary_image':
/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/c64-stubs.c:56: multiple definition of `cartridge_can_save_tertiary_image'; ../src/c64/cart/libc64cartsystem.a(c64carthooks.o):/home/groepaz/Desktop/vice/vice-emu/trunk/vice/src/c64/cart/c64carthooks.c:2952: first defined here
clang++: fatal error: linker command failed with exit code 1 (use -v to see invocation)

[gpz]

seems the stubs in c64-stubs.c are not needed afterall

[ClausS]

Thanks for pointing this out.
I cannot test on Linux locally, and this duplicate-definition issue did not appear in my macOS or Windows builds.
I have removed the unnecessary definitions from c64-stubs.c and successfully rebuilt the updated version on macOS.
An updated patch is attached.

[gpz]

ok looked closer :)

First some nitpicking (Style issues) - the stub functions should not be in a single line, please always make them like the other functions.

Then, it implements a bunch of ROM sizes (32k etc) - does this new hardware even support those small ROMs? (Does it even support any other sizes?) I'd rather remove the sizes that do not exist (yet).

There is something strange with how the variants are implemented - cartconv suggests that the default variant has both SRAM and EEPROM - and a variant without either does not exist at all, is that correct?

And if that is correct - why does SRAM not work when i just attach a crt that is the default variant? (Attaching an SRAM image should NOT be required to enable it, same for the EEPROM of course)

Long story short: selecting the variant should not depend on attaching the extra images - those are always optional. There should be a dropdown box (or radion selection in SDL) to select the respective variant instead.

And last not least, the documentation (vice.texi) needs to be updated (cartconv section and CRT description)

[ClausS]

I have worked on the first two points.
The stub functions have been reformatted to match the usual VICE style.
Magic Desk Plus ROM sizes have been limited to the sizes documented by the hardware project: 256 KiB, 512 KiB, and 1024 KiB.

For the Variants:
Revision 0: SRAM and EEPROM
Revision 1: SRAM only
Revision 2: EEPROM only
In my view, a fourth variant without either SRAM or EEPROM would not be meaningful, since it would effectively be a standard Magic Desk cartridge, which is already covered by CRT ID 19. Please let me know if I am overlooking a relevant use case.

I still have some questions about the intended variant selection. Perhaps I am misunderstanding its purpose.
When a CRT is attached via drag and drop, VICE can determine the hardware variant from the revision stored in the CRT header. The corresponding SRAM and/or EEPROM can then be initialized internally, even when no save image has been configured.
However, without an SRAM or EEPROM image file, the memory contents remain temporary and are lost when the cartridge is detached or VICE exits. A user attaching the CRT via drag and drop may not be aware of this and could therefore lose saved data.
I can add a variant dropdown to the GTK3 settings and a corresponding radio selection to the SDL2 menu. Separate file selectors can be provided for the optional SRAM and EEPROM images. The implementation itself is not a problem.
What is unclear to me is how this should behave for a CRT attached via drag and drop. In that case, the variant is already defined by the CRT header, and the user does not visit the settings menu to select saving images.
Before continuing, I would like to make sure that I understand the intended behavior correctly. I may be overlooking an established VICE convention for handling the CRT revision and optional saving images.
Any guidance would be appreciated.
Claus

[gpz]

In my view, a fourth variant without either SRAM or EEPROM would not be meaningful, since it would effectively be a standard Magic Desk cartridge, which is already covered by CRT ID 19.

I agree, probably doesn't make a lot of sense

When a CRT is attached via drag and drop, VICE can determine the hardware variant from the revision stored in the CRT header. The corresponding SRAM and/or EEPROM can then be initialized internally, even when no save image has been configured.

Exactly - it should be configured for the variant that has been attached - no matter if image files for sram or eeprom are being used

However, without an SRAM or EEPROM image file, the memory contents remain temporary and are lost when the cartridge is detached or VICE exits. A user attaching the CRT via drag and drop may not be aware of this and could therefore lose saved data.

Correct. that is pretty much how it works with other ram based cartridges (reu, georam, etc) as well. (And if we want to change this, we should rework the cartridge system so it checks for changed images at shutdown - but thats a bit out of scope for this patch)

What is unclear to me is how this should behave for a CRT attached via drag and drop. In that case, the variant is already defined by the CRT header, and the user does not visit the settings menu to select saving images.

It should simply set the variant to what is in the CRT header, no more no less (And attaching a .bin should not even change it at all, so you can set it up before or after)