rm/rm380z.cpp: Add support for high resolution graphics, colour and text character dimming #12044

RobinSergeant · 2024-02-19T15:16:18Z

This PR adds supports for the HRG board described here:

It was needed to run most educational software so would be nice to get this in. As I had to change the palette I've also added the missing dimmed text attribute, and a config port to choose between a colour or monochrome monitor.

Support for both the High Resolution mode (320x192, 4 colour) and Medium Resolution (160x96, 16 colour) is included. The two modes share video ram, and the medium mode includes two separate display pages (one page is stored at odd addresses, and the other at even addresses). Each logical colour index maps to one of a possible 256 different colours, which was quite impressive for the time - much more colourful than many 8-bit home computers!

The HRG occupies the first 20 lines of the display, with the lower 4 lines reserved for text. Although it is also possible to draw text over the graphics area (when the VDU is not set to graphics mode).

It is best demonstrated by using RM Basic where you can now easily draw colourful graphics like this:

RobinSergeant · 2024-02-19T18:41:59Z

Not strictly related to this change, but I've just found and pushed a quick fix for the blinking cursor bug. Making the cursor blink (by pressing Ctrl+U) stopped the keyboard from responding. Only clearing the latch when the reset bit has actually been toggled fixes this, and now it can blink and be stopped with Ctrl+W.

RobinSergeant · 2024-02-21T01:02:58Z

I came across a later version of RML Extended BASIC today (version 5.0L) and this failed to recognise the HRG card so I've made another fix for that. Version 5.0H didn't bother to check and would just hang if the card was not installed, but the newer version monitors HRG port 0 and times out with an error message if it does not detect a change in value. It should change when line blanking begins or ends, but the timing of this at our end is suspect. I've extended the line blanking period to ~18% of the the scan line as Wikipedia quotes that %, but I still don't really trust the timing. It fixes the problem as the transition is now happening frequently enough to satisfy the HRG card detection code however.

The new version of basic also does out of bounds writes when clearing the video RAM so I've added bounds checking to fix that. I should have done that anyway. Not sure why it's doing this, but filling the screen with pixels and then clearing the screen works now. Maybe it was just some code optimisation that relied on out of bounds writes having no effect.

RobinSergeant · 2024-02-21T23:15:31Z

Just discovered that on startup RML Extended BASIC v 5.0L writes character data to the HRG memory, and without this the "STPLOT" function does not work. This is what caused the out of bounds writes I noticed before. The HRG reference manual does not mention extra RAM for this, but it predates the "HRG Level 2" feature set (which includes "STPLOT"). The previous version of BASIC I was using does not include the level 2 features.

So another mystery solved :-) I have increased the RAM size to account for this and can now plot text on the HRG display. I don't actually think it uses all this memory, but it writes to pages 12 through 15 to store the data. Without knowing exactly how the address space is mapped to a character table just allowing enough RAM for the full range of addresses seems like the best strategy. The characters are obviously plotted in software because I've not had to change my display routines at all, but I guess it uses video RAM for the character data to free up normal RAM. They can also be redefined using the "DEFCHAR" routine. See the BASIC manual for details:

https://vt100.net/rm/docs/pn11006.pdf

This PR is ready for review and hopefully I won't find any other hidden features now!!!

cuavas

I’m starting to get concerned that this is starting to reinforce bad/outdated patterns in this driver. This is a case of a driver that was never in a good state to begin with being ported to C++ by the path of least resistance, and hasn’t had a great deal of attention paid to it since besides ensuring it still compiles.

It’s better to keep just common things in a common base class, and create derived classes adding things specific to certain machines. That way you can avoid optional object finders for the most part, and it’s a lot easier to understand what’s used where.
Driver init functions should really only be used for stuff like decrypting or unscrambling ROMs, and other stuff that can’t practically be done another way. Most hardware differences should be handled in machine configuration, and you should override machine_start etc. in your derived classes to set stuff up.
The screen is configured with impossible parameters. You should really be setting the screen timing up using set_raw, deriving everything from the master clock.
And that brings us to static_vblank_timer, which is a workaround for not configuring the screen device correctly, because you can’t just ask the screen device for current vblank/hblank state if you configure it to have zero blanking area. It needs to go away.

Getting back to the actual functional changes, is the HRG board an optional add-on? If it is an add-on, is it conceptually a modification to the system, or a plug-in card?

If it’s a modification to the system, you should create clone systems with the HRG board added, so the user can choose a system with or without it. (For an example, see src/mame/osborne1.cpp where there are clones for systems with the SCREEN-PAC and Nuevo Video high-resolution video board modifications.)
If it’s a plug-in card, it should be implemented as a slot device, so the user can choose to run the system with or without it plugged in.

src/mame/rm/rm380z.h

cuavas · 2024-02-22T14:16:55Z

src/mame/rm/rm380z.h

+	void change_hrg_scratchpad(int index, uint8_t value, uint8_t mask);
+	void change_palette(int index, uint8_t value) const;


These are also conceptually not the sort of things that should be const.

Yes, I agree about these so have changed the palette altering methods to be non const.

src/mame/rm/rm380z_m.cpp

RobinSergeant · 2024-02-22T23:55:42Z

I’m starting to get concerned that this is starting to reinforce bad/outdated patterns in this driver. This is a case of a driver that was never in a good state to begin with being ported to C++ by the path of least resistance, and hasn’t had a great deal of attention paid to it since besides ensuring it still compiles.

It’s better to keep just common things in a common base class, and create derived classes adding things specific to certain machines. That way you can avoid optional object finders for the most part, and it’s a lot easier to understand what’s used where.

Driver init functions should really only be used for stuff like decrypting or unscrambling ROMs, and other stuff that can’t practically be done another way. Most hardware differences should be handled in machine configuration, and you should override machine_start etc. in your derived classes to set stuff up.

The screen is configured with impossible parameters. You should really be setting the screen timing up using set_raw, deriving everything from the master clock.

And that brings us to static_vblank_timer, which is a workaround for not configuring the screen device correctly, because you can’t just ask the screen device for current vblank/hblank state if you configure it to have zero blanking area. It needs to go away.

I agree in general and came to similar conclusions about the lack of inheritance. I will look into removing the timer hack, and can also refactor the code to make it more OO. Would you like this to be done now, or could it wait? I think the PR works as it stands and would make the driver better (allowing more software to run on the machine), but I can see that it needs more work.

Getting back to the actual functional changes, is the HRG board an optional add-on? If it is an add-on, is it conceptually a modification to the system, or a plug-in card?

If it’s a modification to the system, you should create clone systems with the HRG board added, so the user can choose a system with or without it. (For an example, see src/mame/osborne1.cpp where there are clones for systems with the SCREEN-PAC and Nuevo Video high-resolution video board modifications.)

If it’s a plug-in card, it should be implemented as a slot device, so the user can choose to run the system with or without it plugged in.

The HRG board was a configurable option when buying the machine, e.g. if you only wanted to use it for word processing then you could buy a cheaper machine without the HRG card. Things were clearly designed with the board in mind. For example the 4 line scrolling mode is called graphics mode and is there to display text below the HRG. Does this sound like something that needs to be made a slot device? Are they any good examples of this to look at?

cuavas · 2024-02-24T13:24:03Z

I agree in general and came to similar conclusions about the lack of inheritance. I will look into removing the timer hack, and can also refactor the code to make it more OO. Would you like this to be done now, or could it wait? I think the PR works as it stands and would make the driver better (allowing more software to run on the machine), but I can see that it needs more work.

I’d really rather the unrealistic screen timings and static_vblank_timer could go away as soon as possible. Are there schematics of the machine showing the video counters, etc.? It’s usually pretty easy to work out the parameters from that.

The HRG board was a configurable option when buying the machine, e.g. if you only wanted to use it for word processing then you could buy a cheaper machine without the HRG card. Things were clearly designed with the board in mind. For example the 4 line scrolling mode is called graphics mode and is there to display text below the HRG. Does this sound like something that needs to be made a slot device? Are they any good examples of this to look at?

That doesn’t sound so much like a case for a slot device. Sounds like it’s more a case for having separate machines with and without the HRG card like the three variants of the Osborne 1 (standard, with SCREEN-PAC graphics, and with Nuevo Video graphics).

RobinSergeant · 2024-02-24T20:48:52Z

I agree in general and came to similar conclusions about the lack of inheritance. I will look into removing the timer hack, and can also refactor the code to make it more OO. Would you like this to be done now, or could it wait? I think the PR works as it stands and would make the driver better (allowing more software to run on the machine), but I can see that it needs more work.

I’d really rather the unrealistic screen timings and static_vblank_timer could go away as soon as possible. Are there schematics of the machine showing the video counters, etc.? It’s usually pretty easy to work out the parameters from that.

The service manual has schematics and can be found here:

https://vt100.net/rm/docs/pn13821a.pdf

Are you able to get anything from that? I only have a software background so cannot read schematics! It shows both the VDU-40 and VDU-80 boards. The VDU-80 description also talks about working with the HRG board.

The firmware guide (chapter 11) says frame blanking occurs for approximately 4.5 milliseconds every 20 milliseconds, and that during line blanking there is only time to output one character. That is the only clue to timing that I could find. It can be found here:

https://vt100.net/rm/docs/pn10971.pdf

The HRG board was a configurable option when buying the machine, e.g. if you only wanted to use it for word processing then you could buy a cheaper machine without the HRG card. Things were clearly designed with the board in mind. For example the 4 line scrolling mode is called graphics mode and is there to display text below the HRG. Does this sound like something that needs to be made a slot device? Are they any good examples of this to look at?

That doesn’t sound so much like a case for a slot device. Sounds like it’s more a case for having separate machines with and without the HRG card like the three variants of the Osborne 1 (standard, with SCREEN-PAC graphics, and with Nuevo Video graphics).

Great, I will make it a separate machine then and try to sort out the class hierarchy while I'm at it.

RobinSergeant · 2024-02-25T12:58:37Z

@cuavas, I've now created a separate machine for HRG and split the state class up into a hierarchy to remove the init functions and basically make it a bit cleaner. Is this the sort of thing you had in mind? Now the VDU-40 machines don't have any code for VDU-80 etc. Also the cassette stuff is isolated which will resolve this defect:

https://mametesters.org/view.php?id=6484

galibert · 2024-02-26T22:28:45Z

For the vdu-40 the timings are given by a pair of seemingly undumped prom, the line prom at ic31, coloured green, and the frame prom at ic36 colored yellow. Vdu-80 seems to be.. the same actually. Which means we won't have correct timing until one can ferret out a dump of those proms, which seem to exist but not publically.

rb6502 · 2024-02-27T00:48:32Z

Yeah, the VDU-80 seems to be the same relative timings, just a doubled pixel clock.

RobinSergeant · 2024-02-27T11:21:37Z

For the vdu-40 the timings are given by a pair of seemingly undumped prom, the line prom at ic31, coloured green, and the frame prom at ic36 colored yellow. Vdu-80 seems to be.. the same actually. Which means we won't have correct timing until one can ferret out a dump of those proms, which seem to exist but not publically.

Thanks guys. I have posted a request on the RM user group just in case anybody has dumps of these. Bit of a long shot, but worth asking...

RobinSergeant · 2024-02-27T17:35:59Z

Just pushed another commit to correct the vram size. I've now figured out how the last 1k of character data is addressed so it now all fits in 16K like its supposed to! Confirmed it still works by running a Basic program that prints the character set.

simzy39 · 2024-03-02T20:37:12Z

How do I contact you to send you dumps or for people with the firmare to contact you? I'm talking to a person whom has everything.

RobinSergeant · 2024-03-02T23:46:34Z

How do I contact you to send you dumps or for people with the firmare to contact you? I'm talking to a person whom has everything.

Thank you, you could email robin.sergeant at Gmail.

RobinSergeant · 2024-03-05T12:58:38Z

Thanks to some great info from the RM forum:

https://groups.io/g/research-machines/topic/vdu_timing_prom_dumps/104600604

I've now replaced the nasty timer hack with raw screen values that seem to work (refresh rate is showing as 50.080128 Hz).

I've also corrected the VDU-40 display code to use the right resolution and the correct graphic characters. It turns out that the 5x9 characters are actually displayed inside an 8x10 grid and not a 6x10 grid as we had before. This gives a resolution of 320x240. The previous graphics characters were created by an initialisation function, which never worked properly as the characters did not match those shown in the firmware guide. Hence, I've done away with that and now generate them using the correct logic as shown on page A.7 of this document:

https://vt100.net/rm/docs/pn10930.pdf

