Merge pull request #731 from TomHarte/ShifterSync

Improves STX track locating, plus minor WD emulation improvements

Components/1770/1770.cpp

@@ -481,7 +481,7 @@ void WD1770::posit_event(int new_event_type) {
 			status.data_request = true;
 		});
 		distance_into_section_++;
-		if(distance_into_section_ == 128 << header_[3]) {
+		if(distance_into_section_ == 128 << (header_[3]&3)) {
 			distance_into_section_ = 0;
 			goto type2_check_crc;
 		}
@@ -564,7 +564,7 @@ void WD1770::posit_event(int new_event_type) {
 		*/
 		write_byte(data_);
 		distance_into_section_++;
-		if(distance_into_section_ == 128 << header_[3]) {
+		if(distance_into_section_ == 128 << (header_[3]&3)) {
 			goto type2_write_crc;
 		}
 

OSBindings/Mac/Clock Signal.xcodeproj/xcshareddata/xcschemes/Clock Signal.xcscheme

@@ -67,7 +67,7 @@
       </Testables>
    </TestAction>
    <LaunchAction
-      buildConfiguration = "Release"
+      buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       enableASanStackUseAfterReturn = "YES"

Storage/Disk/DiskImage/Formats/STX.cpp

@@ -45,24 +45,76 @@ class TrackConstructor {
 
 			/// @returns The byte size of this sector, according to its address mark.
 			uint32_t data_size() const {
-				return uint32_t(128 << address[3]);
+				return uint32_t(128 << (address[3]&3));
 			}
 
+			struct Fragment {
+				int prior_syncs = 1;
+				std::vector<uint8_t> contents;
+			};
+
 			/// @returns The byte stream this sector address would produce if a WD read track command were to observe it.
-			std::vector<uint8_t> get_track_address_image() const {
+			std::vector<Fragment> get_track_address_fragments() const {
-				return track_encoding(address.begin(), address.begin() + 4, {0xa1, 0xa1, 0xfe});
+				return track_fragments(address.begin(), address.begin() + 4, {0xa1, 0xa1, 0xfe});
 			}
 
 			/// @returns The byte stream this sector data would produce if a WD read track command were to observe it.
-			std::vector<uint8_t> get_track_data_image() const {
+			std::vector<Fragment> get_track_data_fragments() const {
-				return track_encoding(contents.begin(), contents.end(), {0xa1, 0xa1, 0xfb});
+				return track_fragments(contents.begin(), contents.end(), {0xa1, 0xa1, 0xfb});
+			}
+
+			/*!
+				Acts like std::search except that it tries to find a start location from which all of the members of @c fragments
+				can be found in successive order with no more than a 'permissible' amount of gap between them.
+
+				Where 'permissible' is derived empirically from trial and error; in practice it's a measure of the number of bytes
+				a WD may produce when it has encountered a false sync, and I don't have documentation on that. So it's
+				derived from in-practice testing of STXs (which, hopefully, contain an accurate copy of what a WD would do,
+				so are themselves possibly a way to research that).
+			*/
+			template <typename Iterator> static Iterator find_fragments(Iterator begin, Iterator end, const std::vector<Fragment> &fragments) {
+				while(true) {
+					// To match the fragments, they must all be found, in order, with at most two bytes of gap.
+					auto this_begin = begin;
+					std::vector<uint8_t>::const_iterator first_location = end;
+					bool is_found = true;
+					bool is_first = true;
+					for(auto fragment: fragments) {
+						auto location = std::search(this_begin, end, fragment.contents.begin(), fragment.contents.end());
+
+						// If fragment wasn't found at all, it's never going to be found. So game over.
+						if(location == end) {
+							return location;
+						}
+
+						// Otherwise, either mark
+						if(is_first) {
+							first_location = location;
+						} else if(location > this_begin + 5*fragment.prior_syncs) {
+							is_found = false;
+							break;
+						}
+
+						is_first = false;
+						this_begin = location + ssize_t(fragment.contents.size());
+					}
+
+					if(is_found) {
+						return first_location;
+					}
+
+					// TODO: can I assume more than this?
+					++begin;
+				}
+				return end;
 			}
 
 			private:
 				/// @returns The effect of encoding @c prefix followed by the bytes from @c begin to @c end as MFM data and then decoding them as if
-				/// observed by a WD read track command.
+				/// observed by a WD read track command, split into fragments separated by any instances of false sync — since it's still unclear to me exactly what
-				template <typename T> static std::vector<uint8_t> track_encoding(T begin, T end, std::initializer_list<uint8_t> prefix) {
+				/// a WD should put out in those instances.
-					std::vector<uint8_t> result;
+				template <typename T> static std::vector<Fragment> track_fragments(T begin, T end, std::initializer_list<uint8_t> prefix) {
+					std::vector<Fragment> result;
 					result.reserve(size_t(end - begin) + prefix.size());
 
 					PCMSegment segment;
@@ -79,17 +131,44 @@ class TrackConstructor {
 						++begin;
 					}
 
-					// Decode, obeying false syncs.
+					// Decode, starting a new segment upon any false sync since I don't have good documentation
+					// presently on exactly how a WD should react to those.
 					using Shifter = Storage::Encodings::MFM::Shifter;
 					Shifter shifter;
 					shifter.set_should_obey_syncs(true);
+					shifter.set_is_double_density(true);
+
+					result.emplace_back();
 
 					// Add whatever comes from the track.
+					int ignore_count = 0;
 					for(auto bit: segment.data) {
 						shifter.add_input_bit(int(bit));
 
-						if(shifter.get_token() != Shifter::None) {
+						const auto token = shifter.get_token();
-							result.push_back(shifter.get_byte());
+						if(token != Shifter::None) {
+							if(ignore_count) {
+								--ignore_count;
+								continue;
+							}
+
+							// If anything other than a byte is encountered,
+							// skip it and the next thing to be reported,
+							// beginning a new fragment.
+							if(token != Shifter::Token::Byte) {
+								ignore_count = 1;
+
+								if(!result.back().contents.empty()) {
+									result.emplace_back();
+								} else {
+									++result.back().prior_syncs;
+								}
+
+								continue;
+							}
+
+							// This was an ordinary byte, retain it.
+							result.back().contents.push_back(shifter.get_byte());
 						}
 					}
 
@@ -127,8 +206,7 @@ class TrackConstructor {
 			// To reconcile the list of sectors with the WD get track-style track image,
 			// use sector bodies as definitive and refer to the track image for in-fill.
 			auto track_position = track_data_.begin();
-			const auto address_mark = {0xa1, 0xa1, 0xfe};
+			const auto sync_mark = {0xa1, 0xa1};
-			const auto track_mark = {0xa1, 0xa1, 0xfb};
 			struct Location {
 				enum Type {
 					Address, Data
@@ -142,35 +220,39 @@ class TrackConstructor {
 			for(const auto &sector: sectors_) {
 				{
 					// Find out what the address would look like, if found in a read track.
-					const auto track_address = sector.get_track_address_image();
+					const auto address_fragments = sector.get_track_address_fragments();
 
 					// Try to locate the header within the track image; if it can't be found then settle for
 					// the next thing that looks like a header of any sort.
-					auto address_position = std::search(track_position, track_data_.end(), track_address.begin(), track_address.end());
+					auto address_position = TrackConstructor::Sector::find_fragments(track_position, track_data_.end(), address_fragments);
 					if(address_position == track_data_.end()) {
-						address_position = std::search(track_position, track_data_.end(), address_mark.begin(), address_mark.end());
+						address_position = std::search(track_position, track_data_.end(), sync_mark.begin(), sync_mark.end());
 					}
 
-					// Stop now if there's nowhere obvious to put this sector.
+					// Place this address only if somewhere to put it was found.
-					if(address_position == track_data_.end()) break;
+					if(address_position != track_data_.end()) {
 						locations.emplace_back(Location::Address, address_position, sector);
 
 						// Advance the track position.
-					track_position = address_position;
+						track_position = address_position + 6;
+					}
 				}
 
 				// Do much the same thing for the data, if it exists.
 				if(!(sector.status & 0x10)) {
-					const auto track_data = sector.get_track_data_image();
+					const auto data_fragments = sector.get_track_data_fragments();
 
-					auto data_position = std::search(track_position, track_data_.end(), track_data.begin(), track_data.end());
+					auto data_position = TrackConstructor::Sector::find_fragments(track_position, track_data_.end(), data_fragments);
 					if(data_position == track_data_.end()) {
-						data_position = std::search(track_position, track_data_.end(), track_mark.begin(), track_mark.end());
+						data_position = std::search(track_position, track_data_.end(), sync_mark.begin(), sync_mark.end());
+					}
+					if(data_position == track_data_.end()) {
+						// Desperation: guess from the given offset.
+						data_position = track_data_.begin() + (sector.bit_position / 16);
 					}
-					if(data_position == track_data_.end()) break;
 
 					locations.emplace_back(Location::Data, data_position, sector);
-					track_position = data_position;
+					track_position = data_position + sector.data_size();
 				}
 			}
 
@@ -184,8 +266,10 @@ class TrackConstructor {
 			auto location = locations.begin();
 			track_position = track_data_.begin();
 			while(location != locations.end()) {
+//				assert(location->position >= track_position && location->position < track_data_.end());
+
 				// Advance to location.position.
-				while(track_position != location->position) {
+				while(track_position < location->position) {
 					encoder->add_byte(*track_position);
 					++track_position;
 				}

Storage/Disk/Encodings/MFM/Shifter.cpp

@@ -25,7 +25,7 @@ void Shifter::set_should_obey_syncs(bool should_obey_syncs) {
 
 void Shifter::add_input_bit(int value) {
 	shift_register_ = (shift_register_ << 1) | static_cast<unsigned int>(value);
-	bits_since_token_++;
+	++bits_since_token_;
 
 	token_ = Token::None;
 	if(should_obey_syncs_) {
@@ -57,6 +57,20 @@ void Shifter::add_input_bit(int value) {
 		} else {
 			switch(shift_register_ & 0xffff) {
 				case Storage::Encodings::MFM::MFMIndexSync:
+					// This models a bit of slightly weird WD behaviour;
+					// if an index sync was detected where it wasn't expected,
+					// the WD will resync but also may return the clock bits
+					// rather than data bits as the next byte read, depending
+					// on framing.
+					//
+					// TODO: make this optional, if ever a Shifter with
+					// well-defined non-WD behaviour is needed.
+					//
+					// TODO: Verify WD behaviour.
+					if(bits_since_token_&1) {
+						shift_register_ >>= 1;
+					}
+
 					bits_since_token_ = 0;
 					is_awaiting_marker_value_ = true;
 

Storage/Disk/Encodings/MFM/Shifter.hpp

@@ -27,6 +27,10 @@ namespace MFM {
 	It will ordinarily honour sync patterns; that should be turned off when within
 	a sector because false syncs can occur. See @c set_should_obey_syncs.
 
+	It aims to implement the same behaviour as WD177x-series controllers when
+	detecting a false sync — the received byte value will be either a 0xc1 or 0x14,
+	depending on phase.
+
 	Bits should be fed in with @c add_input_bit.
 
 	The current output token can be read with @c get_token. It will usually be None but
@@ -62,14 +66,14 @@ class Shifter {
 		}
 
 	private:
-		// Bit stream input state
+		// Bit stream input state.
 		int bits_since_token_ = 0;
 		unsigned int shift_register_ = 0;
 		bool is_awaiting_marker_value_ = false;
 		bool should_obey_syncs_ = true;
 		Token token_ = None;
 
-		// input configuration
+		// Input configuration.
 		bool is_double_density_ = false;
 
 		std::unique_ptr<CRC::CCITT> owned_crc_generator_;