From bf1c0228a65a09cc3b1a299d0d778b650ba7603a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 29 Apr 2026 13:22:53 -0500 Subject: [PATCH] copilot comments --- esphome/__main__.py | 9 +++++++ .../components/esphome/ota/ota_esphome.cpp | 8 +++++- .../components/ota/ota_backend_esp_idf.cpp | 25 ++++++++++++++----- esphome/core/__init__.py | 8 +++++- esphome/espota2.py | 8 +++++- 5 files changed, 49 insertions(+), 9 deletions(-) diff --git a/esphome/__main__.py b/esphome/__main__.py index c931eb179d9..deb4dbb63fa 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -1124,6 +1124,15 @@ def upload_program( binary = CORE.firmware_bin ota_type = espota2.OTA_TYPE_UPDATE_APP if getattr(args, "partition_table", False): + # Fail fast if the resolved ESPHome OTA config does not enable allow_partition_access. + # The device-side handshake also rejects this with "Device only supports app updates", + # but checking here surfaces the misconfiguration before opening a network connection. + if not ota_conf.get("allow_partition_access"): + raise EsphomeError( + "The option --partition-table requires 'allow_partition_access: true' on the " + "esphome OTA platform in the device's YAML configuration. Add it, recompile, " + "flash a build with the option enabled, and then retry --partition-table." + ) binary = CORE.partition_table_bin ota_type = espota2.OTA_TYPE_UPDATE_PARTITION_TABLE if getattr(args, "file", None) is not None: diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index ac4906780e2..3ae7be7e8f6 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -104,13 +104,19 @@ void ESPHomeOTAComponent::dump_config() { // Avoid running esp_image_verify here: it reads and checksums the entire app image, which is too // expensive for a config dump. The address comes from a cached lookup; the precise used size is // computed lazily by update_partition_table() the first time a partition-table OTA is requested. + // Guard against esp_ota_get_running_partition() returning nullptr (can happen after the partition + // cache has been unloaded) so dump_config never crashes. + // Single ESP_LOGCONFIG call so the lines stay together as one log message; on the (rare) + // nullptr path we surface zeros rather than dereferencing. const esp_partition_t *running_app_part = esp_ota_get_running_partition(); ESP_LOGCONFIG(TAG, " Partition access allowed\n" " Running app:\n" " Partition address: 0x%X\n" " Partition size: 0x%X bytes", - running_app_part->address, running_app_part->size); + running_app_part != nullptr ? running_app_part->address : 0u, + running_app_part != nullptr ? running_app_part->size : 0u); + #ifdef USE_ESP32 ESP_LOGCONFIG(TAG, " Partition table:"); esp_partition_iterator_t it = esp_partition_find(ESP_PARTITION_TYPE_ANY, ESP_PARTITION_SUBTYPE_ANY, nullptr); diff --git a/esphome/components/ota/ota_backend_esp_idf.cpp b/esphome/components/ota/ota_backend_esp_idf.cpp index 2f75b35a8af..fe28cb62884 100644 --- a/esphome/components/ota/ota_backend_esp_idf.cpp +++ b/esphome/components/ota/ota_backend_esp_idf.cpp @@ -25,8 +25,12 @@ std::unique_ptr make_ota_backend() { return make_uniqueota_type_ = ota_type; if (this->ota_type_ == ota::OTA_TYPE_UPDATE_PARTITION_TABLE) { - if (image_size > ESP_PARTITION_TABLE_MAX_LEN) { - ESP_LOGE(TAG, "Wrong partition table size"); + // Partition table images produced by gen_esp32part.py are padded with 0xFF and an MD5 entry to + // exactly ESP_PARTITION_TABLE_MAX_LEN bytes. Reject anything else: an undersized image would + // leave trailing bytes from the previous table in place after the partial write, and an + // oversized image cannot fit in the reserved region. This is stricter than verify alone. + if (image_size != ESP_PARTITION_TABLE_MAX_LEN) { + ESP_LOGE(TAG, "Wrong partition table size: expected %u bytes, got %zu", ESP_PARTITION_TABLE_MAX_LEN, image_size); return OTA_RESPONSE_ERROR_PARTITION_TABLE_VERIFY; } memset(this->buf_, 0xFF, sizeof this->buf_); @@ -207,10 +211,17 @@ OTAResponseTypes IDFOTABackend::update_partition_table() { return OTA_RESPONSE_ERROR_PARTITION_TABLE_VERIFY; } - // Get running app partition and used size + // Get running app partition and used size. A zero size means we couldn't determine the running + // app (e.g., esp_ota_get_running_partition() returned nullptr after a previous aborted partition + // table OTA called esp_partition_unload_all()). Without a valid size we cannot safely compute + // overlap or copy bounds, so fail before any flash operation. uint32_t running_app_offset; size_t running_app_size; get_running_app_position(running_app_offset, running_app_size); + if (running_app_size == 0) { + ESP_LOGE(TAG, "Failed to determine running app position"); + return OTA_RESPONSE_ERROR_PARTITION_TABLE_VERIFY; + } // Get partition table partition esp_err_t err = esp_partition_register_external( @@ -328,9 +339,6 @@ OTAResponseTypes IDFOTABackend::update_partition_table() { " DO NOT REMOVE POWER until the device reboots successfully.\n" " Loss of power during this operation may permanently brick the device."); - // Deinitialize NVS to prevent unwanted flash writes - nvs_flash_deinit(); - // Hold the watchdog open for the entire critical section: optional app copy, partition-table // erase/write, and boot partition selection. None of the steps below should yield long enough // to require a refresh, but bundling them under a single guard avoids spurious resets if the @@ -354,6 +362,11 @@ OTAResponseTypes IDFOTABackend::update_partition_table() { } } + // Deinitialize NVS just before the first destructive write to the partition-table region. Doing + // this here (instead of earlier) means that any failure path in the verify or copy phases above + // returns with NVS still functional, so other components on the device aren't broken until reboot. + nvs_flash_deinit(); + // Update the partition table err = esp_ota_begin(this->partition_table_part_, ESP_PARTITION_TABLE_MAX_LEN, &this->update_handle_); if (err != ESP_OK) { diff --git a/esphome/core/__init__.py b/esphome/core/__init__.py index d97944fd2c7..94a48dd31b0 100644 --- a/esphome/core/__init__.py +++ b/esphome/core/__init__.py @@ -781,7 +781,13 @@ class EsphomeCore: @property def partition_table_bin(self) -> Path: - # native ESP-IDF: self.relative_build_path("build", "partition_table", "partition-table.bin") + # Native ESP-IDF (--native-idf): the partition table image is emitted under + # build/partition_table/partition-table.bin alongside firmware.bin. PlatformIO writes the + # equivalent file as partitions.bin in the env-specific .pioenvs directory. + if self.data.get(KEY_NATIVE_IDF): + return self.relative_build_path( + "build", "partition_table", "partition-table.bin" + ) return self.relative_pioenvs_path(self.name, "partitions.bin") @property diff --git a/esphome/espota2.py b/esphome/espota2.py index 9cab3545141..891ed5b29ca 100644 --- a/esphome/espota2.py +++ b/esphome/espota2.py @@ -296,7 +296,13 @@ def perform_ota( else: features = 0 - if ota_type != 0 and not features & SERVER_FEATURE_SUPPORTS_PARTITION_ACCESS: + if ota_type not in (OTA_TYPE_UPDATE_APP, OTA_TYPE_UPDATE_PARTITION_TABLE): + raise OTAError(f"Unsupported OTA type: 0x{ota_type:02X}") + + if ( + ota_type == OTA_TYPE_UPDATE_PARTITION_TABLE + and not features & SERVER_FEATURE_SUPPORTS_PARTITION_ACCESS + ): raise OTAError("Device only supports app updates") if features & SERVER_FEATURE_SUPPORTS_COMPRESSION: