make: add flash-only target by smlng · Pull Request #8373 · RIOT-OS/RIOT

smlng · 2018-01-16T16:39:51Z

Contribution description

PR adds new make target named flash-only which does not depend on target all. In some cases make rebuilds the binary before flashing even though nothing changed or the last build is only seconds old. The flash target still behaves the same and relies on all.

Issues/PRs references

None

jia200x · 2018-01-16T16:54:56Z

looks good... ACK

cladmi · 2018-01-16T16:55:04Z

I like this, but would need some tricks to make it work correctly.

It will not work now with boards where flash depends on depending on preflash.
Its the case for boards based on common/arduino-due as its setting BOSSA_ARDUINO_PREFLASH which then makes bossa.inc.mk add FLASHDEPS += preflash.

smlng · 2018-01-16T16:59:10Z

@cladmi I'll look into it

cladmi · 2018-01-16T17:07:34Z

Could be done by having a FLASH_PREREQUISITE := all variable that you set to an empty value when you want to flash without calling all. But it is not a target.

smlng · 2018-01-16T17:09:07Z

I do not have an Arduino-due available, but I think this should still work now.

jia200x · 2018-01-16T17:18:39Z

good catch, didn't check that. Un-ACK

jia200x · 2018-01-16T17:20:29Z

@smlng I have one, let me check

jia200x · 2018-01-16T17:31:18Z

I confirm it's working for Arduino Due.

cladmi · 2018-01-17T08:51:01Z

Not sure that it always works with parallel execution with "make flash -j 1000" on a clean environment as 'preflash' could be executed before building 'all'.

smlng · 2018-01-17T08:54:37Z

But that should be a problem even with the current way the targets work, right, and hence is not a problem of this PR but rather a general one, even with current master.

cladmi · 2018-01-17T09:05:24Z

No, right now, preflash is done after 'all'.

cladmi · 2018-01-17T09:07:08Z

For the other things in $(FLASHDEPS), they are flasher tools (edbg, bossac) so can be done anytime, not just before flashing.

smlng · 2018-01-17T09:12:17Z

mhm, I'm unsure what you mean. Currently the dependencies calling make flash are:

flash: all $(FLASHDEPS)

preflash: all

Where FLASHDEPS might contain preflash, resolving this would lead to flash: all all preflash?

With this PR this only slightly changed, and make flash should still resolve to flash: all preflash flash-only, or not?

kaspar030 · 2018-01-17T09:43:17Z

With this PR this only slightly changed, and make flash should still resolve to flash: all preflash flash-only, or not?

The difference is that before, the flash recipe did the flashing, after its prerequisites were run (preflash and all). Now, flash has "all" and "flash-only" as prerequisites, they might be run at the same time, thus this breaks.

Try make flash -j.

kaspar030 · 2018-01-17T09:55:04Z

Nah, another layer of recursion?

try this (revert your fixup2 first):

https://gist.github.com/16d57c2d0c1c071a7182714dcc9f726d

smlng · 2018-01-17T10:29:09Z

thanks @kaspar030, seems to work.

kaspar030 · 2018-01-17T10:31:11Z

Makefile.include

    all $(BASELIBS) $(USEPKG:%=$(RIOTPKG)/%/Makefile.include) $(RIOTBUILD_CONFIG_HEADER_C) pkg-prepare: clean
 endif

+ifneq (,$(filter flash, $(MAKECMDGOALS)))


Wouldn't it make more sense to move this down to the flash targets?
I also don't understand how this works using a tab at the beginning. That should make it part of above's recipe, no?

ah my bad, yes better to have it close to the flash targets

kaspar030 · 2018-01-17T10:37:03Z

ACK from my side. @cladmi can you take another look?

smlng · 2018-01-17T14:58:01Z

ACK from my side

then you should hit Approve, too 😉 @cladmi, are we good here?

cladmi · 2018-01-17T15:06:24Z

Nope does not work as I expect it to.

For me the preflash should done as last step just before flashing. For a leonardo board, there must less than 8 seconds between the preflash thing:

https://store.arduino.cc/arduino-leonardo-with-headers -> Documentation -> Automatic (Software) Reset and Bootloader Initiation

I don't have the board here to test so I just add 'echo' for flash-only and preflash and comment the actual command.
When building from a clean environment with -j 2, preflash is executed before all:

$ make BOARD=arduino-due clean; make  BOARD=arduino-due  flash -j2
echo preflash
preflash
#stty -F /dev/ttyACM0 raw ispeed 1200 ospeed 1200 cs8 -cstopb ignpar eol 255 eof 255
Building application "default" for "arduino-due" with MCU "sam3".

