Promote DigitalPhaseLockedLoop to a template, simplify to O(1) add_pulse.

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

@@ -70,7 +70,9 @@
       buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      enableAddressSanitizer = "YES"
       enableASanStackUseAfterReturn = "YES"
+      enableUBSanitizer = "YES"
       disableMainThreadChecker = "YES"
       launchStyle = "0"
       useCustomWorkingDirectory = "NO"

OSBindings/Mac/Clock SignalTests/Bridges/DigitalPhaseLockedLoopBridge.h

@@ -10,7 +10,7 @@
 
 @interface DigitalPhaseLockedLoopBridge : NSObject
 
-- (instancetype)initWithClocksPerBit:(NSUInteger)clocksPerBit historyLength:(NSUInteger)historyLength;
+- (instancetype)initWithClocksPerBit:(NSUInteger)clocksPerBit;
 
 - (void)runForCycles:(NSUInteger)cycles;
 - (void)addPulse;

OSBindings/Mac/Clock SignalTests/Bridges/DigitalPhaseLockedLoopBridge.mm

@@ -15,7 +15,7 @@
 - (void)pushBit:(int)value;
 @end
 
-class DigitalPhaseLockedLoopDelegate: public Storage::DigitalPhaseLockedLoop::Delegate {
+class DigitalPhaseLockedLoopDelegate {
 	public:
 		__weak DigitalPhaseLockedLoopBridge *bridge;
 
@@ -25,14 +25,14 @@ class DigitalPhaseLockedLoopDelegate: public Storage::DigitalPhaseLockedLoop::De
 };
 
 @implementation DigitalPhaseLockedLoopBridge {
-	std::unique_ptr<Storage::DigitalPhaseLockedLoop> _digitalPhaseLockedLoop;
+	std::unique_ptr<Storage::DigitalPhaseLockedLoop<DigitalPhaseLockedLoopDelegate>> _digitalPhaseLockedLoop;
 	DigitalPhaseLockedLoopDelegate _delegate;
 }
 
-- (instancetype)initWithClocksPerBit:(NSUInteger)clocksPerBit historyLength:(NSUInteger)historyLength {
+- (instancetype)initWithClocksPerBit:(NSUInteger)clocksPerBit {
 	self = [super init];
 	if(self) {
-		_digitalPhaseLockedLoop = std::make_unique<Storage::DigitalPhaseLockedLoop>((unsigned int)clocksPerBit, (unsigned int)historyLength);
+		_digitalPhaseLockedLoop = std::make_unique<Storage::DigitalPhaseLockedLoop<DigitalPhaseLockedLoopDelegate>>((unsigned int)clocksPerBit);
 		_delegate.bridge = self;
 		_digitalPhaseLockedLoop->set_delegate(&_delegate);
 	}

OSBindings/Mac/Clock SignalTests/DPLLTests.swift

@@ -26,22 +26,22 @@ class DPLLTests: XCTestCase {
 	}
 
 	func testPerfectInput() {
-		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100, historyLength: 3)
+		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100)
 		testRegularNibblesOnPLL(pll!, bitLength: 100)
 	}
 
 	func testFastButRegular() {
-		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100, historyLength: 3)
+		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100)
 		testRegularNibblesOnPLL(pll!, bitLength: 90)
 	}
 
 	func testSlowButRegular() {
-		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100, historyLength: 3)
+		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100)
 		testRegularNibblesOnPLL(pll!, bitLength: 110)
 	}
 
 	func testTwentyPercentSinePattern() {
-		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100, historyLength: 3)
+		let pll = DigitalPhaseLockedLoopBridge(clocksPerBit: 100)
 		var angle = 0.0
 
 		// clock in two 1s, a 0, and a 1, 200 times over

Storage/Disk/Controller/DiskController.cpp

@@ -15,10 +15,10 @@ using namespace Storage::Disk;
 Controller::Controller(Cycles clock_rate) :
 		clock_rate_multiplier_(128000000 / clock_rate.as_integral()),
 		clock_rate_(clock_rate.as_integral() * clock_rate_multiplier_),
+		pll_(100),
 		empty_drive_(new Drive(int(clock_rate.as_integral()), 1, 1)) {
-	// seed this class with a PLL, any PLL, so that it's safe to assume non-nullptr later
-	Time one(1);
-	set_expected_bit_length(one);
+	pll_.set_delegate(this);
+	set_expected_bit_length(Time(1));
 	set_drive(empty_drive_);
 }
 
@@ -42,13 +42,13 @@ Drive &Controller::get_drive() {
 
 void Controller::process_event(const Drive::Event &event) {
 	switch(event.type) {
-		case Track::Event::FluxTransition:	pll_->add_pulse();		break;
+		case Track::Event::FluxTransition:	pll_.add_pulse();		break;
 		case Track::Event::IndexHole:		process_index_hole();	break;
 	}
 }
 
 void Controller::advance(const Cycles cycles) {
-	if(is_reading_) pll_->run_for(Cycles(cycles.as_integral() * clock_rate_multiplier_));
+	if(is_reading_) pll_.run_for(Cycles(cycles.as_integral() * clock_rate_multiplier_));
 }
 
 void Controller::process_write_completed() {
@@ -66,9 +66,8 @@ void Controller::set_expected_bit_length(Time bit_length) {
 
 	// this conversion doesn't need to be exact because there's a lot of variation to be taken
 	// account of in rotation speed, air turbulence, etc, so a direct conversion will do
-	int clocks_per_bit = cycles_per_bit.get<int>();
-	pll_ = std::make_unique<DigitalPhaseLockedLoop>(clocks_per_bit, 3);
-	pll_->set_delegate(this);
+	const int clocks_per_bit = cycles_per_bit.get<int>();
+	pll_.set_clocks_per_bit(clocks_per_bit);
 }
 
 void Controller::digital_phase_locked_loop_output_bit(int value) {

Storage/Disk/Controller/DiskController.hpp

@@ -29,7 +29,6 @@ namespace Disk {
 	TODO: communication of head size and permissible stepping extents, appropriate simulation of gain.
 */
 class Controller:
-	public DigitalPhaseLockedLoop::Delegate,
 	public Drive::EventDelegate,
 	public ClockingHint::Source,
 	public ClockingHint::Observer {
@@ -111,7 +110,9 @@ class Controller:
 
 		bool is_reading_ = true;
 
-		std::shared_ptr<DigitalPhaseLockedLoop> pll_;
+		DigitalPhaseLockedLoop<Controller> pll_;
+		friend DigitalPhaseLockedLoop<Controller>;
+
 		std::shared_ptr<Drive> drive_;
 
 		std::shared_ptr<Drive> empty_drive_;
@@ -124,7 +125,7 @@ class Controller:
 		void advance(const Cycles cycles) override ;
 
 		// to satisfy DigitalPhaseLockedLoop::Delegate
-		void digital_phase_locked_loop_output_bit(int value) override;
+		void digital_phase_locked_loop_output_bit(int value);
 };
 
 }

Storage/Disk/DPLL/DigitalPhaseLockedLoop.cpp

@@ -12,58 +12,3 @@
 
 using namespace Storage;
 
-DigitalPhaseLockedLoop::DigitalPhaseLockedLoop(int clocks_per_bit, std::size_t length_of_history) :
-		offset_history_(length_of_history, 0),
-		window_length_(clocks_per_bit),
-		clocks_per_bit_(clocks_per_bit) {}
-
-void DigitalPhaseLockedLoop::run_for(const Cycles cycles) {
-	offset_ += cycles.as_integral();
-	phase_ += cycles.as_integral();
-	if(phase_ >= window_length_) {
-		auto windows_crossed = phase_ / window_length_;
-
-		// check whether this triggers any 0s, if anybody cares
-		if(delegate_) {
-			if(window_was_filled_) --windows_crossed;
-			for(int c = 0; c < windows_crossed; c++)
-				delegate_->digital_phase_locked_loop_output_bit(0);
-		}
-
-		window_was_filled_ = false;
-		phase_ %= window_length_;
-	}
-}
-
-void DigitalPhaseLockedLoop::add_pulse() {
-	if(!window_was_filled_) {
-		if(delegate_) delegate_->digital_phase_locked_loop_output_bit(1);
-		window_was_filled_ = true;
-		post_phase_offset(phase_, offset_);
-		offset_ = 0;
-	}
-}
-
-void DigitalPhaseLockedLoop::post_phase_offset(Cycles::IntType new_phase, Cycles::IntType new_offset) {
-	offset_history_[offset_history_pointer_] = new_offset;
-	offset_history_pointer_ = (offset_history_pointer_ + 1) % offset_history_.size();
-
-	// use an unweighted average of the stored offsets to compute current window size,
-	// bucketing them by rounding to the nearest multiple of the base clocks per bit
-	Cycles::IntType total_spacing = 0;
-	Cycles::IntType total_divisor = 0;
-	for(auto offset : offset_history_) {
-		auto multiple = (offset + (clocks_per_bit_ >> 1)) / clocks_per_bit_;
-		if(!multiple) continue;
-		total_divisor += multiple;
-		total_spacing += offset;
-	}
-	if(total_divisor) {
-		window_length_ = total_spacing / total_divisor;
-	}
-
-	auto error = new_phase - (window_length_ >> 1);
-
-	// use a simple spring mechanism as a lowpass filter for phase
-	phase_ -= (error + 1) >> 1;
-}

Storage/Disk/DPLL/DigitalPhaseLockedLoop.hpp

@@ -9,6 +9,8 @@
 #ifndef DigitalPhaseLockedLoop_hpp
 #define DigitalPhaseLockedLoop_hpp
 
+#include <array>
+#include <cassert>
 #include <memory>
 #include <vector>
 
@@ -16,54 +18,125 @@
 
 namespace Storage {
 
-class DigitalPhaseLockedLoop {
+/*!
+	Template parameters:
+
+	@c length_of_history The number of historic pulses to consider in locking to phase.
+*/
+template <typename BitHandler, size_t length_of_history = 10> class DigitalPhaseLockedLoop {
 	public:
 		/*!
 			Instantiates a @c DigitalPhaseLockedLoop.
 
 			@param clocks_per_bit The expected number of cycles between each bit of input.
-			@param length_of_history The number of historic pulses to consider in locking to phase.
 		*/
-		DigitalPhaseLockedLoop(int clocks_per_bit, std::size_t length_of_history);
+		DigitalPhaseLockedLoop(int clocks_per_bit) :
+			window_length_(clocks_per_bit), clocks_per_bit_(clocks_per_bit) {}
+
+		/*!
+			Changes the expected window length.
+		*/
+		void set_clocks_per_bit(int clocks_per_bit) {
+			window_length_ = clocks_per_bit_ = clocks_per_bit;
+		}
 
 		/*!
 			Runs the loop, impliedly posting no pulses during that period.
 
 			@c number_of_cycles The time to run the loop for.
 		*/
-		void run_for(const Cycles cycles);
+		void run_for(const Cycles cycles) {
+			offset_ += cycles.as_integral();
+			phase_ += cycles.as_integral();
+			if(phase_ >= window_length_) {
+				auto windows_crossed = phase_ / window_length_;
+
+				// check whether this triggers any 0s, if anybody cares
+				if(delegate_) {
+					if(window_was_filled_) --windows_crossed;
+					for(int c = 0; c < windows_crossed; c++)
+						delegate_->digital_phase_locked_loop_output_bit(0);
+				}
+
+				window_was_filled_ = false;
+				phase_ %= window_length_;
+			}
+		}
 
 		/*!
 			Announces a pulse at the current time.
 		*/
-		void add_pulse();
+		void add_pulse() {
+			if(!window_was_filled_) {
+				if(delegate_) delegate_->digital_phase_locked_loop_output_bit(1);
+				window_was_filled_ = true;
+				post_phase_offset(phase_, offset_);
+				offset_ = 0;
+			}
+		}
 
 		/*!
 			A receiver for PCM output data; called upon every recognised bit.
 		*/
-		class Delegate {
-			public:
-				virtual void digital_phase_locked_loop_output_bit(int value) = 0;
-		};
-		void set_delegate(Delegate *delegate) {
+		void set_delegate(BitHandler *delegate) {
 			delegate_ = delegate;
 		}
 
 	private:
-		Delegate *delegate_ = nullptr;
+		BitHandler *delegate_ = nullptr;
 
-		void post_phase_offset(Cycles::IntType phase, Cycles::IntType offset);
+		void post_phase_offset(Cycles::IntType new_phase, Cycles::IntType new_offset) {
+			// Erase the effect of whatever is currently in this slot.
+			total_divisor_ -= offset_history_[offset_history_pointer_].divisor;
+			total_spacing_ -= offset_history_[offset_history_pointer_].spacing;
 
-		std::vector<Cycles::IntType> offset_history_;
+			// Fill in the new fields.
+			const auto multiple = (new_offset + (clocks_per_bit_ >> 1)) / clocks_per_bit_;
+			offset_history_[offset_history_pointer_].divisor = multiple;
+			offset_history_[offset_history_pointer_].spacing = new_offset;
+
+			// Add in the new values;
+			total_divisor_ += offset_history_[offset_history_pointer_].divisor;
+			total_spacing_ += offset_history_[offset_history_pointer_].spacing;
+
+			// Advance the write slot.
+			offset_history_pointer_ = (offset_history_pointer_ + 1) % offset_history_.size();
+
+			// In net: use an unweighted average of the stored offsets to compute current window size,
+			// bucketing them by rounding to the nearest multiple of the base clocks per bit
+			window_length_ = total_spacing_ / total_divisor_;
+#ifndef NDEBUG
+			bool are_all_filled = true;
+			for(auto offset: offset_history_) {
+				if(offset.spacing == 1) {
+					are_all_filled = false;
+					break;
+				}
+			}
+			assert(!are_all_filled || (window_length_ >= ((clocks_per_bit_ * 9) / 10) && window_length_ <= ((clocks_per_bit_ * 11) / 10)));
+#endif
+
+			// Also apply a difference to phase, use a simple spring mechanism as a lowpass filter.
+			const auto error = new_phase - (window_length_ >> 1);
+			phase_ -= (error + 1) >> 1;
+		}
+
+		struct LoggedOffset {
+			Cycles::IntType divisor = 1, spacing = 1;
+		};
+		std::array<LoggedOffset, length_of_history> offset_history_;
 		std::size_t offset_history_pointer_ = 0;
-		Cycles::IntType offset_ = 0;
+
+		Cycles::IntType total_spacing_ = length_of_history;
+		Cycles::IntType total_divisor_ = length_of_history;
 
 		Cycles::IntType phase_ = 0;
 		Cycles::IntType window_length_ = 0;
+
+		Cycles::IntType offset_ = 0;
 		bool window_was_filled_ = false;
 
 		int clocks_per_bit_ = 0;
-		int tolerance_ = 0;
 };
 
 }

Storage/Disk/Track/TrackSerialiser.cpp

@@ -14,18 +14,18 @@
 // just return a copy of that segment.
 Storage::Disk::PCMSegment Storage::Disk::track_serialisation(const Track &track, Time length_of_a_bit) {
 	unsigned int history_size = 16;
-	DigitalPhaseLockedLoop pll(100, history_size);
 	std::unique_ptr<Track> track_copy(track.clone());
 
 	// ResultAccumulator exists to append whatever comes out of the PLL to
 	// its PCMSegment.
-	struct ResultAccumulator: public DigitalPhaseLockedLoop::Delegate {
+	struct ResultAccumulator {
 		PCMSegment result;
 		void digital_phase_locked_loop_output_bit(int value) {
 			result.data.push_back(!!value);
 		}
 	} result_accumulator;
 	result_accumulator.result.length_of_a_bit = length_of_a_bit;
+	DigitalPhaseLockedLoop<ResultAccumulator> pll(100);
 
 	// Obtain a length multiplier which is 100 times the reciprocal
 	// of the expected bit length. So a perfect bit length from

Storage/Tape/Parsers/Acorn.cpp

@@ -66,7 +66,7 @@ void Parser::process_pulse(const Storage::Tape::Tape::Pulse &pulse) {
 
 
 Shifter::Shifter() :
-	pll_(PLLClockRate / 4800, 15),
+	pll_(PLLClockRate / 4800),
 	was_high_(false),
 	input_pattern_(0),
 	input_bit_counter_(0),

Storage/Tape/Parsers/Acorn.hpp

@@ -17,7 +17,7 @@ namespace Storage {
 namespace Tape {
 namespace Acorn {
 
-class Shifter: public Storage::DigitalPhaseLockedLoop::Delegate {
+class Shifter {
 	public:
 		Shifter();
 
@@ -34,7 +34,7 @@ class Shifter: public Storage::DigitalPhaseLockedLoop::Delegate {
 		void digital_phase_locked_loop_output_bit(int value);
 
 	private:
-		Storage::DigitalPhaseLockedLoop pll_;
+		Storage::DigitalPhaseLockedLoop<Shifter, 15> pll_;
 		bool was_high_;
 
 		unsigned int input_pattern_;