scripts: west_commands: runners: fix and removes the '--dt-flash' option. #85395
Conversation
miggazElquez
force-pushed
the
dt-flash_fix
branch
from
f120153 to
3f6950f
Compare
|
For the historical record, I added --dt-flash a long time ago when DT was new and experimental in zephyr; I could see a justification for getting rid of it as long we respect the default code partition when it is enabled and the appropriate kconfig is set directing the build system to use it |
|
This PR doesn't change the behavior of any board, on all the concerned boards the flash address comes from |
Due to a bug in scripts/west_commands/runners/core.py, the value of --dt-flash can currently be True, False, or the default value, 'n'. The script silabs_commander.py only check the truth value of --dt-flash, so the default value is interpreted as a truthy. That means that the default case was effectively the same as if dt-flash was set to True. Set this flag to True by default explicitly, in the same way as done for pyocd or jlink. Signed-off-by: Miguel Gazquez <[email protected]>
The '--dt-flash' parameter accepts a string like "y", "yes", "no", etc, and is supposed to be converted into a boolean value. This is only the theory as in practice, the default value is set to 'n' and is never converted to False afterwards. Set the default value to False. Signed-off-by: Miguel Gazquez <[email protected]>
In minichlink and spi_burn, the script checks if dt_flash is True by checking if the value is "y". But dt_flash is a boolean. Fix these checks. Also, when dt-flash is True, spi_burn calculate the address in a convoluted way, by substrating CONFIG_FLASH_LOAD_OFFSET to itself. Simplify this computation. Signed-off-by: Miguel Gazquez <[email protected]>
miggazElquez
force-pushed
the
dt-flash_fix
branch
from
3f6950f to
95c80b8
Compare
zephyrbot
requested review from
alexanderwachter,
henrikbrixandersen,
martinjaeger and
str4t0m
|
Rebased on main, and removed |
|
@mbolivar PTAL |
| import os | ||
| import re | ||
| import subprocess | ||
| import time | ||
|
|
||
| from runners.core import BuildConfiguration, RunnerCaps, ZephyrBinaryRunner | ||
|
|
||
| _YN_CHOICES = ['Y', 'y', 'N', 'n', 'yes', 'no', 'YES', 'NO'] |
There was a problem hiding this comment.
It could be possible to set dt_flash to always on unless a addr is set (which can be 0)
| @@ -558,7 +558,7 @@ def add_parser(cls, parser): | |||
| parser.add_argument('-i', '--dev-id', help=argparse.SUPPRESS) | |||
|
|
|||
| if caps.flash_addr: | |||
| parser.add_argument('--dt-flash', default='n', choices=_YN_CHOICES, | |||
| parser.add_argument('--dt-flash', default=False, choices=_YN_CHOICES, | |||
There was a problem hiding this comment.
This doesn't make sense to me. False is not in _YN_CHOICES. The point of action=_DTFlashAction was to do the conversion to bool based on the value. If that's not working, the solution is to fix the action, not change the default to a value that isn't in choices.
There was a problem hiding this comment.
The action is not run if there is no value passed to the argument, only if the value is passed through the command line. So without argument the final value is 'n', but if we pass --dt-flash n, then _DTFlashAction is called, and we correctly get False.
| @@ -557,15 +535,6 @@ def add_parser(cls, parser): | |||
| else: | |||
| parser.add_argument('-i', '--dev-id', help=argparse.SUPPRESS) | |||
|
|
|||
| if caps.flash_addr: | |||
| parser.add_argument('--dt-flash', default=False, choices=_YN_CHOICES, | |||
There was a problem hiding this comment.
OK, I just got to this commit while reviewing and see the earlier patch changing default=False has no effect in the PR at all, so I'm now I'm just confused why it's needed in the PR.
There was a problem hiding this comment.
The situation currently is that dt-flash is broken in some cases. The idea was to first have commits fixing it (the first three). Then, as in the fixed situation every user of the option set it to True, delete it.
I did it like that because I thought it would be easier to understand why I wanted to delete the option, and I can easily either squash everything into one commit to just remove dt-flash, or just drop the last one to just fix the handling of the option but keeping it. Sorry if instead of helping it just made everything more confusing.
| @@ -309,7 +305,6 @@ class RunnerCaps: | |||
| commands: set[str] = field(default_factory=lambda: set(_RUNNERCAPS_COMMANDS)) | |||
| dev_id: bool = False | |||
| mult_dev_ids: bool = False | |||
| flash_addr: bool = False | |||
There was a problem hiding this comment.
This is a backwards incompatible API change to a stable API:
https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#out-of-tree-runners
I don't mind if you want to take it through the stable API change process, but it seems like it would be easier to just keep it around for compatibility, no?
There was a problem hiding this comment.
Oh I didn't know it was part of the stable API. I guess it would be easier to keep it then.
But it means that a lot of out of tree runners are probably mishandling the option as well, because if they test the option using something like that :
#In the constructor
dt_flash=args.dt_flash
#In the handling of the option
if self.dt_flash:
flash_addr = self.flash_address_from_build_conf(self.build_conf)
else:
flash_addr = 0then they treat the default case as True instead of False (case of the runner for Silabs commander, see first commit). So by fixing the problem we will cause these runners to just go from an effective default of True to False.
If they used this:
dt_flash=(args.dt_flash == "y"),then the option can never be set to True (case of minichlink and spi_burn, see commit 3), but at least the fix will not break anything.
If some runners noticed the bug and were coded accordingly (by testing for 'n' and the boolean values) then this should not break anything.
I don't know what it the way to handle this situation, where fixing a bug in a stable API will probably lead to breaks in the code of the user of the API ?
miggazElquez
force-pushed
the
dt-flash_fix
branch
from
95c80b8 to
aec9c29
Compare
|
I removed the commit removing dt-flash, now there is just the fix left. |
|
@mbolivar please revisit |
The '--dt-flash' parameter accepts a string like "y", "yes", "no", etc, and is supposed to be converted into a boolean value. This is only the theory as in practice, the default value is set to 'n' and is never converted to False afterwards.
Some runners are depending on this behavior, so this pull request fixes these runners, and corrects the value of the '--dt-flash' option.
Once everything is fixed, I noticed that the --dt-flash option was not really useful, as it was set to True almost everywhere. So the fourth commit removes the option, except for the spi_burn runner, which seems to be using it. I didn't really understand what was spi_burn trying to do, so I just moved the handling of the option into
spi_burn.py. Maybe it isn't useful and should also be removed ?If --dt-flash is useful and I just didn't saw why, I can remove the fourth commit and only keep the first three.