refactor: Reduce string copies and allocations, start using std::ranges

.changes/zero-copy-and-ranges.md

@@ -0,0 +1,17 @@
+---
+kind: fixed
+---
+Cut string copies and allocations across the firmware, and fix the issues found along the way
+
+## Release Note
+Setup portal and WiFi fixes, less memory churn
+
+A WiFi password too long for the setup portal is now rejected, instead of the network being saved as an open one. Network names from a WiFi scan are always read correctly, and the hub does less copying and allocating while it parses, builds and logs text.
+
+- The setup portal reads query and form parameters in place, without a 256-byte limit
+- Account link codes must be letters and digits before they are put into a request URL
+- Network names copied from scan results are always terminated
+- Saved WiFi credentials are looked up by id instead of being copied, and the portal's ready message no longer copies every saved password to list the network names
+- JSON fields that are only parsed or compared are read in place, without a copy
+- Firmware versions are printed and compared without building strings
+- RF transmitter and LED task names fit FreeRTOS's 15-character limit

components/captive_portal/src/captiveportal/ApiHandlers.cpp

@@ -246,8 +246,12 @@ static esp_err_t apiWifiNetworksAdd(httpd_req_t* req)
     }
   }
 
-  bool connect = true;
-  if (auto connectStr = getFormParam(*body, "connect")) {
+  bool connect    = true;
+  auto connectStr = getFormParam(*body, "connect");
+  if (!connectStr && connectStr.error() != ParamError::Missing) {
+    return sendParamError(req, connectStr.error());
+  }
+  if (connectStr) {
     auto parsed = parseBool(*connectStr);
     if (!parsed) {
       return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_PARAM);
@@ -256,7 +260,11 @@ static esp_err_t apiWifiNetworksAdd(httpd_req_t* req)
   }
 
   wifi_auth_mode_t authMode = WIFI_AUTH_MAX;
-  if (auto securityStr = getFormParam(*body, "security")) {
+  auto securityStr          = getFormParam(*body, "security");
+  if (!securityStr && securityStr.error() != ParamError::Missing) {
+    return sendParamError(req, securityStr.error());
+  }
+  if (securityStr) {
     auto sec = Convert::ToUint8(*securityStr);
     if (!sec || *sec > static_cast<uint8_t>(WIFI_AUTH_MAX)) {
       return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_PARAM);
@@ -319,14 +327,23 @@ static esp_err_t apiOtaSettings(httpd_req_t* req)
     return response;
   }
 
-  if (auto channelStr = getQueryParam(req, "channel")) {
+  // Present but undecodable (an encoded NUL) is an error like an unknown value, never a silently skipped setting
+  auto channelStr = getQueryParam(req, "channel");
+  if (!channelStr && channelStr.error() != ParamError::Missing) {
+    return sendParamError(req, channelStr.error());
+  }
+  if (channelStr) {
     channel = TryParseOtaUpdateChannel(*channelStr);
     if (!channel) {
       return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_CHANNEL);
     }
   }
 
-  if (auto intervalStr = getQueryParam(req, "interval")) {
+  auto intervalStr = getQueryParam(req, "interval");
+  if (!intervalStr && intervalStr.error() != ParamError::Missing) {
+    return sendParamError(req, intervalStr.error());
+  }
+  if (intervalStr) {
     // Minutes; 0 would make periodic checks fire on every OTA task wake-up
     interval = Convert::ToUint16(*intervalStr);
     if (!interval || *interval == 0) {

components/captive_portal/src/captiveportal/HttpUtils.cpp

@@ -6,6 +6,7 @@ const char* const TAG = "CaptivePortalHttp";
 
 #include "Convert.h"
 #include "Logging.h"
+#include "StringHelpers.h"
 #include "util/HexUtils.h"
 
 using namespace OpenShock;
@@ -33,7 +34,8 @@ void CaptivePortal::registerHandler(httpd_handle_t server, const char* uri, http
   }
 }
 
-// Fails with ParamError::Invalid for an encoded NUL ("%00"), which would silently truncate the value at every c_str() use.
+// Fails with ParamError::Invalid for a NUL, encoded ("%00") or literal (a raw body can carry one): either would silently
+// truncate the value at every c_str() use, e.g. save a password WiFi then only sees part of.
 [[nodiscard]] static std::expected<std::string, ParamError> urlDecode(std::string_view s)
 {
   std::string out;
@@ -44,6 +46,9 @@ void CaptivePortal::registerHandler(httpd_handle_t server, const char* uri, http
       out += ' ';
       continue;
     }
+    if (c == '\0') {
+      return std::unexpected(ParamError::Invalid);
+    }
     // A '%' without two characters after it is kept as-is.
     if (c == '%' && i + 2 < s.size()) {
       if (auto decoded = HexUtils::TryParseHexPair(s[i + 1], s[i + 2])) {
@@ -60,25 +65,35 @@ void CaptivePortal::registerHandler(httpd_handle_t server, const char* uri, http
   return out;
 }
 
-std::expected<std::string, ParamError> CaptivePortal::getQueryParam(httpd_req_t* req, const char* key)
+// The raw (still URL-encoded) value of `key` in "k1=v1&k2=v2", keys compared case-insensitively. A pair without '='
+// has an empty value.
+[[nodiscard]] static std::optional<std::string_view> findParam(std::string_view params, std::string_view key)
 {
-  size_t qlen = httpd_req_get_url_query_len(req);
-  if (qlen == 0) {
-    return std::unexpected(ParamError::Missing);
-  }
-
-  std::string query;
-  query.resize(qlen + 1);
-  if (httpd_req_get_url_query_str(req, query.data(), query.size()) != ESP_OK) {
-    return std::unexpected(ParamError::Missing);
+  while (!params.empty()) {
+    auto [pair, rest]  = StringSplitByFirst(params, '&');
+    auto [name, value] = StringSplitByFirst(pair, '=');
+    if (StringIEquals(name, key)) {
+      return value;
+    }
+    params = rest;
   }
+  return std::nullopt;
+}
 
-  char val[256];
-  if (httpd_query_key_value(query.c_str(), key, val, sizeof(val)) != ESP_OK) {
+[[nodiscard]] static std::expected<std::string, ParamError> getParam(std::string_view params, std::string_view key)
+{
+  auto raw = findParam(params, key);
+  if (!raw) {
     return std::unexpected(ParamError::Missing);
   }
+  return urlDecode(*raw);
+}
 
-  return urlDecode(val);
+std::expected<std::string, ParamError> CaptivePortal::getQueryParam(httpd_req_t* req, std::string_view key)
+{
+  // req->uri is the request target as received, NUL-terminated and valid for the whole handler
+  auto [path, query] = StringSplitByFirst(std::string_view(req->uri), '?');
+  return getParam(query, key);
 }
 
 std::optional<bool> CaptivePortal::parseBool(std::string_view str)
@@ -92,7 +107,7 @@ std::optional<bool> CaptivePortal::parseBool(std::string_view str)
   return Convert::ToBool(str);
 }
 
-std::expected<bool, ParamError> CaptivePortal::getBoolQueryParam(httpd_req_t* req, const char* key)
+std::expected<bool, ParamError> CaptivePortal::getBoolQueryParam(httpd_req_t* req, std::string_view key)
 {
   auto str = getQueryParam(req, key);
   if (!str) {
@@ -135,11 +150,7 @@ std::optional<std::string> CaptivePortal::recvBody(httpd_req_t* req)
   return out;
 }
 
-std::expected<std::string, ParamError> CaptivePortal::getFormParam(const std::string& body, const char* key)
+std::expected<std::string, ParamError> CaptivePortal::getFormParam(std::string_view body, std::string_view key)
 {
-  char val[256];
-  if (httpd_query_key_value(body.c_str(), key, val, sizeof(val)) != ESP_OK) {
-    return std::unexpected(ParamError::Missing);
-  }
-  return urlDecode(val);
+  return getParam(body, key);
 }

components/captive_portal/src/captiveportal/HttpUtils.h

@@ -29,14 +29,15 @@ namespace OpenShock::CaptivePortal {
   /// @brief Registers a route, logging a failure. With wildcard matching, routes registered earlier take priority.
   void registerHandler(httpd_handle_t server, const char* uri, httpd_method_t method, esp_err_t (*handler)(httpd_req_t*), void* userCtx);
 
-  // Read and URL-decode a query-string parameter. httpd does not decode for us.
-  [[nodiscard]] std::expected<std::string, ParamError> getQueryParam(httpd_req_t* req, const char* key);
-  [[nodiscard]] std::expected<bool, ParamError> getBoolQueryParam(httpd_req_t* req, const char* key);
+  // Read and URL-decode a query-string parameter (key matched case-insensitively, as httpd does). The query is
+  // parsed in place in the request URI: no copy, and no limit on the value's length.
+  [[nodiscard]] std::expected<std::string, ParamError> getQueryParam(httpd_req_t* req, std::string_view key);
+  [[nodiscard]] std::expected<bool, ParamError> getBoolQueryParam(httpd_req_t* req, std::string_view key);
 
   // The web UI sends booleans as "1"/"0"; "true"/"false" are accepted too.
   [[nodiscard]] std::optional<bool> parseBool(std::string_view str);
 
   // Read a urlencoded request body (application/x-www-form-urlencoded).
   [[nodiscard]] std::optional<std::string> recvBody(httpd_req_t* req);
-  [[nodiscard]] std::expected<std::string, ParamError> getFormParam(const std::string& body, const char* key);
+  [[nodiscard]] std::expected<std::string, ParamError> getFormParam(std::string_view body, std::string_view key);
 }  // namespace OpenShock::CaptivePortal

components/captive_portal/src/captiveportal/StaticFileServer.cpp

@@ -12,6 +12,7 @@ const char* const TAG = "CaptivePortalStatic";
 #include "util/HexUtils.h"
 
 #include <cstddef>
+#include <cstdio>
 #include <cstring>
 #include <string>
 #include <string_view>
@@ -98,17 +99,25 @@ static bool acceptsGzip(std::string_view acceptEncoding)
 // The length lookup can't tell the two apart, but the value lookup can.
 static bool clientAcceptsGzip(httpd_req_t* req)
 {
-  std::size_t len = httpd_req_get_hdr_value_len(req, "Accept-Encoding");
-  std::string value;
-  value.resize(len + 1);
-  esp_err_t err = httpd_req_get_hdr_value_str(req, "Accept-Encoding", value.data(), value.size());
+  // Browsers send at most "gzip, deflate, br, zstd" (23 bytes), which fits; a longer value goes to the heap
+  char value[32];
+  esp_err_t err = httpd_req_get_hdr_value_str(req, "Accept-Encoding", value, sizeof(value));
   if (err == ESP_ERR_NOT_FOUND) {
     return true;
   }
-  if (err != ESP_OK) {
+  if (err == ESP_OK) {
+    return acceptsGzip(value);
+  }
+  if (err != ESP_ERR_HTTPD_RESULT_TRUNC) {
+    return false;
+  }
+
+  std::size_t len = httpd_req_get_hdr_value_len(req, "Accept-Encoding");
+  std::string longValue(len + 1, '\0');
+  if (httpd_req_get_hdr_value_str(req, "Accept-Encoding", longValue.data(), longValue.size()) != ESP_OK) {
     return false;
   }
-  return acceptsGzip(std::string_view(value.data(), len));
+  return acceptsGzip(std::string_view(longValue.data(), len));
 }
 
 static constexpr const char* kCacheControl          = "max-age=3600";
@@ -194,8 +203,8 @@ CaptivePortal::StaticFileServer::StaticFileServer()
   }
   char hex[Www::kImageHashSize * 2 + 1];
   HexUtils::ToHex(m_www.image().ImageHash(), hex, false);
-  m_etag = std::string("\"") + hex + "\"";
-  OS_LOGI(TAG, "Serving files from the www image (%s)", m_etag.c_str());
+  snprintf(m_etag, sizeof(m_etag), "\"%s\"", hex);
+  OS_LOGI(TAG, "Serving files from the www image (%s)", m_etag);
 }
 
 void CaptivePortal::StaticFileServer::registerHandler(httpd_handle_t server)
@@ -256,7 +265,7 @@ esp_err_t CaptivePortal::StaticFileServer::staticFileHandler(httpd_req_t* req)
   Representation rep {
     .contentType  = contentTypeForPath(uri),
     .cacheControl = asset->immutable() ? kCacheControlImmutable : kCacheControl,
-    .etag         = self->m_etag.c_str(),
+    .etag         = self->m_etag,
     .size         = asset->body.size(),
   };
 

components/captive_portal/src/captiveportal/StaticFileServer.h

@@ -5,8 +5,6 @@
 
 #include <esp_http_server.h>
 
-#include <string>
-
 namespace OpenShock::CaptivePortal {
   /// @brief Serves the portal UI from the flat www image (mapped, served by pointer), mounted for this object's lifetime.
   class StaticFileServer {
@@ -24,6 +22,6 @@ namespace OpenShock::CaptivePortal {
     static esp_err_t staticFileHandler(httpd_req_t* req);
 
     Www::Partition m_www;
-    std::string m_etag;  // the www image's hash, quoted
+    char m_etag[1 + (Www::kImageHashSize * 2) + 1 + 1] = {};  // the www image's hash, quoted; httpd keeps the pointer
   };
 }  // namespace OpenShock::CaptivePortal

components/captive_portal/src/captiveportal/WebSocketEndpoint.cpp

@@ -223,7 +223,7 @@ void CaptivePortal::WebSocketEndpoint::onWsFrame(int fd, httpd_ws_type_t opcode,
         if (!final) {
           m_clients[idx].reasmActive = true;
           m_clients[idx].reasmType   = type;
-          m_clients[idx].reasm.assign(payload.begin(), payload.end());
+          m_clients[idx].reasm.assign_range(payload);
           break;
         }
       }
@@ -249,7 +249,7 @@ void CaptivePortal::WebSocketEndpoint::onWsFrame(int fd, httpd_ws_type_t opcode,
           httpd_sess_trigger_close(m_server, fd);
           break;
         }
-        m_clients[idx].reasm.insert(m_clients[idx].reasm.end(), payload.begin(), payload.end());
+        m_clients[idx].reasm.append_range(payload);
         if (!final) {
           break;
         }
@@ -334,7 +334,7 @@ bool CaptivePortal::WebSocketEndpoint::queueSend(int fd, bool broadcast, bool bi
       OS_LOGW(TAG, "WebSocket send queue full, dropping message");
       return false;
     }
-    m_sendQueue.push_back(WsSend {.fd = broadcast ? -1 : fd, .binary = binary, .data = std::vector<uint8_t>(data.begin(), data.end())});
+    m_sendQueue.push_back(WsSend {.fd = broadcast ? -1 : fd, .binary = binary, .data = std::vector<uint8_t>(std::from_range, data)});
   }
 
   scheduleDrain();

components/common/host_test/main/test_stringutils.cpp

@@ -73,15 +73,6 @@ TEST_CASE("TryStringSplit into a fixed array", "[common][strhelpers]")
   static_assert(!TryStringSplit<2>("kv", '=').has_value());
 }
 
-TEST_CASE("StringSplit into a vector", "[common][strhelpers]")
-{
-  auto v = StringSplit("a,b,,c", ',');
-  TEST_ASSERT_EQUAL_size_t(4, v.size());
-  TEST_ASSERT_TRUE(v[0] == "a");
-  TEST_ASSERT_TRUE(v[2] == "");  // empty field preserved
-  TEST_ASSERT_TRUE(v[3] == "c");
-}
-
 TEST_CASE("StringIContains / StringHasPrefixIC", "[common][strhelpers]")
 {
   TEST_ASSERT_TRUE(StringIContains("Hello World", "LO WO"));
@@ -112,17 +103,6 @@ TEST_CASE("Last-delimiter helpers treat a string delimiter as a substring", "[ut
   TEST_ASSERT_TRUE(tail == "b>c");
 }
 
-TEST_CASE("StringSplit edge cases", "[util][string]")
-{
-  TEST_ASSERT_TRUE(StringSplit("", ',').empty());
-
-  auto parts = StringSplitWhiteSpace("a  b c d", 2);
-  TEST_ASSERT_EQUAL_size_t(3, parts.size());
-  TEST_ASSERT_TRUE(parts[0] == "a");
-  TEST_ASSERT_TRUE(parts[1] == "b");
-  TEST_ASSERT_TRUE(parts[2] == "c d");
-}
-
 TEST_CASE("Case-insensitive helpers are safe on non-ASCII bytes", "[util][string]")
 {
   TEST_ASSERT_TRUE(StringIEquals("Caf\xC3\xA9", "CAF\xC3\xA9"));

components/common/include/SemVer.h

@@ -5,6 +5,10 @@
 #include <string>
 #include <string_view>
 
+// printf a SemVer without building a string: OS_LOGI(TAG, "version " SEMVER_FMT, SEMVER_ARG(v)). Like BSSID_FMT.
+#define SEMVER_FMT    "%u.%u.%u%s%s%s%s"
+#define SEMVER_ARG(v) static_cast<unsigned>((v).major), static_cast<unsigned>((v).minor), static_cast<unsigned>((v).patch), (v).prerelease.empty() ? "" : "-", (v).prerelease.c_str(), (v).build.empty() ? "" : "+", (v).build.c_str()
+
 namespace OpenShock {
   struct SemVer {
     uint16_t major;
@@ -33,8 +37,8 @@ namespace OpenShock {
       : major(major)
       , minor(minor)
       , patch(patch)
-      , prerelease(std::string(prerelease))
-      , build(std::string(build))
+      , prerelease(prerelease)
+      , build(build)
     {
     }
 

components/common/include/StringHelpers.h

@@ -3,12 +3,10 @@
 #include <array>
 #include <cstddef>
 #include <cstdint>
-#include <limits>
 #include <optional>
 #include <string>
 #include <string_view>
 #include <utility>
-#include <vector>
 
 namespace OpenShock {
   bool FormatToString(std::string& out, const char* format, ...) __attribute__((format(printf, 2, 3)));
@@ -18,6 +16,10 @@ namespace OpenShock {
   {
     return c == ' ' || c == '\t' || c == '\n' || c == '\v' || c == '\f' || c == '\r';
   }
+  constexpr bool CharIsAlnum(char c)
+  {
+    return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
+  }
   constexpr char CharToLower(char c)
   {
     return (c >= 'A' && c <= 'Z') ? static_cast<char>(c - 'A' + 'a') : c;
@@ -158,14 +160,6 @@ namespace OpenShock {
     parts[N - 1] = view.substr(pos);  // remainder (may still contain delimiters)
     return parts;
   }
-  // Splits on every `delimiter`, keeping empty fields between delimiters ("a,,b" -> a, "", b). After `maxSplits`
-  // splits the rest is returned as one final part. An empty input, or a trailing delimiter, adds no empty field.
-  std::vector<std::string_view> StringSplit(std::string_view view, char delimiter, std::size_t maxSplits = std::numeric_limits<std::size_t>::max());
-  // Splits on runs of characters matching `predicate`, dropping empty fields. After `maxSplits` fields the rest
-  // (from the start of the next field) is returned as one final part.
-  std::vector<std::string_view> StringSplit(std::string_view view, bool (*predicate)(char delimiter), std::size_t maxSplits = std::numeric_limits<std::size_t>::max());
-  std::vector<std::string_view> StringSplitNewLines(std::string_view view, std::size_t maxSplits = std::numeric_limits<std::size_t>::max());
-  std::vector<std::string_view> StringSplitWhiteSpace(std::string_view view, std::size_t maxSplits = std::numeric_limits<std::size_t>::max());
   constexpr std::pair<std::string_view, std::string_view> StringSplitByFirst(std::string_view view, char delimiter)
   {
     size_t pos = view.find(delimiter);