Fix SPI bus contention crash on serial game upload

Serial uploader was crashing the Pico when launching games because
it accessed SD card (SPI) while Core 1 was refreshing display (also SPI).
Display and SD card share the same SPI bus and cannot be accessed
simultaneously.

Split game launch into prepare and execute phases:
- prepare: Re-scan games directory (safe, SD access done immediately)
- execute: Load Lua script from SD (deferred until display is idle)

Main loop now checks !is_refresh_in_progress() before completing
launch, preventing SPI conflicts.

Also updated SD card best practices skill to document SPI bus
contention as the #1 most critical issue to avoid.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Adolfo Reyna
2026-02-12 23:25:31 -05:00
parent f8fb04db1b
commit 518bc054c4
5 changed files with 158 additions and 24 deletions

View File

@@ -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. 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 ### Critical Rule: Always Set SD Card Speed Before Operations
The display and SD card share the same SPI bus but operate at different speeds: The display and SD card share the same SPI bus but operate at different speeds:
- **Display**: 32 MHz - **Display**: 32 MHz (fast)
- **SD Card**: 12.5 MHz - **SD Card**: 12.5 MHz (slower, more reliable)
**ALWAYS wrap SD card operations with speed switching:** **ALWAYS wrap SD card operations with speed switching:**
@@ -220,6 +306,29 @@ This isolates directory-related issues.
## 6. Common Pitfalls ## 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 ### ❌ DON'T: Forget SPI Speed Management
```cpp ```cpp
@@ -346,10 +455,11 @@ bool SerialUploader::write_file_to_sd() {
} }
``` ```
## 8. Testing Checklist ## 9. Testing Checklist
When implementing new SD card functionality: When implementing new SD card functionality:
- [ ] **SPI bus contention checked** - verify `!is_refresh_in_progress()` before SD operations
- [ ] SPI speed switching is in place - [ ] SPI speed switching is in place
- [ ] All FatFS return codes are checked - [ ] All FatFS return codes are checked
- [ ] Files are properly closed after operations - [ ] Files are properly closed after operations
@@ -363,11 +473,12 @@ When implementing new SD card functionality:
## Summary ## Summary
The most important rules: The most important rules:
1. **Always manage SPI speed** around FatFS operations 1. **⚠️ CRITICAL: Avoid SPI bus contention** - Check `!is_refresh_in_progress()` before SD operations (display and SD share SPI bus)
2. **Poll for SD card responses** - don't assume immediate response 2. **Always manage SPI speed** around FatFS operations
3. **Check error codes** on every operation 3. **Poll for SD card responses** - don't assume immediate response
4. **Clean up memory** before creating new game instances 4. **Check error codes** on every operation
5. **Write in chunks** for large files 5. **Clean up memory** before creating new game instances
6. **Sync before closing** to ensure data is written 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! Following these practices will save hours of debugging SD card issues!

View File

@@ -572,17 +572,24 @@ int main()
while (1) { while (1) {
// 0. Process serial uploads (for rapid game iteration) // 0. Process serial uploads (for rapid game iteration)
bool game_launched_via_serial = serial_uploader.process(); serial_uploader.process();
if (game_launched_via_serial) {
// A new game was uploaded and launched - trigger redraw // If serial uploader wants to launch a game, wait until it's safe (no display refresh)
needs_refresh = true; if (serial_uploader.wants_to_launch_game() && !is_refresh_in_progress()) {
current_game = launcher.get_selected_game(); // Safe to launch now - no SPI conflict with display
// Note: game is already initialized by select_game_by_name() 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 // Determine if we should sleep or stay awake for updates
bool stay_awake = false; bool stay_awake = false;
if (pending_refresh) stay_awake = true; 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()) { if (launcher.is_game_selected()) {
Game* g = launcher.get_selected_game(); Game* g = launcher.get_selected_game();

View File

@@ -49,7 +49,7 @@ function init()
game.vars.won = false game.vars.won = false
-- Enable continuous updates -- Enable continuous updates
game.set_frame_updates(true) -- game.set_frame_updates(true)
print("2048 initialized") print("2048 initialized")
end end
@@ -124,7 +124,7 @@ function draw()
if state == STATE_MENU then 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 - 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, "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 elseif state == STATE_PLAYING or state == STATE_WIN or state == STATE_GAME_OVER then
-- Draw grid -- Draw grid
@@ -159,10 +159,10 @@ function draw()
-- Draw value (simplified) -- Draw value (simplified)
local text = tostring(value) local text = tostring(value)
if string.len(text) <= 2 then if string.len(text) < 2 then
renderer.text_scaled(tile_x + tile_size / 2 - 4, tile_y + tile_size / 2 - 4, text, false, 2) renderer.text_scaled(tile_x + tile_size / 2 - 4, tile_y + tile_size / 2 - 8, text, false, 2)
else 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 end
end end

View File

@@ -153,7 +153,10 @@ void SerialUploader::launch_game() {
// Note: LuaGameLoader::register_all_games handles SPI speed internally // Note: LuaGameLoader::register_all_games handles SPI speed internally
int new_games = LuaGameLoader::register_all_games(game_launcher); int new_games = LuaGameLoader::register_all_games(game_launcher);
printf("Found %d Lua games after re-scan\n", new_games); 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 // Now try to launch the newly uploaded game by name
printf("Attempting to launch game: %s\n", last_uploaded_name); printf("Attempting to launch game: %s\n", last_uploaded_name);
bool launched = game_launcher->select_game_by_name(last_uploaded_name); bool launched = game_launcher->select_game_by_name(last_uploaded_name);
@@ -163,6 +166,11 @@ void SerialUploader::launch_game() {
} else { } else {
printf("ERROR Failed to launch game: %s\n", last_uploaded_name); printf("ERROR Failed to launch game: %s\n", last_uploaded_name);
} }
// Reset state back to IDLE
reset();
return launched;
} }
bool SerialUploader::process() { bool SerialUploader::process() {
@@ -305,6 +313,8 @@ bool SerialUploader::process() {
if (state == WRITING_FILE) { if (state == WRITING_FILE) {
if (write_file_to_sd()) { if (write_file_to_sd()) {
state = LAUNCHING_GAME; state = LAUNCHING_GAME;
// Prepare for launch by scanning games, but don't actually launch yet
launch_game();
} else { } else {
reset(); reset();
} }
@@ -312,9 +322,8 @@ bool SerialUploader::process() {
} }
if (state == LAUNCHING_GAME) { if (state == LAUNCHING_GAME) {
launch_game(); // Stay in this state until main loop calls complete_launch() when safe
reset(); return false;
return true; // Signal that game was launched
} }
return false; return false;

View File

@@ -14,6 +14,13 @@ public:
// Returns true if a game was launched // Returns true if a game was launched
bool process(); 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) // Get the filename of the last uploaded game (without .lua extension)
const char* get_last_uploaded_filename() const { return last_uploaded_name; } const char* get_last_uploaded_filename() const { return last_uploaded_name; }