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.

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

tools/espressif: Improve version checking by subprocess #15854

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Feb 18, 2025

Summary

  • Replace version checking logic using pkg_resources and importlib with a subprocess call to esptool.py version
  • This change enhances compatibility with esptool installed via pipx and simplifies the version retrieval process

Impact

  • No functional changes; the script continues to prompt for installation if esptool is not found
  • Increases maintainability by reducing dependency on Python version checks

Testing

GitHub CI and local build:

Create version.h
CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.api.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/espCPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.newlib.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.libgcc.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.version.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_romCPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/soc/esp32s3/ld/esp32s3.peripherals.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/soc/esp32s3LD: nuttx
Memory region         Used Size  Region Size  %age Used
             ROM:      418660 B   16777184 B      2.50%
     iram0_0_seg:       25344 B       304 KB      8.14%
     irom0_0_seg:      484196 B   16777184 B      2.89%
     dram0_0_seg:       95684 B       288 KB     32.44%
     drom0_0_seg:      106916 B   16777184 B      0.64%
    rtc_iram_seg:           0 B       8168 B      0.00%
    rtc_data_seg:           0 B       8168 B      0.00%
rtc_reserved_seg:           0 B         24 B      0.00%
    rtc_slow_seg:           0 B         8 KB      0.00%
MKIMAGE: ESP32-S3 binary
esptool.py -c esp32s3 elf2image --ram-only-header -fs 16MB -fm dio -ff 80m -o nuttx.bin nuttx
esptool.py v4.8.1
Creating esp32s3 image...
Image has only RAM segments visible. ROM segments are hidden and SHA256 digest is not appended.
Merged 1 ELF section
Successfully created esp32s3 image.
Generated: nuttx.bin

Test with esptool 4.7:

Create version.h
CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.api.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/espCPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.libgcc.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.newlib.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/CPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_rom/esp32s3/ld/esp32s3.rom.version.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/esp_romCPP:  /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/soc/esp32s3/ld/esp32s3.peripherals.ld-> /home/huang/Work/spark/nuttx/arch/xtensa/src/chip/esp-hal-3rdparty/components/soc/esp32s3LD: nuttx
Memory region         Used Size  Region Size  %age Used
             ROM:      601898 B   16777184 B      3.59%
     iram0_0_seg:       39168 B       304 KB     12.58%
     irom0_0_seg:      667434 B   16777184 B      3.98%
     dram0_0_seg:      107932 B       288 KB     36.60%
     drom0_0_seg:       94148 B   16777184 B      0.56%
    rtc_iram_seg:           0 B       8168 B      0.00%
    rtc_data_seg:           0 B       8168 B      0.00%
rtc_reserved_seg:           0 B         24 B      0.00%
    rtc_slow_seg:           0 B         8 KB      0.00%
MKIMAGE: ESP32-S3 binary
Unsupported esptool version: esptool.py v4.7.0
4.7.0 expects >= 4.8.0
Upgrade using: 'pip install --upgrade esptool' and run 'make' again
make: *** [tools/Unix.mk:556: nuttx] Error 1
make: Leaving directory '/home/huang/Work/spark/nuttx'

@github-actions github-actions bot added Area: Tooling Size: S The size of the change in this PR is small labels Feb 18, 2025
@no1wudi no1wudi force-pushed the tool branch 2 times, most recently from 1ed2895 to db2df36 Compare February 18, 2025 06:44
@nuttxpr
Copy link

nuttxpr commented Feb 18, 2025

[Experimental Bot, please feedback here]

This PR appears to meet the NuttX requirements, although some clarifications and additions would improve it.

Strengths:

  • Clear Summary: The summary clearly explains the "why," "what," and "how" of the change.
  • Impact is Addressed: The impact section covers several relevant areas.
  • Testing Provided: Testing logs are included, demonstrating functionality before and after the change.

Weaknesses:

  • Incomplete Impact Assessment: Several "Impact" fields are not addressed. While they may not be applicable, explicitly stating "NO" with a brief justification (e.g., "No impact on documentation as this is an internal scripting change.") is better for clarity.
  • Limited Testing Detail: The testing logs show successful execution but lack details on what was tested. Did the script correctly detect different esptool versions? What happened when esptool was not installed? More comprehensive tests would strengthen the PR.
  • Missing Issue References: If this PR addresses a specific issue, include the relevant links.
  • Build Host Details Missing: The testing section mentions "GitHub CI and local build" but lacks specifics about the local build environment (OS, CPU, compiler).

Recommendations for Improvement:

  1. Complete the Impact Section: Explicitly address all points in the impact section, even if the answer is "NO." Briefly justify negative responses.
  2. Enhance Testing Detail: Provide more specific test cases and their expected outcomes. Include scenarios like:
    • esptool installed via pipx
    • esptool installed system-wide
    • esptool not installed
    • different esptool versions (older, newer)
  3. Add Issue References: If applicable, link to the relevant NuttX or NuttX Apps issue(s).
  4. Specify Build Host Environment: Provide details about your local build environment (OS, CPU, compiler version) in the testing section.

Example of an improved Impact section:

## Impact

* Is new feature added? NO
* Is existing feature changed? YES - The method of detecting the esptool version is changed, but the overall functionality (prompting for installation if not found) remains the same.
* Impact on user (will user need to adapt to change)? NO
* Impact on build (will build process change)? NO
* Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO
* Impact on documentation (is update required / provided)? NO - This change is internal to the build script and does not affect user-facing documentation.
* Impact on security (any sort of implications)? NO
* Impact on compatibility (backward/forward/interoperability)? YES - Improved compatibility with various esptool installation methods (e.g., pipx).
* Anything else to consider?  This simplifies the version checking logic and reduces reliance on specific Python package management internals.

By addressing these points, the PR will be more comprehensive, easier to review, and contribute to higher quality code.

@tmedicci
Copy link
Contributor

@eren-terzioglu and @fdcavalcanti , can you please take a look?

@fdcavalcanti
Copy link
Contributor

Does it solve an issue? Are you having problems with pipx?

@fdcavalcanti
Copy link
Contributor

fdcavalcanti commented Feb 18, 2025

Please provide test with esptool < 4.8.0.
If possible, also test with older Python i.e. < 3.8, as some users might use it on older OSes.

@no1wudi
Copy link
Contributor Author

no1wudi commented Feb 19, 2025

Does it solve an issue? Are you having problems with pipx?

Yes, each app installed by pipx like tools is in a isolated venv, pkg_resources can't fetch the info if not switch to it.

Summary:
- Replace version checking logic using pkg_resources and importlib with a subprocess call to `esptool.py version`
- This change enhances compatibility with esptool installed via pipx and simplifies the version retrieval process

Impact:
- No functional changes; the script continues to prompt for installation if esptool is not found
- Increases maintainability by reducing dependency on Python version checks

Signed-off-by: Huang Qi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Tooling Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants