Skip to content
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

Proposed fix for the behaviour of STP #4

Merged
merged 2 commits into from
Feb 2, 2024
Merged

Conversation

NollKollTroll
Copy link
Contributor

I have 2 setups, one using a real WDC65C02 connected to a Teensy 4.1 and another using vrEmu6502 on a Pico.
The behaviour of STP in the emu did not correspond to reality, so here is a proposed fix that seems to work well for me.
I am not confident in the new code of the STP-function, it might need some more code, but the patched behaviour of STP reflects what I observe on the WDC65C02 version.

@visrealm
Copy link
Owner

visrealm commented Feb 1, 2024

Thanks for the bug report and proposed fix. Appreciated!

I think your proposed fix is good, but (for minor performance gains where branch prediction isn't available) it might be a reasonable idea to combine jam and stp struct members into a single member. That avoids the second if() (branch) in vrEmu6502Tick. Perhaps we replace jam with stp? I'd also be open to combining the two if() statements into a single one if (vr6502->jam | vr6502->stp)

@NollKollTroll
Copy link
Contributor Author

I agree, combining the 2 if would be faster and also a bit clearer. I like the single stp version best, jam can use that and still be easy to understand. I have pushed a new version in my fork reflecting this, hopefully it is what you envisioned.

@visrealm visrealm merged commit 61f7371 into visrealm:main Feb 2, 2024
3 checks passed
@visrealm
Copy link
Owner

visrealm commented Feb 2, 2024

Looks good, Adam. Merged :)

This pull request was closed.
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.

2 participants