[sx127x] Keep constant send_packet payloads in shared flash tables (#20275)

esphome/components/sx127x/__init__.py

@@ -135,6 +135,11 @@ def validate_raw_data(value: Any) -> bytes | list[int]:
     )
 
 
+MAX_PACKET_SIZE = 255
+# The radio's payload length register is 8 bits, and empty packets are rejected.
+validate_packet_data = cv.All(validate_raw_data, cv.Length(min=1, max=MAX_PACKET_SIZE))
+
+
 def validate_config(config: ConfigType) -> ConfigType:
     if config[CONF_MODULATION] == "LORA":
         bws = [
@@ -198,7 +203,9 @@ CONFIG_SCHEMA = (
             cv.Optional(CONF_PA_POWER, default=17): cv.int_range(min=0, max=17),
             cv.Optional(CONF_PA_RAMP, default="40us"): cv.enum(RAMP),
             cv.Optional(CONF_PACKET_MODE): cv.boolean,
-            cv.Optional(CONF_PAYLOAD_LENGTH, default=0): cv.int_range(min=0, max=256),
+            cv.Optional(CONF_PAYLOAD_LENGTH, default=0): cv.int_range(
+                min=0, max=MAX_PACKET_SIZE
+            ),
             cv.Optional(CONF_PREAMBLE_DETECT, default=0): cv.int_range(min=0, max=3),
             cv.Optional(CONF_PREAMBLE_ERRORS, default=0): cv.int_range(min=0, max=31),
             cv.Optional(CONF_PREAMBLE_POLARITY, default=0xAA): cv.All(
@@ -286,7 +293,7 @@ for _name, _call in (
 SEND_PACKET_ACTION_SCHEMA = cv.maybe_simple_value(
     {
         cv.GenerateID(): cv.use_id(SX127x),
-        cv.Required(CONF_DATA): cv.templatable(validate_raw_data),
+        cv.Required(CONF_DATA): cv.templatable(validate_packet_data),
     },
     key=CONF_DATA,
 )
@@ -306,15 +313,11 @@ async def send_packet_action_to_code(
 ) -> MockObj:
     var = cg.new_Pvariable(action_id, template_arg)
     await cg.register_parented(var, config[CONF_ID])
-    data = config[CONF_DATA]
-    if isinstance(data, bytes):
-        data = list(data)
-    if cg.is_template(data):
-        templ = await cg.templatable(data, args, cg.std_vector.template(cg.uint8))
-        cg.add(var.set_data_template(templ))
-    else:
-        # Generate static array in flash to avoid RAM copy
-        arr_id = ID(f"{action_id}_data", is_declaration=True, type=cg.uint8)
-        arr = cg.static_const_array(arr_id, cg.ArrayInitializer(*data))
-        cg.add(var.set_data_static(arr, len(data)))
+    await automation.templatable_bytes(
+        config[CONF_DATA],
+        args,
+        var.set_data_template,
+        var.set_data_static,
+        "sx127x_data",
+    )
     return var

esphome/components/sx127x/automation.h

@@ -7,35 +7,13 @@
 namespace esphome::sx127x {
 
 template<typename... Ts> class SendPacketAction final : public Action<Ts...>, public Parented<SX127x> {
+  TEMPLATABLE_BYTES(data)
+
  public:
-  void set_data_template(std::vector<uint8_t> (*func)(Ts...)) {
-    this->data_.func = func;
-    this->len_ = -1;  // Sentinel value indicates template mode
-  }
-
-  void set_data_static(const uint8_t *data, size_t len) {
-    this->data_.data = data;
-    this->len_ = len;  // Length >= 0 indicates static mode
-  }
-
   void play(const Ts &...x) override {
-    std::vector<uint8_t> data;
-    if (this->len_ >= 0) {
-      // Static mode: copy from flash to vector
-      data.assign(this->data_.data, this->data_.data + this->len_);
-    } else {
-      // Template mode: call function
-      data = this->data_.func(x...);
-    }
-    this->parent_->transmit_packet(data);
+    this->data_.template visit<SX127X_MAX_PACKET_SIZE>(
+        [this](const uint8_t *data, size_t len) { this->parent_->transmit_packet(data, len); }, x...);
   }
-
- protected:
-  ssize_t len_{-1};  // -1 = template mode, >=0 = static mode with length
-  union Data {
-    std::vector<uint8_t> (*func)(Ts...);  // Function pointer (stateless lambdas)
-    const uint8_t *data;                  // Pointer to static data in flash
-  } data_;
 };
 
 }  // namespace esphome::sx127x

esphome/components/sx127x/sx127x.cpp

@@ -43,11 +43,11 @@ void SX127x::read_fifo_(std::vector<uint8_t> &packet) {
   this->disable();
 }
 
-void SX127x::write_fifo_(const std::vector<uint8_t> &packet) {
+void SX127x::write_fifo_(const uint8_t *data, size_t len) {
   this->enable();
   this->write_byte(REG_FIFO | 0x80);
-  for (const auto &byte : packet) {
-    this->transfer_byte(byte);
+  for (size_t i = 0; i < len; i++) {
+    this->transfer_byte(data[i]);
   }
   this->disable();
 }
@@ -250,18 +250,17 @@ size_t SX127x::get_max_packet_size() {
     return this->payload_length_;
   }
   if (this->modulation_ == MOD_LORA) {
-    return 256;
-  } else {
-    return 64;
+    return SX127X_MAX_PACKET_SIZE;
   }
+  return 64;
 }
 
-SX127xError SX127x::transmit_packet(const std::vector<uint8_t> &packet) {
-  if (this->payload_length_ > 0 && this->payload_length_ != packet.size()) {
+SX127xError SX127x::transmit_packet(const uint8_t *data, size_t len) {
+  if (this->payload_length_ > 0 && this->payload_length_ != len) {
     ESP_LOGE(TAG, "Packet size does not match config");
     return SX127xError::INVALID_PARAMS;
   }
-  if (packet.empty() || packet.size() > this->get_max_packet_size()) {
+  if (len == 0 || len > this->get_max_packet_size()) {
     ESP_LOGE(TAG, "Packet size out of range");
     return SX127xError::INVALID_PARAMS;
   }
@@ -275,18 +274,18 @@ SX127xError SX127x::transmit_packet(const std::vector<uint8_t> &packet) {
   if (this->modulation_ == MOD_LORA) {
     this->set_mode_standby();
     if (this->payload_length_ == 0) {
-      this->write_register_(REG_PAYLOAD_LENGTH, packet.size());
+      this->write_register_(REG_PAYLOAD_LENGTH, len);
     }
     this->write_register_(REG_IRQ_FLAGS, 0xFF);
     this->write_register_(REG_FIFO_ADDR_PTR, 0);
-    this->write_fifo_(packet);
+    this->write_fifo_(data, len);
     this->set_mode_tx();
   } else {
     this->set_mode_standby();
     if (this->payload_length_ == 0) {
-      this->write_register_(REG_FIFO, packet.size());
+      this->write_register_(REG_FIFO, len);
     }
-    this->write_fifo_(packet);
+    this->write_fifo_(data, len);
     this->set_mode_tx();
   }
 

esphome/components/sx127x/sx127x.h

@@ -9,6 +9,8 @@
 
 namespace esphome::sx127x {
 
+static constexpr size_t SX127X_MAX_PACKET_SIZE = 255;  // the payload length register is 8 bits
+
 enum SX127xBw : uint8_t {
   SX127X_BW_2_6,
   SX127X_BW_3_1,
@@ -81,7 +83,10 @@ class SX127x final : public Component,
   void set_sync_value(const std::vector<uint8_t> &sync_value) { this->sync_value_ = sync_value; }
   void run_image_cal();
   void configure();
-  SX127xError transmit_packet(const std::vector<uint8_t> &packet);
+  SX127xError transmit_packet(const uint8_t *data, size_t len);
+  SX127xError transmit_packet(const std::vector<uint8_t> &packet) {
+    return this->transmit_packet(packet.data(), packet.size());
+  }
   void register_listener(SX127xListener *listener) { this->listeners_.push_back(listener); }
   Trigger<std::vector<uint8_t>, float, float> *get_packet_trigger() { return &this->packet_trigger_; }
 
@@ -90,7 +95,7 @@ class SX127x final : public Component,
   void configure_fsk_ook_();
   void configure_lora_();
   void set_mode_(uint8_t modulation, uint8_t mode);
-  void write_fifo_(const std::vector<uint8_t> &packet);
+  void write_fifo_(const uint8_t *data, size_t len);
   void read_fifo_(std::vector<uint8_t> &packet);
   void write_register_(uint8_t reg, uint8_t value);
   void call_listeners_(const std::vector<uint8_t> &packet, float rssi, float snr);

tests/component_tests/sx127x/config/packet_tables.yaml

@@ -0,0 +1,24 @@
+esphome:
+  name: test
+  on_boot:
+    then:
+      - sx127x.send_packet: [0xC5, 0x51, 0x78, 0x82]
+      - sx127x.send_packet: [0xC5, 0x51, 0x78, 0x82]
+      - sx127x.send_packet: "hi"
+      - sx127x.send_packet: !lambda return {0x09};
+
+esp32:
+  board: esp32dev
+
+spi:
+  clk_pin: 18
+  mosi_pin: 23
+  miso_pin: 19
+
+sx127x:
+  cs_pin: 12
+  rst_pin: 13
+  dio0_pin: 26
+  frequency: 433920000
+  modulation: LORA
+  preamble_size: 8

tests/component_tests/sx127x/test_packet_tables.py

@@ -0,0 +1,51 @@
+"""Tests for SX127x constant packets in shared PROGMEM tables."""
+
+from collections.abc import Callable
+from pathlib import Path
+import re
+
+import pytest
+
+from esphome.components.sx127x import CONFIG_SCHEMA, validate_packet_data
+import esphome.config_validation as cv
+
+
+def test_constant_packets_share_progmem_tables(
+    generate_main: Callable[[str | Path], str],
+    component_config_path: Callable[[str], Path],
+) -> None:
+    """Equal constant packets share one table; lambdas stay templates."""
+    main_cpp = generate_main(component_config_path("packet_tables.yaml"))
+
+    tables = dict(
+        re.findall(
+            r"static constexpr uint8_t (\w+)\[\] PROGMEM = (\{[^}]*\});", main_cpp
+        )
+    )
+    assert sorted(tables.values()) == sorted(
+        ["{0xC5, 0x51, 0x78, 0x82}", "{0x68, 0x69}"]
+    )
+    shared = next(k for k, v in tables.items() if v == "{0xC5, 0x51, 0x78, 0x82}")
+    assert main_cpp.count(f"set_data_static({shared}, 4);") == 2
+    assert "set_data_template(" in main_cpp
+
+
+def test_packet_data_length_limit() -> None:
+    """Constant packets are limited to the 255 bytes the length register holds."""
+    assert len(validate_packet_data([0x01] * 255)) == 255
+    with pytest.raises(cv.Invalid):
+        validate_packet_data([0x01] * 256)
+    with pytest.raises(cv.Invalid):
+        validate_packet_data("x" * 256)
+    with pytest.raises(cv.Invalid):
+        validate_packet_data([])
+
+
+def test_payload_length_limit() -> None:
+    """payload_length goes into an 8 bit register, so 256 is rejected."""
+    validator = next(
+        v for k, v in CONFIG_SCHEMA.schema.items() if str(k) == "payload_length"
+    )
+    assert validator(255) == 255
+    with pytest.raises(cv.Invalid):
+        validator(256)