[i2s_audio][router] Loop thread controls all state changes (#19089)

esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp

@@ -53,6 +53,13 @@ void I2SAudioSpeakerBase::dump_config() {
 void I2SAudioSpeakerBase::loop() {
   uint32_t event_group_bits = xEventGroupGetBits(this->event_group_);
 
+  // A stop that arrives while stopped cancels any start that has not been processed yet
+  constexpr uint32_t stop_bits = SpeakerEventGroupBits::COMMAND_STOP | SpeakerEventGroupBits::COMMAND_STOP_GRACEFULLY;
+  if ((event_group_bits & stop_bits) && (this->state_ == speaker::STATE_STOPPED)) {
+    xEventGroupClearBits(this->event_group_, stop_bits | SpeakerEventGroupBits::COMMAND_START);
+    event_group_bits &= ~(stop_bits | SpeakerEventGroupBits::COMMAND_START);
+  }
+
   if ((event_group_bits & SpeakerEventGroupBits::COMMAND_START) && (this->state_ == speaker::STATE_STOPPED)) {
     this->state_ = speaker::STATE_STARTING;
     xEventGroupClearBits(this->event_group_, SpeakerEventGroupBits::COMMAND_START);
@@ -239,8 +246,6 @@ void I2SAudioSpeakerBase::start() {
   if ((this->state_ == speaker::STATE_STARTING) || (this->state_ == speaker::STATE_RUNNING))
     return;
 
-  // Mark STARTING immediately to avoid transient STOPPED observations before loop() processes COMMAND_START.
-  this->state_ = speaker::STATE_STARTING;
   xEventGroupSetBits(this->event_group_, SpeakerEventGroupBits::COMMAND_START);
 }
 
@@ -249,11 +254,10 @@ void I2SAudioSpeakerBase::stop() { this->stop_(false); }
 void I2SAudioSpeakerBase::finish() { this->stop_(true); }
 
 void I2SAudioSpeakerBase::stop_(bool wait_on_empty) {
-  if (this->is_failed())
-    return;
-  if (this->state_ == speaker::STATE_STOPPED)
+  if (!this->is_ready() || this->is_failed())
     return;
 
+  // Always set the bit, even when stopped, so loop() can cancel a start that is still pending
   if (wait_on_empty) {
     xEventGroupSetBits(this->event_group_, SpeakerEventGroupBits::COMMAND_STOP_GRACEFULLY);
   } else {

esphome/components/router/speaker/router_speaker.cpp

@@ -2,6 +2,8 @@
 
 #ifdef USE_ESP32
 
+#include "esphome/core/application.h"
+#include "esphome/core/hal.h"
 #include "esphome/core/log.h"
 
 #include "esp_timer.h"
@@ -12,6 +14,9 @@ namespace esphome::router {
 
 static const char *const TAG = "router.speaker";
 
+// Maximum time to wait for the active output to report running after start() before giving up
+static const uint32_t STATE_TRANSITION_TIMEOUT_MS = 5000;
+
 static inline uint32_t atomic_subtract_clamped(std::atomic<uint32_t> &var, uint32_t amount) {
   uint32_t current = var.load(std::memory_order_acquire);
   uint32_t subtracted = 0;
@@ -72,6 +77,7 @@ void Router::loop() {
 
       this->apply_cached_state_to_active_();
       this->state_ = speaker::STATE_STARTING;
+      this->state_start_ms_ = App.get_loop_component_start_time();
       active->start();
     }
     return;
@@ -86,10 +92,17 @@ void Router::loop() {
   // set_audio_stream_info() and never reaches the output on its own; if the format
   // changed while stopped, only start()'s apply_cached_state_to_active_() pushes it
   // down before the output's play()-side auto-start locks in the stale format.
-  if (active->is_stopped()) {
+  // While STARTING, ignore a transient stopped report as speaker running state
+  // is set asynchronously from start(). Timeout if the speaker never transitions.
+  if (this->state_ == speaker::STATE_STARTING) {
+    if (active->is_running()) {
+      this->state_ = speaker::STATE_RUNNING;
+    } else if ((App.get_loop_component_start_time() - this->state_start_ms_) > STATE_TRANSITION_TIMEOUT_MS) {
+      ESP_LOGW(TAG, "Active output did not start; giving up");
+      this->state_ = speaker::STATE_STOPPED;
+    }
+  } else if (active->is_stopped()) {
     this->state_ = speaker::STATE_STOPPED;
-  } else if (this->state_ == speaker::STATE_STARTING && active->is_running()) {
-    this->state_ = speaker::STATE_RUNNING;
   }
 }
 
@@ -133,6 +146,8 @@ void Router::start() {
   this->frames_in_pipeline_.store(0, std::memory_order_release);
   this->apply_cached_state_to_active_();
   this->state_ = speaker::STATE_STARTING;
+  // May run on a producer task, so the cached loop timestamp is not usable here
+  this->state_start_ms_ = millis();
   this->get_active_output()->start();
 }
 

esphome/components/router/speaker/router_speaker.h

@@ -59,6 +59,9 @@ class Router final : public Component, public speaker::Speaker {
   // frames_in_pipeline_.
   std::atomic<uint32_t> frames_in_pipeline_{0};
 
+  // Set when entering STATE_STARTING; used to time out a start the output never acts on
+  uint32_t state_start_ms_{0};
+
   bool cached_pause_{false};
 
   void apply_cached_state_to_active_();