cleanup

2025-08-09 11:57:46 +00:00 · 2025-06-27 06:30:42 -05:00 · 2025-06-27 06:30:42 -05:00 · 6f07b54772
commit 6f07b54772
parent ce0f1dfcb6
3 changed files with 72 additions and 99 deletions
--- a/esphome/components/api/api_connection.cpp
+++ b/esphome/components/api/api_connection.cpp
@ -65,6 +65,10 @@ uint32_t APIConnection::get_batch_delay_ms_() const { return this->parent_->get_
 void APIConnection::start() {
  this->last_traffic_ = App.get_loop_component_start_time();

+  // Set next_ping_retry_ to prevent immediate ping
+  // This ensures the first ping happens after the keepalive period
+  this->next_ping_retry_ = this->last_traffic_ + KEEPALIVE_TIMEOUT_MS;
+
  APIError err = this->helper_->init();
  if (err != APIError::OK) {
    on_fatal_error();
@ -73,6 +77,7 @@ void APIConnection::start() {
    return;
  }
  this->client_info_ = helper_->getpeername();
+  this->client_peername_ = this->client_info_;
  this->helper_->set_log_info(this->client_info_);
 }

@ -90,10 +95,10 @@ APIConnection::~APIConnection() {
 }

 void APIConnection::loop() {
-  if (this->flags_.next_close) {
+  if (this->next_close_) {
    // requested a disconnect
    this->helper_->close();
-    this->flags_.remove = true;
+    this->remove_ = true;
    return;
  }

@ -134,14 +139,15 @@ void APIConnection::loop() {
        } else {
          this->read_message(0, buffer.type, nullptr);
        }
-        if (this->flags_.remove)
+        if (this->remove_)
          return;
      }
    }
  }

  // Process deferred batch if scheduled
-  if (this->flags_.batch_scheduled && now - this->deferred_batch_.batch_start_time >= this->get_batch_delay_ms_()) {
+  if (this->deferred_batch_.batch_scheduled &&
+      now - this->deferred_batch_.batch_start_time >= this->get_batch_delay_ms_()) {
    this->process_batch_();
  }

@ -151,21 +157,26 @@ void APIConnection::loop() {
    this->initial_state_iterator_.advance();
  }

-  if (this->flags_.sent_ping) {
+  if (this->sent_ping_) {
    // Disconnect if not responded within 2.5*keepalive
    if (now - this->last_traffic_ > KEEPALIVE_DISCONNECT_TIMEOUT) {
      on_fatal_error();
      ESP_LOGW(TAG, "%s is unresponsive; disconnecting", this->get_client_combined_info().c_str());
    }
-  } else if (now - this->last_traffic_ > KEEPALIVE_TIMEOUT_MS) {
+  } else if (now - this->last_traffic_ > KEEPALIVE_TIMEOUT_MS && now > this->next_ping_retry_) {
    ESP_LOGVV(TAG, "Sending keepalive PING");
-    this->flags_.sent_ping = this->send_message(PingRequest());
-    if (!this->flags_.sent_ping) {
-      // If we can't send the ping request directly (tx_buffer full),
-      // schedule it at the front of the batch so it will be sent with priority
-      ESP_LOGW(TAG, "Buffer full, ping queued");
-      this->schedule_message_front_(nullptr, &APIConnection::try_send_ping_request, PingRequest::MESSAGE_TYPE);
-      this->flags_.sent_ping = true;  // Mark as sent to avoid scheduling multiple pings
+    this->sent_ping_ = this->send_message(PingRequest());
+    if (!this->sent_ping_) {
+      this->next_ping_retry_ = now + PING_RETRY_INTERVAL;
+      this->ping_retries_++;
+      if (this->ping_retries_ >= MAX_PING_RETRIES) {
+        on_fatal_error();
+        ESP_LOGE(TAG, "%s: Ping failed %u times", this->get_client_combined_info().c_str(), this->ping_retries_);
+      } else if (this->ping_retries_ >= 10) {
+        ESP_LOGW(TAG, "%s: Ping retry %u", this->get_client_combined_info().c_str(), this->ping_retries_);
+      } else {
+        ESP_LOGD(TAG, "%s: Ping retry %u", this->get_client_combined_info().c_str(), this->ping_retries_);
+      }
    }
  }

@ -225,13 +236,13 @@ DisconnectResponse APIConnection::disconnect(const DisconnectRequest &msg) {
  // don't close yet, we still need to send the disconnect response
  // close will happen on next loop
  ESP_LOGD(TAG, "%s disconnected", this->get_client_combined_info().c_str());
-  this->flags_.next_close = true;
+  this->next_close_ = true;
  DisconnectResponse resp;
  return resp;
 }
 void APIConnection::on_disconnect_response(const DisconnectResponse &value) {
  this->helper_->close();
-  this->flags_.remove = true;
+  this->remove_ = true;
 }

 // Encodes a message to the buffer and returns the total number of bytes used,
@ -1157,7 +1168,7 @@ void APIConnection::media_player_command(const MediaPlayerCommandRequest &msg) {

 #ifdef USE_ESP32_CAMERA
 void APIConnection::set_camera_state(std::shared_ptr<esp32_camera::CameraImage> image) {
-  if (!this->flags_.state_subscription)
+  if (!this->state_subscription_)
    return;
  if (this->image_reader_.available())
    return;
@ -1511,7 +1522,7 @@ void APIConnection::update_command(const UpdateCommandRequest &msg) {
 #endif

 bool APIConnection::try_send_log_message(int level, const char *tag, const char *line) {
-  if (this->flags_.log_subscription < level)
+  if (this->log_subscription_ < level)
    return false;

  // Pre-calculate message size to avoid reallocations
@ -1539,11 +1550,12 @@ bool APIConnection::try_send_log_message(int level, const char *tag, const char

 HelloResponse APIConnection::hello(const HelloRequest &msg) {
  this->client_info_ = msg.client_info;
+  this->client_peername_ = this->helper_->getpeername();
  this->helper_->set_log_info(this->get_client_combined_info());
  this->client_api_version_major_ = msg.api_version_major;
  this->client_api_version_minor_ = msg.api_version_minor;
  ESP_LOGV(TAG, "Hello from client: '%s' | %s | API Version %" PRIu32 ".%" PRIu32, this->client_info_.c_str(),
-           this->helper_->getpeername().c_str(), this->client_api_version_major_, this->client_api_version_minor_);
+           this->client_peername_.c_str(), this->client_api_version_major_, this->client_api_version_minor_);

  HelloResponse resp;
  resp.api_version_major = 1;
@ -1551,7 +1563,7 @@ HelloResponse APIConnection::hello(const HelloRequest &msg) {
  resp.server_info = App.get_name() + " (esphome v" ESPHOME_VERSION ")";
  resp.name = App.get_name();

-  this->flags_.connection_state = static_cast<uint8_t>(ConnectionState::CONNECTED);
+  this->connection_state_ = ConnectionState::CONNECTED;
  return resp;
 }
 ConnectResponse APIConnection::connect(const ConnectRequest &msg) {
@ -1562,8 +1574,8 @@ ConnectResponse APIConnection::connect(const ConnectRequest &msg) {
  resp.invalid_password = !correct;
  if (correct) {
    ESP_LOGD(TAG, "%s connected", this->get_client_combined_info().c_str());
-    this->flags_.connection_state = static_cast<uint8_t>(ConnectionState::AUTHENTICATED);
-    this->parent_->get_client_connected_trigger()->trigger(this->client_info_, this->helper_->getpeername());
+    this->connection_state_ = ConnectionState::AUTHENTICATED;
+    this->parent_->get_client_connected_trigger()->trigger(this->client_info_, this->client_peername_);
 #ifdef USE_HOMEASSISTANT_TIME
    if (homeassistant::global_homeassistant_time != nullptr) {
      this->send_time_request();
@ -1676,7 +1688,7 @@ void APIConnection::subscribe_home_assistant_states(const SubscribeHomeAssistant
  state_subs_at_ = 0;
 }
 bool APIConnection::try_to_clear_buffer(bool log_out_of_space) {
-  if (this->flags_.remove)
+  if (this->remove_)
    return false;
  if (this->helper_->can_write_without_blocking())
    return true;
@ -1726,7 +1738,7 @@ void APIConnection::on_no_setup_connection() {
 }
 void APIConnection::on_fatal_error() {
  this->helper_->close();
-  this->flags_.remove = true;
+  this->remove_ = true;
 }

 void APIConnection::DeferredBatch::add_item(EntityBase *entity, MessageCreator creator, uint16_t message_type) {
@ -1745,14 +1757,9 @@ void APIConnection::DeferredBatch::add_item(EntityBase *entity, MessageCreator c
  items.emplace_back(entity, std::move(creator), message_type);
 }

-void APIConnection::DeferredBatch::add_item_front(EntityBase *entity, MessageCreator creator, uint16_t message_type) {
-  // Insert at front for high priority messages (no deduplication check)
-  items.insert(items.begin(), BatchItem(entity, std::move(creator), message_type));
-}
-
 bool APIConnection::schedule_batch_() {
-  if (!this->flags_.batch_scheduled) {
-    this->flags_.batch_scheduled = true;
+  if (!this->deferred_batch_.batch_scheduled) {
+    this->deferred_batch_.batch_scheduled = true;
    this->deferred_batch_.batch_start_time = App.get_loop_component_start_time();
  }
  return true;
@ -1768,7 +1775,7 @@ ProtoWriteBuffer APIConnection::allocate_batch_message_buffer(uint16_t size) {

 void APIConnection::process_batch_() {
  if (this->deferred_batch_.empty()) {
-    this->flags_.batch_scheduled = false;
+    this->deferred_batch_.batch_scheduled = false;
    return;
  }

@ -1931,12 +1938,6 @@ uint16_t APIConnection::try_send_disconnect_request(EntityBase *entity, APIConne
  return encode_message_to_buffer(req, DisconnectRequest::MESSAGE_TYPE, conn, remaining_size, is_single);
 }

-uint16_t APIConnection::try_send_ping_request(EntityBase *entity, APIConnection *conn, uint32_t remaining_size,
-                                              bool is_single) {
-  PingRequest req;
-  return encode_message_to_buffer(req, PingRequest::MESSAGE_TYPE, conn, remaining_size, is_single);
-}
-
 uint16_t APIConnection::get_estimated_message_size(uint16_t message_type) {
  // Use generated ESTIMATED_SIZE constants from each message type
  switch (message_type) {
--- a/esphome/components/api/api_connection.h
+++ b/esphome/components/api/api_connection.h
@ -125,7 +125,7 @@ class APIConnection : public APIServerConnection {
 #endif
  bool try_send_log_message(int level, const char *tag, const char *line);
  void send_homeassistant_service_call(const HomeassistantServiceResponse &call) {
-    if (!this->flags_.service_call_subscription)
+    if (!this->service_call_subscription_)
      return;
    this->send_message(call);
  }
@ -185,7 +185,8 @@ class APIConnection : public APIServerConnection {
  void on_disconnect_response(const DisconnectResponse &value) override;
  void on_ping_response(const PingResponse &value) override {
    // we initiated ping
-    this->flags_.sent_ping = false;
+    this->ping_retries_ = 0;
+    this->sent_ping_ = false;
  }
  void on_home_assistant_state_response(const HomeAssistantStateResponse &msg) override;
 #ifdef USE_HOMEASSISTANT_TIME
@ -198,16 +199,16 @@ class APIConnection : public APIServerConnection {
  DeviceInfoResponse device_info(const DeviceInfoRequest &msg) override;
  void list_entities(const ListEntitiesRequest &msg) override { this->list_entities_iterator_.begin(); }
  void subscribe_states(const SubscribeStatesRequest &msg) override {
-    this->flags_.state_subscription = true;
+    this->state_subscription_ = true;
    this->initial_state_iterator_.begin();
  }
  void subscribe_logs(const SubscribeLogsRequest &msg) override {
-    this->flags_.log_subscription = msg.level;
+    this->log_subscription_ = msg.level;
    if (msg.dump_config)
      App.schedule_dump_config();
  }
  void subscribe_homeassistant_services(const SubscribeHomeassistantServicesRequest &msg) override {
-    this->flags_.service_call_subscription = true;
+    this->service_call_subscription_ = true;
  }
  void subscribe_home_assistant_states(const SubscribeHomeAssistantStatesRequest &msg) override;
  GetTimeResponse get_time(const GetTimeRequest &msg) override {
@ -219,12 +220,9 @@ class APIConnection : public APIServerConnection {
  NoiseEncryptionSetKeyResponse noise_encryption_set_key(const NoiseEncryptionSetKeyRequest &msg) override;
 #endif

-  bool is_authenticated() override {
-    return static_cast<ConnectionState>(this->flags_.connection_state) == ConnectionState::AUTHENTICATED;
-  }
+  bool is_authenticated() override { return this->connection_state_ == ConnectionState::AUTHENTICATED; }
  bool is_connection_setup() override {
-    return static_cast<ConnectionState>(this->flags_.connection_state) == ConnectionState::CONNECTED ||
-           this->is_authenticated();
+    return this->connection_state_ == ConnectionState ::CONNECTED || this->is_authenticated();
  }
  void on_fatal_error() override;
  void on_unauthenticated_access() override;
@ -278,12 +276,11 @@ class APIConnection : public APIServerConnection {
  bool send_buffer(ProtoWriteBuffer buffer, uint16_t message_type) override;

  std::string get_client_combined_info() const {
-    std::string peername = this->helper_->getpeername();
-    if (this->client_info_ == peername) {
+    if (this->client_info_ == this->client_peername_) {
      // Before Hello message, both are the same (just IP:port)
      return this->client_info_;
    }
-    return this->client_info_ + " (" + peername + ")";
+    return this->client_info_ + " (" + this->client_peername_ + ")";
  }

  // Buffer allocator methods for batch processing
@ -444,56 +441,37 @@ class APIConnection : public APIServerConnection {
  // Helper function to get estimated message size for buffer pre-allocation
  static uint16_t get_estimated_message_size(uint16_t message_type);

-  // Batch message method for ping requests
-  static uint16_t try_send_ping_request(EntityBase *entity, APIConnection *conn, uint32_t remaining_size,
-                                        bool is_single);
-
  // Pointers first (4 bytes each, naturally aligned)
  std::unique_ptr<APIFrameHelper> helper_;
  APIServer *parent_;

  // 4-byte aligned types
  uint32_t last_traffic_;
+  uint32_t next_ping_retry_{0};
  int state_subs_at_ = -1;

  // Strings (12 bytes each on 32-bit)
  std::string client_info_;
+  std::string client_peername_;

  // 2-byte aligned types
  uint16_t client_api_version_major_{0};
  uint16_t client_api_version_minor_{0};

-  // Connection state enum
-  enum class ConnectionState : uint8_t {
-    WAITING_FOR_HELLO = 0,
-    CONNECTED = 1,
-    AUTHENTICATED = 2,
-  };
-
  // Group all 1-byte types together to minimize padding
-  struct APIFlags {
-    uint8_t connection_state : 2;  // ConnectionState only needs 2 bits (3 states)
-    uint8_t log_subscription : 3;  // Log levels 0-7 need 3 bits
-    uint8_t remove : 1;
-    uint8_t state_subscription : 1;
-    uint8_t sent_ping : 1;
-
-    uint8_t service_call_subscription : 1;
-    uint8_t next_close : 1;
-    uint8_t batch_scheduled : 1;  // Moved from DeferredBatch
-    uint8_t reserved : 5;         // Reserved for future use
-
-    APIFlags()
-        : connection_state(0),
-          log_subscription(ESPHOME_LOG_LEVEL_NONE),
-          remove(0),
-          state_subscription(0),
-          sent_ping(0),
-          service_call_subscription(0),
-          next_close(0),
-          batch_scheduled(0),
-          reserved(0) {}
-  } flags_;  // 2 bytes total instead of 7+ bytes
+  enum class ConnectionState : uint8_t {
+    WAITING_FOR_HELLO,
+    CONNECTED,
+    AUTHENTICATED,
+  } connection_state_{ConnectionState::WAITING_FOR_HELLO};
+  uint8_t log_subscription_{ESPHOME_LOG_LEVEL_NONE};
+  bool remove_{false};
+  bool state_subscription_{false};
+  bool sent_ping_{false};
+  bool service_call_subscription_{false};
+  bool next_close_ = false;
+  uint8_t ping_retries_{0};
+  // 8 bytes used, no padding needed

  // Larger objects at the end
  InitialStateIterator initial_state_iterator_;
@ -611,6 +589,7 @@ class APIConnection : public APIServerConnection {

    std::vector<BatchItem> items;
    uint32_t batch_start_time{0};
+    bool batch_scheduled{false};

    DeferredBatch() {
      // Pre-allocate capacity for typical batch sizes to avoid reallocation
@ -619,10 +598,9 @@ class APIConnection : public APIServerConnection {

    // Add item to the batch
    void add_item(EntityBase *entity, MessageCreator creator, uint16_t message_type);
-    // Add item to the front of the batch (for high priority messages like ping)
-    void add_item_front(EntityBase *entity, MessageCreator creator, uint16_t message_type);
    void clear() {
      items.clear();
+      batch_scheduled = false;
      batch_start_time = 0;
    }
    bool empty() const { return items.empty(); }
@ -659,12 +637,6 @@ class APIConnection : public APIServerConnection {
  bool schedule_message_(EntityBase *entity, MessageCreatorPtr function_ptr, uint16_t message_type) {
    return schedule_message_(entity, MessageCreator(function_ptr), message_type);
  }
-
-  // Helper function to schedule a high priority message at the front of the batch
-  bool schedule_message_front_(EntityBase *entity, MessageCreatorPtr function_ptr, uint16_t message_type) {
-    this->deferred_batch_.add_item_front(entity, MessageCreator(function_ptr), message_type);
-    return this->schedule_batch_();
-  }
 };

 }  // namespace api
--- a/esphome/components/api/api_server.cpp
+++ b/esphome/components/api/api_server.cpp
@ -104,7 +104,7 @@ void APIServer::setup() {
        return;
      }
      for (auto &c : this->clients_) {
-        if (!c->flags_.remove)
+        if (!c->remove_)
          c->try_send_log_message(level, tag, message);
      }
    });
@ -116,7 +116,7 @@ void APIServer::setup() {
    esp32_camera::global_esp32_camera->add_image_callback(
        [this](const std::shared_ptr<esp32_camera::CameraImage> &image) {
          for (auto &c : this->clients_) {
-            if (!c->flags_.remove)
+            if (!c->remove_)
              c->set_camera_state(image);
          }
        });
@ -176,7 +176,7 @@ void APIServer::loop() {
  while (client_index < this->clients_.size()) {
    auto &client = this->clients_[client_index];

-    if (!client->flags_.remove) {
+    if (!client->remove_) {
      // Common case: process active client
      client->loop();
      client_index++;
@ -184,7 +184,7 @@ void APIServer::loop() {
    }

    // Rare case: handle disconnection
-    this->client_disconnected_trigger_->trigger(client->client_info_, client->helper_->getpeername());
+    this->client_disconnected_trigger_->trigger(client->client_info_, client->client_peername_);
    ESP_LOGV(TAG, "Remove connection %s", client->client_info_.c_str());

    // Swap with the last element and pop (avoids expensive vector shifts)
@ -502,7 +502,7 @@ bool APIServer::save_noise_psk(psk_t psk, bool make_active) {
 #ifdef USE_HOMEASSISTANT_TIME
 void APIServer::request_time() {
  for (auto &client : this->clients_) {
-    if (!client->flags_.remove && client->is_authenticated())
+    if (!client->remove_ && client->is_authenticated())
      client->send_time_request();
  }
 }
@ -526,8 +526,8 @@ void APIServer::on_shutdown() {
  for (auto &c : this->clients_) {
    if (!c->send_message(DisconnectRequest())) {
      // If we can't send the disconnect request directly (tx_buffer full),
-      // schedule it at the front of the batch so it will be sent with priority
-      c->schedule_message_front_(nullptr, &APIConnection::try_send_disconnect_request, DisconnectRequest::MESSAGE_TYPE);
+      // schedule it in the batch so it will be sent with the 5ms timer
+      c->schedule_message_(nullptr, &APIConnection::try_send_disconnect_request, DisconnectRequest::MESSAGE_TYPE);
    }
  }
 }