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

AGENTS.md

@@ -443,7 +443,8 @@ file does, and it is the authority when they disagree. The most useful starting
         - **Hub forwarding entities need no C++ class:** a button, switch, number or select that only calls its hub
           uses `button.ApplyButton` (`switch.ApplySwitch`, `number.ApplyNumber`, `select.ApplySelect`) in its schema
           and `apply=` on the `sub_*` call. The body is a call on the hub with `entity` and `state`, `value` or
-          `index`/`option` in scope; on a switch, number or select `publish="before"` or `"after"` publishes the state.
+          `index`/`option` in scope; on a switch, number or select `publish="before"` or `"after"` publishes the state,
+          and `publish="on_success"` publishes only when the hub call returns true (a write the device can refuse).
           `publish="momentary"` is only for legacy action switches (restart, factory reset): it publishes off and runs
           the body only when turned on. Never use it for a new entity; an action is a button.
           ```python

esphome/core/entity_helpers.py

@@ -5,7 +5,7 @@ from importlib import import_module
 from importlib.util import find_spec
 import logging
 import re
-from typing import Any, Literal
+from typing import Any, Literal, get_args
 
 from esphome.automation import apply_static_function, parent_ref
 import esphome.codegen as cg
@@ -503,7 +503,8 @@ async def _setup_entity_impl(var: MockObj, config: ConfigType, platform: str) ->
     config[_KEY_ICON_IDX] = icon_idx
 
 
-ApplyPublish = Literal["before", "after", "momentary"] | None
+ApplyPublishMode = Literal["before", "after", "momentary", "on_success"]
+ApplyPublish = ApplyPublishMode | None
 
 
 @dataclass(frozen=True, slots=True)
@@ -577,7 +578,8 @@ class SubEntities:
 
         With ``apply`` the entity is the domain's ``Apply*`` class and its control runs
         ``parent->apply``, or ``apply`` as a namespaced free statement when no parent is bound, with
-        ``publish`` ("before"/"after") placing the domain's publish. ``publish="momentary"`` instead
+        ``publish`` ("before"/"after") placing the domain's publish; ``"on_success"`` publishes only when
+        ``apply`` returns true. ``publish="momentary"`` instead
         acknowledges first (a switch publishes off) and runs ``apply`` only when turned on. It is only for
         legacy action switches; a new action entity is a button.
         """
@@ -600,10 +602,8 @@ class SubEntities:
             )
         if publish is not None and (apply is None or self.apply_entity.publish is None):
             raise ValueError(f"{entity_id}: publish= needs apply= on a stateful domain")
-        if publish not in (None, "before", "after", "momentary"):
-            raise ValueError(
-                f'{entity_id}: publish= must be "before", "after" or "momentary"'
-            )
+        if publish is not None and publish not in (modes := get_args(ApplyPublishMode)):
+            raise ValueError(f"{entity_id}: publish= must be one of {', '.join(modes)}")
         if publish == "momentary" and self.apply_entity.momentary is None:
             raise ValueError(f'{entity_id}: publish="momentary" is not supported here')
 
@@ -626,11 +626,21 @@ class SubEntities:
         if isinstance(parent, ID):
             parent = await get_variable(parent)
         apply_entity = self.apply_entity
-        call = f"{apply};" if parent is None else f"{parent_ref(parent)}->{apply};"
+        expr = apply if parent is None else f"{parent_ref(parent)}->{apply}"
+        call = f"{expr};"
         if publish == "before":
             body = [apply_entity.publish, call]
         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} }}",
+            ]
         elif publish == "momentary":
             acknowledge, condition = apply_entity.momentary
             body = [acknowledge, f"if ({condition}) {{ {call} }}"]

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,93 @@
+#include <gtest/gtest.h>
+
+#include <array>
+
+#include "esphome/components/pcm5122/switch/power_switch.h"
+
+namespace esphome::pcm5122::testing {
+
+static constexpr uint8_t ADDRESS = 0x4D;
+
+// 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);
+    this->standby_.set_parent(&this->dac_);
+    this->standby_.set_power_mode(PCM5122_POWER_SWITCH_MODE_STANDBY);
+    this->powerdown_.set_parent(&this->dac_);
+    this->powerdown_.set_power_mode(PCM5122_POWER_SWITCH_MODE_POWERDOWN);
+    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_;
+  PCM5122PowerSwitch standby_;
+  PCM5122PowerSwitch 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

@@ -1711,6 +1711,16 @@ _SELECT_APPLY = entity_helpers.ApplyEntity(
             "after",
             "::hub->set_mode(index, options_);\nentity->publish_state(index);",
         ),
+        (
+            "set_mode(option)",
+            "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); }"
+            ),
+        ),
     ],
 )
 async def test_sub_entities_apply_locals(
@@ -1883,6 +1893,43 @@ async def test_sub_entities_apply_momentary(
     assert apply_functions[0][2] == body
 
 
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    ("parent", "apply", "body"),
+    [
+        (
+            _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); }"
+            ),
+        ),
+        (
+            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); }"
+            ),
+        ),
+    ],
+)
+async def test_sub_entities_apply_on_success(
+    setup_test_environment: list[str],
+    apply_functions: list[tuple[str, str, str]],
+    parent: MockObj | None,
+    apply: str,
+    body: str,
+) -> None:
+    """publish="on_success" publishes only when the hub call returns true."""
+    sub = _apply_sub([], {"k": _apply_conf("k_sw")}, parent=parent)
+    await sub("k", None, apply=apply, publish="on_success")
+    assert apply_functions[0][2] == body
+
+
 @pytest.mark.asyncio
 async def test_sub_entities_apply_without_parent_needs_namespaced_call(
     setup_test_environment: list[str],