Conversation
WalkthroughThe changes include modifications to the device tree overlay for the Xiao BLE Sense Devkit V2, introducing I2C configuration for the LSM6DS3TR-C sensor while disabling certain interfaces. The firmware configuration file is updated with new options related to power management and logging. Enhancements to button handling and power management are implemented in Changes
Possibly related PRs
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: 16
🧹 Outside diff range and nitpick comments (10)
Friend/firmware/firmware_v1.0/src/button.h (1)
14-14: Add documentation for the turnoff_all function.
The function aligns well with the PR's power management objectives, but please add a documentation comment describing:
- The purpose of the function
- Which components it turns off
- The expected behavior when called
+/**
+ * Turns off all device components for power saving.
+ * Called during long button press to prepare the system
+ * for low power state.
+ */
void turnoff_all();Friend/firmware/firmware_v1.0/src/mic.h (1)
16-17: Add documentation for the new microphone control functions.
The new functions would benefit from documentation comments similar to mic_start(). Please add documentation that describes:
- Purpose of each function
- Effect on power consumption
- Any preconditions or side effects
Example documentation:
+/**
+ * @brief Power off the microphone
+ *
+ * Disables the microphone to reduce power consumption during system off state
+ */
void mic_off();
+
+/**
+ * @brief Power on the microphone
+ *
+ * Enables the microphone for audio capture operations
+ */
void mic_on();Friend/firmware/firmware_v1.0/src/transport.h (1)
26-26: Add documentation for the bt_off function.
While the function declaration aligns well with the power management objectives and matches the interface style of bt_on(), it should include documentation for consistency with other functions in this header.
Add documentation block similar to other functions:
+/**
+ * @brief Turn off Bluetooth functionality
+ *
+ * Disables the BLE transport layer to reduce power consumption
+ *
+ * @return 0 if successful, negative errno code if error
+ */
int bt_off();Friend/firmware/firmware_v1.0/overlay/xiao_ble_sense_devkitv2-adafruit_module.overlay (1)
52-54: Consider removing commented code instead of keeping it disabled
The QSPI configuration is commented out. Since this is a device tree overlay, commented configurations don't serve any documentation purpose and should be removed for clarity.
-// &qspi {
-// status = "disabled";
-// };Friend/firmware/firmware_v1.0/src/mic.c (1)
105-114: Consider adding state tracking for power management
The current implementation lacks state tracking, which could lead to unnecessary power state transitions or inconsistent behavior.
Consider these architectural improvements:
- Add state tracking:
static atomic_t mic_powered = ATOMIC_INIT(0);- Add state validation to prevent redundant operations:
bool is_mic_powered(void) {
return atomic_get(&mic_powered) != 0;
}- Update power state atomically in both functions to ensure thread safety.
This would enable better integration with the system's power management and prevent race conditions in multi-threaded scenarios.
Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv2-adafruit.conf (1)
9-9: Remove redundant serial configuration.
The serial configuration appears twice with different states:
- Line 9:
# CONFIG_SERIAL=n(commented out) - Line 187:
CONFIG_SERIAL=n(active)
This could lead to confusion. Consider removing the commented version at line 9 since the active configuration at line 187 takes precedence.
Also applies to: 184-187
Friend/firmware/firmware_v1.0/src/main.c (3)
281-284: Adjust logging for reset reason output
Consider using formatted logging to output the reset reason in hexadecimal for better readability of bitmask values.
Apply this diff:
- LOG_INF("Reset reason: %d", reset_reas);
+ LOG_INF("Reset reason: 0x%08X", reset_reas);145-146: Type casting for boolean variables
While using bitwise operations to determine from_usb_event and from_wakeup, ensure that the results are explicitly cast to bool for clarity.
Apply this diff:
- bool from_usb_event = (reset_reas & VBUS_DETECT);
- bool from_wakeup = (reset_reas & WAKEUP_DETECT);
+ bool from_usb_event = ((reset_reas & VBUS_DETECT) != 0);
+ bool from_wakeup = ((reset_reas & WAKEUP_DETECT) != 0);281-284: Consolidate reset reason logging
Consider logging the reset reason once after the conditional blocks to avoid duplicate logs.
Apply this diff:
}
- LOG_INF("Reset reason: 0x%08X", reset_reas);
+ // Moved outside the conditional blocks
}
+ LOG_INF("Reset reason: 0x%08X", reset_reas);Friend/firmware/firmware_v1.0/src/button.c (1)
271-273: Consistency in Power-Off Sequence
Ensure that the power-off sequence here aligns with other parts of the code. Verify if the k_msleep(1); delay before calling turnoff_all() is necessary for hardware stabilization or if it can be adjusted for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- Friend/firmware/firmware_v1.0/overlay/xiao_ble_sense_devkitv2-adafruit_module.overlay (1 hunks)
- Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv2-adafruit.conf (4 hunks)
- Friend/firmware/firmware_v1.0/src/button.c (7 hunks)
- Friend/firmware/firmware_v1.0/src/button.h (1 hunks)
- Friend/firmware/firmware_v1.0/src/main.c (6 hunks)
- Friend/firmware/firmware_v1.0/src/mic.c (1 hunks)
- Friend/firmware/firmware_v1.0/src/mic.h (1 hunks)
- Friend/firmware/firmware_v1.0/src/sdcard.c (1 hunks)
- Friend/firmware/firmware_v1.0/src/sdcard.h (1 hunks)
- Friend/firmware/firmware_v1.0/src/speaker.c (1 hunks)
- Friend/firmware/firmware_v1.0/src/transport.c (8 hunks)
- Friend/firmware/firmware_v1.0/src/transport.h (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Friend/firmware/firmware_v1.0/src/sdcard.c
🔇 Additional comments (20)
Friend/firmware/firmware_v1.0/overlay/xiao_ble_sense_devkitv2-adafruit_module.overlay (2)
67-69: LGTM: Disabling I2C1 aligns with power optimization
Disabling unused I2C interface helps reduce power consumption, which aligns with the PR's objective of improving battery efficiency.
71-73: Verify impact of disabling temperature sensor
While disabling the temperature sensor will save power, please verify that:
- No other parts of the firmware rely on temperature readings
- This won't affect any thermal protection mechanisms
Let's check for temperature sensor usage in the codebase:
#!/bin/bash
# Search for temperature-related function calls or configurations
rg "temp|temperature" -t c -t hFriend/firmware/firmware_v1.0/src/mic.c (1)
105-114: Verify power management integration across the codebase
Let's verify how these new functions are integrated with the system's power management.
✅ Verification successful
Based on the results, I can now generate the final response as the integration pattern is clear from the shell script output.
Power management integration looks appropriate
The mic power control functions are properly integrated within the system's power management flow:
- Called in transport.c alongside Bluetooth and SD card power state changes
- Used in button.c's turnoff_all() function with other power-down operations
- Follows a consistent pattern where mic_off() is called during system shutdown and mic_on() during initialization
The implementation shows proper synchronization with other power management operations and no concerning race conditions were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check integration with power management system
# Expected: Find calls to mic_off() in power management related code
echo "Checking power management integration..."
rg -l "mic_off|mic_on" | while read -r file; do
echo "=== $file ==="
rg -A 5 -B 5 "mic_off|mic_on" "$file"
done
echo "Checking for potential race conditions..."
ast-grep --pattern 'if ($COND) {
$$$
mic_off();
$$$
}'
Length of output: 2113
Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv2-adafruit.conf (3)
33-33: Power management configuration aligns with PR objectives.
The addition of power management features supports the PR's goal of improving battery efficiency:
CONFIG_PM_DEVICE=yenables device power managementCONFIG_FPU=yenables the floating-point unit, which might be needed for power calculations
Also applies to: 74-74
183-183: Verify ring buffer usage context.
The addition of CONFIG_RING_BUFFER=y suggests implementation of buffered operations. Please ensure this is being used efficiently in the context of power management events or data handling.
✅ Verification successful
Let me search with broader patterns and check configuration files to understand the context better. The previous search might have failed due to the specific AST pattern, so I'm using more generic searches and also looking at configuration files to understand where and how ring buffers are configured in the project.
Ring buffer configuration is appropriately used for audio data handling
The ring buffer configuration is correctly implemented and efficiently used in both codec and transport layers:
- Used in
codec.cfor buffering audio samples before encoding - Used in
transport.cfor managing network packet transmission with proper header handling - Buffer sizes are well-defined with
NETWORK_RING_BUF_SIZEscaled appropriately for audio frames
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ring buffer usage in the codebase
ast-grep --pattern 'ring_buf_$_' || rg "(?i)ring.*buf"
Length of output: 34
Script:
#!/bin/bash
# Search for ring buffer related code using different patterns
rg -i "ring.?buffer|ring.?buf" --type c --type cpp --type h
# Search for Zephyr's ring buffer specific patterns
rg "RING_BUF|k_ring_buf" --type c --type cpp --type h
# Look for ring buffer configuration in other Kconfig files
fd -e conf -x grep -l "RING_BUFFER"
Length of output: 9269
75-75: Verify storage dependencies with QSPI disabled.
While disabling CONFIG_NORDIC_QSPI_NOR can improve power efficiency, please verify that this won't affect any storage functionality, especially considering the SD card support enabled in the configuration.
✅ Verification successful
Disabling QSPI NOR flash is safe
Based on the codebase analysis:
- The storage implementation uses SD card over SPI (
CONFIG_SPI_NRFX=y) and doesn't rely on QSPI NOR flash - Storage initialization only sets up a thread for write operations without any QSPI dependencies
- No QSPI or NOR flash references found in the storage implementation files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for QSPI dependencies in the codebase
rg -l "(?i)qspi|nor.*flash" --type c --type cpp --type h
Length of output: 58
Script:
#!/bin/bash
# Let's check the Kconfig files and source files for any QSPI or storage related configurations
rg -l "(?i)(qspi|nor.*flash|storage|flash)" --type-add 'conf:*.conf' --type conf --type c --type cpp --type h
# Also check specifically in the firmware directory for storage implementations
fd . Friend/firmware -e c -e h -e cpp --exec rg -l "(?i)(storage|flash|memory)"
Length of output: 5584
Script:
#!/bin/bash
# Let's check the storage implementation and configuration files to see if they depend on QSPI
rg -A 5 "(?i)(qspi|nor.*flash)" Friend/firmware/firmware_v1.0/src/storage.{h,c}
rg -A 5 "CONFIG_NORDIC_QSPI_NOR" Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv*.conf
rg "storage.*init" Friend/firmware/firmware_v1.0/src/main.c
Length of output: 1031
Script:
#!/bin/bash
# Let's check the storage initialization implementation and SD card dependencies
rg -B 5 -A 10 "storage_init" Friend/firmware/firmware_v1.0/src/storage.c
rg -A 5 "CONFIG_SPI" Friend/firmware/firmware_v1.0/prj_xiao_ble_sense_devkitv2-adafruit.conf
Length of output: 515
Friend/firmware/firmware_v1.0/src/speaker.c (3)
13-13: Test boot sound with new buffer size.
The reduced MAX_BLOCK_SIZE affects the boot sound generation. The play_boot_sound function uses this size for buffer allocation and I2S operations. Please ensure the boot sound quality remains acceptable with the smaller buffer.
Test scenarios to verify:
- Boot sound plays correctly without distortion
- Duration remains appropriate (currently sleeps for 3000ms)
- No memory-related issues during playback
Also applies to: 171-186
13-13: Validate streaming performance with smaller blocks.
The speak function handles streaming audio data in blocks. The reduced MAX_BLOCK_SIZE might require more frequent I2S operations for the same audio length. Please verify:
- No audio gaps or stuttering during playback
- Performance impact of more frequent I2S operations
- Power consumption profile during extended playback
Consider adding performance metrics or logging to monitor these aspects:
if (current_length < PACKET_SIZE) {
+ LOG_INF("Processing final block with size: %u", len);
LOG_INF("entered the final stretch");Also applies to: 89-157
13-13: Verify audio quality with reduced buffer size.
Reducing MAX_BLOCK_SIZE from 20000 to 10000 aligns with the PR's power efficiency goals by decreasing memory usage. However, this change requires verification to ensure it doesn't adversely affect audio quality or processing efficiency.
Please verify:
Consider adding a comment explaining the rationale for the 10000 size choice and any constraints/requirements.
✅ Verification successful
The previous search didn't give us enough context about the audio requirements. Let's examine the actual implementation file and related configurations to understand the implications of the buffer size change better.
Based on the detailed code analysis, I can now provide a definitive response about the buffer size change:
Buffer size reduction is safe and aligned with power efficiency goals
The reduced MAX_BLOCK_SIZE of 10000 bytes is appropriate for the current audio configuration:
- Sample rate is 8000Hz (SAMPLE_FREQUENCY)
- 16-bit samples (WORD_SIZE)
- 2 channels (NUMBER_OF_CHANNELS)
- Maximum audio duration = 10000/(228000) = 0.3125 seconds per block
The original commented value (24000 * 2 = 48000) was significantly overprovisioned. The new size:
- Matches the actual audio requirements
- Maintains proper I2S operation with the configured parameters
- Reduces memory usage through K_MEM_SLAB_DEFINE_STATIC
- Has sufficient capacity for the boot sound and gentle chime generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any audio quality or buffer size requirements in documentation
rg -i "audio|buffer|block.*size" --type md
# Look for related test cases
rg -i "test.*audio|test.*speaker" --type c
Length of output: 2272
Script:
#!/bin/bash
# Let's check the speaker.c file for any comments or requirements about buffer sizes
cat Friend/firmware/firmware_v1.0/src/speaker.c
# Search for any I2S or audio-related configurations that might depend on block size
rg -i "i2s.*config|sample.*rate|block.*size" --type c -A 3
# Look for any audio processing or buffer usage patterns
ast-grep --pattern 'K_MEM_SLAB_DEFINE_STATIC($$$)'
Length of output: 78864
Friend/firmware/firmware_v1.0/src/transport.c (5)
717-731: Verify cleanup sequence in bt_off().
The new bt_off() function handles Bluetooth shutdown and peripheral device management. The sequence appears logical but needs verification.
#!/bin/bash
# Check for other shutdown sequences in the codebase
rg -A 5 "bt_disable|sd_off|mic_off"Consider:
- Adding error handling for sd_off() and mic_off()
- Implementing a proper shutdown sequence with status checks
- Adding a state check before attempting to disable Bluetooth
734-738:
Verify initialization sequence in bt_on().
The modified bt_on() function includes storage service registration and peripheral device initialization.
#!/bin/bash
# Check for dependencies on the initialization sequence
rg -A 5 "bt_enable.*storage_service"The function needs error handling:
int bt_on()
{
int err = bt_enable(NULL);
+ if (err) {
+ LOG_ERR("Bluetooth enable failed (err %d)", err);
+ return err;
+ }
bt_le_adv_start(BT_LE_ADV_CONN, bt_ad, ARRAY_SIZE(bt_ad), bt_sd, ARRAY_SIZE(bt_sd));
bt_gatt_service_register(&storage_service);
sd_on();
mic_on();
return 0;
}313-313:
Verify logging level change impact.
Changed from LOG_INF to printk for battery logging. While this makes the output more visible, it bypasses the logging subsystem's level control.
#!/bin/bash
# Check if battery logging is consistently handled across the codebase
rg "Battery.*at.*mV.*capacity" Consider keeping the logging consistent by using the logging subsystem:
- printk("Battery at %d mV (capacity %d%%)\n", battery_millivolt, battery_percentage);
+ LOG_INF("Battery at %d mV (capacity %d%%)", battery_millivolt, battery_percentage);676-689: Review storage size check and heartbeat implementation.
The implementation includes a storage size check and a heartbeat counter for periodic file size updates. A few observations:
- The storage size check prevents writes beyond MAX_STORAGE_BYTES
- File size updates occur every 255 heartbeats
#!/bin/bash
# Check for other uses of MAX_STORAGE_BYTES
rg "MAX_STORAGE_BYTES"The implementation looks correct, but consider:
- Making the heartbeat interval (255) configurable
- Adding error handling for failed file size updates
175-175: Consider documenting the sampling frequency change.
The accelerometer sampling frequency has been reduced from 52 to 10 Hz. While this change likely improves power efficiency, it's important to verify that 10 Hz is sufficient for your use case.
Consider adding a comment explaining the rationale for choosing 10 Hz.
✅ Verification successful
From the search results, I can see that the sampling frequency is set for both accelerometer and gyroscope sensors in transport.c, with proper error handling and logging. The change from 52 Hz to 10 Hz appears to be a deliberate configuration choice that affects both sensors. Let me verify if there are any other dependencies on this sampling rate.
Based on the code analysis, I can now provide a final response about the sampling frequency change:
Sampling frequency change appears safe but should be documented
The sampling frequency change from 52 Hz to 10 Hz is consistently applied to both accelerometer and gyroscope sensors with proper error handling. While no direct dependencies on the specific sampling rate were found in the codebase, adding a comment would help future maintainers understand the rationale:
/* Sampling frequency set to 10 Hz to optimize power consumption while maintaining
sufficient motion detection capability */
odr_attr.val1 = 10;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any dependencies on accelerometer sampling frequency
rg -A 3 "SENSOR_ATTR_SAMPLING_FREQUENCY"
Length of output: 2895
Script:
#!/bin/bash
# Search for ODR or sampling frequency related constants and configurations
rg -A 3 "odr_attr|ODR|sampling.*freq" --type c
Length of output: 15700
Script:
#!/bin/bash
# Search for any data processing or analysis code that might depend on the sampling rate
rg -A 3 "process.*accel|analyze.*accel|sample.*accel" --type c
Length of output: 64
Friend/firmware/firmware_v1.0/src/main.c (3)
16-17: Definitions of VBUS_DETECT and WAKEUP_DETECT are correct
The defined bit masks align with the expected bits in the RESETREAS register for detecting USB and wakeup events.
141-142: Confirm necessity of enabling both DCDCEN and DCDCEN0
Both NRF_POWER->DCDCEN and NRF_POWER->DCDCEN0 are set to 1. Ensure that enabling both DC/DC converters is necessary for your application. If only one is needed, removing the redundant assignment can prevent unintended behavior.
Would you like to verify if both converters are required for your power configuration?
76-90: Ensure led_start() does not activate LEDs
Since activate_everything_no_lights() is intended to initialize components without activating LEDs, verify that led_start() only initializes the LED subsystem without turning on any LEDs. If led_start() activates LEDs, consider modifying it or delaying LED activation until explicitly needed.
Would you like assistance in verifying the behavior of led_start()?
Friend/firmware/firmware_v1.0/src/button.c (3)
13-13: Inclusion of "led.h" Header is Appropriate
Including led.h allows access to LED control functions used in the code, such as set_led_blue(), set_led_red(), and set_led_green(). This inclusion is correct and necessary for LED operations.
221-222: Ensure Safe Execution of bt_off() and turnoff_all() Sequence
Confirm that invoking bt_off() followed by turnoff_all() does not lead to unintended side effects, such as resource leaks or incomplete shutdown sequences. Verify that all peripherals and resources are properly released.
Run the following script to examine the implementations and usages:
#!/bin/bash
# Description: Find definitions and usages of bt_off() and turnoff_all()
# Find the definition of bt_off()
rg -A 10 'void bt_off\('
# Find all calls to bt_off()
rg 'bt_off\('
# Find the definition of turnoff_all()
rg -A 15 'void turnoff_all\('181-186: Verify Safe Invocation of bt_on() in This Context
Ensure that calling bt_on() within the check_button_level() function does not introduce race conditions or conflicts, especially if Bluetooth initialization has dependencies or if the function is called from multiple contexts.
Run the following script to check the usage and definition of bt_on():
✅ Verification successful
Let me gather more information about bt_on() specifically, as the previous script's output didn't show its definition or usage.
Let me check one more aspect to understand the Bluetooth initialization and dependencies better.
Based on the gathered information, I can now provide the final response:
bt_on() Implementation is Safe in Current Context
The implementation of bt_on() is safe to use in the button context because:
- It has proper error handling for Bluetooth initialization
- The function is only called in two controlled contexts: system startup (main.c) and button press handler
- The implementation includes sequential initialization of required services
- No concurrent access issues are present as the function handles its own state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find definitions and usages of bt_on()
# Find the definition of bt_on()
rg -A 5 'void bt_on\('
# Find all calls to bt_on()
rg 'bt_on\('
Length of output: 1771
Script:
#!/bin/bash
# Search for bt_on definition and declaration
rg -A 5 'bt_on\(' Friend/firmware/firmware_v1.0/src/
rg -A 5 'bt_on' Friend/firmware/firmware_v1.0/src/transport.h
rg -A 5 'bt_on' Friend/firmware/firmware_v1.0/src/transport.c
# Find the context around the button.c usage
rg -B 10 -A 10 'bt_on\(' Friend/firmware/firmware_v1.0/src/button.c
Length of output: 2270
Script:
#!/bin/bash
# Check for Bluetooth initialization and related functions
rg -A 10 'bt_enable' Friend/firmware/firmware_v1.0/src/
rg -A 10 'BT_LE_ADV_CONN' Friend/firmware/firmware_v1.0/src/
rg -A 10 'storage_service' Friend/firmware/firmware_v1.0/src/transport.c
# Check for any mutex or synchronization mechanisms
rg 'mutex|lock|sem' Friend/firmware/firmware_v1.0/src/transport.c
Length of output: 4098
| void activate_button_work(); | ||
| void register_button_service(); | ||
|
|
||
| void turnoff_all(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling mechanism.
The function might fail to turn off certain components. Consider modifying the signature to return a status code or error information.
-void turnoff_all();
+/**
+ * @return 0 on success, negative error code on failure
+ */
+int turnoff_all(void);📝 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 turnoff_all(); | |
| /** | |
| * @return 0 on success, negative error code on failure | |
| */ | |
| int turnoff_all(void); |
| void mic_off(); | ||
| void mic_on(); |
There was a problem hiding this comment.
💡 Codebase verification
Power management integration verified with minor concerns
The mic power management functions are integrated into the system, but there are a few points that need attention:
mic_off()is properly called during system shutdown inturnoff_all()functionmic_on()is called during transport initialization, but the mic is also powered on again inmic_start()- No explicit error handling or state checking in
mic_on()/mic_off()functions - The PDM (Pulse Density Modulation) peripheral state is not properly managed during power transitions
🔗 Analysis chain
Verify power management integration.
Let's ensure these functions are properly integrated with the system's power management.
Let me gather more context about the power management implementation to ensure a thorough verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of mic_off/on in power management contexts
# Look for mic_off usage in power management related code
echo "Checking mic_off usage in power management contexts:"
rg -A 5 "mic_off"
# Look for mic_on usage, particularly after power state changes
echo "Checking mic_on usage after power state changes:"
rg -B 5 "mic_on"
# Look for potential places where mic power state might be inconsistent
echo "Checking for power state handling in initialization and shutdown paths:"
rg -l "system_off|turnoff_all|power_down"
Length of output: 2915
Script:
#!/bin/bash
# Check mic power state handling implementation
echo "Checking mic.c implementation:"
cat Friend/firmware/firmware_v1.0/src/mic.c
# Check power management related code in button.c
echo "Checking button.c power management:"
rg -A 10 "system_off|turnoff_all|power_down" Friend/firmware/firmware_v1.0/src/button.c
# Check if mic state is handled in initialization
echo "Checking initialization paths:"
rg -A 5 "init|setup|main" Friend/firmware/firmware_v1.0/src/main.c
Length of output: 6180
| &i2c0 { | ||
| lsm6ds3tr_c: lsm6ds3tr-c@6a { | ||
| compatible = "st,lsm6dsl"; | ||
| reg = <0x6a>; | ||
| irq-gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>; | ||
| status = "okay"; | ||
| label = "b"; | ||
| // wakeup-source = true; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
LSM6DS3TR-C sensor configuration needs correction for proper operation
The current configuration has incorrect compatible string and missing wake-up configuration:
- The compatible string
st,lsm6dslis incorrect for LSM6DS3TR-C sensor - The wake-up source configuration is commented out
Apply these fixes:
- compatible = "st,lsm6dsl";
+ compatible = "st,lsm6ds3tr-c";
- // wakeup-source = true;
+ wakeup-source;🔗 Analysis chain
Verify LSM6DS3TR-C sensor configuration for deep sleep compatibility
A few concerns with the sensor configuration:
- The compatible string
st,lsm6dslmight be incorrect - LSM6DS3TR-C typically usesst,lsm6ds3tr-c - The commented
wakeup-sourceproperty suggests this sensor might be intended as a wake-up source, which is relevant for the deep sleep improvements
Let's verify the correct compatible string:
Apply this fix for the compatible string:
- compatible = "st,lsm6dsl";
+ compatible = "st,lsm6ds3tr-c";If this sensor is intended to wake the device from deep sleep:
- Uncomment and properly configure the
wakeup-sourceproperty - Ensure the IRQ GPIO configuration (pin 11) is properly set up for wake-up interrupt handling
Let me try another search to find LSM6DS3TR-C related device tree files:
Let me check the content of the overlay file to see the sensor configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for LSM6DS3TR-C compatible strings in other device tree files
rg "compatible.*lsm6" -A 2
Length of output: 19709
Script:
#!/bin/bash
# Search for LSM6DS3TR-C compatible strings in Zephyr's device tree bindings
fd -t f -e yaml -e overlay -e dts | xargs rg -l "lsm6|LSM6"
Length of output: 147
Script:
#!/bin/bash
# Check the content of the overlay file
cat Friend/firmware/firmware_v1.0/overlay/xiao_ble_sense_devkitv2-adafruit_module.overlay
Length of output: 1700
| void sd_on(); | ||
| void sd_off(); |
There was a problem hiding this comment.
Add documentation and align return types with existing functions.
The new functions lack documentation and deviate from the established error handling pattern. Consider:
- Adding documentation blocks consistent with other functions
- Returning int for error handling like other functions in this interface
Apply this diff:
-void sd_on();
-void sd_off();
+/**
+ * @brief Turn on SD card power
+ *
+ * Enables power to the SD card. Must be called before any SD card operations.
+ *
+ * @return 0 if successful, negative errno code if error
+ */
+int sd_on(void);
+
+/**
+ * @brief Turn off SD card power
+ *
+ * Safely disables power to the SD card. Ensures all pending operations are
+ * complete before power off.
+ *
+ * @return 0 if successful, negative errno code if error
+ */
+int sd_off(void);📝 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 sd_on(); | |
| void sd_off(); | |
| /** | |
| * @brief Turn on SD card power | |
| * | |
| * Enables power to the SD card. Must be called before any SD card operations. | |
| * | |
| * @return 0 if successful, negative errno code if error | |
| */ | |
| int sd_on(void); | |
| /** | |
| * @brief Turn off SD card power | |
| * | |
| * Safely disables power to the SD card. Ensures all pending operations are | |
| * complete before power off. | |
| * | |
| * @return 0 if successful, negative errno code if error | |
| */ | |
| int sd_off(void); |
💡 Codebase verification
Based on the code analysis, I can now provide a definitive response about the power management implementation for the SD card:
Power management implementation needs improvement
The current implementation in sdcard.c shows:
- The power management functions
sd_on()andsd_off()are empty/commented out:
void sd_off() {
// gpio_pin_set_dt(&sd_en_gpio_pin, 0);
}
void sd_on() {
// gpio_pin_set_dt(&sd_en_gpio_pin, 1);
}- While there is a mount function that handles filesystem initialization, there are no proper synchronization or unmounting steps before power off:
- No file system unmounting before power off
- No flushing of pending writes
- No waiting for I/O operations to complete
- The GPIO pin for SD card power control is defined but not utilized:
struct gpio_dt_spec sd_en_gpio_pin = { .port = DEVICE_DT_GET(DT_NODELABEL(gpio0)), .pin=19, .dt_flags = GPIO_INT_DISABLE };This implementation could lead to data corruption if the SD card is powered off while I/O operations are in progress.
🔗 Analysis chain
Verify safe power management implementation.
Power cycling the SD card requires careful synchronization to prevent data corruption. Ensure:
- All pending I/O operations are completed before power off
- Proper initialization sequence is followed during power on
- File system is unmounted before power off
Let's verify the implementation details:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these safety measures are implemented in the source file
rg -A 10 "sd_off" src/
# Look for unmount calls
rg "unmount|sync" src/
# Check for any existing power management patterns
rg "power.*off|disable" src/
Length of output: 213
Script:
#!/bin/bash
# Let's first locate the correct path for the firmware files
fd -t f "sdcard.h"
# Once we find the file, let's check its implementation
fd -t f "sdcard.c|sdcard.cpp"
# Look for power management related functions in the entire codebase
rg -A 5 "sd_[on|off]"
# Search for file system operations
rg -A 5 "(mount|unmount|sync|flush)"
Length of output: 532943
| void mic_on() | ||
| { | ||
| nrfy_gpio_pin_set(PDM_PWR_PIN); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add initialization check to mic_on()
The function should ensure proper initialization sequence when powering on the microphone.
Consider this enhanced implementation:
void mic_on()
{
nrfy_gpio_pin_set(PDM_PWR_PIN);
+
+ // Allow power to stabilize
+ k_sleep(K_MSEC(10));
+
+ // Restart PDM if previously active
+ if (nrfx_pdm_start() != NRFX_SUCCESS) {
+ LOG_ERR("Failed to restart PDM after powering on mic");
+ }
}Committable suggestion was skipped due to low confidence.
| if (from_usb_event) | ||
| { | ||
| k_msleep(100); | ||
| printf("from reset \n"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace printf with LOG_INF for consistent logging
To maintain consistency with the rest of the code and take advantage of the Zephyr logging system, replace printf("from reset \n"); with LOG_INF("From reset");.
Apply this diff to update the logging:
- printf("from reset \n");
+ LOG_INF("From reset");Committable suggestion was skipped due to low confidence.
| NRF_POWER->DCDCEN=1; | ||
| NRF_POWER->DCDCEN0=1; | ||
|
|
||
| NRF_POWER->RESETREAS=1; |
There was a problem hiding this comment.
Correct the way reset reasons are cleared
Assigning NRF_POWER->RESETREAS = 1; only clears bit 0 of the RESETREAS register. To properly clear the reset reasons that have been read, you should write back the value of reset_reas or the specific bits you intend to clear. Writing '1's to corresponding bits clears them.
Apply this diff to clear the reset reasons appropriately:
- NRF_POWER->RESETREAS=1;
+ NRF_POWER->RESETREAS = reset_reas;Alternatively, if you want to clear only specific bits:
- NRF_POWER->RESETREAS=1;
+ NRF_POWER->RESETREAS = VBUS_DETECT | WAKEUP_DETECT;Committable suggestion was skipped due to low confidence.
| void activate_everything_no_lights() | ||
| { | ||
| int err; | ||
|
|
||
| err = led_start(); | ||
| err = transport_start(); | ||
| err = mount_sd_card(); | ||
| err = storage_init(); | ||
| err = init_haptic_pin(); | ||
| set_codec_callback(codec_handler); | ||
| err = codec_start(); | ||
| set_mic_callback(mic_handler); | ||
| err = mic_start(); | ||
| err = init_usb(); | ||
| } |
There was a problem hiding this comment.
Add error handling in activate_everything_no_lights()
Currently, the function initializes several components but does not check the return values of the initialization functions. This could lead to unhandled errors and undefined behavior if any initialization fails. It's important to check for errors after each call and handle them appropriately, possibly by logging the error and returning or taking corrective action.
Apply this diff to add error handling after each initialization step:
void activate_everything_no_lights()
{
int err;
err = led_start();
+ if (err) {
+ LOG_ERR("Failed to initialize LEDs: %d", err);
+ return;
+ }
err = transport_start();
+ if (err) {
+ LOG_ERR("Failed to start transport: %d", err);
+ return;
+ }
err = mount_sd_card();
+ if (err) {
+ LOG_ERR("Failed to mount SD card: %d", err);
+ return;
+ }
err = storage_init();
+ if (err) {
+ LOG_ERR("Failed to initialize storage: %d", err);
+ return;
+ }
err = init_haptic_pin();
+ if (err) {
+ LOG_ERR("Failed to initialize haptic pin: %d", err);
+ return;
+ }
set_codec_callback(codec_handler);
err = codec_start();
+ if (err) {
+ LOG_ERR("Failed to start codec: %d", err);
+ return;
+ }
set_mic_callback(mic_handler);
err = mic_start();
+ if (err) {
+ LOG_ERR("Failed to start microphone: %d", err);
+ return;
+ }
err = init_usb();
+ if (err) {
+ LOG_ERR("Failed to initialize USB: %d", err);
+ 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.
| void activate_everything_no_lights() | |
| { | |
| int err; | |
| err = led_start(); | |
| err = transport_start(); | |
| err = mount_sd_card(); | |
| err = storage_init(); | |
| err = init_haptic_pin(); | |
| set_codec_callback(codec_handler); | |
| err = codec_start(); | |
| set_mic_callback(mic_handler); | |
| err = mic_start(); | |
| err = init_usb(); | |
| } | |
| void activate_everything_no_lights() | |
| { | |
| int err; | |
| err = led_start(); | |
| if (err) { | |
| LOG_ERR("Failed to initialize LEDs: %d", err); | |
| return; | |
| } | |
| err = transport_start(); | |
| if (err) { | |
| LOG_ERR("Failed to start transport: %d", err); | |
| return; | |
| } | |
| err = mount_sd_card(); | |
| if (err) { | |
| LOG_ERR("Failed to mount SD card: %d", err); | |
| return; | |
| } | |
| err = storage_init(); | |
| if (err) { | |
| LOG_ERR("Failed to initialize storage: %d", err); | |
| return; | |
| } | |
| err = init_haptic_pin(); | |
| if (err) { | |
| LOG_ERR("Failed to initialize haptic pin: %d", err); | |
| return; | |
| } | |
| set_codec_callback(codec_handler); | |
| err = codec_start(); | |
| if (err) { | |
| LOG_ERR("Failed to start codec: %d", err); | |
| return; | |
| } | |
| set_mic_callback(mic_handler); | |
| err = mic_start(); | |
| if (err) { | |
| LOG_ERR("Failed to start microphone: %d", err); | |
| return; | |
| } | |
| err = init_usb(); | |
| if (err) { | |
| LOG_ERR("Failed to initialize USB: %d", err); | |
| return; | |
| } | |
| } |
| LOG_INF("D5 ready"); | ||
| } | ||
|
|
||
| // GPIO_INT_LEVEL_INACTIVE |
There was a problem hiding this comment.
Potential Mismatch Between Comment and Code
The comment // GPIO_INT_LEVEL_INACTIVE suggests an interrupt configuration different from the code, which uses GPIO_INT_EDGE_BOTH. If the intent is to use GPIO_INT_LEVEL_INACTIVE, update the code accordingly. Otherwise, modify or remove the comment to prevent confusion.
Apply this diff to resolve the mismatch:
-// GPIO_INT_LEVEL_INACTIVE
err2 = gpio_pin_interrupt_configure_dt(&d5_pin_input,GPIO_INT_EDGE_BOTH);
+err2 = gpio_pin_interrupt_configure_dt(&d5_pin_input,GPIO_INT_LEVEL_INACTIVE);Committable suggestion was skipped due to low confidence.
| } | ||
|
|
||
| void turnoff_all() | ||
| { | ||
| set_led_blue(false); | ||
| set_led_red(false); | ||
| set_led_green(false); | ||
| mic_off(); | ||
| sd_off(); | ||
| play_haptic_milli(100); | ||
| k_msleep(10); | ||
| gpio_remove_callback(d5_pin_input.port, &button_cb_data); | ||
| gpio_pin_interrupt_configure_dt(&d5_pin_input,GPIO_INT_LEVEL_INACTIVE); | ||
| //maybe save something here to indicate success. next time the button is pressed we should know about it | ||
| NRF_USBD->INTENCLR= 0xFFFFFFFF; | ||
| NRF_POWER->SYSTEMOFF=1; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Use Zephyr API for System Power-Off
In the turnoff_all() function, you're directly manipulating hardware registers (e.g., NRF_POWER->SYSTEMOFF = 1;). For better abstraction and future compatibility, consider using Zephyr's power management APIs to handle system power-off.
Apply this diff to replace direct register access with a Zephyr API call:
gpio_remove_callback(d5_pin_input.port, &button_cb_data);
gpio_pin_interrupt_configure_dt(&d5_pin_input,GPIO_INT_LEVEL_INACTIVE);
//maybe save something here to indicate success. next time the button is pressed we should know about it
- NRF_USBD->INTENCLR= 0xFFFFFFFF;
- NRF_POWER->SYSTEMOFF=1;
+ sys_pm_force_power_state(SYS_POWER_STATE_DEEP_SLEEP_1);Ensure you include the necessary headers and handle any return values or errors from the API call.
Committable suggestion was skipped due to low confidence.
|
Hi @kevvz i try your code with my unoffical dev kit v2 and the system only reach about 4.4mA. |
|
|
|
Hi, the newest dev kit, with the custom PCB, has an sd enable pin and a mic pin which I suspect contributes to the power drain. Here is the picture of the multimeter I used to connect to the battery. |
|
Ok @kevvz , I will test and confirm on the official one when i received. Still waiting for it :(. Btw could you also power down the accelerometer? it will reduce about 0.15mA i think. |
New record!! |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (9)
Friend/firmware/firmware_v1.0/src/transport.h (1)
26-28: Add documentation for new functions.The new functions lack documentation comments unlike other functions in the header. Please add documentation blocks that describe:
- Function purpose
- Expected behavior
- Return values and their meanings (for
bt_off)- Any side effects
Example:
+/** + * @brief Turn off Bluetooth functionality + * + * Disables the BLE transport logic and related functionalities + * + * @return 0 if successful, negative errno code if error + */ int bt_off(); +/** + * @brief Turn off the accelerometer + * + * Disables the accelerometer to reduce power consumption + */ void accel_off();Friend/firmware/firmware_v1.0/src/usb.c (3)
Line range hint
3-6: Clean up commented-out includes.Remove the commented-out includes to improve code readability and maintenance.
-// #include <zephyr/usb/usb_device.h> -// #include <zephyr/drivers/usb/usb_dc.h> -// #include <zephyr/drivers/usb/device/usb_dc_nrfx.c>
14-14: Consider encapsulating power state management.The external
is_offvariable creates tight coupling between modules. Consider introducing a power management interface that encapsulates this state.
Line range hint
34-35: Clean up unnecessary return statement.Remove the unnecessary
return;statement for this void function and fix the extra blank lines.} - return; - -Friend/firmware/firmware_v1.0/src/speaker.h (1)
54-54: Add documentation for the speaker_off function.For consistency with other functions in this header, please add documentation that describes the purpose and behavior of this function.
Add documentation block similar to other functions:
+/** + * @brief Turn off the speaker + * + * Powers down the speaker by setting the GPIO pin low to reduce power consumption + */ void speaker_off();Friend/firmware/firmware_v1.0/src/speaker.c (2)
54-68: Document speaker pin initialization state.The speaker pin initialization includes good error handling, but please add a comment explaining why the pin needs to be set high initially. Consider setting the pin low in error paths to ensure minimal power consumption.
gpio_pin_configure_dt(&speaker_gpio_pin, GPIO_OUTPUT_INACTIVE) < 0) { printk("Error setting up Haptic Pin\n"); + gpio_pin_set_dt(&speaker_gpio_pin, 0); // Ensure low power state on error return -1; } + // Set speaker enable pin high to activate the amplifier gpio_pin_set_dt(&speaker_gpio_pin, 1);
281-285: Consider implementing a centralized power management interface.The speaker power management is currently handled through direct GPIO control. Consider implementing a centralized power management interface that:
- Coordinates power states across all audio components (I2S, amplifier, etc.)
- Provides proper sequencing for power up/down
- Tracks power state and prevents operations when powered down
- Reports power consumption metrics
This would better align with the PR's goal of improving battery efficiency and make power management more maintainable.
Friend/firmware/firmware_v1.0/src/transport.c (2)
26-26: Consider using size_t for MAX_STORAGE_BYTESThe constant is used for size comparisons but is defined as a hex literal. Using size_t would be more appropriate for size/capacity constants.
-#define MAX_STORAGE_BYTES 0xFFFF0000 +#define MAX_STORAGE_BYTES ((size_t)0xFFFF0000)
331-331: Use LOG_INF instead of printk for battery level loggingFor consistency with the rest of the codebase, use the logging subsystem instead of printk.
- printk("Battery at %d mV (capacity %d%%)\n", battery_millivolt, battery_percentage); + LOG_INF("Battery at %d mV (capacity %d%%)", battery_millivolt, battery_percentage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Friend/firmware/firmware_v1.0/src/button.c(7 hunks)Friend/firmware/firmware_v1.0/src/button.h(1 hunks)Friend/firmware/firmware_v1.0/src/main.c(6 hunks)Friend/firmware/firmware_v1.0/src/speaker.c(6 hunks)Friend/firmware/firmware_v1.0/src/speaker.h(1 hunks)Friend/firmware/firmware_v1.0/src/transport.c(10 hunks)Friend/firmware/firmware_v1.0/src/transport.h(1 hunks)Friend/firmware/firmware_v1.0/src/usb.c(1 hunks)
🔇 Additional comments (11)
Friend/firmware/firmware_v1.0/src/button.h (1)
14-14:
Add error handling and documentation for critical power management function.
This function appears to be critical for power management. Consider:
- Adding error handling as previously suggested
- Adding documentation to describe the function's purpose, behavior, and component shutdown sequence
Apply this diff:
-void turnoff_all();
+/**
+ * Turns off all system components in preparation for deep sleep or system off.
+ * Components affected: [list the components]
+ *
+ * @return 0 on success, negative error code on failure:
+ * -EINVAL: Invalid component state
+ * -EIO: I/O error while turning off components
+ */
+int turnoff_all(void);Friend/firmware/firmware_v1.0/src/transport.h (1)
28-28: Verify the presence of corresponding 'on' function.
The accel_off() function appears to lack a corresponding accel_on() function. This asymmetry might indicate missing functionality or inconsistent naming.
✅ Verification successful
Let me gather more information to check for any potential accel_on function or related power control functions.
The accelerometer power control is handled correctly despite the naming asymmetry
Looking at the initialization code in transport.c, the accelerometer is powered on during device initialization with gpio_pin_set_dt(&accel_gpio_pin, 1) and the accel_off() function is used for power down. While there isn't an explicit accel_on() function, the power-on functionality is integrated into the device initialization sequence, which is a common embedded systems pattern.
- The accelerometer is enabled by default during initialization in
transport.c accel_off()is used as part of the device shutdown sequence (called inbutton.c)- The naming asymmetry is acceptable in this context since power-on is handled during initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential accelerometer control functions
rg -l "accel_[a-z]+" --type c --type h
# Look for function declarations/definitions related to accelerometer
ast-grep --pattern 'void $FUNC($$$) { $$$ }' | rg -i 'accel'
Length of output: 1423
Script:
#!/bin/bash
# Search for accelerometer GPIO initialization and control
rg -A 5 "accel.*gpio" --type c
rg -B 5 "accel_off" --type c
# Look for power-related functions for accelerometer
rg "void.*accel" --type c --type h
Length of output: 3719
Friend/firmware/firmware_v1.0/src/usb.c (2)
Line range hint 31-33: Review default USB status handling.
The default case sets usb_charge = true which might be incorrect for error states or other unexpected USB status codes. Consider documenting why this is the desired behavior or handle different status codes explicitly.
#!/bin/bash
# Search for other USB status handling patterns in the codebase
rg "USB_DC_\w+" --type c24-28: Verify error handling and state consistency.
The USB disconnect handler performs multiple power-related operations without error handling. Consider:
- What happens if
bt_off()fails? - Could there be race conditions between USB disconnect and power state changes?
Friend/firmware/firmware_v1.0/src/speaker.h (1)
54-54: Function declaration looks good.
The new speaker_off() function aligns well with the PR's objective of improving battery efficiency.
Let's verify its integration with the power management system:
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the integration of speaker_off() with the power management system.
speaker_off() is properly integrated with power management
The function is well-integrated into the system's power management:
- Called in
turnoff_all()alongside other power-saving functions (mic_off(),sd_off()) turnoff_all()is triggered from both button handling and USB management contexts- Implementation exists in
speaker.cwith proper header declaration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of speaker_off in power management related code
# Test 1: Check where speaker_off is called
echo "Checking speaker_off usage:"
rg -l "speaker_off"
# Test 2: Verify integration with button handling for manual turnoff
echo "Checking integration with button handling:"
rg -A 5 "turnoff_all"
Length of output: 2234
Friend/firmware/firmware_v1.0/src/main.c (3)
9-9: LGTM! Power management constants are correctly defined.
The VBUS_DETECT and WAKEUP_DETECT bit masks are properly defined for the nRF52 RESETREAS register.
Also applies to: 17-18
77-92: Skip duplicate comment about error handling.
A previous review already provided detailed feedback about adding error handling to this function.
149-159: Verify USB event handling delay necessity.
The 100ms sleep after USB event detection seems arbitrary. Consider:
- Document why this specific delay is needed
- Make the delay configurable via a define
- Verify if this delay is sufficient across all supported hardware variants
✅ Verification successful
Based on the search results, I can now provide a final response:
The 100ms USB event delay appears to be part of a broader system initialization pattern
The codebase shows consistent use of delays during state transitions and initialization:
- Other USB-related initialization has a 1000ms delay (line 166)
- The delay occurs in the context of power state changes where
is_offandusb_chargeflags are modified - Similar delays (100-1000ms) are used throughout the codebase during hardware initialization and state changes
The 100ms delay appears reasonable given:
- It provides time for USB power rail stabilization before activating peripherals
- It's consistent with other initialization delays in the system
- It's used specifically during the USB wakeup event handling path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other USB-related delays in the codebase
rg -n "k_msleep.*usb" --type c
rg -n "k_sleep.*usb" --type c
Length of output: 64
Script:
#!/bin/bash
# Search for any sleep/delay patterns around USB handling more broadly
rg -n -B2 -A2 "k_m?sleep" Friend/firmware/firmware_v1.0/src/
Length of output: 11736
Script:
#!/bin/bash
# Look for USB-related code patterns to understand the context
rg -n -B2 -A2 "usb|USB" Friend/firmware/firmware_v1.0/src/main.c
Length of output: 1447
Script:
#!/bin/bash
# Search for power state transitions and USB handling functions
ast-grep --pattern 'void $FUNC($$$) {
$$$
usb
$$$
}'
Length of output: 61
Friend/firmware/firmware_v1.0/src/speaker.c (1)
13-13: Verify audio quality with reduced buffer size.
Reducing MAX_BLOCK_SIZE from 20000 to 10000 bytes will help with memory optimization, but we should verify that this doesn't impact audio quality or cause buffer underruns during playback.
✅ Verification successful
Let me gather more information about the audio buffer usage to better understand the implications of this change.
This will help us understand:
- How MAX_BLOCK_SIZE is used in the context
- The speaker initialization logic
- The buffer allocation and usage patterns
Based on the gathered information, I can now provide a final response:
Buffer size reduction is safe for the current audio configuration
The reduced MAX_BLOCK_SIZE of 10000 bytes is sufficient for the current audio setup because:
- The system uses 8kHz sample rate with 2 channels (defined by SAMPLE_FREQUENCY and NUMBER_OF_CHANNELS)
- Data is processed in smaller PACKET_SIZE chunks of 400 bytes
- The I2S configuration shows proper memory management with double buffering (BLOCK_COUNT = 2)
- The buffer is primarily used for audio playback (chimes) and temporary storage, with no evidence of buffer underruns in the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any reported audio quality issues or buffer underrun errors in the codebase
rg -i "buffer (underrun|underflow|overflow)"
rg -i "audio quality"
Length of output: 224
Script:
#!/bin/bash
# Search for buffer usage patterns and audio-related configurations
rg -A 5 "MAX_BLOCK_SIZE"
ast-grep --pattern 'speaker_init($$$)'
rg -B 2 -A 5 "rx_buffer|buzz_buffer"
Length of output: 8442
Friend/firmware/firmware_v1.0/src/button.c (1)
182-187: LGTM! Clear power state handling with user feedback.
The implementation provides good user feedback through haptic response when waking the device, and properly manages Bluetooth state.
Friend/firmware/firmware_v1.0/src/transport.c (1)
176-194:
Verify accelerometer configuration and GPIO setup
Several issues in this segment:
- The sampling rate was reduced from 52 to 10 Hz which may affect motion detection sensitivity
- The GPIO pin configuration uses incorrect logging messages (refers to "Speaker Pin" and "Haptic Pin" instead of accelerometer)
Fix the misleading log messages:
- printk("Speaker Pin ready\n");
+ printk("Accelerometer Pin ready\n");
- printk("Error setting up Haptic Pin\n");
+ printk("Error setting up Accelerometer Pin\n");| void force_button_state(FSM_STATE_T state); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Documentation improvement needed for force_button_state with state validation
The function is used in power management scenarios (main.c) to force GRACE state after wakeup. While initialization check exists in the codebase, the force_button_state implementation lacks validation for valid state transitions. Apply this improved diff:
+/**
+ * Forces the button state machine into a specific state.
+ * IMPORTANT: Use with caution as this bypasses normal state transitions.
+ *
+ * @param state The desired FSM state to force (IDLE, ONE_PRESS, TWO_PRESS, or GRACE)
+ * @pre Button GPIO must be initialized via button_init()
+ * @note This function is primarily used in power management scenarios
+ * like system wakeup where GRACE state needs to be restored
+ */
void force_button_state(FSM_STATE_T state);- Consider adding state validation in
button.cimplementation to ensure only valid states are accepted
🔗 Analysis chain
Add documentation and consider validation for state forcing function.
This function allows direct manipulation of the button state machine, which could lead to inconsistent states if not used carefully.
Apply this diff:
+/**
+ * Forces the button state machine into a specific state.
+ * IMPORTANT: Use with caution as this bypasses normal state transitions.
+ *
+ * @param state The desired FSM state to force
+ * @pre State machine must be initialized
+ * @note This function should only be called in specific power management scenarios
+ */
void force_button_state(FSM_STATE_T state);Let's verify the state machine initialization check:
Let me gather more context about the button initialization and state machine implementation.
Let me check for the FSM state definitions and button initialization implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if force_button_state is called after proper initialization
# Test: Search for force_button_state calls and their context
rg -B 5 "force_button_state"
# Test: Verify button initialization
ast-grep --pattern 'button_init()'
Length of output: 1412
Script:
#!/bin/bash
# Check button initialization implementation and state machine setup
ast-grep --pattern 'button_init() {
$$$
}'
# Look for FSM_STATE_T definition and valid states
rg -A 10 "FSM_STATE_T"
# Check if there's any validation in force_button_state implementation
rg -A 10 "force_button_state\(.*\)\s*{"
Length of output: 2531
Script:
#!/bin/bash
# Find FSM state enum definition
rg -B 5 "FSM_STATE_T" Friend/firmware/firmware_v1.0/src/button.h
# Check button initialization implementation
rg -A 10 "button_init\(\)" Friend/firmware/firmware_v1.0/src/button.c
Length of output: 575
| int bt_on(); | ||
| int bt_off(); | ||
|
|
||
| void accel_off(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consistent error handling for accelerometer control.
While bt_off() returns an error code, accel_off() is void. Consider returning an int for consistent error handling across the transport interface.
-void accel_off();
+int accel_off();📝 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 accel_off(); | |
| int accel_off(); |
| NRF_POWER->DCDCEN=1; | ||
| NRF_POWER->DCDCEN0=1; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance DCDC configuration for optimal power efficiency.
While enabling DCDC converters is good for power efficiency, consider these improvements:
- Add verification that DCDC mode was successfully enabled
- Consider enabling DCDCEN1 for radio operations if applicable
- Move this configuration after system clock initialization
- NRF_POWER->DCDCEN=1;
- NRF_POWER->DCDCEN0=1;
+ // Enable and verify DCDC converters
+ NRF_POWER->DCDCEN = 1;
+ NRF_POWER->DCDCEN0 = 1;
+ if (!(NRF_POWER->DCDCEN) || !(NRF_POWER->DCDCEN0)) {
+ LOG_WRN("DCDC mode not enabled, power consumption may be higher");
+ }Committable suggestion was skipped due to low confidence.
| else if (from_wakeup) | ||
| { | ||
|
|
||
| is_off = false; | ||
| usb_charge = false; | ||
| force_button_state(GRACE); | ||
| k_msleep(1000); | ||
| activate_everything_no_lights(); | ||
| bt_on(); | ||
| play_haptic_milli(100); | ||
|
|
There was a problem hiding this comment.
Improve wakeup sequence robustness.
The current wakeup sequence could be more robust:
- The 1000ms delay is hard-coded and lacks justification
- No validation that components are properly initialized before enabling Bluetooth
- No power state validation before activating haptic feedback
Consider implementing a more structured wakeup sequence:
else if (from_wakeup)
{
- is_off = false;
- usb_charge = false;
- force_button_state(GRACE);
- k_msleep(1000);
- activate_everything_no_lights();
- bt_on();
- play_haptic_milli(100);
+ // Update power state
+ is_off = false;
+ usb_charge = false;
+
+ // Initialize core components
+ err = activate_everything_no_lights();
+ if (err) {
+ LOG_ERR("Failed to initialize components: %d", err);
+ return err;
+ }
+
+ // Configure button state
+ force_button_state(GRACE);
+
+ // Enable wireless features
+ err = bt_on();
+ if (err) {
+ LOG_ERR("Failed to enable Bluetooth: %d", err);
+ return err;
+ }
+
+ // Provide user feedback
+ play_haptic_milli(100);Committable suggestion was skipped due to low confidence.
| // if (k_sem_take(&haptic_sem, K_MSEC(50)) != 0) | ||
| // { | ||
|
|
||
| // } | ||
| // else | ||
| // { | ||
| k_timer_start(&my_status_timer, K_MSEC(duration), K_NO_WAIT); | ||
| // } |
There was a problem hiding this comment.
Remove commented semaphore logic.
The haptic control semaphore logic is commented out but still present in the code. If this synchronization is no longer needed, please remove the commented code. If it might be needed later, add a TODO comment explaining why it's commented out and when it might be needed.
- // if (k_sem_take(&haptic_sem, K_MSEC(50)) != 0)
- // {
-
- // }
- // else
- // {
k_timer_start(&my_status_timer, K_MSEC(duration), K_NO_WAIT);
- // }📝 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.
| // if (k_sem_take(&haptic_sem, K_MSEC(50)) != 0) | |
| // { | |
| // } | |
| // else | |
| // { | |
| k_timer_start(&my_status_timer, K_MSEC(duration), K_NO_WAIT); | |
| // } | |
| k_timer_start(&my_status_timer, K_MSEC(duration), K_NO_WAIT); |
| if(!from_wakeup) | ||
| { | ||
| bt_disable(); | ||
| int err = bt_le_adv_stop(); | ||
| if (err) | ||
| { | ||
| printk("Failed to stop Bluetooth %d\n",err); | ||
| } | ||
| is_off = !is_off; | ||
| } | ||
| else | ||
| { | ||
| int err = bt_enable(NULL); | ||
| if (err) | ||
| { | ||
| printk("Failed to enable Bluetooth %d\n",err); | ||
| } | ||
| bt_on(); | ||
|
|
||
| from_wakeup = false; | ||
| } | ||
| if (is_off) | ||
| { | ||
| bt_off(); | ||
| turnoff_all(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract power state toggle logic to a separate function.
The power state toggle logic is duplicated in multiple places. Consider extracting it to a dedicated function to improve maintainability and reduce duplication.
+static void toggle_power_state() {
+ if (!from_wakeup) {
+ is_off = !is_off;
+ } else {
+ from_wakeup = false;
+ }
+ if (is_off) {
+ bt_off();
+ turnoff_all();
+ }
+}
-if(!from_wakeup) {
- is_off = !is_off;
-} else {
- from_wakeup = false;
-}
-if (is_off) {
- bt_off();
- turnoff_all();
-}
+toggle_power_state();Committable suggestion was skipped due to low confidence.
| void turnoff_all() | ||
| { | ||
|
|
||
| mic_off(); | ||
| sd_off(); | ||
| speaker_off(); | ||
| accel_off(); | ||
| play_haptic_milli(50); | ||
| k_msleep(100); | ||
| set_led_blue(false); | ||
| set_led_red(false); | ||
| set_led_green(false); | ||
| gpio_remove_callback(d5_pin_input.port, &button_cb_data); | ||
| gpio_pin_interrupt_configure_dt(&d5_pin_input,GPIO_INT_LEVEL_INACTIVE); | ||
| //maybe save something here to indicate success. next time the button is pressed we should know about it | ||
| NRF_USBD->INTENCLR= 0xFFFFFFFF; | ||
| NRF_POWER->SYSTEMOFF=1; | ||
| } |
There was a problem hiding this comment.
Add error handling for peripheral power down operations.
The turnoff_all() function performs multiple peripheral operations without checking their success. Consider adding error handling to ensure all peripherals are properly powered down before system off.
void turnoff_all()
{
- mic_off();
- sd_off();
- speaker_off();
- accel_off();
+ int err = 0;
+ err |= mic_off();
+ err |= sd_off();
+ err |= speaker_off();
+ err |= accel_off();
+ if (err) {
+ LOG_ERR("Failed to power down all peripherals: %d", err);
+ // Consider fallback strategy
+ }
play_haptic_milli(50);
k_msleep(100);
set_led_blue(false);
set_led_red(false);
set_led_green(false);
gpio_remove_callback(d5_pin_input.port, &button_cb_data);
gpio_pin_interrupt_configure_dt(&d5_pin_input,GPIO_INT_LEVEL_INACTIVE);
+ // Save power state to persistent storage before shutdown
+ // TODO: Implement state persistence
NRF_USBD->INTENCLR= 0xFFFFFFFF;
NRF_POWER->SYSTEMOFF=1;
}Committable suggestion was skipped due to low confidence.
| void accel_off() | ||
| { | ||
| gpio_pin_set_dt(&accel_gpio_pin, 0); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Add error handling to accel_off()
The accel_off() function doesn't check the return value of gpio_pin_set_dt().
Add error handling:
void accel_off() {
- gpio_pin_set_dt(&accel_gpio_pin, 0);
+ int err = gpio_pin_set_dt(&accel_gpio_pin, 0);
+ if (err) {
+ LOG_ERR("Failed to turn off accelerometer: %d", err);
+ }
}📝 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 accel_off() | |
| { | |
| gpio_pin_set_dt(&accel_gpio_pin, 0); | |
| } | |
| void accel_off() | |
| { | |
| int err = gpio_pin_set_dt(&accel_gpio_pin, 0); | |
| if (err) { | |
| LOG_ERR("Failed to turn off accelerometer: %d", err); | |
| } | |
| } |
| bool result = false; | ||
| if (file_num_array[1] < MAX_STORAGE_BYTES) | ||
| { | ||
| result = write_to_storage(); | ||
| } | ||
| if (result) | ||
| { | ||
|
|
||
| heartbeat_count++; | ||
| if (heartbeat_count == 255) | ||
| { | ||
| update_file_size(); | ||
| heartbeat_count = 0; | ||
| printk("drawing\n"); | ||
| } |
There was a problem hiding this comment.
Review file size update logic
The heartbeat counter and file size update mechanism has potential issues:
- The file size update is only triggered every 255 iterations
- The update_file_size() call is not protected against concurrent access
- The "drawing" log message is unclear about what's being drawn
Consider these improvements:
- Add atomic operations for heartbeat_count
- Add mutex protection for file size updates
- Clarify the log message
-static uint8_t heartbeat_count = 0;
+static atomic_t heartbeat_count = ATOMIC_INIT(0);
void update_file_size() {
+ k_mutex_lock(&file_size_mutex, K_FOREVER);
file_num_array[0] = get_file_size(1);
file_num_array[1] = get_offset();
+ k_mutex_unlock(&file_size_mutex);
}Committable suggestion was skipped due to low confidence.
| int bt_off() | ||
| { | ||
| bt_disable(); | ||
| int err = bt_le_adv_stop(); | ||
| if (err) | ||
| { | ||
| printk("Failed to stop Bluetooth %d\n",err); | ||
| } | ||
| sd_off(); | ||
| mic_off(); | ||
| //everything else off | ||
|
|
||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Enhance error handling in bt_off()
The bt_off() function:
- Doesn't check the return value of bt_disable()
- Doesn't verify if Bluetooth was actually enabled before disabling
- Missing error handling for sd_off() and mic_off()
Add proper error handling:
int bt_off() {
- bt_disable();
- int err = bt_le_adv_stop();
+ int err = bt_disable();
+ if (err) {
+ LOG_ERR("Failed to disable Bluetooth: %d", err);
+ return err;
+ }
+
+ err = bt_le_adv_stop();
if (err) {
- printk("Failed to stop Bluetooth %d\n",err);
+ LOG_ERR("Failed to stop Bluetooth advertising: %d", err);
+ return err;
}
- sd_off();
- mic_off();
+
+ err = sd_off();
+ if (err) {
+ LOG_ERR("Failed to turn off SD card: %d", err);
+ return err;
+ }
+
+ err = mic_off();
+ if (err) {
+ LOG_ERR("Failed to turn off microphone: %d", err);
+ return err;
+ }
return 0;
}Committable suggestion was skipped due to low confidence.
|
cool! please keep collaborating. looking fwd for the next firemware release. feel free to lmk when the code is ready to be merged. |
|
Hi @kevvz, I love the new record! Could you check and confirm the actual code changes that reduced power consumption? For example, changes in the .conf or .overlay files... That will help us identify and confirm the root causes. |
|
This might explain some of heat i get on the "unofficial" red devkit2 - awesome work. |
|
@vincentkoc have you try compiling the firmware code yet? |
|
@beastoin im happy to test in 5 hours or so, i wont flash my device as im worried it could brick and only have 1 for testing / firmware updates |
|
@beastoin build succeeded - fyi overlay file was wrong (not sure if my PR was merged or this PR was not rebased). 
There are a LOT of warnings and same stuff CodeRabbit has picked up which would be great to clean-up. |
|
@vincentkoc super happy to send you a dk2. discord dm pls sir / thank you 💪 |
|
preparing for the merge. |
4 participants
We use system off and manual device turnoffs to further reduce battery drainage. This PR also considers cases where you plug in with the usb, press the button, etc. Now, when you press the button after you long press, it will activate the device rather than turning it on when long pressing.
Issue: #1193
Summary by CodeRabbit
Release Notes
New Features
bt_off()for enhanced transport management.activate_everything_no_lights()for component initialization without LED activation.Improvements
Bug Fixes
These updates enhance device functionality, optimize resource management, and improve user experience.