zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.62k stars 6.5k forks source link

boards: st: Minimize usage of "--erase" stm32cubeprogrammer option #69582

Closed erwango closed 1 month ago

erwango commented 7 months ago

Is your enhancement proposal related to a problem? Please describe. On stm32 boards, following stm32cubeprogrammer configuration is commonly found:

board_runner_args(stm32cubeprogrammer "--erase" "--port=swd" "--reset-mode=hw")

This raises an issue when flashing back to back a bootloader and an application image.

Describe the solution you'd like Use the following:

if(CONFIG_BOOTLOADER_MCUBOOT)
board_runner_args(stm32cubeprogrammer "--port=swd" "--reset-mode=hw")
else()
board_runner_args(stm32cubeprogrammer "--erase" "--port=swd" "--reset-mode=hw")
endif()

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or graphics (drag-and-drop an image) about the feature request here.

erwango commented 7 months ago

As a follow up action:

nordicjm commented 7 months ago

The real solution should be that flashing never erases the full flash of the device (just the sectors being programmed) unless the --erase flash is provided to west flash. This already is the assumption of flash runners and will be going forward with things like sysbuild, so if there are oddities like this where it does an erase on its own, it is likely to break

erwango commented 7 months ago

The real solution should be that flashing never erases the full flash of the device (just the sectors being programmed) unless the --erase flash is provided to west flash. This already is the assumption of flash runners and will be going forward with things like sysbuild, so if there are oddities like this where it does an erase on its own, it is likely to break

Makes sense. The issue is that erase is required in CI, at least in specific tests. So we need to check how to do this first.

nordicjm commented 7 months ago

@PerMac was asking about this recently and my opinion it's wrong for any test to not do a full erase, it contaminates the test with undefined data. If you have 2 tests, one writes something and another checks for data in a certain area and there's no erase in-between, that test case is not valid because you cannot be sure the previous test isn't giving a false positive/negative

erwango commented 7 months ago

my opinion it's wrong for any test to not do a full erase

We're in line. This is why we have these commands in the first place. So before removing the default "--erase" in boards.cmake, we need to find how to force them with twister. This wasn't possible when we looked at it, but potentially this is now possible

nordicjm commented 7 months ago

@nashif @PerMac please see the above

PerMac commented 7 months ago

We run twister in CI with extra: --west-flash="--erase" to achieve clean board. This tells twister to call west for flashing and pass there --erase arg. I believe the arg to pass depends on the flashing tool you use. However, this won't work with susbuild since it is currently broken https://github.com/zephyrproject-rtos/zephyr/issues/50421 and the arg is used for every consecutive flash within the same "system", hence breaking stuff like bootloader tests with multiple images flashed in a sequence

PerMac commented 7 months ago

I was thinking that maybe marking only tests that require an erase in their yamls might be the way to go, so that stuff like mcuboot or sysbuild are not screwed in a common run, but I also understand the reasoning behind getting the clean state every time. Then sysbuild is to be fixed first and --west-flash="your_erase_options" might work

erwango commented 3 months ago

This tells twister to call west for flashing and pass there --erase arg. I believe the arg to pass depends on the flashing tool you use.

@PerMac About this, I've been trying to use --west-flash option and at first sight it doesn't play well with twister's hardware-map option which we're using extensively on our bench. Is this a known issue or maybe I'm doing something wrong ?

Also, it seems that it all depends on runner supporting --erase, which isn't the case for openocd, but which should work for stm32cubeprogrammer which is easy to switch to (if hardware-map is supported, otherwise it will require to update impacted boards upstream configurations)

PerMac commented 3 months ago

@erwango we also use hardware-map and --west-flash=--erase in the call and it works. Not sure what you mean. You can play with max verbosity (-v -v) and see what west command is used for flashing. I add --west-flash="--erase" to the CLI call of twister and use a hardware map. If I have runner: jlink in hw map I see west calling Flash command: ['west', 'flash', '--skip-rebuild', '-d', '/home/maciej/NCS-bis/nrf/twister-out/nrf9160dk_nrf9160/samples/hello_world/sample.basic.helloworld', '--runner', 'jlink', '--tool-opt=-SelectEmuBySN 000960033217', '--', '--erase']

using the same twister's command but having runner: nrfjprog in hw map I get: Flash command: ['west', 'flash', '--skip-rebuild', '-d', '/home/maciej/NCS-bis/nrf/twister-out/nrf9160dk_nrf9160/samples/hello_world/sample.basic.helloworld', '--runner', 'nrfjprog', '--', '--erase', '--dev-id', '000960033217'] To me it looks as expected. You can use hw map where you specify a runner and twister will pass this info in addition to west, along with value you passed with --west-flash

erwango commented 3 months ago

@PerMac Ok, it seems the only issue was coming from openocd not supporting --erase command. Using runner stm32cubeprogrammer, this is all fine. Thanks!

erwango commented 1 month ago

Fixed: https://github.com/zephyrproject-rtos/zephyr/pull/74627