Fix pulse_meter filter logic (#3321)

esphome/components/pulse_meter/pulse_meter_sensor.cpp

@@ -11,62 +11,43 @@ void PulseMeterSensor::setup() {
   this->isr_pin_ = pin_->to_isr();
   this->pin_->attach_interrupt(PulseMeterSensor::gpio_intr, this, gpio::INTERRUPT_ANY_EDGE);
 
+  this->pulse_width_us_ = 0;
   this->last_detected_edge_us_ = 0;
-  this->last_valid_low_edge_us_ = 0;
   this->last_valid_high_edge_us_ = 0;
+  this->last_valid_low_edge_us_ = 0;
   this->sensor_is_high_ = this->isr_pin_.digital_read();
+  this->has_valid_high_edge_ = false;
+  this->has_valid_low_edge_ = false;
 }
 
 void PulseMeterSensor::loop() {
+  // Get a local copy of the volatile sensor values, to make sure they are not
+  // modified by the ISR. This could cause overflow in the following arithmetic
+  const uint32_t last_valid_high_edge_us = this->last_valid_high_edge_us_;
+  const bool has_valid_high_edge = this->has_valid_high_edge_;
   const uint32_t now = micros();
 
-  // Check to see if we should filter this edge out
+  // If we've exceeded our timeout interval without receiving any pulses, assume
-  if (this->filter_mode_ == FILTER_EDGE) {
+  // 0 pulses/min until we get at least two valid pulses.
-    if ((this->last_detected_edge_us_ - this->last_valid_high_edge_us_) >= this->filter_us_) {
+  const uint32_t time_since_valid_edge_us = now - last_valid_high_edge_us;
-      // Don't measure the first valid pulse (we need at least two pulses to measure the width)
+  if ((has_valid_high_edge) && (time_since_valid_edge_us > this->timeout_us_)) {
-      if (this->last_valid_high_edge_us_ != 0) {
-        this->pulse_width_us_ = (this->last_detected_edge_us_ - this->last_valid_high_edge_us_);
-      }
-      this->total_pulses_++;
-      this->last_valid_high_edge_us_ = this->last_detected_edge_us_;
-    }
-  } else {
-    // Make sure the signal has been stable long enough
-    if ((now - this->last_detected_edge_us_) >= this->filter_us_) {
-      // Only consider HIGH pulses and "new" edges if sensor state is LOW
-      if (!this->sensor_is_high_ && this->isr_pin_.digital_read() &&
-          (this->last_detected_edge_us_ != this->last_valid_high_edge_us_)) {
-        // Don't measure the first valid pulse (we need at least two pulses to measure the width)
-        if (this->last_valid_high_edge_us_ != 0) {
-          this->pulse_width_us_ = (this->last_detected_edge_us_ - this->last_valid_high_edge_us_);
-        }
-        this->sensor_is_high_ = true;
-        this->total_pulses_++;
-        this->last_valid_high_edge_us_ = this->last_detected_edge_us_;
-      }
-      // Only consider LOW pulses and "new" edges if sensor state is HIGH
-      else if (this->sensor_is_high_ && !this->isr_pin_.digital_read() &&
-               (this->last_detected_edge_us_ != this->last_valid_low_edge_us_)) {
-        this->sensor_is_high_ = false;
-        this->last_valid_low_edge_us_ = this->last_detected_edge_us_;
-      }
-    }
-  }
-
-  // If we've exceeded our timeout interval without receiving any pulses, assume 0 pulses/min until
-  // we get at least two valid pulses.
-  const uint32_t time_since_valid_edge_us = now - this->last_valid_high_edge_us_;
-  if ((this->last_valid_high_edge_us_ != 0) && (time_since_valid_edge_us > this->timeout_us_) &&
-      (this->pulse_width_us_ != 0)) {
     ESP_LOGD(TAG, "No pulse detected for %us, assuming 0 pulses/min", time_since_valid_edge_us / 1000000);
+
     this->pulse_width_us_ = 0;
+    this->last_detected_edge_us_ = 0;
+    this->last_valid_high_edge_us_ = 0;
+    this->last_valid_low_edge_us_ = 0;
+    this->has_detected_edge_ = false;
+    this->has_valid_high_edge_ = false;
+    this->has_valid_low_edge_ = false;
   }
 
   // We quantize our pulse widths to 1 ms to avoid unnecessary jitter
   const uint32_t pulse_width_ms = this->pulse_width_us_ / 1000;
   if (this->pulse_width_dedupe_.next(pulse_width_ms)) {
     if (pulse_width_ms == 0) {
-      // Treat 0 pulse width as 0 pulses/min (normally because we've not detected any pulses for a while)
+      // Treat 0 pulse width as 0 pulses/min (normally because we've not
+      // detected any pulses for a while)
       this->publish_state(0);
     } else {
       // Calculate pulses/min from the pulse width in ms
@@ -96,9 +77,11 @@ void PulseMeterSensor::dump_config() {
 }
 
 void IRAM_ATTR PulseMeterSensor::gpio_intr(PulseMeterSensor *sensor) {
-  // This is an interrupt handler - we can't call any virtual method from this method
+  // This is an interrupt handler - we can't call any virtual method from this
+  // method
 
-  // Get the current time before we do anything else so the measurements are consistent
+  // Get the current time before we do anything else so the measurements are
+  // consistent
   const uint32_t now = micros();
 
   // We only look at rising edges in EDGE mode, and all edges in PULSE mode
@@ -106,7 +89,45 @@ void IRAM_ATTR PulseMeterSensor::gpio_intr(PulseMeterSensor *sensor) {
     if (sensor->isr_pin_.digital_read()) {
       sensor->last_detected_edge_us_ = now;
     }
+  }
+
+  // Check to see if we should filter this edge out
+  if (sensor->filter_mode_ == FILTER_EDGE) {
+    if ((sensor->last_detected_edge_us_ - sensor->last_valid_high_edge_us_) >= sensor->filter_us_) {
+      // Don't measure the first valid pulse (we need at least two pulses to
+      // measure the width)
+      if (sensor->has_valid_high_edge_) {
+        sensor->pulse_width_us_ = (sensor->last_detected_edge_us_ - sensor->last_valid_high_edge_us_);
+      }
+      sensor->total_pulses_++;
+      sensor->last_valid_high_edge_us_ = sensor->last_detected_edge_us_;
+      sensor->has_valid_high_edge_ = true;
+    }
   } else {
+    // Filter Mode is PULSE
+    bool pin_val = sensor->isr_pin_.digital_read();
+    // Ignore false edges that may be caused by bouncing and exit the ISR ASAP
+    if (pin_val == sensor->sensor_is_high_) {
+      return;
+    }
+    // Make sure the signal has been stable long enough
+    if (sensor->has_detected_edge_ && (now - sensor->last_detected_edge_us_ >= sensor->filter_us_)) {
+      if (pin_val) {
+        sensor->has_valid_high_edge_ = true;
+        sensor->last_valid_high_edge_us_ = sensor->last_detected_edge_us_;
+        sensor->sensor_is_high_ = true;
+      } else {
+        // Count pulses when a sufficiently long high pulse is concluded.
+        sensor->total_pulses_++;
+        if (sensor->has_valid_low_edge_) {
+          sensor->pulse_width_us_ = sensor->last_detected_edge_us_ - sensor->last_valid_low_edge_us_;
+        }
+        sensor->has_valid_low_edge_ = true;
+        sensor->last_valid_low_edge_us_ = sensor->last_detected_edge_us_;
+        sensor->sensor_is_high_ = false;
+      }
+    }
+    sensor->has_detected_edge_ = true;
     sensor->last_detected_edge_us_ = now;
   }
 }

esphome/components/pulse_meter/pulse_meter_sensor.h

@@ -1,8 +1,8 @@
 #pragma once
 
+#include "esphome/components/sensor/sensor.h"
 #include "esphome/core/component.h"
 #include "esphome/core/hal.h"
-#include "esphome/components/sensor/sensor.h"
 #include "esphome/core/helpers.h"
 
 namespace esphome {
@@ -42,11 +42,14 @@ class PulseMeterSensor : public sensor::Sensor, public Component {
   Deduplicator<uint32_t> total_dedupe_;
 
   volatile uint32_t last_detected_edge_us_ = 0;
-  volatile uint32_t last_valid_low_edge_us_ = 0;
   volatile uint32_t last_valid_high_edge_us_ = 0;
+  volatile uint32_t last_valid_low_edge_us_ = 0;
   volatile uint32_t pulse_width_us_ = 0;
   volatile uint32_t total_pulses_ = 0;
   volatile bool sensor_is_high_ = false;
+  volatile bool has_detected_edge_ = false;
+  volatile bool has_valid_high_edge_ = false;
+  volatile bool has_valid_low_edge_ = false;
 };
 
 }  // namespace pulse_meter