"make" -C /home/harter/work/git/RIOT/boards/arduino-due
...
"make" -C /home/harter/work/git/RIOT/sys/uart_stdio
   text	   data	    bss	    dec	    hex	filename
  13608	    564	   2704	  16876	   41ec	/home/harter/work/git/RIOT/examples/default/bin/arduino-due/default.elf
echo flash-only
flash-only
#/home/harter/work/git/RIOT/dist/tools/bossa/bossac -p /dev/ttyACM0 -e -i -w -v -b -R /home/harter/work/git/RIOT/examples/default/bin/arduino-due/default.bin

So they must both flash and preflash depend on something to ensure they are both executed after something. In the new version preflash depends on nothing.

The problem with your solution is that it modifies FLASHDEPS which does not affect preflash. You could put the all in its own variable that both flash and preflash depends on.

smlng · 2018-01-17T15:10:46Z

well, then we need to go for my previous code, which was like

flash-only: $(FLASHDEPS)
     [...]

flash:
   $(MAKE) all
   $(MAKE) flash-only

cladmi · 2018-01-17T15:23:41Z

In the same idea as what you last proposed but inversed.
I want to make sure that if flash-only is not mentioned, its the same behavior as before.
Like if flash is set as a dependency of something, it keeps the original behaviour.

ifneq (,$(filter flash-only, $(MAKECMDGOALS)))
  BUILD_ALL_BEFORE_FLASH :=
else
  BUILD_ALL_BEFORE_FLASH := all
endif

flash-only: flash

flash: $(BUILD_ALL_BEFORE_FLASH) $(FLASHDEPS)
preflash: $(BUILD_ALL_BEFORE_FLASH)

And this has expected behavior even with -j 2000

flash -> all -> preflash -> flash
flash-only -> preflash -> flash

I used horrible variable naming convention to think a second time about it.

miri64 · 2018-01-17T15:23:45Z

flash-only: $(FLASHDEPS)
     [...]

flash:
   $(MAKE) all
   $(MAKE) flash-only

/o\ why not use make instead of sneaking in shell scripts in?

flash-only: $(FLASHDEPS)
     [...]

flash: all flash-only

miri64 · 2018-01-17T15:24:38Z

Ah.... maybe I should have read @cladmi's previous comment .....

smlng · 2018-01-17T17:13:22Z

@cladmi thanks for testing, I adapted your proposed solution a bit, but this should work too. Can you check?

cladmi · 2018-01-18T12:37:16Z

Nop does not work. flash does not depend on all anymore, so flashing for any board that does not have preflash is broken.

$ make BOARD=pba-d-01-kw2x clean; make BOARD=pba-d-01-kw2x flash
arm-none-eabi-as -mthumb -o wdog-disable.o wdog-disable.s
arm-none-eabi-objcopy -O binary -j .text.wdog_disable -S -g wdog-disable.o wdog-disable.bin
/home/harter/work/git/RIOT/dist/tools/openocd/openocd.sh flash
### Flashing Target ###
Error: Unable to locate IMAGE_FILE
       (/home/harter/work/git/RIOT/examples/default/bin/pba-d-01-kw2x/default.elf)
/home/harter/work/git/RIOT/examples/default/../../Makefile.include:394: recipe for target 'flash' failed
make: *** [flash] Error 1

smlng · 2018-01-18T12:47:36Z

@cladmi try again, please 🤞

kaspar030 · 2018-01-18T13:04:04Z

@cladmi try again, please 🤞

@smlng How about a little less fishing and a little more thinking?

cladmi · 2018-01-26T16:53:25Z

Makefile.include

 	-@rm -rf $(BINDIRBASE)

-flash: all $(FLASHDEPS)
+ifneq (,$(filter flash, $(MAKECMDGOALS)))


I prefer having a condition on flash-only here

As the check is on MAKECMDGOALS it means passed on the command line, so could break the current state if flash is set as a dependency of another target.

Also, the special case is required when flash-only is used, not when flash is used.

cladmi · 2018-01-26T17:05:06Z

Makefile.include


-flash: all $(FLASHDEPS)
+ifneq (,$(filter flash, $(MAKECMDGOALS)))
+  PREFLASHDEPS += all


Its not really PREFLASHDEPS as its also a deps for flash.
I do not think it needs to be made generic with the += also.

What we actually mean here is "flash when the firmware is built", by saying firmware.elf/.bin as a dependency but we can't right now. Maybe more a FIRMWARE_IS_BUILT or something.
Could get some other inputs, I am bad at names.

cladmi · 2018-01-26T17:14:18Z

Makefile.include

-preflash: all
+flash-only: flash
+
+preflash: $(PREFLASHDEPS)


The flash: and preflash: works now the same as before.

cladmi · 2018-01-29T13:37:42Z

To summarize, yes the current patch works as expected.

