06640: coco3, coco3h: TRS-80 Color Computer FDC data corruption issue

ID	Category [?]	Severity [?]	Reproducibility	Date Submitted	Last Update
06640	Core	Major	Always	Aug 1, 2017, 21:38	Aug 4, 2017, 06:43

Tester	MrGW	View Status	Public	Platform	MAME (Official Binary)
Assigned To	Duke	Resolution	Fixed	OS	Windows 10 (32-bit)
Status [?]	Resolved			Driver	trs/coco3.cpp
Version	0.188	Fixed in Version	0.189	Build	Normal
		Fixed in Git Commit		Github Pull Request #

Summary	06640: coco3, coco3h: TRS-80 Color Computer FDC data corruption issue
Description	David Ladd (davidwladd@gmail.com) has discovered a serious data corruption issue when using the FDC with the COCOx drivers in MAME. He originally submitted MAME bug #6639 as part of an earlier issue he saw when running OS9 and accessing /d0 and /d1 drives (under certain circumstances). http://mametesters.org/view.php?id=6639 It looks like this latest discovery is what was causing the earlier issue. In any case, MAME 0.162 was the last version known to work correctly with the FDC. Something changed in MAME 0.163 to "break" the FDC and it's been that way ever since.
Steps To Reproduce	Here's a simple test that will demonstrate the issue: Startup CoCo 2 or CoCo3 emulation Create a new disk image in drive 0 Create a new disk image in drive 1 DSKINI 0 DSKINI 1 Enter a quick basic program like this and save it to disk: 10 PRINT "DIR 0" 20 DIR 0 30 PRINT "DIR 1" 40 DIR 1 50 GOTO 10 SAVE"TEST1.BAS" Then all you have to do to see the problem is to: DIR 0:DIR 1 You will see TEST1.BAS show up twice. Which it should not. The issue is also reproducible using the methods detailed in the videos below:
Additional Information	Credit goes to David Ladd for finding this issue (twice). It was never resolved in the original MAME bug report: http://mametesters.org/view.php?id=6639
Github Commit

Flags
Regression Version
Affected Sets / Systems	coco3, coco3h

Attached Files	test_disks.zip (799 bytes) Aug 1, 2017, 23:53 Uploaded by Tafoid Test Disk Images - disk1 has program, disk2 is empty - both are formatted
Attached Files	coco.diff (683 bytes) Aug 2, 2017, 07:50 Uploaded by Osso test patch [Show Content]

