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

Make 4K EEPROM default save type; support carts with no EEPROM #878

Merged
merged 8 commits into from
Jul 7, 2021
Merged

Make 4K EEPROM default save type; support carts with no EEPROM #878

merged 8 commits into from
Jul 7, 2021

Conversation

meeq
Copy link
Contributor

@meeq meeq commented Jun 23, 2021

Proper fix for workaround in DragonMinded/libdragon#125 (review)

@rasky @sp1187 @loganmc10

The Joybus response values have been confirmed on real hardware using the following test ROM which dumps the EEPROM status input and output data: eepromtest.zip

I was not able to run this test ROM on Mupen64Plus; all of my libdragon example ROMs get stuck at "Mupen64Plus Started...":
mupen

I'd appreciate some assistance on getting this to run. I was able to test this ROM successfully with cen64.

@loganmc10
Copy link
Member

I'll test this in a bit, I think it would be cleaner to just leave the type as a uint16_t and do

rx_buf[0] = (uint8_t)(cart->eeprom.type >> 0);
rx_buf[1] = (uint8_t)(cart->eeprom.type >> 8);
rx_buf[2] = (uint8_t)(cart->eeprom.type >> 0);

(Just repeat the first byte, which is 0 when present and 0xff when absent)

@loganmc10
Copy link
Member

loganmc10 commented Jun 24, 2021

The other thing I'll say: the way this is written, it defaults to "no eeprom". This means that any ROM not in the INI won't support eeprom saves. This might be a problem for ROM hacks/new ROMs (like a new version of Smash Remix or something).

I think it would be better to retain the default of 4k eeprom. I don't think we've run across a game that failed because it said it had eeprom when it shouldn't.

EDIT: Or maybe we just set it to "no eeprom" if the INI says it uses another save type like flash or sram

@loganmc10
Copy link
Member

loganmc10 commented Jun 24, 2021

Here is the result of the test ROM (after I applied this PR):

controller_test-000

I think that looks right, but as I said, I think we should still default to 4k eeprom. It's a fairly safe assumption that if a commercial game is probing that address, it expects the eeprom to be there.

@meeq
Copy link
Contributor Author

meeq commented Jun 24, 2021

I have an updated EEPROM test ROM that exercises a variety of EEPROM probe command formats: eepromtest.zip

This branch has been updated to reflect a more-accurate understanding of how the real hardware handles EEPROM checking: If there is no EEPROM, the recv buffer will not be modified.

@meeq
Copy link
Contributor Author

meeq commented Jun 24, 2021

As for the "user-experience question" of whether it's actually a good idea to make this change: it's subjective.

According to this list of save types, 4K EEPROM is the second-most popular save format for N64 titles. There is a lot of sense in the argument that a user-friendly emulator should assume 4K EEPROM if no other save type is specified, since it (almost always) won't hurt and has a decent chance of helping.

My only argument against is that homebrew/mods that want other save types (such as Smash Remix which uses SRAM) will still be a broken experience that requires the user to select the right save type. So it will "just work" some of the time, but silently fail other times.

@loganmc10
Copy link
Member

loganmc10 commented Jun 24, 2021

-    JDT_EEPROM_4K        = 0x8000,  /* 4k EEPROM */
-    JDT_EEPROM_16K       = 0xc000,  /* 16k EEPROM */
+    JDT_EEPROM_4K        = 0x0080,  /* 4k EEPROM */
+   JDT_EEPROM_16K       = 0x00c0,  /* 16k EEPROM */

Did you mean to flip the order of these bytes? I don't think that's right.

Copy link
Contributor

@Jj0YzL5nvJ Jj0YzL5nvJ left a comment

Choose a reason for hiding this comment

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

DK64 demos look better (less sped up), but now doesn't initialize the save file.

@meeq
Copy link
Contributor Author

meeq commented Jun 24, 2021

Sorry, I definitely messed up by flipping the bytes of the EEPROM identifiers. Let me try to figure out how to get my libdragon ROMs running on this branch so I can run the test ROM and verify these changes myself.

@meeq
Copy link
Contributor Author

meeq commented Jun 24, 2021

I have attached the libdragon test suite, which includes EEPROM Joybus command tests: testrom_emu.zip

Line numbers for test_eeprom failures are mapped to this file: libdragon/tests/test_eeprom.c

There are several other failures unrelated to EEPROM that are out of scope for this PR.

Without the changes in this PR, it is not possible to exercise the "EEPROM not detected" code-path since Mupen64Plus will always have a 4K EEPROM (unless 16K EEPROM is specified)

@meeq meeq changed the title Fix EEPROM detection Joybus status response Make 4K EEPROM default save type; support carts with no EEPROM Jun 24, 2021
@loganmc10
Copy link
Member

loganmc10 commented Jun 24, 2021

Here are the results with the PR applied (default settings):

Core: IS64: **** Starting test: test_exception
Core: IS64: ASSERTION FAILED (test_exception.c:258):
Core: IS64: breakpoint_occured != 1 (0 != 1)
Core: IS64: Breakpoint exception was not triggered
Core: IS64: **** Starting test: test_timer_ticks
Core: IS64: ASSERTION FAILED (test_timer.c:195):
Core: IS64: t5-t0 < 1000
Core: IS64: invalid timer_ticks [start=ffffffe9]: 160000000b - 17000002fd
Core: IS64: **** Starting test: test_timer_oneshot
Core: IS64: **** Starting test: test_timer_slow_callback
Core: IS64: **** Starting test: test_timer_continuous
Core: IS64: **** Starting test: test_timer_mixed
Core: IS64: **** Starting test: test_timer_disabled_start
Core: IS64: **** Starting test: test_timer_disabled_restart
Core: IS64: **** Starting test: test_irq_reentrancy
Core: IS64: **** Starting test: test_eeprom
Core: IS64: 4K EEPROM detected.
Core: IS64: 4K EEPROM detected.
Core: IS64: 4K EEPROM detected.
Core: IS64: 4K EEPROM detected.
Core: IS64: **** Starting test: test_dfs_read
Core: IS64: **** Starting test: test_dfs_rom_addr
Core: IS64: **** Starting test: test_eepromfs
Core: IS64: **** Starting test: test_cache_invalidate
Core: IS64: ASSERTION FAILED (test_cache.c:34):
Core: IS64: [----deadbe] != [----aaaaaa]
Core: IS64:      ^^              ^^  idx: 0
Core: IS64: unexpected data in not-invalidated cached cacheline 0 (0/0)
Core: IS64: **** Starting test: test_debug_sdfs
Core: IS64: TEST SKIPPED:
Core: IS64: no SD support
Core: IS64: SKIP
Core: IS64: 
Core: IS64: Testsuite finished in 00:02
Core: IS64: Passed: 11 out of 16 (1 skipped)

When I add an entry for the ROM to the INI and set the SaveType to None:

Core: IS64: **** Starting test: test_exception
Core: IS64: ASSERTION FAILED (test_exception.c:258):
Core: IS64: breakpoint_occured != 1 (0 != 1)
Core: IS64: Breakpoint exception was not triggered
Core: IS64: **** Starting test: test_timer_ticks
Core: IS64: ASSERTION FAILED (test_timer.c:195):
Core: IS64: t5-t0 < 1000
Core: IS64: invalid timer_ticks [start=ffffffe9]: 160000000b - 17000002fd
Core: IS64: **** Starting test: test_timer_oneshot
Core: IS64: **** Starting test: test_timer_slow_callback
Core: IS64: **** Starting test: test_timer_continuous
Core: IS64: **** Starting test: test_timer_mixed
Core: IS64: **** Starting test: test_timer_disabled_start
Core: IS64: **** Starting test: test_timer_disabled_restart
Core: IS64: **** Starting test: test_irq_reentrancy
Core: IS64: **** Starting test: test_eeprom
Core: IS64: EEPROM not detected.
Core: IS64: EEPROM not detected.
Core: IS64: EEPROM not detected.
Core: IS64: EEPROM not detected.
Core: IS64: **** Starting test: test_dfs_read
Core: IS64: **** Starting test: test_dfs_rom_addr
Core: IS64: **** Starting test: test_eepromfs
Core: IS64: TEST SKIPPED:
Core: IS64: EEPROM not found; skipping eepfs tests
Core: IS64: SKIP
Core: IS64: **** Starting test: test_cache_invalidate
Core: IS64: ASSERTION FAILED (test_cache.c:34):
Core: IS64: [----deadbe] != [----aaaaaa]
Core: IS64:      ^^              ^^  idx: 0
Core: IS64: unexpected data in not-invalidated cached cacheline 0 (0/0)
Core: IS64: **** Starting test: test_debug_sdfs
Core: IS64: TEST SKIPPED:
Core: IS64: no SD support
Core: IS64: SKIP
Core: IS64: 
Core: IS64: Testsuite finished in 00:02
Core: IS64: Passed: 10 out of 16 (2 skipped)

The results (for eeprom) look good to me, PR looks good to me in the state it's in currently.

SAVETYPE_SRAM,
SAVETYPE_FLASH_RAM,
SAVETYPE_CONTROLLER_PACK,
SAVETYPE_CONTROLLER_PAK,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -224,11 +224,11 @@ typedef enum

typedef enum
{
SAVETYPE_EEPROM_4KB = 0,
SAVETYPE_EEPROM_16KB,
SAVETYPE_EEPROM_4K = 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

KB is not accurate; the EEPROM sizes are in kilobits: http://n64devkit.square7.ch/pro-man/pro26/26-06.htm

There are two types of EEPROM according to their capacities. One is the 4k-EEPROM. The capacity of this type is 4 kilobits (64 words * 64 bits = 512 bytes). The other is the 16k-EEPROM. The capacity of this type is 16 kilobits (256 words * 64 bits = 2048 bytes).

rx_buf[0] = (uint8_t)(cart->eeprom.type >> 0);
rx_buf[1] = (uint8_t)(cart->eeprom.type >> 8);
rx_buf[2] = 0x00;
if (cart->eeprom.type) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This behavior has been tested on real hardware; if there is no EEPROM, the receive buffer is untouched.

@loganmc10
Copy link
Member

This LGTM, we've got a lot of pending PRs, if we don't start merging things we'll end up with conflicts soon enough

Copy link
Contributor

@Jj0YzL5nvJ Jj0YzL5nvJ left a comment

Choose a reason for hiding this comment

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

I have been using this PR as a patch and have not experienced any noticeable issues.

@Narann Narann merged commit 88b4301 into mupen64plus:master Jul 7, 2021
@Narann
Copy link
Member

Narann commented Jul 7, 2021

Here we go.

@turntable97
Copy link

I just can't manage to save on a DK64 Widescreen hacked ROM.
How is this possible? Do I need to make a specific setting in RetroArch to make it work?
I'd appreciate any help I can get!

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.

None yet

6 participants