-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add internal layout panels for the VFX family of keyboards (vfx, vfxsd, sd1 and sd132). #14358
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
f3de5d0
to
c0553ba
Compare
src/mame/ensoniq/esqpanel.cpp
Outdated
#define VERBOSE 0 | ||
#include "logmacro.h" | ||
|
||
template <typename Format, typename... Params> inline static void logerror(Format &&fmt, Params &&... args) { |
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.
what?
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.
Indeed, removed.
src/mame/ensoniq/esqvfd.cpp
Outdated
* led14seg. Bit 14 activates the dot after a character; bit 15 the underline. | ||
* See layout/{vfx|vfxsd|sd1}.lay . | ||
*/ | ||
static const uint16_t font_vfx[] = { |
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.
tradition by now is to make that a device 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.
OK - does that mean as a ROM file to go along with the dumped ROMs, Or something else? Could you point me to an example?
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 sin table from swp30.cpp for instance? Or the font in mulcd.cpp.
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.
Like this?
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.
Looks good
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 tried to do the same thing with the existing font in there; but that resulted in every variant needing its own copy of that ROM file. That does work, but the duplication of that ROM seems a bit suboptimal. Also, the "esq2x40_device" doesn't seem to be used anywhere; instead there's an "esq2x16_device" in a separate file, esqlcd.cpp, which nevertheless inherits from esqvfd_device, but which clearly does not need the VFD font. I think that's a situation that warrants sorting out before trying to move the existing font into a ROM file.
src/mame/ensoniq/esqpanel.cpp
Outdated
ESQ2X40(config, m_vfd, 60); | ||
ESQ2X40_VFX(config, m_vfd, 60); | ||
|
||
const std::string &name = owner()->shortname(); |
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.
switching on a name of something elsewhere has often been problematic over time. Could you switch that to a config method called at the upper level in machinf config?
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 now passing the "panel type" in when I add the panel in esq5505_device's various configuration methods; is that better?
I also tried just using set_default_layout()
in the esq5505_device's various configuration methods, but then the layout tries to find its input ports etc directly in that device rather than in the panel where they actually live (on the real devices, the buttons are connected to the display panel board, not to the main CPU board).
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.
Yep, passing though the constructor is a good method. You're not going to break it by casually changing a device or driver name that way.
src/mame/ensoniq/esqpanel.cpp
Outdated
else if (m_panel_type == VFX_SD) | ||
config.set_default_layout(layout_vfxsd); | ||
else if (m_panel_type == SD_1 || m_panel_type == SD_1_32) | ||
config.set_default_layout(layout_sd1); |
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.
something went wrong with the indentation just there
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.
Yes, some spaces snuck in instead of a tab.
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 always run the MAME srcclean tool on files before I commit them. It helps normalize tabs/spaces and everything.
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 shall do that!
…d, sd1 and sd132).
esqpenal.{h,cpp}: Remove errant logerror function. Pass the panel type into the esqpanel2x40_vfx_device constructor, and use that to specify the layout to use. esq5505.cpp: in the various keyboard configuration methods in the VFX family, pass the correct panel type. Also move the cartridge and panel from common() to vfx(), since the eps family for example use common() but have no cartridge, and a different panel which they were replacing. Also add a fallback layout with just the VFD, for those devices that are currently using the esqpanel2x40_vfx_device panel even though they're not actually in the VFX family.
esq2x40_vfx_device::esq2x40_vfx_device(const machine_config &mconfig, const char *tag, device_t *owner, uint32_t clock) : | ||
esq2x40_device(mconfig, ESQ2X40_VFX, tag, owner, clock) | ||
{ | ||
} |
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.
missing blank 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.
Should be fixed
1. Fix indentation in esqpanel.cpp 2. Move the VFX-style VFD font to a ROM file
383827d
to
52d20fe
Compare
Looks good to my eyes, but I leave it up to @galibert to merge since he's been doing the review. :) |
I'm waiting for the CI to pass, it gets reset with each push. |
It's definitely better to make sure it's all good. CI and such are there for a reason! |
Thank you! |
Thanks, @cbrunschen! |
This includes implementing some more display commands that are used on the VFX family, and correcting edge-of-screen behaviour to match what is observed there.
Also adds underlines to the 1x22 character VFD, for consistency with the 2x40 displays.