Add TODOs for refactoring according to PR review and refactor ens_fs_write to return error, if entry is not only ones

platformio.ini

@@ -23,8 +23,6 @@ build_flags =
     -DEN_INCLUDE_ZEPHYR_DEPS=1
     -DEN_INIT_MBEDTLS_ENTROPY=0
     -DCOVID_MEASURE_PERFORMANCE=0   # Used to measure device performance
-    -DCONFIG_NORDIC_QSPI_NOR=y # configuration options for MX25R64 flash device
-    -DCONFIG_NORDIC_QSPI_NOR_FLASH_LAYOUT_PAGE_SIZE=4096
 lib_deps =
     prathje/exposure-notification @ ^0.1
 lib_ignore =

src/ens/ens_error.h

@@ -1,9 +1,10 @@
 #ifndef ENS_ERROR_H
 #define ENS_ERROR_H
 
-#define ENS_INTERR 1  // internal error
-#define ENS_NOENT 2   // entry not found or invalid
-#define ENS_DELENT 3  // entry got deleted
-#define ENS_INVARG 4  // invalid argument
+#define ENS_INTERR 1   // internal error
+#define ENS_NOENT 2    // entry not found or invalid
+#define ENS_DELENT 3   // entry got deleted
+#define ENS_ADDRINU 4  // address alread in use or corrupt
+#define ENS_INVARG 5   // invalid argument
 
 #endif

src/ens/ens_fs.c

@@ -1,4 +1,5 @@
 #include <drivers/flash.h>
+#include <errno.h>
 #include <kernel.h>
 #include <math.h>
 #include <storage/flash_map.h>
@@ -48,6 +49,7 @@ int ens_fs_init(ens_fs_t* fs, uint8_t flash_id, uint64_t entry_size) {
         return -ENS_INTERR;
     }
     memset(ptr, 0, internal_size);
+    fs->buffer = ptr;
 
     // init the lock for the fs
     k_mutex_init(&fs->ens_fs_lock);
@@ -101,21 +103,36 @@ end:
 
 int ens_fs_write(ens_fs_t* fs, uint64_t id, void* data) {
     int rc = 0;
+    uint64_t offset = id * fs->interal_size;
+    uint8_t* obj = fs->buffer;
+
     k_mutex_lock(&fs->ens_fs_lock, K_FOREVER);
+    // read current data in flash...
+    if (flash_area_read(fs->area, offset, obj, fs->interal_size)) {
+        rc = -ENS_INTERR;
+        goto end;
+    }
+    // ...and check, if it's all 1's
+    for (int i = 0; i < fs->entry_size; i++) {
+        if (!(obj[i] & 0xff)) {
+            // if this entry is not all 1's, we return error and exit the function
+            rc = -ENS_ADDRINU;
+            goto end;
+        }
+    }
 
     // copy data into interal buffer
     memcpy(fs->buffer, data, fs->entry_size);
 
     // set CRC and not-deleted-flag
-    uint8_t* obj = fs->buffer;
     obj[fs->entry_size] = crc7_be(SEED, obj, fs->entry_size) | 1;
 
-    uint64_t offset = id * fs->interal_size;
-    if (flash_area_write(fs->area, offset, data, fs->interal_size)) {
+    if (flash_area_write(fs->area, offset, obj, fs->interal_size)) {
         // writing to flash was not successful
         rc = -ENS_INTERR;
     }
 
+end:
     k_mutex_unlock(&fs->ens_fs_lock);
     return rc;
 }
@@ -136,16 +153,23 @@ int ens_fs_delete(ens_fs_t* fs, uint64_t id) {
     return rc;
 }
 
-int ens_fs_page_erase(ens_fs_t* fs, uint64_t entry_id, uint64_t sector_count) {
+uint64_t ens_fs_make_space(ens_fs_t* fs, uint64_t entry_id) {
+    // calculate start and check, if it is at the start of a page
+    uint64_t start = entry_id * fs->interal_size;
+    printk("Erasing from byte %llu\n", start);
+    if ((start % fs->sector_size) != 0) {
+        return -ENS_INVARG;
+    }
+
     int rc = 0;
     k_mutex_lock(&fs->ens_fs_lock, K_FOREVER);
 
-    // calculate the next page start before (or at) the given entry_id
-    uint64_t start = (entry_id - entry_id % fs->sector_size) * fs->interal_size;
-
     // erase given amount of pages, starting for the given offset
-    if (flash_area_erase(fs->area, start, fs->sector_size * sector_count)) {
+    if (flash_area_erase(fs->area, start, fs->sector_size)) {
         rc = -ENS_INTERR;
+    } else {
+        // if we are successful, return amount of deleted entries
+        rc = fs->sector_size / fs->interal_size;
     }
 
     k_mutex_unlock(&fs->ens_fs_lock);

src/ens/ens_fs.h

@@ -90,14 +90,13 @@ int ens_fs_write(ens_fs_t* fs, uint64_t id, void* data);
 int ens_fs_delete(ens_fs_t* fs, uint64_t id);
 
 /**
- * Erase a given amount of flash sectors. Starting at offset (if offset is not at page start, it will be rounded down).
+ * Reqeust some free space in flash. Returns -ENS_INVARG, if the given id is not at the start of a page.
  *
  * @param fs file system
- * @param id id of the entry to erase the page for
- * @param sector_count count of sectors to erase
+ * @param id id of the entry to make space for
  *
- * @return 0 on success, -errno otherwise
+ * @return the positive amount of deleted entries, -errno otherwise
  */
-int ens_fs_page_erase(ens_fs_t* fs, uint64_t id, uint64_t sector_count);
+uint64_t ens_fs_make_space(ens_fs_t* fs, uint64_t id);
 
 #endif

src/ens/records.c

@@ -26,10 +26,15 @@ int ens_records_iterator_init_range(record_iterator_t* iterator,
  * @param start lower bound for the binary search
  * @param end upper bound for the binary search
  */
+// TODO lome: maybe add flag for indicating whether older or newer contact shall be loaded
 int find_record_via_binary_search(record_t* record,
                                   uint32_t target,
                                   record_sequence_number_t start,
                                   record_sequence_number_t end) {
+    // TODO lome: 1. handle entries with deleted/invalid data -> load left/right entry
+    // TODO lome: 2. handle possible deadlocks because of 1.
+    // TODO lome: 3. check, if oldest ts is newer than the latest -> return error that needs to be handled by the
+    //              calling function
     record_t start_record;
     record_t end_record;
 
@@ -44,6 +49,7 @@ int find_record_via_binary_search(record_t* record,
     }
 
     do {
+        // TODO lome: first entry for start, last entry for end
         // calculate the contact in the middle between start and end and load it
         record_sequence_number_t middle = (start_record.sn + end_record.sn) / 2;
         int rc = load_record(record, middle);
@@ -77,7 +83,11 @@ int ens_records_iterator_init_timerange(record_iterator_t* iterator, uint32_t* t
         return rc;
     }
     // if starting timestamp lies in our bounds, perform binary search
+    // TODO lome: check, if ts_start and ts_end are NULL -> use oldest/latest sn then
+    // TODO lome: move oldest_sn and latest_sn to binary_search function
+    // TODO lome: maybe keep track of the oldest and newest timestamp (optimization for later)
     if (start_rec.timestamp < *ts_start) {
+        // TODO lome: only "return" sn, not actual record
         rc = find_record_via_binary_search(&start_rec, *ts_start, oldest_sn, latest_sn);
 
         if (rc) {

src/ens/storage.c

@@ -7,6 +7,7 @@
 #include <string.h>
 #include <zephyr.h>
 
+#include "ens_error.h"
 #include "ens_fs.h"
 #include "sequencenumber.h"
 #include "storage.h"
@@ -37,7 +38,7 @@ int load_storage_information() {
     // Check, if read what we wanted
     if (rc != size) {
         // Write our initial data to storage
-        rc = nvs_write(&info_fs, STORED_CONTACTS_INFO_ID, &record_information, size);
+        int rc = nvs_write(&info_fs, STORED_CONTACTS_INFO_ID, &record_information, size);
         if (rc <= 0) {
             return rc;
         }
@@ -59,19 +60,6 @@ int save_storage_information() {
     return rc;
 }
 
-record_sequence_number_t get_next_sequence_number() {
-    k_mutex_lock(&info_fs_lock, K_FOREVER);
-    if (record_information.count >= MAX_CONTACTS) {
-        record_information.oldest_contact = sn_increment(record_information.oldest_contact);
-    } else {
-        record_information.count++;
-    }
-    save_storage_information();
-    record_sequence_number_t next_sn = get_latest_sequence_number();
-    k_mutex_unlock(&info_fs_lock);
-    return next_sn;
-}
-
 int init_record_storage(void) {
     int rc = 0;
     struct flash_pages_info info;
@@ -123,33 +111,63 @@ int load_record(record_t* dest, record_sequence_number_t sn) {
 }
 
 int add_record(record_t* src) {
-    int sectorsToDelete = 1;
+    /**
+     * Some information about the procedure in this function:
+     *      1. we calculate the potential next sn and storage id
+     *      2. we try to write our entry
+     *          2.1 if write was successful, goto 5., otherwise continue at 3.
+     *      3. if our id is already in use, we request the fs to make some space
+     *      4. after making space, we adjust our storage information and try to write again
+     *      5. we actually "increment" our stored contact information
+     *
+     * This order (first erase storage, then increment information) is important, because like this we keep a constant
+     * state of our information about the stored contacts in combination with correct state of our flash.
+     */
 
     k_mutex_lock(&info_fs_lock, K_FOREVER);
 
     // Check, if next sn would be at start of page
-    record_sequence_number_t potential_next_sn = sn_increment(get_latest_sequence_number());
+    record_sequence_number_t potential_next_sn =
+        get_latest_sequence_number() == get_oldest_sequence_number() ? 0 : sn_increment(get_latest_sequence_number());
     storage_id_t potential_next_id = convert_sn_to_storage_id(potential_next_sn);
-    if (((potential_next_id * ens_fs.entry_size) % ens_fs.sector_size) == 0) {
-        // If we are at start of a page, we need to erase it first
-        ens_fs_page_erase(&ens_fs, potential_next_id, sectorsToDelete);
 
-        // if our storage is full, we need to adjust our information about stored records
-        if (get_num_records() == MAX_CONTACTS) {
-            int deletedRecordsCount = (ens_fs.sector_size / ens_fs.entry_size) * sectorsToDelete;
+    // write our entry to flash and check, if the current entry is already in use
+    int rc = ens_fs_write(&ens_fs, potential_next_id, src);
+    // if our error does NOT indicate, that this address is already in use, we just goto end and do nothing
+    if (rc && rc != -ENS_ADDRINU) {
+        // TODO: maybe also increment, if there is an internal error?
+        goto end;
+    } else if (rc == -ENS_ADDRINU) {
+        // the current id is already in use, so make some space for our new entry
+        int deletedRecordsCount = ens_fs_make_space(&ens_fs, potential_next_id);
+        if (deletedRecordsCount < 0) {
+            // some interal error happened (e.g. we are not at a page start)
+            rc = deletedRecordsCount;
+            // we still need to increment our information, so we are not at the exact same id the entire time
+            goto inc;
+        } else if (deletedRecordsCount > 0 && get_num_records() == MAX_CONTACTS) {
             record_information.count -= deletedRecordsCount;
             record_information.oldest_contact = sn_increment_by(record_information.oldest_contact, deletedRecordsCount);
-            save_storage_information();
+        }
+        // after creating some space, try to write again
+        rc = ens_fs_write(&ens_fs, potential_next_id, src);
+        if (rc) {
+            goto inc;
         }
     }
 
-    // Actually increment sn
-    record_sequence_number_t curr_sn = get_next_sequence_number();
-    src->sn = curr_sn;
-    storage_id_t id = convert_sn_to_storage_id(curr_sn);
+inc:
+    // check, how we need to update our storage information
+    if (record_information.count >= MAX_CONTACTS) {
+        record_information.oldest_contact = sn_increment(record_information.oldest_contact);
+    } else {
+        record_information.count++;
+    }
+    save_storage_information();
 
+end:
     k_mutex_unlock(&info_fs_lock);
-    return ens_fs_write(&ens_fs, id, src);
+    return rc;
 }
 
 int delete_record(record_sequence_number_t sn) {
@@ -167,7 +185,6 @@ int delete_record(record_sequence_number_t sn) {
     return rc;
 }
 
-// TODO lome: do we need lock here aswell?
 record_sequence_number_t get_latest_sequence_number() {
     return sn_increment_by(record_information.oldest_contact, record_information.count);
 }

src/ens/storage.h

@@ -14,7 +14,7 @@ typedef struct record {
     rssi_t rssi;                  // TODO: Check correct
     rolling_proximity_identifier_t rolling_proximity_identifier;
     associated_encrypted_metadata_t associated_encrypted_metadata;
-} record_t;
+} __packed record_t;
 
 typedef struct stored_records_information {
     record_sequence_number_t oldest_contact;

zephyr/prj.conf

@@ -33,4 +33,6 @@ CONFIG_DEBUG=y
 CONFIG_LOG=y
 
 CONFIG_NEWLIB_LIBC=y
-CONFIG_HEAP_MEM_POOL_SIZE=1024
+CONFIG_HEAP_MEM_POOL_SIZE=4096
+CONFIG_NORDIC_QSPI_NOR=y # configuration options for MX25R64 flash device
+CONFIG_NORDIC_QSPI_NOR_FLASH_LAYOUT_PAGE_SIZE=4096