From fa555ba5a0cb34dc75c4ba92f2a6b1b8cffa7be6 Mon Sep 17 00:00:00 2001 From: Chris Nussbaum Date: Fri, 31 Jul 2026 16:48:19 -0500 Subject: [PATCH] Distrust stale current readings; measure runtime from flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An ESPHome sensor keeps serving its last value indefinitely once its source stops responding, and nothing downstream can tell the difference. On two consecutive mornings the PZEM stopped returning valid Modbus frames for exactly the hour both pumps ran (04:00-05:00), and because its readings froze rather than disappearing, everything believed them: Pump No Current reported a fault for a solid hour while the pump was drawing ~1500 W, runtime stopped accumulating for a pump that was running, and when the frames came back at 05:00:02 the current sensor jumped from a frozen value to a live one in a single sample — spiking the spread estimate past the variance threshold and setting the anomaly flag chattering 285 times. The energy registers appeared to dump a whole hour at once for the same reason: the meter had been counting internally all along, only the reads were dead. The device never disconnected and the recorder was healthy throughout, so the only signal that could have distinguished a live reading from a frozen one was when it last arrived. Nothing was tracking that. This does not fix the underlying dropout, which is still unexplained and most likely electrical. It stops the controller drawing false conclusions from dead data, and makes the outage visible as itself. Changes: - Track the arrival time of every current-sensor publish and declare the reading stale after current_timeout (default 30s). Add a current_stale_binary_sensor so the real fault is visible as itself. - While stale: hold motor_running_ rather than recomputing it from a frozen value, suppress no_current (a silent sensor supports no claim about the pump), and stop sampling for anomalies. - On recovery, clear the debounce streak and the spread estimate so the step change back to live data cannot trip the variance detector, and discard the inrush peak of any run whose startup window contained a gap. - Measure runtime from the flow sensor whenever one is configured, falling back to current and then to the output command. Moving water is what the schedule is actually buying, and flow stays valid through a current-sensor outage. - Split "a new run started" from "runtime is accumulating". The anomaly startup window must stay anchored to electrical turn-on, since flow lags the motor by long enough to miss the inrush peak entirely. turned_on_ms_ is now 0 until a sensor confirms the motor, which also stops a stale recovery from restarting the window mid-run and recording steady-state current as inrush. - Document the local ESP-IDF toolchain pitfalls in CLAUDE.md, and stop the commit_pr command pointing at the upstream esphome PR workflow. Verified with esphome compile on an ESP32-S3 build of pool.yaml: clean, no warnings from any touched file. Co-Authored-By: Claude --- .claude/commands/commit_pr.md | 15 ++++- CLAUDE.md | 6 +- components/pool_controller/README.md | 30 ++++++++- components/pool_controller/__init__.py | 2 +- .../pool_controller/auxiliary_pump_switch.cpp | 5 ++ components/pool_controller/const.py | 2 + .../pool_controller/primary_pump_switch.cpp | 5 ++ components/pool_controller/pump_anomaly.cpp | 16 +++-- components/pool_controller/pump_current.cpp | 66 ++++++++++++++++++- components/pool_controller/pump_current.h | 16 +++++ components/pool_controller/pump_flow.cpp | 25 ++++--- components/pool_controller/pump_switch.cpp | 33 ++++++---- components/pool_controller/pump_switch.h | 52 +++++++++++++-- components/pool_controller/schema/pump.py | 33 ++++++++++ components/pool_controller/schema/types.py | 3 + 15 files changed, 272 insertions(+), 37 deletions(-) diff --git a/.claude/commands/commit_pr.md b/.claude/commands/commit_pr.md index ad86ee5..4858fc9 100644 --- a/.claude/commands/commit_pr.md +++ b/.claude/commands/commit_pr.md @@ -4,8 +4,21 @@ description: Run pre-commit hooks to a clean state, commit, push, open a PR, mer Do everything the `commit` command does, then take the change all the way through a merged PR and back to a clean `master`. +Every step below operates on **this repo only** (`nuttytree/ESPHome-Devices`, default branch `master`). Never push to, or open a PR against, `esphome/esphome` or any other upstream — the sibling `dev_esphome/` and `esphome.io/` working directories are reference checkouts of other people's projects, not targets for this workflow. + 1. Follow the `commit` command's steps 1–5 in full: branch check/creation (`f/`/`b/`/`chore/` naming, never commit on `master`), `python -m pre_commit run --all-files` to a clean state, the commit itself (with the `Co-Authored-By: Claude ` trailer), and the push (`git push` / `git push -u origin `). Never skip hooks, never work around a real hook failure. -2. Open the PR using the `pr-workflow` skill so it follows this repo's established PR conventions (title, body, labels, etc.) rather than reinventing them here. +2. Open the PR **against this repo only** — `nuttytree/ESPHome-Devices`, base branch `master`: + + ``` + gh pr create --base master --title "" --body-file - + ``` + + > **Do not use the `pr-workflow` skill here, and never target `esphome/esphome`.** That skill lives in the sibling `dev_esphome/` checkout (upstream ESPHome core, wired in only for IntelliSense — see CLAUDE.md) and describes how to contribute *to upstream*: it says to base on `upstream/dev`, fill in ESPHome's `.github/PULL_REQUEST_TEMPLATE.md`, prefix the title with `[component]`, and run `gh pr create --repo esphome/esphome --base dev`. All of that is wrong for this repo. It surfaces as an available skill purely because `dev_esphome/` is an additional working directory. Ignore it; the conventions below are this repo's. + + This repo has no PR template and uses no labels. Match the style of recent merged PRs (`gh pr list --state merged --limit 5`, then `gh pr view <n> --json body`): + - **Title**: sentence-case description of the change, no bracket/component prefix (e.g. "Fix pump anomaly detection: startup window, latching, drift, variance"). + - **Body**: a prose statement of the problem and why it mattered, then a `Changes:` bullet list of what was actually done. Reference related PRs/issues with `#<n>` where relevant, and call out anything that silently changes behaviour on upgrade (e.g. persisted state being discarded). + - Note how the change was verified (`esphome config <device>.yaml`, or an `esphome compile` if the C++ changed). 3. Merge the PR: this repo only allows squash merges (`allow_squash_merge: true`; `allow_merge_commit`/`allow_rebase_merge` are both `false`), so merge with `gh pr merge <number> --squash`. `master` has no required status checks configured, so the merge should go through immediately — if `gh` reports it can't merge (conflicts, an unexpected required check, etc.), stop and report rather than forcing it. Don't pass `--delete-branch`: the repo already has `delete_branch_on_merge` enabled, so GitHub deletes the remote branch itself on merge. 4. Switch back to `master`: `git checkout master`. 5. Sync `master`: `git pull` (should fast-forward cleanly since nothing else is committed directly to `master`). diff --git a/CLAUDE.md b/CLAUDE.md index 5fdba88..92fdd18 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,7 +53,11 @@ Installed via `pipx install esphome`, which creates its own isolated virtualenv **2. Build to a local drive, not the UNC share.** This repo lives on `\\nas\docker\esphome\config`, and `packages/device_base.yaml` sets `build_path: ./build/${device_id}`, which resolves onto that share. `cmd.exe` cannot use a UNC path as a working directory, so ninja's *link* step runs from `C:\Windows` and fails with `ld.exe: cannot open output file ...: Permission denied`. The compile steps succeed first, which makes this read as a linker bug rather than a path problem. Override `build_path` to a local path (e.g. `C:/eh/bld/<name>`) when compiling from Windows. Source files can stay on the share — gcc handles UNC paths fine; only the working directory is the problem. -To compile-check a component without disturbing the container's build caches, copy the device YAML to a throwaway `device_id` with a local `build_path`, compile that copy, then delete both the scratch YAML and its build directory. A full IDF build takes roughly 10–15 minutes. +**3. Don't run any other `esphome` command while a compile is in progress.** Even `esphome config` rewrites state under `.esphome/`, and because that directory is shared with the build, cmake sees a changed input and re-runs configure mid-build. The initial configure succeeds off the resolved dependency lock, but a *re-run* re-resolves component manifests and dies on `.esphome/pio_components/idf/*/esphome/noise-c/idf_component.yml`, which contains `override_path: /config/.esphome/...` — a container path, written by the Docker ESPHome where this directory is mounted at `/config`. Windows resolves it against the UNC root as `\\nas\docker\config\...`, which doesn't exist, and the build dies with `The "override_path" field ... does not point to a directory`. That cache is gitignored and regenerable, so leave it alone rather than "fixing" it — the container needs those paths. Just validate before you build, then stay off the config directory until it finishes. + +Similarly, don't kill a compile during its managed-component download: a partial `lvgl/lvgl` left without its `.component_hash` fails every later build with `File .component_hash or CHECKSUMS.json ... does not exist or cannot be parsed`. Delete the build directory and start over. + +To compile-check a component without disturbing the container's build caches, copy the device YAML to a throwaway `device_id` with a local `build_path`, compile that copy, then delete both the scratch YAML and its build directory. A full IDF build takes roughly 10–15 minutes. Note that `esphome config` validates schema and lambdas' *syntax* but does not compile lambda C++ — only a real compile catches errors inside a `lambda:` block. ## Git workflow diff --git a/components/pool_controller/README.md b/components/pool_controller/README.md index f9da549..37fb40b 100644 --- a/components/pool_controller/README.md +++ b/components/pool_controller/README.md @@ -79,9 +79,11 @@ Accepts all standard [ESPHome Switch options](https://esphome.io/components/swit * **end_time** (Required, time): Window end on the hour; use `'24:00'` for midnight end-of-day. * **minutes_per_hour** (Required, int, 1–60): How many minutes out of each 60 the pump should run within this window. * **days_of_week** (Optional): Days this runtime applies to (e.g. `MON-FRI`, `SAT,SUN`). Defaults to every day. -* **current_sensor** (Optional, id): ID of a sensor reporting current draw in amps. Required when `on_anomaly` is configured. When set, turning the switch on always publishes an on state and energizes the output, but the pump's runtime is only accumulated while current confirms the motor is actually running. +* **current_sensor** (Optional, id): ID of a sensor reporting current draw in amps. Required when `on_anomaly` is configured. Turning the switch on always publishes an on state and energizes the output; current then confirms whether the motor is actually turning, which drives the anomaly startup window, the no-current sensor, and the flow-loss watchdog. It also accumulates runtime, but only when there is no `flow_sensor` — see [Runtime Accounting](#runtime-accounting). * **current_on_threshold** (Optional, float, default: `0.5`): Current in amps above which the pump is considered running (used whenever `current_sensor` is configured). -* **no_current_binary_sensor** (Optional): Problem binary sensor, automatically added whenever `current_sensor` is configured. Turns on whenever the switch is commanded on but no current is detected (e.g. a manual override switch has the pump physically off). Supports all standard [ESPHome Binary Sensor options](https://esphome.io/components/binary_sensor/index.html); defaults to `<pump name> No Current`. +* **no_current_binary_sensor** (Optional): Problem binary sensor, automatically added whenever `current_sensor` is configured. Turns on whenever the switch is commanded on but no current is detected (e.g. a manual override switch has the pump physically off). Suppressed while the current reading is stale — see [Stale Current Readings](#stale-current-readings). Supports all standard [ESPHome Binary Sensor options](https://esphome.io/components/binary_sensor/index.html); defaults to `<pump name> No Current`. +* **current_timeout** (Optional, Time, default: `30s`): How long `current_sensor` may go without publishing before its last value stops being treated as evidence. See [Stale Current Readings](#stale-current-readings). +* **current_stale_binary_sensor** (Optional): Problem binary sensor, automatically added whenever `current_sensor` is configured. Turns on when the current sensor has published nothing for `current_timeout`, meaning its readings are frozen rather than merely low. Supports all standard [ESPHome Binary Sensor options](https://esphome.io/components/binary_sensor/index.html); defaults to `<pump name> Current Sensor Stale`. * **anomaly_detection_switch** (Optional): A Switch entity, created whenever `current_sensor` is configured, that enables or disables anomaly detection at runtime. Supports all standard [ESPHome Switch options](https://esphome.io/components/switch/index.html); restores to on by default and defaults to `<pump name> Anomaly Detection`. * **anomaly_status_binary_sensor** (Optional): Problem binary sensor, created whenever `current_sensor` is configured, that turns on when a current-draw anomaly is currently active. Supports all standard [ESPHome Binary Sensor options](https://esphome.io/components/binary_sensor/index.html); defaults to `<pump name> Anomaly Status`. * **anomaly_baseline_reset_button** (Optional): A Button entity, created whenever `current_sensor` is configured, that discards the learned current baseline (steady state, drift, variance, startup inrush) and clears any latched anomaly. The press is never refused and can be made at any time; relearning does not start immediately but at the pump's next turn-on, so the new baseline is always built from a complete run — see [Baseline Reset](#baseline-reset). Supports all standard [ESPHome Button options](https://esphome.io/components/button/index.html); defaults to `<pump name> Reset Anomaly Baseline`. @@ -89,7 +91,7 @@ Accepts all standard [ESPHome Switch options](https://esphome.io/components/swit * **anomaly_threshold_pct** (Optional, int 1–100, default: `10`): Percentage deviation from baseline required to trigger an anomaly event. * **learning_samples** (Optional, int 10–10000, default: `200`): Number of steady-state current samples to collect before the baseline is locked. * **on_anomaly** (Optional, automation): Automation triggered when an anomaly is detected. The trigger variable `x` is a string describing the anomaly: `CURRENT_HIGH`, `CURRENT_LOW`, `NO_STARTUP_SPIKE`, `BASELINE_DRIFT` (either direction — rising suggests bearing wear, falling suggests prime/flow loss), or `VARIANCE_SPIKE` (current spread has grown well beyond its learned baseline, often the earliest sign of trouble). -* **flow_sensor** (Optional, id): ID of a binary sensor that detects water flow. If no `current_sensor` is configured, the pump's runtime is only accumulated while flow is actually detected. If a `current_sensor` is configured, runtime is controlled by current instead (see above), and the pump is additionally shut down if current confirms the motor is running but flow is lost for longer than `flow_timeout`. Without a `current_sensor`, no-flow alone never shuts the pump down — a manual override switch physically holding the pump off would otherwise look identical to a real no-flow fault. When both sensors are configured, flow also gates anomaly statistics — see [Current-based Anomaly Detection](#current-based-anomaly-detection). +* **flow_sensor** (Optional, id): ID of a binary sensor that detects water flow. Whenever it is configured it owns runtime accounting, current sensor or not — see [Runtime Accounting](#runtime-accounting). If a `current_sensor` is also configured, the pump is additionally shut down when current confirms the motor is running but flow is lost for longer than `flow_timeout`. Without a `current_sensor`, no-flow alone never shuts the pump down — a manual override switch physically holding the pump off would otherwise look identical to a real no-flow fault. Flow also gates anomaly statistics — see [Current-based Anomaly Detection](#current-based-anomaly-detection). * **flow_timeout** (Optional, Time, default: `2s`): How long flow must be absent (while a current sensor confirms the motor is running) before the pump is shut down. * **flow_loss_binary_sensor** (Optional): Problem binary sensor, automatically added whenever both `current_sensor` and `flow_sensor` are configured. Latches on when the pump is shut down due to lost flow, and only clears once the pump is on, current confirms the motor is running, and flow is detected again. Supports all standard [ESPHome Binary Sensor options](https://esphome.io/components/binary_sensor/index.html); defaults to `<pump name> Flow Loss`. * **unexpected_flow_binary_sensor** (Optional): Problem binary sensor, automatically added whenever `flow_sensor` is configured. Turns on when flow is detected while the pump is commanded off for longer than `flow_timeout` (e.g. a stuck valve, a neighboring pump pushing water through this branch, or a manual override running the pump), and clears as soon as flow stops or the pump turns on. Supports all standard [ESPHome Binary Sensor options](https://esphome.io/components/binary_sensor/index.html); defaults to `<pump name> Unexpected Flow`. @@ -149,5 +151,27 @@ Capture is then *armed rather than started*. Nothing is sampled until the pump n Relearning then follows the normal path: the first run seeds the startup reference, `NO_STARTUP_SPIKE` checks resume once three starts have accumulated, and the steady-state baseline locks after `learning_samples` valid samples. +### Runtime Accounting +Two different questions get asked about a running pump, and they want different answers: + +* **How long did the pump do useful work?** Runtime, which the fractional-runtime scheduler spends against `minutes_per_hour`. Moving water is what a pool schedule is actually buying, so this is measured from `flow_sensor` whenever one is configured, falling back to `current_sensor`, and finally to the output command. Flow is also the more robust source: a current sensor that stalls while reading `0 A` would otherwise stop the clock on a pump that is running perfectly well. +* **When was the motor energised?** The anchor for the anomaly startup window, the auxiliary `sequence_delay`, and the heater's warm-up gate. This is measured from `current_sensor` whenever one is configured, because flow lags the motor by a second or more — easily long enough to miss the inrush peak the startup check exists to measure. + +The two are tracked independently. Between the output being commanded on and the first sensor confirming the motor, the turn-on timestamp reads as "no run time yet" rather than as an elapsed time, so nothing that gates on it fires early. + +### Stale Current Readings +An ESPHome sensor keeps serving its last value indefinitely when its source stops responding — there is no outward difference between a live reading and a frozen one. A meter that has gone silent while reporting `0 A` is byte-identical to a pump that genuinely isn't drawing current, and everything downstream believes it: the no-current sensor reports a fault that isn't happening, runtime stops accumulating for a pump that is actually running, and anomaly detection measures a flat line. + +Each pump therefore timestamps every publish from its `current_sensor`. If nothing arrives for `current_timeout`, the reading is declared stale and: + +* `current_stale_binary_sensor` turns on, so the real fault — a dead sensor — is visible and distinguishable in history from a pump fault. +* `motor_running` is no longer recomputed. The last known state is held rather than inferred from a frozen number, so runtime tracking neither invents a stop nor invents a start. +* `no_current_binary_sensor` is suppressed. "Commanded on but drawing nothing" is a claim about the pump, and a sensor that stopped publishing supports no such claim. +* Anomaly detection stops sampling, since every sample would be the same stale number. + +When readings resume, the first value is a step change away from whatever was frozen — potentially a very large one. Feeding that straight into the detector reads as an enormous deviation and sets the variance trip chattering while the estimate re-converges, so the out-of-band debounce streak and the live spread estimate are both cleared on recovery. A run whose startup window contained a gap also has its inrush peak discarded (logged as a warning) rather than averaged into the startup reference, since the window was only partly observed. + +Note that the flow-loss watchdog still acts on the held `motor_running` state. That is deliberate: dry-running is the expensive failure, so a stale current reading should not disarm the protection that stops it. + ### Flow Sensor Protection If a `flow_sensor` is configured and reports no flow for longer than `flow_timeout` while the pump output is commanded on, the pump is shut down to protect against dry-running or blockage. diff --git a/components/pool_controller/__init__.py b/components/pool_controller/__init__.py index 9fffbd3..8057dcb 100644 --- a/components/pool_controller/__init__.py +++ b/components/pool_controller/__init__.py @@ -43,7 +43,7 @@ cv.Optional( CONF_STATE_SAVE_INTERVAL, default="5min" ): cv.positive_time_period_milliseconds, - cv.Optional(CONF_MAX_RESUME_AGE, default="15min"): cv.All( + cv.Optional(CONF_MAX_RESUME_AGE, default="10min"): cv.All( cv.positive_time_period_seconds, cv.Range(min=cv.TimePeriod(seconds=1)), ), diff --git a/components/pool_controller/auxiliary_pump_switch.cpp b/components/pool_controller/auxiliary_pump_switch.cpp index d5869d4..ef15f3d 100644 --- a/components/pool_controller/auxiliary_pump_switch.cpp +++ b/components/pool_controller/auxiliary_pump_switch.cpp @@ -35,9 +35,14 @@ void AuxiliaryPumpSwitch::write_state(bool state) { return; } + if (state) + this->arm_new_run_(); this->output_->set_state(state); this->publish_state(state); if (!this->has_current_sensor() && !this->has_flow_sensor()) { + // No sensors at all — the command is the only evidence available for either job. + if (state) + this->turned_on_ms_ = millis_64(); this->track_runtime(state); } else if (!state) { // Output commanded off — stop runtime counting immediately regardless of sensor confirmation. diff --git a/components/pool_controller/const.py b/components/pool_controller/const.py index 3041da3..9feccc0 100644 --- a/components/pool_controller/const.py +++ b/components/pool_controller/const.py @@ -24,6 +24,8 @@ CONF_CURRENT_ON_THRESHOLD = "current_on_threshold" CONF_NO_CURRENT_BINARY_SENSOR = "no_current_binary_sensor" +CONF_CURRENT_TIMEOUT = "current_timeout" +CONF_CURRENT_STALE_BINARY_SENSOR = "current_stale_binary_sensor" CONF_FLOW_SENSOR = "flow_sensor" CONF_FLOW_TIMEOUT = "flow_timeout" diff --git a/components/pool_controller/primary_pump_switch.cpp b/components/pool_controller/primary_pump_switch.cpp index 5320288..4622fe1 100644 --- a/components/pool_controller/primary_pump_switch.cpp +++ b/components/pool_controller/primary_pump_switch.cpp @@ -52,9 +52,14 @@ void PrimaryPumpSwitch::write_state(bool state) { } } + if (state) + this->arm_new_run_(); this->output_->set_state(state); this->publish_state(state); if (!this->has_current_sensor() && !this->has_flow_sensor()) { + // No sensors at all — the command is the only evidence available for either job. + if (state) + this->turned_on_ms_ = millis_64(); this->track_runtime(state); } else if (!state) { // Output commanded off — stop runtime counting immediately regardless of sensor confirmation. diff --git a/components/pool_controller/pump_anomaly.cpp b/components/pool_controller/pump_anomaly.cpp index e94f218..25995c3 100644 --- a/components/pool_controller/pump_anomaly.cpp +++ b/components/pool_controller/pump_anomaly.cpp @@ -61,6 +61,12 @@ void PumpSwitch::tick_anomaly_() { if (this->awaiting_fresh_start_) return; + // No confirmed turn-on for this run yet, so there is no startup window to measure against. + // Guards run_ms below, where a 0 timestamp would read as "running since boot" and skip + // straight past the inrush window. + if (this->turned_on_ms_ == 0) + return; + const float current = this->current_sensor_->state; if (std::isnan(current) || current < 0.0f) return; @@ -86,10 +92,12 @@ void PumpSwitch::tick_anomaly_() { if (this->startup_capture_valid_()) { this->process_startup_peak_(); } else { - // The motor drew its inrush but never moved water, so this peak says nothing about a - // healthy start. Discard it rather than averaging it into the startup reference. - ESP_LOGW(TAG, "'%s' startup peak %.3fA discarded: no flow established during startup", this->get_name().c_str(), - this->startup_peak_current_); + // The peak says nothing about a healthy start — either the motor drew its inrush without + // ever moving water, or the readings gapped and the window went partly unobserved. + // Discard it rather than averaging it into the startup reference. + ESP_LOGW( + TAG, "'%s' startup peak %.3fA discarded: %s", this->get_name().c_str(), this->startup_peak_current_, + this->run_current_gap_ ? "current readings went stale during startup" : "no flow established during startup"); } } diff --git a/components/pool_controller/pump_current.cpp b/components/pool_controller/pump_current.cpp index d879476..24adae0 100644 --- a/components/pool_controller/pump_current.cpp +++ b/components/pool_controller/pump_current.cpp @@ -3,6 +3,7 @@ #include "esphome/core/hal.h" #include "esphome/core/log.h" +#include <cinttypes> #include <cmath> namespace esphome { @@ -11,7 +12,10 @@ namespace pool_controller { static const char *const TAG = "pool_controller.switch"; void PumpSwitch::update_no_current_() { - const bool fault = this->has_current_sensor() && this->state && !this->motor_running_; + // Suppressed while the reading is stale: "commanded on but no current" is a claim about the + // pump, and a sensor that stopped publishing supports no such claim. The stale sensor carries + // the alarm instead, so the two faults stay distinguishable in the UI and in history. + const bool fault = this->has_current_sensor() && this->state && !this->motor_running_ && !this->current_stale_; if (fault == this->no_current_detected_) return; this->no_current_detected_ = fault; @@ -19,11 +23,56 @@ void PumpSwitch::update_no_current_() { this->no_current_sensor_->publish_state(fault); } +void PumpSwitch::set_current_stale_(bool stale) { + if (stale == this->current_stale_) + return; + this->current_stale_ = stale; + if (this->current_stale_sensor_ != nullptr) + this->current_stale_sensor_->publish_state(stale); + // no_current is derived from staleness, so re-evaluate it on every transition. + this->update_no_current_(); +} + #ifdef USE_SENSOR +bool PumpSwitch::update_current_stale_(uint64_t now) { + const bool stale = (now - this->last_current_update_ms_) >= this->current_timeout_ms_; + if (stale == this->current_stale_) { + if (stale && this->runtime_start_ms_ != 0) + this->run_current_gap_ = true; + return stale; + } + + if (stale) { + ESP_LOGW(TAG, "'%s' current sensor has not published for %" PRIu32 " ms — readings distrusted", + this->get_name().c_str(), this->current_timeout_ms_); + // A gap during a run means that run's startup window was not fully observed. + if (this->runtime_start_ms_ != 0) + this->run_current_gap_ = true; + } else { + ESP_LOGI(TAG, "'%s' current sensor is publishing again", this->get_name().c_str()); + // The first reading back is a step change from a value that was frozen, possibly for a long + // time. Fed into the detector as-is it is a huge apparent deviation: the spread EWMA spikes + // past the variance threshold and the flag chatters while it re-converges. Clear both the + // debounce streak and the spread estimate so detection restarts from live data instead. + this->oob_streak_ = 0; + this->spread_ema_ = 0.0f; + } + + this->set_current_stale_(stale); + return stale; +} + void PumpSwitch::update_current_sensor_(uint64_t now) { // ── Current sensor processing (1 Hz) ────────────────────────────────────── if (now - this->last_sample_ms_ >= 1000) { this->last_sample_ms_ = now; + + // Nothing below may run on a stale reading: motor_running_ would be recomputed from a frozen + // value, which either invents a no-current fault or silently stops accumulating runtime for + // a pump that is in fact running. Holding the last known state is the honest option. + if (this->update_current_stale_(now)) + return; + const float current = this->current_sensor_->state; // Current-based runtime tracking: state always reflects the output command. @@ -35,7 +84,15 @@ void PumpSwitch::update_current_sensor_(uint64_t now) { motor_running ? "YES" : "NO", this->state ? "ON" : "OFF", current, motor_running ? ">=" : "<", this->current_on_threshold_); this->motor_running_ = motor_running; - this->track_runtime(motor_running); + // Anchor the anomaly startup window to electrical turn-on. Only stamped when it is still + // 0 (this run hasn't been confirmed running yet), so regaining sight of a motor that + // never stopped — after a stale period — cannot restart the window mid-run and record + // steady-state current as if it were inrush. + if (motor_running && this->turned_on_ms_ == 0) + this->turned_on_ms_ = millis_64(); + // Runtime follows flow whenever a flow sensor is configured; see update_flow_based_runtime_(). + if (!this->has_flow_sensor()) + this->track_runtime(motor_running); this->update_no_current_(); } } @@ -53,6 +110,11 @@ void PumpNoCurrentBinarySensor::setup() { Component::setup(); this->publish_initial_state(false); } + +void PumpCurrentStaleBinarySensor::setup() { + Component::setup(); + this->publish_initial_state(false); +} #endif // USE_SENSOR } // namespace pool_controller diff --git a/components/pool_controller/pump_current.h b/components/pool_controller/pump_current.h index 7c75269..207e8dd 100644 --- a/components/pool_controller/pump_current.h +++ b/components/pool_controller/pump_current.h @@ -18,5 +18,21 @@ class PumpNoCurrentBinarySensor : public binary_sensor::BinarySensor, public Com PumpSwitch *pump_{nullptr}; }; +/// Problem sensor: true while the current sensor has stopped publishing. +/// +/// An ESPHome sensor keeps its last value forever when its source stops responding, so a dead +/// meter is indistinguishable from a genuine reading — a stalled sensor reporting 0 A looks +/// exactly like a pump that isn't drawing current. Everything downstream then acts on an hour-old +/// number: the no-current sensor cries fault, runtime stops accumulating, and anomaly detection +/// measures noise. This sensor makes the difference visible and gates those consumers. +class PumpCurrentStaleBinarySensor : public binary_sensor::BinarySensor, public Component { + public: + void setup() override; + void set_pump(PumpSwitch *pump) { this->pump_ = pump; } + + protected: + PumpSwitch *pump_{nullptr}; +}; + } // namespace pool_controller } // namespace esphome diff --git a/components/pool_controller/pump_flow.cpp b/components/pool_controller/pump_flow.cpp index 931abd1..9a95030 100644 --- a/components/pool_controller/pump_flow.cpp +++ b/components/pool_controller/pump_flow.cpp @@ -26,17 +26,24 @@ void PumpSwitch::set_unexpected_flow_(bool detected) { this->unexpected_flow_sensor_->publish_state(detected); } -// Only used when a flow sensor is configured without a current sensor — in that case -// runtime is only accumulated while the output is on AND flow is actually detected. -// When a current sensor is also present, runtime is controlled by current instead. +// Whenever a flow sensor is configured it owns runtime accounting, current sensor or not. +// Moving water is what a pool schedule is actually buying, and unlike current it stays valid +// through a current-sensor outage — a stalled meter reading 0 A would otherwise silently stop +// the clock on a pump that is running perfectly well. void PumpSwitch::update_flow_based_runtime_(uint64_t now) { const bool flow_running = this->state && this->flow_sensor_->state; - if (flow_running != this->flow_running_) { - ESP_LOGD(TAG, "'%s' flow running state: %s (output=%s)", this->get_name().c_str(), flow_running ? "YES" : "NO", - this->state ? "ON" : "OFF"); - this->flow_running_ = flow_running; - this->track_runtime(flow_running); - } + if (flow_running == this->flow_running_) + return; + + ESP_LOGD(TAG, "'%s' flow running state: %s (output=%s)", this->get_name().c_str(), flow_running ? "YES" : "NO", + this->state ? "ON" : "OFF"); + this->flow_running_ = flow_running; + // With no current sensor, flow is also the only evidence the motor is turning, so it has to + // anchor the turn-on timestamp as well. When current is available it does that job instead, + // because it sees the motor a second or more earlier. + if (!this->has_current_sensor() && flow_running && this->turned_on_ms_ == 0) + this->turned_on_ms_ = millis_64(); + this->track_runtime(flow_running); } void PumpSwitch::update_flow_watchdogs_(uint64_t now) { diff --git a/components/pool_controller/pump_switch.cpp b/components/pool_controller/pump_switch.cpp index 7e7c722..29684e4 100644 --- a/components/pool_controller/pump_switch.cpp +++ b/components/pool_controller/pump_switch.cpp @@ -35,6 +35,12 @@ void PumpSwitch::setup() { #ifdef USE_SENSOR if (this->current_sensor_ != nullptr) { + // Stamp every publish. A sensor whose source has died keeps serving its last value with no + // outward sign, so the arrival time of a reading is the only thing separating a live + // measurement from a frozen one. Start the clock now so boot gets a full grace period. + this->last_current_update_ms_ = millis_64(); + this->current_sensor_->add_on_state_callback([this](float value) { this->last_current_update_ms_ = millis_64(); }); + this->anomaly_pref_ = this->make_entity_preference<AnomalyBaseline>(ANOMALY_PREF_VERSION); if (this->anomaly_pref_.load(&this->anomaly_baseline_)) { this->sample_count_ = this->anomaly_baseline_.sample_count; @@ -144,18 +150,24 @@ void PumpSwitch::restore_run_state(const ESPTime &now, uint32_t max_age_s) { (slot_hour == state.slot_hour) ? "" : " (new slot, reset)", remaining_ms / 1000); } +void PumpSwitch::arm_new_run_() { + // 0 means "commanded on, not yet confirmed running". Whichever sensor first sees the motor + // stamps the real timestamp; until then get_running_for_ms() correctly reports no run time. + this->turned_on_ms_ = 0; + // Reset per-run anomaly state so each pump cycle gets a fresh startup check. + this->startup_peak_current_ = 0.0f; + this->startup_processed_ = false; + this->oob_streak_ = 0; + this->run_flow_confirmed_ = false; + this->run_current_gap_ = false; + // This is the clean start a pending baseline reset was waiting for: capture may resume, + // and it now begins at a real turn-on with the startup window still ahead of it. + this->awaiting_fresh_start_ = false; +} + void PumpSwitch::track_runtime(bool new_state) { if (new_state && this->runtime_start_ms_ == 0) { this->runtime_start_ms_ = millis_64(); - this->turned_on_ms_ = millis_64(); - // Reset per-run anomaly state so each pump cycle gets a fresh startup check. - this->startup_peak_current_ = 0.0f; - this->startup_processed_ = false; - this->oob_streak_ = 0; - this->run_flow_confirmed_ = false; - // This is the clean start a pending baseline reset was waiting for: capture may resume, - // and it now begins at a real turn-on with the startup window still ahead of it. - this->awaiting_fresh_start_ = false; this->last_off_utc_ = 0; // Snapshot the start immediately: a restart moments from now needs to know the pump was // mid-run, which is what lets it resume without waiting out the minimum off-time. @@ -212,8 +224,7 @@ void PumpSwitch::loop() { #endif if (this->flow_sensor_ != nullptr) { - if (!this->has_current_sensor()) - this->update_flow_based_runtime_(now); + this->update_flow_based_runtime_(now); this->update_flow_watchdogs_(now); } } diff --git a/components/pool_controller/pump_switch.h b/components/pool_controller/pump_switch.h index 7350ea8..bf13351 100644 --- a/components/pool_controller/pump_switch.h +++ b/components/pool_controller/pump_switch.h @@ -131,6 +131,14 @@ class PumpSwitch : public switch_::Switch, public Component { void set_current_on_threshold(float threshold) { this->current_on_threshold_ = threshold; } /// Problem sensor: true while the pump is commanded on but no current is detected. void set_no_current_sensor(binary_sensor::BinarySensor *sensor) { this->no_current_sensor_ = sensor; } + /// How long the current sensor may go without publishing before its last value stops being + /// treated as evidence. Default: 30 s. + void set_current_timeout_ms(uint32_t ms) { this->current_timeout_ms_ = ms; } + /// Problem sensor: true while the current sensor has stopped publishing. + void set_current_stale_sensor(binary_sensor::BinarySensor *sensor) { this->current_stale_sensor_ = sensor; } + + /// Returns true while the current sensor's last value is too old to be trusted. + bool is_current_stale() const { return this->current_stale_; } // ── Flow sensor ──────────────────────────────────────────────────────────── /// Optional binary sensor that detects water flow. @@ -171,8 +179,21 @@ class PumpSwitch : public switch_::Switch, public Component { /// Resets accumulated runtime to zero. Called by PoolController at :00. void reset_runtime(); - /// Call this in write_state() before setting the output so runtime is tracked correctly. + /// Accumulates runtime. Driven by the best available evidence that the pump is doing work: + /// flow when a flow sensor is configured, otherwise current, otherwise the output command. + /// Flow wins because moving water is what the schedule is actually buying, and because it + /// stays valid through a current-sensor outage — see [Stale Current Readings] in the README. void track_runtime(bool new_state); + + /// Marks the start of a new run, at the moment the output is commanded on. Clears the per-run + /// anomaly state and sets turned_on_ms_ to 0, meaning "commanded on, not yet confirmed + /// running"; whichever sensor first confirms the motor stamps the real timestamp. + /// + /// Separate from track_runtime() because the two answer different questions. Runtime asks how + /// long the pump did useful work, which flow answers best. The anomaly startup window asks + /// when the motor was energised, which only current answers — flow lags the motor by a second + /// or more, easily long enough to miss the inrush peak entirely. + void arm_new_run_(); void set_anomaly_detected_(bool detected); /// Persists the current operating state with a wall-clock timestamp. No-op until the clock is @@ -241,6 +262,8 @@ class PumpSwitch : public switch_::Switch, public Component { ///< from the middle of whatever run was in progress at reset time. bool run_flow_confirmed_{false}; ///< True once flow has been observed during this run's startup window; ///< only meaningful when a flow sensor is configured. + bool run_current_gap_{false}; ///< True if the current sensor went stale at any point during this run, + ///< meaning its startup window was not fully observed. ESPPreferenceObject anomaly_pref_; ///< Persists AnomalyBaseline across reboots. binary_sensor::BinarySensor *anomaly_status_sensor_{nullptr}; text_sensor::TextSensor *anomaly_reason_sensor_{nullptr}; ///< Last anomaly reason string (diagnostic). @@ -253,6 +276,18 @@ class PumpSwitch : public switch_::Switch, public Component { binary_sensor::BinarySensor *no_current_sensor_{nullptr}; ///< Problem sensor: on but no current detected. bool no_current_detected_{false}; ///< Last published state of no_current_sensor_. + /// Staleness watchdog. An ESPHome sensor holds its last value indefinitely when its source + /// stops responding, so "0 A" from a dead meter is byte-identical to a pump that genuinely + /// isn't drawing current. Tracking when the sensor last published is the only way to tell + /// them apart, so every consumer of current can stop acting on a value that is no longer real. + uint32_t current_timeout_ms_{30000}; ///< Silence after which the current reading is distrusted. + uint64_t last_current_update_ms_{0}; ///< millis_64() of the last publish from the current sensor. + bool current_stale_{false}; ///< Last published state of current_stale_sensor_. + binary_sensor::BinarySensor *current_stale_sensor_{nullptr}; ///< Problem sensor: current sensor gone quiet. + + /// Sets (or clears) the stale-current problem sensor and re-evaluates what depends on it. + void set_current_stale_(bool stale); + /// Returns true when a current sensor is configured for this pump. #ifdef USE_SENSOR bool has_current_sensor() const { return this->current_sensor_ != nullptr; } @@ -283,16 +318,23 @@ class PumpSwitch : public switch_::Switch, public Component { /// the caller has already confirmed the motor is drawing it (motor_running_). bool stats_capture_allowed_() const { return !this->has_flow_sensor() || this->flow_sensor_->state; } - /// Whether this run's inrush peak is worth folding into the startup reference. With a flow - /// sensor, only a start that actually got water moving counts; a motor that spun up against - /// a closed valve or lost prime produces a peak that would poison the reference. - bool startup_capture_valid_() const { return !this->has_flow_sensor() || this->run_flow_confirmed_; } + /// Whether this run's inrush peak is worth folding into the startup reference. A gap in the + /// current readings means the window was only partly observed, so the "peak" is whatever + /// happened to be sampled either side of it. With a flow sensor, only a start that actually + /// got water moving counts either; a motor that spun up against a closed valve or lost prime + /// produces a peak that would poison the reference. + bool startup_capture_valid_() const { + return !this->run_current_gap_ && (!this->has_flow_sensor() || this->run_flow_confirmed_); + } #ifdef USE_SENSOR /// Samples the current sensor at 1 Hz: updates motor_running_, runtime tracking, the /// no-current problem sensor, and (if enabled) ticks anomaly detection. Implemented in /// pump_current.cpp. Extracted verbatim from the top of loop(). void update_current_sensor_(uint64_t now); + /// Re-evaluates staleness at the 1 Hz sample tick. Returns true when the reading may not be + /// used, in which case the caller must leave motor_running_ and runtime tracking untouched. + bool update_current_stale_(uint64_t now); void tick_anomaly_(); ///< Called at 1 Hz while the pump is running. Implemented in pump_anomaly.cpp. void process_startup_peak_(); ///< Evaluates the inrush peak captured during the startup window. Implemented in ///< pump_anomaly.cpp. diff --git a/components/pool_controller/schema/pump.py b/components/pool_controller/schema/pump.py index b438d68..83fe19f 100644 --- a/components/pool_controller/schema/pump.py +++ b/components/pool_controller/schema/pump.py @@ -31,6 +31,8 @@ CONF_ON_ANOMALY, CONF_CURRENT_ON_THRESHOLD, CONF_NO_CURRENT_BINARY_SENSOR, + CONF_CURRENT_TIMEOUT, + CONF_CURRENT_STALE_BINARY_SENSOR, CONF_FLOW_SENSOR, CONF_FLOW_TIMEOUT, CONF_FLOW_LOSS_BINARY_SENSOR, @@ -45,6 +47,7 @@ PumpAnomalyReasonTextSensor, PumpAnomalyResetButton, PumpNoCurrentBinarySensor, + PumpCurrentStaleBinarySensor, PumpFlowLossBinarySensor, PumpUnexpectedFlowBinarySensor, PumpAnomalyTrigger, @@ -71,6 +74,9 @@ DEFAULT_NO_CURRENT_BINARY_SENSOR_PREFIX = ( "__pool_controller_no_current_binary_sensor_default_name__" ) +DEFAULT_CURRENT_STALE_BINARY_SENSOR_PREFIX = ( + "__pool_controller_current_stale_binary_sensor_default_name__" +) DEFAULT_FLOW_LOSS_BINARY_SENSOR_PREFIX = ( "__pool_controller_flow_loss_binary_sensor_default_name__" ) @@ -154,6 +160,21 @@ def _validate_sensor_config(config): device_class=DEVICE_CLASS_PROBLEM, icon="mdi:flash-off", ), + cv.Optional(CONF_CURRENT_TIMEOUT, default="30s"): cv.All( + cv.positive_time_period_milliseconds, + cv.Range(min=cv.TimePeriod(seconds=1)), + ), + cv.Optional( + CONF_CURRENT_STALE_BINARY_SENSOR, + default=lambda: { + CONF_NAME: f"{DEFAULT_CURRENT_STALE_BINARY_SENSOR_PREFIX}{uuid.uuid4()}" + }, + ): binary_sensor.binary_sensor_schema( + PumpCurrentStaleBinarySensor, + entity_category=ENTITY_CATEGORY_DIAGNOSTIC, + device_class=DEVICE_CLASS_PROBLEM, + icon="mdi:help-network-outline", + ), cv.Optional(CONF_ANOMALY_THRESHOLD_PCT, default=10): cv.int_range(min=1, max=100), cv.Optional(CONF_LEARNING_SAMPLES, default=200): cv.int_range(min=10, max=10000), cv.Optional(CONF_ON_ANOMALY): automation.validate_automation( @@ -292,6 +313,18 @@ async def pump_to_code(var, pump_config, delay_ms, disable_sensor, rtc): await cg.register_component(no_current_sensor, no_current_conf) cg.add(var.set_no_current_sensor(no_current_sensor)) + cg.add(var.set_current_timeout_ms(pump_config[CONF_CURRENT_TIMEOUT])) + + stale_conf = pump_config[CONF_CURRENT_STALE_BINARY_SENSOR] + if stale_conf.get(CONF_NAME, "").startswith( + DEFAULT_CURRENT_STALE_BINARY_SENSOR_PREFIX + ): + stale_conf[CONF_NAME] = f"{pump_config[CONF_NAME]} Current Sensor Stale" + stale_sensor = cg.new_Pvariable(stale_conf[CONF_ID]) + await binary_sensor.register_binary_sensor(stale_sensor, stale_conf) + await cg.register_component(stale_sensor, stale_conf) + cg.add(var.set_current_stale_sensor(stale_sensor)) + if CONF_FLOW_SENSOR in pump_config: flow_sens = await cg.get_variable(pump_config[CONF_FLOW_SENSOR]) cg.add(var.set_flow_sensor(flow_sens)) diff --git a/components/pool_controller/schema/types.py b/components/pool_controller/schema/types.py index 67660d6..d311dc5 100644 --- a/components/pool_controller/schema/types.py +++ b/components/pool_controller/schema/types.py @@ -34,6 +34,9 @@ PumpNoCurrentBinarySensor = pool_controller_ns.class_( "PumpNoCurrentBinarySensor", binary_sensor.BinarySensor, cg.Component ) +PumpCurrentStaleBinarySensor = pool_controller_ns.class_( + "PumpCurrentStaleBinarySensor", binary_sensor.BinarySensor, cg.Component +) PumpFlowLossBinarySensor = pool_controller_ns.class_( "PumpFlowLossBinarySensor", binary_sensor.BinarySensor, cg.Component )