vfxcart.cpp: add a timer to complete the write cycle.

[cbrunschen]

In the normal course of events, when a VFX family keyboard writes to a cartridge, each byte written is followed by some number of reads to verify that the write to the cartridge's EEPROM completes. vfxcart.cpp currently uses this read operation as a signal that the write cycle is complete.

However, if something interrupts the emulated keyboard's write operation somehow, it is at least theoretically possible that the keyboard might begin another write without an intervening read to complete the previous write cycle.

In a real EEPROM as used in these cartridges, any write cycle is automatically completed after there have been no further writes within a certain time T_BLC, on the order of 100 microseconds.

This adds such a timer. So now a write cycle completes when either this timer expires, or a read happens.

Also, when writing to m_storage, apply MASK to offset.

[happppp]

What EEPROM is this? Is it something compatible with devices/machine/intelfsh? Can't it use one of those instead of a local implementation? It'd make the implementation here a whole lot simpler.
I saw a comment about Xicor X27C256, but 27C256 sounds like EPROM, not EEPROM.

[cbrunschen]

What EEPROM is this?

In the cartridge I have, it's a Xicor X28C256.

Is it something compatible with devices/machine/intelfsh?

Not that I can see; those are all Flash devices, this is an EEPROM, closer to src/devices/machine/at28c64b.cpp but with fairly big differences.

Can't it use one of those instead of a local implementation? It'd make the implementation here a whole lot simpler. I saw a comment about Xicor X27C256, but 27C256 sounds like EPROM, not EEPROM.

Typo: it's a Xicor X28C256. Fixed.

I have a more general implementation of the 28-series EEPROMs that I played with before writing the vfxcart code, which is simplified from the full implemtation because the VFX-family keyboards don't use the full featureset of these EEPROMs. For example, they always enable and never disable write protection, so that's a chunk of functionality that isn't needed. So the vfxcart implementation is simplified from a full X28C256 implementation.

[MooglyGuy]

Would you be open to creating a separate X28C256 implementation, while making unsupported features do a fatalerror(), rather than having a limited implementation within vfxcart itself?

Having a partial implementation of a device is fine, just as long as the device has stubs for the unsupported stuff and bails out with a relevant message if something tickles the unsupported things.

In some hypothetical future where someone is emulating a machine that had a X28C256 and uses more of the features, they'll reach for the datasheet and start creating a device, not knowing that there's already a partial implementation in vfxcart that could be extracted out.

Device duplication is already an issue across the MAME codebase, so even if you don't need to implement features unused by the VFX - and it's important to not implement untestable aspects of the device - it would be a really helpful thing to make a separate device for the EEPROM itself.

[cbrunschen]

Would you be open to creating a separate X28C256 implementation, while making unsupported features do a fatalerror(), rather than having a limited implementation within vfxcart itself?

Well, I have a more complete m_28_size EEPROM implementation (with unit tests) outside of MAME at https://github.com/cbrunschen/eeprom28 ; I was experimenting with this when I wrote vfxcart, and then wrote vfxcart as a simplified bespoke version because so much of the complexity was not needed.

A complicating issue is that existing EEPROM devices (like at28c64b.cpp) and the similar intelflsh Flash devices are device_nvram_interface devices, which are specifically backed by .nv files, whereas what I have is a device_image_interface device with its own storage files (.crt, .sc32, etc) that are loaded and unloaded on demand.

To implement this in a more general fashion that I could also use in vfxcart would require at least a two-level split - into a host (write-protect-protocol etc) front-end, and a persistent-storage backend - so that someone who is using this as an EEPROM like other EEPROMs are used as nvram devices can do so, but the vfxcart can still use it with its own device_image_interface storage.

Considering that existing EEPROM devices do not have that separation, this would go rather against existing code patterns, and might rightly be considered unnecessary complexity.

Having a partial implementation of a device is fine, just as long as the device has stubs for the unsupported stuff and bails out with a relevant message if something tickles the unsupported things.

In some hypothetical future where someone is emulating a machine that had a X28C256 and uses more of the features, they'll reach for the datasheet and start creating a device, not knowing that there's already a partial implementation in vfxcart that could be extracted out.

Device duplication is already an issue across the MAME codebase, so even if you don't need to implement features unused by the VFX - and it's important to not implement untestable aspects of the device - it would be a really helpful thing to make a separate device for the EEPROM itself.

All those are great points, but as they involve more substantial changes I think are perhaps better done longer-term, not in this PR.

[rb6502]

