NES: Ignore consecutive reads to 4016/4017

Core/NES/NesControlManager.cpp

@@ -226,17 +226,34 @@ void NesControlManager::UpdateInputState()
 	RemapControllerButtons();
 }
 
+uint8_t NesControlManager::ReadDevice(shared_ptr<BaseControlDevice>& device, uint16_t addr)
+{
+	if(_emu->GetSettings()->GetNesConfig().ConsoleType == NesConsoleType::Hvc001 && _console->GetRegion() == ConsoleRegion::Ntsc) {
+		return device->ReadRam(addr);
+	} else {
+		uint8_t value = device->GetPreviousReadValue();
+		uint64_t cpuCycle = _console->GetMasterClock();
+		if(_prevReadAddr != addr || device->GetPreviousReadCycle() < cpuCycle - 1) {
+			value = device->ReadRam(addr);
+		}
+		device->SetPreviousRead(cpuCycle, value);
+		return value;
+	}
+}
+
 uint8_t NesControlManager::ReadRam(uint16_t addr)
 {
 	SetInputReadFlag();
 
 	uint8_t value = _console->GetMemoryManager()->GetOpenBus(GetOpenBusMask(addr - 0x4016));
 	for(shared_ptr<BaseControlDevice>& device : _controlDevices) {
 		if(device->IsConnected()) {
-			value |= device->ReadRam(addr);
+			value |= ReadDevice(device, addr);
 		}
 	}
 
+	_prevReadAddr = addr;
+
 	return value;
 }
 
@@ -274,6 +291,10 @@ void NesControlManager::Serialize(Serializer& s)
 	SV(_writeValue);
 	SV(_writePending);
 
+	if(s.GetFormat() != SerializeFormat::Map) {
+		SV(_prevReadAddr);
+	}
+
 	if(!s.IsSaving()) {
 		UpdateControlDevices();
 	}

Core/NES/NesControlManager.h

@@ -22,6 +22,9 @@ class NesControlManager : public INesMemoryHandler, public BaseControlManager
 	uint16_t _writeAddr = 0;
 	uint8_t _writeValue = 0;
 	uint8_t _writePending = 0;
+	uint16_t _prevReadAddr = 0;
+
+	uint8_t ReadDevice(shared_ptr<BaseControlDevice>& device, uint16_t addr);
 
 protected:
 	NesConsole* _console;

Core/NES/NesCpu.cpp

@@ -340,32 +340,10 @@ void NesCpu::ProcessPendingDma(uint16_t readAddress, MemoryOperationType opType)
 
 	uint16_t prevReadAddress = readAddress;
 	bool enableInternalRegReads = (readAddress & 0xFFE0) == 0x4000;
-	bool skipFirstInputClock = false;
-	if(enableInternalRegReads && _dmcDmaRunning && (readAddress == 0x4016 || readAddress == 0x4017)) {
-		uint16_t dmcAddress = _console->GetApu()->GetDmcReadAddress();
-		if((dmcAddress & 0x1F) == (readAddress & 0x1F)) {
-			//DMC will cause a read on the same address as the CPU was reading from
-			//This will hide the reads from the controllers because /OE will be active the whole time
-			skipFirstInputClock = true;
-		}
-	}
-
-	//On Famicom, each dummy/idle read to 4016/4017 is intepreted as a read of the joypad registers
-	//On NES (or AV Famicom), only the first dummy/idle read causes side effects (e.g only a single bit is lost)
-	bool isNesBehavior = _console->GetNesConfig().ConsoleType != NesConsoleType::Hvc001;
-	bool skipDummyReads = isNesBehavior && (readAddress == 0x4016 || readAddress == 0x4017);
-
 	_needHalt = false;
 
 	StartCpuCycle(true);
-	if(_abortDmcDma && isNesBehavior && (readAddress == 0x4016 || readAddress == 0x4017)) {
-		//Skip halt cycle dummy read on 4016/4017
-		//The DMA was aborted, and the CPU will read 4016/4017 next
-		//If 4016/4017 is read here, the controllers will see 2 separate reads
-		//even though they would only see a single read on hardware (except the original Famicom)
-	} else if(!skipFirstInputClock) {
-		_memoryManager->Read(readAddress, MemoryOperationType::DmaRead);
-	}
+	_memoryManager->Read(readAddress, MemoryOperationType::DmaRead);
 	EndCpuCycle(true);
 
 	if(_abortDmcDma) {
@@ -407,7 +385,7 @@ void NesCpu::ProcessPendingDma(uint16_t readAddress, MemoryOperationType opType)
 				//DMC DMA is ready to read a byte (both halt and dummy read cycles were performed before this)
 				processCycle();
 				_isDmcDmaRead = true; //used by debugger to distinguish between dmc and oam/dummy dma reads
-				readValue = ProcessDmaRead(_console->GetApu()->GetDmcReadAddress(), prevReadAddress, enableInternalRegReads, isNesBehavior);
+				readValue = ProcessDmaRead(_console->GetApu()->GetDmcReadAddress(), prevReadAddress, enableInternalRegReads);
 				_isDmcDmaRead = false;
 				EndCpuCycle(true);
 				_dmcDmaRunning = false;
@@ -423,17 +401,15 @@ void NesCpu::ProcessPendingDma(uint16_t readAddress, MemoryOperationType opType)
 			} else if(_spriteDmaTransfer) {
 				//DMC DMA is not running, or not ready, run sprite DMA
 				processCycle();
-				readValue = ProcessDmaRead(_spriteDmaOffset * 0x100 + spriteReadAddr, prevReadAddress, enableInternalRegReads, isNesBehavior);
+				readValue = ProcessDmaRead(_spriteDmaOffset * 0x100 + spriteReadAddr, prevReadAddress, enableInternalRegReads);
 				EndCpuCycle(true);
 				spriteReadAddr++;
 				spriteDmaCounter++;
 			} else {
 				//DMC DMA is running, but not ready (need halt/dummy read) and sprite DMA isn't runnnig, perform a dummy read
 				assert(_needHalt || _needDummyRead);
 				processCycle();
-				if(!skipDummyReads) {
-					_memoryManager->Read(readAddress, MemoryOperationType::DmaRead);
-				}
+				_memoryManager->Read(readAddress, MemoryOperationType::DmaRead);
 				EndCpuCycle(true);
 			}
 		} else {
@@ -449,16 +425,14 @@ void NesCpu::ProcessPendingDma(uint16_t readAddress, MemoryOperationType opType)
 			} else {
 				//Align to read cycle before starting sprite DMA (or align to perform DMC read)
 				processCycle();
-				if(!skipDummyReads) {
-					_memoryManager->Read(readAddress, MemoryOperationType::DmaRead);
-				}
+				_memoryManager->Read(readAddress, MemoryOperationType::DmaRead);
 				EndCpuCycle(true);
 			}
 		}
 	}
 }
 
