riotboot: Mode selection for boards by chrysn · Pull Request #16011 · RIOT-OS/RIOT

chrysn · 2021-02-15T12:25:19Z

Contribution description

For USB bootloaders, it is common to have a way of entering the bootloader even when the application is not cooperating. A typical way is to poll a button on startup. (For example, the bootloaders shipped with nrf52840dongle and the particle xenon boards to that; the latter in a more elaborate fashion that includes blink patterns).

It doesn't help with remote bricking, and it doesn't help against flash writes to the bootloader (both of which might be mitigated by additional means), but solves the common case of "I forgot to give my application the right USEMODULES to get back into DFU" or "crash at board_init".

Testing procedure

Flash the bootloader using make -C bootloaders/riotboot_dfu BOARD=particle-xenon PROGRAMMER=openocd DEBUG_ADAPTER=stlink all flash
Dismantle your programmer. (Well, at least assume that from now on you won't want to use it any more).
Notice how it comes up in the bootloader by running dfu-util -l and seeing "Found DFU" (unless, of course, there happens to be a valid image in there; an openocd flash erase_sector 0 0 last before the original programming ensures a cleans state)
Add FEATURES_REQUIRED += riotboot to examples/filesystem/Makefile
make -C examples/usbus_minimal BOARD=particle-xenon APP_VER=$(date +%s) PROGRAMMER=dfu-util all riotboot/flash-slot0`
Notice how we forgot to add USEMODULE += usbus_dfu, and the board doesn't even come up. (That's probably a different bug, at least an ACM TTY should come up). Add the USEMODULE to filesystem example.
To get the device usable without connecting a programmer, just keep the user button pressed while briefly pressing the reset button.
On dfu-util -l the device now shows "Found DFU", indicating it's in bootloader mode.
Run the riotboot/flash-slot0 target again.
There should now be a ttyACM0, and dfu-util would report having found a Runtime.

Open questions

The board's ability to pick an alternate mode is modeled as a FEATURE, which is optionally used by the bootloader. Is that the right choice?
I'm extracting whether or not the feature is used into a CFLAGS+=-D.... That looks uncommon, but creating a complete pseudomodule for it sounds extraneous. What's the best way?
The way the definition is then used in the code is crude; I hope to get some guidance out of board_init: Inconsistent declaration #16007 on whether there should be an extra header file or whether it's really just extern bool ... everywhere it's used.
Do we want to allow boards to do more fancy indicating? (Might be in one go -- a feature that, if set, provides this function and a "switch on whichever LED indicates bootloader mode here").
For the DFU bootloader, there's a pretty obvious thing to do with the information. Can other variants (riotboot without DFU) make any use of it? That bootloader isn't much use if it doesn't find an image, but it might be asked to load the other image if still good (yay downgrade attacks -- but is local downgrading allowed?).

bootloaders/riotboot_dfu/main.c

chrysn · 2021-02-15T13:01:33Z

While I'm not a big fan of adding any more preprocessor stuff where not absolutely necessary, in this case yes, I think that'd be nice. (To the point where the introduction of a function could be postponed until *any* board does a startup check that is *not* just setting and reading a GPIO pin).

fjmolinas · 2021-02-15T14:08:58Z

bootloaders/riotboot_dfu/Makefile

+ifneq (,$(filter bootloader_alternative_mode,$(FEATURES_USED)))
+  CFLAGS += -DALTERNATIVE_MODE
+endif


I would add a PSEUDOMODULE instead (answering your question)

fjmolinas · 2021-02-15T14:10:07Z

boards/common/particle-mesh/board.c

+bool board_bootloader_alternative_mode(void)
+{
+    /* If the application button is pressed, this indicates that the bootloader
+     * should switch to its alternative mode (eg. DFU recovery). */
+    gpio_init(BTN0_PIN, BTN0_MODE);
+    return !gpio_read(BTN0_PIN);
+}


The button should probably be configurable no? I imagine many will implement this, and it will pretty much be the same only maybe with different pin numbers.

If we're going with the Ben's suggestion, the board.h of providing boards would

#eefine BTN_BOOTLOADER_PIN BTN0_PIN

etc -- but per board it can IMO be fixed (especially on boards that have only one button)

boards/common/particle-mesh/Makefile.dep

chrysn · 2021-02-15T14:44:22Z

Based on benpicco's original comment and dylad's note on IRC I think this can be vastly simplified, to the point where any board's BTN0 makes the bootloader divert when pressed, and the boards can override that.

So no features, no pseudomodules, no dependencies, just a small preprocessor default and an optional GPIO step.

chrysn · 2021-02-15T14:53:17Z

Thanks for all your input, this is all much simpler now. (If and when boards need an actual function to be evaluated, that can still be added at that point, but maybe it never will.)

My remaining qualm is that the documentation, even though written, is not visible, for bootloaders are not part of what's built into the documentation. (Which also means that riotboot/README.md is not rendered). It's all prepared though, so whenever we do something there it should show up in the docs.

chrysn · 2021-02-15T15:04:10Z

Sorry for the many fixups; I think by all I've fumbled around here it should be much more readable if I squash.

bootloaders/riotboot_dfu/main.c

chrysn · 2021-02-23T09:09:31Z

Does this mean it's good for squashing final review?

dylad · 2021-02-25T19:35:20Z

Does this mean it's good for squashing final review?

This is fine by me.

dylad · 2021-02-27T21:12:36Z

Looks good to me. I succesfully tested this PR on SAML21-XPRO, press & hold BTN0, then reset and the board stays in bootloader mode as expected.
@fjmolinas any last word ?

dylad

last comments, you can squash directly.

bootloaders/riotboot_dfu/bootloader_selection.h

bootloaders/riotboot_dfu/main.c

Boards can override this; by default it's BTN0

chrysn · 2021-03-18T19:32:17Z

Last changes addressed, thanks for pointing them out. CI seems to be happy -- are you too?

dylad

ACK.
Looks good to me.
I tested this PR on SAML21-XPRO and it works as expected.

dylad · 2021-03-20T20:43:54Z

Here we go

chrysn added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Feb 15, 2021

benpicco requested a review from dylad February 15, 2021 12:44

benpicco reviewed Feb 15, 2021

View reviewed changes

bootloaders/riotboot_dfu/main.c Show resolved Hide resolved

fjmolinas reviewed Feb 15, 2021

View reviewed changes

boards/common/particle-mesh/Makefile.dep Outdated Show resolved Hide resolved

fjmolinas reviewed Feb 15, 2021

View reviewed changes

boards/common/particle-mesh/Makefile.dep Outdated Show resolved Hide resolved

chrysn force-pushed the riotboot-modeselect branch from fdeea7b to 5f18e29 Compare February 15, 2021 15:18

chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 15, 2021

This was referenced Feb 16, 2021

riotboot_dfu: Use allocated USB IDs #16017

Merged

doc: Make bootloaders visible in modules #16025

Merged

riotboot_dfu: Add documentation #16028

Merged

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 16, 2021

dylad reviewed Feb 18, 2021

View reviewed changes

bootloaders/riotboot_dfu/main.c Outdated Show resolved Hide resolved

chrysn force-pushed the riotboot-modeselect branch from 51d8ce5 to 2669f05 Compare February 26, 2021 09:55

chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 26, 2021

chrysn requested a review from dylad February 26, 2021 12:08

dylad reviewed Mar 15, 2021

View reviewed changes

bootloaders/riotboot_dfu/bootloader_selection.h Outdated Show resolved Hide resolved

bootloaders/riotboot_dfu/main.c Outdated Show resolved Hide resolved

riotboot_dfu: Stay in DFU mode if button is pressed

d770917

Boards can override this; by default it's BTN0

chrysn force-pushed the riotboot-modeselect branch from 2669f05 to d770917 Compare March 18, 2021 18:32

dylad approved these changes Mar 20, 2021

View reviewed changes

dylad merged commit 160251b into RIOT-OS:master Mar 20, 2021

chrysn deleted the riotboot-modeselect branch March 20, 2021 22:12

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

chrysn added the Release notes: added Set on PRs that have been processed into the release notes for the current release. label Apr 28, 2021

Conversation

chrysn commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Open questions

Uh oh!

Uh oh!

chrysn commented Feb 15, 2021 via email

Uh oh!

fjmolinas Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

chrysn Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chrysn commented Feb 15, 2021

Uh oh!

chrysn commented Feb 15, 2021

Uh oh!

chrysn commented Feb 15, 2021

Uh oh!

Uh oh!

chrysn commented Feb 23, 2021

Uh oh!

dylad commented Feb 25, 2021

Uh oh!

dylad commented Feb 27, 2021

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chrysn commented Mar 18, 2021

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

dylad commented Mar 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chrysn commented Feb 15, 2021 •

edited

Loading