No.14051 Bletch Developer Aug 1, 2017, 22:11	From the MAME 0.163 release notes: - update all drivers to use the new wd floppy controller implementation and remove the legacy one. I'm guessing that this is more than a coincidence.
No.14052 Tafoid Administrator Aug 1, 2017, 23:52	Added disk1 and disk2 images to assist in testing. I've noticed that it doesn't always file the first time, but usually fails the "DIR 0:DIR 1" test the 2nd time
No.14053 star2root Viewer Aug 2, 2017, 02:03	I think it might have to with these changes… 85 src/emu/bus/coco/coco_fdc.c @@ -291,16 +298,17 @@ void coco_fdc_device::dskreg_w(UINT8 data) else if (data & 0x40) drive = 3; - legacy_floppy_image_device floppy[4]; + floppy_image_device floppy[4]; - floppy[0] = subdevice<legacy_floppy_image_device>(FLOPPY_0); - floppy[1] = subdevice<legacy_floppy_image_device>(FLOPPY_1); - floppy[2] = subdevice<legacy_floppy_image_device>(FLOPPY_2); - floppy[3] = subdevice<legacy_floppy_image_device>(FLOPPY_3); + floppy[0] = subdevice<floppy_connector>(WD_TAG ":0")->get_device(); + floppy[1] = subdevice<floppy_connector>(WD_TAG ":1")->get_device(); + floppy[2] = subdevice<floppy_connector>(WD_TAG ":2")->get_device(); + floppy[3] = subdevice<floppy_connector>(WD_TAG ":3")->get_device(); for (int i = 0; i < 4; i++) { - floppy[i]->floppy_mon_w(i == drive ? CLEAR_LINE : ASSERT_LINE); + if (floppy[i]) + floppy[i]->mon_w(i == drive ? CLEAR_LINE : ASSERT_LINE); } head = ((data & 0x40) && (drive != 3)) ? 1 : 0; @@ -309,8 +317,11 @@ void coco_fdc_device::dskreg_w(UINT8 data) update_lines(); - m_wd17xx->set_drive(drive); - m_wd17xx->set_side(head); + m_wd17xx->set_floppy(floppy[drive]); + + if (floppy[drive]) + floppy[drive]->ss_w(head); + m_wd17xx->dden_w(!BIT(m_dskreg, 5)); } @@ -379,7 +390,7 @@ WRITE8_MEMBER(coco_fdc_device::write) dskreg_w(data); break; case 8: - m_wd17xx->command_w(space, 0, data); + m_wd17xx->cmd_w(space, 0, data); break; case 9: m_wd17xx->track_w(space, 0, data); @@ -388,6 +399,7 @@ WRITE8_MEMBER(coco_fdc_device::write) m_wd17xx->sector_w(space, 0, data); break; case 11: + //printf("data w %02x\n", data); m_wd17xx->data_w(space, 0, data); break; }; @@ -413,10 +425,12 @@ WRITE8_MEMBER(coco_fdc_device::write) //************************************************************************** static MACHINE_CONFIG_FRAGMENT(dragon_fdc) - MCFG_DEVICE_ADD(WD2797_TAG, WD2797, 0) - MCFG_WD17XX_DEFAULT_DRIVE4_TAGS + MCFG_WD2797_ADD(WD2797_TAG, XTAL_1MHz) - MCFG_LEGACY_FLOPPY_4_DRIVES_ADD(coco_floppy_interface) + MCFG_FLOPPY_DRIVE_ADD(WD2797_TAG ":0", coco_fdc_floppies, "qd", coco_fdc_device::floppy_formats) + MCFG_FLOPPY_DRIVE_ADD(WD2797_TAG ":1", coco_fdc_floppies, "qd", coco_fdc_device::floppy_formats) + MCFG_FLOPPY_DRIVE_ADD(WD2797_TAG ":2", coco_fdc_floppies, "", coco_fdc_device::floppy_formats) + MCFG_FLOPPY_DRIVE_ADD(WD2797_TAG ":3", coco_fdc_floppies, "", coco_fdc_device::floppy_formats) MACHINE_CONFIG_END @@ -511,10 +525,20 @@ void dragon_fdc_device::dskreg_w(UINT8 data) data); } - if (data & 0x04) - m_wd2797->set_drive(data & 0x03); + floppy_image_device floppy = NULL; + + switch (data & 0x03) + { + case 0: floppy = subdevice<floppy_connector>(WD2797_TAG ":0")->get_device(); break; + case 1: floppy = subdevice<floppy_connector>(WD2797_TAG ":1")->get_device(); break; + case 2: floppy = subdevice<floppy_connector>(WD2797_TAG ":2")->get_device(); break; + case 3: floppy = subdevice<floppy_connector>(WD2797_TAG ":3")->get_device(); break; + } + + m_wd2797->set_floppy(floppy); m_wd2797->dden_w(BIT(data, 3)); + m_dskreg = data; } @@ -556,12 +580,7 @@ WRITE8_MEMBER(dragon_fdc_device::write) switch(offset & 0xEF) { case 0: - m_wd2797->command_w(space, 0, data); - - / disk head is encoded in the command byte / - / Only for type 3 & 4 commands */ - if (data & 0x80) - m_wd2797->set_side((data & 0x02) ? 1 : 0); + m_wd2797->cmd_w(space, 0, data); break; case 1: m_wd2797->track_w(space, 0, data);
No.14054 Osso Moderator Aug 2, 2017, 07:52	Attached a small patch which seems to work for this case, but may actually cause horrible breakage elsewhere since I know nothing about the coco hardware and haven't tested anything else. None the less, it might be useful as a hint of what's going wrong.
No.14055 Haze Senior Tester Aug 2, 2017, 13:33	the original code is definitely suspicious, it looks like it will turn off the motor for every drive other than the one you're trying to access. so in that sense, I'd say your fix looks sane even if I know nothing about the system
No.14057 lostwizard Tester Aug 2, 2017, 19:14 edited on: Aug 2, 2017, 19:22	In case it helps, the correct drive motor behaviour on the Coco FDC is: If the motor enable bit (bit 3 of FF40) is enabled, all drive motors should start and stay on until the motor enable bit is cleared. That is, the drive select lines have no effect on whether a drive is spinning or not. All are or none are. Edit: Also, if the drive motors are turned off (bit 3 of FF40 clear), the drive selects still operate and "enable" the specified drive(s). The drive light actually comes on with the motors turned off on the real hardware in that case. However, I suspect another issue in addition to the drive motor behaviour since the drive not being spun up properly should result in a read failure (CRC fail leading to I/O error or obviously garbled data) rather than the described behaviour.
No.14058 drencorxeen Tester Aug 2, 2017, 20:30	Osso, I just tested the patch you supplied. It doesn't fully address the issue. You can also do a DIR 1:DIR 0 and then the directory on drive 0 doesn't show up correctly. Also you can have up to 4 single side floppy drives or 3 double sided floppy drives connected to a real CoCo.
No.14059 MrGW Tester Aug 2, 2017, 21:18	Ciaran Anscomb (from the Xroar project) provided this comment and patch to try: "They've come up with a patch, but that fails in a different way. Suggest the attached: update motor state on all drives, not just selected." diff --git a/src/devices/bus/coco/coco_fdc.cpp b/src/devices/bus/coco/coco_fdc.cpp index f56bc047c5..cf9bd3038c 100644 --- a/src/devices/bus/coco/coco_fdc.cpp +++ b/src/devices/bus/coco/coco_fdc.cpp @@ -277,7 +277,7 @@ void coco_fdc_device_base::dskreg_w(uint8_t data) { floppy_image_device *floppy = m_floppies[i]->get_device(); if (floppy) - floppy->mon_w(((i == drive) && (data & 0x08)) ? CLEAR_LINE : ASSERT_LINE); + floppy->mon_w((data & 0x08) ? CLEAR_LINE : ASSERT_LINE); } head = ((data & 0x40) && (drive != 3)) ? 1 : 0;
No.14060 Duke Developer Aug 2, 2017, 21:37	Thanks lostwizard, just committed a patch according to your explanation. It seems to fully address the issue here. Would be good if someone could try 06639 now.
No.14063 drencorxeen Tester Aug 2, 2017, 23:39	Just so everyone knows that Ciaran Anscomb patch seems to have fixed the Disk Basic & OS-9/NitrOS-9 problems I ran into and was doing random reading & writing to both drives for a few minutes and did not notice any corruptions at this time. Before if accessing both floppy drives at the same time would crash OS-9 before within a few seconds. The DIFF code Ciaran and supplied to us is as follows: diff --git a/src/devices/bus/coco/coco_fdc.cpp b/src/devices/bus/coco/coco_fdc.cpp index f56bc047c5..cf9bd3038c 100644 --- a/src/devices/bus/coco/coco_fdc.cpp +++ b/src/devices/bus/coco/coco_fdc.cpp @@ -277,7 +277,7 @@ void coco_fdc_device_base::dskreg_w(uint8_t data) { floppy_image_device *floppy = m_floppies[i]->get_device(); if (floppy) - floppy->mon_w(((i == drive) && (data & 0x08)) ? CLEAR_LINE : ASSERT_LINE); + floppy->mon_w((data & 0x08) ? CLEAR_LINE : ASSERT_LINE); } head = ((data & 0x40) && (drive != 3)) ? 1 : 0;
No.14064 star2root Viewer Aug 3, 2017, 01:53 edited on: Aug 3, 2017, 02:16	While we are in this file, I would like suggest a patch that addresses another minor issue. Currently the CoCo's available floppy setting are either none, or quad density. Most CoCo's had either SSDD or DSDD 5 1/4 drives. This patch below set the default number of drives to 3, and adds the ability to select SSDD, DSDD, or QD (quad density) drives as an option. It also includes the patch above to fix the data corruption issue. Ileft the default at QD since due to another existing bug I have not been able to fix yet, you can't change the drive setting until you select "reset" from the slot devices menu and QD drives do work with almost all existing disk images. --- src/devices/bus/coco/coco_fdc.cpp 2017-08-02 01:36:40.000000000 -0400 +++ src/devices/bus/coco/coco_fdc.cpp 2017-08-02 22:13:09.000000000 -0400 @@ -118,6 +118,8 @@ FLOPPY_FORMATS_END static SLOT_INTERFACE_START( coco_fdc_floppies ) + SLOT_INTERFACE("ssdd", FLOPPY_525_SSDD) + SLOT_INTERFACE("dsdd", FLOPPY_525_DD) SLOT_INTERFACE("qd", FLOPPY_525_QD) SLOT_INTERFACE_END @@ -130,7 +132,7 @@ MCFG_FLOPPY_DRIVE_SOUND(true) MCFG_FLOPPY_DRIVE_ADD(WD_TAG ":1", coco_fdc_floppies, "qd", coco_fdc_device_base::floppy_formats) MCFG_FLOPPY_DRIVE_SOUND(true) - MCFG_FLOPPY_DRIVE_ADD(WD_TAG ":2", coco_fdc_floppies, nullptr, coco_fdc_device_base::floppy_formats) + MCFG_FLOPPY_DRIVE_ADD(WD_TAG ":2", coco_fdc_floppies, "qd", coco_fdc_device_base::floppy_formats) MCFG_FLOPPY_DRIVE_SOUND(true) MCFG_FLOPPY_DRIVE_ADD(WD_TAG ":3", coco_fdc_floppies, nullptr, coco_fdc_device_base::floppy_formats) MCFG_FLOPPY_DRIVE_SOUND(true) @@ -273,25 +275,32 @@ else if (data & 0x40) drive = 3; + // Enable/disable the motor line for all drives since it is common + // to all drives. for (int i = 0; i < 4; i++) { floppy_image_device floppy = m_floppies[i]->get_device(); if (floppy) - floppy->mon_w(((i == drive) && (data & 0x08)) ? CLEAR_LINE : ASSERT_LINE); + floppy->mon_w((data & 0x08) ? CLEAR_LINE : ASSERT_LINE); } + // Determine the value of the head (side) select line. head = ((data & 0x40) && (drive != 3)) ? 1 : 0; + // Update the floppy controller register set_dskreg(data); update_lines(); + // Select the floppy drive that is active. floppy_image_device selected_floppy = m_floppies[drive]->get_device(); m_wd17xx->set_floppy(selected_floppy); + // Apply the side select value. if (selected_floppy) selected_floppy->ss_w(head); + // Set the density m_wd17xx->dden_w(!BIT(dskreg(), 5)); }
No.14067 drencorxeen Tester Aug 4, 2017, 06:43	Duke, So after hours of testing it does seem that the patch was made for coco_fdc.cpp has at least resolved this issue at this time.