fixed disconnect broadcast bug, firmware side offset capture, large packet sending #994
Conversation
kevvz
changed the title
WalkthroughThe changes encompass a series of modifications across various files in the firmware and application layers, focusing on enhancing Bluetooth capabilities, memory management, and file handling. Key updates include renaming device identifiers, adjusting configuration settings, and refining initialization logic. New functions for managing audio file offsets are introduced, while existing functions are optimized for better error handling and performance. The application layer sees improvements in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (17)
Friend/firmware/firmware_v1.0/src/sdcard.h (1)
96-97: Approve new offset functions, but suggest improvements.The addition of
save_offsetandget_offsetfunctions aligns well with the PR objective of allowing the device to manage offsets during storage transmission. These functions will enhance the firmware's capability to handle discontinuous storage transmission efficiently.However, I have a few suggestions for improvement:
Add documentation comments for these new functions, following the style used for other functions in this file. This will maintain consistency and improve clarity for developers.
Consider reorganizing the placement of these functions within the file. Grouping related functions together (e.g., with other file operation functions) might improve readability and maintainability.
Here's an example of how you could document these functions:
/** * @brief Save the current offset value * * Stores the provided offset value for later retrieval. * * @param offset The offset value to save * @return 0 if successful, negative errno code if error */ int save_offset(uint32_t offset); /** * @brief Retrieve the previously saved offset value * * @return The saved offset value if successful, negative errno code if error */ int get_offset();Friend/firmware/firmware_v1.0/src/main.c (1)
Line range hint
207-224: Improved code structure and future feature consideration.The addition of empty lines enhances code readability. Regarding the commented-out NFC initialization code:
- It's good to keep this code for future reference.
- Consider adding a TODO comment explaining why NFC is currently disabled and under what conditions it should be re-enabled.
Example:
// TODO: NFC initialization is currently disabled. // Re-enable when [specific condition] is met or [feature] is implemented.This will help future developers understand the status of the NFC feature.
Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv2-adafruit.conf (2)
129-129: Typographical error in commentThe comment on line 129 contains a spelling mistake. Correcting it improves readability.
Apply the following change:
-#nessessary? +# Necessary?
178-180: Commented out USB and Flash configurationsThe configurations for USB device support and flash memory access are currently commented out:
# CONFIG_NRFX_USBD=y# CONFIG_FLASH=yIf these functionalities are required for your application, consider uncommenting these lines to enable them.
Ensure that only the necessary features are enabled to optimize resource usage.
backend/routers/sdcard.py (4)
66-66: Remove unnecessaryprint()statement.Line 66 contains an empty
print()function call, which serves no purpose and can clutter the output.Apply this diff to remove it:
- print()
70-71: Clarify the intent of commented-out code or remove it.The line adding to
audio_framesis commented out. If this code is no longer needed, consider removing it. If it's temporarily disabled, add a comment explaining why.
176-176: Correct formatting for the comment.Line 176 appears to be a comment but lacks a space after
#. For consistency and readability, add a space.Apply this diff:
-#data_packet_size +# data_packet_size
181-182: Remove unnecessary initialization ofcurrent_packet_size.The variable
current_packet_sizeis initialized to0before the loop and then immediately reassigned within the loop. The initial assignment is redundant.Apply this diff:
max_packet_size_idx = 440 - 1 - current_packet_size = 0 if not socket_segments: return []Friend/firmware/firmware_v1.0/src/sdcard.c (4)
104-104: Redundant Call toinitialize_audio_file(1)The function
initialize_audio_file(1)is called twice withinmount_sd_card(). This redundancy may lead to unnecessary operations or unintended side effects.Consider removing the redundant call to ensure the function is only initialized once.
- initialize_audio_file(1);
158-158: Remove Trailing Newline in Log MessageThe
LOG_ERRmessage contains a trailing newline character\n, which is unnecessary as logging functions typically handle line endings.- LOG_ERR("invalid file in get file size\n"); + LOG_ERR("invalid file in get file size");
173-173: Remove Trailing Newline in Log MessageSimilar to the previous comment, remove the unnecessary newline character.
- LOG_ERR("invalid file in move read ptr\n"); + LOG_ERR("invalid file in move read ptr");
188-188: Remove Trailing Newline in Log MessageAgain, remove the unnecessary newline character for consistency.
- LOG_ERR("invalid file in move write pointer\n"); + LOG_ERR("invalid file in move write pointer");Friend/firmware/firmware_v1.0/src/storage.c (2)
242-266: Remove Commented-Out Code to Clean Up the CodebaseThe code from lines 242 to 266 is commented out. Keeping old code commented can clutter the codebase and may cause confusion. Since version control systems like Git preserve the history of code changes, it's safe to remove this code to maintain cleanliness.
332-337: Enhance Heartbeat Handling for Connection StabilityWhen
heartbeat_countreachesMAX_HEARTBEAT_FRAMES, the current implementation only prints a message and saves the offset. To improve connection stability and user experience, consider implementing a reconnection strategy or notifying other system components to handle the lost heartbeat appropriately.app/lib/providers/capture_provider.dart (2)
Line range hint
517-535: Redundant Assignments tototalBytesReceivedWithin
initiateStorageBytesStreaming(),totalBytesReceivedis initially assigned based on conditions involvingpreviousStorageBytesandSharedPreferencesUtil().currentStorageBytes(lines 521-527). However, at line 531,totalBytesReceivedis overwritten withstorageOffset, making the previous assignments redundant. Consider reviewing this logic to ensure it reflects the intended behavior and remove any unnecessary assignments.
609-614: Use Constants Instead of Magic Numbers for Packet SizesIn the
sendStoragemethod, hardcoded numbers like80and83are used for packet size comparisons and calculations. Using magic numbers can make the code less readable and maintainable. Consider defining constants with descriptive names for these values to improve clarity.Suggested code modification:
const int minPacketSize = 80; const int packetSizeWithHeader = 83; ... } else if (value.length >= minPacketSize) { // enforce a minimum packet size if (value.length == packetSizeWithHeader) { totalBytesReceived += minPacketSize; } else { totalBytesReceived += value.length; }Friend/firmware/firmware_v1.0/src/transport.c (1)
23-23: Consider removing commented-out include directive.The line
// #include "friend.h"is commented out. Iffriend.his no longer needed, consider removing the commented code to keep the codebase clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv2-adafruit.conf (4 hunks)
- Friend/firmware/firmware_v1.0/src/main.c (5 hunks)
- Friend/firmware/firmware_v1.0/src/sdcard.c (7 hunks)
- Friend/firmware/firmware_v1.0/src/sdcard.h (1 hunks)
- Friend/firmware/firmware_v1.0/src/speaker.c (4 hunks)
- Friend/firmware/firmware_v1.0/src/storage.c (11 hunks)
- Friend/firmware/firmware_v1.0/src/transport.c (8 hunks)
- app/lib/providers/capture_provider.dart (5 hunks)
- backend/routers/sdcard.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (16)
Friend/firmware/firmware_v1.0/src/main.c (5)
41-42: LGTM: Improved code readability.The addition of these empty lines helps to separate logical sections of the code, enhancing overall readability.
90-93: Improved LED state management for charging status.This change ensures that the green LED is turned off when the device is not charging, providing a clear visual indication of the charging status. This enhancement aligns well with the PR objective of improving device efficiency and user feedback.
166-166: LGTM: Improved code structure.The addition of this empty line helps to separate logical sections of the code, enhancing overall readability.
182-182: Improved initialization sequence.Moving the
play_haptic_milli(500)call to occur after the codec initialization ensures that haptic feedback is provided only when the device is fully ready. This change enhances the user experience and aligns with the PR objective of refining the initialization flow.
Line range hint
1-238: Overall assessment: Changes align well with PR objectives.The modifications in this file effectively address the PR objectives by:
- Enhancing LED state management for better user feedback.
- Refining the initialization sequence, particularly for haptic feedback.
- Improving code readability and structure.
- Keeping future features (like NFC) in consideration while focusing on current priorities.
These changes contribute to a more efficient and user-friendly firmware. Great job on the improvements!
Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv2-adafruit.conf (4)
19-19: Device name updated to "Omi DevKit 2"The change to
CONFIG_BT_DEVICE_NAME="Omi DevKit 2"reflects the new branding and ensures the device is correctly identified when connecting via Bluetooth.
40-40: Model name updated to "Omi DevKit 2"Updating
CONFIG_BT_DIS_MODELto "Omi DevKit 2" maintains consistency across the Device Information Service and aligns with the updated device name.
44-44: Firmware revision updated to "2.0.2"The firmware revision string
CONFIG_BT_DIS_FW_REV_STR="2.0.2"has been updated appropriately to reflect the new version of the firmware.
55-60: Enhanced Bluetooth capabilities enabledThe addition of advanced Bluetooth configurations enhances the device's performance:
CONFIG_BT_CTLR_DATA_LENGTH_MAX=251CONFIG_BT_CTLR_PHY_2M=yCONFIG_BT_CTLR_PHY_CODED=yCONFIG_BT_CTLR_SDC_MAX_CONN_EVENT_LEN_DEFAULT=400000CONFIG_BT_CONN_TX_MAX=20CONFIG_BT_BUF_ACL_RX_SIZE=1024Ensure that these settings are compatible with the hardware capabilities and that the increased data lengths and PHY options are supported by all intended connected devices.
Friend/firmware/firmware_v1.0/src/speaker.c (1)
107-107: Confirm intentional duplication of audio samplesIn lines 107 and 121, you're writing each sample twice:
*ptr2++ = ((int16_t *)buf)[i]; *ptr2++ = ((int16_t *)buf)[i];If the goal is to duplicate each sample for stereo channels, this is acceptable. Otherwise, this may unintentionally double the data. Please confirm if this duplication is intended.
Also applies to: 121-121
Friend/firmware/firmware_v1.0/src/sdcard.c (1)
140-140: Verify Error Handling forsave_offset(0)Calling
save_offset(0)without checking its return value might overlook potential errors during the offset saving process. It's important to verify whether the operation was successful.Ensure to handle possible errors:
- save_offset(0); + if (save_offset(0) != 0) { + LOG_ERR("Failed to save offset"); + return -1; + }Friend/firmware/firmware_v1.0/src/transport.c (5)
9-9: Include necessary header for Bluetooth HCI functions.The addition of
<zephyr/bluetooth/hci.h>is appropriate if HCI functions are used elsewhere in the code.
67-70: Conditional inclusion of speaker characteristic based on configuration.The speaker characteristic is now conditionally compiled with
#ifdef CONFIG_ENABLE_SPEAKER. This allows for the dynamic inclusion of speaker functionality based on the build configuration.
541-542: Ensure consistent usage of updatedMAX_WRITE_SIZE.
MAX_WRITE_SIZEhas been increased from 400 to 440, andstorage_temp_datais now adjusted accordingly. Verify that all operations relying onMAX_WRITE_SIZEare updated to prevent buffer overflows.
710-710: Confirm necessity of registeringstorage_service.The line
bt_gatt_service_register(&storage_service);is commented out. If thestorage_serviceis still required for storage functionality, ensure it is registered appropriately.
29-29:⚠️ Potential issueVerify reduction of
file_num_arraysize to prevent out-of-bounds access.The size of
file_num_arrayhas been reduced from 40 to 2. Ensure that all accesses to this array do not exceed the new size to prevent potential out-of-bounds errors.Run the following script to verify that all accesses to
file_num_arrayare within bounds:✅ Verification successful
All accesses to
file_num_arrayare within the new bounds.No out-of-bounds accesses detected after reducing the size of
file_num_arrayto 2.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any array accesses where the index of `file_num_array` is 2 or greater. # Test: Search for potential out-of-bounds accesses. rg --type c --pcre2 'file_num_array\s*\[\s*(\d+|[^\]]+)\s*\]' | grep -E 'file_num_array\s*\[\s*([2-9]\d*|[^\]]+)\s*\]' # Expected result: No matches, indicating all indices are less than 2.Length of output: 355
| CONFIG_MAIN_STACK_SIZE=2048 | ||
| # CONFIG_DISK_DRIVER_SDMMC=y | ||
| # CONFIG_SPI=y | ||
| CONFIG_HEAP_MEM_POOL_SIZE=3048 | ||
| CONFIG_HEAP_MEM_POOL_SIZE=512 |
There was a problem hiding this comment.
Potential risks due to reduced stack and heap sizes
Reducing the main stack size and heap memory pool size may lead to stack overflows or heap allocation failures if the application demands exceed the allocated memory:
CONFIG_MAIN_STACK_SIZE=2048(reduced from 5096)CONFIG_HEAP_MEM_POOL_SIZE=512(reduced from 3048)
Consider thoroughly testing the firmware to ensure that all tasks function correctly within the new memory constraints. Pay special attention to memory-intensive operations that may now cause issues.
| @@ -31,6 +31,7 @@ async def sdcard_streaming_endpoint(websocket: WebSocket, uid: str): | |||
| big_file_path = f"_temp/_temp{session_id}.wav" | |||
| first_packet_flag = False | |||
| data_packet_length = 83 | |||
| # data_packet_length = 440 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing commented-out code for clarity.
The line defining data_packet_length = 440 is commented out. If this line is no longer needed, consider removing it to keep the codebase clean and maintainable.
| if not first_packet_flag: | ||
| first_packet_flag = True | ||
| print('first valid packet received') | ||
| max_packet_size_idx = 440 - 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Variable max_packet_size_idx is redefined; consider using a constant.
The variable max_packet_size_idx is defined multiple times in the code. To avoid redundancy and potential errors, define it once at the module level or as a constant.
Define MAX_PACKET_SIZE_IDX at the top of the file:
MAX_PACKET_SIZE_IDX = 440 - 1Then replace instances of max_packet_size_idx with MAX_PACKET_SIZE_IDX.
| current_packet_size = socket_segments[offset] | ||
| if (current_packet_size + offset > max_packet_size_idx): |
There was a problem hiding this comment.
Handle zero current_packet_size to avoid infinite loops.
If current_packet_size is zero, the code may enter an infinite loop or experience unexpected behavior. It's important to handle this scenario explicitly.
Add a condition to check for zero packet size:
while True:
if offset >= len(socket_segments):
break
current_packet_size = socket_segments[offset]
+ if current_packet_size == 0:
+ breakCommittable suggestion was skipped due to low confidence.
| elif len(data) == 440: # the other packet version | ||
| if not first_packet_flag: | ||
| first_packet_flag = True | ||
| print('first valid packet received') | ||
| max_packet_size_idx = 440 - 1 | ||
| # print(i) | ||
| socket_segments = data | ||
| current_packet_size = 0 | ||
| offset = 0 | ||
| while True: | ||
| current_packet_size = socket_segments[offset] | ||
|
|
||
| print() | ||
| if (current_packet_size + offset > max_packet_size_idx): | ||
| break | ||
| elif (current_packet_size + offset == max_packet_size_idx): | ||
| # audio_frames.append(socket_segments[offset+1:offset+current_packet_size+1]) | ||
| break | ||
| elif (current_packet_size + offset < max_packet_size_idx): | ||
| audio_frames.append(socket_segments[offset+1:offset+current_packet_size+1]) | ||
| offset+=current_packet_size+1 | ||
| continue | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate duplicated logic and improve modularity.
The logic within this block for handling packets of length 440 is complex and mirrors the functionality of the split_segments function defined later. Duplicating code can lead to maintenance challenges and potential inconsistencies.
Apply this diff to utilize the split_segments function:
elif len(data) == 440: # the other packet version
if not first_packet_flag:
first_packet_flag = True
print('first valid packet received')
- max_packet_size_idx = 440 - 1
- # print(i)
- socket_segments = data
- current_packet_size = 0
- offset = 0
- while True:
- current_packet_size = socket_segments[offset]
-
- print()
- if (current_packet_size + offset > max_packet_size_idx):
- break
- elif (current_packet_size + offset == max_packet_size_idx):
- # audio_frames.append(socket_segments[offset+1:offset+current_packet_size+1])
- break
- elif (current_packet_size + offset < max_packet_size_idx):
- audio_frames.append(socket_segments[offset+1:offset+current_packet_size+1])
- offset+=current_packet_size+1
- continue
+ segments = split_segments(data)
+ for segment in segments:
+ audio_frames.append(segment)This refactoring enhances code reuse and readability by leveraging the existing split_segments function.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif len(data) == 440: # the other packet version | |
| if not first_packet_flag: | |
| first_packet_flag = True | |
| print('first valid packet received') | |
| max_packet_size_idx = 440 - 1 | |
| # print(i) | |
| socket_segments = data | |
| current_packet_size = 0 | |
| offset = 0 | |
| while True: | |
| current_packet_size = socket_segments[offset] | |
| print() | |
| if (current_packet_size + offset > max_packet_size_idx): | |
| break | |
| elif (current_packet_size + offset == max_packet_size_idx): | |
| # audio_frames.append(socket_segments[offset+1:offset+current_packet_size+1]) | |
| break | |
| elif (current_packet_size + offset < max_packet_size_idx): | |
| audio_frames.append(socket_segments[offset+1:offset+current_packet_size+1]) | |
| offset+=current_packet_size+1 | |
| continue | |
| elif len(data) == 440: # the other packet version | |
| if not first_packet_flag: | |
| first_packet_flag = True | |
| print('first valid packet received') | |
| segments = split_segments(data) | |
| for segment in segments: | |
| audio_frames.append(segment) |
|
|
||
| void setsdCardReady(bool value) { | ||
| sdCardReady = value; |
There was a problem hiding this comment.
Method Name setsdCardReady Does Not Follow Naming Conventions
The method name setsdCardReady should use lowerCamelCase according to Dart naming conventions. Consider renaming it to setSdCardReady for consistency.
Suggested code modification:
-void setsdCardReady(bool value) {
+void setSdCardReady(bool value) {
sdCardReady = value;
notifyListeners();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void setsdCardReady(bool value) { | |
| sdCardReady = value; | |
| void setSdCardReady(bool value) { | |
| sdCardReady = value; | |
| notifyListeners(); | |
| } |
| totalStorageFileBytes = currentStorageFiles[0]; | ||
| var storageOffset = currentStorageFiles[1]; |
There was a problem hiding this comment.
Potential Index Out of Range Exception when accessing currentStorageFiles
In initiateStorageBytesStreaming(), accessing currentStorageFiles[0] and currentStorageFiles[1] without checking if the list has at least two elements may cause an IndexError. Ensure that currentStorageFiles has at least two elements before accessing these indices.
Suggested code modification:
if (currentStorageFiles.length >= 2) {
totalStorageFileBytes = currentStorageFiles[0];
var storageOffset = currentStorageFiles[1];
} else {
debugPrint('Storage file list does not contain enough elements');
// Handle this case appropriately
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| totalStorageFileBytes = currentStorageFiles[0]; | |
| var storageOffset = currentStorageFiles[1]; | |
| if (currentStorageFiles.length >= 2) { | |
| totalStorageFileBytes = currentStorageFiles[0]; | |
| var storageOffset = currentStorageFiles[1]; | |
| } else { | |
| debugPrint('Storage file list does not contain enough elements'); | |
| // Handle this case appropriately | |
| return; | |
| } |
| Future<void> updateStorageList() async { | ||
| currentStorageFiles = await _getStorageList(_recordingDevice!.id); | ||
| if (currentStorageFiles.isEmpty) { | ||
| debugPrint('No storage files found'); | ||
| SharedPreferencesUtil().deviceIsV2 = false; | ||
| debugPrint('Device is not V2'); | ||
|
|
||
| return; | ||
| } | ||
| totalStorageFileBytes = currentStorageFiles[0]; | ||
| var storageOffset = currentStorageFiles[1]; | ||
| totalBytesReceived = storageOffset; | ||
| notifyListeners(); | ||
| } |
There was a problem hiding this comment.
Potential Index Out of Range Exception in updateStorageList()
In the updateStorageList() method, accessing currentStorageFiles[0] and currentStorageFiles[1] without ensuring that the list contains at least two elements may cause an IndexError if the list has fewer than two elements. Consider adding a check to ensure the list has at least two elements before accessing these indices.
Suggested code modification:
Future<void> updateStorageList() async {
currentStorageFiles = await _getStorageList(_recordingDevice!.id);
if (currentStorageFiles.isEmpty) {
debugPrint('No storage files found');
SharedPreferencesUtil().deviceIsV2 = false;
debugPrint('Device is not V2');
return;
}
+ if (currentStorageFiles.length < 2) {
+ debugPrint('Storage file list does not contain enough elements');
+ // Handle this case appropriately
+ return;
+ }
totalStorageFileBytes = currentStorageFiles[0];
var storageOffset = currentStorageFiles[1];
totalBytesReceived = storageOffset;
notifyListeners();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<void> updateStorageList() async { | |
| currentStorageFiles = await _getStorageList(_recordingDevice!.id); | |
| if (currentStorageFiles.isEmpty) { | |
| debugPrint('No storage files found'); | |
| SharedPreferencesUtil().deviceIsV2 = false; | |
| debugPrint('Device is not V2'); | |
| return; | |
| } | |
| totalStorageFileBytes = currentStorageFiles[0]; | |
| var storageOffset = currentStorageFiles[1]; | |
| totalBytesReceived = storageOffset; | |
| notifyListeners(); | |
| } | |
| Future<void> updateStorageList() async { | |
| currentStorageFiles = await _getStorageList(_recordingDevice!.id); | |
| if (currentStorageFiles.isEmpty) { | |
| debugPrint('No storage files found'); | |
| SharedPreferencesUtil().deviceIsV2 = false; | |
| debugPrint('Device is not V2'); | |
| return; | |
| } | |
| if (currentStorageFiles.length < 2) { | |
| debugPrint('Storage file list does not contain enough elements'); | |
| // Handle this case appropriately | |
| return; | |
| } | |
| totalStorageFileBytes = currentStorageFiles[0]; | |
| var storageOffset = currentStorageFiles[1]; | |
| totalBytesReceived = storageOffset; | |
| notifyListeners(); | |
| } |
| uint16_t amount = 400; | ||
| int16_t *int16_buf = (int16_t *)buf; | ||
| uint8_t *data = (uint8_t *)buf; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variables and clarify the logic in audio_data_write_handler.
- The variables
int16_bufanddataare declared but not used. - The variable
amountis initialized to400, used inbt_gatt_notify(), and then overwritten byspeak(len, buf);. This might lead to confusion about the intended value ofamount.
Apply this diff to remove unused variables and clarify amount usage:
static ssize_t audio_data_write_handler(struct bt_conn *conn, const struct bt_gatt_attr *attr, const void *buf, uint16_t len, uint16_t offset, uint8_t flags)
{
- uint16_t amount = 400;
- int16_t *int16_buf = (int16_t *)buf;
- uint8_t *data = (uint8_t *)buf;
- bt_gatt_notify(conn, attr, &amount, sizeof(amount));
- amount = speak(len, buf);
+ uint16_t amount = speak(len, buf);
+ bt_gatt_notify(conn, attr, &amount, sizeof(amount));
return len;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint16_t amount = 400; | |
| int16_t *int16_buf = (int16_t *)buf; | |
| uint8_t *data = (uint8_t *)buf; | |
| uint16_t amount = speak(len, buf); | |
| bt_gatt_notify(conn, attr, &amount, sizeof(amount)); | |
| return len; |
| bool write_to_storage(void) {//max possible packing | ||
| if (!read_from_tx_queue()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| uint8_t *buffer = tx_buffer+2; | ||
| uint8_t packet_size = (uint8_t)(tx_buffer_size + OPUS_PREFIX_LENGTH); | ||
|
|
||
| // buffer_offset = buffer_offset+amount_to_fill; | ||
| //check if adding the new packet will cause a overflow | ||
| if(buffer_offset + packet_size > MAX_WRITE_SIZE-1) | ||
| { | ||
|
|
||
| storage_temp_data[buffer_offset] = tx_buffer_size; | ||
| uint8_t *write_ptr = storage_temp_data; | ||
| write_to_file(write_ptr,MAX_WRITE_SIZE); | ||
|
|
||
| buffer_offset = packet_size; | ||
| storage_temp_data[0] = tx_buffer_size; | ||
| memcpy(storage_temp_data + 1, buffer, tx_buffer_size); | ||
|
|
||
| } | ||
| else if (buffer_offset + packet_size == MAX_WRITE_SIZE-1) | ||
| { //exact frame needed | ||
| storage_temp_data[buffer_offset] = tx_buffer_size; | ||
| memcpy(storage_temp_data + buffer_offset + 1, buffer, tx_buffer_size); | ||
| buffer_offset = 0; | ||
| uint8_t *write_ptr = (uint8_t*)storage_temp_data; | ||
| write_to_file(write_ptr,MAX_WRITE_SIZE); | ||
|
|
||
| } | ||
| else | ||
| { | ||
| storage_temp_data[buffer_offset] = tx_buffer_size; | ||
| memcpy(storage_temp_data+ buffer_offset+1, buffer, tx_buffer_size); | ||
| buffer_offset = buffer_offset + packet_size; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review and refactor buffer handling in write_to_storage.
The new implementation aims to maximize data packing for BLE bandwidth improvement. Please ensure:
- Buffer offsets and sizes are correctly calculated to prevent overflows.
- All edge cases are handled properly to avoid data loss or corruption.
- Code readability is enhanced for easier future maintenance.
Consider refactoring the code for clarity:
bool write_to_storage(void) { // Maximize packing
if (!read_from_tx_queue()) {
return false;
}
uint8_t *buffer = tx_buffer + 2;
uint8_t packet_size = (uint8_t)(tx_buffer_size + OPUS_PREFIX_LENGTH);
// Check if adding the new packet will cause an overflow
if (buffer_offset + packet_size > MAX_WRITE_SIZE - 1) {
// Write out the current buffer
- storage_temp_data[buffer_offset] = tx_buffer_size;
write_to_file(storage_temp_data, buffer_offset);
// Start a new buffer
buffer_offset = 0;
}
+ // Add packet size at the current buffer offset
+ storage_temp_data[buffer_offset] = tx_buffer_size;
+ memcpy(storage_temp_data + buffer_offset + 1, buffer, tx_buffer_size);
+ buffer_offset += packet_size;
// If buffer is full after addition, write it out
if (buffer_offset >= MAX_WRITE_SIZE - 1) {
write_to_file(storage_temp_data, buffer_offset);
buffer_offset = 0;
}
return true;
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Friend/firmware/firmware_v1.0/src/sdcard.c (7 hunks)
🧰 Additional context used
🔇 Additional comments (1)
Friend/firmware/firmware_v1.0/src/sdcard.c (1)
392-417: Improved Error Handling insave_offsetThe
save_offsetfunction now includes proper error checking and handling, enhancing the reliability of file operations.
| @@ -153,7 +155,7 @@ uint32_t get_file_size(uint8_t num) | |||
| int res = fs_stat(¤t_full_path,&entry); | |||
| if (res) | |||
| { | |||
| LOG_ERR("invalid file\n"); | |||
| LOG_ERR("invalid file in get file size\n"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Specificity of Error Messages
The error messages at lines 158, 173, and 188 are generic and may not provide sufficient context for debugging.
Enhance the error messages to include the file paths or operations that failed for better clarity.
- LOG_ERR("invalid file in get file size\n");
+ LOG_ERR("Failed to get file size for %s", current_full_path);
- LOG_ERR("invalid file in move read ptr\n");
+ LOG_ERR("Failed to move read pointer to %s", read_buffer);
- LOG_ERR("invalid file in move write pointer\n");
+ LOG_ERR("Failed to move write pointer to %s", write_buffer); Also applies to: 173-173, 188-188
| @@ -364,25 +366,83 @@ int clear_audio_directory() | |||
| res = fs_unlink("/SD:/audio"); | |||
| if (res) | |||
| { | |||
| printk("error deleting file\n"); | |||
| LOG_ERR("error deleting file"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include Error Codes in Log Messages
At lines 369, 375, and 381, error messages do not include error codes, which can be helpful for diagnosing issues.
Include the error code (res) in the log messages for better troubleshooting.
- LOG_ERR("error deleting file");
+ LOG_ERR("Error deleting audio directory: %d", res);
- LOG_ERR("failed to make directory");
+ LOG_ERR("Failed to create audio directory: %d", res);
- LOG_ERR("failed to make new file in directory files");
+ LOG_ERR("Failed to create new audio file: %d", res);Also applies to: 375-375, 381-381
| return -1; | ||
| } | ||
| printk("done with clearing\n"); | ||
| LOG_ERR("done with clearing"); |
There was a problem hiding this comment.
Incorrect Log Level for Informational Message
At line 384, LOG_ERR is used to log "done with clearing", which is an informational message. Using LOG_INF would be more appropriate.
Apply this diff to correct the log level:
- LOG_ERR("done with clearing");
+ LOG_INF("Done with clearing");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOG_ERR("done with clearing"); | |
| LOG_INF("Done with clearing"); |
| @@ -101,7 +101,7 @@ int mount_sd_card(void) | |||
| return -1; | |||
| } | |||
| LOG_INF("result of opendir: %d",err); | |||
|
|
|||
| initialize_audio_file(1); | |||
There was a problem hiding this comment.
Redundant Call to initialize_audio_file(1)
The function initialize_audio_file(1) is called twice in mount_sd_card(). The first call occurs after creating the audio directory, and the second call is here at line 104. This redundant call may lead to unnecessary operations.
Consider removing the redundant call to prevent duplicate initialization:
- initialize_audio_file(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initialize_audio_file(1); |
| @@ -137,8 +137,10 @@ int mount_sd_card(void) | |||
| if (res) | |||
| { | |||
| res = create_file("info.txt"); | |||
| save_offset(0); | |||
There was a problem hiding this comment.
Ensure save_offset(0); Only After Successful File Creation
At line 140, save_offset(0); is called immediately after attempting to create "info.txt". If the file creation failed, writing to it may result in errors.
Verify that create_file("info.txt"); succeeded before calling save_offset(0);. Update the code to check the return value and handle errors appropriately.
res = create_file("info.txt");
+ if (res) {
+ LOG_ERR("Failed to create info.txt: %d", res);
+ return -1;
+ }
save_offset(0);Committable suggestion was skipped due to low confidence.
|
@kevvz quick question: are you confident with the PR? |
Fixed a bug in the firmware where if you send a stop flag to the storage stream, the bluetooth module will permanently broadcast, wasting battery. Also, instead of the app tracking the offset of the storage in the case of discontinuous storage transmission, the device keeps track of the offset. The app should request the offset every time it needs to continue downloading, although this is not enforced.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation