src/devices/bus/a2bus/a2frob.cpp: Add Apple II frob device by goldnchild · Pull Request #13596 · mamedev/mame

goldnchild · 2025-04-16T16:24:07Z

Adding a driver for the FrobCo Frob Atari 2600 ROM emulator for the Apple II.

https://forums.atariage.com/topic/380267-the-frob-livesagain/

cuavas · 2025-04-21T16:31:50Z

+#include "emu.h"
+
+#include "a2bus.h"
+#include "bus/vcs/rom.h"
+#include "bus/vcs/vcs_slot.h"
+#include "bus/vcs_ctrl/ctrl.h"
+#include "cpu/m6502/m6507.h"
+#include "emupal.h"
+#include "screen.h"
+#include "softlist_dev.h"
+#include "sound/tiaintf.h"
+#include "speaker.h"
+#include "../../mame/atari/tia.h"
+
+#include "machine/mos6530.h"
+
+//#define VERBOSE (LOG_GENERAL)
+#include "logmacro.h"
+
+#include "a2frob.h"


The module’s own header should be the first thing after the prefix header.

Group included headers by module, order from most dependent to least dependent, and sort within groups.

Things that are inline code (e.g. logmacro.h) come after declaration headers.

This is all documented: https://docs.mamedev.org/contributing/cxx.html#structural-organization

Also, you can’t have things in src/devices depending on things in src/mame.

ok, I've resorted the header, I hope it's properly ordered now.

ok, I've copied the tia from src/mame/atari/tia.cpp into a file a2frobtia.cpp so there will be no cross dependency.

cuavas · 2025-04-21T16:33:40Z

+
+	virtual bool take_c800() override { return false; }
+
+	u8 m_frob_mem[0x1000] = { 0 };  // 4k frob memory
+
+	u8 read_frob_mem(offs_t offset) { return !m_frob_apple_control ? 0 : m_frob_mem[offset & 0xfff]; }
+
+	void write_frob_mem(offs_t offset, u8 data) { m_frob_mem[offset & 0xfff] = data; }


Please don’t intermix data members and member functions. Keep each grouped separately.

cuavas · 2025-04-21T16:36:42Z

+public:
+	void a2600_write_fff0(offs_t offset, u8 data)
+	{
+		m_byte_for_apple = data;
+		m_byte_waiting_flag_apple = 1;
+	}
+
+	u8 a2600_read_fff2(offs_t offset)
+	{
+		if (!machine().side_effects_disabled()) m_byte_waiting_flag_vcs = 0;
+		return m_byte_for_vcs;
+	}
+
+	u8 a2600_read_fff1(offs_t offset)
+	{
+		return (m_byte_waiting_flag_apple ? 0 : 1) << 7 | (m_byte_waiting_flag_vcs << 6);
+	}


Why have you made this stuff public?

Also, you need to use synchronize barriers to get data from one CPU to the other to avoid things being seen in the wrong order. The card CPU should see the flag indicating that the mailbox for the host CPU is full as soon as it writes it, but the host CPU should not see the flag or the updated value until your on the other side of a sync barrier. The same applies for communication in the opposite direction.

synchronize() doesn't work the opposite direction*, some drivers do trickery with machine().scheduler().perfect_quantum(x) to make sure the 'opposite' side comms answer timing is correct.

*:
scheduler runs CPUs in serial, CPU A, then CPU B.
let's say CPU A: 1ms.
scheduler runs CPU B.
At 0.5ms point, CPU B wants to write to a latch that shared with CPU A. CPU A however, is already in the future (from CPU B perspective). And if synchronize() is used here, AFAIK it will be delayed until another 0.5ms has passed. If CPU B were to verify its own written latch value during this time, things will go wrong. -- actually, not sure what would happen here. I think CPU B -> CPU C comms here would go fine with a synchronize()

And to solve that properly (since you don’t know in advance whether a CPU is going to attempt to communicate across scheduling domains), you need an efficient way to roll back and retry a smaller timeslice until you get an “acceptable” delay. But since the CPUs themselves don’t know who’s ahead at any given time, putting both directions through synchronize at least means that whichever CPU is ahead will try to end its timeslice early when possible.