@cuavas does it look ok to you now? I've removed the timer hack and generally cleaned up the code. I think it should be a lot cleaner now and with HRG support it is possible to run a lot more software such as Logo and old Basic games.

I do have some PROM dumps, but have been asked not to circulate them publicly as the data is being used to make replacement boards with permission from RM and they don't want to jeopardise that agreement.

cuavas

OK, this is looking massively better. The remaining issues are mostly just coding convention stuff. Thanks for cleaning up this mess.

cuavas · 2024-03-06T15:45:45Z

src/mame/rm/rm380z.h

 		m_chargen(*this, "chargen"),
 		m_maincpu(*this, RM380Z_MAINCPU_TAG),
 		m_screen(*this, "screen"),
-		m_cassette(*this, "cassette"),
 		m_messram(*this, RAM_TAG),
 		m_fdc(*this, "wd1771"),
 		m_floppy0(*this, "wd1771:0"),
 		m_floppy1(*this, "wd1771:1")
 	{
 	}


Constructors of abstract classes should be protected.

cuavas · 2024-03-06T15:47:25Z

src/mame/rm/rm380z.h

-	void rm480z(machine_config &config);
-	void rm380z(machine_config &config);
-
-	void init_rm380z();
-	void init_rm380z34d();
-	void init_rm380z34e();
-	void init_rm480z();
+	virtual void configure(machine_config &config);


Do you need this to be virtual? The COMP macro needs to know the concrete implementation class anyway, so it will be able to find the appropriate member function in the derived class. I’d just make it protected and non-virtual.

No, I only made it virtual as some people don't like shadowing and say it's better to declare as virtual to avoid that. Have changed it now

You don’t have to give it the same name in derived classes. The thing is, making it virtual generates unnecessary additional code, and in something as big as MAME, that all adds up.

cuavas · 2024-03-06T15:47:58Z

src/mame/rm/rm380z.h

-	virtual void machine_reset() override;
-	virtual void machine_start() override;
+	void machine_reset() override;
+
+	static inline constexpr int RM380Z_SCREENROWS = 24;
+	static inline constexpr int RM380Z_SCREENCOLS = 40;
+
+	bool ports_enabled_high() const { return ( m_port0 & 0x80 ); }
+	bool ports_enabled_low() const { return !( m_port0 & 0x80 ); }


Please move machine_reset below the constants so there aren’t constants intermixed with member function declarations.

cuavas · 2024-03-06T15:49:54Z

src/mame/rm/rm380z.h

