Merge remote-tracking branch 'origin/apply-publish-on-success' into pcm5122-apply-switch

esphome/components/pcm5122/switch/__init__.py

@@ -4,29 +4,25 @@ import esphome.config_validation as cv
 from esphome.const import CONF_POWER_MODE, ENTITY_CATEGORY_CONFIG
 from esphome.types import ConfigType
 
-from ..audio_dac import CONF_PCM5122, PCM5122, pcm5122_ns
-
-PCM5122PowerSwitch = pcm5122_ns.class_("PCM5122PowerSwitch", switch.Switch)
-
-pcm5122_power_switch_mode = pcm5122_ns.enum("PCM5122PowerSwitchMode")
-PCM5122_POWER_SWITCH_MODE_ENUM = pcm5122_power_switch_mode.options(
-    "standby", "powerdown", prefix="PCM5122_POWER_SWITCH_MODE_"
-)
+from ..audio_dac import CONF_PCM5122, PCM5122
 
 CONFIG_SCHEMA = switch.switch_schema(
-    PCM5122PowerSwitch,
+    switch.ApplySwitch,
     entity_category=ENTITY_CATEGORY_CONFIG,
 ).extend(
     {
         cv.GenerateID(CONF_PCM5122): cv.use_id(PCM5122),
-        cv.Optional(CONF_POWER_MODE, default="powerdown"): cv.enum(
-            PCM5122_POWER_SWITCH_MODE_ENUM, lower=True
+        cv.Optional(CONF_POWER_MODE, default="powerdown"): cv.one_of(
+            "standby", "powerdown", lower=True
         ),
     }
 )
 
 
 async def to_code(config: ConfigType) -> None:
-    var = await switch.new_switch(config)
-    await cg.register_parented(var, config[CONF_PCM5122])
-    cg.add(var.set_power_mode(config[CONF_POWER_MODE]))
+    hub = await cg.get_variable(config[CONF_PCM5122])
+    setter = "set_standby" if config[CONF_POWER_MODE] == "standby" else "set_powerdown"
+    # The switch shows the new state only once the DAC accepted it
+    await switch.sub_switches(config, parent=hub).create(
+        config, None, apply=f"{setter}(state)", publish="on_success"
+    )

esphome/components/pcm5122/switch/power_switch.cpp

@@ -1,12 +0,0 @@
-#include "power_switch.h"
-
-namespace esphome::pcm5122 {
-
-void PCM5122PowerSwitch::write_state(bool state) {
-  bool ok = (this->mode_ == PCM5122_POWER_SWITCH_MODE_STANDBY) ? this->parent_->set_standby(state)
-                                                               : this->parent_->set_powerdown(state);
-  if (ok)
-    this->publish_state(state);
-}
-
-}  // namespace esphome::pcm5122

esphome/components/pcm5122/switch/power_switch.h

@@ -1,24 +0,0 @@
-#pragma once
-
-#include "esphome/components/switch/switch.h"
-
-#include "../pcm5122.h"
-
-namespace esphome::pcm5122 {
-
-enum PCM5122PowerSwitchMode : uint8_t {
-  PCM5122_POWER_SWITCH_MODE_STANDBY,
-  PCM5122_POWER_SWITCH_MODE_POWERDOWN,
-};
-
-class PCM5122PowerSwitch final : public switch_::Switch, public Parented<PCM5122> {
- public:
-  void set_power_mode(PCM5122PowerSwitchMode mode) { this->mode_ = mode; }
-
- protected:
-  void write_state(bool state) override;
-
-  PCM5122PowerSwitchMode mode_{PCM5122_POWER_SWITCH_MODE_POWERDOWN};
-};
-
-}  // namespace esphome::pcm5122

esphome/core/entity_helpers.py

@@ -633,14 +633,7 @@ class SubEntities:
         elif publish == "after":
             body = [call, apply_entity.publish]
         elif publish == "on_success":
-            # An esp_err_t or int would convert to bool silently and flip the meaning
-            body = [
-                (
-                    f"static_assert(std::is_same_v<decltype({expr}), bool>, "
-                    '"publish=\\"on_success\\" needs a hub call that returns bool");'
-                ),
-                f"if ({expr}) {{ {apply_entity.publish} }}",
-            ]
+            body = [f"if ({expr}) {{ {apply_entity.publish} }}"]
         elif publish == "momentary":
             acknowledge, condition = apply_entity.momentary
             body = [acknowledge, f"if ({condition}) {{ {call} }}"]

tests/components/bq25186/__init__.py

@@ -1,14 +0,0 @@
-import esphome.codegen as cg
-from esphome.types import ConfigType
-from tests.testing_helpers import ComponentManifestOverride
-
-
-def override_manifest(manifest: ComponentManifestOverride) -> None:
-    # One empty config entry so to_code runs and compiles the PG/GPO switch path.
-    manifest.multi_conf = False
-    manifest.dependencies = manifest.dependencies + ["i2c", "switch"]
-
-    async def to_code(config: ConfigType) -> None:
-        cg.add_define("USE_BQ25186_PG_GPO")
-
-    manifest.to_code = to_code

tests/components/bq25186/test_bq25186_pg_gpo.cpp

@@ -1,136 +0,0 @@
-#include <gtest/gtest.h>
-
-#include <array>
-
-#include "esphome/components/bq25186/bq25186.h"
-#include "esphome/components/bq25186/bq25186_switch.h"
-
-namespace esphome::bq25186::testing {
-
-static constexpr uint8_t ADDRESS = 0x6A;
-static constexpr uint8_t SYS_REG_OTHER_BITS = 0x0F;
-
-// The chip's registers behind an address pointer that moves on after every byte; writes can be made to fail.
-class RegisterBus final : public i2c::I2CBus {
- public:
-  i2c::ErrorCode write_readv(uint8_t address, const uint8_t *data, size_t size, uint8_t *out,
-                             size_t out_size) override {
-    EXPECT_EQ(address, ADDRESS);
-    if (size > 1 && this->fail_writes)
-      return i2c::ERROR_NOT_ACKNOWLEDGED;
-    if (size != 0) {
-      this->pointer_ = data[0];
-      for (size_t i = 1; i < size; i++)
-        this->registers[this->next_()] = data[i];
-    }
-    for (size_t i = 0; i < out_size; i++)
-      out[i] = this->registers[this->next_()];
-    return i2c::ERROR_OK;
-  }
-
-  std::array<uint8_t, BQ25186_STATUS_REGISTER_COUNT> registers{};
-  bool fail_writes{false};
-
- protected:
-  size_t next_() { return this->pointer_++ % this->registers.size(); }
-
-  uint8_t pointer_{0};
-};
-
-class BQ25186PgGpoTest : public ::testing::Test {
- protected:
-  void SetUp() override {
-    this->bus_.registers[BQ25186_REG_MASK_ID] = 0x01;  // the BQ25186 device id
-    this->bus_.registers[BQ25186_REG_SYS_REG] = SYS_REG_OTHER_BITS;
-    this->hub_.set_i2c_bus(&this->bus_);
-    this->hub_.set_i2c_address(ADDRESS);
-    this->switch_.set_parent(&this->hub_);
-    this->switch_.set_restore_mode(switch_::SWITCH_RESTORE_DISABLED);
-    this->hub_.set_pg_gpo_switch(&this->switch_);
-  }
-
-  bool level_() const { return (this->bus_.registers[BQ25186_REG_SYS_REG] & BQ25186_SYS_REG_PG_GPO_LEVEL) != 0; }
-  uint8_t other_bits_() const { return this->bus_.registers[BQ25186_REG_SYS_REG] & ~BQ25186_SYS_REG_PG_GPO_LEVEL; }
-
-  RegisterBus bus_;
-  BQ25186Component hub_;
-  BQ25186PgGpoSwitch switch_;
-};
-
-TEST_F(BQ25186PgGpoTest, TurnOnSetsTheLevelAndPublishes) {
-  this->switch_.turn_on();
-  EXPECT_TRUE(this->level_());
-  EXPECT_EQ(this->other_bits_(), SYS_REG_OTHER_BITS);
-  EXPECT_TRUE(this->switch_.has_state());
-  EXPECT_TRUE(this->switch_.state);
-}
-
-TEST_F(BQ25186PgGpoTest, TurnOffClearsTheLevelAndPublishes) {
-  this->bus_.registers[BQ25186_REG_SYS_REG] |= BQ25186_SYS_REG_PG_GPO_LEVEL;
-  this->switch_.publish_state(true);
-  this->switch_.turn_off();
-  EXPECT_FALSE(this->level_());
-  EXPECT_EQ(this->other_bits_(), SYS_REG_OTHER_BITS);
-  EXPECT_FALSE(this->switch_.state);
-}
-
-TEST_F(BQ25186PgGpoTest, FailedWriteLeavesTheStateUnpublished) {
-  this->bus_.fail_writes = true;
-  this->switch_.turn_on();
-  EXPECT_FALSE(this->level_());
-  EXPECT_FALSE(this->switch_.has_state());
-}
-
-TEST_F(BQ25186PgGpoTest, FailedWriteKeepsThePreviousState) {
-  // Start from on, so a reset to the default off state would show
-  this->bus_.registers[BQ25186_REG_SYS_REG] |= BQ25186_SYS_REG_PG_GPO_LEVEL;
-  this->switch_.publish_state(true);
-  this->bus_.fail_writes = true;
-  this->switch_.turn_off();
-  EXPECT_TRUE(this->level_());
-  EXPECT_TRUE(this->switch_.state);
-}
-
-TEST_F(BQ25186PgGpoTest, InvertedSwitchDrivesTheOppositeLevel) {
-  this->switch_.set_inverted(true);
-  this->switch_.turn_on();
-  EXPECT_FALSE(this->level_());
-  EXPECT_TRUE(this->switch_.state);
-  this->switch_.turn_off();
-  EXPECT_TRUE(this->level_());
-  EXPECT_FALSE(this->switch_.state);
-}
-
-TEST_F(BQ25186PgGpoTest, SetupWritesTheRestoredLevel) {
-  this->switch_.set_restore_mode(switch_::SWITCH_ALWAYS_ON);
-  this->hub_.setup();
-  EXPECT_TRUE(this->level_());
-  EXPECT_NE(this->bus_.registers[BQ25186_REG_VBAT_CTRL] & BQ25186_VBAT_CTRL_PG_MODE, 0);
-  EXPECT_TRUE(this->switch_.state);
-}
-
-TEST_F(BQ25186PgGpoTest, SetupWritesTheRestoredLevelInverted) {
-  this->bus_.registers[BQ25186_REG_SYS_REG] |= BQ25186_SYS_REG_PG_GPO_LEVEL;
-  this->switch_.set_restore_mode(switch_::SWITCH_ALWAYS_ON);
-  this->switch_.set_inverted(true);
-  this->hub_.setup();
-  EXPECT_FALSE(this->level_());
-  EXPECT_TRUE(this->switch_.state);
-}
-
-TEST_F(BQ25186PgGpoTest, SetupWithoutRestoreTakesTheChipLevel) {
-  this->bus_.registers[BQ25186_REG_SYS_REG] |= BQ25186_SYS_REG_PG_GPO_LEVEL;
-  this->hub_.setup();
-  EXPECT_TRUE(this->level_());
-  EXPECT_TRUE(this->switch_.state);
-}
-
-TEST_F(BQ25186PgGpoTest, SetupWithoutRestoreTakesTheChipLevelInverted) {
-  this->bus_.registers[BQ25186_REG_SYS_REG] |= BQ25186_SYS_REG_PG_GPO_LEVEL;
-  this->switch_.set_inverted(true);
-  this->hub_.setup();
-  EXPECT_TRUE(this->level_());
-  EXPECT_FALSE(this->switch_.state);
-}
-
-}  // namespace esphome::bq25186::testing

tests/components/pcm5122/__init__.py

@@ -0,0 +1,6 @@
+from tests.testing_helpers import ComponentManifestOverride
+
+
+def override_manifest(manifest: ComponentManifestOverride) -> None:
+    # The DAC is an audio_dac platform on I2C; the tests drive its power switch.
+    manifest.dependencies = manifest.dependencies + ["audio_dac", "i2c", "switch"]

tests/components/pcm5122/common.yaml

@@ -33,3 +33,7 @@ switch:
     pcm5122: pcm5122_dac
     name: PCM5122 Power Down
     power_mode: powerdown
+  - platform: pcm5122
+    pcm5122: pcm5122_dac
+    name: PCM5122 Standby
+    power_mode: standby

tests/components/pcm5122/pcm5122_power_switch_test.cpp

@@ -0,0 +1,102 @@
+#include <gtest/gtest.h>
+
+#include <array>
+
+#include "esphome/components/pcm5122/pcm5122.h"
+#include "esphome/components/switch/switch.h"
+
+namespace esphome::pcm5122::testing {
+
+static constexpr uint8_t ADDRESS = 0x4D;
+
+// The bodies codegen emits for power_mode: standby and power_mode: powerdown.
+static PCM5122 *dac = nullptr;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
+static void standby(switch_::Switch *entity, bool state) {
+  if (dac->set_standby(state))
+    entity->publish_state(state);
+}
+static void powerdown(switch_::Switch *entity, bool state) {
+  if (dac->set_powerdown(state))
+    entity->publish_state(state);
+}
+
+// Page 0 registers written as register/value pairs; failing writes are not acknowledged.
+class RegisterBus final : public i2c::I2CBus {
+ public:
+  i2c::ErrorCode write_readv(uint8_t address, const uint8_t *data, size_t size, uint8_t * /*out*/,
+                             size_t /*out_size*/) override {
+    EXPECT_EQ(address, ADDRESS);
+    if (this->fail)
+      return i2c::ERROR_NOT_ACKNOWLEDGED;
+    if (size == 2)
+      this->registers[data[0]] = data[1];
+    return i2c::ERROR_OK;
+  }
+
+  std::array<uint8_t, 256> registers{};
+  bool fail{false};
+};
+
+class PCM5122PowerSwitchTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    this->dac_.set_i2c_bus(&this->bus_);
+    this->dac_.set_i2c_address(ADDRESS);
+    dac = &this->dac_;
+    this->standby_.add_on_state_callback([this](bool) { ++this->publishes_; });
+    this->powerdown_.add_on_state_callback([this](bool) { ++this->publishes_; });
+  }
+
+  uint8_t power_control_() const { return this->bus_.registers[PCM5122_REG_POWER_CONTROL]; }
+
+  RegisterBus bus_;
+  PCM5122 dac_;
+  switch_::ApplySwitch<standby> standby_;
+  switch_::ApplySwitch<powerdown> powerdown_;
+  unsigned publishes_{0};
+};
+
+TEST_F(PCM5122PowerSwitchTest, StandbySwitchRequestsStandbyAndPublishes) {
+  this->standby_.turn_on();
+  EXPECT_EQ(this->power_control_(), PCM5122_POWER_CONTROL_RQST);
+  EXPECT_TRUE(this->standby_.state);
+  EXPECT_EQ(this->publishes_, 1u);
+
+  this->standby_.turn_off();
+  EXPECT_EQ(this->power_control_(), 0);
+  EXPECT_FALSE(this->standby_.state);
+  EXPECT_EQ(this->publishes_, 2u);
+}
+
+TEST_F(PCM5122PowerSwitchTest, PowerdownSwitchRequestsPowerdownAndPublishes) {
+  this->powerdown_.turn_on();
+  EXPECT_EQ(this->power_control_(), PCM5122_POWER_CONTROL_RQPD);
+  EXPECT_TRUE(this->powerdown_.state);
+  EXPECT_EQ(this->publishes_, 1u);
+
+  this->powerdown_.turn_off();
+  EXPECT_EQ(this->power_control_(), 0);
+  EXPECT_FALSE(this->powerdown_.state);
+  EXPECT_EQ(this->publishes_, 2u);
+}
+
+TEST_F(PCM5122PowerSwitchTest, FailedWriteLeavesTheSwitchUnpublished) {
+  this->bus_.fail = true;
+  this->standby_.turn_on();
+  this->powerdown_.turn_on();
+  EXPECT_EQ(this->publishes_, 0u);
+  EXPECT_FALSE(this->standby_.has_state());
+  EXPECT_FALSE(this->powerdown_.has_state());
+}
+
+TEST_F(PCM5122PowerSwitchTest, FailedWriteDoesNotLeakIntoTheNextRequest) {
+  this->bus_.fail = true;
+  this->standby_.turn_on();
+  this->bus_.fail = false;
+  this->powerdown_.turn_on();
+  EXPECT_EQ(this->power_control_(), PCM5122_POWER_CONTROL_RQPD);
+  EXPECT_FALSE(this->standby_.has_state());
+  EXPECT_TRUE(this->powerdown_.state);
+}
+
+}  // namespace esphome::pcm5122::testing

