Merge pull request #712 from TomHarte/MercsTweaks

Corrects vsync placement and BPP-change pipeline flushing.

Machines/Atari/ST/AtariST.cpp

@@ -16,6 +16,7 @@
 #include "../../../Activity/Source.hpp"
 
 //#define LOG_TRACE
+//bool should_log = false;
 #include "../../../Processors/68000/68000.hpp"
 
 #include "../../../Components/AY38910/AY38910.hpp"

Machines/Atari/ST/Video.cpp

@@ -94,7 +94,7 @@ struct Checker {
 } checker;
 #endif
 
-const int de_delay_period = CYCLE(28);		// Number of half cycles after DE that observed DE changes.
+const int de_delay_period = CYCLE(28);		// Amount of time after DE that observed DE changes. NB: HACK HERE. This currently incorporates the MFP recognition delay. MUST FIX.
 const int vsync_x_position = CYCLE(56);		// Horizontal cycle on which vertical sync changes happen.
 
 const int hsync_start = CYCLE(48);			// Cycles before end of line when hsync starts.
@@ -116,7 +116,7 @@ Video::Video() :
 
 	// Show a total of 260 lines; a little short for PAL but a compromise between that and the ST's
 	// usual output height of 200 lines.
-	crt_.set_visible_area(crt_.get_rect_for_area(33, 260, 216, 850, 4.0f / 3.0f));
+	crt_.set_visible_area(crt_.get_rect_for_area(33, 260, 220, 850, 4.0f / 3.0f));
 }
 
 void Video::set_ram(uint16_t *ram, size_t size) {
@@ -267,10 +267,11 @@ void Video::advance(HalfCycles duration) {
 				next_vertical_.enable = true;
 			} else if(y_ == vertical_timings.reset_enable) {
 				next_vertical_.enable = false;
-			} else if(next_y_ == vertical_timings.height) {
+			} else if(next_y_ == vertical_timings.height - 2) {
 				next_vertical_.sync_schedule = VerticalState::SyncSchedule::Begin;
+			} else if(next_y_ == vertical_timings.height) {
 				next_y_ = 0;
-			} else if(next_y_ == 2) {
+			} else if(y_ == 0) {
 				next_vertical_.sync_schedule = VerticalState::SyncSchedule::End;
 			}
 		}
@@ -730,10 +731,13 @@ void Video::VideoStream::output_pixels(int duration) {
 }
 
 void Video::VideoStream::flush_pixels() {
-	switch(bpp_) {
-		case OutputBpp::One:	crt_.output_data(pixel_pointer_ >> 1, size_t(pixel_pointer_)); break;
-		default: 				crt_.output_data(pixel_pointer_); break;
-		case OutputBpp::Four:	crt_.output_data(pixel_pointer_ << 1, size_t(pixel_pointer_)); break;
+	// Flush only if there's something to flush.
+	if(pixel_pointer_) {
+		switch(bpp_) {
+			case OutputBpp::One:	crt_.output_data(pixel_pointer_ >> 1, size_t(pixel_pointer_)); break;
+			default: 				crt_.output_data(pixel_pointer_); break;
+			case OutputBpp::Four:	crt_.output_data(pixel_pointer_ << 1, size_t(pixel_pointer_)); break;
+		}
 	}
 
 	pixel_pointer_ = 0;
@@ -741,8 +745,14 @@ void Video::VideoStream::flush_pixels() {
 }
 
 void Video::VideoStream::set_bpp(OutputBpp bpp) {
+	// Terminate the allocated block of memory (if any).
+	flush_pixels();
+
+	// Reset the shifter.
 	// TODO: is flushing like this correct?
 	output_shifter_ = 0;
+
+	// Store the new BPP.
 	bpp_ = bpp;
 }
 

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

@@ -68,8 +68,8 @@
    </TestAction>
    <LaunchAction
       buildConfiguration = "Release"
-      selectedDebuggerIdentifier = ""
-      selectedLauncherIdentifier = "Xcode.IDEFoundation.Launcher.PosixSpawn"
+      selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
+      selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       enableASanStackUseAfterReturn = "YES"
       disableMainThreadChecker = "YES"
       launchStyle = "0"

OSBindings/Mac/Clock SignalTests/68000Tests.mm

@@ -10,7 +10,6 @@
 
 #include <cassert>
 
-#define LOG_TRACE
 #include "TestRunner68000.hpp"
 
 class CPU::MC68000::ProcessorStorageTests {

OSBindings/Mac/Clock SignalTests/AtariSTVideoTests.mm

@@ -337,4 +337,29 @@ struct RunLength {
 	[self testSequence:test targetLength:230];
 }
 
+- (void)testPP88 {
+	// Test a full line.
+	{
+		const RunLength test[] = {
+			{72, 8},
+			{50, 364},
+			{60, 16},
+			{50, 116},
+			{72, 8},
+			{-1}
+		};
+		[self testSequence:test targetLength:230];
+	}
+
+	{
+		const RunLength test[] = {
+			{72, 8},
+			{496, 50},
+			{72, 8},
+			{-1}
+		};
+		[self testSequence:test targetLength:186];
+	}
+}
+
 @end

OSBindings/Mac/Clock SignalTests/TestRunner68000.hpp

@@ -11,7 +11,6 @@
 
 #include <array>
 
-#define LOG_TRACE
 #include "../../../Processors/68000/68000.hpp"
 
 using Flag = CPU::MC68000::Flag;

Outputs/CRT/Internals/Flywheel.hpp

@@ -61,7 +61,7 @@ struct Flywheel {
 		@returns The next synchronisation event.
 	*/
 	inline SyncEvent get_next_event_in_period(bool sync_is_requested, int cycles_to_run_for, int *cycles_advanced) {
-		// do we recognise this hsync, thereby adjusting future time expectations?
+		// If sync is signalled _now_, consider adjusting expected_next_sync_.
 		if(sync_is_requested) {
 			if(counter_ < sync_error_window_ || counter_ > expected_next_sync_ - sync_error_window_) {
 				const int time_now = (counter_ < sync_error_window_) ? expected_next_sync_ + counter_ : counter_;

Processors/68000/Implementation/68000Implementation.hpp

@@ -69,10 +69,6 @@
 template <class T, bool dtack_is_implicit, bool signal_will_perform> void Processor<T, dtack_is_implicit, signal_will_perform>::run_for(HalfCycles duration) {
 	const HalfCycles remaining_duration = duration + half_cycles_left_to_run_;
 
-#ifdef LOG_TRACE
-						static bool should_log = false;
-#endif
-
 	// This loop counts upwards rather than downwards because it simplifies calculation of
 	// E as and when required.
 	HalfCycles cycles_run_for;
@@ -243,7 +239,7 @@ template <class T, bool dtack_is_implicit, bool signal_will_perform> void Proces
 
 				case ExecutionState::BeginInterrupt:
 #ifdef LOG_TRACE
-					should_log = true;
+//					should_log = true;
 #endif
 					active_program_ = nullptr;
 					active_micro_op_ = interrupt_micro_ops_;

Storage/Disk/Encodings/MFM/Encoder.cpp

@@ -71,6 +71,7 @@ class MFMEncoder: public Encoder {
 				case SurfaceItem::Data: return 2;	// Just a single encoded byte.
 				default: assert(false);
 			}
+			return 0;	// Should be impossible to reach in debug builds.
 		}
 
 	private: