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

hp/hp98x6.cpp: Support 3 new machines #12599

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Conversation

fulivi
Copy link
Contributor

@fulivi fulivi commented Jul 20, 2024

Hi,
this PR adds the emulation of hp9826a, 9836a & 9836c machines (of the hp98x6 family).
I'll e-mail the relevant ROM images shortly.
Thanks.
--F.Ulivi

@MooglyGuy
Copy link
Contributor

Had a quick scroll through it. Looks pretty good to me, just a few feedback points that popped into my head, but I don't feel strongly enough about them that devs with commit access should hold things up about it:

  • Consider turning verbose logging off. Helps with performance.
  • Some of the multi-line pieces of code that have been added seem like they're being word-wrapped strangely early. Line 440 of hp98x6_upi.cpp is a good example, the word-wrap point isn't even close to the midpoint of my monitor. That said, I don't know what your own editor setup is like, so I don't feel strongly about it.
  • I notice that the ID PROM is an optional device, but you now have a class structure where there is a base class. Would it be feasible to make it a required device, and have it in only the derived classes that need it?

Comment on lines 127 to 131
class hp98x6_base_state : public driver_device
{
public:
hp9816_state(const machine_config &mconfig, device_type type, const char *tag)
hp98x6_base_state(const machine_config &mconfig, device_type type, const char *tag)
: driver_device(mconfig, type, tag)
Copy link
Member

Choose a reason for hiding this comment

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

If this is now an abstract base class, the constructor should be protected.

Comment on lines 1018 to 1019
floppy_image_device *drives[2] = { get_drive(0), get_drive(1) };
floppy_image_device *new_floppy = drives[get_sel_floppy()];
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn’t it be simpler to just do:

	floppy_image_device *const new_floppy = get_drive(get_sel_floppy());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely, that was a relic from an early implementation

src/mame/hp/hp98x6.cpp Outdated Show resolved Hide resolved
src/mame/hp/hp98x6_upi.h Outdated Show resolved Hide resolved
@rb6502 rb6502 changed the title HP9826A, HP9836A, HP9836C: new working machines added hp/hp98x6.cpp: Support 3 new machines Jul 26, 2024
@rb6502 rb6502 merged commit 8472b08 into mamedev:master Jul 26, 2024
5 checks passed
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.

4 participants