feat: add multi-file SD card storage and new sync method

[TuEmb]

Solved #4429 #4351 #4213 #4101 #4052

• The file name now will be with hex format timestamp. eg: 697C6B52.txt, 697C6B6A.txt, ...
• Need to sync the timestamp to use offline mode or the red led will blink with 1Hz frequency.
• New file sync method for multi files

[gemini-code-assist]

Code Review

This pull request introduces a significant and complex feature: multi-file SD card storage using timestamped filenames and a new synchronization protocol. The changes are extensive, touching storage, SD card, and RTC logic. While the overall direction is good, I've identified several critical and high-severity issues that need to be addressed. A critical bug prevents audio from being recorded before the RTC is synced, leading to data loss. Additionally, there are bugs related to incomplete data synchronization and incorrect file handling due to case sensitivity in filenames, which will cause files to be missed during sync and statistics reporting.

[TuEmb]

/gemini

[gemini-code-assist]

Thank you for submitting this pull request, TuEmb! I've reviewed the changes, and it's a significant and well-structured update, moving from a single audio file storage to a multi-file system with timestamp-based filenames, file rotation, and improved sync capabilities. The use of a worker thread and message queue for SD card operations is a good practice for embedded systems, ensuring responsiveness and proper resource management.

Overall, the changes introduce robust multi-file handling, which is a great improvement for offline storage and data synchronization. I have a few comments and suggestions to enhance correctness, efficiency, and maintainability.

Summary of Key Improvements:

• Multi-file Storage: Implemented a new system to store audio data across multiple files, named with hex timestamps.
• File Rotation: Added logic to rotate files based on BLE connection duration, preventing single large files.
• Improved Sync Mechanism: Introduced new commands and state management for listing, reading, and deleting individual audio files, enhancing the synchronization process.
• RTC Time Synchronization: Files created without valid RTC time can now be renamed once time is synced, ensuring accurate timestamps.
• Error Handling: Enhanced error handling for SD card operations, including re-opening files after multiple write errors.
• Backward Compatibility: Maintained compatibility for legacy commands while introducing new multi-file features.

Detailed Review Comments:

omi/firmware/omi/src/lib/core/config.h

• AUDIO_BUFFER_SAMPLES Change: The reduction of AUDIO_BUFFER_SAMPLES from 16000 to 12800 (1s to 0.8s) is noted. This likely impacts memory usage or processing latency. Please ensure this change aligns with the audio processing requirements and doesn't introduce any unintended audio glitches or data loss due to smaller buffer sizes, especially during high-load scenarios.

omi/firmware/omi/src/lib/core/sd_card.h

• MAX_FILENAME_LEN: The MAX_FILENAME_LEN is set to 32. Given the hex timestamp format (08X.txt is 12 characters), this provides ample buffer. However, it's good practice to explicitly state the maximum expected length in the comment for clarity, e.g., // 8 hex chars + ".txt" + null terminator = 13 chars.
• FILE_ROTATION_INTERVAL_MS: Defining this as (30 * 60 * 1000) is clear. Consider adding a comment about why 30 minutes was chosen (e.g., to limit file size, improve sync granularity, etc.).
• sd_offset_info_t: This new struct is well-defined for storing the read offset information. It correctly includes the filename, which is crucial for multi-file reading.

omi/firmware/omi/src/lib/core/storage.c

