From 2638a901d9fcad2f5cde851bc904974ec95be29b Mon Sep 17 00:00:00 2001
From: Thomas Harte <thomas.harte@gmail.com>
Date: Sun, 29 Sep 2019 22:08:16 -0400
Subject: [PATCH] Improves documentation of existing degree of implementation.

---
 Components/8530/z8530.cpp | 47 ++++++++++++++++++++++++++++++---------
 Components/8530/z8530.hpp | 19 +++++++++++++++-
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/Components/8530/z8530.cpp b/Components/8530/z8530.cpp
index a4cc451cb..8e1cc9a4b 100644
--- a/Components/8530/z8530.cpp
+++ b/Components/8530/z8530.cpp
@@ -25,17 +25,25 @@ bool z8530::get_interrupt_line() {
 		);
 }
 
+/*
+	Per the standard defined in the header file, this implementation follows
+	an addressing convention of:
+
+	A0 = A/B		(i.e. channel select)
+	A1 = C/D		(i.e. control or data)
+*/
+
 std::uint8_t z8530::read(int address) {
 	if(address & 2) {
-		// Read data register for channel
-		return 0x00;
+		// Read data register for channel.
+		return channels_[address & 1].read(true, pointer_);
 	} else {
 		// Read control register for channel.
 		uint8_t result = 0;
 
 		switch(pointer_) {
 			default:
-				result = channels_[address & 1].read(address & 2, pointer_);
+				result = channels_[address & 1].read(false, pointer_);
 			break;
 
 			case 2:		// Handled non-symmetrically between channels.
@@ -63,7 +71,11 @@ std::uint8_t z8530::read(int address) {
 			break;
 		}
 
+		// Cf. the two-step control register selection process in ::write. Since this
+		// definitely wasn't a *write* to register 0, it follows that the next selected
+		// control register will be 0.
 		pointer_ = 0;
+
 		update_delegate();
 		return result;
 	}
@@ -73,23 +85,30 @@ std::uint8_t z8530::read(int address) {
 
 void z8530::write(int address, std::uint8_t value) {
 	if(address & 2) {
-		// Write data register for channel.
+		// Write data register for channel. This is completely independent
+		// of whatever is going on over in the control realm.
+		channels_[address & 1].write(true, pointer_, value);
 	} else {
-		// Write control register for channel.
+		// Write control register for channel; there's a two-step sequence
+		// here for the programmer. Initially the selected register
+		// (i.e. `pointer_`) is zero. That register includes a field to
+		// set the next selected register. After any other register has
+		// been written to, register 0 is selected again.
 
-		// Most registers are per channel, but a couple are shared; sever
-		// them here.
+		// Most registers are per channel, but a couple are shared;
+		// sever them here, send the rest to the appropriate chnanel.
 		switch(pointer_) {
 			default:
-				channels_[address & 1].write(address & 2, pointer_, value);
+				channels_[address & 1].write(false, pointer_, value);
 			break;
 
-			case 2:	// Interrupt vector register; shared between both channels.
+			case 2:	// Interrupt vector register; used only by Channel B.
+					// So there's only one of these.
 				interrupt_vector_ = value;
 				LOG("[SCC] Interrupt vector set to " << PADHEX(2) << int(value));
 			break;
 
-			case 9:	// Master interrupt and reset register; also shared between both channels.
+			case 9:	// Master interrupt and reset register; there is also only one of these.
 				LOG("[SCC] Master interrupt and reset register: " << PADHEX(2) << int(value));
 				master_interrupt_control_ = value;
 			break;
@@ -105,7 +124,8 @@ void z8530::write(int address, std::uint8_t value) {
 			pointer_ = value & 7;
 
 			// If the command part of the byte is a 'point high', also set the
-			// top bit of the pointer.
+			// top bit of the pointer. Channels themselves therefore need not
+			// (/should not) respond to the point high command.
 			if(((value >> 3)&7) == 1) {
 				pointer_ |= 8;
 			}
@@ -259,6 +279,11 @@ bool z8530::Channel::get_interrupt_line() {
 	// TODO: other potential causes of an interrupt.
 }
 
+/*!
+	Evaluates the new level of the interrupt line and notifies the delegate if
+	both: (i) there is one; and (ii) the interrupt line has changed since last
+	the delegate was notified.
+*/
 void z8530::update_delegate() {
 	const bool interrupt_line = get_interrupt_line();
 	if(interrupt_line != previous_interrupt_line_) {
diff --git a/Components/8530/z8530.hpp b/Components/8530/z8530.hpp
index 748b71081..7e081d544 100644
--- a/Components/8530/z8530.hpp
+++ b/Components/8530/z8530.hpp
@@ -30,16 +30,33 @@ class z8530 {
 				A/B = A0
 				C/D = A1
 		*/
+
+		/// Performs a read from the SCC; see above for conventions as to 'address'.
 		std::uint8_t read(int address);
+		/// Performs a write to the SCC; see above for conventions as to 'address'.
 		void write(int address, std::uint8_t value);
+		/// Resets the SCC.
 		void reset();
+
+		/// @returns The current value of the status output: @c true for active; @c false for inactive.
 		bool get_interrupt_line();
 
 		struct Delegate {
-			virtual void did_change_interrupt_status(z8530 *, bool new_status) = 0;
+			/*!
+				Communicates that @c scc now has the interrupt line status @c new_status.
+			*/
+			virtual void did_change_interrupt_status(z8530 *scc, bool new_status) = 0;
 		};
+
+		/*!
+			Sets the delegate for this SCC. If this is a new delegate it is sent
+			an immediate did_change_interrupt_status message, to get it
+			up to speed.
+		*/
 		void set_delegate(Delegate *delegate) {
+			if(delegate_ == delegate) return;
 			delegate_ = delegate;
+			delegate_->did_change_interrupt_status(this, get_interrupt_line());
 		}
 
 		/*