Skip to content

Fixed incorrect behavior of SHA, SHX, SHY, and SHS in NesHawk #4309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

100thCoin
Copy link
Contributor

Opcodes $93, $9B, $9C, $9E, and $9F, also referred to as the "Unstable High Byte Group", all rely on the High Byte of the target address being incremented internally when the processor adds the X or Y register as an offset, and this new value of the high byte is ANDed with the value being stored.

In addition to adding the byte H, which is currently only updated in the relevant cycles for the Unstable High Byte Group instructions, I also added the private void AbsIdx_Stage4_SHS(), which is pretty much the same as AbsIdx_Stage4() but it updates H. I've removed some lines from the various AbsIdx_WRITE_Stage5 functions which were incorrectly modifying the target address "ea".

I also renamed AbsIdx_WRITE_Stage5_ERROR to AbsIdx_WRITE_Stage5_SHS

This should now match the behavior documented in the "No More Secrets" MOS 6510 Unintended Opcodes document. I have created a test rom for the purposes of verifying this behavior, and both my console and now NesHawk pass the test. It's worth noting that Blargg's accuracy tests that check for the unofficial opcodes don't actually check any of these except opcode $9E, and that test was written long before the research done for the "No More Secrets" document.

Here's how my test cartridge looks on my physical NES

ConsoleResults

NesHawk before this change fails this test when checking the behavior of opcode $9F: (The error codes in this ROM are a work in progress, but that's what that "D" next to "FAIL" is indicating.)

Results_Before

And after the change:

Results_After

100thCoin added 2 commits May 9, 2025 22:45
Opcodes $93, $9B, $9C, $9E, and $9F, also referred to as the "Unstable High Byte Group", all rely on the High Byte of the target address being incremented internally when the processor adds the X or Y register as an offset, and this new value of the high byte is ANDed with the value being stored.
In the comment made for private byte H, changed the word "varaible" to "variable"
@YoshiRulz YoshiRulz requested a review from SaxxonPike May 10, 2025 03:54
@YoshiRulz
Copy link
Member

see also #4177

Added H to the 6502 SyncState, which is used during the final cycles of a few unofficial instructions.
@100thCoin
Copy link
Contributor Author

Here are the results of the C64 tests for these instructions

Results_C64

100thCoin added 2 commits May 10, 2025 11:46
This now passes Disk2.d64's "LXAB" test.
Does this need to change in Drive1541.cs as well?
This now passes Disk2.d64's "TRAP1" test.
Does this need to change in Drive1541.cs as well?
@100thCoin
Copy link
Contributor Author

100thCoin commented May 10, 2025

I have updated both the LXA and ANE constants specifically for Chip6510.cs, and the emulator now passes the "lxab" and "trap1" tests from disk2.d64 of tsuit215_d64.zip.

Results_C64_ANEB

Results_C64_Trap1Test

Should the constants in Drive1541.cs be updated as well? I'm unsure how to test for that.

SHA, for example, typically writes a value of A & X & H, but if the RDY line is low 2 cycles before the write occurs, (tested with a DMC DMA) the value written is just A & X. To recreate that behavior, H is set to $FF if the RDY line is low during that particular stage of the instruction.
@100thCoin
Copy link
Contributor Author

The newest commit adds the behavior of these instructions when the RDY line goes low on a specific cycle, which omits the & H part of the instruction's calculation. I also re-ran the c64 tests, and it still passes, leading me to assume those tests don't check for the RDY behavior.

Here's the results when ran in the accuracy test rom I've been working on.

(It was also discovered that late RP2A03G CPUs and later CPU revisions seem to have slightly different behavior for SHA and SHS, so this test also prints "Behavior 1"/"Behavior 2" depending on the results of the test. At least, the current hypothesis is that it's late RP2A03G's and later... more research needs to be done there.)

NewResults

100thCoin added 7 commits May 19, 2025 14:31
The ext_ppu_cycle was only used in SubNesHawk, though it would still appear in the tracelogger for NesHawk, just always with a value of 0.
This value is now incremented at the end of runppu() in PPU.cs, and reset at the beginning of a frame inside DoFrame() of SubNesHawk, and FrameAdvance() for NesHawk.
…ect cycles

Added the 16 bit address bus as a public variable to Execute.cs
The 6502 Address Bus is not updated by the DMC DMA or OAM DMA.
The DMC DMA's "halt cycles" and "put cycles" read from wherever the 6502 address bus currently is. This can result in extra reads from read-sensitive PPU and APU registers.
Likewise, APU Register Activation is tied to the 6502 address bus. (Previously using the value of the PC, as the address bus will be the value of the PC during OAM DMAs)
I have no idea what added these, but it was a mistake.
Added the 6502 address bus to the SyncState
Controllers only get strobed when the CPU transitions from a get cycle to a put cycle.
Controllers only get clocked if the previous CPU cycle was not reading from the controller port.
@@ -2509,6 +2518,7 @@ private void AbsIdx_Stage4_SHX()

if (RDY)
{
H |= (byte) ((ea >> 8) + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we checked what happens here if ea is greater than or equal to 0xFF00? There are a few places with a similar line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants