kasjer commented on code in PR #3365:
URL: https://github.com/apache/mynewt-core/pull/3365#discussion_r1976724471
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -22,17 +22,151 @@
#include "os/mynewt.h"
#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-
+#include "log/log.h"
#include "log/log_fcb.h"
-void
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+static int
+log_fcb_init_sector_bmarks(struct fcb_log *fcb_log)
+{
+ int rc = 0;
+ int i = 0;
+ int j = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ struct log_entry_hdr ueh = {0};
+#if MYNEWT_VAL(LOG_FCB)
+ struct fcb_entry loc = {0};
+ struct flash_area *fa = NULL;
+
+ rc = fcb_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#else
+ struct flash_sector_range *range = NULL;
+ struct fcb2_entry loc = {0};
+
+ rc = fcb2_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#endif
+
+ /* Start adding a bookmark from the end of array just before
+ * non-sector bookmarks
+ */
+ bset->lfs_next_sect = bset->lfs_sect_cap - 1;
+ for (i = 0; i < bset->lfs_sect_cap; i++) {
+ rc = log_read_hdr(fcb_log->fl_log, &loc, &ueh);
+ if (rc) {
+ /* Read failed, don't add a bookmark, done adding bookmarks */
+ rc = SYS_EOK;
+ break;
+ }
+
+ rc = log_fcb_add_bmark(fcb_log, &loc, ueh.ue_index, true);
+ if (rc) {
+ return rc;
+ }
+
+#if MYNEWT_VAL(LOG_FCB)
+ j = 0;
+
+ /* Keep skipping sectors until lfs_sect_bmark_itvl is reached */
+ do {
+ fa = fcb_getnext_area(&fcb_log->fl_fcb, loc.fe_area);
+ if (!fa) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!fa) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb_getnext_in_area(&fcb_log->fl_fcb, fa, &loc);
+ if (rc) {
+ break;
+ }
+#else
+ j = 0;
+
+ /* Keep skipping rangess until lfs_sect_bmark_itvl is reached */
+ do {
+ /* Get next range */
+ range = fcb2_getnext_range(&fcb_log->fl_fcb, &loc);
+ if (!range) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!range) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb2_getnext_in_area(&fcb_log->fl_fcb, range, &loc);
+ if (rc) {
+ break;
+ }
+#endif
+ }
+
+ return rc;
+}
+#endif
+
+int
log_fcb_init_bmarks(struct fcb_log *fcb_log,
- struct log_fcb_bmark *buf, int bmark_count)
+ struct log_fcb_bmark *buf, int avl_bmark_cnt,
Review Comment:
Unnecessary change of variable name (mismatch from prototype that is in the
header).
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -61,74 +195,256 @@ log_fcb_rotate_bmarks(struct fcb_log *fcb_log)
i--;
}
bset->lfs_size--;
- bset->lfs_next = bset->lfs_size;
}
}
void
log_fcb_clear_bmarks(struct fcb_log *fcb_log)
{
fcb_log->fl_bset.lfs_size = 0;
- fcb_log->fl_bset.lfs_next = 0;
+ fcb_log->fl_bset.lfs_next_non_sect = 0;
+ fcb_log->fl_bset.lfs_non_sect_size = 0;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ if (diff < *min_diff) {
+ *min_diff = diff;
closest = bmark;
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "index: %u, closest bmark
idx: %u, \n",
+ (unsigned int)index,
+ (unsigned int)bmark->lfb_index);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ bset->lfs_bmarks[bset->lfs_next_sect] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ };
+
+ if (bset->lfs_size < fcb_log->fl_bset.lfs_sect_cap) {
+ bset->lfs_size++;
+ bset->lfs_next_sect = (bset->lfs_next_sect - 1) %
+ fcb_log->fl_bset.lfs_sect_cap;
+ } else {
+ return -1;
}
+ return 0;
+}
+#endif
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+#if MYNEWT_VAL(LOG_FCB)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry
*entry,
+ uint32_t index, int pos)
+#elif MYNEWT_VAL(LOG_FCB2)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry
*entry,
+ uint32_t index, int pos)
+#endif
+{
+ int i = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (bset->lfs_en_sect_bmarks) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_sect_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ } else
+#endif
+ {
+ for (i = 0; i < bset->lfs_non_sect_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ }
+
+ bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
.lfb_entry = *entry,
.lfb_index = index,
};
- if (bset->lfs_size < bset->lfs_cap) {
- bset->lfs_size++;
+ return 0;
+}
+
+#if MYNEWT_VAL(LOG_FCB)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark)
+#elif MYNEWT_VAL(LOG_FCB2)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index, bool sect_bmark)
+#endif
+{
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int i = 0;
+ int pos = 0;
+ bool en_sect_bmark = false;
+ int rc = 0;
+
+ (void)i;
+ (void)pos;
+
+ if (bset->lfs_cap == 0) {
+ return SYS_ENOMEM;
+ }
+
+ en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
+ (void)en_sect_bmark;
Review Comment:
This code is unnecessary complicated.
Variable `en_sect_bmark` (with obfuscated name) is initialized twice
later marked with `(void)mark;` as if not used
then used only in once condition that could be just
```c
if (sect_bmark & bset->lfs_en_sect_bmarks) {
...
```
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -22,17 +22,151 @@
#include "os/mynewt.h"
#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-
+#include "log/log.h"
#include "log/log_fcb.h"
-void
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+static int
+log_fcb_init_sector_bmarks(struct fcb_log *fcb_log)
+{
+ int rc = 0;
+ int i = 0;
+ int j = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ struct log_entry_hdr ueh = {0};
+#if MYNEWT_VAL(LOG_FCB)
+ struct fcb_entry loc = {0};
+ struct flash_area *fa = NULL;
+
+ rc = fcb_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#else
+ struct flash_sector_range *range = NULL;
+ struct fcb2_entry loc = {0};
+
+ rc = fcb2_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#endif
+
+ /* Start adding a bookmark from the end of array just before
+ * non-sector bookmarks
+ */
+ bset->lfs_next_sect = bset->lfs_sect_cap - 1;
+ for (i = 0; i < bset->lfs_sect_cap; i++) {
+ rc = log_read_hdr(fcb_log->fl_log, &loc, &ueh);
+ if (rc) {
+ /* Read failed, don't add a bookmark, done adding bookmarks */
+ rc = SYS_EOK;
+ break;
+ }
+
+ rc = log_fcb_add_bmark(fcb_log, &loc, ueh.ue_index, true);
+ if (rc) {
+ return rc;
+ }
+
+#if MYNEWT_VAL(LOG_FCB)
+ j = 0;
+
+ /* Keep skipping sectors until lfs_sect_bmark_itvl is reached */
+ do {
+ fa = fcb_getnext_area(&fcb_log->fl_fcb, loc.fe_area);
+ if (!fa) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!fa) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb_getnext_in_area(&fcb_log->fl_fcb, fa, &loc);
+ if (rc) {
+ break;
+ }
+#else
+ j = 0;
+
+ /* Keep skipping rangess until lfs_sect_bmark_itvl is reached */
+ do {
+ /* Get next range */
+ range = fcb2_getnext_range(&fcb_log->fl_fcb, &loc);
+ if (!range) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!range) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb2_getnext_in_area(&fcb_log->fl_fcb, range, &loc);
+ if (rc) {
+ break;
+ }
+#endif
+ }
+
+ return rc;
+}
+#endif
+
+int
log_fcb_init_bmarks(struct fcb_log *fcb_log,
- struct log_fcb_bmark *buf, int bmark_count)
+ struct log_fcb_bmark *buf, int avl_bmark_cnt,
+ bool en_sect_bmarks)
{
- fcb_log->fl_bset = (struct log_fcb_bset) {
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int reqd_bmark_cnt = MYNEWT_VAL(LOG_FCB_NUM_ABS_BOOKMARKS);
+
+ (void)reqd_bmark_cnt;
+ if (!bset || !buf || !avl_bmark_cnt) {
+ return SYS_EINVAL;
+ }
+
+ memset(bset, 0, sizeof(*bset));
Review Comment:
memset is not needed, bset is initialized few lines below
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -61,74 +195,256 @@ log_fcb_rotate_bmarks(struct fcb_log *fcb_log)
i--;
}
bset->lfs_size--;
- bset->lfs_next = bset->lfs_size;
}
}
void
log_fcb_clear_bmarks(struct fcb_log *fcb_log)
{
fcb_log->fl_bset.lfs_size = 0;
- fcb_log->fl_bset.lfs_next = 0;
+ fcb_log->fl_bset.lfs_next_non_sect = 0;
+ fcb_log->fl_bset.lfs_non_sect_size = 0;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ if (diff < *min_diff) {
+ *min_diff = diff;
closest = bmark;
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "index: %u, closest bmark
idx: %u, \n",
+ (unsigned int)index,
+ (unsigned int)bmark->lfb_index);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ bset->lfs_bmarks[bset->lfs_next_sect] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ };
+
+ if (bset->lfs_size < fcb_log->fl_bset.lfs_sect_cap) {
+ bset->lfs_size++;
+ bset->lfs_next_sect = (bset->lfs_next_sect - 1) %
+ fcb_log->fl_bset.lfs_sect_cap;
+ } else {
+ return -1;
}
+ return 0;
+}
+#endif
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+#if MYNEWT_VAL(LOG_FCB)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry
*entry,
+ uint32_t index, int pos)
+#elif MYNEWT_VAL(LOG_FCB2)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry
*entry,
+ uint32_t index, int pos)
+#endif
+{
+ int i = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (bset->lfs_en_sect_bmarks) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_sect_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ } else
+#endif
+ {
+ for (i = 0; i < bset->lfs_non_sect_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ }
+
+ bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
.lfb_entry = *entry,
.lfb_index = index,
};
- if (bset->lfs_size < bset->lfs_cap) {
- bset->lfs_size++;
+ return 0;
+}
+
+#if MYNEWT_VAL(LOG_FCB)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark)
+#elif MYNEWT_VAL(LOG_FCB2)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index, bool sect_bmark)
+#endif
+{
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int i = 0;
+ int pos = 0;
+ bool en_sect_bmark = false;
+ int rc = 0;
+
+ (void)i;
+ (void)pos;
+
+ if (bset->lfs_cap == 0) {
+ return SYS_ENOMEM;
+ }
+
+ en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
+ (void)en_sect_bmark;
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (en_sect_bmark) {
+ rc = log_fcb_insert_sect_bmark(fcb_log, entry, index);
+ if (rc) {
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "insert bmark failure: %u\n",
+ index);
+ return rc;
+ }
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "insert bmark index: %u, pos: %u\n",
+ index, bset->lfs_next_sect);
+ } else {
+ /* Replace oldest non-sector bmark */
+ rc = log_fcb_replace_non_sect_bmark(fcb_log, entry, index,
+ bset->lfs_next_non_sect +
+ (bset->lfs_en_sect_bmarks ?
+ fcb_log->fl_fcb.f_sector_cnt :
0));
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "replace bmark index: %u, pos: %u\n",
+ index, bset->lfs_next_non_sect +
+ (bset->lfs_en_sect_bmarks ?
+ fcb_log->fl_fcb.f_sector_cnt : 0));
+
+ if (bset->lfs_non_sect_size < MYNEWT_VAL(LOG_FCB_NUM_ABS_BOOKMARKS)) {
+ bset->lfs_non_sect_size++;
+ bset->lfs_size++;
Review Comment:
If bookmark was already there, size will be incorrect and there will be
empty spaces in the bookmark table
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -22,17 +22,151 @@
#include "os/mynewt.h"
#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-
+#include "log/log.h"
#include "log/log_fcb.h"
-void
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+static int
+log_fcb_init_sector_bmarks(struct fcb_log *fcb_log)
+{
+ int rc = 0;
+ int i = 0;
+ int j = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ struct log_entry_hdr ueh = {0};
+#if MYNEWT_VAL(LOG_FCB)
+ struct fcb_entry loc = {0};
+ struct flash_area *fa = NULL;
+
+ rc = fcb_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#else
+ struct flash_sector_range *range = NULL;
+ struct fcb2_entry loc = {0};
+
+ rc = fcb2_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#endif
+
+ /* Start adding a bookmark from the end of array just before
+ * non-sector bookmarks
+ */
+ bset->lfs_next_sect = bset->lfs_sect_cap - 1;
+ for (i = 0; i < bset->lfs_sect_cap; i++) {
+ rc = log_read_hdr(fcb_log->fl_log, &loc, &ueh);
+ if (rc) {
+ /* Read failed, don't add a bookmark, done adding bookmarks */
+ rc = SYS_EOK;
+ break;
+ }
+
+ rc = log_fcb_add_bmark(fcb_log, &loc, ueh.ue_index, true);
+ if (rc) {
+ return rc;
+ }
+
+#if MYNEWT_VAL(LOG_FCB)
+ j = 0;
+
+ /* Keep skipping sectors until lfs_sect_bmark_itvl is reached */
+ do {
+ fa = fcb_getnext_area(&fcb_log->fl_fcb, loc.fe_area);
+ if (!fa) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!fa) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb_getnext_in_area(&fcb_log->fl_fcb, fa, &loc);
+ if (rc) {
+ break;
+ }
+#else
+ j = 0;
+
+ /* Keep skipping rangess until lfs_sect_bmark_itvl is reached */
+ do {
+ /* Get next range */
+ range = fcb2_getnext_range(&fcb_log->fl_fcb, &loc);
+ if (!range) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!range) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb2_getnext_in_area(&fcb_log->fl_fcb, range, &loc);
+ if (rc) {
+ break;
+ }
+#endif
+ }
+
+ return rc;
+}
+#endif
+
+int
log_fcb_init_bmarks(struct fcb_log *fcb_log,
- struct log_fcb_bmark *buf, int bmark_count)
+ struct log_fcb_bmark *buf, int avl_bmark_cnt,
+ bool en_sect_bmarks)
{
- fcb_log->fl_bset = (struct log_fcb_bset) {
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int reqd_bmark_cnt = MYNEWT_VAL(LOG_FCB_NUM_ABS_BOOKMARKS);
Review Comment:
Obfuscated variable name. It's hard to guess it's meaning.
##########
sys/log/full/include/log/log_fcb.h:
##########
@@ -125,39 +157,46 @@ void log_fcb_init_bmarks(struct fcb_log *fcb_log,
void log_fcb_clear_bmarks(struct fcb_log *fcb_log);
/**
- * @brief Remove bookmarks which point to oldest FCB/FCB2 area. This is
- * meant to get called just before the area is rotated out.
+ * @brief Get bookmarks for a particular log
*
- * @param fcb_log The fcb_log to operate on.
+ * @param log Pointer to the log we want to read bookmarks from
+ * @param bmarks_size Pointer to the variable we want to read bookmarks into
+ *
+ * @return Pointer to the bookmarks array for the provided log
*/
-void log_fcb_rotate_bmarks(struct fcb_log *fcb_log);
+struct log_fcb_bmark *log_fcb_get_bmarks(struct log *log, uint32_t
*bmarks_size);
/**
* @brief Searches an fcb_log for the closest bookmark that comes before or at
* the specified index.
*
* @param fcb_log The log to search.
* @param index The index to look for.
+ * @param min_diff If bookmark was found, fill it up with the
difference
+ * else it will return -1.
*
* @return The closest bookmark on success;
* NULL if the log has no applicable bookmarks.
*/
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index);
+struct log_fcb_bmark *
Review Comment:
Is there a reason for dropping `const` here?
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -22,17 +22,151 @@
#include "os/mynewt.h"
#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-
+#include "log/log.h"
#include "log/log_fcb.h"
-void
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+static int
+log_fcb_init_sector_bmarks(struct fcb_log *fcb_log)
+{
+ int rc = 0;
+ int i = 0;
+ int j = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ struct log_entry_hdr ueh = {0};
+#if MYNEWT_VAL(LOG_FCB)
+ struct fcb_entry loc = {0};
+ struct flash_area *fa = NULL;
+
+ rc = fcb_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#else
+ struct flash_sector_range *range = NULL;
+ struct fcb2_entry loc = {0};
+
+ rc = fcb2_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#endif
+
+ /* Start adding a bookmark from the end of array just before
+ * non-sector bookmarks
+ */
+ bset->lfs_next_sect = bset->lfs_sect_cap - 1;
+ for (i = 0; i < bset->lfs_sect_cap; i++) {
+ rc = log_read_hdr(fcb_log->fl_log, &loc, &ueh);
+ if (rc) {
+ /* Read failed, don't add a bookmark, done adding bookmarks */
+ rc = SYS_EOK;
+ break;
+ }
+
+ rc = log_fcb_add_bmark(fcb_log, &loc, ueh.ue_index, true);
+ if (rc) {
+ return rc;
+ }
+
+#if MYNEWT_VAL(LOG_FCB)
+ j = 0;
+
+ /* Keep skipping sectors until lfs_sect_bmark_itvl is reached */
+ do {
+ fa = fcb_getnext_area(&fcb_log->fl_fcb, loc.fe_area);
+ if (!fa) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
Review Comment:
This loop does not do what comment claims. It does not skip sectors just
calculate same values several times
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -61,74 +195,256 @@ log_fcb_rotate_bmarks(struct fcb_log *fcb_log)
i--;
}
bset->lfs_size--;
- bset->lfs_next = bset->lfs_size;
}
}
void
log_fcb_clear_bmarks(struct fcb_log *fcb_log)
{
fcb_log->fl_bset.lfs_size = 0;
- fcb_log->fl_bset.lfs_next = 0;
+ fcb_log->fl_bset.lfs_next_non_sect = 0;
+ fcb_log->fl_bset.lfs_non_sect_size = 0;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ if (diff < *min_diff) {
+ *min_diff = diff;
closest = bmark;
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "index: %u, closest bmark
idx: %u, \n",
+ (unsigned int)index,
+ (unsigned int)bmark->lfb_index);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ bset->lfs_bmarks[bset->lfs_next_sect] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ };
+
+ if (bset->lfs_size < fcb_log->fl_bset.lfs_sect_cap) {
+ bset->lfs_size++;
+ bset->lfs_next_sect = (bset->lfs_next_sect - 1) %
+ fcb_log->fl_bset.lfs_sect_cap;
+ } else {
+ return -1;
}
+ return 0;
+}
+#endif
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+#if MYNEWT_VAL(LOG_FCB)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry
*entry,
+ uint32_t index, int pos)
+#elif MYNEWT_VAL(LOG_FCB2)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry
*entry,
+ uint32_t index, int pos)
+#endif
+{
+ int i = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (bset->lfs_en_sect_bmarks) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_sect_size + fcb_log->fl_fcb.f_sector_cnt);
Review Comment:
`f_sector_cnt` should not be used in this context
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -61,74 +195,256 @@ log_fcb_rotate_bmarks(struct fcb_log *fcb_log)
i--;
}
bset->lfs_size--;
- bset->lfs_next = bset->lfs_size;
}
}
void
log_fcb_clear_bmarks(struct fcb_log *fcb_log)
{
fcb_log->fl_bset.lfs_size = 0;
- fcb_log->fl_bset.lfs_next = 0;
+ fcb_log->fl_bset.lfs_next_non_sect = 0;
+ fcb_log->fl_bset.lfs_non_sect_size = 0;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ if (diff < *min_diff) {
+ *min_diff = diff;
closest = bmark;
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "index: %u, closest bmark
idx: %u, \n",
+ (unsigned int)index,
+ (unsigned int)bmark->lfb_index);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ bset->lfs_bmarks[bset->lfs_next_sect] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ };
+
+ if (bset->lfs_size < fcb_log->fl_bset.lfs_sect_cap) {
+ bset->lfs_size++;
+ bset->lfs_next_sect = (bset->lfs_next_sect - 1) %
+ fcb_log->fl_bset.lfs_sect_cap;
+ } else {
+ return -1;
}
+ return 0;
+}
+#endif
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+#if MYNEWT_VAL(LOG_FCB)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry
*entry,
+ uint32_t index, int pos)
+#elif MYNEWT_VAL(LOG_FCB2)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry
*entry,
+ uint32_t index, int pos)
+#endif
+{
+ int i = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (bset->lfs_en_sect_bmarks) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_sect_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ } else
+#endif
+ {
+ for (i = 0; i < bset->lfs_non_sect_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ }
+
+ bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
.lfb_entry = *entry,
.lfb_index = index,
};
- if (bset->lfs_size < bset->lfs_cap) {
- bset->lfs_size++;
+ return 0;
+}
+
+#if MYNEWT_VAL(LOG_FCB)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark)
+#elif MYNEWT_VAL(LOG_FCB2)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index, bool sect_bmark)
+#endif
+{
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int i = 0;
+ int pos = 0;
Review Comment:
Unused variables
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -61,74 +195,256 @@ log_fcb_rotate_bmarks(struct fcb_log *fcb_log)
i--;
}
bset->lfs_size--;
- bset->lfs_next = bset->lfs_size;
}
}
void
log_fcb_clear_bmarks(struct fcb_log *fcb_log)
{
fcb_log->fl_bset.lfs_size = 0;
- fcb_log->fl_bset.lfs_next = 0;
+ fcb_log->fl_bset.lfs_next_non_sect = 0;
+ fcb_log->fl_bset.lfs_non_sect_size = 0;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ if (diff < *min_diff) {
+ *min_diff = diff;
closest = bmark;
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "index: %u, closest bmark
idx: %u, \n",
+ (unsigned int)index,
+ (unsigned int)bmark->lfb_index);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ bset->lfs_bmarks[bset->lfs_next_sect] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ };
+
+ if (bset->lfs_size < fcb_log->fl_bset.lfs_sect_cap) {
+ bset->lfs_size++;
+ bset->lfs_next_sect = (bset->lfs_next_sect - 1) %
+ fcb_log->fl_bset.lfs_sect_cap;
Review Comment:
calculation is incorrect, it would result in some strange value depending on
capacity but it does not work similar to what is done to non sector bookmarks.
Fault is never triggered only because all bookmarks are cleared on FCB
rotation.
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -61,74 +195,256 @@ log_fcb_rotate_bmarks(struct fcb_log *fcb_log)
i--;
}
bset->lfs_size--;
- bset->lfs_next = bset->lfs_size;
}
}
void
log_fcb_clear_bmarks(struct fcb_log *fcb_log)
{
fcb_log->fl_bset.lfs_size = 0;
- fcb_log->fl_bset.lfs_next = 0;
+ fcb_log->fl_bset.lfs_next_non_sect = 0;
+ fcb_log->fl_bset.lfs_non_sect_size = 0;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (i < fcb_log->fl_bset.lfs_sect_cap) {
+ /* Jump to the non-sector bookmarks since sector
+ * bookmarks are empty here on
+ */
+ i = fcb_log->fl_bset.lfs_sect_cap - 1;
+ continue;
+ }
+#else
+ /* Empty non-sector bookmark, nothing more to do
+ * Previous closest bookmark is the closest one */
+ break;
+#endif
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ if (diff < *min_diff) {
+ *min_diff = diff;
closest = bmark;
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "index: %u, closest bmark
idx: %u, \n",
+ (unsigned int)index,
+ (unsigned int)bmark->lfb_index);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static int
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ bset->lfs_bmarks[bset->lfs_next_sect] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ };
+
+ if (bset->lfs_size < fcb_log->fl_bset.lfs_sect_cap) {
+ bset->lfs_size++;
+ bset->lfs_next_sect = (bset->lfs_next_sect - 1) %
+ fcb_log->fl_bset.lfs_sect_cap;
+ } else {
+ return -1;
}
+ return 0;
+}
+#endif
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+#if MYNEWT_VAL(LOG_FCB)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry
*entry,
+ uint32_t index, int pos)
+#elif MYNEWT_VAL(LOG_FCB2)
+static int
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry
*entry,
+ uint32_t index, int pos)
+#endif
+{
+ int i = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (bset->lfs_en_sect_bmarks) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_sect_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ } else
+#endif
+ {
+ for (i = 0; i < bset->lfs_non_sect_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return 0;
+ }
+ }
+ }
+
+ bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
.lfb_entry = *entry,
.lfb_index = index,
};
- if (bset->lfs_size < bset->lfs_cap) {
- bset->lfs_size++;
+ return 0;
+}
+
+#if MYNEWT_VAL(LOG_FCB)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark)
+#elif MYNEWT_VAL(LOG_FCB2)
+int
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index, bool sect_bmark)
+#endif
+{
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int i = 0;
+ int pos = 0;
+ bool en_sect_bmark = false;
+ int rc = 0;
+
+ (void)i;
+ (void)pos;
+
+ if (bset->lfs_cap == 0) {
+ return SYS_ENOMEM;
+ }
+
+ en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
+ (void)en_sect_bmark;
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (en_sect_bmark) {
+ rc = log_fcb_insert_sect_bmark(fcb_log, entry, index);
+ if (rc) {
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "insert bmark failure: %u\n",
+ index);
+ return rc;
+ }
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "insert bmark index: %u, pos: %u\n",
+ index, bset->lfs_next_sect);
+ } else {
+ /* Replace oldest non-sector bmark */
+ rc = log_fcb_replace_non_sect_bmark(fcb_log, entry, index,
+ bset->lfs_next_non_sect +
+ (bset->lfs_en_sect_bmarks ?
+ fcb_log->fl_fcb.f_sector_cnt :
0));
Review Comment:
`f_sector_cnt` should not be used for starting point of non sector bookmarks
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -22,17 +22,151 @@
#include "os/mynewt.h"
#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-
+#include "log/log.h"
#include "log/log_fcb.h"
-void
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+static int
+log_fcb_init_sector_bmarks(struct fcb_log *fcb_log)
+{
+ int rc = 0;
+ int i = 0;
+ int j = 0;
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ struct log_entry_hdr ueh = {0};
+#if MYNEWT_VAL(LOG_FCB)
+ struct fcb_entry loc = {0};
+ struct flash_area *fa = NULL;
+
+ rc = fcb_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#else
+ struct flash_sector_range *range = NULL;
+ struct fcb2_entry loc = {0};
+
+ rc = fcb2_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+#endif
+
+ /* Start adding a bookmark from the end of array just before
+ * non-sector bookmarks
+ */
+ bset->lfs_next_sect = bset->lfs_sect_cap - 1;
+ for (i = 0; i < bset->lfs_sect_cap; i++) {
+ rc = log_read_hdr(fcb_log->fl_log, &loc, &ueh);
+ if (rc) {
+ /* Read failed, don't add a bookmark, done adding bookmarks */
+ rc = SYS_EOK;
+ break;
+ }
+
+ rc = log_fcb_add_bmark(fcb_log, &loc, ueh.ue_index, true);
+ if (rc) {
+ return rc;
+ }
+
+#if MYNEWT_VAL(LOG_FCB)
+ j = 0;
+
+ /* Keep skipping sectors until lfs_sect_bmark_itvl is reached */
+ do {
+ fa = fcb_getnext_area(&fcb_log->fl_fcb, loc.fe_area);
+ if (!fa) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!fa) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb_getnext_in_area(&fcb_log->fl_fcb, fa, &loc);
+ if (rc) {
+ break;
+ }
+#else
+ j = 0;
+
+ /* Keep skipping rangess until lfs_sect_bmark_itvl is reached */
+ do {
+ /* Get next range */
+ range = fcb2_getnext_range(&fcb_log->fl_fcb, &loc);
+ if (!range) {
+ break;
+ }
+ j++;
+ } while (j < bset->lfs_sect_bmark_itvl);
+
+ if (!range) {
+ break;
+ }
+
+ /* First entry in the next area */
+ rc = fcb2_getnext_in_area(&fcb_log->fl_fcb, range, &loc);
+ if (rc) {
+ break;
+ }
+#endif
+ }
+
+ return rc;
+}
+#endif
+
+int
log_fcb_init_bmarks(struct fcb_log *fcb_log,
- struct log_fcb_bmark *buf, int bmark_count)
+ struct log_fcb_bmark *buf, int avl_bmark_cnt,
+ bool en_sect_bmarks)
{
- fcb_log->fl_bset = (struct log_fcb_bset) {
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int reqd_bmark_cnt = MYNEWT_VAL(LOG_FCB_NUM_ABS_BOOKMARKS);
+
+ (void)reqd_bmark_cnt;
+ if (!bset || !buf || !avl_bmark_cnt) {
+ return SYS_EINVAL;
+ }
+
+ memset(bset, 0, sizeof(*bset));
+ memset(buf, 0, sizeof(struct log_fcb_bmark) * avl_bmark_cnt);
+
+ *bset = (struct log_fcb_bset) {
.lfs_bmarks = buf,
- .lfs_cap = bmark_count,
+ .lfs_cap = avl_bmark_cnt,
+ .lfs_en_sect_bmarks = en_sect_bmarks
};
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (en_sect_bmarks) {
+ /* Default sector bookmark interval is 1 */
+ bset->lfs_sect_bmark_itvl = 1;
+ reqd_bmark_cnt += fcb_log->fl_fcb.f_sector_cnt;
+ /* Make sure we have allocated the exact number of bookmarks */
+ if (avl_bmark_cnt < reqd_bmark_cnt) {
+ /* Not enough space allocated for sector bookmarks,
+ * add bookmarks at sector intervals */
+ bset->lfs_sect_bmark_itvl =
+ fcb_log->fl_fcb.f_sector_cnt / avl_bmark_cnt;
+ bset->lfs_sect_cap = avl_bmark_cnt -
+ MYNEWT_VAL(LOG_FCB_NUM_ABS_BOOKMARKS);
+ } else {
+ bset->lfs_sect_cap = fcb_log->fl_fcb.f_sector_cnt;
+ }
+ }
+#endif
+
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (en_sect_bmarks) {
Review Comment:
Those two line are exactly like previous section both preprocessor and
normal condition are same there is not need for two separate code sections
##########
sys/log/full/src/log_shell.c:
##########
@@ -251,6 +276,29 @@ shell_log_dump_cmd(int argc, char **argv)
continue;
}
+#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
+ if (dump_bmarks) {
+ bmarks = log_fcb_get_bmarks(log, &bmarks_size);
+ for (i = 0; i < bmarks_size; i++) {
+#if MYNEWT_VAL(LOG_FCB)
+ console_printf("%u: index:%lu entry: %x fa: %x fe_elem_off:
%lx\n", i,
+ bmarks[i].lfb_index,
+ (uintptr_t)&bmarks[i].lfb_entry,
Review Comment:
printing addresses of each element of the table is not really useful.
##########
sys/log/full/src/log_shell.c:
##########
@@ -251,6 +276,29 @@ shell_log_dump_cmd(int argc, char **argv)
continue;
}
+#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
+ if (dump_bmarks) {
+ bmarks = log_fcb_get_bmarks(log, &bmarks_size);
+ for (i = 0; i < bmarks_size; i++) {
+#if MYNEWT_VAL(LOG_FCB)
+ console_printf("%u: index:%lu entry: %x fa: %x fe_elem_off:
%lx\n", i,
+ bmarks[i].lfb_index,
+ (uintptr_t)&bmarks[i].lfb_entry,
+ (uintptr_t)bmarks[i].lfb_entry.fe_area,
Review Comment:
more useful would be to print offset in flash instead of address of array
element
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
