Add ED64 V-series save writeback support - #344
Open
lewilou22 wants to merge 6 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds EverDrive-64 V-series support: low-level hardware (ed64_ll), high-level ED64 logic with SD save writeback/state (ed64), state persistence (ed64_state), vseries delegation, and Makefile inclusion of the new sources. ChangesED64 Implementation Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 202-214: ed64_ll_get_sram performs a PI DMA into the CPU-visible
buffer but never invalidates the CPU data cache afterwards, so add a data-cache
invalidation for the buffer region immediately after the dma_wait() that follows
pi_dma_from_sram (i.e., after dma_wait() and before ed64_ll_set_sdcard_timing())
to ensure the CPU sees fresh data; use the platform's cache-invalidate routine
appropriate for your codebase (call it with the buffer pointer and size and
respect any alignment requirements).
- Around line 129-135: In ed64_ll_set_sdcard_timing the fourth timing byte is
extracted with cfg >> 20 which is incorrect for the 4th byte of ED64_PI_DOM2_SD
(bytes are at 0,8,16,24); update the extraction for PI_BSD_DOM2_RLS_REG to use
cfg >> 24 instead of cfg >> 20 so the function ed64_ll_set_sdcard_timing writes
the correct 8-bit RLS value (references: ED64_PI_DOM2_SD,
ed64_ll_set_sdcard_timing, PI_BSD_DOM2_RLS_REG; other extractions
PI_BSD_DOM2_LAT_REG, PI_BSD_DOM2_PWD_REG, PI_BSD_DOM2_PGS_REG remain unchanged).
In `@src/flashcart/ed64/ed64.c`:
- Around line 223-232: The loop reading the ROM doesn't handle short reads: when
f_read returns FR_OK but br < block_size, the code still advances offset and
leaves gaps; update the loop that calls f_read (and uses variables fil, br,
block_size, offset, ROM_ADDRESS) to treat a short read as an immediate error by
closing fil and returning FLASHCART_ERR_LOAD; ensure you perform this check
right after f_read and before updating offset/progress so partial reads fail
fast.
- Around line 347-349: ed64_set_save_type currently updates
current_state.save_type before calling ed64_apply_save_type, causing an invalid
save_type to be persisted even if the hardware rejects it; modify
ed64_set_save_type to call ed64_apply_save_type(save_type) first, check its
success, and only then assign current_state.save_type = save_type (so subsequent
calls like ed64_writeback_save won't act on unsupported types and won't trigger
ed64_fail_writeback due to a prematurely stored value).
- Around line 263-274: Reject underflow/OOF cases before doing the subtractions:
check that rom_offset <= ED64_SDRAM_SIZE and that file_offset <= f_size(&fil)
(or that file_offset < = f_size - 0) and return FLASHCART_ERR_ARGS if not,
before computing file_size or doing ED64_SDRAM_SIZE - rom_offset; then proceed
with existing logic that computes file_size, compares it to (ED64_SDRAM_SIZE -
rom_offset), seeks and calls f_read into (ROM_ADDRESS + rom_offset). Ensure you
reference the rom_offset, file_offset, ED64_SDRAM_SIZE, f_size(&fil), file_size
and the f_read destination (ROM_ADDRESS + rom_offset) when adding the checks.
- Around line 294-342: Update logic so current_state.last_save_path is recorded
immediately from save_path before attempting f_open, and treat the f_open return
value FR_NO_FILE as an empty-save case instead of an error: strdup save_path
into current_state.last_save_path (freeing any previous value) and set
current_state.is_expecting_save_writeback = true and call
ed64_state_save(¤t_state) before opening; then call f_open and if it
returns FR_NO_FILE continue with save_size = 0 and zeroed cartsave_data (or skip
reads), but for other non-FR_OK errors return FLASHCART_ERR_LOAD; afterwards
proceed to determine ed64_ll_get_save_type() and call the appropriate
ed64_ll_set_* functions with the (possibly zero-length) buffer and save_size,
and preserve current_state.is_fram_save_type handling.
🪄 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: 6ed40775-a13a-4d34-a464-75c737c52d69
📒 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_vseries.csrc/flashcart/ed64/ed64_vseries.hsrc/flashcart/flashcart.c
Comment on lines
+129
to
+135
| void ed64_ll_set_sdcard_timing (void) { | ||
| uint32_t cfg = ED64_PI_DOM2_SD; | ||
| io_write(PI_BSD_DOM2_LAT_REG, cfg >> 0); | ||
| io_write(PI_BSD_DOM2_PWD_REG, cfg >> 8); | ||
| io_write(PI_BSD_DOM2_PGS_REG, cfg >> 16); | ||
| io_write(PI_BSD_DOM2_RLS_REG, cfg >> 20); | ||
| } |
Contributor
There was a problem hiding this comment.
Incorrect bit shift for RLS register: should be 24, not 20.
ED64_PI_DOM2_SD encodes four 8-bit values in bytes 0-3. The extraction for PI_BSD_DOM2_RLS_REG shifts by 20 bits instead of 24, yielding 0x0803 (truncated to lower bits) instead of the intended 0x80.
🐛 Proposed fix
void ed64_ll_set_sdcard_timing (void) {
uint32_t cfg = ED64_PI_DOM2_SD;
io_write(PI_BSD_DOM2_LAT_REG, cfg >> 0);
io_write(PI_BSD_DOM2_PWD_REG, cfg >> 8);
io_write(PI_BSD_DOM2_PGS_REG, cfg >> 16);
- io_write(PI_BSD_DOM2_RLS_REG, cfg >> 20);
+ io_write(PI_BSD_DOM2_RLS_REG, cfg >> 24);
}🤖 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 129 - 135, In
ed64_ll_set_sdcard_timing the fourth timing byte is extracted with cfg >> 20
which is incorrect for the 4th byte of ED64_PI_DOM2_SD (bytes are at 0,8,16,24);
update the extraction for PI_BSD_DOM2_RLS_REG to use cfg >> 24 instead of cfg >>
20 so the function ed64_ll_set_sdcard_timing writes the correct 8-bit RLS value
(references: ED64_PI_DOM2_SD, ed64_ll_set_sdcard_timing, PI_BSD_DOM2_RLS_REG;
other extractions PI_BSD_DOM2_LAT_REG, PI_BSD_DOM2_PWD_REG, PI_BSD_DOM2_PGS_REG
remain unchanged).
Comment on lines
+202
to
+214
| 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(); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing cache invalidation after DMA read may cause stale data.
After PI DMA writes to DRAM (lines 210-211), the CPU data cache may still hold stale values for the buffer region. Without invalidating the cache after dma_wait(), subsequent CPU reads could return old data.
Note that ed64_ll_get_fram (line 227) invalidates before calling this function, but direct callers of ed64_ll_get_sram could be affected.
🐛 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);
pi_dma_from_sram(buffer, offset, size);
dma_wait();
+ data_cache_hit_invalidate(buffer, size);
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.
Suggested change
| 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); | |
| pi_dma_from_sram(buffer, offset, size); | |
| dma_wait(); | |
| data_cache_hit_invalidate(buffer, size); | |
| 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 202 - 214, ed64_ll_get_sram
performs a PI DMA into the CPU-visible buffer but never invalidates the CPU data
cache afterwards, so add a data-cache invalidation for the buffer region
immediately after the dma_wait() that follows pi_dma_from_sram (i.e., after
dma_wait() and before ed64_ll_set_sdcard_timing()) to ensure the CPU sees fresh
data; use the platform's cache-invalidate routine appropriate for your codebase
(call it with the buffer pointer and size and respect any alignment
requirements).
Comment on lines
+223
to
+232
| size_t chunk_size = KiB(128); | ||
| for (size_t offset = 0; offset < rom_size; offset += chunk_size) { | ||
| size_t block_size = MIN(rom_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)); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle short ROM reads immediately.
f_read can return FR_OK with br < block_size. This loop still advances offset by the full chunk, so a short read leaves a gap in SDRAM and only fails later at Lines 235-237. Treat short reads as an immediate load failure here.
🤖 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 223 - 232, The loop reading the ROM
doesn't handle short reads: when f_read returns FR_OK but br < block_size, the
code still advances offset and leaves gaps; update the loop that calls f_read
(and uses variables fil, br, block_size, offset, ROM_ADDRESS) to treat a short
read as an immediate error by closing fil and returning FLASHCART_ERR_LOAD;
ensure you perform this check right after f_read and before updating
offset/progress so partial reads fail fast.
Comment on lines
+263
to
+274
| size_t file_size = f_size(&fil) - file_offset; | ||
| if (file_size > (ED64_SDRAM_SIZE - 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) { |
Contributor
There was a problem hiding this comment.
Reject out-of-range offsets before subtracting.
If rom_offset > ED64_SDRAM_SIZE, ED64_SDRAM_SIZE - rom_offset underflows and the f_read at Line 274 can write past the 64 MiB ED64 SDRAM window. file_offset > f_size(&fil) has the same problem for file_size. Add explicit bounds checks before either subtraction.
Suggested fix
- size_t file_size = f_size(&fil) - file_offset;
- if (file_size > (ED64_SDRAM_SIZE - rom_offset)) {
+ size_t total_size = f_size(&fil);
+ if (rom_offset > ED64_SDRAM_SIZE || file_offset > total_size) {
+ f_close(&fil);
+ return FLASHCART_ERR_ARGS;
+ }
+
+ size_t file_size = total_size - file_offset;
+ if (file_size > (ED64_SDRAM_SIZE - rom_offset)) {
f_close(&fil);
return FLASHCART_ERR_ARGS;
}📝 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.
Suggested change
| size_t file_size = f_size(&fil) - file_offset; | |
| if (file_size > (ED64_SDRAM_SIZE - 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) { | |
| size_t total_size = f_size(&fil); | |
| if (rom_offset > ED64_SDRAM_SIZE || file_offset > total_size) { | |
| f_close(&fil); | |
| return FLASHCART_ERR_ARGS; | |
| } | |
| size_t file_size = total_size - file_offset; | |
| if (file_size > (ED64_SDRAM_SIZE - 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) { |
🤖 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 263 - 274, Reject underflow/OOF cases
before doing the subtractions: check that rom_offset <= ED64_SDRAM_SIZE and that
file_offset <= f_size(&fil) (or that file_offset < = f_size - 0) and return
FLASHCART_ERR_ARGS if not, before computing file_size or doing ED64_SDRAM_SIZE -
rom_offset; then proceed with existing logic that computes file_size, compares
it to (ED64_SDRAM_SIZE - rom_offset), seeks and calls f_read into (ROM_ADDRESS +
rom_offset). Ensure you reference the rom_offset, file_offset, ED64_SDRAM_SIZE,
f_size(&fil), file_size and the f_read destination (ROM_ADDRESS + rom_offset)
when adding the checks.
Comment on lines
+294
to
+342
| if (f_open(&fil, strip_fs_prefix(save_path), FA_READ) != FR_OK) { | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| size_t save_size = f_size(&fil); | ||
| if (save_size > KiB(128)) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
|
|
||
| uint8_t cartsave_data[KiB(128)]; | ||
| if (f_read(&fil, cartsave_data, save_size, &br) != FR_OK) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
| if (br != save_size) { | ||
| f_close(&fil); | ||
| return FLASHCART_ERR_LOAD; | ||
| } | ||
| if (f_close(&fil) != FR_OK) { | ||
| 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); | ||
| current_state.is_fram_save_type = true; | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
|
|
||
| if (current_state.last_save_path) { | ||
| free(current_state.last_save_path); | ||
| } | ||
| current_state.last_save_path = strdup(save_path); | ||
| current_state.is_expecting_save_writeback = true; | ||
| ed64_state_save(¤t_state); |
Contributor
There was a problem hiding this comment.
Register the save path even when no .sav exists yet.
last_save_path is only refreshed after f_open succeeds here. That means a first-run game with no existing save file never records its destination. On V3, Lines 57-60 will still sync battery-backed RAM on the next boot, but into the previous game's last_save_path, corrupting the wrong save file. Record the new path before opening the file and treat FR_NO_FILE as the empty-save case instead of an error.
Suggested fix
static flashcart_err_t ed64_load_save (char *save_path) {
FIL fil;
UINT br;
+ FRESULT fr;
+
+ if (current_state.last_save_path) {
+ free(current_state.last_save_path);
+ }
+ current_state.last_save_path = strdup(save_path);
+ if (!current_state.last_save_path) {
+ return FLASHCART_ERR_LOAD;
+ }
+ current_state.is_expecting_save_writeback = true;
+ current_state.is_fram_save_type = false;
- if (f_open(&fil, strip_fs_prefix(save_path), FA_READ) != FR_OK) {
+ fr = f_open(&fil, strip_fs_prefix(save_path), FA_READ);
+ if (fr == FR_NO_FILE) {
+ ed64_state_save(¤t_state);
+ return FLASHCART_OK;
+ }
+ if (fr != FR_OK) {
return FLASHCART_ERR_LOAD;
}
@@
- current_state.is_fram_save_type = false;
-
ed64_save_type_t type = ed64_ll_get_save_type();
@@
- if (current_state.last_save_path) {
- free(current_state.last_save_path);
- }
- current_state.last_save_path = strdup(save_path);
- current_state.is_expecting_save_writeback = true;
ed64_state_save(¤t_state);🤖 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 294 - 342, Update logic so
current_state.last_save_path is recorded immediately from save_path before
attempting f_open, and treat the f_open return value FR_NO_FILE as an empty-save
case instead of an error: strdup save_path into current_state.last_save_path
(freeing any previous value) and set current_state.is_expecting_save_writeback =
true and call ed64_state_save(¤t_state) before opening; then call f_open
and if it returns FR_NO_FILE continue with save_size = 0 and zeroed
cartsave_data (or skip reads), but for other non-FR_OK errors return
FLASHCART_ERR_LOAD; afterwards proceed to determine ed64_ll_get_save_type() and
call the appropriate ed64_ll_set_* functions with the (possibly zero-length)
buffer and save_size, and preserve current_state.is_fram_save_type handling.
Comment on lines
+347
to
+349
| static flashcart_err_t ed64_set_save_type (flashcart_save_type_t save_type) { | ||
| current_state.save_type = save_type; | ||
| return ed64_apply_save_type(save_type); |
Contributor
There was a problem hiding this comment.
Only persist save_type after the hardware accepts it.
If ed64_apply_save_type(save_type) rejects an unsupported value, this code still leaves current_state.save_type updated. The next boot then reuses that invalid type in ed64_writeback_save, which disables writeback via ed64_fail_writeback(). Save the state only after ed64_apply_save_type succeeds.
Suggested fix
static flashcart_err_t ed64_set_save_type (flashcart_save_type_t save_type) {
- current_state.save_type = save_type;
- return ed64_apply_save_type(save_type);
+ flashcart_err_t err = ed64_apply_save_type(save_type);
+ if (err != FLASHCART_OK) {
+ return err;
+ }
+ current_state.save_type = save_type;
+ return FLASHCART_OK;
}🤖 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 347 - 349, ed64_set_save_type
currently updates current_state.save_type before calling ed64_apply_save_type,
causing an invalid save_type to be persisted even if the hardware rejects it;
modify ed64_set_save_type to call ed64_apply_save_type(save_type) first, check
its success, and only then assign current_state.save_type = save_type (so
subsequent calls like ed64_writeback_save won't act on unsupported types and
won't trigger ed64_fail_writeback due to a prematurely stored value).
Collaborator
|
Linking to #342 which was probably better. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_state.c`:
- Around line 9-14: ed64_state_init currently assigns ed64_state_path =
strdup(path) without checking for allocation failure; add a NULL check after
strdup in ed64_state_init and handle failure (e.g., leave previous path
unchanged or free and set to NULL and return an error) so later functions like
ed64_state_load/ed64_state_save calling ini_try_load(ed64_state_path) won't
dereference NULL; specifically update ed64_state_init to detect if strdup
returned NULL and implement a graceful fallback or return/report an error to
callers that rely on ed64_state_path.
- Around line 16-29: ed64_state_load should check strdup failures and avoid
conflating allocation failure with "no path"; fetch the string into a local
const char* (e.g. path = ini_get_string(...)), free the old
state->last_save_path as already done, then if path is empty set
state->last_save_path = NULL (avoid allocating an empty string), otherwise
attempt state->last_save_path = strdup(path) and if strdup returns NULL handle
the allocation failure (set state->last_save_path = NULL and log/handle the
error appropriately) so downstream checks on state->last_save_path distinguish
"no path" from allocation failure.
- Around line 31-41: The function ed64_state_save currently ignores the return
value of ini_save which can hide persistence failures; update ed64_state_save
(and its callers) to check ini_save's return and handle failures: modify
ed64_state_save to return an error code or boolean instead of void, call
ini_save(ini, ed64_state_path) and if it fails log an error with context
(include ed64_state_path and state->is_expecting_save_writeback) and return a
failure value, otherwise return success; update any callers of ed64_state_save
to check the new return value and propagate or handle the error appropriately.
🪄 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: 88f09a19-51a6-4baf-b8b1-a4ef57a0a517
📒 Files selected for processing (2)
src/flashcart/ed64/ed64_state.csrc/flashcart/ed64/ed64_state.h
Comment on lines
+9
to
+14
| void ed64_state_init (const char *path) { | ||
| if (ed64_state_path) { | ||
| free(ed64_state_path); | ||
| } | ||
| ed64_state_path = strdup(path); | ||
| } |
Contributor
There was a problem hiding this comment.
Add null check after strdup to prevent crash on allocation failure.
If strdup fails and returns NULL, subsequent calls to ed64_state_load or ed64_state_save will dereference ed64_state_path when calling ini_try_load(ed64_state_path), causing a crash.
🛡️ Proposed fix to add null check
void ed64_state_init (const char *path) {
if (ed64_state_path) {
free(ed64_state_path);
}
ed64_state_path = strdup(path);
+ if (!ed64_state_path) {
+ /* Fatal: cannot proceed without state path */
+ return;
+ }
}Alternatively, consider returning an error code or using a fallback path to allow graceful degradation.
🤖 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_state.c` around lines 9 - 14, ed64_state_init
currently assigns ed64_state_path = strdup(path) without checking for allocation
failure; add a NULL check after strdup in ed64_state_init and handle failure
(e.g., leave previous path unchanged or free and set to NULL and return an
error) so later functions like ed64_state_load/ed64_state_save calling
ini_try_load(ed64_state_path) won't dereference NULL; specifically update
ed64_state_init to detect if strdup returned NULL and implement a graceful
fallback or return/report an error to callers that rely on ed64_state_path.
Comment on lines
+16
to
+29
| void ed64_state_load (ed64_pseudo_writeback_t *state) { | ||
| ini_t *ini = ini_try_load(ed64_state_path); | ||
|
|
||
| state->is_expecting_save_writeback = ini_get_bool(ini, "ed64", "is_expecting_save_writeback", false); | ||
| state->is_fram_save_type = ini_get_bool(ini, "ed64", "is_fram_save_type", false); | ||
| state->save_type = (flashcart_save_type_t) ini_get_int(ini, "ed64", "save_type", FLASHCART_SAVE_TYPE_NONE); | ||
|
|
||
| if (state->last_save_path) { | ||
| free(state->last_save_path); | ||
| } | ||
| state->last_save_path = strdup(ini_get_string(ini, "ed64", "last_save_path", "")); | ||
|
|
||
| ini_free(ini); | ||
| } |
Contributor
There was a problem hiding this comment.
Check strdup result to detect allocation failures.
If strdup fails on line 26, state->last_save_path becomes NULL, which is indistinguishable from "no path set." According to context snippet 2 (ed64.c:52-118), the downstream check if (!current_state.last_save_path || !current_state.last_save_path[0]) will silently skip writeback, potentially losing save data when allocation fails rather than when no path is configured.
🛡️ Proposed fix to handle allocation failure
- state->last_save_path = strdup(ini_get_string(ini, "ed64", "last_save_path", ""));
+ const char *path = ini_get_string(ini, "ed64", "last_save_path", "");
+ state->last_save_path = (*path != '\0') ? strdup(path) : NULL;
+ if (*path != '\0' && !state->last_save_path) {
+ /* Could not allocate memory for save path; treat as fatal or log error */
+ }This also avoids allocating memory for an empty string when NULL would suffice.
🤖 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_state.c` around lines 16 - 29, ed64_state_load should
check strdup failures and avoid conflating allocation failure with "no path";
fetch the string into a local const char* (e.g. path = ini_get_string(...)),
free the old state->last_save_path as already done, then if path is empty set
state->last_save_path = NULL (avoid allocating an empty string), otherwise
attempt state->last_save_path = strdup(path) and if strdup returns NULL handle
the allocation failure (set state->last_save_path = NULL and log/handle the
error appropriately) so downstream checks on state->last_save_path distinguish
"no path" from allocation failure.
Comment on lines
+31
to
+41
| void ed64_state_save (ed64_pseudo_writeback_t *state) { | ||
| ini_t *ini = ini_try_load(ed64_state_path); | ||
|
|
||
| ini_set_bool(ini, "ed64", "is_expecting_save_writeback", state->is_expecting_save_writeback); | ||
| ini_set_bool(ini, "ed64", "is_fram_save_type", state->is_fram_save_type); | ||
| ini_set_int(ini, "ed64", "save_type", state->save_type); | ||
| ini_set_string(ini, "ed64", "last_save_path", state->last_save_path ? state->last_save_path : ""); | ||
|
|
||
| ini_save(ini, ed64_state_path); | ||
| ini_free(ini); | ||
| } |
Contributor
There was a problem hiding this comment.
Check the return value of ini_save to detect persistence failures.
If ini_save fails on line 39 (e.g., disk full, write error), the state will not be persisted, but the function returns successfully. This could break the save-writeback flow described in the PR objectives: when is_expecting_save_writeback fails to persist, subsequent resets may either skip writeback or enter a boot loop.
🛡️ Proposed fix to return or log save errors
Option 1: Return an error code:
-void ed64_state_save (ed64_pseudo_writeback_t *state) {
+bool ed64_state_save (ed64_pseudo_writeback_t *state) {
ini_t *ini = ini_try_load(ed64_state_path);
ini_set_bool(ini, "ed64", "is_expecting_save_writeback", state->is_expecting_save_writeback);
ini_set_bool(ini, "ed64", "is_fram_save_type", state->is_fram_save_type);
ini_set_int(ini, "ed64", "save_type", state->save_type);
ini_set_string(ini, "ed64", "last_save_path", state->last_save_path ? state->last_save_path : "");
- ini_save(ini, ed64_state_path);
+ bool success = ini_save(ini, ed64_state_path);
ini_free(ini);
+ return success;
}Then update callers to check the return value.
Option 2 (minimal): At least check and log:
- ini_save(ini, ed64_state_path);
+ if (!ini_save(ini, ed64_state_path)) {
+ /* Log or handle failure to persist ED64 state */
+ }📝 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.
Suggested change
| void ed64_state_save (ed64_pseudo_writeback_t *state) { | |
| ini_t *ini = ini_try_load(ed64_state_path); | |
| ini_set_bool(ini, "ed64", "is_expecting_save_writeback", state->is_expecting_save_writeback); | |
| ini_set_bool(ini, "ed64", "is_fram_save_type", state->is_fram_save_type); | |
| ini_set_int(ini, "ed64", "save_type", state->save_type); | |
| ini_set_string(ini, "ed64", "last_save_path", state->last_save_path ? state->last_save_path : ""); | |
| ini_save(ini, ed64_state_path); | |
| ini_free(ini); | |
| } | |
| bool ed64_state_save (ed64_pseudo_writeback_t *state) { | |
| ini_t *ini = ini_try_load(ed64_state_path); | |
| ini_set_bool(ini, "ed64", "is_expecting_save_writeback", state->is_expecting_save_writeback); | |
| ini_set_bool(ini, "ed64", "is_fram_save_type", state->is_fram_save_type); | |
| ini_set_int(ini, "ed64", "save_type", state->save_type); | |
| ini_set_string(ini, "ed64", "last_save_path", state->last_save_path ? state->last_save_path : ""); | |
| bool success = ini_save(ini, ed64_state_path); | |
| ini_free(ini); | |
| return success; | |
| } |
Suggested change
| void ed64_state_save (ed64_pseudo_writeback_t *state) { | |
| ini_t *ini = ini_try_load(ed64_state_path); | |
| ini_set_bool(ini, "ed64", "is_expecting_save_writeback", state->is_expecting_save_writeback); | |
| ini_set_bool(ini, "ed64", "is_fram_save_type", state->is_fram_save_type); | |
| ini_set_int(ini, "ed64", "save_type", state->save_type); | |
| ini_set_string(ini, "ed64", "last_save_path", state->last_save_path ? state->last_save_path : ""); | |
| ini_save(ini, ed64_state_path); | |
| ini_free(ini); | |
| } | |
| void ed64_state_save (ed64_pseudo_writeback_t *state) { | |
| ini_t *ini = ini_try_load(ed64_state_path); | |
| ini_set_bool(ini, "ed64", "is_expecting_save_writeback", state->is_expecting_save_writeback); | |
| ini_set_bool(ini, "ed64", "is_fram_save_type", state->is_fram_save_type); | |
| ini_set_int(ini, "ed64", "save_type", state->save_type); | |
| ini_set_string(ini, "ed64", "last_save_path", state->last_save_path ? state->last_save_path : ""); | |
| if (!ini_save(ini, ed64_state_path)) { | |
| /* Log or handle failure to persist ED64 state */ | |
| } | |
| ini_free(ini); | |
| } |
🤖 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_state.c` around lines 31 - 41, The function
ed64_state_save currently ignores the return value of ini_save which can hide
persistence failures; update ed64_state_save (and its callers) to check
ini_save's return and handle failures: modify ed64_state_save to return an error
code or boolean instead of void, call ini_save(ini, ed64_state_path) and if it
fails log an error with context (include ed64_state_path and
state->is_expecting_save_writeback) and return a failure value, otherwise return
success; update any callers of ed64_state_save to check the new return value and
propagate or handle the error appropriately.
lewilou22
changed the title
Author
|
should be fixed to the original #342 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added the fix for V2, V3 ED64 Flashcarts
Saves are now operational after a reset
Motivation and Context
#44
How Has This Been Tested?
We have tested via Discord
https://discord.com/channels/205520502922543113/1262536014623146125/1510408587049177170
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