From 6f07b54772f134a6cfa5ecae08979a2f8189856f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 27 Jun 2025 06:30:42 -0500 Subject: [PATCH] cleanup --- esphome/components/api/api_connection.cpp | 75 +++++++++++---------- esphome/components/api/api_connection.h | 82 ++++++++--------------- esphome/components/api/api_server.cpp | 14 ++-- 3 files changed, 72 insertions(+), 99 deletions(-) diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index 0a82566bf6..fdcce6088c 100644 --- 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 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(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(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) { diff --git a/esphome/components/api/api_connection.h b/esphome/components/api/api_connection.h index 8172fcfe7b..e872711e95 100644 --- 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(this->flags_.connection_state) == ConnectionState::AUTHENTICATED; - } + bool is_authenticated() override { return this->connection_state_ == ConnectionState::AUTHENTICATED; } bool is_connection_setup() override { - return static_cast(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 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 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 diff --git a/esphome/components/api/api_server.cpp b/esphome/components/api/api_server.cpp index 2b0a41a780..583837af82 100644 --- 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 &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); } } }