Merge branch 'remote-base-dumper-table-v2' into remote-base-flash-dumpers-v2

# Conflicts:
#	tests/component_tests/remote_receiver/test_slot_counts.py

esphome/components/remote_base/__init__.py

@@ -1,4 +1,6 @@
 from collections.abc import Callable
+import dataclasses
+from dataclasses import dataclass
 from pathlib import Path
 from typing import Any
 
@@ -53,7 +55,7 @@ from esphome.const import (
     CONF_WAND_ID,
     CONF_ZERO,
 )
-from esphome.core import ID, coroutine
+from esphome.core import CORE, ID, CoroPriority, coroutine, coroutine_with_priority
 from esphome.cpp_generator import MockObj
 from esphome.schema_extractors import SCHEMA_EXTRACT, schema_extractor
 from esphome.types import ConfigType, SafeExpType, TemplateArgsType
@@ -115,9 +117,10 @@ REMOTE_TRANSMITTABLE_SCHEMA = cv.Schema(
 )
 
 
-# Listener and dumper lists are StaticVectors sized from these counts, so every registration
-# must go through add_listener / add_dumper. Every receiver's list gets the same capacity, so
-# the slots are keyed by receiver and the define is the largest count any one receiver needs.
+# Listener lists are StaticVectors sized from these counts, so every registration must go
+# through add_listener. Every receiver's list gets the same capacity, so the slots are keyed by
+# receiver and the define is the largest count any one receiver needs. Dumpers go into an exact
+# per receiver table; their define only marks that dumper storage and register_dumper are compiled in.
 LISTENER_COUNT_DEFINE = "REMOTE_BASE_LISTENER_COUNT"
 DUMPER_COUNT_DEFINE = "REMOTE_BASE_DUMPER_COUNT"
 
@@ -131,9 +134,43 @@ def add_listener(receiver: MockObj, listener: MockObj) -> None:
     cg.add(receiver.register_listener(listener))
 
 
+@dataclass
+class _RemoteBaseData:
+    # receiver id -> (receiver, its dumpers in registration order)
+    tables: dict[str, tuple[MockObj, list[MockObj]]] = dataclasses.field(
+        default_factory=dict
+    )
+    # dumper type -> its one flash object, shared by every receiver that lists it
+    flash_dumpers: dict[str, str] = dataclasses.field(default_factory=dict)
+
+
+def _get_data() -> _RemoteBaseData:
+    return CORE.data.setdefault(DOMAIN, _RemoteBaseData())
+
+
+def _get_dumper_tables() -> dict[str, tuple[MockObj, list[MockObj]]]:
+    return _get_data().tables
+
+
+@coroutine_with_priority(CoroPriority.FINAL)
+async def _emit_dumper_tables() -> None:
+    for receiver, dumpers in _get_dumper_tables().values():
+        table = cg.shared_progmem_array(
+            "remote_dumpers",
+            RemoteReceiverDumperBase.operator("ptr"),
+            cg.ArrayInitializer(*dumpers),
+            constexpr=False,
+        )
+        cg.add(receiver.set_dumpers(table, len(dumpers)))
+
+
 def add_dumper(receiver: MockObj, dumper: MockObj) -> None:
+    # Raises once the define is written, so a late dumper can't miss the tables
     _request_dumper_slot(str(receiver))
-    cg.add(receiver.register_dumper(dumper))
+    receivers = _get_dumper_tables()
+    if not receivers:
+        CORE.add_job(_emit_dumper_tables)
+    receivers.setdefault(str(receiver), (receiver, []))[1].append(dumper)
 
 
 async def register_listener(var: MockObj, config: ConfigType) -> None:
@@ -233,7 +270,32 @@ def register_trigger(name, type, data_type):
     return decorator
 
 
+def _new_flash_dumper(dumper_id: ID) -> cg.RawExpression:
+    """Place a stateless dumper in flash; the receiver only ever calls it."""
+    flash_dumpers = _get_data().flash_dumpers
+    if (storage := flash_dumpers.get(str(dumper_id.type))) is None:
+        # A generated name a config id cannot take; registered so later globals avoid it too
+        storage_id = cg.unique_global_id(f"{dumper_id.id}__flash", dumper_id.type)
+        storage = flash_dumpers[str(dumper_id.type)] = storage_id.id
+        CORE.register_variable(storage_id, cg.MockObj(storage))
+        CORE.add_global_after_includes(
+            cg.RawStatement(
+                f"static_assert(sizeof({dumper_id.type}) == sizeof(void *) && "
+                f"std::is_trivially_destructible_v<{dumper_id.type}>, "
+                '"a dumper kept in flash must have no data members or destructor");'
+            )
+        )
+        CORE.add_global_after_includes(
+            cg.RawStatement(f"static constexpr {dumper_id.type} {storage} PROGMEM{{}};")
+        )
+    # A constant address for the receiver's flash table; lambdas get a pointer-like object
+    address = f"const_cast<{dumper_id.type} *>(&{storage})"
+    CORE.register_variable(dumper_id, cg.MockObj(address, "->"))
+    return cg.RawExpression(address)
+
+
 def register_dumper(name, type, schema=None):
+    """Dumpers are stateless, so each is a flash object; one with data members fails the build."""
     if schema is None:
         schema = {}
     registerer = DUMPER_REGISTRY.register(name, type, schema)
@@ -241,7 +303,7 @@ def register_dumper(name, type, schema=None):
     def decorator(func):
         async def new_func(config, dumper_id):
             _request_protocol_if_in_tree(name)
-            var = cg.new_Pvariable(dumper_id)
+            var = _new_flash_dumper(dumper_id)
             await coroutine(func)(var, config)
             return var
 

esphome/components/remote_base/remote_base.cpp

@@ -103,8 +103,7 @@ bool RemoteReceiverBinarySensorBase::on_receive(RemoteReceiveData src) {
 #ifdef REMOTE_BASE_LISTENER_COUNT
 void RemoteReceiverBase::register_listener(RemoteReceiverListener *listener) {
   if (this->listeners_.size() == REMOTE_BASE_LISTENER_COUNT) {
-    ESP_LOGE(TAG, "No %s slot: register it from to_code() with remote_base.add_%s", LOG_STR_LITERAL("listener"),
-             LOG_STR_LITERAL("listener"));
+    ESP_LOGE(TAG, "No listener slot: register it from to_code() with remote_base.add_listener");
     return;
   }
   this->listeners_.push_back(listener);
@@ -112,18 +111,25 @@ void RemoteReceiverBase::register_listener(RemoteReceiverListener *listener) {
 #endif
 
 #ifdef REMOTE_BASE_DUMPER_COUNT
-void RemoteReceiverBase::register_dumper(RemoteReceiverDumperBase *dumper) {
-  if (dumper->is_secondary()) {
-    if (this->secondary_dumper_ == nullptr) {
-      this->secondary_dumper_ = dumper;
-      return;
-    }
-  } else if (this->dumpers_.size() != REMOTE_BASE_DUMPER_COUNT) {
-    this->dumpers_.push_back(dumper);
-    return;
+void RemoteReceiverBase::set_dumpers(RemoteReceiverDumperBase *const *dumpers, size_t count) {
+  if (this->dumpers_.empty()) {
+    this->dumpers_.assign_static(dumpers, count);
+  } else {
+    // register_dumper() ran first from another to_code(); the YAML dumpers stay first
+    this->dumpers_.prepend_copy(dumpers, count);
+  }
+}
+
+void RemoteReceiverBase::register_dumper(RemoteReceiverDumperBase *dumper) { this->dumpers_.append_copy(&dumper, 1); }
+
+// Only the first secondary dumper runs; from dump_config() so it is seen however the dumpers were registered
+void RemoteReceiverBase::warn_extra_secondaries_() {
+  uint8_t secondaries = 0;
+  for (auto *dumper : this->dumpers_)
+    secondaries += dumper->is_secondary();
+  if (secondaries > 1) {
+    ESP_LOGW(TAG, "Only the first of %u secondary dumpers runs", secondaries);
   }
-  ESP_LOGE(TAG, "No %s slot: register it from to_code() with remote_base.add_%s", LOG_STR_LITERAL("dumper"),
-           LOG_STR_LITERAL("dumper"));
 }
 #endif
 
@@ -134,12 +140,17 @@ void RemoteReceiverBase::call_listeners_dumpers_() {
 #endif
 #ifdef REMOTE_BASE_DUMPER_COUNT
   bool success = false;
+  RemoteReceiverDumperBase *secondary = nullptr;
   for (auto *dumper : this->dumpers_) {
-    if (dumper->dump(RemoteReceiveData(this->temp_, this->tolerance_, this->tolerance_mode_)))
+    if (dumper->is_secondary()) {
+      if (secondary == nullptr)
+        secondary = dumper;
+    } else if (dumper->dump(RemoteReceiveData(this->temp_, this->tolerance_, this->tolerance_mode_))) {
       success = true;
+    }
   }
-  if (!success && this->secondary_dumper_ != nullptr)
-    this->secondary_dumper_->dump(RemoteReceiveData(this->temp_, this->tolerance_, this->tolerance_mode_));
+  if (!success && secondary != nullptr)
+    secondary->dump(RemoteReceiveData(this->temp_, this->tolerance_, this->tolerance_mode_));
 #endif
 }
 

esphome/components/remote_base/remote_base.h

@@ -337,8 +337,7 @@ class RemoteReceiverDumperBase {
 class RemoteReceiverBase : public RemoteComponentBase {
  public:
   RemoteReceiverBase(InternalGPIOPin *pin) : RemoteComponentBase(pin) {}
-  // Slots are counted at code generation; without one the call fails at compile time with the same message
-  // the runtime check logs
+  // Listener slots are counted at code generation; without one the call fails at compile time
 #ifdef REMOTE_BASE_LISTENER_COUNT
   void register_listener(RemoteReceiverListener *listener);
 #else
@@ -347,6 +346,9 @@ class RemoteReceiverBase : public RemoteComponentBase {
   }
 #endif
 #ifdef REMOTE_BASE_DUMPER_COUNT
+  /// Codegen only (remote_base.add_dumper): the table must outlive this receiver.
+  void set_dumpers(RemoteReceiverDumperBase *const *dumpers, size_t count);
+  /// For C++ registration, also from setup(); each call copies the list to the heap.
   void register_dumper(RemoteReceiverDumperBase *dumper);
 #else
   template<typename T> void register_dumper(T *) {
@@ -359,14 +361,17 @@ class RemoteReceiverBase : public RemoteComponentBase {
   }
 
  protected:
+#ifdef REMOTE_BASE_DUMPER_COUNT
+  void warn_extra_secondaries_();
+#endif
   void call_listeners_dumpers_();
 
 #ifdef REMOTE_BASE_LISTENER_COUNT
   StaticVector<RemoteReceiverListener *, REMOTE_BASE_LISTENER_COUNT> listeners_;
 #endif
 #ifdef REMOTE_BASE_DUMPER_COUNT
-  StaticVector<RemoteReceiverDumperBase *, REMOTE_BASE_DUMPER_COUNT> dumpers_;
-  RemoteReceiverDumperBase *secondary_dumper_{nullptr};  // runs only when no primary dumper matched
+  // The first secondary dumper runs only when no primary dumper matched
+  ConstVector<RemoteReceiverDumperBase *, true> dumpers_;
 #endif
   RawTimings temp_;
   uint32_t tolerance_{25};

esphome/components/remote_receiver/remote_receiver.cpp

@@ -88,6 +88,9 @@ void RemoteReceiverComponent::dump_config() {
       (this->tolerance_mode_ == remote_base::TOLERANCE_MODE_TIME) ? LOG_STR_LITERAL(" us") : LOG_STR_LITERAL("%"),
       this->filter_us_, this->idle_us_);
   LOG_PIN("  Pin: ", this->pin_);
+#ifdef REMOTE_BASE_DUMPER_COUNT
+  this->warn_extra_secondaries_();
+#endif
 }
 
 void RemoteReceiverComponent::loop() {

esphome/components/remote_receiver/remote_receiver_rmt.cpp

@@ -140,6 +140,9 @@ void RemoteReceiverComponent::dump_config() {
       (this->tolerance_mode_ == remote_base::TOLERANCE_MODE_TIME) ? LOG_STR_LITERAL(" us") : LOG_STR_LITERAL("%"),
       this->carrier_frequency_, this->carrier_duty_percent_, this->filter_us_, this->idle_us_);
   LOG_PIN("  Pin: ", this->pin_);
+#ifdef REMOTE_BASE_DUMPER_COUNT
+  this->warn_extra_secondaries_();
+#endif
 }
 
 void RemoteReceiverComponent::loop() {

tests/component_tests/remote_receiver/config/receiver_dumper_type_id.yaml

@@ -0,0 +1,21 @@
+esphome:
+  name: test
+  on_boot:
+    - lambda: |-
+        ESP_LOGD("test", "secondary %d", id(my_nec).is_secondary());
+
+esp32:
+  board: esp32dev
+
+logger:
+
+remote_receiver:
+  - id: ir_rcvr
+    pin: GPIO4
+    dump:
+      - nec:
+        type_id: my_nec
+  - id: my_nec__flash
+    pin: GPIO5
+    dump:
+      - rc_switch

tests/component_tests/remote_receiver/test_slot_counts.py

@@ -12,6 +12,7 @@ from esphome.automation import ACTION_REGISTRY
 from esphome.components import remote_base
 import esphome.config_validation as cv
 from esphome.core import CORE
+from esphome.cpp_generator import MockObj
 
 from ..helpers import get_define_value
 
@@ -148,12 +149,69 @@ def test_dumper_count_is_the_busiest_receivers(
 
 
 @pytest.mark.usefixtures("restore_protocol_registries")
-def test_external_dumpers_are_constructed_in_ram(
+def test_external_dumpers_live_in_flash(
     generate_main: Callable[[str | Path], str],
     component_config_path: Callable[[str], Path],
 ) -> None:
-    """A dumper registered outside remote_base may hold state."""
+    """External protocol dumpers are stateless too; the size check guards one that is not."""
     main_cpp = generate_main(
         component_config_path("receiver_with_external_protocol.yaml")
     )
-    assert re.search(r"new\(\w+\) fake_protocol::FakeDumper\(\);", main_cpp)
+    assert re.search(
+        r"static constexpr fake_protocol::FakeDumper \w+ PROGMEM\{\};", main_cpp
+    )
+    assert (
+        "static_assert(sizeof(fake_protocol::FakeDumper) == sizeof(void *)" in main_cpp
+    )
+
+
+def test_each_receiver_gets_its_own_dumper_table(
+    generate_main: Callable[[str | Path], str],
+    component_config_path: Callable[[str], Path],
+) -> None:
+    main_cpp = generate_main(component_config_path("receiver_two_dumper_lists.yaml"))
+    assert "register_dumper" not in main_cpp
+    assert re.search(r"ir_rcvr->set_dumpers\(remote_dumpers\w*, 3\);", main_cpp)
+    assert re.search(r"rf_rcvr->set_dumpers\(remote_dumpers\w*, 1\);", main_cpp)
+
+
+def test_add_dumper_after_the_tables_were_emitted_raises(
+    generate_main: Callable[[str | Path], str],
+    component_config_path: Callable[[str], Path],
+) -> None:
+    """The tables are written once at FINAL; a later dumper must raise, not be dropped."""
+    generate_main(component_config_path("receiver_two_dumper_lists.yaml"))
+    with pytest.raises(ValueError, match="after the count define was emitted"):
+        remote_base.add_dumper(MockObj("rcvr", "->"), MockObj("dumper", "->"))
+
+
+def test_in_tree_dumpers_live_in_flash(
+    generate_main: Callable[[str | Path], str],
+    component_config_path: Callable[[str], Path],
+) -> None:
+    main_cpp = generate_main(component_config_path("receiver_two_dumper_lists.yaml"))
+    assert re.search(
+        r"static constexpr remote_base::NECDumper \w+ PROGMEM\{\};", main_cpp
+    )
+    assert not re.search(r"new\(\w+\) remote_base::\w+Dumper\(\);", main_cpp)
+    # A dumper that gains a member must fail the build, not fault on the device
+    assert "static_assert(sizeof(remote_base::NECDumper) == sizeof(void *)" in main_cpp
+    # rc_switch is on both receivers but has one flash object and one size check
+    assert (
+        len(re.findall(r"static constexpr remote_base::RCSwitchDumper ", main_cpp)) == 1
+    )
+    assert main_cpp.count("static_assert(sizeof(remote_base::RCSwitchDumper)") == 1
+
+
+def test_a_dumper_id_works_in_lambdas_and_cannot_collide(
+    generate_main: Callable[[str | Path], str],
+    component_config_path: Callable[[str], Path],
+) -> None:
+    """A user type_id is usable from a lambda, and a config id cannot take the flash storage name."""
+    main_cpp = generate_main(component_config_path("receiver_dumper_type_id.yaml"))
+    assert "const_cast<remote_base::NECDumper *>(&my_nec__flash_" in main_cpp
+    assert "->is_secondary()" in main_cpp
+    assert re.search(
+        r"static constexpr remote_base::NECDumper my_nec__flash_\w+ PROGMEM\{\};",
+        main_cpp,
+    )

tests/components/remote_base/receiver_dumpers_test.cpp

@@ -73,4 +73,37 @@ TEST(RemoteReceiverDumpers, RegisteringAfterConstructionAddsTheDumper) {
   EXPECT_EQ(calls, (std::vector<int>{1, 1, 2}));
 }
 
+TEST(RemoteReceiverDumpers, CodegenTableKeepsTheSameDispatch) {
+  std::vector<int> calls;
+  RecordingDumper raw(&calls, 0, true, true), miss(&calls, 1, false), extra_raw(&calls, 9, true, true);
+  RemoteReceiverDumperBase *const table[] = {&raw, &miss, &extra_raw};
+  TestReceiver receiver;
+  receiver.set_dumpers(table, 3);
+  receiver.receive();
+  EXPECT_EQ(calls, (std::vector<int>{1, 0}));
+}
+
+TEST(RemoteReceiverDumpers, RegisterDumperKeepsTheCodegenTable) {
+  std::vector<int> calls;
+  RecordingDumper from_table(&calls, 1, false), early(&calls, 2, false), late(&calls, 3, false);
+  TestReceiver receiver;
+  receiver.register_dumper(&early);  // an external component registering from to_code()
+  RemoteReceiverDumperBase *const table[] = {&from_table};
+  receiver.set_dumpers(table, 1);
+  receiver.register_dumper(&late);  // or from its own setup()
+  receiver.receive();
+  EXPECT_EQ(calls, (std::vector<int>{1, 2, 3}));  // the YAML table first, as before the table existed
+}
+
+TEST(RemoteReceiverDumpers, RawFromTheTableStaysTheSecondaryOverAnEarlierOne) {
+  std::vector<int> calls;
+  RecordingDumper raw(&calls, 1, false, true), external(&calls, 2, false, true);
+  TestReceiver receiver;
+  receiver.register_dumper(&external);  // an external secondary registered from to_code()
+  RemoteReceiverDumperBase *const table[] = {&raw};
+  receiver.set_dumpers(table, 1);
+  receiver.receive();
+  EXPECT_EQ(calls, (std::vector<int>{1}));
+}
+
 }  // namespace esphome::remote_base::testing