Minimal ED64 support #44
Conversation
networkfusion
force-pushed
the
ed64-basic
branch
from
136c97a to
f094168
Compare
Adds the basic needs to run ROMs on V series hardware.
Add fixme for why it might be broken.
Improve comments for ED64 saves.
networkfusion
added
enhancement
Specifically for ED64 V3
Workaround for need of F/W 3.06 and UNFLoader. Fixes ability to run ROM's
change function name to match main branch.
fix further function that was renamed.
How far should loading roms get with new builds for the ed64p? Menus look great |
Add all current features to switch.
At minimum, loads clays (oss rom) and SM64 and blues brothers 2000. Saves are probably broken now. Just tested on my ED64 V3. |
|
As of right now, the writebacks for ED64P are completely broken. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes enhance support for the ED64 flashcart within the project, introducing new source files that manage its functionality. Documentation updates clarify setup instructions for ED64 and ED64P ROMs, improving user guidance. The overall changes implement robust features for game state management, memory operations, and user interactions with the flashcart. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ED64
participant Memory
participant State
User->>ED64: Start Game
ED64->>Memory: Load ROM
Memory-->>ED64: ROM Data
ED64->>State: Initialize Game State
State-->>ED64: State Initialized
ED64-->>User: Game Ready
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Save corrections
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range, codebase verification and nitpick comments (7)
src/flashcart/ed64/ed64_ll.h (4)
16-16: Clarify the purpose of thePI_SAVE_ADDRmacro.Consider adding a comment to explain the purpose and usage of
PI_SAVE_ADDRfor better readability.
56-65: Consider adding more descriptive comments for save types.Adding comments to each save type in the
ed64_save_type_tenumeration can improve readability and maintainability.typedef enum { SAVE_TYPE_NONE = 0, // No save type SAVE_TYPE_SRAM = 1, // SRAM save type SAVE_TYPE_SRAM_128K = 2, // 128K SRAM save type SAVE_TYPE_EEPROM_4K = 3, // 4K EEPROM save type SAVE_TYPE_EEPROM_16K = 4, // 16K EEPROM save type SAVE_TYPE_FLASHRAM = 5, // FlashRAM save type SAVE_TYPE_CPAK = 8, // Controller Pak save type SAVE_TYPE_DD64_CART_PORT = 16, // 64DD cart port save type } ed64_save_type_t;
69-82: Consider adding comments to function declarations.Adding comments to each function declaration can improve readability and provide context for their usage.
/* Save functions */ /** * @brief Initialize SRAM. */ void pi_initialize_sram (void); /** * @brief Invalidate all data caches. */ void _data_cache_invalidate_all (void); /** * @brief Set the SRAM bank. * @param bank The SRAM bank to set. */ void ed64_ll_set_sram_bank (uint8_t bank); /** * @brief Get the current save type. * @return The current save type. */ ed64_save_type_t ed64_ll_get_save_type (void); /** * @brief Set the save type. * @param type The save type to set. */ void ed64_ll_set_save_type (ed64_save_type_t type); /** * @brief Get SRAM data. * @param buffer The buffer to store the data. * @param size The size of the buffer. */ void ed64_ll_get_sram (uint8_t *buffer, int size); /** * @brief Get EEPROM data. * @param buffer The buffer to store the data. * @param size The size of the buffer. */ void ed64_ll_get_eeprom (uint8_t *buffer, int size); /** * @brief Get FRAM data. * @param buffer The buffer to store the data. * @param size The size of the buffer. */ void ed64_ll_get_fram (uint8_t *buffer, int size); /** * @brief Set SRAM data. * @param buffer The buffer containing the data. * @param size The size of the buffer. */ void ed64_ll_set_sram (uint8_t *buffer, int size); /** * @brief Set EEPROM data. * @param buffer The buffer containing the data. * @param size The size of the buffer. */ void ed64_ll_set_eeprom (uint8_t *buffer, int size); /** * @brief Set FRAM data. * @param buffer The buffer containing the data. * @param size The size of the buffer. */ void ed64_ll_set_fram (uint8_t *buffer, int size);
7-8: Use a more descriptive include guard name.Consider using a more descriptive name for the include guard to avoid potential conflicts with other files.
#ifndef FLASHCART_ED64_LL_H__ #define FLASHCART_ED64_LL_H__</blockquote></details> <details> <summary>Makefile (1)</summary><blockquote> Line range hint `1-1`: **Consider adding a brief description of the project.** Adding a brief description at the top of the Makefile can provide context for new contributors. ```diff # Makefile for the N64FlashcartMenu project PROJECT_NAME = N64FlashcartMenusrc/flashcart/flashcart.c (2)
113-113: Remove commented-out code.The commented-out code for
CART_EDXshould be removed or addressed to avoid confusion.- //flashcart = ed64x_get_flashcart();
Line range hint
1-1:
Consider adding a brief description of the file.Adding a brief description at the top of the file can provide context for new contributors.
// flashcart.c - Implementation of flashcart operations for the N64FlashcartMenu project #include <stddef.h>
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- Makefile (1 hunks)
- README.md (2 hunks)
- src/flashcart/ed64/README.md (1 hunks)
- src/flashcart/ed64/ed64.c (1 hunks)
- src/flashcart/ed64/ed64.h (1 hunks)
- src/flashcart/ed64/ed64_ll.c (1 hunks)
- src/flashcart/ed64/ed64_ll.h (1 hunks)
- src/flashcart/ed64/ed64_state.c (1 hunks)
- src/flashcart/ed64/ed64_state.h (1 hunks)
- src/flashcart/flashcart.c (2 hunks)
Files skipped from review due to trivial changes (1)
- src/flashcart/ed64/README.md
Additional context used
Markdownlint
README.md
110-110: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
112-112: null
Bare URL used(MD034, no-bare-urls)
LanguageTool
README.md
[style] ~143-~143: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...development and debugging, the menu ROM is able to run in the Ares emulator (without most ...(BE_ABLE_TO)
Additional comments not posted (30)
src/flashcart/ed64/ed64.h (4)
1-5: File header comment block looks good.The comment block is well-structured and follows documentation standards.
7-12: Preprocessor directives look good.The include guards are correctly defined, and the included header file is appropriate.
19-19: Function prototype looks good.The function prototype follows standard conventions.
21-24: Closing comment block and include guard look good.The closing comment block and include guard are correctly placed.
src/flashcart/ed64/ed64_state.h (4)
1-5: File header comment block looks good.The comment block is well-structured and follows documentation standards.
7-8: Preprocessor directives look good.The include guards are correctly defined.
11-18: Structure definition looks good.The structure
ed64_pseudo_writeback_tis well-defined with appropriate member comments.
21-23: Function prototypes look good.The function prototypes follow standard conventions.
src/flashcart/ed64/ed64_state.c (4)
1-6: Header includes look good.The included headers are appropriate for the functionality provided.
7-13: Static variable definitions look good.The static variables are well-defined and initialized correctly.
15-20:ed64_state_initfunction looks good.The function follows standard memory management practices.
36-46:ed64_state_savefunction looks good.The function follows standard practices for saving state.
Makefile (1)
35-37: LGTM! Ensure new source files are correctly integrated.The new source files for ED64 flashcart support are correctly included.
src/flashcart/flashcart.c (2)
15-15: LGTM! Ensure the new header file is correctly integrated.The new header file
ed64.his correctly included.
117-117: LGTM! Ensure the new initialization logic is correctly integrated.The new initialization logic for
CART_EDis correctly integrated.src/flashcart/ed64/ed64_ll.c (10)
57-61: LGTM!The function correctly returns the current save type.
112-116: LGTM!The function correctly sets the SRAM bank.
147-152: LGTM!The function correctly initializes the PI and waits for DMA to complete.
154-157: LGTM!The function correctly waits for the PI DMA to complete.
169-177: LGTM!The function correctly performs a DMA read from SRAM.
179-189: LGTM!The function correctly performs a DMA write to SRAM.
232-239: LGTM!The function correctly retrieves EEPROM data.
242-255: LGTM!The function correctly retrieves FRAM data.
160-165: Verify the necessity of reconfiguring PI timings.Reconfiguring PI timings might be redundant if
libcartalready sets them to correct values.Verification successful
Verified: The reconfiguration of PI timings in
pi_initialize_sramis necessary.The search results indicate that PI timing configurations are not set by
libcartbut are present only insrc/flashcart/ed64/ed64_ll.c. Thus, the reconfiguration inpi_initialize_sramis required.
src/flashcart/ed64/ed64_ll.c: PI timing configurations found only here.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if PI timings are already configured by libcart. # Test: Search for PI timing configurations in libcart. Expect: PI timings configured. rg --type c -A 5 'io_write\(PI_BSD_DOM2_LAT_REG' libcart/Length of output: 108
Script:
#!/bin/bash # Description: Verify if PI timings are already configured in the codebase. # Test: Search for PI timing configurations in the entire codebase. Expect: PI timings configured. rg --type c -A 5 'io_write\(PI_BSD_DOM2_LAT_REG'Length of output: 734
191-201: Verify the necessity of reconfiguring PI timings.Reconfiguring PI timings might be redundant if
libcartalready sets them to correct values.Verification successful
The reconfiguration of PI timings in
ed64_ll_set_sdcard_timingis necessary.The search results indicate that PI timings are not configured elsewhere in the codebase, confirming that the configurations in
ed64_ll_set_sdcard_timingare essential.
src/flashcart/ed64/ed64_ll.csrc/flashcart/ed64/ed64_ll.hScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if PI timings are already configured by libcart. # Test: Search for PI timing configurations in libcart. Expect: PI timings configured. rg --type c -A 5 'io_write\(PI_BSD_DOM2_LAT_REG' libcart/Length of output: 108
Script:
#!/bin/bash # Description: Locate files related to libcart or files that might configure PI timings. # Find all files containing the term 'libcart' to identify potential directories or files. fd -I 'libcart' # Find all files that might configure PI timings by searching for 'PI_BSD_DOM2_LAT_REG'. rg --type c 'PI_BSD_DOM2_LAT_REG'Length of output: 268
README.md (3)
97-103: LGTM!The setup instructions for ED64 and ED64P ROMs are clear and provide necessary details.
110-115: LGTM!The instructions for building the project are clear and provide necessary details.
Tools
Markdownlint
110-110: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
112-112: null
Bare URL used(MD034, no-bare-urls)
148-154: LGTM!The instructions for using UNFLoader to upload ROMs via USB are clear and provide necessary details.
src/flashcart/ed64/ed64.c (2)
95-103: LGTM!The function correctly saves the state file and exits using
libCart.
274-304: LGTM!The function correctly sets the save type for the ED64.
| void ed64_state_load (ed64_pseudo_writeback_t *state) { | ||
| if (!file_exists(ed64_state_path)) { | ||
| ed64_state_save(&init); | ||
| } | ||
|
|
||
| mini_t *ini = mini_try_load(ed64_state_path); | ||
|
|
||
| state->is_expecting_save_writeback = mini_get_bool(ini, "ed64", "is_expecting_save_writeback", init.is_expecting_save_writeback); | ||
| state->is_fram_save_type = mini_get_bool(ini, "ed64", "is_fram_save_type", init.is_fram_save_type); | ||
| state->last_save_path = strdup(mini_get_string(ini, "ed64", "last_save_path", init.last_save_path)); | ||
|
|
||
| mini_free(ini); | ||
| } |
There was a problem hiding this comment.
Fix potential memory leak in ed64_state_load.
Ensure state->last_save_path is freed before assigning a new value to avoid memory leaks.
- state->last_save_path = strdup(mini_get_string(ini, "ed64", "last_save_path", init.last_save_path));
+ free(state->last_save_path);
+ state->last_save_path = strdup(mini_get_string(ini, "ed64", "last_save_path", init.last_save_path));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void ed64_state_load (ed64_pseudo_writeback_t *state) { | |
| if (!file_exists(ed64_state_path)) { | |
| ed64_state_save(&init); | |
| } | |
| mini_t *ini = mini_try_load(ed64_state_path); | |
| state->is_expecting_save_writeback = mini_get_bool(ini, "ed64", "is_expecting_save_writeback", init.is_expecting_save_writeback); | |
| state->is_fram_save_type = mini_get_bool(ini, "ed64", "is_fram_save_type", init.is_fram_save_type); | |
| state->last_save_path = strdup(mini_get_string(ini, "ed64", "last_save_path", init.last_save_path)); | |
| mini_free(ini); | |
| } | |
| void ed64_state_load (ed64_pseudo_writeback_t *state) { | |
| if (!file_exists(ed64_state_path)) { | |
| ed64_state_save(&init); | |
| } | |
| mini_t *ini = mini_try_load(ed64_state_path); | |
| state->is_expecting_save_writeback = mini_get_bool(ini, "ed64", "is_expecting_save_writeback", init.is_expecting_save_writeback); | |
| state->is_fram_save_type = mini_get_bool(ini, "ed64", "is_fram_save_type", init.is_fram_save_type); | |
| free(state->last_save_path); | |
| state->last_save_path = strdup(mini_get_string(ini, "ed64", "last_save_path", init.last_save_path)); | |
| mini_free(ini); | |
| } |
| // FIXME: redefined because its in a .c instead of a .h | ||
| #define PI_BASE_REG 0x04600000 | ||
|
|
There was a problem hiding this comment.
Avoid redefinitions of macros.
The PI_BASE_REG macro is redefined because it's in a .c file instead of a .h file. This should be addressed to avoid potential conflicts.
- #define PI_BASE_REG 0x04600000
+ // #define PI_BASE_REG 0x04600000Consider moving the definition to a common header file if it's used across multiple files.
<!-- This is an auto-generated comment by CodeRabbit -->
| // FIXME: redefined because its in a .c instead of a .h | ||
| #define PI_BSD_DOM1_LAT_REG (PI_BASE_REG+0x14) | ||
| #define PI_BSD_DOM1_PWD_REG (PI_BASE_REG+0x18) | ||
| #define PI_BSD_DOM1_PGS_REG (PI_BASE_REG+0x1C) | ||
| #define PI_BSD_DOM1_RLS_REG (PI_BASE_REG+0x20) | ||
| #define PI_BSD_DOM2_LAT_REG (PI_BASE_REG+0x24) | ||
| #define PI_BSD_DOM2_PWD_REG (PI_BASE_REG+0x28) | ||
| #define PI_BSD_DOM2_PGS_REG (PI_BASE_REG+0x2C) | ||
| #define PI_BSD_DOM2_RLS_REG (PI_BASE_REG+0x30) |
There was a problem hiding this comment.
Avoid redefinitions of macros.
The PI_BSD_DOM1_* and PI_BSD_DOM2_* macros are redefined because they are in a .c file instead of a .h file. This should be addressed to avoid potential conflicts.
- #define PI_BSD_DOM1_LAT_REG (PI_BASE_REG+0x14)
- #define PI_BSD_DOM1_PWD_REG (PI_BASE_REG+0x18)
- #define PI_BSD_DOM1_PGS_REG (PI_BASE_REG+0x1C)
- #define PI_BSD_DOM1_RLS_REG (PI_BASE_REG+0x20)
- #define PI_BSD_DOM2_LAT_REG (PI_BASE_REG+0x24)
- #define PI_BSD_DOM2_PWD_REG (PI_BASE_REG+0x28)
- #define PI_BSD_DOM2_PGS_REG (PI_BASE_REG+0x2C)
- #define PI_BSD_DOM2_RLS_REG (PI_BASE_REG+0x30)
+ // #define PI_BSD_DOM1_LAT_REG (PI_BASE_REG+0x14)
+ // #define PI_BSD_DOM1_PWD_REG (PI_BASE_REG+0x18)
+ // #define PI_BSD_DOM1_PGS_REG (PI_BASE_REG+0x1C)
+ // #define PI_BSD_DOM1_RLS_REG (PI_BASE_REG+0x20)
+ // #define PI_BSD_DOM2_LAT_REG (PI_BASE_REG+0x24)
+ // #define PI_BSD_DOM2_PWD_REG (PI_BASE_REG+0x28)
+ // #define PI_BSD_DOM2_PGS_REG (PI_BASE_REG+0x2C)
+ // #define PI_BSD_DOM2_RLS_REG (PI_BASE_REG+0x30)Consider moving the definition to a common header file if it's used across multiple files.
<!-- This is an auto-generated comment by CodeRabbit -->
| void ed64_ll_reg_write (uint32_t reg, uint32_t data); | ||
|
|
||
| void ed64_ll_reg_write (uint32_t reg, uint32_t data) { | ||
|
|
||
| *(volatile uint32_t *) (ED64_CONFIG_REGS_BASE); | ||
| *(volatile uint32_t *) (ED64_CONFIG_REGS_BASE + reg * 4) = data; | ||
| *(volatile uint32_t *) (ROM_ADDRESS); | ||
|
|
||
| } |
There was a problem hiding this comment.
Optimize redundant dummy reads and use libdragon functions.
The dummy reads are likely optimized away by the compiler. Use io_write/io_read functions provided by libdragon for consistency and reliability.
- *(volatile uint32_t *) (ED64_CONFIG_REGS_BASE);
- *(volatile uint32_t *) (ED64_CONFIG_REGS_BASE + reg * 4) = data;
- *(volatile uint32_t *) (ROM_ADDRESS);
+ io_write(ED64_CONFIG_REGS_BASE + reg * 4, data);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void ed64_ll_reg_write (uint32_t reg, uint32_t data); | |
| void ed64_ll_reg_write (uint32_t reg, uint32_t data) { | |
| *(volatile uint32_t *) (ED64_CONFIG_REGS_BASE); | |
| *(volatile uint32_t *) (ED64_CONFIG_REGS_BASE + reg * 4) = data; | |
| *(volatile uint32_t *) (ROM_ADDRESS); | |
| } | |
| void ed64_ll_reg_write (uint32_t reg, uint32_t data) { | |
| io_write(ED64_CONFIG_REGS_BASE + reg * 4, data); | |
| } |
| // FIXME Id like to use libdragon's equivelant for this | ||
| void _data_cache_invalidate_all (void) { | ||
| asm( | ||
| "li $8,0x80000000;" | ||
| "li $9,0x80000000;" | ||
| "addu $9,$9,0x1FF0;" | ||
| "cacheloop:;" | ||
| "cache 1,0($8);" | ||
| "cache 1,16($8);" | ||
| "cache 1,32($8);" | ||
| "cache 1,48($8);" | ||
| "cache 1,64($8);" | ||
| "cache 1,80($8);" | ||
| "cache 1,96($8);" | ||
| "addu $8,$8,112;" | ||
| "bne $8,$9,cacheloop;" | ||
| "cache 1,0($8);" | ||
| : // no outputs | ||
| : // no inputs | ||
| : "$8", "$9" // trashed registers | ||
| ); | ||
| } |
There was a problem hiding this comment.
Use libdragon's cache invalidation function.
The function uses inline assembly for cache invalidation. If libdragon provides an equivalent function, it should be used for consistency and maintainability.
-void _data_cache_invalidate_all (void) {
- asm(
- "li $8,0x80000000;"
- "li $9,0x80000000;"
- "addu $9,$9,0x1FF0;"
- "cacheloop:;"
- "cache 1,0($8);"
- "cache 1,16($8);"
- "cache 1,32($8);"
- "cache 1,48($8);"
- "cache 1,64($8);"
- "cache 1,80($8);"
- "cache 1,96($8);"
- "addu $8,$8,112;"
- "bne $8,$9,cacheloop;"
- "cache 1,0($8);"
- : // no outputs
- : // no inputs
- : "$8", "$9" // trashed registers
- );
-}
+// FIXME: Use libdragon's cache invalidation function if availableCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FIXME Id like to use libdragon's equivelant for this | |
| void _data_cache_invalidate_all (void) { | |
| asm( | |
| "li $8,0x80000000;" | |
| "li $9,0x80000000;" | |
| "addu $9,$9,0x1FF0;" | |
| "cacheloop:;" | |
| "cache 1,0($8);" | |
| "cache 1,16($8);" | |
| "cache 1,32($8);" | |
| "cache 1,48($8);" | |
| "cache 1,64($8);" | |
| "cache 1,80($8);" | |
| "cache 1,96($8);" | |
| "addu $8,$8,112;" | |
| "bne $8,$9,cacheloop;" | |
| "cache 1,0($8);" | |
| : // no outputs | |
| : // no inputs | |
| : "$8", "$9" // trashed registers | |
| ); | |
| } | |
| // FIXME: Use libdragon's cache invalidation function if available |
| static flashcart_err_t ed64_init (void) { | ||
|
|
||
| // TODO: partly already done, see https://github.com/DragonMinded/libdragon/blob/4ec469d26b6dc4e308caf3d5b86c2b340b708bbd/src/libcart/cart.c#L1064 | ||
|
|
||
| // FIXME: Update firmware if needed. | ||
| // FIXME: Enable RTC if available. | ||
|
|
||
| // older everdrives cannot save during gameplay so we need to the reset method. | ||
| // works by checking if a file exists. | ||
|
|
||
| // FIXME: should put the file in the OS dir (ED64_OS_DIRECTORY), but different if a clone (ED64P). | ||
| char *state_path = "sd:/menu/"ED64_STATE_FILE; | ||
| ed64_state_init(state_path); | ||
| ed64_state_load(¤t_state); | ||
|
|
||
| if (current_state.is_expecting_save_writeback == true) { | ||
|
|
||
| // make sure next boot does not trigger the check changing its state. | ||
| current_state.is_expecting_save_writeback = false; | ||
| ed64_state_save(¤t_state); | ||
|
|
||
| // Now save the content back to the SD card! | ||
| FIL fil; | ||
| UINT bw; | ||
| uint8_t cartsave_data[KiB(128)]; | ||
|
|
||
| // find the path to last save | ||
| if (file_exists(strip_fs_prefix(current_state.last_save_path))) { | ||
|
|
||
| int save_size = file_get_size(current_state.last_save_path); | ||
|
|
||
| if ((f_open(&fil, strip_fs_prefix(current_state.last_save_path), FA_CREATE_ALWAYS | FA_WRITE)) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| // everdrive doesn't care about the save type other than flash sram and eeprom | ||
| // so minus flashram we can just check the size | ||
| if (current_state.is_fram_save_type == true) { // flashram is bugged atm | ||
| ed64_ll_get_fram(cartsave_data, save_size); | ||
| // deletes flag | ||
| current_state.is_fram_save_type = false; | ||
| ed64_state_save(¤t_state); | ||
| } | ||
| else if (save_size > KiB(2)) { // sram | ||
| ed64_ll_get_sram(cartsave_data, save_size); | ||
| } | ||
| else { // eeprom | ||
| ed64_ll_get_eeprom(cartsave_data, save_size); | ||
| } | ||
|
|
||
| if (f_write(&fil, cartsave_data, save_size, &bw) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| if (f_close(&fil) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
| } | ||
| else { | ||
| current_state.is_expecting_save_writeback = false; | ||
| current_state.is_fram_save_type = false; | ||
| current_state.last_save_path = ""; | ||
| ed64_state_save(¤t_state); | ||
| } | ||
| } | ||
| return FLASHCART_OK; | ||
| } |
There was a problem hiding this comment.
Address TODO and FIXME comments.
The function contains several TODO and FIXME comments indicating incomplete implementation.
Do you want me to address these TODO and FIXME comments or open a GitHub issue to track these tasks?
| static bool ed64_has_feature (flashcart_features_t feature) { | ||
| switch (feature) { | ||
| case FLASHCART_FEATURE_64DD: | ||
| // FIXME: this is not valid for the V3, as does contain them: | ||
| case FLASHCART_FEATURE_RTC: | ||
| case FLASHCART_FEATURE_USB: | ||
| return false; | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Address FIXME comment.
The function contains a FIXME comment indicating invalid feature checks for V3.
Do you want me to address this FIXME comment or open a GitHub issue to track this task?
| static flashcart_err_t ed64_load_rom (char *rom_path, flashcart_progress_callback_t *progress) { | ||
|
|
||
| FIL fil; | ||
| UINT br; | ||
|
|
||
| if (f_open(&fil, strip_fs_prefix(rom_path), FA_READ) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| fatfs_fix_file_size(&fil); | ||
|
|
||
| size_t rom_size = f_size(&fil); | ||
|
|
||
| // FIXME: if the cart is not V3 or X5 or X7, we need probably need to - 128KiB for save compatibility. | ||
| // Or somehow warn that certain ROM's will have corruption due to the address space being used for saves. | ||
| // Conker's Bad Fur Day doesn't have this issue because eeprom data is at a fixed address in pif ram. | ||
| if (rom_size > MiB(64)) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| // FIXME: is this actually needed? | ||
| /* | ||
| if (rom_size == MiB(64)) { | ||
| ed64_save_type_t type = ed64_ll_get_save_type(); | ||
| switch (type) { | ||
| case SAVE_TYPE_SRAM: | ||
| rom_size -= KiB(32); | ||
| break; | ||
| case SAVE_TYPE_SRAM_128K: | ||
| case SAVE_TYPE_FLASHRAM: | ||
| rom_size -= KiB(128); | ||
| break; | ||
| case SAVE_TYPE_DD64_CART_PORT: | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| */ | ||
|
|
||
| size_t sdram_size = rom_size; | ||
|
|
||
| size_t chunk_size = MiB(1); | ||
| for (int offset = 0; offset < sdram_size; offset += chunk_size) | ||
| { | ||
| size_t block_size = MIN(sdram_size - offset, chunk_size); | ||
| if (f_read(&fil, (void *)(ROM_ADDRESS + offset), block_size, &br) != FR_OK) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
| if (progress) { | ||
| progress(f_tell(&fil) / (float)(f_size(&fil))); | ||
| } | ||
| } | ||
|
|
||
| if (f_close(&fil) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| return FLASHCART_OK; |
There was a problem hiding this comment.
Address FIXME comments.
The function contains several FIXME comments indicating incomplete implementation.
Do you want me to address these FIXME comments or open a GitHub issue to track these tasks?
| static flashcart_err_t ed64_load_file (char *file_path, uint32_t rom_offset, uint32_t file_offset) | ||
| { | ||
| FIL fil; | ||
| UINT br; | ||
|
|
||
| if (f_open(&fil, strip_fs_prefix(file_path), FA_READ) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| fatfs_fix_file_size(&fil); | ||
|
|
||
| size_t file_size = f_size(&fil) - file_offset; | ||
|
|
||
| // FIXME: if the cart is not V3 or X5 or X7, we need probably need to - 128KiB for save compatibility. | ||
| // Or somehow warn that certain ROM's will have corruption due to the address space being used for saves. | ||
|
|
||
| if (file_size > (MiB(64) - rom_offset)) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_ARGS; | ||
| } | ||
|
|
||
| if (f_lseek(&fil, file_offset) != FR_OK) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| if (f_read(&fil, (void *)(ROM_ADDRESS + rom_offset), file_size, &br) != FR_OK) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
| if (br != file_size) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| if (f_close(&fil) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| return FLASHCART_OK; |
There was a problem hiding this comment.
Address FIXME comment.
The function contains a FIXME comment indicating incomplete implementation.
Do you want me to address this FIXME comment or open a GitHub issue to track this task?
| static flashcart_err_t ed64_load_save (char *save_path) { | ||
|
|
||
| FIL fil; | ||
| UINT br; | ||
|
|
||
| if (f_open(&fil, strip_fs_prefix(save_path), FA_READ) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| size_t save_size = file_get_size(save_path); | ||
| uint8_t cartsave_data[save_size]; | ||
|
|
||
| if (f_read(&fil, cartsave_data, save_size, &br) != FR_OK) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| if (br != save_size) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| current_state.is_fram_save_type = false; | ||
|
|
||
| ed64_save_type_t type = ed64_ll_get_save_type(); | ||
| switch (type) { | ||
| case SAVE_TYPE_EEPROM_4K: | ||
| case SAVE_TYPE_EEPROM_16K: | ||
| ed64_ll_set_eeprom(cartsave_data, save_size); | ||
| break; | ||
| case SAVE_TYPE_SRAM: | ||
| case SAVE_TYPE_SRAM_128K: | ||
| ed64_ll_set_sram(cartsave_data, save_size); | ||
| break; | ||
| case SAVE_TYPE_FLASHRAM: | ||
| ed64_ll_set_fram(cartsave_data, save_size); | ||
| // a cold and warm boot has no way of seeing save types and most types can be determined by size | ||
| // this tells the cart to use flash instead of sram 128 since they are the same size | ||
| current_state.is_fram_save_type = true; | ||
| ed64_state_save(¤t_state); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
|
|
||
|
|
||
| current_state.last_save_path = save_path; | ||
| current_state.is_expecting_save_writeback = true; | ||
| ed64_state_save(¤t_state); | ||
|
|
There was a problem hiding this comment.
Avoid variable length arrays.
The variable cartsave_data is declared with a variable length, which is not standard C. Use dynamic memory allocation instead.
- uint8_t cartsave_data[save_size];
+ uint8_t *cartsave_data = malloc(save_size);
+ if (cartsave_data == NULL) {
+ f_close(&fil);
+ return FLASHCART_ERR_LOAD;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static flashcart_err_t ed64_load_save (char *save_path) { | |
| FIL fil; | |
| UINT br; | |
| if (f_open(&fil, strip_fs_prefix(save_path), FA_READ) != FR_OK) { | |
| return FLASHCART_ERR_LOAD; | |
| } | |
| size_t save_size = file_get_size(save_path); | |
| uint8_t cartsave_data[save_size]; | |
| if (f_read(&fil, cartsave_data, save_size, &br) != FR_OK) { | |
| f_close(&fil); | |
| return FLASHCART_ERR_LOAD; | |
| } | |
| if (br != save_size) { | |
| return FLASHCART_ERR_LOAD; | |
| } | |
| current_state.is_fram_save_type = false; | |
| ed64_save_type_t type = ed64_ll_get_save_type(); | |
| switch (type) { | |
| case SAVE_TYPE_EEPROM_4K: | |
| case SAVE_TYPE_EEPROM_16K: | |
| ed64_ll_set_eeprom(cartsave_data, save_size); | |
| break; | |
| case SAVE_TYPE_SRAM: | |
| case SAVE_TYPE_SRAM_128K: | |
| ed64_ll_set_sram(cartsave_data, save_size); | |
| break; | |
| case SAVE_TYPE_FLASHRAM: | |
| ed64_ll_set_fram(cartsave_data, save_size); | |
| // a cold and warm boot has no way of seeing save types and most types can be determined by size | |
| // this tells the cart to use flash instead of sram 128 since they are the same size | |
| current_state.is_fram_save_type = true; | |
| ed64_state_save(¤t_state); | |
| break; | |
| default: | |
| break; | |
| } | |
| current_state.last_save_path = save_path; | |
| current_state.is_expecting_save_writeback = true; | |
| ed64_state_save(¤t_state); | |
| static flashcart_err_t ed64_load_save (char *save_path) { | |
| FIL fil; | |
| UINT br; | |
| if (f_open(&fil, strip_fs_prefix(save_path), FA_READ) != FR_OK) { | |
| return FLASHCART_ERR_LOAD; | |
| } | |
| size_t save_size = file_get_size(save_path); | |
| uint8_t *cartsave_data = malloc(save_size); | |
| if (cartsave_data == NULL) { | |
| f_close(&fil); | |
| return FLASHCART_ERR_LOAD; | |
| } | |
| if (f_read(&fil, cartsave_data, save_size, &br) != FR_OK) { | |
| f_close(&fil); | |
| return FLASHCART_ERR_LOAD; | |
| } | |
| if (br != save_size) { | |
| return FLASHCART_ERR_LOAD; | |
| } | |
| current_state.is_fram_save_type = false; | |
| ed64_save_type_t type = ed64_ll_get_save_type(); | |
| switch (type) { | |
| case SAVE_TYPE_EEPROM_4K: | |
| case SAVE_TYPE_EEPROM_16K: | |
| ed64_ll_set_eeprom(cartsave_data, save_size); | |
| break; | |
| case SAVE_TYPE_SRAM: | |
| case SAVE_TYPE_SRAM_128K: | |
| ed64_ll_set_sram(cartsave_data, save_size); | |
| break; | |
| case SAVE_TYPE_FLASHRAM: | |
| ed64_ll_set_fram(cartsave_data, save_size); | |
| // a cold and warm boot has no way of seeing save types and most types can be determined by size | |
| // this tells the cart to use flash instead of sram 128 since they are the same size | |
| current_state.is_fram_save_type = true; | |
| ed64_state_save(¤t_state); | |
| break; | |
| default: | |
| break; | |
| } | |
| current_state.last_save_path = save_path; | |
| current_state.is_expecting_save_writeback = true; | |
| ed64_state_save(¤t_state); |
Improve commenting.
networkfusion
force-pushed
the
develop
branch
from
6990621 to
fb0a04e
Compare
Description
Adds the basic needs to run ROMs on V series hardware (specifically the ED64P).
NOTE: this PR will not implement the ability to load the latest firmware and relies upon
libCartfor basic needs.Improvements will be made in further PR's.
Motivation and Context
#15 but without all of the fat (needed for the future).
How Has This Been Tested?
Screenshots
Types of changes
Checklist:
Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
Summary by CodeRabbit
New Features
Documentation
Bug Fixes