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

Only set firmware paths when internal firmware is not used #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wordmage
Copy link

When the user uses internal firmware without configuring external BIOS path, a crash can be observed when running a game:

pid: 3681, tid: 3971, name: RxCachedThreadS  >>> me.magnum.melonds.dev <<<
    01 pc 00000000004d2ae8  /data/app/~~usflvTWmCZy5aZBFXYAWbw==/me.magnum.melonds.dev-25PisQX1TquKFQM3tiMHeA==/lib/arm64/libmelonDS-lib.so (std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >::assign(char const*)+32) (BuildId: e35bf127bac26d87c4586a7e22a1ca23e0a8e37d)
    02 pc 0000000000373400  /data/app/~~usflvTWmCZy5aZBFXYAWbw==/me.magnum.melonds.dev-25PisQX1TquKFQM3tiMHeA==/lib/arm64/libmelonDS-lib.so (MelonDSAndroid::setConfiguration(MelonDSAndroid::EmulatorConfiguration)+116) (BuildId: e35bf127bac26d87c4586a7e22a1ca23e0a8e37d)
    03 pc 000000000000d5b0  /data/app/~~usflvTWmCZy5aZBFXYAWbw==/me.magnum.melonds.dev-25PisQX1TquKFQM3tiMHeA==/lib/arm64/libmelonDS-android-frontend.so (Java_me_magnum_melonds_MelonEmulator_setupEmulator+260) (BuildId: 8a43c4a5b4f6d670c96f04b0c7ca13ff73987116)

Inspecting setConfiguration, it seems that the firmware paths are set regardless of whether internal firmware is used. The internal firmware shouldn't require these paths, and having them unset results in the crash detailed above.

To fix this, only set the paths when required. The check for internal firmware is sufficient for this purpose.

Test:
1. Run a clean install of melonDS, without configuring BIOS and without enabling custom BIOS & firmware (internal firmware only)
2. Initialise a known-affected game (Pokemon HG)
3. The game runs without any crash observed

When the user uses internal firmware without configuring external BIOS path,
a crash can be observed when running a game:

pid: 3681, tid: 3971, name: RxCachedThreadS  >>> me.magnum.melonds.dev <<<
    01 pc 00000000004d2ae8  /data/app/~~usflvTWmCZy5aZBFXYAWbw==/me.magnum.melonds.dev-25PisQX1TquKFQM3tiMHeA==/lib/arm64/libmelonDS-lib.so (std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >::assign(char const*)+32) (BuildId: e35bf127bac26d87c4586a7e22a1ca23e0a8e37d)
    02 pc 0000000000373400  /data/app/~~usflvTWmCZy5aZBFXYAWbw==/me.magnum.melonds.dev-25PisQX1TquKFQM3tiMHeA==/lib/arm64/libmelonDS-lib.so (MelonDSAndroid::setConfiguration(MelonDSAndroid::EmulatorConfiguration)+116) (BuildId: e35bf127bac26d87c4586a7e22a1ca23e0a8e37d)
    03 pc 000000000000d5b0  /data/app/~~usflvTWmCZy5aZBFXYAWbw==/me.magnum.melonds.dev-25PisQX1TquKFQM3tiMHeA==/lib/arm64/libmelonDS-android-frontend.so (Java_me_magnum_melonds_MelonEmulator_setupEmulator+260) (BuildId: 8a43c4a5b4f6d670c96f04b0c7ca13ff73987116)

Inspecting setConfiguration, it seems that the firmware paths are set regardless of
whether internal firmware is used. The external firmware shouldn't require these paths,
and having them unset results in the crash detailed above.

To fix this, only set the paths when required. The check for internal firmware is sufficient for this purpose.

Test:
	1. Run a clean install of melonDS, without configuring BIOS and without enabling custom BIOS & firmware (internal firmware only)
	2. Initialise a known-affected game (Pokemon HG)
	3. The game runs without any crash observed
@wordmage
Copy link
Author

This may resolve issues such as:
rafaelvcaetano/melonDS-android#1042
rafaelvcaetano/melonDS-android#1029

And it follows the discovery found in this ticket: rafaelvcaetano/melonDS-android#1003

@rafaelvcaetano
Copy link
Owner

Once again, thanks for your support in this project and in finding this issue! Your really is really valuable!

However, this time, your suggested fix is not the best one. The setConfiguration(EmulatorConfiguration) method is also used when we want to work with the DSi NAND in the DSiWare Manager. In that case, we want to always have the BIOS, firmware and NAND paths set, independently of the userInternalFirmwareAndBios flag. I think the best fix here is to make a null check and use empty strings as a fallback. It doesn't matter if we set invalid paths because the code should only read them if the userInternalFirmwareAndBios flag is set to true anyway and it always check if the files have been opened correctly.

So, in summary, my proposed alternative is to do the following:

Config::BIOS7Path = emulatorConfiguration.dsBios7Path ?: "";
Config::BIOS9Path = emulatorConfiguration.dsBios9Path ?: "";
Config::FirmwarePath = emulatorConfiguration.dsFirmwarePath ?: "";

Config::DSiBIOS7Path = emulatorConfiguration.dsiBios7Path ?: "";
Config::DSiBIOS9Path = emulatorConfiguration.dsiBios9Path ?: "";
Config::DSiFirmwarePath = emulatorConfiguration.dsiFirmwarePath ?: "";
Config::DSiNANDPath = emulatorConfiguration.dsiNandPath ?: "";

@wordmage
Copy link
Author

I'd say the alternative is more applicable if we don't want to do more intrusive rework to the frontend itself. I do still find it odd that the DS portion here isn't reliant on the check, though, since while the DSi portion is required for DSiWare Manager, the DS portion of the configuration is mostly unused there (or maybe my interpretation is more than likely wrong).

In any case, in my honest opinion, it's also appropriate that we go through these configurations that may not have a sane default, lest a user manages to wrench together a hack that crashes the frontend in a similar manner. I'd make another PR for this, mirroring the proposed alternative change, but I don't yet have the time for it (timing gets in the way..).

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