You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/03/09 12:53:56 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Ouss4 opened a new pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012
## Summary
This PR has two commits:
1. Always flash with DIO. Some issues have been observed with QIO.
2. Let make continues if the QEMU generation script fails.
## Impact
ESP32xx chips/boards only.
## Testing
Building and testing ESP32 and ESP32-C3 boards.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#discussion_r590355046
##########
File path: tools/esp32c3/Config.mk
##########
@@ -52,13 +52,12 @@ else ifeq ($(CONFIG_ESP32C3_FLASH_FREQ_20M),y)
FLASH_FREQ := 20m
endif
-ESPTOOL_FLASH_OPTS := -fs $(FLASH_SIZE) -fm $(FLASH_MODE) -ff $(FLASH_FREQ)
-ESPTOOL_ELF2IMG_OPTS := $(ESPTOOL_FLASH_OPTS)
+ESPTOOL_ELF2IMG_OPTS := -fs $(FLASH_SIZE) -fm $(FLASH_MODE) -ff $(FLASH_FREQ)
ifeq ($(CONFIG_ESP32C3_FLASH_DETECT),y)
- ESPTOOL_WRITEFLASH_OPTS := -fs detect -fm $(FLASH_MODE) -ff $(FLASH_FREQ)
+ ESPTOOL_WRITEFLASH_OPTS := -fs detect -fm dio -ff $(FLASH_FREQ)
else
- ESPTOOL_WRITEFLASH_OPTS := $(ESPTOOL_FLASH_OPTS)
+ ESPTOOL_WRITEFLASH_OPTS := -fs detect -fm dio -ff $(FLASH_FREQ)
Review comment:
```suggestion
ESPTOOL_WRITEFLASH_OPTS := -fs $(FLASH_SIZE) -fm dio -ff $(FLASH_FREQ)
```
Here I think you forgot the FLASH_SIZE param
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#discussion_r590360977
##########
File path: tools/esp32/mk_qemu_img.sh
##########
@@ -71,6 +72,8 @@ done
# Make sure we have the required argument(s)
if [ -z "${BOOTLOADER_IMG}" ] || [ -z "${PARTITION_IMG}" ] ; then
+ echo ""
+ echo "Failed to generate QEMU image!"
Review comment:
There is no output before "USAGE" that explains what action was going on, it seemed an out of the blue "Missing bootloader and partition table binary images."
I don't feel too strong about keeping that line, though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#issuecomment-794706483
> @gustavonihei PTAL.
>
> A motivation for the second commit is:
>
> 1. Enable QEMU generation.
>
> 2. Test with QEMU
>
> 3. Verify in hardware if things are also working/not working --> Can't use `make` directly because the script aborts
>
> 4. `menuconfig` disable QEMU generation and build again...
>
> 5. Try with QEMU directly from `make` --> Same issue as 3.
i don't understand.
* by `Enable QEMU generation`, do you mean CONFIG_ESP32_QEMU_IMAGE=y ?
* if the script failed when you have CONFIG_ESP32_QEMU_IMAGE=y, it sounds like a real error it shouldn't be ignored.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#issuecomment-794702841
can you give me an example of ` Some issues have been observed with QIO`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#discussion_r590357228
##########
File path: tools/esp32c3/Config.mk
##########
@@ -52,13 +52,12 @@ else ifeq ($(CONFIG_ESP32C3_FLASH_FREQ_20M),y)
FLASH_FREQ := 20m
endif
-ESPTOOL_FLASH_OPTS := -fs $(FLASH_SIZE) -fm $(FLASH_MODE) -ff $(FLASH_FREQ)
-ESPTOOL_ELF2IMG_OPTS := $(ESPTOOL_FLASH_OPTS)
+ESPTOOL_ELF2IMG_OPTS := -fs $(FLASH_SIZE) -fm $(FLASH_MODE) -ff $(FLASH_FREQ)
ifeq ($(CONFIG_ESP32C3_FLASH_DETECT),y)
- ESPTOOL_WRITEFLASH_OPTS := -fs detect -fm $(FLASH_MODE) -ff $(FLASH_FREQ)
+ ESPTOOL_WRITEFLASH_OPTS := -fs detect -fm dio -ff $(FLASH_FREQ)
else
- ESPTOOL_WRITEFLASH_OPTS := $(ESPTOOL_FLASH_OPTS)
+ ESPTOOL_WRITEFLASH_OPTS := -fs detect -fm dio -ff $(FLASH_FREQ)
Review comment:
That's right. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#issuecomment-793855954
@gustavonihei PTAL.
A motivation for the second commit is:
1. Enable QEMU generation.
2. Test with QEMU
3. Verify in hardware if things are also working/not working --> Can't use `make` directly because the script aborts
4. `menuconfig` disable QEMU generation and build again...
5. Try with QEMU directly from `make` --> Same issue as 3.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#discussion_r590368658
##########
File path: tools/esp32/mk_qemu_img.sh
##########
@@ -71,6 +72,8 @@ done
# Make sure we have the required argument(s)
if [ -z "${BOOTLOADER_IMG}" ] || [ -z "${PARTITION_IMG}" ] ; then
+ echo ""
+ echo "Failed to generate QEMU image!"
Review comment:
I understand. Do you think that maybe adding ${SCRIPT_NAME} to the echo line below could help indicate that the issue is on the QEMU image generation?
`echo "${SCRIPT_NAME}: Missing bootloader and partition table binary images."`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#discussion_r590357999
##########
File path: tools/esp32/mk_qemu_img.sh
##########
@@ -71,6 +72,8 @@ done
# Make sure we have the required argument(s)
if [ -z "${BOOTLOADER_IMG}" ] || [ -z "${PARTITION_IMG}" ] ; then
+ echo ""
+ echo "Failed to generate QEMU image!"
Review comment:
Is it really necessary to output these lines?
I would let the "failure" only for the generation process itself. For the omission of parameters I think the USAGE output is already intuitive.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#issuecomment-795014793
@yamt
> can you give me an example of Some issues have been observed with QIO?
Flashing with QIO was resulting in a boot loop (from the ROM bootloader). Usually such issue comes from a flash that doesn't support QIO, but I tried with multiple boards and had the same issue.
When you generate a bootloader with QIO, it will still enable QIO when started, so the flashing with DIO will not removing the whole option. I still don't know the real cause, but I found that IDF also flashes only with DIO. I will investigate this in a later time, we have plans to also add pre-built binaries for the different modes in this [repo](https://github.com/espressif/esp-nuttx-bootloader), so I will get to the bottom of this. Right now, it's best to remove the option to flash with QIO.
> by Enable QEMU generation, do you mean CONFIG_ESP32_QEMU_IMAGE=y ?
Yes, what I'm trying to do here is to be able to flash the board and generate the QEMU image without going through menuconfig and build again each time.
> if the script failed when you have CONFIG_ESP32_QEMU_IMAGE=y, it sounds like a real error it shouldn't be ignored.
When the script is used from `make`, the only issue that should be reported is a missing directory for the bootloader and partition table, which is caused deliberately by not providing the ESPTOOL_BINDIR variable.
The QEMU script is the last thing executed during the POSTBUILD hook, when `CONFIG_ESP32_QEMU_IMAGE` is enabled and I'm only interested in flashing the board with the `download` target, I'd like to ignore the script error and carry on with flashing the board.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#issuecomment-795043412
I see your point. Note, however, that the "failure" is on purpose, so ignoring it didn't seem too big of an issue, but I understand that this can cause other issues.
I'll restore the old behaviour.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#issuecomment-795029656
> @yamt
>
> > can you give me an example of Some issues have been observed with QIO?
>
> Flashing with QIO was resulting in a boot loop (from the ROM bootloader). Usually such issue comes from a flash that doesn't support QIO, but I tried with multiple boards and had the same issue.
> When you generate a bootloader with QIO, it will still enable QIO when started, so the flashing with DIO will not removing the whole option. I still don't know the real cause, but I found that IDF also flashes only with DIO. I will investigate this in a later time, we have plans to also add pre-built binaries for the different modes in this [repo](https://github.com/espressif/esp-nuttx-bootloader), so I will get to the bottom of this. Right now, it's best to remove the option to flash with QIO.
ok. thank you for explaination.
>
> > by Enable QEMU generation, do you mean CONFIG_ESP32_QEMU_IMAGE=y ?
>
> Yes, what I'm trying to do here is to be able to flash the board and generate the QEMU image without going through menuconfig and build again each time.
>
> > if the script failed when you have CONFIG_ESP32_QEMU_IMAGE=y, it sounds like a real error it shouldn't be ignored.
>
> When the script is used from `make`, the only issue that should be reported is a missing directory for the bootloader and partition table, which is caused deliberately by not providing the ESPTOOL_BINDIR variable.
>
> The QEMU script is the last thing executed during the POSTBUILD hook, when `CONFIG_ESP32_QEMU_IMAGE` is enabled and I'm only interested in flashing the board with the `download` target, I'd like to ignore the script error and carry on with flashing the board.
i disagree here.
if it failed to produce the one of the main artifacts, the entire make should be flagged as failed.
i guess you can do:
* enable and disable `CONFIG_ESP32_QEMU_IMAGE` depending on what you want
* or, always provide ESPTOOL_BINDIR so that the script doesn't fail
* or, don't use `CONFIG_ESP32_QEMU_IMAGE` even when you use qemu. (this is what i usually do)
* or, make it a separate target. eg. `make qemu-image`
any of the above options are better than ignoring the error, IMO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3012: tools/esp32xx: Few fixes/improvements to the post build scripts.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3012:
URL: https://github.com/apache/incubator-nuttx/pull/3012#discussion_r590373273
##########
File path: tools/esp32/mk_qemu_img.sh
##########
@@ -71,6 +72,8 @@ done
# Make sure we have the required argument(s)
if [ -z "${BOOTLOADER_IMG}" ] || [ -z "${PARTITION_IMG}" ] ; then
+ echo ""
+ echo "Failed to generate QEMU image!"
Review comment:
> echo "${SCRIPT_NAME}: Missing bootloader and partition table binary images."
Yeah, that works fine for me!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org