• STORAGE_BUFFER_SIZE Calculation: The new STORAGE_BUFFER_SIZE calculation (SD_BLE_SIZE * TCP_CHUNK_COUNT + 5 * TCP_CHUNK_COUNT) is a bit opaque. The 5 * TCP_CHUNK_COUNT part is not immediately clear. Could you add a comment explaining the 5 bytes (e.g., 1 byte for file_idx + 4 bytes for timestamp)? This would improve readability and maintainability.
• storage_read_characteristic - amount[0]: The comment for amount[0] states // Total size of all files. However, the current implementation of setup_storage_tx calculates remaining_length only for the current_read_filename. If amount[0] is intended to be the total size of all files, then get_audio_file_stats should be used to populate it, which is correctly done. The comment seems to be accurate, but it's worth double-checking the intent of amount[0] in the context of the BLE characteristic read. It seems to be used for backward compatibility, so returning the total size of all files is appropriate.
• setup_storage_tx - remaining_length Calculation: The comment /* For simplicity, calculate remaining from current file */ /* In a full implementation, we'd track across multiple files */ is a good self-awareness note. This implies a potential future improvement. For now, it's acceptable given the complexity of tracking remaining length across multiple files for a single BLE transfer session.
• parse_storage_command - CMD_LIST_FILES Response: For CMD_LIST_FILES, the function returns 0xFF and defers processing to the storage thread. This is a good approach to avoid blocking the BLE callback. Ensure that the send_file_list_response function correctly handles potential BLE connection drops or timeouts if the response is delayed.
• parse_storage_command - Legacy READ_COMMAND: The legacy READ_COMMAND still checks file_num != 1. This is good for backward compatibility, but it might be worth considering if there's any scenario where a legacy client might try to read a non-existent file_num and how that's handled (currently returns INVALID_FILE_SIZE).
• write_to_gatt and write_to_tcp - current_read_filename: The read_audio_data function now takes current_read_filename. This is a crucial change for multi-file support. Ensure that current_read_filename is always correctly set before these functions are called, especially when switching between files during a sync session.
• storage_write - transport_started Flag: The comment /* Only call legacy setup for legacy mode (current_sync_file_index == -1) New protocol (CMD_READ_FILE) already set up via setup_file_transfer() */ clarifies the conditional call to setup_storage_tx. Setting transport_started = 0; immediately after this block is important to prevent re-entry.
• storage_write - WiFi Sync All Logic: The WiFi sync logic for wifi_sync_all_requested is quite comprehensive, including sending a header packet and iterating through files. The k_msleep(100) after sending the header is a good addition to give the app time to process. The logic to skip empty files is also a nice touch for efficiency.
• storage_write - delete_file_index Handling: The deferred deletion of files using delete_file_index and delete_file_by_index is a robust way to handle file system operations in the worker thread. The error handling and notification back to the BLE client are well-implemented.
• storage_write - save_offset Calls: All calls to save_offset now correctly pass current_read_filename and current_read_offset, which is essential for the new multi-file offset tracking.

omi/firmware/omi/src/main.c

• RTC Not Synced LED Warning: The addition of the red LED blinking when RTC is not valid and offline storage is enabled is a great user experience improvement. It clearly indicates to the user that time synchronization is needed for proper file naming.

omi/firmware/omi/src/rtc.c

• sd_update_filename_after_timesync Call: Calling sd_update_filename_after_timesync after rtc_set_utc_time is a critical integration point. This ensures that any files created with temporary uptime-based names are correctly renamed with actual timestamps once the device has a valid time source.

omi/firmware/omi/src/sd_card.c

• sd_unmount - Flush Batch Buffer: Flushing the batch buffer before unmounting is a good safety measure to prevent data loss.
• filename_equals_ignore_case: This helper function is a good addition for robust filename comparison, especially considering FAT filesystem's case-insensitivity for extensions.
• print_audio_files_at_boot: This function is very helpful for debugging and understanding the state of the SD card at startup. It provides valuable logging information.
• try_continue_latest_file: This is a smart optimization to avoid creating excessive small files if recording resumes shortly after a pause. The FILE_CONTINUE_THRESHOLD_SEC is a good configurable parameter.
• create_audio_file_with_timestamp - RTC Check: The rtc_is_valid() check and EAGAIN return are crucial for ensuring files are named correctly. Discarding data if RTC is not valid (in write_to_file) is a reasonable approach to prevent corrupt or unmanageable files.
• flush_batch_buffer - Error Handling: The improved error handling in flush_batch_buffer, including truncating to the last good packet position and re-opening the file after multiple errors, significantly enhances the robustness of SD card writes.
• should_rotate_file: The file rotation logic based on BLE connection duration is a good way to manage file sizes and prevent single large files that are difficult to sync. This helps with maintainability and transfer efficiency.
• sd_update_filename_after_timesync: This function is well-implemented for renaming files. The calculation of correct_timestamp based on elapsed uptime is clever. Flushing and closing the file before renaming, then re-opening, is the correct procedure.
• sd_notify_ble_state - current_file_deleted Handling: The logic to create a new file on BLE disconnect if the current recording file was deleted while connected is a good edge case handling. This ensures continuous recording with proper file management.
• sd_worker_thread - REQ_WRITE_DATA Logic: The REQ_WRITE_DATA handler now includes checks for current_file_deleted, current_filename (to create a new file if none is open), and should_rotate_file(). This centralizes the file management logic effectively.
• sd_worker_thread - REQ_READ_DATA: Opening a new fs_file_t for each read request is safer than using a global fil_data for reads, as it avoids conflicts with ongoing writes. This is a good design choice.
• sd_worker_thread - REQ_CLEAR_AUDIO_DIR: The new implementation correctly iterates and unlinks all files, then resets the offset info and creates a new file. This is a much more thorough clear operation.
• get_audio_file_list_internal: The internal function for getting the file list, including sorting by timestamp, is well-designed. Using qsort for sorting is efficient.

This pull request significantly enhances the SD card functionality, making it more robust and feature-rich. The attention to detail in handling file operations, error conditions, and time synchronization is commendable.

[beastoin]

Hi @TuEmb — could you add test steps or demo evidence (screenshot/video) to the PR description? This is a significant feature (multi-file SD card storage) and reviewers need to see it working. Thanks!