Is it possible for CPU B to be ahead of CPU A? And I don't mean slight deviations with icount < 0.

eg. you have 3 CPUs in a driver. Then the scheduler runs the CPUs in this order A,B.C.A,B,C,A,B,C, etc. (or A,B,C,B,C,B,C if CPU B does a synchronize()). It doesn't eenie meenie miney mo them. CPU B is never ahead of CPU A. CPU C is never ahead of CPU A or B.
The order is the same as the machine config order. It's why for arcade drivers with a 1-way soundlatch, it's important to list the audiocpu after maincpu.

cuavas · 2025-04-21T16:39:45Z

+u8 a2600_frob_base_device::read_c0nx(u8 offset)
+{
+	// offset 0 is read from frob status  (bits 7 = write ok, 6 = read ok) (other bits will float)
+	switch (offset & 0x1)
+	{
+		case 0:
+			return ((m_byte_waiting_flag_vcs ? 0 : 1) << 7) |
+					(m_byte_waiting_flag_apple << 6);
+			break;
+		case 1:
+			if (!machine().side_effects_disabled())
+			{
+				m_byte_waiting_flag_apple = 0; // clear byte waiting for apple
+			}
+			return m_byte_for_apple;
+		default:
+			return 0;
+	}
+}


Comment about needing sync barriers applies. Host CPU should see the flag indicating the mailbox for the card CPU is full as soon as it writes it, but should not see the mailbox full from the card CPU until it’s on the other side of a sync barrier.

ok, I have put in some synchronize calls to do the sync, I hope that that will work.

cuavas · 2025-04-21T16:40:31Z

+			m_bidirectional_active = BIT(data, 5);
+			m_frob_page = BIT(data, 0, 4);
+//          printf("m_frob_apple_control = %x  m_frob_page = %x\n",m_frob_apple_control, m_frob_page);
+			break;
+		case 1:


Use logmacro.h helpers for configurable logging, not commented prints. Commented code always rots.

cuavas · 2025-04-21T16:41:00Z

+void a2600_frob_base_device::switch_A_w(uint8_t data)
+{
+	/* Left controller port */
+	m_joy1->joy_w( data >> 4 );
+
+	/* Right controller port */
+	m_joy2->joy_w( data & 0x0f );
+}


Please use consistent formatting for function calls.

goldnchild added 2 commits April 14, 2025 21:47

src/devices/bus/a2bus/a2frob.cpp: Add Apple II frob device

e36e5da

bus/a2bus/a2frob.cpp: cleanups

b95e985

cuavas requested changes Apr 21, 2025

View reviewed changes

goldnchild added 2 commits May 4, 2025 22:41

a2frob.cpp: add a2frobtia.cpp and cleanups

eba9222

a2frob.cpp: fix duplicate name to pass validation

e3c11b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src/devices/bus/a2bus/a2frob.cpp: Add Apple II frob device#13596

src/devices/bus/a2bus/a2frob.cpp: Add Apple II frob device#13596
goldnchild wants to merge 4 commits intomamedev:masterfrom
goldnchild:frob

goldnchild commented Apr 16, 2025

Uh oh!

cuavas Apr 21, 2025

Uh oh!

goldnchild May 5, 2025

Uh oh!

cuavas Apr 21, 2025

Uh oh!

goldnchild May 5, 2025

Uh oh!

cuavas Apr 21, 2025

Uh oh!

happppp Apr 21, 2025 •

edited

Loading

Uh oh!

cuavas Apr 21, 2025

Uh oh!

happppp Apr 21, 2025

Uh oh!

cuavas Apr 21, 2025

Uh oh!

goldnchild May 5, 2025

Uh oh!

cuavas Apr 21, 2025

Uh oh!

cuavas Apr 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

goldnchild commented Apr 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

happppp Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

happppp Apr 21, 2025 •

edited

Loading