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

Memcard: Add tracking for when memory cards are busy writing #10372

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

RedPanda4552
Copy link
Contributor

@RedPanda4552 RedPanda4552 commented Dec 8, 2023

Description of Changes

Adds a counter, set to 300 frames when a write operation is sent to a memory card. Every frame, the counter is decremented. On shutdown, the counter is cleared.

Also moved memory card auto eject countdown function out to Counters.cpp. It will run at the same frequency, it's just now it's not stuck in CDVD anymore, it's in Counters.cpp which makes more sense.

Rationale behind Changes

New MemcardBusy::IsBusy() function can be used by the frontend and/or emu thread to detect if memcard activity has happened within the last 300 frames. That can then be used to determine whether to continue with or abort shutdowns, restarts, disc swaps, savestates, etc.

I attempted to make the frontend changes but quickly realized I did not understand how anything worked. I am going to either need assistance or pass that off to someone who does.

Suggested Testing Steps

MAKE NEW MEMCARDS. DO NOT PUT MEMCARDS YOU CARE ABOUT ANYWHERE NEAR THIS.

Boot a game, get it to a point where it will write something to the memcard. Purposely try to shut it down, reset it, load a savestate, while it is writing to the memcard. You should be blocked from completing your "desired" action.

@github-actions github-actions bot added the GUI/Qt label Dec 9, 2023
@RedPanda4552
Copy link
Contributor Author

Attempted to cover all entry points for VM shutdowns/resets/states which could be possible danger points for memcards. Try to find ways to fool the system into allowing you to load a state, save a state, shutdown, reset, etc.

Copy link
Contributor

@stenzek stenzek left a comment

Choose a reason for hiding this comment

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

There probably should be two variants of the warning - one from within Qt (like you've done here), and another that uses ImGuiFullscreen's confirm prompt, for if the shutdown is initiated from Big Picture. Locking the VM and exiting fullscreen still tends to be problematic on some platforms, and janky at best.

@@ -2730,6 +2784,22 @@ void MainWindow::doDiscChange(CDVD_SourceType source, const QString& path)
{
const auto lock = pauseAndLockVM();

// Check if memcard is busy, deny request if so
if (!GSDumpReplayer::IsReplayingDump() && MemcardBusy::IsBusy())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be extracted out into a function - don't duplicate it 3 times.

VMLock lock(pauseAndLockVM());

QMessageBox msgbox(lock.getDialogParent());
msgbox.setIcon(QMessageBox::Warning);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to QMessageBox::question(...).

// Decremented once per frame if nonzero, indicates how many more frames must pass before
// memcards are considered "no longer being written to". Used as a way to detect if it is
// unsafe to shutdown the VM due to memcard access.
size_t MemcardBusy::currentTicks = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not accessed outside of these functions, so it can be static/file-local.

@RedPanda4552
Copy link
Contributor Author

RedPanda4552 commented Dec 10, 2023

@stenzek Code quality issues should be addressed on the existing bits.

Also added for big picture. I did however end up doing more duplication, but that is because I could not for the life of me figure out how to pass a function reference and then have it also survive down to the lambda callback. Since shutdown takes a bool param and the others do not I tried templating it and for whatever reason I could not get anything to compile.

pcsx2-qt/MainWindow.cpp Outdated Show resolved Hide resolved
pcsx2-qt/MainWindow.cpp Outdated Show resolved Hide resolved
pcsx2/ImGui/FullscreenUI.cpp Show resolved Hide resolved
pcsx2/SIO/Sio.cpp Outdated Show resolved Hide resolved
pcsx2/SIO/Sio.cpp Outdated Show resolved Hide resolved
pcsx2/SIO/Sio.cpp Outdated Show resolved Hide resolved
pcsx2/SIO/Sio.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@stenzek stenzek left a comment

Choose a reason for hiding this comment

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

Code LGTM, untested

Copy link
Contributor

@Mrlinkwii Mrlinkwii left a comment

Choose a reason for hiding this comment

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

image

image

works well

const QMessageBox::StandardButton res = QMessageBox::question(
lock.getDialogParent(),
tr("WARNING: Memory Card Busy"),
tr("WARNING: Your memory card is still writing data. Shutting down now will IRREVERSIBLY DESTORY YOUR MEMORY CARD. It is strongly recommended to resume your game and let it finish writing to your memory card.\n\nDo you wish to shutdown anyways and IRREVERSIBLY DESTROY YOUR MEMORY CARD?")

This comment was marked as resolved.

@stenzek stenzek merged commit feb9d7b into PCSX2:master Dec 12, 2023
12 checks passed
@RedPanda4552 RedPanda4552 deleted the memcard-warnings branch December 13, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants