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.

Sign up for GitHub

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

Jump to bottom

Add downloader selection #2997

Merged
merged 8 commits into from May 26, 2023
Merged

Add downloader selection #2997

merged 8 commits into from May 26, 2023

Conversation

kasjer
Copy link
Contributor

@kasjer kasjer commented May 19, 2023

So far (almost) every BSP had its own version of download script.
Changing download software specially for ST devices was cumbersome.
Lately it was possible to override download script in target but
it still is more complicated then just changing syscfg value.

This adds common hw/scripts/download.sh file that can use all available
options to download image.

sh/scripts is converted to package to allow syscfg definitions that can
be overriden in BSPs or targets.

jlink, openocd, stutil, stm32_programmer_cli and pyocd can be selected.

Two example BSP were converted to common download script to see if it will catch up.

@kasjer kasjer force-pushed the kasjer/download-scripts branch 3 times, most recently from f7e876e to d6e729b Compare May 22, 2023 10:47
rymanluk
rymanluk approved these changes May 24, 2023
View reviewed changes
Copy link
Contributor

@rymanluk rymanluk left a comment

Choose a reason for hiding this comment

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

LGTM

@rymanluk rymanluk requested a review from mkiiskila May 24, 2023 14:08
@rymanluk
Copy link
Contributor

@kasjer could you update all other bsp so it use new way?

mkiiskila
mkiiskila approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@mkiiskila mkiiskila left a comment

Choose a reason for hiding this comment

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

I like the idea. It might also be helpful for 'download.cmd' to exist, for devs using windows?

I just had minor comments, nothing stopping this from going ahead.

hw/bsp/nordic_pca10095_net/bsp.yml Outdated
@@ -29,7 +29,7 @@ bsp.linkerscript.BOOT_LOADER.OVERWRITE:
- "hw/bsp/nordic_pca10095_net/boot-nrf5340_net.ld"
- "@apache-mynewt-core/hw/mcu/nordic/nrf5340_net/nrf5340_net.ld"
bsp.part2linkerscript: "hw/bsp/nordic_pca10095_net/split-nordic_pca10095_net.ld"
bsp.downloadscript: "hw/bsp/nordic_pca10095_net/nordic_pca10095_net_download.sh"
bsp.downloadscript: "@apache-mynewt-core/hw/scripts/download.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the repo name needed in the location for this BSP for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I will remove it for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

hw/scripts/download.sh
;;
"jlink")
. $CORE_PATH/hw/scripts/jlink.sh
JLINK_DEV=${MYNEWT_VAL_JLINK_TARGET}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we complain if the required variable is not set? The same applies to openocd case.

This might save users some debug time...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, fix on the way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kasjer
Copy link
Contributor Author

kasjer commented May 25, 2023

I like the idea. It might also be helpful for 'download.cmd' to exist, for devs using windows?

Originally .cmd files were needed on Windows because newt tool executed scripts just like other commands.
All the .cmd files were exactly the same executing bash with arguments.
For a log time now newt on Windows when some_script.sh file is to be executed executes bash some_script.sh instead.
The only reason to provide .cmd files again would be allow flashing devices from naked cmd.exe, but to make it work user would have to specify a lot of environment variables (that newt tool currently sets up) and still have bash.exe available in PATH variable directories.
I don't expect anyone is hardcore enough to do this this way.
All development for those scripts was done on Windows (at this point as a remainder I will make sure that those scripts actually have +x attribute set which they probably don't have since they were developed on NTFS).

In short as Windows user I don't see any reason to add .cmd files unless I'm missing something.

@kasjer
hw/scripts: Add downloader selection
93d97fa 
So far (almost) every BSP had its own version of download script.
Changing download software specially for ST devices was cumbersome.
Lately it was possible to override download script in target but
it still is more complicated then just changing syscfg value.

This adds common hw/scripts/download.sh file that can use all available
options to download image.

sh/scripts is converted to package to allow syscfg definitions that can
be overriden in BSPs or targets.

jlink, openocd, stutil, stm32_programmer_cli and pyocd can be selected.

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer
hw/scripts: download: Add support for nrfjprog
e84fa5e 
This change adds support for nrfjprog as an option to flash NRF devices

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer
hw/scripts: download: Add partial support for ezFlashCLI
d4daff9 
This change partial support for ezFlashCLI.
It is not possible to flash program header yet so bootloader should
not be flash this way (yet).

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer
hw/scripts: download: Add support for stm32_programmer_cli
78334ae 
This adds explicit support for stm32_programmer_cli.
It was possible to use this tool instead of st-flash
with environment variable.

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer
hw/scripts: pyocd fix for repo applications
a9299e8 
When pyocd is used to flash device is checks if file to flash ends with @<address>.
If @ is found in file name it tries to use remaining string as flash address.
For mcuboot or other applications that are in mynewt-core or nimble it would lead
to pyocd exception since "mcuboot" can't be converted to number.

To fix this -a argument that was used to specify load address is removed and
(possible) second @ with FLASH_OFFSET is appended to file name for pycd sake

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer
hw/bsp: nucleo-f767zi add syscfg selectable download script
64477a3 
This removes:
- .cmd files that were not need anymore
- nucleo_f767zi_download.sh (which is replaced by hw/scripts/download.sh)

Values for various download commands are put in syscfg so they don't have to
be specified in target when downloader is change to other option.

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer
hw/bsp: nordic_pca10095: Add syscfg selectable download script
f8a540f 
This switches to common download script that can be used to
flash device with nrfjprog and jlink

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer
hw/bsp: nucleo-f411re add syscfg selectable download script
1029206 
Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
@kasjer kasjer changed the title [RFC] Add downloader selection Add downloader selection May 26, 2023
@kasjer kasjer marked this pull request as ready for review May 26, 2023 07:14
@kasjer kasjer merged commit 4411709 into apache:master May 26, 2023
9 checks passed
@kasjer kasjer deleted the kasjer/download-scripts branch May 26, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkiiskila mkiiskila mkiiskila approved these changes

@rymanluk rymanluk rymanluk approved these changes

@utzig utzig Awaiting requested review from utzig

@andrzej-kaczmarek andrzej-kaczmarek Awaiting requested review from andrzej-kaczmarek

@sjanc sjanc Awaiting requested review from sjanc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kasjer @rymanluk @mkiiskila