Add Football Crazy game to Rasterspeed driver #9383

Paul-Arnold · 2022-03-09T15:32:09Z

The Football Crazy game has been added to rastersp.cpp

In addition to this the following changes were also required...
In the scsi driver the DFE bit was being reset when the status register was read. This was wrong as the chip itself leaves the bit unchanged.

There was an issue with 386 break points as follows...
The breakpoint address some times has to be translated based on a control bit in another register. This is now implemented.
The instruction which caused the break point was being re-executed, it shouldn't be.
add_change_notiifer calling dri_changed caused it to be called twice simultaneously. Have to admit to not being certain of the purpose of add_change_notifier!

Breakpoint caused instruction to be retried when it shouldn't be. Breakpoint address wasn't being translated depending on bit 31 of m_cr[0] Calling of dr7_changed has to be after dr7 has changed otherwise it can't be actioned. add_change_notifier removed as it was causing dr7_changed to be called mutiple time simultaneously.

cuavas · 2022-03-09T17:36:34Z

src/devices/cpu/i386/i386.cpp

@@ -2025,7 +2025,6 @@ void i386_device::i386_common_init()
 	m_ferr_handler(0);

 	set_icountptr(m_cycles);
-	m_notifier = m_program->add_change_notifier([this] (read_or_write mode) { dri_changed(); });


If you remove this, the CPU core won’t be notified if handlers are changed in the address space, possibly requiring passthrough handlers to be reinstalled.

