-
Notifications
You must be signed in to change notification settings - Fork 2.2k
heathzenith/h8.cpp: Implement H8 Bus #13560
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
base: master
Are you sure you want to change the base?
Conversation
@Robbbert since you have the copyright on h8.cpp, do you have any feedback on these changes? |
@mgarlanger I have involuntarily abandoned this driver, so you can remove my name from it if you wish. The content is now entirely in your hands. |
#pragma once | ||
#include "emu.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad things happen if you #include "emu.h"
from headers – it has to be the first thing in each .cpp file (due to the way we use precompiled prefix headers in our build system).
#include <functional> | ||
#include <utility> | ||
#include <vector> | ||
#include <string.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many of these are actually used in this header? It looks like <utility>
and <functional>
are used, but I can’t see anything from <string.h>
at a glance.
inline void device_h8bus_card_interface::set_slot_int1(int state) | ||
{ | ||
h8bus().set_int1_line(state); | ||
} | ||
|
||
inline void device_h8bus_card_interface::set_slot_int2(int state) | ||
{ | ||
h8bus().set_int2_line(state); | ||
} | ||
|
||
inline void device_h8bus_card_interface::set_slot_int3(int state) | ||
{ | ||
h8bus().set_int3_line(state); | ||
} | ||
|
||
inline void device_h8bus_card_interface::set_slot_int4(int state) | ||
{ | ||
h8bus().set_int4_line(state); | ||
} | ||
|
||
inline void device_h8bus_card_interface::set_slot_int5(int state) | ||
{ | ||
h8bus().set_int5_line(state); | ||
} | ||
|
||
inline void device_h8bus_card_interface::set_slot_int6(int state) | ||
{ | ||
h8bus().set_int6_line(state); | ||
} | ||
|
||
inline void device_h8bus_card_interface::set_slot_int7(int state) | ||
{ | ||
h8bus().set_int7_line(state); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naïve approach to implementing “party line” signals causes issues if multiple cards assert the same signal simultaneously. Consider what would happen with the code as-is if two cards both assert INT2:
- Card 1 asserts INT2, bus sees INT2 asserted (correct)
- Card 2 asserts INT2, bus still sees INT2 asserted (correct)
- Card 1 deasserts INT2, bus sees INT2 deasserted (incorrect, card 2 still has it asserted)
- Card 2 deasserts INT2, bus sees INT2 deasserted (correct)
This is why the INTELLEC 4 bus (src/devices/bus/intellec4) keeps track of which cards have which lines asserted in bit flags. It also facilitates implementing cards that react when any other card asserts a line. I’d suggest doing something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any Heath software that would properly handle shared interrupts. Would it be ok, to add a TODO in this PR about this issue, and address it in a followup PR?
// Verify required cards are installed. | ||
if (!m_p1_card) | ||
{ | ||
fatalerror("Can't find H-8 P1 - Front panel card\n"); | ||
} | ||
|
||
if (!m_p2_card) | ||
{ | ||
fatalerror("Can't find H-8 P2 - CPU card\n"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think this is a great idea – I realise the system won’t do much without these cards, but MAME doesn’t have a nice way of displaying fatal errors when starting a system from the internal UI, or reporting them back to an external launcher. Letting the user discover the system does nothing is the lesser evil IMO (much like removing the video card from a Mac II or IBM PC clone in MAME).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll remove these. Should I also remove the fatalerror() that is in device_h8bus_card_interface::interface_pre_start()
if m_h8bus is not set? Or should it be a throw device_missing_dependencies();
like it is in the intellec4.cpp code?
void h8bus_device::set_int1_line(int state) | ||
{ | ||
m_p2_card->int1_w(state); | ||
} | ||
|
||
void h8bus_device::set_int2_line(int state) | ||
{ | ||
m_p2_card->int2_w(state); | ||
} | ||
|
||
void h8bus_device::set_int3_line(int state) | ||
{ | ||
m_p2_card->int3_w(state); | ||
} | ||
|
||
void h8bus_device::set_int4_line(int state) | ||
{ | ||
m_p2_card->int4_w(state); | ||
} | ||
|
||
void h8bus_device::set_int5_line(int state) | ||
{ | ||
m_p2_card->int5_w(state); | ||
} | ||
|
||
void h8bus_device::set_int6_line(int state) | ||
{ | ||
m_p2_card->int6_w(state); | ||
} | ||
|
||
void h8bus_device::set_int7_line(int state) | ||
{ | ||
m_p2_card->int7_w(state); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember any card on the bus can see when another card asserts one of these, as they’re all wired together. It’s better to use an approach that allows telling all the cards the state they’d see on these lines.
// 2.048 MHz | ||
static constexpr XTAL H8_CLOCK = XTAL(18'432'000) / 9; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a local in the machine configuration function, as it’s only used in one place (it increases static data size if you make it static).
Also, assuming the CPU card drives the bus clock, consider having the CPU card set the bus clock on start so it can propagate to other cards.
|
||
required_device_array<ram_device, 8> m_ram; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you can’t use -ramsize
with these anyway, you’re better off just using memory_share_array_creator
rather than an array of ram_device
.
DEFINE_DEVICE_TYPE_PRIVATE(H8BUS_WH_8_64, device_h8bus_card_interface, wh_8_64_device, "wh8_h_8_64", "Heath WH-8-64 64k Dynamic RAM"); | ||
DEFINE_DEVICE_TYPE_PRIVATE(H8BUS_WH_8_64_48K, device_h8bus_card_interface, wh_8_64_48k_device, "wh8_h_8_64_48k", "Heath WH-8-64 48k Dynamic RAM"); | ||
DEFINE_DEVICE_TYPE_PRIVATE(H8BUS_WH_8_64_32K, device_h8bus_card_interface, wh_8_64_32k_device, "wh8_h_8_64_32k", "Heath WH-8-64 32k Dynamic RAM"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth having separate card devices for the different RAM configurations, or would you be better off just having a machine configuration “switch” to set the amount of RAM present? (For example the MDC4•8/MDC8•24 cards use a fake switch to select the video RAM size.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this prior comment from @pmackinlay - #13040 (comment) about how to handle the m_connected_tlbc, I split that card into 2 slot options. On the other PR it felt cleaner with the 2 cards, but I'm not sure which would be better with this board. And followed that with this PR. But if you want it handled by a machine config switch, I can change it.
@cuavas all comments should be addressed now, including properly handling "party line" signals. |
Implements the H8 bus otherwise known as the Benton Harbor Bus.
This moves all of the H8 functionality from h8.cpp on to separate virtual cards which match up with the real H8's architecture.
Added:
Note: