[binary_sensor] Clarify why the autorepeat reset does not use Filter::reset

esphome/components/binary_sensor/binary_sensor.cpp

@@ -31,6 +31,13 @@ void BinarySensor::publish_initial_state(bool new_state) {
   this->publish_state(new_state);
 }
 bool BinarySensor::set_new_state(const optional<bool> &new_state) {
+#ifdef USE_BINARY_SENSOR_FILTER
+  // An unknown state drops each filter's pending output and last value, so the next value is not dropped as a repeat
+  if (!new_state.has_value()) {
+    for (Filter *filter = this->filter_list_; filter != nullptr; filter = filter->next_)
+      filter->reset();
+  }
+#endif
   if (StatefulEntityBase::set_new_state(new_state)) {
     // weirdly, this file could be compiled even without USE_BINARY_SENSOR defined
 #if defined(USE_BINARY_SENSOR) && defined(USE_CONTROLLER_REGISTRY)

esphome/components/binary_sensor/filter.cpp

@@ -17,6 +17,10 @@ void Filter::output(bool value) {
     this->next_->input(value);
   }
 }
+void Filter::reset() {
+  this->dedup_.next_unknown();
+  App.scheduler.cancel_timeout(this);
+}
 void Filter::input(bool value) {
   if (!this->dedup_.next(value))
     return;
@@ -28,7 +32,7 @@ void Filter::input(bool value) {
 
 void TimeoutFilter::input(bool value) {
   App.scheduler.set_timeout(this, this->timeout_delay_.value(), [this]() { this->parent_->invalidate_state(); });
-  // we do not de-dup here otherwise changes from invalid to valid state will not be output
+  // Every input re-arms the timeout, including repeats, so this filter must not de-dup
   this->output(value);
 }
 
@@ -82,6 +86,12 @@ optional<bool> AutorepeatFilterBase::new_value(bool value) {
   }
 }
 
+void AutorepeatFilterBase::reset() {
+  this->dedup_.next_unknown();
+  // The release cancels both timers; Filter::reset() would leave the on/off timer running
+  AutorepeatFilterBase::new_value(false);
+}
+
 void AutorepeatFilterBase::next_timing_() {
   if (this->active_timing_ < this->timings_count_) {
     App.scheduler.set_timeout(this, this->timings_[this->active_timing_].delay, [this]() { this->next_timing_(); });
@@ -96,6 +106,9 @@ void AutorepeatFilterBase::next_timing_() {
 void AutorepeatFilterBase::next_value_(bool val) {
   const AutorepeatFilterTiming &timing = this->timings_[this->active_timing_ - 2];
   this->output(val);
+  // A callback of that output may have stopped the filter
+  if (this->active_timing_ < 2)
+    return;
   App.scheduler.set_timeout(&this->active_timing_, val ? timing.time_on : timing.time_off,
                             [this, val]() { this->next_value_(!val); });
 }
@@ -104,6 +117,11 @@ LambdaFilter::LambdaFilter(std::function<optional<bool>(bool)> f) : f_(std::move
 
 optional<bool> LambdaFilter::new_value(bool value) { return this->f_(value); }
 
+void SettleFilter::reset() {
+  Filter::reset();
+  this->steady_ = true;
+}
+
 optional<bool> SettleFilter::new_value(bool value) {
   if (!this->steady_) {
     App.scheduler.set_timeout(this, this->delay_.value(), [this, value]() {

esphome/components/binary_sensor/filter.h

@@ -24,6 +24,9 @@ class Filter {
  protected:
   friend BinarySensor;
 
+  /// Forget the last value and drop any pending scheduled output; called when the sensor's state is invalidated.
+  virtual void reset();
+
   Filter *next_{nullptr};
   BinarySensor *parent_{nullptr};
   Deduplicator<bool> dedup_;
@@ -100,6 +103,7 @@ class AutorepeatFilterBase : public Filter {
 
  protected:
   AutorepeatFilterBase() = default;
+  void reset() override;
   void next_timing_();
   void next_value_(bool val);
 
@@ -156,6 +160,8 @@ class SettleFilter : public Filter {
   template<typename T> void set_delay(T delay) { this->delay_ = delay; }
 
  protected:
+  void reset() override;
+
   TemplatableFn<uint32_t> delay_{};
   bool steady_{true};
 };

tests/components/binary_sensor/__init__.py

@@ -0,0 +1,11 @@
+import esphome.codegen as cg
+from esphome.types import ConfigType
+from tests.testing_helpers import ComponentManifestOverride
+
+
+def override_manifest(manifest: ComponentManifestOverride) -> None:
+    async def to_code_testing(config: ConfigType) -> None:
+        # filter.cpp only compiles with the define the real to_code emits for configured filters
+        cg.add_define("USE_BINARY_SENSOR_FILTER")
+
+    manifest.to_code = to_code_testing

tests/components/binary_sensor/binary_sensor_invalidate_test.cpp

@@ -0,0 +1,111 @@
+#include <gtest/gtest.h>
+
+#include "esphome/components/binary_sensor/binary_sensor.h"
+#include "esphome/components/binary_sensor/filter.h"
+#include "esphome/core/application.h"
+#include "esphome/core/hal.h"
+
+namespace esphome::binary_sensor::testing {
+
+static void run_scheduler_after(uint32_t ms) {
+  delay(ms);
+  App.scheduler.call(millis());
+}
+
+TEST(BinarySensorInvalidate, RepeatedValuePassesTheFiltersAfterInvalidate) {
+  BinarySensor sensor;
+  InvertFilter invert;
+  sensor.add_filter(&invert);
+  sensor.publish_state(false);
+  EXPECT_TRUE(sensor.state);
+
+  sensor.invalidate_state();
+  EXPECT_FALSE(sensor.has_state());
+  sensor.publish_state(false);
+  ASSERT_TRUE(sensor.has_state());
+  EXPECT_TRUE(sensor.state);
+
+  // publish_initial_state() invalidates first, so a repeated value passes there too
+  sensor.publish_state(true);
+  sensor.publish_initial_state(true);
+  ASSERT_TRUE(sensor.has_state());
+  EXPECT_FALSE(sensor.state);
+}
+
+TEST(BinarySensorInvalidate, PendingDelayedOutputIsDropped) {
+  BinarySensor sensor;
+  DelayedOnFilter delayed_on;
+  delayed_on.set_delay([]() -> uint32_t { return 10; });
+  sensor.add_filter(&delayed_on);
+  sensor.publish_state(true);
+  sensor.invalidate_state();
+  run_scheduler_after(30);
+  EXPECT_FALSE(sensor.has_state());
+}
+
+TEST(BinarySensorInvalidate, SettleFilterStartsSteadyAfterInvalidate) {
+  BinarySensor sensor;
+  SettleFilter settle;
+  settle.set_delay([]() -> uint32_t { return 10; });
+  sensor.add_filter(&settle);
+  sensor.publish_state(true);
+  EXPECT_TRUE(sensor.state);
+
+  sensor.invalidate_state();
+  sensor.publish_state(true);
+  ASSERT_TRUE(sensor.has_state());
+  EXPECT_TRUE(sensor.state);
+
+  // A change inside the settle time is pending; invalidating drops it
+  sensor.publish_state(false);
+  EXPECT_TRUE(sensor.state);
+  sensor.invalidate_state();
+  run_scheduler_after(30);
+  EXPECT_FALSE(sensor.has_state());
+}
+
+// TimeoutFilter invalidates the sensor from its own scheduler callback, which resets that same filter
+TEST(BinarySensorInvalidate, TimeoutFilterInvalidatesAndTheSameValuePassesAgain) {
+  BinarySensor sensor;
+  TimeoutFilter timeout;
+  timeout.set_timeout_value([]() -> uint32_t { return 10; });
+  InvertFilter invert;
+  sensor.add_filters({&timeout, &invert});
+  sensor.publish_state(false);
+  ASSERT_TRUE(sensor.has_state());
+  EXPECT_TRUE(sensor.state);
+
+  run_scheduler_after(30);
+  EXPECT_FALSE(sensor.has_state());
+
+  sensor.publish_state(false);
+  ASSERT_TRUE(sensor.has_state());
+  EXPECT_TRUE(sensor.state);
+
+  // Cancel the timeout that publish armed before the sensor on this stack frame goes away
+  sensor.invalidate_state();
+}
+
+// An on_release automation that invalidates the sensor resets the autorepeat filter while it is still outputting
+TEST(BinarySensorInvalidate, AutorepeatResetFromItsOwnOutputStopsTheRepeat) {
+  BinarySensor sensor;
+  AutorepeatFilter<1> autorepeat({{10, 10, 10}});
+  sensor.add_filter(&autorepeat);
+  bool invalidate_on_release = true;
+  sensor.add_on_state_callback([&sensor, &invalidate_on_release](bool state) {
+    if (!state && invalidate_on_release) {
+      invalidate_on_release = false;
+      sensor.invalidate_state();
+    }
+  });
+  sensor.publish_state(true);
+  ASSERT_TRUE(sensor.state);
+
+  // The repeat starts after 10 ms with a release, and that release invalidates the sensor
+  for (int i = 0; i < 3; i++) {
+    run_scheduler_after(10);
+  }
+  EXPECT_FALSE(sensor.has_state());
+}
+
+}  // namespace esphome::binary_sensor::testing