Re: [PR] espressif: Add variable to override MCUboot version and URL [nuttx]
xiaoxiang781216 merged PR #16320: URL: https://github.com/apache/nuttx/pull/16320 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] espressif: Add variable to override MCUboot version and URL [nuttx]
tmedicci commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2075788287 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: Can it be provided on a separate PR? I would need to change the original implementation (for `ESP_HAL_3RDPARTY_[VERSION|URL]` and re-test everything on our internal CI. For the sake of consistency, we should keep them equal. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] espressif: Add variable to override MCUboot version and URL [nuttx]
anchao commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2075628670 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: I just think the `?=` syntax here is more concise and easier to understand. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] espressif: Add variable to override MCUboot version and URL [nuttx]
tmedicci commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2075363360 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: It's redundant. We already checked for the variable being (or not) defined one line before (and we did that to be more explicit). Are there other reasons that I'm missing here? (just for some background: this PR is highly inspired by https://github.com/apache/nuttx/pull/9545/commits/f8d8852cd6b722255f2da2ffe72b3d2fd7cd8b37, which our internal CI is using for two years) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] espressif: Add variable to override MCUboot version and URL [nuttx]
anchao commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2074362653 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) +endif + +ifndef MCUBOOT_URL + MCUBOOT_URL = https://github.com/mcu-tools/mcuboot Review Comment: ```suggestion MCUBOOT_URL ?= https://github.com/mcu-tools/mcuboot ``` ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: ```suggestion MCUBOOT_VERSION ?= $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] espressif: Add variable to override MCUboot version and URL [nuttx]
nuttxpr commented on PR #16320: URL: https://github.com/apache/nuttx/pull/16320#issuecomment-2852025746 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR meets the NuttX requirements. The summary clearly explains the change, its purpose, and how it works. The impact section adequately covers all the points, specifically noting the build change and explaining it. The testing section provides clear instructions on how to reproduce the test, including expected results. The use of specific commit hashes strengthens the verification. The information presented is concise and well-organized. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] espressif: Add variable to override MCUboot version and URL [nuttx]
tmedicci opened a new pull request, #16320:
URL: https://github.com/apache/nuttx/pull/16320
## Summary
* espressif: Add variable to override MCUboot version and URL
The version and the git repository of Espressif's MCUboot port can be
changed by setting the `MCUBOOT_VERSION` and `MCUBOOT_URL` environment
variables before running the `make bootloader` command.
## Impact
Impact on user: NO.
Impact on build: YES. If the environment variables `MCUBOOT_VERSION` and
`MCUBOOT_URL` are set, use them instead of the default values.
Impact on hardware: NO.
Impact on documentation: NO.
Impact on security: NO.
Impact on compatibility: NO.
## Testing
This can be tested by setting it to other values and checking the folder
where MCUboot is cloned. For ESP32-S3, for instance, the default values for
`MCUBOOT_VERSION` and `MCUBOOT_URL` are defined by, respectively, by
`CONFIG_ESP32S3_MCUBOOT_VERSION` (which currently is set to
`20f98e0a975c24864872e0df5701eb1082e9c957`) and
`https://github.com/mcu-tools/mcuboot`.
### Building
In this test, set another version and clone it from another repository (a
fork, for instance):
```
make -j distclean && ./tools/configure.sh esp32s3-devkit:mcuboot_nsh &&
MCUBOOT_VERSION="f6e8e88aa693c376a7e71d44aa04bc9908f0e74e"
MCUBOOT_URL="https://github.com/tmedicci/mcuboot"; make bootloader
```
### Running
Wait for the bootloader to be built...
### Results
Check the `nuttx/arch/xtensa/src/chip/bootloader/mcuboot` folder:
The version
```
$ git --no-pager show
commit f6e8e88aa693c376a7e71d44aa04bc9908f0e74e (HEAD, origin/main,
origin/HEAD, main)
```
The git repository
```
git remote -v
origin https://github.com/tmedicci/mcuboot (fetch)
origin https://github.com/tmedicci/mcuboot (push)
```
Both the git repository and the version retrieved correspond to the
overridden values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