It wouldn't be super difficult to separate. You'd have a base class with the actual 28C256 behavior that just saves to a buffer and then an nvram version that inherits device_nvram_interface and adds the necessary nvram_default/read/write overrides. vfxcart then would just inherit the base class and do its own I/O.

[cbrunschen]

Now a much bigger change instead of a small bug fix.

[cbrunschen]

Now a much bigger change instead of a small bug fix.

To expand a little:

This PR now implements the Xicor X28-series EEPROMs, with the full write protection protocol and timings. This is a template, with timings and sizes adjustable to suit. All is based on my own code, where there is also a suite of unit tests using Catch2.

Seven of these - X28C64, X28C256, X28HC256, X28C512, X28C010, XM28C020 and XM28C040 - are implemented as in their respective datasheets.

One (which I call X28F256) is based on the real X28C256 but with an immediate write cycle.

I use this X28F256 in vfxcart.cpp to implement the same functionality as in the initial version of this PR: a cartridge that does not need the keyboard to wait excessively for each byte to finish being written.

These do not currently implement device_memory_interface or device_nvram_interface, because I'm not using either of those.

You may note that among the datasheets I've also included an OnSemi CAT28C256; this seems to behave almost exactly like an X28C256.

There are also datasheets for a couple of [Atmel AT28 EEPROMS, AT28C256 and AT28C040. That's because the same write-protection protocol is also used in Atmel's AT28 series of EEPROMs, and those can be used as drop-in replacement for Xicor X28 EEPROMs.

But Atmel's AT28 EEPROMs have an additional page of data outside the normal address range of the EEPROM, which is accessed by raising the A9 pin to +12V, which then makes that page available as the top page_size bytes within the EEPROM's address range. This code does not currently implement that. But if a board contains an Atmel AT28 EEPROM, but never raises A9 to +12V and thus never accesses those identification bytes, then an X28 series EEPROM can be used as a substitute.

[MooglyGuy]

Do each of these additional models only vary based on parameters that are reasonably consistent across the device line?

My line of thinking was just to create a solitary X28C256 device and let whoever else needs any variants implement those themselves, but I can get that there's probably a lot of commonality that only differs on word-width and things like that.

[MooglyGuy]

No feedback on these changes other than that they're great and should be a template for the sort of code quality that other external contributors should strive towards. I want an additional pair of eyes via @cuavas or @happppp or @galibert, but this looks really great.

[cbrunschen]

Do each of these additional models only vary based on parameters that are reasonably consistent across the device line?

Yes, they really do, and they really are.

I've provided links to the datasheets for these devices; they're not long, so please do have a look.

I have read them all, and they read almost identical to each other - except for the sizes and timings. It's almost as if the manufacturer had a pattern that they followed, that even as they added new devices and their sizes increased, within the same product family (X28) they maintained compatibility with existing hardware and software.

(And at the top end, the XM28C020 is literally made by combining 4 X28C513:s (siblings to the X28C512, they're on the same datasheet), and the XM28C040 is 4 X28C010:s, so aside from the size, those share the protocol and timings because it's literally just multiplexing 4 other devices together.)

In fact the write-protection protocol and T_BLC_ and T_WC_ timing that are implemented here apply seemingly identically not just to Xicor devices, but to OnSemi CAT28 and Atmel AT28 devices as well (with the Atmel AT28 devices having additional capabilities) - again, as if there was recognition that '28' series EEPROMs behaving sufficiently similarly across different manufacturers might be useful for these companies to compete for the same customers by offering compatible devices.

My line of thinking was just to create a solitary X28C256 device and let whoever else needs any variants implement those themselves, but I can get that there's probably a lot of commonality that only differs on word-width and things like that.

Word width across these devices actually remains the same 8 bits. But the overall size, page size, and speeds (especially T_WC_, the time taken for the Write Cycle - for example. X28C devices tend to have 5ms <= T_WC_ <= 10ms whereas X28HC devices 3ms <= T_WC_ <= 5ms) do differ.

The "more complete implementation" that I had pointed to already contains the template code. So it would have been more work to reduce this down to a single device.

And the commonalities here are so strong that implementing just one device, and eventually perhaps leading to a number of devices with essentially the same behaviour and command set - differing only really in the specific sizes and timings - as separate classes (with possible separate bugs!) seems like precisely the kind of code duplication that you said is an issue in MAME, and to be avoided.

Having a template that can be instantiated for the specific sizes & timings seems a solid solution, including allowing for the testing to happen across different combinations of parameters.

[MooglyGuy]

If there are any further follow-ups that are necessary, contact me and I will perform the necessary rework.