+	enum class hrg_display_mode: uint8_t
+	{


Just a minor thing, but we usually put spaces on both sides of “inheritance colons” (as opposed to “label colons” which have no space before them) in MAME, like enum class hrg_display_mode : uint8_t.

I think that was a typo - fixed now.

cuavas · 2024-03-06T15:50:44Z

src/mame/rm/rm380z.h

-	uint8_t m_port1 = 0;
-	uint8_t m_fbfd_mask = 0;
-	uint8_t m_fbfe = 0;
+	void machine_reset() override;


Overridden protected member functions should generally be protected – please move machine_reset to the protected section.

I've moved the others as well.

cuavas · 2024-03-06T15:52:23Z

src/mame/rm/rm380z.h

-	optional_device<fd1771_device> m_fdc;
-	optional_device<floppy_connector> m_floppy0;
-	optional_device<floppy_connector> m_floppy1;
+	DECLARE_MACHINE_RESET(rm480z);


Now that this is a separate class, can you avoid the DECLARE_MACHINE_RESET and MCFG_MACHINE_RESET_OVERRIDE and just override machine_reset in this class?

Yes, done that now.

cuavas · 2024-03-06T15:52:54Z

src/mame/rm/rm380z_m.cpp

+	default:
+		printf("unknown port [%2.2x] write of [%2.2x]\n", offset, data);


Please use logerror rather than printf for these kinds of diagnostics.

cuavas · 2024-03-06T15:54:40Z

src/mame/rm/rm380z_m.cpp

+	default:
+		printf("read from unknown port [%2.2x]\n", offset);


Please use logerror for this diagnostic.

RobinSergeant · 2024-03-06T22:11:42Z

OK, this is looking massively better. The remaining issues are mostly just coding convention stuff. Thanks for cleaning up this mess.

Thanks @cuavas, I've just addressed your latest review comments and also squeezed in one last fix to this defect that I found last night (which I hope is ok!):

https://mametesters.org/view.php?id=6483

Basically at startup the firmware reads F600 and then tries to execute code at F601 if this comes back as zero. It does this because F600 is the location for a possible ROM extension module. As this was blank it just executed a lot of NOP instructions before eventually hitting opcode FF which triggers the built in debugger (front panel).

To fix I've just used the ROMREGION_ERASEFF flag so that unused areas of ROM read FF instead of zero. Now the COS prompt appears like it should on the COS 3.4 machines.

cuavas · 2024-03-07T02:13:08Z

Thanks @cuavas, I've just addressed your latest review comments and also squeezed in one last fix to this defect that I found last night (which I hope is ok!):

https://mametesters.org/view.php?id=6483

Basically at startup the firmware reads F600 and then tries to execute code at F601 if this comes back as zero. It does this because F600 is the location for a possible ROM extension module. As this was blank it just executed a lot of NOP instructions before eventually hitting opcode FF which triggers the built in debugger (front panel).

To fix I've just used the ROMREGION_ERASEFF flag so that unused areas of ROM read FF instead of zero. Now the COS prompt appears like it should on the COS 3.4 machines.

That fix will be OK for now. Maybe in the future a media device can be mapped there so people can use it with user ROMs.

…erited from different base classes.

… text character dimming. (mamedev#12044) * Fixed MT06483 by making empty ROM areas read high (0xff). * Also refactored the code to better align with current practices.

RobinSergeant added 7 commits February 17, 2024 17:19

add character dimming

3fdbca2

initial HRG work

6560e90

add colour palette

a46909c

add support for medium res

83f97f4

scale graphics to fit screen size

6a0fd61

tidy up

5ee7b9a

fix blinking cursor bug

a3a24a1

bounds checking and timing fix

d5bdb21

support HRG Level 2 text plotting

870831d

cuavas reviewed Feb 22, 2024

View reviewed changes

address review comments

abc5031

RobinSergeant requested a review from cuavas February 23, 2024 00:18

split state into multiple classes, with new HRG machine

223f926

Merge branch 'master' into rm380z_add_hrg_support

c1fa546

correct hrg vram size

65a5225

RobinSergeant added 2 commits March 5, 2024 10:56

remove vblank timer hack

74b196f

draw VDU-40 graphic characters properly

1a88b86

cuavas reviewed Mar 6, 2024

View reviewed changes

review comments and COS 3.4 startup fix

5744279

rm/rm380z.h: Tidy - mostly visually separating overridden members inh…

8cc3e28

…erited from different base classes.

cuavas merged commit 3a800bb into mamedev:master Mar 7, 2024
5 checks passed

		void change_hrg_scratchpad(int index, uint8_t value, uint8_t mask);
		void change_palette(int index, uint8_t value) const;

		default:
		printf("unknown port [%2.2x] write of [%2.2x]\n", offset, data);

rm/rm380z.cpp: Add support for high resolution graphics, colour and text character dimming #12044

rm/rm380z.cpp: Add support for high resolution graphics, colour and text character dimming #12044

Conversation

RobinSergeant commented Feb 19, 2024

RobinSergeant commented Feb 19, 2024

RobinSergeant commented Feb 21, 2024 • edited

RobinSergeant commented Feb 21, 2024

cuavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobinSergeant commented Feb 22, 2024 • edited

cuavas commented Feb 24, 2024

RobinSergeant commented Feb 24, 2024 • edited

RobinSergeant commented Feb 25, 2024

galibert commented Feb 26, 2024

rb6502 commented Feb 27, 2024

RobinSergeant commented Feb 27, 2024

RobinSergeant commented Feb 27, 2024

simzy39 commented Mar 2, 2024 • edited

RobinSergeant commented Mar 2, 2024

RobinSergeant commented Mar 5, 2024 • edited

cuavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobinSergeant commented Mar 6, 2024 • edited

cuavas commented Mar 7, 2024

RobinSergeant commented Feb 21, 2024 •

edited

RobinSergeant commented Feb 22, 2024 •

edited

RobinSergeant commented Feb 24, 2024 •

edited

simzy39 commented Mar 2, 2024 •

edited

RobinSergeant commented Mar 5, 2024 •

edited

RobinSergeant commented Mar 6, 2024 •

edited