@cuavas Firstly, sorry for causing such a mess of this pull request :(
OK, having refreshed my memory the problem with the notifier is that it seems to causes dri_changed() to be called while actually within dri_changed() :(
What happens is the adding of the read/write tap within dri_changed causes dri_changed to be called which can't be right. Placing debug in dri_changed at the top and bottom to show "enter" and "exit" produces the following output when the taps are changed...
enter, enter, exit, exit.
Further debug shows the point of the second entry as when the tap is installed.
How to prevent this if dri_changed really needs to be called when handlers are changed ?

You have to guard against installing taps recursively. For example see the watchpoint code in https://github.com/mamedev/mame/blob/master/src/emu/debug/points.cpp

cuavas · 2022-03-09T17:39:20Z

src/devices/cpu/i386/i386ops.hxx

-			dr7_changed(m_dr[7], rm32);
-			CYCLES(CYCLES_MOV_DR6_7_REG);
+			uint32_t m_old_dr7 = m_dr[ 7 ] ;
 			m_dr[dr] = rm32;
+			dr7_changed( m_old_dr7, m_dr[7] );
+			CYCLES(CYCLES_MOV_DR6_7_REG);


This code doesn’t match the formatting of the surrounding code, and using the m_ prefix that signifies a member variable on a local variable will cause confusion.

cuavas · 2022-03-09T17:40:28Z

src/devices/cpu/i386/i386segs.hxx

@@ -2490,7 +2490,7 @@ inline void i386_device::dri_changed()
 			int breakpoint_length = (m_dr[7] >> ((dr << 2) + 16 + 2)) & 3;
 			uint32_t phys_addr = m_dr[dr];
 			uint32_t error;
-			phys_addr = translate_address(m_CPL, TRANSLATE_READ, &phys_addr, &error);
+			phys_addr = (m_cr[0] & ( 1<<31 )) ? translate_address(m_CPL, TRANSLATE_READ, &m_dr[dr], &error) : m_dr[dr];


Once again, you’re ignoring the formatting of surrounding code. You can also use the BIT helper.

This doesn't really make sense. translate_address returns a bool indicating whether the address translation is valid and always succeeds if paging is disabled.

@cracyc I'm trying to get back into this as it was a while ago I did it. Yes, you're right it makes no sense!. However, it also means the original line which I took out is also equally senseless as it would set the phys_addr to either true or false :(
So, the correct fix here is the remove of "phys_addr =" and just call translate_address(...) Actually, just tried that and it works fine. I'm just not sure how existing things manage to work unless nothing uses this ?

I think that this is the first user of the debug registers outside of pc debuggers which aren't well tested. Although, @philipjbennett told me on discord that he was working on another machine the uses them. Probably should also fail out if translate_address returns false otherwise it'll set the taps at the wrong address. I'm sure what a real cpu would do if a page fault occurs at the same address as a breakpoint but it probably would take the page fault first.

Thanks @cracyc, I've modified it to not add the tap if translate_address fails.
Just need to workout how to stop dri_changed getting called while still in the function.

cuavas · 2022-03-09T17:41:18Z

src/devices/machine/53c7xx.cpp

-				m_dstat = 0;
+				//DFE isn't cleared on read
+				m_dstat &= DSTAT_DFE ;


This doesn’t match the formatting of the rest of the file.

Hopefully updated correctly.

cuavas · 2022-03-09T17:42:26Z

src/mame/drivers/rastersp.cpp

+	emu_timer *m_edc_tb_timer;
+	void aux_port0_w(offs_t offset, uint8_t data);
+	void aux_port1_w(offs_t offset, uint8_t data);
+	void aux_port2_w(offs_t offset, uint8_t data);
+	void aux_port3_w(offs_t offset, uint8_t data);
+	void aux_port4_w(offs_t offset, uint8_t data);
+	uint8_t m_aux_port3_data;


Please group data members.

To be sure, you mean move the m_aux_port3_data and m_edc_tb_timer so they're together with m_uart_regs etc ?
So not intermixed with function definitions ?
Have done that locally.

cuavas · 2022-03-09T17:43:57Z

src/mame/drivers/rastersp.cpp

+	if (!m_edc_tb_timer)
+		m_edc_tb_timer = machine().scheduler().timer_alloc(timer_expired_delegate(FUNC(fbcrazy_state::edc_tb_timer), this));


You can assume timers don’t exist when starting unless you’re expecting a derived class to throw a missing dependencies exception after calling the base implementation.

OK, hadn't realised that. I've made the change locally and will commit with all the changes which are going to be needed.

cuavas · 2022-03-09T17:46:14Z

src/mame/drivers/rastersp.cpp

@@ -427,6 +614,12 @@ void rastersp_state::update_irq(uint32_t which, uint32_t state)
 WRITE_LINE_MEMBER( rastersp_state::scsi_irq )
 {
 	update_irq(IRQ_SCSI, state);
+
+	if ( state && m_dsp_ctrl_data&0x20)


Spacing on this line is totally whack.

Agreed! Fixed locally and will commit with all the other changes which are going to need to happen in this file

cuavas · 2022-03-09T17:47:39Z

src/mame/drivers/rastersp.cpp

+/*
+	Mimic a Bacta Dataport unit just enough to satisfy Football Crazy.
+	Also mimic trackball unit. This uses a small PIC based interface board
+	which converts the trackball movement into RS232 data.
+	The data format is as follows:
+	Byte 0 : 0100abAB bits 7 & 6 = 01 signifies 1st byte
+	Byte 1 : 00CDEFGH bits 7 & 6 = 00 signifies 2 or 3rd byte (not identified)
+	Byte 2 : 00cdefgh
+
+	The trackball X movement is ABCDEFGH and Y movement abcdefgh
+	We do not have a PCB nor a dump of the PIC :(
+*/


There’s already a stub BACTA data logger in src/mame/machine/bacta_datalogger.{h,cpp} – please consider using it rather than creating another stub device to mimic it.

cuavas · 2022-03-09T17:48:48Z

src/mame/drivers/rastersp.cpp

+	}
+}
+
+uint8_t fbcrazy_state::edc_tb_r(offs_t offset)


What UART is this supposed to be? The simulation looks complex enough that it might justify a device.

It's a 85C30. I know that this is already supported however, when I originally looked at using it with an external clock I mistook the comment "Support for external clock as source for BRG yet to be finished" in the file z80scc.cpp to mean that mode of operation wasn't supported and I couldn't understand why and what to do with it. Looking again the comment is a bit misleading and it does support an external clock - just nothing to do with that function!
If I try and update to this I should be able to use the BACTA datalogger mentioned above. That just leaves the track ball interface which is serial. I'd have to make a serial device for that to connect to the 85c30. We don't have the code for the track ball interface board so can't emulate it.
Is that what you'd prefer ? So remove all the stuff that's in there at the moment and replace with 85c30, datalogger and track balll device ?

Yeah, better to use the 85C30 device. It’s known to be buggy, but more things using it will hopefully help get more bugs fixed. You don’t necessarily need to make a device for the trackball – you could hook up callbacks in the driver state class to the SCC’s transmit/receive lines and simulate the trackball there. If you do decide to create a device, the device_serial_interface mixin should simplify the implementation if it isn’t doing anything exotic.

Trackball is just a tx device, no handshake or anything so just need to worry about tx line. But I'd still need to create a stream of serial data if I hook onto the scc receive line, unless I misunderstood what you meant ? Looking at z80scc.cpp shows that receive_data(data) which places an rx'd byte into the fifo is public so could use that and push straight into the fifo ? Or would that be frowned upon.

cuavas · 2022-03-09T17:51:13Z

src/mame/drivers/rastersp.cpp

-	map(0xfc0000, 0xffffff).bankrw("bank3");
+	map(0xfc0000, 0xffffff).bankrw("bank1");//was3
 }

+void rastersp_state::dsp_map(address_map &map)
+{
+	dsp_map_base(map);
+	map(0x000000, 0x0fffff).bankrw("bank1");//4 meg
+}
+
+void fbcrazy_state::dsp_map(address_map &map)
+{
+	rastersp_state::dsp_map_base(map);
+	map(0x000000, 0x1fffff).bankrw("bank1");//8 meg
+}


Is it correct that the DSP and main CPU always see the same bank?

I believe so. I can't see anything in the documentation for the board which suggests they can be different.

smf- · 2022-03-11T12:18:52Z

Yes.

…

On 10/03/2022 20:10, Paul-Arnold wrote: Or would that be frowned upon. Message ID: ***@***.***>

Re-instated handler change notification for dri_changed() Corrected use of translate_address() Prevent dri_changed from re-entry

Sometimes a seemingly random baud rate would be set rather than that requested. Due to two uninitialised variables.

Uses proper drivers for bacta data logger and 85c30 duart.

Paul-Arnold · 2022-03-12T16:55:55Z

I believe I've implemented all the changes requested.
I've switched to using the 85c30 uart driver and the bacta data logger. There was a fault in each of these which I've corrected hence the extra files added to the PR.

cuavas

This is looking much better now. Just a couple of relatively small things I noticed.

cuavas · 2022-03-24T14:26:04Z

src/devices/cpu/i386/i386segs.hxx

+				else if(breakpoint_type == 1) m_dr_breakpoints[dr] = m_program->install_write_tap(phys_addr, phys_addr + 3, "i386_debug_write_breakpoint",
+				[&, dr, true_mask](offs_t offset, u32& data, u32 mem_mask)


It’s not safe to capture anything by reference when a lambda will outlive its containing scope. Also, auto capture is a bug waiting to happen. Please capture what you need explicitly.

Also, please clean up the formatting here – if the controlled statement spans multiple lines, it’s cleared if you use braces, and statement continuations should be intended (I realise it was already a mess. Make is something like:

else if(breakpoint_type == 1) { m_dr_breakpoints[dr] = m_program->install_write_tap( phys_addr, phys_addr + 3, "i386_debug_write_breakpoint", [...] (offs_t offset, u32 &data, u32 mem_mask) { ... }, &m_dr_breakpoints[dr]); }

cuavas · 2022-03-24T14:28:00Z

src/devices/cpu/i386/i386segs.hxx

+				else if(breakpoint_type == 3) m_dr_breakpoints[dr] = m_program->install_readwrite_tap(phys_addr, phys_addr + 3, "i386_debug_readwrite_breakpoint",
+				[this, dr, true_mask](offs_t offset, u32& data, u32 mem_mask)
+				{
+					if(true_mask & mem_mask)
+					{
+						m_dr[6] |= 1 << dr;
+						i386_trap(1,1,0);
+					}
+				},
+				[this, dr, true_mask](offs_t offset, u32& data, u32 mem_mask)
+				{
+					if(true_mask & mem_mask)
+					{
+						m_dr[6] |= 1 << dr;
+						i386_trap(1,1,0);
+					}
+				}, &m_dr_breakpoints[dr]);


Same for this, please format the code so it’s obvious there’s an assignment/member function call controlled by the if.

cuavas · 2022-03-24T14:30:40Z

src/mame/drivers/rastersp.cpp

+			newxpos = ioport("TRACK_X")->read();
+			newypos = ioport("TRACK_Y")->read();


Please don’t add run-time tag map lookups. Use required_ioport to find these at object resolution time.

cuavas · 2022-03-24T14:33:32Z

src/mame/drivers/rastersp.cpp

+				if (m_trackball_data[(m_trackball_ctr/12)]&(1<<(bitnum-1)))
+				{
+					m_duart->rxb_w(1);
+				}
+				else
+				{
+					m_duart->rxb_w(0);
+				}


There are a lot of parentheses and it looks like your space bar was broken for a bit. Couldn’t you just do m_duart->rxb_w(BIT(m_trackball_data[m_trackball_ctr / 12], bitnum - 1)); and save some lines?

Simplified trackball data transmit function. Use required_ioport instead of run-time tag map lookup. Added missing items to save state

Paul-Arnold · 2022-03-27T09:57:54Z

Hopefully done the changes requested correctly.
I noticed the game didn't always run correctly after loading a saved state. This was due to items missing in the save state for the duart added last time.

* machine/53c7xx.cpp: DFE bit is not reset when status register is read. * cpu/i386: Fixed multiple issues with breakpoint emulation. * machine/bacta_datalogger.cpp: Prevent continuous transmission of 0xff. * machine/z80scc.cpp: Fixed incorrect setting of baud rate due to uninitialised variables. New working machines -------------- Football Crazy (Video Quiz) [Paul-Arnold]

Paul-Arnold added 3 commits March 9, 2022 14:57

DFE bit is not reset when status reg is read

1e89168

Add Football Crazy to Raster Speed driver

1010b12

cuavas requested changes Mar 9, 2022

View reviewed changes

rb6502 requested a review from cracyc March 9, 2022 21:34

Fixed formatting

7845f7f

Paul-Arnold added 4 commits March 12, 2022 16:42

Fixed setting of x86 breakpoints

1ae8541

Re-instated handler change notification for dri_changed() Corrected use of translate_address() Prevent dri_changed from re-entry

Prevent continuous transmission of 0xff

c9ed45f

Fixed incorrect setting of baud rate

8e50a42

Sometimes a seemingly random baud rate would be set rather than that requested. Due to two uninitialised variables.

Use bacta data logger and 85c30

7420e41

Uses proper drivers for bacta data logger and 85c30 duart.

cracyc approved these changes Mar 13, 2022

View reviewed changes

Paul-Arnold requested review from cuavas and cracyc March 14, 2022 17:39

cuavas requested changes Mar 24, 2022

View reviewed changes

Paul-Arnold added 3 commits March 27, 2022 10:06

Add missing save state items

089d42a

Code simplification. Remove run-time tag map lookups

144f745

Simplified trackball data transmit function. Use required_ioport instead of run-time tag map lookup. Added missing items to save state

Lambdas no longer use capture by reference.

59b7c79

Paul-Arnold requested a review from cuavas March 28, 2022 22:22

Merge branch 'master' into master

563379b

cuavas merged commit 209b0bf into mamedev:master Mar 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Football Crazy game to Rasterspeed driver #9383

Add Football Crazy game to Rasterspeed driver #9383

Paul-Arnold commented Mar 9, 2022

cuavas Mar 9, 2022

Paul-Arnold Mar 10, 2022 •

edited

Loading

cuavas Mar 10, 2022

cuavas Mar 9, 2022

cuavas Mar 9, 2022

cracyc Mar 9, 2022

Paul-Arnold Mar 9, 2022

cracyc Mar 10, 2022 •

edited

Loading

Paul-Arnold Mar 10, 2022

cuavas Mar 9, 2022

Paul-Arnold Mar 10, 2022

cuavas Mar 9, 2022

Paul-Arnold Mar 10, 2022

cuavas Mar 9, 2022

Paul-Arnold Mar 10, 2022

cuavas Mar 9, 2022

Paul-Arnold Mar 10, 2022

cuavas Mar 9, 2022

cuavas Mar 9, 2022

Paul-Arnold Mar 10, 2022

cuavas Mar 10, 2022

Paul-Arnold Mar 10, 2022

cuavas Mar 9, 2022

Paul-Arnold Mar 10, 2022

smf- commented Mar 11, 2022 via email

Paul-Arnold commented Mar 12, 2022 •

edited

Loading

cuavas left a comment

cuavas Mar 24, 2022

cuavas Mar 24, 2022

cuavas Mar 24, 2022

cuavas Mar 24, 2022

Paul-Arnold commented Mar 27, 2022

		if (!m_edc_tb_timer)
		m_edc_tb_timer = machine().scheduler().timer_alloc(timer_expired_delegate(FUNC(fbcrazy_state::edc_tb_timer), this));

		else if(breakpoint_type == 1) m_dr_breakpoints[dr] = m_program->install_write_tap(phys_addr, phys_addr + 3, "i386_debug_write_breakpoint",
		[&, dr, true_mask](offs_t offset, u32& data, u32 mem_mask)

		newxpos = ioport("TRACK_X")->read();
		newypos = ioport("TRACK_Y")->read();

Add Football Crazy game to Rasterspeed driver #9383

Add Football Crazy game to Rasterspeed driver #9383

Conversation

Paul-Arnold commented Mar 9, 2022

Choose a reason for hiding this comment

Paul-Arnold Mar 10, 2022 • edited Loading

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

cracyc Mar 10, 2022 • edited Loading

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

smf- commented Mar 11, 2022 via email

Paul-Arnold commented Mar 12, 2022 • edited Loading

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

Paul-Arnold commented Mar 27, 2022

Paul-Arnold Mar 10, 2022 •

edited

Loading

cracyc Mar 10, 2022 •

edited

Loading

Paul-Arnold commented Mar 12, 2022 •

edited

Loading