Technics keyboards - initial skeleton

[felipesanches]

No description provided.

[felipesanches]

This is broken due to usage of outdated VGA-related methods. I will need to fix it later. Maybe this weekend. Until then, I'll keep this marked as "draft".

[cuavas]

There’s a lot wrong with this.

• It can’t have been built against mainline code for months, and won’t compile.
• It breaks building unidasm.
• There’s code that looks like it could really be simplified with arrays and member function templates.
• There’s a lot of unnecessary macro use. Macros that don’t actually make things clearer are unhelpful. Macros should be avoided if enum, constexpr, etc. can be used instead.
• Implementation details aren’t kept in anonymous namespaces.
• Formatting is wildly inconsistent in some of the source files.

At the very least, you should make sure your code builds against latest upstream before opening a pull request.

[cuavas]

Please use snake_case for member names, and visually separate instance members from static members.

Also, since you’re creating an instance anyway, would it be worth processing the SFR names into a std::vector of std::string_view or something on construction to make it easier to work with?

[felipesanches]

At the very least, you should make sure your code builds against latest upstream before opening a pull request.

Sorry about that! My work-in-progress code was building OK locally, but it was based on a many-months old codebase. During a conversation with another mamedev person who offered some help, I was asked to submit a PR as soon as possible. In order to do so, I git rebased my code with latest master branch and since there were no code-conflicts to fix, I was silly to forget to rebuild before pushing the code and opening the PR. I know I had no reason to believe the code was good just because it had no conflicts, but I was a bit distracted by the ongoing conversation. As soon as I noticed that the code was actually not building correctly, I quickly marked this PR as a "draft" and sent a message here (#11835 (comment)) informing that I planned to get back to it very soon. Your code-review was then posted here 12 minutes later.

Today was the day I was able to dedicate some more time to MAME, and I am now trying to address all problems pointed out here.

* It can’t have been built against mainline code for months, and won’t compile.

I've just rebased with current git master again and pushed additional commits addressing the broken build.

* It breaks building unidasm.

• I've now fixed the unidasm build as well.

* There’s code that looks like it could really be simplified with arrays and member function templates.

Can you please mention explicitly which code is that? (maybe by referencing the source file and the initial line-number where such code is). It would be useful if you could also point to other portions of the code-base that have the coding approach you are suggesting, or provide an example of what you mean with the proposed usage of "arrays and member function templates", because I did not understand that comment.

* There’s a lot of unnecessary macro use.  Macros that don’t actually make things clearer are unhelpful.  Macros should be avoided if `enum`, `constexpr`, etc. can be used instead.

Would you like to make a list of the macros that you want me to delete?

I know that there are also other things to be addressed based on your code-review, so I'll keep pushing more commits later here.

[felipesanches]

This is ready for another code-review.

[angelosa]

Couple things that needs to be addressed here:

1. Right now kn5000 will segfault without anything attached to the bus. This is unacceptable for production code.
2. (If the hdae5000 is attached) it will try to draw by reading $80'0000-$9f'ffff areas. I haven't investigated in great details but I sense it's accessing an unavailable ROM here, at worst it needs to be addressed with a NO_DUMP / ROMREGION_ERASEFF region for now.

[felipesanches]

I'd like to ask for some code-review / feedback on the changes made by me on fffecba

I know that this is the commit that introduced the segfault. And I admit that commit was written based on a bit of guesswork and adapting snippets of other drivers, but I lack a full understanding of the concepts & mechanisms used here for optionally attaching ROM and RAM from a card on an extension interface. Also, I know that I will have to not only fix the segfault, but also improve this mapping to also include I/O with the hard disk controller and the PPI that are also in this extension board. And there will also be some interrupt lines, I think. But I am not sure yet how to implement any of those things correctly. So, I'd appreciate some guidance here to grasp the concepts and techniques employed on the MAME codebase for that sort of task.

[felipesanches]

I'd like to ask for some code-review / feedback on the changes made by me on fffecba

I know that this is the commit that introduced the segfault. And I admit that commit was written based on a bit of guesswork and adapting snippets of other drivers, but I lack a full understanding of the concepts & mechanisms used here for optionally attaching ROM and RAM from a card on an extension interface. Also, I know that I will have to not only fix the segfault, but also improve this mapping to also include I/O with the hard disk controller and the PPI that are also in this extension board. And there will also be some interrupt lines, I think. But I am not sure yet how to implement any of those things correctly. So, I'd appreciate some guidance here to grasp the concepts and techniques employed on the MAME codebase for that sort of task.

This is the segfault:

[felipesanches]

I do this on the memory map:

	map(0x200000, 0x2fffff).view(m_extension_view);
	m_extension_view[0](0x200000, 0x27ffff).ram(); //optional hsram: 2 * 256k bytes Static RAM @ IC5, IC6 (CS5)
	m_extension_view[0](0x280000, 0x2fffff).rom(); // 512k bytes FLASH ROM @ IC4 (CS5)

Then I do this on machine_start:

void kn5000_state::machine_start()
{
	if(m_extension)
	{
		m_extension->rom_map(m_extension_view[0], 0x280000, 0x2fffff);
		m_extension_view.select(0);
	}
	else
	{
		m_extension_view.disable();
	}
}

This is the rom_map method on the interface:

void kn5000_extension_device::rom_map(address_space_installer &space, offs_t start, offs_t end)
{
	auto dev = get_card_device();
	if(dev)
		space.install_device(start, end, *dev, &kn5000_extension_interface::rom_map);
}

And this is the declaration of HDAE5000 rom-set and rom_map:

ROM_START(hdae5000)
	ROM_REGION16_LE(0x80000, "rom" , 0)
	ROM_DEFAULT_BIOS("v2.01i")

	ROM_SYSTEM_BIOS(0, "v1.10i", "Version 1.10i - July 6th, 1998")
	ROMX_LOAD("hd-ae5000_v1_10i.ic4", 0x000000, 0x80000, CRC(7461374b) SHA1(6019f3c28b6277730418974dde4dc6893fced00e), ROM_BIOS(0))

	ROM_SYSTEM_BIOS(1, "v1.15i", "Version 1.15i - October 13th, 1998")
	ROMX_LOAD("hd-ae5000_v1_15i.ic4", 0x000000, 0x80000, CRC(e76d4b9f) SHA1(581fa58e2cd6fe381cfc312c73771d25ff2e662c), ROM_BIOS(1))

	// Version 2.01i is described as having "additions like lyrics display etc."
	ROM_SYSTEM_BIOS(2, "v2.01i", "Version 2.01i - January 15th, 1999") // installation file indicated "v2.0i" but signature inside the ROM is "v2.01i"
	ROMX_LOAD("hd-ae5000_v2_01i.ic4", 0x000000, 0x80000, CRC(961e6dcd) SHA1(0160c17baa7b026771872126d8146038a19ef53b), ROM_BIOS(2))
ROM_END

void hdae5000_device::rom_map(address_map &map)
{
	map(0x00000, 0x7ffff).rom().region(m_rom, 0);
}

With this, the driver runs properly with the HDAE5000 board selected, but segfaults when the driver is run without the extension board, when the boot code tries to detect its presence by reading from its rom address range (at address 0x00283232).

I may have misunderstood how one is supposed to use a memory_view to achieve the expected results.

[galibert]

That use of views looks correct to me. Did you try to gdb to see where it crashes?

[felipesanches]

That use of views looks correct to me. Did you try to gdb to see where it crashes?

yes, that's the screenshot at #11835 (comment)

[felipesanches]

Last year (as seen on the message log above) I was trying to fix the segfault but I was unable to figure it out. Then I stopped working on this driver.

Now I want to dedicate some more time to this driver, but I'd like to first address any pending issue on the existing code of this pull request, that is roughly 3 years old.

In addition to the several commits addressing issues pointed out last December, I have now also made an additional one disabling support for the HDAE5000 extension board that I was unable to fix. With this, we won't get any SegFault, which would be a deal-breaker for merging this. Proper support for that feature will require further research/debugging.

I have also git-rebased this PR to the latest master branch and I have checked that it builds and runs as expected, even though these are still a couple of non-working drivers that will need further development in the future.

[felipesanches]

Testing with this command line:
./technics -rp ~/ROM_DUMPS/FSanches/ kn5000 -extension hdae5000 -artpath ~/devel/github_felipesanches/KN5000_MAME_artwork/

Artwork layout (drawn by myself) is available at: https://github.com/ArqueologiaDigital/KN5000_MAME_artwork/

And the emulator boots up until it reaches this half-gray screen and turns on the correct LEDs in the control panel:

Please let me know if there's anything else that still needs any improvement before a merge.

[JWatersMeet]

Beautiful work on the layout.

[felipesanches]

I noticed that this PR was missing an internal .lay file, so I prepared one. It is not as beautiful as the external artwork.

It looks like this:

[felipesanches]

During the weekend, I made further improvements to the internal layout, adding all labels and a few line drawings to the control panel:

[felipesanches]

I now consider this ready for merge(feel free to squash it all into a single commit). In the past 6 months, this was reviewed by @MooglyGuy, @cuavas and @angelosa. And I tried to address everything that was pointed out. Please let me know if there's anything else still needed here.

Naturally, additional research will still need to be done after merge for further improvements to the emulation.

[felipesanches]

This continues at PR #12684 with:

• all commits squashed
• rebased, since a minimal kn5000 driver was recently merged
• white-space cleaned up
• And the kn1500 driver temporarily removed.

[ajrhacker]

I can't test this theory now, but I suspect the segfault is caused by putting rom() in the view map without specifying which region to bind to. This practice is still allowed but quasi-deprecated, and perhaps its interaction with memory views wasn't fully tested.