Repository navigation
Conversation
📝 WalkthroughWalkthroughThis PR adds a complete EverDrive-64 V-series flashcart driver to replace the prior implementation. It introduces low-level hardware primitives for save access, INI-based persistent state tracking, a main driver orchestrating ROM and save operations, and updates the build system and integration points. ChangesEverDrive-64 Driver Implementation
Sequence Diagram(s)sequenceDiagram
participant Menu as N64Menu
participant Driver as ed64_driver
participant State as ed64_state
participant LL as ed64_ll
participant SD as SD Card
Menu->>Driver: ed64_init()
Driver->>State: ed64_state_load()
State->>SD: read ed64_state.ini
State-->>Driver: load state with writeback flag
alt is_expecting_save_writeback
Driver->>LL: ed64_ll_get_sram/eeprom/fram()
LL-->>Driver: cart save buffer
Driver->>SD: write buffer to last_save_path
Driver->>State: clear flag and persist
end
Menu->>Driver: load_save(file)
Driver->>SD: read save file
Driver->>LL: ed64_ll_set_sram/eeprom/fram()
Driver->>State: store save_path and mark next boot
Driver->>State: ed64_state_save()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/flashcart/ed64/ed64_ll.c`:
- Around line 150-162: ed64_ll_get_sram starts a PI DMA into buffer via
pi_dma_from_sram but does not invalidate the CPU data cache for buffer, so stale
cached lines can be read later; add a destination cache invalidation (matching
the convention used by pi_dma_read_data and the FRAM path) immediately before
calling pi_dma_from_sram in ed64_ll_get_sram (e.g., call the platform
cache-invalidate helper such as osInvalDCache or the project's equivalent on
buffer/size), keeping the rest of the sequence (pi_initialize_sram, dma_wait,
pi_initialize, wait_ms) intact and ensuring ed64_ll_get_sram now invalidates
buffer before the DMA read.
In `@src/flashcart/ed64/ed64.c`:
- Around line 86-117: The early-return error paths in this block leave
current_state.is_expecting_save_writeback set, causing persistent boot failures;
before each return FLASHCART_ERR_LOAD in the save writeback flow (after
save_size validation, after ed64_apply_save_type failure, and on any file
write/close failure) clear current_state.is_expecting_save_writeback = false and
persist that change (call the existing state persistence routine, e.g.
ed64_persist_state() or the equivalent function used elsewhere) so the cleared
flag is saved to disk; do this immediately prior to each return to ensure the
flag is not left set across reboots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 796e74ee-5779-42b8-943d-efd8033d8815
📒 Files selected for processing (8)
Makefilesrc/flashcart/ed64/ed64.csrc/flashcart/ed64/ed64.hsrc/flashcart/ed64/ed64_ll.csrc/flashcart/ed64/ed64_ll.hsrc/flashcart/ed64/ed64_state.csrc/flashcart/ed64/ed64_state.hsrc/flashcart/flashcart.c
| void ed64_ll_get_sram (uint8_t *buffer, int size) { | ||
| int offset = (size == (int) KiB(32)) ? 0 : SRAM_128K_OFFSET; | ||
|
|
||
| pi_initialize_sram(); | ||
| dma_wait(); | ||
| pi_initialize(); | ||
| wait_ms(250); | ||
|
|
||
| pi_dma_from_sram(buffer, offset, size); | ||
| dma_wait(); | ||
|
|
||
| ed64_ll_set_sdcard_timing(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare cache handling between read and write SRAM paths and any reference impl
rg -nP 'data_cache_hit_writeback_invalidate|pi_dma_from_sram|pi_dma_to_sram|PI_WR_LEN|PI_RD_LEN' -C2
# Look for any prior ed64 implementation for reference patterns
fd -i 'ed64' -e cRepository: Polprzewodnikowy/N64FlashcartMenu
Length of output: 4121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant sections of ED64 low-level implementation
sed -n '1,260p' src/flashcart/ed64/ed64_ll.c | nl -ba | sed -n '120,240p'
# Find and show where ed64_writeback_save calls ed64_ll_get_sram / get_fram
rg -n "ed64_writeback_save|get_sram|get_fram" src/flashcart/ed64 -SRepository: Polprzewodnikowy/N64FlashcartMenu
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant portions of ed64_ll.c without relying on nl
sed -n '1,260p' src/flashcart/ed64/ed64_ll.c | sed -n '120,260p'
echo "---- caller search (ed64_writeback_save) ----"
rg -n "ed64_writeback_save" src/flashcart/ed64 -S
echo "---- show ed64_writeback_save function body ----"
# Print the function block by grabbing lines around the match and a bit more context
rg -n "ed64_writeback_save" src/flashcart/ed64 -n -S --context 15
echo "---- find where ed64_ll_get_sram is called ----"
rg -n "ed64_ll_get_sram" src/flashcart/ed64 -S --context 5
echo "---- find where ed64_ll_get_fram is called ----"
rg -n "ed64_ll_get_fram" src/flashcart/ed64 -S --context 5Repository: Polprzewodnikowy/N64FlashcartMenu
Length of output: 11309
Add destination cache invalidation for ed64_ll_get_sram PI DMA reads
ed64_ll_get_sram() starts a PI DMA that writes into buffer (pi_dma_from_sram(buffer, ...)) but never invalidates the destination cache lines for buffer. When ed64.c loads saves from SRAM (the !is_fram_save_type / save_size > KiB(2) path), it calls ed64_ll_get_sram(cartsave_data, save_size) directly, so stale cached lines in cartsave_data can be used and corrupt what gets written to SD. The FRAM path already invalidates buffer before delegating to ed64_ll_get_sram, and pi_dma_read_data() follows the same “invalidate destination before DMA read” convention.
🛡️ Proposed fix
void ed64_ll_get_sram (uint8_t *buffer, int size) {
int offset = (size == (int) KiB(32)) ? 0 : SRAM_128K_OFFSET;
pi_initialize_sram();
dma_wait();
pi_initialize();
wait_ms(250);
+ data_cache_hit_writeback_invalidate(buffer, size);
pi_dma_from_sram(buffer, offset, size);
dma_wait();
ed64_ll_set_sdcard_timing();
}📝 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_get_sram (uint8_t *buffer, int size) { | |
| int offset = (size == (int) KiB(32)) ? 0 : SRAM_128K_OFFSET; | |
| pi_initialize_sram(); | |
| dma_wait(); | |
| pi_initialize(); | |
| wait_ms(250); | |
| pi_dma_from_sram(buffer, offset, size); | |
| dma_wait(); | |
| ed64_ll_set_sdcard_timing(); | |
| } | |
| void ed64_ll_get_sram (uint8_t *buffer, int size) { | |
| int offset = (size == (int) KiB(32)) ? 0 : SRAM_128K_OFFSET; | |
| pi_initialize_sram(); | |
| dma_wait(); | |
| pi_initialize(); | |
| wait_ms(250); | |
| data_cache_hit_writeback_invalidate(buffer, size); | |
| pi_dma_from_sram(buffer, offset, size); | |
| dma_wait(); | |
| ed64_ll_set_sdcard_timing(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/flashcart/ed64/ed64_ll.c` around lines 150 - 162, ed64_ll_get_sram starts
a PI DMA into buffer via pi_dma_from_sram but does not invalidate the CPU data
cache for buffer, so stale cached lines can be read later; add a destination
cache invalidation (matching the convention used by pi_dma_read_data and the
FRAM path) immediately before calling pi_dma_from_sram in ed64_ll_get_sram
(e.g., call the platform cache-invalidate helper such as osInvalDCache or the
project's equivalent on buffer/size), keeping the rest of the sequence
(pi_initialize_sram, dma_wait, pi_initialize, wait_ms) intact and ensuring
ed64_ll_get_sram now invalidates buffer before the DMA read.
| int save_size = (int) file_get_size(current_state.last_save_path); | ||
| if (save_size <= 0 || save_size > (int) sizeof(cartsave_data)) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| ed64_ll_set_sdcard_timing(); | ||
| if (ed64_apply_save_type(current_state.save_type) != FLASHCART_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
| wait_ms(250); | ||
|
|
||
| if (current_state.is_fram_save_type) { | ||
| ed64_ll_get_fram(cartsave_data, save_size); | ||
| current_state.is_fram_save_type = false; | ||
| } else if (save_size > (int) KiB(2)) { | ||
| ed64_ll_get_sram(cartsave_data, save_size); | ||
| } else { | ||
| ed64_ll_get_eeprom(cartsave_data, save_size); | ||
| } | ||
|
|
||
| if (f_open(&fil, strip_fs_prefix(current_state.last_save_path), FA_CREATE_ALWAYS | FA_WRITE) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| if (f_write(&fil, cartsave_data, save_size, &bw) != FR_OK || bw != (UINT) save_size) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| if (f_close(&fil) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } |
There was a problem hiding this comment.
Writeback error paths leave is_expecting_save_writeback set.
If save_size validation fails (Lines 87‑89), ed64_apply_save_type fails (92‑94), or the file write fails (106‑117), the function returns FLASHCART_ERR_LOAD without clearing the flag or persisting state. Since ed64_init calls this on every boot when the flag is set, a persistently failing writeback blocks menu init on each reboot. Consider clearing/persisting state (or distinguishing transient vs. permanent failures) before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/flashcart/ed64/ed64.c` around lines 86 - 117, The early-return error
paths in this block leave current_state.is_expecting_save_writeback set, causing
persistent boot failures; before each return FLASHCART_ERR_LOAD in the save
writeback flow (after save_size validation, after ed64_apply_save_type failure,
and on any file write/close failure) clear
current_state.is_expecting_save_writeback = false and persist that change (call
the existing state persistence routine, e.g. ed64_persist_state() or the
equivalent function used elsewhere) so the cleared flag is saved to disk; do
this immediately prior to each return to ensure the flag is not left set across
reboots.
|
Some one with a ED64 V3 please test the save feature |
|
V2, V2.5 & V3 Confirmed to be working with saves https://discord.com/channels/205520502922543113/1262536014623146125/1510786446099877999 |
|
Thanks for getting the saves working. I would encourage the refactor to use the existing naming for v series carts |
2 participants
Description
Saves for ED64 devices on V2 or clones
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist:
You agree with the license terms and that other license types may be granted with permission of the original
N64FlashcartMenuproject license holders.Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
Summary by CodeRabbit
New Features
Refactor