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

Sega/Mega CD Add Configurable Backup Ram Cart size #327

Merged
merged 1 commit into from
Apr 2, 2023
Merged

Sega/Mega CD Add Configurable Backup Ram Cart size #327

merged 1 commit into from
Apr 2, 2023

Conversation

ichee
Copy link

@ichee ichee commented Mar 30, 2023

Adds a user configurable option for setting the backup ram cart size.

Range is from 128Kbit thru 4Mbit, with 4Mbit remaining the default same as before. Also adds an option to disable the backup ram cart.

Changing backup cart sizes will reformat the cart when core is restarted. Carts will no longer be reformatted when changing sizes.

Also, addressed an issue with internal backup ram being overwritten when changing from 'Per-BIOS' to 'Per-Game' if the RetroArch menu is closed after changing between the two settings, but before closing the core.

Addresses upstream issue ekeeke#267.

Sega/Mega CD Configurable Backup Ram Cart size
@LibretroAdmin LibretroAdmin merged commit 3c7a0c4 into libretro:master Apr 2, 2023
@ekeeke
Copy link

ekeeke commented Apr 2, 2023

There are several issues with this commit:

  1. it breaks emulation core portability as it does not compile outside libretro (requires cart_size data which is only declared in libretro.c). The correct approach would be to add a dedicated field in config structure (ramcart_id for example, which is less ambiguous than cart_size) for all ports (and force default value in all ports besides libretro) instead of having extern definitions in random core modules that point to libretro specific data

  2. the value '0' for RAM cart id does not indicate 'no RAM cart' but 8KB RAM cart (or 64Kbits if you prefer). The correct value to return when no RAM cart is inserted is 0xff (actually, anything from 0x07 or above will be considered as invalid size by BIOS)

  3. it is not a good idea to rename the RAM cart files according to their size as it will breaks backward compatibility with existing save files (created prior this commit). A better solution would be to keep default name for the default size (512KB/4Mbits) and only use specific names for smaller RAM size, as they will rarely be used (most common use case is for people wanting to import saves from flashcarts).

As a general note, I would suggest that for commits that modify core sourcecode (files under /core directory), it would be better to first submit it upstream for review of potential flaws or impact outside libretro . Thanks.

@ichee
Copy link
Author

ichee commented Apr 3, 2023

@ekeeke

To address some of your concerns:

  1. That was the best way I could come up with to make the sizes configurable with minimal issue, and the best I could do with
    limited experience with the core. I'm aware it breaks standalone builds, but I was unable to address what needed to be
    changed for them to work, as well as I was unable to successfully compile any GC/Wii builds to even attempt to test. That is
    why I only committed these changes here, since only libretro builds would have mattered here.

  2. Setting cart ram id to 0 does disable the cart ram as far as the BIOS is concerned, as well as no additional carts are created, as
    can be seen below.

scd_cart_off

  1. I understand your concern about the 4Mbit cart size, but it would look a little awkward to have each size cart name
    appended with a size, only to leave the 4Mbit cart without one. Cart names can be easily renamed even on the GC/Wii builds
    so it would only, at most, pose a one time inconvenience.

And, thank you for showing interest and expressing your concerns.

Edit:

I stand corrected on 0 disabling the ram cart.

The BIOS shows it is not present, but any game that can directly access it will complain and refuse to load.

Fix incoming.

@ekeeke
Copy link

ekeeke commented Apr 3, 2023

Hi,

That was the best way I could come up with to make the sizes configurable with minimal issue, and the best I could do with limited experience with the core. I'm aware it breaks standalone builds, but I was unable to address what needed to be changed for them to work, as well as I was unable to successfully compile any GC/Wii builds to even attempt to test. That is
why I only committed these changes here, since only libretro builds would have mattered here.

The thing is, when I will add same feature upstream (there is a pending feature request for this, it was postponed to deal with formatting/conversion issues), I will do it like I suggested so there will be conflict when merging upstream changes back to libretro (which occurs regularely). Hence why I prefer that changes that have impact on mylti-platform emulation code (and could eventually have impact or benefit to other ports) to be at least submitted for review on my side. All I suggest (since you are comitting some fixes anyway) is to remove the cart_size variable and instead add a ramcart_size or ramcart_id field in existing config structure (only on libretro side if you want) so that it fits better with actual code design and future changes.

@ekeeke
Copy link

ekeeke commented Apr 3, 2023

I understand your concern about the 4Mbit cart size, but it would look a little awkward to have each size cart name
appended with a size, only to leave the 4Mbit cart without one. Cart names can be easily renamed even on the GC/Wii builds
so it would only, at most, pose a one time inconvenience.

My concern about this was more about libretro/retroarch users, as I don't think there will be any warning that Mega CD RAM cart file names need to be changed so most users (who will have no use for that new option or know it was even added) will think their existing saves are lost. I agree it is a one-time rename but it should, in my opinion, be transparent to users who, for the majority, won't change that option.

@ekeeke
Copy link

ekeeke commented Apr 3, 2023

I stand corrected on 0 disabling the ram cart.

The BIOS shows it is not present, but any game that can directly access it will complain and refuse to load.

That I didn't knew, I thought 0 was still a valid size for RAM cart (the minimal one). Thanks for the notice.

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.

3 participants