k573mcr: Implementation of JVS memory card reader device for System 573 #7659

987123879113 · 2021-01-11T14:32:32Z

This is a software implementation of the JVS memory card reader device used by System 573. This allows for saving and loading scores in DDR games, loading custom edit chart data in DDR and Guitar Freaks games, and PS1 guitar controller inputs in Guitar Freaks games.

I've included packet captures for every implemented commands and the corresponding responses in the code for documentation purposes. If it's too much or not required then I can remove them.

The following games are affected:

ddr2mc2
ddr2ml
ddr2mla
ddr3mka
ddr3ma
ddr3mk
ddr3mj
ddr3mp
ddr4m
ddr4mj
ddr4ms
ddr4msj
ddr4mp
ddr4mps
ddr5m
ddrmax
ddrmax2
ddrextrm
gtrfrk3m
gtfrk3ma
gtrfrk4m

All of the above have been tested to make sure that the memory card and controller functionality is working. I've also tried to test as much of the other System 573 games as possible (~80 or so) to make sure nothing broke as a result of these changes.

Overview of changes:

jvsdev: Added a custom device-specific message handler that can be overridden by devices implementing jvsdev
k573mcr: Added documentation about the PCB layout
k573mcr: Implemented k573mcr as a jvsdev using the custom message handler
ksys573: Implemented registers used for JVS communication
ksys573: Removed hacks for ddr2ml/ddr2mla that patched out the security plate errors since this PR fixes the problem

987123879113 · 2021-01-11T14:33:06Z

src/mame/machine/k573mcr.cpp

+uint8_t k573mcr_device::controller_port_send_byte(uint32_t port_no, uint8_t data)
+{
+	auto port = m_ports[port_no];
+	uint8_t output = 0;
+
+	for (int i = 0; i < 8; i++) {
+		port->clock_w(0);
+		port->tx_w(!!(data & (1 << i)));
+		port->clock_w(1);
+		output |= port->rx_r() << i;
+	}
+
+	return output;
+}


Is there a better way to implement communication with the PS1 controller ports? I didn't think it was appropriate to reimplement something separate entirely because the hardware uses real PSX controller ports. My other idea was to expose psxcard_device::transfer and psx_standard_controller_device::get_pad but I don't feel comfortable making that change on my own.

Well it actually is serial, and transfers take time. Pretending it happens instantaneously like this is bad enough; pretending it has a parallel interface would be even worse.

I rewrote the PSX controller code to use the PSX controller bus instead of communicating to the ports directly in preparation for trying to implement the PSX code in a more proper way that took transfer time into consideration. The memory card part worked fine because the game polls waiting for a status flag and will just keep polling until the read/write operation has finished, but I ran into a big problem when I tried to implement the pad reads. The pad read function expects to be able to return the pad status as the response to the pad read command. It does not rely on polling the status flag like memory card functions. I think a more skilled person would be able to implement this part in a way that respects transfer times, but that level of implementation is currently outside of my skill level.

Don't let yourself get intimidated by the codebase. Given the level of skill you've shown in turning around nearly all of the feedback points, I think you're more than skilled enough to handle it.

As part of its core, MAME has a class called emu_timer. It's not meant to encapsulate any particular timer in a chip, it's just intended for use when you want to fire a callback a given amount of time in the future. It can either be implemented using a device's built-in device_timer callback (preferable if a device only has one thing that needs to be timed), or using a specifically-named TIMER_CALLBACK_MEMBER function, which is preferable in the case of a device that has multiple potential timing-related tasks that need to be coordinated.

If you need an example on how to make use of it, I would refer to the SA1110 or SA1111 devices, both of which make heavy use of delayed operations.

If you have any questions, we're here to help.

@MooglyGuy Thanks for the comment. I will check out the devices you mentioned. I actually was using TIMER_CALLBACK_MEMBER in my attempt. I pushed the WIP code to a separate branch if you're willing to look at it and give me feedback on what I could do to fix it up. Everything except the pad reporting functionality is working, and reading the pad state itself actually works even. I just can't figure out how to get the data out of the buffer fast enough to return in the response for handle_message.

987123879113@5053cb0

This is kind of a slow realization that I made while rereading my code for this response, but I just realized that I don't need to poll the pad when it's requested. I can just poll for pad status updates in the timer update and store the results for when it's requested and that should probably solve the issue for the most part.

Your realization sounds about right. In reality, the actual pad status is most likely only latched when the status changes, and the timer should only be kicked off when a new request comes in. If there's a request currently in-flight, then presumably the status register would indicate that, without the need for kicking off a new read cycle.

987123879113 · 2021-01-11T14:33:51Z

src/mame/machine/k573mcr.cpp

+INPUT_PORTS_START( k573mcr_meta_controls )
+	PORT_START("META")
+	PORT_BIT( 0x01, IP_ACTIVE_LOW, IPT_SERVICE1) PORT_TOGGLE PORT_NAME("Insert/Eject Memory Card 1")
+	PORT_BIT( 0x02, IP_ACTIVE_LOW, IPT_SERVICE2) PORT_TOGGLE PORT_NAME("Insert/Eject Memory Card 2")
+INPUT_PORTS_END


I implemented meta controls for the memory card reader to simulate the action of inserting and ejecting the memory card slot. Is this ok?

The intent was to make interactions like ending a credit in DDR smoother when it tells you to eject your card. Instead of waiting for the counter to tick down, it's possible to press the toggle button to eject your card and then press the button again to insert it, like you would on a real machine.

Well, the usual way of removing/inserting media devices is using the file manager menu. It becomes impractical when the media devices are slot devices that can instantiate different objects depending on what you insert into them because those require a hard reset to change. However, as far as I can tell, the PlayStation memory isn’t one of these - the user can pull up the menu and take the memory card out. It doesn’t require a special input.

You are correct, the Playstation memory cards do not require a hard reset to change. My reasoning for this was that I think it's less practical to open the menu every time to eject and reopen the memory card file. Especially so if you are playing on something like a MAME cabinet and don't have as easy access to menus.

For a normal Playstation game you can reasonably expect that the player will generally keep the memory card in the slot. For the supported System 573 games, it's expected that the player will be removing and inserting memory cards after every credit and the games are coded as such. For example, unskippable timers until the player removes their card as a way to remind the player to take their card with them instead of leaving it in the machine.

It's not required but but I believe it's a keybind that smoothly replicates actions that the game expects you to perform. If this isn't convincing enough then I will remove it.

Yeah, the UI could be better. With the recent changes exposing media images to Lua, I think this can be handled in a more generic way using a plugin to allow binding keys to remove/reinsert last media. That leaves the question of whether to leave the convenience hack here in the mean time or not.

cuavas · 2021-01-11T14:43:18Z

src/devices/machine/jvsdev.cpp

+int jvs_device::device_handle_message(const uint8_t *send_buffer, uint32_t send_size, uint8_t *&recv_buffer)
+{
+	// Override this function to provide device-specific JVS message handling.
+	// If -1 is returned then the base message handler will be used.
+	return -1;
+}


This is an antipattern. You should provide default message handling in a protected virtual method, and allow derived classes to override it, and call the base method if they don’t handle the message specially.

cuavas · 2021-01-11T14:44:55Z

src/mame/drivers/ksys573.cpp

@@ -375,6 +376,90 @@ G: gun mania only, drives air soft gun (this game uses real BB bullet)

 #define ATAPI_CYCLES_PER_SECTOR ( 5000 )  // plenty of time to allow DMA setup etc.  BIOS requires this be at least 2000, individual games may vary.

+// #define JVS_DEBUG


Please use the stuff from logmacro.h for configurable logging. Especially don’t use printf or its friends, they’re not type-safe, and semantics vary depending on the C standard library.

Looking into this now and I just noticed the rest of the driver uses verboselog. Would it be preferable to just use verboselog to keep with the rest of the driver, or would it be better to update the driver's logging code to use logmacro.h? I'd rather not mix the two so I can update the entire driver's logging code if logmacro.h is the preferable method going forward.

