From 436bb65ae611a5fb62ba14412dfd5e6afa978591 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 19:58:50 -0700 Subject: [PATCH] fix(mqtt): consult the RTC when libc cannot vouch for the fallback clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The usable-clock fallback asked libc only, which does not answer for the case it was written to cover. On a cold boot ESP32RTCClock::begin() stamps libc with a 2024 placeholder on power-on; AutoDiscoverRTCClock::begin() probes the chip but never copies its time across, and getCurrentTime() reads the chip directly. So a Station G3 or T-Beam Supreme that knows exactly what time it is, on a network with UDP/123 blocked, still failed the plausibility test, left _ntp_synced false, and brought up no slots — precisely the deployment the fallback exists for. Ask the RTC when libc is below the floor. libc still wins when it is usable: a clock SNTP set recently outranks a chip that may have drifted. Accepting the RTC value then flows through the same block, so settimeofday() repairs libc and the epoch is written back to the chip. The choice is chooseFallbackClock() in MQTTConnectionPolicy, host-tested across the four states including the power-on placeholder and the exact floor. Also corrects the previous commit's claim that configTime() is called only when a server replied — the fallback necessarily points it at each server before knowing that; it is the post-acceptance call that is now conditional. --- src/helpers/MQTTConnectionPolicy.h | 21 ++++++++++ src/helpers/bridges/MQTTBridge.cpp | 25 +++++++---- .../test_mqtt_connection_policy.cpp | 42 +++++++++++++++++++ 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/helpers/MQTTConnectionPolicy.h b/src/helpers/MQTTConnectionPolicy.h index b2c57690..096c5d38 100644 --- a/src/helpers/MQTTConnectionPolicy.h +++ b/src/helpers/MQTTConnectionPolicy.h @@ -234,4 +234,25 @@ static inline StaleTokenAction classifyStaleToken(bool minted, bool connected, return broker_enforces_exp ? StaleTokenAction::Bounce : StaleTokenAction::KeepAlive; } +// Which clock to fall back on when no NTP server answered. +enum class ClockSource : uint8_t { + None, // nothing plausible to work from — stay unsynced + System, // libc already holds a usable time + Rtc, // libc does not, but the RTC does +}; + +// System first: a clock SNTP set recently outranks an RTC that may have drifted. +// The RTC matters on a cold boot, where ESP32RTCClock::begin() seeds libc with a 2024 +// placeholder on power-on while a detected chip already holds real time and +// AutoDiscoverRTCClock::begin() never copies one into the other. Never while +// validating a server: that asks whether a specific host answers, and no clock can +// answer it. Pass rtc_time 0 when the board has no clock to consult. +static inline ClockSource chooseFallbackClock(bool validating_server, uint32_t system_time, + uint32_t rtc_time, uint32_t min_valid_epoch) { + if (validating_server) return ClockSource::None; + if (system_time >= min_valid_epoch) return ClockSource::System; + if (rtc_time >= min_valid_epoch) return ClockSource::Rtc; + return ClockSource::None; +} + } // namespace MQTTConnectionPolicy diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 4da87149..7afb2e28 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -4026,12 +4026,21 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { // all. Keep the behaviour, but as its own decision rather than as a claim about a // server that never replied. Not on the validation path — `set mqtt.ntp` asks // whether that server works, and the clock cannot answer for it. - if (!ntp_ok && !primary_only) { - unsigned long existing = (unsigned long)time(nullptr); - if (existing >= kMinValidEpoch) { - epochTime = existing; + if (!ntp_ok) { + const unsigned long system_time = (unsigned long)time(nullptr); + // On a cold boot with a detected RTC chip these disagree: ESP32RTCClock::begin() + // stamps libc with a 2024 placeholder on power-on, AutoDiscoverRTCClock::begin() + // never copies the chip into it, and getCurrentTime() reads the chip. Asking libc + // alone would reject a board that knows exactly what time it is. + const unsigned long rtc_time = _rtc ? (unsigned long)_rtc->getCurrentTime() : 0; + const MQTTConnectionPolicy::ClockSource source = MQTTConnectionPolicy::chooseFallbackClock( + primary_only, (uint32_t)system_time, (uint32_t)rtc_time, (uint32_t)kMinValidEpoch); + if (source != MQTTConnectionPolicy::ClockSource::None) { + const bool from_rtc = (source == MQTTConnectionPolicy::ClockSource::Rtc); + epochTime = from_rtc ? rtc_time : system_time; ntp_ok = true; - MQTT_DEBUG_PRINTLN("No NTP server answered; continuing on the existing clock: %lu", existing); + MQTT_DEBUG_PRINTLN("No NTP server answered; continuing on the existing %s: %lu", + from_rtc ? "RTC" : "system clock", epochTime); } } @@ -4048,8 +4057,10 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { accepted.tv_usec = 0; settimeofday(&accepted, nullptr); - // Only when a server actually answered: there is nothing to point SNTP at - // otherwise, and the existing configuration is the best guess available. + // Only when a server supplied the accepted epoch. The fallback above necessarily + // points configTime() at each server before knowing whether it replies; this is + // the post-acceptance call, and there is nothing to re-point it at when the epoch + // came from a local clock. if (ntp_server_used) { configTime(0, 0, ntp_server_used); } diff --git a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp index 7c7bd7de..c3cd469e 100644 --- a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp +++ b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp @@ -290,6 +290,48 @@ TEST(StaleToken, FailedMintNeverReconnects) { EXPECT_EQ(StaleTokenAction::Defer, Policy::classifyStaleToken(false, true, false)); } +using Policy::ClockSource; + +// 2026-01-01, the bridge's plausibility floor, and the 2024 placeholder +// ESP32RTCClock::begin() stamps into libc on a power-on reset. +static const uint32_t kFloor = 1767225600; +static const uint32_t kPowerOnPlaceholder = 1715770351; +static const uint32_t kPlausibleNow = 1786000000; + +TEST(FallbackClock, PrefersTheSystemClockWhenItIsUsable) { + // A clock SNTP set recently outranks an RTC that may have drifted. + EXPECT_EQ(ClockSource::System, + Policy::chooseFallbackClock(false, kPlausibleNow, kPlausibleNow - 900, kFloor)); +} + +TEST(FallbackClock, FallsBackToTheRtcOnAColdBoot) { + // The case the system-clock-only check missed: libc holds the power-on + // placeholder while a detected chip holds real time. + EXPECT_EQ(ClockSource::Rtc, + Policy::chooseFallbackClock(false, kPowerOnPlaceholder, kPlausibleNow, kFloor)); +} + +TEST(FallbackClock, NothingUsableStaysUnsynced) { + EXPECT_EQ(ClockSource::None, + Policy::chooseFallbackClock(false, kPowerOnPlaceholder, kPowerOnPlaceholder, kFloor)); + // rtc_time 0 is how a board with no clock to consult is passed in. + EXPECT_EQ(ClockSource::None, + Policy::chooseFallbackClock(false, kPowerOnPlaceholder, 0, kFloor)); +} + +TEST(FallbackClock, ServerValidationNeverAcceptsALocalClock) { + // `set mqtt.ntp` asks whether that host answers. No clock can answer for it, + // however plausible — this is the path where a typo has to fail. + EXPECT_EQ(ClockSource::None, + Policy::chooseFallbackClock(true, kPlausibleNow, kPlausibleNow, kFloor)); +} + +TEST(FallbackClock, TheFloorItselfIsAccepted) { + EXPECT_EQ(ClockSource::System, Policy::chooseFallbackClock(false, kFloor, 0, kFloor)); + EXPECT_EQ(ClockSource::Rtc, Policy::chooseFallbackClock(false, kFloor - 1, kFloor, kFloor)); + EXPECT_EQ(ClockSource::None, Policy::chooseFallbackClock(false, kFloor - 1, kFloor - 1, kFloor)); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS();