diff --git a/.claude/skills/sd_card_best_practices.md b/.claude/skills/sd_card_best_practices.md index a98e455..c4b1d19 100644 --- a/.claude/skills/sd_card_best_practices.md +++ b/.claude/skills/sd_card_best_practices.md @@ -2,13 +2,99 @@ This document captures critical best practices for working with SD card operations in this project, based on lessons learned during development. -## 1. SPI Speed Management +## 1. SPI Bus Contention (CRITICAL) + +### ⚠️ CRITICAL: Display and SD Card Share the Same SPI Bus + +**The display and SD card use the same SPI bus and CANNOT be accessed simultaneously.** Attempting to do so will cause the Pico to crash or behave unpredictably. + +### Real-World Example: Game Launch Crash + +The serial uploader originally crashed when launching games because: + +1. `SerialUploader::launch_game()` writes file to SD (SPI) +2. Immediately calls `select_game_by_name()` +3. `LuaGame::load_script()` reads from SD (SPI) +4. **Meanwhile**, Core 1 is refreshing the display (also SPI) +5. **CRASH** due to simultaneous SPI access + +### Solution: Wait for Display to Be Idle + +Before any SD card operation that isn't already protected, ensure no display refresh is in progress: + +```cpp +// In main loop: +if (serial_uploader.wants_to_launch_game() && !is_refresh_in_progress()) { + // Safe to launch now - no SPI conflict with display + bool game_launched = serial_uploader.complete_launch(); + // ... +} +``` + +### Key Patterns for Avoiding Contention + +**Pattern 1: Check `is_refresh_in_progress()` before SD operations** +```cpp +if (!is_refresh_in_progress()) { + uint prev_speed = sd_card_set_spi_speed(); + // SD card operations + sd_card_restore_spi_speed(prev_speed); +} +``` + +**Pattern 2: Split operations into "prepare" and "execute" phases** +```cpp +// Phase 1: Prepare (safe, no SD access) +void prepare_operation() { + state = READY_TO_EXECUTE; +} + +// Phase 2: Execute (only when !is_refresh_in_progress()) +bool execute_operation() { + uint prev_speed = sd_card_set_spi_speed(); + // SD card operations here + sd_card_restore_spi_speed(prev_speed); +} +``` + +**Pattern 3: Keep main loop responsive** +```cpp +while (1) { + // Check if we need to do SD operation + if (needs_sd_operation && !is_refresh_in_progress()) { + perform_sd_operation(); + } + + // Don't sleep if waiting for SD operation window + bool stay_awake = pending_refresh || needs_sd_operation; + if (!stay_awake) { + __wfi(); // Sleep until interrupt + } +} +``` + +### When This Matters Most + +- **Game loading**: Reading Lua scripts from SD during game launch +- **Serial uploads**: Writing files and immediately loading them +- **Save/load operations**: Writing game state to SD +- **Directory scanning**: Re-scanning games while display is active + +### The Core Architecture + +This project uses **dual-core display refresh**: +- **Core 0**: Main logic, input processing, game updates, SD card operations +- **Core 1**: Display refresh (writes framebuffer to display via SPI) + +Core 1 runs asynchronously, so you must explicitly check `is_refresh_in_progress()` before SD operations. + +## 2. SPI Speed Management ### Critical Rule: Always Set SD Card Speed Before Operations The display and SD card share the same SPI bus but operate at different speeds: -- **Display**: 32 MHz -- **SD Card**: 12.5 MHz +- **Display**: 32 MHz (fast) +- **SD Card**: 12.5 MHz (slower, more reliable) **ALWAYS wrap SD card operations with speed switching:** @@ -220,6 +306,29 @@ This isolates directory-related issues. ## 6. Common Pitfalls +### ❌ DON'T: Access SD Card During Display Refresh (MOST CRITICAL) + +```cpp +// BAD - Will crash the Pico! +void launch_game() { + scan_games(); // Reads SD card + selected_game = create_game(); // Reads Lua script from SD +} +// Called directly without checking if display is refreshing +``` + +### ✅ DO: Wait for Display to Be Idle + +```cpp +// GOOD - Wait for safe window +if (!is_refresh_in_progress()) { + uint prev_speed = sd_card_set_spi_speed(); + scan_games(); + selected_game = create_game(); + sd_card_restore_spi_speed(prev_speed); +} +``` + ### ❌ DON'T: Forget SPI Speed Management ```cpp @@ -346,10 +455,11 @@ bool SerialUploader::write_file_to_sd() { } ``` -## 8. Testing Checklist +## 9. Testing Checklist When implementing new SD card functionality: +- [ ] **SPI bus contention checked** - verify `!is_refresh_in_progress()` before SD operations - [ ] SPI speed switching is in place - [ ] All FatFS return codes are checked - [ ] Files are properly closed after operations @@ -363,11 +473,12 @@ When implementing new SD card functionality: ## Summary The most important rules: -1. **Always manage SPI speed** around FatFS operations -2. **Poll for SD card responses** - don't assume immediate response -3. **Check error codes** on every operation -4. **Clean up memory** before creating new game instances -5. **Write in chunks** for large files -6. **Sync before closing** to ensure data is written +1. **⚠️ CRITICAL: Avoid SPI bus contention** - Check `!is_refresh_in_progress()` before SD operations (display and SD share SPI bus) +2. **Always manage SPI speed** around FatFS operations +3. **Poll for SD card responses** - don't assume immediate response +4. **Check error codes** on every operation +5. **Clean up memory** before creating new game instances +6. **Write in chunks** for large files +7. **Sync before closing** to ensure data is written Following these practices will save hours of debugging SD card issues! diff --git a/basic1.cpp b/basic1.cpp index 4302afd..780600b 100644 --- a/basic1.cpp +++ b/basic1.cpp @@ -572,17 +572,24 @@ int main() while (1) { // 0. Process serial uploads (for rapid game iteration) - bool game_launched_via_serial = serial_uploader.process(); - if (game_launched_via_serial) { - // A new game was uploaded and launched - trigger redraw - needs_refresh = true; - current_game = launcher.get_selected_game(); - // Note: game is already initialized by select_game_by_name() + serial_uploader.process(); + + // If serial uploader wants to launch a game, wait until it's safe (no display refresh) + if (serial_uploader.wants_to_launch_game() && !is_refresh_in_progress()) { + // Safe to launch now - no SPI conflict with display + bool game_launched = serial_uploader.complete_launch(); + if (game_launched) { + // A new game was uploaded and launched - trigger redraw + needs_refresh = true; + current_game = launcher.get_selected_game(); + // Note: game is already initialized by select_game_by_name() + } } // Determine if we should sleep or stay awake for updates bool stay_awake = false; if (pending_refresh) stay_awake = true; + if (serial_uploader.wants_to_launch_game()) stay_awake = true; // Don't sleep while waiting to launch if (launcher.is_game_selected()) { Game* g = launcher.get_selected_game(); diff --git a/games/lua_examples/2048.lua b/games/lua_examples/2048.lua index acd3109..ea32ba9 100644 --- a/games/lua_examples/2048.lua +++ b/games/lua_examples/2048.lua @@ -49,7 +49,7 @@ function init() game.vars.won = false -- Enable continuous updates - game.set_frame_updates(true) + -- game.set_frame_updates(true) print("2048 initialized") end @@ -124,7 +124,7 @@ function draw() if state == STATE_MENU then renderer.text_scaled(game.width() / 2 - 15, game.height() / 2 - 30, "2048", true, 2) renderer.text_scaled(game.width() / 2 - 50, game.height() / 2, "Tap to Start", true, 2) - renderer.text_scaled(game.width() / 2 - 50, game.height() / 2 + 30, "Welcome Adolfo2!", true, 2) + renderer.text_scaled(game.width() / 2 - 50, game.height() / 2 + 30, "Welcome Adolfo", true, 2) elseif state == STATE_PLAYING or state == STATE_WIN or state == STATE_GAME_OVER then -- Draw grid @@ -159,10 +159,10 @@ function draw() -- Draw value (simplified) local text = tostring(value) - if string.len(text) <= 2 then - renderer.text_scaled(tile_x + tile_size / 2 - 4, tile_y + tile_size / 2 - 4, text, false, 2) + if string.len(text) < 2 then + renderer.text_scaled(tile_x + tile_size / 2 - 4, tile_y + tile_size / 2 - 8, text, false, 2) else - renderer.text_scaled(tile_x + tile_size / 2 - 8, tile_y + tile_size / 2 - 4, text, false, 2) + renderer.text_scaled(tile_x + tile_size / 2 - 12, tile_y + tile_size / 2 - 8, text, false, 2) end end end diff --git a/lib/serial_uploader.cpp b/lib/serial_uploader.cpp index 1225b3e..1d6dda4 100644 --- a/lib/serial_uploader.cpp +++ b/lib/serial_uploader.cpp @@ -153,7 +153,10 @@ void SerialUploader::launch_game() { // Note: LuaGameLoader::register_all_games handles SPI speed internally int new_games = LuaGameLoader::register_all_games(game_launcher); printf("Found %d Lua games after re-scan\n", new_games); +} +bool SerialUploader::complete_launch() { + // This should only be called when it's safe (no display refresh in progress) // Now try to launch the newly uploaded game by name printf("Attempting to launch game: %s\n", last_uploaded_name); bool launched = game_launcher->select_game_by_name(last_uploaded_name); @@ -163,6 +166,11 @@ void SerialUploader::launch_game() { } else { printf("ERROR Failed to launch game: %s\n", last_uploaded_name); } + + // Reset state back to IDLE + reset(); + + return launched; } bool SerialUploader::process() { @@ -305,6 +313,8 @@ bool SerialUploader::process() { if (state == WRITING_FILE) { if (write_file_to_sd()) { state = LAUNCHING_GAME; + // Prepare for launch by scanning games, but don't actually launch yet + launch_game(); } else { reset(); } @@ -312,9 +322,8 @@ bool SerialUploader::process() { } if (state == LAUNCHING_GAME) { - launch_game(); - reset(); - return true; // Signal that game was launched + // Stay in this state until main loop calls complete_launch() when safe + return false; } return false; diff --git a/lib/serial_uploader.h b/lib/serial_uploader.h index 185b308..28493f8 100644 --- a/lib/serial_uploader.h +++ b/lib/serial_uploader.h @@ -14,6 +14,13 @@ public: // Returns true if a game was launched bool process(); + // Check if uploader wants to launch a game (after upload complete) + bool wants_to_launch_game() const { return state == LAUNCHING_GAME; } + + // Complete the game launch (call only when safe - no display refresh in progress) + // Returns true if launch succeeded + bool complete_launch(); + // Get the filename of the last uploaded game (without .lua extension) const char* get_last_uploaded_filename() const { return last_uploaded_name; }