Core: Fix NMI code to better reflect the hardware

Fixes a crash in Super Chinese 3 caused by the PPU rapidly switching the NMI signal on and then back off (which caused the CPU to incorrectly jump to the IRQ handler and crashed the game)

Core/CPU.cpp

@@ -145,7 +145,7 @@ void CPU::Reset(bool softReset, NesModel model)
 		string cpuAlignment = " CPU: " + std::to_string(cpuOffset) + "/" + std::to_string(cpuDivider - 1);
 		MessageManager::Log("CPU/PPU alignment -" + ppuAlignment + cpuAlignment);
 	} else {
-		_ppuOffset = 2;
+		_ppuOffset = 1;
 		cpuOffset = 0;
 	}
 
@@ -165,7 +165,7 @@ void CPU::Exec()
 	_operand = FetchOperand();
 	(this->*_opTable[opCode])();
 	
-	if(_prevRunIrq) {
+	if(_prevRunIrq || _prevNeedNmi) {
 		IRQ();
 	}
 }
@@ -180,12 +180,12 @@ void CPU::IRQ()
 	DummyRead();  //read next instruction byte (actually the same as above, since PC increment is suppressed. Also discarded.)
 	Push((uint16_t)(PC()));
 
-	if(_state.NMIFlag) {
+	if(_needNmi) {
+		_needNmi = false;
 		Push((uint8_t)(PS() | PSFlags::Reserved));
 		SetFlags(PSFlags::Interrupt);
 
 		SetPC(MemoryReadWord(CPU::NMIVector));
-		_state.NMIFlag = false;
 
 		#ifndef DUMMYCPU
 		_console->DebugAddTrace("NMI");
@@ -208,7 +208,8 @@ void CPU::BRK() {
 	Push((uint16_t)(PC() + 1));
 
 	uint8_t flags = PS() | PSFlags::Break | PSFlags::Reserved;
-	if(_state.NMIFlag) {
+	if(_needNmi) {
+		_needNmi = false;
 		Push((uint8_t)flags);
 		SetFlags(PSFlags::Interrupt);
 
@@ -228,8 +229,8 @@ void CPU::BRK() {
 		#endif
 	}
 
-	//Since we just set the flag to prevent interrupts, do not run one right away after this (fixes nmi_and_brk & nmi_and_irq tests)
-	_prevRunIrq = false;
+	//Ensure we don't start an NMI right after running a BRK instruction (first instruction in IRQ handler must run first - needed for nmi_and_brk test)
+	_prevNeedNmi = false;
 }
 
 void CPU::MemoryWrite(uint16_t addr, uint8_t value, MemoryOperationType operationType)
@@ -320,10 +321,22 @@ void CPU::EndCpuCycle(bool forRead)
 	_masterClock += forRead ? (_endClockCount + 1) : (_endClockCount - 1);
 	_console->GetPpu()->Run(_masterClock - _ppuOffset);
 
+	//"The internal signal goes high during φ1 of the cycle that follows the one where the edge is detected,
+	//and stays high until the NMI has been handled. "
+	_prevNeedNmi = _needNmi;
+
+	//"This edge detector polls the status of the NMI line during φ2 of each CPU cycle (i.e., during the 
+	//second half of each cycle) and raises an internal signal if the input goes from being high during 
+	//one cycle to being low during the next"
+	if(!_prevNmiFlag && _state.NMIFlag) {
+		_needNmi = true;
+	}
+	_prevNmiFlag = _state.NMIFlag;
+
 	//"it's really the status of the interrupt lines at the end of the second-to-last cycle that matters."
 	//Keep the irq lines values from the previous cycle.  The before-to-last cycle's values will be used
 	_prevRunIrq = _runIrq;
-	_runIrq = _state.NMIFlag || ((_state.IRQFlag & _irqMask) > 0 && !CheckFlag(PSFlags::Interrupt));
+	_runIrq = ((_state.IRQFlag & _irqMask) > 0 && !CheckFlag(PSFlags::Interrupt));
 }
 
 void CPU::StartCpuCycle(bool forRead)
@@ -464,7 +477,8 @@ void CPU::StreamState(bool saving)
 	Stream(_state.PC, _state.SP, _state.PS, _state.A, _state.X, _state.Y, _cycleCount, _state.NMIFlag, 
 			_state.IRQFlag, _dmcDmaRunning, _spriteDmaTransfer,
 			extraScanlinesBeforeNmi, extraScanlinesBeforeNmi, dipSwitches,
-			_needDummyRead, _needHalt, _startClockCount, _endClockCount, _ppuOffset, _masterClock);
+			_needDummyRead, _needHalt, _startClockCount, _endClockCount, _ppuOffset, _masterClock,
+			_prevNeedNmi, _prevNmiFlag, _needNmi);
 
 	if(!saving) {
 		settings->SetPpuNmiConfig(extraScanlinesBeforeNmi, extraScanlinesAfterNmi);

Core/CPU.h

@@ -58,6 +58,10 @@ private:
 
 	bool _prevRunIrq = false;
 	bool _runIrq = false;
+	
+	bool _prevNmiFlag = false;
+	bool _prevNeedNmi = false;
+	bool _needNmi = false;
 
 	uint64_t _lastCrashWarning = 0;
 

Core/PPU.cpp

@@ -269,7 +269,7 @@ uint8_t PPU::PeekRAM(uint16_t addr)
 	switch(GetRegisterID(addr)) {
 		case PPURegisters::Status:
 			returnValue = ((uint8_t)_statusFlags.SpriteOverflow << 5) | ((uint8_t)_statusFlags.Sprite0Hit << 6) | ((uint8_t)_statusFlags.VerticalBlank << 7);
-			if(_scanline == 241 && _cycle < 3) {
+			if(_scanline == _nmiScanline && _cycle < 3) {
 				//Clear vertical blank flag
 				returnValue &= 0x7F;
 			}
@@ -518,15 +518,13 @@ void PPU::SetControlRegister(uint8_t value)
 	_flags.BackgroundPatternAddr = ((_state.Control & 0x10) == 0x10) ? 0x1000 : 0x0000;
 	_flags.LargeSprites = (_state.Control & 0x20) == 0x20;
 
-	//"By toggling NMI_output ($2000 bit 7) during vertical blank without reading $2002, a program can cause /NMI to be pulled low multiple times, causing multiple NMIs to be generated."
-	bool originalVBlank = _flags.VBlank;
 	_flags.VBlank = (_state.Control & 0x80) == 0x80;
-
-	if(!originalVBlank && _flags.VBlank && _statusFlags.VerticalBlank && (_scanline != -1 || _cycle != 0)) {
-		_console->GetCpu()->SetNmiFlag();
-	}
-	if(_scanline == 241 && _cycle < 3 && !_flags.VBlank) {
+	
+	//"By toggling NMI_output ($2000 bit 7) during vertical blank without reading $2002, a program can cause /NMI to be pulled low multiple times, causing multiple NMIs to be generated."
+	if(!_flags.VBlank) {
 		_console->GetCpu()->ClearNmiFlag();
+	} else if(_flags.VBlank && _statusFlags.VerticalBlank) {
+		_console->GetCpu()->SetNmiFlag();
 	}
 }
 
@@ -578,15 +576,11 @@ void PPU::UpdateStatusFlag()
 		((uint8_t)_statusFlags.Sprite0Hit << 6) |
 		((uint8_t)_statusFlags.VerticalBlank << 7);
 	_statusFlags.VerticalBlank = false;
+	_console->GetCpu()->ClearNmiFlag();
 
-	if(_scanline == 241 && _cycle < 3) {
-		//"Reading on the same PPU clock or one later reads it as set, clears it, and suppresses the NMI for that frame."
-		_statusFlags.VerticalBlank = false;
-		_console->GetCpu()->ClearNmiFlag();
-
-		if(_cycle == 0) {
-			_preventVblFlag = true;
-		}
+	if(_scanline == _nmiScanline && _cycle == 0) {
+		//"Reading one PPU clock before reads it as clear and never sets the flag or generates NMI for that frame."
+		_preventVblFlag = true;
 	}
 }
 
@@ -898,10 +892,10 @@ void PPU::UpdateGrayscaleAndIntensifyBits()
 	int pixelNumber;
 	if(_scanline >= 240) {
 		pixelNumber = 61439;
-	} else if(_cycle < 4) {
+	} else if(_cycle < 3) {
 		pixelNumber = (_scanline << 8) - 1;
 	} else if(_cycle <= 258) {
-		pixelNumber = (_scanline << 8) + _cycle - 4;
+		pixelNumber = (_scanline << 8) + _cycle - 3;
 	} else {
 		pixelNumber = (_scanline << 8) + 255;
 	}
@@ -945,6 +939,7 @@ void PPU::ProcessScanline()
 			//Pre-render scanline logic
 			if(_cycle == 1) {
 				_statusFlags.VerticalBlank = false;
+				_console->GetCpu()->ClearNmiFlag();
 			}
 			if(_state.SpriteRamAddr >= 0x08 && IsRenderingEnabled() && !_settings->CheckFlag(EmulationFlags::DisableOamAddrBug)) {
 				//This should only be done if rendering is enabled (otherwise oam_stress test fails immediately)