mame
mame copied to clipboard
mamedev
imagedev/floppy.cpp: Add support for hard-sectored floppy disks
Here is an alternative to the PR adding floppy hard-sectored support - https://github.com/mamedev/mame/pull/11705.
There are 2 key differences between this PR and the existing open PR linked above:
- It uses a separate timer for the sector holes. This leave the index hole logical 100% untouched, this should help avoid any regression in the soft-sectored floppy handling.
- The decision for hard-sectored processing is in the floppy image, not the drive. This better mirrors the real hardware. The floppy drives are 100% changeable between the hard-sectored and soft-sectored controllers.
@ejona86 @galibert I've opened this to get some feedback on this approach. I haven't been able to test with the hard-sectored logic, since I don't have any vector graphic disk images, but I did test the existing soft-sectored handling and didn't see any problem.
This leave the index hole logical 100% untouched, this should help avoid any regression in the soft-sectored floppy handling.
That's not a problem, but I will note that there were no logic failures previously. The issues were with floating point.
The decision for hard-sectored processing is in the floppy image, not the drive.
I went more with this approach originally as well, where the drive isn't told the "supported" hard sector types. But it doesn't work for mfi files. galibert pointed out you can't create a new mfi disk image and use it with that approach. That's why I informed the drive of what image formats we are expecting so that fs_unformatted can initialize a new mfi. Originally I had been fine if mfi didn't work, but that wasn't acceptable. By adding FSI_525_DSQD16 and related, the hard sectoring was saved in mfi.
since I don't have any vector graphic disk images
If you want, you can use 6000-0022-vector-4-cpm-v1.4r5.vgi in flop2. Press f to boot from floppy. The image includes format.com if you want to format a blank disk (flop1 will be b:).
This leave the index hole logical 100% untouched, this should help avoid any regression in the soft-sectored floppy handling.
That's not a problem, but I will note that there were no logic failures previously. The issues were with floating point.
Sorry, didn't mean to imply there was any logic issue with the change, just that with the change it that function it caused a regression, which seems to be causing extra concern among the mame devs and delaying the merging of the reverted PR.
The decision for hard-sectored processing is in the floppy image, not the drive.
I went more with this approach originally as well, where the drive isn't told the "supported" hard sector types. But it doesn't work for mfi files. galibert pointed out you can't create a new mfi disk image and use it with that approach. That's why I informed the drive of what image formats we are expecting so that fs_unformatted can initialize a new mfi. Originally I had been fine if mfi didn't work, but that wasn't acceptable. By adding FSI_525_DSQD16 and related, the hard sectoring was saved in mfi.
Is the hard-sectoring saved in the mfi file? I didn't see any changes to that code. Maybe there is some other way to allow this to work with MFI files, while using standard drives. Looking back at the thread you linked to, I did want to point out that there was a setup for the H89 that supported both soft and hard-sectored disks in the same drives. The Percom Z controller allowed the drives to share the connect to the drives with the hard-sectored controller. Ad for that controller can be found here - https://archive.org/details/kilobaudmagazine-1982-01/page/n1/mode/2up
since I don't have any vector graphic disk images
If you want, you can use 6000-0022-vector-4-cpm-v1.4r5.vgi in flop2. Press
fto boot from floppy. The image includesformat.comif you want to format a blank disk (flop1 will beb:).
Thanks, I'll try that out.
Sorry, didn't mean to imply there was any logic issue with the change
Yeah, I didn't think you did. It made sense to me. But I just didn't want others to get the wrong impression.
Is the hard-sectoring saved in the mfi file?
Yes, by the FSI_525_DSQD16 and other integers. The specific integer for the image is saved in the mfi. So it wasn't stored in a new field, but was stored.
I did want to point out that there was a setup for the H89 that supported both soft and hard-sectored disks in the same drives
The approach taken isn't really bad in practice. Pre-existing images could still be of either type, as a drive could support multiple types. H89 could read existing soft and hard images without trouble. The only question is what happens with new files created by mame itself; there's no UI to specify so some choice will be taken without user input. For systems only supporting soft or hard, that works perfectly. For systems that support both, you'll be wrong half the time, but that's the case no matter the approach. (Except for more invasive changes like not allowing images to support both hard and soft sectors. So you'd have a separate format for hard sector: mfi vs mfihs. That is quite ugly to workaround such a UI limitation.) There's the workaround of copying a blank image file of the correct type, which doesn't seem too bad.
Ok, so variant (and form_factor) is stored in the header portion of MFI file. But it is not storing the encoding field, and I didn't see any place for a version number or an way to extend it to add a new sectoring field in a backward compatible way.
With these changes, I'm still having problems booting. Pushing the current changes out, if anyone wants to look at it. I'll continue trying to get it working.
Getting closer, the previous code would not read sectors, since the sector hole was offset from the sector data. Now the track is rotated, and most sectors are read properly. There is still occasional glitches when reading sector 15, which crosses rotation time going from 200'000'000 back to zero.
With the track rotated, the errors always occur on sector 15, and on byte 154 of that sector which occurs right at the index hole and the 200'000'000 -> 0 rollover. On the first side of 6000-0022-vector-4-cpm-v1.4r5.vgi, 10 of the 77 tracks had the error. By setting bit 1 to 1 for those particular bytes, the first side could be read completely without error. Although many of the bytes in the 154th position of sector 15 already had bit 0 set to zero, there were some bytes that are read properly and has bit 1 set to 1.
Formatting a blank disk is working, it properly shows "NO FILE" but attempting to copy files, a few copied properly, but then failed on the copy.
Very interesting, let me know how I can repro the failures, I'll try to track down the problem.
I'll push the debug code I have, which allows the system to boot. The instructions Eric provided above https://github.com/mamedev/mame/pull/11952#issuecomment-1902186619 should get it booting. I was able to use the format command to create an empty disk. I then tried copying all the files with PIP - PIP B:=A:*.*. And I also tried using the diskcopy command to do a full disk copy.
The problem is the floppy hiccups when passing over the start of track. I don't know if that's because of the std::rotate or elsewhere in the floppy emulation. It is clear you spent some time fighting that, and you knew that bit flips were happening.
But we really don't want the middle of the last track to span the index. The start of hard sector tracks is the sector 0 pulse. The sector 0 pulse is the index pulse equivalent for soft sectors. It is confusing naming because the hard sector "index" denotes which sector pulse is the index pulse.
Swapping back to sector 0 being the start of track fixed the problem you were having with corruption in the last sector. I re-combined sector and index callbacks; I agree it is good to have the timers and code separate, but that doesn't mean they can't share a callback. I didn't fully clean that up, so the sector callback still exists but isn't called.
position %= ROTATION_POS_MAX was needed because of the continued rounding issues with double. Even though delta < m_rev_time, I still saw times position == ROTATION_POS_MAX. I removed the adjustment of m_revolution_start_time in sector_hole_resync since index_resync should handle that.
diff --git a/src/devices/bus/s100/vectordualmode.cpp b/src/devices/bus/s100/vectordualmode.cpp
index 14c1f83781..5455b5539b 100644
--- a/src/devices/bus/s100/vectordualmode.cpp
+++ b/src/devices/bus/s100/vectordualmode.cpp
@@ -203,7 +203,6 @@ void s100_vector_dualmode_device::s100_sout_w(offs_t offset, uint8_t data)
flop->stp_w(!step);
flop->dir_w(!step_in);
flop->setup_index_pulse_cb(floppy_image_device::index_pulse_cb(&s100_vector_dualmode_device::floppy_index_cb, this));
- flop->setup_sector_pulse_cb(floppy_image_device::index_pulse_cb(&s100_vector_dualmode_device::floppy_sector_hole_cb, this));
}
} else if (offset == 0xc1) { // control (1) port
m_sector = BIT(data, 0, 5);
@@ -228,30 +227,24 @@ bool s100_vector_dualmode_device::get_next_bit(attotime &tm, const attotime &lim
return true;
}
-void s100_vector_dualmode_device::floppy_sector_hole_cb(floppy_image_device *floppy, int state)
-{
- if (hdd_selected() || m_floppy[m_drive]->get_device() != floppy)
- return;
- if (!state)
- return;
-
- m_last_sector_pulse = machine().time();
- m_fdd_sector_counter++;
- m_fdd_sector_counter &= 0xf;
-
- LOGSECTOR("floppy_sector_hole_cb: %d\n", m_fdd_sector_counter);
- start_of_sector();
-}
-
void s100_vector_dualmode_device::floppy_index_cb(floppy_image_device *floppy, int state)
{
if (hdd_selected() || m_floppy[m_drive]->get_device() != floppy)
return;
if (!state)
return;
-
- LOGINDEX("index_cb\n");
- m_fdd_sector_counter = 0xf;
+ attotime now = machine().time();
+ // U25 74LS221: 61.9 KOhm * .22 uF * .75
+ if (now - m_last_sector_pulse < attotime::from_nsec(10213500)) {
+ m_fdd_sector_counter = 0xf;
+ LOGINDEX("index_cb: index\n");
+ } else {
+ m_last_sector_pulse = now;
+ m_fdd_sector_counter++;
+ m_fdd_sector_counter &= 0xf;
+ LOGINDEX("index_cb: sector %d\n", m_fdd_sector_counter);
+ start_of_sector();
+ }
}
void s100_vector_dualmode_device::start_of_sector()
@@ -314,7 +307,7 @@ TIMER_CALLBACK_MEMBER(s100_vector_dualmode_device::byte_cb)
if (m_cmar == 1) {
current_track = data;
- }
+ }/*
else if ((current_track == 2) && (m_sector == 15) && (data == 0x11) && (m_cmar == 154))
{
data = 0x13;
@@ -364,7 +357,7 @@ TIMER_CALLBACK_MEMBER(s100_vector_dualmode_device::byte_cb)
{
data = 0x03;
LOGREAD("updated byte_cb: m_ram[%d] = 0x%02x\n", m_cmar, data);
- }
+ }*/
// side 2
// this one can't be used without adding code to determine the side. The first side also
// has the data == 0x21, but that is correct, and with this code it would cause it to fail
@@ -412,7 +405,6 @@ void s100_vector_dualmode_device::device_start()
if (f->get_device()) {
auto flop = f->get_device();
flop->setup_index_pulse_cb(floppy_image_device::index_pulse_cb(&s100_vector_dualmode_device::floppy_index_cb, this));
- flop->setup_sector_pulse_cb(floppy_image_device::index_pulse_cb(&s100_vector_dualmode_device::floppy_sector_hole_cb, this));
}
}
diff --git a/src/devices/bus/s100/vectordualmode.h b/src/devices/bus/s100/vectordualmode.h
index 8cfae0f004..2df40dd819 100644
--- a/src/devices/bus/s100/vectordualmode.h
+++ b/src/devices/bus/s100/vectordualmode.h
@@ -30,7 +30,6 @@ private:
bool hdd_selected();
bool get_next_bit(attotime &tm, const attotime &limit);
void floppy_index_cb(floppy_image_device *floppy, int state);
- void floppy_sector_hole_cb(floppy_image_device *floppy, int state);
void start_of_sector();
required_device_array<floppy_connector, 4> m_floppy;
diff --git a/src/devices/imagedev/floppy.cpp b/src/devices/imagedev/floppy.cpp
index 0ade49fd3a..d557cc7657 100644
--- a/src/devices/imagedev/floppy.cpp
+++ b/src/devices/imagedev/floppy.cpp
@@ -980,6 +980,10 @@ TIMER_CALLBACK_MEMBER(floppy_image_device::index_resync)
}
}
+/* The hard sector pulse for sector 0 is the soft-sector index pulse (start of
+ * track). This needs to add a pulse for every sector but sector 0, and it
+ * needs to add the hard sector index pulse in the middle of the last sector.
+ */
TIMER_CALLBACK_MEMBER(floppy_image_device::sector_hole_resync)
{
if ((m_phys_sectors == 0) || m_mon) {
@@ -990,8 +994,8 @@ TIMER_CALLBACK_MEMBER(floppy_image_device::sector_hole_resync)
if (m_sector_hole) {
m_sector_hole = 0;
- if (!m_cur_sector_pulse_cb.isnull())
- m_cur_sector_pulse_cb(this, m_sector_hole);
+ if (!m_cur_index_pulse_cb.isnull())
+ m_cur_index_pulse_cb(this, m_sector_hole);
}
return;
@@ -1001,21 +1005,20 @@ TIMER_CALLBACK_MEMBER(floppy_image_device::sector_hole_resync)
logerror("sector_hole_resync\n");
int sector_spacing = ROTATION_POS_MAX / m_phys_sectors;
- int start_sector_offset = sector_spacing / 2;
attotime delta = machine().time() - m_revolution_start_time;
- while (delta >= m_rev_time) {
- delta -= m_rev_time;
- m_revolution_start_time += m_rev_time;
- m_revolution_count++;
- }
int position = int(delta.as_double() * m_angular_speed + 0.5);
+ position %= ROTATION_POS_MAX;
- int offset_position = position - start_sector_offset;
- int sector_offset = offset_position % sector_spacing;
+ int sector = position / sector_spacing;
+ int sector_offset = position % sector_spacing;
+ if (sector == m_phys_sectors-1) {
+ sector_spacing /= 2;
+ sector_offset = position % sector_spacing;
+ }
if (TRACE_SECTORING) {
- logerror("delta: %s - pos: %d - offset_pos: %d - sector_offset: %d\n", delta.to_string(), position, offset_position, sector_offset);
+ logerror("delta: %s - pos: %d - sector: %d - sector_offset: %d\n", delta.to_string(), position, sector, sector_offset);
}
int new_sector_hole = (sector_offset < HOLE_LENGTH);
@@ -1035,13 +1038,13 @@ TIMER_CALLBACK_MEMBER(floppy_image_device::sector_hole_resync)
if (TRACE_SECTORING)
logerror("new_sector_hole: new_sector_hole: %d, next_event: %s - adjust: %s\n", new_sector_hole, next_event_time.to_string(), adjust_time.to_string());
- if (new_sector_hole != m_sector_hole) {
+ if (sector != 0 && new_sector_hole != m_sector_hole) {
if (TRACE_SECTORING)
logerror("change in sector_hole: new: %d - old: %d\n", new_sector_hole, m_sector_hole);
m_sector_hole = new_sector_hole;
- if (!m_cur_sector_pulse_cb.isnull())
- m_cur_sector_pulse_cb(this, m_sector_hole);
+ if (!m_cur_index_pulse_cb.isnull())
+ m_cur_index_pulse_cb(this, m_sector_hole);
}
}
diff --git a/src/lib/formats/vgi_dsk.cpp b/src/lib/formats/vgi_dsk.cpp
index a625ea4df4..ae6cd53018 100644
--- a/src/lib/formats/vgi_dsk.cpp
+++ b/src/lib/formats/vgi_dsk.cpp
@@ -15,10 +15,8 @@ http://www.bitsavers.org/pdf/micropolis/metafloppy/1084-01_1040_1050_Users_Manua
#include "vgi_dsk.h"
#include "ioprocs.h"
-#include "imageutl.h"
#include <cstring>
-#include <algorithm>
static const int track_size = 100'000;
static const int half_bitcell_size = 2000;
@@ -87,8 +85,6 @@ bool micropolis_vgi_format::load(util::random_read &io, uint32_t form_factor, co
while (buf.size() < track_size/16 * (sector+1))
mfm_w(buf, 8, 0, half_bitcell_size);
}
- LOG_FORMATS("loading disk: head: %d, track: %d, size: %ld\n", head, track, buf.size());
- std::rotate(buf.rbegin(), buf.rbegin() + (buf.size()/32), buf.rend());
generate_track_from_levels(track, head, buf, 0, image);
buf.clear();
}
The problem is the floppy hiccups when passing over the start of track. I don't know if that's because of the
std::rotateor elsewhere in the floppy emulation. It is clear you spent some time fighting that, and you knew that bit flips were happening.But we really don't want the middle of the last track to span the index. The start of hard sector tracks is the sector 0 pulse. The sector 0 pulse is the index pulse equivalent for soft sectors. It is confusing naming because the hard sector "index" denotes which sector pulse is the index pulse.
I still think we should try to find the cause of the "hiccups", for several reasons:
- it could be causing problems with other formats which do not use the index hole to sync track data. This system showing a consistent repro of the issue should help track down the root cause.
- I think the overall logic is cleaner, and we don't have to check to see if the hole detected is the index or one of the sector holes. This should make it easier to add other hard-sectored formats.
- Looking at the limited docs for MFI, it states that the track data is always aligned to the index hole - https://github.com/mamedev/mame/blob/master/src/lib/formats/mfi_dsk.cpp#L45-L46
I had a few guesses what is causing the errors at the index 0, such as MFM needing to look back for encoding/decoding, the PLL logic may not handle the wrap around correctly, or the modification of the data to make sure it is exactly the 200,000,000 length. I'm hoping @galibert will have some time to look into the issue. I think it is worthwhile to investigate this further to see if we can fix the issue when the index hole starts the track.
There are some TBD issues, like the MFI file format, that doesn't have a version field or a clean way to represent that it is a hard-sectored disk. I don't like having the floppy drive being defined as a hard-sectored floppy disk drive, since that does not match up with real floppy drives.
Looking into it for other formats is fair. But for hard sectoring itself, sector 0 should be the start of the track. Yes, it makes that one function simpler to change the start of the track, but it makes everything else more complicated, especially debugging. All hard sector formats align to sector boundaries; not a single one uses the position of the "index" hole. For MFI, we can just change the docs. It doesn't actually support hard sectors today, and that documentation is based on a misunderstanding of hard sectors. (Again, "index hole" means different things for hard sector and soft sector. The "index" in hard sector is there to tell you which sector hole is sector 0. It is like a pointer. It does nothing by itself and is not used for timing in any system.)
The floating point calculations causes inaccuracies that are hard to address without a test suite. That's why my previous attempt that increased precision caused regressions. This fixes the wraparound issue when trying to boot:
diff --git a/src/devices/imagedev/floppy.cpp b/src/devices/imagedev/floppy.cpp
index 0ade49fd3a..a5b74cf476 100644
--- a/src/devices/imagedev/floppy.cpp
+++ b/src/devices/imagedev/floppy.cpp
@@ -1230,8 +1226,7 @@ uint32_t floppy_image_device::find_position(attotime &base, const attotime &when
if (res >= ROTATION_POS_MAX) {
// Due to rounding errors in the previous operation,
// 'res' sometimes overflows 2E+8
- res -= ROTATION_POS_MAX;
- base += m_rev_time;
+ res = ROTATION_POS_MAX-1;
}
return res;
}
That does make sense to start the track at sector hole 0. I know in my own emulators, that is what I did. Unless @galibert wants to have the tracks start at the index hole, we should probably just go with your PR. My only hesitation is the requirement for special hard-sectored floppy drives. I still think there should be a way to not require special drives. 🤔 maybe have some function the controller can call on the drive to inform it that it is connected to a hard-sectored controller. I think that would be cleaner than having to duplicate all the drives for hard-sector.
This is now working with sector 0 starting at the 0 index of the track data, instead of having the index hole at the start of the track data. Everything appears to be working, tested a soft-sector system (h89), and tested the Vector 4, both with .vgi and .mfi files. Both loading and saving files. The one thing I see as an advantage over the other hard-sectored PR, is that floppy drives do not need to be cloned for hard-sectored use. Also index hole processing is separate from the sector hole processing.
The "sectoring" FourCC is a no, like I told before.
Sorry, I'm not sure what you mean by that. I don't see any related comments on my PR, and I looked through the comments on the other hard-sectored PRs, but can't figure out what that means.