-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Fix EMSK EM7D makefile and introduce new defconfigs #4884
Conversation
Signed-off-by: Anas Nashif <[email protected]>
Signed-off-by: Anas Nashif <[email protected]>
We do not need python defined, we are running in python already. Signed-off-by: Anas Nashif <[email protected]>
scripts/support/runner/openocd.py
Outdated
@@ -94,8 +94,12 @@ def create_from_env(command, debug): | |||
zephyr_base = get_env_or_bail('ZEPHYR_BASE') | |||
arch = get_env_or_bail('ARCH') | |||
board_name = get_env_or_bail('BOARD_NAME') | |||
openocd_config = path.join(zephyr_base, 'boards', arch, | |||
board_name, 'support', 'openocd.cfg') | |||
for dirpath, dirnames, filenames in os.walk(path.join(zephyr_base, 'boards', arch), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this is trying to do (the commit message is pretty skimpy); would you mind explaining? This seems to have some problems.
In particular:
- do you really want to use os.walk()? That will look for $BOARD.yaml in every directory under boards/$ARCH/, including the doc/ directories, etc.
- Won't this fail in a confusing way (to the user) if there is no such yaml file?
- What is the correct behavior if there are multiple candidate yaml files?
- This is duplicated code, why not add it as a method to ZephyrBinaryRunner in core.py?
It seems like you might want something like this instead (in a new method in ZephyrBinaryRunner):
board_yaml_files = glob.glob(path.join(zephyr_base, 'boards', arch, '*',
'{}.yaml'.format(board_name)))
if not board_yaml_files:
raise ValueError('no {}.yaml was found'.format(board_name))
board_dir = path.dirname(board_yaml_files[0])
openocd_config = path.join(board_dir, 'support', 'openocd.cfg')
But that just takes the directory from the first found YAML file, which might not be what you want (though it's what this code is doing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, above code works better I guess.
I still need to wrap my header around those scripts and understand them better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, since it's openocd-specific, maybe a freestanding function in core.py instead of a method in the runner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found another easier way to do that and cleaning up and removing the need for some variables. cmake already finds the BOARD_DIR for us, so no reason to search for it again in the flasher
Signed-off-by: Anas Nashif <[email protected]>
We now have 3 configurations, so reflect this in the documentation. Signed-off-by: Anas Nashif <[email protected]>
Some boards define multiple configuration which all are maintained under the same board directory. The flasher was looking for an openocd.cfg based on the board name, which can't be found for such boards. Use the variable BOARD_DIR provided by cmake instead of trying to assemble the board directory location on our own. Signed-off-by: Anas Nashif <[email protected]>
@@ -16,7 +16,7 @@ | |||
|
|||
|
|||
class ArcBinaryRunner(ZephyrBinaryRunner): | |||
'''Runner front-end for the ARC architecture, using openocd.''' | |||
'''Runner front-end for the EM Starterkit board, using openocd.''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's true, why is this still named arc.py? Is this really not useful for any other boards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not trying to stand in the way, just honestly confused about what this script's purpose is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, there is nothing arc specific about this, it is just "flashing" using gdb, i.e. it is not really flashing and just loading the ELF into memory, calling this a runner for the ARC architecture is wrong and probably the name should be changed and the script made to be more generic to handle this case on other boards where this is the only way to load an image on a device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the explanations
Fixed #4889