-
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
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. |
Comments have been addressed, can I get another review? |
ping... |
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.
Completed my review on this. I think it's in pretty good shape and will move to merge this if my feedback is addressed.
// default mem access to h8bus | ||
map(0x0000, 0xffff).rw(FUNC(h_8_cpu_8080_device::bus_mem_r), FUNC(h_8_cpu_8080_device::bus_mem_w)); | ||
|
||
map(0x0000, 0x0fff).rom(); // main rom |
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.
These rom()
handlers don't reference any regions?
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 believe it's the maincpu rom defined below, without this line, they system does not come up.
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 suggesting you remove it, I'm suggesting you make it explicit about what region it's referring to. Please just add the relevant region()
call. There's a second rom()
without region()
just after this one.
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.
The mapping for the FDC RAM and ROM are not present on this board, so I'm going to remove them. My plan was to keep the H8 functionality identical to the pre-BUS implementation, but since the hard-sectored floppy controller is not implemented, this doesn't provide anything. It will be added back once the hard-sectored controller card is implemented.
|
||
if (keyin) | ||
{ | ||
for (int i = 1; i < 8; i++) |
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.
Bit 0 is not being scanned?
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 the gemini analysis above, "Key 0 is handled implicitly if no other keys (1-7) are pressed."
{ | ||
if (BIT(keyin, i)) | ||
{ | ||
data &= ~(i << 5); |
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 does this work when data
is only u8
?
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
is at most 7, which still fits in the u8 when shifted by 5.
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 mis-read the encoding step (too used to assuming 1 << n
). I accept it as-is; can you confirm the logic matches hardware when more than one key is pressed within the same row? For example, if keys 1 and 2 are both pressed in X0
, the encoded result should be 0xf8
(which is the same as if key 3 is pressed)?
|
||
if (keyin) | ||
{ | ||
for (int i = 1; i < 8; i++) |
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 really necessary to iterate the bits instead of just bitwise combining the port value with data
?
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.
All the key scanning logic is just lifted from the existing h8.cpp code - https://github.com/mamedev/mame/blob/master/src/mame/heathzenith/h8.cpp#L163-L181. I didn't fully understand the logic, so I asked gemini ai, if it could be simplified and it responded with:
The highlighted code reads the state of a row of keys and encodes the pressed key(s) into the data variable. The current implementation is compact and directly manipulates data within the loop.
Here's how the original code works:
1. keyin = m_io_keyboard[0]->read() ^ 0xff;: Reads the 8-bit keyboard port for row 0 and inverts the bits. So, a pressed key (which is active low) results in a '1' in the corresponding bit of keyin.
2. if (keyin): Checks if any key in this row is pressed.
3. for (int i = 1; i < 8; i++): This loop iterates for keys 1 through 7. Key 0 is handled implicitly if no other keys (1-7) are pressed.
4. if (BIT(keyin, i)): If key i (where i is the numeric value 1-7) is pressed.
5. data &= ~(i << 1);: This is the encoding step. It clears specific bits in data based on the value of i. For example:
- If key 1 is pressed, i=1. data is ANDed with ~(1 << 1) which is ~0x02. Bit 1 of data is cleared.
- If key 3 is pressed, i=3. data is ANDed with ~(3 << 1) which is ~0x06. Bits 1 and 2 of data are cleared.
- If multiple keys are pressed (e.g., 1 and 2), data is first ANDed with ~0x02, then the result is ANDed with ~0x04. This is equivalent to data & ~((1<<1) | (2<<1)), which means if keys k1, k2, ... are pressed, the effective encoded value is k1 | k2 | .... The bits ( (k1|k2|...) << 1) are cleared in data.
6. data &= 0xfe;: This clears bit 0 of data if any key in the row was pressed. This bit acts as a "key pressed in this row" indicator.
The logic is sound and correctly reflects how a simple keyboard matrix without a dedicated encoder might behave, including how multiple key presses are combined.
An alternative way to write this, which might be considered "simpler" by some due to increased explicitness, is to first determine the combined "effective key value" from keys 1-7, and then apply the encoding.
Because the suggested code was longer and didn't simplify. I kept the existing logic with just minor cosmetic changes.
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 going to consider or trust an AI-generated analysis of this code. Please review it carefully yourself and assess if it's correct or not.
{ | ||
m_device_list.emplace_back(card); | ||
char* endptr; | ||
long slot_num = strtol(&card.m_h8bus_slottag[1], &endptr, 10) - 1; |
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.
Not a fan of depending on specific tag values, and not sure it's necessary here. Have you considered counting interrupt assert/deassert instead of tracking state bitwise by slot? While it does mean each card needs to be careful to only output its interrupt state when it changes, my opinion is that it more closely represents what's really happening with a wired-or signal like this. That is, the state of the bus line only changes when the first card asserts or last card deasserts the line.
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.
My concern with that, is that the entire system behaving properly depends on every add-on card doing the right thing with the interrupt line. Any card doing an extra assert or clear, and the system crashes.
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 bit-wise approach for the interrupts is also what Vas suggested on his comment - #13560 (comment)
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.
The bigger issue about not depending on the tags is still there; please look at how the intellec4 code implements this approach and do that instead.
|
||
#include "h_8_1.h" | ||
|
||
#include "machine/ram.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.
This isn't being used any more.
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.
👍 removed.
|
||
#include "wh_8_64.h" | ||
|
||
#include "machine/ram.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.
No longer required.
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.
👍 removed.
|
||
LOGINIT("%s: added card to slot %d, tag: %s\n", FUNCNAME, slot_num, card.m_h8bus_slottag); | ||
|
||
if (strcmp(card.m_h8bus_slottag, "p1") == 0) |
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.
Without looking deeply into the details, can this rather be done through a derived slot class (same for p2)?
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 sure how that would work. If there is an example you could point me to that would help.
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 have the time to look for examples; you have access to the same code as everyone else, so you need to help yourself.
protected: | ||
device_h8bus_p1_card_interface(const machine_config &mconfig, device_t &device); | ||
|
||
void set_p201_reset(int 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.
Is it possible to use a finder in the driver to locate the p1 and p2 cards and hook these signals card-to-card instead of via the bus?
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.
The P1 and P2 cards are inter-connected by the P201 cable, I thought it would be simplest to have it as part of the h8bus, but I can look at using finders and connecting them directly 🤔 .
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 is the point; they're connected, but not via the bus. If you can connect them directly it would make the bus code cleaner.
void mem_map(address_map &map) ATTR_COLD; | ||
void io_map(address_map &map) ATTR_COLD; | ||
|
||
void bus_mem_w(offs_t offset, u8 data) { h8bus().space(AS_PROGRAM).write_byte(offset, data); } |
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.
It's preferred to use memory_space::specific
instead of looking up the space each time.
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 think I found an example of that in altos586.cpp. Is that similar to how I should do it?
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.
Again, please do your own homework. You can search for any use of memory_access::specific
in the code base and see how it works and then test your own implementation.
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.
Just to be clear, your initial comment said memory_space::specific
, since I didn't find anything with memory_space
, but did see a memory_access::specific
used in the altos586 file, I wanted to make sure that was what you were referring to.
And related to asking for examples to look at, I do search for examples myself, but the issue that has come up more than once, is that I find something, but I'm not aware it is an outdated/bad approach, so once I'm done coding/testing/etc. and put up the PR, I get told it needs to totally change, and I've just wasted all that time. I also thought you may have worked on something similar and could identify the code using the current best practices without having to search for it.
I'll work on addressing your comments.
@pmackinlay Still not sure how to address #13560 (comment) and #13560 (comment), but the other comments should be addressed either by code changes, or reason why I went with that approach. |
Here's an attempt to give more step-by-step instructions on how to untangle the "p201" from the bus.
void h8_state::device_config_complete()
{
device_h8bus_p1_card_interface *p1 = dynamic_cast<device_h8bus_p1_card_interface *>(m_p1.lookup()->get_card_device());
device_h8bus_p2_card_interface *p2 = dynamic_cast<device_h8bus_p2_card_interface *>(m_p2.lookup()->get_card_device());
p1->p201_reset().set(*p2, FUNC(device_h8bus_p2_card_interface::p201_reset_w));
} (add more lines and error checking as needed). This will remove the p201 stuff from your bus, and connect the p1 and p2 cards together separately, which I think better reflects the actual hardware. Are the "p1" and "p2" slots themselves actually special/fixed, or could you theoretically install the front panel and CPU cards in different slots and still hook them together via this cable? If the latter, you might need a more elaborate way to identify which if any slots contain a "p1" or "p2" device, but it should be possible to do it by iterating the slots and looking for the relevant interfaces. |
@pmackinlay thanks for the detailed instructions, with my latest commit, I think I've addressed everything. Let me know if I missed anything. |
ping - any chance we can get this wrapped up before the release branch is cut? |
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.
Mostly minor feedback this time; I think moving the memory_access:specific
instances to the card which uses them is the most significant and if that's resolved I will merge this.
m_maincpu->set_addrmap(AS_IO, &h_8_cpu_8080_device::io_map); | ||
m_maincpu->out_status_func().set(FUNC(h_8_cpu_8080_device::h8_status_callback)); | ||
m_maincpu->out_inte_func().set(FUNC(h_8_cpu_8080_device::h8_inte_callback)); | ||
m_maincpu->set_irq_acknowledge_callback("intr_socket", FUNC(heath_intr_socket::irq_callback)); |
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 think you can use the m_intr_socket
finder here instead of the literal tag.
address_space_config const m_mem_config; | ||
address_space_config const m_io_config; | ||
|
||
memory_access<16, 0, 0, ENDIANNESS_LITTLE>::specific m_mem; |
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 think these should be removed from the bus device and added to the card(s) that use them, in this case only the CPU card.
} | ||
} | ||
|
||
device_p201_p1_card_interface::device_p201_p1_card_interface(device_t &device, device_type type, const char *tag) |
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.
The device_type type
argument does not appear to be used or required here (or on device_p201_p2_card_interface
).
|
||
class device_h8bus_card_interface : public device_interface | ||
{ | ||
friend class h8bus_device; |
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 this friend
actually necessary?
|
||
class h8bus_device : public device_t, public device_memory_interface | ||
{ | ||
friend class h8bus_slot_device; |
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.
Same here - are these friend
declarations required?
{ | ||
m_cass_data[3]++; | ||
auto p1_lookup = m_p1.lookup()->get_card_device(); |
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.
Suggest adding a comment to explain this code represents connecting the p1 and p2 cards through a cable which is separate from the backplane. This is obvious to you but might help a future developer understand why it's done this way.
…d other minor cleanup
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: