From 8e319058bc5b71c559afd3d9fe04916584b6d84f Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 19:00:25 +0200 Subject: [PATCH 1/9] docs(011): spec, plan, tasks for WateringController port (PR-11) Spec-kit artifacts for PR-11 (pure, 100%-host-tested WateringController + ReservoirController), approved at Checkpoint 2. Records the resolved decisions: soak gate enforced (from burst END), manual cap 300s, QUIRK-1 mode semantics, reservoir truth table + invalid-row + post-abort cooldown (FR-012a, deliberate divergence), fail-safe never delayed by the gate, 30s staleness, snapshot-helper enablers, periodic soil reader. No implementation yet. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../checklists/requirements.md | 42 +++ .../contracts/reservoir-controller.md | 46 +++ .../contracts/snapshot-helpers.md | 43 +++ .../contracts/watering-controller.md | 38 +++ .../data-model.md | 129 ++++++++ .../plan.md | 136 +++++++++ .../quickstart.md | 62 ++++ .../research.md | 118 ++++++++ .../spec.md | 275 ++++++++++++++++++ .../tasks.md | 178 ++++++++++++ 10 files changed, 1067 insertions(+) create mode 100644 specs/011-watering-controller-host-tests/checklists/requirements.md create mode 100644 specs/011-watering-controller-host-tests/contracts/reservoir-controller.md create mode 100644 specs/011-watering-controller-host-tests/contracts/snapshot-helpers.md create mode 100644 specs/011-watering-controller-host-tests/contracts/watering-controller.md create mode 100644 specs/011-watering-controller-host-tests/data-model.md create mode 100644 specs/011-watering-controller-host-tests/plan.md create mode 100644 specs/011-watering-controller-host-tests/quickstart.md create mode 100644 specs/011-watering-controller-host-tests/research.md create mode 100644 specs/011-watering-controller-host-tests/spec.md create mode 100644 specs/011-watering-controller-host-tests/tasks.md diff --git a/specs/011-watering-controller-host-tests/checklists/requirements.md b/specs/011-watering-controller-host-tests/checklists/requirements.md new file mode 100644 index 0000000..e985df2 --- /dev/null +++ b/specs/011-watering-controller-host-tests/checklists/requirements.md @@ -0,0 +1,42 @@ +# Specification Quality Checklist: Watering Controller (host-tested application logic) + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-07-05 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- The three pre-made decisions (soak-gate enforcement, manual 300 s cap, QUIRK-1 mode semantics) and the + parity constants (30 s staleness, 300 s reservoir cap, 5 min data-log, 30/55 % thresholds, 20 s/300 s + defaults, implausible=high-without-low) are recorded as requirements/assumptions from the PRD + parity + §1–§3 — not open questions, so no [NEEDS CLARIFICATION] markers were needed. +- The booked implementation enablers (locked-sensor snapshot helpers, soil-mock coherence helpers, + rs485test race fix, periodic soil reader) are captured in Assumptions/Dependencies as plan-time enablers + of FR-004/FR-016/edge-cases, not as behavioral requirements. +- The controller is deliberately specified as pure/host-tested logic (US1/US2 = the CI deliverable) with a + distinct on-target integration story (US3) — matching the master-PRD "100 % host-tested" criterion. diff --git a/specs/011-watering-controller-host-tests/contracts/reservoir-controller.md b/specs/011-watering-controller-host-tests/contracts/reservoir-controller.md new file mode 100644 index 0000000..ddf6e64 --- /dev/null +++ b/specs/011-watering-controller-host-tests/contracts/reservoir-controller.md @@ -0,0 +1,46 @@ +# Contract: `ReservoirController` (pure state machine) + +`firmware/components/control/include/control/ReservoirController.h` + `src/ReservoirController.cpp`. Pure, +board-independent; host-tested regardless of board (the `BOARD_HAS_RESERVOIR_PUMP` flag gates only +construction/wiring in `app_main`, never the logic — FR-013). + +## Construction +`ReservoirController(ILevelSensor& lowMark, ILevelSensor& highMark, IWaterPump& fillPump, +ITimeProvider& clock, EventLogger& events)` (or the `Locked*` wrappers). Fill duration + the 300 s cap come +from the pump (`runFor` / max-runtime). + +## Public surface +| Member | Contract | +|---|---| +| `void tick(bool enabled, bool autoLevelControl)` | `fillPump.update()` (cap) → running-safety (stop on high / cap) → if enabled+auto+not-running, evaluate the truth table. | +| `bool startManualFill(int durationS)` | manual fill (refuse if already full = high wet); capped at 300 s. | +| `void stop()` | `fillPump.stop()`. | + +## Truth table (auto; enabled, auto-level, pump not running) +| low mark | high mark | action | +|---|---|---| +| invalid, or high invalid | — | **no action** (invalid never = dry — ILevelSensor contract) | +| wet | wet | full → ensure stopped | +| wet | dry | sufficient → no action | +| dry | dry | **start fill** (`fillPump.runFor(fill)`) | +| dry | wet | physically implausible → **no action** | + +## Running safety (manual + auto) +- high mark reads wet → `stop()` immediately. +- `fillPump.update()` aborts at the 300 s hard max fill. + +## Post-abort cooldown (resolved by Paul 2026-07-05 — deliberate divergence from parity) +After a fill ends by the max-runtime abort (`StopReason::MaxRuntimeForced`), record the abort time and do +NOT start another automatic fill until a cooldown (`kReservoirRefillCooldownMs`, a documented constant, +default ~60 s, tunable) has elapsed — even if the water still reads low. This prevents a stuck high sensor +or an empty source from producing an endless 300 s pump cycle. A normal stop (high-wet reached) does NOT +trigger the cooldown. Manual fill is unaffected. + +## Feature gate (FR-013) +`!enabled` (or the board lacks the pump) → `fillPump.stop()` (force off) + skip all logic. + +## Host-test targets (SC-004) +Every truth-table row incl. both invalid-sensor and implausible → no-action; start-on-low-dry; +stop-on-high-wet while running; max-fill abort at 300 s; disabled forces off; after-abort re-evaluation is +safe (no tight re-slam). Driven over two `MockLevelSensor` + `MockWaterPump` (real `WaterPump` over +`FakeTimeProvider`). diff --git a/specs/011-watering-controller-host-tests/contracts/snapshot-helpers.md b/specs/011-watering-controller-host-tests/contracts/snapshot-helpers.md new file mode 100644 index 0000000..246f312 --- /dev/null +++ b/specs/011-watering-controller-host-tests/contracts/snapshot-helpers.md @@ -0,0 +1,43 @@ +# Contract: consistent-snapshot helpers on the Locked* sensor wrappers + +Resolves the booked `TODO(PR-11)` on `LockedEnvironmentalSensor`/`LockedLevelSensor` (+ the documented +cross-call gap on `LockedSoilSensor`). Each adds ONE locked call returning validity + values together, so a +reader (controller / periodic soil reader / API status / console) never observes a torn read from the +`read()`-then-getters two-lock sequence. Target-safe (`std::mutex`); the underlying pure sensors are +unchanged. + +## Helpers (added to the existing wrappers) +```cpp +// LockedSoilSensor +struct SoilSnapshot { bool readOk; bool available; int lastError; + float moisture, temperature, humidity, ph, ec, nitrogen, phosphorus, potassium; }; +SoilSnapshot snapshot(); // one lock: read() (or last-good) + all getters + availability + error + +// LockedEnvironmentalSensor +struct EnvSnapshot { bool valid; float temperature, humidity, pressure; }; +EnvSnapshot snapshot(); + +// LockedLevelSensor +struct LevelSnapshot { bool valid; bool waterPresent; }; +LevelSnapshot snapshot(); // update() need not be here; snapshot reads current validity + logical state +``` + +Semantics: the snapshot is taken under a single mutex acquisition so `valid`/`readOk` and the values are +mutually consistent. For soil, whether `snapshot()` performs a fresh `read()` or returns the last-good +snapshot is decided at implementation — the periodic reader owns the `read()` cadence; the controller/API +consume the last-good snapshot (non-blocking; no bus I/O in a status/API path — QUIRK 5 still holds). + +## Consumers updated +- `WateringController` + `ReservoirController` read via these snapshots (no two-lock races). +- The API `/sensors` + `/status` and the diag console read via the snapshot (consistent tuple). +- `sensor_task` may switch its read+error to the snapshot (resolving the `sensor_task.cpp:79` TODO). + +## rs485test race fix (D8) +The diag-console `rs485test` currently drives the raw `EspModbusClient` beneath `LockedSoilSensor`'s mutex. +Once the periodic soil reader runs, that is a real RS485 bus race. Route `rs485test` through a locked path +(via the sensor or a locked client) so the two tasks never drive the UART/Modbus concurrently. + +## MockSoilSensor coherence +Add `scriptSuccessfulRead(moisture, temp, humidity, ph, ec, n, p, k)` and `scriptFailedRead(error)` to +`MockSoilSensor` (mirroring `MockEnvironmentalSensor`) so host tests script coherent soil state (readOk + +values + availability move together) instead of hand-setting fields into an incoherent combination. diff --git a/specs/011-watering-controller-host-tests/contracts/watering-controller.md b/specs/011-watering-controller-host-tests/contracts/watering-controller.md new file mode 100644 index 0000000..19420e4 --- /dev/null +++ b/specs/011-watering-controller-host-tests/contracts/watering-controller.md @@ -0,0 +1,38 @@ +# Contract: `WateringController` (pure) + +`firmware/components/control/include/control/WateringController.h` + `src/WateringController.cpp`. Pure C++17 +over interfaces + injected clock; NO IDF includes; host-tested over mocks + `FakeTimeProvider`/`FakeWallClock`. + +## Construction (references injected; must outlive the controller) +`WateringController(ISoilSensor& soil, IWaterPump& plant, IConfigStore& config, IDataStorage& storage, +ITimeProvider& clock, IWallClock& wallClock, EventLogger& events)` — or the `Locked*` wrappers at the +wiring site. No watering thresholds hard-coded — all read from `config` each evaluation (runtime-tunable). + +## Public surface (proposed) +| Member | Contract | +|---|---| +| `void tick()` | one evaluation: `plant.update()` (self-stop/cap) → read soil snapshot → fail-safe → watering decision → periodic data-log. Non-blocking; called at a fixed cadence from the controller task. | +| `bool startManual(int durationS)` | operator override: `plant.runFor(clamp 1..300)`, set `manualRunActive`; returns the pump result. Exempt from automatic fail-safe. | +| `void stop()` | `plant.stop()`; clears `manualRunActive`. Always works, any mode. | +| (status getters as needed for the API/console) | current mode, running, last-burst/last-valid times — for reporting. | + +## Behavioral contract (host-test targets — SC-001) +- **Automatic start**: enabled + not running + valid moisture ≤ low + soak elapsed → `runFor(burst)`. +- **Soak gate (FR-003)**: after a burst ends, no new automatic burst until `soakPause` elapsed, even if soil + ≤ low. Assert: no start at soak-1ms; start at soak. Manual is exempt. +- **Stop at target (FR-002)**: running + moisture ≥ high → `stop()`. +- **Fail-safe (FR-005/006)**: in automatic, running, when soil unavailable OR stale (>30 000 ms / never) OR + moisture out of 0..100 → immediate `stop()` + `logFailsafe`, no watering decision. Checked BEFORE the soak + gate — assert a pending soak pause does not delay a safety stop. +- **Gate on read (FR-004)**: before the first successful read, no action on placeholder values. +- **Manual (FR-007/008/009/010)**: a manual run continues despite sensor failure; capped at 300 s; + auto-started runs count as automatic (fail-safe applies); `stop()` clears the override. +- **Graceful degradation (FR-015)**: constructs + allows manual even if a sensor failed to init (pump + + storage present). +- **Logging (FR-014)**: every `dataLogInterval`, log env + soil (NPK only ≥ 0) with `nowEpoch()`; gate + validity on `isTimeSet()`. Fail-safe events via `logFailsafe`; pump transitions NOT logged here + (owned by `SystemObserver`). + +## Isolation (FR-017) +The controller only calls the injected interfaces; it makes no hardware/network calls and no blocking I/O. +On-target it runs on a watchdog-registered task, never blocking or blocked by network/HTTP. diff --git a/specs/011-watering-controller-host-tests/data-model.md b/specs/011-watering-controller-host-tests/data-model.md new file mode 100644 index 0000000..59800eb --- /dev/null +++ b/specs/011-watering-controller-host-tests/data-model.md @@ -0,0 +1,129 @@ +# Phase 1 Data Model: Watering Controller + +Pure in-memory state + the exact decision tables. All timing via injected `ITimeProvider::nowMs()` +(monotonic) for gates/staleness and `IWallClock::nowEpoch()` for log timestamps. No persisted schema new to +this PR (config → PR-06 NVS, history/events → PR-06 littlefs). + +## Constants (parity §1–§3 + config-store defaults) + +| Name | Value | Source | +|---|---|---| +| moisture low / high threshold | 30 % / 55 % (config) | `IConfigStore` defaults | +| burst (watering) duration | 20 s (config, 1..300) | `IConfigStore` | +| soak pause (min interval) | 300 s (config, ≥1) | `IConfigStore` (enforced — divergence) | +| staleness window | 30 000 ms | parity §2 | +| reservoir max fill | 300 s (pump cap) | parity §3 | +| data-log cadence | 300 000 ms (5 min) | `IConfigStore` (≥60 000) | +| pump hard max runtime | 300 s | `WaterPump::kDefaultMaxRunTimeMs` | + +## WateringController state + +| Field | Type | Meaning | +|---|---|---| +| mode | derived | automatic when `getWateringEnabled()` and no active operator run; else manual/override | +| lastBurstEndMs | int64 | monotonic time the last automatic burst ended (soak gate origin) | +| lastValidSoilMs | int64 | monotonic time of the last successful, in-range soil read (0 = never) | +| manualRunActive | bool | an operator-started run is in progress (bypasses fail-safe) | +| lastDataLogMs | int64 | monotonic time of the last data-log write | + +## Automatic evaluation (per controller tick; pump `update()` runs first for self-stop/cap) + +Order is safety-first; the soak gate is checked LAST and never gates a safety stop. + +```text +tick(): + pump.update() # enforce timed self-stop + 300 s cap (actuator layer) + soil = lockedSoil.snapshot() # {readOk, available, moisture, ...} one locked read + + if manualRunActive: # operator override — bypass automatic safety + gate + if !pump.isRunning(): manualRunActive = false # run ended -> back to automatic + return + + if !getWateringEnabled(): # manual mode (suspended automatic) + return + + # ---- FAIL-SAFE (unconditional, FR-005/006) ---- + stale = (lastValidSoilMs == 0) || (now - lastValidSoilMs > 30_000) + invalid = soil.readOk && (soil.moisture < 0 || soil.moisture > 100) + if !soil.available || stale || invalid: + if pump.isRunning(): pump.stop(); logFailsafe(reason) + return # no watering decision + + if soil.readOk: lastValidSoilMs = now + else: return # gate on read result, not placeholder (FR-004) + + # ---- WATERING DECISION ---- + if pump.isRunning(): + if soil.moisture >= high: pump.stop(); lastBurstEndMs = now # reached target + # else keep running (within a burst) + else: + if soil.moisture <= low: + soakElapsed = (lastBurstEndMs == 0) || (now - lastBurstEndMs >= soakPauseMs) + if soakElapsed: pump.runFor(burstDurationS) # start next burst + # else: soak pause active -> do NOT start (FR-003), even though dry +``` + +Transitions: +- **start burst**: enabled + not running + moisture ≤ low + soak elapsed → `runFor(burst)`. +- **stop at target**: running + moisture ≥ high → `stop()`, set `lastBurstEndMs=now`. +- **burst self-stop**: the pump's `update()` stops it at `burstDurationS`; the controller observes + `!isRunning()` next tick and (if still ≤ low) waits out the soak pause before the next burst. +- **fail-safe stop**: any of unavailable/stale/invalid while running (automatic) → immediate `stop()`. +- **manual**: `startManual(duration≤300)` → `runFor`, `manualRunActive=true`; exempt from fail-safe; `stop()` + clears it. + +Invariants: the soak gate NEVER blocks a `stop()` (safety or high-threshold). Manual is never blocked by +automatic logic. `lastValidSoilMs` advances only on a successful in-range read. + +## ReservoirController state machine (pure; board-independent) + +Inputs: `low.snapshot()` + `high.snapshot()` (each {valid, waterPresent}), `enabled`, `pump`. + +Evaluate only when `enabled` and pump not running: + +| low | high | action | +|---|---|---| +| !valid OR high !valid | — | **no action** (invalid ≠ dry) | +| wet | wet | full → ensure stopped | +| wet | dry | sufficient → no action | +| dry | dry | **`pump.runFor(fillDuration)`** (start fill) | +| dry | wet | implausible → **no action** | + +While the fill pump runs (regardless of how started): +- high mark reads wet → `pump.stop()` immediately. +- `pump.update()` aborts at the 300 s max fill (pump cap). + +Feature gate: `!enabled` (or `!BOARD_HAS_RESERVOIR_PUMP`) → `pump.stop()` (force off), skip all logic. + +**Post-abort cooldown (resolved 2026-07-05):** when a fill ends via `StopReason::MaxRuntimeForced`, record +`lastAbortMs`; suppress a new automatic fill while `now - lastAbortMs < kReservoirRefillCooldownMs` (a +documented constant, default ~60 s, tunable) even if still low-dry — prevents an endless 300 s cycle on a +stuck high sensor / empty source. A normal high-wet stop does NOT arm the cooldown. Manual fill bypasses it. +Reservoir state adds `lastAbortMs` (int64, 0 = none). + +## Snapshot helper shape (added to Locked* wrappers, resolves TODO(PR-11)) + +```cpp +struct SoilSnapshot { bool readOk; bool available; int lastError; float moisture, temperature, humidity, + ph, ec, nitrogen, phosphorus, potassium; }; // one locked call +struct EnvSnapshot { bool valid; float temperature, humidity, pressure; }; +struct LevelSnapshot{ bool valid; bool waterPresent; }; +``` +One mutex acquisition returns validity + values together, closing the read()-then-getters cross-call gap. + +## Logging (FR-014, D9) + +- Every `dataLogInterval`: `storeSensorReading("env_temperature", nowEpoch, …)` etc. + soil metrics; NPK + only when ≥ 0. Timestamp = `IWallClock::nowEpoch()`; if `!isTimeSet()`, do not treat it as a valid + wall-clock time (log with the best-known / mark unsynced — no bogus 1970 as valid). +- Events: `logFailsafe("soil-unavailable" | "soil-stale" | "moisture-invalid" | "reservoir-…")`; + pump start/stop transitions remain owned by `SystemObserver` (no double-log). + +## Host-test matrix (must cover — SC-001) + +Plant: start-at-low; no-start-in-soak; restart-after-soak-while-dry; stop-at-high; fail-safe +unavailable/stale(>30 s)/invalid each stops+blocks; fail-safe-not-delayed-by-soak; gate-on-read (no act +before first read); manual bypasses fail-safe; manual capped 300 s; stop clears override; graceful +degradation (sensor init fail). Reservoir: all 5 truth-table rows; stop-on-high; max-fill abort; feature +disabled forces off. Logging: cadence + epoch + NPK≥0 + time-not-set. Config: setter validation + +runtime change picked up next tick. diff --git a/specs/011-watering-controller-host-tests/plan.md b/specs/011-watering-controller-host-tests/plan.md new file mode 100644 index 0000000..8273486 --- /dev/null +++ b/specs/011-watering-controller-host-tests/plan.md @@ -0,0 +1,136 @@ +# Implementation Plan: Watering Controller (host-tested application logic) + +**Branch**: `011-watering-controller-host-tests` | **Date**: 2026-07-05 | **Spec**: [spec.md](./spec.md) + +**Input**: `specs/011-watering-controller-host-tests/spec.md`; PR brief +`docs/prd/PR-11-watering-controller-host-tests.md`; parity `docs/parity-checklist.md` §1–§3 (+ QUIRK 1/2). + +## Summary + +Port the watering application logic to a **pure, host-tested** `WateringController` (+ a pure reservoir +state machine) in a new `control` component, consuming only the merged interfaces + an injected clock — zero +IDF includes, 100 % host-tested in CI. The controller adds the **soak-pause gate** (enforced +minimum-watering-interval, measured from burst end — the deliberate divergence), the **fail-safe stops** +(unavailable / stale > 30 s / invalid moisture in automatic mode, never delayed by the gate), **manual +override** (= mode flag, capped 300 s), the **reservoir truth table** (with the invalid + implausible +"do-not-act" rows), and **data logging** (5 min, epoch, time-not-set aware). On-target, a controller task +(watchdog-registered) plus a periodic soil reader replace PR-09's interim direct-drive path and make soil +data live. Enablers land alongside: consistent-snapshot helpers on the locked sensor wrappers (the booked +`TODO(PR-11)`), coherent soil-mock helpers, and routing `rs485test` through a locked path. + +## Technical Context + +**Language/Version**: C++17, ESP-IDF v6.0.1 (Docker), target esp32. The controller + reservoir logic are +pure (host-buildable on the linux preview target); no `esp_*`/`esp_timer` — time is injected. + +**Primary Dependencies**: project interfaces only — `ISoilSensor`, `IEnvironmentalSensor`, `ILevelSensor` +(×2), `IWaterPump`, `IConfigStore`, `IDataStorage`, `ITimeProvider`, `IWallClock`; `EventLogger` (pure). +Their `Locked*` wrappers + mocks + `FakeTimeProvider`/`FakeWallClock` back the tests. No new managed deps. + +**Storage**: reads config via `IConfigStore` (thresholds/duration/soak/enabled/intervals); logs history + +events via `IDataStorage` (epoch from `IWallClock`). + +**Testing**: Unity host suite — `test_watering_controller.cpp` (+ `test_reservoir.cpp`) over the mocks + +fakes; every decision/safety branch covered (CI gate, master-PRD 100 %-host-tested criterion). On-target +wiring (controller task, periodic soil reader, snapshot helpers, rs485test) is HIL-verified. + +**Target Platform**: ESP32-WROOM-32E; `BOARD_REV1_DEVKIT` (plant + reservoir) / `BOARD_REV2` (plant only; +level sensors status-only). Reservoir logic host-tested on both; wiring gated by `BOARD_HAS_RESERVOIR_PUMP`. + +**Project Type**: Embedded firmware component (`control`) + `main/` task wiring. + +**Performance Goals**: controller cadence well under the 20 s watchdog; fail-safe stop within the 30 s +staleness window; watering fully independent of network/HTTP (Constitution I / FR-017). + +**Constraints**: `pumps_force_off()` stays first in `app_main`; controller makes no hardware calls except +through interfaces; manual/burst capped at 300 s (pump's own cap); soak gate never delays a safety stop; +both targets build; controller must not double-log pump transitions with `SystemObserver`. + +**Scale/Scope**: one pure `WateringController` + one pure `ReservoirController` (state machine), snapshot +helpers on 4 locked wrappers, soil-mock helpers, a `soil_reader` task (or extend `sensor_task`), controller +task wiring in `app_main`, `rs485test` fix, 2 host suites. This is an L PR. + +## Constitution Check + +*GATE: evaluated before Phase 0 and re-checked after Phase 1. Result: PASS (no violations).* + +- **I. Safety First (NON-NEGOTIABLE)** — PASS and central: the fail-safe stops (FR-005/006), the manual + bypass (FR-007), the reservoir caps + invalid-do-not-act (FR-011/012), and "safety stop never delayed by + the soak gate" (FR-006) are the core deliverable, ALL host-tested (SC-001). `pumps_force_off()` stays the + first `app_main` action; the controller runs on a watchdog-registered task isolated from network (FR-017). +- **II. Host-Testability** — PASS, maximally: the entire controller + reservoir state machine are pure over + interfaces + an injected clock, giving the master-PRD "100 % host-tested watering logic". No IDF includes; + hardware stays behind the existing drivers/wrappers. +- **III. Reproducible Builds** — PASS. No new managed deps; both targets build; `dependencies.lock` + untouched. +- **IV. Frozen Legacy** — PASS. `src/`/`include/`/`data/`/`test/`/`platformio.ini` are read-only reference + (the legacy `WateringController.cpp`/`main.cpp` behavior is *ported*, not modified). +- **V. Checkpoint-Gated Workflow** — PASS. Implementation delegated to `implementer` after CP2. +- **VI. English Outward** — PASS. + +**Deliberate-divergence notes (recorded so they don't surface as review findings):** the soak gate is +enforced (frozen code doesn't), measured from burst END; manual runs are capped at 300 s (frozen code is +uncapped); "manual" is the mode flag (`wateringEnabled==false`), not a pump flag (the new `IWaterPump` has +no `isManualMode()`); QUIRK-1 mode semantics are corrected (auto-started = automatic/fail-safe-subject, +operator-started = manual/bypass). The reservoir state machine adds the "sensor invalid → do not act" row +the frozen code lacks. + +## Project Structure + +### Documentation (this feature) + +```text +specs/011-watering-controller-host-tests/ +├── plan.md · research.md · data-model.md · quickstart.md +├── contracts/{watering-controller.md, reservoir-controller.md, snapshot-helpers.md} +├── checklists/requirements.md +└── tasks.md # /speckit-tasks output (not created here) +``` + +### Source Code (repository root) + +```text +firmware/ +├── components/ +│ ├── control/ # NEW component (pure, host + target) +│ │ ├── CMakeLists.txt # REQUIRES interfaces events; pure — no linux guard needed +│ │ ├── include/control/ +│ │ │ ├── WateringController.h # pure: automatic decision + soak gate + fail-safe + manual + logging +│ │ │ ├── ReservoirController.h # pure: level truth table + fill timer (board-independent) +│ │ │ └── ControllerConfig.h? # (optional) snapshot/param structs +│ │ └── src/{WateringController.cpp, ReservoirController.cpp} +│ ├── sensors/include/sensors/ +│ │ ├── LockedSoilSensor.h # EDIT — add consistent-snapshot helper (read+values+validity, one lock) +│ │ ├── LockedEnvironmentalSensor.h# EDIT — snapshot helper (resolve TODO(PR-11)) +│ │ ├── LockedLevelSensor.h # EDIT — snapshot helper (resolve TODO(PR-11)) +│ │ └── testing/MockSoilSensor.h # EDIT — add scriptSuccessfulRead/scriptFailedRead (coherence) +│ └── (LockedPowerSensor.h snapshot TODO — optional, power not used by the controller) +├── main/ +│ ├── watering_task.{h,cpp} # NEW — controller task (watchdog-registered) OR fold into main loop +│ ├── soil_reader_task.{h,cpp} # NEW — periodic soil read (or extend sensor_task to soil) +│ ├── app_main.cpp # EDIT — construct WateringController + ReservoirController; start +│ │ # the tasks; API mode flag drives it; keep pumps_force_off first +│ └── diag_console.cpp # EDIT — route rs485test through a locked path (fix the PR-11 race) +└── test_apps/host/main/ + ├── test_watering_controller.cpp # NEW — automatic + soak + fail-safe + manual + logging branches + ├── test_reservoir.cpp # NEW — reservoir truth table + caps + invalid/implausible rows + ├── test_main.cpp · CMakeLists.txt # EDIT — register run_watering_controller_tests/run_reservoir_tests; REQUIRES control +``` + +**Structure Decision**: A new pure `control` component holds `WateringController` + `ReservoirController` +(both depend only on `interfaces` + `EventLogger`; no linux guard needed — fully portable), giving the +master-PRD "100 % host-tested" logic. The reservoir state machine is a separate pure class so it is tested +independently of the board wiring (the `BOARD_HAS_RESERVOIR_PUMP` flag gates only construction in +`app_main`). The snapshot helpers are added to the existing `Locked*` wrappers (resolving the booked +`TODO(PR-11)` and giving the controller/periodic-reader torn-read-free access). The controller runs on a +dedicated watchdog-registered task in `main/` (mirroring `sensor_task`); a periodic soil reader (a small +task, or `sensor_task` extended to soil) makes soil data live. The API's mode flag drives the controller +indirectly (it reads `getWateringEnabled()` each cycle — no direct ApiServer↔controller call path). + +## Complexity Tracking + +> No constitution violations. Table intentionally empty. + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|--------------------------------------| +| — | — | — | diff --git a/specs/011-watering-controller-host-tests/quickstart.md b/specs/011-watering-controller-host-tests/quickstart.md new file mode 100644 index 0000000..1677352 --- /dev/null +++ b/specs/011-watering-controller-host-tests/quickstart.md @@ -0,0 +1,62 @@ +# Quickstart / Validation Guide: Watering Controller + +CI-tagged items gate the merge (this PR's deliverable is the host-tested logic). HIL-tagged items are a +smoke on the rev1 rig at Checkpoint 3. + +## Prerequisites + +- ESP-IDF v6.0.1 via Docker; rsync to `/tmp` first (Docker can't mount OneDrive): + `rsync -a --delete --exclude build --exclude managed_components --exclude sdkconfig "$PWD/firmware/" /tmp/ws011-firmware/` +- Between board builds: `idf.py fullclean` + `rm -f sdkconfig`. If the host build dir is stale from a + failed run: `rm -rf test_apps/host/build` before `set-target linux`. + +## CI validation (host tests — the core deliverable + both board builds) + +### Host tests — `run_watering_controller_tests()` + `run_reservoir_tests()` (100 % of decision/safety branches, SC-001) +Drive the pure controllers over the mocks + `FakeTimeProvider`/`FakeWallClock`: +- **Automatic**: start at ≤ low; no start during soak; restart after soak while still dry; stop at ≥ high; + gate-on-read (no action before first successful read). +- **Fail-safe**: soil unavailable / stale (>30 000 ms / never) / moisture out-of-range each stops the pump + and blocks watering in automatic; a pending soak pause does NOT delay a safety stop; manual bypasses. +- **Manual**: continues despite sensor failure; capped at 300 s; `stop()` clears the override; auto-started + runs are automatic (fail-safe applies). +- **Reservoir**: all 5 truth-table rows (incl. invalid-sensor + implausible → no action); start on low-dry; + stop on high-wet; max-fill abort at 300 s; feature disabled forces off. +- **Logging**: data-log cadence + epoch timestamp + NPK≥0; time-not-set handled (no bogus 1970 as valid). +- **Config**: setter validation; a runtime config change is picked up on the next tick. + +```bash +docker run --rm -v /tmp/ws011-firmware:/project -w /project espressif/idf:v6.0.1 \ + bash -lc "cd test_apps/host && idf.py --preview set-target linux && idf.py build && ./build/pump_host_tests.elf" +# exit code == Unity failures; must be 0 +``` + +### Both board targets build (controller integrated) +```bash +for b in rev1_devkit rev2; do + docker run --rm -v /tmp/ws011-firmware:/project -w /project espressif/idf:v6.0.1 \ + bash -lc "idf.py fullclean >/dev/null 2>&1; rm -f sdkconfig; \ + idf.py -DSDKCONFIG_DEFAULTS='sdkconfig.defaults;sdkconfig.board.$b' build" +done +``` +- **[CI]** both build; `idf.py size` fits the 1.5 MiB OTA slot; `dependencies.lock` unchanged. rev1 wires + the reservoir pump; rev2 does not (level sensors status-only) — both compile the pure reservoir logic. + +## HIL validation (rev1 rig — Checkpoint 3 smoke) + +1. **Automatic waters**: set moisture below the low threshold (or a dry sensor) in automatic mode → the + plant pump runs a burst, pauses (soak), re-waters until high. +2. **Fail-safe on cable pull**: in automatic mode with the pump running, pull the RS485 cable → the pump + stops within the staleness window (~30 s). +3. **Manual override**: a manual run keeps going despite a sensor failure; caps at 300 s. +4. **Reservoir**: fill starts on low, stops on high, aborts at 300 s if the high sensor never trips. +5. **Live soil**: `/api/v1/sensors` now shows soil `valid:true` with fresh values (periodic reader); + `/status` mode follows the flag and drives the controller. +6. **Isolation**: pull WiFi during an active watering cycle → watering is unaffected. + +## Definition of done + +- Host suite green (0 failures), every controller decision/safety branch covered (branch checklist in the + PR per the acceptance criteria); both board targets build + fit the slot. +- `firmware/CLAUDE.md` gains a Watering Controller section. +- HIL smoke executed on the rig, or deferred with rationale if no rig time (deferred-HIL register). diff --git a/specs/011-watering-controller-host-tests/research.md b/specs/011-watering-controller-host-tests/research.md new file mode 100644 index 0000000..5cc562d --- /dev/null +++ b/specs/011-watering-controller-host-tests/research.md @@ -0,0 +1,118 @@ +# Phase 0 Research: Watering Controller + +Grounded in the verified codebase map (origin/main, PR-01..09 merged), the legacy behavior +(`src/WateringController.cpp`, `src/main.cpp` — frozen reference), `docs/prd/PR-11-*.md`, and +`docs/parity-checklist.md` §1–§3. No open `NEEDS CLARIFICATION`. + +## D1 — pure `control` component (WateringController + ReservoirController) + +- **Decision**: New `control` component with two pure classes over interfaces + injected clock; 100 % + host-tested. `ReservoirController` is a separate pure state machine so it is tested independently of the + board that wires the pump. +- **Rationale**: master-PRD success criterion (100 % host-tested watering logic); mirrors the established + pure-logic pattern (WifiManager, DebouncedLevelSensor). No IDF includes → linux-buildable. +- **Alternatives**: one monolithic controller incl. reservoir — rejected (reservoir logic is board-gated; + separating it keeps each state machine independently testable). + +## D2 — soak gate: enforce min-interval, measured from burst END + +- **Decision**: Automatic mode enforces the minimum-watering-interval as a soak pause: after a burst ENDS, + no new automatic burst starts until `soakPause` has elapsed, even if soil still reads ≤ low. Burst + duration + soak length are runtime config (`getWateringDurationS`/`getMinWateringIntervalS`, defaults + 20 s / 300 s). Loop: burst → soak pause → re-evaluate → repeat until moisture ≥ high. +- **Rationale**: resolved by Paul 2026-06-10 — deliberate divergence (legacy comment `WateringController.cpp:303` + "water immediately", never enforced). Pause from burst END models absorption (water pools if poured + continuously while the sensor lags). Values tuned empirically later — the behavior is fixed here. +- **Alternatives**: measure from burst START (legacy `lastWateringTime = millis()` at start) — rejected: if + the pause < burst duration there is no effective soak; from-end is the intended absorption semantics. + +## D3 — fail-safe precedence: safety stop is unconditional, never gated by soak + +- **Decision**: On every automatic evaluation, check fail-safe FIRST: soil unavailable, OR data stale + (> 30 000 ms since last valid read, or never), OR moisture outside 0–100 → emergency-stop the pump and + take no watering decision. This runs regardless of the soak pause or any scheduling state. Manual mode + (operator override) is exempt (FR-007). +- **Rationale**: Constitution I; parity §2 (`WateringController.cpp:229-301`). FR-006 "a safety stop must + not be delayed by the gate" — host-tested with a pending soak pause + a fail-safe condition. +- **Alternatives**: evaluating the gate before safety — rejected (would delay a safety stop). + +## D4 — "manual" is the mode/enabled flag, not a pump flag + +- **Decision**: The new `IWaterPump` has no `isManualMode()`. Express mode via the controller's own state + driven by `IConfigStore::getWateringEnabled()` (+ who started a run): automatic = enabled and controller- + started (subject to fail-safe); manual = an operator-started run (bypass), and automatic evaluation is + suspended while `wateringEnabled==false`. A stop always clears the override (FR-010). +- **Rationale**: research §5 — legacy guarded fail-safes on `isManualMode()` which no longer exists; QUIRK 1 + requires auto-started = automatic (the legacy inverts by flagging every timed run manual). Manual runs + are capped at 300 s by the pump's own `runFor` cap (deliberate divergence; legacy allowed uncapped). +- **Alternatives**: add `isManualMode()` back to the pump — rejected (mode is controller/config policy, not + pump state; keeps the pump a dumb, safe actuator). + +## D5 — reservoir truth table (compose two ILevelSensor; add the invalid row) + +- **Decision**: `ReservoirController` composes the two marks (each: valid + wet/dry). Auto control, only + when enabled and pump not running: + + | low mark | high mark | action | + |---|---|---| + | invalid OR high invalid | (either invalid) | **do not act** (never treat invalid as dry) | + | wet | wet | full → ensure stopped | + | wet | dry | sufficient → no action | + | dry | dry | **start fill** | + | dry | wet | physically implausible → **no action** | + + While filling (manual or auto): stop when high mark reads wet; abort at 300 s max fill (the pump's own + cap). Feature disabled/absent → force pump off, skip logic. +- **Rationale**: research §5 legacy truth table (`src/main.cpp:533-550`) + the `ILevelSensor` "invalid = + do-not-act" contract (the fifth row the legacy lacks). 300 s cap = the pump's `runFor(cap)`/max-runtime. +- **Alternatives**: numeric level — N/A (marks are boolean). Aggregating in the sensor layer — rejected + (`ILevelSensor` never aggregates; the controller composes). + +## D6 — snapshot helpers on the locked wrappers (resolve TODO(PR-11)); soil-mock coherence + +- **Decision**: Add a consistent-snapshot helper (one locked call returning validity + values) to + `LockedSoilSensor`, `LockedEnvironmentalSensor`, `LockedLevelSensor` (the booked TODOs; soil has the gap + but no stub). The controller/periodic-reader use these to avoid the documented read()-then-getters + two-lock cross-call race. Add coherent `scriptSuccessfulRead/scriptFailedRead` to `MockSoilSensor` + (mirroring `MockEnvironmentalSensor`), so tests can't script incoherent soil state. +- **Rationale**: research §1/§2 — the periodic soil reader + controller become cross-task readers; the + cross-call gap becomes a real torn read without a snapshot helper. +- **Alternatives**: leave the gap — rejected (torn reads once the reader lands). + +## D7 — periodic soil reader; controller task; API mode handoff + +- **Decision**: Add a periodic soil read (a small `soil_reader_task`, or extend `sensor_task` to also read + soil) at the configured sensor interval, refreshing the locked soil snapshot — this also makes PR-09 + `/sensors` soil `valid`. The `WateringController` runs on its own watchdog-registered task (mirroring + `sensor_task`: `watchdog_subscribe_current_task()` + `watchdog_feed()` each cycle), reading + `getWateringEnabled()` to decide automatic vs. suspended — no direct ApiServer↔controller call path. +- **Rationale**: research §3/§4 — no periodic soil reader exists; the API only flips the config flag. + Watchdog policy (PR-08): watering-critical tasks subscribe. +- **Alternatives**: fold the controller into the existing 10 Hz main loop — acceptable, but a dedicated + task keeps cadence/責任 clean and mirrors sensor_task; decide at implementation (either is watchdog-fed). + +## D8 — fix the rs485test race; coordinate event logging + +- **Decision**: Route the diag-console `rs485test` through a locked path (via the sensor or a locked + client) so it no longer drives the raw `EspModbusClient` beneath `LockedSoilSensor`'s mutex once the + periodic reader runs. The controller logs fail-safe events (`EventLogger::logFailsafe`, "producer is + PR-11"); pump start/stop transitions stay owned by `SystemObserver` to avoid double-logging. +- **Rationale**: research §2/§6 — the rs485test raw path becomes a real RS485 bus race with a concurrent + reader; `SystemObserver` already edge-logs pump transitions. + +## D9 — data logging + time-not-set + +- **Decision**: Log env + soil to `IDataStorage` every `dataLogInterval` (default 5 min) with + `IWallClock::nowEpoch()`; NPK only when ≥ 0; gate a "valid timestamp" on `isTimeSet()` (pre-SNTP → do not + present a bogus 1970 as valid). Automatic watering itself does NOT depend on wall-clock (soil-driven), so + it runs offline. +- **Rationale**: parity §1 (`WateringController.cpp:332-363`) + PR-08 time-not-set contract. + +## Open risks + +- **R1** — soak/threshold values are placeholders (tuned empirically on the rig); the spec fixes behavior, + not the numbers (config-driven). +- **R2** — controller-task vs main-loop placement + soil-reader-task vs sensor_task-extension are the two + structural choices left to implementation; both must stay watchdog-fed and isolated from watering-blocking. +- **R3** — coordinating fail-safe event logging with `SystemObserver`'s pump-transition logging (avoid + double-log); verify at review/HIL. diff --git a/specs/011-watering-controller-host-tests/spec.md b/specs/011-watering-controller-host-tests/spec.md new file mode 100644 index 0000000..e2ea48d --- /dev/null +++ b/specs/011-watering-controller-host-tests/spec.md @@ -0,0 +1,275 @@ +# Feature Specification: Watering Controller (host-tested application logic) + +**Feature Branch**: `011-watering-controller-host-tests` + +**Created**: 2026-07-05 + +**Status**: Draft + +**Input**: PR-11 (`docs/prd/PR-11-watering-controller-host-tests.md`) — port `WateringController` (all +watering logic, scheduling, reservoir state machine, fail-safe) to pure interface-based C++ with **100 % of +watering logic + safety conditions under host tests in CI**. Behavior ground truth: +`docs/parity-checklist.md` §1–§3. Depends on PR-02..05 (interfaces + mocks + drivers), PR-06 (config/ +storage), PR-08 (time source + watchdog). Deliberate divergences from the frozen Arduino code are called out +below (soak gate, manual cap, mode semantics). + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Safe, pulsed automatic watering (Priority: P1) + +In automatic mode the system waters the plant when the soil is dry, in **bursts with an enforced soak +pause** so the water can absorb, stops when the soil reaches the high threshold, and — the safety-critical +part — **immediately stops and refuses to water on invalid, stale, or missing sensor data**. Every one of +these decisions is provable in CI without hardware. + +**Why this priority**: This is the heart of the product and the master-PRD success criterion ("watering +logic and safety conditions 100 % host-tested"). The fail-safe behavior is non-negotiable (Constitution I); +the pulsed soak gate is the deliberate correctness improvement over the legacy "water immediately" bug. + +**Independent Test**: Drive the pure controller over mock sensors/pump + a fake clock: assert it starts at +≤ low threshold, respects the soak pause, stops at ≥ high threshold, and emergency-stops on +unavailable/stale/invalid soil data — all as host tests. + +**Acceptance Scenarios**: + +1. **Given** automatic mode, pump stopped, valid moisture ≤ the low threshold, **When** the controller + evaluates, **Then** it starts a watering burst for the configured duration. +2. **Given** a burst just finished, **When** the soil still reads dry but the soak pause has not elapsed, + **Then** the controller does NOT start another burst (enforced soak/absorption pause). +3. **Given** the soak pause has elapsed and moisture is still ≤ low, **When** the controller evaluates, + **Then** it starts the next burst; this repeats until moisture reaches the high threshold. +4. **Given** the pump is running and moisture rises to ≥ the high threshold, **When** the controller + evaluates, **Then** it stops the pump. +5. **Given** automatic mode with the pump running, **When** the soil sensor becomes unavailable, or its + data is stale beyond the staleness window, or the moisture reading is outside the valid range, **Then** + the controller emergency-stops the pump and takes no watering decision (fail-safe, FR4). +6. **Given** a pending soak pause, **When** a fail-safe stop condition arises, **Then** the safety stop is + applied immediately and is NEVER delayed by the soak gate. +7. **Given** the soil sensor has not yet produced a first successful reading, **When** the controller + evaluates, **Then** it does not act on the placeholder values (gates on the read result, not the value). + +--- + +### User Story 2 - Reservoir auto-fill state machine (Priority: P2) + +On a board with a local refill pump the system keeps the reservoir topped up: it starts filling when the +water is low, stops when the high mark is reached, aborts at a hard maximum fill time as a safety net, and +does nothing on a physically impossible sensor combination. The whole state machine is host-tested even +though only one board wires the pump. + +**Why this priority**: The second safety-critical control domain (FR5). Board-independent logic gated by a +capability flag, so it is fully host-tested regardless of which board runs it. + +**Independent Test**: Drive the pure reservoir logic over two mock level sensors + a mock pump + fake +clock; assert every row of the level truth table, the high-mark stop, the max-fill-time abort, and the +"sensor invalid → do not act" and "physically impossible → do not act" rows. + +**Acceptance Scenarios**: + +1. **Given** auto level control and the pump not running, **When** both marks read dry (low water), **Then** + the fill pump starts. +2. **Given** the fill pump running, **When** the high mark reads wet, **Then** the pump stops immediately. +3. **Given** the fill pump running, **When** the high mark never trips, **Then** the pump is aborted at the + hard maximum fill time. +4. **Given** an implausible combination (low mark dry while high mark wet), **When** evaluated, **Then** no + action is taken. +5. **Given** either level sensor is not-yet-valid/invalid, **When** evaluated, **Then** no action is taken + (invalid is never treated as "water absent"). +6. **Given** the reservoir feature is disabled, **When** evaluated, **Then** the pump is forced off and all + reservoir control is skipped. + +--- + +### User Story 3 - Manual override, logging, and on-target integration (Priority: P3) + +An operator can run a pump manually as an explicit override that bypasses the automatic safety stops; the +system logs important events and sensor history with correct timestamps (and behaves sanely before the +clock is set); and the controller runs on the device, driven by the mode flag, replacing the interim +direct-drive path — including a periodic soil read so live soil data and automatic decisions actually work. + +**Why this priority**: Completes the feature on-target: manual control, observability, and the wiring that +makes the host-tested logic run in the real system. Lower risk than the core logic but required for a +usable, releasable controller. + +**Independent Test**: Manual run continues despite a sensor failure (host); an automatic run stops on the +same failure (host); data-log entries carry epoch timestamps and are gated on time-set (host); on the rig, +flipping the mode drives automatic watering and pulling the RS485 cable stops the pump within the staleness +window (HIL). + +**Acceptance Scenarios**: + +1. **Given** a manual run (explicit operator override), **When** a sensor fails or reports stale/invalid + data, **Then** the pump keeps running (manual bypasses the automatic fail-safe stops). +2. **Given** any mode, **When** stop is commanded, **Then** the pump stops and the override is cleared. +3. **Given** the data-log interval elapses, **When** the controller logs, **Then** it records the + environmental + soil readings (NPK only when valid/non-negative) with an epoch timestamp; **when** the + clock is not yet set, it does not fabricate a valid timestamp. +4. **Given** the device running, **When** the mode flag is automatic, **Then** the controller performs + automatic watering; **when** manual, automatic watering is suspended (operator override only). +5. **Given** the controller task, **When** it runs, **Then** it is registered with the hardware watchdog and + never blocks or is blocked by network/HTTP activity (watering isolation). +6. **Given** the device in automatic mode, **When** soil is polled periodically, **Then** fresh soil data + feeds both the automatic decision and the status/sensors reporting (soil is no longer perpetually + not-valid). + +--- + +### Edge Cases + +- **Pump start rejected** (e.g. already running, or duration out of range): the controller handles the + false return without crashing or double-starting. +- **Clock never set (offline)**: automatic watering still runs (it depends on soil, not wall-clock); + data-log timestamps reflect the not-set state rather than a bogus 1970 value. +- **Config changed at runtime** (thresholds/duration/soak/enabled): the next evaluation uses the new values; + an in-flight burst is not retroactively invalidated except by the normal stop/fail-safe rules. +- **Manual → automatic transition while a manual run is active**: defined behavior (the run continues or is + reconciled per the mode rules; no undefined double-control). +- **Soak pause active when the operator starts a manual run**: manual is unaffected by the gate. +- **Both moisture thresholds equal / low ≥ high (misconfiguration)**: the controller behaves deterministically + (no oscillation/undefined state). +- **Reservoir max-fill abort then still low**: after an aborted fill the controller does not immediately + re-slam the pump on (a safe re-evaluation, not a tight retry loop). +- **Concurrent access**: the controller task and other readers (console, HTTP status) reach shared sensors + through the locked wrappers; a cross-call read is consistent (snapshot), with no torn reads or bus races. + +## Requirements *(mandatory)* + +### Functional Requirements + +**Automatic watering (FR1)** + +- **FR-001**: In automatic mode the system MUST start a watering burst (configured duration) when watering + is enabled, the pump is not running, and the latest valid moisture is at or below the low threshold. +- **FR-002**: The system MUST stop watering when moisture reaches or exceeds the high threshold. +- **FR-003**: The system MUST enforce a soak/absorption pause (the minimum-watering-interval): after a + burst, automatic mode MUST NOT start another burst until the pause has elapsed, **even if the soil still + reads dry**. Burst duration and pause length are both runtime-configurable. (Deliberate divergence — the + frozen Arduino code stores the interval but never enforces it.) +- **FR-004**: The system MUST gate every automatic decision on a successful sensor read result, never on the + placeholder values a sensor holds before its first successful read. + +**Fail-safe (FR4 — Constitution I)** + +- **FR-005**: In automatic mode with the pump running, the system MUST emergency-stop the pump when the soil + sensor is unavailable, when its data is stale beyond the staleness window, or when the moisture reading is + outside the valid range; and MUST take no watering decision in those conditions. +- **FR-006**: A fail-safe stop MUST take effect immediately and MUST NOT be delayed or suppressed by the + soak-pause gate or any scheduling logic. +- **FR-007**: Manual operation MUST be exempt from the automatic fail-safe stops (explicit operator + override): a manual run continues despite sensor failure. + +**Manual / mode (QUIRK 1)** + +- **FR-008**: A manual run MUST be an explicit override; runs started by the automatic controller count as + automatic (subject to all fail-safe stops), runs started by the operator count as manual (bypass). (The + frozen code inverts this — target = the intended semantics.) +- **FR-009**: A manual run MUST be capped at the hard maximum runtime (300 s). (Deliberate divergence — the + frozen code allows uncapped indefinite manual runs.) +- **FR-010**: A stop command MUST always stop the pump regardless of mode and clear the override state. + +**Reservoir (FR5)** + +- **FR-011**: On a board with a local refill pump, the system MUST run the reservoir level state machine: + start filling when the water is low (both marks dry), stop when the high mark is wet, and abort filling at + a hard maximum fill time (300 s). +- **FR-012**: The reservoir logic MUST take no action on a physically impossible sensor combination (low + mark dry while high mark wet) and MUST take no action when either level sensor is not-yet-valid/invalid + (invalid is never treated as "water absent"). +- **FR-012a**: After a fill ends by the max-runtime abort (not by reaching the high mark), the system MUST + wait a cooldown before starting another automatic fill, even if the water still reads low — preventing an + endless refill cycle on a stuck high sensor or empty source (resolved 2026-07-05; deliberate divergence + from parity). Manual fill is unaffected. +- **FR-013**: When the reservoir feature is disabled (or absent on the board), the reservoir pump MUST be + forced off and its control logic skipped. The state-machine logic MUST be host-tested regardless of board + (the capability flag gates the wiring, not the logic). + +**Logging, time, integration** + +- **FR-014**: The system MUST log the environmental + soil readings to persistent storage at the data-log + cadence (NPK only when valid/non-negative), timestamped with epoch time, and MUST gate timestamp validity + on the clock being set. It MUST log the safety-relevant events (pump start/stop with cause, fail-safe + activations) without double-logging with the existing event observer. +- **FR-015**: The controller MUST initialize and offer manual operation even when one or both sensors fail + to initialize (graceful degradation), as long as the pump and storage are available. +- **FR-016**: The controller MUST run on the device driven by the mode flag (automatic vs. manual override), + replacing the interim direct-drive path; a periodic soil read MUST provide fresh soil data to both the + automatic decision and the status reporting. +- **FR-017**: The controller MUST run on its own watchdog-registered task (or the watchdog-fed watering + loop) and MUST never block, or be blocked by, network/HTTP activity — watering safety is independent of + connectivity. +- **FR-018**: All watering-logic and safety-condition branches MUST be covered by host tests running in CI; + the build MUST fail on any test failure. Both board targets MUST still build with the controller + integrated. + +### Key Entities *(include if data involved)* + +- **Watering configuration**: low/high moisture thresholds, burst duration, soak-pause length, enabled flag, + sensor-read + data-log intervals — from the typed config store, validated + persisted. +- **Controller state**: current mode (automatic / manual override), last-burst time (for the soak gate), + last-valid-sensor time (for staleness), pump running state. +- **Sensor snapshot**: a consistent (single locked read) view of a sensor's validity + values, used by the + controller and other readers to avoid torn cross-call reads. +- **Reservoir state**: composed from the two level marks (valid + wet/dry) into the fill decision + the + fill-timer. +- **Logged records**: sensor-history entries (metric, epoch, value) and events (pump start/stop, fail-safe) + with cause. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: 100 % of the controller's watering-decision and safety-condition branches are covered by host + tests that run in CI and gate the build (the master-PRD success criterion). +- **SC-002**: In automatic mode, dry soil is watered in bursts with the enforced soak pause between them, and + watering stops at the high threshold — demonstrated deterministically without hardware. +- **SC-003**: A sensor failure / stale / invalid reading in automatic mode stops the pump within the + staleness window and blocks further automatic watering; a manual run is unaffected — all host-verified; + the staleness stop also confirmed on the rig by pulling the RS485 cable. +- **SC-004**: Every row of the reservoir truth table (including the implausible and invalid-sensor rows), the + high-mark stop, and the max-fill abort are host-verified; on the rig, fill starts on low, stops on high, + and aborts at the cap. +- **SC-005**: A manual run is capped at 300 s and is never delayed or stopped by the automatic safety/soak + logic. +- **SC-006**: Sensor history + events are logged at the configured cadence with correct epoch timestamps, + and the device behaves sanely (no bogus 1970 timestamps treated as valid) before the clock is set. +- **SC-007**: Both board targets build with the controller integrated; the API mode flag drives automatic + watering on-target, and live soil data is available (no longer perpetually not-valid). + +## Assumptions + +- **Soak gate is pre-decided (not re-opened)**: enforcing the minimum-watering-interval as a soak pause is + the resolved project decision (Paul, 2026-06-10). Default burst duration 20 s, default soak/interval + 300 s, thresholds 30 %/55 % (the config-store defaults); the good field values are unknown and tuned + empirically later — the spec fixes the *behavior*, not the tuned numbers. +- **"Manual" = the mode flag, not a pump flag**: the new pump interface has no manual-mode flag; manual is + expressed via the watering-enabled/mode state (auto when enabled, operator override otherwise). Fail-safe + guards are re-expressed against that state. +- **Staleness window = 30 s**, **reservoir max fill = 300 s**, **data-log cadence = 5 min default**, + **implausible = high-wet without low-wet** — parity constants (`docs/parity-checklist.md` §1–§3). +- **Enabling helpers (implementation, not new behavior)**: consistent-snapshot helpers on the locked sensor + wrappers (the booked `TODO(PR-11)` on env/level/power + one for soil), coherent + `scriptSuccessfulRead/scriptFailedRead` helpers on the soil mock, and routing the diagnostic `rs485test` + through a locked path — all needed so the periodic soil reader and the controller do not create torn reads + or an RS485 bus race. These are enablers of FR-004/FR-016/edge-cases, decided at plan time. +- **Consumers do not branch on mock-only error codes**: the controller reacts to read success/failure + + availability, never to specific numeric error codes that exist only in the mock (real esp-modbus collapses + non-timeout errors), per the booked review guidance. +- **Event-logging coordination**: the existing `SystemObserver` already logs pump start/stop transitions; + the controller adds fail-safe events and coordinates so pump transitions are not double-logged. + +### Dependencies + +- **PR-02..05** — the sensor/pump interfaces + mocks + drivers the controller consumes. +- **PR-06** — the typed config store (thresholds/duration/soak/enabled) + data storage (history + events). +- **PR-08** — the injectable wall clock (data-log timestamps + time-not-set) and the task watchdog the + controller task registers with. +- **PR-09** — the API mode flag (`wateringEnabled`) the controller reads; the controller replaces PR-09's + interim direct-drive path and makes soil/power `valid` by adding the periodic read. + +### Out of Scope + +- Running the parity checklist on the rig (PR-12). +- New control features beyond parity + the resolved soak gate. +- INA226-based pump protections (separate PRD). +- The multi-zone / central-reservoir refill-request design (future PRD) — rev2 level sensors feed status + only here. diff --git a/specs/011-watering-controller-host-tests/tasks.md b/specs/011-watering-controller-host-tests/tasks.md new file mode 100644 index 0000000..f4ae5a2 --- /dev/null +++ b/specs/011-watering-controller-host-tests/tasks.md @@ -0,0 +1,178 @@ +--- + +description: "Task list for the WateringController port (PR-11)" +--- + +# Tasks: Watering Controller (host-tested application logic) + +**Input**: Design documents from `specs/011-watering-controller-host-tests/` + +**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/ + +**Tests**: INCLUDED and CENTRAL — the master-PRD criterion is 100 % host-tested watering logic. Pure logic +is test-first. On-target wiring (tasks, app_main, rs485test) is HIL-verified. + +**Organization**: US1 automatic+fail-safe (P1), US2 reservoir (P2), US3 manual/logging/integration (P3). + +## Format: `[ID] [P?] [Story] Description` +- **[P]** parallelizable; **[Story]** US1/US2/US3; paths under `firmware/`. + +--- + +## Phase 1: Setup + +- [ ] T001 Create the `control` component: `firmware/components/control/CMakeLists.txt` + (`idf_component_register(SRCS ... INCLUDE_DIRS include REQUIRES interfaces events)`; pure — compiles on + host + target, no linux guard). Create `include/control/`. +- [ ] T002 [P] Register host suites: create `test_watering_controller.cpp` + `test_reservoir.cpp` (empty + `run_watering_controller_tests()` / `run_reservoir_tests()`); add to `SRCS` + `control` to `REQUIRES` in + `firmware/test_apps/host/main/CMakeLists.txt`; declare + call both in `test_main.cpp`. + +**Checkpoint**: control component + suites compile empty on host + both targets. + +--- + +## Phase 2: Foundational (enablers — block the controller + coherent tests) + +- [ ] T003 Add the consistent-snapshot helper to `LockedSoilSensor.h` (`SoilSnapshot snapshot()` — one lock, + read/last-good + all getters + availability + error) per `contracts/snapshot-helpers.md`. Resolve the + documented cross-call gap. +- [ ] T004 [P] Add `snapshot()` to `LockedEnvironmentalSensor.h` (`EnvSnapshot`) + `LockedLevelSensor.h` + (`LevelSnapshot`), resolving their `TODO(PR-11)`. +- [ ] T005 [P] Add coherent `scriptSuccessfulRead(...)` / `scriptFailedRead(error)` to + `MockSoilSensor.h` (mirror `MockEnvironmentalSensor`), so soil test state stays coherent. + +**Checkpoint**: snapshot helpers + soil-mock helpers available; existing host suite still green. + +--- + +## Phase 3: User Story 1 - Safe, pulsed automatic watering (Priority: P1) 🎯 MVP + +**Goal**: pure `WateringController` automatic logic — moisture thresholds + soak gate + fail-safe stops + +gate-on-read + graceful degradation — 100 % host-tested. + +**Independent Test**: host tests over mocks + fakes prove start/soak/stop/fail-safe branches. + +### Tests for US1 (write first) + +- [ ] T006 [P] [US1] `test_watering_controller.cpp`: automatic branches — start at ≤ low; NO start during + soak (assert none at soak-1ms, start at soak); restart after soak while still dry; stop at ≥ high; + gate-on-read (no action before first successful read); config runtime change picked up next tick. +- [ ] T007 [P] [US1] fail-safe branches — soil unavailable / stale (>30 000 ms / never) / moisture + out-of-range each → immediate stop + no watering, in automatic; **a pending soak pause does NOT delay a + safety stop**; graceful degradation (constructs + manual works when a sensor failed to init). + +### Implementation for US1 + +- [ ] T008 [US1] `WateringController` (pure) — `include/control/WateringController.h` + `src/…cpp`: + constructor injects `ISoilSensor&/IWaterPump&/IConfigStore&/IDataStorage&/ITimeProvider&/IWallClock&/ + EventLogger&` (or the Locked snapshot wrappers). `tick()` per `data-model.md` order: `pump.update()` → + soil snapshot → fail-safe (unconditional, before the gate) → watering decision (start ≤low + soak-elapsed + from burst END; stop ≥high; set lastBurstEndMs). Reads all thresholds/durations from `config` each tick. + No IDF includes. `logFailsafe` on each safety stop. + +**Checkpoint**: automatic + fail-safe + soak host-tested green; controller pure/host-buildable. + +--- + +## Phase 4: User Story 2 - Reservoir auto-fill state machine (Priority: P2) + +**Goal**: pure `ReservoirController` — level truth table (+ invalid + implausible rows), stop-on-high, +max-fill abort, feature gate — host-tested regardless of board. + +### Tests for US2 (write first) + +- [ ] T009 [P] [US2] `test_reservoir.cpp`: all 5 truth-table rows (incl. invalid-sensor + implausible → no + action); start on low-dry+high-dry; stop-on-high-wet while running; max-fill abort at 300 s; feature + disabled forces off; **post-abort cooldown** — after a MaxRuntimeForced abort, NO new auto fill until the + cooldown elapses even if still low-dry, then a fill starts (a normal high-wet stop does NOT arm cooldown; + manual bypasses it). + +### Implementation for US2 + +- [ ] T010 [US2] `ReservoirController` (pure) — `include/control/ReservoirController.h` + `src/…cpp` per + `contracts/reservoir-controller.md`: two `ILevelSensor` snapshots, `IWaterPump`, injected clock; `tick( + enabled, autoLevelControl)`; the truth table; stop-on-high; the pump's `runFor`/cap for the 300 s abort; + the post-abort cooldown (`kReservoirRefillCooldownMs` ~60 s, suppress new auto fill after a + MaxRuntimeForced stop; normal high-wet stop does not arm it; manual bypasses); force-off when disabled. + Board-independent. + +**Checkpoint**: reservoir state machine fully host-tested (both boards compile it). + +--- + +## Phase 5: User Story 3 - Manual override, logging & on-target integration (Priority: P3) + +**Goal**: manual override + data logging in the controller (host-tested), then wire the controller + +periodic soil reader on-target driven by the mode flag; fix the rs485test race. + +### Tests for US3 (write first) + +- [ ] T011 [P] [US3] `test_watering_controller.cpp`: manual override — `startManual` runs despite sensor + failure (bypass); capped at 300 s; auto-started run is automatic (fail-safe applies); `stop()` clears the + override. Data logging — cadence + epoch timestamp + NPK≥0 + time-not-set handled. + +### Implementation for US3 + +- [ ] T012 [US3] Add `startManual(int)` / `stop()` + the data-log path to `WateringController` (manual = + `manualRunActive`, exempt from fail-safe; `stop()` clears it; log env+soil every `dataLogInterval` via + `IWallClock::nowEpoch()` gated on `isTimeSet()`; fail-safe events via `logFailsafe`, NOT pump transitions + — those stay with `SystemObserver`). +- [ ] T013 [US3] Periodic soil reader: add a `main/soil_reader_task.{h,cpp}` (or extend `sensor_task` to + also read soil) at the config sensor interval, refreshing the locked soil snapshot — makes PR-09 + `/sensors` soil `valid`. Watchdog-subscribed + fed like `sensor_task`. +- [ ] T014 [US3] Controller task: `main/watering_task.{h,cpp}` (or fold into the 10 Hz main loop) — + watchdog-registered (`watchdog_subscribe_current_task()` + `watchdog_feed()` each cycle); calls + `controller.tick()` + `reservoir.tick(enabled, auto)`. Isolated from network/HTTP. +- [ ] T015 [US3] Wire into `app_main.cpp`: construct `WateringController` + (rev1) `ReservoirController` with + the live `Locked*` refs + config/storage/clocks/EventLogger; start the controller + soil-reader tasks; + the API mode flag (`getWateringEnabled()`) drives automatic vs. suspended (no direct ApiServer↔controller + call). Keep `pumps_force_off()` first; add `control` to `main/CMakeLists.txt` PRIV_REQUIRES. +- [ ] T016 [US3] Fix the `diag_console.cpp` rs485test race: route it through a locked path (via the sensor + or a locked client) so it no longer drives the raw `EspModbusClient` beneath `LockedSoilSensor` once the + periodic reader runs. Update the `sensor_task.cpp:79` read+error to the snapshot helper if trivial. + +**Checkpoint**: manual + logging host-tested; controller runs on-target driven by the mode flag; live soil. + +--- + +## Phase 6: Polish & Cross-Cutting + +- [ ] T017 Add a "Watering controller (feature 011)" section to `firmware/CLAUDE.md` (control component, + pure/host-tested logic, soak-gate divergence, manual=flag + 300 s cap, reservoir truth table + invalid + row, fail-safe precedence, snapshot helpers, periodic soil reader, controller task + watchdog). +- [ ] T018 [P] Confirm `dependencies.lock` + esp-modbus pins unchanged (no new managed deps). +- [ ] T019 `idf.py size` both targets — confirm fit in the 1.5 MiB OTA slot; record margin. +- [ ] T020 Author `specs/011-watering-controller-host-tests/checklists/hil.md` from quickstart §HIL (auto + waters, RS485-pull fail-safe within staleness, manual override, reservoir fill/stop/abort, live soil, + WiFi-loss isolation). Note the deferral path if no rig time. +- [ ] T021 Branch-coverage checklist for the PR (per the acceptance criteria — every decision/safety branch + ticked) + full host suite + both board builds green. + +--- + +## Dependencies & Execution Order + +- **Setup (P1)** → **Foundational (P2, blocks all — snapshot + mock helpers)** → US1 → US2 → US3 → Polish. +- US1 (`WateringController`) and US3 (manual/logging on it) edit the SAME class — sequence T012 after T008. +- US3 integration (T013–T016) edits `app_main.cpp`/`sensor_task`/`diag_console` — sequence after the pure + logic (US1/US2) is green. +- Pure/host-tested: T003–T012 (snapshot helpers, controllers, their tests). Target/HIL: T013–T016 (tasks, + wiring, rs485test). +- MockSoilSensor helpers (T005) are needed by the US1/US3 soil tests. + +### Parallel opportunities +- T004/T005 (foundational) parallel. Test tasks T006/T007, T009, T011 are [P] within their story. T018 [P]. + +--- + +## Implementation Strategy + +- **MVP = US1** (safe pulsed automatic watering — the core + the fail-safe, 100 % host-tested). +- Then US2 (reservoir), then US3 (manual/logging + on-target integration). +- Commit after each phase; write host tests FIRST for the pure logic and confirm they fail before impl. +- Build via Docker on an rsync'd `/tmp/ws011-firmware`; fullclean + rm sdkconfig between boards; rm the host + build dir if stale. +- Never modify frozen legacy (`src/` is read-only reference). Keep `pumps_force_off()` first. Controller + pure (no IDF/esp_timer). Avoid `/*` inside block comments (`-Werror=comment`). Coordinate fail-safe vs + pump-transition logging with `SystemObserver` (no double-log). From 0c318a7c90503b6428ab58444ca083d61c3d46c3 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 19:08:36 +0200 Subject: [PATCH 2/9] =?UTF-8?q?feat(control):=20PR-11=20setup=20+=20founda?= =?UTF-8?q?tional=20=E2=80=94=20control=20component=20+=20snapshot=20helpe?= =?UTF-8?q?rs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1+2 of PR-11: - control component skeleton (pure, header-only until US1/US2 sources land) - consistent-snapshot helpers on LockedSoil/Env/LevelSensor (one locked call -> {validity + values}, closing the read()-then-getters cross-call gap; non-blocking, snapshot caches read() outcome, no bus I/O). Resolves the TODO(PR-11) markers. - MockSoilSensor scriptSuccessfulRead/scriptFailedRead (coherent FIFO, back-compat) - host control test suites registered (stubs) Verified: host 252/0, rev1 + rev2 build (0 errors) + board-config verified. Co-Authored-By: Claude Opus 4.8 (1M context) --- firmware/components/control/CMakeLists.txt | 18 +++++ .../control/include/control/.gitkeep | 3 + .../sensors/LockedEnvironmentalSensor.h | 60 +++++++++++--- .../include/sensors/LockedLevelSensor.h | 47 +++++++++-- .../include/sensors/LockedSoilSensor.h | 80 +++++++++++++++++-- .../include/sensors/testing/MockSoilSensor.h | 80 ++++++++++++++++++- firmware/test_apps/host/main/CMakeLists.txt | 4 +- firmware/test_apps/host/main/test_main.cpp | 4 + .../test_apps/host/main/test_reservoir.cpp | 18 +++++ .../host/main/test_watering_controller.cpp | 18 +++++ 10 files changed, 304 insertions(+), 28 deletions(-) create mode 100644 firmware/components/control/CMakeLists.txt create mode 100644 firmware/components/control/include/control/.gitkeep create mode 100644 firmware/test_apps/host/main/test_reservoir.cpp create mode 100644 firmware/test_apps/host/main/test_watering_controller.cpp diff --git a/firmware/components/control/CMakeLists.txt b/firmware/components/control/CMakeLists.txt new file mode 100644 index 0000000..bec58f1 --- /dev/null +++ b/firmware/components/control/CMakeLists.txt @@ -0,0 +1,18 @@ +# control — pure watering application logic (feature 011). +# +# Holds the pure, host-tested WateringController and ReservoirController +# (arriving in US1/US2). Both depend ONLY on the header-only `interfaces` +# component and the pure `events` EventLogger — no IDF/esp_* code, no +# esp_timer (time is injected via ITimeProvider/IWallClock). The component +# therefore compiles identically on the linux host (host-tested against the +# mocks + fakes) and on both device targets. No linux guard needed. +# +# TODO(US1/US2): add SRCS "src/WateringController.cpp" +# "src/ReservoirController.cpp" +# once those pure sources land. Until then the component registers as a +# header-only/interface component (INCLUDE_DIRS only) so it configures +# cleanly on host + both targets. +idf_component_register( + INCLUDE_DIRS "include" + REQUIRES interfaces events +) diff --git a/firmware/components/control/include/control/.gitkeep b/firmware/components/control/include/control/.gitkeep new file mode 100644 index 0000000..3c472c2 --- /dev/null +++ b/firmware/components/control/include/control/.gitkeep @@ -0,0 +1,3 @@ +# Placeholder so include/control/ exists for the control component's +# INCLUDE_DIRS before WateringController.h / ReservoirController.h land +# (US1/US2). Remove once real headers are added. diff --git a/firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h b/firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h index eb2c7ff..41b7f10 100644 --- a/firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h +++ b/firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h @@ -24,15 +24,14 @@ * registration, sensor task, controllers, ...) goes through the * LockedEnvironmentalSensor, never through the wrapped object directly. * - * SCOPE: this decorator provides PER-CALL atomicity only, not cross-call. - * A read-then-getters sequence spanning multiple calls (read() then - * getTemperature()/getHumidity()/getPressure()) is NOT protected against - * an interleaving read() from another task in between — another task may - * refresh (or invalidate) the values first. Such sequences need - * higher-level coordination. - * TODO(PR-11): add a consistent-snapshot helper (one locked call returning - * all three values + validity) when the controller becomes a second - * periodic reader — same bookkeeping as LockedSoilSensor's PR-11 notes. + * SCOPE (per-call atomicity, cross-call gap): each interface call is + * individually atomic, but a read-then-getters sequence spanning multiple + * calls (read() then getTemperature()/getHumidity()/getPressure()) is NOT + * protected against an interleaving read() from another task in between. + * snapshot() (PR-11) CLOSES that gap: it copies all three values + validity + * out under a SINGLE lock, so a reader (controller / API status / console) + * never observes a torn read/getter tuple. Any other multi-call sequence + * still needs higher-level coordination. * * Pure C++ ( is available via pthread on ESP-IDF and on the linux * preview target), so the decorator is host-testable. @@ -45,6 +44,21 @@ #include "interfaces/IEnvironmentalSensor.h" +/** + * @brief Consistent, non-blocking environmental snapshot (PR-11). + * + * Temperature/humidity/pressure plus validity, copied out under one lock so + * they are mutually consistent. valid reports whether the most recent read() + * through the wrapper succeeded (fresh data); when false the values are the + * last-good reading (or the NaN placeholders before the first success). + */ +struct EnvSnapshot { + bool valid; + float temperature; + float humidity; + float pressure; +}; + /** * @brief IEnvironmentalSensor decorator that serializes every call with a * mutex. @@ -74,7 +88,11 @@ class LockedEnvironmentalSensor : public IEnvironmentalSensor { bool read() override { std::lock_guard lock(mutex_); - return sensor_.read(); + const bool ok = sensor_.read(); + // Cache the outcome so snapshot() can report validity without a + // fresh (blocking) bus transaction. + lastReadOk_ = ok; + return ok; } bool isAvailable() override @@ -107,9 +125,31 @@ class LockedEnvironmentalSensor : public IEnvironmentalSensor { return sensor_.getPressure(); } + /** + * @brief Copy the last-good T/RH/P + validity out under one lock (PR-11), + * closing the read-then-getter cross-call gap. + * + * NON-BLOCKING by contract: it performs NO fresh read() and NO + * isAvailable() probe — both are I2C bus I/O and must never run in a + * status/API/controller path (QUIRK 5). The sensor task owns the read() + * cadence; this returns the current cached values. valid = the most + * recent read() through this wrapper succeeded. + */ + EnvSnapshot snapshot() + { + std::lock_guard lock(mutex_); + EnvSnapshot s; + s.valid = lastReadOk_; + s.temperature = sensor_.getTemperature(); + s.humidity = sensor_.getHumidity(); + s.pressure = sensor_.getPressure(); + return s; + } + private: IEnvironmentalSensor& sensor_; mutable std::mutex mutex_; + bool lastReadOk_ = false; ///< result of the most recent read() }; #endif /* WATERINGSYSTEM_SENSORS_LOCKEDENVIRONMENTALSENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/LockedLevelSensor.h b/firmware/components/sensors/include/sensors/LockedLevelSensor.h index 0e7ed44..62ba688 100644 --- a/firmware/components/sensors/include/sensors/LockedLevelSensor.h +++ b/firmware/components/sensors/include/sensors/LockedLevelSensor.h @@ -19,14 +19,14 @@ * update, console registration, controllers, ...) goes through the * LockedLevelSensor, never through the wrapped object directly. * - * SCOPE: this decorator provides PER-CALL atomicity only, not cross-call. - * An isValid()-then-isWaterPresent() sequence is NOT protected against an - * interleaving update() from another task in between — the state may - * change (or invalidate) between the two calls. Such sequences need - * higher-level coordination. - * TODO(PR-11): add a consistent-snapshot helper (one locked call returning - * validity + logical state) when the controller becomes a second periodic - * reader — same bookkeeping as the other Locked* wrappers' PR-11 notes. + * SCOPE (per-call atomicity, cross-call gap): each interface call is + * individually atomic, but an isValid()-then-isWaterPresent() sequence is + * NOT protected against an interleaving update() from another task in + * between — the state may change (or invalidate) between the two calls. + * snapshot() (PR-11) CLOSES that gap: it copies validity + logical state out + * under a SINGLE lock, so a reader (controller / API status / console) never + * observes a torn validity/state tuple. Any other multi-call sequence still + * needs higher-level coordination. * * Pure C++ ( is available via pthread on ESP-IDF and on the linux * preview target), so the decorator is host-testable. @@ -39,6 +39,20 @@ #include "interfaces/ILevelSensor.h" +/** + * @brief Consistent level snapshot (PR-11): validity + logical state copied + * out under one lock so they are mutually consistent. + * + * waterPresent is meaningful ONLY while valid is true; when invalid it is + * false (never a stale or phantom value — the interface contract). The + * reservoir truth table treats an invalid sensor as "do not act", never as + * "water absent". + */ +struct LevelSnapshot { + bool valid; + bool waterPresent; +}; + /** * @brief ILevelSensor decorator that serializes every call with a mutex. * @@ -84,6 +98,23 @@ class LockedLevelSensor : public ILevelSensor { sensor_.notifyPowerOn(); } + /** + * @brief Copy validity + logical water state out under one lock (PR-11), + * closing the isValid()-then-isWaterPresent() cross-call gap. + * + * update() is NOT part of this call — the main loop owns the update() + * cadence; snapshot() only reads the current debounced state, which is + * pure in-memory state (no I/O). waterPresent is false whenever invalid. + */ + LevelSnapshot snapshot() + { + std::lock_guard lock(mutex_); + LevelSnapshot s; + s.valid = sensor_.isValid(); + s.waterPresent = sensor_.isWaterPresent(); + return s; + } + private: ILevelSensor& sensor_; mutable std::mutex mutex_; diff --git a/firmware/components/sensors/include/sensors/LockedSoilSensor.h b/firmware/components/sensors/include/sensors/LockedSoilSensor.h index 55662f5..bc701f4 100644 --- a/firmware/components/sensors/include/sensors/LockedSoilSensor.h +++ b/firmware/components/sensors/include/sensors/LockedSoilSensor.h @@ -20,12 +20,16 @@ * registration, controllers, ...) goes through the LockedSoilSensor, never * through the wrapped object directly. * - * SCOPE: this decorator provides PER-CALL atomicity only, not cross-call. - * A read-then-get sequence spanning two calls (read() then getMoisture()) - * is NOT protected against an interleaving read() from another task in - * between — another task may refresh (or invalidate) the values first. - * Such sequences need higher-level coordination (a caller-held lock or - * single-owner task). + * SCOPE (per-call atomicity, cross-call gap): each interface call is + * individually atomic, but a read-then-get sequence spanning two calls + * (read() then getMoisture()) is NOT protected against an interleaving + * read() from another task in between — another task may refresh (or + * invalidate) the values first. snapshot() (PR-11) CLOSES that gap for the + * common consumer case: it copies the last-good values + validity + error + * out under a SINGLE lock, so a reader (controller / API status / console) + * never observes a torn read/getter tuple. Any other multi-call sequence + * still needs higher-level coordination (a caller-held lock or single-owner + * task). * * Pure C++ ( is available via pthread on ESP-IDF and on the linux * preview target), so the decorator is host-testable. @@ -38,6 +42,31 @@ #include "interfaces/ISoilSensor.h" +/** + * @brief Consistent, non-blocking soil-reading snapshot (PR-11). + * + * All eight quantity values plus the validity/error flags, copied out under + * one lock so they are mutually consistent (no torn read()-then-getter + * tuple). readOk reports whether the most recent read() through the wrapper + * succeeded; available reports whether at least one successful read is on + * record, so the last-good values are meaningful even when the latest read + * failed (stale-but-usable). lastError is the wrapped sensor's most recent + * error code. + */ +struct SoilSnapshot { + bool readOk; + bool available; + int lastError; + float moisture; + float temperature; + float humidity; + float ph; + float ec; + float nitrogen; + float phosphorus; + float potassium; +}; + /** * @brief ISoilSensor decorator that serializes every call with a mutex. * @@ -62,7 +91,12 @@ class LockedSoilSensor : public ISoilSensor { bool read() override { std::lock_guard lock(mutex_); - return sensor_.read(); + const bool ok = sensor_.read(); + // Cache the outcome so snapshot() can report validity without a + // fresh (blocking) bus transaction. + lastReadOk_ = ok; + hasEverReadOk_ = hasEverReadOk_ || ok; + return ok; } bool isAvailable() override @@ -143,9 +177,41 @@ class LockedSoilSensor : public ISoilSensor { return sensor_.calibrateEC(referenceValue); } + /** + * @brief Copy the last-good reading + validity + error out under one + * lock (PR-11), closing the read-then-getter cross-call gap. + * + * NON-BLOCKING by contract: it performs NO fresh read() and NO + * isAvailable() probe — both are RS485/Modbus bus I/O and must never run + * in a status/API/controller path (QUIRK 5). The periodic soil reader + * owns the read() cadence; this returns the current cached values. + * readOk = the most recent read() through this wrapper succeeded; + * available = at least one successful read is on record (last-good values + * are meaningful); lastError = the wrapped sensor's current error code. + */ + SoilSnapshot snapshot() + { + std::lock_guard lock(mutex_); + SoilSnapshot s; + s.readOk = lastReadOk_; + s.available = hasEverReadOk_; + s.lastError = sensor_.getLastError(); + s.moisture = sensor_.getMoisture(); + s.temperature = sensor_.getTemperature(); + s.humidity = sensor_.getHumidity(); + s.ph = sensor_.getPH(); + s.ec = sensor_.getEC(); + s.nitrogen = sensor_.getNitrogen(); + s.phosphorus = sensor_.getPhosphorus(); + s.potassium = sensor_.getPotassium(); + return s; + } + private: ISoilSensor& sensor_; mutable std::mutex mutex_; + bool lastReadOk_ = false; ///< result of the most recent read() + bool hasEverReadOk_ = false; ///< any successful read() on record }; #endif /* WATERINGSYSTEM_SENSORS_LOCKEDSOILSENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h b/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h index 5389959..a51213f 100644 --- a/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h +++ b/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h @@ -5,17 +5,26 @@ * @brief Scriptable ISoilSensor test double (header-only). * * Serves the host tests of soil-sensor CONSUMERS (watering controller / - * sensor manager, PR-11): set the seven quantity values and the + * sensor manager, PR-11): set the eight quantity values and the * initialize()/read()/isAvailable() results, script the error code, and * assert on the call counters. The sensor's own decode/validation logic is * tested against the REAL ModbusSoilSensor via MockModbusClient — never * through this mock. Never compiled into target builds (only included from * test code). No IDF includes. + * + * Consistency helpers (PR-11, mirroring MockEnvironmentalSensor): + * scriptSuccessfulRead()/scriptFailedRead() keep outcome, error code and + * getter values coherent per read() step (a failed read never touches the + * values — the interface's last-good contract), instead of hand-setting the + * public fields into an incoherent combination. The plain public fields stay + * for back-compat: with NO script, read() behaves exactly as before + * (returns readResult, getters serve the manually-set values). */ #ifndef WATERINGSYSTEM_SENSORS_TESTING_MOCKSOILSENSOR_H #define WATERINGSYSTEM_SENSORS_TESTING_MOCKSOILSENSOR_H +#include #include #include "interfaces/ISoilSensor.h" @@ -62,6 +71,29 @@ class MockSoilSensor : public ISoilSensor { std::vector calibratePhCalls; std::vector calibrateEcCalls; + // -- Consistency helpers (script one read() outcome each, FIFO) ---------- + + /// Script the next read() to succeed: read() returns true, error 0, and + /// the getters serve exactly these values afterwards (readResult and the + /// public value fields are updated coherently when the step is consumed). + void scriptSuccessfulRead(float moistureValue, float temperatureValue, + float humidityValue, float phValue, + float ecValue, float nitrogenValue, + float phosphorusValue, float potassiumValue) + { + script_.push_back({true, 0, moistureValue, temperatureValue, + humidityValue, phValue, ecValue, nitrogenValue, + phosphorusValue, potassiumValue}); + } + + /// Script the next read() to fail with @p error: read() returns false and + /// leaves the getter values untouched (last-good contract). + void scriptFailedRead(int error) + { + script_.push_back( + {false, error, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f}); + } + // -- ISoilSensor --------------------------------------------------------- bool initialize() override @@ -73,7 +105,33 @@ class MockSoilSensor : public ISoilSensor { bool read() override { ++readCalls; - return readResult; + if (script_.empty()) { + // Unscripted: legacy behaviour — return the plain field, getters + // serve the manually-set values. + return readResult; + } + const Step& step = script_[next_]; + if (next_ + 1 < script_.size()) { + ++next_; // exhausted scripts repeat the last step forever + } + if (!step.ok) { + readResult = false; + lastError = step.error; + return false; // values untouched — last-good contract + } + // Publish the coherent values into the public fields so the getters + // (which serve those fields) stay in sync with the read outcome. + moisture = step.moisture; + temperature = step.temperature; + humidity = step.humidity; + ph = step.ph; + ec = step.ec; + nitrogen = step.nitrogen; + phosphorus = step.phosphorus; + potassium = step.potassium; + readResult = true; + lastError = 0; + return true; } bool isAvailable() override @@ -110,6 +168,24 @@ class MockSoilSensor : public ISoilSensor { calibrateEcCalls.push_back(referenceValue); return calibrateResult; } + +private: + /// One scripted read() outcome; values are only meaningful when ok. + struct Step { + bool ok; + int error; + float moisture; + float temperature; + float humidity; + float ph; + float ec; + float nitrogen; + float phosphorus; + float potassium; + }; + + std::vector script_; + std::size_t next_ = 0; }; #endif /* WATERINGSYSTEM_SENSORS_TESTING_MOCKSOILSENSOR_H */ diff --git a/firmware/test_apps/host/main/CMakeLists.txt b/firmware/test_apps/host/main/CMakeLists.txt index 68a2226..460f5b7 100644 --- a/firmware/test_apps/host/main/CMakeLists.txt +++ b/firmware/test_apps/host/main/CMakeLists.txt @@ -13,10 +13,12 @@ idf_component_register( "test_api_serialize.cpp" "test_api_requests.cpp" "test_api_routes.cpp" + "test_watering_controller.cpp" + "test_reservoir.cpp" # Compile-time board-contract TUs (T016): each defines one board # selector before including the REAL board/board.h — passing = # compiling (static_asserts only, nothing registers with Unity). "test_board_contract_rev1.cpp" "test_board_contract_rev2.cpp" - REQUIRES unity actuators interfaces storage nvs_flash sensors board network time events api cjson + REQUIRES unity actuators interfaces storage nvs_flash sensors board network time events api cjson control ) diff --git a/firmware/test_apps/host/main/test_main.cpp b/firmware/test_apps/host/main/test_main.cpp index 968bedb..37eb3ab 100644 --- a/firmware/test_apps/host/main/test_main.cpp +++ b/firmware/test_apps/host/main/test_main.cpp @@ -26,6 +26,8 @@ void run_event_logger_tests(void); void run_api_serialize_tests(void); void run_api_requests_tests(void); void run_api_routes_tests(void); +void run_watering_controller_tests(void); +void run_reservoir_tests(void); // Unity requires setUp/tearDown definitions (shared by all suites). extern "C" void setUp(void) {} @@ -47,5 +49,7 @@ extern "C" void app_main(void) run_api_serialize_tests(); run_api_requests_tests(); run_api_routes_tests(); + run_watering_controller_tests(); + run_reservoir_tests(); std::exit(UNITY_END()); } diff --git a/firmware/test_apps/host/main/test_reservoir.cpp b/firmware/test_apps/host/main/test_reservoir.cpp new file mode 100644 index 0000000..5229071 --- /dev/null +++ b/firmware/test_apps/host/main/test_reservoir.cpp @@ -0,0 +1,18 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file test_reservoir.cpp + * @brief Host suite for the pure ReservoirController (feature 011, US2). + * + * The level truth table, stop-on-high, max-fill abort, post-abort cooldown + * and feature-gate branches (US2) are added here in a later task (T009). + * This file registers the suite entry point now so the host test app links + * and runs empty until those tests land. + */ + +#include "unity.h" + +// TODO(T009): register the ReservoirController tests here. +void run_reservoir_tests(void) +{ +} diff --git a/firmware/test_apps/host/main/test_watering_controller.cpp b/firmware/test_apps/host/main/test_watering_controller.cpp new file mode 100644 index 0000000..005011d --- /dev/null +++ b/firmware/test_apps/host/main/test_watering_controller.cpp @@ -0,0 +1,18 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file test_watering_controller.cpp + * @brief Host suite for the pure WateringController (feature 011, US1/US3). + * + * The automatic + soak-gate + fail-safe branches (US1) and the manual + * override + data-logging branches (US3) are added here in later tasks + * (T006/T007/T011). This file registers the suite entry point now so the + * host test app links and runs empty until those tests land. + */ + +#include "unity.h" + +// TODO(T006/T007/T011): register the WateringController tests here. +void run_watering_controller_tests(void) +{ +} From 0a5f831698b9f815502faa4b0fb21f8b08933086 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 19:48:34 +0200 Subject: [PATCH 3/9] =?UTF-8?q?feat(control):=20PR-11=20US1=20=E2=80=94=20?= =?UTF-8?q?WateringController=20automatic=20+=20soak=20+=20fail-safe=20(ho?= =?UTF-8?q?st-tested)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 / US1 (pure, host-tested MVP): - WateringController.tick(): fail-safe FIRST (soil unavailable / stale >30s / invalid moisture in auto -> immediate stop + logFailsafe, never delayed by the soak gate), then the watering decision (start at <=low when soak elapsed; stop at >=high). Soak measured from burst END (edge-detects the pump's own timed self-stop). Thresholds/ duration/soak read from config each tick. Gate on read result, not placeholder. lastValidSoilMs recorded before the stale test (fixes a first-read deadlock). - 14 host tests (8 automatic/soak + 6 fail-safe incl. not-delayed-by-soak + graceful degradation) On-target read()/isAvailable() vs snapshot integration is US3. Reservoir is US2. Verified: host 266/0 (+14), rev1 + rev2 build + board-config verified. Co-Authored-By: Claude Opus 4.8 (1M context) --- firmware/components/control/CMakeLists.txt | 7 +- .../include/control/WateringController.h | 115 +++++ .../control/src/WateringController.cpp | 112 +++++ .../host/main/test_watering_controller.cpp | 405 +++++++++++++++++- 4 files changed, 628 insertions(+), 11 deletions(-) create mode 100644 firmware/components/control/include/control/WateringController.h create mode 100644 firmware/components/control/src/WateringController.cpp diff --git a/firmware/components/control/CMakeLists.txt b/firmware/components/control/CMakeLists.txt index bec58f1..7c29574 100644 --- a/firmware/components/control/CMakeLists.txt +++ b/firmware/components/control/CMakeLists.txt @@ -7,12 +7,9 @@ # therefore compiles identically on the linux host (host-tested against the # mocks + fakes) and on both device targets. No linux guard needed. # -# TODO(US1/US2): add SRCS "src/WateringController.cpp" -# "src/ReservoirController.cpp" -# once those pure sources land. Until then the component registers as a -# header-only/interface component (INCLUDE_DIRS only) so it configures -# cleanly on host + both targets. +# TODO(US2): add SRCS "src/ReservoirController.cpp" once that pure source lands. idf_component_register( + SRCS "src/WateringController.cpp" INCLUDE_DIRS "include" REQUIRES interfaces events ) diff --git a/firmware/components/control/include/control/WateringController.h b/firmware/components/control/include/control/WateringController.h new file mode 100644 index 0000000..b9b05f5 --- /dev/null +++ b/firmware/components/control/include/control/WateringController.h @@ -0,0 +1,115 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file WateringController.h + * @brief Pure automatic watering logic: pulsed watering + soak gate + fail-safe. + * + * Feature 011 (PR-11), User Story 1. All watering-decision and safety-condition + * logic lives here as pure C++17 over the injected interfaces + clocks, so it is + * exercised on the IDF linux preview target against the mocks/fakes + * (test_watering_controller.cpp). This class MUST NOT include esp_* / esp_timer + * or make any hardware/network call: monotonic time comes from ITimeProvider, + * wall-clock time from IWallClock, and every threshold/duration is read from + * IConfigStore each tick (runtime-tunable). Normative contract: + * specs/011-watering-controller-host-tests/contracts/watering-controller.md; + * decision tables + constants: .../data-model.md. + * + * Scope note: this US1 slice implements the automatic path (tick()). The manual + * override (startManual/stop bookkeeping) and periodic data-logging are US3 and + * are only left as clean seams here — not implemented. + * + * Concurrency: the controller is unsynchronized and single-writer (its own + * watchdog-registered task). On-target US3 wires the sensor through a + * LockedSoilSensor snapshot so read()+availability come from one locked + * acquisition; the pure US1 logic drives the injected ISoilSensor directly. + */ + +#ifndef WATERINGSYSTEM_CONTROL_WATERINGCONTROLLER_H +#define WATERINGSYSTEM_CONTROL_WATERINGCONTROLLER_H + +#include + +#include "events/EventLogger.h" +#include "interfaces/IConfigStore.h" +#include "interfaces/IDataStorage.h" +#include "interfaces/ISoilSensor.h" +#include "interfaces/ITimeProvider.h" +#include "interfaces/IWallClock.h" +#include "interfaces/IWaterPump.h" + +/** + * @brief Pulsed automatic watering with an enforced soak pause and fail-safe. + * + * Invariants (host-tested): + * 1. A fail-safe stop (sensor unavailable / stale / out-of-range moisture) is + * applied BEFORE the soak gate and is never delayed by it (FR-005/006). + * 2. Automatic decisions gate on a successful, in-range read result, never on + * the sensor's placeholder/last-good values (FR-004). + * 3. After a burst ends (self-stop at the configured duration or a stop at the + * high threshold) no new automatic burst starts until the soak pause has + * elapsed, even while the soil still reads dry (FR-003). + */ +class WateringController { +public: + /// Staleness window: a valid soil read older than this fails safe (§2). + static constexpr int64_t kStalenessMs = 30'000; + + /// Valid moisture range (percent); a read outside it is treated as invalid. + static constexpr float kMoistureMinPct = 0.0f; + static constexpr float kMoistureMaxPct = 100.0f; + + /** + * @brief Construct over injected collaborators (references; must outlive). + * + * Constructing with a sensor that fails to initialize is safe: no work runs + * in the constructor and tick() simply fails safe (FR-015). + */ + WateringController(ISoilSensor& soil, IWaterPump& plant, IConfigStore& config, + IDataStorage& storage, ITimeProvider& clock, + IWallClock& wallClock, EventLogger& events) + : soil_(soil), + plant_(plant), + config_(config), + storage_(storage), + clock_(clock), + wallClock_(wallClock), + events_(events) + { + } + + WateringController(const WateringController&) = delete; + WateringController& operator=(const WateringController&) = delete; + + /** + * @brief One automatic evaluation. Non-blocking; call at a fixed cadence. + * + * Order (safety first, soak gate last): pump self-stop/cap enforcement → + * enabled gate → fail-safe (unavailable/stale/invalid) → gate-on-read → + * watering decision (stop-at-high / start-burst-if-soak-elapsed). + */ + void tick(); + +private: + ISoilSensor& soil_; + IWaterPump& plant_; + IConfigStore& config_; + IDataStorage& storage_; ///< US3 data-logging seam (unused in US1) + ITimeProvider& clock_; + IWallClock& wallClock_; ///< US3 data-logging seam (unused in US1) + EventLogger& events_; + + /// Monotonic time the last automatic burst ended — the soak-gate origin + /// (0 = no burst has ended yet). + int64_t lastBurstEndMs_ = 0; + + /// Monotonic time of the last successful, in-range soil read + /// (0 = never — treated as stale until the first valid read). + int64_t lastValidSoilMs_ = 0; + + /// True while an automatic burst is in progress; lets tick() detect the + /// pump's own timed self-stop (duration/cap) as a burst end so the soak + /// gate starts from the burst end regardless of how it stopped. + bool burstActive_ = false; +}; + +#endif /* WATERINGSYSTEM_CONTROL_WATERINGCONTROLLER_H */ diff --git a/firmware/components/control/src/WateringController.cpp b/firmware/components/control/src/WateringController.cpp new file mode 100644 index 0000000..b489ef4 --- /dev/null +++ b/firmware/components/control/src/WateringController.cpp @@ -0,0 +1,112 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file WateringController.cpp + * @brief Automatic watering logic implementation (pure C++, host-tested). + * + * Pure: no esp_* / esp_timer includes, no hardware or network calls. All time + * is injected (ITimeProvider), all tunables are read from IConfigStore each + * tick. See WateringController.h for the invariants and the data-model for the + * decision tables. + */ + +#include "control/WateringController.h" + +void WateringController::tick() +{ + const int64_t now = clock_.nowMs(); + + // Actuator layer first: enforce timed self-stop and the hard 300 s cap. + plant_.update(); + + // A burst that ended on its own (duration/cap self-stop) is the soak-gate + // origin too — record the burst end so a self-stopped burst is not exempt + // from the pause (FR-003). A stop we command at the high threshold sets the + // same field inline below. + if (burstActive_ && !plant_.isRunning()) { + lastBurstEndMs_ = now; + burstActive_ = false; + } + + // Automatic path only. When automatic watering is disabled the mode is + // manual/suspended (operator override, US3): take no automatic action. + if (!config_.getWateringEnabled()) { + return; + } + + // ---- Read the sensor (US1 drives ISoilSensor directly; US3 wires a + // LockedSoilSensor snapshot so read()+availability come from one locked + // acquisition). We react only to the read RESULT and the availability + // signal, never to specific numeric error codes. ------------------------ + const bool readOk = soil_.read(); + const bool available = soil_.isAvailable(); + const float moisture = soil_.getMoisture(); + const bool inRange = + moisture >= kMoistureMinPct && moisture <= kMoistureMaxPct; + const bool invalid = readOk && !inRange; + + // A fresh, in-range read is by definition not stale; record it before the + // staleness test so the FIRST valid read is acted on (a fresh valid read is + // never "stale", even when lastValidSoilMs_ was still 0). + if (readOk && inRange) { + lastValidSoilMs_ = now; + } + const bool stale = + (lastValidSoilMs_ == 0) || (now - lastValidSoilMs_ > kStalenessMs); + + // ---- FAIL-SAFE (unconditional, checked BEFORE the soak gate) ---------- + // Never delayed or suppressed by the soak-pause/scheduling logic (FR-006). + const char* failsafeReason = nullptr; + if (!available) { + failsafeReason = "soil-unavailable"; + } else if (invalid) { + failsafeReason = "moisture-invalid"; + } else if (stale) { + failsafeReason = "soil-stale"; + } + if (failsafeReason != nullptr) { + if (plant_.isRunning()) { + plant_.stop(); + events_.logFailsafe(failsafeReason); + } + // Abandon any in-flight automatic burst; take no watering decision. + burstActive_ = false; + return; + } + + // Gate on the read result (FR-004): a transient read failure while the last + // valid reading is still within the staleness window is not a fail-safe + // condition — wait for fresh data rather than act on placeholder values. + if (!readOk) { + return; + } + + // ---- WATERING DECISION (soak gate is the LAST thing checked) ----------- + const float lowThreshold = config_.getMoistureThresholdLow(); + const float highThreshold = config_.getMoistureThresholdHigh(); + + if (plant_.isRunning()) { + if (moisture >= highThreshold) { + // Target reached: stop and arm the soak pause from this burst end. + plant_.stop(); + lastBurstEndMs_ = now; + burstActive_ = false; + } + // else: keep running within the burst. + return; + } + + if (moisture <= lowThreshold) { + const int64_t soakMs = + static_cast(config_.getMinWateringIntervalS()) * 1000; + const bool soakElapsed = + (lastBurstEndMs_ == 0) || (now - lastBurstEndMs_ >= soakMs); + if (soakElapsed) { + if (plant_.runFor(static_cast(config_.getWateringDurationS()))) { + burstActive_ = true; + } + } + // else: soak pause active — do NOT start another burst, even though the + // soil still reads dry (FR-003). + } +} diff --git a/firmware/test_apps/host/main/test_watering_controller.cpp b/firmware/test_apps/host/main/test_watering_controller.cpp index 005011d..b665371 100644 --- a/firmware/test_apps/host/main/test_watering_controller.cpp +++ b/firmware/test_apps/host/main/test_watering_controller.cpp @@ -2,17 +2,410 @@ // SPDX-License-Identifier: AGPL-3.0-or-later /** * @file test_watering_controller.cpp - * @brief Host suite for the pure WateringController (feature 011, US1/US3). + * @brief Host suite for the pure WateringController (feature 011, US1). * - * The automatic + soak-gate + fail-safe branches (US1) and the manual - * override + data-logging branches (US3) are added here in later tasks - * (T006/T007/T011). This file registers the suite entry point now so the - * host test app links and runs empty until those tests land. + * Drives the REAL WateringController logic over MockSoilSensor + + * MockWaterPump (real WaterPump enforcement over FakeTimeProvider) + + * MockConfigStore + MockDataStorage + FakeWallClock + EventLogger. Registered + * via run_watering_controller_tests() from the shared Unity runner + * (test_main.cpp); the process exit code equals the failure count (CI gate). + * + * Coverage maps to tasks.md T006 (automatic + soak gate) and T007 (fail-safe): + * start-at-low, no-start/allow-restart across the soak pause, stop-at-high, + * runtime config change, disabled -> no action, gate-on-read (placeholder + + * transient failed read), fail-safe unavailable/stale/invalid stops, fail-safe + * never delayed by the soak gate, and graceful degradation (sensor always + * failing -> never waters, no crash). The manual override + periodic + * data-logging branches (US3) land later (T011). */ +#include + #include "unity.h" -// TODO(T006/T007/T011): register the WateringController tests here. +#include "actuators/testing/FakeTimeProvider.h" +#include "actuators/testing/MockWaterPump.h" +#include "control/WateringController.h" +#include "events/EventLogger.h" +#include "interfaces/IDataStorage.h" +#include "sensors/testing/MockSoilSensor.h" +#include "storage/testing/MockConfigStore.h" +#include "storage/testing/MockDataStorage.h" +#include "time/testing/FakeWallClock.h" + +namespace { + +// --------------------------------------------------------------------------- +// Fixture: fresh collaborators + controller per test, with deterministic +// config that mirrors the store defaults (low 30 %, high 55 %, burst 20 s, +// soak 300 s, enabled). +// --------------------------------------------------------------------------- +struct Fixture { + FakeTimeProvider clock; + MockSoilSensor soil; + MockWaterPump pump{"plant", clock}; + MockConfigStore config; + MockDataStorage storage; + FakeWallClock wallClock; + EventLogger events{storage, wallClock}; + WateringController controller{soil, pump, config, storage, + clock, wallClock, events}; + + Fixture() + { + TEST_ASSERT_TRUE(pump.initialize()); + config.stored.moistureThresholdLow = 30.0f; + config.stored.moistureThresholdHigh = 55.0f; + config.stored.wateringDurationS = 20; + config.stored.minWateringIntervalS = 300; + config.stored.wateringEnabled = 1; + } +}; + +/// Number of output ON transitions recorded by the real pump — a proxy for +/// "how many bursts were started" (each successful runFor drives applyOutput +/// true exactly once; stops drive false and are not counted). +int onTransitions(const MockWaterPump& pump) +{ + int count = 0; + for (bool on : pump.outputCalls) { + if (on) { + ++count; + } + } + return count; +} + +/// Number of persisted fail-safe events (category kCategoryFailsafe). +int failsafeEventCount(const MockDataStorage& storage) +{ + int count = 0; + for (const auto& event : storage.events) { + if (event.category == IDataStorage::kCategoryFailsafe) { + ++count; + } + } + return count; +} + +/// Script the sensor's next tick outcome via the plain public fields (no FIFO +/// script): read() returns @p readOk, isAvailable() returns @p available and +/// the getters serve @p moisture. A failed read leaves the value in place +/// (mirroring the last-good contract); the controller must ignore it. +void setSensor(Fixture& f, bool readOk, bool available, float moisture) +{ + f.soil.readResult = readOk; + f.soil.isAvailableResult = available; + f.soil.moisture = moisture; +} + +// =========================================================================== +// T006 — automatic + soak-gate branches +// =========================================================================== + +// Scenario 1: enabled + not running + valid moisture <= low + soak elapsed +// (first burst) -> start a burst. +void test_starts_burst_when_dry_and_enabled(void) +{ + Fixture f; + setSensor(f, /*readOk=*/true, /*available=*/true, /*moisture=*/20.0f); + f.controller.tick(); + + TEST_ASSERT_TRUE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); +} + +// Disabled automatic watering -> no automatic action at all. +void test_no_start_when_disabled(void) +{ + Fixture f; + f.config.stored.wateringEnabled = 0; + setSensor(f, true, true, 20.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// Scenario 4: running + moisture >= high -> stop. +void test_stops_at_high_threshold(void) +{ + Fixture f; + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1000); + setSensor(f, true, true, 60.0f); // >= high 55 + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(static_cast(StopReason::Commanded), + static_cast(f.pump.getLastStopReason())); +} + +// Scenarios 2 + 3: after a burst ends (here by the pump's own duration +// self-stop), no new burst starts until the soak pause elapses (assert at +// soak-1 ms), then a burst starts at exactly the soak pause and repeats while +// still dry. +void test_soak_pause_blocks_then_allows_restart(void) +{ + Fixture f; // burst 20 s, soak 300 s + setSensor(f, true, true, 20.0f); + f.controller.tick(); // burst 1 starts + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // Advance the full burst duration: the pump self-stops (DurationElapsed); + // the controller records the burst end as the soak-gate origin. + f.clock.advance(20'000); + setSensor(f, true, true, 20.0f); // still dry + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); // no immediate restart + + // 1 ms before the soak pause elapses: still no new burst. + f.clock.advance(299'999); + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); + + // Exactly at the soak pause: the next burst starts (still dry). + f.clock.advance(1); + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(2, onTransitions(f.pump)); +} + +// A runtime config change (lower threshold) is picked up on the next tick. +void test_config_change_picked_up_next_tick(void) +{ + Fixture f; // low 30 + setSensor(f, true, true, 40.0f); // 40 > 30 -> no start + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + f.config.stored.moistureThresholdLow = 50.0f; // now 40 <= 50 + f.clock.advance(1000); + setSensor(f, true, true, 40.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); +} + +// Scenario 7 (FR-004): before the first successful read the controller does not +// act on the sensor's placeholder value (here 0 %, which is <= low). +void test_no_action_before_first_successful_read(void) +{ + Fixture f; + setSensor(f, /*readOk=*/false, /*available=*/true, /*moisture=*/0.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); + // Pump was not running, so no fail-safe event is logged. + TEST_ASSERT_EQUAL_INT(0, failsafeEventCount(f.storage)); +} + +// FR-004: a transient read failure within the staleness window (after a prior +// valid read) is gated on the read result — no action, not a fail-safe. +void test_gate_on_transient_failed_read(void) +{ + Fixture f; + setSensor(f, true, true, 40.0f); // valid read, 40 > low -> no start + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + f.clock.advance(1000); // still well within the 30 s window + setSensor(f, /*readOk=*/false, /*available=*/true, /*moisture=*/20.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); // did not act on the failed read + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); + TEST_ASSERT_EQUAL_INT(0, failsafeEventCount(f.storage)); +} + +// Threshold comparisons are inclusive: moisture == low starts, == high stops. +void test_boundaries_low_and_high_inclusive(void) +{ + Fixture f; + f.config.stored.wateringDurationS = 300; // avoid an in-test self-stop + + setSensor(f, true, true, 30.0f); // == low + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1000); + setSensor(f, true, true, 55.0f); // == high + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); +} + +// =========================================================================== +// T007 — fail-safe branches (Constitution I) +// =========================================================================== + +// Scenario 5: automatic + running, sensor becomes unavailable -> emergency +// stop + logged fail-safe + no watering decision. +void test_failsafe_unavailable_stops_running_pump(void) +{ + Fixture f; + f.config.stored.wateringDurationS = 300; // keep the burst running + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1000); + setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, failsafeEventCount(f.storage)); + TEST_ASSERT_EQUAL_UINT32(0, f.events.droppedEvents()); +} + +// Scenario 5: data stale beyond the 30 s window -> emergency stop. +void test_failsafe_stale_stops_running_pump(void) +{ + Fixture f; + f.config.stored.wateringDurationS = 300; + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // Reads keep failing while the sensor still probes available; once the last + // valid read ages past 30 s the controller fails safe. + f.clock.advance(30'001); + setSensor(f, /*readOk=*/false, /*available=*/true, /*moisture=*/20.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, failsafeEventCount(f.storage)); +} + +// Scenario 5: a successful read with out-of-range moisture (both bounds) +// -> emergency stop. +void test_failsafe_invalid_moisture_stops_running_pump(void) +{ + { + Fixture f; + f.config.stored.wateringDurationS = 300; + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1000); + setSensor(f, /*readOk=*/true, /*available=*/true, /*moisture=*/150.0f); + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, failsafeEventCount(f.storage)); + } + { + Fixture f; + f.config.stored.wateringDurationS = 300; + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1000); + setSensor(f, /*readOk=*/true, /*available=*/true, /*moisture=*/-5.0f); + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, failsafeEventCount(f.storage)); + } +} + +// Scenario 6 (FR-006): a fail-safe stop is applied immediately and is never +// delayed by the soak gate. The pump is running with the soak-gate origin +// already armed (a prior burst ended) and the soil reads dry — the exact state +// where a mis-ordered controller might "keep running"; the fail-safe must still +// stop the pump on the same tick. +void test_failsafe_not_delayed_by_soak(void) +{ + Fixture f; + f.config.stored.wateringDurationS = 300; // no self-stop confound + f.config.stored.minWateringIntervalS = 1; // 1 s soak so a 2nd burst runs + + setSensor(f, true, true, 20.0f); + f.controller.tick(); // burst 1 running + + f.clock.advance(2000); + setSensor(f, true, true, 60.0f); // >= high -> stop, arm the soak origin + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + f.clock.advance(2000); // soak (1 s) elapsed + setSensor(f, true, true, 20.0f); // dry -> burst 2 runs + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // Running, dry, soak origin set: trigger a fail-safe -> immediate stop. + f.clock.advance(1000); + setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_TRUE(failsafeEventCount(f.storage) >= 1); +} + +// During a pending soak pause (pump stopped, dry soil) a fail-safe condition is +// handled on the fail-safe path and no watering is started. +void test_failsafe_during_pending_soak_takes_no_action(void) +{ + Fixture f; // burst 20 s, soak 300 s + setSensor(f, true, true, 20.0f); + f.controller.tick(); // burst 1 running + + f.clock.advance(20'000); + setSensor(f, true, true, 20.0f); + f.controller.tick(); // self-stop; soak pending, pump stopped + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); + + f.clock.advance(1000); // still inside the soak window + setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); // no new burst + // Pump was not running, so no fail-safe event is logged. + TEST_ASSERT_EQUAL_INT(0, failsafeEventCount(f.storage)); +} + +// FR-015 graceful degradation: a sensor whose reads always fail (and probes +// unavailable) never causes watering and never crashes the controller. +void test_graceful_degradation_never_waters(void) +{ + Fixture f; + for (int i = 0; i < 5; ++i) { + setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + f.clock.advance(5000); + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + } + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); + TEST_ASSERT_EQUAL_UINT32(0, f.events.droppedEvents()); +} + +} // namespace + void run_watering_controller_tests(void) { + // T006 — automatic + soak gate + RUN_TEST(test_starts_burst_when_dry_and_enabled); + RUN_TEST(test_no_start_when_disabled); + RUN_TEST(test_stops_at_high_threshold); + RUN_TEST(test_soak_pause_blocks_then_allows_restart); + RUN_TEST(test_config_change_picked_up_next_tick); + RUN_TEST(test_no_action_before_first_successful_read); + RUN_TEST(test_gate_on_transient_failed_read); + RUN_TEST(test_boundaries_low_and_high_inclusive); + + // T007 — fail-safe + RUN_TEST(test_failsafe_unavailable_stops_running_pump); + RUN_TEST(test_failsafe_stale_stops_running_pump); + RUN_TEST(test_failsafe_invalid_moisture_stops_running_pump); + RUN_TEST(test_failsafe_not_delayed_by_soak); + RUN_TEST(test_failsafe_during_pending_soak_takes_no_action); + RUN_TEST(test_graceful_degradation_never_waters); } From e8f972d45af4e8342e34d957abac3f8ea6dbe346 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 20:00:18 +0200 Subject: [PATCH 4/9] =?UTF-8?q?feat(control):=20PR-11=20US2=20=E2=80=94=20?= =?UTF-8?q?ReservoirController=20auto-fill=20state=20machine?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure, board-independent reservoir fill controller: - level truth table (invalid/implausible rows -> no action; dry+dry -> fill) - running safety: stop on high-wet; pump cap aborts at 300 s (MaxRuntimeForced) - post-abort cooldown (kReservoirRefillCooldownMs=60 s): after a max-runtime abort, suppress a new auto fill until cooldown elapses even if still low-dry (guards a stuck high sensor / empty source); normal high-wet stop does not arm it; manual fill bypasses it - feature gate: !enabled forces the pump off and skips all logic 14 host tests (test_reservoir.cpp): all truth-table rows, stop-on-high, max-fill abort, disabled-forces-off, cooldown arm/bypass. Host 280/0; rev1 + rev2 build green. Co-Authored-By: Claude Opus 4.8 --- firmware/components/control/CMakeLists.txt | 15 +- .../include/control/ReservoirController.h | 146 ++++++++ .../control/src/ReservoirController.cpp | 114 ++++++ .../test_apps/host/main/test_reservoir.cpp | 337 +++++++++++++++++- 4 files changed, 599 insertions(+), 13 deletions(-) create mode 100644 firmware/components/control/include/control/ReservoirController.h create mode 100644 firmware/components/control/src/ReservoirController.cpp diff --git a/firmware/components/control/CMakeLists.txt b/firmware/components/control/CMakeLists.txt index 7c29574..4540354 100644 --- a/firmware/components/control/CMakeLists.txt +++ b/firmware/components/control/CMakeLists.txt @@ -1,15 +1,14 @@ # control — pure watering application logic (feature 011). # -# Holds the pure, host-tested WateringController and ReservoirController -# (arriving in US1/US2). Both depend ONLY on the header-only `interfaces` -# component and the pure `events` EventLogger — no IDF/esp_* code, no -# esp_timer (time is injected via ITimeProvider/IWallClock). The component -# therefore compiles identically on the linux host (host-tested against the -# mocks + fakes) and on both device targets. No linux guard needed. -# -# TODO(US2): add SRCS "src/ReservoirController.cpp" once that pure source lands. +# Holds the pure, host-tested WateringController (US1) and ReservoirController +# (US2). Both depend ONLY on the header-only `interfaces` component and the +# pure `events` EventLogger — no IDF/esp_* code, no esp_timer (time is injected +# via ITimeProvider/IWallClock). The component therefore compiles identically +# on the linux host (host-tested against the mocks + fakes) and on both device +# targets. No linux guard needed. idf_component_register( SRCS "src/WateringController.cpp" + "src/ReservoirController.cpp" INCLUDE_DIRS "include" REQUIRES interfaces events ) diff --git a/firmware/components/control/include/control/ReservoirController.h b/firmware/components/control/include/control/ReservoirController.h new file mode 100644 index 0000000..fe4d76a --- /dev/null +++ b/firmware/components/control/include/control/ReservoirController.h @@ -0,0 +1,146 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file ReservoirController.h + * @brief Pure reservoir auto-fill state machine (level truth table + safety). + * + * Feature 011 (PR-11), User Story 2. Board-independent: the whole state + * machine is host-tested regardless of which board wires the fill pump — the + * BOARD_HAS_RESERVOIR_PUMP capability flag gates only the construction/wiring + * in app_main, never this logic (FR-013). Composed from TWO independent + * ILevelSensor marks (low + high), one IWaterPump (the fill pump), an injected + * ITimeProvider (monotonic time for the post-abort cooldown) and the pure + * EventLogger. Normative contract: + * specs/011-watering-controller-host-tests/contracts/reservoir-controller.md; + * decision table + constants: .../data-model.md. + * + * This class MUST NOT include esp_* / esp_timer or make any hardware/network + * call: monotonic time comes from ITimeProvider and every input is an injected + * interface, so it is exercised on the IDF linux preview target against the + * mocks/fakes (test_reservoir.cpp). + * + * Concurrency: the controller is unsynchronized and single-writer (its own + * watchdog-registered task). On-target US3 drives the shared level sensors and + * pump through the Locked* wrappers; the pure US2 logic drives the injected + * interfaces directly. + */ + +#ifndef WATERINGSYSTEM_CONTROL_RESERVOIRCONTROLLER_H +#define WATERINGSYSTEM_CONTROL_RESERVOIRCONTROLLER_H + +#include + +#include "events/EventLogger.h" +#include "interfaces/ILevelSensor.h" +#include "interfaces/ITimeProvider.h" +#include "interfaces/IWaterPump.h" + +/** + * @brief Reservoir auto-fill with running safety and a post-abort cooldown. + * + * Invariants (host-tested): + * 1. An invalid level sensor is NEVER treated as "water absent": if either + * mark is not-yet-valid/invalid the controller takes no automatic action + * (FR-012). + * 2. Running safety is unconditional: whenever the fill pump runs (auto or + * manual), the high mark reading wet stops it immediately, and the pump's + * own update() aborts it at the hard 300 s max-fill cap. + * 3. After a fill ends by the max-runtime abort (StopReason::MaxRuntimeForced) + * no new AUTOMATIC fill starts until kReservoirRefillCooldownMs has elapsed + * — even while the water still reads low — preventing an endless 300 s + * cycle on a stuck high sensor or an empty source (FR-012a). A normal + * high-wet stop does NOT arm the cooldown; a manual fill bypasses it. + * 4. The feature gate forces the pump OFF and skips all logic when the + * reservoir feature is disabled / absent on the board (FR-013). + */ +class ReservoirController { +public: + /// Cooldown after a max-runtime abort before another AUTOMATIC fill may + /// start (FR-012a). Documented constant, tunable; a manual fill bypasses + /// it and a normal high-wet stop never arms it. + static constexpr int64_t kReservoirRefillCooldownMs = 60'000; + + /// Automatic fill duration, in seconds. Deliberately equal to the pump's + /// hard max-runtime cap (WaterPump::kDefaultMaxRunTimeMs == 300 s): an + /// auto fill runs until the high mark trips (running safety stops it early) + /// or, if the high mark never trips, until the cap aborts it as + /// StopReason::MaxRuntimeForced. The IWaterPump interface does not expose + /// its cap, so the duration is pinned here to match it (contract: "fill + /// duration + the 300 s cap come from the pump"). + static constexpr int kReservoirFillDurationS = 300; + + /** + * @brief Construct over injected collaborators (references; must outlive). + * + * Constructing with sensors that fail to initialize is safe: no work runs + * in the constructor and tick() simply takes no action while a mark is + * invalid (FR-012/FR-015). + */ + ReservoirController(ILevelSensor& lowMark, ILevelSensor& highMark, + IWaterPump& fillPump, ITimeProvider& clock, + EventLogger& events) + : lowMark_(lowMark), + highMark_(highMark), + fillPump_(fillPump), + clock_(clock), + events_(events) + { + } + + ReservoirController(const ReservoirController&) = delete; + ReservoirController& operator=(const ReservoirController&) = delete; + + /** + * @brief One reservoir evaluation. Non-blocking; call at a fixed cadence. + * + * Order (safety first): fill-pump self-stop/cap enforcement → arm the + * post-abort cooldown on a max-runtime abort edge → feature gate (force off + * when disabled) → running safety (stop on high-wet) → when enabled + auto + * and the pump is not running, evaluate the level truth table (respecting + * the cooldown for the dry/dry start). + * + * @param enabled reservoir feature enabled (board has the pump). + * @param autoLevelControl automatic level control active (else manual only). + */ + void tick(bool enabled, bool autoLevelControl); + + /** + * @brief Start a manual fill (explicit operator override). + * + * Refused when the reservoir is already full (the high mark reads wet). + * Bypasses the post-abort cooldown. The duration is bounded by the pump's + * runFor() contract (1..300 s; out-of-range requests are rejected, no + * silent clamping); running safety and the 300 s cap still apply while it + * runs. + * + * @param durationS requested fill duration in seconds. + * @return true if the fill was started, false if refused/rejected. + */ + bool startManualFill(int durationS); + + /// Stop the fill pump (any mode). + void stop(); + +private: + /// Arm the post-abort cooldown when the fill pump self-stopped at the hard + /// max-runtime cap during this tick's update() — a running->stopped edge + /// with StopReason::MaxRuntimeForced. A normal Commanded/high-wet stop is + /// issued AFTER this check, so it is never mistaken for an abort. + void armCooldownOnAbortEdge(bool wasRunning, int64_t now); + + /// Evaluate the level truth table (auto mode, pump not running). + void evaluateAuto(int64_t now); + + ILevelSensor& lowMark_; + ILevelSensor& highMark_; + IWaterPump& fillPump_; + ITimeProvider& clock_; + EventLogger& events_; ///< US3 event-logging seam (unused in US2) + + /// Monotonic time of the last max-runtime abort (0 = none). While + /// now - lastAbortMs_ < kReservoirRefillCooldownMs, an automatic fill is + /// suppressed even when the water reads low (FR-012a). + int64_t lastAbortMs_ = 0; +}; + +#endif /* WATERINGSYSTEM_CONTROL_RESERVOIRCONTROLLER_H */ diff --git a/firmware/components/control/src/ReservoirController.cpp b/firmware/components/control/src/ReservoirController.cpp new file mode 100644 index 0000000..51812b6 --- /dev/null +++ b/firmware/components/control/src/ReservoirController.cpp @@ -0,0 +1,114 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file ReservoirController.cpp + * @brief Reservoir auto-fill state machine implementation (pure C++, tested). + * + * Pure: no esp_* / esp_timer includes, no hardware or network calls. All time + * is injected (ITimeProvider). See ReservoirController.h for the invariants and + * the data-model for the level truth table and the post-abort cooldown. + */ + +#include "control/ReservoirController.h" + +void ReservoirController::tick(bool enabled, bool autoLevelControl) +{ + // Snapshot the running state BEFORE update() so a max-runtime abort that + // happens inside update() this tick is observable as a running->stopped + // edge (armCooldownOnAbortEdge below). + const bool wasRunning = fillPump_.isRunning(); + const int64_t now = clock_.nowMs(); + + // Actuator layer first: enforce the hard 300 s max-fill cap (self-stop). + fillPump_.update(); + + // Arm the post-abort cooldown on a max-runtime abort edge. This runs before + // the high-wet stop below, so a normal Commanded stop never arms it. + armCooldownOnAbortEdge(wasRunning, now); + + // Feature gate (FR-013): reservoir disabled / board lacks the pump -> force + // the pump OFF and skip ALL reservoir logic. + if (!enabled) { + fillPump_.stop(); + return; + } + + // Running safety (manual + auto): the high mark reading wet stops the fill + // immediately, however it was started. While a fill is in progress (or was + // just stopped this tick) the truth table is never evaluated, so nothing + // re-starts on the same tick. + if (fillPump_.isRunning()) { + if (highMark_.isValid() && highMark_.isWaterPresent()) { + fillPump_.stop(); + } + return; + } + + // Automatic level control only (manual mode leaves the reservoir to the + // operator via startManualFill()). + if (!autoLevelControl) { + return; + } + evaluateAuto(now); +} + +bool ReservoirController::startManualFill(int durationS) +{ + // Refuse a manual fill when the reservoir is already full (high mark wet). + if (highMark_.isValid() && highMark_.isWaterPresent()) { + return false; + } + // Bypasses the post-abort cooldown; the pump's runFor() enforces the + // 1..300 s bound (no silent clamping) and the 300 s cap while it runs. + return fillPump_.runFor(durationS); +} + +void ReservoirController::stop() +{ + fillPump_.stop(); +} + +void ReservoirController::armCooldownOnAbortEdge(bool wasRunning, int64_t now) +{ + if (wasRunning && !fillPump_.isRunning() && + fillPump_.getLastStopReason() == StopReason::MaxRuntimeForced) { + lastAbortMs_ = now; + } +} + +void ReservoirController::evaluateAuto(int64_t now) +{ + // An invalid mark is NEVER "water absent" (FR-012): take no action while + // either mark is not-yet-valid/invalid. + if (!lowMark_.isValid() || !highMark_.isValid()) { + return; + } + + const bool lowWet = lowMark_.isWaterPresent(); + const bool highWet = highMark_.isWaterPresent(); + + if (highWet) { + // wet/wet = full -> ensure stopped (a no-op here: the pump is not + // running, running safety already handled any in-flight fill). + // dry/wet = physically implausible -> no action. + if (lowWet) { + fillPump_.stop(); + } + return; + } + + // High mark dry from here on. + if (lowWet) { + // wet/dry = sufficient water -> no action. + return; + } + + // dry/dry = low water -> start a fill, unless the post-abort cooldown is + // still active (FR-012a) — do not re-slam the pump after a max-runtime + // abort while the water still reads low. + if (lastAbortMs_ != 0 && + (now - lastAbortMs_) < kReservoirRefillCooldownMs) { + return; + } + fillPump_.runFor(kReservoirFillDurationS); +} diff --git a/firmware/test_apps/host/main/test_reservoir.cpp b/firmware/test_apps/host/main/test_reservoir.cpp index 5229071..fb66490 100644 --- a/firmware/test_apps/host/main/test_reservoir.cpp +++ b/firmware/test_apps/host/main/test_reservoir.cpp @@ -4,15 +4,342 @@ * @file test_reservoir.cpp * @brief Host suite for the pure ReservoirController (feature 011, US2). * - * The level truth table, stop-on-high, max-fill abort, post-abort cooldown - * and feature-gate branches (US2) are added here in a later task (T009). - * This file registers the suite entry point now so the host test app links - * and runs empty until those tests land. + * Drives the REAL ReservoirController logic over two MockLevelSensor (the low + * and high marks) + MockWaterPump (real WaterPump enforcement over + * FakeTimeProvider) + MockDataStorage + FakeWallClock + EventLogger. Registered + * via run_reservoir_tests() from the shared Unity runner (test_main.cpp); the + * process exit code equals the failure count (CI gate). + * + * Coverage maps to tasks.md T009 (SC-004): all five level truth-table rows + * (either/both invalid -> no action; wet/wet full ensure-stopped; wet/dry + * sufficient -> no action; dry/dry -> start fill; dry/wet implausible -> no + * action), stop-on-high-wet while running, the max-fill abort at the 300 s cap + * (StopReason::MaxRuntimeForced), the post-abort cooldown (blocks then allows a + * new auto fill; a normal high-wet stop does not arm it; a manual fill bypasses + * it), the manual-fill refusal when already full, and the feature gate + * (disabled -> pump forced off + all logic skipped). */ +#include + #include "unity.h" -// TODO(T009): register the ReservoirController tests here. +#include "actuators/testing/FakeTimeProvider.h" +#include "actuators/testing/MockWaterPump.h" +#include "control/ReservoirController.h" +#include "events/EventLogger.h" +#include "sensors/testing/MockLevelSensor.h" +#include "storage/testing/MockDataStorage.h" +#include "time/testing/FakeWallClock.h" + +namespace { + +// --------------------------------------------------------------------------- +// Fixture: fresh collaborators + controller per test. The two level marks +// start INVALID (the mock's construction state, mirroring the real sensor's +// settle/warm-up); each test scripts a coherent valid state as needed. +// --------------------------------------------------------------------------- +struct Fixture { + FakeTimeProvider clock; + MockLevelSensor low; + MockLevelSensor high; + MockWaterPump pump{"reservoir", clock}; + MockDataStorage storage; + FakeWallClock wallClock; + EventLogger events{storage, wallClock}; + ReservoirController controller{low, high, pump, clock, events}; + + Fixture() { TEST_ASSERT_TRUE(pump.initialize()); } +}; + +/// Number of output ON transitions recorded by the real pump — a proxy for +/// "how many fills were started" (each successful runFor drives applyOutput +/// true exactly once; stops drive false and are not counted). +int onTransitions(const MockWaterPump& pump) +{ + int count = 0; + for (bool on : pump.outputCalls) { + if (on) { + ++count; + } + } + return count; +} + +// =========================================================================== +// Level truth table (auto; enabled + auto-level, pump not running) +// =========================================================================== + +// Row: low invalid -> no action (invalid is never treated as "water absent"). +void test_invalid_low_no_action(void) +{ + Fixture f; + f.low.scriptInvalid(); + f.high.scriptValidState(false); + f.controller.tick(/*enabled=*/true, /*autoLevelControl=*/true); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// Row: high invalid -> no action. +void test_invalid_high_no_action(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptInvalid(); + f.controller.tick(true, true); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// Row: both marks invalid -> no action. +void test_both_invalid_no_action(void) +{ + Fixture f; + f.low.scriptInvalid(); + f.high.scriptInvalid(); + f.controller.tick(true, true); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// Row: wet/wet = full -> ensure stopped (no fill started; pump stays off). +void test_full_wet_wet_ensures_stopped(void) +{ + Fixture f; + f.low.scriptValidState(true); + f.high.scriptValidState(true); + f.controller.tick(true, true); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// Row: wet/dry = sufficient water -> no action. +void test_sufficient_wet_dry_no_action(void) +{ + Fixture f; + f.low.scriptValidState(true); + f.high.scriptValidState(false); + f.controller.tick(true, true); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// Row: dry/dry = low water -> start a fill. +void test_low_dry_dry_starts_fill(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(false); + f.controller.tick(true, true); + + TEST_ASSERT_TRUE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); +} + +// Row: dry/wet = physically implausible -> no action. +void test_implausible_low_dry_high_wet_no_action(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(true); + f.controller.tick(true, true); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// =========================================================================== +// Running safety (manual + auto) +// =========================================================================== + +// A fill in progress stops immediately when the high mark reads wet. +void test_stop_on_high_wet_while_running(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(false); + f.controller.tick(true, true); // dry/dry -> start fill + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(5000); + f.high.scriptValidState(true); // high mark reached + f.controller.tick(true, true); + + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(static_cast(StopReason::Commanded), + static_cast(f.pump.getLastStopReason())); +} + +// The fill aborts at the hard 300 s max-fill cap when the high mark never trips +// (the pump self-stops in update() with StopReason::MaxRuntimeForced). +void test_max_fill_abort_at_cap(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(false); + f.controller.tick(true, true); // start fill + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(299'999); + f.controller.tick(true, true); // 1 ms before the cap: still running + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1); + f.controller.tick(true, true); // at the cap: aborted + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(static_cast(StopReason::MaxRuntimeForced), + static_cast(f.pump.getLastStopReason())); +} + +// =========================================================================== +// Post-abort cooldown (FR-012a) +// =========================================================================== + +// After a max-runtime abort with the water still dry/dry, no new AUTOMATIC +// fill starts before the cooldown elapses; a fill DOES start once it does. +void test_cooldown_blocks_then_allows_auto_refill(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(false); + f.controller.tick(true, true); // start fill + + // Never reaches high: the fill aborts at the cap and arms the cooldown. + f.clock.advance(300'000); + f.controller.tick(true, true); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(static_cast(StopReason::MaxRuntimeForced), + static_cast(f.pump.getLastStopReason())); + // The abort tick itself must not re-slam the pump (still one ON so far). + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); + + // 1 ms before the cooldown elapses, still dry/dry: no new fill. + f.clock.advance(ReservoirController::kReservoirRefillCooldownMs - 1); + f.controller.tick(true, true); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); + + // Exactly at the cooldown: a new auto fill starts. + f.clock.advance(1); + f.controller.tick(true, true); + TEST_ASSERT_TRUE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(2, onTransitions(f.pump)); +} + +// A normal high-wet stop does NOT arm the cooldown: a later dry/dry starts a +// fill immediately. +void test_normal_high_wet_stop_does_not_arm_cooldown(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(false); + f.controller.tick(true, true); // start fill + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // High mark trips wet: running safety stops it (Commanded, not an abort). + f.clock.advance(1000); + f.high.scriptValidState(true); + f.controller.tick(true, true); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(static_cast(StopReason::Commanded), + static_cast(f.pump.getLastStopReason())); + + // Water drains back to dry/dry: a new fill starts at once (no cooldown). + f.clock.advance(1000); + f.high.scriptValidState(false); + f.controller.tick(true, true); + TEST_ASSERT_TRUE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(2, onTransitions(f.pump)); +} + +// A manual fill bypasses the post-abort cooldown. +void test_manual_fill_bypasses_cooldown(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(false); + f.controller.tick(true, true); // start auto fill + f.clock.advance(300'000); + f.controller.tick(true, true); // abort at the cap -> arm the cooldown + TEST_ASSERT_FALSE(f.pump.isRunning()); + + // Within the cooldown an auto tick refuses to refill... + f.controller.tick(true, true); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + // ...but an explicit manual fill bypasses the cooldown and starts. + TEST_ASSERT_TRUE(f.controller.startManualFill(60)); + TEST_ASSERT_TRUE(f.pump.isRunning()); +} + +// =========================================================================== +// Manual fill + feature gate +// =========================================================================== + +// A manual fill is refused when the reservoir is already full (high mark wet). +void test_manual_fill_refused_when_full(void) +{ + Fixture f; + f.low.scriptValidState(true); + f.high.scriptValidState(true); // full + + TEST_ASSERT_FALSE(f.controller.startManualFill(60)); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); +} + +// Feature disabled (FR-013): the pump is forced OFF and ALL logic is skipped, +// even when the marks read dry/dry (which would otherwise start a fill). +void test_feature_disabled_forces_off_and_skips_logic(void) +{ + Fixture f; + // A manual fill is running... + TEST_ASSERT_TRUE(f.controller.startManualFill(60)); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // ...disabling the feature forces the pump off, ignoring the dry/dry marks. + f.low.scriptValidState(false); + f.high.scriptValidState(false); + f.controller.tick(/*enabled=*/false, /*autoLevelControl=*/true); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + // A further disabled tick never starts a fill despite the dry/dry marks. + f.clock.advance(1000); + f.controller.tick(false, true); + TEST_ASSERT_FALSE(f.pump.isRunning()); + // Only the manual ON transition ever happened. + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); +} + +} // namespace + void run_reservoir_tests(void) { + // Level truth table + RUN_TEST(test_invalid_low_no_action); + RUN_TEST(test_invalid_high_no_action); + RUN_TEST(test_both_invalid_no_action); + RUN_TEST(test_full_wet_wet_ensures_stopped); + RUN_TEST(test_sufficient_wet_dry_no_action); + RUN_TEST(test_low_dry_dry_starts_fill); + RUN_TEST(test_implausible_low_dry_high_wet_no_action); + + // Running safety + RUN_TEST(test_stop_on_high_wet_while_running); + RUN_TEST(test_max_fill_abort_at_cap); + + // Post-abort cooldown (FR-012a) + RUN_TEST(test_cooldown_blocks_then_allows_auto_refill); + RUN_TEST(test_normal_high_wet_stop_does_not_arm_cooldown); + RUN_TEST(test_manual_fill_bypasses_cooldown); + + // Manual fill + feature gate + RUN_TEST(test_manual_fill_refused_when_full); + RUN_TEST(test_feature_disabled_forces_off_and_skips_logic); } From eba11d29fb2a76ac9d0a31f29dadb3456b6795ba Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 20:22:50 +0200 Subject: [PATCH 5/9] =?UTF-8?q?feat(control):=20PR-11=20US3(1)=20=E2=80=94?= =?UTF-8?q?=20manual=20override=20+=20periodic=20data=20logging?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WateringController gains the operator-override and telemetry-logging paths (still pure, host-tested): - startManual(int) clamps to [1,300] s, runs the plant pump, sets an override flag that exempts the run from the automatic fail-safe; stop() clears it. A pump self-stop (duration/cap) clears the override on the next tick. - tick() now logs env+soil telemetry via maybeLogData() BEFORE the mode / fail-safe branches, so telemetry is recorded regardless of watering state. Soil is read once per tick and reused by both logging and the decision. - maybeLogData() gates on IWallClock::isTimeSet() (never logs a 1970 epoch), honours getDataLogIntervalMs(), stamps IWallClock::nowEpoch(), logs env (temperature/humidity/pressure) and soil (moisture/temperature/ph/ec) plus NPK only when >= 0. soil_humidity is intentionally NOT logged: getHumidity() is identical to getMoisture() (parity), so the max distinct-metric set is exactly 10 = kMaxMetrics (3 env + 4 soil base + 3 NPK), no data loss. Constructor now injects IEnvironmentalSensor& (env) so the controller is the single host-tested owner of periodic data logging. +9 host tests (manual bypass/clamp/clear, auto-not-manual, log cadence/epoch/ NPK filter/time-not-set gate/failsafe-path). Host 289/0; rev1 + rev2 green. Co-Authored-By: Claude Opus 4.8 --- .../include/control/WateringController.h | 80 ++++- .../control/src/WateringController.cpp | 121 +++++++- .../host/main/test_watering_controller.cpp | 288 +++++++++++++++++- 3 files changed, 449 insertions(+), 40 deletions(-) diff --git a/firmware/components/control/include/control/WateringController.h b/firmware/components/control/include/control/WateringController.h index b9b05f5..a76a55e 100644 --- a/firmware/components/control/include/control/WateringController.h +++ b/firmware/components/control/include/control/WateringController.h @@ -4,19 +4,21 @@ * @file WateringController.h * @brief Pure automatic watering logic: pulsed watering + soak gate + fail-safe. * - * Feature 011 (PR-11), User Story 1. All watering-decision and safety-condition - * logic lives here as pure C++17 over the injected interfaces + clocks, so it is - * exercised on the IDF linux preview target against the mocks/fakes - * (test_watering_controller.cpp). This class MUST NOT include esp_* / esp_timer - * or make any hardware/network call: monotonic time comes from ITimeProvider, - * wall-clock time from IWallClock, and every threshold/duration is read from - * IConfigStore each tick (runtime-tunable). Normative contract: + * Feature 011 (PR-11), User Stories 1 + 3. All watering-decision and + * safety-condition logic lives here as pure C++17 over the injected interfaces + + * clocks, so it is exercised on the IDF linux preview target against the + * mocks/fakes (test_watering_controller.cpp). This class MUST NOT include esp_* / + * esp_timer or make any hardware/network call: monotonic time comes from + * ITimeProvider, wall-clock time from IWallClock, and every threshold/duration is + * read from IConfigStore each tick (runtime-tunable). Normative contract: * specs/011-watering-controller-host-tests/contracts/watering-controller.md; * decision tables + constants: .../data-model.md. * - * Scope note: this US1 slice implements the automatic path (tick()). The manual - * override (startManual/stop bookkeeping) and periodic data-logging are US3 and - * are only left as clean seams here — not implemented. + * Scope note: the US1 slice implements the automatic path (thresholds, soak gate, + * fail-safe). The US3 slice adds the manual override (startManual/stop, exempt + * from the automatic fail-safe) and the periodic env+soil data-logging that runs + * every tick regardless of mode. On-target soil access is wired through a + * LockedSoilSensor snapshot; the pure logic drives the injected sensors directly. * * Concurrency: the controller is unsynchronized and single-writer (its own * watchdog-registered task). On-target US3 wires the sensor through a @@ -32,6 +34,7 @@ #include "events/EventLogger.h" #include "interfaces/IConfigStore.h" #include "interfaces/IDataStorage.h" +#include "interfaces/IEnvironmentalSensor.h" #include "interfaces/ISoilSensor.h" #include "interfaces/ITimeProvider.h" #include "interfaces/IWallClock.h" @@ -64,10 +67,12 @@ class WateringController { * Constructing with a sensor that fails to initialize is safe: no work runs * in the constructor and tick() simply fails safe (FR-015). */ - WateringController(ISoilSensor& soil, IWaterPump& plant, IConfigStore& config, + WateringController(ISoilSensor& soil, IEnvironmentalSensor& env, + IWaterPump& plant, IConfigStore& config, IDataStorage& storage, ITimeProvider& clock, IWallClock& wallClock, EventLogger& events) : soil_(soil), + env_(env), plant_(plant), config_(config), storage_(storage), @@ -81,21 +86,55 @@ class WateringController { WateringController& operator=(const WateringController&) = delete; /** - * @brief One automatic evaluation. Non-blocking; call at a fixed cadence. + * @brief One evaluation. Non-blocking; call at a fixed cadence. * * Order (safety first, soak gate last): pump self-stop/cap enforcement → - * enabled gate → fail-safe (unavailable/stale/invalid) → gate-on-read → - * watering decision (stop-at-high / start-burst-if-soak-elapsed). + * burst-end detection → single soil read → periodic data-log (runs in every + * mode, before any early return) → manual-override bypass → enabled gate → + * fail-safe (unavailable/stale/invalid) → gate-on-read → watering decision + * (stop-at-high / start-burst-if-soak-elapsed). */ void tick(); + /** + * @brief Operator manual override: run the plant pump for a bounded time. + * + * Clamps @p durationS to [1, 300] s, calls plant.runFor(clamped) and, on + * success, flags the run manual so tick() exempts it from the automatic + * fail-safe and the soak gate (FR-007/008). The reservoir cooldown is N/A + * here (that gate lives in ReservoirController). Returns the pump result. + */ + bool startManual(int durationS); + + /** + * @brief Stop the plant pump and clear any manual override. Works in any + * mode; a subsequent tick() resumes automatic evaluation (FR-010). + */ + void stop(); + private: + /** + * @brief Periodic env + soil telemetry log (FR-014). Runs every tick from + * tick(), before any mode/fail-safe early return, so telemetry is recorded + * even while watering is disabled or a fail-safe is active. + * + * Gated on IWallClock::isTimeSet() (never logs a bogus 1970 epoch) and on + * the configured data-log interval (the first eligible log after time is set + * fires immediately). Env readings are logged on a successful, available + * read; soil readings are logged only when @p soilValid (the result of the + * single soil read() this tick), with NPK included only when >= 0. All + * writes carry IWallClock::nowEpoch(); storage self-bounds, so the store + * result is ignored. + */ + void maybeLogData(int64_t now, bool soilValid); + ISoilSensor& soil_; + IEnvironmentalSensor& env_; ///< periodic data-logging source (US3) IWaterPump& plant_; IConfigStore& config_; - IDataStorage& storage_; ///< US3 data-logging seam (unused in US1) + IDataStorage& storage_; ITimeProvider& clock_; - IWallClock& wallClock_; ///< US3 data-logging seam (unused in US1) + IWallClock& wallClock_; EventLogger& events_; /// Monotonic time the last automatic burst ended — the soak-gate origin @@ -110,6 +149,15 @@ class WateringController { /// pump's own timed self-stop (duration/cap) as a burst end so the soak /// gate starts from the burst end regardless of how it stopped. bool burstActive_ = false; + + /// True while an operator manual run is active. A manual run is exempt from + /// the automatic fail-safe and the soak gate; it clears on stop() or when + /// the pump self-stops at the 300 s cap. + bool manualRunActive_ = false; + + /// Monotonic time of the last data-log batch (0 = none yet — the first + /// eligible log after the wall clock is set fires immediately). + int64_t lastDataLogMs_ = 0; }; #endif /* WATERINGSYSTEM_CONTROL_WATERINGCONTROLLER_H */ diff --git a/firmware/components/control/src/WateringController.cpp b/firmware/components/control/src/WateringController.cpp index b489ef4..e129428 100644 --- a/firmware/components/control/src/WateringController.cpp +++ b/firmware/components/control/src/WateringController.cpp @@ -28,16 +28,11 @@ void WateringController::tick() burstActive_ = false; } - // Automatic path only. When automatic watering is disabled the mode is - // manual/suspended (operator override, US3): take no automatic action. - if (!config_.getWateringEnabled()) { - return; - } - - // ---- Read the sensor (US1 drives ISoilSensor directly; US3 wires a - // LockedSoilSensor snapshot so read()+availability come from one locked - // acquisition). We react only to the read RESULT and the availability - // signal, never to specific numeric error codes. ------------------------ + // ---- Read the sensor ONCE per tick (US1 drives ISoilSensor directly; US3 + // wires a LockedSoilSensor snapshot so read()+availability come from one + // locked acquisition). We react only to the read RESULT and the + // availability signal, never to specific numeric error codes. The soil + // getters below serve the values from THIS read — no second I/O. --------- const bool readOk = soil_.read(); const bool available = soil_.isAvailable(); const float moisture = soil_.getMoisture(); @@ -54,6 +49,29 @@ void WateringController::tick() const bool stale = (lastValidSoilMs_ == 0) || (now - lastValidSoilMs_ > kStalenessMs); + // ---- PERIODIC DATA-LOG (runs in EVERY mode, before any early return) ---- + // Telemetry must be recorded even when automatic watering is disabled, a + // manual override is active, or a fail-safe is about to fire (FR-014). Soil + // is logged only when this tick's read was successful and in range. + maybeLogData(now, /*soilValid=*/(readOk && inRange)); + + // ---- MANUAL OVERRIDE (bypasses fail-safe + soak/decision logic) -------- + // A manual run is an explicit operator override (FR-007/008): it is exempt + // from the automatic fail-safe and the soak gate. A pump self-stop at the + // 300 s cap clears the override on the next tick. + if (manualRunActive_) { + if (!plant_.isRunning()) { + manualRunActive_ = false; + } + return; + } + + // Automatic path only. When automatic watering is disabled the mode is + // manual/suspended: take no automatic action. + if (!config_.getWateringEnabled()) { + return; + } + // ---- FAIL-SAFE (unconditional, checked BEFORE the soak gate) ---------- // Never delayed or suppressed by the soak-pause/scheduling logic (FR-006). const char* failsafeReason = nullptr; @@ -110,3 +128,86 @@ void WateringController::tick() // soil still reads dry (FR-003). } } + +bool WateringController::startManual(int durationS) +{ + // Clamp to the pump's accepted range [1, 300] s (the pump rejects 0 and + // anything over its 300 s hard cap; we clamp rather than reject so an + // operator command always maps to a bounded run). + int clamped = durationS; + if (clamped < 1) { + clamped = 1; + } else if (clamped > 300) { + clamped = 300; + } + const bool started = plant_.runFor(clamped); + if (started) { + manualRunActive_ = true; + } + return started; +} + +void WateringController::stop() +{ + plant_.stop(); + manualRunActive_ = false; +} + +void WateringController::maybeLogData(int64_t now, bool soilValid) +{ + // No plausible wall-clock timestamp yet — never log a bogus 1970 epoch. + if (!wallClock_.isTimeSet()) { + return; + } + + // Interval gate: the first eligible log after time is set fires immediately + // (lastDataLogMs_ == 0 counts as due); otherwise wait out the interval. + const int64_t interval = static_cast(config_.getDataLogIntervalMs()); + const bool due = + (lastDataLogMs_ == 0) || (now - lastDataLogMs_ >= interval); + if (!due) { + return; + } + lastDataLogMs_ = now; + + const uint32_t epoch = wallClock_.nowEpoch(); + + // Environmental telemetry (only on a successful, available read). + if (env_.read() && env_.isAvailable()) { + storage_.storeSensorReading("env_temperature", epoch, + env_.getTemperature()); + storage_.storeSensorReading("env_humidity", epoch, env_.getHumidity()); + storage_.storeSensorReading("env_pressure", epoch, env_.getPressure()); + } + + // Soil telemetry (uses the values from this tick's single read()). + if (soilValid) { + storage_.storeSensorReading("soil_moisture", epoch, + soil_.getMoisture()); + storage_.storeSensorReading("soil_temperature", epoch, + soil_.getTemperature()); + // NOTE: soil humidity is deliberately NOT logged. ISoilSensor's + // getHumidity() is documented as identical to getMoisture() (a single + // moisture/humidity quantity in register 0x0000; the legacy driver + // exposed it under both names), so logging it would be a pure duplicate + // of soil_moisture. Dropping it keeps the max distinct metric set at + // exactly kMaxMetrics (10): 3 env + 4 soil-base + 3 NPK. + storage_.storeSensorReading("soil_ph", epoch, soil_.getPH()); + storage_.storeSensorReading("soil_ec", epoch, soil_.getEC()); + + // NPK is only meaningful when >= 0 (the sensor reports -1 when a + // channel is unsupported/unavailable); skip a negative channel. + const float n = soil_.getNitrogen(); + const float p = soil_.getPhosphorus(); + const float k = soil_.getPotassium(); + if (n >= 0) { + storage_.storeSensorReading("soil_nitrogen", epoch, n); + } + if (p >= 0) { + storage_.storeSensorReading("soil_phosphorus", epoch, p); + } + if (k >= 0) { + storage_.storeSensorReading("soil_potassium", epoch, k); + } + } +} diff --git a/firmware/test_apps/host/main/test_watering_controller.cpp b/firmware/test_apps/host/main/test_watering_controller.cpp index b665371..7fcf259 100644 --- a/firmware/test_apps/host/main/test_watering_controller.cpp +++ b/firmware/test_apps/host/main/test_watering_controller.cpp @@ -2,21 +2,25 @@ // SPDX-License-Identifier: AGPL-3.0-or-later /** * @file test_watering_controller.cpp - * @brief Host suite for the pure WateringController (feature 011, US1). + * @brief Host suite for the pure WateringController (feature 011, US1 + US3). * * Drives the REAL WateringController logic over MockSoilSensor + - * MockWaterPump (real WaterPump enforcement over FakeTimeProvider) + - * MockConfigStore + MockDataStorage + FakeWallClock + EventLogger. Registered - * via run_watering_controller_tests() from the shared Unity runner - * (test_main.cpp); the process exit code equals the failure count (CI gate). + * MockEnvironmentalSensor + MockWaterPump (real WaterPump enforcement over + * FakeTimeProvider) + MockConfigStore + MockDataStorage + FakeWallClock + + * EventLogger. Registered via run_watering_controller_tests() from the shared + * Unity runner (test_main.cpp); the process exit code equals the failure count + * (CI gate). * - * Coverage maps to tasks.md T006 (automatic + soak gate) and T007 (fail-safe): - * start-at-low, no-start/allow-restart across the soak pause, stop-at-high, - * runtime config change, disabled -> no action, gate-on-read (placeholder + - * transient failed read), fail-safe unavailable/stale/invalid stops, fail-safe - * never delayed by the soak gate, and graceful degradation (sensor always - * failing -> never waters, no crash). The manual override + periodic - * data-logging branches (US3) land later (T011). + * Coverage maps to tasks.md T006 (automatic + soak gate), T007 (fail-safe) and + * T011 (manual override + periodic data-logging): start-at-low, + * no-start/allow-restart across the soak pause, stop-at-high, runtime config + * change, disabled -> no action, gate-on-read (placeholder + transient failed + * read), fail-safe unavailable/stale/invalid stops, fail-safe never delayed by + * the soak gate, graceful degradation (sensor always failing -> never waters, + * no crash), manual override (bypasses fail-safe, 300 s cap, lower clamp, + * auto-runs stay automatic, stop() clears the override) and data-logging + * (cadence, epoch timestamp, NPK >= 0 filter, time-not-set gate, independence + * from the fail-safe path). */ #include @@ -28,6 +32,7 @@ #include "control/WateringController.h" #include "events/EventLogger.h" #include "interfaces/IDataStorage.h" +#include "sensors/testing/MockEnvironmentalSensor.h" #include "sensors/testing/MockSoilSensor.h" #include "storage/testing/MockConfigStore.h" #include "storage/testing/MockDataStorage.h" @@ -43,13 +48,14 @@ namespace { struct Fixture { FakeTimeProvider clock; MockSoilSensor soil; + MockEnvironmentalSensor env; MockWaterPump pump{"plant", clock}; MockConfigStore config; MockDataStorage storage; FakeWallClock wallClock; EventLogger events{storage, wallClock}; - WateringController controller{soil, pump, config, storage, - clock, wallClock, events}; + WateringController controller{soil, env, pump, config, + storage, clock, wallClock, events}; Fixture() { @@ -88,6 +94,17 @@ int failsafeEventCount(const MockDataStorage& storage) return count; } +/// Total sensor readings persisted across all metrics (a proxy for "how many +/// data-log values were written"). Independent of the event log. +int sensorReadingCount(const MockDataStorage& storage) +{ + int count = 0; + for (const auto& entry : storage.history) { + count += static_cast(entry.second.size()); + } + return count; +} + /// Script the sensor's next tick outcome via the plain public fields (no FIFO /// script): read() returns @p readOk, isAvailable() returns @p available and /// the getters serve @p moisture. A failed read leaves the value in place @@ -387,6 +404,238 @@ void test_graceful_degradation_never_waters(void) TEST_ASSERT_EQUAL_UINT32(0, f.events.droppedEvents()); } +// =========================================================================== +// T011 — manual override + periodic data-logging (US3) +// =========================================================================== + +// FR-007/008: a manual run is an explicit operator override — it keeps running +// even when the soil sensor is failing/unavailable (exempt from the automatic +// fail-safe). The wall clock is left unset so no data-logging noise interferes. +void test_manual_run_bypasses_sensor_failure(void) +{ + Fixture f; + setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + + TEST_ASSERT_TRUE(f.controller.startManual(60)); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // A tick that would fail-safe an automatic run leaves the manual run alone. + f.clock.advance(1000); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, failsafeEventCount(f.storage)); +} + +// FR-008: an over-long manual duration is clamped to the 300 s cap — the run is +// bounded, stopping at 300 s with MaxRuntimeForced (not earlier). +void test_manual_run_capped_at_300s(void) +{ + Fixture f; + setSensor(f, false, false, 20.0f); + + TEST_ASSERT_TRUE(f.controller.startManual(9999)); // clamps to 300 + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // Well before the cap the run is still going (proves it was not clamped low). + f.clock.advance(250'000); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // At exactly 300 s the pump self-stops and the override clears next tick. + f.clock.advance(50'000); // total 300 s + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(static_cast(StopReason::MaxRuntimeForced), + static_cast(f.pump.getLastStopReason())); +} + +// FR-008: a below-range manual duration is clamped up to 1 s — startManual +// succeeds and starts a bounded 1 s run. +void test_manual_run_lower_clamp(void) +{ + Fixture f; + setSensor(f, false, false, 20.0f); + + TEST_ASSERT_TRUE(f.controller.startManual(0)); // clamps to 1 + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1000); // the 1 s run self-stops + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(static_cast(StopReason::DurationElapsed), + static_cast(f.pump.getLastStopReason())); +} + +// FR-007: an automatically-started burst is NOT flagged manual — the fail-safe +// still applies to it (the mirror of the manual-bypass case). +void test_auto_run_is_not_flagged_manual(void) +{ + Fixture f; + f.config.stored.wateringDurationS = 300; // keep the burst running + setSensor(f, true, true, 20.0f); + f.controller.tick(); // automatic burst starts + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.clock.advance(1000); + setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + f.controller.tick(); + + TEST_ASSERT_FALSE(f.pump.isRunning()); // fail-safe stopped it + TEST_ASSERT_EQUAL_INT(1, failsafeEventCount(f.storage)); +} + +// FR-010: stop() clears the manual override so a later tick resumes automatic +// evaluation (proved by an automatic burst starting afterwards). +void test_stop_clears_manual_override(void) +{ + Fixture f; + setSensor(f, false, false, 20.0f); + TEST_ASSERT_TRUE(f.controller.startManual(60)); + TEST_ASSERT_TRUE(f.pump.isRunning()); + + f.controller.stop(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + // Override cleared: dry + valid + enabled + soak elapsed -> automatic burst. + f.clock.advance(1000); + setSensor(f, /*readOk=*/true, /*available=*/true, /*moisture=*/20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); + // Two ON transitions total: the manual run + the fresh automatic burst + // (proving the override was cleared and automatic evaluation resumed). + TEST_ASSERT_EQUAL_INT(2, onTransitions(f.pump)); +} + +// FR-014: with time set + env/soil valid, the first eligible tick logs one +// env+soil batch; a tick inside the interval logs nothing new; a tick past the +// interval logs the next batch. +// +// All three NPK channels are scripted >= 0, so the batch spans the full, +// maximal metric set: 3 env + 4 soil-base (moisture, temperature, ph, ec) + +// 3 NPK = exactly 10 distinct metrics, which fits the store's kMaxMetrics=10 +// cap with no silent drop (soil_humidity is not logged — it duplicates +// soil_moisture). +void test_data_log_cadence(void) +{ + Fixture f; + f.config.stored.dataLogIntervalMs = 60'000; + f.wallClock.setEpoch(1'700'000'000); // time set + f.env.scriptSuccessfulRead(21.5f, 55.0f, 1013.0f); + // Soil valid, moisture 40 (> low 30 -> no watering); all NPK >= 0. + f.soil.scriptSuccessfulRead(40.0f, 18.0f, 40.0f, 6.5f, 1.2f, 3.0f, 5.0f, + 8.0f); + + // First eligible tick logs one batch of 10. + f.clock.advance(1000); + f.controller.tick(); + TEST_ASSERT_EQUAL_INT(10, sensorReadingCount(f.storage)); + + // Inside the interval: nothing new. + f.clock.advance(59'999); + f.controller.tick(); + TEST_ASSERT_EQUAL_INT(10, sensorReadingCount(f.storage)); + + // Exactly at the interval: the next batch is logged (+10 -> 20). + f.clock.advance(1); + f.controller.tick(); + TEST_ASSERT_EQUAL_INT(20, sensorReadingCount(f.storage)); +} + +// FR-014: stored readings carry nowEpoch() as their timestamp, and an NPK +// channel < 0 is skipped while the >= 0 channels are logged. +void test_data_log_epoch_and_npk_filter(void) +{ + Fixture f; + f.config.stored.dataLogIntervalMs = 60'000; + const uint32_t kEpoch = 1'700'000'123; + f.wallClock.setEpoch(kEpoch); + f.env.scriptSuccessfulRead(21.5f, 55.0f, 1013.0f); + // Nitrogen negative -> skipped; phosphorus + potassium >= 0 -> logged. + f.soil.scriptSuccessfulRead(40.0f, 18.0f, 40.0f, 6.5f, 1.2f, -1.0f, 5.0f, + 8.0f); + + f.clock.advance(1000); + f.controller.tick(); + + // One NPK channel negative -> filtered out: 3 env + 4 soil-base + 2 NPK + // = 9 metrics logged this tick, none dropped by the store. + TEST_ASSERT_EQUAL_INT(9, sensorReadingCount(f.storage)); + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("soil_nitrogen", 0, UINT32_MAX) + .size())); + // soil_humidity is never logged (it duplicates soil_moisture). + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("soil_humidity", 0, UINT32_MAX) + .size())); + const auto phos = + f.storage.getSensorReadings("soil_phosphorus", 0, UINT32_MAX); + TEST_ASSERT_EQUAL_INT(1, static_cast(phos.size())); + TEST_ASSERT_EQUAL_INT( + 1, static_cast( + f.storage.getSensorReadings("soil_potassium", 0, UINT32_MAX) + .size())); + + // Epoch on both an env and a soil reading equals nowEpoch(). + TEST_ASSERT_EQUAL_UINT32(kEpoch, phos[0].epoch); + const auto temp = + f.storage.getSensorReadings("env_temperature", 0, UINT32_MAX); + TEST_ASSERT_EQUAL_INT(1, static_cast(temp.size())); + TEST_ASSERT_EQUAL_UINT32(kEpoch, temp[0].epoch); +} + +// FR-014: while the wall clock is unset nothing is logged (no bogus 1970 +// epoch), regardless of the interval; logging resumes once time is set. +void test_data_log_gated_on_time_set(void) +{ + Fixture f; + f.config.stored.dataLogIntervalMs = 60'000; + f.env.scriptSuccessfulRead(21.5f, 55.0f, 1013.0f); + // All NPK >= 0 -> the full 10 distinct metrics, fits the store cap. + f.soil.scriptSuccessfulRead(40.0f, 18.0f, 40.0f, 6.5f, 1.2f, 3.0f, 5.0f, + 8.0f); + + // Time not set: no logging even after more than a full interval. + f.clock.advance(1000); + f.controller.tick(); + f.clock.advance(120'000); + f.controller.tick(); + TEST_ASSERT_EQUAL_INT(0, sensorReadingCount(f.storage)); + + // Time set: the next eligible tick logs a full batch of 10. + f.wallClock.setEpoch(1'700'000'000); + f.clock.advance(1000); + f.controller.tick(); + TEST_ASSERT_EQUAL_INT(10, sensorReadingCount(f.storage)); +} + +// FR-014: data-logging runs before the fail-safe early return — while the soil +// sensor is unavailable (fail-safe path) but env is valid and time is set, env +// metrics are still logged and soil metrics are skipped that tick. +void test_data_log_runs_on_failsafe_path(void) +{ + Fixture f; + f.config.stored.dataLogIntervalMs = 60'000; + f.wallClock.setEpoch(1'700'000'000); + f.env.scriptSuccessfulRead(21.5f, 55.0f, 1013.0f); + // Soil read fails + unavailable -> fail-safe path, soilValid == false. + setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + + f.clock.advance(1000); + f.controller.tick(); + + TEST_ASSERT_EQUAL_INT(3, sensorReadingCount(f.storage)); // env only + TEST_ASSERT_EQUAL_INT( + 1, static_cast( + f.storage.getSensorReadings("env_temperature", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("soil_moisture", 0, UINT32_MAX) + .size())); +} + } // namespace void run_watering_controller_tests(void) @@ -408,4 +657,15 @@ void run_watering_controller_tests(void) RUN_TEST(test_failsafe_not_delayed_by_soak); RUN_TEST(test_failsafe_during_pending_soak_takes_no_action); RUN_TEST(test_graceful_degradation_never_waters); + + // T011 — manual override + periodic data-logging (US3) + RUN_TEST(test_manual_run_bypasses_sensor_failure); + RUN_TEST(test_manual_run_capped_at_300s); + RUN_TEST(test_manual_run_lower_clamp); + RUN_TEST(test_auto_run_is_not_flagged_manual); + RUN_TEST(test_stop_clears_manual_override); + RUN_TEST(test_data_log_cadence); + RUN_TEST(test_data_log_epoch_and_npk_filter); + RUN_TEST(test_data_log_gated_on_time_set); + RUN_TEST(test_data_log_runs_on_failsafe_path); } From 97b6f1d7f418046d819f4742129ba2614923f3d1 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 20:31:13 +0200 Subject: [PATCH 6/9] =?UTF-8?q?feat(main):=20PR-11=20US3(2)=20=E2=80=94=20?= =?UTF-8?q?wire=20watering=20+=20reservoir=20controllers=20on-target?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the decision-layer watering task and constructs the controllers in app_main (T013/T014/T015): - new main/watering_task.{h,cpp}: a watchdog-subscribed FreeRTOS task that ticks at config.getSensorReadIntervalMs() (floored at 1 s). Each cycle it calls WateringController::tick() and (rev1) ReservoirController::tick(). The controller-as-reader does the periodic soil Modbus read here, refreshing the LockedSoilSensor cache the PR-09 /sensors endpoint serves (this is why no separate soil-reader task is needed). Blocking bus I/O is isolated on this task, off the 10 Hz safety loop. - app_main constructs WateringController (+ ReservoirController on rev1) as function-local statics over the live Locked* wrappers, after the sensors and before console registration, and starts the watering task after sensor_task. pumps_force_off() stays first; the 10 Hz enforcement loop is unchanged (it still owns precise pump-timing). The API mode flag reaches the controller purely through config (getWateringEnabled(), read each tick) — no direct ApiServer<->controller call. - reservoir tick mapping (rev1): tick(true, getWateringEnabled()) — the pump always exists so enabled is always true; automatic level control is gated by the same mode flag, so manual mode suspends auto-fill while manual API fills still work. There is deliberately no dedicated auto-level config flag. - main/CMakeLists: +watering_task.cpp, +control in PRIV_REQUIRES. rev1 + rev2 build green; host suite unchanged (289/0, control component untouched). Co-Authored-By: Claude Opus 4.8 --- firmware/main/CMakeLists.txt | 4 +- firmware/main/app_main.cpp | 31 ++++++++ firmware/main/watering_task.cpp | 134 ++++++++++++++++++++++++++++++++ firmware/main/watering_task.h | 47 +++++++++++ 4 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 firmware/main/watering_task.cpp create mode 100644 firmware/main/watering_task.h diff --git a/firmware/main/CMakeLists.txt b/firmware/main/CMakeLists.txt index e3dd4c4..2dc008a 100644 --- a/firmware/main/CMakeLists.txt +++ b/firmware/main/CMakeLists.txt @@ -1,12 +1,12 @@ idf_component_register( SRCS "app_main.cpp" "diag_console.cpp" "sensor_task.cpp" "wifi_task.cpp" - "system_observer.cpp" "task_watchdog.cpp" + "system_observer.cpp" "task_watchdog.cpp" "watering_task.cpp" # esp_task_wdt (task watchdog, feature 008 US3) lives in esp_system, already # required below — no new provider needed. PRIV_REQUIRES board esp_driver_gpio esp_app_format actuators interfaces console esp_timer storage nvs_flash sensors esp_netif esp_event - network time events esp_system api + network time events esp_system api control ) # Build-time littlefs image of the committed seed directory diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index c4110a3..696ec73 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -53,6 +53,10 @@ #include "sensors/LockedPowerSensor.h" #endif #include "api/ApiServer.h" +#include "control/WateringController.h" +#if BOARD_HAS_RESERVOIR_PUMP +#include "control/ReservoirController.h" +#endif #include "network/EspWifiDriver.h" #include "network/ProvisioningPortal.h" #include "network/WifiBootMode.h" @@ -68,6 +72,7 @@ #include "diag_console.h" #include "sensor_task.h" +#include "watering_task.h" #include "system_observer.h" #include "task_watchdog.h" #include "wifi_task.h" @@ -624,6 +629,18 @@ extern "C" void app_main(void) BOARD_PIN_LEVEL_HIGH); } + // Watering controllers (feature 011). Pure decision logic over the same + // Locked* wrappers every other task uses; run on their own watchdog- + // registered task (below), never on the network/HTTP path (FR-017). The + // 10 Hz loop still owns precise pump-timing enforcement. + static WateringController watering_controller( + soil_sensor, env_sensor, plant, config, storage, time_provider, + wall_clock, event_logger); +#if BOARD_HAS_RESERVOIR_PUMP + static ReservoirController reservoir_controller( + level_low, level_high, reservoir, time_provider, event_logger); +#endif + // Serial diagnostic REPL (rig testing; contracts/serial-diagnostic.md). // Pump registration is capability-aware: single-pump boards register // exactly the plant pump — `pump reservoir` does not exist there @@ -660,6 +677,20 @@ extern "C" void app_main(void) // sensor failed init above — lazy re-init recovers later (parity). sensor_task_start(env_sensor); + // Decision-layer watering task (feature 011). Runs the pure controllers at + // the sensor-read cadence on their own watchdog-registered task: the + // WateringController is the periodic soil reader (its blocking Modbus read + // refreshes the LockedSoilSensor cache /sensors serves) and the reservoir + // fill state machine ticks alongside it (rev1). The 10 Hz loop below is + // unchanged and still owns precise pump-timing enforcement. The mode flag + // reaches the controllers purely through `config` (read each tick) — no + // direct API↔controller call. +#if BOARD_HAS_RESERVOIR_PUMP + watering_task_start(watering_controller, reservoir_controller, config); +#else + watering_task_start(watering_controller, config); +#endif + // /api/v1/ HTTP server (feature 009 US1). Constructed here — after EVERY // sensor plus the config/storage/clock it reports exist — and only in // station mode (wifi_manager is nullptr in provisioning/headless mode, so no diff --git a/firmware/main/watering_task.cpp b/firmware/main/watering_task.cpp new file mode 100644 index 0000000..40b0333 --- /dev/null +++ b/firmware/main/watering_task.cpp @@ -0,0 +1,134 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file watering_task.cpp + * @brief Decision-layer watering task at the sensor-read cadence (feature 011). + * + * This task runs the DECISION layer, separate from the 10 Hz main loop that + * still owns precise pump-timing enforcement (pump update() self-stop + 300 s + * cap, level update(), observer.poll()). Each cycle it calls + * WateringController::tick() and, on boards with a reservoir pump, + * ReservoirController::tick(...). + * + * Controller-as-reader: WateringController::tick() performs the periodic soil + * read() (the blocking Modbus round-trip) which refreshes the LockedSoilSensor + * cache that the PR-09 /sensors endpoint serves — so NO separate soil-reader + * task is needed. That blocking read (plus the periodic littlefs data-log) is + * isolated on THIS task and never runs on the 10 Hz safety loop. + * + * Watchdog: this is a watering-critical task, so it subscribes to the task WDT + * and feeds once per cycle (mirrors sensor_task). The blocking Modbus read is + * well under the WDT timeout. + * + * Isolation: the task shares nothing with the network/HTTP path beyond the same + * Locked* wrappers every other task uses (FR-017). + */ + +#include "watering_task.h" + +#include "esp_log.h" +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" + +#include "task_watchdog.h" + +static const char *TAG = "watering_task"; + +namespace { + +constexpr uint32_t kStackBytes = 8192; ///< blocking Modbus read + littlefs log +constexpr UBaseType_t kPriority = 1; ///< same class as sensor_task +constexpr uint32_t kFloorMs = 1000; ///< IConfigStore sensor-interval floor + +/// Long-lived task context (the task never exits). A single static instance +/// holds borrowed pointers to the app_main collaborators. +struct WateringTaskCtx { + WateringController* controller; + IConfigStore* config; +#if BOARD_HAS_RESERVOIR_PUMP + ReservoirController* reservoir; +#endif +}; + +WateringTaskCtx ctx; + +[[noreturn]] void watering_task(void* arg) +{ + WateringTaskCtx* c = static_cast(arg); + + // Watering-critical task: subscribe to the task WDT (feature 008 US3). The + // period is re-read each loop and floored at 1000 ms, well under the 20 s + // default timeout; the feed each cycle is the liveness proof the WDT needs. + watchdog_subscribe_current_task(); + + while (true) { + // Re-read the cadence each loop so a config change takes effect without + // a restart. vTaskDelay (not vTaskDelayUntil) because the period is + // dynamic. + uint32_t periodMs = c->config->getSensorReadIntervalMs(); + if (periodMs < kFloorMs) { + periodMs = kFloorMs; + } + vTaskDelay(pdMS_TO_TICKS(periodMs)); + + // Feed once per cycle: this task is alive and servicing the WDT. + watchdog_feed(); + + // Decision layer. tick() does the periodic soil read (refreshing the + // LockedSoilSensor cache) + the watering decision + the periodic + // data-log. + c->controller->tick(); +#if BOARD_HAS_RESERVOIR_PUMP + // Reservoir flag mapping: `enabled` is always true — on rev1 the + // reservoir pump always exists and the feature is on, and we never + // force-off a pump the operator might be manually filling. Automatic + // level-based filling is gated by the SAME mode flag as plant watering + // (getWateringEnabled), so "manual mode" suspends all automation while + // manual API fills still work. There is deliberately NO dedicated + // auto-level config flag. + c->reservoir->tick(true, c->config->getWateringEnabled()); +#endif + } +} + +} // namespace + +#if BOARD_HAS_RESERVOIR_PUMP +void watering_task_start(WateringController& controller, ReservoirController& reservoir, + IConfigStore& config) +{ + ctx.controller = &controller; + ctx.config = &config; + ctx.reservoir = &reservoir; + + const BaseType_t created = + xTaskCreate(watering_task, "watering_task", kStackBytes, &ctx, + kPriority, nullptr); + if (created != pdPASS) { + // Not a safety function: log and continue. The 10 Hz loop still + // enforces pump timing; only the decision layer is absent. + ESP_LOGE(TAG, "failed to create watering task"); + return; + } + ESP_LOGI(TAG, "watering task started (%lu ms cadence floor)", + static_cast(kFloorMs)); +} +#else +void watering_task_start(WateringController& controller, IConfigStore& config) +{ + ctx.controller = &controller; + ctx.config = &config; + + const BaseType_t created = + xTaskCreate(watering_task, "watering_task", kStackBytes, &ctx, + kPriority, nullptr); + if (created != pdPASS) { + // Not a safety function: log and continue. The 10 Hz loop still + // enforces pump timing; only the decision layer is absent. + ESP_LOGE(TAG, "failed to create watering task"); + return; + } + ESP_LOGI(TAG, "watering task started (%lu ms cadence floor)", + static_cast(kFloorMs)); +} +#endif diff --git a/firmware/main/watering_task.h b/firmware/main/watering_task.h new file mode 100644 index 0000000..24f8a37 --- /dev/null +++ b/firmware/main/watering_task.h @@ -0,0 +1,47 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file watering_task.h + * @brief Decision-layer watering task (app wiring, feature 011). + * + * App-level FreeRTOS task, not a component: it runs the pure WateringController + * (and, on boards with a reservoir pump, the ReservoirController) at the sensor- + * read cadence. The controller acts as the periodic soil reader — its tick() + * performs the blocking Modbus read that refreshes the LockedSoilSensor cache + * (the same cache the /sensors endpoint serves), so no separate soil-reader task + * is needed. The 10 Hz main loop still owns precise pump-timing enforcement. + * Task/behaviour contract: specs/011-watering-controller-host-tests/. + */ + +#ifndef WATERINGSYSTEM_MAIN_WATERING_TASK_H +#define WATERINGSYSTEM_MAIN_WATERING_TASK_H + +#include "board/board.h" +#include "control/WateringController.h" +#include "interfaces/IConfigStore.h" +#if BOARD_HAS_RESERVOIR_PUMP +#include "control/ReservoirController.h" +#endif + +/** + * @brief Start the decision-layer watering task. + * + * Pass the Locked*-backed controllers built in app_main; they must outlive the + * task (i.e. forever — function-local statics). The task subscribes to the task + * WDT and ticks every IConfigStore::getSensorReadIntervalMs() (re-read each + * loop so config changes take effect), floored at 1000 ms. A task-creation + * failure is logged and swallowed — like the sensor task, the decision layer is + * started best-effort and the 10 Hz safety loop is unaffected. + * + * @param controller Automatic + manual plant watering logic (soil reader). + * @param reservoir Reservoir auto-fill state machine (rev1 only). + * @param config Runtime-tunable cadence and mode flag, re-read each tick. + */ +#if BOARD_HAS_RESERVOIR_PUMP +void watering_task_start(WateringController& controller, ReservoirController& reservoir, + IConfigStore& config); +#else +void watering_task_start(WateringController& controller, IConfigStore& config); +#endif + +#endif /* WATERINGSYSTEM_MAIN_WATERING_TASK_H */ From 17810d40182bd43719e6b37abf52ea5eb8833268 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 20:35:02 +0200 Subject: [PATCH 7/9] =?UTF-8?q?fix(main):=20PR-11=20US3(T016)=20=E2=80=94?= =?UTF-8?q?=20serialize=20rs485test=20with=20the=20soil=20reader?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diag console rs485test command drove the raw EspModbusClient directly, bypassing LockedSoilSensor's mutex. With PR-11's periodic soil reader now issuing bus transactions from the watering task, that was a cross-task RS485 bus-transaction race (documented TODO at diag_console rs485test). Fix: new header-only LockedModbusClient (IModbusClient decorator, one mutex around every call, same pattern as LockedSoilSensor/LockedWaterPump). app_main wraps the EspModbusClient in it and injects the wrapper everywhere — the ModbusSoilSensor's transactions and rs485test now share one mutex, so no two bus transactions overlap. Lock order is always (soil mutex -> modbus mutex) or (modbus mutex alone); the wrapper never calls back into the sensor, so no deadlock. Per-transaction serialization suffices: interleaving complete request/response transactions on the bus is safe, only overlapping ones corrupt. rs485test code is unchanged (the fix is purely the injected locked instance); its stale TODO comment is updated to note the resolution. rev1 + rev2 build green; host suite unchanged (289/0). Co-Authored-By: Claude Opus 4.8 --- .../include/sensors/LockedModbusClient.h | 104 ++++++++++++++++++ firmware/main/app_main.cpp | 4 +- firmware/main/diag_console.cpp | 10 +- 3 files changed, 113 insertions(+), 5 deletions(-) create mode 100644 firmware/components/sensors/include/sensors/LockedModbusClient.h diff --git a/firmware/components/sensors/include/sensors/LockedModbusClient.h b/firmware/components/sensors/include/sensors/LockedModbusClient.h new file mode 100644 index 0000000..3c02673 --- /dev/null +++ b/firmware/components/sensors/include/sensors/LockedModbusClient.h @@ -0,0 +1,104 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file LockedModbusClient.h + * @brief Mutex-serializing IModbusClient decorator (header-only). + * + * WHY THIS EXISTS: the RS485 Modbus bus is driven from more than one + * FreeRTOS task. PR-11's watering task issues periodic soil reads + * (LockedSoilSensor -> ModbusSoilSensor -> this client) while the diag + * console REPL task runs the rs485test command, which probes the same + * client directly. LockedSoilSensor's mutex only serializes ISoilSensor + * calls; it does NOT cover a direct modbus-client call, so rs485test could + * overlap a soil read's bus transaction and corrupt the shared UART. + * + * This decorator wraps an IModbusClient and takes a mutex around every + * interface call, so BOTH the soil sensor's transactions AND rs485test + * serialize on the SAME mutex. Each readHoldingRegisters/writeSingleRegister + * is a complete request/response transaction; serializing per-transaction is + * sufficient (interleaving complete transactions on the bus is safe, only + * overlapping ones corrupt it). + * + * LOCK ORDERING: the soil path acquires LockedSoilSensor's mutex first, then + * (inside ModbusSoilSensor) this client's mutex; rs485test acquires ONLY + * this client's mutex. This decorator never calls back into the soil sensor, + * so the order is always (SoilSensor mutex -> Modbus mutex) or (Modbus mutex + * alone) — no inversion, no deadlock. + * + * USAGE RULE: once a client is wrapped, the underlying client must ONLY be + * accessed through the wrapper — every call site (boot init, soil sensor, + * console rs485test) goes through the LockedModbusClient, never through the + * wrapped object directly. + * + * Pure C++ ( is available via pthread on ESP-IDF and on the linux + * preview target), so the decorator is host-includable. + */ + +#ifndef WATERINGSYSTEM_SENSORS_LOCKEDMODBUSCLIENT_H +#define WATERINGSYSTEM_SENSORS_LOCKEDMODBUSCLIENT_H + +#include +#include + +#include "interfaces/IModbusClient.h" + +/** + * @brief IModbusClient decorator that serializes every call with a mutex. + * + * Composition, not inheritance from a concrete client: the base client stays + * pure (no locking) and the host tests are unchanged. The wrapped client + * must outlive this object. + */ +class LockedModbusClient : public IModbusClient { +public: + /// Wrap @p client; the wrapped client must outlive this object. + explicit LockedModbusClient(IModbusClient& client) : client_(client) {} + + LockedModbusClient(const LockedModbusClient&) = delete; + LockedModbusClient& operator=(const LockedModbusClient&) = delete; + + bool initialize() override + { + std::lock_guard lock(mutex_); + return client_.initialize(); + } + + bool readHoldingRegisters(uint8_t deviceAddress, uint16_t startRegister, + uint16_t count, uint16_t* buffer) override + { + std::lock_guard lock(mutex_); + return client_.readHoldingRegisters(deviceAddress, startRegister, count, + buffer); + } + + bool writeSingleRegister(uint8_t deviceAddress, uint16_t registerAddress, + uint16_t value) override + { + std::lock_guard lock(mutex_); + return client_.writeSingleRegister(deviceAddress, registerAddress, value); + } + + int getLastError() override + { + std::lock_guard lock(mutex_); + return client_.getLastError(); + } + + void setTimeout(uint32_t timeoutMs) override + { + std::lock_guard lock(mutex_); + client_.setTimeout(timeoutMs); + } + + void getStatistics(uint32_t* successCount, uint32_t* errorCount) override + { + std::lock_guard lock(mutex_); + client_.getStatistics(successCount, errorCount); + } + +private: + IModbusClient& client_; + mutable std::mutex mutex_; +}; + +#endif /* WATERINGSYSTEM_SENSORS_LOCKEDMODBUSCLIENT_H */ diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index 696ec73..06a508c 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -44,6 +44,7 @@ #include "sensors/GpioLevelSensor.h" #include "sensors/LockedEnvironmentalSensor.h" #include "sensors/LockedLevelSensor.h" +#include "sensors/LockedModbusClient.h" #include "sensors/LockedSoilSensor.h" #include "sensors/ModbusSoilSensor.h" #if BOARD_HAS_INA226 @@ -513,7 +514,8 @@ extern "C" void app_main(void) // sensor access goes through the wrapper. No periodic read task in // feature 004 (arrives with PR-11) — reads happen on console command // only. - static EspModbusClient modbus_client; + static EspModbusClient modbus_client_raw; + static LockedModbusClient modbus_client(modbus_client_raw); static ModbusSoilSensor soil_sensor_raw(modbus_client); static LockedSoilSensor soil_sensor(soil_sensor_raw); diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index 0ebd633..6f4d08d 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -580,10 +580,12 @@ int soil_cmd(int argc, char **argv) /// `rs485test`: one raw 1-register probe (slave 0x01, register 0x0000 — /// the parity availability probe) + cumulative transaction statistics. /// -/// TODO(PR-11): this drives the raw, unsynchronized EspModbusClient BELOW -/// LockedSoilSensor's mutex; once PR-11 adds the main-loop reader this -/// becomes a cross-task race — route it through a locked client wrapper -/// (or through the sensor) then. +/// Cross-task race RESOLVED (PR-11): this drives the injected IModbusClient, +/// which in app_main is a LockedModbusClient. PR-11's watering task issues +/// periodic soil reads over the SAME wrapped client, so both this probe and +/// the soil reader serialize on the one LockedModbusClient mutex — concurrent +/// bus transactions can no longer overlap. The fix is purely the injected +/// instance; the code below is unchanged. int rs485test_cmd(int argc, char **argv) { (void)argv; From 3c2f722786cea3763cb50cef103f070393000f91 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 20:39:11 +0200 Subject: [PATCH 8/9] =?UTF-8?q?docs(control):=20PR-11=20polish=20=E2=80=94?= =?UTF-8?q?=20CLAUDE.md=20section,=20HIL=20+=20branch-coverage=20checklist?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - firmware/CLAUDE.md: new "Watering controller (feature 011)" section (control component, pure/host-tested logic, soak-from-burst-end + enforcement, fail-safe precedence, manual=flag+300 s cap, reservoir truth table + cooldown divergence, data-log metric set incl. why soil_humidity is dropped, snapshot helpers, controller-as-reader periodic soil, watering task + watchdog, LockedModbusClient race fix). - specs/011/checklists/hil.md: rev1 bench-rig smoke (live soil, automatic + soak, fail-safe precedence, manual override, reservoir fill/stop/abort/cooldown, logging, isolation, rs485 race) with the deferral path. - specs/011/checklists/branch-coverage.md: every decision/safety branch mapped to its covering host test; records the green run (289/0, both boards, sizes). Size (T019): rev1 1037.3 KiB, rev2 1039.4 KiB in the 1536 KiB OTA slot (~32 % margin). No new managed deps (T018). Co-Authored-By: Claude Opus 4.8 --- firmware/CLAUDE.md | 74 ++++++++++++ .../checklists/branch-coverage.md | 85 +++++++++++++ .../checklists/hil.md | 113 ++++++++++++++++++ 3 files changed, 272 insertions(+) create mode 100644 specs/011-watering-controller-host-tests/checklists/branch-coverage.md create mode 100644 specs/011-watering-controller-host-tests/checklists/hil.md diff --git a/firmware/CLAUDE.md b/firmware/CLAUDE.md index 630b00d..7f9e104 100644 --- a/firmware/CLAUDE.md +++ b/firmware/CLAUDE.md @@ -494,6 +494,80 @@ The server is constructed in `app_main` and `start()`ed on the first interface); `start()`/`stop()` are idempotent and non-fatal. HIL checklist: `specs/009-http-server-api-v1/checklists/hil.md`. +## Watering controller (feature 011) + +Feature 011 (PR-11) adds the `control` component — the automatic watering +logic, 100 % host-tested pure C++ over the interfaces (the master-PRD +criterion). No IDF/`esp_*` includes; all time is injected (`ITimeProvider` +monotonic for gates/staleness, `IWallClock` epoch for log timestamps) and every +threshold/duration is read from `IConfigStore` each tick (runtime-tunable). Two +classes, both driven over mocks + `FakeTimeProvider`/`FakeWallClock` in +`test_apps/host/main/test_watering_controller.cpp` + `test_reservoir.cpp`: + +- **`WateringController`** — pulsed automatic watering. `tick()` order is + safety-first: `plant.update()` (self-stop + 300 s cap) → burst-end detection → + single soil read → periodic data-log → manual-override branch → enabled gate → + **fail-safe** → gate-on-read → watering decision. Start at moisture ≤ low when + the soak pause has elapsed; stop at ≥ high. **Fail-safe (soil unavailable / + stale > 30 000 ms / moisture out of 0–100 %) is checked BEFORE the soak gate + and is never delayed by it** — a pending soak pause never postpones a safety + stop. Automatic decisions gate on a successful in-range read, never on + placeholder/last-good values. +- **Soak-pause divergence (parity):** the min-watering-interval soak is measured + from the burst **END** (true absorption), and it is ENFORCED — no new burst + starts until it elapses even while the soil still reads dry. The burst-end + edge is detected whether the pump self-stopped (duration/cap) or was stopped + at the high threshold. +- **Manual override:** `startManual(int)` clamps to 1..300 s, runs the plant + pump and sets a flag that exempts the run from the automatic fail-safe; + `stop()` clears it; a pump self-stop clears it on the next tick. Manual is + `wateringEnabled == false` at the API layer — the controller never calls an + `isManualMode()` on the pump. +- **`ReservoirController`** (rev1 only, but host-tested regardless of board) — + the level truth table over two `ILevelSensor` snapshots (invalid or + implausible dry+wet rows → no action; dry+dry → fill; high-wet → stop), + running-safety stop-on-high, the pump's 300 s cap for the max-fill abort, and + a **post-abort cooldown** (`kReservoirRefillCooldownMs`, ~60 s): after a + `MaxRuntimeForced` abort no new automatic fill starts until the cooldown + elapses even if still low-dry — guards a stuck high sensor / empty source. A + normal high-wet stop does not arm the cooldown; manual fill bypasses it. This + cooldown is a deliberate divergence from parity (`docs/parity-checklist.md`). +- **Logging (FR-014):** every `dataLogInterval` the controller logs env + (`env_temperature/humidity/pressure`) + soil (`soil_moisture/temperature/ph/ + ec`, plus NPK only when ≥ 0), stamped with `IWallClock::nowEpoch()` and gated + on `isTimeSet()` (never a bogus 1970). `soil_humidity` is intentionally NOT + logged — `ISoilSensor::getHumidity()` is identical to `getMoisture()` (parity, + register 0x0000), so the max distinct-metric set is exactly 10 = the + `IDataStorage` `kMaxMetrics` cap, no data loss. Fail-safe events go through + `EventLogger::logFailsafe`; pump start/stop transitions stay owned by + `SystemObserver` (no double-log). + +**Snapshot helpers:** `LockedSoilSensor::snapshot()` / `LockedEnvironmentalSensor` +/ `LockedLevelSensor` return `{Soil,Env,Level}Snapshot` — all values + validity + +error copied out under ONE lock, closing the read()-then-getter cross-call gap +without a fresh (blocking) bus read (QUIRK 5). + +**On-target wiring:** the pure logic runs on `main/watering_task.cpp`, a +watchdog-subscribed FreeRTOS task ticking at `config.getSensorReadIntervalMs()` +(floored at 1 s). The controller is the **periodic soil reader** (controller-as- +reader): its per-tick `read()` is the blocking Modbus transaction that refreshes +the `LockedSoilSensor` cache the API `/sensors` endpoint serves — so there is no +separate soil-reader task, and the blocking bus I/O is isolated off the 10 Hz +safety loop (which still owns precise pump-timing enforcement + `observer.poll()`). +The API mode flag reaches the controller purely through `config` +(`getWateringEnabled()`, read each tick) — no direct ApiServer↔controller call +(FR-017 isolation). Reservoir (rev1): `tick(true, getWateringEnabled())` — the +pump always exists so `enabled` is always true; auto level control is gated by +the same mode flag (manual mode suspends auto-fill; manual API fills still work). +There is deliberately no dedicated auto-level config flag. + +**RS485 race fix (T016):** `LockedModbusClient` (header-only `IModbusClient` +decorator) wraps the `EspModbusClient` in `app_main`; the `ModbusSoilSensor` and +the console `rs485test` command now share one mutex, so the periodic reader and +the diagnostic probe can no longer overlap bus transactions. Lock order is always +(soil mutex → modbus mutex) or (modbus mutex alone) — no deadlock. HIL checklist: +`specs/011-watering-controller-host-tests/checklists/hil.md`. + ## Partition layout (4MB flash) nvs (0x9000, 16K) | otadata (0xd000, 8K) | phy_init (0xf000, 4K) | diff --git a/specs/011-watering-controller-host-tests/checklists/branch-coverage.md b/specs/011-watering-controller-host-tests/checklists/branch-coverage.md new file mode 100644 index 0000000..4e07784 --- /dev/null +++ b/specs/011-watering-controller-host-tests/checklists/branch-coverage.md @@ -0,0 +1,85 @@ +# Branch-coverage Checklist: Watering Controller (011) + +**Purpose**: prove every decision + safety branch of the pure controllers is exercised by a host test (the +merge-gating deliverable — 100 % host-tested watering logic). Each row maps a branch from `data-model.md` / +the contracts to the covering test. All boxes are checked only because the full host suite is green. + +**Verification run (this branch, `011-watering-controller-host-tests`):** + +- [x] Full host suite green: **289 Tests, 0 Failures, 0 Ignored** (`pump_host_tests.elf`, exit 0) + — of which 23 in `test_watering_controller.cpp` + 14 in `test_reservoir.cpp` are new for feature 011. +- [x] rev1 build green (`sdkconfig.board.rev1_devkit`) — `REV1_RC=0`, app 1037.3 KiB / 1536 KiB slot (32.5 % + margin). +- [x] rev2 build green (`sdkconfig.board.rev2`) — `REV2_RC=0`, app 1039.4 KiB / 1536 KiB slot (32.3 % margin). +- [x] No new managed dependencies; `dependencies.lock` + esp-modbus (`==2.1.2`) unchanged (T018). + +## WateringController — decision branches + +- [x] Start burst: enabled + not running + moisture ≤ low + soak elapsed → `runFor(burst)` + — `test_starts_burst_when_dry_and_enabled` +- [x] No automatic action when watering disabled (manual/suspended mode) — `test_no_start_when_disabled` +- [x] Stop at target: running + moisture ≥ high → `stop()`, arm soak from burst end + — `test_stops_at_high_threshold` +- [x] Soak gate (FR-003): after a burst ends, no new burst until the pause elapses even while dry; then + restart — `test_soak_pause_blocks_then_allows_restart` +- [x] Config thresholds/durations re-read each tick (runtime-tunable) — `test_config_change_picked_up_next_tick` +- [x] Low/high thresholds are inclusive boundaries — `test_boundaries_low_and_high_inclusive` + +## WateringController — read gating + +- [x] No action before the first successful read (gate on read, not placeholder) — FR-004 + — `test_no_action_before_first_successful_read` +- [x] Transient failed read while last-good is fresh → wait, no fail-safe — `test_gate_on_transient_failed_read` + +## WateringController — fail-safe (safety branches, FR-005/006) + +- [x] Soil unavailable while running → immediate stop + `logFailsafe` — `test_failsafe_unavailable_stops_running_pump` +- [x] Soil stale (> 30 000 ms / never) while running → stop + log — `test_failsafe_stale_stops_running_pump` +- [x] Moisture out of 0..100 → stop + log — `test_failsafe_invalid_moisture_stops_running_pump` +- [x] **Fail-safe is NOT delayed by the soak gate** (checked before it) — `test_failsafe_not_delayed_by_soak` +- [x] Fail-safe during a pending soak pause takes no watering action — `test_failsafe_during_pending_soak_takes_no_action` +- [x] Graceful degradation: constructs + never waters when a sensor failed init — `test_graceful_degradation_never_waters` + +## WateringController — manual override (FR-007..010) + +- [x] Manual run bypasses fail-safe (runs despite sensor failure) — `test_manual_run_bypasses_sensor_failure` +- [x] Manual duration capped at 300 s — `test_manual_run_capped_at_300s` +- [x] Manual duration lower-clamped to 1 s — `test_manual_run_lower_clamp` +- [x] Auto-started run is NOT flagged manual (fail-safe still applies) — `test_auto_run_is_not_flagged_manual` +- [x] `stop()` clears the override (automatic resumes) — `test_stop_clears_manual_override` + +## WateringController — data logging (FR-014) + +- [x] Logs at the data-log cadence (interval gate) — `test_data_log_cadence` +- [x] Epoch timestamp + NPK-only-when-≥0 filter — `test_data_log_epoch_and_npk_filter` +- [x] Gated on `isTimeSet()` (no bogus 1970) — `test_data_log_gated_on_time_set` +- [x] Telemetry logs even on the fail-safe path (env still logged, soil skipped) — `test_data_log_runs_on_failsafe_path` + +## ReservoirController — truth table (FR-012/013) + +- [x] Low invalid → no action (invalid ≠ dry) — `test_invalid_low_no_action` +- [x] High invalid → no action — `test_invalid_high_no_action` +- [x] Both invalid → no action — `test_both_invalid_no_action` +- [x] wet + wet (full) → ensure stopped — `test_full_wet_wet_ensures_stopped` +- [x] wet + dry (sufficient) → no action — `test_sufficient_wet_dry_no_action` +- [x] dry + dry → start fill — `test_low_dry_dry_starts_fill` +- [x] dry + wet (implausible) → no action — `test_implausible_low_dry_high_wet_no_action` + +## ReservoirController — running safety + cooldown (FR-012a) + +- [x] Stop on high-wet while running — `test_stop_on_high_wet_while_running` +- [x] Max-fill abort at the 300 s cap (MaxRuntimeForced) — `test_max_fill_abort_at_cap` +- [x] Post-abort cooldown blocks a new auto fill, then allows it — `test_cooldown_blocks_then_allows_auto_refill` +- [x] A normal high-wet stop does NOT arm the cooldown — `test_normal_high_wet_stop_does_not_arm_cooldown` +- [x] Manual fill bypasses the cooldown — `test_manual_fill_bypasses_cooldown` +- [x] Manual fill refused when already full (high wet) — `test_manual_fill_refused_when_full` +- [x] Feature disabled forces the pump off and skips all logic — `test_feature_disabled_forces_off_and_skips_logic` + +## Not host-covered (target-only / HIL) + +These branches are on-target integration, verified by the board builds + the HIL checklist, not the host suite: + +- [ ] Periodic soil reader refreshes the `/sensors` cache (controller-as-reader) — HIL §A +- [ ] Watering task watchdog subscribe/feed — HIL (implicit: no TASK_WDT reboot during a cycle) +- [ ] `LockedModbusClient` serializes rs485test with the reader (no bus overlap) — HIL §H +- [ ] Isolation: watering unaffected by WiFi/HTTP load — HIL §G diff --git a/specs/011-watering-controller-host-tests/checklists/hil.md b/specs/011-watering-controller-host-tests/checklists/hil.md new file mode 100644 index 0000000..fba9463 --- /dev/null +++ b/specs/011-watering-controller-host-tests/checklists/hil.md @@ -0,0 +1,113 @@ +# HIL Checklist: Watering Controller (011) — rev1 bench rig + +**Purpose**: hardware-in-the-loop smoke of PR-11 at Checkpoint 3 (Paul, bench rig). The merge-gating +deliverable is the 100 % host-tested watering logic (289/0 in CI); this HIL is a confidence smoke that the +wired-up controller behaves on real hardware. +**Rig**: ESP32 devkit (rev1) provisioned with valid home-WiFi credentials (station mode, so `/api/v1/` binds +and the flag can be driven remotely); BME280 on I2C, RS485 soil sensor, plant + reservoir pumps, and the two +XKC-Y26 level sensors as wired for the previous features. A laptop/phone on the same LAN with `curl`, plus a +serial console (`ws>`, 115200) for the RS485 pull and event-log checks. +**Build**: rev1 target (`sdkconfig.board.rev1_devkit`), flash per `firmware/CLAUDE.md`. Confirm the serial +log shows `watering task started (... ms cadence)` and `sensor task started` after boot. +**Reference**: acceptance criteria `docs/prd/PR-11-*.md`; spec FR-001..FR-017 (esp. FR-003 soak, FR-005/006 +fail-safe, FR-012a cooldown, FR-014 logging, FR-017 isolation); `quickstart.md` §HIL; `data-model.md` +decision tables; parity `docs/parity-checklist.md`. +**Setup**: note the device IP (console `wifi` / router / `GET /api/v1/status`). Export it, e.g. +`export DEV=192.168.1.50`, and prefix curls with `http://$DEV/api/v1/...`. Set short thresholds/intervals via +`POST /api/v1/config` (or the `config` console command) so a full cycle is observable in minutes: +a low soak (`minWateringIntervalS`), a short burst (`wateringDurationS`), a low sensor interval +(`sensorReadIntervalMs`), and low/high moisture thresholds bracketing the current soil reading. + +**Note on cadence**: the controller ticks at `sensorReadIntervalMs` (floored at 1 s), so a decision may lag a +config/soil change by up to one interval — allow for that when timing the observations below. + +## A. Live soil reader (US3, quickstart §HIL #5) + +- [ ] A1. `curl http://$DEV/api/v1/sensors` now shows `soil.valid:true` with fresh, plausible values + (moisture/temp/pH/EC and NPK when the sensor reports them) — the periodic reader is running (this is + the PR-09 limitation being lifted) +- [ ] A2. The soil values refresh over time (poll twice ~2 intervals apart while touching the probe / moving + it between dry and wet media — the reading tracks) +- [ ] A3. `soil.valid` returns to `false` (last-good placeholders) after the RS485 pull in D1, then recovers + to `true` once reconnected + +## B. Automatic pulsed watering + soak (US1, quickstart §HIL #1, FR-002/003) + +- [ ] B1. In automatic mode (`GET /api/v1/status` → `mode:automatic`; set via `POST /api/v1/config` + `{"wateringEnabled":true}`) with the soil below the low threshold (dry probe), the plant pump starts a + burst of ~`wateringDurationS` +- [ ] B2. After the burst ends the pump does NOT immediately restart even while the soil still reads dry — + it waits out the soak pause (`minWateringIntervalS`) measured from the burst END, then re-waters +- [ ] B3. When the soil rises to ≥ the high threshold (wet the probe), a running burst stops and no new burst + starts +- [ ] B4. Change `wateringDurationS` / thresholds via `POST /api/v1/config` mid-run → the next tick/burst + picks up the new values (runtime-tunable, no reflash) + +## C. Fail-safe precedence (US1, quickstart §HIL #2, FR-005/006) + +- [ ] C1. With the plant pump mid-burst in automatic mode, pull the RS485 soil cable → the pump stops within + the staleness window (~30 s), and `/api/v1/sensors` shows `soil.valid:false` +- [ ] C2. A `reset=`/`failsafe` entry appears in `GET /api/v1/events` (or console `storage events`) with a + detail like `soil-stale` / `soil-unavailable` +- [ ] C3. **Soak does not delay a safety stop**: trigger the pull DURING an active soak pause (pump just + stopped, waiting to re-water) — confirm the fail-safe still fires (no watering resumes) rather than + waiting for the soak to elapse +- [ ] C4. Reconnect the cable → after a fresh valid read the controller resumes normal automatic behavior + (no reboot occurred — check uptime in `/status`) + +## D. Manual override (US3, quickstart §HIL #3, FR-007..010) + +- [ ] D1. With the RS485 cable pulled (soil failing), a manual run still executes: + `POST /api/v1/pumps/plant {"action":"run","durationS":15}` → the plant pump runs despite the sensor + failure (manual bypasses fail-safe) +- [ ] D2. A manual `durationS` above 300 is capped at 300 s; the pump never runs past the hard cap +- [ ] D3. `POST {"action":"stop"}` stops the manual run and clears the override; switching back to automatic + (`wateringEnabled:true`) with a valid dry sensor then resumes automatic bursts +- [ ] D4. In manual mode (`wateringEnabled:false`) the controller takes NO automatic action (no bursts start + on their own) + +## E. Reservoir auto-fill (US2, quickstart §HIL #4, FR-012/012a) + +- [ ] E1. With both level marks dry (low-dry + high-dry) in automatic mode, the reservoir fill pump starts +- [ ] E2. When the high mark goes wet, the fill pump stops immediately +- [ ] E3. **Max-fill abort**: with the high mark forced to stay dry (disconnected / empty source), the fill + pump aborts at the 300 s cap rather than running forever +- [ ] E4. **Post-abort cooldown**: after the E3 abort, no new automatic fill starts for ~`kReservoirRefill + CooldownMs` (~60 s) even though low still reads dry; after the cooldown a fill re-attempts. A normal + high-wet stop (E2) does NOT impose the cooldown; a manual fill + (`POST /api/v1/pumps/reservoir {"action":"run",...}`) starts immediately regardless of cooldown +- [ ] E5. A stuck/settling level mark (`not_yet_valid`) never triggers a fill (invalid ≠ dry) + +## F. Data logging (US3, FR-014) + +- [ ] F1. After the first SNTP sync (`/status` → time `synced:true`), `GET /api/v1/history?metric=env_ + temperature&range=1h` returns a growing series at roughly `dataLogIntervalMs` cadence +- [ ] F2. Soil metrics log too (`metric=soil_moisture`); `soil_humidity` is intentionally absent (identical + to `soil_moisture`); NPK metrics appear only when the sensor reports them (≥ 0) +- [ ] F3. Before the first sync (power-cycle, no network) no bogus 1970-epoch rows are written (query shows + nothing logged until the clock is set) + +## G. Isolation (FR-017, quickstart §HIL #6) + +- [ ] G1. During an active automatic watering cycle, drop WiFi (power off the AP / `wifi-clear` is too + destructive — just move out of range or disable the AP) → the plant/reservoir behavior is UNAFFECTED + (bursts, stops, soak continue on schedule) +- [ ] G2. Flood the API with requests (a `curl` loop against `/api/v1/sensors`) while a burst runs → watering + timing is unaffected; the 10 Hz pump-timing stays crisp (pump stops at the burst duration, not late) + +## H. RS485 race fix (T016) + +- [ ] H1. While the periodic reader is active (watering task running), issue `ws> rs485test` repeatedly on + the console → it returns clean probe results + statistics with no bus corruption, garbled frames, or + lock-ups, and `/api/v1/sensors` soil readings stay coherent throughout (shared-mutex serialization) + +## Definition of done + +- [ ] HIL smoke executed on the rev1 rig at Checkpoint 3, OR deferred with rationale if no rig time is + available (record in the deferred-HIL register alongside the earlier features' deferrals). +- [ ] Any anomaly filed and triaged; a fail-safe or isolation miss (Section C or G) blocks merge, the rest + are logged for follow-up. + +**Deferral note**: like PR-04/07/08/09, if the rig is unavailable this checklist is deferred (not skipped). +The merge is gated by the host suite (289/0) + both board builds; the HIL is the on-hardware confidence pass +Paul runs when the bench is free. From 562385eec7f036106c72272dcb7e95b46e078b56 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 21:14:36 +0200 Subject: [PATCH 9/9] =?UTF-8?q?fix(control):=20PR-11=20CP3=20review=20fixe?= =?UTF-8?q?s=20=E2=80=94=20watchdog,=20snapshot=20path,=20test=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Checkpoint-3 review findings. M1 (watchdog boot-loop): watering_task fed the WDT once per loop while sleeping the full, operator-configurable getSensorReadIntervalMs() — set above the ~20 s task-WDT timeout it would panic-reboot and boot-loop (NVS-persisted). Now uses chunked feed-and-sleep (1 s slices, feed each) and ticks once per full period, so a large configured cadence can no longer starve the watchdog. M2 (redundant blocking probe + dead snapshot helpers): the controller did read() then a second blocking isAvailable() RS485 probe per tick — a transient glitch on that probe after a good read could spuriously fail-safe-stop the pump. Promoted snapshot() to the ISoilSensor interface (ModbusSoilSensor tracks read history; LockedSoilSensor locks + delegates; MockSoilSensor implements it). tick() now reads once then consumes ONE coherent snapshot() — no second probe, no torn tuple; availability = ever-read-ok (never-read -> soil-unavailable, worked-then- lost -> soil-stale). maybeLogData logs from the same snapshot. The snapshot helpers are now live, not dead code, and the header/manual-flag comments match the code. Hardening: watering_task_start now logs a durable EventLogger::logFailsafe( "watering-task-start-failed") on xTaskCreate failure (visible in /api/v1/events). Test gaps (the deliverable is 100 % branch coverage): +T-A high-threshold soak origin, +T-B reservoir auto-level-off suppresses fill (manual still works), +T-C drop-sensitive 10-metric cap assertion (rejectedWrites==0), +NPK phosphorus/potassium negative, +env-read-failure branch. branch-coverage.md updated. Docs: retagged stale TODO(PR-11) markers to PR-14 (diag_console env/level/power, LockedPowerSensor); CLAUDE.md snapshot payload note qualified (error is soil-only). Host 293/0; rev1 + rev2 build green. Co-Authored-By: Claude Opus 4.8 --- firmware/CLAUDE.md | 6 +- .../include/control/WateringController.h | 16 +- .../control/src/WateringController.cpp | 45 +++-- .../include/interfaces/ISoilSensor.h | 41 ++++ .../include/sensors/LockedPowerSensor.h | 6 +- .../include/sensors/LockedSoilSensor.h | 65 ++---- .../include/sensors/ModbusSoilSensor.h | 7 + .../include/sensors/testing/MockSoilSensor.h | 35 +++- .../sensors/src/ModbusSoilSensor.cpp | 24 +++ firmware/main/app_main.cpp | 5 +- firmware/main/diag_console.cpp | 21 +- firmware/main/watering_task.cpp | 55 +++-- firmware/main/watering_task.h | 12 +- .../test_apps/host/main/test_reservoir.cpp | 22 ++ .../host/main/test_watering_controller.cpp | 189 ++++++++++++++++-- .../checklists/branch-coverage.md | 21 +- 16 files changed, 441 insertions(+), 129 deletions(-) diff --git a/firmware/CLAUDE.md b/firmware/CLAUDE.md index 7f9e104..aaf8b03 100644 --- a/firmware/CLAUDE.md +++ b/firmware/CLAUDE.md @@ -543,9 +543,9 @@ classes, both driven over mocks + `FakeTimeProvider`/`FakeWallClock` in `SystemObserver` (no double-log). **Snapshot helpers:** `LockedSoilSensor::snapshot()` / `LockedEnvironmentalSensor` -/ `LockedLevelSensor` return `{Soil,Env,Level}Snapshot` — all values + validity + -error copied out under ONE lock, closing the read()-then-getter cross-call gap -without a fresh (blocking) bus read (QUIRK 5). +/ `LockedLevelSensor` return `{Soil,Env,Level}Snapshot` — all values + validity +(plus the error code for `SoilSnapshot`) copied out under ONE lock, closing the +read()-then-getter cross-call gap without a fresh (blocking) bus read (QUIRK 5). **On-target wiring:** the pure logic runs on `main/watering_task.cpp`, a watchdog-subscribed FreeRTOS task ticking at `config.getSensorReadIntervalMs()` diff --git a/firmware/components/control/include/control/WateringController.h b/firmware/components/control/include/control/WateringController.h index a76a55e..5cadacd 100644 --- a/firmware/components/control/include/control/WateringController.h +++ b/firmware/components/control/include/control/WateringController.h @@ -21,9 +21,10 @@ * LockedSoilSensor snapshot; the pure logic drives the injected sensors directly. * * Concurrency: the controller is unsynchronized and single-writer (its own - * watchdog-registered task). On-target US3 wires the sensor through a - * LockedSoilSensor snapshot so read()+availability come from one locked - * acquisition; the pure US1 logic drives the injected ISoilSensor directly. + * watchdog-registered task). tick() drives one blocking read() (controller-as- + * reader) and then consumes one non-blocking snapshot() for the availability + + * values it decides on — no second bus probe; on-target the sensor is a + * LockedSoilSensor so that snapshot is copied under a single lock. */ #ifndef WATERINGSYSTEM_CONTROL_WATERINGCONTROLLER_H @@ -122,11 +123,13 @@ class WateringController { * the configured data-log interval (the first eligible log after time is set * fires immediately). Env readings are logged on a successful, available * read; soil readings are logged only when @p soilValid (the result of the - * single soil read() this tick), with NPK included only when >= 0. All + * single soil read() this tick), with NPK included only when >= 0. Soil + * metrics come from @p soil — the same coherent snapshot tick() decided on, + * so the logged values match the decision values with no second read. All * writes carry IWallClock::nowEpoch(); storage self-bounds, so the store * result is ignored. */ - void maybeLogData(int64_t now, bool soilValid); + void maybeLogData(int64_t now, bool soilValid, const SoilSnapshot& soil); ISoilSensor& soil_; IEnvironmentalSensor& env_; ///< periodic data-logging source (US3) @@ -152,7 +155,8 @@ class WateringController { /// True while an operator manual run is active. A manual run is exempt from /// the automatic fail-safe and the soak gate; it clears on stop() or when - /// the pump self-stops at the 300 s cap. + /// the pump self-stops (its clamped run duration elapses, or the 300 s hard + /// cap). bool manualRunActive_ = false; /// Monotonic time of the last data-log batch (0 = none yet — the first diff --git a/firmware/components/control/src/WateringController.cpp b/firmware/components/control/src/WateringController.cpp index e129428..5650581 100644 --- a/firmware/components/control/src/WateringController.cpp +++ b/firmware/components/control/src/WateringController.cpp @@ -28,14 +28,20 @@ void WateringController::tick() burstActive_ = false; } - // ---- Read the sensor ONCE per tick (US1 drives ISoilSensor directly; US3 - // wires a LockedSoilSensor snapshot so read()+availability come from one - // locked acquisition). We react only to the read RESULT and the - // availability signal, never to specific numeric error codes. The soil - // getters below serve the values from THIS read — no second I/O. --------- - const bool readOk = soil_.read(); - const bool available = soil_.isAvailable(); - const float moisture = soil_.getMoisture(); + // ---- Read the sensor ONCE per tick (controller-as-reader: read() drives + // the bus and refreshes the cache), then take ONE coherent, NON-BLOCKING + // snapshot() for the availability + values we decide on — NO second bus + // probe. We react only to the read RESULT and the availability signal, + // never to specific numeric error codes. `available` now means "at least + // one successful read on record": a sensor that has NEVER read OK is + // !available -> "soil-unavailable"; a sensor that worked then stopped + // responding keeps available true, the read fails, and it fails safe as + // "soil-stale". Both stop the pump — only the reason string differs. ------ + soil_.read(); // drives the bus, refreshes cache + const SoilSnapshot soil = soil_.snapshot(); // one coherent, non-blocking tuple + const bool readOk = soil.readOk; + const bool available = soil.available; // ever-read-ok; no second probe + const float moisture = soil.moisture; const bool inRange = moisture >= kMoistureMinPct && moisture <= kMoistureMaxPct; const bool invalid = readOk && !inRange; @@ -53,7 +59,7 @@ void WateringController::tick() // Telemetry must be recorded even when automatic watering is disabled, a // manual override is active, or a fail-safe is about to fire (FR-014). Soil // is logged only when this tick's read was successful and in range. - maybeLogData(now, /*soilValid=*/(readOk && inRange)); + maybeLogData(now, /*soilValid=*/(readOk && inRange), soil); // ---- MANUAL OVERRIDE (bypasses fail-safe + soak/decision logic) -------- // A manual run is an explicit operator override (FR-007/008): it is exempt @@ -153,7 +159,8 @@ void WateringController::stop() manualRunActive_ = false; } -void WateringController::maybeLogData(int64_t now, bool soilValid) +void WateringController::maybeLogData(int64_t now, bool soilValid, + const SoilSnapshot& soil) { // No plausible wall-clock timestamp yet — never log a bogus 1970 epoch. if (!wallClock_.isTimeSet()) { @@ -180,26 +187,26 @@ void WateringController::maybeLogData(int64_t now, bool soilValid) storage_.storeSensorReading("env_pressure", epoch, env_.getPressure()); } - // Soil telemetry (uses the values from this tick's single read()). + // Soil telemetry (uses the values from this tick's single snapshot(), so + // the logged values match the values tick() decided on — one read/tick). if (soilValid) { - storage_.storeSensorReading("soil_moisture", epoch, - soil_.getMoisture()); + storage_.storeSensorReading("soil_moisture", epoch, soil.moisture); storage_.storeSensorReading("soil_temperature", epoch, - soil_.getTemperature()); + soil.temperature); // NOTE: soil humidity is deliberately NOT logged. ISoilSensor's // getHumidity() is documented as identical to getMoisture() (a single // moisture/humidity quantity in register 0x0000; the legacy driver // exposed it under both names), so logging it would be a pure duplicate // of soil_moisture. Dropping it keeps the max distinct metric set at // exactly kMaxMetrics (10): 3 env + 4 soil-base + 3 NPK. - storage_.storeSensorReading("soil_ph", epoch, soil_.getPH()); - storage_.storeSensorReading("soil_ec", epoch, soil_.getEC()); + storage_.storeSensorReading("soil_ph", epoch, soil.ph); + storage_.storeSensorReading("soil_ec", epoch, soil.ec); // NPK is only meaningful when >= 0 (the sensor reports -1 when a // channel is unsupported/unavailable); skip a negative channel. - const float n = soil_.getNitrogen(); - const float p = soil_.getPhosphorus(); - const float k = soil_.getPotassium(); + const float n = soil.nitrogen; + const float p = soil.phosphorus; + const float k = soil.potassium; if (n >= 0) { storage_.storeSensorReading("soil_nitrogen", epoch, n); } diff --git a/firmware/components/interfaces/include/interfaces/ISoilSensor.h b/firmware/components/interfaces/include/interfaces/ISoilSensor.h index 3c0f1aa..69d50c3 100644 --- a/firmware/components/interfaces/include/interfaces/ISoilSensor.h +++ b/firmware/components/interfaces/include/interfaces/ISoilSensor.h @@ -27,6 +27,31 @@ #ifndef WATERINGSYSTEM_INTERFACES_ISOILSENSOR_H #define WATERINGSYSTEM_INTERFACES_ISOILSENSOR_H +/** + * @brief Consistent, non-blocking soil-reading snapshot (PR-11). + * + * All eight quantity values plus the validity/error flags, copied out + * atomically so they are mutually consistent (no torn read()-then-getter + * tuple). readOk reports whether the most recent read() succeeded; available + * reports whether at least one successful read is on record, so the last-good + * values are meaningful even when the latest read failed (stale-but-usable). + * lastError is the sensor's most recent error code. Default member + * initializers make a default-constructed snapshot safe (no read yet). + */ +struct SoilSnapshot { + bool readOk = false; + bool available = false; + int lastError = 0; + float moisture = 0.0f; + float temperature = 0.0f; + float humidity = 0.0f; + float ph = 0.0f; + float ec = 0.0f; + float nitrogen = 0.0f; + float phosphorus = 0.0f; + float potassium = 0.0f; +}; + /** * @brief Soil sensor: atomic multi-quantity reads with parity validation. */ @@ -57,6 +82,22 @@ class ISoilSensor { */ virtual bool read() = 0; + /** + * @brief Coherent, NON-BLOCKING snapshot of the last-good reading. + * + * Returns {readOk, available, lastError, values} copied out atomically + * (the Locked wrapper copies under a single lock, closing the + * read()-then-getter cross-call gap). readOk = the most recent read() + * succeeded; available = at least one successful read is on record (the + * ever-read-ok history), so the last-good values are meaningful even after + * a later read failure; lastError = the current error code. + * + * NON-BLOCKING by contract: it performs NO fresh read() and NO + * isAvailable() bus I/O — the caller drives the read() cadence and this + * only reads cached state (QUIRK 5). + */ + virtual SoilSnapshot snapshot() = 0; + /** * @brief Probe sensor presence with a REAL 1-register bus read (parity). * diff --git a/firmware/components/sensors/include/sensors/LockedPowerSensor.h b/firmware/components/sensors/include/sensors/LockedPowerSensor.h index 87e52d0..5afe767 100644 --- a/firmware/components/sensors/include/sensors/LockedPowerSensor.h +++ b/firmware/components/sensors/include/sensors/LockedPowerSensor.h @@ -31,9 +31,9 @@ * interleaving read() from another task in between — another task may * refresh (or invalidate) the values first. Such sequences need * higher-level coordination. - * TODO(PR-11): add a consistent-snapshot helper (one locked call returning - * all three values + validity) when a second periodic reader appears — - * same bookkeeping as the other Locked* wrappers' PR-11 notes. + * TODO(PR-14): add a consistent-snapshot helper (one locked call returning + * all three values + validity) when a second periodic reader appears — the + * sibling Soil/Env/Level snapshots landed in PR-11; power's is deferred. * * Pure C++ ( is available via pthread on ESP-IDF and on the linux * preview target), so the decorator is host-testable. diff --git a/firmware/components/sensors/include/sensors/LockedSoilSensor.h b/firmware/components/sensors/include/sensors/LockedSoilSensor.h index bc701f4..b551875 100644 --- a/firmware/components/sensors/include/sensors/LockedSoilSensor.h +++ b/firmware/components/sensors/include/sensors/LockedSoilSensor.h @@ -42,30 +42,8 @@ #include "interfaces/ISoilSensor.h" -/** - * @brief Consistent, non-blocking soil-reading snapshot (PR-11). - * - * All eight quantity values plus the validity/error flags, copied out under - * one lock so they are mutually consistent (no torn read()-then-getter - * tuple). readOk reports whether the most recent read() through the wrapper - * succeeded; available reports whether at least one successful read is on - * record, so the last-good values are meaningful even when the latest read - * failed (stale-but-usable). lastError is the wrapped sensor's most recent - * error code. - */ -struct SoilSnapshot { - bool readOk; - bool available; - int lastError; - float moisture; - float temperature; - float humidity; - float ph; - float ec; - float nitrogen; - float phosphorus; - float potassium; -}; +// SoilSnapshot is defined in interfaces/ISoilSensor.h (owned by the interface +// so the pure controller can consume snapshot() through ISoilSensor). /** * @brief ISoilSensor decorator that serializes every call with a mutex. @@ -91,12 +69,7 @@ class LockedSoilSensor : public ISoilSensor { bool read() override { std::lock_guard lock(mutex_); - const bool ok = sensor_.read(); - // Cache the outcome so snapshot() can report validity without a - // fresh (blocking) bus transaction. - lastReadOk_ = ok; - hasEverReadOk_ = hasEverReadOk_ || ok; - return ok; + return sensor_.read(); } bool isAvailable() override @@ -181,37 +154,25 @@ class LockedSoilSensor : public ISoilSensor { * @brief Copy the last-good reading + validity + error out under one * lock (PR-11), closing the read-then-getter cross-call gap. * - * NON-BLOCKING by contract: it performs NO fresh read() and NO - * isAvailable() probe — both are RS485/Modbus bus I/O and must never run - * in a status/API/controller path (QUIRK 5). The periodic soil reader + * Locks and delegates to the wrapped sensor's snapshot(): the base tracks + * the read history (readOk / ever-read-ok) and holds the last-good values, + * and the lock makes that member-read atomic vs a concurrent read() on + * another task. + * + * NON-BLOCKING by contract: the base's snapshot() performs NO fresh read() + * and NO isAvailable() probe — both are RS485/Modbus bus I/O and must never + * run in a status/API/controller path (QUIRK 5). The periodic soil reader * owns the read() cadence; this returns the current cached values. - * readOk = the most recent read() through this wrapper succeeded; - * available = at least one successful read is on record (last-good values - * are meaningful); lastError = the wrapped sensor's current error code. */ - SoilSnapshot snapshot() + SoilSnapshot snapshot() override { std::lock_guard lock(mutex_); - SoilSnapshot s; - s.readOk = lastReadOk_; - s.available = hasEverReadOk_; - s.lastError = sensor_.getLastError(); - s.moisture = sensor_.getMoisture(); - s.temperature = sensor_.getTemperature(); - s.humidity = sensor_.getHumidity(); - s.ph = sensor_.getPH(); - s.ec = sensor_.getEC(); - s.nitrogen = sensor_.getNitrogen(); - s.phosphorus = sensor_.getPhosphorus(); - s.potassium = sensor_.getPotassium(); - return s; + return sensor_.snapshot(); } private: ISoilSensor& sensor_; mutable std::mutex mutex_; - bool lastReadOk_ = false; ///< result of the most recent read() - bool hasEverReadOk_ = false; ///< any successful read() on record }; #endif /* WATERINGSYSTEM_SENSORS_LOCKEDSOILSENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/ModbusSoilSensor.h b/firmware/components/sensors/include/sensors/ModbusSoilSensor.h index 0b2f279..34d1ee6 100644 --- a/firmware/components/sensors/include/sensors/ModbusSoilSensor.h +++ b/firmware/components/sensors/include/sensors/ModbusSoilSensor.h @@ -59,6 +59,7 @@ class ModbusSoilSensor : public ISoilSensor { // ISoilSensor bool initialize() override; bool read() override; + SoilSnapshot snapshot() override; bool isAvailable() override; int getLastError() override; @@ -123,6 +124,12 @@ class ModbusSoilSensor : public ISoilSensor { bool initialized_ = false; int lastError_ = 0; + // Read history for snapshot() (no bus I/O): lastReadOk_ tracks the most + // recent read() outcome; hasEverReadOk_ latches once any read() succeeded + // so the last-good values are known to be meaningful (available). + bool lastReadOk_ = false; + bool hasEverReadOk_ = false; + // Last-good reading (published only by a fully successful read()). float moisture_ = 0.0f; float temperature_ = 0.0f; diff --git a/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h b/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h index a51213f..668c48e 100644 --- a/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h +++ b/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h @@ -65,6 +65,14 @@ class MockSoilSensor : public ISoilSensor { int initializeCalls = 0; int readCalls = 0; int isAvailableCalls = 0; + + // Read history consumed by snapshot() (mirrors the real sensor). read() + // maintains them, but they are public (mock "all state public" style) so a + // consumer test can drive snapshot()'s availability directly — e.g. force + // the controller's "soil-unavailable" branch. lastReadOk = the most recent + // read() outcome; hasEverReadOk = any read() has ever succeeded (available). + bool lastReadOk = false; + bool hasEverReadOk = false; /// Every calibrate*() reference-value argument, in call order (the /// vector size doubles as the per-quantity call counter). std::vector calibrateMoistureCalls; @@ -107,7 +115,10 @@ class MockSoilSensor : public ISoilSensor { ++readCalls; if (script_.empty()) { // Unscripted: legacy behaviour — return the plain field, getters - // serve the manually-set values. + // serve the manually-set values. Track the same outcome so + // snapshot() stays coherent with the getters. + lastReadOk = readResult; + hasEverReadOk = hasEverReadOk || readResult; return readResult; } const Step& step = script_[next_]; @@ -117,6 +128,7 @@ class MockSoilSensor : public ISoilSensor { if (!step.ok) { readResult = false; lastError = step.error; + lastReadOk = false; return false; // values untouched — last-good contract } // Publish the coherent values into the public fields so the getters @@ -131,9 +143,30 @@ class MockSoilSensor : public ISoilSensor { potassium = step.potassium; readResult = true; lastError = 0; + lastReadOk = true; + hasEverReadOk = true; return true; } + SoilSnapshot snapshot() override + { + // Coherent, non-blocking: report the read history + the current field + // values (the same ones the getters serve). + SoilSnapshot s; + s.readOk = lastReadOk; + s.available = hasEverReadOk; + s.lastError = lastError; + s.moisture = moisture; + s.temperature = temperature; + s.humidity = humidity; + s.ph = ph; + s.ec = ec; + s.nitrogen = nitrogen; + s.phosphorus = phosphorus; + s.potassium = potassium; + return s; + } + bool isAvailable() override { ++isAvailableCalls; diff --git a/firmware/components/sensors/src/ModbusSoilSensor.cpp b/firmware/components/sensors/src/ModbusSoilSensor.cpp index da02969..c852f13 100644 --- a/firmware/components/sensors/src/ModbusSoilSensor.cpp +++ b/firmware/components/sensors/src/ModbusSoilSensor.cpp @@ -85,6 +85,7 @@ bool ModbusSoilSensor::read() // Lazy initialization (legacy :77-81). A failure here is already // logged inside initialize() (both exits) and lastError_ is set there. if (!initialized_ && !initialize()) { + lastReadOk_ = false; // read outcome for snapshot() return false; } @@ -96,6 +97,7 @@ bool ModbusSoilSensor::read() // the ad-hoc code 4) and leave the last-good values untouched. lastError_ = client_.getLastError(); ESP_LOGW(TAG, "read failed: bus error %d", lastError_); + lastReadOk_ = false; return false; } @@ -132,6 +134,7 @@ bool ModbusSoilSensor::read() "ph=%.1f)", static_cast(moisture), static_cast(temperature), static_cast(ph)); + lastReadOk_ = false; return false; } @@ -147,9 +150,30 @@ bool ModbusSoilSensor::read() potassium_ = potassium; lastError_ = 0; + lastReadOk_ = true; + hasEverReadOk_ = true; return true; } +SoilSnapshot ModbusSoilSensor::snapshot() +{ + // Cached-only, NO bus I/O (QUIRK 5): report the read history and the + // last-good values. available = at least one successful read on record. + SoilSnapshot s; + s.readOk = lastReadOk_; + s.available = hasEverReadOk_; + s.lastError = lastError_; + s.moisture = moisture_; + s.temperature = temperature_; + s.humidity = humidity_; + s.ph = ph_; + s.ec = ec_; + s.nitrogen = nitrogen_; + s.phosphorus = phosphorus_; + s.potassium = potassium_; + return s; +} + bool ModbusSoilSensor::isAvailable() { // Lazy initialization doubles as the probe (legacy :136-138). diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index 06a508c..24361af 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -688,9 +688,10 @@ extern "C" void app_main(void) // reaches the controllers purely through `config` (read each tick) — no // direct API↔controller call. #if BOARD_HAS_RESERVOIR_PUMP - watering_task_start(watering_controller, reservoir_controller, config); + watering_task_start(watering_controller, reservoir_controller, config, + event_logger); #else - watering_task_start(watering_controller, config); + watering_task_start(watering_controller, config, event_logger); #endif // /api/v1/ HTTP server (feature 009 US1). Constructed here — after EVERY diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index 6f4d08d..31300b6 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -695,8 +695,9 @@ int env_cmd(int argc, char **argv) if (!s_env->read()) { // read() and getLastError() are separate locked calls; a sensor- // task poll interleaving between them can succeed and reset the - // error to 0 (benign cross-lock race, TODO(PR-11) snapshot helper - // in LockedEnvironmentalSensor.h) — hint accordingly. + // error to 0 (benign cross-lock race) — hint accordingly. The + // LockedEnvironmentalSensor snapshot() helper now exists; migrating + // this console read to it is a future cleanup — TODO(PR-14). const int error = s_env->getLastError(); const char *hint = (error == 1) ? "sensor not found" : (error == 2) ? "read failed" @@ -725,9 +726,9 @@ const char *level_state_str(ILevelSensor &sensor) // report not_yet_valid, or a reading that lost validity in the gap // print "dry" — benign either way, because isWaterPresent() returns // false whenever invalid (ILevelSensor contract): never a stale or - // phantom "water". One-poll diagnostic glitch only; TODO(PR-11): - // migrate to the snapshot helper in LockedLevelSensor.h (PR-14 keeps - // relying on this console path). + // phantom "water". One-poll diagnostic glitch only. The + // LockedLevelSensor snapshot() helper now exists; migrating this + // console read to it is deferred — TODO(PR-14). if (!sensor.isValid()) { return "not_yet_valid"; } @@ -774,10 +775,12 @@ int power_cmd(int argc, char **argv) return 1; } if (!s_power->read()) { - // read() and getLastError() are separate locked calls; when - // PR-09/PR-11 add readers an interleaving read can reset the - // error to 0 between them (benign cross-lock race, TODO(PR-11) - // snapshot helper in LockedPowerSensor.h) — hint accordingly. + // read() and getLastError() are separate locked calls; a + // concurrent read from another task could reset the error to 0 + // between them (benign cross-lock race) — hint accordingly. No + // periodic power reader exists yet (the controller never touches + // power); a LockedPowerSensor snapshot() helper plus a reader are + // future work — TODO(PR-14). const int error = s_power->getLastError(); const char *hint = (error == 1) ? "sensor not found" : (error == 2) ? "read failed" diff --git a/firmware/main/watering_task.cpp b/firmware/main/watering_task.cpp index 40b0333..ee476d6 100644 --- a/firmware/main/watering_task.cpp +++ b/firmware/main/watering_task.cpp @@ -16,9 +16,15 @@ * task is needed. That blocking read (plus the periodic littlefs data-log) is * isolated on THIS task and never runs on the 10 Hz safety loop. * - * Watchdog: this is a watering-critical task, so it subscribes to the task WDT - * and feeds once per cycle (mirrors sensor_task). The blocking Modbus read is - * well under the WDT timeout. + * Watchdog: this is a watering-critical task, so it subscribes to the task WDT. + * The sensor-read cadence (IConfigStore::getSensorReadIntervalMs()) is + * operator-writable with NO upper bound, so it can legally exceed the task WDT + * timeout. A single vTaskDelay of the whole period would then starve the + * watchdog between feeds and panic-reboot; because the interval is NVS-persisted + * that would boot-loop. So the sleep is chunked: the task sleeps in bounded + * kFeedChunkMs slices, feeding the WDT after each slice, and runs the controller + * tick exactly once per full period. The blocking Modbus read inside tick() is + * itself well under the WDT timeout. * * Isolation: the task shares nothing with the network/HTTP path beyond the same * Locked* wrappers every other task uses (FR-017). @@ -39,6 +45,7 @@ namespace { constexpr uint32_t kStackBytes = 8192; ///< blocking Modbus read + littlefs log constexpr UBaseType_t kPriority = 1; ///< same class as sensor_task constexpr uint32_t kFloorMs = 1000; ///< IConfigStore sensor-interval floor +constexpr uint32_t kFeedChunkMs = 1000; ///< max sleep between WDT feeds /// Long-lived task context (the task never exits). A single static instance /// holds borrowed pointers to the app_main collaborators. @@ -57,8 +64,9 @@ WateringTaskCtx ctx; WateringTaskCtx* c = static_cast(arg); // Watering-critical task: subscribe to the task WDT (feature 008 US3). The - // period is re-read each loop and floored at 1000 ms, well under the 20 s - // default timeout; the feed each cycle is the liveness proof the WDT needs. + // liveness proof is the per-chunk feed below, NOT one feed per cycle: the + // period can exceed the WDT timeout, so the feed must be decoupled from the + // tick cadence. watchdog_subscribe_current_task(); while (true) { @@ -69,14 +77,24 @@ WateringTaskCtx ctx; if (periodMs < kFloorMs) { periodMs = kFloorMs; } - vTaskDelay(pdMS_TO_TICKS(periodMs)); - // Feed once per cycle: this task is alive and servicing the WDT. - watchdog_feed(); + // Chunked feed-and-sleep: never sleep the whole (unbounded) period in + // one vTaskDelay. Sleep in bounded kFeedChunkMs slices, feeding the WDT + // after each, so a large configured period cannot starve the watchdog + // and boot-loop the device. + uint32_t slept = 0; + while (slept < periodMs) { + const uint32_t chunk = + (periodMs - slept < kFeedChunkMs) ? (periodMs - slept) + : kFeedChunkMs; + vTaskDelay(pdMS_TO_TICKS(chunk)); + watchdog_feed(); + slept += chunk; + } - // Decision layer. tick() does the periodic soil read (refreshing the - // LockedSoilSensor cache) + the watering decision + the periodic - // data-log. + // Decision layer, once per full period. tick() does the periodic soil + // read (refreshing the LockedSoilSensor cache) + the watering decision + + // the periodic data-log. c->controller->tick(); #if BOARD_HAS_RESERVOIR_PUMP // Reservoir flag mapping: `enabled` is always true — on rev1 the @@ -95,7 +113,7 @@ WateringTaskCtx ctx; #if BOARD_HAS_RESERVOIR_PUMP void watering_task_start(WateringController& controller, ReservoirController& reservoir, - IConfigStore& config) + IConfigStore& config, EventLogger& events) { ctx.controller = &controller; ctx.config = &config; @@ -106,15 +124,19 @@ void watering_task_start(WateringController& controller, ReservoirController& re kPriority, nullptr); if (created != pdPASS) { // Not a safety function: log and continue. The 10 Hz loop still - // enforces pump timing; only the decision layer is absent. + // enforces pump timing; only the decision layer is absent. Record a + // durable, operator-visible event (survives reboot, served by + // /api/v1/events) so the missing decision layer is not silent. ESP_LOGE(TAG, "failed to create watering task"); + events.logFailsafe("watering-task-start-failed"); return; } ESP_LOGI(TAG, "watering task started (%lu ms cadence floor)", static_cast(kFloorMs)); } #else -void watering_task_start(WateringController& controller, IConfigStore& config) +void watering_task_start(WateringController& controller, IConfigStore& config, + EventLogger& events) { ctx.controller = &controller; ctx.config = &config; @@ -124,8 +146,11 @@ void watering_task_start(WateringController& controller, IConfigStore& config) kPriority, nullptr); if (created != pdPASS) { // Not a safety function: log and continue. The 10 Hz loop still - // enforces pump timing; only the decision layer is absent. + // enforces pump timing; only the decision layer is absent. Record a + // durable, operator-visible event (survives reboot, served by + // /api/v1/events) so the missing decision layer is not silent. ESP_LOGE(TAG, "failed to create watering task"); + events.logFailsafe("watering-task-start-failed"); return; } ESP_LOGI(TAG, "watering task started (%lu ms cadence floor)", diff --git a/firmware/main/watering_task.h b/firmware/main/watering_task.h index 24f8a37..453d644 100644 --- a/firmware/main/watering_task.h +++ b/firmware/main/watering_task.h @@ -18,6 +18,7 @@ #include "board/board.h" #include "control/WateringController.h" +#include "events/EventLogger.h" #include "interfaces/IConfigStore.h" #if BOARD_HAS_RESERVOIR_PUMP #include "control/ReservoirController.h" @@ -30,18 +31,23 @@ * task (i.e. forever — function-local statics). The task subscribes to the task * WDT and ticks every IConfigStore::getSensorReadIntervalMs() (re-read each * loop so config changes take effect), floored at 1000 ms. A task-creation - * failure is logged and swallowed — like the sensor task, the decision layer is + * failure is logged AND recorded as a durable failsafe event (so the absent + * decision layer is operator-visible via /api/v1/events across a reboot); the + * failure is otherwise swallowed — like the sensor task, the decision layer is * started best-effort and the 10 Hz safety loop is unaffected. * * @param controller Automatic + manual plant watering logic (soil reader). * @param reservoir Reservoir auto-fill state machine (rev1 only). * @param config Runtime-tunable cadence and mode flag, re-read each tick. + * @param events Persistent event log; a task-creation failure is recorded + * here as a durable, operator-visible failsafe event. */ #if BOARD_HAS_RESERVOIR_PUMP void watering_task_start(WateringController& controller, ReservoirController& reservoir, - IConfigStore& config); + IConfigStore& config, EventLogger& events); #else -void watering_task_start(WateringController& controller, IConfigStore& config); +void watering_task_start(WateringController& controller, IConfigStore& config, + EventLogger& events); #endif #endif /* WATERINGSYSTEM_MAIN_WATERING_TASK_H */ diff --git a/firmware/test_apps/host/main/test_reservoir.cpp b/firmware/test_apps/host/main/test_reservoir.cpp index fb66490..b24dcfb 100644 --- a/firmware/test_apps/host/main/test_reservoir.cpp +++ b/firmware/test_apps/host/main/test_reservoir.cpp @@ -294,6 +294,27 @@ void test_manual_fill_refused_when_full(void) TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); } +// Auto level control OFF (manual mode) while the feature is enabled: the +// dry/dry row that would normally start a fill is suppressed, but an explicit +// manual fill still works. Guards the tick(enabled=true, autoLevelControl=false) +// gate a regression would otherwise drop unnoticed. +void test_auto_level_off_suppresses_fill_but_manual_works(void) +{ + Fixture f; + f.low.scriptValidState(false); + f.high.scriptValidState(false); // dry/dry: would normally start a fill + + // Auto level control off (manual mode): no auto fill despite dry/dry. + f.controller.tick(/*enabled=*/true, /*autoLevelControl=*/false); + TEST_ASSERT_FALSE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(0, onTransitions(f.pump)); + + // A manual fill still works while auto level control is off. + TEST_ASSERT_TRUE(f.controller.startManualFill(60)); + TEST_ASSERT_TRUE(f.pump.isRunning()); + TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); +} + // Feature disabled (FR-013): the pump is forced OFF and ALL logic is skipped, // even when the marks read dry/dry (which would otherwise start a fill). void test_feature_disabled_forces_off_and_skips_logic(void) @@ -341,5 +362,6 @@ void run_reservoir_tests(void) // Manual fill + feature gate RUN_TEST(test_manual_fill_refused_when_full); + RUN_TEST(test_auto_level_off_suppresses_fill_but_manual_works); RUN_TEST(test_feature_disabled_forces_off_and_skips_logic); } diff --git a/firmware/test_apps/host/main/test_watering_controller.cpp b/firmware/test_apps/host/main/test_watering_controller.cpp index 7fcf259..4447510 100644 --- a/firmware/test_apps/host/main/test_watering_controller.cpp +++ b/firmware/test_apps/host/main/test_watering_controller.cpp @@ -24,6 +24,7 @@ */ #include +#include #include "unity.h" @@ -94,6 +95,19 @@ int failsafeEventCount(const MockDataStorage& storage) return count; } +/// Detail string of the most recent fail-safe event (the reason the controller +/// passed to EventLogger::logFailsafe), or "" when none was logged. +std::string lastFailsafeReason(const MockDataStorage& storage) +{ + std::string reason; + for (const auto& event : storage.events) { + if (event.category == IDataStorage::kCategoryFailsafe) { + reason = event.detail; // keep the newest (append order) + } + } + return reason; +} + /// Total sensor readings persisted across all metrics (a proxy for "how many /// data-log values were written"). Independent of the event log. int sensorReadingCount(const MockDataStorage& storage) @@ -106,9 +120,16 @@ int sensorReadingCount(const MockDataStorage& storage) } /// Script the sensor's next tick outcome via the plain public fields (no FIFO -/// script): read() returns @p readOk, isAvailable() returns @p available and -/// the getters serve @p moisture. A failed read leaves the value in place -/// (mirroring the last-good contract); the controller must ignore it. +/// script): read() returns @p readOk and the getters serve @p moisture. +/// +/// NOTE on @p available (post-M2): the controller no longer calls isAvailable() +/// — it consumes snapshot().available, which the sensor derives from its read +/// history (hasEverReadOk). So a read that has EVER succeeded makes the sensor +/// "available" from then on, regardless of this argument; @p available only +/// sets the legacy isAvailableResult (kept for any direct isAvailable() caller). +/// Tests that need the "never read OK -> unavailable" state simply never let a +/// read succeed; tests that need availability to regress on a running pump set +/// f.soil.hasEverReadOk directly. void setSensor(Fixture& f, bool readOk, bool available, float moisture) { f.soil.readResult = readOk; @@ -262,8 +283,13 @@ void test_boundaries_low_and_high_inclusive(void) // T007 — fail-safe branches (Constitution I) // =========================================================================== -// Scenario 5: automatic + running, sensor becomes unavailable -> emergency -// stop + logged fail-safe + no watering decision. +// Scenario 5 (post-M2): automatic + running, the sensor's availability signal +// drops (snapshot().available == false) -> emergency stop with the +// "soil-unavailable" reason + no watering decision. A running pump implies a +// prior good read, so in production the ever-read-ok latch keeps `available` +// true (a later drop shows up as "soil-stale", covered below); here we drive +// the availability signal false directly to exercise the controller's +// "soil-unavailable" branch on a running pump and assert its reason. void test_failsafe_unavailable_stops_running_pump(void) { Fixture f; @@ -272,16 +298,25 @@ void test_failsafe_unavailable_stops_running_pump(void) f.controller.tick(); TEST_ASSERT_TRUE(f.pump.isRunning()); + // Force the availability signal false on the next snapshot (impossible via + // the real latch, but a valid controller input to unit-test). f.clock.advance(1000); setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + f.soil.hasEverReadOk = false; // snapshot().available -> false f.controller.tick(); TEST_ASSERT_FALSE(f.pump.isRunning()); TEST_ASSERT_EQUAL_INT(1, failsafeEventCount(f.storage)); + TEST_ASSERT_EQUAL_STRING("soil-unavailable", + lastFailsafeReason(f.storage).c_str()); TEST_ASSERT_EQUAL_UINT32(0, f.events.droppedEvents()); } -// Scenario 5: data stale beyond the 30 s window -> emergency stop. +// Scenario 5: data stale beyond the 30 s window -> emergency stop with the +// "soil-stale" reason. This is the production shape of "the sensor worked then +// stopped responding": the ever-read-ok latch keeps `available` true, the reads +// now fail, and once the last valid read ages past 30 s the controller fails +// safe as stale (a transient failure inside the window does NOT stop it). void test_failsafe_stale_stops_running_pump(void) { Fixture f; @@ -290,14 +325,16 @@ void test_failsafe_stale_stops_running_pump(void) f.controller.tick(); TEST_ASSERT_TRUE(f.pump.isRunning()); - // Reads keep failing while the sensor still probes available; once the last - // valid read ages past 30 s the controller fails safe. + // Reads keep failing while the sensor stays available (latched); once the + // last valid read ages past 30 s the controller fails safe. f.clock.advance(30'001); setSensor(f, /*readOk=*/false, /*available=*/true, /*moisture=*/20.0f); f.controller.tick(); TEST_ASSERT_FALSE(f.pump.isRunning()); TEST_ASSERT_EQUAL_INT(1, failsafeEventCount(f.storage)); + TEST_ASSERT_EQUAL_STRING("soil-stale", + lastFailsafeReason(f.storage).c_str()); } // Scenario 5: a successful read with out-of-range moisture (both bounds) @@ -356,9 +393,12 @@ void test_failsafe_not_delayed_by_soak(void) f.controller.tick(); TEST_ASSERT_TRUE(f.pump.isRunning()); - // Running, dry, soak origin set: trigger a fail-safe -> immediate stop. + // Running, dry, soak origin set: trigger a fail-safe -> immediate stop. A + // successful read with out-of-range moisture is the running-pump fail-safe + // that stays reachable under the M2 model (a transient unavailable/failed + // read inside the staleness window no longer stops mid-burst). f.clock.advance(1000); - setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + setSensor(f, /*readOk=*/true, /*available=*/true, /*moisture=*/150.0f); f.controller.tick(); TEST_ASSERT_FALSE(f.pump.isRunning()); @@ -379,8 +419,11 @@ void test_failsafe_during_pending_soak_takes_no_action(void) TEST_ASSERT_FALSE(f.pump.isRunning()); TEST_ASSERT_EQUAL_INT(1, onTransitions(f.pump)); - f.clock.advance(1000); // still inside the soak window - setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + // Age the last valid read past the staleness window while still inside the + // soak window: the fail-safe (stale) branch runs but takes no action beyond + // not-watering because the pump is already stopped. + f.clock.advance(30'001); + setSensor(f, /*readOk=*/false, /*available=*/true, /*moisture=*/20.0f); f.controller.tick(); TEST_ASSERT_FALSE(f.pump.isRunning()); @@ -476,8 +519,10 @@ void test_auto_run_is_not_flagged_manual(void) f.controller.tick(); // automatic burst starts TEST_ASSERT_TRUE(f.pump.isRunning()); + // A successful read with out-of-range moisture fails safe immediately on the + // running automatic burst (the running-pump fail-safe reachable post-M2). f.clock.advance(1000); - setSensor(f, /*readOk=*/false, /*available=*/false, /*moisture=*/20.0f); + setSensor(f, /*readOk=*/true, /*available=*/true, /*moisture=*/150.0f); f.controller.tick(); TEST_ASSERT_FALSE(f.pump.isRunning()); // fail-safe stopped it @@ -529,6 +574,9 @@ void test_data_log_cadence(void) f.clock.advance(1000); f.controller.tick(); TEST_ASSERT_EQUAL_INT(10, sensorReadingCount(f.storage)); + // The full batch is exactly the kMaxMetrics=10 distinct-metric budget: + // nothing hit the cap, so the store rejected no write (drop-sensitive). + TEST_ASSERT_EQUAL_INT(0, f.storage.rejectedWrites); // Inside the interval: nothing new. f.clock.advance(59'999); @@ -636,6 +684,118 @@ void test_data_log_runs_on_failsafe_path(void) .size())); } +// FR-014: env read fails while soil is valid and time is set -> no env metrics +// are logged (the env branch is gated on read() && isAvailable()), while the +// soil metrics are still logged this tick. Mirror of the fail-safe-path test, +// exercising the OTHER data-log branch (env failure instead of soil failure). +void test_data_log_env_read_failure(void) +{ + Fixture f; + f.config.stored.dataLogIntervalMs = 60'000; + f.wallClock.setEpoch(1'700'000'000); + f.env.scriptFailedRead(2); // env read fails -> env branch skipped + // Soil valid, all NPK >= 0 -> 4 soil-base + 3 NPK = 7 soil metrics. + f.soil.scriptSuccessfulRead(40.0f, 18.0f, 40.0f, 6.5f, 1.2f, 3.0f, 5.0f, + 8.0f); + + f.clock.advance(1000); + f.controller.tick(); + + TEST_ASSERT_EQUAL_INT(7, sensorReadingCount(f.storage)); // soil only + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("env_temperature", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("env_humidity", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("env_pressure", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 1, static_cast( + f.storage.getSensorReadings("soil_moisture", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 1, static_cast( + f.storage.getSensorReadings("soil_nitrogen", 0, UINT32_MAX) + .size())); +} + +// FR-014: NPK filter is per-channel — phosphorus < 0 AND potassium < 0 are both +// skipped while nitrogen (>= 0) and the soil-base metrics are logged. +void test_data_log_npk_phosphorus_potassium_negative(void) +{ + Fixture f; + f.config.stored.dataLogIntervalMs = 60'000; + f.wallClock.setEpoch(1'700'000'000); + f.env.scriptSuccessfulRead(21.5f, 55.0f, 1013.0f); + // Nitrogen >= 0 -> logged; phosphorus + potassium < 0 -> both skipped. + f.soil.scriptSuccessfulRead(40.0f, 18.0f, 40.0f, 6.5f, 1.2f, 3.0f, -1.0f, + -2.0f); + + f.clock.advance(1000); + f.controller.tick(); + + // 3 env + 4 soil-base + 1 NPK (nitrogen only) = 8. + TEST_ASSERT_EQUAL_INT(8, sensorReadingCount(f.storage)); + TEST_ASSERT_EQUAL_INT( + 1, static_cast( + f.storage.getSensorReadings("soil_nitrogen", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("soil_phosphorus", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 0, static_cast( + f.storage.getSensorReadings("soil_potassium", 0, UINT32_MAX) + .size())); + // Soil-base + env still present. + TEST_ASSERT_EQUAL_INT( + 1, static_cast( + f.storage.getSensorReadings("soil_moisture", 0, UINT32_MAX) + .size())); + TEST_ASSERT_EQUAL_INT( + 1, static_cast( + f.storage.getSensorReadings("env_temperature", 0, UINT32_MAX) + .size())); +} + +// FR-003: a burst stopped at the HIGH threshold arms the soak pause from the +// high-stop time (lastBurstEndMs_ = now on the high-stop path). No new burst +// starts until the soak elapses even while the soil reads dry. +void test_soak_origin_armed_on_high_threshold_stop(void) +{ + Fixture f; // soak 300 s + f.config.stored.wateringDurationS = 300; // no self-stop confound + + setSensor(f, true, true, 20.0f); + f.controller.tick(); // burst runs + TEST_ASSERT_TRUE(f.pump.isRunning()); + + // Stop at the high threshold: this arms the soak origin at t = now. + f.clock.advance(1000); + setSensor(f, true, true, 60.0f); // >= high 55 -> stop + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + // 1 ms before the soak pause elapses (measured from the high-stop): still + // blocked even though the soil now reads dry. + f.clock.advance(299'999); + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_FALSE(f.pump.isRunning()); + + // Exactly at the soak pause: the next burst starts. + f.clock.advance(1); + setSensor(f, true, true, 20.0f); + f.controller.tick(); + TEST_ASSERT_TRUE(f.pump.isRunning()); +} + } // namespace void run_watering_controller_tests(void) @@ -645,6 +805,7 @@ void run_watering_controller_tests(void) RUN_TEST(test_no_start_when_disabled); RUN_TEST(test_stops_at_high_threshold); RUN_TEST(test_soak_pause_blocks_then_allows_restart); + RUN_TEST(test_soak_origin_armed_on_high_threshold_stop); RUN_TEST(test_config_change_picked_up_next_tick); RUN_TEST(test_no_action_before_first_successful_read); RUN_TEST(test_gate_on_transient_failed_read); @@ -668,4 +829,6 @@ void run_watering_controller_tests(void) RUN_TEST(test_data_log_epoch_and_npk_filter); RUN_TEST(test_data_log_gated_on_time_set); RUN_TEST(test_data_log_runs_on_failsafe_path); + RUN_TEST(test_data_log_env_read_failure); + RUN_TEST(test_data_log_npk_phosphorus_potassium_negative); } diff --git a/specs/011-watering-controller-host-tests/checklists/branch-coverage.md b/specs/011-watering-controller-host-tests/checklists/branch-coverage.md index 4e07784..48a53d5 100644 --- a/specs/011-watering-controller-host-tests/checklists/branch-coverage.md +++ b/specs/011-watering-controller-host-tests/checklists/branch-coverage.md @@ -6,8 +6,9 @@ the contracts to the covering test. All boxes are checked only because the full **Verification run (this branch, `011-watering-controller-host-tests`):** -- [x] Full host suite green: **289 Tests, 0 Failures, 0 Ignored** (`pump_host_tests.elf`, exit 0) - — of which 23 in `test_watering_controller.cpp` + 14 in `test_reservoir.cpp` are new for feature 011. +- [x] Full host suite green: **293 Tests, 0 Failures, 0 Ignored** (`pump_host_tests.elf`, exit 0) + — of which 26 in `test_watering_controller.cpp` + 15 in `test_reservoir.cpp` are new for feature 011 + (incl. the CP3-round additions T-A/T-B/T-C + the NPK and env-read-failure branches). - [x] rev1 build green (`sdkconfig.board.rev1_devkit`) — `REV1_RC=0`, app 1037.3 KiB / 1536 KiB slot (32.5 % margin). - [x] rev2 build green (`sdkconfig.board.rev2`) — `REV2_RC=0`, app 1039.4 KiB / 1536 KiB slot (32.3 % margin). @@ -22,6 +23,8 @@ the contracts to the covering test. All boxes are checked only because the full — `test_stops_at_high_threshold` - [x] Soak gate (FR-003): after a burst ends, no new burst until the pause elapses even while dry; then restart — `test_soak_pause_blocks_then_allows_restart` +- [x] Soak origin armed on the HIGH-THRESHOLD stop path (`lastBurstEndMs_` set at line 110) — T-A + — `test_soak_origin_armed_on_high_threshold_stop` - [x] Config thresholds/durations re-read each tick (runtime-tunable) — `test_config_change_picked_up_next_tick` - [x] Low/high thresholds are inclusive boundaries — `test_boundaries_low_and_high_inclusive` @@ -51,9 +54,19 @@ the contracts to the covering test. All boxes are checked only because the full ## WateringController — data logging (FR-014) - [x] Logs at the data-log cadence (interval gate) — `test_data_log_cadence` -- [x] Epoch timestamp + NPK-only-when-≥0 filter — `test_data_log_epoch_and_npk_filter` +- [x] Epoch timestamp + NPK-only-when-≥0 filter (nitrogen<0) — `test_data_log_epoch_and_npk_filter` +- [x] NPK ≥0 filter, phosphorus + potassium negative branches — `test_data_log_npk_phosphorus_potassium_negative` +- [x] Exactly-10-metric cap with NO silent drop (drop-sensitive: `rejectedWrites == 0`) — T-C, asserted in + `test_data_log_cadence` - [x] Gated on `isTimeSet()` (no bogus 1970) — `test_data_log_gated_on_time_set` - [x] Telemetry logs even on the fail-safe path (env still logged, soil skipped) — `test_data_log_runs_on_failsafe_path` +- [x] Env read failure → env metrics skipped, soil still logged — `test_data_log_env_read_failure` + +## WateringController — soil access (M2, post-CP3) + +- [x] `tick()` reads once then consumes ONE `snapshot()` (no second blocking `isAvailable()` probe); + availability = ever-read-ok — exercised across the fail-safe reason tests (`lastFailsafeReason()` + asserts `soil-unavailable` vs `soil-stale`) ## ReservoirController — truth table (FR-012/013) @@ -73,6 +86,8 @@ the contracts to the covering test. All boxes are checked only because the full - [x] A normal high-wet stop does NOT arm the cooldown — `test_normal_high_wet_stop_does_not_arm_cooldown` - [x] Manual fill bypasses the cooldown — `test_manual_fill_bypasses_cooldown` - [x] Manual fill refused when already full (high wet) — `test_manual_fill_refused_when_full` +- [x] Auto-level-control OFF while enabled suppresses auto fill; manual fill still works — T-B + — `test_auto_level_off_suppresses_fill_but_manual_works` - [x] Feature disabled forces the pump off and skips all logic — `test_feature_disabled_forces_off_and_skips_logic` ## Not host-covered (target-only / HIL)