ref:

mame/src/mame/drivers/ksys573.cpp

Lines 744 to 755 in 9839515

void ATTR_PRINTF( 3,4 ) ksys573_state::verboselog( int n_level, const char *s_fmt, ... )

{

if( VERBOSE_LEVEL >= n_level )

{

va_list v;

char buf[ 32768 ];

va_start( v, s_fmt );

vsprintf( buf, s_fmt, v );

va_end( v );

logerror( "%s: %s", machine().describe_context(), buf );

}

}

If you want to work out some categories for the logging that’s already there and convert it, go ahead.

I think the JVS_DEBUG macro is completely redundant now.

I thought I deleted that but it looks like I left the #define in. Thanks.

cuavas · 2021-01-11T14:45:37Z

src/mame/drivers/ksys573.cpp

+DECLARE_DEVICE_TYPE(JVS_MASTER, jvs_master)
+
+class jvs_master : public jvs_host


The names for this device (class name, device type name, short name and description) are too generic for something driver-specific like this.

cuavas · 2021-01-11T14:46:42Z

src/mame/drivers/ksys573.cpp

+		uint8_t checksum = 0;
+		for (int i = 0; i < length; i++) {
+			checksum += data[i];
+		}


This could be done with std::accumulate.

cuavas · 2021-01-11T14:49:14Z

src/mame/drivers/ksys573.cpp

