diff --git a/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp b/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp index a82ee48718..ebcdd5f36e 100644 --- a/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp +++ b/esphome/components/esp32_hosted/update/esp32_hosted_update.cpp @@ -11,6 +11,7 @@ #include #ifdef USE_ESP32_HOSTED_HTTP_UPDATE +#include "esphome/components/http_request/http_request.h" #include "esphome/components/json/json_util.h" #include "esphome/components/network/util.h" #endif @@ -184,15 +185,23 @@ bool Esp32HostedUpdate::fetch_manifest_() { } // Read manifest JSON into string (manifest is small, ~1KB max) + // NOTE: HttpContainer::read() has non-BSD socket semantics - see http_request.h + // Use http_read_loop_result() helper instead of checking return values directly std::string json_str; json_str.reserve(container->content_length); uint8_t buf[256]; + uint32_t last_data_time = millis(); + const uint32_t read_timeout = this->http_request_parent_->get_timeout(); while (container->get_bytes_read() < container->content_length) { - int read = container->read(buf, sizeof(buf)); - if (read > 0) { - json_str.append(reinterpret_cast(buf), read); - } + int read_or_error = container->read(buf, sizeof(buf)); + App.feed_wdt(); yield(); + auto result = http_request::http_read_loop_result(read_or_error, last_data_time, read_timeout); + if (result == http_request::HttpReadLoopResult::RETRY) + continue; + if (result != http_request::HttpReadLoopResult::DATA) + break; // ERROR or TIMEOUT + json_str.append(reinterpret_cast(buf), read_or_error); } container->end(); @@ -297,32 +306,38 @@ bool Esp32HostedUpdate::stream_firmware_to_coprocessor_() { } // Stream firmware to coprocessor while computing SHA256 + // NOTE: HttpContainer::read() has non-BSD socket semantics - see http_request.h + // Use http_read_loop_result() helper instead of checking return values directly sha256::SHA256 hasher; hasher.init(); uint8_t buffer[CHUNK_SIZE]; + uint32_t last_data_time = millis(); + const uint32_t read_timeout = this->http_request_parent_->get_timeout(); while (container->get_bytes_read() < total_size) { - int read = container->read(buffer, sizeof(buffer)); + int read_or_error = container->read(buffer, sizeof(buffer)); // Feed watchdog and give other tasks a chance to run App.feed_wdt(); yield(); - // Exit loop if no data available (stream closed or end of data) - if (read <= 0) { - if (read < 0) { - ESP_LOGE(TAG, "Stream closed with error"); - esp_hosted_slave_ota_end(); // NOLINT - container->end(); - this->status_set_error(LOG_STR("Download failed")); - return false; + auto result = http_request::http_read_loop_result(read_or_error, last_data_time, read_timeout); + if (result == http_request::HttpReadLoopResult::RETRY) + continue; + if (result != http_request::HttpReadLoopResult::DATA) { + if (result == http_request::HttpReadLoopResult::TIMEOUT) { + ESP_LOGE(TAG, "Timeout reading firmware data"); + } else { + ESP_LOGE(TAG, "Error reading firmware data: %d", read_or_error); } - // read == 0: no more data available, exit loop - break; + esp_hosted_slave_ota_end(); // NOLINT + container->end(); + this->status_set_error(LOG_STR("Download failed")); + return false; } - hasher.add(buffer, read); - err = esp_hosted_slave_ota_write(buffer, read); // NOLINT + hasher.add(buffer, read_or_error); + err = esp_hosted_slave_ota_write(buffer, read_or_error); // NOLINT if (err != ESP_OK) { ESP_LOGE(TAG, "Failed to write OTA data: %s", esp_err_to_name(err)); esp_hosted_slave_ota_end(); // NOLINT diff --git a/esphome/components/http_request/http_request.h b/esphome/components/http_request/http_request.h index a8c2cdfc63..fb39ca504c 100644 --- a/esphome/components/http_request/http_request.h +++ b/esphome/components/http_request/http_request.h @@ -79,6 +79,81 @@ inline bool is_redirect(int const status) { */ inline bool is_success(int const status) { return status >= HTTP_STATUS_OK && status < HTTP_STATUS_MULTIPLE_CHOICES; } +/* + * HTTP Container Read Semantics + * ============================= + * + * IMPORTANT: These semantics differ from standard BSD sockets! + * + * BSD socket read() returns: + * > 0: bytes read + * == 0: connection closed (EOF) + * < 0: error (check errno) + * + * HttpContainer::read() returns: + * > 0: bytes read successfully + * == 0: no data available yet OR all content read + * (caller should check bytes_read vs content_length) + * < 0: error or connection closed (caller should EXIT) + * HTTP_ERROR_CONNECTION_CLOSED (-1) = connection closed prematurely + * other negative values = platform-specific errors + * + * Platform behaviors: + * - ESP-IDF: blocking reads, 0 only returned when all content read + * - Arduino: non-blocking, 0 means "no data yet" or "all content read" + * + * Use the helper functions below instead of checking return values directly: + * - http_read_loop_result(): for manual loops with per-chunk processing + * - http_read_fully(): for simple "read N bytes into buffer" operations + */ + +/// Error code returned by HttpContainer::read() when connection closed prematurely +/// NOTE: Unlike BSD sockets where 0 means EOF, here 0 means "no data yet, retry" +static constexpr int HTTP_ERROR_CONNECTION_CLOSED = -1; + +/// Status of a read operation +enum class HttpReadStatus : uint8_t { + OK, ///< Read completed successfully + ERROR, ///< Read error occurred + TIMEOUT, ///< Timeout waiting for data +}; + +/// Result of an HTTP read operation +struct HttpReadResult { + HttpReadStatus status; ///< Status of the read operation + int error_code; ///< Error code from read() on failure, 0 on success +}; + +/// Result of processing a non-blocking read with timeout (for manual loops) +enum class HttpReadLoopResult : uint8_t { + DATA, ///< Data was read, process it + RETRY, ///< No data yet, already delayed, caller should continue loop + ERROR, ///< Read error, caller should exit loop + TIMEOUT, ///< Timeout waiting for data, caller should exit loop +}; + +/// Process a read result with timeout tracking and delay handling +/// @param bytes_read_or_error Return value from read() - positive for bytes read, negative for error +/// @param last_data_time Time of last successful read, updated when data received +/// @param timeout_ms Maximum time to wait for data +/// @return DATA if data received, RETRY if should continue loop, ERROR/TIMEOUT if should exit +inline HttpReadLoopResult http_read_loop_result(int bytes_read_or_error, uint32_t &last_data_time, + uint32_t timeout_ms) { + if (bytes_read_or_error > 0) { + last_data_time = millis(); + return HttpReadLoopResult::DATA; + } + if (bytes_read_or_error < 0) { + return HttpReadLoopResult::ERROR; + } + // bytes_read_or_error == 0: no data available yet + if (millis() - last_data_time >= timeout_ms) { + return HttpReadLoopResult::TIMEOUT; + } + delay(1); // Small delay to prevent tight spinning + return HttpReadLoopResult::RETRY; +} + class HttpRequestComponent; class HttpContainer : public Parented { @@ -88,6 +163,33 @@ class HttpContainer : public Parented { int status_code; uint32_t duration_ms; + /** + * @brief Read data from the HTTP response body. + * + * WARNING: These semantics differ from BSD sockets! + * BSD sockets: 0 = EOF (connection closed) + * This method: 0 = no data yet OR all content read, negative = error/closed + * + * @param buf Buffer to read data into + * @param max_len Maximum number of bytes to read + * @return + * - > 0: Number of bytes read successfully + * - 0: No data available yet OR all content read + * (check get_bytes_read() >= content_length to distinguish) + * - HTTP_ERROR_CONNECTION_CLOSED (-1): Connection closed prematurely + * - < -1: Other error (platform-specific error code) + * + * Platform notes: + * - ESP-IDF: blocking read, 0 only when all content read + * - Arduino: non-blocking, 0 can mean "no data yet" or "all content read" + * + * Use get_bytes_read() and content_length to track progress. + * When get_bytes_read() >= content_length, all data has been received. + * + * IMPORTANT: Do not use raw return values directly. Use these helpers: + * - http_read_loop_result(): for loops with per-chunk processing + * - http_read_fully(): for simple "read N bytes" operations + */ virtual int read(uint8_t *buf, size_t max_len) = 0; virtual void end() = 0; @@ -110,6 +212,38 @@ class HttpContainer : public Parented { std::map> response_headers_{}; }; +/// Read data from HTTP container into buffer with timeout handling +/// Handles feed_wdt, yield, and timeout checking internally +/// @param container The HTTP container to read from +/// @param buffer Buffer to read into +/// @param total_size Total bytes to read +/// @param chunk_size Maximum bytes per read call +/// @param timeout_ms Read timeout in milliseconds +/// @return HttpReadResult with status and error_code on failure +inline HttpReadResult http_read_fully(HttpContainer *container, uint8_t *buffer, size_t total_size, size_t chunk_size, + uint32_t timeout_ms) { + size_t read_index = 0; + uint32_t last_data_time = millis(); + + while (read_index < total_size) { + int read_bytes_or_error = container->read(buffer + read_index, std::min(chunk_size, total_size - read_index)); + + App.feed_wdt(); + yield(); + + auto result = http_read_loop_result(read_bytes_or_error, last_data_time, timeout_ms); + if (result == HttpReadLoopResult::RETRY) + continue; + if (result == HttpReadLoopResult::ERROR) + return {HttpReadStatus::ERROR, read_bytes_or_error}; + if (result == HttpReadLoopResult::TIMEOUT) + return {HttpReadStatus::TIMEOUT, 0}; + + read_index += read_bytes_or_error; + } + return {HttpReadStatus::OK, 0}; +} + class HttpRequestResponseTrigger : public Trigger, std::string &> { public: void process(const std::shared_ptr &container, std::string &response_body) { @@ -124,6 +258,7 @@ class HttpRequestComponent : public Component { void set_useragent(const char *useragent) { this->useragent_ = useragent; } void set_timeout(uint32_t timeout) { this->timeout_ = timeout; } + uint32_t get_timeout() const { return this->timeout_; } void set_watchdog_timeout(uint32_t watchdog_timeout) { this->watchdog_timeout_ = watchdog_timeout; } uint32_t get_watchdog_timeout() const { return this->watchdog_timeout_; } void set_follow_redirects(bool follow_redirects) { this->follow_redirects_ = follow_redirects; } @@ -249,15 +384,21 @@ template class HttpRequestSendAction : public Action { RAMAllocator allocator; uint8_t *buf = allocator.allocate(max_length); if (buf != nullptr) { + // NOTE: HttpContainer::read() has non-BSD socket semantics - see top of this file + // Use http_read_loop_result() helper instead of checking return values directly size_t read_index = 0; + uint32_t last_data_time = millis(); + const uint32_t read_timeout = this->parent_->get_timeout(); while (container->get_bytes_read() < max_length) { - int read = container->read(buf + read_index, std::min(max_length - read_index, 512)); - if (read <= 0) { - break; - } + int read_or_error = container->read(buf + read_index, std::min(max_length - read_index, 512)); App.feed_wdt(); yield(); - read_index += read; + auto result = http_read_loop_result(read_or_error, last_data_time, read_timeout); + if (result == HttpReadLoopResult::RETRY) + continue; + if (result != HttpReadLoopResult::DATA) + break; // ERROR or TIMEOUT + read_index += read_or_error; } response_body.reserve(read_index); response_body.assign((char *) buf, read_index); diff --git a/esphome/components/http_request/http_request_arduino.cpp b/esphome/components/http_request/http_request_arduino.cpp index a653942b18..8ec4d2bc4b 100644 --- a/esphome/components/http_request/http_request_arduino.cpp +++ b/esphome/components/http_request/http_request_arduino.cpp @@ -139,6 +139,23 @@ std::shared_ptr HttpRequestArduino::perform(const std::string &ur return container; } +// Arduino HTTP read implementation +// +// WARNING: Return values differ from BSD sockets! See http_request.h for full documentation. +// +// Arduino's WiFiClient is inherently non-blocking - available() returns 0 when +// no data is ready. We use connected() to distinguish "no data yet" from +// "connection closed". +// +// WiFiClient behavior: +// available() > 0: data ready to read +// available() == 0 && connected(): no data yet, still connected +// available() == 0 && !connected(): connection closed +// +// We normalize to HttpContainer::read() contract (NOT BSD socket semantics!): +// > 0: bytes read +// 0: no data yet, retry <-- NOTE: 0 means retry, NOT EOF! +// < 0: error/connection closed <-- connection closed returns -1, not 0 int HttpContainerArduino::read(uint8_t *buf, size_t max_len) { const uint32_t start = millis(); watchdog::WatchdogManager wdm(this->parent_->get_watchdog_timeout()); @@ -146,7 +163,7 @@ int HttpContainerArduino::read(uint8_t *buf, size_t max_len) { WiFiClient *stream_ptr = this->client_.getStreamPtr(); if (stream_ptr == nullptr) { ESP_LOGE(TAG, "Stream pointer vanished!"); - return -1; + return HTTP_ERROR_CONNECTION_CLOSED; } int available_data = stream_ptr->available(); @@ -154,7 +171,15 @@ int HttpContainerArduino::read(uint8_t *buf, size_t max_len) { if (bufsize == 0) { this->duration_ms += (millis() - start); - return 0; + // Check if we've read all expected content + if (this->bytes_read_ >= this->content_length) { + return 0; // All content read successfully + } + // No data available - check if connection is still open + if (!stream_ptr->connected()) { + return HTTP_ERROR_CONNECTION_CLOSED; // Connection closed prematurely + } + return 0; // No data yet, caller should retry } App.feed_wdt(); diff --git a/esphome/components/http_request/http_request_idf.cpp b/esphome/components/http_request/http_request_idf.cpp index eedd321d80..b6fb7f7ea9 100644 --- a/esphome/components/http_request/http_request_idf.cpp +++ b/esphome/components/http_request/http_request_idf.cpp @@ -209,26 +209,57 @@ std::shared_ptr HttpRequestIDF::perform(const std::string &url, c return container; } +// ESP-IDF HTTP read implementation (blocking mode) +// +// WARNING: Return values differ from BSD sockets! See http_request.h for full documentation. +// +// esp_http_client_read() in blocking mode returns: +// > 0: bytes read +// 0: connection closed (end of stream) +// < 0: error +// +// We normalize to HttpContainer::read() contract: +// > 0: bytes read +// 0: no data yet / all content read (caller should check bytes_read vs content_length) +// < 0: error/connection closed int HttpContainerIDF::read(uint8_t *buf, size_t max_len) { const uint32_t start = millis(); watchdog::WatchdogManager wdm(this->parent_->get_watchdog_timeout()); - this->feed_wdt(); - int read_len = esp_http_client_read(this->client_, (char *) buf, max_len); - this->feed_wdt(); - if (read_len > 0) { - this->bytes_read_ += read_len; + // Check if we've already read all expected content + if (this->bytes_read_ >= this->content_length) { + return 0; // All content read successfully } + + this->feed_wdt(); + int read_len_or_error = esp_http_client_read(this->client_, (char *) buf, max_len); + this->feed_wdt(); + this->duration_ms += (millis() - start); - return read_len; + if (read_len_or_error > 0) { + this->bytes_read_ += read_len_or_error; + return read_len_or_error; + } + + // Connection closed by server before all content received + if (read_len_or_error == 0) { + return HTTP_ERROR_CONNECTION_CLOSED; + } + + // Negative value - error, return the actual error code for debugging + return read_len_or_error; } void HttpContainerIDF::end() { + if (this->client_ == nullptr) { + return; // Already cleaned up + } watchdog::WatchdogManager wdm(this->parent_->get_watchdog_timeout()); esp_http_client_close(this->client_); esp_http_client_cleanup(this->client_); + this->client_ = nullptr; } void HttpContainerIDF::feed_wdt() { diff --git a/esphome/components/http_request/ota/ota_http_request.cpp b/esphome/components/http_request/ota/ota_http_request.cpp index 2a7db9137f..6c77e75d8c 100644 --- a/esphome/components/http_request/ota/ota_http_request.cpp +++ b/esphome/components/http_request/ota/ota_http_request.cpp @@ -115,39 +115,47 @@ uint8_t OtaHttpRequestComponent::do_ota_() { return error_code; } + // NOTE: HttpContainer::read() has non-BSD socket semantics - see http_request.h + // Use http_read_loop_result() helper instead of checking return values directly + uint32_t last_data_time = millis(); + const uint32_t read_timeout = this->parent_->get_timeout(); + while (container->get_bytes_read() < container->content_length) { - // read a maximum of chunk_size bytes into buf. (real read size returned) - int bufsize = container->read(buf, OtaHttpRequestComponent::HTTP_RECV_BUFFER); - ESP_LOGVV(TAG, "bytes_read_ = %u, body_length_ = %u, bufsize = %i", container->get_bytes_read(), - container->content_length, bufsize); + // read a maximum of chunk_size bytes into buf. (real read size returned, or negative error code) + int bufsize_or_error = container->read(buf, OtaHttpRequestComponent::HTTP_RECV_BUFFER); + ESP_LOGVV(TAG, "bytes_read_ = %u, body_length_ = %u, bufsize_or_error = %i", container->get_bytes_read(), + container->content_length, bufsize_or_error); // feed watchdog and give other tasks a chance to run App.feed_wdt(); yield(); - // Exit loop if no data available (stream closed or end of data) - if (bufsize <= 0) { - if (bufsize < 0) { - ESP_LOGE(TAG, "Stream closed with error"); - this->cleanup_(std::move(backend), container); - return OTA_CONNECTION_ERROR; + auto result = http_read_loop_result(bufsize_or_error, last_data_time, read_timeout); + if (result == HttpReadLoopResult::RETRY) + continue; + if (result != HttpReadLoopResult::DATA) { + if (result == HttpReadLoopResult::TIMEOUT) { + ESP_LOGE(TAG, "Timeout reading data"); + } else { + ESP_LOGE(TAG, "Error reading data: %d", bufsize_or_error); } - // bufsize == 0: no more data available, exit loop - break; + this->cleanup_(std::move(backend), container); + return OTA_CONNECTION_ERROR; } - if (bufsize <= OtaHttpRequestComponent::HTTP_RECV_BUFFER) { + // At this point bufsize_or_error > 0, so it's a valid size + if (bufsize_or_error <= OtaHttpRequestComponent::HTTP_RECV_BUFFER) { // add read bytes to MD5 - md5_receive.add(buf, bufsize); + md5_receive.add(buf, bufsize_or_error); // write bytes to OTA backend this->update_started_ = true; - error_code = backend->write(buf, bufsize); + error_code = backend->write(buf, bufsize_or_error); if (error_code != ota::OTA_RESPONSE_OK) { // error code explanation available at // https://github.com/esphome/esphome/blob/dev/esphome/components/ota/ota_backend.h ESP_LOGE(TAG, "Error code (%02X) writing binary data to flash at offset %d and size %d", error_code, - container->get_bytes_read() - bufsize, container->content_length); + container->get_bytes_read() - bufsize_or_error, container->content_length); this->cleanup_(std::move(backend), container); return error_code; } @@ -244,19 +252,19 @@ bool OtaHttpRequestComponent::http_get_md5_() { } this->md5_expected_.resize(MD5_SIZE); - int read_len = 0; - while (container->get_bytes_read() < MD5_SIZE) { - read_len = container->read((uint8_t *) this->md5_expected_.data(), MD5_SIZE); - if (read_len <= 0) { - break; - } - App.feed_wdt(); - yield(); - } + auto result = http_read_fully(container.get(), (uint8_t *) this->md5_expected_.data(), MD5_SIZE, MD5_SIZE, + this->parent_->get_timeout()); container->end(); - ESP_LOGV(TAG, "Read len: %u, MD5 expected: %u", read_len, MD5_SIZE); - return read_len == MD5_SIZE; + if (result.status != HttpReadStatus::OK) { + if (result.status == HttpReadStatus::TIMEOUT) { + ESP_LOGE(TAG, "Timeout reading MD5"); + } else { + ESP_LOGE(TAG, "Error reading MD5: %d", result.error_code); + } + return false; + } + return true; } bool OtaHttpRequestComponent::validate_url_(const std::string &url) { diff --git a/esphome/components/http_request/update/http_request_update.cpp b/esphome/components/http_request/update/http_request_update.cpp index 82b391e01f..c63e55d159 100644 --- a/esphome/components/http_request/update/http_request_update.cpp +++ b/esphome/components/http_request/update/http_request_update.cpp @@ -11,7 +11,12 @@ namespace http_request { // The update function runs in a task only on ESP32s. #ifdef USE_ESP32 -#define UPDATE_RETURN vTaskDelete(nullptr) // Delete the current update task +// vTaskDelete doesn't return, but clang-tidy doesn't know that +#define UPDATE_RETURN \ + do { \ + vTaskDelete(nullptr); \ + __builtin_unreachable(); \ + } while (0) #else #define UPDATE_RETURN return #endif @@ -70,19 +75,21 @@ void HttpRequestUpdate::update_task(void *params) { UPDATE_RETURN; } - size_t read_index = 0; - while (container->get_bytes_read() < container->content_length) { - int read_bytes = container->read(data + read_index, MAX_READ_SIZE); - - yield(); - - if (read_bytes <= 0) { - // Network error or connection closed - break to avoid infinite loop - break; + auto read_result = http_read_fully(container.get(), data, container->content_length, MAX_READ_SIZE, + this_update->request_parent_->get_timeout()); + if (read_result.status != HttpReadStatus::OK) { + if (read_result.status == HttpReadStatus::TIMEOUT) { + ESP_LOGE(TAG, "Timeout reading manifest"); + } else { + ESP_LOGE(TAG, "Error reading manifest: %d", read_result.error_code); } - - read_index += read_bytes; + // Defer to main loop to avoid race condition on component_state_ read-modify-write + this_update->defer([this_update]() { this_update->status_set_error(LOG_STR("Failed to read manifest")); }); + allocator.deallocate(data, container->content_length); + container->end(); + UPDATE_RETURN; } + size_t read_index = container->get_bytes_read(); bool valid = false; { // Ensures the response string falls out of scope and deallocates before the task ends