Skip to content

hotfix - miximum 50 files for each syncing time #7163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TuEmb wants to merge 3 commits into
base: main
Choose a base branch
from
+91 −42
Open

hotfix - miximum 50 files for each syncing time #7163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63e0f5d
miximum 50 files for each syncing
TuEmb May 4, 2026
3277adf
revert PR 6822
TuEmb May 4, 2026
56e0d3b
add STORAGE_NOT_READY status code return
TuEmb May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions omi/firmware/omi/omi.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ CONFIG_REGULATOR=y
CONFIG_ENTROPY_GENERATOR=y

CONFIG_BT=y
CONFIG_BT_SETTINGS=y
CONFIG_BT_SMP=y
CONFIG_BT_SETTINGS=y
CONFIG_BT_PERIPHERAL=y
CONFIG_BT_DIS=n
CONFIG_BT_BAS=n
Expand Down Expand Up @@ -113,7 +111,6 @@ CONFIG_BT_PERIPHERAL=y
CONFIG_BT_MAX_CONN=1
# CONFIG_BT_EXT_ADV_MAX_ADV_SET=2
CONFIG_BT_MAX_PAIRED=1
CONFIG_BT_KEYS_OVERWRITE_OLDEST=y
CONFIG_BT_DEVICE_APPEARANCE=22
CONFIG_BT_GATT_DYNAMIC_DB=y

Expand Down
106 changes: 91 additions & 15 deletions omi/firmware/omi/src/lib/core/storage.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ static uint32_t current_read_offset = 0;
#define INVALID_COMMAND 6
#define FILE_NOT_FOUND 7
#define FILE_INDEX_OUT_OF_RANGE 8
#define STORAGE_NOT_READY 9
#define STORAGE_LIST_MAX_FILES_PER_RESPONSE 50
#define STORAGE_FILE_LIST_ENTRY_SIZE 8

#define MAX_HEARTBEAT_FRAMES 100
#define HEARTBEAT 50
Expand All @@ -48,6 +51,7 @@ static int sync_file_count = 0;
static int current_sync_file_index = -1;
static uint8_t list_files_requested = 0; /* Deferred to storage thread */
static int8_t delete_file_index = -1; /* -1 = no delete, >=0 = file index to delete */
static uint8_t transfer_end_status = 0;

static void storage_config_changed_handler(const struct bt_gatt_attr *attr, uint16_t value);
static ssize_t storage_write_handler(struct bt_conn *conn,
Expand Down Expand Up @@ -214,6 +218,19 @@ static void sync_speed_add_bytes(uint32_t bytes)
}
}

static uint8_t storage_status_from_error(int err, uint8_t fallback_status)
{
switch (err) {
case -ETIMEDOUT:
case -EBUSY:
case -ECANCELED:
case -EAGAIN:
return STORAGE_NOT_READY;
default:
return fallback_status;
}
}

static uint16_t get_ble_chunk_size(struct bt_conn *conn, uint8_t include_timestamp)
{
if (!conn) {
Expand Down Expand Up @@ -265,19 +282,44 @@ static int refresh_file_list_cache(void)
*/
static int send_file_list_response(struct bt_conn *conn)
{
if (sync_file_count == 0 && refresh_file_list_cache() < 0) {
uint8_t error_resp[1] = {0xFF};
storage_notify(conn, error_resp, 1);
return -1;
if (sync_file_count == 0) {
int ret = refresh_file_list_cache();
if (ret < 0) {
uint8_t error_resp[1] = {storage_status_from_error(ret, STORAGE_NOT_READY)};
storage_notify(conn, error_resp, 1);
return ret;
}
}

if (sync_file_count == 0) {
uint8_t empty_resp[1] = {0};
storage_notify(conn, empty_resp, 1);
return 0;
}

uint16_t att_payload = 20;
if (conn) {
uint16_t mtu = bt_gatt_get_mtu(conn);
if (mtu > 3U) {
att_payload = mtu - 3U;
}
}

int max_files_by_payload = (att_payload > 1U) ? ((att_payload - 1U) / STORAGE_FILE_LIST_ENTRY_SIZE) : 0;
int response_file_count = MIN(sync_file_count, STORAGE_LIST_MAX_FILES_PER_RESPONSE);
response_file_count = MIN(response_file_count, max_files_by_payload);

if (response_file_count < 0) {
response_file_count = 0;
}
Comment on lines +308 to +314
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 4, 2026

Choose a reason for hiding this comment

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

P1 Payload-based cap can silently break pagination protocol

max_files_by_payload is used as a hard upper bound on response_file_count, but the app's "request more" trigger is exactly response_file_count == STORAGE_LIST_MAX_FILES_PER_RESPONSE (50). If the negotiated MTU yields a payload where max_files_by_payload < 50 (any MTU below ~404 bytes, i.e., att_payload < 401), the device will send fewer than 50 entries even when 50+ files exist. The app treats anything less than 50 as "done" and stops requesting, leaving remaining files stranded on the device.

For example: MTU = 350 → att_payload = 347 → max_files_by_payload = (347-1)/8 = 43. With 60 files on disk, response_file_count = min(60, 50, 43) = 43. The app receives 43, concludes syncing is complete, and never retrieves the remaining 17 files.

The 50-file cap alone already guarantees the payload stays within 1 + 50×8 = 401 bytes (plus ATT overhead of 3), fitting any MTU ≥ 404. The max_files_by_payload secondary cap is redundant in the common case and silently breaks pagination in the edge case. Consider removing it, or adding a "more files available" flag so the app can continue paginating regardless of batch size.

Comment on lines +312 to +314
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 4, 2026

Choose a reason for hiding this comment

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

P2 Dead-code negative guard

response_file_count is the result of two successive MIN calls where both operands are always non-negative (sync_file_count >= 1 at this point, STORAGE_LIST_MAX_FILES_PER_RESPONSE = 50 > 0, and max_files_by_payload is derived from an unsigned division). The < 0 branch can never be taken.

Suggested change
if (response_file_count < 0) {
response_file_count = 0;
}
/* response_file_count is always >= 0 here; no clamp needed */


/* Use storage_buffer to build response (max 4440 bytes) */
/* Each file: ts(4) + size(4) = 8 bytes, max ~550 files */
int resp_len = 0;

storage_buffer[resp_len++] = (uint8_t)sync_file_count;
storage_buffer[resp_len++] = (uint8_t)response_file_count;

for (int i = 0; i < sync_file_count && resp_len + 8 <= STORAGE_BUFFER_SIZE; i++) {
for (int i = 0; i < response_file_count && resp_len + STORAGE_FILE_LIST_ENTRY_SIZE <= STORAGE_BUFFER_SIZE; i++) {
uint32_t timestamp = (uint32_t)strtoul(sync_file_list[i], NULL, 16);
uint32_t size = sync_file_sizes[i];

Expand All @@ -292,7 +334,11 @@ static int send_file_list_response(struct bt_conn *conn)
storage_buffer[resp_len++] = size & 0xFF;
}

LOG_INF("Sending file list: %d files, %d bytes", sync_file_count, resp_len);
LOG_INF("Sending file list: %d/%d files, %d bytes (att_payload=%u)",
response_file_count,
sync_file_count,
resp_len,
att_payload);
return storage_notify(conn, storage_buffer, resp_len);
}

Expand All @@ -309,6 +355,7 @@ static int setup_file_transfer(int file_index, uint32_t start_offset)
strncpy(current_read_filename, sync_file_list[file_index], MAX_FILENAME_LEN - 1);
current_read_offset = start_offset;
current_sync_file_index = file_index;
transfer_end_status = 0;

if (current_read_offset < sync_file_sizes[file_index]) {
remaining_length = sync_file_sizes[file_index] - current_read_offset;
Expand Down Expand Up @@ -372,7 +419,12 @@ static uint8_t parse_storage_command(void *buf, uint16_t len, struct bt_conn *co
((uint8_t *) buf)[4] << 8 | ((uint8_t *) buf)[5];
}

if (sync_file_count == 0) refresh_file_list_cache();
if (sync_file_count == 0) {
int ret = refresh_file_list_cache();
if (ret < 0) {
return storage_status_from_error(ret, STORAGE_NOT_READY);
}
}

if (file_index >= sync_file_count) {
return FILE_INDEX_OUT_OF_RANGE;
Expand Down Expand Up @@ -498,6 +550,7 @@ static void write_to_gatt(struct bt_conn *conn)
int r = read_audio_data(current_read_filename, storage_buffer, batch_audio_size, current_read_offset);
if (r <= 0) {
LOG_ERR("Failed to read audio data: %d", r);
transfer_end_status = storage_status_from_error(r, FILE_NOT_FOUND);
remaining_length = 0;
return;
}
Expand Down Expand Up @@ -572,25 +625,36 @@ void storage_write(void)
list_files_requested = 0;
/* Always refresh cache so the response is up-to-date, then send.
* send_file_list_response() will not re-refresh if count > 0. */
refresh_file_list_cache();
if (conn) {
send_file_list_response(conn);
int ret = refresh_file_list_cache();
if (ret < 0) {
uint8_t result = storage_status_from_error(ret, STORAGE_NOT_READY);
storage_notify(conn, &result, 1);
} else {
send_file_list_response(conn);
}
}
}
if (delete_file_index >= 0) {
int8_t idx = delete_file_index;
delete_file_index = -1;
uint8_t result = 0;

/* Ensure file list is cached */
if (sync_file_count == 0) {
refresh_file_list_cache();
int ret = refresh_file_list_cache();
if (ret < 0) {
result = storage_status_from_error(ret, STORAGE_NOT_READY);
}
}

uint8_t result = 0;
if (idx >= sync_file_count) {
if (result == 0 && idx >= sync_file_count) {
result = FILE_INDEX_OUT_OF_RANGE;
} else if (delete_file_by_index(idx) < 0) {
result = FILE_NOT_FOUND;
} else if (result == 0) {
int delete_ret = delete_file_by_index(idx);
if (delete_ret < 0) {
result = storage_status_from_error(delete_ret, FILE_NOT_FOUND);
}
}

if (conn) {
Expand Down Expand Up @@ -627,6 +691,17 @@ void storage_write(void)
if (remaining_length == 0) {
if (stop_started) {
stop_started = 0;
transfer_end_status = 0;
current_sync_file_index = -1;
} else if (transfer_end_status != 0) {
uint8_t result = transfer_end_status;
save_offset(current_read_filename, current_read_offset);
LOG_WRN("Transfer aborted for %s with status %u", current_read_filename, result);
transfer_end_status = 0;
current_sync_file_index = -1;
if (conn) {
(void)storage_notify(conn, &result, 1);
}
} else {
save_offset(current_read_filename, current_read_offset);
LOG_INF("File done: %s", current_read_filename);
Expand Down Expand Up @@ -655,6 +730,7 @@ void storage_write(void)
LOG_PRINTK("done. attempting to download more files\n");
uint8_t stop_result[1] = {100};
(void)storage_notify(get_current_connection(), &stop_result, 1);
current_sync_file_index = -1;
k_msleep(10);
}
}
Expand Down
24 changes: 0 additions & 24 deletions omi/firmware/omi/src/lib/core/transport.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include <zephyr/logging/log.h>
#include <zephyr/settings/settings.h>
#include <shell/shell_bt_nus.h>
#include <zephyr/settings/settings.h>
#include <zephyr/sys/atomic.h>
#include <zephyr/sys/ring_buffer.h>

Expand Down Expand Up @@ -649,14 +648,6 @@ static void _transport_connected(struct bt_conn *conn, uint8_t err)
shell_bt_nus_enable(conn);
}

#if defined(CONFIG_BT_SMP)
/* Request bonding so link keys are persisted by BT settings backend. */
int sec_err = bt_conn_set_security(conn, BT_SECURITY_L2);
if (sec_err && sec_err != -EALREADY) {
LOG_WRN("bt_conn_set_security failed (err %d)", sec_err);
}
#endif

// Notify SD module about BLE connection (flush current file)
#ifdef CONFIG_OMI_ENABLE_OFFLINE_STORAGE
sd_notify_ble_state(true);
Expand Down Expand Up @@ -1285,23 +1276,8 @@ int transport_start()
return err;
}

#if defined(CONFIG_BT_SETTINGS)
err = settings_load_subtree("bt");
if (err == -ENOENT) {
LOG_INF("No persisted BT bond keys yet");
} else if (err) {
LOG_WRN("Failed to load BT settings (err %d)", err);
}
#endif

LOG_INF("Transport bluetooth initialized");

// Load settings AFTER bt_enable so BLE identity address is available
err = settings_load();
if (err) {
LOG_WRN("BLE settings_load failed (err %d), advertising may fail", err);
}

if (IS_ENABLED(CONFIG_SHELL_BT_NUS)) {
err = shell_bt_nus_init();
if (err) {
Expand Down
Loading