kasjer commented on code in PR #3365:
URL: https://github.com/apache/mynewt-core/pull/3365#discussion_r1981159018


##########
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:
   Fresh output from log -b (only few last entries are shown here) 4 ABS 
bookmarks enabled
   ```
   ...
   001165 186: index:252 fa: 103000 fe_elem_off: 4d
   001166 187: index:155 fa: 102000 fe_elem_off: 6c
   001166 188: index:108 fa: 101000 fe_elem_off: 76
   001167 189: index:62 fa: 100000 fe_elem_off: 8
   001167 190: unused
   001167 191: unused
   001168 192: unused
   001168 193: unused
   ```
   now few walks, 3 of them for same index, 4th for different index
   ```
   compat> log -i 62 -n 2 -t test_log
   005561 Log test_log 2 entries walked in 0 ms
   compat> log -i 62 -n 2 -t test_log
   005774 Log test_log 2 entries walked in 7 ms
   compat> log -i 62 -n 2 -t test_log
   006266 Log test_log 2 entries walked in 0 ms
   compat> log -i 108 -n 2 -t test_log
   007396 Log test_log 2 entries walked in 46 ms
   ```
   now `log -b test_log` again
   ```
   007771 187: index:155 fa: 102000 fe_elem_off: 6c
   007772 188: index:108 fa: 101000 fe_elem_off: 76
   007772 189: index:62 fa: 100000 fe_elem_off: 8
   007773 190: index:62 fa: 100000 fe_elem_off: 8
   007773 191: unused
   007773 192: unused
   007774 193: index:108 fa: 101000 fe_elem_off: 76
   ```
   At this point `lfs_size` and `lfs_non_sect_size` were incremented even 
though bookmark was not added.
   `lfs_next_non_sect` was also modified leaving spaces in bookmark array



-- 
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]

Reply via email to