[riot-notifications] [RIOT-OS/RIOT] Feat/add flashwrite APIs (#15511)

Francisco notifications at github.com
Thu Nov 26 17:17:58 CET 2020


@fjmolinas requested changes on this pull request.

Hi @firas-hamdi thanks for your contribution and welcome to RIOT!

Some questions regarding this change, what is the use case you have for this? Otherwise I don't have anything against the change. Some comments and questions below.

Also have you taken a look at out [Commit conventions](https://github.com/miri64/RIOT/blob/master/CONTRIBUTING.md#commit-conventions)? Would you min taking a look to reformat the commits? I think here you could have 2 commits:

- the one adding the new api functions
- the one fixing the unused `image_size`

> +/**
+ * @brief       Get a slot's size
+ *
+ * @param[in]   state   ptr to state struct
+ *
+ * @returns     the size of the slot that @p state is configured to update to
+ */

```suggestion
```

copy pasta

> +    int _slot_to_revert = -1;
+    _slot_to_revert = (riotboot_slot_get_hdr(riotboot_slot_other())->version
+            > riotboot_slot_get_hdr(riotboot_slot_current())->version) ? riotboot_slot_other() : riotboot_slot_current();

Is checking the version enough? Should it check if the version is valid?

> @@ -128,6 +128,11 @@ typedef struct {
  */
 #define RIOTBOOT_FLASHWRITE_SKIPLEN     sizeof(RIOTBOOT_MAGIC)
 
+/**
+ * @brief Magic number used to invalidate a slot
+ */
+#define INVALIDATE_HDR                  0

This would depend on the flash, in some cases `0x00` is the erase state in others `0xFF`, and therefore this could be a `NOP` depending on the operation.

> +    if (riotboot_slot_numof == 1){
+        LOG_WARNING(LOG_PREFIX "Only one slot configured\n");
+        return -1;
+    }

This behavior does not match the function documentation, @bergzand do you think it should allow invalidating in this case?

> +    if ((riotboot_slot_validate(riotboot_slot_other()) != 0) || (riotboot_slot_validate(riotboot_slot_current()) != 0)){
+        LOG_INFO(LOG_PREFIX "There will be no valid images to run after reboot\n");
+        return -2;
+    }

Same here, if it returns then it will not actually invalidate.

> +int riotboot_flashwrite_invalidate(int slot)
+{
+    extern const unsigned riotboot_slot_numof;
+
+    if (riotboot_slot_numof == 1){
+        LOG_WARNING(LOG_PREFIX "Only one slot configured\n");
+        return -1;
+    }
+    if ((riotboot_slot_validate(riotboot_slot_other()) != 0) || (riotboot_slot_validate(riotboot_slot_current()) != 0)){
+        LOG_INFO(LOG_PREFIX "There will be no valid images to run after reboot\n");
+        return -2;
+    }
+
+    uint8_t data_flash[4] = { INVALIDATE_HDR };
+
+    flashpage_write_raw((void *) riotboot_slot_get_hdr(slot), (const void *) data_flash, 4);

```suggestion
    flashpage_write_raw((void *) riotboot_slot_get_hdr(slot), (const void *) data_flash, sizeof(RIOTBOOT_MAGIC));
```

> @@ -97,6 +97,7 @@ static inline void _print_download_progress(suit_manifest_t *manifest,
         }
     }
 #endif
+    (void) image_size;

This change should be in its own commit IMO

> @@ -162,6 +191,7 @@ int riotboot_flashwrite_finish_raw(riotboot_flashwrite_t *state,
 #if CONFIG_RIOTBOOT_FLASHWRITE_RAW
     memcpy(state->firstblock_buf, bytes, len);
     flashpage_write(slot_start, state->firstblock_buf, RIOTBOOT_FLASHPAGE_BUFFER_SIZE);
+    res = 0;

This one should not be needed, I think it is already fixed upstream, should go away with a rebase.

> @@ -128,6 +128,11 @@ typedef struct {
  */
 #define RIOTBOOT_FLASHWRITE_SKIPLEN     sizeof(RIOTBOOT_MAGIC)
 
+/**
+ * @brief Magic number used to invalidate a slot
+ */
+#define INVALIDATE_HDR                  0

Maybe we should add a `FLASH_ERASE_STATE` value, and use that as the write value. What do you think @bergzand should we add something like that?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15511#pullrequestreview-539425827
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201126/f2e0426f/attachment-0001.htm>


More information about the notifications mailing list