sdcard: replace VLA with static buffer, add card presence guard#205
Merged
bessman merged 2 commits intoMay 3, 2026
Merged
Conversation
Contributor
Reviewer's GuideRefactors sdcard.c to remove stack-allocated VLAs in favor of a single static sector buffer, adds an SD card presence guard before mounting the filesystem in all handlers, and centralizes the drive string into a named constant while making minor style/ordering tweaks. Sequence diagram for SD card command handlers with presence guardsequenceDiagram
actor Host
participant MCU
participant UART1
participant SD_SPI
participant FatFs
Host->>MCU: Send command and filename
MCU->>UART1: UART1_Read (get_filename loop)
UART1-->>MCU: Filename characters
MCU->>UART1: UART1_Read (mode or request params)
UART1-->>MCU: Mode or params
MCU->>SD_SPI: SD_SPI_IsMediaPresent
SD_SPI-->>MCU: Card present?
alt Card not present
MCU->>UART1: UART1_Write FR_NOT_READY
MCU-->>Host: Return DO_NOT_BOTHER
else Card present
MCU->>FatFs: f_mount FATFS SDCARD_DRIVE 1
FatFs-->>MCU: Mount result
opt Write_file handler
loop For each data block
MCU->>UART1: UART1_Read block into s_sector_buf
UART1-->>MCU: Block bytes
MCU->>FatFs: f_write file s_sector_buf block_size
FatFs-->>MCU: Write result and bytes_written
end
end
opt Read_file handler
loop For each data block
MCU->>FatFs: f_read file s_sector_buf block_size
FatFs-->>MCU: Read result and bytes_read
MCU->>UART1: UART1_Write bytes from s_sector_buf
UART1-->>Host: Block bytes
end
end
opt Get_file_info handler
MCU->>FatFs: f_stat filename FILINFO
FatFs-->>MCU: Populate FILINFO
MCU->>UART1: UART1_Write info fields
end
MCU->>FatFs: f_close file (if opened)
FatFs-->>MCU: Close result
MCU->>FatFs: f_mount 0 SDCARD_DRIVE 0
FatFs-->>MCU: Unmount result
MCU-->>Host: Command response complete
end
Flow diagram for block-based I/O using static sector bufferflowchart TD
A[Start handler with total_size] --> B[Set remaining = total_size]
B --> C{remaining > 0?}
C -->|No| Z[End loop]
C -->|Yes| D[Set block_size = remaining if remaining < BUF_MAX else BUF_MAX]
D --> E[Clear watchdog timer]
E --> F[For i from 0 to block_size - 1 read byte from UART1 into s_sector_buf for write or prepare for read]
F --> G{Write_file handler?}
G -->|Yes| H[Call f_write with s_sector_buf and block_size]
H --> I[Increment bytes_written by written]
G -->|No| J[Call f_read into s_sector_buf with block_size]
J --> K[Increment bytes_read by read]
I --> L[For i from 0 to block_size - 1 write s_sector_buf_i to UART1 for read response]
K --> L
L --> M[remaining = remaining - block_size]
M --> C
Z --> N[Send final byte count and close file]
N --> O[Unmount drive and return response]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_Static_assertons_sector_bufhardcodes512rather than asserting againstBUF_MAX, which will cause an unnecessary compile-time failure ifFF_MIN_SSis ever changed to a different valid sector size; consider assertingsizeof(s_sector_buf) == BUF_MAXinstead. - Using a single global
s_sector_buffor both read and write paths makes these handlers non-reentrant and unsafe for concurrent use (e.g., from multiple tasks/contexts); if concurrent access is possible, consider adding serialization or documenting this constraint explicitly in the module.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_Static_assert` on `s_sector_buf` hardcodes `512` rather than asserting against `BUF_MAX`, which will cause an unnecessary compile-time failure if `FF_MIN_SS` is ever changed to a different valid sector size; consider asserting `sizeof(s_sector_buf) == BUF_MAX` instead.
- Using a single global `s_sector_buf` for both read and write paths makes these handlers non-reentrant and unsafe for concurrent use (e.g., from multiple tasks/contexts); if concurrent access is possible, consider adding serialization or documenting this constraint explicitly in the module.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the SD card UART command handlers in src/sdcard/sdcard.c by reducing stack usage and avoiding hangs when no SD card is inserted.
Changes:
- Replaces per-iteration variable-length sector buffers with a single static 512-byte buffer.
- Adds an SD card presence guard (
SD_SPI_IsMediaPresent()) before attempting to mount. - Introduces
SDCARD_DRIVEto centralize the FatFS drive path string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
bessman
approved these changes
May 3, 2026
bessman
left a comment
bessman
left a comment
Collaborator
There was a problem hiding this comment.
LGTM.
Note that SD_SPI_IsMediaPresent does not actually check media presence; it always returns true, so the added check currently adds no functionality. However, it is still good to have it in case SD_SPI_IsMediaPresent is properly implemented in the future.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BYTE const mode = UART1_Read(); | ||
|
|
||
| if (!SD_SPI_IsMediaPresent()) { | ||
| return FR_NOT_READY; |
| get_filename(filename, sizeof filename); | ||
|
|
||
| if (!SD_SPI_IsMediaPresent()) { | ||
| UART1_Write(FR_NOT_READY); |
Comment on lines
+102
to
+110
| FRESULT read_result = f_read(&file, s_sector_buf, (UINT)block_size, &read); | ||
| bytes_read += read; | ||
|
|
||
| for (UINT i = 0; i < block_size; ++i) { | ||
| UART1_Write(buffer[i]); | ||
| for (UINT i = 0; i < read; ++i) { | ||
| UART1_Write(s_sector_buf[i]); | ||
| } | ||
|
|
||
| if (read_result != FR_OK || read < block_size) { | ||
| break; |
Comment on lines
+19
to
+23
| static TCHAR s_sector_buf[BUF_MAX]; | ||
| _Static_assert( | ||
| sizeof(s_sector_buf) == 512, | ||
| "SD sector buffer must be 512 bytes" | ||
| ); |
Comment on lines
+25
to
28
| static void get_filename(TCHAR *const fn_buf, UINT const size) | ||
| { | ||
| for (UINT i = 0; i < size; ++i) { | ||
| if (!(fn_buf[i] = UART1_Read())) { |
| TCHAR filename[SFN_MAX + SFN_SUFFIX_LEN + 1]; // +1 is null-terminator. | ||
| get_filename(filename, sizeof filename); | ||
|
|
||
| if (!SD_SPI_IsMediaPresent()) { |
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
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.
This PR fixes three code quality issues in src/sdcard/sdcard.c identified
during codebase analysis for the standalone SD logging project.
Changes
1. Replace VLA with static sector buffer
TCHAR buffer[block_size]in the read and write loops is a variable-lengtharray (VLA) allocated on the stack at runtime. On PIC24EP256GP204 with limited
stack space, this risks silent stack overflow if block_size is unexpectedly
large. Replaced with a single static 512-byte buffer
s_sector_bufin .bss.A
_Static_assertis added to catch any future size mismatch at compile time.2. Add SD card presence check before f_mount
All three command handlers called
f_mountunconditionally. If no SD card isinserted, the SPI driver waits for responses that never arrive.
SD_SPI_IsMediaPresent()already exists in sd_spi.c — this PR uses it as aguard at the top of each handler, returning FR_NOT_READY to the host
immediately if no card is present.
3. Consistent drive path via named constant
Introduce
#define SDCARD_DRIVE "0:"and replace all inline"0:"strings.This makes the drive path a single source of truth.
No functional change
All existing commands (write_file, read_file, get_file_info) behave
identically when an SD card is present. The only behavioural change is
a fast FR_NOT_READY response when no card is inserted, instead of hanging.
Summary by Sourcery
Guard SD card file operations with a media presence check and replace stack-allocated variable-length sector buffers with a shared static buffer.
Bug Fixes:
Enhancements: