Merge pull request #816 from TomHarte/RelaxedTracks

Corrects a regression in disk image handling; liberalises Disk II analyser

Components/DiskII/DiskII.cpp

@@ -191,7 +191,6 @@ void DiskII::set_state_machine(const std::vector<uint8_t> &state_machine) {
 				((source_address&0x02) ? 0x02 : 0x00);
 			uint8_t source_value = state_machine[source_address];
 
-			// Remap into Beneath Apple Pro-DOS value form.
 			source_value =
 				((source_value & 0x80) ? 0x10 : 0x0) |
 				((source_value & 0x40) ? 0x20 : 0x0) |

Machines/Apple/AppleII/DiskIICard.cpp

@@ -20,7 +20,9 @@ DiskIICard::DiskIICard(const ROMMachine::ROMFetcher &rom_fetcher, bool is_16_sec
 	} else {
 		roms = rom_fetcher({
 			{"DiskII", "the Disk II 13-sector boot ROM", "boot-13.rom", 256, 0xd34eb2ff},
-			{"DiskII", "the Disk II 13-sector state machine ROM", "state-machine-13.rom", 256, 0x62e22620 }
+			{"DiskII", "the Disk II 16-sector state machine ROM", "state-machine-16.rom", 256, { 0x9796a238, 0xb72a2c70 } }
+//			{"DiskII", "the Disk II 13-sector state machine ROM", "state-machine-13.rom", 256, 0x62e22620 }
+			/* TODO: once the DiskII knows how to decode common images of the 13-sector state machine, use that instead of the 16-sector. */
 		});
 	}
 	if(!roms[0] || !roms[1]) {

Storage/Disk/Disk.hpp

@@ -61,6 +61,12 @@ class Disk {
 			@returns whether the disk image is read only. Defaults to @c true if not overridden.
 		*/
 		virtual bool get_is_read_only() = 0;
+
+		/*!
+			@returns @c true if the tracks at the two addresses are different. @c false if they are the same track.
+				This can avoid some degree of work when disk images offer sub-head-position precision.
+		*/
+		virtual bool tracks_differ(Track::Address, Track::Address) = 0;
 };
 
 }

Storage/Disk/DiskImage/DiskImage.hpp

@@ -67,6 +67,12 @@ class DiskImage {
 			@returns whether the disk image is read only. Defaults to @c true if not overridden.
 		*/
 		virtual bool get_is_read_only() { return true; }
+
+		/*!
+			@returns @c true if the tracks at the two addresses are different. @c false if they are the same track.
+				This can avoid some degree of work when disk images offer sub-head-position precision.
+		*/
+		virtual bool tracks_differ(Track::Address lhs, Track::Address rhs) { return lhs != rhs; }
 };
 
 class DiskImageHolderBase: public Disk {
@@ -93,6 +99,7 @@ template <typename T> class DiskImageHolder: public DiskImageHolderBase {
 		void set_track_at_position(Track::Address address, const std::shared_ptr<Track> &track);
 		void flush_tracks();
 		bool get_is_read_only();
+		bool tracks_differ(Track::Address lhs, Track::Address rhs);
 
 	private:
 		T disk_image_;

Storage/Disk/DiskImage/DiskImageImplementation.hpp

@@ -58,3 +58,7 @@ template <typename T> std::shared_ptr<Track> DiskImageHolder<T>::get_track_at_po
 template <typename T> DiskImageHolder<T>::~DiskImageHolder() {
 	if(update_queue_) update_queue_->flush();
 }
+
+template <typename T> bool DiskImageHolder<T>::tracks_differ(Track::Address lhs, Track::Address rhs) {
+	return disk_image_.tracks_differ(lhs, rhs);
+}

Storage/Disk/DiskImage/Formats/WOZ.cpp

@@ -112,10 +112,22 @@ int WOZ::get_head_count() {
 }
 
 long WOZ::file_offset(Track::Address address) {
-	// Calculate table position; if this track is defined to be unformatted, return no track.
-	const int table_position = address.head * (is_3_5_disk_ ? 80 : 160) +
-		(is_3_5_disk_ ? address.position.as_int() : address.position.as_quarter());
-	if(track_map_[table_position] == 0xff) return NoSuchTrack;
+	// Calculate table position.
+	int table_position;
+	if(!is_3_5_disk_) {
+		table_position = address.head * 160 + address.position.as_quarter();
+	} else {
+		if(type_ == Type::WOZ1) {
+			table_position = address.head * 80 + address.position.as_int();
+		} else {
+			table_position = address.head + (address.position.as_int() * 2);
+		}
+	}
+
+	// Check that this track actually exists.
+	if(track_map_[table_position] == 0xff) {
+		return NoSuchTrack;
+	}
 
 	// Seek to the real track.
 	switch(type_) {
@@ -125,9 +137,17 @@ long WOZ::file_offset(Track::Address address) {
 	}
 }
 
+bool WOZ::tracks_differ(Track::Address lhs, Track::Address rhs) {
+	const long offset1 = file_offset(lhs);
+	const long offset2 = file_offset(rhs);
+	return offset1 != offset2;
+}
+
 std::shared_ptr<Track> WOZ::get_track_at_position(Track::Address address) {
 	const long offset = file_offset(address);
-	if(offset == NoSuchTrack) return nullptr;
+	if(offset == NoSuchTrack) {
+		return nullptr;
+	}
 
 	// Seek to the real track.
 	std::vector<uint8_t> track_contents;
@@ -194,5 +214,12 @@ void WOZ::set_tracks(const std::map<Track::Address, std::shared_ptr<Track>> &tra
 }
 
 bool WOZ::get_is_read_only() {
-	return file_.get_is_known_read_only() || is_read_only_ || type_ == Type::WOZ2;	// WOZ 2 disks are currently read only.
+	/*
+		There is an unintended issue with the disk code that sites above here: it doesn't understand the idea
+		of multiple addresses mapping to the same track, yet it maintains a cache of track contents. Therefore
+		if a WOZ is written to, what's written will magically be exactly 1/4 track wide, not affecting its
+		neighbours. I've made WOZs readonly until I can correct that issue.
+	*/
+	return true;
+//	return file_.get_is_known_read_only() || is_read_only_ || type_ == Type::WOZ2;	// WOZ 2 disks are currently read only.
 }

Storage/Disk/DiskImage/Formats/WOZ.hpp

@@ -31,6 +31,7 @@ class WOZ: public DiskImage {
 		std::shared_ptr<Track> get_track_at_position(Track::Address address) final;
 		void set_tracks(const std::map<Track::Address, std::shared_ptr<Track>> &tracks) final;
 		bool get_is_read_only() final;
+		bool tracks_differ(Track::Address, Track::Address) final;
 
 	private:
 		Storage::FileHolder file_;

Storage/Disk/Drive.cpp

@@ -80,6 +80,10 @@ bool Drive::get_is_track_zero() const {
 }
 
 void Drive::step(HeadPosition offset) {
+	if(offset == HeadPosition(0)) {
+		return;
+	}
+
 	if(ready_type_ == ReadyType::IBMRDY) {
 		is_ready_ = true;
 	}
@@ -94,7 +98,7 @@ void Drive::step(HeadPosition offset) {
 	}
 
 	// If the head moved, flush the old track.
-	if(head_position_ != old_head_position) {
+	if(disk_ && disk_->tracks_differ(Track::Address(head_, head_position_), Track::Address(head_, old_head_position))) {
 		track_ = nullptr;
 	}
 
@@ -300,11 +304,6 @@ void Drive::get_next_event(float duration_already_passed) {
 		current_event_.type = Track::Event::IndexHole;
 	}
 
-	// Begin a 2ms period of holding the index line pulse active if this is an index pulse event.
-	if(current_event_.type == Track::Event::IndexHole) {
-		index_pulse_remaining_ = Cycles((get_input_clock_rate() * 2) / 1000);
-	}
-
 	// divide interval, which is in terms of a single rotation of the disk, by rotation speed to
 	// convert it into revolutions per second; this is achieved by multiplying by rotational_multiplier_
 	float interval = std::max((current_event_.length - duration_already_passed) * rotational_multiplier_, 0.0f);
@@ -327,6 +326,9 @@ void Drive::process_next_event() {
 			is_ready_ = true;
 		}
 		cycles_since_index_hole_ = 0;
+
+		// Begin a 2ms period of holding the index line pulse active.
+		index_pulse_remaining_ = Cycles((get_input_clock_rate() * 2) / 1000);
 	}
 	if(
 		event_delegate_ &&
@@ -355,8 +357,8 @@ void Drive::setup_track() {
 	}
 
 	float offset = 0.0f;
-	const auto track_time_now = get_time_into_track();
-	const auto time_found = track_->seek_to(Time(track_time_now)).get<float>();
+	const float track_time_now = get_time_into_track();
+	const float time_found = track_->seek_to(track_time_now);
 
 	// `time_found` can be greater than `track_time_now` if limited precision caused rounding.
 	if(time_found <= track_time_now) {

Storage/Disk/Encodings/AppleGCR/Sector.hpp

@@ -21,9 +21,9 @@ struct Sector {
 		Describes the location of a sector, implementing < to allow for use as a set key.
 	*/
 	struct Address {
-		struct {
+		union {
 			/// For Apple II-type sectors, provides the volume number.
-			uint_fast8_t volume = 0;
+			uint_fast8_t volume;
 			/// For Macintosh-type sectors, provides the format from the sector header.
 			uint_fast8_t format = 0;
 		};

Storage/Disk/Encodings/AppleGCR/SegmentParser.cpp

@@ -57,14 +57,14 @@ uint8_t unmap_five_and_three(uint8_t source) {
 	return five_and_three_unmapping[source - 0xab];
 }
 
-std::unique_ptr<Sector> decode_macintosh_sector(const std::array<uint_fast8_t, 8> &header, const std::unique_ptr<Sector> &original) {
-	// There must be at least 704 bytes to decode from.
-	if(original->data.size() < 704) return nullptr;
+std::unique_ptr<Sector> decode_macintosh_sector(const std::array<uint_fast8_t, 8> *header, const std::unique_ptr<Sector> &original) {
+	// There must be a header and at least 704 bytes to decode from.
+	if(!header || original->data.size() < 704) return nullptr;
 
 	// Attempt a six-and-two unmapping of the header.
 	std::array<uint_fast8_t, 5> decoded_header;
 	for(size_t c = 0; c < decoded_header.size(); ++c) {
-		decoded_header[c] = unmap_six_and_two(header[c]);
+		decoded_header[c] = unmap_six_and_two((*header)[c]);
 		if(decoded_header[c] == 0xff) {
 			return nullptr;
 		}
@@ -140,29 +140,32 @@ std::unique_ptr<Sector> decode_macintosh_sector(const std::array<uint_fast8_t, 8
 	return sector;
 }
 
-std::unique_ptr<Sector> decode_appleii_sector(const std::array<uint_fast8_t, 8> &header, const std::unique_ptr<Sector> &original, bool is_five_and_three) {
+std::unique_ptr<Sector> decode_appleii_sector(const std::array<uint_fast8_t, 8> *header, const std::unique_ptr<Sector> &original, bool is_five_and_three) {
 	// There must be at least 411 bytes to decode a five-and-three sector from;
 	// there must be only 343 if this is a six-and-two sector.
 	const size_t data_size = is_five_and_three ? 411 : 343;
 	if(original->data.size() < data_size) return nullptr;
 
-	// Check for apparent four and four encoding.
-	uint_fast8_t header_mask = 0xff;
-	for(auto c : header) header_mask &= c;
-	header_mask &= 0xaa;
-	if(header_mask != 0xaa) return nullptr;
-
-	// Allocate a sector and fill the header fields.
+	// Allocate a sector.
 	auto sector = std::make_unique<Sector>();
 	sector->data.resize(data_size);
 
-	sector->address.volume = ((header[0] << 1) | 1) & header[1];
-	sector->address.track = ((header[2] << 1) | 1) & header[3];
-	sector->address.sector = ((header[4] << 1) | 1) & header[5];
-
-	// Check the header checksum.
-	const uint_fast8_t checksum = ((header[6] << 1) | 1) & header[7];
-	if(checksum != (sector->address.volume^sector->address.track^sector->address.sector)) return nullptr;
+	// If there is a header, check for apparent four and four encoding.
+	if(header) {
+		uint_fast8_t header_mask = 0xff;
+		for(auto c : *header) header_mask &= c;
+		header_mask &= 0xaa;
+		if(header_mask != 0xaa) return nullptr;
+
+		// Fill the header fields.
+		sector->address.volume = (((*header)[0] << 1) | 1) & (*header)[1];
+		sector->address.track = (((*header)[2] << 1) | 1) & (*header)[3];
+		sector->address.sector = (((*header)[4] << 1) | 1) & (*header)[5];
+
+		// Check the header checksum.
+		const uint_fast8_t checksum = (((*header)[6] << 1) | 1) & (*header)[7];
+		if(checksum != (sector->address.volume^sector->address.track^sector->address.sector)) return nullptr;
+	}
 
 	// Unmap the sector contents.
 	for(size_t index = 0; index < data_size; ++index) {
@@ -178,9 +181,6 @@ std::unique_ptr<Sector> decode_appleii_sector(const std::array<uint_fast8_t, 8>
 	}
 	if(sector->data.back()) return nullptr;
 
-	// Having checked the checksum, remove it.
-	sector->data.resize(sector->data.size() - 1);
-
 	if(is_five_and_three) {
 		// TODO: the below is almost certainly incorrect; Beneath Apple DOS partly documents
 		// the process, enough to give the basic outline below of how five source bytes are
@@ -250,6 +250,7 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 	size_t bit = 0;
 	int header_delay = 0;
 	bool is_five_and_three = false;
+	bool has_header = false;
 	while(bit < segment.data.size() || pointer != scanning_sentinel || header_delay) {
 		shift_register = uint_fast8_t((shift_register << 1) | (segment.data[bit % segment.data.size()] ? 1 : 0));
 		++bit;
@@ -290,6 +291,7 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 					sector_location = size_t(bit % segment.data.size());
 					header_delay = 200;	// Allow up to 200 bytes to find the body, if the
 										// track split comes in between.
+					has_header = true;
 				}
 			}
 		} else {
@@ -308,18 +310,19 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 					pointer = scanning_sentinel;
 
 					// Potentially this is a Macintosh sector.
-					auto macintosh_sector = decode_macintosh_sector(header, sector);
+					auto macintosh_sector = decode_macintosh_sector(has_header ? &header : nullptr, sector);
 					if(macintosh_sector) {
 						result.insert(std::make_pair(sector_location, std::move(*macintosh_sector)));
 						continue;
 					}
 
 					// Apple II then?
-					auto appleii_sector = decode_appleii_sector(header, sector, is_five_and_three);
+					auto appleii_sector = decode_appleii_sector(has_header ? &header : nullptr, sector, is_five_and_three);
 					if(appleii_sector) {
 						result.insert(std::make_pair(sector_location, std::move(*appleii_sector)));
 					}
 
+					has_header = false;
 				} else {
 					new_sector->data.push_back(value);
 				}

Storage/Disk/Track/PCMSegment.cpp

@@ -109,9 +109,9 @@ Storage::Time PCMSegmentEventSource::get_length() {
 	return segment_->length_of_a_bit * unsigned(segment_->data.size());
 }
 
-Storage::Time PCMSegmentEventSource::seek_to(const Time &time_from_start) {
+float PCMSegmentEventSource::seek_to(float time_from_start) {
 	// test for requested time being beyond the end
-	const Time length = get_length();
+	const float length = get_length().get<float>();
 	if(time_from_start >= length) {
 		next_event_.type = Track::Event::IndexHole;
 		bit_pointer_ = segment_->data.size()+1;
@@ -122,21 +122,21 @@ Storage::Time PCMSegmentEventSource::seek_to(const Time &time_from_start) {
 	next_event_.type = Track::Event::FluxTransition;
 
 	// test for requested time being before the first bit
-	Time half_bit_length = segment_->length_of_a_bit;
-	half_bit_length.length >>= 1;
+	const float bit_length = segment_->length_of_a_bit.get<float>();
+	const float half_bit_length = bit_length / 2.0f;
 	if(time_from_start < half_bit_length) {
 		bit_pointer_ = 0;
-		return Storage::Time(0);
+		return 0.0f;
 	}
 
 	// adjust for time to get to bit zero and determine number of bits in;
 	// bit_pointer_ always records _the next bit_ that might trigger an event,
 	// so should be one beyond the one reached by a seek.
-	const Time relative_time = time_from_start - half_bit_length;
-	bit_pointer_ = 1 + (relative_time / segment_->length_of_a_bit).get<unsigned int>();
+	const float relative_time = time_from_start + half_bit_length;	// the period [0, 0.5) should map to window 0, ending with bit 0; [0.5, 1.5) should map to window 1; etc.
+	bit_pointer_ = size_t(relative_time / bit_length);
 
-	// map up to the correct amount of time
-	return half_bit_length + segment_->length_of_a_bit * unsigned(bit_pointer_ - 1);
+	// Map up to the correct amount of time; this should be the start of the window that ends upon the bit at bit_pointer_.
+	return bit_length * float(bit_pointer_) - half_bit_length;
 }
 
 const PCMSegment &PCMSegmentEventSource::segment() const {

Storage/Disk/Track/PCMSegment.hpp

@@ -183,7 +183,7 @@ class PCMSegmentEventSource {
 
 			@returns the time the source is now at.
 		*/
-		Time seek_to(const Time &time_from_start);
+		float seek_to(float time_from_start);
 
 		/*!
 			@returns the total length of the stream of data that the source will provide.

Storage/Disk/Track/PCMTrack.cpp

@@ -121,17 +121,17 @@ Track::Event PCMTrack::get_next_event() {
 	return event;
 }
 
-Storage::Time PCMTrack::seek_to(const Time &time_since_index_hole) {
+float PCMTrack::seek_to(float time_since_index_hole) {
 	// initial condition: no time yet accumulated, the whole thing requested yet to navigate
-	Storage::Time accumulated_time;
-	Storage::Time time_left_to_seek = time_since_index_hole;
+	float accumulated_time = 0.0f;
+	float time_left_to_seek = time_since_index_hole;
 
 	// search from the first segment
 	segment_pointer_ = 0;
 	do {
 		// if this segment extends beyond the amount of time left to seek, trust it to complete
 		// the seek
-		Storage::Time segment_time = segment_event_sources_[segment_pointer_].get_length();
+		const float segment_time = segment_event_sources_[segment_pointer_].get_length().get<float>();
 		if(segment_time > time_left_to_seek) {
 			return accumulated_time + segment_event_sources_[segment_pointer_].seek_to(time_left_to_seek);
 		}

Storage/Disk/Track/PCMTrack.hpp

@@ -50,7 +50,7 @@ class PCMTrack: public Track {
 
 		// as per @c Track
 		Event get_next_event() final;
-		Time seek_to(const Time &time_since_index_hole) final;
+		float seek_to(float time_since_index_hole) final;
 		Track *clone() const final;
 
 		// Obtains a copy of this track, flattened to a single PCMSegment, which