[improv_serial] Support non-Wi-Fi network interfaces (#17598)

Co-authored-by: J. Nick Koston <nick@koston.org>

esphome/components/improv_serial/__init__.py

@@ -16,7 +16,7 @@ from esphome.types import ConfigType
 
 AUTO_LOAD = ["improv_base"]
 CODEOWNERS = ["@esphome/core"]
-DEPENDENCIES = ["logger", "wifi"]
+DEPENDENCIES = ["logger", "network"]
 
 improv_serial_ns = cg.esphome_ns.namespace("improv_serial")
 

esphome/components/improv_serial/improv_serial_component.cpp

@@ -1,5 +1,5 @@
 #include "improv_serial_component.h"
-#ifdef USE_WIFI
+#ifdef USE_IMPROV_SERIAL
 #include "esphome/core/application.h"
 #include "esphome/core/defines.h"
 #include "esphome/core/hal.h"
@@ -7,7 +7,10 @@
 #include "esphome/core/version.h"
 
 #include "esphome/components/logger/logger.h"
+#include "esphome/components/network/util.h"
+#ifdef USE_WIFI
 #include "esphome/components/wifi/scan_list.h"
+#endif
 
 #include <array>
 
@@ -26,13 +29,17 @@ void ImprovSerialComponent::setup() {
   this->hw_serial_ = logger::global_logger->get_hw_serial();
 #endif
 
-  if (wifi::global_wifi_component->has_sta()) {
+  // The Improv state machine tracks Wi-Fi provisioning only. General device
+  // connectivity (e.g. Ethernet) is reported separately via GET_NETWORK_STATE.
+#ifdef USE_WIFI
+  if (wifi::global_wifi_component != nullptr && wifi::global_wifi_component->has_sta()) {
     this->state_ = improv::STATE_PROVISIONED;
-  } else if (!wifi::global_wifi_component->is_disabled()) {
+  } else if (wifi::global_wifi_component != nullptr && !wifi::global_wifi_component->is_disabled()) {
     // Respect Wi-Fi's disabled state; forcing a scan while disabled throws
     // the wifi component into an invalid state from which it cannot recover.
     wifi::global_wifi_component->start_scanning();
   }
+#endif
 }
 
 void ImprovSerialComponent::loop() {
@@ -55,8 +62,14 @@ void ImprovSerialComponent::loop() {
     }
   }
 
-  if (this->state_ == improv::STATE_PROVISIONING) {
-    if (wifi::global_wifi_component->is_connected()) {
+#ifdef USE_WIFI
+  if (this->state_ == improv::STATE_PROVISIONING && wifi::global_wifi_component != nullptr &&
+      wifi::global_wifi_component->is_connected()) {
+    // Being connected is not enough: re-provisioning a device that is already online leaves the
+    // prior network up until it drops, so check that the joined network is the requested one
+    // before reporting success. Same test as the wifi.connect action.
+    char ssid_buf[wifi::SSID_BUFFER_SIZE];
+    if (strcmp(wifi::global_wifi_component->wifi_ssid_to(ssid_buf), this->connecting_sta_.get_ssid().c_str()) == 0) {
       wifi::global_wifi_component->save_wifi_sta(this->connecting_sta_.get_ssid(),
                                                  this->connecting_sta_.get_password());
       this->connecting_sta_ = {};
@@ -66,6 +79,7 @@ void ImprovSerialComponent::loop() {
       this->send_settings_response_(improv::WIFI_SETTINGS);
     }
   }
+#endif
 }
 
 void ImprovSerialComponent::dump_config() { ESP_LOGCONFIG(TAG, "Improv Serial:"); }
@@ -143,15 +157,17 @@ void ImprovSerialComponent::write_data_(const uint8_t *data, const size_t size)
 #endif
 }
 
-void ImprovSerialComponent::send_settings_response_(improv::Command command) {
-  std::array<uint8_t, improv::RPC_RESPONSE_MAX_SIZE> buf;
-  improv::RpcResponseBuilder builder(buf, command);
-#ifdef USE_IMPROV_SERIAL_NEXT_URL
-  this->add_next_url_(builder, MAX_NEXT_URL_LEN);
-#endif
 #ifdef USE_WEBSERVER
-  for (auto &ip : wifi::global_wifi_component->wifi_sta_ip_addresses()) {
-    if (ip.is_ip4()) {
+void ImprovSerialComponent::add_webserver_urls_(improv::RpcResponseBuilder &builder, [[maybe_unused]] bool wifi_first) {
+  // The webserver listens on every interface, so advertise each one that has a usable IPv4.
+  // network::get_ip_addresses() can't be used here: it returns only the highest-priority
+  // interface's addresses, which are all-unset (0.0.0.0) when e.g. Ethernet has no link while
+  // the device is online via Wi-Fi, and 0.0.0.0 must not become the advertised URL. OpenThread
+  // is omitted: it only ever has IPv6 addresses, which cannot form an IPv4 http:// URL.
+  const auto append_urls = [&builder](const network::IPAddresses &addresses) {
+    for (const auto &ip : addresses) {
+      if (!ip.is_ip4() || !ip.is_set())
+        continue;
       char ip_buf[network::IP_ADDRESS_BUFFER_SIZE];
       ip.str_to(ip_buf);
       // "http://" (7) + IP (40) + ":" (1) + port (5) + null (1) = 54
@@ -162,9 +178,43 @@ void ImprovSerialComponent::send_settings_response_(improv::Command command) {
       if (!builder.add_string(webserver_url, len)) {
         ESP_LOGW(TAG, "Response full; URL dropped");
       }
-      break;
     }
-  }
+  };
+#ifdef USE_WIFI
+  // Clients redirect to the first URL, so the interface the client just configured has to lead:
+  // another interface's address can be on a subnet that client cannot reach.
+  const auto append_wifi_urls = [&append_urls]() {
+    if (wifi::global_wifi_component != nullptr)
+      append_urls(wifi::global_wifi_component->get_ip_addresses());
+  };
+  if (wifi_first)
+    append_wifi_urls();
+#endif
+#ifdef USE_ETHERNET
+  if (ethernet::global_eth_component != nullptr)
+    append_urls(ethernet::global_eth_component->get_ip_addresses());
+#endif
+#ifdef USE_MODEM
+  if (modem::global_modem_component != nullptr)
+    append_urls(modem::global_modem_component->get_ip_addresses());
+#endif
+#ifdef USE_WIFI
+  if (!wifi_first)
+    append_wifi_urls();
+#endif
+}
+#endif  // USE_WEBSERVER
+
+void ImprovSerialComponent::send_settings_response_(improv::Command command) {
+  std::array<uint8_t, improv::RPC_RESPONSE_MAX_SIZE> buf;
+  improv::RpcResponseBuilder builder(buf, command);
+#ifdef USE_IMPROV_SERIAL_NEXT_URL
+  this->add_next_url_(builder, MAX_NEXT_URL_LEN);
+#endif
+#ifdef USE_WEBSERVER
+  // This response only ever answers Wi-Fi provisioning, so lead with the Wi-Fi URL as it did
+  // before other interfaces were reported.
+  this->add_webserver_urls_(builder, /*wifi_first=*/true);
 #endif
   this->send_response_(builder.finish(false));
 }
@@ -231,7 +281,8 @@ bool ImprovSerialComponent::parse_improv_serial_byte_(uint8_t byte) {
 bool ImprovSerialComponent::parse_improv_payload_(improv::ImprovCommand &command) {
   switch (command.command) {
     case improv::WIFI_SETTINGS: {
-      if (wifi::global_wifi_component->is_disabled()) {
+#ifdef USE_WIFI
+      if (wifi::global_wifi_component == nullptr || wifi::global_wifi_component->is_disabled()) {
         // Wi-Fi is disabled, so we can't provision. Respond immediately
         // instead of letting the client wait out its provisioning timeout.
         ESP_LOGW(TAG, "Wi-Fi is disabled; cannot provision");
@@ -243,21 +294,32 @@ bool ImprovSerialComponent::parse_improv_payload_(improv::ImprovCommand &command
       sta.set_password(command.password.c_str());
       this->connecting_sta_ = sta;
 
+      // Sampled before start_connecting(): the old connection drops asynchronously after it.
+      const bool switching = wifi::global_wifi_component->is_connected();
       wifi::global_wifi_component->set_sta(sta);
       wifi::global_wifi_component->start_connecting(sta);
       this->set_state_(improv::STATE_PROVISIONING);
       ESP_LOGD(TAG, "Received settings: SSID=%s, password=" LOG_SECRET("%s"), command.ssid.c_str(),
                command.password.c_str());
 
-      this->set_timeout("wifi-connect-timeout", 30000, [this]() { this->on_wifi_connect_timeout_(); });
+      this->set_timeout("wifi-connect-timeout", switching ? WIFI_SWITCH_TIMEOUT_MS : WIFI_CONNECT_TIMEOUT_MS,
+                        [this]() { this->on_wifi_connect_timeout_(); });
+#else
+      // No Wi-Fi support compiled in; there is nothing to provision.
+      ESP_LOGW(TAG, "Wi-Fi not supported; cannot provision");
+      this->set_error_(improv::ERROR_UNABLE_TO_CONNECT);
+#endif
       return true;
     }
-    case improv::GET_CURRENT_STATE:
-      if (wifi::global_wifi_component->is_disabled()) {
-        // Wi-Fi is disabled; report the Improv "stopped" state so a client can tell
-        // the user that provisioning is unavailable. Reported transiently without
-        // disturbing our internal provisioning state machine, so a later `wifi.enable`
-        // still reports the correct state.
+    case improv::GET_CURRENT_STATE: {
+      // This state machine tracks Wi-Fi provisioning only. When Wi-Fi is disabled or not
+      // compiled in, provisioning is unavailable -> report STOPPED so the client doesn't
+      // offer a Wi-Fi form. General connectivity (e.g. Ethernet) is reported separately
+      // via GET_NETWORK_STATE.
+#ifdef USE_WIFI
+      if (wifi::global_wifi_component == nullptr || wifi::global_wifi_component->is_disabled()) {
+        // Reported transiently without disturbing our internal provisioning state machine,
+        // so a later `wifi.enable` still reports the correct state.
         this->send_current_state_(improv::STATE_STOPPED);
         return true;
       }
@@ -265,14 +327,20 @@ bool ImprovSerialComponent::parse_improv_payload_(improv::ImprovCommand &command
       if (this->state_ == improv::STATE_PROVISIONED) {
         this->send_settings_response_(improv::GET_CURRENT_STATE);
       }
+#else
+      this->send_current_state_(improv::STATE_STOPPED);
+#endif
       return true;
+    }
     case improv::GET_DEVICE_INFO: {
       this->send_version_info_();
       return true;
     }
     case improv::GET_WIFI_NETWORKS: {
-      const auto &results = wifi::global_wifi_component->get_scan_result();
+      // Declared out here because the terminating empty response is sent with or without Wi-Fi
       std::array<uint8_t, improv::RPC_RESPONSE_MAX_SIZE> buf;
+#ifdef USE_WIFI
+      const auto &results = wifi::global_wifi_component->get_scan_result();
       for (const auto &scan : results) {
         bool with_auth = false;
         if (!wifi::should_show_scan_entry(results, scan, with_auth))
@@ -289,11 +357,52 @@ bool ImprovSerialComponent::parse_improv_payload_(improv::ImprovCommand &command
         builder.add_string(YESNO(with_auth));
         this->send_response_(builder.finish(false));
       }
+#endif  // USE_WIFI
       // Send empty response to signify the end of the list.
       improv::RpcResponseBuilder builder(buf, improv::GET_WIFI_NETWORKS);
       this->send_response_(builder.finish(false));
       return true;
     }
+    case improv::GET_NETWORK_STATE: {
+      // Reports general device connectivity and which network interfaces are present, decoupled
+      // from the Wi-Fi-only provisioning state machine. data[0] is a decimal flags byte;
+      // when online, the reachable device URL(s) follow.
+      uint8_t flags = 0;
+      if (network::is_connected())
+        flags |= improv::NETWORK_IS_ONLINE;
+#ifdef USE_WIFI
+      flags |= improv::NETWORK_SUPPORTS_WIFI;
+#endif
+#ifdef USE_ETHERNET
+      flags |= improv::NETWORK_SUPPORTS_ETHERNET;
+#endif
+#ifdef USE_OPENTHREAD
+      flags |= improv::NETWORK_SUPPORTS_THREAD;
+#endif
+#ifdef USE_MODEM
+      flags |= improv::NETWORK_SUPPORTS_MODEM;
+#endif
+      std::array<uint8_t, improv::RPC_RESPONSE_MAX_SIZE> buf;
+      improv::RpcResponseBuilder builder(buf, improv::GET_NETWORK_STATE);
+      // Every flag bit fits int8_t's positive range, so int8_to_str renders the byte
+      static_assert(improv::NETWORK_SUPPORTS_MODEM <= 0x7F, "network flags no longer fit int8_to_str");
+      char flags_buf[4];  // uint8_t: max "255" + null
+      char *flags_end = int8_to_str(flags_buf, static_cast<int8_t>(flags));
+      builder.add_string(flags_buf, flags_end - flags_buf);
+#ifdef USE_WEBSERVER
+      // Not tied to one interface, so follow the configured priority the way
+      // network::get_ip_addresses() does: a wifi-first network priority list leads with Wi-Fi.
+      if (flags & improv::NETWORK_IS_ONLINE) {
+#if defined(USE_NETWORK_PRIMARY_INTERFACE_WIFI) && defined(USE_WIFI)
+        this->add_webserver_urls_(builder, /*wifi_first=*/true);
+#else
+        this->add_webserver_urls_(builder, /*wifi_first=*/false);
+#endif
+      }
+#endif
+      this->send_response_(builder.finish(false));
+      return true;
+    }
     default: {
       ESP_LOGW(TAG, "Unknown payload");
       this->set_error_(improv::ERROR_UNKNOWN_RPC);
@@ -331,12 +440,14 @@ void ImprovSerialComponent::send_response_(std::span<const uint8_t> response) {
   this->write_data_(response.data(), response.size());
 }
 
+#ifdef USE_WIFI
 void ImprovSerialComponent::on_wifi_connect_timeout_() {
   this->set_error_(improv::ERROR_UNABLE_TO_CONNECT);
   this->set_state_(improv::STATE_AUTHORIZED);
   ESP_LOGW(TAG, "Timed out while connecting to Wi-Fi network");
   wifi::global_wifi_component->clear_sta();
 }
+#endif
 
 ImprovSerialComponent *global_improv_serial_component =  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
     nullptr;                                             // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)

esphome/components/improv_serial/improv_serial_component.h

@@ -2,15 +2,19 @@
 
 #include "esphome/components/improv_base/improv_base.h"
 #include "esphome/components/logger/logger.h"
-#include "esphome/components/wifi/wifi_component.h"
+#include "esphome/components/network/util.h"
 #include "esphome/core/component.h"
 #include "esphome/core/defines.h"
 #include "esphome/core/helpers.h"
-#ifdef USE_WIFI
+#ifdef USE_IMPROV_SERIAL
 #include <improv.h>
 #include <span>
 #include <vector>
 
+#ifdef USE_WIFI
+#include "esphome/components/wifi/wifi_component.h"
+#endif
+
 #ifdef USE_IMPROV_SERIAL_UART
 #include "esphome/components/uart/uart_component.h"
 #elif defined(USE_ESP32)
@@ -48,13 +52,22 @@ enum ImprovSerialType : uint8_t {
 static const uint16_t IMPROV_SERIAL_TIMEOUT = 100;
 static const uint8_t IMPROV_SERIAL_VERSION = 1;
 
+#ifdef USE_WIFI
+// Wi-Fi connect failure timers: a fresh provision reports at 30 s (stock behavior), while
+// switching networks on an already-connected device (disconnect + reconnect) can legitimately
+// take longer; 90 s matches esp32_improv's default wifi_timeout.
+static const uint32_t WIFI_CONNECT_TIMEOUT_MS = 30000;
+static const uint32_t WIFI_SWITCH_TIMEOUT_MS = 90000;
+#endif
+
 // The serial frame length field is one byte
 static constexpr size_t MAX_SERIAL_RESPONSE = 255;
 // command + data length + trailing byte
 static constexpr size_t RPC_RESPONSE_OVERHEAD = 3;
 static constexpr size_t MAX_SERIAL_PAYLOAD = MAX_SERIAL_RESPONSE - RPC_RESPONSE_OVERHEAD;
 #ifdef USE_WEBSERVER
-// length byte + "http://" + IPv4 + ":" + port
+// length byte + "http://" + IPv4 + ":" + port. Reserves the first URL only; a device with
+// several interfaces online adds the rest best-effort and warns if one no longer fits.
 static constexpr size_t WEBSERVER_URL_RESERVE = 1 + 7 + 15 + 1 + 5;
 #else
 static constexpr size_t WEBSERVER_URL_RESERVE = 0;
@@ -84,8 +97,15 @@ class ImprovSerialComponent final : public Component, public improv_base::Improv
   void send_current_state_(improv::State state);
   void set_error_(improv::Error error);
   void send_response_(std::span<const uint8_t> response);
+#ifdef USE_WIFI
   void on_wifi_connect_timeout_();
+#endif
 
+#ifdef USE_WEBSERVER
+  /// Append one web server URL per interface that has a usable IPv4. With wifi_first the Wi-Fi
+  /// URL leads, for responses to Wi-Fi provisioning; otherwise interfaces go in priority order.
+  void add_webserver_urls_(improv::RpcResponseBuilder &builder, [[maybe_unused]] bool wifi_first);
+#endif
   void send_settings_response_(improv::Command command);
   void send_version_info_();
 
@@ -167,7 +187,9 @@ class ImprovSerialComponent final : public Component, public improv_base::Improv
 
   std::vector<uint8_t> rx_buffer_;
   uint32_t last_read_byte_{0};
+#ifdef USE_WIFI
   wifi::WiFiAP connecting_sta_;
+#endif
   improv::State state_{improv::STATE_AUTHORIZED};
 };
 

tests/components/improv_serial/common-ethernet.yaml

@@ -0,0 +1,17 @@
+ethernet:
+  type: W5500
+  clk_pin: 19
+  mosi_pin: 21
+  miso_pin: 17
+  cs_pin: 18
+  interrupt_pin: 36
+  reset_pin: 12
+  clock_speed: 10Mhz
+
+logger:
+  hardware_uart: UART0
+
+# Exercises the per-interface webserver URL collection at compile time
+web_server:
+
+improv_serial:

tests/components/improv_serial/test-ethernet.esp32-idf.yaml

@@ -0,0 +1,2 @@
+packages:
+  improv_serial: !include common-ethernet.yaml

tests/integration/fixtures/external_components/wifi/wifi_component.cpp

@@ -29,6 +29,8 @@ void WiFiComponent::set_sta(const WiFiAP &ap) { ESP_LOGI(TAG, "set_sta ssid=%s",
 
 void WiFiComponent::start_connecting(const WiFiAP &ap) {
   ESP_LOGI(TAG, "start_connecting ssid=%s", ap.get_ssid().c_str());
+  // Connecting succeeds immediately, so the requested network is the connected one
+  this->connected_ssid_ = ap.get_ssid().c_str();
 }
 
 void WiFiComponent::clear_sta() { ESP_LOGI(TAG, "clear_sta"); }

tests/integration/fixtures/external_components/wifi/wifi_component.h

@@ -13,11 +13,15 @@
 #include "esphome/core/component.h"
 #include "esphome/core/string_ref.h"
 
+#include <cstdio>
+#include <span>
 #include <string>
 #include <vector>
 
 namespace esphome::wifi {
 
+static constexpr size_t SSID_BUFFER_SIZE = 33;
+
 class WiFiAP {
  public:
   void set_ssid(const char *ssid) { this->ssid_ = ssid; }
@@ -58,6 +62,12 @@ class WiFiComponent : public Component {
   bool is_disabled() const { return false; }
   // Always connected so network::is_connected() keeps the API server accepting clients
   bool is_connected() const { return true; }
+  // Reports the network start_connecting() was last asked for, so a consumer checking that it
+  // joined the network it requested (rather than an earlier one) sees the connect succeed
+  const char *wifi_ssid_to(std::span<char, SSID_BUFFER_SIZE> buffer) {
+    snprintf(buffer.data(), buffer.size(), "%s", this->connected_ssid_.c_str());
+    return buffer.data();
+  }
   void start_scanning();
   const std::vector<WiFiScanResult> &get_scan_result() const { return this->scan_result_; }
   void set_sta(const WiFiAP &ap);
@@ -70,6 +80,7 @@ class WiFiComponent : public Component {
 
  protected:
   std::vector<WiFiScanResult> scan_result_;
+  std::string connected_ssid_;
 };
 
 extern WiFiComponent *global_wifi_component;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)