refactor: move HttpDownloader onto esp_http_client (#2075)

Based on the learnings from
https://github.com/crosspoint-reader/crosspoint-reader/pull/2074 , I
wanted to bring the same buffer savings to the rest of our HTTP Client
stack. That being said, HttpDownloader (fonts/OPDS) used the Arduino
HTTPClient.

HttpDownloader was the last consumer of the Arduino HTTPClient +
NetworkClientSecure stack. OtaUpdater already runs on esp_http_client,
so this drops the parallel HTTP/TLS implementation. It also fixes a
class of OPDS/font download failures: HTTPClient's setTimeout is uint16
and truncates, and its short per-read deadline killed slow or chunked
responses (the -11 / incomplete-data errors).

What changed:

- Rewrote fetchUrl/downloadToFile around esp_http_client with a
streaming open() -> fetch_headers() -> read() loop, manual redirect
following, and is_complete_data_received() as the completeness gate.
Body bytes go straight to the sink (OPDS parser stream, std::string, or
file), so nothing buffers the payload.

- HTTPS is now verified against the CA bundle instead of
NetworkClientSecure::setInsecure(). esp-tls is built with
CONFIG_ESP_TLS_INSECURE off, so an unverified handshake can't be set up
anyway; the model is public servers over verified https and local
servers over plain http (transport is chosen from the URL scheme).

** Self-signed https servers are no longer supported, by design. **

- timeout_ms is 60s; esp_http_client's timeout is uint32, so unlike
HTTPClient it doesn't silently truncate.

- HTTP buffers are 4096 (rx) / 1024 (tx). 4096 holds real OPDS server
headers; the GitHub release CDN sends more and logs a non-fatal
truncation warning, but the headers we read (Location, Content-Length)
come first and survive.

- Removed the now-unused UrlUtils::isHttpsUrl and a stale HTTPClient
comment in FontDownloadActivity.

Validated on device: OPDS browse and a 3.4 MB book download over
verified https, GitHub font downloads (crc-checked), redirect handling
matching curl, and slow/erroring servers surfaced correctly.


Did you use AI tools to help write this code? partial

src/activities/settings/FontDownloadActivity.h

@@ -36,9 +36,9 @@ class FontDownloadActivity : public Activity {
   void render(RenderLock&&) override;
   bool preventAutoSleep() override {
     return state_ == LOADING_MANIFEST || state_ == DOWNLOADING ||
-           // This is added because HTTPClient is a synchronous/blocking function,
+           // The download is synchronous and blocks the main loop until it
-           // and blocks the main loop until the download is complete.
+           // completes, so activityManager.preventAutoSleep() is never polled
-           // So `activityManager.preventAutoSleep()` is never called during downloading
+           // during downloading.
            state_ == COMPLETE || state_ == ERROR;
   }
   bool skipLoopDelay() override { return true; }

src/network/HttpDownloader.cpp

@@ -1,204 +1,201 @@
 #include "HttpDownloader.h"
 
-#include <HTTPClient.h>
+#include <Arduino.h>
 #include <Logging.h>
-#include <NetworkClient.h>
+#include <Memory.h>
-#include <NetworkClientSecure.h>
-#include <StreamString.h>
 #include <base64.h>
+#include <esp_crt_bundle.h>
+#include <esp_http_client.h>
 
 #include <cstring>
-#include <memory>
+#include <functional>
-#include <utility>
+#include <string>
-
-#include "util/UrlUtils.h"
 
 namespace {
-class FileWriteStream final : public Stream {
+// RX holds the response headers. 4096 fits real OPDS servers; GitHub's release
- public:
+// CDN sends more and logs HTTP_HEADER "Buffer length is small", but that's
-  FileWriteStream(FsFile& file, size_t total, HttpDownloader::ProgressCallback progress, bool* cancelFlag)
+// non-fatal: the headers we read (Location, Content-Length) come first and
-      : file_(file), total_(total), progress_(std::move(progress)), cancelFlag_(cancelFlag) {}
+// survive. Smaller keeps contiguous heap free while WiFi and TLS are up. TX
+// only carries our GET; the body streams in READ_CHUNK pieces.
+constexpr int HTTP_RX_BUF = 4096;
+constexpr int HTTP_TX_BUF = 1024;
+// Per-socket-op timeout. Some OPDS download endpoints are slow to send headers
+// (>15s) and chunked catalogs stall mid-body, so 15s killed them. 60s gives
+// slow servers room. esp_http_client's timeout_ms is uint32, so unlike Arduino
+// HTTPClient's uint16 setTimeout it doesn't silently truncate.
+constexpr int HTTP_TIMEOUT_MS = 60000;
+constexpr size_t READ_CHUNK = 2048;
 
-  size_t write(uint8_t byte) override { return write(&byte, 1); }
+struct Sink {
+  std::function<bool(const uint8_t*, size_t)> write;  // returns false to abort the transfer
+  HttpDownloader::ProgressCallback progress;
+  bool* cancelFlag = nullptr;
+  size_t total = 0;
+  size_t downloaded = 0;
+};
 
-  size_t write(const uint8_t* buffer, size_t size) override {
+bool isRedirect(int status) {
-    // Write-through stream for HTTPClient::writeToStream with progress tracking.
+  return status == 301 || status == 302 || status == 303 || status == 307 || status == 308;
-    if (cancelFlag_ && *cancelFlag_) {
+}
-      writeOk_ = false;
+
-      return 0;
+// Streams a GET body through sink.write in READ_CHUNK pieces. Uses the manual
-    }
+// open/fetch_headers/read path rather than esp_http_client_perform(): perform()
-    const size_t written = file_.write(buffer, size);
+// pushes the whole body through an event callback and reports a chunked body
-    if (written != size) {
+// that ends early as ESP_ERR_HTTP_INCOMPLETE_DATA, whereas the read loop streams
-      writeOk_ = false;
+// large/slow files and surfaces a short read directly.
-    }
+HttpDownloader::DownloadError runGet(const std::string& url, const std::string& username, const std::string& password,
-    downloaded_ += written;
+                                     Sink& sink) {
-    if (progress_ && total_ > 0) {
+  esp_http_client_config_t config = {};
-      progress_(downloaded_, total_);
+  config.url = url.c_str();
-    }
+  config.buffer_size = HTTP_RX_BUF;
-    return written;
+  config.buffer_size_tx = HTTP_TX_BUF;
+  config.timeout_ms = HTTP_TIMEOUT_MS;
+  // Verify HTTPS against the bundled CA roots. This build has esp-tls
+  // CONFIG_ESP_TLS_INSECURE off, so an unverified TLS handshake can't be set
+  // up at all; the model is public servers over verified https and local
+  // servers over plain http (esp_http_client picks the transport from the URL
+  // scheme, so http:// needs no cert config). The prior setInsecure() worked
+  // only because Arduino's ssl_client drives mbedtls directly.
+  config.crt_bundle_attach = esp_crt_bundle_attach;
+  config.keep_alive_enable = true;
+
+  esp_http_client_handle_t client = esp_http_client_init(&config);
+  if (!client) {
+    LOG_ERR("HTTP", "client init failed");
+    return HttpDownloader::HTTP_ERROR;
   }
 
-  int available() override { return 0; }
+  esp_http_client_set_header(client, "User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION);
-  int read() override { return -1; }
+  if (!username.empty() && !password.empty()) {
-  int peek() override { return -1; }
+    // Preemptive Basic auth, like the prior addHeader; don't wait for a 401.
-  void flush() override { file_.flush(); }
+    const std::string credentials = username + ":" + password;
+    const String header = "Basic " + base64::encode(credentials.c_str());
+    esp_http_client_set_header(client, "Authorization", header.c_str());
+  }
 
-  size_t downloaded() const { return downloaded_; }
+  // open()/read() does not auto-follow redirects (only perform() does), so step
-  bool ok() const { return writeOk_; }
+  // 30x responses manually. OPDS download endpoints and the GitHub release CDN
+  // both redirect.
+  esp_err_t err = esp_http_client_open(client, 0);
+  if (err != ESP_OK) {
+    LOG_ERR("HTTP", "open failed: %s", esp_err_to_name(err));
+    esp_http_client_cleanup(client);
+    return HttpDownloader::HTTP_ERROR;
+  }
+  int64_t contentLength = esp_http_client_fetch_headers(client);
+  int status = esp_http_client_get_status_code(client);
+  for (int hop = 0; isRedirect(status) && hop < 5; ++hop) {
+    if (esp_http_client_set_redirection(client) != ESP_OK) break;
+    err = esp_http_client_open(client, 0);
+    if (err != ESP_OK) {
+      LOG_ERR("HTTP", "redirect open failed: %s", esp_err_to_name(err));
+      esp_http_client_cleanup(client);
+      return HttpDownloader::HTTP_ERROR;
+    }
+    contentLength = esp_http_client_fetch_headers(client);
+    status = esp_http_client_get_status_code(client);
+  }
 
- private:
+  if (status != 200) {
-  FsFile& file_;
+    LOG_ERR("HTTP", "unexpected status: %d", status);
-  size_t total_;
+    esp_http_client_cleanup(client);
-  size_t downloaded_ = 0;
+    return HttpDownloader::HTTP_ERROR;
-  bool writeOk_ = true;
+  }
-  HttpDownloader::ProgressCallback progress_;
+
-  bool* cancelFlag_;
+  // fetch_headers returns 0 for a chunked response (no Content-Length); leave
-};
+  // total at 0 so progress stays silent and the size check is skipped.
+  sink.total = contentLength > 0 ? static_cast<size_t>(contentLength) : 0;
+
+  auto buf = makeUniqueNoThrow<char[]>(READ_CHUNK);
+  if (!buf) {
+    LOG_ERR("HTTP", "OOM: %u byte read buffer", (unsigned)READ_CHUNK);
+    esp_http_client_cleanup(client);
+    return HttpDownloader::HTTP_ERROR;
+  }
+
+  while (true) {
+    if (sink.cancelFlag && *sink.cancelFlag) {
+      esp_http_client_cleanup(client);
+      return HttpDownloader::ABORTED;
+    }
+    const int read = esp_http_client_read(client, buf.get(), READ_CHUNK);
+    if (read < 0) {
+      LOG_ERR("HTTP", "read error after %zu bytes", sink.downloaded);
+      esp_http_client_cleanup(client);
+      return HttpDownloader::HTTP_ERROR;
+    }
+    if (read == 0) break;  // all data received
+    if (!sink.write(reinterpret_cast<const uint8_t*>(buf.get()), read)) {
+      esp_http_client_cleanup(client);
+      return HttpDownloader::FILE_ERROR;
+    }
+    sink.downloaded += read;
+    if (sink.progress && sink.total > 0) sink.progress(sink.downloaded, sink.total);
+  }
+
+  const bool complete = esp_http_client_is_complete_data_received(client);
+  esp_http_client_cleanup(client);
+  if (!complete) {
+    LOG_ERR("HTTP", "incomplete: got %zu of %zu bytes", sink.downloaded, sink.total);
+    return HttpDownloader::HTTP_ERROR;
+  }
+  return HttpDownloader::OK;
+}
 }  // namespace
 
 bool HttpDownloader::fetchUrl(const std::string& url, Stream& outContent, const std::string& username,
                               const std::string& password) {
-  std::unique_ptr<NetworkClient> client;
-  if (UrlUtils::isHttpsUrl(url)) {
-    auto* secureClient = new NetworkClientSecure();
-    secureClient->setInsecure();
-    client.reset(secureClient);
-  } else {
-    client.reset(new NetworkClient());
-  }
-  HTTPClient http;
-
   LOG_DBG("HTTP", "Fetching: %s", url.c_str());
-
+  Sink sink;
-  http.begin(*client, url.c_str());
+  sink.write = [&outContent](const uint8_t* data, size_t len) { return outContent.write(data, len) == len; };
-  http.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS);
+  return runGet(url, username, password, sink) == OK;
-  http.addHeader("User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION);
-
-  if (!username.empty() && !password.empty()) {
-    std::string credentials = username + ":" + password;
-    String encoded = base64::encode(credentials.c_str());
-    http.addHeader("Authorization", "Basic " + encoded);
-  }
-
-  const int httpCode = http.GET();
-  if (httpCode != HTTP_CODE_OK) {
-    LOG_ERR("HTTP", "Fetch failed: %d", httpCode);
-    http.end();
-    return false;
-  }
-
-  http.writeToStream(&outContent);
-
-  http.end();
-
-  LOG_DBG("HTTP", "Fetch success");
-  return true;
 }
 
 bool HttpDownloader::fetchUrl(const std::string& url, std::string& outContent, const std::string& username,
                               const std::string& password) {
-  StreamString stream;
+  LOG_DBG("HTTP", "Fetching: %s", url.c_str());
-  if (!fetchUrl(url, stream, username, password)) {
+  outContent.clear();  // start clean; the sink appends, so don't carry prior content
-    return false;
+  Sink sink;
-  }
+  sink.write = [&outContent](const uint8_t* data, size_t len) {
-  outContent = stream.c_str();
+    outContent.append(reinterpret_cast<const char*>(data), len);
-  return true;
+    return true;
+  };
+  return runGet(url, username, password, sink) == OK;
 }
 
 HttpDownloader::DownloadError HttpDownloader::downloadToFile(const std::string& url, const std::string& destPath,
                                                              ProgressCallback progress, bool* cancelFlag,
                                                              const std::string& username, const std::string& password) {
-  std::unique_ptr<NetworkClient> client;
+  LOG_DBG("HTTP", "Downloading: %s -> %s", url.c_str(), destPath.c_str());
-  if (UrlUtils::isHttpsUrl(url)) {
-    auto* secureClient = new NetworkClientSecure();
-    secureClient->setInsecure();
-    client.reset(secureClient);
-  } else {
-    client.reset(new NetworkClient());
-  }
-  HTTPClient http;
 
-  LOG_DBG("HTTP", "Downloading: %s", url.c_str());
-  LOG_DBG("HTTP", "Destination: %s", destPath.c_str());
-
-  http.begin(*client, url.c_str());
-  http.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS);
-  http.addHeader("User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION);
-
-  if (!username.empty() && !password.empty()) {
-    std::string credentials = username + ":" + password;
-    String encoded = base64::encode(credentials.c_str());
-    http.addHeader("Authorization", "Basic " + encoded);
-  }
-
-  const int httpCode = http.GET();
-  if (httpCode != HTTP_CODE_OK) {
-    LOG_ERR("HTTP", "Download failed: %d", httpCode);
-    http.end();
-    return HTTP_ERROR;
-  }
-
-  const int64_t reportedLength = http.getSize();
-  const size_t contentLength = reportedLength > 0 ? static_cast<size_t>(reportedLength) : 0;
-  if (contentLength > 0) {
-    LOG_DBG("HTTP", "Content-Length: %zu", contentLength);
-  } else {
-    LOG_DBG("HTTP", "Content-Length: unknown");
-  }
-
-  // Remove existing file if present
   if (Storage.exists(destPath.c_str())) {
     Storage.remove(destPath.c_str());
   }
-
-  // Open file for writing
   FsFile file;
   if (!Storage.openFileForWrite("HTTP", destPath.c_str(), file)) {
     LOG_ERR("HTTP", "Failed to open file for writing");
-    http.end();
     return FILE_ERROR;
   }
 
-  // Let HTTPClient handle chunked decoding and stream body bytes into the file.
+  Sink sink;
-  FileWriteStream fileStream(file, contentLength, progress, cancelFlag);
+  sink.progress = std::move(progress);
-  const int writeResult = http.writeToStream(&fileStream);
+  sink.cancelFlag = cancelFlag;
+  sink.write = [&file](const uint8_t* data, size_t len) { return file.write(data, len) == len; };
 
+  const DownloadError result = runGet(url, username, password, sink);
+  // Close before any remove() on the same path; DESTRUCTOR_CLOSES_FILE would
+  // otherwise close only after the remove.
   file.close();
-  http.end();
 
-  if (cancelFlag && *cancelFlag) {
+  if (result != OK) {
     Storage.remove(destPath.c_str());
-    return ABORTED;
+    return result;
   }
-
+  if (sink.downloaded == 0) {
-  if (writeResult < 0) {
+    LOG_ERR("HTTP", "no data received");
-    LOG_ERR("HTTP", "writeToStream error: %d", writeResult);
     Storage.remove(destPath.c_str());
     return HTTP_ERROR;
   }
-
+  LOG_DBG("HTTP", "Downloaded %zu bytes", sink.downloaded);
-  const size_t downloaded = fileStream.downloaded();
-  LOG_DBG("HTTP", "Downloaded %zu bytes", downloaded);
-
-  // Guard against partial writes even if HTTPClient completes.
-  if (!fileStream.ok()) {
-    LOG_ERR("HTTP", "Write failed during download");
-    Storage.remove(destPath.c_str());
-    return FILE_ERROR;
-  }
-
-  if (contentLength == 0 && downloaded == 0) {
-    LOG_ERR("HTTP", "Download failed: no data received");
-    Storage.remove(destPath.c_str());
-    return HTTP_ERROR;
-  }
-
-  // Verify download size if known
-  if (contentLength > 0 && downloaded != contentLength) {
-    LOG_ERR("HTTP", "Size mismatch: got %zu, expected %zu", downloaded, contentLength);
-    Storage.remove(destPath.c_str());
-    return HTTP_ERROR;
-  }
-
   return OK;
 }

src/network/HttpDownloader.h

@@ -5,8 +5,9 @@
 #include <string>
 
 /**
- * HTTP client utility for fetching content and downloading files.
+ * HTTP client utility for fetching content and downloading files. Built on
- * Wraps NetworkClientSecure and HTTPClient for HTTPS requests.
+ * esp_http_client: https is verified against the CA bundle, plain http is
+ * used for local servers (transport is chosen from the URL scheme).
  */
 class HttpDownloader {
  public:

src/util/UrlUtils.cpp

@@ -2,8 +2,6 @@
 
 namespace UrlUtils {
 
-bool isHttpsUrl(const std::string& url) { return url.rfind("https://", 0) == 0; }
-
 std::string ensureProtocol(const std::string& url) {
   if (url.find("://") == std::string::npos) {
     return "http://" + url;

src/util/UrlUtils.h

@@ -3,11 +3,6 @@
 
 namespace UrlUtils {
 
-/**
- * Check if URL uses HTTPS protocol
- */
-bool isHttpsUrl(const std::string& url);
-
 /**
  * Prepend http:// if no protocol specified (server will redirect to https if needed)
  */