From 4710ceca85b242b76648b454b98df1f60bbf9e4b Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sat, 4 Jul 2026 18:34:50 +0200 Subject: [PATCH 1/7] docs(008): spec, plan, tasks for SNTP/watchdog/event-logging (PR-08) Spec-kit artifacts for PR-08 (SNTP time + task watchdog + event logging), approved at Checkpoint 2. Records: reuse of PR-06 IDataStorage event log, the new IWallClock seam, CET/CEST TZ, watchdog excludes the WiFi task, WS_TASK_WDT_TIMEOUT_S=20s and se.pool.ntp.org defaults. No implementation yet. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../checklists/requirements.md | 41 ++++ .../contracts/event-logger.md | 36 +++ .../contracts/task-watchdog.md | 40 +++ .../contracts/wall-clock-time.md | 45 ++++ specs/008-sntp-watchdog-logging/data-model.md | 77 ++++++ specs/008-sntp-watchdog-logging/plan.md | 139 +++++++++++ specs/008-sntp-watchdog-logging/quickstart.md | 67 +++++ specs/008-sntp-watchdog-logging/research.md | 94 +++++++ specs/008-sntp-watchdog-logging/spec.md | 230 ++++++++++++++++++ specs/008-sntp-watchdog-logging/tasks.md | 188 ++++++++++++++ 10 files changed, 957 insertions(+) create mode 100644 specs/008-sntp-watchdog-logging/checklists/requirements.md create mode 100644 specs/008-sntp-watchdog-logging/contracts/event-logger.md create mode 100644 specs/008-sntp-watchdog-logging/contracts/task-watchdog.md create mode 100644 specs/008-sntp-watchdog-logging/contracts/wall-clock-time.md create mode 100644 specs/008-sntp-watchdog-logging/data-model.md create mode 100644 specs/008-sntp-watchdog-logging/plan.md create mode 100644 specs/008-sntp-watchdog-logging/quickstart.md create mode 100644 specs/008-sntp-watchdog-logging/research.md create mode 100644 specs/008-sntp-watchdog-logging/spec.md create mode 100644 specs/008-sntp-watchdog-logging/tasks.md diff --git a/specs/008-sntp-watchdog-logging/checklists/requirements.md b/specs/008-sntp-watchdog-logging/checklists/requirements.md new file mode 100644 index 0000000..add1e31 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/checklists/requirements.md @@ -0,0 +1,41 @@ +# Specification Quality Checklist: SNTP Time, Task Watchdog & Event Logging + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-07-04 +**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 + +- Parity facts recorded as requirements/assumptions rather than open questions: Swedish NTP pool, CET/CEST + TZ rules, epoch/monotonic timestamps, esp_reset_reason persistence, the ineffective legacy software + watchdog (QUIRK 3), and the new event-log surface reusing PR-06 storage. +- Deliberate scoping recorded (so it won't surface as a review finding): watchdog registers today's + critical task(s) with the mechanism ready for PR-11's watering/control tasks; the WiFi task is + deliberately excluded from the watchdog (a network stall must not reboot the device — PR-07 isolation). +- SNTP server, plausible-time threshold, watchdog timeout, and log levels are build-configurable defaults + (documented in Assumptions) — no [NEEDS CLARIFICATION] needed. diff --git a/specs/008-sntp-watchdog-logging/contracts/event-logger.md b/specs/008-sntp-watchdog-logging/contracts/event-logger.md new file mode 100644 index 0000000..b7de8c4 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/contracts/event-logger.md @@ -0,0 +1,36 @@ +# Contract: `EventLogger` (over PR-06 `IDataStorage`) + +`firmware/main/event_logger.{h,cpp}`. Composes `IDataStorage& storage` + `IWallClock& clock`. Formatting + +category selection are host-tested against `MockDataStorage` + `FakeWallClock`. Does NOT extend +`IDataStorage`. + +## Methods (typed producers) + +| Method | Category | Example `detail` (≤120 B) | +|---|---|---| +| `void logReset(esp_reset_reason_t r)` | `kCategoryReset` | `"reset=TASK_WDT"` | +| `void logWifi(WifiState s)` | `kCategoryConnectivity` | `"wifi=Connected"` / `"wifi=Reconnecting"` | +| `void logPumpStart(pumpName, cause)` | `kCategoryPump` | `"pump=plant start cause=console dur=30s"` | +| `void logPumpStop(pumpName, cause)` | `kCategoryPump` | `"pump=plant stop cause=timeout"` | +| `void logFailsafe(detail)` | `kCategoryFailsafe` | `"failsafe=soil-invalid pump=plant"` (producer = PR-11) | +| `void logOta(detail)` | `kCategoryOta` | (producer = PR-13) | + +## Behavioral contract + +- Every method calls `storage.storeEvent(clock.nowEpoch(), category, detail)`. +- **Never throws / never blocks watering.** A `storeEvent` returning `false` is counted (a `droppedEvents` + counter) and `ESP_LOGW`'d once; the caller is unaffected (FR-014). +- Credential values are never logged (WiFi events log the *state*, not SSID/password). +- Detail strings are built deterministically so the host tests can assert exact bytes; the store truncates + at 120 B (never rejects) so builders need not pre-truncate but should stay concise. +- `EventLogger` holds references (not ownership); the injected `IDataStorage` must be the cross-task + `LockedDataStorage` when shared (it is — `app_main` passes the locked wrapper). + +## Host tests (`test_event_logger.cpp`) + +- Each producer writes exactly one event with the expected category and a detail matching the documented + format (assert against `MockDataStorage`'s captured records + `FakeWallClock`'s epoch). +- `storeEvent`==false path: `MockDataStorage` set to fail writes → `EventLogger` increments its dropped + counter and does not crash. +- Reset-reason mapping is total over `esp_reset_reason_t` (mapping function host-tested with the enum + values available on the host, or a small local mirror enum if the IDF enum isn't linux-visible). diff --git a/specs/008-sntp-watchdog-logging/contracts/task-watchdog.md b/specs/008-sntp-watchdog-logging/contracts/task-watchdog.md new file mode 100644 index 0000000..dd926d2 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/contracts/task-watchdog.md @@ -0,0 +1,40 @@ +# Contract: task watchdog (`task_watchdog` helper) + +`firmware/main/task_watchdog.{h,cpp}`. Thin wrappers over `esp_task_wdt`; the configuration lives in +Kconfig/sdkconfig. Target-only (no host test — HIL-verified); the *policy* (which tasks, timeout margin) is +documented and reviewed. + +## API + +| Function | Contract | +|---|---| +| `esp_err_t watchdog_init()` | Ensure the task WDT is initialized with `CONFIG_WS_TASK_WDT_TIMEOUT_S`, panic-on-timeout enabled. Idempotent if IDF already inited it (`ESP_ERR_INVALID_STATE` treated as OK). Called once early in `app_main`. | +| `void watchdog_subscribe_current_task()` | `esp_task_wdt_add(NULL)` for the calling task; logs on failure (non-fatal). | +| `void watchdog_feed()` | `esp_task_wdt_reset()` for the calling task; called once per loop iteration. | + +## Subscription policy (reviewed) + +| Task | Subscribed? | Feeds where | Rationale | +|---|---|---|---| +| `app_main` 10 Hz main loop | **YES** | each loop iteration (100 ms) | drives pump/level `update()` — watering-critical | +| `sensor_task` | **YES** | each 5 s cycle | environmental read — critical; timeout must exceed 5 s | +| `wifi_task` | **NO** | — | a network stall must NOT reboot the device (PR-07 isolation, FR-014) | +| esp_console REPL | **NO** | — | blocks on UART by design; not watering-critical | +| PR-11 watering/reservoir tasks | later | via this helper | registered when they land (scope note) | + +## Behavioral contract + +- **Timeout** `CONFIG_WS_TASK_WDT_TIMEOUT_S` has a safe margin over the slowest subscribed cadence (5 s + sensor task) to avoid false positives; default chosen accordingly (e.g. 15–30 s). +- **Action = reboot**: on a non-servicing subscribed task the WDT panics → reboot. At the next boot + `pumps_force_off()` runs first (unchanged), and `esp_reset_reason()==ESP_RST_TASK_WDT` is logged + (FR-008/FR-009). +- The IDF idle-task watchdog default must not be left misconfigured such that normal idle triggers it; + align `CONFIG_ESP_TASK_WDT_*` in `sdkconfig.defaults` with the subscribed-task model. + +## HIL verification (`checklists/hil.md`) + +- Deliberately starve a subscribed task (e.g. a `while(true){}` injected behind a debug command or build + flag on the rig) → device reboots within the timeout → pumps measured OFF immediately after reset → + `storage events` shows a `reset=TASK_WDT` entry. +- Normal operation over an extended run triggers no spurious reboot. diff --git a/specs/008-sntp-watchdog-logging/contracts/wall-clock-time.md b/specs/008-sntp-watchdog-logging/contracts/wall-clock-time.md new file mode 100644 index 0000000..e4c5e60 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/contracts/wall-clock-time.md @@ -0,0 +1,45 @@ +# Contract: wall-clock time (`IWallClock` + `TimeService` + `SntpClient`) + +## `IWallClock` (interfaces, pure, no IDF/`` in the header) + +`firmware/components/interfaces/include/interfaces/IWallClock.h` + +```cpp +class IWallClock { +public: + virtual ~IWallClock() = default; + virtual uint32_t nowEpoch() const = 0; // wall-clock epoch seconds (low/boot value before sync) + virtual bool isTimeSet() const = 0; // true iff nowEpoch >= plausibility threshold +}; +``` + +- Target impl `SystemWallClock`: `nowEpoch()` = `time(nullptr)`; `isTimeSet()` = `nowEpoch() >= + kMinPlausibleEpoch`. +- Host fake `FakeWallClock`: settable epoch + is-set, deterministic for `EventLogger` tests. + +## `TimeService` (pure — host-tested) + +`firmware/components/time/include/time/TimeService.h` + +| Member | Contract | +|---|---| +| `static bool isPlausibleEpoch(uint32_t e)` | `e >= kMinPlausibleEpoch` (2020-01-01Z). | +| `static std::string formatLocal(uint32_t epoch)` | Renders Swedish local time; requires the process TZ set to `CET-1CEST,M3.5.0,M10.5.0/3`. Winter→CET(+01), summer→CEST(+02). | +| `SyncStatus status() const` / `void onSynced(uint32_t epoch)` | Tracks `synced` + `lastSyncEpoch` for the console `time` line. | + +Host tests (`test_time.cpp`): a known winter epoch and summer epoch convert with the correct offsets; the +DST boundary (last Sun Mar / last Sun Oct) flips correctly; `isPlausibleEpoch` false for 0/1970, true for a +2020+ epoch. + +## `SntpClient` (target-only, thin) + +`firmware/components/time/src/SntpClient.cpp` (excluded from linux build) + +| Method | Contract | +|---|---| +| `void applyTimezone()` | `setenv("TZ", CET-1CEST,M3.5.0,M10.5.0/3, 1); tzset();` once at init. | +| `void start()` | `esp_netif_sntp_init` against `CONFIG_WS_SNTP_SERVER` (default `se.pool.ntp.org`), step-set mode, register a sync callback; idempotent. Non-blocking; failure to reach the server is non-fatal and retried by the SNTP service. | +| sync callback | on a plausible time, updates `SyncStatus` (synced + lastSyncEpoch). | + +- `start()` is invoked once when the WiFi station first reaches `Connected` (by the `system_observer`). +- The device never blocks waiting for sync; `isTimeSet()` stays false until the first successful sync. diff --git a/specs/008-sntp-watchdog-logging/data-model.md b/specs/008-sntp-watchdog-logging/data-model.md new file mode 100644 index 0000000..488f44f --- /dev/null +++ b/specs/008-sntp-watchdog-logging/data-model.md @@ -0,0 +1,77 @@ +# Phase 1 Data Model: SNTP Time, Task Watchdog & Event Logging + +In-memory firmware state + the reused PR-06 event-log record. No new persisted schema (events reuse PR-06). + +## Entities + +### WallClock / time state (`IWallClock` + `SyncStatus`) + +| Field | Type | Rules | +|---|---|---| +| nowEpoch | uint32 (epoch s) | current wall-clock; before sync it is the low/boot value | +| isTimeSet | bool | true iff nowEpoch ≥ plausibility threshold (year ≥ 2020) | +| synced | bool | a successful SNTP sync has occurred | +| lastSyncEpoch | uint32 | epoch of the last successful sync (0 if never) | + +- `IWallClock` (interfaces): `uint32_t nowEpoch() const; bool isTimeSet() const;` — pure seam. +- `SyncStatus` (pure): `synced`, `lastSyncEpoch`; updated by the SNTP callback. Exposed for the console + `time` line. +- Plausibility threshold is a compile-time constant (min plausible epoch for 2020-01-01). + +### Event log entry (reused from PR-06 `IDataStorage::EventRecord`) + +| Field | Type | Source in PR-08 | +|---|---|---| +| epoch | uint32 | `IWallClock::nowEpoch()` at the moment of the event (best-known) | +| category | uint8 | `kCategoryReset/Connectivity/Pump/Failsafe/Ota` (existing constants) | +| detail | string ≤120 B | cause string built by `EventLogger` (e.g. `"reset=TASK_WDT"`, `"wifi=Connected"`, `"pump=plant start cause=console 30s"`) | + +- Rotation/bounding is PR-06's (2×16 KiB, newest-retained). PR-08 does not change it. + +### Reset reason + +- Captured once at boot via `esp_reset_reason()` → mapped to a short string + (`POWERON/SW/PANIC/INT_WDT/TASK_WDT/BROWNOUT/DEEPSLEEP/…`). Logged as one `kCategoryReset` event. + +### Watchdog registration + +| Field | Type | Notes | +|---|---|---| +| subscribed tasks | set | main loop (app_main task) + sensor task now; PR-11 tasks later | +| timeout | seconds | `CONFIG_WS_TASK_WDT_TIMEOUT_S` (default with margin over 5 s sensor cadence) | +| action | fixed | panic → reboot | +| excluded | — | WiFi task (deliberate — network stall must not reboot) | + +## State / transitions + +### Time sync lifecycle + +```text +[boot] time-not-set (nowEpoch < threshold, synced=false) + │ STA reaches Connected → SntpClient.start() (once) + ▼ + waiting-for-sync (still not-set; non-fatal if server unreachable, keeps retrying) + │ SNTP callback fires with a plausible time → settimeofday + ▼ + time-set (synced=true, lastSyncEpoch set, isTimeSet()=true) ── periodic re-sync keeps it fresh +``` + +- Never blocks: the device runs in time-not-set indefinitely if offline. Consumers gate on `isTimeSet()`. + +### Watchdog lifecycle (per subscribed task) + +```text +task start → esp_task_wdt_add(NULL) → loop{ do work; esp_task_wdt_reset() } + │ a loop iteration exceeds the timeout without reset + ▼ + TASK_WDT panic → reboot → pumps_force_off() first → esp_reset_reason()==TASK_WDT logged +``` + +## Validation rules (pure, host-tested) + +- `isPlausibleEpoch(uint32 e)`: `e >= kMinPlausibleEpoch` (2020-01-01T00:00ःZ). +- `formatLocal(uint32 e) -> string`: with `TZ=CET-1CEST,M3.5.0,M10.5.0/3`, a winter epoch renders `+01:00` + (CET), a summer epoch renders `+02:00` (CEST); DST switch at the last Sunday of March/October. +- `resetReasonString(esp_reset_reason_t) -> const char*`: total mapping over the enum. +- `EventLogger` detail builders: category chosen per producer; detail truncated by the store at 120 B + (never rejected); a `storeEvent`==false is counted, not fatal. diff --git a/specs/008-sntp-watchdog-logging/plan.md b/specs/008-sntp-watchdog-logging/plan.md new file mode 100644 index 0000000..cfb20b6 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/plan.md @@ -0,0 +1,139 @@ +# Implementation Plan: SNTP Time, Task Watchdog & Event Logging + +**Branch**: `008-sntp-watchdog-logging` | **Date**: 2026-07-04 | **Spec**: [spec.md](./spec.md) + +**Input**: `specs/008-sntp-watchdog-logging/spec.md`; PR brief `docs/prd/PR-08-sntp-watchdog-logging.md`; +parity `docs/parity-checklist.md` §7 (NTP), QUIRK 2/3, §6 (event-log surface). + +## Summary + +Complete the phase-2 exit criteria with three loosely-coupled additions, all keeping watering untouched: +(1) **SNTP wall-clock time** — set system time from the Swedish pool after the STA connects, apply the +Europe/Stockholm TZ, and expose an explicit "time-not-set" state; (2) **hardware task watchdog** — subscribe +the watering-critical tasks (the 10 Hz main loop that drives pump `update()`, and the sensor task) to +`esp_task_wdt` so a hung task forces a safe reboot (pumps OFF at boot, reset reason persisted), while the +WiFi task is deliberately excluded; (3) **event logging** — wire producers (reset reason at boot, WiFi +state changes, pump start/stop with cause) into the **existing** PR-06 `IDataStorage` rotating event log +via a small host-tested `EventLogger`, readable over the serial console. Pure logic (TZ/DST conversion, +plausibility, event formatting, reset-reason mapping) is host-tested; the IDF surfaces (`esp_sntp`, +`esp_task_wdt`, `esp_reset_reason`) are thin and target-only. + +## Technical Context + +**Language/Version**: C++17 on ESP-IDF v6.0.1 (Docker `espressif/idf:v6.0.1`), target esp32. + +**Primary Dependencies**: IDF built-ins only — `esp_netif_sntp`/`esp_sntp`, `esp_task_wdt`, +`esp_system` (`esp_reset_reason`), standard C `` (`localtime_r`, `setenv("TZ")`). No new managed +components → `dependencies.lock` untouched. Reuses PR-06 `IDataStorage` (event log) and PR-07 `WifiManager` +(connection snapshot). + +**Storage**: Event log reuses PR-06 `IDataStorage::storeEvent(epoch, category, detail)` / `getEvents(n)` — +categories already defined (`kCategoryPump=1, kCategoryFailsafe=2, kCategoryConnectivity=3, kCategoryOta=4, +kCategoryReset=5`), 2×16 KiB rotation, `detail` ≤120 B. **No interface extension needed.** + +**Testing**: Unity host suite (`test_apps/host/`). New host tests: CET/CEST DST conversion (via +`setenv("TZ")`+`localtime_r` on the linux host), time-not-set plausibility, `EventLogger` formatting/ +category selection against `MockDataStorage`, reset-reason→string mapping, and event-log rotation/content +via the existing `LittleFsDataStorage` tempdir pattern. Hardware paths (`SntpClient`, `esp_task_wdt` +wiring, `esp_reset_reason`) are HIL-verified. + +**Target Platform**: ESP32-WROOM-32E, board targets `BOARD_REV1_DEVKIT` and `BOARD_REV2`. + +**Project Type**: Embedded firmware component(s) + `main/` wiring. + +**Performance Goals**: SNTP/logging never block or delay watering (FR-014). Watchdog timeout has a safe +margin over the slowest critical-task cadence (sensor task 5 s → default timeout well above that). + +**Constraints**: `pumps_force_off()` stays the first action in `app_main`; watchdog action = reboot; +WiFi task excluded from the watchdog; storage-write failure while logging is contained (never crashes or +touches watering); epoch stored, local time only user-facing; app binary stays within the 1.5 MiB OTA slot. + +**Scale/Scope**: One `time` component (SNTP + wall-clock seam + pure conversion), one `EventLogger` + +watchdog helper (in `main/` or a small `system` component), reset-reason at boot, a WiFi/SNTP observer at a +snapshot consumer, pump-event logging at the main-loop pump observer, one Kconfig block, one host suite. + +## Constitution Check + +*GATE: evaluated before Phase 0 and re-checked after Phase 1. Result: PASS (no violations).* + +- **I. Safety First (NON-NEGOTIABLE)** — PASS, and this feature *strengthens* safety. The watchdog gives + real auto-recovery from a hung watering task (replacing the ineffective legacy software watchdog, QUIRK + 3); after a watchdog reset `pumps_force_off()` (unchanged, first action) guarantees pumps OFF (FR-008); + the reset reason is persisted (FR-009). SNTP/logging run outside the watering path and are non-blocking + (FR-014); a storage-write failure is contained. The **WiFi task is deliberately NOT watchdog-subscribed** + so a network stall cannot reboot the device and interrupt watering (preserves PR-07 isolation). The + constitution's "safety-relevant events MUST be persisted" is exactly what the event log delivers. +- **II. Host-Testability** — PASS. TZ/DST conversion, time-not-set plausibility, `EventLogger` + formatting/category selection, and reset-reason mapping are pure and host-tested; `esp_sntp`/ + `esp_task_wdt`/`esp_reset_reason` are thin target-only shims behind seams (`IWallClock`, the SNTP starter, + the watchdog helper), excluded from the linux build. +- **III. Reproducible Builds** — PASS. Only IDF built-ins; no new managed components; + `dependencies.lock` and both `esp-modbus` pins untouched. Both board targets build. +- **IV. Frozen Legacy** — PASS. No change under `src/`/`include/`/`data/`/`test/`/`platformio.ini`. +- **V. Checkpoint-Gated Workflow** — PASS. Implementation delegated to the `implementer` after CP2. +- **VI. English Outward** — PASS. + +**Additional-constraints note:** watchdog config lives in Kconfig/sdkconfig (`CONFIG_ESP_TASK_WDT_*` + +`WS_TASK_WDT_TIMEOUT_S`), board differences stay in the board component (no scattered ifdefs), and the +`COMPILER_OPTIMIZATION_ASSERTIONS_ENABLE` safety pin in `sdkconfig.defaults` is left untouched. + +## Project Structure + +### Documentation (this feature) + +```text +specs/008-sntp-watchdog-logging/ +├── plan.md · research.md · data-model.md · quickstart.md +├── contracts/{wall-clock-time.md, event-logger.md, task-watchdog.md} +├── checklists/requirements.md +└── tasks.md # /speckit-tasks output (not created here) +``` + +### Source Code (repository root) + +```text +firmware/ +├── components/ +│ ├── interfaces/include/interfaces/ +│ │ └── IWallClock.h # NEW — pure seam: nowEpoch(), isTimeSet() (no IDF) +│ ├── time/ # NEW component +│ │ ├── CMakeLists.txt # linux-guarded (pure on host, SNTP on target) +│ │ ├── include/time/ +│ │ │ ├── TimeService.h # pure: plausibility + local-time formatting + sync-status +│ │ │ ├── SyncStatus.h # pure: synced flag, last-sync epoch +│ │ │ ├── SntpClient.h # thin esp_netif_sntp starter (target-only) +│ │ │ ├── SystemWallClock.h # IWallClock over time(nullptr)/settimeofday (target) +│ │ │ └── testing/FakeWallClock.h # host fake (settable epoch / is-set) +│ │ └── src/{TimeService.cpp, SntpClient.cpp, SystemWallClock.cpp} +│ └── (storage/network/... reused unchanged; event log = PR-06 IDataStorage) +├── main/ +│ ├── app_main.cpp # EDIT — esp_reset_reason() boot event; construct TimeService + +│ │ # SntpClient + EventLogger; subscribe main loop to watchdog + feed +│ ├── event_logger.{h,cpp} # NEW — EventLogger over IDataStorage + IWallClock (host-testable core) +│ ├── task_watchdog.{h,cpp} # NEW — esp_task_wdt subscribe/feed helpers + registration pattern +│ ├── system_observer.{h,cpp} # NEW — polls WifiManager snapshot: start SNTP on first Connected, +│ │ # log WiFi state changes + pump start/stop transitions (holds storage) +│ ├── sensor_task.cpp # EDIT — subscribe to watchdog + feed each cycle +│ ├── Kconfig.projbuild # EDIT — WS_SNTP_SERVER, WS_TASK_WDT_TIMEOUT_S, log-level option(s) +│ └── sdkconfig.defaults # EDIT — CONFIG_ESP_TASK_WDT_* as needed (per-component log levels) +└── test_apps/host/main/ + ├── test_time.cpp # NEW — DST conversion, plausibility/time-not-set + ├── test_event_logger.cpp # NEW — EventLogger formatting/category vs MockDataStorage + ├── test_main.cpp · CMakeLists.txt # EDIT — register run_time_tests()/run_event_logger_tests() +``` + +**Structure Decision**: A new `time` component holds SNTP + the wall-clock seam + pure conversion (mirrors +the sensor/network components; SNTP source excluded from the linux build via the +`if(${IDF_TARGET} STREQUAL "linux")` guard). `IWallClock` goes in `interfaces` so pure consumers never see +``/IDF. `EventLogger`, `task_watchdog`, and the `system_observer` live in `main/` because they are +wiring that composes existing components (storage + wifi + pumps) — the same place `sensor_task`/ +`wifi_task` live; `EventLogger`'s formatting core is header-light and host-tested against `MockDataStorage`. +The event log itself is **not** re-implemented — it is PR-06's `IDataStorage`. + +## Complexity Tracking + +> No constitution violations. Table intentionally empty. + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|--------------------------------------| +| — | — | — | diff --git a/specs/008-sntp-watchdog-logging/quickstart.md b/specs/008-sntp-watchdog-logging/quickstart.md new file mode 100644 index 0000000..3c8246d --- /dev/null +++ b/specs/008-sntp-watchdog-logging/quickstart.md @@ -0,0 +1,67 @@ +# Quickstart / Validation Guide: SNTP Time, Task Watchdog & Event Logging + +CI-tagged items gate the merge; HIL-tagged items run on the rev1 rig at Checkpoint 3. + +## Prerequisites + +- ESP-IDF v6.0.1 via Docker. **Docker cannot mount OneDrive** — rsync first: + ```bash + rsync -a --delete --exclude build --exclude managed_components --exclude sdkconfig \ + "$PWD/firmware/" /tmp/ws008-firmware/ + ``` +- On board switch / to avoid stale sdkconfig: `idf.py fullclean` + `rm -f sdkconfig` before each target + build (a leftover sdkconfig from the other board is NOT overridden by `-DSDKCONFIG_DEFAULTS`). + +## CI validation (host tests + both board builds) + +### Host tests — `run_time_tests()` + `run_event_logger_tests()` must cover + +- **DST conversion**: a winter epoch → CET (+01:00), a summer epoch → CEST (+02:00); the last-Sunday-of- + March/October boundaries flip correctly (`setenv("TZ", "CET-1CEST,M3.5.0,M10.5.0/3")` on the host). +- **Time-not-set**: `isPlausibleEpoch(0)` false, a 2020+ epoch true. +- **EventLogger**: each producer (`logReset/logWifi/logPumpStart/logPumpStop`) writes one event with the + expected category + detail (vs `MockDataStorage` + `FakeWallClock`); write-failure path increments the + dropped counter without crashing; reset-reason mapping total. +- **Event-log rotation/content**: reuse the `LittleFsDataStorage` tempdir pattern (as `test_data_storage`) + to confirm events persist + rotate + read newest-first. + +```bash +docker run --rm -v /tmp/ws008-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 failure count +``` + +### Both board targets build + +```bash +for b in rev1_devkit rev2; do + docker run --rm -v /tmp/ws008-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 && \ + grep -qx 'CONFIG_BOARD_${b^^}=y' sdkconfig || grep -q CONFIG_BOARD sdkconfig" +done +``` + +- **[CI]** both targets build; `idf.py size` confirms the app still fits the 1.5 MiB OTA slot; + `dependencies.lock` + both `esp-modbus` pins unchanged (only IDF built-ins added). + +## HIL validation (rev1 rig — Checkpoint 3) + +1. **Correct Swedish time + sync status**: boot networked → console `time` first shows "not set", then + correct CET/CEST local time after sync; sync status reports synced + last-sync. +2. **Watchdog reboot + pumps safe**: starve a subscribed critical task (debug hook/build flag) → device + reboots within the timeout → pumps measured OFF immediately after reset → `storage events` shows + `reset=TASK_WDT`. +3. **No spurious reboot**: extended normal run triggers no watchdog reset. +4. **Event log persists across power-cycle**: generate pump start/stop (console `pump ...`) + a WiFi drop → + power-cycle → `storage events` still lists them with timestamps + causes. +5. **WiFi outage does not reboot**: pull WiFi during operation → device keeps running, no watchdog reset, + watering unaffected; a `wifi=Reconnecting`/`wifi=Connected` event pair is logged. +6. **Non-fatal SNTP**: block NTP (no internet) → device runs in "time not set" indefinitely, no crash. + +## Definition of done + +- Host suite green (0 failures); both board targets build; app fits the OTA slot. +- HIL checklist (`specs/008-sntp-watchdog-logging/checklists/hil.md`) executed on the rig, or explicitly + deferred with rationale if no rig time (deferred-HIL register). +- `firmware/CLAUDE.md` gains a short SNTP/watchdog/event-log section. diff --git a/specs/008-sntp-watchdog-logging/research.md b/specs/008-sntp-watchdog-logging/research.md new file mode 100644 index 0000000..b5b41c4 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/research.md @@ -0,0 +1,94 @@ +# Phase 0 Research: SNTP Time, Task Watchdog & Event Logging + +Grounded in the verified codebase map (origin/main, PR-06 + PR-07 merged) and the pre-made decisions in +`docs/prd/PR-08-*.md` + `docs/parity-checklist.md`. No open `NEEDS CLARIFICATION`. + +## D1 — Event log reuses PR-06 `IDataStorage` (no interface extension) + +- **Decision**: Wire PR-08's events into the existing `IDataStorage::storeEvent(epoch, category, detail)` / + `getEvents(n)`. Categories are already defined (`kCategoryPump/Failsafe/Connectivity/Ota/Reset`, open + uint8); `detail` (≤120 B) carries the cause; caller supplies the epoch. `storage events [n]` already + reads it over serial. +- **Rationale**: The surface fully covers PR-08's needs (research §1). Inventing new storage would + duplicate PR-06 and diverge the rotation logic. `LockedDataStorage` already provides cross-task safety. +- **Alternatives**: A new event API — rejected (redundant); a severity field — rejected (category is the + only axis; severity can be encoded in the detail prefix if ever needed). + +## D2 — SNTP sets system time; a thin `IWallClock` seam makes epoch testable + +- **Decision**: Start `esp_netif_sntp` against `CONFIG_WS_SNTP_SERVER` (default `se.pool.ntp.org`) after + the STA connects; `setenv("TZ", "CET-1CEST,M3.5.0,M10.5.0/3")` + `tzset()` at init so `localtime_r` + yields Swedish local time. Introduce `IWallClock { uint32_t nowEpoch(); bool isTimeSet(); }` — target + impl over `time(nullptr)`, host `FakeWallClock` — so consumers (EventLogger, console) get a testable + epoch source. System time set by SNTP also makes the existing raw `time(nullptr)` call sites correct. +- **Rationale**: `ITimeProvider` is monotonic-ms only (research §2) — no wall-clock seam exists. TZ/DST + conversion is host-testable with `setenv("TZ")`+`localtime_r` (available on the linux host). Keeping the + epoch behind `IWallClock` lets EventLogger tests be deterministic. +- **Alternatives**: Extend `ITimeProvider` with epoch — rejected (conflates monotonic vs wall-clock, and + its many existing consumers don't want wall-clock); SNTP smooth-slew — rejected, use step-set + (immediate) so time becomes correct in one jump (simpler; the log tolerates the discontinuity). + +## D3 — Explicit time-not-set via plausibility threshold + +- **Decision**: `isTimeSet()` = current epoch ≥ a plausible minimum (year ≥ 2020, matching the legacy + plausibility check). Before first sync the state is "not set"; consumers (PR-11 schedule) gate on it. +- **Rationale**: Parity §7 (legacy waited for year ≥ 2020). A pure predicate is trivially host-tested and + prevents acting on a 1970 clock (FR-004, safety-relevant for PR-11). + +## D4 — Watchdog: subscribe watering-critical tasks; exclude WiFi; action = reboot + +- **Decision**: Enable `esp_task_wdt` (config via sdkconfig + `WS_TASK_WDT_TIMEOUT_S`), subscribe the + **10 Hz `app_main` main loop** (it drives pump `update()`) and the **sensor task**; each calls + `esp_task_wdt_add(NULL)` + `esp_task_wdt_reset()` in its loop. The **WiFi task is NOT subscribed**. On a + non-servicing task the watchdog panics → reboot; `pumps_force_off()` (unchanged) runs first at the next + boot. +- **Rationale**: The watering-critical loops are the main loop (pump/level `update()`) and the sensor task; + a hang there must self-heal. The WiFi task must never reboot the device (PR-07 isolation, FR-014) — a + network stall is not a watering failure. Replaces the ineffective legacy software watchdog (QUIRK 3). + Timeout margin: sensor task services every 5 s, so a default well above that (e.g. 15–30 s) avoids false + positives while still catching a genuine hang. +- **Alternatives**: Subscribe all tasks incl. WiFi — rejected (a WiFi stall would reboot and interrupt + watering); watchdog = log-only — rejected (no auto-recovery; the PRD requires reboot). +- **Scope note**: PR-11's watering/reservoir-control tasks subscribe through the same helper when they land + (recorded so it is not a review finding). + +## D5 — Reset reason captured at boot → event + +- **Decision**: Call `esp_reset_reason()` early in `app_main` (after storage is up) and log it via + `EventLogger` as a `kCategoryReset` event (mapping POWERON/SW/PANIC/INT_WDT/TASK_WDT/BROWNOUT/… to a + short detail string). Epoch is the best-known (pre-sync) time at boot. +- **Rationale**: FR-009 / acceptance criterion. `esp_reset_reason` is not called anywhere today (research + §3). Logging pre-sync is acceptable (D2/edge case) — the reset fact matters more than its exact epoch. + +## D6 — Event producers wired at snapshot consumers (isolation-safe) + +- **Decision**: A `system_observer` (polled from the main loop, holding `LockedDataStorage` + the + `WifiManager*` + the pump handles) detects and logs: WiFi state transitions (from `snapshot().state`), + and pump start/stop transitions with cause. It also kicks off SNTP once on the first `Connected`. The + reset event is logged directly at boot. +- **Rationale**: `WifiManager` structurally holds no storage (FR-014) and exposes no observer hook + (research §4) — state-change logging must live at a consumer that already reads `snapshot()`. The main + loop already polls pumps and can hold storage. Fail-safe-activation events (sensor-invalid → pump stop) + are produced by the **PR-11 controller**; PR-08 defines the `kCategoryFailsafe` logging path and the + `EventLogger` helper so PR-11 just calls it (scope note). +- **Alternatives**: Inject `IDataStorage` into `WifiManager` — rejected (breaks FR-014 isolation); a global + event bus — rejected (over-engineered for a handful of producers). + +## D7 — `EventLogger` is a thin, host-tested formatting layer over `IDataStorage` + +- **Decision**: `EventLogger` composes an `IDataStorage&` + `IWallClock&` and offers typed helpers + (`logReset(reason)`, `logWifi(state)`, `logPumpStart/Stop(pump, cause)`, `logFailsafe(detail)`, + `logOta(detail)`), each building the `detail` string + category and calling `storeEvent(nowEpoch, ...)`. + A storage-write `false` is counted/`ESP_LOGW`'d, never thrown. +- **Rationale**: Centralizes the category+detail format (host-tested against `MockDataStorage`), keeps + producers terse, and contains write failures (FR-014). Pure-enough for host tests (the IDF-free parts). + +## Open risks + +- **R1 — SNTP reachability at HIL**: greenhouse network/DNS must resolve the pool; failure is non-fatal by + design (FR-001). Verify at HIL. +- **R2 — Watchdog false positive**: if a legitimately slow operation exceeds the timeout, tune + `WS_TASK_WDT_TIMEOUT_S`. Chosen default leaves margin over the 5 s sensor cadence. +- **R3 — Binary size**: SNTP + task-wdt are small IDF built-ins; confirm the app still fits 1.5 MiB. +- **R4 — Clock step & getEvents ordering**: `getEvents` assumes monotonic epochs; a pre-sync→synced jump + can interleave ordering. Acceptable (entries are insertion-ordered on disk); documented in the edge case. diff --git a/specs/008-sntp-watchdog-logging/spec.md b/specs/008-sntp-watchdog-logging/spec.md new file mode 100644 index 0000000..9d856b4 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/spec.md @@ -0,0 +1,230 @@ +# Feature Specification: SNTP Time, Task Watchdog & Event Logging + +**Feature Branch**: `008-sntp-watchdog-logging` + +**Created**: 2026-07-04 + +**Status**: Draft + +**Input**: PR-08 (`docs/prd/PR-08-sntp-watchdog-logging.md`) — completes the phase-2 exit criteria: +correct wall-clock time (SNTP, Swedish pool, CET/CEST), a hardware task watchdog on critical tasks, and +structured logging with persistence of important events. Depends on PR-06 (littlefs event storage) and +PR-07 (WiFi network). Parity: `docs/parity-checklist.md` §7 (NTP), QUIRK 2/3, §6 (event log surface). + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Correct local time after boot, safe before sync (Priority: P1) + +Once the device is on the network it learns the correct wall-clock time from the internet and keeps it, +reported in Swedish local time (CET in winter, CEST in summer) wherever a person reads it. Before the +first successful sync the device is explicitly in a "time-not-set" state that the rest of the system can +detect, so nothing that depends on the calendar/clock misfires on a bogus 1970 time. + +**Why this priority**: Correct, monotonic timestamps underpin every logged event and every future +schedule decision. The explicit not-yet-synced state is the safety-relevant part: watering schedule logic +(PR-11) must never act on an unset clock. + +**Independent Test**: Boot a networked device; confirm it reports "time not set" until sync, then reports +correct Swedish local time with the right DST offset; confirm timezone conversion is correct across a DST +boundary (host-tested with fixed epochs). + +**Acceptance Scenarios**: + +1. **Given** a freshly booted device that has not yet synced, **When** its time state is queried, **Then** + it reports "time not set" and any consumer can detect that state (rather than reading a 1970/epoch-zero + time as if valid). +2. **Given** the device has joined the network, **When** SNTP obtains a plausible time from the Swedish + pool, **Then** the device transitions to "time set", records the sync, and reports correct Swedish + local time. +3. **Given** a known epoch on a winter date and one on a summer date, **When** each is converted to local + time, **Then** the winter time shows the CET offset and the summer time shows the CEST offset (DST + rules for Europe/Stockholm). +4. **Given** SNTP cannot reach the server, **When** sync fails, **Then** the device keeps running normally + (non-fatal), stays in "time not set", and keeps retrying in the background. +5. **Given** any point in time, **When** an internal timestamp is recorded, **Then** it is epoch seconds + and monotonic across the migration (user-facing displays convert to local time; storage stays epoch). + +--- + +### User Story 2 - Persistent record of important events (Priority: P2) + +Important system events are written to a durable, bounded log that survives power loss, each entry +carrying a timestamp and a cause. An operator can read recent events over the serial console to +understand what the device did and why — including why it last restarted. + +**Why this priority**: This is the observability backbone and the safety audit trail. It captures the +fail-safe activations and the reset reason that make watchdog/brownout events diagnosable after the fact. +It reuses the durable event-log storage already provided by PR-06. + +**Independent Test**: Trigger several loggable events (pump start/stop with cause, a sensor-failure +fail-safe activation, a WiFi state change), read them back over the serial console with timestamps and +causes; power-cycle and confirm the log persists; confirm rotation bounds the size (host-tested against +mock storage). + +**Acceptance Scenarios**: + +1. **Given** a loggable event occurs (pump start/stop with cause, sensor-failure fail-safe activation, + WiFi state change), **When** it is recorded, **Then** the entry carries a timestamp and the cause and + is readable over the serial diagnostic console. +2. **Given** the device just booted, **When** the boot completes, **Then** the reset reason (normal, + watchdog, brownout, panic, etc.) is captured and persisted as an event. +3. **Given** the log has reached its size bound, **When** new events are written, **Then** older entries + are rotated out (bounded size) and the newest events are always retained. +4. **Given** events were logged before a power cut, **When** the device is powered back on, **Then** the + previously logged events are still present. +5. **Given** the clock is not yet set when an event occurs, **When** it is recorded, **Then** the entry is + still logged (with the best timestamp available / marked as pre-sync) rather than dropped. + +--- + +### User Story 3 - Automatic recovery from a hung task, pumps safe (Priority: P3) + +If a critical task hangs, the device reboots itself automatically instead of freezing; after that reboot +the pumps are OFF, and the reason for the restart is recorded so the operator can see it happened. + +**Why this priority**: Reliability and safety net. A frozen controller must not leave a pump running or +the greenhouse unattended; hardware watchdog-forced recovery + the boot fail-safe together guarantee a +safe, self-healing restart. + +**Independent Test**: Deliberately starve a registered critical task on the rig; confirm the device +reboots, the pumps are measured OFF immediately after the reset, and the reset reason is persisted in the +event log. + +**Acceptance Scenarios**: + +1. **Given** a critical task is registered with the watchdog, **When** that task stops servicing the + watchdog for longer than the timeout, **Then** the device reboots automatically. +2. **Given** a watchdog-induced reboot, **When** the device comes back up, **Then** both pump outputs are + OFF immediately at boot (existing fail-safe invariant), before any task starts. +3. **Given** a watchdog (or brownout/panic) reset occurred, **When** the device reboots, **Then** the + reset reason is captured and persisted to the event log. +4. **Given** all critical tasks are servicing the watchdog normally, **When** the system runs, **Then** + the watchdog never fires (no spurious reboots). + +--- + +### Edge Cases + +- **Clock never syncs (offline device)**: device runs indefinitely in "time not set"; events still log; + schedule logic (PR-11) treats not-set as "do not act on calendar/clock". +- **Clock jumps forward on first sync**: timestamps recorded before sync were low/pre-sync; after sync + they are correct — the log tolerates the discontinuity (entries are ordered by insertion, timestamps + reflect best-known time at write). +- **DST transition instant**: the spring-forward/fall-back boundaries convert correctly (the ambiguous/ + skipped local hour follows the standard POSIX TZ rule). +- **WiFi task hangs**: a WiFi/network stall must NOT reboot the device (watering must be unaffected) — the + watchdog covers watering-critical tasks, not the network task (see Assumptions). +- **Event written while storage is full/rotating**: rotation makes room; a storage write failure is + logged/counted but never crashes the device or affects watering. +- **Repeated watchdog reboots (boot loop risk)**: each reboot is safe (pumps OFF) and the reset reason is + recorded each time so the loop is diagnosable. + +## Requirements *(mandatory)* + +### Functional Requirements + +**Time / SNTP (FR10)** + +- **FR-001**: The system MUST obtain wall-clock time via SNTP from the Swedish NTP pool after the WiFi + station connects; failure to reach the server MUST be non-fatal (the device keeps running and keeps + retrying). +- **FR-002**: The system MUST apply the Europe/Stockholm timezone with correct CET/CEST daylight-saving + rules so that user-facing times are shown in Swedish local time. +- **FR-003**: Internal timestamps MUST be epoch seconds and remain monotonic across the migration; + conversion to local time happens only for user-facing output. +- **FR-004**: The system MUST expose an explicit "time-not-set" state (no plausible time yet) that any + consumer can detect, and MUST NOT let a pre-sync placeholder time be treated as valid wall-clock time. +- **FR-005**: The system MUST expose sync status (synced / not-yet-synced, and the time of last successful + sync) for status reporting and the serial diagnostic console. + +**Task watchdog** + +- **FR-006**: The system MUST register a hardware task watchdog on all watering-critical tasks (the + environmental sensor task today; the watering/control and reservoir-control tasks arriving in PR-11 — the + registration mechanism MUST be ready for them). +- **FR-007**: When a registered critical task fails to service the watchdog within the timeout, the system + MUST reboot automatically. +- **FR-008**: After a watchdog-induced reset, both pump outputs MUST be OFF immediately at boot, before any + task starts (existing fail-safe invariant — pumps off at boot / after watchdog reset / after OTA restart). +- **FR-009**: The system MUST capture the reset reason at boot and persist it to the event log. + +**Logging & event persistence** + +- **FR-010**: The system MUST maintain a persistent, bounded, rotating event log (reusing PR-06 storage) + that survives power loss; the newest events are always retained when older ones are rotated out. +- **FR-011**: Each event entry MUST carry a timestamp and a cause/detail. Logged event categories MUST + include: pump start/stop with cause, sensor-failure fail-safe activations, WiFi state changes, reset + reasons (watchdog/brownout/panic/normal), and a defined surface for OTA events (emitted by PR-13). +- **FR-012**: The event log MUST be readable over the serial diagnostic console. (HTTP/API exposure is + out of scope — PR-09.) +- **FR-013**: Per-component log verbosity MUST be configurable (build-time configuration). +- **FR-014**: Logging and event persistence MUST NOT block or affect watering; a storage failure while + logging MUST be contained (logged/counted) and never crash the device or disturb the watering path. + +### Key Entities *(include if feature involves data)* + +- **Time state**: synced flag, last-sync epoch, current epoch; "not set" when no plausible time yet. + Plausibility threshold (a minimum reasonable year) distinguishes set from not-set. +- **Event log entry**: timestamp (epoch, best-known at write), category (pump / fail-safe / wifi / reset / + ota), and a cause/detail string. Stored in the durable rotating event log from PR-06. +- **Reset reason**: the hardware-reported cause of the last boot (normal, watchdog, brownout, panic, + power-on, software), captured once at boot and logged. +- **Watchdog registration**: the set of critical tasks subscribed to the hardware task watchdog and the + timeout after which a non-servicing task forces a reboot. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: After boot on a working network, the device shows correct Swedish local time (correct DST + offset) and reports "synced"; before that it reports "time not set" — never a bogus 1970 time treated as + valid. +- **SC-002**: Timezone conversion is correct on both sides of a DST boundary (verified for winter and + summer epochs without any hardware). +- **SC-003**: A deliberately hung critical task causes an automatic reboot within the watchdog timeout, + and after that reboot the pumps are measured OFF and the reset reason appears in the event log. +- **SC-004**: The event log survives a power cycle: events logged before the cut (with timestamps and + causes) are present after power-on. +- **SC-005**: The event log stays within its size bound under sustained event volume, always retaining the + most recent events (verified without hardware against mock storage). +- **SC-006**: A network/SNTP outage or a storage-write failure never reboots the device on its own and + never interrupts an active watering cycle. +- **SC-007**: Both firmware board targets build with the feature included. + +## Assumptions + +- **SNTP server**: the Swedish pool `se.pool.ntp.org` is the default (the legacy unit used + `0.se.pool.ntp.org`; the pooled hostname is kept). The server is a documented, build-configurable value. +- **Time-not-set detection**: a plausible-time threshold (minimum year, e.g. 2020 as the legacy unit used) + distinguishes an unset clock from a synced one — matching the legacy plausibility check. +- **Watchdog scope now vs later**: only the environmental sensor task exists as a watering-relevant task + today; the watering/control and reservoir-control tasks land in PR-11. PR-08 delivers the watchdog + registration mechanism and registers the currently-existing critical task(s); PR-11 registers its tasks + through the same mechanism. This scoping is recorded so it does not surface as a review finding. +- **WiFi task deliberately excluded from the watchdog**: a WiFi/network stall MUST NOT reboot the device + (watering must be unaffected — the PR-07 isolation guarantee). The watchdog covers watering-critical + tasks, not the network task. +- **Watchdog action = reboot**: the watchdog is configured to force a reboot (panic/abort → restart) on a + non-servicing task, so recovery is automatic; the timeout is a documented, build-configurable value with + a safe margin over the slowest critical-task cadence (the legacy software watchdog used 30 s but was + ineffective — QUIRK 3). +- **Event log storage reuse**: the durable, bounded, rotating event log is the one delivered by PR-06 + (`IDataStorage` event log); PR-08 defines the event categories and wires the producers into it rather + than inventing new storage. +- **Pre-sync timestamps**: events logged before the first sync use the best available time (low/pre-sync + epoch) and are still recorded, never dropped; ordering is by insertion. + +### Dependencies + +- **PR-06 (littlefs / `IDataStorage`)** — the durable rotating event log this feature writes to. +- **PR-07 (WiFi network)** — SNTP starts after the station connects; WiFi state changes are a logged + event source. +- **Board fail-safe (pumps OFF at boot)** — the existing `app_main` invariant that FR-008 relies on after + a watchdog reset; this feature reaffirms and records the reset reason, it does not change the fail-safe. + +### Out of Scope + +- The watering schedule itself and its use of the clock/not-set state (PR-11). +- HTTP/API exposure of the event log or time/sync status (PR-09). +- OTA event sources (PR-13 emits into the event-log surface defined here). +- Migrating any legacy log/history data (the event log is a new surface, no legacy equivalent). diff --git a/specs/008-sntp-watchdog-logging/tasks.md b/specs/008-sntp-watchdog-logging/tasks.md new file mode 100644 index 0000000..75ccbc2 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/tasks.md @@ -0,0 +1,188 @@ +--- + +description: "Task list for SNTP time, task watchdog & event logging (PR-08)" +--- + +# Tasks: SNTP Time, Task Watchdog & Event Logging + +**Input**: Design documents from `specs/008-sntp-watchdog-logging/` + +**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/ + +**Tests**: INCLUDED (Constitution II; CI host suite is the merge gate). Pure logic is test-first. Hardware +paths (`SntpClient`, `esp_task_wdt` wiring, `esp_reset_reason`) are excluded from the linux build and +HIL-verified. + +**Organization**: by user story — US1 time (P1), US2 event log (P2), US3 watchdog (P3). + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: parallelizable (different files, no incomplete dependency) +- **[Story]**: US1 / US2 / US3 (setup/foundational/polish unlabeled) +- Paths under repo root; firmware in `firmware/`. + +--- + +## Phase 1: Setup (Shared Infrastructure) + +- [ ] T001 Create the `time` component skeleton: `firmware/components/time/CMakeLists.txt` with the + `if(${IDF_TARGET} STREQUAL "linux")` guard (linux: pure sources `TimeService.cpp` + include dir; target: + + `SntpClient.cpp`, `SystemWallClock.cpp` with `PRIV_REQUIRES` for the IDF SNTP/netif/esp_system comps); + `REQUIRES interfaces`. Create `include/time/`. +- [ ] T002 Add Kconfig to `firmware/main/Kconfig.projbuild` (mirror the `WS_*` style): `WS_SNTP_SERVER` + (string, default `"se.pool.ntp.org"`), `WS_TASK_WDT_TIMEOUT_S` (int, default ~20, range with a safe + margin over the 5 s sensor cadence), and a per-component log-level knob if warranted (else document + using `CONFIG_LOG_*`). Add the `CONFIG_ESP_TASK_WDT_*` settings needed in `firmware/sdkconfig.defaults` + (do NOT disable `COMPILER_OPTIMIZATION_ASSERTIONS_ENABLE`). +- [ ] T003 [P] Register two host suites: create `firmware/test_apps/host/main/test_time.cpp` and + `test_event_logger.cpp` (empty-but-linking `run_time_tests()` / `run_event_logger_tests()`); add both to + `SRCS` in `test_apps/host/main/CMakeLists.txt` (add `time` to `REQUIRES`), forward-declare + call both in + `test_main.cpp`. + +**Checkpoint**: component + host suites compile empty on both targets + linux. + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +- [ ] T004 Define `IWallClock` in `firmware/components/interfaces/include/interfaces/IWallClock.h` (pure, + no ``/IDF): `uint32_t nowEpoch() const` + `bool isTimeSet() const`; include guard + `WATERINGSYSTEM_INTERFACES_IWALLCLOCK_H`. +- [ ] T005 [P] Host fake `FakeWallClock` in `firmware/components/time/include/time/testing/FakeWallClock.h` + (settable epoch + is-set), for `EventLogger`/consumer tests. +- [ ] T006 [P] `SyncStatus` in `firmware/components/time/include/time/SyncStatus.h` (pure: `synced`, + `lastSyncEpoch`). + +**Checkpoint**: seam + fake + sync-status type available. + +--- + +## Phase 3: User Story 1 - Correct local time after boot, safe before sync (Priority: P1) 🎯 MVP + +**Goal**: SNTP sets Swedish local time after STA connect; explicit time-not-set state before sync; DST +correct. + +**Independent Test**: host DST/plausibility tests pass; on the rig the console `time` shows not-set → +correct CET/CEST after sync. + +### Tests for US1 (write first) + +- [ ] T007 [P] [US1] Host tests in `test_time.cpp`: `isPlausibleEpoch(0)`=false, 2020+ epoch=true; + `formatLocal` winter→CET(+01:00), summer→CEST(+02:00); DST boundary (last Sun Mar/Oct) flips (set + `TZ=CET-1CEST,M3.5.0,M10.5.0/3` in the test). + +### Implementation for US1 + +- [ ] T008 [US1] Implement pure `TimeService` in `firmware/components/time/include/time/TimeService.h` + + `src/TimeService.cpp`: `isPlausibleEpoch`, `formatLocal(epoch)`, `status()`/`onSynced(epoch)` per the + contract. No IDF includes; uses `` (`localtime_r`) which is host-available. +- [ ] T009 [US1] Implement `SystemWallClock` (target) in `include/time/SystemWallClock.h` + + `src/SystemWallClock.cpp`: `nowEpoch()`=`time(nullptr)`, `isTimeSet()`=`TimeService::isPlausibleEpoch`. +- [ ] T010 [US1] Implement `SntpClient` (target-only) in `include/time/SntpClient.h` + + `src/SntpClient.cpp`: `applyTimezone()` (`setenv TZ`+`tzset`), `start()` (`esp_netif_sntp_init` against + `CONFIG_WS_SNTP_SERVER`, step-set, sync callback → `SyncStatus`), idempotent, non-blocking, non-fatal. +- [ ] T011 [US1] Add a `time` console command in `firmware/main/diag_console.{h,cpp}` (mirror `wifi_cmd`): + print sync status + local time (via injected `IWallClock`/`SyncStatus`), or "time not set". Register via + a new `diag_console_register_time(...)`; nullptr-tolerant. + +**Checkpoint**: DST/plausibility green in CI; time console line ready (HIL shows real time after sync). + +--- + +## Phase 4: User Story 2 - Persistent record of important events (Priority: P2) + +**Goal**: reset reason + WiFi state changes + pump start/stop logged to the PR-06 event log with timestamp ++ cause, readable over serial, surviving power-cycle. + +**Independent Test**: `EventLogger` host tests (formatting/category/failure) pass; on the rig `storage +events` lists the events with timestamps after a power-cycle. + +### Tests for US2 (write first) + +- [ ] T012 [P] [US2] Host tests in `test_event_logger.cpp` (vs `MockDataStorage` + `FakeWallClock`): each + producer (`logReset/logWifi/logPumpStart/logPumpStop`) writes one event with the expected category + + detail; a write-failure (`MockDataStorage.failWrites`) increments the dropped counter without crashing; + reset-reason mapping is total. + +### Implementation for US2 + +- [ ] T013 [US2] Implement `EventLogger` in `firmware/main/event_logger.{h,cpp}`: composes `IDataStorage&` + + `IWallClock&`; typed producers per `contracts/event-logger.md`; `storeEvent`==false counted + + `ESP_LOGW`, never throws/blocks; never logs credential values. Reset-reason→string mapping here. +- [ ] T014 [US2] Reset-reason boot event in `firmware/main/app_main.cpp`: after storage is up, call + `esp_reset_reason()` and `EventLogger::logReset(...)` once. Keep `pumps_force_off()` first. +- [ ] T015 [US2] `system_observer` in `firmware/main/system_observer.{h,cpp}`: holds `LockedDataStorage` + (via `EventLogger`) + `WifiManager*` + pump handles; polled from the main loop. Detects WiFi + `snapshot().state` transitions → `EventLogger::logWifi`; detects pump start/stop transitions → + `EventLogger::logPumpStart/Stop` with cause. (SNTP kickoff added in T018.) +- [ ] T016 [US2] Wire the `system_observer` into the `app_main` 10 Hz loop (call its `poll()` each + iteration) and register the event-log/time consumers with the diag console. Confirm the existing + `storage events [n]` command reads these entries (enrich formatting to render local time if cheap). + +**Checkpoint**: events logged + readable; host formatting green; power-cycle persistence is HIL. + +--- + +## Phase 5: User Story 3 - Automatic recovery from a hung task, pumps safe (Priority: P3) + +**Goal**: esp_task_wdt on the watering-critical tasks; hung task → reboot; pumps OFF after; reset reason +logged. WiFi task excluded. + +**Independent Test**: HIL — starve a subscribed task → reboot → pumps OFF → `reset=TASK_WDT` logged; no +spurious reboots. + +### Implementation for US3 + +- [ ] T017 [US3] `task_watchdog` helper in `firmware/main/task_watchdog.{h,cpp}` per + `contracts/task-watchdog.md`: `watchdog_init()` (timeout from `CONFIG_WS_TASK_WDT_TIMEOUT_S`, + panic-on-timeout; `ESP_ERR_INVALID_STATE`→OK), `watchdog_subscribe_current_task()`, `watchdog_feed()`. +- [ ] T018 [US3] Subscribe + feed the watering-critical tasks: the `app_main` 10 Hz main loop + (subscribe once, `watchdog_feed()` each iteration) and `firmware/main/sensor_task.cpp` (subscribe at + task start, feed each 5 s cycle). Do NOT subscribe `wifi_task` or the console REPL. Also start SNTP once + in the `system_observer`/main loop on the first WiFi `Connected` (`SntpClient::start()` + `applyTimezone` + at init). +- [ ] T019 [US3] Confirm the boot fail-safe + reset-reason path end-to-end: `pumps_force_off()` remains the + first `app_main` action and the `reset=TASK_WDT` event (T013/T014) is emitted after a watchdog reset + (verified at HIL). No code beyond ensuring ordering/wiring is correct. + +**Checkpoint**: watchdog subscribed with correct policy; auto-recovery + pumps-safe verified at HIL. + +--- + +## Phase 6: Polish & Cross-Cutting + +- [ ] T020 Add an SNTP/watchdog/event-log section to `firmware/CLAUDE.md` (component layout, the + watchdog subscription policy, the time-not-set contract for PR-11). +- [ ] T021 `idf.py size` on both targets — confirm the app fits the 1.5 MiB OTA slot; note margin. +- [ ] T022 [P] Confirm `dependencies.lock` + both `esp-modbus` pins unchanged (only IDF built-ins added). +- [ ] T023 Author `specs/008-sntp-watchdog-logging/checklists/hil.md` from quickstart §HIL (time+sync, + watchdog reboot + pumps OFF + reset event, event-log power-cycle persistence, WiFi-outage-no-reboot, + non-fatal SNTP). Note deferral path if no rig time. +- [ ] T024 Full host suite + both board builds green (quickstart CI section). + +--- + +## Dependencies & Execution Order + +- **Setup (P1)** → **Foundational (P2, blocks all)** → US1 → US2 → US3 → Polish. +- US2's `system_observer` (T015) and US3's SNTP-start + watchdog-feed (T018) both edit the `app_main` main + loop and `system_observer` — sequence US3's T018 after US2's T015/T016. +- US1's `SyncStatus`/`SntpClient` (T010) is consumed by US3's SNTP-start wiring (T018) — US1 before US3. +- `EventLogger` (T013) is used by the reset event (T014), the observer (T015), and later PR-11 fail-safe. +- Pure/host-tested: T007 (time), T012 (event logger). Hardware/HIL: T009/T010 (sntp/wall-clock), + T017/T018 (watchdog), T014/T019 (boot/reset wiring). + +### Parallel opportunities + +- T005/T006 (foundational) parallel. T007 and T012 (tests) parallel. T022 parallel with other polish. + +--- + +## Implementation Strategy + +- **MVP = US1** (correct time + not-set state) — the base every timestamp/schedule depends on. +- Then US2 (durable event record incl. reset reason), then US3 (watchdog auto-recovery). +- Commit after each phase/logical group; verify host tests fail before implementing the pure logic. +- Build via Docker on an rsync'd `/tmp/ws008-firmware` copy; fullclean + rm sdkconfig between board builds. +- Never modify frozen legacy paths. Keep `pumps_force_off()` first in `app_main`. WiFi task stays out of + the watchdog. From 7bc4d8c364471786a6e5bf909ffa7b299b53f32a Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sat, 4 Jul 2026 18:58:45 +0200 Subject: [PATCH 2/7] =?UTF-8?q?feat(time):=20PR-08=20setup=20+=20foundatio?= =?UTF-8?q?nal=20=E2=80=94=20IWallClock=20seam,=20time=20component,=20watc?= =?UTF-8?q?hdog=20config?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1+2 of PR-08: - time component skeleton (linux-guarded CMake; header-only until US1 adds SRCS) - IWallClock pure interface (nowEpoch/isTimeSet) + FakeWallClock host fake + SyncStatus - Kconfig WS_SNTP_SERVER (se.pool.ntp.org) + WS_TASK_WDT_TIMEOUT_S (20s); sdkconfig CONFIG_ESP_TASK_WDT_INIT/PANIC (timeout reconfigured from Kconfig at init) - host time + event_logger suites registered (stubs) Verified: host tests 178/0, both board targets build + board-config verified. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../include/interfaces/IWallClock.h | 46 +++++++++++++++++ firmware/components/time/CMakeLists.txt | 36 +++++++++++++ .../components/time/include/time/SyncStatus.h | 29 +++++++++++ .../time/include/time/testing/FakeWallClock.h | 50 +++++++++++++++++++ firmware/main/Kconfig.projbuild | 25 ++++++++++ firmware/sdkconfig.defaults | 10 ++++ firmware/test_apps/host/main/CMakeLists.txt | 4 +- .../test_apps/host/main/test_event_logger.cpp | 18 +++++++ firmware/test_apps/host/main/test_main.cpp | 4 ++ firmware/test_apps/host/main/test_time.cpp | 17 +++++++ 10 files changed, 238 insertions(+), 1 deletion(-) create mode 100644 firmware/components/interfaces/include/interfaces/IWallClock.h create mode 100644 firmware/components/time/CMakeLists.txt create mode 100644 firmware/components/time/include/time/SyncStatus.h create mode 100644 firmware/components/time/include/time/testing/FakeWallClock.h create mode 100644 firmware/test_apps/host/main/test_event_logger.cpp create mode 100644 firmware/test_apps/host/main/test_time.cpp diff --git a/firmware/components/interfaces/include/interfaces/IWallClock.h b/firmware/components/interfaces/include/interfaces/IWallClock.h new file mode 100644 index 0000000..b664474 --- /dev/null +++ b/firmware/components/interfaces/include/interfaces/IWallClock.h @@ -0,0 +1,46 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file IWallClock.h + * @brief Injected wall-clock time source (epoch seconds). + * + * Separate from ITimeProvider (monotonic milliseconds for timing/safety): + * this is calendar/wall-clock time used only for stamping events and + * user-facing time display. It is injected so consumers (e.g. EventLogger) + * stay deterministic under host tests (FakeWallClock) and never call + * time()/esp_* directly. + * + * Part of the header-only `interfaces` component: no IDF and no + * includes allowed here — only . + */ + +#ifndef WATERINGSYSTEM_INTERFACES_IWALLCLOCK_H +#define WATERINGSYSTEM_INTERFACES_IWALLCLOCK_H + +#include + +/** + * @brief Wall-clock (calendar) time source in epoch seconds. + */ +class IWallClock { +public: + virtual ~IWallClock() = default; + + /** + * @brief Current wall-clock time in seconds since the Unix epoch. + * + * Returns a low/boot value (well below a plausible present-day epoch) + * until the clock has been set (e.g. by SNTP). Callers that need to + * distinguish "not yet set" MUST consult isTimeSet(). + */ + virtual uint32_t nowEpoch() const = 0; + + /** + * @brief True iff nowEpoch() is at or beyond the plausibility threshold. + * + * False before the wall clock has been set from a trusted source. + */ + virtual bool isTimeSet() const = 0; +}; + +#endif /* WATERINGSYSTEM_INTERFACES_IWALLCLOCK_H */ diff --git a/firmware/components/time/CMakeLists.txt b/firmware/components/time/CMakeLists.txt new file mode 100644 index 0000000..2884984 --- /dev/null +++ b/firmware/components/time/CMakeLists.txt @@ -0,0 +1,36 @@ +# time — wall-clock time: pure conversion/plausibility (TimeService), the +# IWallClock target implementation (SystemWallClock) and the SNTP starter +# (SntpClient). +# +# TimeService.cpp is pure C++ (epoch plausibility + Swedish local-time +# formatting via /localtime_r, which both the host and target +# provide) and builds on the linux preview target used by the host test +# suite. SystemWallClock.cpp (time(nullptr) wall clock) and SntpClient.cpp +# (esp_netif_sntp against CONFIG_WS_SNTP_SERVER + TZ/DST setup) are the only +# hardware/IDF touchpoints and are excluded from the linux build, same +# mechanism as storage/sensors/network. +# +# TODO(T008-T010): add the SRCS below as the sources are implemented, and +# uncomment the PRIV_REQUIRES on the target branch. Until then the component +# is header-only (SyncStatus.h + testing/FakeWallClock.h) so it configures +# and builds empty on both the linux and target builds. +if(${IDF_TARGET} STREQUAL "linux") + idf_component_register( + # TODO(T008): SRCS "src/TimeService.cpp" + INCLUDE_DIRS "include" + REQUIRES interfaces + ) +else() + # Target build only: SystemWallClock (IWallClock over time(nullptr)) and + # the SNTP starter. PRIV: esp_netif provides esp_netif_sntp.h, lwip the + # underlying SNTP module; esp_timer/esp_system for scheduling and reset + # reason. These headers appear only in the target-only .cpp files, never + # in this component's public headers. + idf_component_register( + # TODO(T008-T010): SRCS "src/TimeService.cpp" "src/SystemWallClock.cpp" + # "src/SntpClient.cpp" + INCLUDE_DIRS "include" + REQUIRES interfaces + # TODO(T009-T010): PRIV_REQUIRES esp_netif lwip esp_timer esp_system + ) +endif() diff --git a/firmware/components/time/include/time/SyncStatus.h b/firmware/components/time/include/time/SyncStatus.h new file mode 100644 index 0000000..1af4293 --- /dev/null +++ b/firmware/components/time/include/time/SyncStatus.h @@ -0,0 +1,29 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file SyncStatus.h + * @brief Pure value type tracking SNTP synchronisation state. + * + * Shared between TimeService (pure, host-tested) and SntpClient (target). + * No IDF and no includes — only . Consumed by the console + * `time` line to report whether the wall clock has ever been set and when. + */ + +#ifndef WATERINGSYSTEM_TIME_SYNCSTATUS_H +#define WATERINGSYSTEM_TIME_SYNCSTATUS_H + +#include + +/** + * @brief Snapshot of the wall-clock synchronisation state. + * + * @var SyncStatus::synced true once a plausible time has been set. + * @var SyncStatus::lastSyncEpoch epoch seconds of the most recent successful + * sync (0 while never synced). + */ +struct SyncStatus { + bool synced = false; + uint32_t lastSyncEpoch = 0; +}; + +#endif /* WATERINGSYSTEM_TIME_SYNCSTATUS_H */ diff --git a/firmware/components/time/include/time/testing/FakeWallClock.h b/firmware/components/time/include/time/testing/FakeWallClock.h new file mode 100644 index 0000000..2ee3738 --- /dev/null +++ b/firmware/components/time/include/time/testing/FakeWallClock.h @@ -0,0 +1,50 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file FakeWallClock.h + * @brief Deterministic fake wall clock for host tests (header-only). + * + * Implements IWallClock with a settable epoch. "is set" is derived from a + * settable plausibility threshold (default: the 2020-01-01Z epoch, matching + * TimeService::kMinPlausibleEpoch) so consumer tests (EventLogger, the time + * console line) can exercise both the not-yet-set and synced states without + * any IDF or real clock. Never compiled into target builds — included only + * from host test code. No IDF includes. + */ + +#ifndef WATERINGSYSTEM_TIME_TESTING_FAKEWALLCLOCK_H +#define WATERINGSYSTEM_TIME_TESTING_FAKEWALLCLOCK_H + +#include + +#include "interfaces/IWallClock.h" + +/** + * @brief Manually settable wall clock for deterministic host tests. + */ +class FakeWallClock : public IWallClock { +public: + /// 2020-01-01T00:00:00Z — mirrors TimeService::kMinPlausibleEpoch. + static constexpr uint32_t kDefaultThreshold = 1577836800u; + + /// Start unset (epoch 0) unless a starting epoch is supplied. + explicit FakeWallClock(uint32_t startEpoch = 0, + uint32_t setThreshold = kDefaultThreshold) + : epoch_(startEpoch), threshold_(setThreshold) {} + + uint32_t nowEpoch() const override { return epoch_; } + + bool isTimeSet() const override { return epoch_ >= threshold_; } + + /// Set the current epoch (e.g. simulate an SNTP sync). + void setEpoch(uint32_t epoch) { epoch_ = epoch; } + + /// Adjust the "is set" plausibility threshold. + void setThreshold(uint32_t threshold) { threshold_ = threshold; } + +private: + uint32_t epoch_; + uint32_t threshold_; +}; + +#endif /* WATERINGSYSTEM_TIME_TESTING_FAKEWALLCLOCK_H */ diff --git a/firmware/main/Kconfig.projbuild b/firmware/main/Kconfig.projbuild index 5ff2775..3246d51 100644 --- a/firmware/main/Kconfig.projbuild +++ b/firmware/main/Kconfig.projbuild @@ -74,4 +74,29 @@ menu "WateringSystem" help Health-check cadence while connected; suspended in provisioning/AP mode (parity default 5 s). + + config WS_SNTP_SERVER + string "SNTP server hostname" + default "se.pool.ntp.org" + help + NTP pool hostname queried once the WiFi station first reaches + Connected (feature 008). The Swedish pool by default. SNTP runs + outside the watering path and never blocks; failure to reach the + server is non-fatal and retried by the SNTP service. Until the + first successful sync the wall clock reports "time not set". + + config WS_TASK_WDT_TIMEOUT_S + int "Task watchdog timeout (seconds)" + default 20 + range 6 120 + help + Timeout for the hardware task watchdog applied at runtime to the + watering-critical tasks (the 10 Hz main loop and the sensor + task; the WiFi task is deliberately NOT subscribed so a network + stall cannot reboot the device). A non-serviced subscribed task + triggers a panic reboot; pumps are forced OFF at boot and the + reset reason is logged (feature 008). Keep a safe margin over the + slowest critical-task cadence (the sensor task's 5 s cycle) so a + healthy task is never falsely tripped — hence the >= 6 s floor + and 20 s default. endmenu diff --git a/firmware/sdkconfig.defaults b/firmware/sdkconfig.defaults index 2239eed..0ad26b6 100644 --- a/firmware/sdkconfig.defaults +++ b/firmware/sdkconfig.defaults @@ -24,3 +24,13 @@ CONFIG_LOG_DEFAULT_LEVEL_INFO=y # disabling assertions (NDEBUG) silently turns those checks into no-ops. # Never switch this to SILENT/DISABLE for "production" builds. CONFIG_COMPILER_OPTIMIZATION_ASSERTIONS_ENABLE=y + +# Task watchdog (feature 008): initialise the hardware task WDT at startup +# and panic (reboot) on timeout, so a hung watering-critical task forces a +# safe reboot (pumps OFF at boot + reset reason logged). The watering tasks +# are subscribed at runtime (task_watchdog helper); the WiFi task is +# deliberately left unsubscribed. The effective timeout is driven from code +# via CONFIG_WS_TASK_WDT_TIMEOUT_S (esp_task_wdt_reconfigure at init), so the +# IDF init timeout below is left at its default and not overridden here. +CONFIG_ESP_TASK_WDT_INIT=y +CONFIG_ESP_TASK_WDT_PANIC=y diff --git a/firmware/test_apps/host/main/CMakeLists.txt b/firmware/test_apps/host/main/CMakeLists.txt index 548a037..62cece9 100644 --- a/firmware/test_apps/host/main/CMakeLists.txt +++ b/firmware/test_apps/host/main/CMakeLists.txt @@ -8,10 +8,12 @@ idf_component_register( "test_level_sensor.cpp" "test_ina226.cpp" "test_wifi.cpp" + "test_time.cpp" + "test_event_logger.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 + REQUIRES unity actuators interfaces storage nvs_flash sensors board network time ) diff --git a/firmware/test_apps/host/main/test_event_logger.cpp b/firmware/test_apps/host/main/test_event_logger.cpp new file mode 100644 index 0000000..ab49c30 --- /dev/null +++ b/firmware/test_apps/host/main/test_event_logger.cpp @@ -0,0 +1,18 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file test_event_logger.cpp + * @brief Host suite for the EventLogger producers (feature 008). + * + * Registered by test_main.cpp via run_event_logger_tests(). Empty for now — + * the per-producer formatting/category, write-failure counting and + * reset-reason mapping tests (against MockDataStorage + FakeWallClock) are + * added in T012 alongside the EventLogger implementation (T013). + */ + +#include "unity.h" + +// TODO(T012): register EventLogger formatting/category/failure tests here. +void run_event_logger_tests(void) +{ +} diff --git a/firmware/test_apps/host/main/test_main.cpp b/firmware/test_apps/host/main/test_main.cpp index c0ac4db..9cee655 100644 --- a/firmware/test_apps/host/main/test_main.cpp +++ b/firmware/test_apps/host/main/test_main.cpp @@ -21,6 +21,8 @@ void run_bme280_tests(void); void run_level_sensor_tests(void); void run_ina226_tests(void); void run_wifi_tests(void); +void run_time_tests(void); +void run_event_logger_tests(void); // Unity requires setUp/tearDown definitions (shared by all suites). extern "C" void setUp(void) {} @@ -37,5 +39,7 @@ extern "C" void app_main(void) run_level_sensor_tests(); run_ina226_tests(); run_wifi_tests(); + run_time_tests(); + run_event_logger_tests(); std::exit(UNITY_END()); } diff --git a/firmware/test_apps/host/main/test_time.cpp b/firmware/test_apps/host/main/test_time.cpp new file mode 100644 index 0000000..58a36a5 --- /dev/null +++ b/firmware/test_apps/host/main/test_time.cpp @@ -0,0 +1,17 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file test_time.cpp + * @brief Host suite for the pure time logic (feature 008). + * + * Registered by test_main.cpp via run_time_tests(). Empty for now — the + * TimeService plausibility + CET/CEST DST-conversion tests are added in + * T007 alongside the TimeService implementation (T008). + */ + +#include "unity.h" + +// TODO(T007): register TimeService plausibility + DST-conversion tests here. +void run_time_tests(void) +{ +} From 6c2a48c25e11742d72d6d029bb50188d0eabd312 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sat, 4 Jul 2026 22:32:28 +0200 Subject: [PATCH 3/7] =?UTF-8?q?feat(time):=20PR-08=20US1=20=E2=80=94=20SNT?= =?UTF-8?q?P=20wall-clock=20time=20(CET/CEST)=20+=20time-not-set=20state?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 / US1: - TimeService (pure): isPlausibleEpoch (>=2020) + formatLocal (localtime_r/strftime), host-tested for CET(+0100)/CEST(+0200) incl. the spring DST boundary - SystemWallClock (target): IWallClock over time(nullptr) - SntpClient (target): esp_netif_sntp against CONFIG_WS_SNTP_SERVER, step-set, sync callback updates SyncStatus; idempotent, non-blocking, non-fatal; TZ applied - diag console 'time' command + diag_console_register_time (app_main wiring deferred to US3) Verified: host 181/0 (+3), both board targets build + verified; SNTP links with PRIV_REQUIRES esp_netif. Co-Authored-By: Claude Opus 4.8 (1M context) --- firmware/components/time/CMakeLists.txt | 22 ++--- .../components/time/include/time/SntpClient.h | 67 +++++++++++++ .../time/include/time/SystemWallClock.h | 30 ++++++ .../time/include/time/TimeService.h | 50 ++++++++++ firmware/components/time/src/SntpClient.cpp | 95 +++++++++++++++++++ .../components/time/src/SystemWallClock.cpp | 22 +++++ firmware/components/time/src/TimeService.cpp | 24 +++++ firmware/main/CMakeLists.txt | 2 +- firmware/main/diag_console.cpp | 57 +++++++++++ firmware/main/diag_console.h | 18 ++++ firmware/test_apps/host/main/test_time.cpp | 66 ++++++++++++- 11 files changed, 434 insertions(+), 19 deletions(-) create mode 100644 firmware/components/time/include/time/SntpClient.h create mode 100644 firmware/components/time/include/time/SystemWallClock.h create mode 100644 firmware/components/time/include/time/TimeService.h create mode 100644 firmware/components/time/src/SntpClient.cpp create mode 100644 firmware/components/time/src/SystemWallClock.cpp create mode 100644 firmware/components/time/src/TimeService.cpp diff --git a/firmware/components/time/CMakeLists.txt b/firmware/components/time/CMakeLists.txt index 2884984..d5aaccc 100644 --- a/firmware/components/time/CMakeLists.txt +++ b/firmware/components/time/CMakeLists.txt @@ -10,27 +10,23 @@ # hardware/IDF touchpoints and are excluded from the linux build, same # mechanism as storage/sensors/network. # -# TODO(T008-T010): add the SRCS below as the sources are implemented, and -# uncomment the PRIV_REQUIRES on the target branch. Until then the component -# is header-only (SyncStatus.h + testing/FakeWallClock.h) so it configures -# and builds empty on both the linux and target builds. +# TimeService.cpp is pure and builds on both targets. SystemWallClock.cpp and +# SntpClient.cpp are the hardware/IDF touchpoints (esp_netif_sntp) and are +# excluded from the linux build; esp_netif provides esp_netif_sntp.h. Those +# headers appear only in the target-only .cpp files, never in public headers. if(${IDF_TARGET} STREQUAL "linux") idf_component_register( - # TODO(T008): SRCS "src/TimeService.cpp" + SRCS "src/TimeService.cpp" INCLUDE_DIRS "include" REQUIRES interfaces ) else() - # Target build only: SystemWallClock (IWallClock over time(nullptr)) and - # the SNTP starter. PRIV: esp_netif provides esp_netif_sntp.h, lwip the - # underlying SNTP module; esp_timer/esp_system for scheduling and reset - # reason. These headers appear only in the target-only .cpp files, never - # in this component's public headers. idf_component_register( - # TODO(T008-T010): SRCS "src/TimeService.cpp" "src/SystemWallClock.cpp" - # "src/SntpClient.cpp" + SRCS "src/TimeService.cpp" + "src/SystemWallClock.cpp" + "src/SntpClient.cpp" INCLUDE_DIRS "include" REQUIRES interfaces - # TODO(T009-T010): PRIV_REQUIRES esp_netif lwip esp_timer esp_system + PRIV_REQUIRES esp_netif ) endif() diff --git a/firmware/components/time/include/time/SntpClient.h b/firmware/components/time/include/time/SntpClient.h new file mode 100644 index 0000000..6133b63 --- /dev/null +++ b/firmware/components/time/include/time/SntpClient.h @@ -0,0 +1,67 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file SntpClient.h + * @brief SNTP wall-clock synchroniser + timezone setup — target only. + * + * Thin, non-blocking starter around esp_netif_sntp. applyTimezone() installs + * the Swedish TZ (CET-1CEST,M3.5.0,M10.5.0/3) once; start() launches the SNTP + * service against CONFIG_WS_SNTP_SERVER in step-set mode and is idempotent and + * non-fatal (server unreachable is retried by the SNTP service, never a boot + * failure). A static sync callback updates the client's SyncStatus on each + * successful sync. Excluded from the linux host build (esp_* dependencies). + */ + +#ifndef WATERINGSYSTEM_TIME_SNTPCLIENT_H +#define WATERINGSYSTEM_TIME_SNTPCLIENT_H + +#include + +#include "time/SyncStatus.h" + +/** + * @brief Starts SNTP synchronisation and tracks the resulting SyncStatus. + * + * Single-instance by design: the ESP-IDF SNTP module is a process-global + * service, and the sync callback routes into the one live instance so the + * console `time` line can report sync state. + */ +class SntpClient { +public: + SntpClient(); + ~SntpClient(); + + SntpClient(const SntpClient&) = delete; + SntpClient& operator=(const SntpClient&) = delete; + + /** + * @brief Install the Swedish timezone (CET/CEST) into the process env. + * + * `setenv("TZ", "CET-1CEST,M3.5.0,M10.5.0/3", 1); tzset();`. Call once at + * init, before any local-time rendering. + */ + void applyTimezone(); + + /** + * @brief Start the SNTP service (idempotent, non-blocking, non-fatal). + * + * Initialises esp_netif_sntp against CONFIG_WS_SNTP_SERVER in step-set + * (immediate) mode with the sync callback registered. Re-invocation is a + * no-op (an already-initialised service is treated as success). Failure to + * reach the server is non-fatal — logged as a warning; the SNTP service + * keeps retrying on its own. + */ + void start(); + + /// Immutable view of the current synchronisation state. + const SyncStatus& status() const { return status_; } + +private: + /// C-ABI SNTP time-sync callback; routes to the live instance. + static void onSyncCb(struct timeval* tv); + + SyncStatus status_; + bool started_ = false; +}; + +#endif /* WATERINGSYSTEM_TIME_SNTPCLIENT_H */ diff --git a/firmware/components/time/include/time/SystemWallClock.h b/firmware/components/time/include/time/SystemWallClock.h new file mode 100644 index 0000000..d0d081a --- /dev/null +++ b/firmware/components/time/include/time/SystemWallClock.h @@ -0,0 +1,30 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file SystemWallClock.h + * @brief IWallClock over the system clock (time(nullptr)) — target only. + * + * The production wall-clock source: reads the C library system clock, which + * SNTP steps once it syncs (SntpClient, step-set mode). Excluded from the + * linux host build (host consumers use FakeWallClock); the declaration is + * trivial so it lives target-side with its .cpp. isTimeSet() reuses the pure + * TimeService plausibility threshold so it matches FakeWallClock exactly. + */ + +#ifndef WATERINGSYSTEM_TIME_SYSTEMWALLCLOCK_H +#define WATERINGSYSTEM_TIME_SYSTEMWALLCLOCK_H + +#include + +#include "interfaces/IWallClock.h" + +/** + * @brief IWallClock backed by the system clock (SNTP-stepped on target). + */ +class SystemWallClock : public IWallClock { +public: + uint32_t nowEpoch() const override; + bool isTimeSet() const override; +}; + +#endif /* WATERINGSYSTEM_TIME_SYSTEMWALLCLOCK_H */ diff --git a/firmware/components/time/include/time/TimeService.h b/firmware/components/time/include/time/TimeService.h new file mode 100644 index 0000000..6e120d9 --- /dev/null +++ b/firmware/components/time/include/time/TimeService.h @@ -0,0 +1,50 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file TimeService.h + * @brief Pure wall-clock helpers: epoch plausibility + Swedish local-time + * formatting. + * + * Pure C++ (no IDF, no esp_*): builds on both the linux preview host and the + * ESP32 target. formatLocal() renders via /localtime_r, which both + * platforms provide, and assumes the process timezone has already been set to + * `CET-1CEST,M3.5.0,M10.5.0/3` (done on target by SntpClient::applyTimezone(), + * and in the host test by setenv("TZ", ...)+tzset()). + */ + +#ifndef WATERINGSYSTEM_TIME_TIMESERVICE_H +#define WATERINGSYSTEM_TIME_TIMESERVICE_H + +#include +#include + +/** + * @brief Stateless wall-clock utilities (plausibility + local-time render). + */ +class TimeService { +public: + /// 2020-01-01T00:00:00Z — the lower bound for a "set" wall clock. Anything + /// below this is treated as an un-synced boot value (matches + /// FakeWallClock::kDefaultThreshold and IWallClock::isTimeSet()). + static constexpr uint32_t kMinPlausibleEpoch = 1577836800u; + + /** + * @brief True iff @p e is at or beyond the plausibility threshold. + * @param e Candidate epoch seconds. + */ + static bool isPlausibleEpoch(uint32_t e) { return e >= kMinPlausibleEpoch; } + + /** + * @brief Render @p epoch as Swedish local time "YYYY-MM-DD HH:MM:SS +ZZZZ". + * + * Uses the process timezone (assumed already set to + * `CET-1CEST,M3.5.0,M10.5.0/3`): winter -> CET (+0100), summer -> CEST + * (+0200), with the DST boundaries at the last Sunday of March/October. + * + * @param epoch Epoch seconds to convert. + * @return Formatted local timestamp string. + */ + static std::string formatLocal(uint32_t epoch); +}; + +#endif /* WATERINGSYSTEM_TIME_TIMESERVICE_H */ diff --git a/firmware/components/time/src/SntpClient.cpp b/firmware/components/time/src/SntpClient.cpp new file mode 100644 index 0000000..1519b66 --- /dev/null +++ b/firmware/components/time/src/SntpClient.cpp @@ -0,0 +1,95 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file SntpClient.cpp + * @brief SNTP synchroniser implementation — target-only (excluded on linux). + * + * Non-blocking, non-fatal SNTP against CONFIG_WS_SNTP_SERVER (default + * se.pool.ntp.org). The device never blocks waiting for sync; the wall clock + * stays un-set until the first successful step. The sync callback updates the + * live instance's SyncStatus so the console `time` line can report it. + */ + +#include "time/SntpClient.h" + +#include + +#include "esp_err.h" +#include "esp_log.h" +#include "esp_netif_sntp.h" +#include "esp_sntp.h" + +#include "sdkconfig.h" + +namespace { +const char* TAG = "sntp"; + +// The ESP-IDF SNTP module is a process-global service; the C-ABI callback has +// no user context, so it routes into the single live SntpClient instance. +SntpClient* s_instance = nullptr; +} // namespace + +SntpClient::SntpClient() +{ + s_instance = this; +} + +SntpClient::~SntpClient() +{ + if (started_) { + esp_netif_sntp_deinit(); + started_ = false; + } + if (s_instance == this) { + s_instance = nullptr; + } +} + +void SntpClient::applyTimezone() +{ + // Swedish local time: CET (+01) in winter, CEST (+02) in summer, DST on the + // last Sunday of March/October (matches TimeService::formatLocal()). + setenv("TZ", "CET-1CEST,M3.5.0,M10.5.0/3", 1); + tzset(); +} + +void SntpClient::start() +{ + if (started_) { + return; + } + + esp_sntp_config_t cfg = ESP_NETIF_SNTP_DEFAULT_CONFIG(CONFIG_WS_SNTP_SERVER); + cfg.start = true; + cfg.sync_cb = &SntpClient::onSyncCb; + + esp_err_t err = esp_netif_sntp_init(&cfg); + if (err == ESP_ERR_INVALID_STATE) { + // Already initialised (idempotent re-invocation) — treat as success. + started_ = true; + return; + } + if (err != ESP_OK) { + // Non-fatal: log and keep running. isTimeSet() stays false until a + // later attempt succeeds; the caller must never block on sync. + ESP_LOGW(TAG, "esp_netif_sntp_init failed: %s", esp_err_to_name(err)); + return; + } + + // Step-set (immediate) mode: apply the server time in one jump rather than + // slewing — appropriate for a device that boots with an un-set clock. + sntp_set_sync_mode(SNTP_SYNC_MODE_IMMED); + ESP_LOGI(TAG, "SNTP started against %s", CONFIG_WS_SNTP_SERVER); + started_ = true; +} + +void SntpClient::onSyncCb(struct timeval* tv) +{ + if (s_instance == nullptr || tv == nullptr) { + return; + } + s_instance->status_.synced = true; + s_instance->status_.lastSyncEpoch = static_cast(tv->tv_sec); + ESP_LOGI(TAG, "wall clock synced: epoch=%u", + static_cast(tv->tv_sec)); +} diff --git a/firmware/components/time/src/SystemWallClock.cpp b/firmware/components/time/src/SystemWallClock.cpp new file mode 100644 index 0000000..60ced86 --- /dev/null +++ b/firmware/components/time/src/SystemWallClock.cpp @@ -0,0 +1,22 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file SystemWallClock.cpp + * @brief IWallClock over time(nullptr) — target-only (excluded on linux). + */ + +#include "time/SystemWallClock.h" + +#include + +#include "time/TimeService.h" + +uint32_t SystemWallClock::nowEpoch() const +{ + return static_cast(time(nullptr)); +} + +bool SystemWallClock::isTimeSet() const +{ + return TimeService::isPlausibleEpoch(nowEpoch()); +} diff --git a/firmware/components/time/src/TimeService.cpp b/firmware/components/time/src/TimeService.cpp new file mode 100644 index 0000000..3a8d82a --- /dev/null +++ b/firmware/components/time/src/TimeService.cpp @@ -0,0 +1,24 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file TimeService.cpp + * @brief Pure implementation of the TimeService local-time formatter. + * + * No IDF / esp_* dependencies: compiled into BOTH the linux host build and the + * ESP32 target build. localtime_r honours the process timezone set elsewhere + * (SntpClient::applyTimezone() on target; setenv+tzset in host tests). + */ + +#include "time/TimeService.h" + +#include + +std::string TimeService::formatLocal(uint32_t epoch) +{ + time_t t = static_cast(epoch); + struct tm lt; + localtime_r(&t, <); + char buf[32]; + strftime(buf, sizeof buf, "%Y-%m-%d %H:%M:%S %z", <); + return std::string(buf); +} diff --git a/firmware/main/CMakeLists.txt b/firmware/main/CMakeLists.txt index 0bbc12c..2e1515d 100644 --- a/firmware/main/CMakeLists.txt +++ b/firmware/main/CMakeLists.txt @@ -3,7 +3,7 @@ idf_component_register( PRIV_REQUIRES board esp_driver_gpio esp_app_format actuators interfaces console esp_timer storage nvs_flash sensors esp_netif esp_event - network + network time ) # Build-time littlefs image of the committed seed directory diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index c5bf459..2a99448 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -90,8 +90,11 @@ #include "interfaces/IModbusClient.h" #include "interfaces/IPowerSensor.h" #include "interfaces/ISoilSensor.h" +#include "interfaces/IWallClock.h" #include "interfaces/IWaterPump.h" #include "network/WifiManager.h" +#include "time/SyncStatus.h" +#include "time/TimeService.h" namespace { @@ -158,6 +161,12 @@ IPowerSensor *s_power = nullptr; // initialization rule. The `wifi` command reads only its snapshot(). WifiManager *s_wifi = nullptr; +// Wall clock + SNTP sync status (set from app_main in US3; nullptr until then, +// and nullptr-tolerant so the `time` command reports "time not set"). Same +// trivial-initialization rule. The status is read-only (owned by SntpClient). +IWallClock *s_clock = nullptr; +const SyncStatus *s_sync = nullptr; + const char *stop_reason_str(StopReason reason) { switch (reason) { @@ -831,6 +840,32 @@ int wifi_cmd(int argc, char **argv) return 0; } +// --- time command (feature 008 US1) -------------------------------------- + +/// `time`: current Swedish local time plus SNTP sync state. Before the wall +/// clock is set (never synced, or no clock injected yet) it reports "time not +/// set" — a distinct state, never a bogus 1970 timestamp. +int time_cmd(int argc, char **argv) +{ + (void)argv; + if (argc != 1) { + printf("ERR usage: time\n"); + return 1; + } + if (s_clock == nullptr || !s_clock->isTimeSet()) { + printf("time not set\n"); + return 0; + } + const std::string now = TimeService::formatLocal(s_clock->nowEpoch()); + if (s_sync != nullptr && s_sync->synced) { + const std::string last = TimeService::formatLocal(s_sync->lastSyncEpoch); + printf("OK %s (last sync %s)\n", now.c_str(), last.c_str()); + } else { + printf("OK %s (never synced)\n", now.c_str()); + } + return 0; +} + } // namespace #if BOARD_HAS_RESERVOIR_PUMP @@ -882,6 +917,14 @@ void diag_console_register_wifi(WifiManager* manager) s_wifi = manager; } +void diag_console_register_time(IWallClock* clock, const SyncStatus* sync) +{ + // Both nullptr-tolerant: the wiring lands in US3, until then `time` reports + // "time not set". + s_clock = clock; + s_sync = sync; +} + esp_err_t diag_console_start(void) { esp_console_repl_t *repl = nullptr; @@ -1075,5 +1118,19 @@ esp_err_t diag_console_start(void) return err; } + const esp_console_cmd_t cmd_time = { + .command = "time", + .help = "time — current local time (CET/CEST) and SNTP sync status", + .hint = nullptr, + .func = &time_cmd, + .argtable = nullptr, + .func_w_context = nullptr, + .context = nullptr, + }; + err = esp_console_cmd_register(&cmd_time); + if (err != ESP_OK) { + return err; + } + return esp_console_start_repl(repl); } diff --git a/firmware/main/diag_console.h b/firmware/main/diag_console.h index c783cee..cada773 100644 --- a/firmware/main/diag_console.h +++ b/firmware/main/diag_console.h @@ -20,8 +20,10 @@ #include "interfaces/IModbusClient.h" #include "interfaces/IPowerSensor.h" #include "interfaces/ISoilSensor.h" +#include "interfaces/IWallClock.h" #include "interfaces/IWaterPump.h" #include "network/WifiManager.h" +#include "time/SyncStatus.h" /** * @brief Register the pump instances the console commands operate on. @@ -118,6 +120,22 @@ void diag_console_register_power(IPowerSensor& sensor); */ void diag_console_register_wifi(WifiManager* manager); +/** + * @brief Register the wall clock + SNTP sync status the `time` command reads + * (feature 008 US1). + * + * Both pointers are nullptr-tolerant: pass nullptr before the clock/SNTP + * client exist (the `time` command then reports "time not set"). The + * construction/wiring into app_main arrives with US3; this PR only provides + * the command and its register hook. The command reads the injected clock and + * status directly (no credentials involved). Must be called before + * diag_console_start(); plain pointer registration. + * + * @param clock Wall-clock source (SystemWallClock on target), or nullptr. + * @param sync SNTP sync status (owned by SntpClient), or nullptr. + */ +void diag_console_register_time(IWallClock* clock, const SyncStatus* sync); + /** * @brief Start the UART REPL (prompt "ws>") and register the commands. * diff --git a/firmware/test_apps/host/main/test_time.cpp b/firmware/test_apps/host/main/test_time.cpp index 58a36a5..f4e9a24 100644 --- a/firmware/test_apps/host/main/test_time.cpp +++ b/firmware/test_apps/host/main/test_time.cpp @@ -2,16 +2,72 @@ // SPDX-License-Identifier: AGPL-3.0-or-later /** * @file test_time.cpp - * @brief Host suite for the pure time logic (feature 008). + * @brief Host suite for the pure time logic (feature 008, T007). * - * Registered by test_main.cpp via run_time_tests(). Empty for now — the - * TimeService plausibility + CET/CEST DST-conversion tests are added in - * T007 alongside the TimeService implementation (T008). + * Registered by test_main.cpp via run_time_tests(). Exercises + * TimeService::isPlausibleEpoch() and the Swedish CET/CEST local-time + * formatter, including the DST spring-forward boundary. The process timezone + * is set to the Swedish rule once at suite start (mirrors + * SntpClient::applyTimezone() on target) so the +ZZZZ offset in the formatted + * string is deterministic. */ +#include +#include +#include + #include "unity.h" -// TODO(T007): register TimeService plausibility + DST-conversion tests here. +#include "time/TimeService.h" + +namespace { + +/// True iff @p s ends with @p suffix. +bool ends_with(const std::string& s, const std::string& suffix) +{ + return s.size() >= suffix.size() && + s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0; +} + +void test_plausible_epoch_threshold(void) +{ + // 0/1970 is an un-synced boot value; the 2020-01-01Z threshold is plausible. + TEST_ASSERT_FALSE(TimeService::isPlausibleEpoch(0)); + TEST_ASSERT_FALSE( + TimeService::isPlausibleEpoch(TimeService::kMinPlausibleEpoch - 1)); + TEST_ASSERT_TRUE(TimeService::isPlausibleEpoch(1577836800u)); + TEST_ASSERT_TRUE( + TimeService::isPlausibleEpoch(TimeService::kMinPlausibleEpoch)); +} + +void test_format_winter_and_summer_offsets(void) +{ + // 2024-01-01 00:00:00Z -> 01:00 CET (+0100). + TEST_ASSERT_TRUE(ends_with(TimeService::formatLocal(1704067200u), "+0100")); + // 2024-07-01 00:00:00Z -> 02:00 CEST (+0200). + TEST_ASSERT_TRUE(ends_with(TimeService::formatLocal(1719792000u), "+0200")); +} + +void test_format_dst_spring_boundary(void) +{ + // Spring-forward is the last Sunday of March (2024-03-31) at 02:00 local + // CET == 01:00 UTC, jumping to 03:00 CEST. + // 2024-03-31 01:00:00Z — at/after the switch -> CEST (+0200). + TEST_ASSERT_TRUE(ends_with(TimeService::formatLocal(1711846800u), "+0200")); + // 2024-03-31 00:00:00Z — before the switch -> CET (+0100). + TEST_ASSERT_TRUE(ends_with(TimeService::formatLocal(1711843200u), "+0100")); +} + +} // namespace + void run_time_tests(void) { + // Swedish timezone rule (mirrors SntpClient::applyTimezone()); set once so + // localtime_r renders the correct CET/CEST offset in the assertions above. + setenv("TZ", "CET-1CEST,M3.5.0,M10.5.0/3", 1); + tzset(); + + RUN_TEST(test_plausible_epoch_threshold); + RUN_TEST(test_format_winter_and_summer_offsets); + RUN_TEST(test_format_dst_spring_boundary); } From 6f13d852f7df05d63641989fcb31bab90f4728ff Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sat, 4 Jul 2026 22:57:13 +0200 Subject: [PATCH 4/7] =?UTF-8?q?feat(events):=20PR-08=20US2=20=E2=80=94=20p?= =?UTF-8?q?ersistent=20event=20log=20(reset/wifi/pump)=20+=20boot=20reset?= =?UTF-8?q?=20reason?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 4 / US2: - events component: EventLogger (pure, over IDataStorage + IWallClock) with typed producers (reset/wifi/pump start-stop/failsafe/ota), droppedEvents counter, resetReasonName mapping; reuses the PR-06 rotating event log (no interface change) - app_main: SystemWallClock + EventLogger; esp_reset_reason() -> logReset at boot - system_observer (main/): polls WifiManager snapshot + pump isRunning() from the 10 Hz loop, logs WiFi state changes + pump start/stop; null-guarded - 6 host tests (EventLogger producers/failure/resetReasonName) vs MockDataStorage - TODO(US3/T018): construct SntpClient + register the time console Verified: host 187/0 (+6), both board targets build + verified. Co-Authored-By: Claude Opus 4.8 (1M context) --- firmware/components/events/CMakeLists.txt | 14 ++ .../events/include/events/EventLogger.h | 87 ++++++++++++ .../components/events/src/EventLogger.cpp | 82 ++++++++++++ firmware/main/CMakeLists.txt | 3 +- firmware/main/app_main.cpp | 43 +++++- firmware/main/system_observer.cpp | 73 ++++++++++ firmware/main/system_observer.h | 86 ++++++++++++ firmware/test_apps/host/main/CMakeLists.txt | 2 +- .../test_apps/host/main/test_event_logger.cpp | 126 +++++++++++++++++- 9 files changed, 507 insertions(+), 9 deletions(-) create mode 100644 firmware/components/events/CMakeLists.txt create mode 100644 firmware/components/events/include/events/EventLogger.h create mode 100644 firmware/components/events/src/EventLogger.cpp create mode 100644 firmware/main/system_observer.cpp create mode 100644 firmware/main/system_observer.h diff --git a/firmware/components/events/CMakeLists.txt b/firmware/components/events/CMakeLists.txt new file mode 100644 index 0000000..61689e7 --- /dev/null +++ b/firmware/components/events/CMakeLists.txt @@ -0,0 +1,14 @@ +# events — persistent event logging (feature 008 US2). +# +# EventLogger is PURE C++: it composes an IDataStorage& and an IWallClock& +# (both from the header-only `interfaces` component) and builds the category + +# detail string for each typed producer, then calls storeEvent(). It contains +# NO IDF/esp_* code — producers take primitive args (int reset reason, const +# char* state/cause strings), so this component stays free of any network or +# IDF coupling and compiles identically on the linux host (host-tested against +# MockDataStorage + FakeWallClock) and on the target. No linux guard needed. +idf_component_register( + SRCS "src/EventLogger.cpp" + INCLUDE_DIRS "include" + REQUIRES interfaces +) diff --git a/firmware/components/events/include/events/EventLogger.h b/firmware/components/events/include/events/EventLogger.h new file mode 100644 index 0000000..6f1eb45 --- /dev/null +++ b/firmware/components/events/include/events/EventLogger.h @@ -0,0 +1,87 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file EventLogger.h + * @brief Typed producers for the PR-06 persistent event log (feature 008 US2). + * + * Composes an IDataStorage& (the cross-task LockedDataStorage when shared) and + * an IWallClock&; each producer picks the event category, builds a + * deterministic detail string and calls storage.storeEvent(clock.nowEpoch(), + * category, detail). Normative contract: + * specs/008-sntp-watchdog-logging/contracts/event-logger.md. + * + * PURE by design: no IDF/esp_* includes, no WifiState/esp_reset_reason_t enums + * in the signatures — producers take primitive args (int reset reason, + * const char* state/pump/cause strings) so this lives in the `events` + * component that depends only on `interfaces` and is host-testable against + * MockDataStorage + FakeWallClock. + * + * Never throws, never blocks watering: a storeEvent() returning false is + * counted (droppedEvents()) and dropped. A pure component cannot ESP_LOGW, so + * the counter is the surfaced signal — target callers may log it periodically. + * Credential VALUES are never logged (WiFi events carry the state name only). + */ + +#ifndef WATERINGSYSTEM_EVENTS_EVENTLOGGER_H +#define WATERINGSYSTEM_EVENTS_EVENTLOGGER_H + +#include +#include + +#include "interfaces/IDataStorage.h" +#include "interfaces/IWallClock.h" + +/** + * @brief Map an esp_reset_reason_t integer value to a short name. + * + * Pure free function (no IDF include): the caller passes + * static_cast(esp_reset_reason()). Total over the ESP_RST_* values known + * at authoring time (0..10); any other value returns "UNKNOWN". Host-tested. + */ +const char* resetReasonName(int espResetReason); + +/** + * @brief Builds + persists typed events over an injected IDataStorage. + */ +class EventLogger { +public: + /// Holds references (not ownership); both must outlive the logger. Pass the + /// cross-task LockedDataStorage as `storage` when the store is shared. + EventLogger(IDataStorage& storage, IWallClock& clock) + : storage_(storage), clock_(clock) {} + + /// kCategoryReset, detail "reset=" (e.g. "reset=TASK_WDT"). + /// `reason` is the raw esp_reset_reason_t int (kept for the caller's API + /// symmetry; the detail carries the human-readable name). + void logReset(int reason, const char* reasonName); + + /// kCategoryConnectivity, detail "wifi=" (e.g. "wifi=Connected"). + void logWifi(const char* stateName); + + /// kCategoryPump, detail "pump= start cause=". + void logPumpStart(const char* pump, const char* cause); + + /// kCategoryPump, detail "pump= stop cause=". + void logPumpStop(const char* pump, const char* cause); + + /// kCategoryFailsafe, detail passed verbatim (producer is PR-11). + void logFailsafe(const char* detail); + + /// kCategoryOta, detail passed verbatim (producer is PR-13). + void logOta(const char* detail); + + /// Number of events dropped because storeEvent() returned false. Never + /// resets; a pure component's only failure signal (no ESP_LOGW here). + uint32_t droppedEvents() const { return droppedEvents_; } + +private: + /// Single write path: stamp with the wall clock, store, count a failure. + /// Never throws. + void emit(uint8_t category, const std::string& detail); + + IDataStorage& storage_; + IWallClock& clock_; + uint32_t droppedEvents_ = 0; +}; + +#endif /* WATERINGSYSTEM_EVENTS_EVENTLOGGER_H */ diff --git a/firmware/components/events/src/EventLogger.cpp b/firmware/components/events/src/EventLogger.cpp new file mode 100644 index 0000000..0eeb1bd --- /dev/null +++ b/firmware/components/events/src/EventLogger.cpp @@ -0,0 +1,82 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file EventLogger.cpp + * @brief Typed event producers + reset-reason name mapping (pure). + * + * No IDF/esp_* includes: detail strings are built with std::string only and + * category constants come from IDataStorage. See EventLogger.h for the + * behavioural contract. + */ + +#include "events/EventLogger.h" + +const char* resetReasonName(int espResetReason) +{ + // Mirrors esp_reset_reason_t (esp_system.h) by integer value so this stays + // pure (no IDF include). Total: any unlisted value maps to "UNKNOWN". + switch (espResetReason) { + case 0: return "UNKNOWN"; // ESP_RST_UNKNOWN + case 1: return "POWERON"; // ESP_RST_POWERON + case 2: return "EXT"; // ESP_RST_EXT + case 3: return "SW"; // ESP_RST_SW + case 4: return "PANIC"; // ESP_RST_PANIC + case 5: return "INT_WDT"; // ESP_RST_INT_WDT + case 6: return "TASK_WDT"; // ESP_RST_TASK_WDT + case 7: return "WDT"; // ESP_RST_WDT + case 8: return "DEEPSLEEP"; // ESP_RST_DEEPSLEEP + case 9: return "BROWNOUT"; // ESP_RST_BROWNOUT + case 10: return "SDIO"; // ESP_RST_SDIO + default: return "UNKNOWN"; + } +} + +void EventLogger::emit(uint8_t category, const std::string& detail) +{ + // Never throws / never blocks watering: a failed append is counted only + // (pure component — no ESP_LOGW). The store truncates over-long detail at + // kEventDetailMaxLen, so producers need not pre-truncate. + if (!storage_.storeEvent(clock_.nowEpoch(), category, detail)) { + ++droppedEvents_; + } +} + +void EventLogger::logReset(int reason, const char* reasonName) +{ + // `reason` is retained in the API for caller symmetry; the detail carries + // the mapped human-readable name only (contract example: "reset=TASK_WDT"). + (void)reason; + emit(IDataStorage::kCategoryReset, + std::string("reset=") + (reasonName ? reasonName : "UNKNOWN")); +} + +void EventLogger::logWifi(const char* stateName) +{ + // The STATE name only — never the SSID/password (FR-004). + emit(IDataStorage::kCategoryConnectivity, + std::string("wifi=") + (stateName ? stateName : "unknown")); +} + +void EventLogger::logPumpStart(const char* pump, const char* cause) +{ + emit(IDataStorage::kCategoryPump, + std::string("pump=") + (pump ? pump : "?") + " start cause=" + + (cause ? cause : "unknown")); +} + +void EventLogger::logPumpStop(const char* pump, const char* cause) +{ + emit(IDataStorage::kCategoryPump, + std::string("pump=") + (pump ? pump : "?") + " stop cause=" + + (cause ? cause : "unknown")); +} + +void EventLogger::logFailsafe(const char* detail) +{ + emit(IDataStorage::kCategoryFailsafe, detail ? detail : ""); +} + +void EventLogger::logOta(const char* detail) +{ + emit(IDataStorage::kCategoryOta, detail ? detail : ""); +} diff --git a/firmware/main/CMakeLists.txt b/firmware/main/CMakeLists.txt index 2e1515d..a62d9cc 100644 --- a/firmware/main/CMakeLists.txt +++ b/firmware/main/CMakeLists.txt @@ -1,9 +1,10 @@ idf_component_register( SRCS "app_main.cpp" "diag_console.cpp" "sensor_task.cpp" "wifi_task.cpp" + "system_observer.cpp" PRIV_REQUIRES board esp_driver_gpio esp_app_format actuators interfaces console esp_timer storage nvs_flash sensors esp_netif esp_event - network time + network time events esp_system ) # Build-time littlefs image of the committed seed directory diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index e6391a5..762bb8e 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -27,6 +27,7 @@ #include "esp_idf_version.h" #include "esp_log.h" #include "esp_netif.h" +#include "esp_system.h" #include "driver/gpio.h" #include "freertos/FreeRTOS.h" #include "freertos/task.h" @@ -35,6 +36,7 @@ #include "actuators/EspTimeProvider.h" #include "actuators/GpioWaterPump.h" #include "actuators/LockedWaterPump.h" +#include "events/EventLogger.h" #include "sensors/Bme280Sensor.h" #include "sensors/DebouncedLevelSensor.h" #include "sensors/EspI2cBus.h" @@ -60,9 +62,11 @@ #include "storage/LockedDataStorage.h" #include "storage/NvsConfigStore.h" #include "storage/StorageMount.h" +#include "time/SystemWallClock.h" #include "diag_console.h" #include "sensor_task.h" +#include "system_observer.h" #include "wifi_task.h" static const char *TAG = "app_main"; @@ -324,6 +328,23 @@ extern "C" void app_main(void) static LockedConfigStore config(config_store); static LockedDataStorage storage(data_storage); + // Persistent event logger (feature 008 US2). Function-local statics after + // pumps_force_off() (boot fail-safe rule): the SystemWallClock is trivial + // (reads time(nullptr); SNTP steps it in US3) and the EventLogger only + // stores references. It composes the SAME cross-task LockedDataStorage + // (`storage`) so events written from the main loop and later producers + // serialize with every other store access. A failed store is counted, never + // thrown — logging never blocks or crashes watering (FR-014). + static SystemWallClock wall_clock; + static EventLogger event_logger(storage, wall_clock); + + // Record why this boot happened (watchdog/panic/brownout/power-on) exactly + // once, before anything else can reset the reason. The pump fail-safe still + // ran first (top of app_main); this only observes the cause. + const esp_reset_reason_t reset_reason = esp_reset_reason(); + event_logger.logReset(static_cast(reset_reason), + resetReasonName(static_cast(reset_reason))); + // One-line usage report (parity: storage usage in the serial status // block; FR-008). const StorageStats stats = storage.getStorageStats(); @@ -594,6 +615,10 @@ extern "C" void app_main(void) #endif // nullptr in provisioning mode — the `wifi` command reports unavailable. diag_console_register_wifi(wifi_manager); + // TODO(US3/T018): construct SntpClient + register the time console + // (diag_console_register_time(&wall_clock, &sntp_client.syncStatus())). + // The SyncStatus owner is SntpClient, built in US3 — do NOT construct it + // here. esp_err_t err = diag_console_start(); if (err != ESP_OK) { // Console is a diagnostic aid, not a safety function: log and keep @@ -606,11 +631,26 @@ extern "C" void app_main(void) // sensor failed init above — lazy re-init recovers later (parity). sensor_task_start(env_sensor); + // System observer (feature 008 US2): edge-detects WiFi state changes and + // pump start/stop and forwards them to the event log. Function-local static + // after pumps_force_off() (boot fail-safe rule): it stores only references/ + // pointers and never touches pump control. wifi_manager is nullptr in + // provisioning/headless mode (the observer null-guards it); the pump set is + // capability-aware (rev2 single-pump node passes no reservoir). Polled from + // the 10 Hz loop below. +#if BOARD_HAS_RESERVOIR_PUMP + static SystemObserver observer(event_logger, wifi_manager, &plant, + &reservoir); +#else + static SystemObserver observer(event_logger, wifi_manager, &plant); +#endif + // Main loop: poll pump enforcement (every pump that exists on this // board) and the level sensors at 10 Hz. Pump update() applies the // timed self-stop and the hard 300 s max-runtime cap; level update() // samples the raw pins and advances the settle/debounce state - // machines (~3 samples per 300 ms window). + // machines (~3 samples per 300 ms window). observer.poll() then records + // any WiFi/pump transition — best-effort logging, never blocking watering. while (true) { plant.update(); #if BOARD_HAS_RESERVOIR_PUMP @@ -618,6 +658,7 @@ extern "C" void app_main(void) #endif level_low.update(); level_high.update(); + observer.poll(); vTaskDelay(pdMS_TO_TICKS(100)); } } diff --git a/firmware/main/system_observer.cpp b/firmware/main/system_observer.cpp new file mode 100644 index 0000000..c7696c3 --- /dev/null +++ b/firmware/main/system_observer.cpp @@ -0,0 +1,73 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file system_observer.cpp + * @brief Transition detection + EventLogger forwarding (feature 008 US2). + * + * The WifiState → name mapping lives here (not in the pure EventLogger, which + * takes a const char* state name only). Pump start/stop causes are best-effort + * "unknown" for now: the IWaterPump seam exposes isRunning() (edge source) and + * getLastStopReason(), but the running/stop-reason tuple visible from an + * external poll cannot reliably attribute the cause (console vs controller), + * so a deterministic placeholder is used until the controller (PR-11) owns and + * records the cause at the command site. + */ + +#include "system_observer.h" + +namespace { + +/// Short, stable name for each WifiState (matches the enum labels so the +/// event detail reads e.g. "wifi=Connected"). Total over the enum. +const char* wifiStateName(WifiState state) +{ + switch (state) { + case WifiState::Provisioning: return "Provisioning"; + case WifiState::Connecting: return "Connecting"; + case WifiState::Connected: return "Connected"; + case WifiState::Reconnecting: return "Reconnecting"; + case WifiState::ReconnectPaused: return "ReconnectPaused"; + } + return "unknown"; +} + +} // namespace + +void SystemObserver::poll() +{ + pollWifi(); + pollPump(plant_, "plant", plantLastRunning_); + pollPump(reservoir_, "reservoir", reservoirLastRunning_); +} + +void SystemObserver::pollWifi() +{ + if (wifi_ == nullptr) { + return; // provisioning / headless: nothing to observe + } + const WifiState state = wifi_->snapshot().state; + if (!haveWifiState_ || state != lastWifiState_) { + logger_.logWifi(wifiStateName(state)); + lastWifiState_ = state; + haveWifiState_ = true; + } +} + +void SystemObserver::pollPump(IWaterPump* pump, const char* name, + bool& lastRunning) +{ + if (pump == nullptr) { + return; // pump not present on this board + } + const bool running = pump->isRunning(); + if (running == lastRunning) { + return; + } + if (running) { + // Cause attribution belongs to the controller (PR-11); best-effort. + logger_.logPumpStart(name, "unknown"); + } else { + logger_.logPumpStop(name, "unknown"); + } + lastRunning = running; +} diff --git a/firmware/main/system_observer.h b/firmware/main/system_observer.h new file mode 100644 index 0000000..8a78ab8 --- /dev/null +++ b/firmware/main/system_observer.h @@ -0,0 +1,86 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file system_observer.h + * @brief Edge-detecting bridge from live system state to the EventLogger + * (feature 008 US2). + * + * Target-side glue (NOT a pure component): it observes the WifiManager + * snapshot and each pump's running state once per main-loop iteration and + * emits a typed EventLogger event on every transition — WiFi state change, + * pump off→on (logPumpStart) and pump on→off (logPumpStop). It holds only + * borrowed references/pointers (no ownership) and NEVER blocks or crashes + * watering: the underlying EventLogger drops (counts) a failed store rather + * than throwing, and poll() never touches pump control. + * + * Every dependency is null-guarded: `wifi` is nullptr in provisioning mode + * (and whenever WiFi init failed), and a pump pointer is nullptr when that + * pump does not exist on the board (rev2 single-pump node has no reservoir). + * + * Lives in main/ because it depends on the target WifiManager + pump handles; + * the pure formatting/category logic stays in the host-tested EventLogger. + */ + +#ifndef WATERINGSYSTEM_MAIN_SYSTEM_OBSERVER_H +#define WATERINGSYSTEM_MAIN_SYSTEM_OBSERVER_H + +#include "events/EventLogger.h" +#include "interfaces/IWaterPump.h" +#include "network/WifiManager.h" +#include "network/WifiState.h" + +/** + * @brief Detects WiFi/pump state transitions and forwards them to EventLogger. + * + * Construct once at the wiring site after the logger, WifiManager and pumps + * exist, then call poll() once per 10 Hz main-loop iteration. Unsynchronized + * by design: poll() runs from the single main loop; WifiManager exposes an + * immutable snapshot() and the pump handles are the LockedWaterPump wrappers. + */ +class SystemObserver { +public: + /** + * @brief Inject the logger, the (nullable) WifiManager and the pump handles. + * + * @param logger Typed event sink (borrowed; must outlive this observer). + * @param wifi WiFi state source, or nullptr in provisioning/headless + * mode (WiFi transitions are then simply not observed). + * @param plant Plant pump handle, or nullptr if absent. + * @param reservoir Reservoir pump handle, or nullptr on single-pump boards. + */ + SystemObserver(EventLogger& logger, WifiManager* wifi, IWaterPump* plant, + IWaterPump* reservoir = nullptr) + : logger_(logger), wifi_(wifi), plant_(plant), reservoir_(reservoir) + { + } + + /** + * @brief Sample WiFi + pump state once and emit an event per transition. + * + * Non-blocking, null-safe. The first observed WiFi state logs once; each + * subsequent state change logs once. A pump off→on logs a start, on→off + * logs a stop (cause best-effort). Never affects watering. + */ + void poll(); + +private: + void pollWifi(); + void pollPump(IWaterPump* pump, const char* name, bool& lastRunning); + + EventLogger& logger_; + WifiManager* wifi_; + IWaterPump* plant_; + IWaterPump* reservoir_; + + // Last-seen WiFi state; haveWifiState_ stays false until the first poll so + // the initial state is logged exactly once. + WifiState lastWifiState_ = WifiState::Provisioning; + bool haveWifiState_ = false; + + // Pumps are forced OFF at boot (app_main invariant), so "not running" is + // the correct initial edge baseline — no spurious start on the first poll. + bool plantLastRunning_ = false; + bool reservoirLastRunning_ = false; +}; + +#endif /* WATERINGSYSTEM_MAIN_SYSTEM_OBSERVER_H */ diff --git a/firmware/test_apps/host/main/CMakeLists.txt b/firmware/test_apps/host/main/CMakeLists.txt index 62cece9..65e5a55 100644 --- a/firmware/test_apps/host/main/CMakeLists.txt +++ b/firmware/test_apps/host/main/CMakeLists.txt @@ -15,5 +15,5 @@ idf_component_register( # 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 + REQUIRES unity actuators interfaces storage nvs_flash sensors board network time events ) diff --git a/firmware/test_apps/host/main/test_event_logger.cpp b/firmware/test_apps/host/main/test_event_logger.cpp index ab49c30..9eaed7f 100644 --- a/firmware/test_apps/host/main/test_event_logger.cpp +++ b/firmware/test_apps/host/main/test_event_logger.cpp @@ -2,17 +2,131 @@ // SPDX-License-Identifier: AGPL-3.0-or-later /** * @file test_event_logger.cpp - * @brief Host suite for the EventLogger producers (feature 008). + * @brief Host suite for the EventLogger producers (feature 008 US2). * - * Registered by test_main.cpp via run_event_logger_tests(). Empty for now — - * the per-producer formatting/category, write-failure counting and - * reset-reason mapping tests (against MockDataStorage + FakeWallClock) are - * added in T012 alongside the EventLogger implementation (T013). + * Registered by test_main.cpp via run_event_logger_tests(). Drives the pure + * EventLogger over MockDataStorage + FakeWallClock (no IDF, no real clock). + * Each producer must write exactly ONE event carrying the expected category, + * the exact deterministic detail string (contracts/event-logger.md) and the + * FakeWallClock epoch; a failing store increments droppedEvents() without + * crashing; resetReasonName() maps the ESP_RST_* integer values. + * + * Coverage maps to specs/008-sntp-watchdog-logging/contracts/event-logger.md. */ +#include + #include "unity.h" -// TODO(T012): register EventLogger formatting/category/failure tests here. +#include "events/EventLogger.h" +#include "interfaces/IDataStorage.h" +#include "storage/testing/MockDataStorage.h" +#include "time/testing/FakeWallClock.h" + +namespace { + +/// A fixed, plausible epoch (2021-01-01T00:00:00Z) so "is set" is irrelevant — +/// the logger stamps every event with clock.nowEpoch() regardless. +constexpr uint32_t kFixedEpoch = 1609459200u; + +void test_log_reset_writes_one_reset_event(void) +{ + MockDataStorage store; + FakeWallClock clock(kFixedEpoch); + EventLogger logger(store, clock); + + logger.logReset(6 /* ESP_RST_TASK_WDT */, resetReasonName(6)); + + TEST_ASSERT_EQUAL_size_t(1u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(kFixedEpoch, store.events[0].epoch); + TEST_ASSERT_EQUAL_UINT8(IDataStorage::kCategoryReset, + store.events[0].category); + TEST_ASSERT_EQUAL_STRING("reset=TASK_WDT", store.events[0].detail.c_str()); + TEST_ASSERT_EQUAL_UINT32(0u, logger.droppedEvents()); +} + +void test_log_wifi_writes_one_connectivity_event(void) +{ + MockDataStorage store; + FakeWallClock clock(kFixedEpoch); + EventLogger logger(store, clock); + + logger.logWifi("Connected"); + + TEST_ASSERT_EQUAL_size_t(1u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(kFixedEpoch, store.events[0].epoch); + TEST_ASSERT_EQUAL_UINT8(IDataStorage::kCategoryConnectivity, + store.events[0].category); + TEST_ASSERT_EQUAL_STRING("wifi=Connected", store.events[0].detail.c_str()); +} + +void test_log_pump_start_writes_one_pump_event(void) +{ + MockDataStorage store; + FakeWallClock clock(kFixedEpoch); + EventLogger logger(store, clock); + + logger.logPumpStart("plant", "unknown"); + + TEST_ASSERT_EQUAL_size_t(1u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(kFixedEpoch, store.events[0].epoch); + TEST_ASSERT_EQUAL_UINT8(IDataStorage::kCategoryPump, + store.events[0].category); + TEST_ASSERT_EQUAL_STRING("pump=plant start cause=unknown", + store.events[0].detail.c_str()); +} + +void test_log_pump_stop_writes_one_pump_event(void) +{ + MockDataStorage store; + FakeWallClock clock(kFixedEpoch); + EventLogger logger(store, clock); + + logger.logPumpStop("reservoir", "unknown"); + + TEST_ASSERT_EQUAL_size_t(1u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(kFixedEpoch, store.events[0].epoch); + TEST_ASSERT_EQUAL_UINT8(IDataStorage::kCategoryPump, + store.events[0].category); + TEST_ASSERT_EQUAL_STRING("pump=reservoir stop cause=unknown", + store.events[0].detail.c_str()); +} + +void test_write_failure_increments_dropped_and_never_crashes(void) +{ + MockDataStorage store; + FakeWallClock clock(kFixedEpoch); + EventLogger logger(store, clock); + + store.failWrites = true; + + // Every producer path is exercised; none may throw and none may store. + logger.logReset(1, resetReasonName(1)); + logger.logWifi("Reconnecting"); + logger.logPumpStart("plant", "unknown"); + logger.logPumpStop("plant", "unknown"); + + TEST_ASSERT_EQUAL_size_t(0u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(4u, logger.droppedEvents()); +} + +void test_reset_reason_name_mapping(void) +{ + TEST_ASSERT_EQUAL_STRING("POWERON", resetReasonName(1)); // ESP_RST_POWERON + TEST_ASSERT_EQUAL_STRING("PANIC", resetReasonName(4)); // ESP_RST_PANIC + TEST_ASSERT_EQUAL_STRING("TASK_WDT", resetReasonName(6)); // ESP_RST_TASK_WDT + TEST_ASSERT_EQUAL_STRING("BROWNOUT", resetReasonName(9)); // ESP_RST_BROWNOUT + TEST_ASSERT_EQUAL_STRING("UNKNOWN", resetReasonName(999)); // out of range +} + +} // namespace + void run_event_logger_tests(void) { + RUN_TEST(test_log_reset_writes_one_reset_event); + RUN_TEST(test_log_wifi_writes_one_connectivity_event); + RUN_TEST(test_log_pump_start_writes_one_pump_event); + RUN_TEST(test_log_pump_stop_writes_one_pump_event); + RUN_TEST(test_write_failure_increments_dropped_and_never_crashes); + RUN_TEST(test_reset_reason_name_mapping); } From a928b4f5629e05da8bce0a4b663c74a6705e8f5b Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sat, 4 Jul 2026 23:05:37 +0200 Subject: [PATCH 5/7] =?UTF-8?q?feat(watchdog):=20PR-08=20US3=20=E2=80=94?= =?UTF-8?q?=20esp=5Ftask=5Fwdt=20on=20watering-critical=20tasks=20+=20SNTP?= =?UTF-8?q?=20wiring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 5 / US3 + polish: - task_watchdog helper: reconfigure to WS_TASK_WDT_TIMEOUT_S (20s, panic->reboot), subscribe/feed. Subscribes ONLY the 10 Hz main loop + the 5 s sensor task; the WiFi task and console REPL are deliberately NOT subscribed (a network stall must not reboot the device - FR-014). Idle-task WDT from sdkconfig retained. - SNTP wiring: app_main constructs SntpClient + applyTimezone(); SystemObserver starts SNTP once on the first WifiState::Connected; diag console 'time' registered. - boot order verified: pumps_force_off() first -> esp_reset_reason()->logReset -> watchdog_init() -> tasks subscribe (a prior TASK_WDT reset shows in storage events). - firmware/CLAUDE.md feature-008 section; HIL checklist. Verified: host 187/0; rev1 1034400B (34% slot free) + rev2 build, both config-verified; dependencies.lock + esp-modbus pins unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- firmware/CLAUDE.md | 51 +++++++++++ firmware/main/CMakeLists.txt | 4 +- firmware/main/app_main.cpp | 43 ++++++++-- firmware/main/sensor_task.cpp | 10 +++ firmware/main/system_observer.cpp | 11 +++ firmware/main/system_observer.h | 22 ++++- firmware/main/task_watchdog.cpp | 57 +++++++++++++ firmware/main/task_watchdog.h | 56 ++++++++++++ .../checklists/hil.md | 85 +++++++++++++++++++ 9 files changed, 328 insertions(+), 11 deletions(-) create mode 100644 firmware/main/task_watchdog.cpp create mode 100644 firmware/main/task_watchdog.h create mode 100644 specs/008-sntp-watchdog-logging/checklists/hil.md diff --git a/firmware/CLAUDE.md b/firmware/CLAUDE.md index a2a1abc..23efd60 100644 --- a/firmware/CLAUDE.md +++ b/firmware/CLAUDE.md @@ -380,6 +380,57 @@ device clears credentials first, per the data-model boot rule) or station §7/§9): 500 ms connect-attempt toggle (wifi task) + 100 ms config-button-hold blink (app_main); HIL checklist in `specs/007-wifi-provisioning/checklists/hil.md`. +## SNTP time, task watchdog & event logging (feature 008) + +Feature 008 (PR-08) adds two small components plus target-side glue in `main/`. + +**`time` component** — pure `TimeService` (epoch plausibility + Swedish +local-time formatting via ``, host-tested), the `IWallClock` target +implementation `SystemWallClock` (`time(nullptr)`) and the `SntpClient` starter +(`esp_netif_sntp` against `CONFIG_WS_SNTP_SERVER`, TZ `CET-1CEST,M3.5.0,M10.5.0/3`). +`SystemWallClock.cpp` + `SntpClient.cpp` are the only IDF touchpoints (excluded +from the linux build, same mechanism as storage/sensors/network). `app_main` +constructs one `SntpClient`, calls `applyTimezone()` once at init, and the +`SystemObserver` starts the SNTP service **once, on the first +`WifiState::Connected` transition** (SNTP needs an IP) — never in +provisioning/headless mode. `start()` is idempotent and non-fatal: an +unreachable server is retried by the SNTP service, never a boot/watering +failure. The diag console `time` command reads `SystemWallClock` + the +`SntpClient`'s `SyncStatus`. + +**Time-not-set contract (for PR-11):** until the first successful sync the wall +clock is implausible (`isPlausibleEpoch(0)` is false); consumers MUST treat a +not-set clock as "no timestamp" rather than 1970. The event log records events +regardless (with whatever the clock reports); PR-11's scheduler must gate any +time-of-day watering decision on a plausible clock. + +**`events` component** — the pure, host-tested `EventLogger` (categories +`reset`/`wifi`/`pump`; producers `logReset`/`logWifi`/`logPumpStart`/`logPumpStop`; +a failed store increments a dropped counter, never throws — logging never blocks +or crashes watering, FR-014). It writes through the shared `LockedDataStorage`. +The target-side `SystemObserver` (`main/system_observer.*`) edge-detects WiFi +state changes and pump start/stop from the 10 Hz loop and forwards them. **Reset +reason:** at boot, exactly once, `app_main` calls `esp_reset_reason()` → +`event_logger.logReset(...)` (before `watchdog_init()`), so a prior watchdog +reboot appears in `storage events` as `reset=TASK_WDT`. The pump fail-safe still +runs first (top of `app_main`); this only observes the cause. + +**Task watchdog** (`main/task_watchdog.*`, thin wrappers over `esp_task_wdt`). +`CONFIG_ESP_TASK_WDT_INIT=y` + `CONFIG_ESP_TASK_WDT_PANIC=y` (sdkconfig.defaults) +init the WDT at boot; `watchdog_init()` **reconfigures** it to +`CONFIG_WS_TASK_WDT_TIMEOUT_S` (default 20 s, panic→reboot) — +`esp_task_wdt_reconfigure()`, with an `esp_task_wdt_init()` fallback if it was +not inited. **Subscription policy (contracts/task-watchdog.md):** ONLY the two +watering-critical tasks subscribe (`esp_task_wdt_add(NULL)`) and feed +(`esp_task_wdt_reset()`) — the 10 Hz main loop (feeds each 100 ms iteration) and +the 5 s sensor task (feeds each cycle). The **WiFi task is deliberately NOT +subscribed** (a network stall must never reboot the device — FR-014 isolation) +and neither is the esp_console REPL (it blocks on UART by design). The 20 s +default keeps a safe margin over the slowest subscribed cadence (5 s) so a +healthy task is never falsely tripped. PR-11's watering/reservoir tasks register +through the same helper when they land. HIL checklist: +`specs/008-sntp-watchdog-logging/checklists/hil.md`. + ## Partition layout (4MB flash) nvs (0x9000, 16K) | otadata (0xd000, 8K) | phy_init (0xf000, 4K) | diff --git a/firmware/main/CMakeLists.txt b/firmware/main/CMakeLists.txt index a62d9cc..7d8794c 100644 --- a/firmware/main/CMakeLists.txt +++ b/firmware/main/CMakeLists.txt @@ -1,6 +1,8 @@ idf_component_register( SRCS "app_main.cpp" "diag_console.cpp" "sensor_task.cpp" "wifi_task.cpp" - "system_observer.cpp" + "system_observer.cpp" "task_watchdog.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 diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index 762bb8e..7fb2e3b 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -62,11 +62,13 @@ #include "storage/LockedDataStorage.h" #include "storage/NvsConfigStore.h" #include "storage/StorageMount.h" +#include "time/SntpClient.h" #include "time/SystemWallClock.h" #include "diag_console.h" #include "sensor_task.h" #include "system_observer.h" +#include "task_watchdog.h" #include "wifi_task.h" static const char *TAG = "app_main"; @@ -338,6 +340,17 @@ extern "C" void app_main(void) static SystemWallClock wall_clock; static EventLogger event_logger(storage, wall_clock); + // SNTP wall-clock synchroniser (feature 008 US3). Function-local static + // after pumps_force_off() (boot fail-safe rule): the constructor only zeroes + // its SyncStatus, no IDF work. Install the Swedish timezone (CET/CEST) now + // so any local-time rendering is correct the moment the clock is first + // stepped. The SNTP service itself is NOT started here: the SystemObserver + // starts it lazily on the first transition into WifiState::Connected (SNTP + // needs an IP), and never in provisioning/headless mode. SNTP runs outside + // the watering path and is non-fatal (FR-014). + static SntpClient sntp; + sntp.applyTimezone(); + // Record why this boot happened (watchdog/panic/brownout/power-on) exactly // once, before anything else can reset the reason. The pump fail-safe still // ran first (top of app_main); this only observes the cause. @@ -352,6 +365,14 @@ extern "C" void app_main(void) static_cast(stats.usedBytes / 1024), static_cast(stats.totalBytes / 1024)); + // Task watchdog (feature 008 US3). Reconfigure the boot-time WDT to the + // Kconfig timeout with panic (reboot) on a non-serviced subscribed task. + // Placed here — after storage/netif init and AFTER the reset-reason event + // was logged above (so a prior TASK_WDT reset is recorded), and before the + // watering-critical tasks start and subscribe. Non-fatal on failure (logged + // inside): the watering path still runs, just unwatched. + watchdog_init(); + // WiFi boot-mode decision (feature 007, US1 + US3). A missing stored SSID // (the factory/unconfigured state) OR a held config button forces // first-boot/recovery provisioning; a configured device otherwise comes up @@ -615,10 +636,12 @@ extern "C" void app_main(void) #endif // nullptr in provisioning mode — the `wifi` command reports unavailable. diag_console_register_wifi(wifi_manager); - // TODO(US3/T018): construct SntpClient + register the time console - // (diag_console_register_time(&wall_clock, &sntp_client.syncStatus())). - // The SyncStatus owner is SntpClient, built in US3 — do NOT construct it - // here. + // Wall clock + SNTP sync status (feature 008 US3): the `time` command reads + // the local time and reports the sync state. The SyncStatus is owned by the + // SntpClient (constructed above); the observer starts the SNTP service on + // the first Connected transition, so before the first sync `time` reports + // "time not set". + diag_console_register_time(&wall_clock, &sntp.status()); esp_err_t err = diag_console_start(); if (err != ESP_OK) { // Console is a diagnostic aid, not a safety function: log and keep @@ -639,10 +662,10 @@ extern "C" void app_main(void) // capability-aware (rev2 single-pump node passes no reservoir). Polled from // the 10 Hz loop below. #if BOARD_HAS_RESERVOIR_PUMP - static SystemObserver observer(event_logger, wifi_manager, &plant, + static SystemObserver observer(event_logger, wifi_manager, &sntp, &plant, &reservoir); #else - static SystemObserver observer(event_logger, wifi_manager, &plant); + static SystemObserver observer(event_logger, wifi_manager, &sntp, &plant); #endif // Main loop: poll pump enforcement (every pump that exists on this @@ -651,6 +674,13 @@ extern "C" void app_main(void) // samples the raw pins and advances the settle/debounce state // machines (~3 samples per 300 ms window). observer.poll() then records // any WiFi/pump transition — best-effort logging, never blocking watering. + // + // This main loop is a watering-critical task: subscribe it to the task WDT + // (feature 008 US3) once here, then feed it every iteration. A stall in the + // pump/level enforcement path therefore forces a panic reboot (pumps OFF at + // the next boot, reset reason logged as TASK_WDT). The 100 ms cadence is far + // under the 20 s default timeout. + watchdog_subscribe_current_task(); while (true) { plant.update(); #if BOARD_HAS_RESERVOIR_PUMP @@ -659,6 +689,7 @@ extern "C" void app_main(void) level_low.update(); level_high.update(); observer.poll(); + watchdog_feed(); vTaskDelay(pdMS_TO_TICKS(100)); } } diff --git a/firmware/main/sensor_task.cpp b/firmware/main/sensor_task.cpp index 08b658c..b7c0c5e 100644 --- a/firmware/main/sensor_task.cpp +++ b/firmware/main/sensor_task.cpp @@ -24,6 +24,7 @@ #include "freertos/task.h" #include "sensors/SensorTaskLogPolicy.h" +#include "task_watchdog.h" static const char *TAG = "sensor_task"; @@ -42,12 +43,21 @@ constexpr UBaseType_t kPriority = 1; ///< parity priority (R7) // (e.g. booting with no sensor attached) WARNs exactly once. SensorTaskLogPolicy logPolicy; + // Watering-critical task: subscribe to the task WDT (feature 008 US3). The + // 20 s default timeout has a safe margin over this 5 s cycle. A stalled + // read never blocks (the driver is bounded), so the feed each cycle is the + // liveness proof the WDT needs. + watchdog_subscribe_current_task(); + TickType_t lastWake = xTaskGetTickCount(); while (true) { // First poll one period after start — the NORMAL-mode first // conversion completes well within 5 s (research.md R9). vTaskDelayUntil(&lastWake, pdMS_TO_TICKS(kPeriodMs)); + // Feed once per 5 s cycle: this task is alive and servicing the WDT. + watchdog_feed(); + const bool ok = sensor.read(); switch (logPolicy.onReadResult(ok)) { case SensorTaskLogPolicy::Event::Recovery: diff --git a/firmware/main/system_observer.cpp b/firmware/main/system_observer.cpp index c7696c3..e49ee3d 100644 --- a/firmware/main/system_observer.cpp +++ b/firmware/main/system_observer.cpp @@ -51,6 +51,17 @@ void SystemObserver::pollWifi() lastWifiState_ = state; haveWifiState_ = true; } + + // Start SNTP the first time the station reaches Connected — SNTP needs an + // IP, so starting it before the link is up is pointless. start() is + // idempotent and non-fatal (server unreachable is retried by the service, + // never a boot/watering failure, FR-014); the guard keeps it to one call. + // sntp_ is nullptr in provisioning/headless mode, so SNTP is simply never + // started there. + if (state == WifiState::Connected && sntp_ != nullptr && !sntpStarted_) { + sntp_->start(); + sntpStarted_ = true; + } } void SystemObserver::pollPump(IWaterPump* pump, const char* name, diff --git a/firmware/main/system_observer.h b/firmware/main/system_observer.h index 8a78ab8..9bbb6ec 100644 --- a/firmware/main/system_observer.h +++ b/firmware/main/system_observer.h @@ -28,6 +28,7 @@ #include "interfaces/IWaterPump.h" #include "network/WifiManager.h" #include "network/WifiState.h" +#include "time/SntpClient.h" /** * @brief Detects WiFi/pump state transitions and forwards them to EventLogger. @@ -40,17 +41,24 @@ class SystemObserver { public: /** - * @brief Inject the logger, the (nullable) WifiManager and the pump handles. + * @brief Inject the logger, the (nullable) WifiManager, the SNTP client and + * the pump handles. * * @param logger Typed event sink (borrowed; must outlive this observer). * @param wifi WiFi state source, or nullptr in provisioning/headless * mode (WiFi transitions are then simply not observed). + * @param sntp SNTP client to start on the first Connected transition, + * or nullptr to never start SNTP (borrowed). SNTP is thus + * never started in provisioning/headless mode (wifi is + * nullptr) nor when no client is injected — the wall clock + * simply stays "time not set" (feature 008 US3). * @param plant Plant pump handle, or nullptr if absent. * @param reservoir Reservoir pump handle, or nullptr on single-pump boards. */ - SystemObserver(EventLogger& logger, WifiManager* wifi, IWaterPump* plant, - IWaterPump* reservoir = nullptr) - : logger_(logger), wifi_(wifi), plant_(plant), reservoir_(reservoir) + SystemObserver(EventLogger& logger, WifiManager* wifi, SntpClient* sntp, + IWaterPump* plant, IWaterPump* reservoir = nullptr) + : logger_(logger), wifi_(wifi), sntp_(sntp), plant_(plant), + reservoir_(reservoir) { } @@ -69,6 +77,7 @@ class SystemObserver { EventLogger& logger_; WifiManager* wifi_; + SntpClient* sntp_; IWaterPump* plant_; IWaterPump* reservoir_; @@ -77,6 +86,11 @@ class SystemObserver { WifiState lastWifiState_ = WifiState::Provisioning; bool haveWifiState_ = false; + // SNTP is started exactly once, on the first transition into Connected. + // SntpClient::start() is itself idempotent; this guard avoids calling it on + // every later Connected transition. + bool sntpStarted_ = false; + // Pumps are forced OFF at boot (app_main invariant), so "not running" is // the correct initial edge baseline — no spurious start on the first poll. bool plantLastRunning_ = false; diff --git a/firmware/main/task_watchdog.cpp b/firmware/main/task_watchdog.cpp new file mode 100644 index 0000000..05a4395 --- /dev/null +++ b/firmware/main/task_watchdog.cpp @@ -0,0 +1,57 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file task_watchdog.cpp + * @brief Task watchdog wrappers (feature 008 US3). See task_watchdog.h for the + * subscription policy; the timeout comes from CONFIG_WS_TASK_WDT_TIMEOUT_S. + */ + +#include "task_watchdog.h" + +#include "esp_log.h" +#include "esp_task_wdt.h" +#include "sdkconfig.h" + +static const char *TAG = "task_wdt"; + +esp_err_t watchdog_init() +{ + // The watchdog is ALREADY initialised at boot (CONFIG_ESP_TASK_WDT_INIT=y, + // watching the idle tasks), so the normal path is a reconfigure of the + // runtime timeout — NOT a fresh init. Panic (reboot) on timeout is enabled + // (CONFIG_ESP_TASK_WDT_PANIC=y) and re-asserted here. + esp_task_wdt_config_t cfg = { + .timeout_ms = (uint32_t)CONFIG_WS_TASK_WDT_TIMEOUT_S * 1000u, + .idle_core_mask = 0, // leave idle-task watching as configured by sdkconfig + .trigger_panic = true, + }; + esp_err_t err = esp_task_wdt_reconfigure(&cfg); + if (err == ESP_ERR_INVALID_STATE) { + // The watchdog was not inited (CONFIG_ESP_TASK_WDT_INIT disabled) — + // init it from scratch instead. + err = esp_task_wdt_init(&cfg); + } + if (err != ESP_OK) { + // Non-fatal: without the WDT a hung task is not caught, but watering + // still runs. Log loudly so the misconfiguration is visible. + ESP_LOGE(TAG, "task WDT configure failed: %s", esp_err_to_name(err)); + } else { + ESP_LOGI(TAG, "task WDT active: timeout %d s, panic reboot on stall", + CONFIG_WS_TASK_WDT_TIMEOUT_S); + } + return err; +} + +void watchdog_subscribe_current_task() +{ + const esp_err_t err = esp_task_wdt_add(NULL); + if (err != ESP_OK) { + // Non-fatal: an unsubscribed task is simply not watched by the WDT. + ESP_LOGW(TAG, "task WDT subscribe failed: %s", esp_err_to_name(err)); + } +} + +void watchdog_feed() +{ + esp_task_wdt_reset(); +} diff --git a/firmware/main/task_watchdog.h b/firmware/main/task_watchdog.h new file mode 100644 index 0000000..75b51ca --- /dev/null +++ b/firmware/main/task_watchdog.h @@ -0,0 +1,56 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file task_watchdog.h + * @brief Thin wrappers over the IDF task watchdog (feature 008 US3) — + * target only. + * + * The task WDT configuration lives in Kconfig/sdkconfig: CONFIG_ESP_TASK_WDT_INIT + * initialises the watchdog at boot and CONFIG_ESP_TASK_WDT_PANIC makes a timeout + * panic (reboot). watchdog_init() only RECONFIGURES the runtime timeout to + * CONFIG_WS_TASK_WDT_TIMEOUT_S; the watering-critical tasks then subscribe + * themselves and feed the watchdog once per loop iteration. + * + * Subscription policy (contracts/task-watchdog.md): ONLY the watering-critical + * tasks are subscribed — the 10 Hz main loop and the 5 s sensor task. The WiFi + * task is deliberately NOT subscribed (a network stall must never reboot the + * device, FR-014) and neither is the esp_console REPL (it blocks on UART by + * design). On a non-serviced subscribed task the WDT panics → reboot; at the + * next boot pumps_force_off() runs first (unchanged) and the reset reason is + * logged as TASK_WDT. + */ + +#ifndef WATERINGSYSTEM_MAIN_TASK_WATCHDOG_H +#define WATERINGSYSTEM_MAIN_TASK_WATCHDOG_H + +#include "esp_err.h" + +/** + * @brief Ensure the task WDT runs with CONFIG_WS_TASK_WDT_TIMEOUT_S and + * panic-on-timeout enabled. Call once early in app_main. + * + * Idempotent with respect to the IDF boot-time init: because + * CONFIG_ESP_TASK_WDT_INIT already initialised the watchdog, this reconfigures + * the timeout via esp_task_wdt_reconfigure(). If the watchdog was NOT inited + * (ESP_ERR_INVALID_STATE) it falls back to esp_task_wdt_init(). + * + * @return ESP_OK on success, the failing esp_err_t otherwise (logged). + */ +esp_err_t watchdog_init(); + +/** + * @brief Subscribe the CALLING task to the task WDT (esp_task_wdt_add(NULL)). + * + * Call once at the start of a watering-critical task, before its loop. A + * failure is logged and non-fatal — the task simply is not watched. + */ +void watchdog_subscribe_current_task(); + +/** + * @brief Feed the task WDT for the CALLING task (esp_task_wdt_reset()). + * + * Call once per loop iteration of a subscribed task. + */ +void watchdog_feed(); + +#endif /* WATERINGSYSTEM_MAIN_TASK_WATCHDOG_H */ diff --git a/specs/008-sntp-watchdog-logging/checklists/hil.md b/specs/008-sntp-watchdog-logging/checklists/hil.md new file mode 100644 index 0000000..4a8fda8 --- /dev/null +++ b/specs/008-sntp-watchdog-logging/checklists/hil.md @@ -0,0 +1,85 @@ +# HIL Checklist: SNTP Time, Task Watchdog & Event Logging (008) — rev1 bench rig + +**Purpose**: hardware-in-the-loop verification of PR-08 at Checkpoint 3 (Paul, bench rig) +**Rig**: ESP32 devkit + status LED on GPIO2, config button on GPIO18, pumps, BME280 and RS485 soil sensor as wired for the previous features. A WiFi AP with real internet access (so SNTP can reach `se.pool.ntp.org`) plus a phone/laptop, and a way to block internet / pull the AP. +**Build**: rev1 target (`sdkconfig.board.rev1_devkit`), flash per `firmware/CLAUDE.md`. The device must be provisioned with valid home-WiFi credentials (station mode) — see PR-07 §A if it is still unconfigured. +**Reference**: acceptance criteria `docs/prd/PR-08-*.md`; spec FR-008/FR-009/FR-014; `quickstart.md` §HIL; `contracts/task-watchdog.md`; parity `docs/parity-checklist.md` +**Watchdog note**: sections B and C need a subscribed watering-critical task to be starved on purpose — build with a debug hook / build flag that spins a `while(true){}` inside the 10 Hz main loop or the sensor task (never the WiFi task). Remove the hook before merge. + +## A. Correct Swedish time + sync status (US1, quickstart §HIL #1) + +- [ ] A1. Boot networked → console `time` immediately after boot reports + "not set" (or an implausible 1970 epoch flagged as not-set) BEFORE the + first sync +- [ ] A2. After the station reaches Connected and SNTP syncs (a few seconds), + `time` shows the correct **local** Swedish time — CET (+01:00) in winter, + CEST (+02:00) in summer (confirm the offset matches the current season) +- [ ] A3. `time` sync status reports **synced** with a plausible last-sync + timestamp +- [ ] A4. Confirm SNTP is only started once WiFi is Connected (serial shows the + SNTP start after the `wifi=Connected` event, not at boot) + +## B. Watchdog reboot + pumps safe (US3, quickstart §HIL #2, FR-008/FR-009) + +- [ ] B1. With the debug starve-hook armed on a subscribed critical task + (main loop OR sensor task), trigger the stall → the device **reboots** + within `CONFIG_WS_TASK_WDT_TIMEOUT_S` (default 20 s) +- [ ] B2. Measure the pump outputs immediately after the reset → **OFF** + (pump fail-safe still runs first at boot, unchanged) +- [ ] B3. On the reboot, `storage events` lists a fresh `reset=TASK_WDT` entry + with a timestamp (reset-reason event path) +- [ ] B4. Repeat starving the OTHER subscribed task (sensor task if B1 used the + main loop, or vice versa) → same result: reboot + pumps OFF + `TASK_WDT` + +## C. No spurious reboot (US3, quickstart §HIL #3) + +- [ ] C1. Remove/disarm the starve-hook. Run the device normally for an + extended period (≥ 30 min, sensor polling + a couple of pump timed runs) → + **no watchdog reset** occurs (serial banner appears once; `storage events` + shows no unexpected `reset=TASK_WDT`) + +## D. Event log persists across power-cycle (US2, quickstart §HIL #4) + +- [ ] D1. Generate events: `pump plant start 5` (start + self-stop), and drop + the WiFi briefly (pull the AP, then restore) to log a + `wifi=Reconnecting`/`wifi=Connected` pair +- [ ] D2. `storage events` lists the pump start/stop and WiFi events with + timestamps + causes +- [ ] D3. **Power-cycle** the device (full power off/on, not a reset) → after + reboot `storage events` STILL lists the pre-power-cycle events (persisted + to littlefs, newest-first, survives the boot) + +## E. WiFi outage does NOT reboot (US3, quickstart §HIL #5, FR-014) + +- [ ] E1. With the device connected in station mode, start a watering activity + (`pump plant start 30`) and let the 5 s environmental poll run +- [ ] E2. Pull the WiFi (power off the AP) for longer than the watchdog timeout + → the device keeps running, the pump timed run + self-stop and the + sensor cadence are UNAFFECTED, and **no watchdog reset** occurs (the WiFi + task is not subscribed) +- [ ] E3. A `wifi=Reconnecting` (and later `wifi=Connected` on restore) event + pair is logged + +## F. Non-fatal SNTP offline (US1, quickstart §HIL #6) + +- [ ] F1. Block NTP (connect to a WiFi network with no internet, or firewall + the NTP pool) → the device runs indefinitely in "time not set" with no + crash and no reboot +- [ ] F2. `time` keeps reporting not-set / not-synced; the rest of the system + (pumps, sensors, console) is unaffected +- [ ] F3. Restore internet → SNTP eventually syncs on its own retry and `time` + switches to the correct local time (no reboot required) + +## Deferral path + +If the rig, a real-internet AP, or the debug starve-hook build is unavailable, +defer the affected items with a written rationale and add them to the +deferred-HIL register (mirroring the handover's deferred-HIL pattern, e.g. +PR-04's `hil-004`). The host-test suite (`test_time.cpp` DST/plausibility, +`test_event_logger.cpp` producers + drop counter + reset-reason mapping + +littlefs rotation/persistence) already covers the pure logic deterministically; +the items above are the hardware-only confirmations — the SNTP wall-clock step, +the physical watchdog reboot + pump-OFF measurement, and cross-power-cycle +persistence. + +**Sign-off**: date + result per item as a PR comment (pattern from PR #7). From 6dab7c459940ed480705c320e2aa9c0b366dc912 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 10:41:43 +0200 Subject: [PATCH 6/7] =?UTF-8?q?fix(008):=20CP3=20review=20fixes=20(source)?= =?UTF-8?q?=20=E2=80=94=20event/watchdog/SNTP=20robustness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CP3 findings F1-F6 + polish (host 187/0, both boards green): - F1: SystemObserver surfaces EventLogger droppedEvents() via ESP_LOGW (was a dead counter — a failing event store went silent) - F2: task watchdog preserves idle-task watching on all cores (idle_core_mask = (1< --- firmware/CLAUDE.md | 6 ++-- .../events/include/events/EventLogger.h | 10 +++---- .../components/events/src/EventLogger.cpp | 14 +++++---- firmware/components/time/CMakeLists.txt | 20 ++++++------- .../components/time/include/time/SntpClient.h | 10 ++++--- .../components/time/include/time/SyncStatus.h | 18 +++++++++--- .../time/include/time/testing/FakeWallClock.h | 6 ++-- firmware/components/time/src/SntpClient.cpp | 29 ++++++++++++++----- firmware/components/time/src/TimeService.cpp | 6 +++- firmware/main/app_main.cpp | 14 +++++---- firmware/main/diag_console.cpp | 2 +- firmware/main/system_observer.cpp | 24 +++++++++++++-- firmware/main/system_observer.h | 13 +++++++-- firmware/main/task_watchdog.cpp | 7 ++++- .../test_apps/host/main/test_event_logger.cpp | 4 +-- 15 files changed, 127 insertions(+), 56 deletions(-) diff --git a/firmware/CLAUDE.md b/firmware/CLAUDE.md index 23efd60..00b991d 100644 --- a/firmware/CLAUDE.md +++ b/firmware/CLAUDE.md @@ -388,8 +388,10 @@ Feature 008 (PR-08) adds two small components plus target-side glue in `main/`. local-time formatting via ``, host-tested), the `IWallClock` target implementation `SystemWallClock` (`time(nullptr)`) and the `SntpClient` starter (`esp_netif_sntp` against `CONFIG_WS_SNTP_SERVER`, TZ `CET-1CEST,M3.5.0,M10.5.0/3`). -`SystemWallClock.cpp` + `SntpClient.cpp` are the only IDF touchpoints (excluded -from the linux build, same mechanism as storage/sensors/network). `app_main` +`SntpClient.cpp` is the only genuine IDF touchpoint; `SystemWallClock.cpp` is +pure POSIX (`time(nullptr)`), kept target-side by convention (the host injects +`FakeWallClock`). Both are excluded from the linux build (same mechanism as +storage/sensors/network). `app_main` constructs one `SntpClient`, calls `applyTimezone()` once at init, and the `SystemObserver` starts the SNTP service **once, on the first `WifiState::Connected` transition** (SNTP needs an IP) — never in diff --git a/firmware/components/events/include/events/EventLogger.h b/firmware/components/events/include/events/EventLogger.h index 6f1eb45..c93a1c3 100644 --- a/firmware/components/events/include/events/EventLogger.h +++ b/firmware/components/events/include/events/EventLogger.h @@ -35,8 +35,8 @@ * @brief Map an esp_reset_reason_t integer value to a short name. * * Pure free function (no IDF include): the caller passes - * static_cast(esp_reset_reason()). Total over the ESP_RST_* values known - * at authoring time (0..10); any other value returns "UNKNOWN". Host-tested. + * static_cast(esp_reset_reason()). Total over the ESP_RST_* values defined + * by IDF v6 (0..15); any other value returns "UNKNOWN". Host-tested. */ const char* resetReasonName(int espResetReason); @@ -51,9 +51,9 @@ class EventLogger { : storage_(storage), clock_(clock) {} /// kCategoryReset, detail "reset=" (e.g. "reset=TASK_WDT"). - /// `reason` is the raw esp_reset_reason_t int (kept for the caller's API - /// symmetry; the detail carries the human-readable name). - void logReset(int reason, const char* reasonName); + /// `reason` is the raw esp_reset_reason_t int; the detail is built from the + /// internal resetReasonName(reason) mapping so the name cannot mismatch. + void logReset(int reason); /// kCategoryConnectivity, detail "wifi=" (e.g. "wifi=Connected"). void logWifi(const char* stateName); diff --git a/firmware/components/events/src/EventLogger.cpp b/firmware/components/events/src/EventLogger.cpp index 0eeb1bd..cb9321d 100644 --- a/firmware/components/events/src/EventLogger.cpp +++ b/firmware/components/events/src/EventLogger.cpp @@ -27,6 +27,11 @@ const char* resetReasonName(int espResetReason) case 8: return "DEEPSLEEP"; // ESP_RST_DEEPSLEEP case 9: return "BROWNOUT"; // ESP_RST_BROWNOUT case 10: return "SDIO"; // ESP_RST_SDIO + case 11: return "USB"; // ESP_RST_USB + case 12: return "JTAG"; // ESP_RST_JTAG + case 13: return "EFUSE"; // ESP_RST_EFUSE + case 14: return "PWR_GLITCH"; // ESP_RST_PWR_GLITCH + case 15: return "CPU_LOCKUP"; // ESP_RST_CPU_LOCKUP default: return "UNKNOWN"; } } @@ -41,13 +46,12 @@ void EventLogger::emit(uint8_t category, const std::string& detail) } } -void EventLogger::logReset(int reason, const char* reasonName) +void EventLogger::logReset(int reason) { - // `reason` is retained in the API for caller symmetry; the detail carries - // the mapped human-readable name only (contract example: "reset=TASK_WDT"). - (void)reason; + // The detail carries the mapped human-readable name only (contract example: + // "reset=TASK_WDT"); mapping internally makes a name mismatch unrepresentable. emit(IDataStorage::kCategoryReset, - std::string("reset=") + (reasonName ? reasonName : "UNKNOWN")); + std::string("reset=") + resetReasonName(reason)); } void EventLogger::logWifi(const char* stateName) diff --git a/firmware/components/time/CMakeLists.txt b/firmware/components/time/CMakeLists.txt index d5aaccc..1b04309 100644 --- a/firmware/components/time/CMakeLists.txt +++ b/firmware/components/time/CMakeLists.txt @@ -3,17 +3,15 @@ # (SntpClient). # # TimeService.cpp is pure C++ (epoch plausibility + Swedish local-time -# formatting via /localtime_r, which both the host and target -# provide) and builds on the linux preview target used by the host test -# suite. SystemWallClock.cpp (time(nullptr) wall clock) and SntpClient.cpp -# (esp_netif_sntp against CONFIG_WS_SNTP_SERVER + TZ/DST setup) are the only -# hardware/IDF touchpoints and are excluded from the linux build, same -# mechanism as storage/sensors/network. -# -# TimeService.cpp is pure and builds on both targets. SystemWallClock.cpp and -# SntpClient.cpp are the hardware/IDF touchpoints (esp_netif_sntp) and are -# excluded from the linux build; esp_netif provides esp_netif_sntp.h. Those -# headers appear only in the target-only .cpp files, never in public headers. +# formatting via /localtime_r, which both the host and target provide) +# and builds on the linux preview target used by the host test suite. +# SystemWallClock.cpp is also pure POSIX (time(nullptr)); it is kept target-side +# by convention only (host tests inject FakeWallClock instead). SntpClient.cpp +# (esp_netif_sntp against CONFIG_WS_SNTP_SERVER + TZ/DST setup) is the only +# genuine IDF touchpoint. Both target-only .cpp files are excluded from the linux +# build, same mechanism as storage/sensors/network; esp_netif provides +# esp_netif_sntp.h. Those esp_* headers appear only in SntpClient.cpp, never in +# public headers. if(${IDF_TARGET} STREQUAL "linux") idf_component_register( SRCS "src/TimeService.cpp" diff --git a/firmware/components/time/include/time/SntpClient.h b/firmware/components/time/include/time/SntpClient.h index 6133b63..e1a79aa 100644 --- a/firmware/components/time/include/time/SntpClient.h +++ b/firmware/components/time/include/time/SntpClient.h @@ -47,11 +47,13 @@ class SntpClient { * * Initialises esp_netif_sntp against CONFIG_WS_SNTP_SERVER in step-set * (immediate) mode with the sync callback registered. Re-invocation is a - * no-op (an already-initialised service is treated as success). Failure to - * reach the server is non-fatal — logged as a warning; the SNTP service - * keeps retrying on its own. + * no-op (an already-initialised service is treated as success). Reaching the + * server later is the SNTP service's own job; this call never blocks on it. + * + * @return true if the SNTP service is now running (init OK, or already + * inited); false if init failed — the caller should retry later. */ - void start(); + bool start(); /// Immutable view of the current synchronisation state. const SyncStatus& status() const { return status_; } diff --git a/firmware/components/time/include/time/SyncStatus.h b/firmware/components/time/include/time/SyncStatus.h index 1af4293..6c72c35 100644 --- a/firmware/components/time/include/time/SyncStatus.h +++ b/firmware/components/time/include/time/SyncStatus.h @@ -5,8 +5,9 @@ * @brief Pure value type tracking SNTP synchronisation state. * * Shared between TimeService (pure, host-tested) and SntpClient (target). - * No IDF and no includes — only . Consumed by the console - * `time` line to report whether the wall clock has ever been set and when. + * No IDF and no includes — only and the pure + * time/TimeService.h. Consumed by the console `time` line to report whether the + * wall clock has ever been set and when. */ #ifndef WATERINGSYSTEM_TIME_SYNCSTATUS_H @@ -14,16 +15,25 @@ #include +#include "time/TimeService.h" + /** * @brief Snapshot of the wall-clock synchronisation state. * - * @var SyncStatus::synced true once a plausible time has been set. * @var SyncStatus::lastSyncEpoch epoch seconds of the most recent successful * sync (0 while never synced). + * + * synced() is derived from lastSyncEpoch — one source of truth, so the illegal + * "synced but epoch 0" pair cannot be represented. */ struct SyncStatus { - bool synced = false; uint32_t lastSyncEpoch = 0; + + /// True once a plausible time has been synced. + bool synced() const + { + return lastSyncEpoch >= TimeService::kMinPlausibleEpoch; + } }; #endif /* WATERINGSYSTEM_TIME_SYNCSTATUS_H */ diff --git a/firmware/components/time/include/time/testing/FakeWallClock.h b/firmware/components/time/include/time/testing/FakeWallClock.h index 2ee3738..6e034ef 100644 --- a/firmware/components/time/include/time/testing/FakeWallClock.h +++ b/firmware/components/time/include/time/testing/FakeWallClock.h @@ -18,14 +18,16 @@ #include #include "interfaces/IWallClock.h" +#include "time/TimeService.h" /** * @brief Manually settable wall clock for deterministic host tests. */ class FakeWallClock : public IWallClock { public: - /// 2020-01-01T00:00:00Z — mirrors TimeService::kMinPlausibleEpoch. - static constexpr uint32_t kDefaultThreshold = 1577836800u; + /// 2020-01-01T00:00:00Z — the shared plausibility threshold (compile-time + /// reference, not a hand-copied literal). + static constexpr uint32_t kDefaultThreshold = TimeService::kMinPlausibleEpoch; /// Start unset (epoch 0) unless a starting epoch is supplied. explicit FakeWallClock(uint32_t startEpoch = 0, diff --git a/firmware/components/time/src/SntpClient.cpp b/firmware/components/time/src/SntpClient.cpp index 1519b66..1330fbc 100644 --- a/firmware/components/time/src/SntpClient.cpp +++ b/firmware/components/time/src/SntpClient.cpp @@ -31,6 +31,13 @@ SntpClient* s_instance = nullptr; SntpClient::SntpClient() { + // Single-instance by design (the C-ABI sync callback has no user context and + // routes into s_instance). A second live instance would silently steal the + // callback route — flag it loudly rather than overwriting quietly. + if (s_instance != nullptr) { + ESP_LOGE(TAG, "second SntpClient constructed — sync callback routes to " + "the latest instance only"); + } s_instance = this; } @@ -53,10 +60,10 @@ void SntpClient::applyTimezone() tzset(); } -void SntpClient::start() +bool SntpClient::start() { if (started_) { - return; + return true; } esp_sntp_config_t cfg = ESP_NETIF_SNTP_DEFAULT_CONFIG(CONFIG_WS_SNTP_SERVER); @@ -65,15 +72,17 @@ void SntpClient::start() esp_err_t err = esp_netif_sntp_init(&cfg); if (err == ESP_ERR_INVALID_STATE) { - // Already initialised (idempotent re-invocation) — treat as success. + // Already initialised (idempotent re-invocation) — the service is + // running, so report success. started_ = true; - return; + return true; } if (err != ESP_OK) { - // Non-fatal: log and keep running. isTimeSet() stays false until a - // later attempt succeeds; the caller must never block on sync. + // Non-fatal: log and keep running. Init failed, so the service is NOT + // running; report failure so the caller can retry on a later Connected + // transition. The caller must never block on sync. ESP_LOGW(TAG, "esp_netif_sntp_init failed: %s", esp_err_to_name(err)); - return; + return false; } // Step-set (immediate) mode: apply the server time in one jump rather than @@ -81,6 +90,7 @@ void SntpClient::start() sntp_set_sync_mode(SNTP_SYNC_MODE_IMMED); ESP_LOGI(TAG, "SNTP started against %s", CONFIG_WS_SNTP_SERVER); started_ = true; + return true; } void SntpClient::onSyncCb(struct timeval* tv) @@ -88,7 +98,10 @@ void SntpClient::onSyncCb(struct timeval* tv) if (s_instance == nullptr || tv == nullptr) { return; } - s_instance->status_.synced = true; + // synced() is derived from lastSyncEpoch, so stamping the epoch is the whole + // update. This runs on the SNTP task; the console `time` command reads + // status_ from the REPL task without a lock — a deliberate benign divergence + // (aligned word-size reads, diagnostic-only), so no Locked* wrapper is used. s_instance->status_.lastSyncEpoch = static_cast(tv->tv_sec); ESP_LOGI(TAG, "wall clock synced: epoch=%u", static_cast(tv->tv_sec)); diff --git a/firmware/components/time/src/TimeService.cpp b/firmware/components/time/src/TimeService.cpp index 3a8d82a..4182e52 100644 --- a/firmware/components/time/src/TimeService.cpp +++ b/firmware/components/time/src/TimeService.cpp @@ -17,7 +17,11 @@ std::string TimeService::formatLocal(uint32_t epoch) { time_t t = static_cast(epoch); struct tm lt; - localtime_r(&t, <); + if (localtime_r(&t, <) == nullptr) { + // Conversion failed (e.g. out-of-range time_t) — never format an + // uninitialised tm; return a sentinel instead. + return std::string("(time unavailable)"); + } char buf[32]; strftime(buf, sizeof buf, "%Y-%m-%d %H:%M:%S %z", <); return std::string(buf); diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index 7fb2e3b..6917f37 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -351,12 +351,16 @@ extern "C" void app_main(void) static SntpClient sntp; sntp.applyTimezone(); - // Record why this boot happened (watchdog/panic/brownout/power-on) exactly - // once, before anything else can reset the reason. The pump fail-safe still - // ran first (top of app_main); this only observes the cause. + // Record why this boot happened (watchdog/panic/brownout/power-on). + // esp_reset_reason() is RTC-latched and not cleared by watchdog_init(), so + // ordering is not load-bearing; log it early for clarity. Also ESP_LOGI it + // to serial so the reason is visible independent of the event store (which + // may drop the event if the log is full). The pump fail-safe still ran first + // (top of app_main); this only observes the cause. const esp_reset_reason_t reset_reason = esp_reset_reason(); - event_logger.logReset(static_cast(reset_reason), - resetReasonName(static_cast(reset_reason))); + ESP_LOGI(TAG, "reset reason: %s", + resetReasonName(static_cast(reset_reason))); + event_logger.logReset(static_cast(reset_reason)); // One-line usage report (parity: storage usage in the serial status // block; FR-008). diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index 2a99448..0ebd633 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -857,7 +857,7 @@ int time_cmd(int argc, char **argv) return 0; } const std::string now = TimeService::formatLocal(s_clock->nowEpoch()); - if (s_sync != nullptr && s_sync->synced) { + if (s_sync != nullptr && s_sync->synced()) { const std::string last = TimeService::formatLocal(s_sync->lastSyncEpoch); printf("OK %s (last sync %s)\n", now.c_str(), last.c_str()); } else { diff --git a/firmware/main/system_observer.cpp b/firmware/main/system_observer.cpp index e49ee3d..df5ba73 100644 --- a/firmware/main/system_observer.cpp +++ b/firmware/main/system_observer.cpp @@ -15,8 +15,13 @@ #include "system_observer.h" +#include "esp_log.h" + namespace { +const char* TAG = "sys_observer"; + + /// Short, stable name for each WifiState (matches the enum labels so the /// event detail reads e.g. "wifi=Connected"). Total over the enum. const char* wifiStateName(WifiState state) @@ -38,6 +43,17 @@ void SystemObserver::poll() pollWifi(); pollPump(plant_, "plant", plantLastRunning_); pollPump(reservoir_, "reservoir", reservoirLastRunning_); + + // Give the pure logger's dropped-event counter a target-side voice: it can + // only count a failed store, so surface any increase here (mirrors the + // EspWifiDriver dropped-counter pattern). Never blocks or touches watering. + const uint32_t dropped = logger_.droppedEvents(); + if (dropped > lastDropped_) { + ESP_LOGW(TAG, "event log dropped %u events (total %u)", + static_cast(dropped - lastDropped_), + static_cast(dropped)); + lastDropped_ = dropped; + } } void SystemObserver::pollWifi() @@ -59,8 +75,12 @@ void SystemObserver::pollWifi() // sntp_ is nullptr in provisioning/headless mode, so SNTP is simply never // started there. if (state == WifiState::Connected && sntp_ != nullptr && !sntpStarted_) { - sntp_->start(); - sntpStarted_ = true; + // Latch only on success: a failed init (OOM/bad config) leaves the guard + // clear so the next Connected transition retries rather than permanently + // blocking sync. + if (sntp_->start()) { + sntpStarted_ = true; + } } } diff --git a/firmware/main/system_observer.h b/firmware/main/system_observer.h index 9bbb6ec..7361e23 100644 --- a/firmware/main/system_observer.h +++ b/firmware/main/system_observer.h @@ -86,11 +86,18 @@ class SystemObserver { WifiState lastWifiState_ = WifiState::Provisioning; bool haveWifiState_ = false; - // SNTP is started exactly once, on the first transition into Connected. - // SntpClient::start() is itself idempotent; this guard avoids calling it on - // every later Connected transition. + // SNTP is started exactly once, on the first Connected transition where + // start() reports success. SntpClient::start() is itself idempotent; this + // guard avoids calling it on every later Connected transition, but a failed + // init leaves it false so the next Connected transition retries. bool sntpStarted_ = false; + // Last observed EventLogger::droppedEvents() value. The pure logger only + // counts a failed store; poll() gives that counter a target-side voice by + // ESP_LOGW-ing the delta whenever it increases (mirrors EspWifiDriver's + // dropped-counter pattern). + uint32_t lastDropped_ = 0; + // Pumps are forced OFF at boot (app_main invariant), so "not running" is // the correct initial edge baseline — no spurious start on the first poll. bool plantLastRunning_ = false; diff --git a/firmware/main/task_watchdog.cpp b/firmware/main/task_watchdog.cpp index 05a4395..e03b977 100644 --- a/firmware/main/task_watchdog.cpp +++ b/firmware/main/task_watchdog.cpp @@ -10,6 +10,7 @@ #include "esp_log.h" #include "esp_task_wdt.h" +#include "freertos/FreeRTOS.h" // portNUM_PROCESSORS #include "sdkconfig.h" static const char *TAG = "task_wdt"; @@ -22,7 +23,11 @@ esp_err_t watchdog_init() // (CONFIG_ESP_TASK_WDT_PANIC=y) and re-asserted here. esp_task_wdt_config_t cfg = { .timeout_ms = (uint32_t)CONFIG_WS_TASK_WDT_TIMEOUT_S * 1000u, - .idle_core_mask = 0, // leave idle-task watching as configured by sdkconfig + // Watch the idle task on every core, preserving the CONFIG_ESP_TASK_WDT_INIT + // default (esp_task_wdt_reconfigure applies the FULL config, so a 0 mask + // would UNsubscribe the idle tasks). This keeps the per-core hang detector + // in addition to the explicitly-subscribed watering-critical tasks. + .idle_core_mask = (1 << portNUM_PROCESSORS) - 1, .trigger_panic = true, }; esp_err_t err = esp_task_wdt_reconfigure(&cfg); diff --git a/firmware/test_apps/host/main/test_event_logger.cpp b/firmware/test_apps/host/main/test_event_logger.cpp index 9eaed7f..08b85b2 100644 --- a/firmware/test_apps/host/main/test_event_logger.cpp +++ b/firmware/test_apps/host/main/test_event_logger.cpp @@ -35,7 +35,7 @@ void test_log_reset_writes_one_reset_event(void) FakeWallClock clock(kFixedEpoch); EventLogger logger(store, clock); - logger.logReset(6 /* ESP_RST_TASK_WDT */, resetReasonName(6)); + logger.logReset(6 /* ESP_RST_TASK_WDT */); TEST_ASSERT_EQUAL_size_t(1u, store.events.size()); TEST_ASSERT_EQUAL_UINT32(kFixedEpoch, store.events[0].epoch); @@ -101,7 +101,7 @@ void test_write_failure_increments_dropped_and_never_crashes(void) store.failWrites = true; // Every producer path is exercised; none may throw and none may store. - logger.logReset(1, resetReasonName(1)); + logger.logReset(1); logger.logWifi("Reconnecting"); logger.logPumpStart("plant", "unknown"); logger.logPumpStop("plant", "unknown"); From a89285b1a5090fc3cfb3a269e04e2a121119567f Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Sun, 5 Jul 2026 10:45:42 +0200 Subject: [PATCH 7/7] =?UTF-8?q?test(008):=20CP3=20host=20tests=20=E2=80=94?= =?UTF-8?q?=20event=20producers,=20reset-reason=20total,=20DST=20autumn,?= =?UTF-8?q?=20clock-step?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - logFailsafe/logOta producer tests (category + verbatim detail + epoch) - store-failure test extended to all six producers (droppedEvents==6) - resetReasonName total mapping 0..15 + unknown->UNKNOWN - multi-event clock-step: epoch stamped at emit time (not construction), getEvents newest-first - DST autumn (fall-back) boundary: CEST->CET flip at the last-Sunday-of-October instant Verified: host tests 191/0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test_apps/host/main/test_event_logger.cpp | 96 +++++++++++++++++-- firmware/test_apps/host/main/test_time.cpp | 11 +++ 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/firmware/test_apps/host/main/test_event_logger.cpp b/firmware/test_apps/host/main/test_event_logger.cpp index 08b85b2..1a5aede 100644 --- a/firmware/test_apps/host/main/test_event_logger.cpp +++ b/firmware/test_apps/host/main/test_event_logger.cpp @@ -15,6 +15,7 @@ */ #include +#include #include "unity.h" @@ -92,6 +93,41 @@ void test_log_pump_stop_writes_one_pump_event(void) store.events[0].detail.c_str()); } +void test_log_failsafe_writes_one_failsafe_event(void) +{ + MockDataStorage store; + FakeWallClock clock(kFixedEpoch); + EventLogger logger(store, clock); + + // Detail is passed verbatim (producer is PR-11); no reformatting. + logger.logFailsafe("failsafe=soil-invalid pump=plant"); + + TEST_ASSERT_EQUAL_size_t(1u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(kFixedEpoch, store.events[0].epoch); + TEST_ASSERT_EQUAL_UINT8(IDataStorage::kCategoryFailsafe, + store.events[0].category); + TEST_ASSERT_EQUAL_STRING("failsafe=soil-invalid pump=plant", + store.events[0].detail.c_str()); + TEST_ASSERT_EQUAL_UINT32(0u, logger.droppedEvents()); +} + +void test_log_ota_writes_one_ota_event(void) +{ + MockDataStorage store; + FakeWallClock clock(kFixedEpoch); + EventLogger logger(store, clock); + + // Detail is passed verbatim (producer is PR-13); no reformatting. + logger.logOta("ota=begin"); + + TEST_ASSERT_EQUAL_size_t(1u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(kFixedEpoch, store.events[0].epoch); + TEST_ASSERT_EQUAL_UINT8(IDataStorage::kCategoryOta, + store.events[0].category); + TEST_ASSERT_EQUAL_STRING("ota=begin", store.events[0].detail.c_str()); + TEST_ASSERT_EQUAL_UINT32(0u, logger.droppedEvents()); +} + void test_write_failure_increments_dropped_and_never_crashes(void) { MockDataStorage store; @@ -100,23 +136,66 @@ void test_write_failure_increments_dropped_and_never_crashes(void) store.failWrites = true; - // Every producer path is exercised; none may throw and none may store. + // ALL SIX producer paths are exercised; none may throw and none may store. logger.logReset(1); logger.logWifi("Reconnecting"); logger.logPumpStart("plant", "unknown"); logger.logPumpStop("plant", "unknown"); + logger.logFailsafe("failsafe=soil-invalid pump=plant"); + logger.logOta("ota=begin"); TEST_ASSERT_EQUAL_size_t(0u, store.events.size()); - TEST_ASSERT_EQUAL_UINT32(4u, logger.droppedEvents()); + TEST_ASSERT_EQUAL_UINT32(6u, logger.droppedEvents()); +} + +void test_multi_event_stamps_at_call_time(void) +{ + // Proves emit() stamps with clock.nowEpoch() at CALL time, not at logger + // construction: two events logged around a clock step carry each step's + // epoch, and getEvents() returns them newest-first. + constexpr uint32_t kE = kFixedEpoch; + MockDataStorage store; + FakeWallClock clock(kE); + EventLogger logger(store, clock); + + logger.logWifi("Connecting"); + clock.setEpoch(kE + 3600u); + logger.logWifi("Connected"); + + TEST_ASSERT_EQUAL_size_t(2u, store.events.size()); + TEST_ASSERT_EQUAL_UINT32(kE, store.events[0].epoch); + TEST_ASSERT_EQUAL_UINT32(kE + 3600u, store.events[1].epoch); + + // Retrieval is newest-first (monotonic epochs). + std::vector recent = store.getEvents(10); + TEST_ASSERT_EQUAL_size_t(2u, recent.size()); + TEST_ASSERT_EQUAL_UINT32(kE + 3600u, recent[0].epoch); + TEST_ASSERT_EQUAL_STRING("wifi=Connected", recent[0].detail.c_str()); + TEST_ASSERT_EQUAL_UINT32(kE, recent[1].epoch); + TEST_ASSERT_EQUAL_STRING("wifi=Connecting", recent[1].detail.c_str()); } void test_reset_reason_name_mapping(void) { - TEST_ASSERT_EQUAL_STRING("POWERON", resetReasonName(1)); // ESP_RST_POWERON - TEST_ASSERT_EQUAL_STRING("PANIC", resetReasonName(4)); // ESP_RST_PANIC - TEST_ASSERT_EQUAL_STRING("TASK_WDT", resetReasonName(6)); // ESP_RST_TASK_WDT - TEST_ASSERT_EQUAL_STRING("BROWNOUT", resetReasonName(9)); // ESP_RST_BROWNOUT - TEST_ASSERT_EQUAL_STRING("UNKNOWN", resetReasonName(999)); // out of range + // Total over the ESP_RST_* integer values (esp_system.h, IDF v6: 0..15); + // any other value maps to "UNKNOWN". + TEST_ASSERT_EQUAL_STRING("UNKNOWN", resetReasonName(0)); // ESP_RST_UNKNOWN + TEST_ASSERT_EQUAL_STRING("POWERON", resetReasonName(1)); // ESP_RST_POWERON + TEST_ASSERT_EQUAL_STRING("EXT", resetReasonName(2)); // ESP_RST_EXT + TEST_ASSERT_EQUAL_STRING("SW", resetReasonName(3)); // ESP_RST_SW + TEST_ASSERT_EQUAL_STRING("PANIC", resetReasonName(4)); // ESP_RST_PANIC + TEST_ASSERT_EQUAL_STRING("INT_WDT", resetReasonName(5)); // ESP_RST_INT_WDT + TEST_ASSERT_EQUAL_STRING("TASK_WDT", resetReasonName(6)); // ESP_RST_TASK_WDT + TEST_ASSERT_EQUAL_STRING("WDT", resetReasonName(7)); // ESP_RST_WDT + TEST_ASSERT_EQUAL_STRING("DEEPSLEEP", resetReasonName(8)); // ESP_RST_DEEPSLEEP + TEST_ASSERT_EQUAL_STRING("BROWNOUT", resetReasonName(9)); // ESP_RST_BROWNOUT + TEST_ASSERT_EQUAL_STRING("SDIO", resetReasonName(10)); // ESP_RST_SDIO + TEST_ASSERT_EQUAL_STRING("USB", resetReasonName(11)); // ESP_RST_USB + TEST_ASSERT_EQUAL_STRING("JTAG", resetReasonName(12)); // ESP_RST_JTAG + TEST_ASSERT_EQUAL_STRING("EFUSE", resetReasonName(13)); // ESP_RST_EFUSE + TEST_ASSERT_EQUAL_STRING("PWR_GLITCH", resetReasonName(14)); // ESP_RST_PWR_GLITCH + TEST_ASSERT_EQUAL_STRING("CPU_LOCKUP", resetReasonName(15)); // ESP_RST_CPU_LOCKUP + TEST_ASSERT_EQUAL_STRING("UNKNOWN", resetReasonName(999)); // out of range } } // namespace @@ -127,6 +206,9 @@ void run_event_logger_tests(void) RUN_TEST(test_log_wifi_writes_one_connectivity_event); RUN_TEST(test_log_pump_start_writes_one_pump_event); RUN_TEST(test_log_pump_stop_writes_one_pump_event); + RUN_TEST(test_log_failsafe_writes_one_failsafe_event); + RUN_TEST(test_log_ota_writes_one_ota_event); RUN_TEST(test_write_failure_increments_dropped_and_never_crashes); + RUN_TEST(test_multi_event_stamps_at_call_time); RUN_TEST(test_reset_reason_name_mapping); } diff --git a/firmware/test_apps/host/main/test_time.cpp b/firmware/test_apps/host/main/test_time.cpp index f4e9a24..31d5d65 100644 --- a/firmware/test_apps/host/main/test_time.cpp +++ b/firmware/test_apps/host/main/test_time.cpp @@ -58,6 +58,16 @@ void test_format_dst_spring_boundary(void) TEST_ASSERT_TRUE(ends_with(TimeService::formatLocal(1711843200u), "+0100")); } +void test_format_dst_autumn_boundary(void) +{ + // Fall-back is the last Sunday of October (2024-10-27); the M10.5.0/3 rule + // switches at 03:00 local CEST == 01:00 UTC, dropping to 02:00 CET. + // 2024-10-27 00:00:00Z — before the switch -> still CEST (+0200). + TEST_ASSERT_TRUE(ends_with(TimeService::formatLocal(1729987200u), "+0200")); + // 2024-10-27 01:00:00Z — at the fall-back instant -> CET (+0100). + TEST_ASSERT_TRUE(ends_with(TimeService::formatLocal(1729990800u), "+0100")); +} + } // namespace void run_time_tests(void) @@ -70,4 +80,5 @@ void run_time_tests(void) RUN_TEST(test_plausible_epoch_threshold); RUN_TEST(test_format_winter_and_summer_offsets); RUN_TEST(test_format_dst_spring_boundary); + RUN_TEST(test_format_dst_autumn_boundary); }