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

mia, desktop-ui: Add basic error handling to ROM loading #1785

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

Conversation

jcm93
Copy link
Contributor

@jcm93 jcm93 commented Jan 20, 2025

Note

Seeking testers on this to validate that errors appear correctly, are accurate, and we didn't break anything.

Description

Adds the LoadResult type, used to identify and propagate upwards errors encountered when loading ROMs and systems.

Modifies the mia and desktop-ui load routines to use this type instead of generic booleans, so that if we have a load failure, we can propagate a precise and useful error to the user rather than a vague and possibly mismatched one.

Currently the scope of the error handling is just mia and desktop-ui; errors encountered when loading in ares itself are currently generic "otherError"s. LoadResult is placed in mia/mia.hpp so it is accessible to both mia and desktop-ui. If LoadResult is added to ares loading in the future, the LoadResult type may need to be moved.

Move LoadResult to ares.hpp, handle modal cancellation

handle firmware errors more gooder, add invalid rom error
@LukeUsher
Copy link
Member

Since most of these errors are regarding ROM loading and manifest parsing; which mia is responsible for creating/handling; I believe these error types would be better suited as part of mia.hpp.

Copy link
Member

@LukeUsher LukeUsher left a comment

Choose a reason for hiding this comment

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

Just a few notes

desktop-ui/emulator/game-gear.cpp Outdated Show resolved Hide resolved
desktop-ui/emulator/master-system.cpp Outdated Show resolved Hide resolved
desktop-ui/emulator/mega-cd-32x.cpp Outdated Show resolved Hide resolved
@jcm93
Copy link
Contributor Author

jcm93 commented Jan 21, 2025

Taking this out of draft now that it's in a better state.

if(!manifest) return romNotFoundInDatabase;

I am still not 100% on if this is correct/representative of the underlying behavior. Arcade seems to be the only system with an existing failure condition here. It seems like it's bailing out if the game entry wasn't in the database rather than using heuristics like the other cores. Just want to make sure this is accurate. And if so, maybe a more descriptive error message is appropriate? "The selected [game/Arcade system] is not currently supported by ares"?

@jcm93 jcm93 marked this pull request as ready for review January 21, 2025 19:58
@LukeUsher
Copy link
Member

Taking this out of draft now that it's in a better state.

if(!manifest) return romNotFoundInDatabase;

I am still not 100% on if this is correct/representative of the underlying behavior. Arcade seems to be the only system with an existing failure condition here. It seems like it's bailing out if the game entry wasn't in the database rather than using heuristics like the other cores. Just want to make sure this is accurate. And if so, maybe a more descriptive error message is appropriate? "The selected [game/Arcade system] is not currently supported by ares"?

This is indeed correct; due to how arcade roms are formatted, ares does not know how to load them without a database entry specifying which individual roms need to be loaded where.

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

Successfully merging this pull request may close these issues.

2 participants