From 0adfd082706d890ebeecdd07b771c5a8332cf66b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 10 May 2026 23:18:48 -0500 Subject: [PATCH] [cli] Address review feedback on --prebuilt-dir - upload_using_ltchiptool: rename unused config arg to _config and note the signature parity with upload_using_platformio/upload_using_picotool so dispatch stays symmetric. - upload_program: explicit exit_code = 1 when _rp2040_serial_reset_to_bootsel fails, instead of relying on the function-level default. - upload_using_picotool: distinct error message for the prebuilt-dir case that names both candidates (idedata ELF + prebuilt firmware) instead of just pointing at the missing ELF. - _load_idedata: document the prebuilt-dir idedata.json contract more loudly (absolute paths under prebuilt_dir, no schema validation, dashboard owns the rewrites). - --prebuilt-dir help text: drop the placeholder docs URL; describe the dashboard-internal intent inline so users who hit the flag in --help understand they don't want it. - New test: upload_using_esptool with ESP-IDF toolchain + --prebuilt-dir uses /firmware.factory.bin at offset 0x0. Issue: esphome/device-builder#572 --- esphome/__main__.py | 31 ++++++++++++++++++++++++------- esphome/platformio_api.py | 11 ++++++++--- tests/unit_tests/test_main.py | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/esphome/__main__.py b/esphome/__main__.py index 7398d1ab79..1f4d379bca 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -966,7 +966,7 @@ def upload_using_esptool( return run_esptool(115200) -def upload_using_ltchiptool(config: ConfigType, port: str) -> int: +def upload_using_ltchiptool(_config: ConfigType, port: str) -> int: """Upload a libretiny ``.uf2`` directly via ``ltchiptool flash write``. Bypasses PlatformIO so the dashboard's transparent install can flash a @@ -978,6 +978,10 @@ def upload_using_ltchiptool(config: ConfigType, port: str) -> int: Chip family is auto-detected from the UF2 header so we don't need to plumb through ``CORE.data[KEY_LIBRETINY][KEY_FAMILY]``. + + The ``_config`` parameter is unused but kept for signature parity with + ``upload_using_platformio`` / ``upload_using_picotool`` so the dispatch + in ``upload_program`` can stay symmetric. """ firmware = CORE.firmware_bin if not firmware.is_file(): @@ -1060,6 +1064,14 @@ def upload_using_picotool(config: ConfigType) -> int: firmware_file = elf_path elif CORE.prebuilt_dir is not None and CORE.firmware_bin.is_file(): firmware_file = CORE.firmware_bin + elif CORE.prebuilt_dir is not None: + _LOGGER.error( + "No firmware found for picotool: neither %s (from idedata) nor " + "%s (prebuilt) exists.", + elf_path, + CORE.firmware_bin, + ) + return 1 else: _LOGGER.error( "Firmware ELF file not found at %s. " @@ -1290,6 +1302,12 @@ def upload_program( # PlatformIO path is preserved for back-compat. if _rp2040_serial_reset_to_bootsel(host): exit_code = upload_using_picotool(config) + else: + # Touch reset failed: the helper already logged a specific + # error (port open failure, picotool missing, or BOOTSEL + # never enumerated). Be explicit about the failed exit code + # rather than relying on the function-level default of 1. + exit_code = 1 elif CORE.target_platform == PLATFORM_RP2040 or CORE.is_libretiny: exit_code = upload_using_platformio(config, host) # else: Unknown target platform, exit_code remains 1 @@ -2260,12 +2278,11 @@ def parse_args(argv): parser_upload.add_argument( "--prebuilt-dir", help=( - "Directory of prebuilt artifacts to flash instead of re-deriving " - "paths from the local build tree. The configuration is still read " - "(to load OTA settings, target platform, esp32 variant, etc.) but " - "the upload path reads bytes from this directory. Layout is " - "documented at " - "https://developers.esphome.io/architecture/upload-prebuilt-dir/." + "Advanced: directory of prebuilt artifacts to flash instead of " + "re-deriving paths from the local build tree. Intended for the " + "ESPHome dashboard's transparent install flow on hosts that " + "don't compile firmware themselves. End users should not need " + "this flag." ), ) parser_upload.add_argument( diff --git a/esphome/platformio_api.py b/esphome/platformio_api.py index 86c7cdfd4f..33941b8f65 100644 --- a/esphome/platformio_api.py +++ b/esphome/platformio_api.py @@ -104,9 +104,14 @@ def _run_idedata(config): def _load_idedata(config): # `esphome upload --prebuilt-dir` ships a pre-rendered idedata.json next # to the artifacts. When present we use it verbatim: ``firmware_bin_path`` - # and ``extra.flash_images[*].path`` already point at absolute paths under - # the prebuilt directory, so the esptool / picotool helpers find the right - # bytes without re-running PlatformIO or consulting the local build tree. + # (i.e. ``prog_path`` with the ``.bin`` suffix), ``firmware_elf_path``, + # ``cc_path`` and ``extra.flash_images[*].path`` are expected to be + # absolute paths that resolve under ``CORE.prebuilt_dir``. The dashboard + # is responsible for rewriting those paths when it stages the directory. + # No schema validation or referenced-path existence check happens here; + # a malformed prebuilt idedata.json will surface as a downstream + # "file not found" from esptool / picotool. That's an acceptable trade + # for keeping this path zero-cost when the dashboard's contract is met. if CORE.prebuilt_dir is not None: prebuilt_idedata = CORE.prebuilt_dir / "idedata.json" if prebuilt_idedata.is_file(): diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index cc732c95f5..4dde900827 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -1343,6 +1343,41 @@ def test_upload_using_esptool_with_file_path( assert firmware_path.endswith("custom_firmware.bin") +def test_upload_using_esptool_idf_prebuilt_factory_bin( + tmp_path: Path, + mock_run_external_command_main: Mock, +) -> None: + """ESP-IDF builds flash a single ``firmware.factory.bin`` (bootloader + + partitions + app bundled) at offset 0x0. With --prebuilt-dir set the + esptool helper must pick up ``/firmware.factory.bin`` + instead of ``CORE.relative_build_path("build", "firmware.factory.bin")`` + so the dashboard can flash an ESP-IDF device without a local build.""" + setup_core(platform=PLATFORM_ESP32, tmp_path=tmp_path, name="test") + CORE.toolchain = Toolchain.ESP_IDF + CORE.data[KEY_ESP32] = {KEY_VARIANT: VARIANT_ESP32} + + prebuilt = tmp_path / "prebuilt" + prebuilt.mkdir() + factory_bin = prebuilt / "firmware.factory.bin" + factory_bin.write_bytes(b"\x00" * 1024) + CORE.prebuilt_dir = prebuilt + + config = {CONF_ESPHOME: {"platformio_options": {}}} + + result = upload_using_esptool(config, "/dev/ttyUSB0", None, None) + + assert result == 0 + cmd_list = list(mock_run_external_command_main.call_args[0][1:]) + + # ESP-IDF prebuilt: single image at offset 0x0 pointing at the prebuilt + # factory file, not the local build tree's /firmware.factory.bin. + write_flash_idx = cmd_list.index("write-flash") + offset_idx = write_flash_idx + 4 + assert cmd_list[offset_idx] == "0x0" + path_arg = cmd_list[offset_idx + 1] + assert path_arg == str(factory_bin) + + @pytest.mark.parametrize( "platform,device", [