Merge pull request #365 from TomHarte/CartridgeDetermination

Works towards eliminating the special cases for Atari 2600 ROM handling.

Analyser/Dynamic/MultiMachine/MultiMachine.cpp

@@ -62,8 +62,15 @@ Configurable::Device *MultiMachine::configurable_device() {
 	}
 }
 
+bool MultiMachine::would_collapse(const std::vector<std::unique_ptr<DynamicMachine>> &machines) {
+	return
+		(machines.front()->crt_machine()->get_confidence() > 0.9f) ||
+		(machines.front()->crt_machine()->get_confidence() >= 2.0f * machines[1]->crt_machine()->get_confidence());
+}
+
 void MultiMachine::multi_crt_did_run_machines() {
 	std::lock_guard<std::mutex> machines_lock(machines_mutex_);
+#ifdef DEBUG
 	for(const auto &machine: machines_) {
 		CRTMachine::Machine *crt = machine->crt_machine();
 		printf("%0.2f ", crt->get_confidence());
@@ -71,6 +78,7 @@ void MultiMachine::multi_crt_did_run_machines() {
 		printf("; ");
 	}
 	printf("\n");
+#endif
 
 	DynamicMachine *front = machines_.front().get();
 	std::stable_sort(machines_.begin(), machines_.end(),
@@ -84,10 +92,7 @@ void MultiMachine::multi_crt_did_run_machines() {
 		crt_machine_.did_change_machine_order();
 	}
 
-	if(
-		(machines_.front()->crt_machine()->get_confidence() > 0.9f) ||
-		(machines_.front()->crt_machine()->get_confidence() >= 2.0f * machines_[1]->crt_machine()->get_confidence())
-	) {
+	if(would_collapse(machines_)) {
 		pick_first();
 	}
 }

Analyser/Dynamic/MultiMachine/MultiMachine.hpp

@@ -40,6 +40,14 @@ namespace Dynamic {
 */
 class MultiMachine: public ::Machine::DynamicMachine, public MultiCRTMachine::Delegate {
 	public:
+		/*!
+			Allows a potential MultiMachine creator to enquire as to whether there's any benefit in
+			requesting this class as a proxy.
+
+			@returns @c true if the multimachine would discard all but the first machine in this list;
+				@c false otherwise.
+		*/
+		static bool would_collapse(const std::vector<std::unique_ptr<DynamicMachine>> &machines);
 		MultiMachine(std::vector<std::unique_ptr<DynamicMachine>> &&machines);
 
 		ConfigurationTarget::Machine *configuration_target() override;

Analyser/Static/Acorn/StaticAnalyser.cpp

@@ -59,7 +59,7 @@ static std::vector<std::shared_ptr<Storage::Cartridge::Cartridge>>
 void Analyser::Static::Acorn::AddTargets(const Media &media, std::vector<std::unique_ptr<Target>> &destination) {
 	std::unique_ptr<Target> target(new Target);
 	target->machine = Machine::Electron;
-	target->confidence = 1.0; // TODO: a proper estimation
+	target->confidence = 0.5; // TODO: a proper estimation
 	target->acorn.has_dfs = false;
 	target->acorn.has_adfs = false;
 	target->acorn.should_shift_restart = false;

Analyser/Static/AmstradCPC/StaticAnalyser.cpp

@@ -180,7 +180,7 @@ static bool CheckBootSector(const std::shared_ptr<Storage::Disk::Disk> &disk, co
 void Analyser::Static::AmstradCPC::AddTargets(const Media &media, std::vector<std::unique_ptr<Target>> &destination) {
 	std::unique_ptr<Target> target(new Target);
 	target->machine = Machine::AmstradCPC;
-	target->confidence = 1.0;
+	target->confidence = 0.5;
 	target->media.disks = media.disks;
 	target->media.tapes = media.tapes;
 	target->media.cartridges = media.cartridges;

Analyser/Static/Atari/StaticAnalyser.cpp

@@ -179,10 +179,10 @@ static void DeterminePagingForCartridge(Analyser::Static::Target &target, const
 }
 
 void Analyser::Static::Atari::AddTargets(const Media &media, std::vector<std::unique_ptr<Target>> &destination) {
-	// TODO: sanity checking; is this image really for an Atari 2600.
+	// TODO: sanity checking; is this image really for an Atari 2600?
 	std::unique_ptr<Target> target(new Target);
 	target->machine = Machine::Atari2600;
-	target->confidence = 1.0;
+	target->confidence = 0.5;
 	target->media.cartridges = media.cartridges;
 	target->atari.paging_model = Atari2600PagingModel::None;
 	target->atari.uses_superchip = false;

Analyser/Static/Coleco/StaticAnalyser.cpp

@@ -55,7 +55,7 @@ static std::vector<std::shared_ptr<Storage::Cartridge::Cartridge>>
 void Analyser::Static::Coleco::AddTargets(const Media &media, std::vector<std::unique_ptr<Target>> &destination) {
 	std::unique_ptr<Target> target(new Target);
 	target->machine = Machine::ColecoVision;
-	target->confidence = 0.5;
+	target->confidence = 1.0f - 1.0f / 32768.0f;
 	target->media.cartridges = ColecoCartridgesFrom(media.cartridges);
 	if(!target->media.empty())
 		destination.push_back(std::move(target));

Analyser/Static/Commodore/StaticAnalyser.cpp

@@ -41,7 +41,7 @@ static std::vector<std::shared_ptr<Storage::Cartridge::Cartridge>>
 void Analyser::Static::Commodore::AddTargets(const Media &media, std::vector<std::unique_ptr<Target>> &destination) {
 	std::unique_ptr<Target> target(new Target);
 	target->machine = Machine::Vic20;	// TODO: machine estimation
-	target->confidence = 1.0; // TODO: a proper estimation
+	target->confidence = 0.5; // TODO: a proper estimation
 
 	int device = 0;
 	std::vector<File> files;

Analyser/Static/MSX/StaticAnalyser.cpp

@@ -286,7 +286,7 @@ void Analyser::Static::MSX::AddTargets(const Media &media, std::vector<std::uniq
 
 	if(!target->media.empty()) {
 		target->machine = Machine::MSX;
-		target->confidence = 1.0;
+		target->confidence = 0.5;
 		destination.push_back(std::move(target));
 	}
 }

Analyser/Static/Oric/StaticAnalyser.cpp

@@ -76,7 +76,7 @@ static int Basic11Score(const Analyser::Static::MOS6502::Disassembly &disassembl
 void Analyser::Static::Oric::AddTargets(const Media &media, std::vector<std::unique_ptr<Target>> &destination) {
 	std::unique_ptr<Target> target(new Target);
 	target->machine = Machine::Oric;
-	target->confidence = 1.0;
+	target->confidence = 0.5;
 
 	int basic10_votes = 0;
 	int basic11_votes = 0;

Analyser/Static/StaticAnalyser.cpp

@@ -88,7 +88,7 @@ static Media GetMediaAndPlatforms(const char *file_name, TargetPlatform::IntType
 		Format("81", result.tapes, Tape::ZX80O81P, TargetPlatform::ZX8081)										// 81
 		Format("a26", result.cartridges, Cartridge::BinaryDump, TargetPlatform::Atari2600)						// A26
 		Format("adf", result.disks, Disk::DiskImageHolder<Storage::Disk::AcornADF>, TargetPlatform::Acorn)		// ADF
-		Format("bin", result.cartridges, Cartridge::BinaryDump, TargetPlatform::Atari2600)						// BIN
+		Format("bin", result.cartridges, Cartridge::BinaryDump, TargetPlatform::AllCartridge)					// BIN
 		Format("cas", result.tapes, Tape::CAS, TargetPlatform::MSX)												// CAS
 		Format("cdt", result.tapes, Tape::TZX, TargetPlatform::AmstradCPC)										// CDT
 		Format("col", result.cartridges, Cartridge::BinaryDump, TargetPlatform::ColecoVision)					// COL
@@ -120,7 +120,7 @@ static Media GetMediaAndPlatforms(const char *file_name, TargetPlatform::IntType
 		Format(	"rom",
 				result.cartridges,
 				Cartridge::BinaryDump,
-				TargetPlatform::Acorn | TargetPlatform::MSX | TargetPlatform::ColecoVision)						// ROM
+				TargetPlatform::AcornElectron | TargetPlatform::ColecoVision | TargetPlatform::MSX)				// ROM
 		Format("ssd", result.disks, Disk::DiskImageHolder<Storage::Disk::SSD>, TargetPlatform::Acorn)			// SSD
 		Format("tap", result.tapes, Tape::CommodoreTAP, TargetPlatform::Commodore)								// TAP (Commodore)
 		Format("tap", result.tapes, Tape::OricTAP, TargetPlatform::Oric)										// TAP (Oric)

Machines/Atari2600/Atari2600.cpp

@@ -74,9 +74,7 @@ class ConcreteMachine:
 	public Machine,
 	public Outputs::CRT::Delegate {
 	public:
-		ConcreteMachine() :
-			frame_record_pointer_(0),
-			is_ntsc_(true) {
+		ConcreteMachine() {
 			set_clock_rate(NTSC_clock_rate);
 		}
 
@@ -179,6 +177,7 @@ class ConcreteMachine:
 
 		void run_for(const Cycles cycles) override {
 			bus_->run_for(cycles);
+			bus_->apply_confidence(confidence_counter_);
 		}
 
 		// to satisfy Outputs::CRT::Delegate
@@ -219,6 +218,10 @@ class ConcreteMachine:
 			}
 		}
 
+		float get_confidence() override {
+			return confidence_counter_.get_confidence();
+		}
+
 	private:
 		// the bus
 		std::unique_ptr<Bus> bus_;
@@ -230,9 +233,12 @@ class ConcreteMachine:
 
 			FrameRecord() : number_of_frames(0), number_of_unexpected_vertical_syncs(0) {}
 		} frame_records_[4];
-		unsigned int frame_record_pointer_;
-		bool is_ntsc_;
+		unsigned int frame_record_pointer_ = 0;
+		bool is_ntsc_ = true;
 		std::vector<std::unique_ptr<Inputs::Joystick>> joysticks_;
+
+		// a confidence counter
+		Analyser::Dynamic::ConfidenceCounter confidence_counter_;
 };
 
 }

Machines/Atari2600/Bus.hpp

@@ -14,6 +14,7 @@
 #include "TIA.hpp"
 #include "TIASound.hpp"
 
+#include "../../Analyser/Dynamic/ConfidenceCounter.hpp"
 #include "../../ClockReceiver/ClockReceiver.hpp"
 #include "../../Outputs/Speaker/Implementation/LowpassSpeaker.hpp"
 
@@ -23,11 +24,10 @@ class Bus {
 	public:
 		Bus() :
 			tia_sound_(audio_queue_),
-			speaker_(tia_sound_),
-			tia_input_value_{0xff, 0xff},
-			cycles_since_speaker_update_(0) {}
+			speaker_(tia_sound_) {}
 
 		virtual void run_for(const Cycles cycles) = 0;
+		virtual void apply_confidence(Analyser::Dynamic::ConfidenceCounter &confidence_counter) = 0;
 		virtual void set_reset_line(bool state) = 0;
 
 		// the RIOT, TIA and speaker
@@ -39,7 +39,7 @@ class Bus {
 		Outputs::Speaker::LowpassSpeaker<TIASound> speaker_;
 
 		// joystick state
-		uint8_t tia_input_value_[2];
+		uint8_t tia_input_value_[2] = {0xff, 0xff};
 
 	protected:
 		// speaker backlog accumlation counter

Machines/Atari2600/Cartridges/Cartridge.hpp

@@ -39,7 +39,23 @@ template<class T> class Cartridge:
 			// consider doing something less fragile.
 		}
 
-		void run_for(const Cycles cycles)	{ m6502_.run_for(cycles);		}
+		void run_for(const Cycles cycles)	{
+			// Horizontal counter resets are used as a proxy for whether this really is an Atari 2600
+			// title. Random memory accesses are likely to trigger random counter resets.
+			horizontal_counter_resets_ = 0;
+			cycle_count_ = cycles;
+			m6502_.run_for(cycles);
+		}
+
+		/*!
+			Adjusts @c confidence_counter according to the results of the most recent run_for.
+		*/
+		void apply_confidence(Analyser::Dynamic::ConfidenceCounter &confidence_counter) {
+			if(cycle_count_.as_int() < 200) return;
+			if(horizontal_counter_resets_ > 10)
+				confidence_counter.add_miss();
+		}
+
 		void set_reset_line(bool state)		{ m6502_.set_reset_line(state);	}
 
 		// to satisfy CPU::MOS6502::Processor
@@ -108,7 +124,11 @@ template<class T> class Cartridge:
 							case 0x01:	update_video();	tia_->set_blank(*value & 0x02);		break;
 
 							case 0x02:	m6502_.set_ready_line(true);						break;
-							case 0x03:	update_video();	tia_->reset_horizontal_counter();	break;
+							case 0x03:
+								update_video();
+								tia_->reset_horizontal_counter();
+								horizontal_counter_resets_++;
+							break;
 								// TODO: audio will now be out of synchronisation — fix
 
 							case 0x04:
@@ -189,6 +209,9 @@ template<class T> class Cartridge:
 
 	private:
 		T bus_extender_;
+		int horizontal_counter_resets_ = 0;
+		Cycles cycle_count_;
+
 };
 
 }

Machines/ColecoVision/ColecoVision.cpp

@@ -23,6 +23,8 @@
 #include "../../Outputs/Speaker/Implementation/CompoundSource.hpp"
 #include "../../Outputs/Speaker/Implementation/LowpassSpeaker.hpp"
 
+#include "../../Analyser/Dynamic/ConfidenceCounter.hpp"
+
 namespace {
 const int sn76489_divider = 2;
 }
@@ -198,6 +200,7 @@ class ConcreteMachine:
 			uint16_t address = cycle.address ? *cycle.address : 0x0000;
 			switch(cycle.operation) {
 				case CPU::Z80::PartialMachineCycle::ReadOpcode:
+					if(!address) pc_zero_accesses_++;
 				case CPU::Z80::PartialMachineCycle::Read:
 					if(address < 0x2000) {
 						if(super_game_module_.replace_bios) {
@@ -248,6 +251,11 @@ class ConcreteMachine:
 							} else {
 								*cycle.value = joystick->get_direction_input();
 							}
+
+							// Hitting exactly the recommended joypad input port is an indicator that
+							// this really is a ColecoVision game. The BIOS won't do this when just waiting
+							// to start a game (unlike accessing the VDP and SN).
+							if((address&0xfc) == 0xfc) confidence_counter_.add_hit();
 						} break;
 
 						default:
@@ -332,6 +340,11 @@ class ConcreteMachine:
 			audio_queue_.perform();
 		}
 
+		float get_confidence() override {
+			if(pc_zero_accesses_ > 1) return 0.0f;
+			return confidence_counter_.get_confidence();
+		}
+
 	private:
 		inline void page_megacart(uint16_t address) {
 			const std::size_t selected_start = (static_cast<std::size_t>(address&63) << 14) % cartridge_.size();
@@ -371,6 +384,9 @@ class ConcreteMachine:
 		HalfCycles time_since_vdp_update_;
 		HalfCycles time_since_sn76489_update_;
 		HalfCycles time_until_interrupt_;
+
+		Analyser::Dynamic::ConfidenceCounter confidence_counter_;
+		int pc_zero_accesses_ = 0;
 };
 
 }

Machines/Utility/MachineForTarget.cpp

@@ -79,7 +79,13 @@ namespace {
 			}
 		}
 
-		return new Analyser::Dynamic::MultiMachine(std::move(machines));
+		// If a multimachine would just instantly collapse the list to a single machine, do
+		// so without the ongoing baggage of a multimachine.
+		if(Analyser::Dynamic::MultiMachine::would_collapse(machines)) {
+			return machines.front().release();
+		} else {
+			return new Analyser::Dynamic::MultiMachine(std::move(machines));
+		}
 	}
 
 	// There's definitely exactly one target.

OSBindings/Mac/Clock SignalTests/MSXStaticAnalyserTests.mm

@@ -10,21 +10,22 @@
 
 #import <CommonCrypto/CommonDigest.h>
 #include "../../../Analyser/Static/StaticAnalyser.hpp"
+#include "../../../Analyser/Static/MSX/Cartridge.hpp"
 
 @interface MSXROMRecord : NSObject
-@property(nonatomic, readonly) Analyser::Static::MSXCartridgeType cartridgeType;
-+ (instancetype)recordWithCartridgeType:(Analyser::Static::MSXCartridgeType)cartridgeType;
+@property(nonatomic, readonly) Analyser::Static::MSX::Cartridge::Type cartridgeType;
++ (instancetype)recordWithCartridgeType:(Analyser::Static::MSX::Cartridge::Type)cartridgeType;
 @end
 
 @implementation MSXROMRecord
-+ (instancetype)recordWithCartridgeType:(Analyser::Static::MSXCartridgeType)cartridgeType {
++ (instancetype)recordWithCartridgeType:(Analyser::Static::MSX::Cartridge::Type)cartridgeType {
 	MSXROMRecord *record = [[MSXROMRecord alloc] init];
 	record->_cartridgeType = cartridgeType;
 	return record;
 }
 @end
 
-#define Record(sha, type) sha : [MSXROMRecord recordWithCartridgeType:Analyser::Static::MSXCartridgeType::type],
+#define Record(sha, type) sha : [MSXROMRecord recordWithCartridgeType:Analyser::Static::MSX::Cartridge::type],
 static NSDictionary<NSString *, MSXROMRecord *> *romRecordsBySHA1 = @{
 	Record(@"da397e783d677d1a78fff222d9d6cb48b915dada", ASCII8kb)		// 1942 (1986)(ASCII)(JP).rom
 	Record(@"0733cd627467a866846e15caf1770a5594eaf4cc", ASCII8kb)		// 1942 (1986)(ASCII)(JP)[a].rom
@@ -222,7 +223,13 @@ static NSDictionary<NSString *, MSXROMRecord *> *romRecordsBySHA1 = @{
 		// assert equality
 		XCTAssert(!targets.empty(), "%@ should be recognised as an MSX file", testFile);
 		if(!targets.empty()) {
-			XCTAssert(targets.front()->msx.cartridge_type == romRecord.cartridgeType, @"%@; should be %d, is %d", testFile, romRecord.cartridgeType, targets.front()->msx.cartridge_type);
+			XCTAssert(!targets.front()->media.cartridges.empty(), "%@ should be interpreted as a cartridge", testFile);
+
+			if(!targets.front()->media.cartridges.empty()) {
+				const Analyser::Static::MSX::Cartridge *const cartridge =
+					dynamic_cast<Analyser::Static::MSX::Cartridge *>(targets.front()->media.cartridges.front().get());
+				XCTAssert(cartridge->type == romRecord.cartridgeType, @"%@; should be %d, is %d", testFile, romRecord.cartridgeType, cartridge->type);
+			}
 		}
 	}
 }

Storage/TargetPlatforms.hpp

@@ -29,8 +29,9 @@ enum Type: IntType {
 
 	Acorn			=	AcornAtom | AcornElectron | BBCMaster | BBCModelA | BBCModelB,
 	ZX8081			=	ZX80 | ZX81,
-	AllTape			=	Acorn | AmstradCPC | Commodore | Oric | ZX80 | ZX81 | MSX,
+	AllCartridge	=	Atari2600 | AcornElectron | ColecoVision | MSX,
 	AllDisk			=	Acorn | AmstradCPC | Commodore | Oric | MSX,
+	AllTape			=	Acorn | AmstradCPC | Commodore | Oric | ZX80 | ZX81 | MSX,
 };
 
 class TypeDistinguisher {