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

trs/mc10.cpp: Add "Multiport" cartridge and RAM expansion for the Matra & Hachette Alice #12080

Merged
merged 2 commits into from Feb 29, 2024

Conversation

Mokona
Copy link
Contributor

@Mokona Mokona commented Feb 28, 2024

This addition to the Alice line (4k, 32 and 90) adds RAM and Cartridge support in a different way as the already supported Darren Atkinson's MCX-128 cartridge.

@rb6502 rb6502 changed the title Alice cartridge and RAM extension called "Multiport" trs/mc10.cpp: Add "Multiport" cartridge and RAM expansion for the Matra & Hachette Alice Feb 29, 2024
@rb6502 rb6502 merged commit 5466b7f into mamedev:master Feb 29, 2024
5 checks passed
@happppp
Copy link
Member

happppp commented Feb 29, 2024

@Mokona the new files are missing license header, see other source files to see what I mean.

@@ -38,6 +42,7 @@
#include "mcx128.h"
#include "pak.h"
#include "ram.h"
#include "multiports_ext.h"
Copy link
Member

Choose a reason for hiding this comment

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

These should be kept sorted.

Comment on lines +103 to +106
void mc10_multiports_ext_device::device_post_load()
{
update_bank();
}
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary – bank objects automatically save and restore state.

Comment on lines +117 to +120
void mc10_multiports_ext_device::update_bank()
{
m_bank.target()->set_entry(rom_bank_index);
}
Copy link
Member

Choose a reason for hiding this comment

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

The object finders implement operator-> so you can use natural syntax as though they were pointers. Adding .target() is ugly.

Comment on lines +124 to +130
memory_region *const romregion(memregion("^rom"));
if (!romregion)
{
return std::make_pair(image_error::BADSOFTWARE, "Software item lacks 'rom' data area");
}

m_bank.target()->configure_entries(0, 8, romregion->base(), 0x2000);
Copy link
Member

Choose a reason for hiding this comment

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

There’s no check that the ROM is large enough here – you aren’t catering for ROMs with less than 64KiB, so you need to add a check to ensure the ROM is big enough, or you’ll end up reading off the end of the memory region.

Also, the check that the rom region exists is redundant, because you won’t get here if it isn’t since it’s already checked by the slot device.

Comment on lines +58 to +59

DEFINE_DEVICE_TYPE_PRIVATE(ALICE_MULTIPORTS_EXT, device_mc10cart_interface, mc10_multiports_ext_device, "mc10_multiports_ext", "Fred_72 and 6502man's Multiports Extension")
Copy link
Member

Choose a reason for hiding this comment

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

The idea of this is to put the content of the file in an anonymous namespace to minimise global symbols.

@Mokona
Copy link
Contributor Author

Mokona commented Feb 29, 2024

Thanks for the review. As this was already merged, I will provide another PR soon.

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