Skip to content

riotboot: add riotboot_flashwrite module#11181

Merged
kaspar030 merged 4 commits intoRIOT-OS:masterfrom
kaspar030:add_riotboot_flashwrite
Mar 18, 2019
Merged

riotboot: add riotboot_flashwrite module#11181
kaspar030 merged 4 commits intoRIOT-OS:masterfrom
kaspar030:add_riotboot_flashwrite

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Mar 14, 2019 •
edited
Loading

Contribution description

This PR adds a module to riotboot that allows writing a firmware image to an inactive firmware slot, along with a simple test application.

On its own, it doesn't do any validity checks of the to-be-flashed image. The module is supposed to be a building block used by higher level modules, e.g., for updating via validated suit manifests.

Testing procedure

Testing is a bit manual at the moment... Please see the README.md that comes with the test application.

Issues/PRs references

Depends on #11180 for the testing instructions to work. (merged)

@kaspar030 kaspar030 added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: OTA Area: Over-the-air updates labels Mar 14, 2019
@kaspar030 kaspar030 requested a review from bergzand March 14, 2019 11:16
@kaspar030 kaspar030 force-pushed the add_riotboot_flashwrite branch 2 times, most recently from dc64523 to 35c72cb Compare March 14, 2019 16:05
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Mar 14, 2019

bergzand
bergzand requested changes Mar 14, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review, more to follow

sys/include/riotboot/flashwrite.h Outdated
/**
* @brief Amount of bytes to skip at initial write of first page
*/
#define RIOTBOOT_FLASHWRITE_SKIPLEN 4
Copy link
Copy Markdown
Member

@bergzand bergzand Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use sizeof(RIOTBOOT_MAGIC) here, that would provide a direct reference to why it is this exact value.

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

bergzand
bergzand requested changes Mar 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick, feel free to squash immediately

tests/riotboot_flashwrite/README.md Outdated

$ BOARD=<board> make riotboot/flash

Configm it booted from slot 0, then recompile in order to get an image for the
Copy link
Copy Markdown
Member

@bergzand bergzand Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence doesn't parse for me :-/

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleared that up!

Copy link
Copy Markdown
Member

@bergzand bergzand Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bergzand bergzand added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Mar 15, 2019
@kaspar030 kaspar030 force-pushed the add_riotboot_flashwrite branch from 9dcdf78 to c170d64 Compare March 15, 2019 11:08
@bergzand bergzand added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Mar 15, 2019
bergzand
bergzand approved these changes Mar 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, test worked for me on a samr21-xpro. Code looks good to me

bergzand
bergzand reviewed Mar 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Murdock is complaining

sys/include/riotboot/flashwrite.h Outdated
*/

/**
* @ingroup sys_firmware
Copy link
Copy Markdown
Member

@bergzand bergzand Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ingroup sys_riotboot?

sys/riotboot/flashwrite.c Outdated
*/

/**
* @ingroup sys_riotboot
Copy link
Copy Markdown
Member

@bergzand bergzand Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ingroup sys_riotboot?

@kaspar030 kaspar030 force-pushed the add_riotboot_flashwrite branch from c170d64 to 641a32b Compare March 15, 2019 11:32
bergzand
bergzand reviewed Mar 15, 2019
View reviewed changes
sys/include/riotboot/flashwrite.h
*/

/**
* @defgroup sys_riotboot_flashwrite riotboot flash writer
Copy link
Copy Markdown
Member

@bergzand bergzand Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Riotboot?

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO all the riotboot module documentation edit organization edit needs work. I suggest we postpone that to a later PR.

Copy link
Copy Markdown
Member

@bergzand bergzand Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Mar 18, 2019

&go.

@kaspar030 kaspar030 merged commit fb2c7ea into RIOT-OS:master Mar 18, 2019
@kaspar030 kaspar030 deleted the add_riotboot_flashwrite branch March 18, 2019 10:30
@danpetry danpetry mentioned this pull request Mar 18, 2019
Closed
2 tasks
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Mar 18, 2019

This pull request broke this that was working:

FEATURES_REQUIRED+=riotboot BOARD=samr21-xpro make -C examples/hello-world/ riotboot/flash

@kaspar030 kaspar030 mentioned this pull request Mar 18, 2019
Closed
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Mar 19, 2019 •
edited
Loading

I disagree, the fix from #11203 do not fix the issues.

This pull request changed the API without mentioning in the title or giving an adequate testing description.

The third commit a15f07b is added without justification and changes the definition scope of the makefiles variables and made some non namespaced macros globally defined.
This may be correct, but not without a specific discussion and a justification other than "move slot variables". It should have be in its own pull request. Also, if the variables are now parsed by Makefile.include why keep the export ?

If it was required for ae35860 why is it defined after the commit introducing needing it ?

And the fix proposed in #11203 now makes that having FEATURES_REQUIRED+=riotboot define global non namespaced macros for no reasons.

This should really have been split to a separate pull request with dedicated testing procedure and justification as this pull request did not gave any and did not mention it either.

@cladmi cladmi mentioned this pull request Mar 19, 2019
Merged
@gschorcht gschorcht mentioned this pull request Mar 23, 2019
Open
@danpetry danpetry added this to the Release 2019.04 milestone Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergzand bergzand bergzand approved these changes

Assignees

No one assigned

Labels

Area: OTA Area: Over-the-air updates CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2019.04

Development

Successfully merging this pull request may close these issues.

4 participants

@kaspar030 @cladmi @bergzand @danpetry