From 9cff70de2e3c1d738c501963df169f4dffa87366 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 22 Apr 2026 07:41:37 +0200 Subject: [PATCH 1/4] Address Copilot review: add BK72xx static_assert, clarify ISR dispatch comment --- esphome/components/libretiny/core.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/esphome/components/libretiny/core.cpp b/esphome/components/libretiny/core.cpp index 1d7cb6ff60..eb749a828c 100644 --- a/esphome/components/libretiny/core.cpp +++ b/esphome/components/libretiny/core.cpp @@ -19,8 +19,9 @@ void HOT yield() { ::yield(); } // Skip the Arduino core's ::millis() wrapper so the public esphome::millis() is // inlinable at its call sites and matches MillisInternal::get()'s fast path. // IRAM_ATTR is kept because components (e.g. rotary_encoder) call millis() from -// ISR handlers; xTaskGetTickCountFromISR() is used in ISR context to satisfy the -// FreeRTOS API contract. +// ISR handlers. On RTL87xx / LN882x, millis() dispatches to xTaskGetTickCountFromISR() +// in ISR context to satisfy the FreeRTOS API contract; on BK72xx it intentionally +// matches the Arduino core's wiring.c and always uses xTaskGetTickCount(). // // RTL87xx and LN882x run FreeRTOS at 1 kHz, so xTaskGetTickCount() is already in // milliseconds. BK72xx runs at 500 Hz — multiply by portTICK_PERIOD_MS (== 2) to @@ -32,7 +33,7 @@ uint32_t IRAM_ATTR HOT millis() { } #elif defined(USE_BK72XX) uint32_t IRAM_ATTR HOT millis() { - // BK72xx's Arduino millis() does not dispatch on ISR context; match that. + static_assert(configTICK_RATE_HZ == 500, "BK72xx millis() fast path assumes 500 Hz FreeRTOS tick"); return xTaskGetTickCount() * portTICK_PERIOD_MS; } #else From 2e31b19d4b69aeeb486d34bfaf1a7a8e4782d8bd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 22 Apr 2026 07:42:29 +0200 Subject: [PATCH 2/4] Drop vestigial IRAM_ATTR from BK72xx millis(); SDK masks IRQ during flash ops --- esphome/components/libretiny/core.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/esphome/components/libretiny/core.cpp b/esphome/components/libretiny/core.cpp index eb749a828c..54d5d68fe7 100644 --- a/esphome/components/libretiny/core.cpp +++ b/esphome/components/libretiny/core.cpp @@ -18,21 +18,24 @@ namespace esphome { void HOT yield() { ::yield(); } // Skip the Arduino core's ::millis() wrapper so the public esphome::millis() is // inlinable at its call sites and matches MillisInternal::get()'s fast path. -// IRAM_ATTR is kept because components (e.g. rotary_encoder) call millis() from -// ISR handlers. On RTL87xx / LN882x, millis() dispatches to xTaskGetTickCountFromISR() -// in ISR context to satisfy the FreeRTOS API contract; on BK72xx it intentionally -// matches the Arduino core's wiring.c and always uses xTaskGetTickCount(). // // RTL87xx and LN882x run FreeRTOS at 1 kHz, so xTaskGetTickCount() is already in -// milliseconds. BK72xx runs at 500 Hz — multiply by portTICK_PERIOD_MS (== 2) to -// convert ticks to milliseconds, matching the Arduino core's wiring.c. +// milliseconds. IRAM_ATTR is kept on those families because components (e.g. +// rotary_encoder) call millis() from ISR handlers, and in ISR context millis() +// dispatches to xTaskGetTickCountFromISR() to satisfy the FreeRTOS API contract. +// +// BK72xx runs FreeRTOS at 500 Hz — multiply by portTICK_PERIOD_MS (== 2) to convert +// ticks to milliseconds, matching the Arduino core's wiring.c. IRAM_ATTR is a no-op +// on BK72xx (see hal.h): the SDK masks FIQ + IRQ at the CPU around every flash +// operation, so no ISR runs while flash is stalled and IRAM placement is unnecessary. +// BK72xx therefore also skips the ISR dispatch, matching the Arduino core. #if defined(USE_RTL87XX) || defined(USE_LN882X) uint32_t IRAM_ATTR HOT millis() { static_assert(configTICK_RATE_HZ == 1000, "millis() fast path requires 1 kHz FreeRTOS tick"); return in_isr_context() ? xTaskGetTickCountFromISR() : xTaskGetTickCount(); } #elif defined(USE_BK72XX) -uint32_t IRAM_ATTR HOT millis() { +uint32_t HOT millis() { static_assert(configTICK_RATE_HZ == 500, "BK72xx millis() fast path assumes 500 Hz FreeRTOS tick"); return xTaskGetTickCount() * portTICK_PERIOD_MS; } From dd42d31e3fda5294ff0fff609babc4b93ec18d66 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 22 Apr 2026 07:44:34 +0200 Subject: [PATCH 3/4] Explain BK72xx GLOBAL_INT_DISABLE rationale in MillisInternal --- esphome/core/millis_internal.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/esphome/core/millis_internal.h b/esphome/core/millis_internal.h index ded49f9cd2..367b1a3d96 100644 --- a/esphome/core/millis_internal.h +++ b/esphome/core/millis_internal.h @@ -42,8 +42,13 @@ class MillisInternal { return xTaskGetTickCount(); #elif defined(USE_BK72XX) // BK72xx runs FreeRTOS at 500 Hz; scale ticks by portTICK_PERIOD_MS (== 2). - // Inlined here because esphome::millis() on BK72xx has no-op IRAM_ATTR but - // is still out-of-line, so calling it would cost a real function call. + // Inlined here because esphome::millis() on BK72xx is out-of-line (its + // IRAM_ATTR is a no-op — see hal.h — because the BK72xx SDK wraps flash + // operations in GLOBAL_INT_DISABLE() which masks FIQ + IRQ at the CPU for + // the duration of every write, so no ISR runs while flash is stalled and + // IRAM placement / FromISR dispatch are both unnecessary). Calling the + // out-of-line esphome::millis() would still cost a real function call, + // which this inlined path avoids. static_assert(configTICK_RATE_HZ == 500, "BK72xx MillisInternal assumes 500 Hz FreeRTOS tick"); return xTaskGetTickCount() * portTICK_PERIOD_MS; #else From 161cb622f1cf6e97ea7f751493ab330066d1fce4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 22 Apr 2026 07:45:34 +0200 Subject: [PATCH 4/4] Tighten millis() fast-path comments --- esphome/components/libretiny/core.cpp | 18 +++++++----------- esphome/core/millis_internal.h | 14 ++++---------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/esphome/components/libretiny/core.cpp b/esphome/components/libretiny/core.cpp index 54d5d68fe7..1b74e3addb 100644 --- a/esphome/components/libretiny/core.cpp +++ b/esphome/components/libretiny/core.cpp @@ -16,19 +16,15 @@ void loop(); namespace esphome { void HOT yield() { ::yield(); } -// Skip the Arduino core's ::millis() wrapper so the public esphome::millis() is -// inlinable at its call sites and matches MillisInternal::get()'s fast path. +// Inline the tick read so esphome::millis() matches MillisInternal::get()'s fast +// path instead of going through the Arduino core's out-of-line ::millis() wrapper. // -// RTL87xx and LN882x run FreeRTOS at 1 kHz, so xTaskGetTickCount() is already in -// milliseconds. IRAM_ATTR is kept on those families because components (e.g. -// rotary_encoder) call millis() from ISR handlers, and in ISR context millis() -// dispatches to xTaskGetTickCountFromISR() to satisfy the FreeRTOS API contract. +// RTL87xx / LN882x (1 kHz): xTaskGetTickCount() is already ms. IRAM_ATTR + ISR +// dispatch are needed because ISR handlers (e.g. rotary_encoder) call millis(). // -// BK72xx runs FreeRTOS at 500 Hz — multiply by portTICK_PERIOD_MS (== 2) to convert -// ticks to milliseconds, matching the Arduino core's wiring.c. IRAM_ATTR is a no-op -// on BK72xx (see hal.h): the SDK masks FIQ + IRQ at the CPU around every flash -// operation, so no ISR runs while flash is stalled and IRAM placement is unnecessary. -// BK72xx therefore also skips the ISR dispatch, matching the Arduino core. +// BK72xx (500 Hz): ticks * portTICK_PERIOD_MS (== 2). IRAM_ATTR and ISR dispatch +// are both unnecessary — the SDK masks FIQ + IRQ during flash writes (see hal.h), +// so no ISR runs while flash is stalled. #if defined(USE_RTL87XX) || defined(USE_LN882X) uint32_t IRAM_ATTR HOT millis() { static_assert(configTICK_RATE_HZ == 1000, "millis() fast path requires 1 kHz FreeRTOS tick"); diff --git a/esphome/core/millis_internal.h b/esphome/core/millis_internal.h index 367b1a3d96..bc1d55a1c4 100644 --- a/esphome/core/millis_internal.h +++ b/esphome/core/millis_internal.h @@ -36,19 +36,13 @@ class MillisInternal { #if defined(USE_ESP32) && CONFIG_FREERTOS_HZ == 1000 return xTaskGetTickCount(); #elif defined(USE_LIBRETINY) && (defined(USE_RTL87XX) || defined(USE_LN882X)) - // RTL87xx and LN882x run FreeRTOS at 1 kHz, so xTaskGetTickCount() is - // already in milliseconds. + // 1 kHz: xTaskGetTickCount() is already ms. static_assert(configTICK_RATE_HZ == 1000, "MillisInternal fast path requires 1 kHz FreeRTOS tick"); return xTaskGetTickCount(); #elif defined(USE_BK72XX) - // BK72xx runs FreeRTOS at 500 Hz; scale ticks by portTICK_PERIOD_MS (== 2). - // Inlined here because esphome::millis() on BK72xx is out-of-line (its - // IRAM_ATTR is a no-op — see hal.h — because the BK72xx SDK wraps flash - // operations in GLOBAL_INT_DISABLE() which masks FIQ + IRQ at the CPU for - // the duration of every write, so no ISR runs while flash is stalled and - // IRAM placement / FromISR dispatch are both unnecessary). Calling the - // out-of-line esphome::millis() would still cost a real function call, - // which this inlined path avoids. + // 500 Hz: scale by portTICK_PERIOD_MS (== 2). Inlined to avoid the + // out-of-line call to esphome::millis() (IRAM_ATTR is a no-op on BK72xx — + // SDK masks FIQ + IRQ during flash writes, see hal.h). static_assert(configTICK_RATE_HZ == 500, "BK72xx MillisInternal assumes 500 Hz FreeRTOS tick"); return xTaskGetTickCount() * portTICK_PERIOD_MS; #else