Merge branch 'am43-codec-tests' into am43-stack-packets

esphome/components/am43/am43_base.cpp

@@ -1,129 +1,127 @@
 #include "am43_base.h"
 #include "esphome/core/helpers.h"
+#include "esphome/core/log.h"
 #include <cstring>
 
 namespace esphome::am43 {
 
-const uint8_t START_PACKET[5] = {0x00, 0xff, 0x00, 0x00, 0x9a};
+ESPHOME_LOG_TAG(TAG, "am43");
 
-Am43Packet *Am43Encoder::get_battery_level_request() {
+static constexpr uint8_t START_PACKET[5] = {0x00, 0xff, 0x00, 0x00, 0x9a};
+
+Am43Packet Am43Encoder::get_battery_level_request() {
   uint8_t data = 0x1;
-  return this->encode_(0xA2, &data, 1);
+  return encode(CMD_GET_BATTERY_LEVEL, &data, 1);
 }
 
-Am43Packet *Am43Encoder::get_light_level_request() {
+Am43Packet Am43Encoder::get_light_level_request() {
   uint8_t data = 0x1;
-  return this->encode_(0xAA, &data, 1);
+  return encode(CMD_GET_LIGHT_LEVEL, &data, 1);
 }
 
-Am43Packet *Am43Encoder::get_position_request() {
+Am43Packet Am43Encoder::get_position_request() {
   uint8_t data = 0x1;
-  return this->encode_(CMD_GET_POSITION, &data, 1);
+  return encode(CMD_GET_POSITION, &data, 1);
 }
 
-Am43Packet *Am43Encoder::get_send_pin_request(uint16_t pin) {
+Am43Packet Am43Encoder::get_send_pin_request(uint16_t pin) {
   uint8_t data[2];
   data[0] = (pin & 0xFF00) >> 8;
   data[1] = pin & 0xFF;
-  return this->encode_(CMD_SEND_PIN, data, 2);
+  return encode(CMD_SEND_PIN, data, 2);
 }
 
-Am43Packet *Am43Encoder::get_open_request() {
+Am43Packet Am43Encoder::get_open_request() {
   uint8_t data = 0xDD;
-  return this->encode_(CMD_SET_STATE, &data, 1);
+  return encode(CMD_SET_STATE, &data, 1);
 }
 
-Am43Packet *Am43Encoder::get_close_request() {
+Am43Packet Am43Encoder::get_close_request() {
   uint8_t data = 0xEE;
-  return this->encode_(CMD_SET_STATE, &data, 1);
+  return encode(CMD_SET_STATE, &data, 1);
 }
 
-Am43Packet *Am43Encoder::get_stop_request() {
+Am43Packet Am43Encoder::get_stop_request() {
   uint8_t data = 0xCC;
-  return this->encode_(CMD_SET_STATE, &data, 1);
+  return encode(CMD_SET_STATE, &data, 1);
 }
 
-Am43Packet *Am43Encoder::get_set_position_request(uint8_t position) {
-  return this->encode_(CMD_SET_POSITION, &position, 1);
-}
+Am43Packet Am43Encoder::get_set_position_request(uint8_t position) { return encode(CMD_SET_POSITION, &position, 1); }
 
-void Am43Encoder::checksum_() {
-  uint8_t checksum = 0;
-  int i = 0;
-  for (i = 0; i < this->packet_.length; i++)
-    checksum = checksum ^ this->packet_.data[i];
-  this->packet_.data[i] = checksum ^ 0xff;
-  this->packet_.length++;
+Am43Packet Am43Encoder::encode(uint8_t command, const uint8_t *data, uint8_t length) {
+  Am43Packet packet;
+  memcpy(packet.data, START_PACKET, sizeof(START_PACKET));
+  packet.data[5] = command;
+  packet.data[6] = length;
+  memcpy(&packet.data[7], data, length);
+  packet.length = length + 7;
+  // The device checks the inverted XOR of everything before it
+  packet.data[packet.length] = xor_bytes(packet.data, packet.length, 0xff);
+  packet.length++;
+  char hex_buf[format_hex_size(sizeof(packet.data))];
+  ESP_LOGV(TAG, "ENC(%d): 0x%s", packet.length, format_hex_to(hex_buf, packet.data, packet.length));
+  return packet;
 }
 
-Am43Packet *Am43Encoder::encode_(uint8_t command, uint8_t *data, uint8_t length) {
-  memcpy(this->packet_.data, START_PACKET, 5);
-  this->packet_.data[5] = command;
-  this->packet_.data[6] = length;
-  memcpy(&this->packet_.data[7], data, length);
-  this->packet_.length = length + 7;
-  this->checksum_();
-  char hex_buf[format_hex_size(sizeof(this->packet_.data))];
-  ESP_LOGV("am43", "ENC(%d): 0x%s", this->packet_.length,
-           format_hex_to(hex_buf, this->packet_.data, this->packet_.length));
-  return &this->packet_;
-}
-
-#define VERIFY_MIN_LENGTH(x) \
-  if (length < (x)) \
-    return;
-
 void Am43Decoder::decode(const uint8_t *data, uint16_t length) {
+  // Reset so a decoder reused across frames reports only the latest one
   this->has_battery_level_ = false;
   this->has_light_level_ = false;
   this->has_set_position_response_ = false;
   this->has_set_state_response_ = false;
   this->has_position_ = false;
   this->has_pin_response_ = false;
-  char hex_buf[format_hex_size(24)];  // Max expected packet size
-  ESP_LOGV("am43", "DEC(%d): 0x%s", length, format_hex_to(hex_buf, data, length));
+  char hex_buf[format_hex_size(sizeof(Am43Packet::data))];  // Max expected packet size
+  ESP_LOGV(TAG, "DEC(%d): 0x%s", length, format_hex_to(hex_buf, data, length));
 
   if (length < 2 || data[0] != 0x9a)
     return;
   switch (data[1]) {
     case CMD_GET_BATTERY_LEVEL: {
-      VERIFY_MIN_LENGTH(8);
+      if (length < 8)
+        return;
       this->battery_level_ = data[7];
       this->has_battery_level_ = true;
       break;
     }
     case CMD_GET_LIGHT_LEVEL: {
-      VERIFY_MIN_LENGTH(5);
+      if (length < 5)
+        return;
       this->light_level_ = 100 * ((float) data[4] / 9);
       this->has_light_level_ = true;
       break;
     }
     case CMD_GET_POSITION: {
-      VERIFY_MIN_LENGTH(6);
+      if (length < 6)
+        return;
       this->position_ = data[5];
       this->has_position_ = true;
       break;
     }
     case CMD_NOTIFY_POSITION: {
-      VERIFY_MIN_LENGTH(5);
+      if (length < 5)
+        return;
       this->position_ = data[4];
       this->has_position_ = true;
       break;
     }
     case CMD_SEND_PIN: {
-      VERIFY_MIN_LENGTH(4);
+      if (length < 4)
+        return;
       this->pin_ok_ = data[3] == RESPONSE_ACK;
       this->has_pin_response_ = true;
       break;
     }
     case CMD_SET_POSITION: {
-      VERIFY_MIN_LENGTH(4);
+      if (length < 4)
+        return;
       this->set_position_ok_ = data[3] == RESPONSE_ACK;
       this->has_set_position_response_ = true;
       break;
     }
     case CMD_SET_STATE: {
-      VERIFY_MIN_LENGTH(4);
+      if (length < 4)
+        return;
       this->set_state_ok_ = data[3] == RESPONSE_ACK;
       this->has_set_state_response_ = true;
       break;
@@ -131,6 +129,6 @@ void Am43Decoder::decode(const uint8_t *data, uint16_t length) {
     default:
       break;
   }
-};
+}
 
 }  // namespace esphome::am43

esphome/components/am43/am43_base.h

@@ -1,59 +1,57 @@
 #pragma once
 
-#include "esphome/core/helpers.h"
-#include "esphome/core/log.h"
+#include <cstdint>
 
 namespace esphome::am43 {
 
-static const uint16_t AM43_SERVICE_UUID = 0xFE50;
-static const uint16_t AM43_CHARACTERISTIC_UUID = 0xFE51;
-//
+static constexpr uint16_t AM43_SERVICE_UUID = 0xFE50;
+static constexpr uint16_t AM43_CHARACTERISTIC_UUID = 0xFE51;
 // Tuya identifiers, only to detect and warn users as they are incompatible.
-static const uint16_t AM43_TUYA_SERVICE_UUID = 0x1910;
-static const uint16_t AM43_TUYA_CHARACTERISTIC_UUID = 0x2b11;
+static constexpr uint16_t AM43_TUYA_SERVICE_UUID = 0x1910;
+static constexpr uint16_t AM43_TUYA_CHARACTERISTIC_UUID = 0x2b11;
 
 struct Am43Packet {
   uint8_t length;
   uint8_t data[24];
 };
 
-static const uint8_t CMD_GET_BATTERY_LEVEL = 0xA2;
-static const uint8_t CMD_GET_LIGHT_LEVEL = 0xAA;
-static const uint8_t CMD_GET_POSITION = 0xA7;
-static const uint8_t CMD_SEND_PIN = 0x17;
-static const uint8_t CMD_SET_STATE = 0x0A;
-static const uint8_t CMD_SET_POSITION = 0x0D;
-static const uint8_t CMD_NOTIFY_POSITION = 0xA1;
+static constexpr uint8_t CMD_GET_BATTERY_LEVEL = 0xA2;
+static constexpr uint8_t CMD_GET_LIGHT_LEVEL = 0xAA;
+static constexpr uint8_t CMD_GET_POSITION = 0xA7;
+static constexpr uint8_t CMD_SEND_PIN = 0x17;
+static constexpr uint8_t CMD_SET_STATE = 0x0A;
+static constexpr uint8_t CMD_SET_POSITION = 0x0D;
+static constexpr uint8_t CMD_NOTIFY_POSITION = 0xA1;
 
-static const uint8_t RESPONSE_ACK = 0x5A;
-static const uint8_t RESPONSE_NACK = 0xA5;
+static constexpr uint8_t RESPONSE_ACK = 0x5A;
+static constexpr uint8_t RESPONSE_NACK = 0xA5;
 
+/// Builds request packets; each returns a packet the caller sends from its own stack.
 class Am43Encoder {
  public:
-  Am43Packet *get_battery_level_request();
-  Am43Packet *get_light_level_request();
-  Am43Packet *get_position_request();
-  Am43Packet *get_send_pin_request(uint16_t pin);
-  Am43Packet *get_open_request();
-  Am43Packet *get_close_request();
-  Am43Packet *get_stop_request();
-  Am43Packet *get_set_position_request(uint8_t position);
+  static Am43Packet get_battery_level_request();
+  static Am43Packet get_light_level_request();
+  static Am43Packet get_position_request();
+  static Am43Packet get_send_pin_request(uint16_t pin);
+  static Am43Packet get_open_request();
+  static Am43Packet get_close_request();
+  static Am43Packet get_stop_request();
+  static Am43Packet get_set_position_request(uint8_t position);
 
  protected:
-  void checksum_();
-  Am43Packet *encode_(uint8_t command, uint8_t *data, uint8_t length);
-  Am43Packet packet_;
+  static Am43Packet encode(uint8_t command, const uint8_t *data, uint8_t length);
 };
 
+/// Decodes one notification; construct it on the stack for each frame.
 class Am43Decoder {
  public:
   void decode(const uint8_t *data, uint16_t length);
-  bool has_battery_level() { return this->has_battery_level_; }
-  bool has_light_level() { return this->has_light_level_; }
-  bool has_set_position_response() { return this->has_set_position_response_; }
-  bool has_set_state_response() { return this->has_set_state_response_; }
-  bool has_position() { return this->has_position_; }
-  bool has_pin_response() { return this->has_pin_response_; }
+  bool has_battery_level() const { return this->has_battery_level_; }
+  bool has_light_level() const { return this->has_light_level_; }
+  bool has_set_position_response() const { return this->has_set_position_response_; }
+  bool has_set_state_response() const { return this->has_set_state_response_; }
+  bool has_position() const { return this->has_position_; }
+  bool has_pin_response() const { return this->has_pin_response_; }
 
   union {
     uint8_t position_;
@@ -65,12 +63,12 @@ class Am43Decoder {
   };
 
  protected:
-  bool has_battery_level_;
-  bool has_light_level_;
-  bool has_set_position_response_;
-  bool has_set_state_response_;
-  bool has_position_;
-  bool has_pin_response_;
+  bool has_battery_level_{false};
+  bool has_light_level_{false};
+  bool has_set_position_response_{false};
+  bool has_set_state_response_{false};
+  bool has_position_{false};
+  bool has_pin_response_{false};
 };
 
 }  // namespace esphome::am43

esphome/components/am43/cover/am43_cover.cpp

@@ -17,25 +17,12 @@ void Am43Component::dump_config() {
                 this->pin_, (int) this->invert_position_);
 }
 
-void Am43Component::setup() {
-  this->position = COVER_OPEN;
-  this->encoder_ = make_unique<Am43Encoder>();
-  this->decoder_ = make_unique<Am43Decoder>();
-  this->logged_in_ = false;
-}
+void Am43Component::setup() { this->position = COVER_OPEN; }
 
 void Am43Component::loop() {
   if (this->node_state == espbt::ClientState::ESTABLISHED && !this->logged_in_) {
-    auto *packet = this->encoder_->get_send_pin_request(this->pin_);
-    auto status =
-        esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(), this->char_handle_,
-                                 packet->length, packet->data, ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
     ESP_LOGI(TAG, "[%s] Logging into AM43", LOG_STR_ARG(this->get_log_name()));
-    if (status) {
-      ESP_LOGW(TAG, "[%s] Error writing set_pin to device, error = %d", LOG_STR_ARG(this->get_log_name()), status);
-    } else {
-      this->logged_in_ = true;
-    }
+    this->logged_in_ = this->write_packet_(Am43Encoder::get_send_pin_request(this->pin_), LOG_STR("set_pin"));
   }
 }
 
@@ -53,29 +40,15 @@ void Am43Component::control(const CoverCall &call) {
     ESP_LOGW(TAG, "[%s] Cannot send cover control, not connected", LOG_STR_ARG(this->get_log_name()));
     return;
   }
-  if (call.get_stop()) {
-    auto *packet = this->encoder_->get_stop_request();
-    auto status =
-        esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(), this->char_handle_,
-                                 packet->length, packet->data, ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
-    if (status) {
-      ESP_LOGW(TAG, "[%s] Error writing stop command to device, error = %d", LOG_STR_ARG(this->get_log_name()), status);
-    }
-  }
+  if (call.get_stop())
+    this->write_packet_(Am43Encoder::get_stop_request(), LOG_STR("stop"));
   auto opt_pos = call.get_position();
   if (opt_pos.has_value()) {
     auto pos = *opt_pos;
 
     if (this->invert_position_)
       pos = 1 - pos;
-    auto *packet = this->encoder_->get_set_position_request(100 - (uint8_t) (pos * 100));
-    auto status =
-        esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(), this->char_handle_,
-                                 packet->length, packet->data, ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
-    if (status) {
-      ESP_LOGW(TAG, "[%s] Error writing set_position command to device, error = %d", LOG_STR_ARG(this->get_log_name()),
-               status);
-    }
+    this->write_packet_(Am43Encoder::get_set_position_request(100 - (uint8_t) (pos * 100)), LOG_STR("set_position"));
   }
 }
 
@@ -113,10 +86,11 @@ void Am43Component::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_
     case ESP_GATTC_NOTIFY_EVT: {
       if (param->notify.handle != this->char_handle_)
         break;
-      this->decoder_->decode(param->notify.value, param->notify.value_len);
+      Am43Decoder decoder;
+      decoder.decode(param->notify.value, param->notify.value_len);
 
-      if (this->decoder_->has_position()) {
-        this->position = ((float) this->decoder_->position_ / 100.0f);
+      if (decoder.has_position()) {
+        this->position = ((float) decoder.position_ / 100.0f);
         if (!this->invert_position_)
           this->position = 1 - this->position;
         if (this->position > 0.97f)
@@ -126,27 +100,20 @@ void Am43Component::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_
         this->publish_state();
       }
 
-      if (this->decoder_->has_pin_response()) {
-        if (this->decoder_->pin_ok_) {
+      if (decoder.has_pin_response()) {
+        if (decoder.pin_ok_) {
           ESP_LOGI(TAG, "[%s] AM43 pin accepted.", LOG_STR_ARG(this->get_log_name()));
-          auto *packet = this->encoder_->get_position_request();
-          auto status = esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(),
-                                                 this->char_handle_, packet->length, packet->data,
-                                                 ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
-          if (status) {
-            ESP_LOGW(TAG, "[%s] Error writing set_position to device, error = %d", LOG_STR_ARG(this->get_log_name()),
-                     status);
-          }
+          this->write_packet_(Am43Encoder::get_position_request(), LOG_STR("get_position"));
         } else {
           ESP_LOGW(TAG, "[%s] AM43 pin rejected!", LOG_STR_ARG(this->get_log_name()));
         }
       }
 
-      if (this->decoder_->has_set_position_response() && !this->decoder_->set_position_ok_) {
+      if (decoder.has_set_position_response() && !decoder.set_position_ok_) {
         ESP_LOGW(TAG, "[%s] Got nack after sending set_position. Bad pin?", LOG_STR_ARG(this->get_log_name()));
       }
 
-      if (this->decoder_->has_set_state_response() && !this->decoder_->set_state_ok_) {
+      if (decoder.has_set_state_response() && !decoder.set_state_ok_) {
         ESP_LOGW(TAG, "[%s] Got nack after sending set_state. Bad pin?", LOG_STR_ARG(this->get_log_name()));
       }
       break;
@@ -156,6 +123,19 @@ void Am43Component::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_
   }
 }
 
+bool Am43Component::write_packet_(const Am43Packet &packet, const LogString *what) {
+  // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) the IDF API takes a mutable buffer it only reads
+  auto status = esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(),
+                                         this->char_handle_, packet.length, const_cast<uint8_t *>(packet.data),
+                                         ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
+  if (status) {
+    ESP_LOGW(TAG, "[%s] Error writing %s to device, error = %d", LOG_STR_ARG(this->get_log_name()), LOG_STR_ARG(what),
+             status);
+    return false;
+  }
+  return true;
+}
+
 }  // namespace esphome::am43
 
 #endif

esphome/components/am43/cover/am43_cover.h

@@ -27,14 +27,12 @@ class Am43Component final : public cover::Cover, public esphome::ble_client::BLE
 
  protected:
   void control(const cover::CoverCall &call) override;
-  uint16_t char_handle_;
-  uint16_t pin_;
-  bool invert_position_;
-  std::unique_ptr<Am43Encoder> encoder_;
-  std::unique_ptr<Am43Decoder> decoder_;
-  bool logged_in_;
-
-  float position_;
+  /// Writes a request to the control characteristic; logs and returns false on failure.
+  bool write_packet_(const Am43Packet &packet, const LogString *what);
+  uint16_t char_handle_{0};
+  uint16_t pin_{0};
+  bool invert_position_{false};
+  bool logged_in_{false};
 };
 
 }  // namespace esphome::am43

esphome/components/am43/sensor/am43_sensor.cpp

@@ -14,24 +14,9 @@ void Am43::dump_config() {
   LOG_SENSOR(" ", "Illuminance", this->illuminance_);
 }
 
-void Am43::setup() {
-  this->encoder_ = make_unique<Am43Encoder>();
-  this->decoder_ = make_unique<Am43Decoder>();
-  this->logged_in_ = false;
-  this->last_battery_update_ = 0;
-  this->current_sensor_ = 0;
-}
-
 void Am43::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_t gattc_if, esp_ble_gattc_cb_param_t *param) {
   switch (event) {
-    case ESP_GATTC_OPEN_EVT: {
-      if (param->open.status == ESP_GATT_OK) {
-        this->logged_in_ = false;
-      }
-      break;
-    }
     case ESP_GATTC_DISCONNECT_EVT: {
-      this->logged_in_ = false;
       this->node_state = espbt::ClientState::IDLE;
       if (this->battery_ != nullptr)
         this->battery_->publish_state(NAN);
@@ -60,28 +45,21 @@ void Am43::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_t gattc_i
     case ESP_GATTC_NOTIFY_EVT: {
       if (param->notify.handle != this->char_handle_)
         break;
-      this->decoder_->decode(param->notify.value, param->notify.value_len);
+      Am43Decoder decoder;
+      decoder.decode(param->notify.value, param->notify.value_len);
 
-      if (this->battery_ != nullptr && this->decoder_->has_battery_level() &&
-          millis() - this->last_battery_update_ > 10000) {
-        this->battery_->publish_state(this->decoder_->battery_level_);
+      if (this->battery_ != nullptr && decoder.has_battery_level() && millis() - this->last_battery_update_ > 10000) {
+        this->battery_->publish_state(decoder.battery_level_);
         this->last_battery_update_ = millis();
       }
 
-      if (this->illuminance_ != nullptr && this->decoder_->has_light_level()) {
-        this->illuminance_->publish_state(this->decoder_->light_level_);
+      if (this->illuminance_ != nullptr && decoder.has_light_level()) {
+        this->illuminance_->publish_state(decoder.light_level_);
       }
 
       if (this->current_sensor_ > 0) {
-        if (this->illuminance_ != nullptr) {
-          auto *packet = this->encoder_->get_light_level_request();
-          auto status = esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(),
-                                                 this->char_handle_, packet->length, packet->data,
-                                                 ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
-          if (status) {
-            ESP_LOGW(TAG, "[%s] esp_ble_gattc_write_char failed, status=%d", this->parent_->address_str(), status);
-          }
-        }
+        if (this->illuminance_ != nullptr)
+          this->write_packet_(Am43Encoder::get_light_level_request());
         this->current_sensor_ = 0;
       }
       break;
@@ -97,19 +75,22 @@ void Am43::update() {
     return;
   }
   if (this->current_sensor_ == 0) {
-    if (this->battery_ != nullptr) {
-      auto *packet = this->encoder_->get_battery_level_request();
-      auto status =
-          esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(), this->char_handle_,
-                                   packet->length, packet->data, ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
-      if (status) {
-        ESP_LOGW(TAG, "[%s] esp_ble_gattc_write_char failed, status=%d", this->parent_->address_str(), status);
-      }
-    }
+    if (this->battery_ != nullptr)
+      this->write_packet_(Am43Encoder::get_battery_level_request());
     this->current_sensor_++;
   }
 }
 
+void Am43::write_packet_(const Am43Packet &packet) {
+  // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) the IDF API takes a mutable buffer it only reads
+  auto status = esp_ble_gattc_write_char(this->parent_->get_gattc_if(), this->parent_->get_conn_id(),
+                                         this->char_handle_, packet.length, const_cast<uint8_t *>(packet.data),
+                                         ESP_GATT_WRITE_TYPE_NO_RSP, ESP_GATT_AUTH_REQ_NONE);
+  if (status) {
+    ESP_LOGW(TAG, "[%s] esp_ble_gattc_write_char failed, status=%d", this->parent_->address_str(), status);
+  }
+}
+
 }  // namespace esphome::am43
 
 #endif

esphome/components/am43/sensor/am43_sensor.h

@@ -16,7 +16,6 @@ namespace espbt = esphome::esp32_ble_tracker;
 
 class Am43 final : public esphome::ble_client::BLEClientNode, public PollingComponent {
  public:
-  void setup() override;
   void update() override;
   void gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_t gattc_if,
                            esp_ble_gattc_cb_param_t *param) override;
@@ -25,16 +24,15 @@ class Am43 final : public esphome::ble_client::BLEClientNode, public PollingComp
   void set_illuminance(sensor::Sensor *illuminance) { illuminance_ = illuminance; }
 
  protected:
-  uint16_t char_handle_;
-  std::unique_ptr<Am43Encoder> encoder_;
-  std::unique_ptr<Am43Decoder> decoder_;
-  bool logged_in_;
+  /// Writes a request to the control characteristic; logs on failure.
+  void write_packet_(const Am43Packet &packet);
   sensor::Sensor *battery_{nullptr};
   sensor::Sensor *illuminance_{nullptr};
-  uint8_t current_sensor_;
   // The AM43 often gets into a state where it spams loads of battery update
   // notifications. Here we will limit to no more than every 10s.
-  uint32_t last_battery_update_;
+  uint32_t last_battery_update_{0};
+  uint16_t char_handle_{0};
+  uint8_t current_sensor_{0};
 };
 
 }  // namespace esphome::am43

tests/components/am43/am43_codec_test.cpp

@@ -0,0 +1,127 @@
+#include <gtest/gtest.h>
+
+#include <vector>
+
+#include "esphome/components/am43/am43_base.h"
+
+namespace esphome::am43::testing {
+
+static std::vector<uint8_t> bytes(const Am43Packet &packet) {
+  return std::vector<uint8_t>(packet.data, packet.data + packet.length);
+}
+
+TEST(Am43Encoder, RequestsMatchTheDeviceFraming) {
+  // 00 ff 00 00 9a, command, length, data, then the inverted XOR of everything before it
+  EXPECT_EQ(bytes(Am43Encoder::get_battery_level_request()),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0xa2, 0x01, 0x01, 0x38}));
+  EXPECT_EQ(bytes(Am43Encoder::get_light_level_request()),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0xaa, 0x01, 0x01, 0x30}));
+  EXPECT_EQ(bytes(Am43Encoder::get_position_request()),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0xa7, 0x01, 0x01, 0x3d}));
+  EXPECT_EQ(bytes(Am43Encoder::get_send_pin_request(0x1234)),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0x17, 0x02, 0x12, 0x34, 0xa9}));
+  EXPECT_EQ(bytes(Am43Encoder::get_open_request()),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0x0a, 0x01, 0xdd, 0x4c}));
+  EXPECT_EQ(bytes(Am43Encoder::get_close_request()),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0x0a, 0x01, 0xee, 0x7f}));
+  EXPECT_EQ(bytes(Am43Encoder::get_stop_request()),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0x0a, 0x01, 0xcc, 0x5d}));
+  EXPECT_EQ(bytes(Am43Encoder::get_set_position_request(42)),
+            (std::vector<uint8_t>{0x00, 0xff, 0x00, 0x00, 0x9a, 0x0d, 0x01, 0x2a, 0xbc}));
+}
+
+TEST(Am43Decoder, ReadsEachResponse) {
+  Am43Decoder decoder;
+  const uint8_t battery[] = {0x9a, CMD_GET_BATTERY_LEVEL, 0, 0, 0, 0, 0, 77};
+  decoder.decode(battery, sizeof(battery));
+  ASSERT_TRUE(decoder.has_battery_level());
+  EXPECT_EQ(decoder.battery_level_, 77);
+  EXPECT_FALSE(decoder.has_position());
+
+  const uint8_t light[] = {0x9a, CMD_GET_LIGHT_LEVEL, 0, 0, 9};
+  decoder.decode(light, sizeof(light));
+  ASSERT_TRUE(decoder.has_light_level());
+  EXPECT_FLOAT_EQ(decoder.light_level_, 100.0f);
+  EXPECT_FALSE(decoder.has_battery_level());  // flags reset on every frame
+
+  const uint8_t position[] = {0x9a, CMD_GET_POSITION, 0, 0, 0, 30};
+  decoder.decode(position, sizeof(position));
+  ASSERT_TRUE(decoder.has_position());
+  EXPECT_EQ(decoder.position_, 30);
+
+  const uint8_t notify[] = {0x9a, CMD_NOTIFY_POSITION, 0, 0, 55};
+  decoder.decode(notify, sizeof(notify));
+  ASSERT_TRUE(decoder.has_position());
+  EXPECT_EQ(decoder.position_, 55);
+
+  const uint8_t pin_ack[] = {0x9a, CMD_SEND_PIN, 0, RESPONSE_ACK};
+  decoder.decode(pin_ack, sizeof(pin_ack));
+  ASSERT_TRUE(decoder.has_pin_response());
+  EXPECT_TRUE(decoder.pin_ok_);
+
+  const uint8_t position_nack[] = {0x9a, CMD_SET_POSITION, 0, RESPONSE_NACK};
+  decoder.decode(position_nack, sizeof(position_nack));
+  ASSERT_TRUE(decoder.has_set_position_response());
+  EXPECT_FALSE(decoder.set_position_ok_);
+
+  const uint8_t state_ack[] = {0x9a, CMD_SET_STATE, 0, RESPONSE_ACK};
+  decoder.decode(state_ack, sizeof(state_ack));
+  ASSERT_TRUE(decoder.has_set_state_response());
+  EXPECT_TRUE(decoder.set_state_ok_);
+}
+
+TEST(Am43Decoder, IgnoresShortAndForeignFrames) {
+  Am43Decoder decoder;
+  const uint8_t short_battery[] = {0x9a, CMD_GET_BATTERY_LEVEL, 0, 0};
+  decoder.decode(short_battery, sizeof(short_battery));
+  EXPECT_FALSE(decoder.has_battery_level());
+
+  const uint8_t foreign[] = {0x55, CMD_GET_POSITION, 0, 0, 0, 30};
+  decoder.decode(foreign, sizeof(foreign));
+  EXPECT_FALSE(decoder.has_position());
+}
+
+TEST(Am43Decoder, EachResponseNeedsItsMinimumLength) {
+  struct Case {
+    uint8_t command;
+    uint8_t min_length;
+    bool (*has)(Am43Decoder &);
+  };
+  const Case cases[] = {
+      {CMD_GET_BATTERY_LEVEL, 8, [](Am43Decoder &d) { return d.has_battery_level(); }},
+      {CMD_GET_LIGHT_LEVEL, 5, [](Am43Decoder &d) { return d.has_light_level(); }},
+      {CMD_GET_POSITION, 6, [](Am43Decoder &d) { return d.has_position(); }},
+      {CMD_NOTIFY_POSITION, 5, [](Am43Decoder &d) { return d.has_position(); }},
+      {CMD_SEND_PIN, 4, [](Am43Decoder &d) { return d.has_pin_response(); }},
+      {CMD_SET_POSITION, 4, [](Am43Decoder &d) { return d.has_set_position_response(); }},
+      {CMD_SET_STATE, 4, [](Am43Decoder &d) { return d.has_set_state_response(); }},
+  };
+  for (const auto &c : cases) {
+    uint8_t frame[8] = {0x9a, c.command, 0, RESPONSE_ACK, 0, 0, 0, 0};
+    Am43Decoder decoder;
+    decoder.decode(frame, c.min_length - 1);
+    EXPECT_FALSE(c.has(decoder)) << "command 0x" << std::hex << int(c.command);
+    decoder.decode(frame, c.min_length);
+    EXPECT_TRUE(c.has(decoder)) << "command 0x" << std::hex << int(c.command);
+  }
+}
+
+TEST(Am43Decoder, IgnoresTinyAndUnknownFrames) {
+  auto expect_nothing = [](Am43Decoder &decoder) {
+    EXPECT_FALSE(decoder.has_battery_level());
+    EXPECT_FALSE(decoder.has_light_level());
+    EXPECT_FALSE(decoder.has_position());
+    EXPECT_FALSE(decoder.has_pin_response());
+    EXPECT_FALSE(decoder.has_set_position_response());
+    EXPECT_FALSE(decoder.has_set_state_response());
+  };
+  Am43Decoder decoder;
+  const uint8_t one_byte[] = {0x9a};
+  decoder.decode(one_byte, sizeof(one_byte));
+  expect_nothing(decoder);
+  const uint8_t unknown[] = {0x9a, 0x42, 0, RESPONSE_ACK, 0, 0, 0, 0};
+  decoder.decode(unknown, sizeof(unknown));
+  expect_nothing(decoder);
+}
+
+}  // namespace esphome::am43::testing