devkitv2-power-optimize #4008
devkitv2-power-optimize #4008
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and beneficial refactoring of the SD card handling logic. By moving all filesystem operations to a dedicated worker thread and using a message queue, the system becomes more robust and responsive, avoiding blocking calls in other threads. The introduction of batched writes and periodic fs_sync calls are good optimizations for performance and data integrity. The power optimizations, such as using k_msleep to avoid busy-waiting, are also well-aligned with the PR's goal.
My review focuses on improving the robustness of this new implementation by adding error handling for critical filesystem and asynchronous operations. I've identified several places where unchecked return values could lead to silent failures and potential data corruption. Addressing these will make the new SD card module much more reliable.
| return; | ||
| } else { | ||
| /* Move to end for append writes by default */ | ||
| res = fs_seek(&fil_data, 0, FS_SEEK_END); |
There was a problem hiding this comment.
The return value of fs_seek() is not checked. If this operation fails, subsequent file operations might occur at an incorrect position, potentially leading to data corruption. A failure here should probably terminate the worker thread as the filesystem state would be unknown.
res = fs_seek(&fil_data, 0, FS_SEEK_END);
if (res < 0) {
LOG_ERR("[SD_WORK] seek to end of data file failed: %d", res);
return;
}| fs_write(&fil_info, &zero, sizeof(zero)); | ||
| fs_sync(&fil_info); |
There was a problem hiding this comment.
The return values of fs_write() and fs_sync() are not checked. If writing or syncing the new zero offset fails, the info.txt file will be out of sync with the actual state, which can lead to data being read incorrectly later. This is critical after clearing the audio directory.
Other filesystem calls in this REQ_CLEAR_AUDIO_DIR handler also have unchecked return values (e.g., fs_close on line 356, fs_seek on lines 366 and 382). Please ensure all filesystem call return values are checked and errors are handled.
ssize_t bw_info = fs_write(&fil_info, &zero, sizeof(zero));
if (bw_info < 0 || bw_info != sizeof(zero)) {
LOG_ERR("[SD_WORK] Failed to write zero offset: %d", (int)bw_info);
} else {
if (fs_sync(&fil_info) < 0) {
LOG_ERR("[SD_WORK] Failed to sync info file after clearing");
}
}| fs_close(&fil_data); | ||
| fs_close(&fil_info); |
There was a problem hiding this comment.
The return values of fs_close() are not checked. If closing a file fails, it might mean that pending data was not written to the storage medium, leading to data loss. This is especially critical during a power-off sequence.
if (fs_close(&fil_data) != 0) {
LOG_ERR("Failed to close data file");
}
if (fs_close(&fil_info) != 0) {
LOG_ERR("Failed to close info file");
}| clear_audio_directory(); | ||
| save_offset(0); | ||
| nuke_started = 0; |
There was a problem hiding this comment.
The return value of clear_audio_directory() is not checked. This function sends a request to a message queue and can fail if the queue is full. If it fails, the 'nuke' operation is silently ignored, but nuke_started is reset, preventing future retries. The flag should only be reset on successful queuing.
if (clear_audio_directory() == 0) {
nuke_started = 0;
} else {
LOG_ERR("Failed to queue nuke request");
}| // k_yield(); | ||
| // continue; | ||
| LOG_INF("no heartbeat sent"); | ||
| save_offset(file_offset); |
There was a problem hiding this comment.
The return value of save_offset() is not checked. If saving the offset fails (e.g., because the message queue is full), the failure is silent. This can lead to the device resuming from an old, incorrect offset after a restart, potentially causing data loss or corruption. This issue also exists on line 290.
if (save_offset(file_offset) != 0) {
LOG_ERR("Failed to queue save_offset on heartbeat timeout");
}
- Implemented a blinking LED sequence for connected and not connected states, improving Battery, - Adjusted Bluetooth advertising parameters to a slower interval (300ms) for power savings, reducing radio activity during advertising.
Before BLE and LED fix:
After BLE and LED fix
|
|
Nice! thanks for your PR @harshithsunku |
Hi @TuEmb , will split to smaller PR's Thanks. |
2 participants
1) Ported PR #3490
2) Ported PR #3959
3) With LED and Bluetooth Advertising Optimization
Battery Runtime Estimation (250 mAh, 3.7 V, Continuous Operation)