From 8aeebdbc991b633e5dd1d01d7d287660181e0f04 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Thu, 16 Jul 2020 23:26:45 -0400 Subject: [PATCH 01/11] Remove redundant comment. --- Components/DiskII/DiskII.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/Components/DiskII/DiskII.cpp b/Components/DiskII/DiskII.cpp index e223f908d..dc8e6029e 100644 --- a/Components/DiskII/DiskII.cpp +++ b/Components/DiskII/DiskII.cpp @@ -191,7 +191,6 @@ void DiskII::set_state_machine(const std::vector &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) | From cbb0594e6b19713cc6aacd6d522d21bed931e585 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Thu, 16 Jul 2020 23:27:27 -0400 Subject: [PATCH 02/11] Use 16-sector state machine even with the 13-sector boot ROM. I think I've proven that the Disk II doesn't decode the 13-sector state machine correctly. Work to do there. --- Machines/Apple/AppleII/DiskIICard.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Machines/Apple/AppleII/DiskIICard.cpp b/Machines/Apple/AppleII/DiskIICard.cpp index ef9bd238d..7f0bd7a60 100644 --- a/Machines/Apple/AppleII/DiskIICard.cpp +++ b/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]) { From 4ee29b3266d89e0fbb8aee3f2de41eb53ddff962 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 17 Jul 2020 22:08:58 -0400 Subject: [PATCH 03/11] Switches disk seeking logic fully to floating point. --- Storage/Disk/Drive.cpp | 10 +++++++--- Storage/Disk/Track/PCMSegment.cpp | 16 ++++++++-------- Storage/Disk/Track/PCMSegment.hpp | 2 +- Storage/Disk/Track/PCMTrack.cpp | 8 ++++---- Storage/Disk/Track/PCMTrack.hpp | 2 +- Storage/Disk/Track/TrackSerialiser.cpp | 4 ++-- Storage/Disk/Track/UnformattedTrack.cpp | 4 ++-- Storage/Disk/Track/UnformattedTrack.hpp | 2 +- 8 files changed, 26 insertions(+), 22 deletions(-) diff --git a/Storage/Disk/Drive.cpp b/Storage/Disk/Drive.cpp index ae7c76c56..f74b5a152 100644 --- a/Storage/Disk/Drive.cpp +++ b/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; } @@ -355,8 +359,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(); + 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) { diff --git a/Storage/Disk/Track/PCMSegment.cpp b/Storage/Disk/Track/PCMSegment.cpp index 36e644bfa..31900965b 100644 --- a/Storage/Disk/Track/PCMSegment.cpp +++ b/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(); 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(); + 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(); + const float relative_time = time_from_start + half_bit_length; // the period [0, 0.5) should map to window 0; [0.5, 1.5) should map to window 1; etc. + bit_pointer_ = 1 + 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); + return half_bit_length + segment_->length_of_a_bit.get() * float(bit_pointer_ - 1); } const PCMSegment &PCMSegmentEventSource::segment() const { diff --git a/Storage/Disk/Track/PCMSegment.hpp b/Storage/Disk/Track/PCMSegment.hpp index 64fe9e7ef..38389eb96 100644 --- a/Storage/Disk/Track/PCMSegment.hpp +++ b/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. diff --git a/Storage/Disk/Track/PCMTrack.cpp b/Storage/Disk/Track/PCMTrack.cpp index bf4f6a9de..19bb7599c 100644 --- a/Storage/Disk/Track/PCMTrack.cpp +++ b/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(); if(segment_time > time_left_to_seek) { return accumulated_time + segment_event_sources_[segment_pointer_].seek_to(time_left_to_seek); } diff --git a/Storage/Disk/Track/PCMTrack.hpp b/Storage/Disk/Track/PCMTrack.hpp index 5012820a3..888dc1688 100644 --- a/Storage/Disk/Track/PCMTrack.hpp +++ b/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 diff --git a/Storage/Disk/Track/TrackSerialiser.cpp b/Storage/Disk/Track/TrackSerialiser.cpp index ca673cc36..851074b58 100644 --- a/Storage/Disk/Track/TrackSerialiser.cpp +++ b/Storage/Disk/Track/TrackSerialiser.cpp @@ -35,7 +35,7 @@ Storage::Disk::PCMSegment Storage::Disk::track_serialisation(const Track &track, length_multiplier.simplify(); // start at the index hole - track_copy->seek_to(Time(0)); + track_copy->seek_to(0.0f); // grab events until the next index hole Time time_error = Time(0); @@ -54,7 +54,7 @@ Storage::Disk::PCMSegment Storage::Disk::track_serialisation(const Track &track, if(history_size) { history_size--; if(!history_size) { - track_copy->seek_to(Time(0)); + track_copy->seek_to(0.0f); time_error.set_zero(); result_accumulator.is_recording = true; } diff --git a/Storage/Disk/Track/UnformattedTrack.cpp b/Storage/Disk/Track/UnformattedTrack.cpp index b80f75e7b..b00105d9e 100644 --- a/Storage/Disk/Track/UnformattedTrack.cpp +++ b/Storage/Disk/Track/UnformattedTrack.cpp @@ -17,8 +17,8 @@ Track::Event UnformattedTrack::get_next_event() { return event; } -Storage::Time UnformattedTrack::seek_to(const Time &) { - return Time(0); +float UnformattedTrack::seek_to(float) { + return 0.0f; } Track *UnformattedTrack::clone() const { diff --git a/Storage/Disk/Track/UnformattedTrack.hpp b/Storage/Disk/Track/UnformattedTrack.hpp index 7498fe58a..91d84fa74 100644 --- a/Storage/Disk/Track/UnformattedTrack.hpp +++ b/Storage/Disk/Track/UnformattedTrack.hpp @@ -20,7 +20,7 @@ namespace Disk { class UnformattedTrack: public Track { public: 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; }; From 9d1d162cc85d5828f784ff00987e9f9617c4dc73 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 17 Jul 2020 22:09:21 -0400 Subject: [PATCH 04/11] Add an ability to avoid track flushing when file formats have sub-track precision. --- Storage/Disk/Disk.hpp | 6 ++++++ Storage/Disk/DiskImage/DiskImage.hpp | 7 +++++++ Storage/Disk/DiskImage/DiskImageImplementation.hpp | 4 ++++ Storage/Disk/Track/Track.hpp | 11 +++++++++-- 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/Storage/Disk/Disk.hpp b/Storage/Disk/Disk.hpp index b85db192b..46dac2db8 100644 --- a/Storage/Disk/Disk.hpp +++ b/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; }; } diff --git a/Storage/Disk/DiskImage/DiskImage.hpp b/Storage/Disk/DiskImage/DiskImage.hpp index 32c535761..a1a230f54 100644 --- a/Storage/Disk/DiskImage/DiskImage.hpp +++ b/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 class DiskImageHolder: public DiskImageHolderBase { void set_track_at_position(Track::Address address, const std::shared_ptr &track); void flush_tracks(); bool get_is_read_only(); + bool tracks_differ(Track::Address lhs, Track::Address rhs); private: T disk_image_; diff --git a/Storage/Disk/DiskImage/DiskImageImplementation.hpp b/Storage/Disk/DiskImage/DiskImageImplementation.hpp index 6812cc20d..799687419 100644 --- a/Storage/Disk/DiskImage/DiskImageImplementation.hpp +++ b/Storage/Disk/DiskImage/DiskImageImplementation.hpp @@ -58,3 +58,7 @@ template std::shared_ptr DiskImageHolder::get_track_at_po template DiskImageHolder::~DiskImageHolder() { if(update_queue_) update_queue_->flush(); } + +template bool DiskImageHolder::tracks_differ(Track::Address lhs, Track::Address rhs) { + return disk_image_.tracks_differ(lhs, rhs); +} diff --git a/Storage/Disk/Track/Track.hpp b/Storage/Disk/Track/Track.hpp index 7debed708..bb4ae83e1 100644 --- a/Storage/Disk/Track/Track.hpp +++ b/Storage/Disk/Track/Track.hpp @@ -84,6 +84,13 @@ class Track { int rhs_largest_position = rhs.position.as_largest(); return std::tie(head, largest_position) < std::tie(rhs.head, rhs_largest_position); } + constexpr bool operator == (const Address &rhs) const { + return head == rhs.head && position == rhs.position; + } + constexpr bool operator != (const Address &rhs) const { + return head != rhs.head || position != rhs.position; + } + constexpr Address(int head, HeadPosition position) : head(head), position(position) {} }; @@ -107,11 +114,11 @@ class Track { virtual Event get_next_event() = 0; /*! - Jumps to the event latest offset that is less than or equal to the input time. + Jumps to the start of the fist event that will occur after @c time_since_index_hole. @returns the time jumped to. */ - virtual Time seek_to(const Time &time_since_index_hole) = 0; + virtual float seek_to(float time_since_index_hole) = 0; /*! The virtual copy constructor pattern; returns a copy of the Track. From f6b7467d75a111c0e8804a00ef61e5a08945b1c8 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 17 Jul 2020 22:09:55 -0400 Subject: [PATCH 05/11] Implement custom `tracks_differ`; support WOZ 2 3.5" drive geometry properly. --- Storage/Disk/DiskImage/Formats/WOZ.cpp | 30 +++++++++++++++++++++----- Storage/Disk/DiskImage/Formats/WOZ.hpp | 1 + 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/Storage/Disk/DiskImage/Formats/WOZ.cpp b/Storage/Disk/DiskImage/Formats/WOZ.cpp index bea9fd702..9dd11b047 100644 --- a/Storage/Disk/DiskImage/Formats/WOZ.cpp +++ b/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 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 track_contents; diff --git a/Storage/Disk/DiskImage/Formats/WOZ.hpp b/Storage/Disk/DiskImage/Formats/WOZ.hpp index f131f46c7..cf2d2e5a6 100644 --- a/Storage/Disk/DiskImage/Formats/WOZ.hpp +++ b/Storage/Disk/DiskImage/Formats/WOZ.hpp @@ -31,6 +31,7 @@ class WOZ: public DiskImage { std::shared_ptr get_track_at_position(Track::Address address) final; void set_tracks(const std::map> &tracks) final; bool get_is_read_only() final; + bool tracks_differ(Track::Address, Track::Address) final; private: Storage::FileHolder file_; From 8dcb48254a02f96cb81993ba85c5ba16520ddcbf Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 17 Jul 2020 23:18:08 -0400 Subject: [PATCH 06/11] Simplifies calculations very slightly. --- Storage/Disk/Track/PCMSegment.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Storage/Disk/Track/PCMSegment.cpp b/Storage/Disk/Track/PCMSegment.cpp index 31900965b..5dc4afe6b 100644 --- a/Storage/Disk/Track/PCMSegment.cpp +++ b/Storage/Disk/Track/PCMSegment.cpp @@ -132,11 +132,11 @@ float PCMSegmentEventSource::seek_to(float time_from_start) { // 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 float relative_time = time_from_start + half_bit_length; // the period [0, 0.5) should map to window 0; [0.5, 1.5) should map to window 1; etc. - bit_pointer_ = 1 + size_t(relative_time / bit_length); + 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.get() * float(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 { From a7855e8c98e83d29455f6b1ed5574e4ff86a1878 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 17 Jul 2020 23:18:41 -0400 Subject: [PATCH 07/11] Ensure float literals are floats. --- Storage/TimedEventLoop.cpp | 10 +++++----- Storage/TimedEventLoop.hpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Storage/TimedEventLoop.cpp b/Storage/TimedEventLoop.cpp index 2fa932ff9..b083ef060 100644 --- a/Storage/TimedEventLoop.cpp +++ b/Storage/TimedEventLoop.cpp @@ -69,16 +69,16 @@ void TimedEventLoop::set_next_event_time_interval(Time interval) { void TimedEventLoop::set_next_event_time_interval(float interval) { // Calculate [interval]*[input clock rate] + [subcycles until this event] - float float_interval = interval * float(input_clock_rate_) + subcycles_until_event_; + const float float_interval = interval * float(input_clock_rate_) + subcycles_until_event_; - // So this event will fire in the integral number of cycles from now, putting us at the remainder - // number of subcycles + // This event will fire in the integral number of cycles from now, putting us at the remainder + // number of subcycles. const Cycles::IntType addition = Cycles::IntType(float_interval); cycles_until_event_ += addition; - subcycles_until_event_ = fmodf(float_interval, 1.0); + subcycles_until_event_ = fmodf(float_interval, 1.0f); assert(cycles_until_event_ >= 0); - assert(subcycles_until_event_ >= 0.0); + assert(subcycles_until_event_ >= 0.0f); } Time TimedEventLoop::get_time_into_next_event() { diff --git a/Storage/TimedEventLoop.hpp b/Storage/TimedEventLoop.hpp index 88d80bf77..bc21e61ad 100644 --- a/Storage/TimedEventLoop.hpp +++ b/Storage/TimedEventLoop.hpp @@ -103,7 +103,7 @@ namespace Storage { private: Cycles::IntType input_clock_rate_ = 0; Cycles::IntType cycles_until_event_ = 0; - float subcycles_until_event_ = 0.0; + float subcycles_until_event_ = 0.0f; }; } From d8b699c8695db49c83ac5ab800627e23421c44c8 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Sun, 19 Jul 2020 00:06:27 -0400 Subject: [PATCH 08/11] Corrects index pulse signalling. --- Storage/Disk/Drive.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Storage/Disk/Drive.cpp b/Storage/Disk/Drive.cpp index f74b5a152..32962e3e4 100644 --- a/Storage/Disk/Drive.cpp +++ b/Storage/Disk/Drive.cpp @@ -304,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); @@ -331,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_ && From 47f121ee4c0bba331c0fa9d53f20d362d57dcbf8 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Sun, 19 Jul 2020 00:08:49 -0400 Subject: [PATCH 09/11] Mark WOZs as read-only, with exposition as to why. --- Storage/Disk/DiskImage/Formats/WOZ.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Storage/Disk/DiskImage/Formats/WOZ.cpp b/Storage/Disk/DiskImage/Formats/WOZ.cpp index 9dd11b047..c11292078 100644 --- a/Storage/Disk/DiskImage/Formats/WOZ.cpp +++ b/Storage/Disk/DiskImage/Formats/WOZ.cpp @@ -214,5 +214,12 @@ void WOZ::set_tracks(const std::map> &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. } From 84dd194afdd7a1bcbc9087105f0c0e1889c8fe91 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Mon, 20 Jul 2020 19:48:20 -0400 Subject: [PATCH 10/11] Corrects test for ::tracks_differ. --- Storage/Disk/DiskImage/DiskImage.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Storage/Disk/DiskImage/DiskImage.hpp b/Storage/Disk/DiskImage/DiskImage.hpp index a1a230f54..ed051756f 100644 --- a/Storage/Disk/DiskImage/DiskImage.hpp +++ b/Storage/Disk/DiskImage/DiskImage.hpp @@ -72,7 +72,7 @@ class DiskImage { @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; } + virtual bool tracks_differ(Track::Address lhs, Track::Address rhs) { return lhs != rhs; } }; class DiskImageHolderBase: public Disk { From 5ebbab6f3507108f6ef74e99e3e56a41bcd6dbbe Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Mon, 20 Jul 2020 19:50:33 -0400 Subject: [PATCH 11/11] Further relax Apple GCR static analysis requirements. --- Storage/Disk/Encodings/AppleGCR/Sector.hpp | 4 +- .../Disk/Encodings/AppleGCR/SegmentParser.cpp | 49 ++++++++++--------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/Storage/Disk/Encodings/AppleGCR/Sector.hpp b/Storage/Disk/Encodings/AppleGCR/Sector.hpp index 3515a4474..315ee7459 100644 --- a/Storage/Disk/Encodings/AppleGCR/Sector.hpp +++ b/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; }; diff --git a/Storage/Disk/Encodings/AppleGCR/SegmentParser.cpp b/Storage/Disk/Encodings/AppleGCR/SegmentParser.cpp index 9531229d0..d68be8d7e 100644 --- a/Storage/Disk/Encodings/AppleGCR/SegmentParser.cpp +++ b/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 decode_macintosh_sector(const std::array &header, const std::unique_ptr &original) { - // There must be at least 704 bytes to decode from. - if(original->data.size() < 704) return nullptr; +std::unique_ptr decode_macintosh_sector(const std::array *header, const std::unique_ptr &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 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 decode_macintosh_sector(const std::array decode_appleii_sector(const std::array &header, const std::unique_ptr &original, bool is_five_and_three) { +std::unique_ptr decode_appleii_sector(const std::array *header, const std::unique_ptr &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->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]; + // 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; - // 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; + // 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 decode_appleii_sector(const std::array } 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 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 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 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); }