[riot-notifications] [RIOT-OS/RIOT] riotboot: implement serial flasher (#15493)

Francisco notifications at github.com
Tue Jan 26 17:03:51 CET 2021


@fjmolinas commented on this pull request.

Some first comments based only on testing experience.

> +    while (_get_char(fd) != RIOTBOOT_STAT_WAITING) {
+        printf("\rconnecting [%c]", spinner[tries % (sizeof(spinner) - 1)]);
+        _put_char(fd, RIOTBOOT_PROBE);
+        ++tries;
+    }

I think this is missing a timeout somewhere. 

> @@ -997,6 +997,18 @@ ifneq (,$(filter riotboot_slot, $(USEMODULE)))
   USEMODULE += riotboot_hdr
 endif
 
+ifneq (,$(filter riotboot_serial, $(USEMODULE)))
+  FEATURES_REQUIRED += periph_flashpage
+  FEATURES_REQUIRED += periph_uart
+  USEMODULE += riotboot_reset
+  USEMODULE += checksum
+endif
+
+ifneq (,$(filter riotboot_reset, $(USEMODULE)))
+  USEMODULE += riotboot
+  USEMODULE += usb_board_reset

Doesn't this miss a dependency to stdio_cdc_acm? Doesn't that change the firmware size by quite a bit? I would probably have different applications in that case since the one with USB will be very big.

> +# Include riotboot flash partition functionality
+# USEMODULE += riotboot_slot

Do we need partitioning here?

> +# limit riotboot bootloader size
+# TODO: Manage to set this variable for boards which already embed a
+# bootloader, currently it will be overwritten

This should also be helped by adding a new variable, you could flash the USB bootloader at an offset. It would mean stacking bootloaders though :)

-- 
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/15493#pullrequestreview-576487399
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210126/969f7e5c/attachment-0001.htm>


More information about the notifications mailing list