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

HD44780A00, KS0066_F00, SED1278_0B: fix errors in the ROM transcriptions from the datasheets. Add HD44780UA00 and HD44780UA02 variant devices. [Lord Nightmare] #11762

Merged
merged 2 commits into from Nov 21, 2023

Conversation

Lord-Nightmare
Copy link
Contributor

@Lord-Nightmare Lord-Nightmare commented Nov 19, 2023

HD44780A00: fix errors in the ROM transcription from the 1985 datasheet. [Lord Nightmare]
Added HD44780UA00 variant device with ROM transcribed from the 1999 datasheet. [Lord Nightmare]
KS0066_F00: fix errors in the ROM transcription from the datasheet, this ends up being identical to the HD44780A00 [Lord Nightmare]
Added HD44780UA02 variant device with ROM transcribed from the 1999 datasheet. [Lord Nightmare]
SED1278_0B: fix errors in the ROM transcription from the datasheet. [Lord Nightmare]

…ich matches videos of vintage devices). Added HD44780UA00 variant device with rom from 1999 datasheet. [Lord Nightmare]
@Lord-Nightmare Lord-Nightmare requested review from cuavas and happppp and removed request for cuavas November 19, 2023 20:37
@happppp
Copy link
Member

happppp commented Nov 20, 2023

Could you make a decision on either removing U_A02 or having it in? Not a commented-out version of it.

@Lord-Nightmare
Copy link
Contributor Author

I think maybe I should just leave it out entirely. We don't (yet) have anything which uses it, but the rom is done and waiting once we do.

…he international characters from UA02 are required for some strings in cc2schach.

Fix two incorrectly transcribed characters from the Epson SED1278 datasheet. [Lord Nightmare]
@Lord-Nightmare
Copy link
Contributor Author

Lord-Nightmare commented Nov 21, 2023

Scratch that, I proved that the elektor/avrmax cc2schach driver does need the HD44780UA02, so I've fully added it. I also did a cursory check of the SED1278_0B ROM we use vs the datasheet and spotted two incorrect characters, so I've fixed those as well.
(I also added a note about the weird, unusually slow clock that the Hitachi LM044L module, used by Sprow's LCD addon for the bbc micro, uses)

@Lord-Nightmare Lord-Nightmare changed the title Replace HD44780A00 rom with corrected version from 1985 datasheet HD44780A00, KS0066_F00, SED1278_0B: fix errors in the ROM transcriptions from the datasheets. Add HD44780A00 and HD44780A02 variant devices. [Lord Nightmare] Nov 21, 2023
@Lord-Nightmare Lord-Nightmare changed the title HD44780A00, KS0066_F00, SED1278_0B: fix errors in the ROM transcriptions from the datasheets. Add HD44780A00 and HD44780A02 variant devices. [Lord Nightmare] HD44780A00, KS0066_F00, SED1278_0B: fix errors in the ROM transcriptions from the datasheets. Add HD44780UA00 and HD44780UA02 variant devices. [Lord Nightmare] Nov 21, 2023
@happppp happppp merged commit 8064ed2 into mamedev:master Nov 21, 2023
5 checks passed
Comment on lines 147 to 154
switch (m_charset_type)
{
case CHARSET_HD44780_A00: return ROM_NAME( hd44780_a00 );
case CHARSET_HD44780U_A00: return ROM_NAME( hd44780u_a00 );
case CHARSET_HD44780U_A02: return ROM_NAME( hd44780u_a02 );
case CHARSET_SED1278_0B: return ROM_NAME( sed1278_0b );
case CHARSET_KS0066_F00: return ROM_NAME( ks0066_f00 );
case CHARSET_KS0066_F05: return ROM_NAME( ks0066_f05 );
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 really gross. Why aren’t you overriding device_rom_region in each device with a different ROM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the same coding pattern/style that the device already had.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn’t change the fact that it’s completely nonsensical, and it should be pretty obvious there are no other devices doing this.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't Sandro the correct person to direct that to, not LN?

Copy link
Member

Choose a reason for hiding this comment

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

Well in a perfect world, we could go back and ensure it didn’t get to this state over a decade ago. But the way it got into this state was that there was originally a single device type with the ROM selected after it was instantiated at machine configuration time. You couldn’t set device BIOS options from machine configuration back then, so there wasn’t a proper way to select between multiple ROM choices for a single device type, and this is what we ended up with. Of course, back then devices didn’t participate in -validate, -romident, etc. so hackery like that wasn’t as visible to end users.

Anyway, we’re carrying so many of these things that are in a bad state due to accidents of history that we really need to maintain a policy of, “If you encounter something nonsensical, don’t perpetuate it.”

Comment on lines 72 to 82
CHARSET_HD44780_A00,
CHARSET_HD44780U_A00,
CHARSET_HD44780U_A02,
CHARSET_SED1278_0B,
CHARSET_KS0066_F00,
CHARSET_KS0066_F05 /*,
CHARSET_HD44780_A01,
CHARSET_HD44780_A02,
CHARSET_HD44780U_A01,
CHARSET_SED1278_0A,
CHARSET_KS0066_F03,
CHARSET_KS0066_F04,
CHARSET_KS0066_F06,
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, this is only used to implement device_rom_region the wrong way, and should go.

Comment on lines +146 to +162
// ======================> hd44780u_a00_device

class hd44780u_a00_device : public hd44780_device
{
public:
// construction/destruction
hd44780u_a00_device(const machine_config &mconfig, const char *tag, device_t *owner, uint32_t clock);
};

// ======================> hd44780u_a02_device

class hd44780u_a02_device : public hd44780_device
{
public:
// construction/destruction
hd44780u_a02_device(const machine_config &mconfig, const char *tag, device_t *owner, uint32_t clock);
};
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t at least some of these implement parent_rom_device_type?

Copy link
Member

Choose a reason for hiding this comment

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

These 2 new additions each have a unique char rom.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but one of the device seems to have an identical ROM, and making the whole group of devices use a single parent would reduce the overall file count in merged sets for people who are into that kind of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The KS0066_F00 device already existed and already had its own ROM, which WAS different from the hd44780a00 ROM, but this turned out to be an error (the two are actually supposed to be identical). I changed it so (correctly) both devices now use the same ROM, but (to prevent user annoyance) left the two ROMs as separate files.

Copy link
Member

Choose a reason for hiding this comment

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

But it is going to cause user annoyance because they’ll need to update their ks0066_f00.zip now, and it won’t be able to share the HD44780A00’s copy of that ROM because there’s no relationship between the devices.

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

3 participants