There are things I would prefer to be written differently but its my personal point of view and if other maintainers like it, it can go like this.

smlng · 2018-02-12T11:04:46Z

@cladmi I changed the logic/condition to work on the new flash-only target as you suggested. Do you approve?

cladmi · 2018-02-13T15:56:38Z

Looks good. I will test again but you can already squash and rebase.

cladmi · 2018-02-13T16:02:22Z

Sorry, please also add a comment for the ifeq (,$(filter flash-only, $(MAKECMDGOALS))) conditional.
Would help the next person seeing this.

cladmi · 2018-02-13T16:09:04Z

Tested with PREFLASHER="echo PREFLASHER" FLASHER="echo FLASHER" make BOARD=arduino-due -j 1000 flash. We correctly get all, then preflash then flash.
As expected flash-only does not re-compile.

cladmi · 2018-02-13T16:19:38Z

Please squash and rebase.

smlng · 2018-02-14T09:02:37Z

@kaspar030 addressed your comments.

When flash-only was introduced (in RIOT-OS#8373), the `flash` rule was made conditionally dependent on `all` by looking for `flash-only` in MAKECMDGOALS. This was done to avoid code duplication. There's a cleaner way, by using canned recipes.

When flash-only was introduced (in RIOT-OS#8373), the `flash` rule was made conditionally dependent on `all` by looking for `flash-only` in MAKECMDGOALS. This was done to avoid code duplication. There's a cleaner way, by using canned recipes. When we upgrade the requirements to gnu make 4, the flash recipe can be defined as ?=.

smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Jan 16, 2018

smlng requested review from jia200x and kaspar030 January 16, 2018 16:39

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2018

jia200x approved these changes Jan 16, 2018

View reviewed changes

smlng added this to the Release 2018.01 milestone Jan 16, 2018

smlng force-pushed the make/target/flash_only branch from 39869bd to 5158ed1 Compare January 17, 2018 10:15

kaspar030 requested changes Jan 17, 2018

View reviewed changes

smlng removed this from the Release 2018.01 milestone Jan 18, 2018

smlng force-pushed the make/target/flash_only branch from cae1ab1 to 28e3fc0 Compare January 18, 2018 12:46

cladmi reviewed Jan 26, 2018

View reviewed changes

smlng force-pushed the make/target/flash_only branch from 28e3fc0 to 0374bbc Compare February 12, 2018 11:03

smlng force-pushed the make/target/flash_only branch from 2328fe2 to c4194d4 Compare February 13, 2018 16:21

make: add flash-only target

c4194d4

cladmi approved these changes Feb 13, 2018

View reviewed changes

kaspar030 approved these changes Feb 16, 2018

View reviewed changes

kaspar030 merged commit d2fc38f into RIOT-OS:master Feb 16, 2018

smlng deleted the make/target/flash_only branch June 29, 2018 18:34

jcarrano mentioned this pull request Dec 5, 2018

Makefile.include: flash: do not peek into MAKECMDGOALS. #10548

Merged

Conversation

smlng commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

jia200x commented Jan 16, 2018

Uh oh!

cladmi commented Jan 16, 2018

Uh oh!

smlng commented Jan 16, 2018

Uh oh!

cladmi commented Jan 16, 2018

Uh oh!

smlng commented Jan 16, 2018

Uh oh!

jia200x commented Jan 16, 2018

Uh oh!

jia200x commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jia200x commented Jan 16, 2018

Uh oh!

cladmi commented Jan 17, 2018

Uh oh!

smlng commented Jan 17, 2018

Uh oh!

cladmi commented Jan 17, 2018

Uh oh!

cladmi commented Jan 17, 2018

Uh oh!

smlng commented Jan 17, 2018

Uh oh!

kaspar030 commented Jan 17, 2018

Uh oh!

kaspar030 commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Jan 17, 2018

Uh oh!

kaspar030 Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

smlng Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Jan 17, 2018

Uh oh!

smlng commented Jan 17, 2018

Uh oh!

cladmi commented Jan 17, 2018

Uh oh!

smlng commented Jan 17, 2018

Uh oh!

cladmi commented Jan 17, 2018

Uh oh!

miri64 commented Jan 17, 2018

Uh oh!

miri64 commented Jan 17, 2018

Uh oh!

smlng commented Jan 17, 2018

Uh oh!

cladmi commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Jan 18, 2018

Uh oh!

kaspar030 commented Jan 18, 2018

Uh oh!

cladmi Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jan 29, 2018

Uh oh!

smlng commented Jan 16, 2018 •

edited

Loading

jia200x commented Jan 16, 2018 •

edited

Loading

kaspar030 commented Jan 17, 2018 •

edited

Loading

cladmi commented Jan 18, 2018 •

edited

Loading