Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make: verify before flash #11093

Closed
wants to merge 2 commits into from

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Mar 4, 2019

Contribution description

Some flashers (jlink) check if the flash contents already matches the to-be-written binary and if yes, skip re-programming. That saves time and flash cycles.

This PR adds make infrastructure to support that with other flashers, too.
Basically it works by, instead of doing $flash $flash_args, it does $flash_verify || $flash $flash_args.
If no FLASH_VERIFY is provided, false is used thus flashing is done as before.

Another commit implements this for edbg.

Testing procedure

With an edbg supported board, try make all; make flash-only; make flash-only and observe that the first flashing actually programs the device, the second skips programming.

Also test non-edbg boards (which don't yet supply "FLASH_VERIFY").

Issues/PRs references

none

@kaspar030 kaspar030 added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Mar 4, 2019
@kaspar030 kaspar030 requested a review from cladmi March 4, 2019 10:42
@cladmi
Copy link
Contributor

cladmi commented Mar 4, 2019

I like the idea, but I would define it empty by default and add the handling if defined.
Seeing false || ./openocd.sh firmware feels out of place.

Also, both variables need to be documented.

@jcarrano
Copy link
Contributor

jcarrano commented Mar 4, 2019

One observation: this works only if the flasher only flashes and nothing more. If the flasher also resets, then the reset has to be added as an additional step that is always performed.

@cladmi
Copy link
Contributor

cladmi commented Mar 6, 2019

I tested, and with edbg it does not break incremental flashing like done in makefiles/boot/riotboot.mk. The file currently does not use flash-recipe but I updated it to test.

@jcarrano On edbg it resets the board on the check.

Both these case should be documented as requirements.

Another solution is to allow overwriting flash-recipe and currently overwrite it for edbg only.
I think would just need to replace the current recipe name by default-flash-recipe and do flash-recipe ?= default-flash-recipe. And then in edbg.inc.mk do flash-recipe = edbg-flash-recipe that defines the new behavior.

@maribu
Copy link
Member

maribu commented Mar 6, 2019

I fixed a typo in the PR description. (false || flash should is used when no verification command is present, not true || flash)

@jcarrano
Copy link
Contributor

jcarrano commented Mar 7, 2019

@jcarrano On edbg it resets the board on the check.

I don't think that's a problem. I'd even say it's better: after FLASH the board is reset, so having it reset after verify is consistent.

@stale
Copy link

stale bot commented Sep 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Sep 8, 2019
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. I'd love to see this merged. In a previous post @cladmi already confirmed that it works, so testing is already done.

I agree that more documentation would be nice. But honestly, this would require some dedicated section in Doxygen for the build system to add this to. This is however currently missing. (I think that such a section would be helpful to keep the use of the build system more consisted, as people will have some place to look at when trying to figure out how to add something to the build system. But that is out of scope of this PR anyway.)

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Sep 8, 2019
@maribu maribu 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 Sep 8, 2019
@aabadie
Copy link
Contributor

aabadie commented Sep 9, 2019

more documentation would be nice

@maribu, I think @cladmi suggestion was to add some documentation of the new variable in vars.inc.mk.

@kaspar030
Copy link
Contributor Author

Closing in favor of #12301. Indeed @cladmi's suggestion is nicer (more localized).

@kaspar030 kaspar030 closed this Sep 25, 2019
@kaspar030 kaspar030 deleted the verify_before_flash branch September 25, 2019 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants