[riot-notifications] [RIOT-OS/RIOT] usbus/dfu: add Device Firmware Upgrade support for USBUS (2nd attempt) (#15460)

[benpicco]

@benpicco commented on this pull request.

looks pretty straightforward.
Would be nice to have the flash process integrated into the build system, so `make PROGRAMMER=dfu-util flash` flash works instead of having to manually invoke `dfu-util`.

I tried to flash a third firmware:

```
APP_VER=5 BOARD=same54-xpro USEMODULE=usbus_dfu make -C tests/usbus_cdc_acm_stdio riotboot/slot
 sudo dfu-util -a 0 -R -D tests/usbus_cdc_acm_stdio/bin/same54-xpro/tests_usbus_cdc_acm_stdio-slot0.bin
```

However, this didn't seem to work - the firmware in slot 1 is always booted.
Also, do we always have to flash alternating slots?

It's seems like I can't flash into slot 1 when the last firmware is running from slot1, but that shouldn't matter for riotboot.

It's also a bit strange that the user has to take care about the firmware version - shouldn't the firmware that was flashed by the user always be booted, not matter what version the previously installed firmware had?

> +#ifdef MODULE_USBUS_DFU
+    if (pkt->type & USB_SETUP_REQUEST_TYPE_CLASS) {
+        if (pkt->type & USB_SETUP_REQUEST_RECIPIENT_INTERFACE) {
+            dfu_control_req(usbus, handler);
+            res = 1;
+        }
+    }
+#endif

This needs a comment

> +    for (unsigned i = 0; i < riotboot_slot_numof; i++) {
+        const riotboot_hdr_t *riot_hdr = riotboot_slot_get_hdr(i);
+        if (riotboot_slot_validate(i)) {
+            /* skip slot if metadata broken */
+            continue;
+        }
+        if (riot_hdr->start_addr != riotboot_slot_get_image_startaddr(i)) {
+            continue;
+        }
+        if (slot == -1 || riot_hdr->version > version) {
+            version = riot_hdr->version;
+            slot = i;
+        }
+    }

Should we also be able to use 'raw' images (without a riotboot header) when not using the `riotboot_slot` module? 

> +ifneq (,$(filter usbus_dfu,$(USEMODULE)))
+  CFLAGS += -DCPU_RAM_BASE=$(RAM_START_ADDR)
+  CFLAGS += -DCPU_RAM_SIZE=$(RAM_LEN)
+endif

I wonder if we should set this unconditionally 

> +            /* Host indicates end of firmware download */
+            if (pkt->length == 0) {
+                /* Set DFU to manifest sync */
+                dfu_state = USBUS_DFU_STATE_DFU_MANIFEST_SYNC;
+                riotboot_flashwrite_flush(&_writer);
+                riotboot_flashwrite_finish(&_writer);
+            }
+            else if (dfu_state != USBUS_DFU_STATE_DFU_DL_SYNC) {
+                dfu_state = USBUS_DFU_STATE_DFU_DL_SYNC;
+            }
+            else {
+                /* Retrieve firmware data */
+                size_t len = 0;
+                usbdev_ep_get(handler->out, USBOPT_EP_AVAILABLE,
+                              &len, sizeof(size_t));
+                if (offset == 0) {

```suggestion
                /* skip writing the riotboot signature */
                if (offset == 0) {
```

> +                dfu_state = USBUS_DFU_STATE_DFU_MANIFEST_SYNC;
+                riotboot_flashwrite_flush(&_writer);
+                riotboot_flashwrite_finish(&_writer);
+            }
+            else if (dfu_state != USBUS_DFU_STATE_DFU_DL_SYNC) {
+                dfu_state = USBUS_DFU_STATE_DFU_DL_SYNC;
+            }
+            else {
+                /* Retrieve firmware data */
+                size_t len = 0;
+                usbdev_ep_get(handler->out, USBOPT_EP_AVAILABLE,
+                              &len, sizeof(size_t));
+                if (offset == 0) {
+                    offset = RIOTBOOT_FLASHWRITE_SKIPLEN;
+                    len -= RIOTBOOT_FLASHWRITE_SKIPLEN;
+                    riotboot_flashwrite_putbytes(&_writer, &handler->out->buf[4], len, true);

```suggestion
                    riotboot_flashwrite_putbytes(&_writer, &handler->out->buf[RIOTBOOT_FLASHWRITE_SKIPLEN], len, true);
```

-- 
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/15460#pullrequestreview-540408243
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201128/3a10aad6/attachment-0001.htm>