Skip to content

Conversation

@TuEmb
Copy link
Contributor

@TuEmb TuEmb commented Nov 26, 2025

No description provided.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to increase the speed of storage syncs over BLE by reducing the connection interval and removing a sleep in the data transfer loop. While these changes will likely improve throughput, they also introduce potential issues with power consumption and connection stability. My review includes two main points:

  1. The new fixed, short connection interval is aggressive and may not be supported by all central devices, and will increase power draw. I've suggested using a range to provide more flexibility.
  2. Removing the k_msleep can lead to a busy-wait loop when BLE buffers are full, which will also consume excessive power. I've recommended switching to a more efficient event-driven approach using callbacks and semaphores.

Addressing these points will help ensure the performance improvements don't come at the cost of significantly reduced battery life or connection reliability.

Comment on lines +210 to +212
CONFIG_BT_PERIPHERAL_PREF_MIN_INT=6
# 7.5ms
CONFIG_BT_PERIPHERAL_PREF_MAX_INT=6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Reducing the connection interval to 7.5ms will significantly increase data throughput, but it will also substantially increase power consumption. Furthermore, requesting a fixed and very short connection interval might cause issues with some central devices (e.g., smartphones) that may not be able to sustain such a fast interval, potentially leading to connection instability. Consider using a slightly larger range for MIN and MAX intervals to give the central device more flexibility.

CONFIG_BT_PERIPHERAL_PREF_MIN_INT=6
# 15ms
CONFIG_BT_PERIPHERAL_PREF_MAX_INT=12


heartbeat_count = (heartbeat_count + 1) % (MAX_HEARTBEAT_FRAMES + 1);
k_msleep(100);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

By removing k_msleep(100), this loop can become a busy-wait loop if the BLE transmit buffers fill up. When bt_gatt_notify() in write_to_gatt() returns an error indicating it cannot accept more data, this loop will spin very quickly, consuming significant CPU resources and power, even with k_yield(). This could negatively impact battery life.

A more power-efficient approach would be to use an event-driven mechanism. For instance, you could use bt_gatt_notify_cb() with a callback that signals a semaphore when the transmission is complete, allowing this thread to sleep until it's possible to send more data.

@TuEmb TuEmb marked this pull request as draft November 26, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant