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

SI: Generate NOREP on timeout instead of generating Dolphin SDK reply #9551

Merged
merged 2 commits into from May 24, 2021

Conversation

endrift
Copy link
Contributor

@endrift endrift commented Mar 3, 2021

I've noticed that NOREP timeouts aren't actually generated in Dolphin when there's no reply, instead favoring generating the reply that I believe is actually created by the Dolphin SDK instead. I don't have proof that this behavior is correct, but I'm also not sure where to begin investigating.

@phire
Copy link
Member

phire commented May 3, 2021

Well, we have two choices, hardware test, or just merge it and see if anything is reported as broken.

@JMC47
Copy link
Contributor

JMC47 commented May 3, 2021

I don't know what it says about me that I feel like the second one would be more effective. Every time I rely on hardware tests it turns out that the hardware test was missing some weird edge case. When we merge without thinking, users report broken stuff, then we make a hardware test because we have to and it has more cases which is good.

Edit: This definitely needs more HW testing upon further review.

@Bonta0
Copy link
Contributor

Bonta0 commented May 19, 2021

This PR with this commit Bonta0@820fb0c fix FFCC and 4 Swords+ not noticing disconnected GBAs. This has been confirmed to be HW accurate: COMERR should only be updated upon transfer completion
There are still cases where devices RunBuffer will return 0 by default. Those should probably be changed to return an error as well since returning 0 without setting up TransferInterval would only freeze Dolphin

@endrift
Copy link
Contributor Author

endrift commented May 23, 2021

I'm going to cherry-pick that patch into this PR, but the RunBuffer issue should probably be handled separately.

@JMC47
Copy link
Contributor

JMC47 commented May 23, 2021

So is this good to merge then? I assume the windows buildbots are the way they are because of the lack of a rebase.

Hardware testing has been done.

@JMC47 JMC47 merged commit 96c1f62 into dolphin-emu:master May 24, 2021
10 of 11 checks passed
@endrift endrift deleted the si-norep branch May 25, 2021 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants