[nextion] Fix: Properly deliver numeric responses (#17744)

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

esphome/components/nextion/nextion.cpp

@@ -1,11 +1,13 @@
 #include "nextion.h"
 
 #include <cinttypes>
+#include <cstring>
 #include <new>
 
 #include "esphome/core/application.h"
 #include "esphome/core/helpers.h"
 #include "esphome/core/log.h"
+#include "esphome/core/progmem.h"
 #include "esphome/core/string_ref.h"
 #include "esphome/core/util.h"
 
@@ -18,6 +20,18 @@ static bool is_sleep_wake_command(const NextionComponentBase *component) {
   return component->get_queue_type() == NextionQueueType::NO_RESULT && component->get_variable_name() == "sleep_wake";
 }
 
+// The display answers a raw "get" with a value instead of 0x01. Its queue entry is named RAW_GET_NAME so that
+// value removes it. An entity with this variable name would clash when it writes its state; none is expected.
+static constexpr const char *RAW_GET_NAME = "raw_get";
+
+static bool is_raw_get_command(const NextionComponentBase *component) {
+  return component->get_queue_type() == NextionQueueType::NO_RESULT && component->get_variable_name() == RAW_GET_NAME;
+}
+
+static const char *raw_command_name(const char *command, const char *name) {
+  return ESPHOME_strncmp_P(command, ESPHOME_PSTR("get "), 4) == 0 ? RAW_GET_NAME : name;
+}
+
 // Nextion command terminator: three consecutive 0xFF bytes (per Nextion Instruction Set v1.1).
 static constexpr uint8_t COMMAND_DELIMITER[3] = {0xFF, 0xFF, 0xFF};
 static constexpr size_t DELIMITER_SIZE = sizeof(COMMAND_DELIMITER);
@@ -287,7 +301,7 @@ bool Nextion::send_command(const char *command) {
   if ((!this->is_setup() && !this->connection_state_.ignore_is_setup_) || this->is_sleeping())
     return false;
 
-  this->add_no_result_to_queue_with_command_("command", command);
+  this->add_no_result_to_queue_with_command_(raw_command_name(command, "command"), command);
   return true;
 }
 
@@ -305,7 +319,7 @@ bool Nextion::send_command_printf(const char *format, ...) {
     return false;
   }
 
-  this->add_no_result_to_queue_with_command_("command_printf", buffer);
+  this->add_no_result_to_queue_with_command_(raw_command_name(buffer, "command_printf"), buffer);
   return true;
 }
 
@@ -448,6 +462,75 @@ bool Nextion::remove_from_q_(bool report_empty) {
   return true;
 }
 
+void Nextion::deliver_queue_response_(const LogString *response_name, const NextionQueueResponse &resp) {
+  auto it = this->nextion_queue_.begin();
+  while (it != this->nextion_queue_.end()) {
+    NextionQueue *nb = *it;
+    if (nb == nullptr || nb->component == nullptr) {
+      ESP_LOGE(TAG, "Invalid queue entry");
+      if (nb != nullptr) {
+        this->release_queue_entry_(nb);
+      }
+      it = this->nextion_queue_.erase(it);
+      continue;
+    }
+    NextionComponentBase *component = nb->component;
+    NextionQueueType qtype = component->get_queue_type();
+    const bool raw_get = is_raw_get_command(component);
+
+    if (!nb->pending_command.empty()) {
+      ESP_LOGV(TAG, "%s: skipping unsent '%s'", LOG_STR_ARG(response_name), component->get_variable_name().c_str());
+      ++it;
+      continue;
+    }
+
+    if (qtype == NextionQueueType::NO_RESULT && !raw_get) {
+      // Leave in queue -- its 0x01 ACK will remove it via remove_from_q_().
+      ESP_LOGV(TAG, "%s: skipping NO_RESULT '%s'", LOG_STR_ARG(response_name), component->get_variable_name().c_str());
+      ++it;
+      continue;
+    }
+
+    // The display answers queries in order, so the response belongs to this query even when the type does
+    // not match. Consume only this entry and never hand the value to a later query.
+    if (raw_get) {
+      // A raw "get" from send_command() has no entity to update
+      ESP_LOGV(TAG, "%s for raw get dropped", LOG_STR_ARG(response_name));
+    } else if (!resp.is_string && (qtype == NextionQueueType::SENSOR || qtype == NextionQueueType::BINARY_SENSOR ||
+                                   qtype == NextionQueueType::SWITCH)) {
+      ESP_LOGN(TAG, "Numeric: %s type %d:%s val %d", component->get_variable_name().c_str(), qtype,
+               component->get_queue_type_string(), resp.int_value);
+      component->set_state_from_int(resp.int_value, true, false);
+    } else if (resp.is_string && qtype == NextionQueueType::TEXT_SENSOR) {
+      ESP_LOGN(TAG, "String resp: '%s' id: %s type: %s", resp.str_value->c_str(),
+               component->get_variable_name().c_str(), component->get_queue_type_string());
+      component->set_state_from_string(*resp.str_value, true, false);
+    } else {
+      ESP_LOGE(TAG, "%s but '%s' invalid type %d", LOG_STR_ARG(response_name), component->get_variable_name().c_str(),
+               qtype);
+    }
+
+    this->release_queue_entry_(nb);
+    this->nextion_queue_.erase(it);
+    return;
+  }
+  ESP_LOGE(TAG, "%s but no sent query in queue", LOG_STR_ARG(response_name));
+}
+
+void Nextion::set_numeric_return_(int value) {
+  NextionQueueResponse resp{};
+  resp.is_string = false;
+  resp.int_value = value;
+  this->deliver_queue_response_(LOG_STR("Numeric return"), resp);
+}
+
+void Nextion::set_string_return_(const std::string &value) {
+  NextionQueueResponse resp{};
+  resp.is_string = true;
+  resp.str_value = &value;
+  this->deliver_queue_response_(LOG_STR("String return"), resp);
+}
+
 void Nextion::process_serial_() {
   // Read all available bytes in batches to reduce UART call overhead.
   size_t avail = this->available();
@@ -653,28 +736,7 @@ void Nextion::process_nextion_commands_() {
           break;
         }
 
-        NextionQueue *nb = this->nextion_queue_.front();
-        if (!nb || !nb->component) {
-          ESP_LOGE(TAG, "Invalid queue entry");
-          if (nb != nullptr) {
-            this->release_queue_entry_(nb);
-          }
-          this->nextion_queue_.pop_front();
-          return;
-        }
-        NextionComponentBase *component = nb->component;
-
-        if (component->get_queue_type() != NextionQueueType::TEXT_SENSOR) {
-          ESP_LOGE(TAG, "String return but '%s' not text sensor", component->get_variable_name().c_str());
-        } else {
-          ESP_LOGN(TAG, "String resp: '%s' id: %s type: %s", to_process.c_str(), component->get_variable_name().c_str(),
-                   component->get_queue_type_string());
-          component->set_state_from_string(to_process, true, false);
-        }
-
-        this->release_queue_entry_(nb);
-        this->nextion_queue_.pop_front();
-
+        this->set_string_return_(to_process);
         break;
       }
         //  0x71 0x01 0x02 0x03 0x04 0xFF 0xFF 0xFF
@@ -696,31 +758,7 @@ void Nextion::process_nextion_commands_() {
 
         int value = static_cast<int>(encode_uint32(to_process[3], to_process[2], to_process[1], to_process[0]));
 
-        NextionQueue *nb = this->nextion_queue_.front();
-        if (!nb || !nb->component) {
-          ESP_LOGE(TAG, "Invalid queue");
-          if (nb != nullptr) {
-            this->release_queue_entry_(nb);
-          }
-          this->nextion_queue_.pop_front();
-          return;
-        }
-        NextionComponentBase *component = nb->component;
-
-        if (component->get_queue_type() != NextionQueueType::SENSOR &&
-            component->get_queue_type() != NextionQueueType::BINARY_SENSOR &&
-            component->get_queue_type() != NextionQueueType::SWITCH) {
-          ESP_LOGE(TAG, "Numeric return but '%s' invalid type %d", component->get_variable_name().c_str(),
-                   component->get_queue_type());
-        } else {
-          ESP_LOGN(TAG, "Numeric: %s type %d:%s val %d", component->get_variable_name().c_str(),
-                   component->get_queue_type(), component->get_queue_type_string(), value);
-          component->set_state_from_int(value, true, false);
-        }
-
-        this->release_queue_entry_(nb);
-        this->nextion_queue_.pop_front();
-
+        this->set_numeric_return_(value);
         break;
       }
 
@@ -1154,6 +1192,8 @@ void Nextion::add_no_result_to_queue_with_command_(const std::string &variable_n
   if ((!this->is_setup() && !this->connection_state_.ignore_is_setup_) || command.empty())
     return;
 
+  ESP_LOGV(TAG, "Queue NORESULT '%s': %s", variable_name.c_str(), command.c_str());
+
   if (this->send_command_(command)) {
     this->add_no_result_to_queue_(variable_name);
 #ifdef USE_NEXTION_COMMAND_SPACING

esphome/components/nextion/nextion.h

@@ -1066,6 +1066,7 @@ class Nextion final : public NextionBase, public PollingComponent, public uart::
 
   /**
    * Manually send a raw command to the display.
+   * A "get" command is accepted, but the returned value is dropped. Use a Nextion sensor or text sensor to read values.
    * @param command The pcommand, like "page 0"
    * @return Whether the send was successful.
    */
@@ -1073,6 +1074,7 @@ class Nextion final : public NextionBase, public PollingComponent, public uart::
 
   /**
    * Manually send a raw formatted command to the display.
+   * A "get" command is accepted, but the returned value is dropped. Use a Nextion sensor or text sensor to read values.
    * @param format The printf-style command format, like "vis %s,0"
    * @param ... The format arguments
    * @return Whether the send was successful.
@@ -1470,6 +1472,21 @@ class Nextion final : public NextionBase, public PollingComponent, public uart::
   void release_queue_entry_(NextionQueue *nb);
   NextionQueue *make_no_result_entry_(const std::string &variable_name);
 
+  /// Payload for deliver_queue_response_(): either a numeric (0x71) or string (0x70) return value.
+  struct NextionQueueResponse {
+    bool is_string;
+    int int_value;
+    const std::string *str_value;
+  };
+  /// Deliver a 0x70/0x71 response to the first query in the queue that was sent and expects a value, then
+  /// remove that query. NO_RESULT entries are skipped (their own 0x01 ACK removes them), as are entries whose
+  /// command is still waiting to be sent. Invalid entries are dropped. If the first query has the wrong type
+  /// (for example a numeric return for a TEXT_SENSOR), it is removed without a state update. A raw "get" sent
+  /// with send_command() counts as a query, and its value is dropped.
+  void deliver_queue_response_(const LogString *response_name, const NextionQueueResponse &resp);
+  void set_numeric_return_(int value);
+  void set_string_return_(const std::string &value);
+
   /**
    * @brief Status flags for Nextion display state management
    *

tests/components/nextion/__init__.py

@@ -0,0 +1,12 @@
+from tests.testing_helpers import ComponentManifestOverride
+
+
+def override_manifest(manifest: ComponentManifestOverride) -> None:
+    # nextion.h includes esphome/components/display/display.h and
+    # esphome/components/uart/uart.h.  Declare both as test dependencies
+    # so their headers are in the include path during C++ unit test builds.
+    deps = list(manifest.dependencies or [])
+    for dep in ("display", "uart"):
+        if dep not in deps:
+            deps.append(dep)
+    manifest.dependencies = deps

tests/components/nextion/deliver_queue_response_test.cpp

@@ -0,0 +1,243 @@
+#include <gtest/gtest.h>
+
+#include <list>
+#include <memory>
+#include <new>
+#include <string>
+#include <vector>
+
+#include "esphome/components/nextion/nextion.h"
+#include "esphome/components/uart/uart_component.h"
+#include "esphome/core/helpers.h"
+
+namespace esphome::nextion::testing {
+
+// Explicit instantiation skips access checks, which lets the test reach the queue and the response
+// handlers without making them public on the component.
+template<std::list<NextionQueue *> Nextion::*Q, void (Nextion::*N)(int), void (Nextion::*S)(const std::string &),
+         void (Nextion::*R)(NextionQueue *), bool NextionBase::*U>
+struct NextionAccess {
+  friend std::list<NextionQueue *> &queue(Nextion &nextion) { return nextion.*Q; }
+  friend void numeric_return(Nextion &nextion, int value) { (nextion.*N)(value); }
+  friend void string_return(Nextion &nextion, const std::string &value) { (nextion.*S)(value); }
+  friend void release_queue_entry(Nextion &nextion, NextionQueue *entry) { (nextion.*R)(entry); }
+  friend void mark_setup(Nextion &nextion) { nextion.*U = true; }
+};
+std::list<NextionQueue *> &queue(Nextion &nextion);
+void numeric_return(Nextion &nextion, int value);
+void string_return(Nextion &nextion, const std::string &value);
+void release_queue_entry(Nextion &nextion, NextionQueue *entry);
+void mark_setup(Nextion &nextion);
+template struct NextionAccess<&Nextion::nextion_queue_, &Nextion::set_numeric_return_, &Nextion::set_string_return_,
+                              &Nextion::release_queue_entry_, &Nextion::is_setup_>;
+
+/// Accepts the commands the component writes; the display never answers
+class FakeUART : public uart::UARTComponent {
+ public:
+  void write_array(const uint8_t *data, size_t len) override {}
+  bool peek_byte(uint8_t *data) override { return false; }
+  bool read_array(uint8_t *data, size_t len) override { return false; }
+  size_t available() override { return 0; }
+  uart::UARTFlushResult flush() override { return uart::UARTFlushResult::UART_FLUSH_RESULT_SUCCESS; }
+#if defined(USE_ESP8266) || defined(USE_ESP32)
+  void load_settings(bool dump_config) override {}
+#endif  // defined(USE_ESP8266) || defined(USE_ESP32)
+
+ protected:
+  void check_logger_conflict() override {}
+};
+
+/// Stands in for a sensor, switch or text sensor and records the value it receives
+class MockComponent : public NextionComponentBase {
+ public:
+  MockComponent(const std::string &name, NextionQueueType type) : type_(type) { this->set_variable_name(name); }
+
+  NextionQueueType get_queue_type() const override { return this->type_; }
+  void set_state_from_int(int state_value, bool publish, bool send_to_nextion) override {
+    this->received = true;
+    this->int_value = state_value;
+  }
+  void set_state_from_string(const std::string &state_value, bool publish, bool send_to_nextion) override {
+    this->received = true;
+    this->str_value = state_value;
+  }
+
+  bool received{false};
+  int int_value{0};
+  std::string str_value;
+
+ protected:
+  NextionQueueType type_;
+};
+
+class DeliverQueueResponse : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    this->nextion_.set_uart_parent(&this->uart_);
+    mark_setup(this->nextion_);
+  }
+
+  void TearDown() override {
+    for (auto *entry : queue(this->nextion_)) {
+      if (entry != nullptr)
+        release_queue_entry(this->nextion_, entry);
+    }
+    queue(this->nextion_).clear();
+  }
+
+  /// Queues a query the way add_to_get_queue() does. An unsent query still waits for command spacing.
+  MockComponent *add_query_(const std::string &name, NextionQueueType type, bool sent = true) {
+    auto *component = this->components_.emplace_back(std::make_unique<MockComponent>(name, type)).get();
+    auto *entry = RAMAllocator<NextionQueue>().allocate(1);
+    new (entry) NextionQueue;
+    entry->component = component;
+    if (!sent)
+      entry->pending_command = "get " + name;
+    queue(this->nextion_).push_back(entry);
+    return component;
+  }
+
+  /// Names of the queued entries, front first. send_command() names its entries "command", or "raw_get" for a "get".
+  std::vector<std::string> queued_() {
+    std::vector<std::string> names;
+    for (auto *entry : queue(this->nextion_)) {
+      if (entry == nullptr || entry->component == nullptr) {
+        names.emplace_back("<invalid>");
+      } else {
+        names.push_back(entry->component->get_variable_name());
+      }
+    }
+    return names;
+  }
+
+  FakeUART uart_;
+  Nextion nextion_;
+  std::vector<std::unique_ptr<MockComponent>> components_;
+};
+
+// The regression: a numeric return must not consume a command that is still waiting for its ACK
+TEST_F(DeliverQueueResponse, NumericSkipsNoResult) {
+  this->nextion_.send_command("page 0");
+  auto *sensor = this->add_query_("temp", NextionQueueType::SENSOR);
+
+  numeric_return(this->nextion_, 42);
+
+  EXPECT_TRUE(sensor->received);
+  EXPECT_EQ(sensor->int_value, 42);
+  EXPECT_EQ(this->queued_(), std::vector<std::string>{"command"});
+}
+
+TEST_F(DeliverQueueResponse, StringSkipsNoResult) {
+  this->nextion_.send_command("page 0");
+  auto *text = this->add_query_("status", NextionQueueType::TEXT_SENSOR);
+
+  string_return(this->nextion_, "online");
+
+  EXPECT_TRUE(text->received);
+  EXPECT_EQ(text->str_value, "online");
+  EXPECT_EQ(this->queued_(), std::vector<std::string>{"command"});
+}
+
+TEST_F(DeliverQueueResponse, NumericGoesToBinarySensorAndSwitch) {
+  auto *binary = this->add_query_("bin", NextionQueueType::BINARY_SENSOR);
+  auto *sw = this->add_query_("sw", NextionQueueType::SWITCH);
+
+  numeric_return(this->nextion_, 1);
+  EXPECT_TRUE(binary->received);
+  EXPECT_EQ(binary->int_value, 1);
+  EXPECT_FALSE(sw->received);
+
+  numeric_return(this->nextion_, 0);
+  EXPECT_TRUE(sw->received);
+  EXPECT_EQ(sw->int_value, 0);
+  EXPECT_TRUE(this->queued_().empty());
+}
+
+// With command spacing, a query can be queued before its command is sent; the display cannot be answering it yet
+TEST_F(DeliverQueueResponse, NumericSkipsUnsentQuery) {
+  auto *unsent = this->add_query_("unsent", NextionQueueType::SENSOR, false);
+  auto *sent = this->add_query_("sent", NextionQueueType::SENSOR);
+
+  numeric_return(this->nextion_, 7);
+
+  EXPECT_FALSE(unsent->received);
+  EXPECT_TRUE(sent->received);
+  EXPECT_EQ(this->queued_(), std::vector<std::string>{"unsent"});
+}
+
+TEST_F(DeliverQueueResponse, NumericWithoutSentQueryLeavesQueue) {
+  this->nextion_.send_command("page 0");
+  auto *unsent = this->add_query_("temp", NextionQueueType::SENSOR, false);
+
+  numeric_return(this->nextion_, 99);
+
+  EXPECT_FALSE(unsent->received);
+  EXPECT_EQ(this->queued_(), (std::vector<std::string>{"command", "temp"}));
+}
+
+// The display answers queries in order, so a wrong-type return still belongs to the first sent query.
+// It must consume only that query and never hand the value to a later one.
+TEST_F(DeliverQueueResponse, WrongTypeConsumesOnlyFirstQuery) {
+  auto *text = this->add_query_("status", NextionQueueType::TEXT_SENSOR);
+  auto *sensor = this->add_query_("temp", NextionQueueType::SENSOR);
+
+  numeric_return(this->nextion_, 100);
+
+  EXPECT_FALSE(text->received);
+  EXPECT_FALSE(sensor->received);
+  EXPECT_EQ(this->queued_(), std::vector<std::string>{"temp"});
+}
+
+TEST_F(DeliverQueueResponse, StringWrongTypeConsumesOnlyFirstQuery) {
+  auto *sensor = this->add_query_("temp", NextionQueueType::SENSOR);
+  auto *text = this->add_query_("status", NextionQueueType::TEXT_SENSOR);
+
+  string_return(this->nextion_, "online");
+
+  EXPECT_FALSE(sensor->received);
+  EXPECT_FALSE(text->received);
+  EXPECT_EQ(this->queued_(), std::vector<std::string>{"status"});
+}
+
+TEST_F(DeliverQueueResponse, InvalidEntriesAreDropped) {
+  auto *no_component = RAMAllocator<NextionQueue>().allocate(1);
+  new (no_component) NextionQueue;
+  queue(this->nextion_).push_back(nullptr);
+  queue(this->nextion_).push_back(no_component);
+  auto *sensor = this->add_query_("temp", NextionQueueType::SENSOR);
+
+  numeric_return(this->nextion_, 5);
+
+  EXPECT_TRUE(sensor->received);
+  EXPECT_TRUE(this->queued_().empty());
+}
+
+// The display answers a raw "get" with a value instead of 0x01, so that value removes it.
+// The value must not reach a later query, and no orphan entry may stay behind for later ACKs to remove.
+TEST_F(DeliverQueueResponse, RawGetTakesItsOwnValue) {
+  this->nextion_.send_command("get n0.val");
+  this->nextion_.send_command_printf("get %s", "t0.txt");
+  auto *sensor = this->add_query_("temp", NextionQueueType::SENSOR);
+  auto *text = this->add_query_("status", NextionQueueType::TEXT_SENSOR);
+  EXPECT_EQ(this->queued_(), (std::vector<std::string>{"raw_get", "raw_get", "temp", "status"}));
+
+  numeric_return(this->nextion_, 3);
+  string_return(this->nextion_, "raw");
+
+  EXPECT_FALSE(sensor->received);
+  EXPECT_FALSE(text->received);
+  EXPECT_EQ(this->queued_(), (std::vector<std::string>{"temp", "status"}));
+}
+
+// A raw "get" is found by its entry name; a query from a user entity with the same name is still a normal query
+TEST_F(DeliverQueueResponse, QueryNamedRawGetIsNotRawGet) {
+  auto *sensor = this->add_query_("raw_get", NextionQueueType::SENSOR);
+
+  numeric_return(this->nextion_, 8);
+
+  EXPECT_TRUE(sensor->received);
+  EXPECT_EQ(sensor->int_value, 8);
+  EXPECT_TRUE(this->queued_().empty());
+}
+
+}  // namespace esphome::nextion::testing