+{
+	if (offset == 0) {
+		// 0x1f400008-0x1f400009 are for inputs
+		return ioport("IN2")->read();


Please don’t add runtime tag map lookups. They’re slow and you don’t benefit from validation.

cuavas · 2021-01-11T14:49:43Z

src/mame/drivers/ksys573.cpp

+	memset(m_jvs_input_buffer, 0, 512);
+	memset(m_jvs_output_buffer, 0, 512);


Please use std::fill in preference to memset.

cuavas · 2021-01-11T14:50:14Z

src/mame/drivers/ksys573.cpp

@@ -2250,6 +2423,8 @@ void ksys573_state::konami573(machine_config &config)

 	adc0834_device &adc(ADC0834(config, "adc0834"));
 	adc.set_input_callback(FUNC(ksys573_state::analogue_inputs_callback));
+
+	JVS_MASTER(config, "jvs_master", 0);


You can use object finders in place of literal tags to make things less error-prone.

cuavas · 2021-01-11T14:51:25Z

src/mame/machine/k573mcr.cpp

+{
+	jvs_device::device_reset();
+
+	memset(m_ram.get(), 0, RAM_SIZE);


Please use std::fill_n in preference to memset.

cuavas · 2021-01-11T14:51:58Z

src/mame/machine/k573mcr.cpp

+
+	for (int i = 0; i < 8; i++) {
+		port->clock_w(0);
+		port->tx_w(!!(data & (1 << i)));


Please use the BIT helper for doing this.

cuavas · 2021-01-11T14:53:47Z

src/mame/machine/k573mcr.h

+	uint16_t m_status;
+	bool m_is_memcard_initialized;
+
+	required_device<psx_controller_port_device> m_ports[2];


required_device_array should be used rather than an array of required_device.

…e names

…om overridden function when required

cuavas · 2021-01-12T14:07:41Z

src/mame/drivers/ksys573.cpp

@@ -375,6 +376,90 @@ G: gun mania only, drives air soft gun (this game uses real BB bullet)

 #define ATAPI_CYCLES_PER_SECTOR ( 5000 )  // plenty of time to allow DMA setup etc.  BIOS requires this be at least 2000, individual games may vary.

+// #define JVS_DEBUG


I think the JVS_DEBUG macro is completely redundant now.

cuavas · 2021-01-12T14:13:26Z

src/mame/machine/k573mcr.cpp

+	PSX_CONTROLLER_PORT(config, "port1", psx_controllers, nullptr);
+	PSX_CONTROLLER_PORT(config, "port2", psx_controllers, nullptr);


This allows the user to select the connected devices. Since this is a sealed-in-a-cabinet system, would it be better to have fixed peripherals set in machine configuration? You can add .set_fixed(true) to each of them to not expose them as user-selectable options.

I think technically allowing the user to select the connected devices is more accurate. Whether or not the game will read it is another story, you can connect any memory card and controller device to the machines. The same machine plate with the memory card and controller ports are used on all supported machines.

Picture:

Oh, I didn’t realise the controller ports were actually on the front panel like that, I thought it was more like the Neo Geo MVS variants with console-style controller ports inside the machine. My bad.

Implementation of JVS memory card readers for System 573

9839515

987123879113 commented Jan 11, 2021

View reviewed changes

cuavas requested changes Jan 11, 2021

View reviewed changes

987123879113 added 9 commits January 12, 2021 00:42

Use std functions when possible

a2082eb

Use required_device_array

afd3128

Use BIT when appropriate

6c4f875

Use logmacro.h throughout ksys573

c492f58

Rename jvs_master to sys573_jvs_host based on other similar JVS devic…

623c278

…e names

Use device finder when possible instead of tag

3a520ca

Remove runtime tag lookups

52b12a6

Expose handle_message as a virtual function and call base function fr…

594d707

…om overridden function when required

Use PSX controller bus instead of commnicating with ports directly

09dc242

cuavas reviewed Jan 12, 2021

View reviewed changes

Remove JVS_DEBUG define

bb96ca5

cuavas merged commit ee6e820 into mamedev:master Jan 12, 2021

987123879113 mentioned this pull request Jan 18, 2021

Improve accuracy of System 573's digital I/O audio emulation #7664

Merged

This was referenced Jan 27, 2021

libretro/mame 0.228 libretro/mame#225

Merged

libretro/mame 0.228 libretro/mame#226

Merged

987123879113 deleted the k573mcr_pr branch September 18, 2021 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

k573mcr: Implementation of JVS memory card reader device for System 573 #7659

k573mcr: Implementation of JVS memory card reader device for System 573 #7659

987123879113 commented Jan 11, 2021

987123879113 Jan 11, 2021

cuavas Jan 11, 2021

987123879113 Jan 12, 2021 •

edited

MooglyGuy Jan 12, 2021

987123879113 Jan 12, 2021

MooglyGuy Jan 12, 2021

987123879113 Jan 11, 2021

cuavas Jan 11, 2021

987123879113 Jan 12, 2021

cuavas Jan 12, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

987123879113 Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 12, 2021

987123879113 Jan 12, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 11, 2021

cuavas Jan 12, 2021

cuavas Jan 12, 2021

987123879113 Jan 12, 2021

cuavas Jan 12, 2021

		@@ -375,6 +376,90 @@ G: gun mania only, drives air soft gun (this game uses real BB bullet)

		#define ATAPI_CYCLES_PER_SECTOR ( 5000 ) // plenty of time to allow DMA setup etc. BIOS requires this be at least 2000, individual games may vary.

		// #define JVS_DEBUG

	void ATTR_PRINTF( 3,4 ) ksys573_state::verboselog( int n_level, const char *s_fmt, ... )
	{
	if( VERBOSE_LEVEL >= n_level )
	{
	va_list v;
	char buf[ 32768 ];
	va_start( v, s_fmt );
	vsprintf( buf, s_fmt, v );
	va_end( v );
	logerror( "%s: %s", machine().describe_context(), buf );
	}
	}

		DECLARE_DEVICE_TYPE(JVS_MASTER, jvs_master)

		class jvs_master : public jvs_host

		memset(m_jvs_input_buffer, 0, 512);
		memset(m_jvs_output_buffer, 0, 512);

		PSX_CONTROLLER_PORT(config, "port1", psx_controllers, nullptr);
		PSX_CONTROLLER_PORT(config, "port2", psx_controllers, nullptr);

k573mcr: Implementation of JVS memory card reader device for System 573 #7659

k573mcr: Implementation of JVS memory card reader device for System 573 #7659

Conversation

987123879113 commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

987123879113 Jan 12, 2021 • edited

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

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

987123879113 Jan 12, 2021 •

edited