From 3c2396ab86cea654f9a17543d34f9093767fb2d7 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 23 Apr 2026 06:11:03 -0500 Subject: [PATCH] [core] Apply __atomic_load_n/store_n pattern to Millis64 NO_ATOMICS path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same treatment as the scheduler counters (#15947): - Unlocked reads of millis_major / last_millis at the top of Millis64Impl::compute(): switch from plain reads to __atomic_load_n(&..., __ATOMIC_RELAXED). - Unlocked write of last_millis in the "normal forward progression" branch: switch from plain assignment to __atomic_store_n(..., __ATOMIC_RELAXED). This is the one write that happens without the lock, so it needs to be formally atomic to pair cleanly with the unlocked atomic reader in the C++ memory model. - Writes under `lock` stay plain (millis_major++, last_millis = now inside the near-rollover branch). The lock serialises them against other writers. On ARMv5TE the builtins compile to plain LDR/STR — same codegen, no libatomic dependency. Updates the "accepting minor races" comment to describe the formally-defined version of the race. --- esphome/core/time_64.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/esphome/core/time_64.cpp b/esphome/core/time_64.cpp index b8a299ff7e..0d381dc322 100644 --- a/esphome/core/time_64.cpp +++ b/esphome/core/time_64.cpp @@ -72,10 +72,14 @@ uint64_t Millis64Impl::compute(uint32_t now) { // Without atomics, this implementation uses locks more aggressively: // 1. Always locks when near the rollover boundary (within 10 seconds) // 2. Always locks when detecting a large backwards jump - // 3. Updates without lock in normal forward progression (accepting minor races) - // This is less efficient but necessary without atomic operations. - uint16_t major = millis_major; - uint32_t last = last_millis; + // 3. Updates without lock in normal forward progression. + // Concurrent reads/writes use __atomic_load_n / __atomic_store_n with + // __ATOMIC_RELAXED so the cross-thread accesses are well-defined in the + // C++ memory model. On ARMv5TE these compile to plain LDR/STR. Writers + // holding `lock` use plain assignments; the lock serialises them against + // other writers. + uint16_t major = __atomic_load_n(&millis_major, __ATOMIC_RELAXED); + uint32_t last = __atomic_load_n(&last_millis, __ATOMIC_RELAXED); // Define a safe window around the rollover point (10 seconds) // This covers any reasonable scheduler delays or thread preemption @@ -101,13 +105,14 @@ uint64_t Millis64Impl::compute(uint32_t now) { // Update last_millis while holding lock last_millis = now; } else if (now > last) { - // Normal case: Not near rollover and time moved forward - // Update without lock. While this may cause minor races (microseconds of - // backwards time movement), they're acceptable because: + // Normal case: Not near rollover and time moved forward. Publish the new + // low word without taking the lock. A concurrent writer under lock may + // overwrite this with a slightly-different value, which can produce a + // few microseconds of backwards time movement — acceptable because: // 1. The scheduler operates at millisecond resolution, not microsecond // 2. We've already prevented the critical rollover race condition // 3. Any backwards movement is orders of magnitude smaller than scheduler delays - last_millis = now; + __atomic_store_n(&last_millis, now, __ATOMIC_RELAXED); } // If now <= last and we're not near rollover, don't update // This minimizes backwards time movement