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

rm/rm380z.cpp: Use real character ROM for COS 4.0, correct screen resolution and add support for user defined characters #12031

Merged
merged 8 commits into from
Feb 17, 2024

Conversation

RobinSergeant
Copy link
Contributor

I've managed to obtain a dump of the real character ROM for COS 4.0 courtesy of the Research Machines user group, and this PR pulls this in and updates the display code to use it.

What is the correct process for updating a rom set? This my working version that includes the new character rom:

rm380z.zip

The real characters are actually larger than the fake set we used before (8x10 rather then 5x9) and even include 2 extra rows to be used when the underline attribute is set. This all makes the display handling much easier and improves the display greatly.

So with these characters 80 column mode has a display resolution of 640x240 and 40 column mode uses 320x240.

For COS 3.4 I've set the display resolution to 240x240 which I think is correct because COS 3.0 only supported the VDU40 adaptor which uses the TI 74LS262 character ROM. These characters are 5x9, padded to 6x10 for display. It's a shame we don't have the real ROM for this as well, but at the moment I'm concentrating on getting COS 4.0 to work properly anyway!

The new ROM only includes characters 0x00 to 0x79 because the additional teletext characters (0x80 to 0xFF) are actually generated by COS as user defined characters. I've also added support for user defined characters so that these continue to work.

This PR includes all my changes for HW scrolling from the last PR which is still in review (#12014), but I don't want to close that until the review comment has been cleared. I can do any further re-work under this PR.

@RobinSergeant
Copy link
Contributor Author

Just fixed a logic error with my underline attribute check. I was only swapping the last row (instead of the last two rows) when underlining. Visually it made no difference as only the last row is normally different, but still a bug! Everything should be ok now and this is ready for review.

Comment on lines 32 to 56
template <int ROWS, int COLS>
class rm380z_vram
{
public:
void set_char(int row, int col, uint8_t data) { m_chars[get_row(row)][col] = data; }
void set_attrib(int row, int col, uint8_t data) { m_attribs[get_row(row)][col] = data; }
void set_scroll_register(uint8_t value) { m_scroll_reg = value; }
void reset();

uint8_t get_char(int row, int col) const { return m_chars[get_row(row)][col]; }
uint8_t get_attrib(int row, int col) const { return m_attribs[get_row(row)][col]; }
private:
int get_row(int row) const { return (row + m_scroll_reg) % ROWS; }

uint8_t m_chars[ROWS][COLS];
uint8_t m_attribs[ROWS][COLS];
uint8_t m_scroll_reg = 0;
};

template <int ROWS, int COLS>
void rm380z_vram<ROWS, COLS>::reset()
{
memset(m_attribs, 0, sizeof(m_attribs));
memset(m_chars, 0x80, sizeof(m_chars));
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you nest this class inside rm380z_state to keep it out of the global namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I've now done that.

Comment on lines 10 to 11

//#include <iostream>
//#include <bitset>
Copy link
Member

Choose a reason for hiding this comment

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

Are these leftovers from something? At any rate, #includeing things before the prefix header is problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, leftovers from my debug tracing. Now removed. std::bitset is quite useful for printing binary numbers in trace.

@cuavas
Copy link
Member

cuavas commented Feb 16, 2024

This does look like a big simplification that will be nice to get in.

@RobinSergeant
Copy link
Contributor Author

This does look like a big simplification that will be nice to get in.

Great, thanks for reviewing @cuavas. I've addressed your comments so should be good to go in now. Do I need to do anything to make the updated romset available? It's attached to this PR.

@simzy39
Copy link
Contributor

simzy39 commented Feb 17, 2024

Usually, the way to submit data/roms is by following this guide: https://wiki.mamedev.org/index.php/Submitting_Source_Code#Submitting_ROM_or_disk_dumps

This file has started to move away from “European” spacing around parantheses, and MAME’s convention is lowercase hex digits.
@cuavas cuavas merged commit e47f024 into mamedev:master Feb 17, 2024
5 checks passed
@RobinSergeant
Copy link
Contributor Author

Usually, the way to submit data/roms is by following this guide: https://wiki.mamedev.org/index.php/Submitting_Source_Code#Submitting_ROM_or_disk_dumps

Thanks @simzy39, I've followed that guide and emailed the rom.

Mokona pushed a commit to Mokona/mame that referenced this pull request Feb 28, 2024
* Use real character generator ROM for COS 4.0.
* Corrected screen resolution.
* Implemented user-defined character support.
stonedDiscord pushed a commit to stonedDiscord/mame that referenced this pull request Apr 8, 2024
* Use real character generator ROM for COS 4.0.
* Corrected screen resolution.
* Implemented user-defined character support.
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