tests/components/pcm5122/switch/__init__.py

@@ -0,0 +1,6 @@
+from tests.testing_helpers import ComponentManifestOverride
+
+
+def override_manifest(manifest: ComponentManifestOverride) -> None:
+    # Builds the power switch platform against the DAC.
+    manifest.dependencies = manifest.dependencies + ["pcm5122"]

tests/unit_tests/core/test_entity_helpers.py

@@ -1716,8 +1716,6 @@ _SELECT_APPLY = entity_helpers.ApplyEntity(
             "on_success",
             (
                 "const char *option = entity->option_at(index);\n"
-                "static_assert(std::is_same_v<decltype(::hub->set_mode(option)), bool>, "
-                '"publish=\\"on_success\\" needs a hub call that returns bool");\n'
                 "if (::hub->set_mode(option)) { entity->publish_state(index); }"
             ),
         ),
@@ -1900,20 +1898,12 @@ async def test_sub_entities_apply_momentary(
         (
             _HUB,
             "write_level(state)",
-            (
-                "static_assert(std::is_same_v<decltype(::hub->write_level(state)), bool>, "
-                '"publish=\\"on_success\\" needs a hub call that returns bool");\n'
-                "if (::hub->write_level(state)) { entity->publish_state(state); }"
-            ),
+            "if (::hub->write_level(state)) { entity->publish_state(state); }",
         ),
         (
             None,
             "chip::write_level(state)",
-            (
-                "static_assert(std::is_same_v<decltype(chip::write_level(state)), bool>, "
-                '"publish=\\"on_success\\" needs a hub call that returns bool");\n'
-                "if (chip::write_level(state)) { entity->publish_state(state); }"
-            ),
+            "if (chip::write_level(state)) { entity->publish_state(state); }",
         ),
     ],
 )