Skip to content

RTC state machine improvements #3459

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

Merged
merged 10 commits into from
Apr 30, 2025

Conversation

CasualPokePlayer
Copy link
Contributor

@CasualPokePlayer CasualPokePlayer commented Apr 9, 2025

Unsure if all of this is entirely right (haven't ran any tests to confirm behavior here). Fixes RTC within Pokemon games (and maybe other games) due to the added write latch code adding a _readPins call to direction writes.

Unsure if all of this is entirely right (haven't ran any tests to confirm behavior here). Fixes RTC within Pokemon games (and maybe other games) due to the added write latch code adding a _readPins call.
@CasualPokePlayer
Copy link
Contributor Author

CasualPokePlayer commented Apr 9, 2025

Some quick testing seems to indicate this isn't entirely correct. It is correct in that spamming CS | SCK writes will not continously increase the bits read (makes sense, the RTC should have no way to distinguish these since it should only be able to react to edges happening). However, initial testing made it seem like simply setting CS to low would not abort the RTC command.

However, the old code seems rather incorrect completely? The RTC transfer step 0 and 1 seem to be unneeded in practice. Those steps seem to be more how games usually interact with RTC. They kind of make sense with a more strict reading of the spec sheet, but in practice, it seems all that really happens here is a few things:

  1. CS going high starts a new RTC command (or inversely CS going low aborts the current RTC command).
  2. An SCK rising edge is required to "read" a bit.
  3. However, the actual data which is read will be SIO from before the SCK rising edge. It also doesn't appear to actually come from an SCK falling edge, after the falling edge the data can be changed without raising SCK and that data will be used, although if the data is changed at exactly the SCK rising edge that data will not be used (I suspect at the hardware level this is actually a race condition during the SCK rising edge which the old SIO data ends up winning).

@endrift
Copy link
Member

endrift commented Apr 10, 2025

I can review this once you've gotten it tested well. You mention it fixes some Pokémon stuff but don't say how.

@CasualPokePlayer
Copy link
Contributor Author

CasualPokePlayer commented Apr 10, 2025

The current (well, before the write latch PR) RTC state machine kind of only works with game code, but not any code which decides to be more naughty. The write latch PR proceeded to insert an extra _readPins call within direction change, which is sort of more the same as a "naughty" write writing the exact same data as before (which should do nothing, and my testing indicates that is the case), but the RTC state machine did not operate strictly on edges everywhere, so this triggered an extra bit read (but this is without SCK actually rising, so this should not happen).

also correctly handle rtc output in general, even in cases outside of an rtc read cmd
_outputPins needed to be corrected here, it shouldn't be reading gpioBase here...
Separate out command start and command write data processing
Command start processing happens again if the command magic is invalid (note: doesn't apply to the unmapped command 5)
Ensure command data processing loops
Output 1s for commands with no actual output
@CasualPokePlayer CasualPokePlayer changed the title Try to correct the RTC state machine to only operate on edges RTC state machine improvements Apr 17, 2025
@CasualPokePlayer CasualPokePlayer marked this pull request as ready for review April 17, 2025 06:23
Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

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

The savestate format change is the only part of this that I think needs any revision. Otherwise, it all looks good.

@endrift endrift merged commit c5cddc0 into mgba-emu:master Apr 30, 2025
1 check failed
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