Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bus/a2bus: Added Vista A800 8inch Disk Controller Card #11885

Merged
merged 2 commits into from Dec 31, 2023

Conversation

robjustice
Copy link
Contributor

Added support for the Vista A800 8" disk controller card.

I picked up a non working one of these cards, so wanted to understand it better. I was able to use the Vista A800 disks and rom available on the Apple2 documentation project and then format and boot DOS 3.3 and CPM ok from the 8 inch disks.

@rb6502
Copy link
Contributor

rb6502 commented Dec 30, 2023

This looks decent at a glance, but our standards have advanced in the last year for slot cards. You can see a full-scale modernization on a NuBus card at 1ff5748 but similar things apply for Apple II and other cards.

The parts that specifically apply here are:

  • Move the class definition into the .cpp and change the DECLARE_DEVICE_TYPE() to use device_a2bus_card_interface as the type instead of the specific class type.
  • Wrap the "guts" in the .cpp in an anonymous namespace.
  • Use DEFINE_DEVICE_TYPE_PRIVATE in the .cpp instead of DEFINE_DEVICE_TYPE.

a2pic.cpp/.h is an example of this modern style for Apple II cards. I will be updating all of the A2 cards to that format in time. The advantage is that it means each card only exports one symbol per card type, which helps the linker with the amount of work it has to do. This isn't as bad on Linux or macOS, but the linker used for MAME on Windows is very slow for whatever reason.

Additionally, boilerplate comments like PARAMETERS and LIVE DEVICE are not considered all that useful anymore, and I personally have fallen out of love with #define-ing the region and device names. In general if you're using required_device and required_region_ptr to look things up, and you should, those aren't all that useful.

@robjustice
Copy link
Contributor Author

No worries, i'll update it. Things are always moving on, hard to keep up. And thanks for explaining it in detail. One other thing i notice in the a2pic, the use of u8,etc instead if uint8_t for types. Is that preferred?

@rb6502
Copy link
Contributor

rb6502 commented Dec 31, 2023

I personally use the shorter u8/u16/u32 types because it's less typing and I'm old enough to where I'm really getting in touch with my inner laziness. But officially MAME allows either as long as you don't mix them in a single file.

@robjustice
Copy link
Contributor Author

i've updated it, let me know how this looks.

@rb6502 rb6502 merged commit 9826239 into mamedev:master Dec 31, 2023
5 checks passed
Comment on lines +241 to +252
case 0xa:
m_dmaenable_read = true;
break;

case 0xb:
m_dmaenable_write = true;
break;

case 0xc:
m_dmaenable_read = false;
m_dmaenable_write = false;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to guard against debugger accesses here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this a bit more, is the guard for the debugger access needed for reads or writes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's typically only required for reads, because memory views can generated multiple unexpected reads and cause unwanted state change. On the other hand, writes from the debugger are often intended to trigger such state changes, and won't happen without an explicit user action.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here for implementation for this device:

uint8_t a2bus_vistaa800_device::read_c0nx(uint8_t offset)

Read this section of the documentation for an explanation from the opposite direction (i.e. from a debugger user’s point of view): https://docs.mamedev.org/debugger/#memory-accesses

In simple terms, debugger memory and disassembly windows generate read accesses in order to display the contents of address spaces as seen by the emulated system. If read accesses have side effects, you need to guard against debugger accesses or simply having debugger windows open will change the state of the emulated system.

einstein95 pushed a commit to einstein95/mame that referenced this pull request Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants