Skip to content

TSDB feature: Added FDB_TSDB_FIXED_BLOB_SIZE compile-time option to reduce flash usage in fixed-size blob scenarios. #387

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

Merged
armink merged 7 commits into from
Dec 17, 2025
Merged

TSDB feature: Added FDB_TSDB_FIXED_BLOB_SIZE compile-time option to reduce flash usage in fixed-size blob scenarios. #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0fb447a
TSDB feature: Added FDB_TSDB_FIXED_BLOB_SIZE compile-time option to r…
EzraZigenbine Dec 15, 2025
a32bcba
Fix inconsistent indentation noted in PR review
EzraZigenbine Dec 16, 2025
b2bd249
Add warning that enabling this macro will cause incompatibility in Fl…
EzraZigenbine Dec 16, 2025
0d3899a
Improve FLASH_WRITE reusability as requested in PR review
EzraZigenbine Dec 16, 2025
6dcd4ad
tsdb: fix missing log address/size for variable blobs
EzraZigenbine Dec 17, 2025
dc35884
Move fixed-size variables to reduce #ifdef usage (pre-PR review)
EzraZigenbine Dec 17, 2025
cf3275c
fix spelling and formatting per PR review
EzraZigenbine Dec 17, 2025
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
6 changes: 6 additions & 0 deletions inc/fdb_cfg_template.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@
/* using TSDB (Time series database) feature */
#define FDB_USING_TSDB

/* Use fixed-size blobs in TSDB to save flash overhead (8 bytes per entry).
* Define this to the fixed blob size in bytes when all TSL entries are the same size.
* Ideal for logging fixed-size sensor data (e.g., float + timestamp).
* Warning: If defined will be incompatible with variable blob flash store or if fixed blob size is later changed */
/* #define FDB_TSDB_FIXED_BLOB_SIZE 4 */

/* Using FAL storage mode */
#define FDB_USING_FAL_MODE

Expand Down
29 changes: 27 additions & 2 deletions src/fdb_tsdb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,10 @@ typedef struct sector_hdr_data *sector_hdr_data_t;
struct log_idx_data {
uint8_t status_table[TSL_STATUS_TABLE_SIZE]; /**< node status, @see fdb_tsl_status_t */
fdb_time_t time; /**< node timestamp */
#ifndef FDB_TSDB_FIXED_BLOB_SIZE
uint32_t log_len; /**< node total length (header + name + value), must align by FDB_WRITE_GRAN */
uint32_t log_addr; /**< node address */
#endif
Comment on lines +97 to +100
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

Critical design issue: LOG_IDX_DATA_SIZE is defined as FDB_WG_ALIGN(sizeof(struct log_idx_data)) and is used throughout the codebase for sector layout calculations, address computations, and space accounting. When FDB_TSDB_FIXED_BLOB_SIZE is enabled, the struct size changes (removing 8 bytes), which changes LOG_IDX_DATA_SIZE. This creates an incompatible on-flash format - databases created with this feature enabled cannot be read by code compiled without it, and vice versa. This breaks the entire storage layout and makes the feature fundamentally incompatible with existing data. Consider using a separate macro or ensuring the struct size remains constant to maintain format compatibility.

Suggested change
#ifndef FDB_TSDB_FIXED_BLOB_SIZE
uint32_t log_len; /**< node total length (header + name + value), must align by FDB_WRITE_GRAN */
uint32_t log_addr; /**< node address */
#endif
uint32_t log_len; /**< node total length (header + name + value), must align by FDB_WRITE_GRAN */
uint32_t log_addr; /**< node address */

Copilot uses AI. Check for mistakes.
Copy link
Owner

@armink armink Dec 16, 2025

Choose a reason for hiding this comment

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

Ignoring the AI's comment for now, enabling this macro will indeed cause incompatibility in the data format stored in Flash. Just specify this when configuring the macro.

Copy link
Contributor Author

@EzraZigenbine EzraZigenbine Dec 16, 2025

Choose a reason for hiding this comment

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

updated see link

};
typedef struct log_idx_data *log_idx_data_t;

Expand All @@ -114,6 +116,7 @@ struct check_sec_hdr_cb_args {
static fdb_err_t read_tsl(fdb_tsdb_t db, fdb_tsl_t tsl)
{
struct log_idx_data idx;

/* read TSL index raw data */
_fdb_flash_read((fdb_db_t)db, tsl->addr.index, (uint32_t *) &idx, sizeof(struct log_idx_data));
tsl->status = (fdb_tsl_status_t) _fdb_get_status(idx.status_table, FDB_TSL_STATUS_NUM);
Expand All @@ -122,9 +125,19 @@ static fdb_err_t read_tsl(fdb_tsdb_t db, fdb_tsl_t tsl)
tsl->addr.log = FDB_DATA_UNUSED;
tsl->time = 0;
} else {
#ifdef FDB_TSDB_FIXED_BLOB_SIZE
Copy link
Owner

@armink armink Dec 17, 2025

Choose a reason for hiding this comment

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

this code can move to here

   uint32_t tsl_index_in_sector;
    uint32_t sector_addr;

Copy link
Contributor Author

@EzraZigenbine EzraZigenbine Dec 17, 2025

Choose a reason for hiding this comment

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

done

uint32_t tsl_index_in_sector;
uint32_t sector_addr;
tsl->log_len = FDB_TSDB_FIXED_BLOB_SIZE;
sector_addr = FDB_ALIGN_DOWN(tsl->addr.index, db_sec_size(db));
tsl_index_in_sector = (tsl->addr.index - sector_addr - SECTOR_HDR_DATA_SIZE) / LOG_IDX_DATA_SIZE;
tsl->addr.log = sector_addr + db_sec_size(db) - (tsl_index_in_sector + 1) * FDB_WG_ALIGN(FDB_TSDB_FIXED_BLOB_SIZE);
Copy link
Owner

@armink armink Dec 17, 2025

Choose a reason for hiding this comment

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

@copilot Is there a way to simplify the logic here?

tsl->time = idx.time;
#else
tsl->log_len = idx.log_len;
tsl->addr.log = idx.log_addr;
tsl->time = idx.time;
#endif
}

return FDB_NO_ERR;
Expand Down Expand Up @@ -306,16 +319,21 @@ static fdb_err_t write_tsl(fdb_tsdb_t db, fdb_blob_t blob, fdb_time_t time)
fdb_err_t result = FDB_NO_ERR;
struct log_idx_data idx;
uint32_t idx_addr = db->cur_sec.empty_idx;
uint32_t log_addr = db->cur_sec.empty_data - FDB_WG_ALIGN(blob->size);

#ifndef FDB_TSDB_FIXED_BLOB_SIZE
// variable-size blobs must store address and size in flash
idx.log_addr = log_addr;
idx.log_len = blob->size;
#endif
idx.time = time;
idx.log_addr = db->cur_sec.empty_data - FDB_WG_ALIGN(idx.log_len);

/* write the status will by write granularity */
_FDB_WRITE_STATUS(db, idx_addr, idx.status_table, FDB_TSL_STATUS_NUM, FDB_TSL_PRE_WRITE, false);
/* write other index info */
FLASH_WRITE(db, idx_addr + LOG_IDX_TS_OFFSET, &idx.time, sizeof(struct log_idx_data) - LOG_IDX_TS_OFFSET, false);
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

Critical bug: When FDB_TSDB_FIXED_BLOB_SIZE is defined, the log_idx_data structure no longer contains log_len and log_addr fields. This write operation attempts to write "sizeof(struct log_idx_data) - LOG_IDX_TS_OFFSET" bytes from &idx.time, which would only write the time field. However, in the non-fixed-size case, this writes time, log_len, and log_addr. The size calculation needs to be conditional on FDB_TSDB_FIXED_BLOB_SIZE to ensure the correct number of bytes are written.

Suggested change
FLASH_WRITE(db, idx_addr + LOG_IDX_TS_OFFSET, &idx.time, sizeof(struct log_idx_data) - LOG_IDX_TS_OFFSET, false);
#ifdef FDB_TSDB_FIXED_BLOB_SIZE
FLASH_WRITE(db, idx_addr + LOG_IDX_TS_OFFSET, &idx.time, sizeof(idx.time), false);
#else
FLASH_WRITE(db, idx_addr + LOG_IDX_TS_OFFSET, &idx.time, sizeof(struct log_idx_data) - LOG_IDX_TS_OFFSET, false);
#endif

Copilot uses AI. Check for mistakes.
Copy link
Owner

@armink armink Dec 16, 2025

Choose a reason for hiding this comment

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

This AI comments can be ignored

/* write blob data */
FLASH_WRITE(db, idx.log_addr, blob->buf, blob->size, false);
FLASH_WRITE(db, log_addr, blob->buf, blob->size, false);
/* write the status will by write granularity */
_FDB_WRITE_STATUS(db, idx_addr, idx.status_table, FDB_TSL_STATUS_NUM, FDB_TSL_WRITE, true);

Expand Down Expand Up @@ -388,13 +406,20 @@ static fdb_err_t tsl_append(fdb_tsdb_t db, fdb_blob_t blob, fdb_time_t *timestam
fdb_err_t result = FDB_NO_ERR;
fdb_time_t cur_time = timestamp == NULL ? db->get_time() : *timestamp;

#ifdef FDB_TSDB_FIXED_BLOB_SIZE
if (blob->size != FDB_TSDB_FIXED_BLOB_SIZE) {
FDB_INFO("Error: blob size (%zu) must equal FDB_TSDB_FIXED_BLOB_SIZE (%d)\n", blob->size, FDB_TSDB_FIXED_BLOB_SIZE);
return FDB_WRITE_ERR;
}
#else
/* check the append length, MUST less than the db->max_len */
if(blob->size > db->max_len)
{
FDB_INFO("Warning: append length (%" PRIdMAX ") is more than the db->max_len (%" PRIdMAX "). This tsl will be dropped.\n",
(intmax_t)blob->size, (intmax_t)(db->max_len));
return FDB_WRITE_ERR;
}
#endif

/* check the current timestamp, MUST more than the last save timestamp */
if (cur_time <= db->last_time) {
Expand Down