[GitHub] [incubator-nuttx] maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU
maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU URL: https://github.com/apache/incubator-nuttx/pull/437#discussion_r389096387 ## File path: tools/Makefile.unix ## @@ -501,6 +501,23 @@ ifeq ($(CONFIG_UBOOT_UIMAGE),y) cp -f uImage /tftpboot/uImage; \ fi endif +ifeq ($(CONFIG_ESP32_BINARY),y) + @echo "MKIMAGE: ESP32 binary" + $(Q) if ! esptool.py version | grep "v2[.]"; then \ Review comment: Thanks for the info, I will try, it could be a simpler way than the current approach. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU
maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU URL: https://github.com/apache/incubator-nuttx/pull/437#discussion_r388424886 ## File path: tools/Makefile.unix ## @@ -501,6 +501,23 @@ ifeq ($(CONFIG_UBOOT_UIMAGE),y) cp -f uImage /tftpboot/uImage; \ fi endif +ifeq ($(CONFIG_ESP32_BINARY),y) + @echo "MKIMAGE: ESP32 binary" + $(Q) if ! esptool.py version | grep "v2[.]"; then \ + echo ""; \ + echo "Please install ESP-IDF tools"; \ + echo ""; \ + echo "Check https://docs.espressif.com/projects/esp-idf/en/v4.0/get-started/index.html#installation-step-by-step or run the following command"; \ + echo ""; \ + echo "cd tools/esp32 && make && cd ../.."; \ + echo ""; \ + echo "run make again to create the nuttx.bin image."; \ +else \ + echo "Generating: $(NUTTXNAME).bin (ESP32 compatible)"; \ + esptool.py --chip esp32 elf2image --flash_mode dio --flash_size 4MB -o nuttx.bin nuttx && \ + echo "Generated: $(NUTTXNAME).bin (ESP32 compatible)"; \ + fi +endif Review comment: Ok, then try to move the other examples of bad practice of this case or at least leave a message in the code if the efforts spans more than a week or so. I think once there is no "bad example" on place, it would be easier to anyone to point out what is accepted and what is not, so the human review effort would be less, hopefully. And by the way, from my part, thanks for being so strict :) It pays off on the long term. I will try to port some of the other cases if I have some time or contribute as needed. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU
maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU URL: https://github.com/apache/incubator-nuttx/pull/437#discussion_r388406435 ## File path: tools/Makefile.unix ## @@ -501,6 +501,23 @@ ifeq ($(CONFIG_UBOOT_UIMAGE),y) cp -f uImage /tftpboot/uImage; \ fi endif +ifeq ($(CONFIG_ESP32_BINARY),y) + @echo "MKIMAGE: ESP32 binary" + $(Q) if ! esptool.py version | grep "v2[.]"; then \ + echo ""; \ + echo "Please install ESP-IDF tools"; \ + echo ""; \ + echo "Check https://docs.espressif.com/projects/esp-idf/en/v4.0/get-started/index.html#installation-step-by-step or run the following command"; \ + echo ""; \ + echo "cd tools/esp32 && make && cd ../.."; \ + echo ""; \ + echo "run make again to create the nuttx.bin image."; \ +else \ + echo "Generating: $(NUTTXNAME).bin (ESP32 compatible)"; \ + esptool.py --chip esp32 elf2image --flash_mode dio --flash_size 4MB -o nuttx.bin nuttx && \ + echo "Generated: $(NUTTXNAME).bin (ESP32 compatible)"; \ + fi +endif Review comment: > I do have to fight on an almost daily basis to maintain good modularity in the OS, but I feel like I am losing the battle. I am being overwhelmed by self-serving, ill-conceived changes that keep the submitter from having do the amount of work that is necessary to maintain the modularity. Sorry to hear that, I don't wanted to cause trouble. But in my defense, if something is not good in the first place, it generates a dilemma on new additions: should we care more about consistency with existing practices (even bad ones) or should we do the correct one despite that will create inconsistencies. The ideal solution should migrate everything, but that puts a burden on the people contributing that was not involved on the original bad practice. So, I will suggest that we should identify that bad practices and at least, if not fix them, mark them as bad _in the code_ (like some FIXME commment). That way the new contributors could be more aware about the preference of other merits over inconsistency in that particular case. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU
maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU URL: https://github.com/apache/incubator-nuttx/pull/437#discussion_r388402325 ## File path: tools/Makefile.unix ## @@ -501,6 +501,23 @@ ifeq ($(CONFIG_UBOOT_UIMAGE),y) cp -f uImage /tftpboot/uImage; \ fi endif +ifeq ($(CONFIG_ESP32_BINARY),y) + @echo "MKIMAGE: ESP32 binary" + $(Q) if ! esptool.py version | grep "v2[.]"; then \ + echo ""; \ + echo "Please install ESP-IDF tools"; \ + echo ""; \ + echo "Check https://docs.espressif.com/projects/esp-idf/en/v4.0/get-started/index.html#installation-step-by-step or run the following command"; \ + echo ""; \ + echo "cd tools/esp32 && make && cd ../.."; \ + echo ""; \ + echo "run make again to create the nuttx.bin image."; \ +else \ + echo "Generating: $(NUTTXNAME).bin (ESP32 compatible)"; \ + esptool.py --chip esp32 elf2image --flash_mode dio --flash_size 4MB -o nuttx.bin nuttx && \ + echo "Generated: $(NUTTXNAME).bin (ESP32 compatible)"; \ + fi +endif Review comment: > I wonder if we don't need to hook to run: > > ``` > $(TOPDIR)/tools/arch/Makefile at the conclusion of each build. > ``` > > Where arch is a symbolic link to either a dummy directory or to a directory holding a platform-specific fixup? Yes, I was looking at the code to see if something like that was available, but I only saw other architectures were doing directly there. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU
maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU URL: https://github.com/apache/incubator-nuttx/pull/437#discussion_r388345251 ## File path: tools/Makefile.unix ## @@ -501,6 +501,23 @@ ifeq ($(CONFIG_UBOOT_UIMAGE),y) cp -f uImage /tftpboot/uImage; \ fi endif +ifeq ($(CONFIG_ESP32_BINARY),y) + @echo "MKIMAGE: ESP32 binary" + $(Q) if ! esptool.py version | grep "v2[.]"; then \ Review comment: I tried without success, but maybe there is some way, since I am not familiar with it, sorry. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU
maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU URL: https://github.com/apache/incubator-nuttx/pull/437#discussion_r388336900 ## File path: tools/Makefile.unix ## @@ -501,6 +501,23 @@ ifeq ($(CONFIG_UBOOT_UIMAGE),y) cp -f uImage /tftpboot/uImage; \ fi endif +ifeq ($(CONFIG_ESP32_BINARY),y) + @echo "MKIMAGE: ESP32 binary" + $(Q) if ! esptool.py version | grep "v2[.]"; then \ + echo ""; \ + echo "Please install ESP-IDF tools"; \ + echo ""; \ + echo "Check https://docs.espressif.com/projects/esp-idf/en/v4.0/get-started/index.html#installation-step-by-step or run the following command"; \ + echo ""; \ + echo "cd tools/esp32 && make && cd ../.."; \ + echo ""; \ + echo "run make again to create the nuttx.bin image."; \ +else \ + echo "Generating: $(NUTTXNAME).bin (ESP32 compatible)"; \ + esptool.py --chip esp32 elf2image --flash_mode dio --flash_size 4MB -o nuttx.bin nuttx && \ + echo "Generated: $(NUTTXNAME).bin (ESP32 compatible)"; \ + fi +endif Review comment: But now I remember: I did it because it seemed that there was a precedent some lines before in `CONFIG_CXD56_BINARY` (lines 483 to 495). Not that I want to repeat it if that is a "bad practice" or special case/rationale behind it. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU
maht commented on a change in pull request #437: Support to run NuttX on ESP32 QEMU URL: https://github.com/apache/incubator-nuttx/pull/437#discussion_r388333738 ## File path: tools/Makefile.unix ## @@ -501,6 +501,23 @@ ifeq ($(CONFIG_UBOOT_UIMAGE),y) cp -f uImage /tftpboot/uImage; \ fi endif +ifeq ($(CONFIG_ESP32_BINARY),y) + @echo "MKIMAGE: ESP32 binary" + $(Q) if ! esptool.py version | grep "v2[.]"; then \ + echo ""; \ + echo "Please install ESP-IDF tools"; \ + echo ""; \ + echo "Check https://docs.espressif.com/projects/esp-idf/en/v4.0/get-started/index.html#installation-step-by-step or run the following command"; \ + echo ""; \ + echo "cd tools/esp32 && make && cd ../.."; \ + echo ""; \ + echo "run make again to create the nuttx.bin image."; \ +else \ + echo "Generating: $(NUTTXNAME).bin (ESP32 compatible)"; \ + esptool.py --chip esp32 elf2image --flash_mode dio --flash_size 4MB -o nuttx.bin nuttx && \ + echo "Generated: $(NUTTXNAME).bin (ESP32 compatible)"; \ + fi +endif Review comment: Thanks for the feedback. Yes, I figured out that this code was not good enough to be ready to merge, that is the reason I wanted to work more on it before uploading ... but some people requested some info about how to use QEMU, and it was easier to just create the pull request to share and accelerate the feedback loop. I will try to see if there is a way to move the code to proper location. 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: us...@infra.apache.org With regards, Apache Git Services
