Viewing Issue Advanced Details
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 coco3.cpp
Version 0.188 Fixed in Version 0.189 Build Normal
Summary MESS-specific 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


Flags
Regression Version
Affected Sets / Systems coco3, coco3h
Attached Files
zip file icon 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
? file icon coco.diff (683 bytes) Aug 2, 2017, 07:50 Uploaded by Osso
test patch
[Show Content]
Relationships
related to 06639ResolvedDuke coco3, coco3h: crash of emulated system accessing multiple floppy drives 
Notes
12
User avatar
No.14051
Nathan Woods
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.
User avatar
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
User avatar
No.14053
star2root
Tester
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);
User avatar
No.14054
Osso
Developer
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.
User avatar
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
User avatar
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.

User avatar
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.
User avatar
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;
User avatar
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.
User avatar
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;
User avatar
No.14064
star2root
Tester
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));
 }

User avatar
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.