From 214b6a254ad722d7d63cd4a3dcc5035935aa5d02 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Sun, 29 Dec 2019 17:37:09 -0500 Subject: [PATCH] Adds a delay on visibility of the hsync signal, and a test on address reload. --- Machines/Atari/ST/Video.cpp | 55 +++++---- Machines/Atari/ST/Video.hpp | 6 +- .../Clock SignalTests/AtariSTVideoTests.mm | 114 +++++++++++++----- 3 files changed, 122 insertions(+), 53 deletions(-) diff --git a/Machines/Atari/ST/Video.cpp b/Machines/Atari/ST/Video.cpp index 1285a2ee4..162b1226e 100644 --- a/Machines/Atari/ST/Video.cpp +++ b/Machines/Atari/ST/Video.cpp @@ -95,13 +95,17 @@ struct Checker { #endif const int de_delay_period = CYCLE(28); // Number of half cycles after DE that observed DE changes. -const int vsync_x_position = CYCLE(54); // Horizontal cycle on which vertical sync changes happen. -const int address_reload_x_position = CYCLE(54); // Horizontal cycle on which the base address is reloaded. +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. const int hsync_end = CYCLE(8); // Cycles before end of line when hsync ends. +const int hsync_delay_period = hsync_end; // Signal hsync at the end of the line. // "VSYNC starts 104 cycles after the start of the previous line's HSYNC, so that's 4 cycles before DE would be activated. "; -// hsync is at -50, so that's +54, or thereabouts. +// that's an inconsistent statement since it would imply VSYNC at +54, which is 2 cycles before DE in 60Hz mode and 6 before +// in 50Hz mode. I've gone with 56, to be four cycles ahead of DE in 50Hz mode. +// +// TODO: some sort of latency on vsync and hsync visibility, I would imagine? } @@ -178,6 +182,7 @@ void Video::advance(HalfCycles duration) { // Determine current output mode and number of cycles to output for. const int run_length = std::min(integer_duration, next_event - x_); const bool display_enable = vertical_.enable && horizontal_.enable; + const bool hsync = horizontal_.sync; // Ensure proper fetching irrespective of the output. if(load_) { @@ -243,23 +248,6 @@ void Video::advance(HalfCycles duration) { } } - // Check for address reload; this timing is _highly_ speculative on my part. I originally had it at frame end, - // then Enchanted Woods seemed to be doing its things three lines too late, so I moved it forward a little. - // Which didn't solve the problem, so I guess that title's logic isn't triggered on address reload, but it makes - // sense to keep it out separate and I've no better intel about its actual positioning. - if(y_ == vertical_timings.height - 3 && x_ <= address_reload_x_position && (x_ + run_length) > address_reload_x_position) { - current_address_ = base_address_ >> 1; - reset_fifo(); // TODO: remove this, probably, once otherwise stable? - - // Consider a shout out to the range observer. - if(previous_base_address_ != base_address_) { - previous_base_address_ = base_address_; - if(range_observer_) { - range_observer_->video_did_change_access_range(this); - } - } - } - // Check for whether line length should have been latched during this run. if(x_ <= CYCLE(54) && (x_ + run_length) > CYCLE(54)) line_length_ = horizontal_timings.length; @@ -278,11 +266,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 - 1) { + } 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; } } @@ -311,6 +299,8 @@ void Video::advance(HalfCycles duration) { if(vertical_.sync_schedule != VerticalState::SyncSchedule::None && x_ == vsync_x_position) { vertical_.sync = vertical_.sync_schedule == VerticalState::SyncSchedule::Begin; vertical_.enable &= !vertical_.sync; + + reset_fifo(); // TODO: remove this, probably, once otherwise stable? } // Check whether the terminating event was end-of-line; if so then advance @@ -321,6 +311,20 @@ void Video::advance(HalfCycles duration) { y_ = next_y_; } + // The address is reloaded during the entire period of vertical sync. + // Cf. http://www.atari-forum.com/viewtopic.php?t=31954&start=50#p324730 + if(vertical_.sync) { + current_address_ = base_address_ >> 1; + + // Consider a shout out to the range observer. + if(previous_base_address_ != base_address_) { + previous_base_address_ = base_address_; + if(range_observer_) { + range_observer_->video_did_change_access_range(this); + } + } + } + // Chuck any deferred output changes into the queue. const bool next_display_enable = vertical_.enable && horizontal_.enable; if(display_enable != next_display_enable) { @@ -330,6 +334,11 @@ void Video::advance(HalfCycles duration) { // Schedule change in inwardly-visible effect. next_load_toggle_ = x_ + 8; // 4 cycles = 8 half-cycles } + + if(horizontal_.sync != hsync) { + // Schedule change in outwardly-visible hsync line. + add_event(hsync_delay_period - integer_duration, horizontal_.sync ? Event::Type::SetHsync : Event::Type::ResetHsync); + } } } @@ -351,7 +360,7 @@ void Video::reset_fifo() { } bool Video::hsync() { - return horizontal_.sync; + return public_state_.hsync; } bool Video::vsync() { diff --git a/Machines/Atari/ST/Video.hpp b/Machines/Atari/ST/Video.hpp index ef4c3c19d..b605d3c8c 100644 --- a/Machines/Atari/ST/Video.hpp +++ b/Machines/Atari/ST/Video.hpp @@ -227,12 +227,14 @@ class Video { /// Contains copies of the various observeable fields, after the relevant propagation delay. struct PublicState { bool display_enable = false; + bool hsync = false; } public_state_; struct Event { int delay; enum class Type { - SetDisplayEnable, ResetDisplayEnable + SetDisplayEnable, ResetDisplayEnable, + SetHsync, ResetHsync, } type; Event(Type type, int delay) : delay(delay), type(type) {} @@ -246,6 +248,8 @@ class Video { default: case Type::SetDisplayEnable: state.display_enable = true; break; case Type::ResetDisplayEnable: state.display_enable = false; break; + case Type::SetHsync: state.hsync = true; break; + case Type::ResetHsync: state.hsync = false; break; } } }; diff --git a/OSBindings/Mac/Clock SignalTests/AtariSTVideoTests.mm b/OSBindings/Mac/Clock SignalTests/AtariSTVideoTests.mm index 305b732ab..70a4a6b45 100644 --- a/OSBindings/Mac/Clock SignalTests/AtariSTVideoTests.mm +++ b/OSBindings/Mac/Clock SignalTests/AtariSTVideoTests.mm @@ -20,6 +20,8 @@ uint16_t _ram[256*1024]; } +// MARK: - Setup and tear down. + - (void)setUp { [super setUp]; @@ -35,35 +37,7 @@ _video = nullptr; } -/// Tests that no events occur outside of the sequence points the video predicts. -- (void)testSequencePoints { - // Set 4bpp, 50Hz. - _video->write(0x05, 0x0200); - _video->write(0x30, 0x0000); - - // Run for [more than] a whole frame making sure that no observeable outputs - // change at any time other than a sequence point. - HalfCycles next_event; - bool display_enable = false; - bool vsync = false; - bool hsync = false; - for(size_t c = 0; c < 10 * 1000 * 1000; ++c) { - const bool is_transition_point = next_event == HalfCycles(0); - - if(is_transition_point) { - display_enable = _video->display_enabled(); - vsync = _video->vsync(); - hsync = _video->hsync(); - next_event = _video->get_next_sequence_point(); - } else { - NSAssert(display_enable == _video->display_enabled(), @"Unannounced change in display enabled at cycle %zu [%d before next sequence point]", c, next_event.as()); - NSAssert(vsync == _video->vsync(), @"Unannounced change in vsync at cycle %zu [%d before next sequence point]", c, next_event.as()); - NSAssert(hsync == _video->hsync(), @"Unannounced change in hsync at cycle %zu [%d before next sequence point]", c, next_event.as()); - } - _video->run_for(HalfCycles(2)); - next_event -= HalfCycles(2); - } -} +// MARK: - Helpers - (void)runVideoForCycles:(int)cycles { while(cycles--) { @@ -108,6 +82,45 @@ ((_video->read(0x02) & 0xff) << 16); } +- (void)setVideoBaseAddress:(uint32_t)baseAddress { + _video->write(0x00, baseAddress >> 16); + _video->write(0x01, baseAddress >> 8); +} + +// MARK: - Sequence Point Prediction Tests + +/// Tests that no events occur outside of the sequence points the video predicts. +- (void)testSequencePoints { + // Set 4bpp, 50Hz. + _video->write(0x05, 0x0200); + _video->write(0x30, 0x0000); + + // Run for [more than] a whole frame making sure that no observeable outputs + // change at any time other than a sequence point. + HalfCycles next_event; + bool display_enable = false; + bool vsync = false; + bool hsync = false; + for(size_t c = 0; c < 10 * 1000 * 1000; ++c) { + const bool is_transition_point = next_event == HalfCycles(0); + + if(is_transition_point) { + display_enable = _video->display_enabled(); + vsync = _video->vsync(); + hsync = _video->hsync(); + next_event = _video->get_next_sequence_point(); + } else { + NSAssert(display_enable == _video->display_enabled(), @"Unannounced change in display enabled at cycle %zu [%d before next sequence point]", c, next_event.as()); + NSAssert(vsync == _video->vsync(), @"Unannounced change in vsync at cycle %zu [%d before next sequence point]", c, next_event.as()); + NSAssert(hsync == _video->hsync(), @"Unannounced change in hsync at cycle %zu [%d before next sequence point]", c, next_event.as()); + } + _video->run_for(HalfCycles(2)); + next_event -= HalfCycles(2); + } +} + +// MARK: - Sync Line Length Tests + struct RunLength { int frequency; int length; @@ -260,4 +273,47 @@ struct RunLength { [self testSequence:test targetLength:230]; } +// MARK: - Address Reload Timing tests + +/// Tests that the current video address is reloaded constantly throughout hsync +- (void)testHsyncReload { + // Set an initial video address of 0. + [self setVideoBaseAddress:0]; + + // Find next area of non-vsync. + while(_video->vsync()) { + _video->run_for(Cycles(1)); + } + + // Set a different base video address. + [self setVideoBaseAddress:0x800000]; + + // Find next area of vsync, checking that the address isn't + // reloaded before then. + while(!_video->vsync()) { + XCTAssertNotEqual([self currentVideoAddress], 0x800000); + _video->run_for(Cycles(1)); + } + + // Vsync has now started, test that video address has been set. + XCTAssertEqual([self currentVideoAddress], 0x800000); + + // Run a few cycles, set a different video base address, + // confirm that has been set. + [self runVideoForCycles:200]; + XCTAssertEqual([self currentVideoAddress], 0x800000); + [self setVideoBaseAddress:0xc00000]; + [self runVideoForCycles:1]; + XCTAssertEqual([self currentVideoAddress], 0xc00000); + + // Find end of vertical sync, set a different base address, + // check that it doesn't become current. + while(_video->vsync()) { + _video->run_for(Cycles(1)); + } + [self setVideoBaseAddress:0]; + [self runVideoForCycles:1]; + XCTAssertNotEqual([self currentVideoAddress], 0); +} + @end