-uint8_t NesCpu::ProcessDmaRead(uint16_t addr, uint16_t& prevReadAddress, bool enableInternalRegReads, bool isNesBehavior)
+uint8_t NesCpu::ProcessDmaRead(uint16_t addr, uint16_t& prevReadAddress, bool enableInternalRegReads)
 {
 	//This is to reproduce a CPU bug that can occur during DMA which can cause the 2A03 to read from
 	//its internal registers (4015, 4016, 4017) at the same time as the DMA unit reads a byte from
@@ -499,15 +473,7 @@ uint8_t NesCpu::ProcessDmaRead(uint16_t addr, uint16_t& prevReadAddress, bool en
 
 			case 0x4016:
 			case 0x4017:
-				if(_console->GetRegion() == ConsoleRegion::Pal || (isNesBehavior && prevReadAddress == internalAddr)) {
-					//Reading from the same input register twice in a row, skip the read entirely to avoid
-					//triggering a bit loss from the read, since the controller won't react to this read
-					//Return the same value as the last read, instead
-					//On PAL, the behavior is unknown - for now, don't cause any bit deletions
-					val = _memoryManager->GetOpenBus();
-				} else {
-					val = _memoryManager->Read(internalAddr, MemoryOperationType::DmaRead);
-				}
+				val = _memoryManager->Read(internalAddr, MemoryOperationType::DmaRead);
 
 				if(!isSameAddress) {
 					//The DMA unit is reading from a different address, read from it too (external bus)

Core/NES/NesCpu.h

@@ -68,7 +68,7 @@ class NesCpu : public ISerializable
 	{
 		ProcessPendingDma(readAddress, opType);
 	}
-	uint8_t ProcessDmaRead(uint16_t addr, uint16_t& prevReadAddress, bool enableInternalRegReads, bool isNesBehavior);
+	uint8_t ProcessDmaRead(uint16_t addr, uint16_t& prevReadAddress, bool enableInternalRegReads);
 	__forceinline uint16_t FetchOperand();
 	__forceinline void EndCpuCycle(bool forRead);
 	void IRQ();

Core/Shared/BaseControlDevice.cpp

@@ -95,6 +95,23 @@ void BaseControlDevice::DrawController(InputHud& hud)
 	hud.EndDrawController();
 }
 
+void BaseControlDevice::SetPreviousRead(uint64_t cycle, uint8_t value)
+{
+	//Used by the NES core specifically
+	_prevReadCycle = cycle;
+	_prevReadValue = value;
+}
+
+uint8_t BaseControlDevice::GetPreviousReadValue()
+{
+	return _prevReadValue;
+}
+
+uint64_t BaseControlDevice::GetPreviousReadCycle()
+{
+	return _prevReadCycle;
+}
+
 void BaseControlDevice::SetRawState(ControlDeviceState state)
 {
 	auto lock = _stateLock.AcquireSafe();
@@ -335,4 +352,9 @@ void BaseControlDevice::Serialize(Serializer& s)
 	auto lock = _stateLock.AcquireSafe();
 	SV(_strobe);
 	SVVector(_state.State);
+
+	if(s.GetFormat() != SerializeFormat::Map) {
+		SV(_prevReadCycle);
+		SV(_prevReadValue);
+	}
 }

Core/Shared/BaseControlDevice.h

@@ -26,6 +26,10 @@ class BaseControlDevice : public ISerializable
 	bool _strobe = false;
 	ControllerType _type = ControllerType::None;
 	uint8_t _port = 0;
+
+	uint8_t _prevReadValue = 0;
+	uint64_t _prevReadCycle = 0;
+
 	bool _connected = true;
 	SimpleLock _stateLock;
 
@@ -101,6 +105,10 @@ class BaseControlDevice : public ISerializable
 	virtual uint8_t ReadRam(uint16_t addr) = 0;
 	virtual void WriteRam(uint16_t addr, uint8_t value) = 0;
 
+	void SetPreviousRead(uint64_t cycle, uint8_t value);
+	uint8_t GetPreviousReadValue();
+	uint64_t GetPreviousReadCycle();
+
 	//Used by Lua API
 	virtual vector<DeviceButtonName> GetKeyNameAssociations() { return {}; }