esq5505.cpp: Add VFX-family ROM & EEPROM Cartridge support, and improve floppy support.

[cbrunschen]

Second attempt, simpler and hopefully better.

I've tried to address the issues identified in the Revert commit message. Specifically, the cartridge is now represented by a single image device class, and vfxcart.h includes only diimage.h, not emu.h. All indentation should also now be tabs, not spaces, and there should be no trailing spaces (running srcclean changes nothing further).

[cbrunschen]

While I've tried to address the issues that were raised when the previous attempt at this was reverted, I would appreciate feedback on what I have missed, even in small things.

[MooglyGuy]

Looks reasonable to me other than some minor suggestions/requests. Hopefully @rb6502 or @galibert can take the time to give it a look-see as well.

[MooglyGuy]

No need to explicitly void out unused function parameters, I believe any warnings that could be caused have been suppressed since forever.

[cbrunschen]

Habit from other C++ code I work on. Removed.

[MooglyGuy]

I would use LOGMASKED for this rather than #if-ing it. #if'd-out code tends to rot. Should be plenty of examples to choose from when it comes to LOGMASKED, the only thing to know is that LOG(...) is effectively an alias for LOGMASKED(LOG_GENERAL, ...), so user-defined verbose masks should start at 1U << 1.

[cbrunschen]

The #ifdef here is also guarding the static int prev_state, which is only there to track the previous state, so the logging doesn't spam non-changing updates; so it's not as straightforward as just replacing LOG with LOGMASKED.
But as this LOG was really only useful during development, I'm removing it; it's simple enough to add something similar back in the future if the need arises.

[MooglyGuy]

Same as above, don't need to explicitly void out unused function parameters.

[cbrunschen]

Done.

[MooglyGuy]

MAME isn't super-consistent all the time about this sort of thing, but it would be slightly more preferred to put this on the following line.

[cbrunschen]

Done.

[MooglyGuy]

updated_irq instead of v, perhaps?

[cbrunschen]

Done.

[MooglyGuy]

Commented code can be deleted, or if you don't want it logged all the time, give it a separate bitfield and use LOGMASKED.

[cbrunschen]

Removed.

[MooglyGuy]

Two more contenders for LOGMASKED here.

[cbrunschen]

Removed.

[MooglyGuy]

Suggest newline here and on the one above.

[cbrunschen]

Done.

[MooglyGuy]

Newline.

[cbrunschen]

Done.

[MooglyGuy]

Is this behavior accurate to a real VFX (i.e., write-once cartridges)?

[cbrunschen]

No, I have never encountered a write-once cartridge.

The idea is simply one of usability in a simulated environment: this lets you create a read-only cartridge (.cart or rom), but as an empty read-only cartridge makes little sense, it allows writing to it once; otherwise you'll have to first create it as a writable cartridge, then eject it and rename it (from .eeprom or .sc32 to .cart or .rom). This way, you can just create it, with the intention that it will be a read-only cartridge once it has been written to; and without running the risk of forgetting to rename the cartridge, loading it and accidentally overwriting.

[cbrunschen]

I changed this so that the vfxcart device will only create .eeprom or .sc32 images, and will return a suitable error otherwise.

However, I note that MAME still creates the files, leaving them in place and empty.

[MooglyGuy]

So the user still has the option to create any given extension supported by the image device - .rom, .cart, .eeprom or .sc32 - but only .eeprom or .sc32 images are writable?

I'm admittedly unsure if there's a good workaround for this.

IMO, there isn't so much of a need for any differentiation in function like that. Support creation and writing to any of the above. Items that exist in a softlist have their underlying data inherently static, with only diffs against the fundamental data being stored, and in a separate file at that. Even if a user is able to write to a ROM-only cartridge, that's about the best one can do in the current implementation.

I'd like to hear from @cuavas or whoever rejected the initial implementation, because this seems like the exact sort of situation that calls for a cartslot-type bus device which supports two different device_cartrom_slot_interfaces, one that bounces out to an image device, and one that only does straight loading of ROM data. Per @cbrunschen, the original PR was rejected for being too complicated, but I'm not sure I would agree, given the two distinct types of pluggable cartridges.

As far as the UX of the VFX goes, users could buy blank, writable cartridges to make their own patches, or they could be non-writable cartridges preloaded with patches. It seems like a perfect fit for the intention behind the original PR, whereas going with a "pure" imagedev just results in annoying hoop-jumping on @cbrunschen's part.

[cbrunschen]

Users could buy either pre-made ROM cartridges (Such as Ensoniq's fairly ubiquitous VPC-100 series), or EEPROM cartridges such as Ensoniq's own StorCart-32.
These were made on the same PCB, just with either a programmed 27256 series EPROM, or a 28256 EEPROM, and with two out of four zero-Ohm resistors placed to make the correct connections.

To the best of my knowledge, the actual contents of each is identical. The main difference is simply that writes to an EEPROM cartridge succeed, but attempted writes to a ROM cartridge fail.

To me, this is not really much different from a floppy image that may or may not be write protected.

I had originally written a Slot device and an Image device, which could load either ROM or EEPROM cartridges; and I understand that this was considered overly complex, since the two could be combined into a single device. And I don't disagree; I wrote it the way I did largely because that seemed to be a patter among other slot and image devices.

That image device that could load both read-only and writable images, differentiated by their filetype, fairly arbitrarily chose as '.rom' or '.cart' for ROM cartridges, '.eeprom' or '.sc32' (for StorCart-32) for EEPROM ones. And in order to make things easier for the user to create a cartridge that is intended to be write-protected, it also allowed creating not only a writable .eeprom or .sc32 image, but also a .rom or .cart image, which would be loaded as effectively "once-writable"; with the idea being that the user would write to it once (copying over the entire contents of the internal RAM sounds and presets), and after it was unloaded, it would then be effectively read-only the next time it was loaded.

In the first round of this second PR I eliminated the separate slot device but left the image device mostly the same, and able to load either ROM (read-only) or EEPROM (writable) images, as well as create them as above.

Per comments earlier, I then changed the image device so that it will only create EEPROM cartridges that can actually be written to, and returns errors for attempts to create ROM cartridge images (instead of allowing them to be written once), which makes sense. However, MAME always creates the files, and returning an error from call_create() only prevents that file from being written to and loaded.

I don't really think this is a situation where there are two significantly different kinds of devices; After all, floppy disk images are not loaded into different image device classes, depending on their write protect status.. So consolidating both the slot and cartridge into a single image device class makes sense to me.

At the same time, I think that the pre-written cartridges and the EEPROM ones do deserve differentiation beyond whether the user chooses to load a particular image as read-only - which is something you can do with a floppy image - or, say, removing write permissions on the image file: a different file type (.rom/.cart vs .eeprom/.sc32) seems a good indicator of that, for me.

I'm not entirely sure what the best solution here is. Perhaps the code in diimage.cpp around line 994 should remove the file if the call to call_create() returns an error?

[edited to add]

Or perhaps rather, this conditional should also remove any created file?

[cbrunschen]

Also, just to mention some theoretical added complexity that I'm not even really going to bother with: there are aftermarket cartridges that use larger EEPROM chips, and which use an external switch to provide values for the extra address lines - thus resulting in "multi-cartridges", like this SC-2048 that is available to buy for the VFX family. One of these offers the advantage over switching between different cartridges, that you don't wear out the PCB edge connectors or the receptacle in the keyboard, while still having quick access to effectively 64 different cartridges at the flick of a switch.
This might well be better done as a separate image device, though the current device could fairly easily be extended to simply open any file that is a straight multiple of 32 kBytes in size, and allow some way for the user to switch between these on the fly.

[MooglyGuy]

We're getting into territory that I'm not even familiar enough with, and you're asking questions that I can't really give a clear-cut answer to. I'd rather someone more associated with steering MAME's trajectory, like RB, OG, or Vas, to weigh in on the matter.

But at the very end of your follow-up comment, you're describing yet a third type of cartridge, one that could pretty trivially be handled by a slot device, even including device-specific DIPs to select which page is currently visible. If anything, that's an argument in favor of going the slot-device route.

I agree that, fundamentally, if image creation fails for reasons forced by the image device, yet the file for the image was still successfully created, the resulting file should be deleted so as to not crap up the user's hard drive. Beyond that, I genuinely don't have any proverbial dog in this fight, other than to figure out how to get these changes into the driver in the least-unappealing way possible.

You could override is_creatable to prevent image creation, since set_image_filename is called prior to determine_open_plan is called.

...But to me, that's missing the forest for the trees.

If it was just a matter of cartridges that are identical component-wise but not electrically identical (you can't say they're identical; different resistors in different places is still different!), the above exactly how I would do it.

But now you're bringing up that there are aftermarket cartridges.

With a slot device and implementations for individual cartridge types, adding that third type would be easy. If this all only exists in the realm of diimage, that instantly makes life a far larger pain in the ass. And I don't like it when people who want to contribute decent changes to MAME are having a pain in the ass.

To me, it's an argument in favor of going right back with the originally-intended implementation of making the VFX cartridge slot an actual slot device, having and having three cartslot-device-type implementations: One that is a simple ROM loader, one that interfaces with an imagedev class, and one that interfaces with the same imagedev class, but also has input ports so that the user can select which page is presented on the bus.

That third option pushes me strongly in the direction of wanting the old, "too complex," implementation. But I want to hear RB, OG, or Vas weigh in, because the last thing I need in my life is getting my expectations rug-pulled by colleagues.

[cbrunschen]

For reference regarding the (EP)ROM vs EEPROM cartridges, there's for example this blog post about an adapter where you literally drop a chip into a ZIF socket, which refers to this web page where someone converted a ROM to an EEPROM cartridge.

[cbrunschen]

Gathering some thoughts:

• I largely draw parallels between these cartridges and their images, and floppies and their images.
• Like floppies, cartridges come in both writable and read-only varieties, but those are not considered different enough to require different image classes; a single image class handles both.
• The aftermarket multi-cartridges I see as analogous to floppy-drive emulators: they allow you to choose between different storage media (real or virtual) to attach to the system, and yes, people do connect them to the original hardware, but they are also outside the remit of what emulating the system needs to do.
• The cartridge equivalent to the floppy drive is the cartridge slot, which literally just passes through reads and writes, so does not need a separate device class, as was pointed out on the original PR.
• This leads me to the design here: a single cartridge class that represents both the slot and the cartridge in it as loaded from an image, which can be either a writable or a read-only one.
• My little shortcut to allow users to create a write-once image is just that, a shortcut; I consider it a nice-to-have but not important. Users can always create a writable image, and then rename it to make it clear it should be read-only.
• I had clearly misunderstood the is_{read,write,creat}able methods; updating them to look at the currently set image filetype() lets us avoid trying to create read-only images in the first place, and indeed, creating or loading images of any other than the supported filetypes.

[cbrunschen]

Some further investigation leads me to reassess some of the above. As ever, where I'm wrong or have misunderstood something, please point this out so I can learn better.

device_image_int::is_creatable() seems to be called in two different contexts:

• from within device_image_interface::determine_open_plan(), when attempting to open or create an image file; at this point, the image file name has been set
• from [within menu_control_device_image::menu_activated()]e(https://github.com/mamedev/mame/blob/master/src/frontend/mame/ui/imgcntrl.cpp#L256), in order to determine whether or not to show a [create] menu item in the file manager; at this point, the image device either has the name and file type of whatever is currently loaded into the image device, or NULL if there's nothing loaded.

That means that in order to allow creation of images, I need to be able to return true from is_creatable() whenever it's called from menu_control_device_image::menu_activated().

However, there doesn't seem to be a way to reliably detect that. So the best thing I could do is always return true.

That also means I can't check the image's filetype() when is_creatable() is called from device_image_interface::determine_open_plan() to detect whether it is possible to create an image with a particular filename - i.e., I can't reject creation of .rom or .cart images here.

Incidentally, when I tried returning false from is_creatable() when the filetype was not one of the eeprom cartridge ones, MAME would show an error message "Error loading media image: no such file or directory", which was not the most informative.

So I can still reject creation of .rom or .cart images in ensoniq_vfx_cartridge::call_create(), but that then leaves MAME to first create the file, because is_creatable() returned true and allowed MAME to create the file in the first place; but at least here, MAME shows the error message that call_create() returned.

It seems that:

• is_creatable() is a bit overloaded to mean two different things;
• when determine_open_plan() does not find a file that it can successfully open, it results in a very generic error message

Interestingly enough, the latter also applies in another case. MAME's file manager lets me choose any file I want to load, even though I've specified the filetypes that I can handle in ensoniq_vfx_cartridge::file_extensions().

If I try to return false from is_readable() for any file that has the wrong filetype in order to prevent loading non-images, then that again results in a message of "Error loading media image: no such file or directory" which is confusing, because I literally just chose an existing file.

If instead I check the filetype in ensoniq_vfx_cartridge::call_load() then MAME will show the error message returned from call_load().

So in order to give a user experience with the most informative error messages, and generally following the principle of least astonishment, I can't really return false from any of the is_{read,write,creat}able() methods.

• I have to return true from is_readable() to prevent an inaccurate "no such file or directory" error when trying to open an existing file that happens to have the wrong extension.

• I have to return true from is_creatable() to make sure that the [create] menu item always appears.

• Since I can't return false from is_creatable(), I have to do something reasonable for any arbitrary file that I am asked to create, because MAME has already created the file by the time I can do anything else here.

• So even if the file I'm asked to create is technically intended to be read-only - or even, of entirely the wrong filetype - I kind of have to treat it as writable at least once, at least on this occasion, or the user will have a bad experience.

What am I missing? What should I be doing differently?