[esp32_ble_server] Keep constant set_value payloads in shared flash tables (#20314)

esphome/components/esp32_ble_server/__init__.py

@@ -1,4 +1,6 @@
 import encodings
+import struct
+from typing import Any
 
 from esphome import automation
 import esphome.codegen as cg
@@ -25,8 +27,10 @@ from esphome.const import (
     __version__ as ESPHOME_VERSION,
 )
 from esphome.core import CORE
+from esphome.cpp_generator import MockObj
 import esphome.final_validate as fv
 from esphome.schema_extractors import SCHEMA_EXTRACT
+from esphome.types import ConfigType, TemplateArgsType
 
 AUTO_LOAD = ["esp32_ble", "bytebuffer"]
 CODEOWNERS = ["@jesserockz", "@clydebarrow", "@Rapsssito"]
@@ -111,8 +115,6 @@ BLECharacteristicNotifyAction = esp32_ble_server_automations_ns.class_(
 )
 bytebuffer_ns = cg.esphome_ns.namespace("bytebuffer")
 Endianness_ns = bytebuffer_ns.namespace("Endian")
-ByteBuffer_ns = bytebuffer_ns.namespace("ByteBuffer")
-ByteBuffer = bytebuffer_ns.class_("ByteBuffer")
 
 
 PROPERTY_MAP = {
@@ -141,6 +143,15 @@ class ValueType:
         return value
 
 
+def float32(value: Any) -> float:
+    value = cv.float_(value)
+    try:
+        struct.pack("<f", value)
+    except OverflowError as e:
+        raise cv.Invalid(f"{value} is out of range for a float") from e
+    return value
+
+
 VALUE_TYPES = {
     type_name: ValueType(type_name, validator, length)
     for type_name, validator, length in (
@@ -152,7 +163,7 @@ VALUE_TYPES = {
         ("int16_t", cv.int_range(-32768, 32767), 2),
         ("int32_t", cv.int_range(-2147483648, 2147483647), 4),
         ("int64_t", cv.int_range(-9223372036854775808, 9223372036854775807), 8),
-        ("float", cv.float_, 4),
+        ("float", float32, 4),
         ("double", cv.float_, 8),
         ("string", cv.string_strict, None),  # Length is variable
     )
@@ -495,19 +506,48 @@ def parse_uuid(uuid):
     return ESPBTUUID_ns.from_raw(uuid)
 
 
+_STRUCT_FORMATS = {
+    "uint8_t": "B",
+    "uint16_t": "H",
+    "uint32_t": "I",
+    "uint64_t": "Q",
+    "int8_t": "b",
+    "int16_t": "h",
+    "int32_t": "i",
+    "int64_t": "q",
+    "float": "f",
+    "double": "d",
+}
+
+
+def value_bytes(value_config: ConfigType) -> list[int]:
+    """The bytes of a constant value, as ByteBuffer::wrap packed them at runtime."""
+    value = value_config[CONF_DATA]
+    if isinstance(value, str):
+        return list(value.encode(value_config[CONF_STRING_ENCODING]))
+    if isinstance(value, list):
+        return value
+    order = ">" if value_config[CONF_ENDIANNESS] == "BIG" else "<"
+    return list(struct.pack(order + _STRUCT_FORMATS[value_config[CONF_TYPE]], value))
+
+
 async def parse_value(value_config, args):
     value = value_config[CONF_DATA]
     if isinstance(value, cv.Lambda):
         return await cg.templatable(value, args, cg.std_vector.template(cg.uint8))
+    # An initializer list calls the set_value(std::initializer_list<uint8_t>) overload
+    return cg.ArrayInitializer(*value_bytes(value_config))
 
-    if isinstance(value, str):
-        value = list(value.encode(value_config[CONF_STRING_ENCODING]))
-    if isinstance(value, list):
-        # Generate initializer list {1, 2, 3} instead of std::vector<uint8_t>({1, 2, 3})
-        # This calls the set_value(std::initializer_list<uint8_t>) overload
-        return cg.ArrayInitializer(*value)
-    val = cg.RawExpression(f"{value_config[CONF_TYPE]}({cg.safe_exp(value)})")
-    return ByteBuffer_ns.wrap(val, value_config[CONF_ENDIANNESS])
+
+async def set_value_buffer(
+    var: MockObj, value_config: ConfigType, args: TemplateArgsType
+) -> None:
+    value = value_config[CONF_DATA]
+    if not isinstance(value, cv.Lambda):
+        value = value_bytes(value_config)
+    await automation.templatable_bytes(
+        value, args, var.set_buffer_template, var.set_buffer_static, "ble_server_value"
+    )
 
 
 def calculate_num_handles(service_config):
@@ -662,8 +702,7 @@ async def to_code(config):
 async def ble_server_characteristic_set_value(config, action_id, template_arg, args):
     paren = await cg.get_variable(config[CONF_ID])
     var = cg.new_Pvariable(action_id, template_arg, paren)
-    value = await parse_value(config[CONF_VALUE], args)
-    cg.add(var.set_buffer(value))
+    await set_value_buffer(var, config[CONF_VALUE], args)
     cg.add_define("USE_ESP32_BLE_SERVER_SET_VALUE_ACTION")
     return var
 
@@ -682,8 +721,7 @@ async def ble_server_characteristic_set_value(config, action_id, template_arg, a
 async def ble_server_descriptor_set_value(config, action_id, template_arg, args):
     paren = await cg.get_variable(config[CONF_ID])
     var = cg.new_Pvariable(action_id, template_arg, paren)
-    value = await parse_value(config[CONF_VALUE], args)
-    cg.add(var.set_buffer(value))
+    await set_value_buffer(var, config[CONF_VALUE], args)
     cg.add_define("USE_ESP32_BLE_SERVER_DESCRIPTOR_SET_VALUE_ACTION")
     return var
 

esphome/components/esp32_ble_server/ble_characteristic.cpp

@@ -30,6 +30,14 @@ BLECharacteristic::BLECharacteristic(const ESPBTUUID uuid, uint32_t properties)
 
 void BLECharacteristic::set_value(ByteBuffer buffer) { this->set_value(buffer.get_data()); }
 
+void BLECharacteristic::set_value(const uint8_t *data, size_t length) {
+  if (length == 0) {  // An empty constant is a nullptr table
+    this->value_.clear();
+    return;
+  }
+  this->value_.assign(data, data + length);
+}
+
 void BLECharacteristic::set_value(std::vector<uint8_t> &&buffer) { this->value_ = std::move(buffer); }
 
 void BLECharacteristic::set_value(std::initializer_list<uint8_t> data) {

esphome/components/esp32_ble_server/ble_characteristic.h

@@ -30,6 +30,7 @@ class BLECharacteristic final {
   ~BLECharacteristic();
 
   void set_value(ByteBuffer buffer);
+  void set_value(const uint8_t *data, size_t length);
   void set_value(std::vector<uint8_t> &&buffer);
   void set_value(std::initializer_list<uint8_t> data);
   void set_value(const std::string &buffer);

esphome/components/esp32_ble_server/ble_descriptor.cpp

@@ -49,17 +49,18 @@ void BLEDescriptor::do_create(BLECharacteristic *characteristic) {
   this->state_ = CREATING;
 }
 
-void BLEDescriptor::set_value(std::vector<uint8_t> &&buffer) { this->set_value_impl_(buffer.data(), buffer.size()); }
+void BLEDescriptor::set_value(std::vector<uint8_t> &&buffer) { this->set_value(buffer.data(), buffer.size()); }
 
-void BLEDescriptor::set_value(std::initializer_list<uint8_t> data) { this->set_value_impl_(data.begin(), data.size()); }
+void BLEDescriptor::set_value(std::initializer_list<uint8_t> data) { this->set_value(data.begin(), data.size()); }
 
-void BLEDescriptor::set_value_impl_(const uint8_t *data, size_t length) {
+void BLEDescriptor::set_value(const uint8_t *data, size_t length) {
   if (length > this->value_.attr_max_len) {
     ESP_LOGE(TAG, "Size %d too large, must be no bigger than %d", length, this->value_.attr_max_len);
     return;
   }
   this->value_.attr_len = length;
-  memcpy(this->value_.attr_value, data, length);
+  if (length != 0)  // An empty constant is a nullptr table
+    memcpy(this->value_.attr_value, data, length);
 }
 
 void BLEDescriptor::gatts_event_handler(esp_gatts_cb_event_t event, esp_gatt_if_t gatts_if,

esphome/components/esp32_ble_server/ble_descriptor.h

@@ -26,6 +26,7 @@ class BLEDescriptor {
   void do_create(BLECharacteristic *characteristic);
   ESPBTUUID get_uuid() const { return this->uuid_; }
 
+  void set_value(const uint8_t *data, size_t length);
   void set_value(std::vector<uint8_t> &&buffer);
   void set_value(std::initializer_list<uint8_t> data);
   void set_value(ByteBuffer buffer) { this->set_value(buffer.get_data()); }
@@ -42,8 +43,6 @@ class BLEDescriptor {
   }
 
  protected:
-  void set_value_impl_(const uint8_t *data, size_t length);
-
   BLECharacteristic *characteristic_{nullptr};
   ESPBTUUID uuid_;
   uint16_t handle_{0xFFFF};

esphome/components/esp32_ble_server/ble_server_automations.h

@@ -16,6 +16,16 @@ namespace esphome::esp32_ble_server::esp32_ble_server_automations {
 
 using namespace esp32_ble;
 
+/// Sets a characteristic or descriptor value from a constant flash table or a lambda's vector.
+// Not TemplatableBytes::visit, which would copy the lambda's vector instead of moving it into the value.
+template<typename T, typename V, typename... Xs> void apply_value(T *target, const V &value, Xs &&...x) {
+  if (value.is_static()) {
+    target->set_value(value.data(), value.size());
+  } else {
+    target->set_value(value.value(x...));
+  }
+}
+
 class BLETriggers {
  public:
 #ifdef USE_ESP32_BLE_SERVER_CHARACTERISTIC_ON_WRITE
@@ -67,25 +77,23 @@ class BLECharacteristicSetValueActionManager {
 template<typename... Ts> class BLECharacteristicSetValueAction final : public Action<Ts...> {
  public:
   BLECharacteristicSetValueAction(BLECharacteristic *characteristic) : parent_(characteristic) {}
-  TEMPLATABLE_VALUE(std::vector<uint8_t>, buffer)
-  void set_buffer(std::initializer_list<uint8_t> buffer) { this->buffer_ = std::vector<uint8_t>(buffer); }
-  void set_buffer(ByteBuffer buffer) { this->set_buffer(buffer.get_data()); }
+  TEMPLATABLE_BYTES(buffer)
   void play(const Ts &...x) override {
     // If the listener is already set, do nothing
     if (BLECharacteristicSetValueActionManager::get_instance()->has_listener(this->parent_))
       return;
     // Set initial value
-    this->parent_->set_value(this->buffer_.value(x...));
+    apply_value(this->parent_, this->buffer_, x...);
     // Set the listener for read events
     // ``mutable`` keeps by-copy captures non-const for triggers passing args by reference
     // (e.g. climate on_control's ClimateCall&). See #17142.
     this->parent_->on_read([this, x...](uint16_t id) mutable {
       // Set the value of the characteristic every time it is read
-      this->parent_->set_value(this->buffer_.value(x...));
+      apply_value(this->parent_, this->buffer_, x...);
     });
     // Set the listener in the global manager so only one BLECharacteristicSetValueAction is set for each characteristic
     BLECharacteristicSetValueActionManager::get_instance()->set_listener(
-        this->parent_, [this, x...]() mutable { this->parent_->set_value(this->buffer_.value(x...)); });
+        this->parent_, [this, x...]() mutable { apply_value(this->parent_, this->buffer_, x...); });
   }
 
  protected:
@@ -115,10 +123,8 @@ template<typename... Ts> class BLECharacteristicNotifyAction final : public Acti
 template<typename... Ts> class BLEDescriptorSetValueAction final : public Action<Ts...> {
  public:
   BLEDescriptorSetValueAction(BLEDescriptor *descriptor) : parent_(descriptor) {}
-  TEMPLATABLE_VALUE(std::vector<uint8_t>, buffer)
-  void set_buffer(std::initializer_list<uint8_t> buffer) { this->buffer_ = std::vector<uint8_t>(buffer); }
-  void set_buffer(ByteBuffer buffer) { this->set_buffer(buffer.get_data()); }
-  void play(const Ts &...x) override { this->parent_->set_value(this->buffer_.value(x...)); }
+  TEMPLATABLE_BYTES(buffer)
+  void play(const Ts &...x) override { apply_value(this->parent_, this->buffer_, x...); }
 
  protected:
   BLEDescriptor *parent_;

tests/component_tests/esp32_ble_server/config/set_value.yaml

@@ -0,0 +1,37 @@
+esphome:
+  name: test
+
+esp32:
+  variant: esp32
+
+esp32_ble_server:
+  id: ble_server
+  services:
+    - uuid: 2a24b789-7aab-4535-af3e-ee76a35cc12d
+      characteristics:
+        - id: test_characteristic
+          uuid: cad48e28-7fbe-41cf-bae9-d77a6c233423
+          read: true
+          value: [1, 2, 3, 4]
+          descriptors:
+            - id: test_descriptor
+              uuid: 0x4414
+              value: "Initial"
+        - uuid: 2a24b789-7a1b-4535-af3e-ee76a35cc99a
+          write: true
+          on_write:
+            - ble_server.characteristic.set_value:
+                id: test_characteristic
+                value:
+                  data: 4660
+                  type: uint16_t
+                  endianness: BIG
+            - ble_server.descriptor.set_value:
+                id: test_descriptor
+                value:
+                  data: 4660
+                  type: uint16_t
+                  endianness: BIG
+            - ble_server.characteristic.set_value:
+                id: test_characteristic
+                value: !lambda return x;

tests/component_tests/esp32_ble_server/test_esp32_ble_server.py

@@ -9,10 +9,16 @@ import pytest
 
 from esphome.components.esp32_ble_server import (
     CCCD_DESCRIPTOR_UUID,
+    CONF_ENDIANNESS,
+    CONF_STRING_ENCODING,
     CUD_DESCRIPTOR_UUID,
     DEVICE_INFORMATION_SERVICE_UUID,
+    float32,
     uuid_is,
+    value_bytes,
 )
+import esphome.config_validation as cv
+from esphome.const import CONF_DATA, CONF_TYPE
 
 
 @pytest.mark.parametrize(
@@ -73,3 +79,69 @@ def test_advertising_required(
     main_cpp = generate_main(component_config_path(config_file))
 
     assert f"set_advertising_required({str(required).lower()})" in main_cpp
+
+
+# The same values and bytes as tests/components/bytebuffer/test_wrap_values.cpp, which pins
+# what ByteBuffer::wrap produced from the expressions this component used to emit.
+@pytest.mark.parametrize(
+    ("data", "type_", "endianness", "expected"),
+    [
+        (18, "uint8_t", "LITTLE", [0x12]),
+        (18, "uint16_t", "LITTLE", [0x12, 0x00]),
+        (4660, "uint16_t", "BIG", [0x12, 0x34]),
+        (305419896, "uint32_t", "LITTLE", [0x78, 0x56, 0x34, 0x12]),
+        (
+            1311768467294899695,
+            "uint64_t",
+            "BIG",
+            [0x12, 0x34, 0x56, 0x78, 0x90, 0xAB, 0xCD, 0xEF],
+        ),
+        (-5, "int8_t", "LITTLE", [0xFB]),
+        (-2, "int16_t", "BIG", [0xFF, 0xFE]),
+        (-2, "int32_t", "LITTLE", [0xFE, 0xFF, 0xFF, 0xFF]),
+        (-2, "int64_t", "LITTLE", [0xFE] + [0xFF] * 7),
+        (123.1, "float", "BIG", [0x42, 0xF6, 0x33, 0x33]),
+        (0.1, "float", "LITTLE", [0xCD, 0xCC, 0xCC, 0x3D]),
+        (0.1, "double", "LITTLE", [0x9A, 0x99, 0x99, 0x99, 0x99, 0x99, 0xB9, 0x3F]),
+        (2.5, "double", "BIG", [0x40, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]),
+    ],
+)
+def test_value_bytes_match_bytebuffer(
+    data: float, type_: str, endianness: str, expected: list[int]
+) -> None:
+    config = {
+        CONF_DATA: data,
+        CONF_TYPE: type_,
+        CONF_ENDIANNESS: endianness,
+        CONF_STRING_ENCODING: "utf_8",
+    }
+    assert value_bytes(config) == expected
+
+
+def test_value_bytes_strings_and_lists() -> None:
+    assert value_bytes({CONF_DATA: "hé", CONF_STRING_ENCODING: "utf_8"}) == [
+        0x68,
+        0xC3,
+        0xA9,
+    ]
+    assert value_bytes({CONF_DATA: [1, 2], CONF_STRING_ENCODING: "utf_8"}) == [1, 2]
+
+
+def test_set_value_constants_share_a_flash_table(
+    generate_main: Callable[[str | Path], str],
+    component_config_path: Callable[[str], Path],
+) -> None:
+    main_cpp = generate_main(component_config_path("set_value.yaml"))
+
+    assert main_cpp.count("ble_server_value[] PROGMEM = {0x12, 0x34};") == 1
+    assert main_cpp.count("set_buffer_static(ble_server_value, 2);") == 2
+    assert "set_buffer_template(" in main_cpp
+    assert "ByteBuffer::wrap" not in main_cpp
+
+
+def test_float32_rejects_out_of_range() -> None:
+    for value in (1e40, -1e40):
+        with pytest.raises(cv.Invalid, match="out of range for a float"):
+            float32(value)
+    assert float32(3.4028235e38) == 3.4028235e38
+    assert float32(float("inf")) == float("inf")

tests/components/bytebuffer/test_wrap_values.cpp

@@ -8,8 +8,7 @@
 
 namespace esphome::bytebuffer {
 
-// The expressions esp32_ble_server emits for typed constant values, and the bytes they produce.
-// Doubles are written as float literals, so they carry float precision.
+// Typed values as ByteBuffer::wrap packs them; esp32_ble_server's value_bytes() is tested against the same bytes.
 TEST(ByteBufferWrapValues, MatchesGeneratedConstants) {
   EXPECT_EQ(ByteBuffer::wrap(uint8_t(18), LITTLE).get_data(), (std::vector<uint8_t>{0x12}));
   EXPECT_EQ(ByteBuffer::wrap(uint16_t(18), LITTLE).get_data(), (std::vector<uint8_t>{0x12, 0x00}));
@@ -24,9 +23,9 @@ TEST(ByteBufferWrapValues, MatchesGeneratedConstants) {
             (std::vector<uint8_t>{0xfe, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}));
   EXPECT_EQ(ByteBuffer::wrap(float(123.1f), BIG).get_data(), (std::vector<uint8_t>{0x42, 0xf6, 0x33, 0x33}));
   EXPECT_EQ(ByteBuffer::wrap(float(0.1f), LITTLE).get_data(), (std::vector<uint8_t>{0xcd, 0xcc, 0xcc, 0x3d}));
-  EXPECT_EQ(ByteBuffer::wrap(double(0.1f), LITTLE).get_data(),
-            (std::vector<uint8_t>{0x00, 0x00, 0x00, 0xa0, 0x99, 0x99, 0xb9, 0x3f}));
-  EXPECT_EQ(ByteBuffer::wrap(double(2.5f), BIG).get_data(),
+  EXPECT_EQ(ByteBuffer::wrap(double(0.1), LITTLE).get_data(),
+            (std::vector<uint8_t>{0x9a, 0x99, 0x99, 0x99, 0x99, 0x99, 0xb9, 0x3f}));
+  EXPECT_EQ(ByteBuffer::wrap(double(2.5), BIG).get_data(),
             (std::vector<uint8_t>{0x40, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}));
 }
 

tests/components/esp32_ble_server/common.yaml

@@ -110,6 +110,13 @@ esp32_ble_server:
               - ble_server.descriptor.set_value:
                   id: test_change_descriptor
                   value: "Changed"
+              # Empty constants
+              - ble_server.characteristic.set_value:
+                  id: test_change_characteristic
+                  value: ""
+              - ble_server.descriptor.set_value:
+                  id: test_change_descriptor
+                  value: ""
               # Lambdas that read the trigger argument
               - ble_server.characteristic.set_value:
                   id: test_change_characteristic