diff --git a/MQTT_IMPLEMENTATION.md b/MQTT_IMPLEMENTATION.md index 2184177f..d5eeb8bc 100644 --- a/MQTT_IMPLEMENTATION.md +++ b/MQTT_IMPLEMENTATION.md @@ -880,6 +880,9 @@ the radio actually performs in that case. report a connection that no longer exists. When the same link returns, each slot gets one immediate attempt at its current backoff rung (a tripped circuit breaker gets one immediate probe); a switch to the other medium also clears backoff and breakers +- Each medium's DHCP DNS servers are remembered when it gets its lease and restored when it is + selected again. lwIP keeps one global resolver list, so without this a node that fell back to + WiFi and then failed back to Ethernet would keep asking the WiFi network's DNS server - WiFi credentials changed at runtime (`set wifi.ssid` / `set wifi.pwd`) are used on the next reconnect attempt without a reboot - Packets are queued while a slot is disconnected and flushed when it recovers diff --git a/src/helpers/NetworkLink.cpp b/src/helpers/NetworkLink.cpp index 3b3b981f..3590ffa8 100644 --- a/src/helpers/NetworkLink.cpp +++ b/src/helpers/NetworkLink.cpp @@ -11,6 +11,7 @@ #include #include +#include #if defined(NETWORK_PREFER_ETHERNET) #include "ethernet/ch390/CH390Config.h" @@ -40,6 +41,8 @@ class NetworkLinkBase : public NetworkLink { bool _status_initialized = false; bool _last_connected = false; unsigned long _last_status_check = 0; + static constexpr int kDnsServers = 2; + std::atomic _dns_snapshot[kDnsServers] = {{0}, {0}}; AlertFaultPolicy::OutageSnapshot outage() const { return AlertFaultPolicy::unpackOutageSnapshot( @@ -65,6 +68,7 @@ class NetworkLinkBase : public NetworkLink { (uint32_t)now_ms, reason, outage())); } + public: unsigned long connectedAtMillis() const override { return _connected_at.load(std::memory_order_relaxed); @@ -81,6 +85,39 @@ class NetworkLinkBase : public NetworkLink { AlertFaultPolicy::OutageSnapshot outageSnapshot() const override { return outage(); } + + // Remember the resolver this link's DHCP lease installed, so switching back + // to it can put that resolver back. + // + // lwIP keeps ONE global server list, not one per interface: a DHCP lease on + // any medium calls dns_setserver() and overwrites whatever the other medium + // had. IDF 4.4 has no per-interface retention either — esp_netif_get_dns_info() + // reads the same globals — so the medium that leased last owns DNS for every + // socket. Without this, an Ethernet node that falls back to Wi-Fi and then + // fails back keeps asking the Wi-Fi network's DNS server. When the two are on + // different subnets that server is unreachable from Ethernet, and the node + // reads as connected while every broker and NTP hostname fails to resolve, + // until Ethernet's own DHCP renewal happens to fix it hours later. + void snapshotDns() { + for (int i = 0; i < kDnsServers; i++) { + const ip_addr_t* server = dns_getserver(i); + _dns_snapshot[i].store( + (server != nullptr && !ip_addr_isany(server)) + ? ip4_addr_get_u32(ip_2_ip4(server)) : 0, + std::memory_order_relaxed); + } + } + + // Only servers this link actually leased are restored; a medium that has + // never held a lease leaves the current resolver alone rather than blanking it. + void restoreDns() const { + for (int i = 0; i < kDnsServers; i++) { + const uint32_t addr = _dns_snapshot[i].load(std::memory_order_relaxed); + if (addr == 0) continue; + const ip_addr_t server = IPADDR4_INIT(addr); + dns_setserver(i, &server); + } + } }; class WiFiNetworkLink final : public NetworkLinkBase { @@ -199,7 +236,10 @@ class WiFiNetworkLink final : public NetworkLinkBase { // Already associated when the link started (end()/begin() leaves STA up): // there is no connect transition below to carry the setting, so apply it // here or the node keeps running whatever mode was set before. - if (connected) applyPowerPrefs(wifi_power_save); + if (connected) { + applyPowerPrefs(wifi_power_save); + snapshotDns(); + } } if ((uint32_t)(now_ms - _last_status_check) <= 10000) { @@ -216,6 +256,7 @@ class WiFiNetworkLink final : public NetworkLinkBase { _connected_at.store(now_ms, std::memory_order_relaxed); _reconnect_backoff_attempt = 0; applyPowerPrefs(wifi_power_save); + snapshotDns(); } _last_connected = true; return transitioned ? NetworkTransition::Up : NetworkTransition::None; @@ -308,6 +349,10 @@ class EthernetNetworkLink final : public NetworkLinkBase { _event_state.store(static_cast(EventState::GotIp), std::memory_order_relaxed); noteConnected(millis()); + // Taken here rather than at the maintain() edge: this is the moment + // the lease installed the resolver, before a Wi-Fi fallback lease + // can overwrite it. + snapshotDns(); break; case ARDUINO_EVENT_ETH_DISCONNECTED: _event_state.store(static_cast(EventState::LinkDown), @@ -350,7 +395,10 @@ class EthernetNetworkLink final : public NetworkLinkBase { _status_initialized = true; setOutage(AlertFaultPolicy::applyWifiStatus( now_ms, connected, outage(), false)); - if (connected) noteConnected(now_ms); + if (connected) { + noteConnected(now_ms); + snapshotDns(); + } return NetworkTransition::None; } @@ -364,6 +412,7 @@ class EthernetNetworkLink final : public NetworkLinkBase { _connected_at.store(now_ms, std::memory_order_relaxed); setOutage(AlertFaultPolicy::applyWifiStatus( now_ms, true, outage(), true)); + snapshotDns(); return NetworkTransition::Up; } @@ -525,6 +574,13 @@ class AutomaticNetworkLink final : public NetworkLink { } _selected_down_since.store(0, std::memory_order_relaxed); _selected.store(medium, std::memory_order_release); + // Put back the resolver this medium leased. lwIP's server list is global, + // so whichever medium leased last still owns it here (see snapshotDns()). + if (medium == NetworkMedium::Ethernet) { + _ethernet.restoreDns(); + } else if (medium == NetworkMedium::WiFi) { + _wifi.restoreDns(); + } } bool startOrRetryEthernet(uint32_t now_ms, bool restart) { diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index d0ce8c02..29edc89e 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -2335,31 +2335,38 @@ static constexpr unsigned long kNetworkTransitionDisconnectMs = 2000; // Close a slot's transport because the route under it changed. // -// Same shape as closeLiveClientForReconfigure(), and for the same F04 reason: a -// client that is still Starting has an attempt aimed at the OLD route, and -// softDisconnect() returns immediately on one that is not yet connected, so the -// attempt would run to completion and deliver a CONNECTED event that is -// indistinguishable from the new route's. On a medium switch the old route can -// still be briefly usable, so that event is not hypothetical. Cancelling it -// needs a real stop; reconnectSlotClient() starts a stopped client again. +// Deliberately NOT the reconfigure path, even though both close a live client. +// A reconfigure gives the slot a new endpoint and new credentials, so an +// in-flight attempt that completes afterwards reports a connection to the OLD +// broker under the new configuration, and cancelling it is worth a real stop +// (F04). A link transition changes neither: the attempt is aimed at the same +// broker with the same credentials, so if it completes it is credited to the +// endpoint it actually reached. It is stale only in that its socket sits on a +// route that is going away, and the ordinary DISCONNECTED path handles that. // -// A connected client takes the cheap path instead — bounded, and the esp-mqtt -// task stays, which is what keeps a Wi-Fi flap from recreating every slot's -// task and refragmenting internal heap. The wait is short because the old route -// is already gone; a client whose event is late is aborted by keepalive and -// picked up by the normal backoff retry. +// So an attempt in flight is left alone here. Stopping it would mean calling +// esp_mqtt_client_stop(), which waits on the SDK's API mutex and its task's +// stopped event with no bound (see stopSlotClient()) — the client task notices +// only when it returns from whatever transport call it is in, up to +// network_timeout_ms for a TLS connect. During teardown that is contained by +// the bridge's StopUnproven timeout; during a transition nothing contains it, +// so paying it serially for up to five slots would freeze the sole MQTT worker +// far past the bound this path advertises, and would block the bridge's own +// stop handshake behind it. A routine Wi-Fi flap must not cost that. // -// Nothing here touches a client that is not live, so a Quarantined one (its SDK -// task was never joined) is left alone, as it must be for the rest of the boot. +// A connected client takes the cheap bounded path, and the esp-mqtt task stays, +// which is what keeps a flap from recreating every slot's task and +// refragmenting internal heap. Nothing here touches a client that is not live, +// so a Quarantined one (its SDK task was never joined) is left alone, as it +// must be for the rest of the boot. void MQTTBridge::closeLiveClientForLinkTransition(int index) { if (index < 0 || index >= RUNTIME_MQTT_SLOTS) return; MQTTSlot& slot = _slots[index]; if (slot.client == nullptr || !clientStateIsLive(slot.client_state)) return; if (mqttClientStateHasAttemptInFlight(slot.client_state)) { - MQTT_DEBUG_PRINTLN("MQTT%d link transition during connect - stopping to cancel the attempt", + MQTT_DEBUG_PRINTLN("MQTT%d link transition during connect - leaving the attempt to resolve", index + 1); - stopSlotClient(index); return; } diff --git a/test/test_mqtt_client_state/test_mqtt_client_state.cpp b/test/test_mqtt_client_state/test_mqtt_client_state.cpp index 76cfba73..1ff2b97f 100644 --- a/test/test_mqtt_client_state/test_mqtt_client_state.cpp +++ b/test/test_mqtt_client_state/test_mqtt_client_state.cpp @@ -76,26 +76,31 @@ TEST(MqttClientState, StopIsAcknowledgedOnlyWhenEveryClientIsProvenStopped) { } // A link transition closes each slot's transport because the route under it -// changed. The two predicates above are what decide how, and getting the -// Starting case wrong is not cosmetic: softDisconnect() returns immediately on -// a client that is not yet connected, so that attempt would run to completion -// against the OLD route and deliver a CONNECTED event the new attempt cannot be -// told apart from. -TEST(MqttClientState, LinkTransitionStopsOnlyClientsWithAnAttemptInFlight) { +// changed. These two predicates are what decide which clients it may touch. +// +// The distinction from a reconfigure matters: a reconfigure must cancel an +// in-flight attempt, because that attempt would report the OLD endpoint under +// the new configuration. A transition changes no configuration, so the attempt +// is left to resolve — cancelling it would mean an unbounded +// esp_mqtt_client_stop() on the sole MQTT worker during a routine link flap. +TEST(MqttClientState, LinkTransitionOnlyDisconnectsClientsWithNoAttemptInFlight) { for (MqttClientState s : kAllStates) { - const bool touched = mqttClientStateIsLive(s); - const bool needs_stop = touched && mqttClientStateHasAttemptInFlight(s); + const bool live = mqttClientStateIsLive(s); + const bool disconnected_here = live && !mqttClientStateHasAttemptInFlight(s); // Quarantined is never touched: its SDK task was not joined, so it stays // out of service for the rest of the boot. - if (s == MqttClientState::Quarantined) EXPECT_FALSE(touched) << mqttClientStateName(s); + if (s == MqttClientState::Quarantined) EXPECT_FALSE(live) << mqttClientStateName(s); // Nothing that is already proven stopped is worth a disconnect. - if (mqttClientStateIsProvenStopped(s)) EXPECT_FALSE(touched) << mqttClientStateName(s); + if (mqttClientStateIsProvenStopped(s)) EXPECT_FALSE(live) << mqttClientStateName(s); - EXPECT_EQ(s == MqttClientState::Starting, needs_stop) << mqttClientStateName(s); - // The cheap bounded path, which keeps the esp-mqtt task alive. EXPECT_EQ(s == MqttClientState::Connected || s == MqttClientState::Disconnected, - touched && !needs_stop) << mqttClientStateName(s); + disconnected_here) << mqttClientStateName(s); + // Starting is live, and still must not be disconnected here. + if (s == MqttClientState::Starting) { + EXPECT_TRUE(live); + EXPECT_FALSE(disconnected_here); + } } }