From e5a79ca07334ee1547e76e153e14021b652a84a7 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 16:14:35 +0200 Subject: [PATCH 1/5] docs(spec): BME280 i2c_master spec, plan, research and tasks (005-bme280-i2c) Full spec-kit artifact set for PR-03: feature spec with CP1 clarifications (parity NORMAL-mode sampling profile; env-only 5 s sensor task), research R1-R10 (own Bosch compensation code over espressif/bme280 registry component; verified i2c_master bus lock and legacy driver status on IDF v6.0.1), data model, interface contracts, quickstart and 25 tasks. Co-Authored-By: Claude Fable 5 --- .specify/feature.json | 4 +- .../005-bme280-i2c/checklists/requirements.md | 43 +++ specs/005-bme280-i2c/contracts/interfaces.md | 156 +++++++++ specs/005-bme280-i2c/data-model.md | 97 ++++++ specs/005-bme280-i2c/plan.md | 150 +++++++++ specs/005-bme280-i2c/quickstart.md | 58 ++++ specs/005-bme280-i2c/research.md | 160 +++++++++ specs/005-bme280-i2c/spec.md | 318 ++++++++++++++++++ specs/005-bme280-i2c/tasks.md | 87 +++++ 9 files changed, 1070 insertions(+), 3 deletions(-) create mode 100644 specs/005-bme280-i2c/checklists/requirements.md create mode 100644 specs/005-bme280-i2c/contracts/interfaces.md create mode 100644 specs/005-bme280-i2c/data-model.md create mode 100644 specs/005-bme280-i2c/plan.md create mode 100644 specs/005-bme280-i2c/quickstart.md create mode 100644 specs/005-bme280-i2c/research.md create mode 100644 specs/005-bme280-i2c/spec.md create mode 100644 specs/005-bme280-i2c/tasks.md diff --git a/.specify/feature.json b/.specify/feature.json index 9101c8f..2b5ef90 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1 @@ -{ - "feature_directory": "specs/004-modbus-soil-sensor" -} +{"feature_directory":"specs/005-bme280-i2c"} \ No newline at end of file diff --git a/specs/005-bme280-i2c/checklists/requirements.md b/specs/005-bme280-i2c/checklists/requirements.md new file mode 100644 index 0000000..734c01e --- /dev/null +++ b/specs/005-bme280-i2c/checklists/requirements.md @@ -0,0 +1,43 @@ +# Specification Quality Checklist: BME280 Environmental Sensor over I2C + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-07-02 +**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 + +- Domain-necessary technical terms (I2C addresses 0x76/0x77, sampling profile values, + pin numbers, 5 s cadence) are parity facts from `docs/parity-checklist.md` §5 and + the mini-PRD — they are the observable contract, not implementation choices. This + matches the precedent set by spec 004. +- Deliberate divergences from legacy (address probing, last-good-value contract, + live availability checks) are called out explicitly in Assumptions, mirroring how + spec 004 documented its divergences. +- Registry-component-vs-own-code and bus-instance placement are explicitly deferred + to the plan phase per the mini-PRD. +- Items marked incomplete require spec updates before `/speckit-clarify` or `/speckit-plan` diff --git a/specs/005-bme280-i2c/contracts/interfaces.md b/specs/005-bme280-i2c/contracts/interfaces.md new file mode 100644 index 0000000..f02e501 --- /dev/null +++ b/specs/005-bme280-i2c/contracts/interfaces.md @@ -0,0 +1,156 @@ +# Interface Contracts: BME280 Environmental Sensor over I2C + +Normative contracts for the interfaces, console command and task behavior this +feature introduces. Header doc comments summarize these; this file is the source +of truth. Style follows `specs/004-modbus-soil-sensor/contracts/interfaces.md`. + +## `IEnvironmentalSensor` (interfaces component, header-only, no IDF includes) + +Standalone pure C++ interface (soil-sensor conventions: no base class, no +`getName()` — research R5). Deliberate divergences from the legacy driver are +listed at the end of this file. + +``` +class IEnvironmentalSensor { + virtual bool initialize() = 0; // idempotent; lazy-capable + virtual bool read() = 0; // one atomic snapshot + virtual bool isAvailable() = 0; // REAL bus probe (chip-ID read) + virtual int getLastError() = 0; // 0/1/2, see data-model.md + virtual float getTemperature() = 0; // °C — last-good value + virtual float getHumidity() = 0; // %RH — last-good value + virtual float getPressure() = 0; // hPa — last-good value +}; +``` + +Contract: + +- **`initialize()`**: probes 0x76 then 0x77, verifies chip identity (0x60), reads + calibration, writes the parity sampling profile (ctrl_hum → ctrl_meas → config, + data-model.md). Returns false with error 1 if no BME280 is found. Idempotent; + callers need not call it first — `read()`/`isAvailable()` initialize lazily + (parity). +- **`read()`**: burst-reads 0xF7–0xFE in one transaction, compensates T → P → H + (t_fine ordering), converts to °C/%RH/hPa. Atomic: either all three getters are + refreshed, or the call fails (error 1 on lost/absent sensor, error 2 on bus + error or NaN) and the last-good values remain untouched. **Consumers gate on the + read result, never on value plausibility.** Before the first successful read the + getters return meaningless placeholders. A bus error marks the driver + uninitialized → the next call re-probes both addresses (recovery, spec FR-004). +- **`isAvailable()`**: real chip-ID read every call — never cached state. Does not + modify `getLastError()`. +- Exactly one bus attempt per operation; no automatic retry (recovery comes from + the caller's poll cadence — same philosophy as the soil sensor). + +## `II2cBus` (interfaces component, header-only, no IDF includes) + +The hardware seam (research R6). Minimal register-oriented I2C master contract: + +``` +class II2cBus { + virtual bool probe(uint8_t address7) = 0; // ACK check + virtual bool readRegisters(uint8_t address7, uint8_t startReg, + uint8_t *buf, size_t len) = 0; // write-reg, repeated-start read + virtual bool writeRegister(uint8_t address7, uint8_t reg, + uint8_t value) = 0; +}; +``` + +Contract: + +- 7-bit addresses. `readRegisters` = register-pointer write followed by an + N-byte read in one transaction (repeated start — required for correct BME280 + burst reads). Returns false on NACK/bus error/timeout; no retries at this layer. +- Implementations serialize safely for multi-task use at transaction granularity + (the ESP implementation inherits this from the i2c_master driver's bus lock, + research R3; mocks are used single-threaded in host tests). +- No BME280 knowledge in this interface — it is reused by PR-05 (INA226 uses + 16-bit register values; PR-05 may extend the interface or compose two 8-bit + operations, its call). + +## `Bme280Sensor` (sensors component, pure logic — builds on linux) + +`Bme280Sensor(II2cBus &bus)` implements `IEnvironmentalSensor`. Holds ALL policy: +probing order, chip-ID check, calibration parsing, sampling configuration, +compensation math, unit conversion, NaN check, error codes, lazy re-init, +uninitialize-on-bus-error. No IDF includes. Host-tested against `MockI2cBus`. + +## `EspI2cBus` (sensors component, target-only — excluded on linux) + +Implements `II2cBus` over `driver/i2c_master.h` (new API — never the legacy +`driver/i2c.h`): + +- Owns the single `i2c_master_bus_handle_t`: created once from + `BOARD_PIN_I2C_SDA`/`BOARD_PIN_I2C_SCL` (board component; never hard-coded), + port auto-select, internal pull-ups enabled (breakouts carry their own; the + internal ones are harmless belt-and-braces). +- Manages per-address device handles internally, created on first use, + **100 kHz** standard mode per device (spec FR-002). +- `probe()` maps to `i2c_master_probe`; `readRegisters` to + `i2c_master_transmit_receive`; `writeRegister` to `i2c_master_transmit`. + Finite timeouts (no infinite waits); errors logged at debug level and returned + as false — classification is `Bme280Sensor`'s job. +- **Bus sharing (spec FR-003)**: the one `EspI2cBus` instance is constructed in + `app_main` (function-local static, established pattern) and passed to every + I2C driver — PR-05's INA226 receives the same instance. No second bus creation + on these pins is permitted. + +## `LockedEnvironmentalSensor` (sensors component, header-only) + +Mutex decorator wrapping any `IEnvironmentalSensor`; serializes each interface +call (FreeRTOS mutex, same pattern as `LockedSoilSensor`/`LockedWaterPump`). +Anything accessed from more than one task (sensor task + console REPL now, +web/controller later) is wrapped and accessed ONLY through the wrapper. +Known pattern limitation (documented, matches LockedSoilSensor): a +read-then-getters sequence spans multiple lock acquisitions; the consistent- +snapshot helper is PR-11 bookkeeping (see PR-04's TODO(PR-11) notes). + +## `MockI2cBus` / `MockEnvironmentalSensor` (sensors component, testing/, header-only) + +- `MockI2cBus`: scriptable register map + probe results + failure injection + (NACK on address X, error on register read N, corrupt data) — drives the real + `Bme280Sensor` in host tests (chip-ID mismatch, absent sensor, mid-read loss, + recovery re-probe, calibration parsing, compensation vectors). +- `MockEnvironmentalSensor`: scripted values/validity/error sequences for + consumer tests (console-level logic now, PR-11 controller tests later). Include + consistency helpers from the start (scriptFailedRead/scriptSuccessfulRead — + lesson recorded from PR-04's MockSoilSensor bookkeeping). + +## Console command (main/diag_console) + +HIL verification path; thin wrapper, no logic (established rule). Registered as +`diag_console_register_env(IEnvironmentalSensor &sensor)` before console start. + +``` +env # one read(); prints temperature/humidity/pressure or error +``` + +Output contract (exact format fixed at implementation, these fields are binding): + +- Success: the three values with units (`temperature=23.4 C humidity=45.2 %RH + pressure=1013.2 hPa`). +- Failure: `ERROR ` plus a human hint distinguishing code 1 ("sensor not + found") from code 2 ("read failed") — SC-006. + +## Sensor task (main/, app wiring) + +- FreeRTOS task, 4096 B stack, priority 1, `vTaskDelayUntil` period 5000 ms + (parity parameters, research R7). Polls `LockedEnvironmentalSensor::read()`. +- Starts even when the sensor failed init (lazy re-init recovers later — parity); + never exits; never reboots on failures. +- Logging: WARN on valid→invalid transition and on recovery; repeated failures at + bounded cadence (every Nth failure), INFO-level periodic readings consistent + with the legacy 5 s status print. +- Publishing = the Locked sensor itself (last-good values + validity via + getLastError); no separate shared-state structure this PR (PR-09/PR-11 consume + the same wrapper). + +## Deliberate divergences from legacy (parity-checklist §6 candidates) + +1. **Address probing 0x76/0x77 + chip-ID check** — legacy hard-codes 0x77, no + identity check. New capability (spec US3). +2. **Last-good getter values after failed read** — legacy leaves NaN in getters. + Aligned with the soil-sensor contract; consumers gate on read(). +3. **`isAvailable()` = real bus probe** — legacy caches available-after-init + forever. Aligned with the soil-sensor contract (spec FR-009). +4. **Synchronized cross-task access via Locked decorator** — legacy has two + unsynchronized readers on the same I2C device (main loop + web server). diff --git a/specs/005-bme280-i2c/data-model.md b/specs/005-bme280-i2c/data-model.md new file mode 100644 index 0000000..40d766b --- /dev/null +++ b/specs/005-bme280-i2c/data-model.md @@ -0,0 +1,97 @@ +# Data Model: BME280 Environmental Sensor over I2C + +## Environmental reading + +One atomic snapshot per successful `read()`; on failure the previous good values +remain in the getters (validity contract, spec FR-001/FR-007). + +| Quantity | Unit | Type | Derivation | +|-------------|------|-------|---------------------------------------------------| +| Temperature | °C | float | Bosch compensation (int32 path) ÷ 100 | +| Humidity | %RH | float | Bosch compensation (int32 Q22.10 path) ÷ 1024 | +| Pressure | hPa | float | Bosch compensation (int64 Q24.8 path) ÷ 256 ÷ 100 (parity: legacy converts Pa → hPa) | + +No range validation beyond the NaN check (parity: legacy validates nothing else). +A compensation result that is NaN fails the whole read (legacy error 2). + +## Error codes (`getLastError()`) + +Same philosophy as the soil sensor's table: small, parity-anchored, distinct where +consumers must distinguish (SC-006: console separates "absent" from "read failed"). + +| Code | Meaning | Produced when | Parity | +|------|---------|---------------|--------| +| 0 | OK | last operation succeeded | legacy 0 | +| 1 | Sensor not found | probe of 0x76 and 0x77 finds no device ACK, or a responding device fails the chip-identity check (logged distinctly) | legacy 1 ("sensor not found") | +| 2 | Read failed | bus/communication error during a data read, or compensation yields NaN | legacy 2 ("read failed / NaN") | + +`isAvailable()` never modifies the error code (soil-sensor convention). + +## BME280 register map (used subset) + +| Register | Name | Use | +|----------|------|-----| +| 0xD0 | chip_id | identity check — must read **0x60** (BME280) | +| 0x88–0xA1 | calib00–25 | calibration: dig_T1–T3, dig_P1–P9, dig_H1 | +| 0xE1–0xE7 | calib26–32 | calibration: dig_H2–H6 | +| 0xF2 | ctrl_hum | humidity oversampling — write **before** ctrl_meas (datasheet: takes effect on ctrl_meas write) | +| 0xF4 | ctrl_meas | T/P oversampling + mode | +| 0xF5 | config | standby + IIR filter | +| 0xF7–0xFE | press/temp/hum data | burst-read all 8 bytes in one transaction (atomic snapshot within the chip's shadowing rules) | + +I2C addresses probed: **0x76, then 0x77** (bench rig module is at 0x77; greenhouse +unit hard-codes 0x77). Recovery after loss re-probes both (spec FR-004). + +## Sampling profile (parity, spec FR-006) + +Exact legacy configuration (`src/sensors/BME280Sensor.cpp:41-46`, +`docs/parity-checklist.md` §5): + +| Register | Field values | Byte | +|----------|-------------|------| +| ctrl_hum (0xF2) | osrs_h = ×1 (001) | 0x01 | +| ctrl_meas (0xF4) | osrs_t = ×2 (010), osrs_p = ×16 (101), mode = NORMAL (11) | 0x57 | +| config (0xF5) | t_sb = 500 ms (100), filter = ×16 (100), spi3w = 0 | 0x90 | + +Write order at init: chip-ID check → calibration readout → ctrl_hum → ctrl_meas → +config. (The implementer verifies the byte encodings against the datasheet tables; +the field values above are the binding contract.) + +## Calibration data (read once at init) + +Bosch trimming parameters, parsed little-endian from calib00–25 + calib26–32: + +- `dig_T1` (u16), `dig_T2`, `dig_T3` (s16) +- `dig_P1` (u16), `dig_P2`–`dig_P9` (s16) +- `dig_H1` (u8), `dig_H2` (s16), `dig_H3` (u8), `dig_H4` (s12, split-nibble), + `dig_H5` (s12, split-nibble), `dig_H6` (s8) +- `t_fine` (s32) — temperature fine resolution, shared input to P and H + compensation; temperature MUST be compensated first each cycle. + +Re-read on every (re-)initialization — a re-attached module may be a different +physical sensor. + +## Driver state + +``` +UNINITIALIZED ──initialize()/lazy──▶ probe 0x76/0x77 ──ACK+chip-id 0x60──▶ INITIALIZED + ▲ │ fail: error 1 │ + └──────────── read()/isAvailable() detects loss (bus error) ◀───────────┘ +``` + +- Lazy (re-)initialization: `read()` and `isAvailable()` attempt `initialize()` + when uninitialized (parity, `docs/parity-checklist.md` §5). +- A bus error during read sets error 2 and marks the driver uninitialized so the + next poll re-probes both addresses (spec FR-004 recovery; covers the + swapped-address replug edge case). +- `isAvailable()` on an initialized sensor performs a real chip-ID read + (deliberate divergence from legacy cached availability — spec FR-009). + +## Concurrency + +`Bme280Sensor` is unsynchronized by design (soil-sensor convention). Cross-task +access (sensor task + console REPL now; web PR-09, controller PR-11) goes through +`LockedEnvironmentalSensor`, which serializes the interface per call. Bus-level +safety across devices (INA226, PR-05) is provided by the i2c_master driver's +per-transaction bus lock (research R3) — the decorator exists for reading-snapshot +consistency, not bus safety. diff --git a/specs/005-bme280-i2c/plan.md b/specs/005-bme280-i2c/plan.md new file mode 100644 index 0000000..b0359a2 --- /dev/null +++ b/specs/005-bme280-i2c/plan.md @@ -0,0 +1,150 @@ +# Implementation Plan: BME280 Environmental Sensor over I2C + +**Branch**: `005-bme280-i2c` | **Date**: 2026-07-02 | **Spec**: [spec.md](spec.md) + +**Input**: Feature specification from `specs/005-bme280-i2c/spec.md` + +## Summary + +Port the BME280 environmental sensor from the Adafruit library to a self-contained +driver on ESP-IDF's new `i2c_master` API. All policy — 0x76/0x77 probing, chip-ID +verification, calibration parsing, the exact legacy sampling profile (NORMAL, +T×2/P×16/H×1, IIR ×16, standby 500 ms — CP1 decision), Bosch datasheet compensation +math, NaN-fails-read, lazy re-init — lives in a pure C++ `Bme280Sensor` behind a new +minimal `II2cBus` seam, host-tested against Bosch reference vectors with a +`MockI2cBus`. The only hardware-touching class is `EspI2cBus`, which owns the single +shared bus handle (board pins, 100 kHz) that PR-05's INA226 will reuse. A dedicated +5 s sensor task (CP1 decision: env-only) publishes readings through +`LockedEnvironmentalSensor`; the console gains an `env` command for HIL. No registry +component: `espressif/bme280` was rejected on verified grounds (floating dependency, +bus-ownership conflict, un-host-testable compensation) — see [research.md](research.md) R1. + +## Technical Context + +**Language/Version**: C++ (modern, RAII, no Arduino layers) on ESP-IDF v6.0.1 +(pinned docker image `espressif/idf:v6.0.1`) + +**Primary Dependencies**: ESP-IDF `esp_driver_i2c` (new `driver/i2c_master.h` API — +the deprecated legacy `driver/i2c.h` is never included), esp_console (existing +REPL). **No new managed components** — the Bosch compensation math is implemented +in-repo (research R1); `dependencies.lock` stays unchanged. + +**Storage**: N/A (readings are RAM-only; history/logging is PR-06/PR-09 territory) + +**Testing**: host tests on IDF linux preview target (`firmware/test_apps/host`, +exit code = number of failures), CI via esp-idf-ci-action with `target: linux` +(known pitfall: default IDF_TARGET aborts `set-target linux`); HIL checklist on the +rev1 bench rig at Checkpoint 3 + +**Target Platform**: ESP32-WROOM-32E (rev1 devkit rig + rev2 custom PCB), dual +board targets via Kconfig `BOARD_REV1_DEVKIT`/`BOARD_REV2` (same I2C pins 21/22 on +both, rev2 provisional until SYNC1) + +**Project Type**: ESP-IDF component extension within existing `firmware/` project + +**Performance Goals**: one burst read (0xF7–0xFE, 8 bytes + register pointer) at +100 kHz ≪ the 5 s poll cadence; no busy-waiting; finite bus timeouts everywhere + +**Constraints**: parity contract `docs/parity-checklist.md` §5 (pins SDA 21/SCL 22, +sampling profile, °C/%RH/hPa with Pa→hPa, NaN fails read, lazy re-init, graceful +degradation); shared-bus requirement for PR-05 (spec FR-003); pumps/safety layer +untouched; implementation starts only after PR #10 (004-modbus-soil-sensor) merges +— file overlap documented in research R10 + +**Scale/Scope**: 1 I2C device (2 candidate addresses), 2 new interfaces +(`IEnvironmentalSensor`, `II2cBus`), 1 component extended (`sensors`), 1 new +console command, 1 new FreeRTOS task, 1 new host-test suite + +## Constitution Check + +*GATE: evaluated pre-Phase 0 and re-checked post-Phase 1 design — PASS (no +violations, no Complexity Tracking entries).* + +- **I. Safety First**: PASS — no pump paths touched. Contribution to safety is the + validity/error contract (FR-001/FR-007) PR-11's fail-safe will consume; note the + legacy fail-safe gates on the SOIL sensor only, so this PR adds no new safety + gate — it delivers the validity signal. Sensor task starts after + `pumps_force_off()` in the established app_main order. +- **II. Host-Testability**: PASS — all policy in pure `Bme280Sensor` behind + `II2cBus`; compensation math verified against Bosch reference vectors on the + host; only `EspI2cBus` touches IDF APIs and contains no business logic; mocks + provided; host suite extended in CI. +- **III. Reproducible Builds**: PASS — no new managed dependencies (own + compensation code was chosen partly BECAUSE the registry alternative carries a + floating `i2c_bus: "*"` dependency, research R1); `dependencies.lock` unchanged; + both board targets built in CI from clean checkout. +- **IV. Frozen Legacy**: PASS — legacy files are read-only porting reference. +- **V. Checkpoint-Gated Workflow**: PASS — CP1 held (2 questions, both answered A: + parity sampling profile; task now, env-only); this plan stops at CP2; + implementation via implementer subagent after PR #10 merges; review + CP3 before + commit/PR. +- **VI. English Outward**: PASS — all artifacts/code/commits in English. +- **Additional constraints**: pins only from the board component; `ESP_LOG*` with + component tag; include guards `WATERINGSYSTEM_*_H`; no partition changes; no + non-trivial global constructors (bus + sensor are function-local statics in + app_main). + +## Project Structure + +### Documentation (this feature) + +```text +specs/005-bme280-i2c/ +├── spec.md +├── plan.md # This file +├── research.md # Phase 0 — decisions R1–R10 +├── data-model.md # Phase 1 — reading, registers, sampling profile, errors +├── contracts/ +│ └── interfaces.md # Phase 1 — IEnvironmentalSensor, II2cBus, console, task +├── quickstart.md # Phase 1 — host-test/build/HIL validation guide +├── checklists/ +│ └── requirements.md +└── tasks.md # Phase 2 (/speckit-tasks — not created by /speckit-plan) +``` + +### Source Code (repository root) + +Planned against PR-04's file versions (worktree `004-modbus-soil-sensor`); +implementation rebases on main once PR #10 has merged (research R10). + +```text +firmware/ +├── components/ +│ ├── interfaces/include/interfaces/ +│ │ ├── IEnvironmentalSensor.h # NEW — standalone, soil-sensor style (R5) +│ │ └── II2cBus.h # NEW — minimal I2C register seam (R6) +│ ├── sensors/ # EXTENDED (component exists after PR-04) +│ │ ├── CMakeLists.txt # + Bme280Sensor.cpp (all targets), +│ │ │ # EspI2cBus.cpp (target-only, linux-excluded) +│ │ ├── include/sensors/ +│ │ │ ├── Bme280Sensor.h # pure logic: probe/chip-ID/calib/compensation +│ │ │ ├── EspI2cBus.h # i2c_master bus owner (shared with PR-05) +│ │ │ ├── LockedEnvironmentalSensor.h # mutex decorator +│ │ │ └── testing/ +│ │ │ ├── MockI2cBus.h # scripted registers/probe/failures +│ │ │ └── MockEnvironmentalSensor.h # for consumer tests (PR-11) +│ │ └── src/ +│ │ ├── Bme280Sensor.cpp +│ │ └── EspI2cBus.cpp # target-only +│ └── board/include/board/board.h # unchanged (I2C pins already defined) +├── main/ +│ ├── app_main.cpp # wire EspI2cBus + Bme280Sensor + Locked +│ │ # wrapper + sensor task (5 s, env-only) +│ ├── sensor_task.cpp/.h # NEW — app-level poller (R7) +│ └── diag_console.cpp/.h # + env command +└── test_apps/host/main/ + ├── test_bme280.cpp # NEW — Bosch vectors, probe/chip-ID, + │ # error paths, recovery, mock consistency + ├── test_main.cpp # + run_bme280_tests() + └── CMakeLists.txt # + test_bme280.cpp +``` + +**Structure Decision**: extend the `sensors` component (PR-04's home for sensor +drivers) rather than create a new component — BME280 is a sensor driver with the +same architecture split (pure logic / hardware touchpoint / Locked / mocks). The +sensor task is app wiring in `main/` (not a component) because PR-11's controller +is expected to absorb its cadence (R7). + +## Complexity Tracking + +No constitution violations — table intentionally empty. diff --git a/specs/005-bme280-i2c/quickstart.md b/specs/005-bme280-i2c/quickstart.md new file mode 100644 index 0000000..4b35ef1 --- /dev/null +++ b/specs/005-bme280-i2c/quickstart.md @@ -0,0 +1,58 @@ +# Quickstart Validation: BME280 Environmental Sensor over I2C + +How to prove the feature works, end to end. Prerequisites: docker (pinned image +`espressif/idf:v6.0.1`), repo checkout of branch `005-bme280-i2c`. + +## 1. Host tests (CI gate — compensation math + error paths) + +```bash +cd firmware/test_apps/host +docker run --rm -v "$PWD":/project -w /project espressif/idf:v6.0.1 bash -c \ + "idf.py --preview set-target linux && idf.py build && ./build/pump_host_tests.elf" +``` + +Expected: exit code 0 (= zero Unity failures); the run includes the +`test_bme280.cpp` suite — Bosch reference-vector compensation checks (incl. +negative temperature), calibration parsing, probe/chip-ID logic, absent-sensor and +mid-read-loss error paths, recovery re-probe, mock-consistency checks. + +## 2. Both board targets build green (CI gate) + +```bash +cd firmware +docker run --rm -v "$PWD":/project -w /project espressif/idf:v6.0.1 \ + idf.py -DSDKCONFIG_DEFAULTS="sdkconfig.defaults;sdkconfig.board.rev1_devkit" build +docker run --rm -v "$PWD":/project -w /project espressif/idf:v6.0.1 bash -c \ + "idf.py fullclean && idf.py -DSDKCONFIG_DEFAULTS='sdkconfig.defaults;sdkconfig.board.rev2' build" +``` + +Expected: both builds succeed from clean checkout (SC-001). `dependencies.lock` +unchanged (no new managed dependencies in this feature). + +## 3. HIL on the rev1 bench rig (Checkpoint 3, Paul) + +Full checklist: `checklists/hil.md` (created at implementation). Outline: + +1. **Periodic readings** — flash rig with BME280 attached, open monitor: a + temperature/humidity/pressure log line every 5 s with plausible values + (SC-003). +2. **Arduino agreement** — compare against the Arduino unit in the same + environment: within ±0.5 °C, ±3 %RH, ±1 hPa (SC-003; parity sampling profile + makes this like-for-like). +3. **Console** — `ws>` prompt: `env` returns the three values; output + distinguishes sensor-absent from read-failed (SC-006). +4. **Unplug/replug** — unplug the module while running: invalid readings + logged + warning, no reboot; replug: automatic recovery (SC-004). +5. **Boot without sensor** — power up with the module disconnected: normal boot, + invalid readings; attach module: readings start without restart (SC-004). +6. **Address variants** — if a 0x76-strapped module is on hand, swap it in: works + with no config change (SC-005; otherwise 0x77 verified + host-level probe-order + tests count). +7. **Regression guard** — pump console commands still work; pumps OFF at boot + (safety invariant untouched). + +## References + +- Interface contracts: [contracts/interfaces.md](contracts/interfaces.md) +- Registers, sampling profile, error codes: [data-model.md](data-model.md) +- Design decisions: [research.md](research.md) diff --git a/specs/005-bme280-i2c/research.md b/specs/005-bme280-i2c/research.md new file mode 100644 index 0000000..e8e0a66 --- /dev/null +++ b/specs/005-bme280-i2c/research.md @@ -0,0 +1,160 @@ +# Phase 0 Research: BME280 Environmental Sensor over I2C + +All decisions below resolve the open items flagged in the spec's Assumptions and the +pre-spec research report. Verification was done against authoritative sources: the +pinned toolchain container (`espressif/idf:v6.0.1`) and the IDF Component Registry +API (2026-07-02). + +## R1 — Own Bosch compensation code, not the `espressif/bme280` registry component + +**Decision**: Implement the BME280 driver in-repo: register access + Bosch datasheet +compensation math in our own pure C++ class. Do not depend on `espressif/bme280`. + +**Rationale** (registry state verified via `components.espressif.com` API, +2026-07-02): + +- Latest release is **v0.1.1 (2024-12-23)** with a **floating dependency** + `espressif/i2c_bus: "*"` — a floating range violates Constitution III + (exact pins only). We would have to pin `i2c_bus` transitively and track two + third-party components for one sensor. +- It builds on the `i2c_bus` abstraction (esp-iot-solution), which **owns bus + creation** internally. FR-003 requires one shareable native + `i2c_master_bus_handle_t` for PR-05's INA226; forcing INA226 through `i2c_bus` + or fighting over bus ownership adds an abstraction layer we don't control. +- SC-002 requires the compensation math to be host-tested against Bosch reference + vectors. A third-party component compiled target-only cannot satisfy that; our + own pure C++ compensation unit satisfies it trivially. +- Maintenance signals are weak: 0.1.x version line, sparse metadata, no examples, + no declared targets. + +**Alternatives considered**: `espressif/bme280` (rejected above); Bosch's official +`BME280_SensorAPI` vendored as C sources (viable, but it is a generic HAL-callback +C API — porting the ~200 lines of datasheet compensation into our C++ class is +simpler than wrapping the vendor HAL, and the datasheet publishes the reference +algorithms we must match anyway). + +## R2 — Legacy I2C driver status on IDF v6.0.1 (verified) + +**Finding**: `driver/i2c.h` (legacy) **still exists** in v6.0.1 +(`/opt/esp/idf/components/driver/i2c/include/driver/i2c.h` in the pinned container) +— deprecated, not removed. The earlier assumption that v6 removed it was wrong. + +**Consequence**: none for scope — the PRD/FR-002 mandates the new `esp_driver_i2c` +API (`driver/i2c_master.h`, also verified present). IDF forbids mixing legacy and +new drivers on the same port; nothing else in `firmware/` uses I2C, so the rule is +satisfied by simply never including the legacy header. + +## R3 — New i2c_master driver serializes multi-task transactions (verified) + +**Finding**: `i2c_master.c` in the pinned container takes a bus-level semaphore +(`bus_lock_mux`) around each transaction (take at `i2c_master.c:1004`, release at +`:1023`/`:1029`), so transactions issued from different tasks against devices on +the same bus serialize safely at transaction granularity. + +**Consequence**: + +- PR-05's INA226 reader (potentially another task) can share the bus handle without + extra locking at the bus layer. +- `LockedEnvironmentalSensor` (mutex decorator) is still required — but for + *snapshot consistency* of our reading (a `read()` updates three values + validity + atomically as seen by console/web/controller), not for bus safety. + +## R4 — Shared I2C bus: `EspI2cBus` class in the sensors component + +**Decision**: A target-only `EspI2cBus` class (sensors component) owns +`i2c_new_master_bus()` using the board component's pins (`BOARD_PIN_I2C_SDA`/`SCL`), +implements the `II2cBus` interface (R6), and manages per-address device handles +internally (created on first use, 100 kHz per device). `app_main` instantiates it +as a function-local static (established pattern) and passes it to the BME280 +driver; PR-05 passes the **same instance** to the INA226 driver. + +**Rationale**: the board component stays a header-only pin table (established +convention); putting bus ownership in app wiring code would spread hardware setup +logic outside driver components. The class boundary gives PR-05 a ready seam and +keeps one single owner for the bus handle. + +**Alternatives considered**: bus creation in the board component (rejected: board +is compile-time pin data, no runtime code today); raw handle created in `app_main` +(rejected: hardware calls in wiring code, no mockable seam). + +## R5 — Interface shape: standalone `IEnvironmentalSensor` (soil-sensor style) + +**Decision**: Port `IEnvironmentalSensor` as a standalone pure C++ interface in the +`interfaces` component, mirroring PR-04's `ISoilSensor` conventions: no `ISensor` +base class, no `getName()`; explicit validity contract — consumers gate on the +`read()` result; getters return last-good values after a failed read (documented: +meaningless before the first successful read); `initialize()` recommended but +lazy-capable; `isAvailable()` performs a real bus probe. + +**Rationale**: consistency across the sensor interfaces PR-11 will consume; the +legacy NaN-in-getters behavior is a foot-gun the soil-sensor contract already +removed. Confirmed direction in the spec's Assumptions (divergences documented). + +## R6 — Host-test seam: minimal `II2cBus` interface + +**Decision**: New `II2cBus` interface (interfaces component, pure C++): + +- `probe(address)` → bool/error — device ACK check (maps to `i2c_master_probe`) +- `readRegisters(address, startReg, buf, len)` → error +- `writeRegister(address, reg, value)` → error + +`Bme280Sensor` (pure logic, builds on linux target) holds all policy: 0x76/0x77 +probing order + re-probe on recovery, chip-ID verification (0xD0 == 0x60), +calibration readout/parsing, sampling configuration sequence (parity profile), +compensation math, Pa→hPa conversion, NaN check, error codes, lazy re-init. +`EspI2cBus` is the only hardware-touching class — no business logic (analogous to +PR-04's `ModbusSoilSensor`/`EspModbusClient` split). `MockI2cBus` (testing header) +scripts register maps, probe results and failure sequences for host tests. + +**Rationale**: this is the exact split Constitution II prescribes and PR-04 +established; it makes compensation *and* every error path host-testable. + +## R7 — Sensor task: app-level, env-only, parity parameters + +**Decision**: `sensor_task` in `firmware/main` (app wiring, not a component): +FreeRTOS task, 4096 B stack, priority 1, `vTaskDelayUntil` at 5000 ms (parity +values from the legacy controller task), each cycle calls +`LockedEnvironmentalSensor::read()` and logs validity transitions. Task starts even +if the sensor failed init (lazy re-init does the recovery, parity) and never exits. +Log discipline: WARN once on valid→invalid transition and on recovery; repeated +failures logged at a bounded cadence (every Nth failure) to avoid log flood. + +**Rationale**: CP1 confirmed the task is in scope, polling the environmental sensor +only. It lives in app wiring because PR-11's controller will take over or absorb +this cadence — keeping it out of the sensors component avoids baking scheduling +policy into a driver component. + +## R8 — Compensation reference vectors + +**Decision**: Test vectors come from the Bosch BME280 datasheet's reference +implementation (int32 temperature, int64 pressure, int32 humidity — the +`BME280_compensate_*` routines): a fixed set of (calibration constants, raw ADC +values) → expected physical outputs, including the datasheet's worked example and +edge vectors (negative temperature; extreme-but-legal raw values). Vectors are +baked into the host test as constants with a comment citing their derivation. +Our implementation must reproduce the reference outputs exactly (integer paths) +before the float conversion to °C/%RH/hPa. + +**Rationale**: SC-002/FR-005 make the datasheet algorithms the binding reference; +fixed vectors keep the test deterministic and hardware-free. + +## R9 — NORMAL-mode readiness at first poll + +**Decision**: After init writes the parity sampling profile (NORMAL, T×2/P×16/H×1, +IIR ×16, standby 500 ms), the first conversion completes well within one 5 s task +period, so the first scheduled poll reads real data. The driver additionally treats +all-zero/reset raw values as any other reading (compensation yields a number; no +special-casing) but MUST fail the read cleanly if the chip is mid-reset +(`readRegisters` error or chip-ID mismatch on lazy re-init). No status-register +polling loop is added — parity: the legacy driver never polled status either. + +## R10 — File overlap with PR-04 (sequencing constraint) + +**Finding**: PR-03's implementation extends files PR-04 (open PR #10) also touches: +`firmware/components/sensors/` (CMakeLists, component yml), `firmware/main/app_main.cpp`, +`diag_console.{h,cpp}`, `test_apps/host/main/{CMakeLists.txt,test_main.cpp}`, +`firmware/CLAUDE.md`. + +**Decision**: Implementation starts only after PR #10 merges; this plan is written +against PR-04's versions of those files (read from the 004 worktree). The spec/plan +phase itself is conflict-free (touches only `specs/005-*`). diff --git a/specs/005-bme280-i2c/spec.md b/specs/005-bme280-i2c/spec.md new file mode 100644 index 0000000..a088c6b --- /dev/null +++ b/specs/005-bme280-i2c/spec.md @@ -0,0 +1,318 @@ +# Feature Specification: BME280 Environmental Sensor over I2C + +**Feature Branch**: `005-bme280-i2c` + +**Created**: 2026-07-02 + +**Status**: Draft + +**Input**: User description: "Port the BME280 environmental sensor to the new ESP-IDF +i2c_master API behind a hardware-independent environmental-sensor interface, replacing +the Adafruit library, per docs/prd/PR-03-bme280-i2c.md (authoritative mini-PRD; ground +truth for behavior is docs/parity-checklist.md §5). Includes the periodic sensor task +(5 s cadence) and mock implementations for host tests." + +## Clarifications + +### Session 2026-07-02 + +- Q: Sampling profile — exact legacy configuration (parity) or a deliberate switch to + forced-mode reads suited to the 5 s cadence? → A: Parity — NORMAL mode with the + exact legacy settings (oversampling T×2 / P×16 / H×1, IIR ×16, standby 500 ms), so + the HIL comparison against the Arduino unit is like-for-like. Forced mode remains a + possible future power optimization. Confirmed by Paul at Checkpoint 1. +- Q: Deliver the periodic sensor task now (PRD-03 scope) or defer periodic polling to + PR-11 as PR-04 did? → A: Deliver the task now, polling the environmental sensor + only at 5 s; the soil-sensor cadence stays with PR-11's controller, which may take + over or reuse this task's pattern. Confirmed by Paul at Checkpoint 1. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Environmental readings on the bench rig (Priority: P1) + +Paul flashes the rev1 rig, which has the real BME280 module wired to the I2C pins. The +firmware reads temperature, humidity and pressure on a steady 5-second cadence and the +values agree with what the production Arduino unit shows for the same environment — +same units (°C, %RH, hPa), same smoothness characteristics (identical sensor sampling +configuration). From the serial console he can also trigger an immediate reading for +spot checks. + +**Why this priority**: Air temperature and humidity feed the greenhouse status display +and future watering decisions (PR-11); a driver that reproduces the production unit's +readings on the rig is the core deliverable of this PR. + +**Independent Test**: Flash the rig with the BME280 attached, watch the periodic log +output and/or issue the console reading command, compare values against the Arduino +unit in the same environment (±0.5 °C, ±3 %RH, ±1 hPa). + +**Acceptance Scenarios**: + +1. **Given** the rig with the BME280 connected, **When** the firmware runs, **Then** + temperature (°C), humidity (%RH) and pressure (hPa) are read every 5 seconds and + the values are plausible for the environment. +2. **Given** the rig and the Arduino unit side by side, **When** readings are compared, + **Then** they agree within sensor tolerance (±0.5 °C, ±3 %RH, ±1 hPa) — the sensor + is configured with the exact legacy sampling profile (continuous measurement, + oversampling T×2 / P×16 / H×1, IIR filter ×16, standby 500 ms), so filtering and + smoothness match. +3. **Given** the rig, **When** the operator runs the environmental-reading console + command, **Then** an immediate reading is taken and displayed with all three values + and their validity state. + +--- + +### User Story 2 - Sensor faults yield invalid data, never wrong data (Priority: P2) + +The BME280 module is unplugged (or fails) while the system runs. The system does not +crash, does not reboot, and does not present garbage as truth: readings are flagged +invalid with a logged warning, and when the module is plugged back in the driver +recovers on a subsequent poll without a restart. The same applies at boot: a missing +sensor never blocks startup — the system comes up, keeps retrying, and starts +delivering readings as soon as the sensor appears. + +**Why this priority**: Constitution principle I (Safety First) — downstream automatic +watering logic (PR-11) must be able to trust the validity signal. Graceful degradation +on sensor loss is parity behavior (`docs/parity-checklist.md` §5). + +**Independent Test**: On the rig, unplug the BME280 mid-operation; observe invalid +readings + logged warnings and no reboot; replug and observe automatic recovery +without restart. Boot with the sensor absent; observe normal startup and recovery +when attached. + +**Acceptance Scenarios**: + +1. **Given** a running system with a connected sensor, **When** the module is + unplugged and the next poll occurs, **Then** the read fails with a distinct error, + the reading is flagged invalid, a warning is logged, and the system keeps running + (no crash, no watchdog reset). +2. **Given** an unplugged sensor, **When** the module is reconnected, **Then** a + subsequent poll succeeds without any manual intervention or restart. +3. **Given** a system booting with no sensor attached, **When** startup completes, + **Then** the system runs normally with readings flagged invalid, and attaching the + sensor later leads to automatic recovery (lazy re-initialization, parity). +4. **Given** a sensor whose measurement comes back unusable (not-a-number after + compensation), **When** the reading is evaluated, **Then** the read fails with a + distinct error and no partial values are presented as valid (parity: NaN fails the + read). +5. **Given** a failed read, **When** downstream code checks the reading, **Then** + validity is unambiguous — consumers gate on the read result, never on sniffing + value contents. + +--- + +### User Story 3 - Works with both module address variants (Priority: P3) + +Paul (or a future builder) wires up a BME280 breakout. Some modules ship strapped to +I2C address 0x76, others to 0x77 (the greenhouse unit's module is at 0x77). The +firmware finds the sensor on either address automatically and verifies it is really a +BME280 (chip identity check) before trusting it — no code change or build flag needed +to swap modules. + +**Why this priority**: Removes a hardware-assembly foot-gun. This is a deliberate new +capability (the legacy firmware hard-codes 0x77); it makes the rig and rev2 builds +robust to module sourcing. + +**Independent Test**: Attach a 0x76-strapped module and a 0x77-strapped module in +turn; both are detected and deliver readings without any configuration change. + +**Acceptance Scenarios**: + +1. **Given** a module strapped to 0x76, **When** the driver initializes, **Then** the + sensor is found, its chip identity is verified, and readings flow. +2. **Given** a module strapped to 0x77, **When** the driver initializes, **Then** the + same happens. +3. **Given** a device at a probed address that is not a BME280 (wrong chip identity), + **When** the driver initializes, **Then** that device is rejected and the sensor is + reported unavailable rather than misread. + +--- + +### User Story 4 - Sensor behavior testable without hardware (Priority: P4) + +An AI developer changes compensation, validation or error-handling logic and runs the +host test suite in CI. The raw-to-physical compensation math is verified against the +sensor vendor's published reference vectors, and the invalid-data paths (bus errors, +absent sensor, unusable values) are verified against mock implementations — no devkit, +no sensor, failures block the merge. + +**Why this priority**: Constitution principle II (Host-Testability). The mock +environmental sensor created here is also an input for PR-11's watering-controller +tests. + +**Independent Test**: Run the host test suite on a machine with no hardware attached; +compensation and error-path tests pass deterministically. + +**Acceptance Scenarios**: + +1. **Given** the vendor datasheet's example calibration constants and raw readings, + **When** the compensation math runs on the host, **Then** the computed temperature, + humidity and pressure match the datasheet's reference results. +2. **Given** a mocked bus/sensor that reports a communication error, **When** a + reading is requested, **Then** the result is invalid with a distinct error and no + stale values leak through as fresh. +3. **Given** a mock environmental sensor scripted with known values, **When** a + consumer (e.g. a future controller test) reads it, **Then** it observes exactly the + scripted values and validity states. +4. **Given** CI on a clean checkout, **When** the host tests run, **Then** they + complete without any hardware or device-specific environment. + +--- + +### Edge Cases + +- Sensor disappears *between* the availability check and the read, or mid-transaction: + the read fails with an error — never a hang, never a partially-updated "valid" + reading. +- Sensor is replugged at the *other* address (module swapped 0x76 ↔ 0x77 while + unpowered): recovery re-probes both addresses, so the swap is transparent. +- A different I2C device ACKs at 0x76/0x77 (address collision): chip identity check + rejects it; the driver keeps reporting unavailable instead of decoding garbage. +- First reading immediately after initialization: in continuous-measurement mode the + first data may not be ready instantly; the first poll must yield either a valid + reading or a clean invalid result — never garbage from empty registers. +- Concurrent access (periodic task + console command, later web server): reads and + accessor calls serialize safely; torn reads (temperature from one cycle, humidity + from another presented as one snapshot) must not happen across the shared-access + boundary. +- The I2C bus will be shared with another device family later (INA226, PR-05): bus + setup is one-time and additional devices must be attachable without reworking this + driver. +- Repeated failed polls (sensor absent for hours): no resource leak, no log flood + beyond a reasonable warning cadence, and the retry cost stays bounded. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: Environmental sensor functionality MUST be exposed through a + hardware-independent interface (pure C++, no hardware-SDK types) providing + temperature (°C), humidity (%RH), pressure (hPa) and explicit validity/availability + reporting, usable from host tests without any hardware headers. The interface + contract MUST follow the conventions established by the existing soil-sensor + interface: consumers gate on the read result; accessors return last-good values + after a failed read; initialization is lazy-capable. +- **FR-002**: The driver MUST communicate over the I2C bus defined by the board + component (rev1: SDA 21, SCL 22) at 100 kHz standard mode, using the current + (non-deprecated) platform I2C master driver — not the legacy I2C API. +- **FR-003**: The I2C bus MUST be created once and be shareable: additional devices + (INA226 in PR-05) attach to the same bus without changes to this driver or bus + teardown/re-init. +- **FR-004**: At initialization the driver MUST locate the sensor by probing I2C + addresses 0x76 and 0x77 and MUST verify the chip identity before use; a responding + device with the wrong identity is rejected (sensor reported unavailable). Recovery + after sensor loss MUST re-probe both addresses. +- **FR-005**: The driver MUST read the sensor's factory calibration data and apply the + vendor-specified compensation to produce physical values: temperature in °C, + relative humidity in %RH, pressure in hPa (parity: legacy converts Pa → hPa). The + compensation math MUST be host-testable against the vendor datasheet's reference + vectors. +- **FR-006**: Sensor sampling configuration MUST match the legacy unit exactly + (parity, `docs/parity-checklist.md` §5): continuous measurement (NORMAL mode), + oversampling T×2 / P×16 / H×1, IIR filter ×16, standby 500 ms — set explicitly, not + relying on any library default. +- **FR-007**: A failed read (bus/communication error, absent sensor, or a + compensation result that is not a number) MUST flag the reading invalid with a + distinct error and MUST NOT crash, hang, or present partial values as valid + (parity: NaN fails the read). No numeric range validation is applied beyond the + NaN check (parity: the legacy driver validates nothing else). +- **FR-008**: A sensor absent at boot MUST NOT block startup; the driver MUST support + lazy (re-)initialization so that a sensor attached or re-attached later is picked up + on a subsequent poll without restart (parity, `docs/parity-checklist.md` §5). +- **FR-009**: Availability reporting MUST reflect reality: an availability check on an + initialized sensor MUST be able to detect that the sensor has disappeared (deliberate + divergence from legacy, which caches "available" forever after first init — aligned + with the soil-sensor contract where availability checks touch the bus). +- **FR-010**: A dedicated periodic sensor task MUST poll the environmental sensor + every 5 seconds (parity cadence) and publish readings for other parts of the system; + shared access MUST be protected so concurrent readers (console now, web server in + PR-09, controller in PR-11) never observe torn or interleaved state (established + mutex-decorator pattern). The task keeps running and retrying through sensor + failures. +- **FR-011**: A serial console command MUST exist on the rig to trigger and display an + immediate environmental reading including validity state — the HIL verification + path, following the existing console command pattern. +- **FR-012**: A mock environmental sensor MUST exist for host tests, sufficient to + script known values, validity states and failure sequences for consumer tests; the + driver's pure logic (compensation, error paths) MUST be host-testable behind a + mockable seam. +- **FR-013**: Both board targets MUST build green in CI with the component enabled; + hardware-touching code MUST be excluded from the host-test build following the + established component split. + +### Key Entities + +- **Environmental reading**: temperature (°C), humidity (%RH), pressure (hPa) plus + validity/error state; produced atomically per poll — one reading is one snapshot. +- **Environmental sensor interface**: the hardware-independent contract (read, + availability, last-good accessors, error reporting) consumed by the sensor task, + console, and later PRs (PR-09 web, PR-11 controller). +- **I2C bus**: the single shared bus instance defined by board pins; owner of device + attachments (BME280 now, INA226 in PR-05). +- **Sensor task**: the periodic poller (5 s) that turns the driver into + continuously-published readings under concurrency protection. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Both board variants build green in CI from a clean checkout with the new + component enabled. +- **SC-002**: Host test suite verifies the compensation math against the vendor + datasheet's reference vectors and the invalid-data paths (bus error, absent sensor, + NaN) with zero hardware dependencies, passing deterministically in CI. +- **SC-003**: On the rig, temperature/humidity/pressure are logged every 5 seconds and + agree with the Arduino unit's readings in the same environment within ±0.5 °C, + ±3 %RH, ±1 hPa (HIL checklist). +- **SC-004**: Unplugging the BME280 on the running rig yields invalid readings and a + logged warning with zero crashes or reboots; replugging recovers automatically + without restart; booting without the sensor and attaching it later also recovers + (HIL checklist). +- **SC-005**: Modules strapped to 0x76 and 0x77 both work with no configuration change + (HIL checklist, if both module variants are on hand; otherwise verified for the + available variant plus host-level probe-order tests). +- **SC-006**: The console reading command works on the first attempt using documented + syntax and its output is sufficient to distinguish "sensor absent" from "sensor + present but read failed" (HIL checklist). + +## Assumptions + +- **Sampling profile stays on parity** (continuous NORMAL mode with the exact legacy + oversampling/filter/standby settings) rather than switching to on-demand + forced-mode reads — confirmed at Checkpoint 1 (see Clarifications). Rationale: the + HIL acceptance criterion compares readings against the Arduino unit — identical + filtering makes that comparison meaningful. Forced mode remains a possible future + optimization (power) once the parity baseline is proven. +- **Interface style follows the soil-sensor precedent**, not the legacy `ISensor` + hierarchy: a standalone environmental-sensor interface with an explicit validity + contract (gate on read result, last-good values retained after failure). This is a + documented divergence from the legacy driver, which leaves NaN in its accessors + after a failed read; the legacy behavior is a foot-gun the new contract removes. + The PRD's mention of `ISensor`/`IEnvironmentalSensor` is read as "the interface + role", not a mandate for a base-class hierarchy — final shape is a plan-phase + decision. +- **Availability semantics diverge deliberately from legacy**: legacy caches + "available" after first init and can report a dead sensor as alive; the new driver + aligns with the soil-sensor contract (availability checks reflect the bus truth). + The unplug/replug HIL criterion depends on this. +- **Address probing is a new capability, not parity**: legacy hard-codes 0x77 (the + greenhouse unit's module). Probe order and re-probe-on-recovery behavior are as + specified in FR-004; the bench rig's module is at 0x77. +- **Registry component vs own compensation code is a plan-phase decision** (PRD + explicitly defers it). The binding requirements are observable: compensation math + host-tested against vendor reference vectors (FR-005) and the new-driver-API + mandate (FR-002). Note the host-testability criterion weighs toward own + compensation code; the registry component's IDF v6.0.1 compatibility is unverified. +- **Where the bus instance lives** (board layer, sensor component, or app wiring) is + a plan-phase decision; the binding requirement is FR-003 (one bus, shareable with + PR-05). +- **The sensor task polls the environmental sensor only** — confirmed at + Checkpoint 1 (see Clarifications). The legacy task also read the soil sensor; here + the soil read cadence stays out of scope (PR-04 delivered on-demand reads; the + periodic soil reader and fail-safe consumption arrive with the controller in + PR-11, which may then take over or consume this task's pattern). +- **Web exposure and data logging are out of scope** (PR-09/PR-06 territory); the + reading contract (cached last-good values, validity flag) is designed so PR-09 can + serve cached values without blocking on a fresh read (parity QUIRK 5). +- **No numeric plausibility-range validation** on temperature/humidity/pressure + (parity: legacy checks only NaN). Range validation would be a behavior change; + downstream consumers may add their own guards. diff --git a/specs/005-bme280-i2c/tasks.md b/specs/005-bme280-i2c/tasks.md new file mode 100644 index 0000000..1138721 --- /dev/null +++ b/specs/005-bme280-i2c/tasks.md @@ -0,0 +1,87 @@ +# Tasks: BME280 Environmental Sensor over I2C + +**Input**: Design documents from `specs/005-bme280-i2c/` +**Prerequisites**: plan.md, research.md (R1–R10), data-model.md, contracts/interfaces.md, quickstart.md + +**Tests**: Host tests are explicitly required by the spec (SC-002, Constitution II) — test +tasks are in scope and follow the harness conventions in `firmware/test_apps/host/`. + +**Organization**: Tasks are grouped by user story (US1–US4 from spec.md). Hard +sequencing constraint (research R10): implementation starts only after **PR #10 +(004-modbus-soil-sensor) has merged** — the `sensors` component, `diag_console` +surface and host-test harness this feature extends arrive with it. + +## Phase 1: Setup + +- [ ] T001 Rebase/merge branch `005-bme280-i2c` onto origin/main AFTER PR #10 has merged; verify `firmware/components/sensors/`, `diag_console_register_soil` and `test_apps/host/main/test_soil_sensor.cpp` exist on the branch; verify both board targets and the host suite build green from clean checkout (baseline before any change) + +## Phase 2: Foundational (blocking prerequisites for all user stories) + +- [ ] T002 [P] Create `IEnvironmentalSensor` interface in `firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h` — standalone pure C++ (no IDF includes), soil-sensor conventions: initialize/read/isAvailable/getLastError + getTemperature (°C)/getHumidity (%RH)/getPressure (hPa), last-good-value validity contract in doc comments per `contracts/interfaces.md` +- [ ] T003 [P] Create `II2cBus` interface in `firmware/components/interfaces/include/interfaces/II2cBus.h` — probe(address7)/readRegisters(address7, startReg, buf, len)/writeRegister(address7, reg, value), repeated-start semantics documented, no IDF includes, per `contracts/interfaces.md` +- [ ] T004 Create `MockI2cBus` in `firmware/components/sensors/include/sensors/testing/MockI2cBus.h` — header-only scriptable register map (chip-ID, calibration block, data registers), probe results per address, failure injection (NACK on address, error on register read/write, corrupt data), call/write recording for config-byte assertions + +## Phase 3: User Story 1 — Environmental readings on the bench rig (P1) 🎯 MVP + +**Goal**: BME280 read every 5 s on the rig with parity sampling profile; console `env` command; values match the Arduino unit. + +**Independent Test**: flash rig, watch 5 s log cadence, run `env`, compare against Arduino unit (±0.5 °C, ±3 %RH, ±1 hPa). + +- [ ] T005 [US1] Implement `Bme280Sensor` (pure logic) in `firmware/components/sensors/include/sensors/Bme280Sensor.h` + `firmware/components/sensors/src/Bme280Sensor.cpp`: constructor takes `II2cBus&`; initialize() = probe 0x76→0x77, chip-ID check (0xD0 == 0x60), calibration readout/parse (dig_T/P/H incl. split-nibble H4/H5), sampling profile writes ctrl_hum(0xF2)=0x01 → ctrl_meas(0xF4)=0x57 → config(0xF5)=0x90 (data-model.md — verify byte encodings against Bosch datasheet field tables); read() = burst 0xF7–0xFE, compensate T→P→H via t_fine (Bosch int32/int64 reference algorithms), convert to °C/%RH/hPa (P: ÷256 ÷100), NaN → error 2; error codes 0/1/2 per data-model.md +- [ ] T006 [P] [US1] Host tests for init + happy-path read in `firmware/test_apps/host/main/test_bme280.cpp` (new suite `run_bme280_tests()`): scripted MockI2cBus with full register map → initialize() succeeds, exact config bytes written in order (ctrl_hum before ctrl_meas), read() produces expected values; getters-before-first-read documented behavior +- [ ] T007 [P] [US1] Implement `EspI2cBus` in `firmware/components/sensors/include/sensors/EspI2cBus.h` + `firmware/components/sensors/src/EspI2cBus.cpp` (target-only): owns `i2c_master_bus_handle_t` from `BOARD_PIN_I2C_SDA/SCL`, port auto, internal pull-ups on; per-address device handles created on first use at 100 kHz; probe→`i2c_master_probe`, readRegisters→`i2c_master_transmit_receive`, writeRegister→`i2c_master_transmit`; finite timeouts; errors returned as false, logged at debug; PRIV_REQUIRES `esp_driver_i2c` +- [ ] T008 [P] [US1] Implement `LockedEnvironmentalSensor` in `firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h` — header-only FreeRTOS-mutex decorator, same pattern as `LockedSoilSensor` (per-call serialization; document the read-then-getters pattern limitation + PR-11 snapshot bookkeeping) +- [ ] T009 [US1] Update `firmware/components/sensors/CMakeLists.txt`: add `Bme280Sensor.cpp` to all-target SRCS, `EspI2cBus.cpp` + `esp_driver_i2c` dep inside the existing `if(NOT ${IDF_TARGET} STREQUAL "linux")` guard +- [ ] T010 [US1] Implement sensor task in `firmware/main/sensor_task.cpp` + `firmware/main/sensor_task.h`: `sensor_task_start(IEnvironmentalSensor&)` creates FreeRTOS task (4096 B stack, priority 1, `vTaskDelayUntil` 5000 ms); each cycle read() + INFO log of values on success; WARN once on valid→invalid transition and on recovery, repeated failures at bounded cadence (every Nth); task starts even when sensor absent, never exits (contracts/interfaces.md) +- [ ] T011 [US1] Add `env` console command: `diag_console_register_env(IEnvironmentalSensor&)` in `firmware/main/diag_console.h` + handler in `firmware/main/diag_console.cpp` — thin wrapper, one read(), success prints the three values with units, failure prints `ERROR ` + hint distinguishing code 1 (not found) from code 2 (read failed) per contracts/interfaces.md +- [ ] T012 [US1] Wire it all in `firmware/main/app_main.cpp`: function-local static `EspI2cBus` → `Bme280Sensor` → `LockedEnvironmentalSensor`; `diag_console_register_env(...)` before `diag_console_start()`; `sensor_task_start(...)` after console registration; init failure = log-and-continue (non-safety subsystem), everything after `pumps_force_off()` — follow the PR-04 wiring pattern + +**Checkpoint**: rig delivers periodic readings + `env` works → MVP demonstrable. + +## Phase 4: User Story 2 — Sensor faults yield invalid data, never wrong data (P2) + +**Goal**: unplug/replug and boot-without-sensor are non-events: invalid + logged, auto-recovery, no reboot. + +**Independent Test**: unplug mid-run → invalid + warning, no crash; replug → recovery; boot sensorless → normal boot, later attach recovers. + +- [ ] T013 [US2] Harden `Bme280Sensor` failure semantics in `firmware/components/sensors/src/Bme280Sensor.cpp`: bus error during read → error 2, getters keep last-good values, driver marked uninitialized (next call re-probes — recovery path); failed probe/chip-ID → error 1; lazy re-init from read() AND isAvailable() when uninitialized; isAvailable() = real chip-ID read, never cached, never touches lastError (contracts/interfaces.md, data-model.md state machine) +- [ ] T014 [P] [US2] Host tests for error paths in `firmware/test_apps/host/main/test_bme280.cpp`: absent sensor (probe NACK both addresses) → initialize false + error 1; mid-read bus error → read false + error 2 + last-good values intact + subsequent recovery re-probe succeeds; NaN-producing raw values → error 2; boot-sensorless then attach → lazy re-init delivers reading; isAvailable true/false against scripted bus, error code untouched +- [ ] T015 [US2] Verify sensor-task logging discipline against a scripted failing/recovering sensor: WARN on transitions, bounded repeat cadence, no task exit — host-test the pure log-decision helper if extracted, otherwise verify by targeted code review note in the task (logging policy per research R7) + +## Phase 5: User Story 3 — Works with both module address variants (P3) + +**Goal**: 0x76- and 0x77-strapped modules both just work; wrong chip identity rejected. + +**Independent Test**: scripted buses with the device at each address; wrong-chip-ID device rejected (host); module swap on rig if hardware available. + +- [ ] T016 [US3] Verify/complete probing policy in `firmware/components/sensors/src/Bme280Sensor.cpp`: probe order 0x76 → 0x77, first ACK + correct chip-ID wins; ACK with wrong chip-ID logged distinctly and rejected (continues to next candidate, else error 1); recovery after loss re-probes BOTH addresses (covers swapped-address replug edge case) +- [ ] T017 [P] [US3] Host tests for address handling in `firmware/test_apps/host/main/test_bme280.cpp`: device at 0x76 found; device at 0x77 found; wrong-chip-ID at 0x76 with real device at 0x77 → 0x77 chosen; wrong-chip-ID everywhere → error 1; loss then reappearance at the OTHER address → recovered + +## Phase 6: User Story 4 — Sensor behavior testable without hardware (P4) + +**Goal**: compensation math proven against Bosch reference vectors; mocks ready for PR-11 consumers; all of it in CI. + +**Independent Test**: host suite passes on a hardware-less machine; CI job green. + +- [ ] T018 [US4] Add Bosch reference-vector tests in `firmware/test_apps/host/main/test_bme280.cpp`: fixed (calibration set, raw T/P/H) → expected compensated outputs from the Bosch datasheet reference implementation (int32 T, int64 P, int32 H), incl. the datasheet worked example, a negative-temperature vector and extreme-but-legal raws; each vector's derivation cited in a comment (research R8); float conversions (÷100, ÷256÷100, ÷1024) asserted +- [ ] T019 [P] [US4] Create `MockEnvironmentalSensor` in `firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h` — scripted value/validity/error sequences WITH consistency helpers `scriptSuccessfulRead(t,h,p)` / `scriptFailedRead(error)` from the start (PR-04 lesson), for PR-11 consumer tests +- [ ] T020 [P] [US4] Host tests for MockEnvironmentalSensor consistency in `firmware/test_apps/host/main/test_bme280.cpp`: scripted sequences observed exactly; helpers keep values/validity/error coherent +- [ ] T021 [US4] Register the suite: add `test_bme280.cpp` to SRCS in `firmware/test_apps/host/main/CMakeLists.txt`, declare + call `run_bme280_tests()` in `firmware/test_apps/host/main/test_main.cpp` between UNITY_BEGIN/UNITY_END + +## Phase 7: Polish & Cross-Cutting + +- [ ] T022 [P] Update `firmware/CLAUDE.md`: directory tree (Bme280Sensor/EspI2cBus/Locked/testing files, sensor_task), console command list (`env`), a "BME280 environmental sensor" section (architecture split, shared-bus note for PR-05, parity sampling profile), host-test description +- [ ] T023 [P] Record deliberate divergences in `docs/parity-checklist.md` §6: address probing + chip-ID check, last-good getter values (legacy: NaN), live availability probe (legacy: cached), Locked-decorator synchronization (legacy: unsynchronized dual readers) — per contracts/interfaces.md list +- [ ] T024 [P] Write HIL checklist `specs/005-bme280-i2c/checklists/hil.md` following `specs/004-modbus-soil-sensor/checklists/hil.md` format: (A) periodic readings + Arduino agreement, (B) console `env` incl. absent-vs-failed distinction, (C) unplug/replug + boot-without-sensor, (D) 0x76 module swap (if hardware available, else mark host-covered), (E) regression guard (pumps OFF at boot, pump/soil console commands intact) +- [ ] T025 Full verification per `specs/005-bme280-i2c/quickstart.md`: host suite exit 0; rev1 AND rev2 build green from clean checkout (fullclean between overlays); `dependencies.lock` unchanged; capture outputs for the CP3 dossier + +## Dependencies & Execution Order + +- **Phase 1 → Phase 2 → Phase 3**: strictly sequential gates (T001 is a hard external gate on PR #10). +- **US1 (Phase 3)** is the MVP and blocks nothing after it: US2 (T013 hardening) modifies files created in US1; US3 refines the same probing code (T016 after T013 — same file); US4's vector tests (T018) only need T005. +- **Parallel within phases**: T002/T003 together; T006/T007/T008 after T005; T014 after T013; T017 after T016; T019/T020 anytime after T002; T022/T023/T024 together. +- **Story order**: US1 → US2 → US3 → US4 → Polish. (US4's T018 may start as soon as T005 exists if an extra pair of hands is available.) + +## Implementation Strategy + +MVP = Phase 1–3 (US1): rig shows parity readings every 5 s + `env` command — demonstrable and HIL-testable on its own. US2/US3 harden the same driver files incrementally (each leaves the suite green). US4 locks the math and the consumer-facing mocks. Polish updates docs and produces the HIL checklist Paul runs at Checkpoint 3. From ec14e9b880bdb21723150c3f3c867e157998e312 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 16:33:48 +0200 Subject: [PATCH 2/5] feat(sensors): BME280 driver on i2c_master with env console command and 5 s sensor task Pure-logic Bme280Sensor (0x76/0x77 probing, chip-ID check, Bosch datasheet compensation, parity NORMAL-mode sampling profile) behind new IEnvironmentalSensor/II2cBus interfaces; EspI2cBus owns the shared master bus (100 kHz, board pins) for PR-05 reuse; LockedEnvironmentalSensor decorator, MockI2cBus, env console command and app-level 5 s sensor task. Host suite extends to 86 tests incl. Bosch reference vectors (T002-T012, T021). Co-Authored-By: Claude Fable 5 --- .../include/interfaces/IEnvironmentalSensor.h | 109 +++++ .../interfaces/include/interfaces/II2cBus.h | 79 ++++ firmware/components/sensors/CMakeLists.txt | 35 +- .../sensors/include/sensors/Bme280Sensor.h | 183 +++++++++ .../sensors/include/sensors/EspI2cBus.h | 88 +++++ .../sensors/LockedEnvironmentalSensor.h | 115 ++++++ .../include/sensors/testing/MockI2cBus.h | 149 +++++++ .../components/sensors/src/Bme280Sensor.cpp | 372 ++++++++++++++++++ firmware/components/sensors/src/EspI2cBus.cpp | 166 ++++++++ firmware/main/CMakeLists.txt | 2 +- firmware/main/app_main.cpp | 31 ++ firmware/main/diag_console.cpp | 64 +++ firmware/main/diag_console.h | 12 + firmware/main/sensor_task.cpp | 101 +++++ firmware/main/sensor_task.h | 35 ++ firmware/test_apps/host/main/CMakeLists.txt | 1 + firmware/test_apps/host/main/test_bme280.cpp | 259 ++++++++++++ firmware/test_apps/host/main/test_main.cpp | 2 + specs/005-bme280-i2c/tasks.md | 24 +- 19 files changed, 1799 insertions(+), 28 deletions(-) create mode 100644 firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h create mode 100644 firmware/components/interfaces/include/interfaces/II2cBus.h create mode 100644 firmware/components/sensors/include/sensors/Bme280Sensor.h create mode 100644 firmware/components/sensors/include/sensors/EspI2cBus.h create mode 100644 firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h create mode 100644 firmware/components/sensors/include/sensors/testing/MockI2cBus.h create mode 100644 firmware/components/sensors/src/Bme280Sensor.cpp create mode 100644 firmware/components/sensors/src/EspI2cBus.cpp create mode 100644 firmware/main/sensor_task.cpp create mode 100644 firmware/main/sensor_task.h create mode 100644 firmware/test_apps/host/main/test_bme280.cpp diff --git a/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h b/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h new file mode 100644 index 0000000..18a686d --- /dev/null +++ b/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h @@ -0,0 +1,109 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file IEnvironmentalSensor.h + * @brief Interface for the BME280 environmental sensor (T/RH/P). + * + * Ported from the frozen Arduino firmware + * (include/sensors/IEnvironmentalSensor.h) in the soil-sensor style: no + * ISensor base class, no getName() (research.md R5). Normative contract: + * specs/005-bme280-i2c/contracts/interfaces.md; registers, sampling profile + * and error codes: specs/005-bme280-i2c/data-model.md. + * + * Deliberate divergences from the legacy driver (parity-checklist §6): + * 0x76/0x77 address probing with a chip-identity check (legacy hard-codes + * 0x77, no identity check); last-good getter values after a failed read + * (legacy left NaN in the getters); isAvailable() as a real bus probe + * (legacy cached available-after-init forever). + * + * Validity contract (FR-001/FR-007): a false return from read() means the + * data is invalid. The getters keep returning the last-good values, so + * consumers MUST gate on the read result / getLastError() — never on value + * plausibility. + * + * Concurrency: implementations are unsynchronized by design; cross-task + * consumers (sensor task + console REPL) wrap them in the + * LockedEnvironmentalSensor decorator, same pattern as LockedSoilSensor. + * + * Part of the header-only `interfaces` component: no IDF includes allowed. + */ + +#ifndef WATERINGSYSTEM_INTERFACES_IENVIRONMENTALSENSOR_H +#define WATERINGSYSTEM_INTERFACES_IENVIRONMENTALSENSOR_H + +/** + * @brief Environmental sensor: atomic T/RH/P snapshots with lazy recovery. + * + * Error codes reported by getLastError() + * (specs/005-bme280-i2c/data-model.md): + * + * 0 OK — last operation succeeded + * 1 sensor not found — no device ACK on 0x76/0x77, or a responding + * device failed the chip-identity check + * 2 read failed — bus/communication error during a data read, or the + * compensation produced NaN + */ +class IEnvironmentalSensor { +public: + virtual ~IEnvironmentalSensor() = default; + + /** + * @brief Find and configure the sensor. + * + * Probes address 0x76 then 0x77, verifies the chip identity (register + * 0xD0 == 0x60), reads the calibration data and writes the parity + * sampling profile (ctrl_hum → ctrl_meas → config, data-model.md). + * Returns false with error 1 when no BME280 is found. Idempotent, and + * lazy-capable: read()/isAvailable() attempt initialization themselves + * when it has not happened yet — calling this first is recommended, + * not required (parity). + */ + virtual bool initialize() = 0; + + /** + * @brief Take ONE atomic snapshot of temperature, humidity and pressure. + * + * Burst-reads the data registers 0xF7–0xFE in one bus transaction and + * compensates T → P → H (t_fine ordering). Atomic: either all three + * getters are refreshed, or the call fails (error 1 on lost/absent + * sensor, error 2 on bus error or NaN) and the last-good values remain + * untouched. A bus error marks the driver uninitialized, so the next + * call re-probes both addresses (recovery, FR-004). Exactly one bus + * attempt, no retry — recovery comes from the caller's poll cadence. + * + * @return true if a fully valid reading was taken. + */ + virtual bool read() = 0; + + /** + * @brief Probe sensor presence with a REAL chip-ID read (FR-009). + * + * Every call performs an actual bus transaction — never cached state + * (deliberate divergence from the legacy cached availability). Does not + * modify getLastError(). Recovery from earlier failures is implicit: a + * sensor that identifies itself again is available again. + */ + virtual bool isAvailable() = 0; + + /** + * @brief Error code of the most recent initialize()/read() (0 = OK; + * table in the class comment). isAvailable() never touches this code. + */ + virtual int getLastError() = 0; + + // Values from the most recent successful read(). Before the FIRST + // successful read() they are meaningless placeholders, and after a + // failed read() they still hold the previous good reading — consumers + // gate on the read() result, never on the values. + + /// Temperature in °C. + virtual float getTemperature() = 0; + + /// Relative humidity in %RH. + virtual float getHumidity() = 0; + + /// Barometric pressure in hPa (parity: legacy converts Pa → hPa). + virtual float getPressure() = 0; +}; + +#endif /* WATERINGSYSTEM_INTERFACES_IENVIRONMENTALSENSOR_H */ diff --git a/firmware/components/interfaces/include/interfaces/II2cBus.h b/firmware/components/interfaces/include/interfaces/II2cBus.h new file mode 100644 index 0000000..c8f7b0e --- /dev/null +++ b/firmware/components/interfaces/include/interfaces/II2cBus.h @@ -0,0 +1,79 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file II2cBus.h + * @brief Minimal register-oriented I2C master interface (hardware seam). + * + * The host-test seam for I2C sensor drivers (research.md R6): Bme280Sensor + * holds all policy above this interface and is host-tested against + * MockI2cBus; EspI2cBus is the only hardware-touching implementation. + * Normative contract: specs/005-bme280-i2c/contracts/interfaces.md. + * + * This interface carries no BME280 knowledge — PR-05's INA226 driver reuses + * it on the same bus instance (INA226 uses 16-bit register values; PR-05 + * may extend the interface or compose two 8-bit operations, its call). + * + * Part of the header-only `interfaces` component: no IDF includes allowed. + */ + +#ifndef WATERINGSYSTEM_INTERFACES_II2CBUS_H +#define WATERINGSYSTEM_INTERFACES_II2CBUS_H + +#include +#include + +/** + * @brief I2C master: probe + 8-bit register reads/writes, one transaction + * per call. + * + * All addresses are 7-bit. Every method returns false on NACK, bus error + * or timeout; there are NO retries at this layer — recovery policy belongs + * to the sensor driver above. Implementations serialize safely for + * multi-task use at transaction granularity (the ESP implementation + * inherits this from the i2c_master driver's bus lock, research.md R3; + * mocks are used single-threaded in host tests). + */ +class II2cBus { +public: + virtual ~II2cBus() = default; + + /** + * @brief Check whether a device ACKs at @p address7. + * + * @param address7 7-bit device address. + * @return true if the device acknowledged its address. + */ + virtual bool probe(uint8_t address7) = 0; + + /** + * @brief Read @p len consecutive registers starting at @p startReg. + * + * One transaction: register-pointer write followed by an N-byte read + * with a REPEATED START in between — required for correct BME280 burst + * reads (the chip's register shadowing guarantees a consistent + * measurement only within a single burst transaction). + * + * @param address7 7-bit device address. + * @param startReg First register address. + * @param buf Caller-owned buffer of at least @p len bytes; contents are + * defined only when the call returns true. + * @param len Number of registers (bytes) to read. + * @return true if all requested bytes were read. + */ + virtual bool readRegisters(uint8_t address7, uint8_t startReg, + uint8_t* buf, size_t len) = 0; + + /** + * @brief Write one byte to one register (register pointer + value in a + * single transaction). + * + * @param address7 7-bit device address. + * @param reg Register address. + * @param value Value to write. + * @return true if the device acknowledged the write. + */ + virtual bool writeRegister(uint8_t address7, uint8_t reg, + uint8_t value) = 0; +}; + +#endif /* WATERINGSYSTEM_INTERFACES_II2CBUS_H */ diff --git a/firmware/components/sensors/CMakeLists.txt b/firmware/components/sensors/CMakeLists.txt index 224f082..2cd1d78 100644 --- a/firmware/components/sensors/CMakeLists.txt +++ b/firmware/components/sensors/CMakeLists.txt @@ -1,28 +1,33 @@ -# sensors — Modbus soil sensor driver layer. +# sensors — sensor driver layer (RS485 Modbus soil sensor + I2C BME280). # -# ModbusSoilSensor.cpp is pure C++ (decode/validation/calibration logic) and -# builds on every target, including the linux preview target used by the host -# test suite. EspModbusClient.cpp is the only hardware touchpoint (esp-modbus -# master + UART RS485 half-duplex + RX pull-up) and is excluded — together -# with its driver/esp-modbus dependencies — when building for linux -# (research.md R7, same mechanism as storage/actuators). +# ModbusSoilSensor.cpp and Bme280Sensor.cpp are pure C++ (decode/validation/ +# calibration resp. probe/compensation logic) and build on every target, +# including the linux preview target used by the host test suite. +# EspModbusClient.cpp (esp-modbus master + UART RS485 half-duplex + RX +# pull-up) and EspI2cBus.cpp (i2c_master bus owner) are the only hardware +# touchpoints and are excluded — together with their driver/esp-modbus +# dependencies — when building for linux (research.md R7/005 R6, same +# mechanism as storage/actuators). if(${IDF_TARGET} STREQUAL "linux") idf_component_register( - SRCS "src/ModbusSoilSensor.cpp" + SRCS "src/ModbusSoilSensor.cpp" "src/Bme280Sensor.cpp" INCLUDE_DIRS "include" REQUIRES interfaces board ) else() - # Target build only: the esp-modbus client. esp-modbus is a managed - # component (pinned in this component's idf_component.yml, rule-gated to - # non-linux targets; registered under its namespaced component name). - # PRIV: esp-modbus and driver headers appear only in - # src/EspModbusClient.cpp / private code, never in this component's - # public headers (same rule as storage's littlefs). + # Target build only: the esp-modbus client and the I2C bus. esp-modbus + # is a managed component (pinned in this component's idf_component.yml, + # rule-gated to non-linux targets; registered under its namespaced + # component name). PRIV: esp-modbus, driver and i2c_master headers + # appear only in src/EspModbusClient.cpp / src/EspI2cBus.cpp / private + # code, never in this component's public headers (same rule as + # storage's littlefs). idf_component_register( - SRCS "src/ModbusSoilSensor.cpp" "src/EspModbusClient.cpp" + SRCS "src/ModbusSoilSensor.cpp" "src/Bme280Sensor.cpp" + "src/EspModbusClient.cpp" "src/EspI2cBus.cpp" INCLUDE_DIRS "include" REQUIRES interfaces board PRIV_REQUIRES espressif__esp-modbus esp_driver_uart esp_driver_gpio + esp_driver_i2c ) endif() diff --git a/firmware/components/sensors/include/sensors/Bme280Sensor.h b/firmware/components/sensors/include/sensors/Bme280Sensor.h new file mode 100644 index 0000000..fb3c6fe --- /dev/null +++ b/firmware/components/sensors/include/sensors/Bme280Sensor.h @@ -0,0 +1,183 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file Bme280Sensor.h + * @brief Pure C++ BME280 driver logic over an injected II2cBus. + * + * Ported from the frozen Arduino firmware (src/sensors/BME280Sensor.cpp, + * read-only reference), replacing the Adafruit library with the Bosch + * BME280 datasheet reference compensation (research.md R1). Registers, + * sampling profile, calibration layout and error codes are normative in + * specs/005-bme280-i2c/data-model.md. + * + * This class holds ALL policy — 0x76/0x77 probing order, chip-ID + * verification, calibration readout/parsing (incl. the split-nibble + * dig_H4/dig_H5), the exact legacy sampling profile, compensation math, + * unit conversion, the NaN check, error codes, lazy re-initialization and + * uninitialize-on-bus-error recovery. It contains NO hardware access — + * every bus transaction goes through the injected II2cBus, so all of it is + * compiled and unit-tested on the IDF linux preview target against + * MockI2cBus (constitution II). + * + * Concurrency: unsynchronized by design (host-testable); cross-task + * consumers (sensor task + console REPL) wrap it in + * LockedEnvironmentalSensor and access it only through the wrapper. + */ + +#ifndef WATERINGSYSTEM_SENSORS_BME280SENSOR_H +#define WATERINGSYSTEM_SENSORS_BME280SENSOR_H + +#include +#include + +#include "interfaces/IEnvironmentalSensor.h" +#include "interfaces/II2cBus.h" + +/** + * @brief IEnvironmentalSensor over the BME280 I2C register map. + * + * One read() = one 8-byte burst transaction (0xF7–0xFE), compensated + * T → P → H via t_fine (Bosch int32/int64 reference algorithms) and + * converted to °C/%RH/hPa atomically: on any failure the last-good getter + * values remain untouched and getLastError() carries the cause (0/1/2, + * data-model.md). + */ +class Bme280Sensor : public IEnvironmentalSensor { +public: + /// Probing order: 0x76 first, then 0x77 (data-model.md; the bench rig + /// module is strapped to 0x77, the greenhouse legacy unit too). + static constexpr uint8_t kPrimaryAddress = 0x76; + static constexpr uint8_t kSecondaryAddress = 0x77; + + /// Chip identity: register 0xD0 must read 0x60 (BME280 — a BMP280 + /// would report 0x58 and is rejected: it has no humidity path). + static constexpr uint8_t kChipId = 0x60; + + /** + * @brief Bosch trimming parameters (datasheet table "compensation + * parameter storage"), parsed little-endian from calib00–25 (0x88–0xA1) + * and calib26–32 (0xE1–0xE7). + * + * Public together with the static compensate*() functions so the host + * test suite can drive the integer reference paths directly with fixed + * calibration vectors (research.md R8). + */ + struct Calibration { + uint16_t digT1; + int16_t digT2; + int16_t digT3; + uint16_t digP1; + int16_t digP2; + int16_t digP3; + int16_t digP4; + int16_t digP5; + int16_t digP6; + int16_t digP7; + int16_t digP8; + int16_t digP9; + uint8_t digH1; + int16_t digH2; + uint8_t digH3; + int16_t digH4; ///< 12-bit signed, split-nibble (0xE4 / 0xE5[3:0]) + int16_t digH5; ///< 12-bit signed, split-nibble (0xE5[7:4] / 0xE6) + int8_t digH6; + }; + + /** + * @brief Construct the sensor over an injected I2C bus. + * + * @param bus I2C master used for every transaction; must outlive this + * object (same injection style as ModbusSoilSensor's + * IModbusClient). + */ + explicit Bme280Sensor(II2cBus& bus); + + ~Bme280Sensor() override = default; + + Bme280Sensor(const Bme280Sensor&) = delete; + Bme280Sensor& operator=(const Bme280Sensor&) = delete; + + // IEnvironmentalSensor + bool initialize() override; + bool read() override; + bool isAvailable() override; + int getLastError() override; + + float getTemperature() override; + float getHumidity() override; + float getPressure() override; + + // Bosch BME280 datasheet reference compensation (section "Compensation + // formulas", 32/64-bit integer implementations) — transcribed exactly; + // host-tested against reference vectors (research.md R8). Static and + // public so the vector tests exercise the integer paths before the + // float conversion. + + /** + * @brief BME280_compensate_T_int32: temperature in 0.01 °C (output + * 5123 = 51.23 °C). Writes t_fine, the shared fine-resolution + * temperature input of the P and H compensation — temperature MUST be + * compensated first each cycle. + */ + static int32_t compensateTemperature(int32_t adcT, const Calibration& cal, + int32_t& tFine); + + /** + * @brief BME280_compensate_P_int64: pressure in Pa as unsigned Q24.8 + * (output 24674867 = 24674867/256 = 96386.2 Pa). Returns 0 when the + * calibration would divide by zero (datasheet reference behavior). + */ + static uint32_t compensatePressure(int32_t adcP, const Calibration& cal, + int32_t tFine); + + /** + * @brief bme280_compensate_H_int32: humidity in %RH as unsigned Q22.10 + * (output 47445 = 47445/1024 = 46.333 %RH), clamped to 0–100 %RH by the + * reference algorithm itself. + */ + static uint32_t compensateHumidity(int32_t adcH, const Calibration& cal, + int32_t tFine); + +private: + // Register map (used subset, data-model.md). + static constexpr uint8_t kRegChipId = 0xD0; + static constexpr uint8_t kRegCalib00 = 0x88; ///< calib00–25 block start + static constexpr size_t kCalib00Len = 26; ///< 0x88–0xA1 + static constexpr uint8_t kRegCalib26 = 0xE1; ///< calib26–32 block start + static constexpr size_t kCalib26Len = 7; ///< 0xE1–0xE7 + static constexpr uint8_t kRegCtrlHum = 0xF2; + static constexpr uint8_t kRegCtrlMeas = 0xF4; + static constexpr uint8_t kRegConfig = 0xF5; + static constexpr uint8_t kRegData = 0xF7; ///< press/temp/hum burst start + static constexpr size_t kDataLen = 8; ///< 0xF7–0xFE + + // Parity sampling profile (legacy src/sensors/BME280Sensor.cpp:41-46, + // docs/parity-checklist.md §5; byte encodings verified against the + // Bosch datasheet register tables — field breakdowns at the write site + // in Bme280Sensor.cpp). + static constexpr uint8_t kCtrlHumValue = 0x01; ///< osrs_h ×1 + static constexpr uint8_t kCtrlMeasValue = 0x57; ///< T×2, P×16, NORMAL + static constexpr uint8_t kConfigValue = 0x90; ///< 500 ms, IIR ×16 + + /// Probe 0x76 → 0x77 and verify the chip identity; sets address_. + bool probeAndIdentify(); + + /// Burst-read and parse both calibration blocks into cal_. + bool readCalibration(); + + /// Write the parity sampling profile (ctrl_hum → ctrl_meas → config). + bool writeSamplingProfile(); + + II2cBus& bus_; + uint8_t address_ = 0; ///< resolved device address (valid when initialized_) + bool initialized_ = false; + int lastError_ = 0; + Calibration cal_{}; + + // Last-good reading (published only by a fully successful read()). + float temperature_ = 0.0f; + float humidity_ = 0.0f; + float pressure_ = 0.0f; +}; + +#endif /* WATERINGSYSTEM_SENSORS_BME280SENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/EspI2cBus.h b/firmware/components/sensors/include/sensors/EspI2cBus.h new file mode 100644 index 0000000..f0f045c --- /dev/null +++ b/firmware/components/sensors/include/sensors/EspI2cBus.h @@ -0,0 +1,88 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file EspI2cBus.h + * @brief II2cBus over ESP-IDF's i2c_master driver (esp_driver_i2c). + * + * ESP32-ONLY: excluded from the linux-target build together with the + * esp_driver_i2c dependency (see this component's CMakeLists.txt). This is + * the I2C hardware touchpoint — all sensor policy lives above II2cBus + * (research.md R6, same split as EspModbusClient). + * + * PRIV rule: driver/i2c_master.h appears only in the .cpp, never here — + * the bus and device handles are held as opaque pointers. The NEW + * i2c_master API is used exclusively; the deprecated legacy driver/i2c.h + * is never included (FR-002, research.md R2). + * + * Bus sharing (FR-003): app_main constructs ONE EspI2cBus (function-local + * static) and passes it to every I2C driver — PR-05's INA226 receives the + * same instance. No second bus creation on these pins is permitted. Pins + * come from board/board.h inside the .cpp (BOARD_PIN_I2C_SDA/SCL). + * + * Concurrency: transaction-level safety across tasks comes from the + * i2c_master driver's per-transaction bus lock (research.md R3). Reading- + * snapshot consistency above this layer is LockedEnvironmentalSensor's + * job, not this class's. + */ + +#ifndef WATERINGSYSTEM_SENSORS_ESPI2CBUS_H +#define WATERINGSYSTEM_SENSORS_ESPI2CBUS_H + +#include +#include + +#include "interfaces/II2cBus.h" + +/** + * @brief I2C master on the board's SDA/SCL pins (100 kHz standard mode). + * + * The bus handle is created lazily on first use (no work in the + * constructor — no error path there); per-address device handles are + * created on first use at 100 kHz (FR-002) and cached. All transactions + * use finite timeouts; failures are returned as false and logged at debug + * level — error classification is the sensor driver's job. + */ +class EspI2cBus : public II2cBus { +public: + /// Standard-mode clock per device (FR-002). + static constexpr uint32_t kSclSpeedHz = 100000; + + /// Finite per-transaction timeout — no infinite waits on a wedged bus. + static constexpr int kTimeoutMs = 100; + + EspI2cBus() = default; + + /// Removes all device handles and deletes the master bus. + ~EspI2cBus() override; + + EspI2cBus(const EspI2cBus&) = delete; + EspI2cBus& operator=(const EspI2cBus&) = delete; + + // II2cBus + bool probe(uint8_t address7) override; + bool readRegisters(uint8_t address7, uint8_t startReg, uint8_t* buf, + size_t len) override; + bool writeRegister(uint8_t address7, uint8_t reg, uint8_t value) override; + +private: + /// Enough for BME280 (one of two addresses) + PR-05's INA226 devices. + static constexpr size_t kMaxDevices = 8; + + struct Device { + uint8_t address = 0; + void* handle = nullptr; ///< opaque i2c_master_dev_handle_t + }; + + /// Create the master bus on first use (board pins, port auto, + /// internal pull-ups on). Returns false when creation fails. + bool ensureBus(); + + /// Cached-or-created device handle for @p address7; nullptr on failure. + void* deviceHandle(uint8_t address7); + + void* busHandle_ = nullptr; ///< opaque i2c_master_bus_handle_t + Device devices_[kMaxDevices] = {}; + size_t deviceCount_ = 0; +}; + +#endif /* WATERINGSYSTEM_SENSORS_ESPI2CBUS_H */ diff --git a/firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h b/firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h new file mode 100644 index 0000000..eb2c7ff --- /dev/null +++ b/firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h @@ -0,0 +1,115 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file LockedEnvironmentalSensor.h + * @brief Mutex-serializing IEnvironmentalSensor decorator (header-only). + * + * WHY THIS EXISTS: the environmental sensor is reached from more than one + * FreeRTOS task — the 5 s sensor task polls read() while the diag console + * REPL task issues `env` commands (web server in PR-09 and the watering + * controller in PR-11 add more readers). Bme280Sensor itself is + * deliberately unsynchronized pure C++ (host-testable), so its plain float + * members would race: read() publishes three values member-by-member, and + * a concurrent getter could observe a half-published reading (fresh + * temperature with stale pressure). This decorator wraps an + * IEnvironmentalSensor and takes a mutex around every interface call, + * serializing all access (pattern of LockedSoilSensor/LockedWaterPump). + * + * Bus-level safety across I2C devices (INA226, PR-05) is provided by the + * i2c_master driver's per-transaction bus lock (research.md R3) — this + * decorator exists for reading-snapshot consistency, not bus safety. + * + * USAGE RULE: once a sensor is wrapped, the underlying sensor must ONLY be + * accessed through the wrapper — every call site (boot wiring, console + * registration, sensor task, controllers, ...) goes through the + * LockedEnvironmentalSensor, never through the wrapped object directly. + * + * SCOPE: this decorator provides PER-CALL atomicity only, not cross-call. + * A read-then-getters sequence spanning multiple calls (read() then + * getTemperature()/getHumidity()/getPressure()) is NOT protected against + * an interleaving read() from another task in between — another task may + * refresh (or invalidate) the values first. Such sequences need + * higher-level coordination. + * TODO(PR-11): add a consistent-snapshot helper (one locked call returning + * all three values + validity) when the controller becomes a second + * periodic reader — same bookkeeping as LockedSoilSensor's PR-11 notes. + * + * Pure C++ ( is available via pthread on ESP-IDF and on the linux + * preview target), so the decorator is host-testable. + */ + +#ifndef WATERINGSYSTEM_SENSORS_LOCKEDENVIRONMENTALSENSOR_H +#define WATERINGSYSTEM_SENSORS_LOCKEDENVIRONMENTALSENSOR_H + +#include + +#include "interfaces/IEnvironmentalSensor.h" + +/** + * @brief IEnvironmentalSensor decorator that serializes every call with a + * mutex. + * + * Composition, not inheritance from a concrete sensor: the base class + * stays pure (no locking) and the host tests are unchanged. The wrapped + * sensor must outlive this object. + */ +class LockedEnvironmentalSensor : public IEnvironmentalSensor { +public: + /// Wrap @p sensor; the wrapped sensor must outlive this object. + explicit LockedEnvironmentalSensor(IEnvironmentalSensor& sensor) + : sensor_(sensor) + { + } + + LockedEnvironmentalSensor(const LockedEnvironmentalSensor&) = delete; + LockedEnvironmentalSensor& operator=(const LockedEnvironmentalSensor&) = + delete; + + bool initialize() override + { + std::lock_guard lock(mutex_); + return sensor_.initialize(); + } + + bool read() override + { + std::lock_guard lock(mutex_); + return sensor_.read(); + } + + bool isAvailable() override + { + std::lock_guard lock(mutex_); + return sensor_.isAvailable(); + } + + int getLastError() override + { + std::lock_guard lock(mutex_); + return sensor_.getLastError(); + } + + float getTemperature() override + { + std::lock_guard lock(mutex_); + return sensor_.getTemperature(); + } + + float getHumidity() override + { + std::lock_guard lock(mutex_); + return sensor_.getHumidity(); + } + + float getPressure() override + { + std::lock_guard lock(mutex_); + return sensor_.getPressure(); + } + +private: + IEnvironmentalSensor& sensor_; + mutable std::mutex mutex_; +}; + +#endif /* WATERINGSYSTEM_SENSORS_LOCKEDENVIRONMENTALSENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/testing/MockI2cBus.h b/firmware/components/sensors/include/sensors/testing/MockI2cBus.h new file mode 100644 index 0000000..ecf02de --- /dev/null +++ b/firmware/components/sensors/include/sensors/testing/MockI2cBus.h @@ -0,0 +1,149 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file MockI2cBus.h + * @brief Scriptable II2cBus test double (header-only). + * + * Serves the Bme280Sensor host tests: script a 256-byte register map per + * device address (chip-ID, calibration block, data registers), control + * probe results via device presence, inject failures (NACK on an absent + * address, queued error on register read/write; corrupt data is simply + * scripted wrong register bytes), and assert on the recorded call log + * (config bytes written in order, burst-read shape, re-probe on recovery). + * Never compiled into target builds (only included from test code). No IDF + * includes. + */ + +#ifndef WATERINGSYSTEM_SENSORS_TESTING_MOCKI2CBUS_H +#define WATERINGSYSTEM_SENSORS_TESTING_MOCKI2CBUS_H + +#include +#include +#include +#include +#include + +#include "interfaces/II2cBus.h" + +/** + * @brief II2cBus over scripted per-address register maps, instrumented for + * tests. + * + * Device presence drives the natural failure mode: probe() ACKs only + * addresses added with addDevice(), and readRegisters()/writeRegister() + * against an absent address fail (address NACK). For failures on a PRESENT + * device (mid-read bus error), queue outcomes with queueReadOutcome()/ + * queueWriteOutcome(): the front of the queue is consumed per call; an + * empty queue means success. Writes land in the register map, so config + * sequences are observable both in `calls` and in the map itself. + */ +class MockI2cBus : public II2cBus { +public: + /// One recorded bus transaction (in `calls`, chronological). + struct Call { + enum class Type { Probe, Read, Write }; + Type type; + uint8_t address; + uint8_t reg; ///< startReg (reads) / register (writes); 0 for probes + size_t len; ///< byte count (reads); 1 for writes; 0 for probes + uint8_t value; ///< written value (writes); 0 otherwise + bool succeeded; ///< outcome reported to the caller + }; + + // Instrumentation (public, MockModbusClient style). + std::vector calls; ///< every probe/read/write, in call order + + // -- Scripting ---------------------------------------------------------- + + /// Make @p address7 present (ACKs probes) with an all-zero register map. + void addDevice(uint8_t address7) { devices_[address7]; } + + /// Remove @p address7: probes NACK, reads/writes fail (unplug scenario). + /// The register map is discarded — re-adding yields a fresh device. + void removeDevice(uint8_t address7) { devices_.erase(address7); } + + /// Script one register byte on a device (added implicitly if absent). + void setRegister(uint8_t address7, uint8_t reg, uint8_t value) + { + devices_[address7][reg] = value; + } + + /// Script consecutive register bytes starting at @p startReg (the device + /// is added implicitly if absent; the block must fit below 0x100). + void setRegisters(uint8_t address7, uint8_t startReg, + const std::vector& values) + { + auto& map = devices_[address7]; + for (size_t i = 0; i < values.size(); ++i) { + map[startReg + i] = values[i]; + } + } + + /** + * @brief Queue the outcome of the NEXT readRegisters() call on a + * PRESENT device (FIFO; empty queue = success). Queue e.g. + * {false, true} for a fail-then-recover scenario. + */ + void queueReadOutcome(bool ok) { readOutcomes_.push_back(ok); } + + /// Same as queueReadOutcome() for writeRegister() calls. + void queueWriteOutcome(bool ok) { writeOutcomes_.push_back(ok); } + + // -- II2cBus ------------------------------------------------------------- + + bool probe(uint8_t address7) override + { + const bool present = devices_.count(address7) != 0; + calls.push_back({Call::Type::Probe, address7, 0, 0, 0, present}); + return present; + } + + bool readRegisters(uint8_t address7, uint8_t startReg, uint8_t* buf, + size_t len) override + { + Call call{Call::Type::Read, address7, startReg, len, 0, false}; + const auto device = devices_.find(address7); + if (device == devices_.end() || !nextOutcome(readOutcomes_)) { + calls.push_back(call); + return false; + } + for (size_t i = 0; i < len; ++i) { + buf[i] = device->second[static_cast(startReg + i)]; + } + call.succeeded = true; + calls.push_back(call); + return true; + } + + bool writeRegister(uint8_t address7, uint8_t reg, uint8_t value) override + { + Call call{Call::Type::Write, address7, reg, 1, value, false}; + const auto device = devices_.find(address7); + if (device == devices_.end() || !nextOutcome(writeOutcomes_)) { + calls.push_back(call); + return false; + } + device->second[reg] = value; + call.succeeded = true; + calls.push_back(call); + return true; + } + +private: + /// Consume the front of @p queue; an empty queue means success. + static bool nextOutcome(std::vector& queue) + { + if (queue.empty()) { + return true; + } + const bool ok = queue.front(); + queue.erase(queue.begin()); + return ok; + } + + std::map> devices_; + std::vector readOutcomes_; + std::vector writeOutcomes_; +}; + +#endif /* WATERINGSYSTEM_SENSORS_TESTING_MOCKI2CBUS_H */ diff --git a/firmware/components/sensors/src/Bme280Sensor.cpp b/firmware/components/sensors/src/Bme280Sensor.cpp new file mode 100644 index 0000000..cab6239 --- /dev/null +++ b/firmware/components/sensors/src/Bme280Sensor.cpp @@ -0,0 +1,372 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file Bme280Sensor.cpp + * @brief BME280 probe/calibration/compensation logic (pure C++). + * + * Uses ESP_LOG only — the log component is simulated on the IDF linux + * preview target, so this file builds and runs in the host test suite + * (same rule as sensors/ModbusSoilSensor.cpp). + * + * Port reference: src/sensors/BME280Sensor.cpp (frozen Arduino firmware, + * read-only). Legacy behaviors ported: lazy (re-)initialization from + * read()/isAvailable(), sampling profile (NORMAL, T×2/P×16/H×1, IIR ×16, + * standby 500 ms), Pa → hPa conversion, NaN-fails-read (error 2), error + * codes 1 (not found) / 2 (read failed). The Adafruit library is replaced + * by the Bosch datasheet reference compensation, transcribed exactly + * (research.md R1/R8). + * + * Deliberate divergences (specs/005-bme280-i2c/contracts/interfaces.md, + * parity-checklist §6 candidates): 0x76/0x77 probing + chip-ID check + * (legacy hard-codes 0x77, no identity check); last-good getter values + * after a failed read (legacy left NaN in the getters); isAvailable() as a + * real chip-ID probe (legacy cached available-after-init forever); + * bus-error recovery re-probes BOTH addresses (covers a module replugged + * at the other address). + */ + +#include "sensors/Bme280Sensor.h" + +#include + +#include "esp_log.h" + +static const char *TAG = "bme280"; + +namespace { + +/// Little-endian 16-bit assembly from two calibration bytes. +uint16_t le16(uint8_t lsb, uint8_t msb) +{ + return static_cast(static_cast(msb) << 8 | lsb); +} + +} // namespace + +Bme280Sensor::Bme280Sensor(II2cBus& bus) + : bus_(bus) +{ +} + +bool Bme280Sensor::initialize() +{ + // Idempotent, like the legacy sensor (:30-32). + if (initialized_) { + return true; + } + + // No BME280 answers (or only wrong-identity devices answer): error 1, + // "sensor not found" (legacy error 1). + if (!probeAndIdentify()) { + lastError_ = 1; + ESP_LOGW(TAG, "initialize failed: no BME280 found (0x%02x/0x%02x)", + kPrimaryAddress, kSecondaryAddress); + return false; + } + + // A device that identified as a BME280 but then fails mid-init is a + // bus/communication failure, not "not found": error 2. The driver + // stays uninitialized, so the next attempt re-probes from scratch — + // calibration is re-read on EVERY (re-)initialization because a + // re-attached module may be a different physical sensor + // (data-model.md). + if (!readCalibration()) { + lastError_ = 2; + ESP_LOGW(TAG, "initialize failed: calibration readout error at 0x%02x", + address_); + return false; + } + + if (!writeSamplingProfile()) { + lastError_ = 2; + ESP_LOGW(TAG, "initialize failed: sampling profile write error at " + "0x%02x", address_); + return false; + } + + initialized_ = true; + lastError_ = 0; + ESP_LOGI(TAG, "BME280 initialized at 0x%02x (NORMAL, T x2 / P x16 / " + "H x1, IIR x16, standby 500 ms)", address_); + return true; +} + +bool Bme280Sensor::probeAndIdentify() +{ + // Probing policy (data-model.md): 0x76 first, then 0x77; the first + // address that ACKs AND identifies as a BME280 wins. A device that + // ACKs but reports a foreign chip ID is logged distinctly and skipped + // (US3 — e.g. a BMP280 or another part strapped to the same address). + static constexpr uint8_t kCandidates[] = {kPrimaryAddress, + kSecondaryAddress}; + for (const uint8_t candidate : kCandidates) { + if (!bus_.probe(candidate)) { + continue; + } + uint8_t chipId = 0; + if (!bus_.readRegisters(candidate, kRegChipId, &chipId, 1)) { + ESP_LOGW(TAG, "device at 0x%02x ACKed but chip-ID read failed", + candidate); + continue; + } + if (chipId != kChipId) { + ESP_LOGW(TAG, "device at 0x%02x is not a BME280 (chip ID 0x%02x, " + "expected 0x%02x) — rejected", candidate, chipId, + kChipId); + continue; + } + address_ = candidate; + return true; + } + return false; +} + +bool Bme280Sensor::readCalibration() +{ + // Two burst reads cover all trimming parameters (datasheet memory map): + // calib00–25 (0x88–0xA1) and calib26–32 (0xE1–0xE7). All multi-byte + // parameters are little-endian; dig_H4/dig_H5 are 12-bit signed values + // sharing the split-nibble register 0xE5. + uint8_t block1[kCalib00Len] = {}; + uint8_t block2[kCalib26Len] = {}; + if (!bus_.readRegisters(address_, kRegCalib00, block1, kCalib00Len) || + !bus_.readRegisters(address_, kRegCalib26, block2, kCalib26Len)) { + return false; + } + + cal_.digT1 = le16(block1[0], block1[1]); + cal_.digT2 = static_cast(le16(block1[2], block1[3])); + cal_.digT3 = static_cast(le16(block1[4], block1[5])); + cal_.digP1 = le16(block1[6], block1[7]); + cal_.digP2 = static_cast(le16(block1[8], block1[9])); + cal_.digP3 = static_cast(le16(block1[10], block1[11])); + cal_.digP4 = static_cast(le16(block1[12], block1[13])); + cal_.digP5 = static_cast(le16(block1[14], block1[15])); + cal_.digP6 = static_cast(le16(block1[16], block1[17])); + cal_.digP7 = static_cast(le16(block1[18], block1[19])); + cal_.digP8 = static_cast(le16(block1[20], block1[21])); + cal_.digP9 = static_cast(le16(block1[22], block1[23])); + // block1[24] is calib24 (0xA0), unused padding in the map. + cal_.digH1 = block1[25]; + + cal_.digH2 = static_cast(le16(block2[0], block2[1])); + cal_.digH3 = block2[2]; + // dig_H4: bits [11:4] in 0xE4, bits [3:0] in 0xE5[3:0]; dig_H5: bits + // [3:0] in 0xE5[7:4], bits [11:4] in 0xE6. Sign extension of the + // 12-bit values comes from the int8_t cast of the top byte before the + // ×16 shift (Bosch reference driver parsing). + cal_.digH4 = static_cast( + static_cast(static_cast(block2[3])) * 16 | + static_cast(block2[4] & 0x0F)); + cal_.digH5 = static_cast( + static_cast(static_cast(block2[5])) * 16 | + static_cast(block2[4] >> 4)); + cal_.digH6 = static_cast(block2[6]); + return true; +} + +bool Bme280Sensor::writeSamplingProfile() +{ + // Parity sampling profile (data-model.md; legacy + // src/sensors/BME280Sensor.cpp:41-46). Byte encodings verified against + // the Bosch BME280 datasheet register tables ("ctrl_hum", "ctrl_meas", + // "config"). Order matters: the datasheet requires ctrl_hum to be + // written BEFORE ctrl_meas — changes to ctrl_hum take effect only + // after a write to ctrl_meas. + // + // ctrl_hum (0xF2) = 0x01: osrs_h[2:0] = 001 (humidity oversampling ×1) + // ctrl_meas (0xF4) = 0x57: osrs_t[7:5] = 010 (temperature ×2), + // osrs_p[4:2] = 101 (pressure ×16), + // mode[1:0] = 11 (NORMAL) + // → 010'101'11 = 0x57 + // config (0xF5) = 0x90: t_sb[7:5] = 100 (standby 500 ms), + // filter[4:2] = 100 (IIR ×16), + // spi3w_en[0] = 0 + // → 100'100'00 = 0x90 + return bus_.writeRegister(address_, kRegCtrlHum, kCtrlHumValue) && + bus_.writeRegister(address_, kRegCtrlMeas, kCtrlMeasValue) && + bus_.writeRegister(address_, kRegConfig, kConfigValue); +} + +bool Bme280Sensor::read() +{ + // Lazy (re-)initialization (legacy :55-59). A failure here is already + // logged inside initialize() and lastError_ is set there (1 or 2). + if (!initialized_ && !initialize()) { + return false; + } + + // One 8-byte burst 0xF7–0xFE, single bus attempt (the chip shadows the + // data registers during a burst, so the raw values form one consistent + // measurement). + uint8_t raw[kDataLen] = {}; + if (!bus_.readRegisters(address_, kRegData, raw, kDataLen)) { + // Bus error: error 2 AND back to UNINITIALIZED so the next call + // re-probes both addresses (FR-004 recovery; the last-good getter + // values remain untouched). + lastError_ = 2; + initialized_ = false; + ESP_LOGW(TAG, "read failed: bus error at 0x%02x — sensor lost, " + "will re-probe", address_); + return false; + } + + // Raw ADC assembly (datasheet "raw data readout"): pressure and + // temperature are 20-bit (msb, lsb, xlsb[7:4]), humidity is 16-bit. + const int32_t adcP = static_cast(raw[0]) << 12 | + static_cast(raw[1]) << 4 | + static_cast(raw[2]) >> 4; + const int32_t adcT = static_cast(raw[3]) << 12 | + static_cast(raw[4]) << 4 | + static_cast(raw[5]) >> 4; + const int32_t adcH = static_cast(raw[6]) << 8 | + static_cast(raw[7]); + + // Compensate T first — t_fine feeds P and H (data-model.md). + int32_t tFine = 0; + const int32_t t = compensateTemperature(adcT, cal_, tFine); + const uint32_t p = compensatePressure(adcP, cal_, tFine); + const uint32_t h = compensateHumidity(adcH, cal_, tFine); + + // Unit conversion (data-model.md): T 0.01 °C → °C; P Q24.8 Pa → Pa → + // hPa (parity: legacy converts Pa → hPa); H Q22.10 → %RH. + const float temperature = static_cast(t) / 100.0f; + const float pressure = static_cast(p) / 256.0f / 100.0f; + const float humidity = static_cast(h) / 1024.0f; + + // Parity NaN check (legacy :66-69): a NaN fails the whole read with + // error 2. Unreachable with the integer compensation paths above, but + // it is the binding legacy contract and stays as a safety net. Not a + // bus loss — the driver stays initialized. + if (std::isnan(temperature) || std::isnan(humidity) || + std::isnan(pressure)) { + lastError_ = 2; + ESP_LOGW(TAG, "read failed: compensation produced NaN"); + return false; + } + + // Publish atomically w.r.t. this object: plain members are fine — + // cross-task exclusion is LockedEnvironmentalSensor's job. + temperature_ = temperature; + humidity_ = humidity; + pressure_ = pressure; + + lastError_ = 0; + return true; +} + +bool Bme280Sensor::isAvailable() +{ + // Lazy initialization doubles as the probe (parity, legacy :75-80). + if (!initialized_) { + return initialize(); + } + + // REAL chip-ID read on every call, never cached (FR-009 — deliberate + // divergence from the legacy cached availability). Does not touch + // lastError_ — read() owns the reading's error state. + uint8_t chipId = 0; + const bool available = + bus_.readRegisters(address_, kRegChipId, &chipId, 1) && + chipId == kChipId; + if (!available) { + // Loss detected: back to UNINITIALIZED so the next call re-probes + // both addresses (same recovery path as a failed read()). + initialized_ = false; + ESP_LOGW(TAG, "availability probe failed at 0x%02x — sensor lost, " + "will re-probe", address_); + } + return available; +} + +int Bme280Sensor::getLastError() +{ + return lastError_; +} + +float Bme280Sensor::getTemperature() +{ + return temperature_; +} + +float Bme280Sensor::getHumidity() +{ + return humidity_; +} + +float Bme280Sensor::getPressure() +{ + return pressure_; +} + +// --------------------------------------------------------------------------- +// Bosch BME280 datasheet reference compensation, transcribed exactly from +// the datasheet's 32/64-bit integer routines (BME280_compensate_T_int32, +// BME280_compensate_P_int64, bme280_compensate_H_int32). Do not "clean up" +// these expressions: bit-exact equivalence with the reference is the +// binding requirement (FR-005/SC-002), verified by host-test vectors +// (research.md R8). +// --------------------------------------------------------------------------- + +int32_t Bme280Sensor::compensateTemperature(int32_t adcT, + const Calibration& cal, + int32_t& tFine) +{ + const int32_t var1 = + (((adcT >> 3) - (static_cast(cal.digT1) << 1)) * + static_cast(cal.digT2)) >> 11; + const int32_t var2 = + (((((adcT >> 4) - static_cast(cal.digT1)) * + ((adcT >> 4) - static_cast(cal.digT1))) >> 12) * + static_cast(cal.digT3)) >> 14; + tFine = var1 + var2; + // Resolution 0.01 °C: output 5123 = 51.23 °C. + return (tFine * 5 + 128) >> 8; +} + +uint32_t Bme280Sensor::compensatePressure(int32_t adcP, + const Calibration& cal, + int32_t tFine) +{ + int64_t var1 = static_cast(tFine) - 128000; + int64_t var2 = var1 * var1 * static_cast(cal.digP6); + var2 = var2 + ((var1 * static_cast(cal.digP5)) << 17); + var2 = var2 + (static_cast(cal.digP4) << 35); + var1 = ((var1 * var1 * static_cast(cal.digP3)) >> 8) + + ((var1 * static_cast(cal.digP2)) << 12); + var1 = (((static_cast(1) << 47) + var1) * + static_cast(cal.digP1)) >> 33; + if (var1 == 0) { + // Avoid division by zero (datasheet reference behavior). + return 0; + } + int64_t p = 1048576 - adcP; + p = (((p << 31) - var2) * 3125) / var1; + var1 = (static_cast(cal.digP9) * (p >> 13) * (p >> 13)) >> 25; + var2 = (static_cast(cal.digP8) * p) >> 19; + p = ((p + var1 + var2) >> 8) + (static_cast(cal.digP7) << 4); + // Unsigned Q24.8 Pa: output 24674867 = 96386.2 Pa. + return static_cast(p); +} + +uint32_t Bme280Sensor::compensateHumidity(int32_t adcH, + const Calibration& cal, + int32_t tFine) +{ + int32_t v = tFine - static_cast(76800); + v = ((((adcH << 14) - (static_cast(cal.digH4) << 20) - + (static_cast(cal.digH5) * v)) + + static_cast(16384)) >> 15) * + (((((((v * static_cast(cal.digH6)) >> 10) * + (((v * static_cast(cal.digH3)) >> 11) + + static_cast(32768))) >> 10) + + static_cast(2097152)) * + static_cast(cal.digH2) + + 8192) >> 14); + v = v - (((((v >> 15) * (v >> 15)) >> 7) * + static_cast(cal.digH1)) >> 4); + v = (v < 0) ? 0 : v; + v = (v > 419430400) ? 419430400 : v; + // Unsigned Q22.10 %RH: output 47445 = 46.333 %RH. + return static_cast(v >> 12); +} diff --git a/firmware/components/sensors/src/EspI2cBus.cpp b/firmware/components/sensors/src/EspI2cBus.cpp new file mode 100644 index 0000000..0201fa0 --- /dev/null +++ b/firmware/components/sensors/src/EspI2cBus.cpp @@ -0,0 +1,166 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file EspI2cBus.cpp + * @brief i2c_master (esp_driver_i2c) implementation of II2cBus. + * + * Target-only translation unit (excluded from the linux host build). The + * only file in the sensors component that touches the I2C driver. Uses the + * NEW driver/i2c_master.h API exclusively — the deprecated legacy + * driver/i2c.h must never be included anywhere in this firmware (FR-002, + * research.md R2: IDF forbids mixing the two drivers on one port). + */ + +#include "sensors/EspI2cBus.h" + +#include "board/board.h" +#include "driver/i2c_master.h" +#include "esp_log.h" + +static const char *TAG = "esp_i2c_bus"; + +EspI2cBus::~EspI2cBus() +{ + // RAII teardown, failures logged (never discarded). In practice the + // app_main instance lives forever; this path serves tests/future use. + for (size_t i = 0; i < deviceCount_; ++i) { + const esp_err_t err = i2c_master_bus_rm_device( + static_cast(devices_[i].handle)); + if (err != ESP_OK) { + ESP_LOGE(TAG, "i2c_master_bus_rm_device(0x%02x) failed: %s", + devices_[i].address, esp_err_to_name(err)); + } + } + if (busHandle_ != nullptr) { + const esp_err_t err = i2c_del_master_bus( + static_cast(busHandle_)); + if (err != ESP_OK) { + ESP_LOGE(TAG, "i2c_del_master_bus failed: %s", + esp_err_to_name(err)); + } + } +} + +bool EspI2cBus::ensureBus() +{ + if (busHandle_ != nullptr) { + return true; + } + + // Single shared bus (FR-003) on the board pins — never hard-coded + // GPIOs. Port auto-select; internal pull-ups enabled (the breakout + // modules carry their own — the internal ones are harmless + // belt-and-braces, contracts/interfaces.md). + i2c_master_bus_config_t bus_config = {}; + bus_config.i2c_port = -1; // auto-select + bus_config.sda_io_num = static_cast(BOARD_PIN_I2C_SDA); + bus_config.scl_io_num = static_cast(BOARD_PIN_I2C_SCL); + bus_config.clk_source = I2C_CLK_SRC_DEFAULT; + bus_config.glitch_ignore_cnt = 7; // driver-recommended default + bus_config.flags.enable_internal_pullup = true; + + i2c_master_bus_handle_t bus = nullptr; + const esp_err_t err = i2c_new_master_bus(&bus_config, &bus); + if (err != ESP_OK) { + // One-time infrastructure failure — worth more than a debug line. + ESP_LOGE(TAG, "i2c_new_master_bus failed: %s", esp_err_to_name(err)); + return false; + } + busHandle_ = bus; + ESP_LOGI(TAG, "I2C master bus up (SDA=%d SCL=%d, %lu Hz per device)", + BOARD_PIN_I2C_SDA, BOARD_PIN_I2C_SCL, + static_cast(kSclSpeedHz)); + return true; +} + +void* EspI2cBus::deviceHandle(uint8_t address7) +{ + if (!ensureBus()) { + return nullptr; + } + + for (size_t i = 0; i < deviceCount_; ++i) { + if (devices_[i].address == address7) { + return devices_[i].handle; + } + } + + if (deviceCount_ >= kMaxDevices) { + ESP_LOGE(TAG, "device table full (%u) — cannot add 0x%02x", + static_cast(kMaxDevices), address7); + return nullptr; + } + + // Created on first use, 100 kHz standard mode per device (FR-002). + i2c_device_config_t dev_config = {}; + dev_config.dev_addr_length = I2C_ADDR_BIT_LEN_7; + dev_config.device_address = address7; + dev_config.scl_speed_hz = kSclSpeedHz; + + i2c_master_dev_handle_t dev = nullptr; + const esp_err_t err = i2c_master_bus_add_device( + static_cast(busHandle_), &dev_config, &dev); + if (err != ESP_OK) { + ESP_LOGD(TAG, "i2c_master_bus_add_device(0x%02x) failed: %s", + address7, esp_err_to_name(err)); + return nullptr; + } + devices_[deviceCount_++] = {address7, dev}; + return dev; +} + +bool EspI2cBus::probe(uint8_t address7) +{ + if (!ensureBus()) { + return false; + } + const esp_err_t err = i2c_master_probe( + static_cast(busHandle_), address7, + kTimeoutMs); + if (err != ESP_OK) { + // Expected for absent devices — debug level (classification is the + // sensor driver's job, contracts/interfaces.md). + ESP_LOGD(TAG, "probe(0x%02x): %s", address7, esp_err_to_name(err)); + return false; + } + return true; +} + +bool EspI2cBus::readRegisters(uint8_t address7, uint8_t startReg, + uint8_t* buf, size_t len) +{ + i2c_master_dev_handle_t dev = + static_cast(deviceHandle(address7)); + if (dev == nullptr) { + return false; + } + // Register-pointer write + N-byte read in ONE transaction with a + // repeated start (i2c_master_transmit_receive) — required for correct + // BME280 burst reads (II2cBus contract). + const esp_err_t err = + i2c_master_transmit_receive(dev, &startReg, 1, buf, len, kTimeoutMs); + if (err != ESP_OK) { + ESP_LOGD(TAG, "readRegisters(0x%02x, 0x%02x, %u): %s", address7, + startReg, static_cast(len), esp_err_to_name(err)); + return false; + } + return true; +} + +bool EspI2cBus::writeRegister(uint8_t address7, uint8_t reg, uint8_t value) +{ + i2c_master_dev_handle_t dev = + static_cast(deviceHandle(address7)); + if (dev == nullptr) { + return false; + } + const uint8_t payload[2] = {reg, value}; + const esp_err_t err = + i2c_master_transmit(dev, payload, sizeof(payload), kTimeoutMs); + if (err != ESP_OK) { + ESP_LOGD(TAG, "writeRegister(0x%02x, 0x%02x, 0x%02x): %s", address7, + reg, value, esp_err_to_name(err)); + return false; + } + return true; +} diff --git a/firmware/main/CMakeLists.txt b/firmware/main/CMakeLists.txt index 97ff861..448c4da 100644 --- a/firmware/main/CMakeLists.txt +++ b/firmware/main/CMakeLists.txt @@ -1,5 +1,5 @@ idf_component_register( - SRCS "app_main.cpp" "diag_console.cpp" + SRCS "app_main.cpp" "diag_console.cpp" "sensor_task.cpp" PRIV_REQUIRES board esp_driver_gpio esp_app_format actuators interfaces console esp_timer storage nvs_flash sensors diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index 58e6f9c..83d4f96 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -30,7 +30,10 @@ #include "actuators/EspTimeProvider.h" #include "actuators/GpioWaterPump.h" #include "actuators/LockedWaterPump.h" +#include "sensors/Bme280Sensor.h" +#include "sensors/EspI2cBus.h" #include "sensors/EspModbusClient.h" +#include "sensors/LockedEnvironmentalSensor.h" #include "sensors/LockedSoilSensor.h" #include "sensors/ModbusSoilSensor.h" #include "storage/LittleFsDataStorage.h" @@ -40,6 +43,7 @@ #include "storage/StorageMount.h" #include "diag_console.h" +#include "sensor_task.h" static const char *TAG = "app_main"; @@ -195,10 +199,33 @@ extern "C" void app_main(void) modbus_client.getLastError()); } + // BME280 environmental sensor on the shared I2C bus (feature 005). + // Not safety-critical: a failed init is logged and the system keeps + // running — the sensor layer reports invalid data and the lazy re-init + // recovers on later polls (US2 semantics). Function-local statics after + // pumps_force_off() (boot fail-safe rule). The ONE EspI2cBus instance + // is the shared bus owner — PR-05's INA226 driver receives this same + // instance (FR-003); no second bus creation on these pins is permitted. + // The sensor is wrapped in the mutex-serializing decorator: accessed + // from the 5 s sensor task and the console REPL task, so EVERY sensor + // access goes through the wrapper. + static EspI2cBus i2c_bus; + static Bme280Sensor env_sensor_raw(i2c_bus); + static LockedEnvironmentalSensor env_sensor(env_sensor_raw); + + if (env_sensor.initialize()) { + ESP_LOGI(TAG, "BME280 environmental sensor up"); + } else { + ESP_LOGW(TAG, "BME280 init failed (error %d) — environmental " + "readings unavailable until recovery", + env_sensor.getLastError()); + } + // Serial diagnostic REPL (rig testing; contracts/serial-diagnostic.md). diag_console_register_pumps(plant, reservoir); diag_console_register_storage(config, storage); diag_console_register_soil(soil_sensor, modbus_client); + diag_console_register_env(env_sensor); esp_err_t err = diag_console_start(); if (err != ESP_OK) { // Console is a diagnostic aid, not a safety function: log and keep @@ -207,6 +234,10 @@ extern "C" void app_main(void) esp_err_to_name(err)); } + // 5 s environmental poll task (feature 005). Started even when the + // sensor failed init above — lazy re-init recovers later (parity). + sensor_task_start(env_sensor); + // Main loop: poll pump enforcement at 10 Hz. update() applies the timed // self-stop and the hard 300 s max-runtime cap. while (true) { diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index 1a826b7..e7b3f5c 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -38,6 +38,13 @@ * soil_cal_ph # value; a failed calibration- * soil_cal_ec # register write is NON-FATAL * + * Environmental sensor command (HIL verification path for feature 005; + * console contract in specs/005-bme280-i2c/contracts/interfaces.md — the + * failure output distinguishes error 1, sensor not found, from error 2, + * read failed — SC-006): + * + * env # one read(); T/RH/P or ERROR + * * Handler exit codes follow the esp_console convention: 0 on OK, 1 on ERR. * * State is plain pointers/PODs set from app_main — no non-trivial static @@ -59,6 +66,7 @@ #include "interfaces/IConfigStore.h" #include "interfaces/IDataStorage.h" +#include "interfaces/IEnvironmentalSensor.h" #include "interfaces/IModbusClient.h" #include "interfaces/ISoilSensor.h" #include "interfaces/IWaterPump.h" @@ -90,6 +98,10 @@ IDataStorage *s_storage = nullptr; ISoilSensor *s_soil = nullptr; IModbusClient *s_modbus = nullptr; +// Environmental sensor (set from app_main; expected to be the +// LockedEnvironmentalSensor decorator). Same trivial-initialization rule. +IEnvironmentalSensor *s_env = nullptr; + const char *stop_reason_str(StopReason reason) { switch (reason) { @@ -596,6 +608,39 @@ int soil_cal_ec_cmd(int argc, char **argv) &ISoilSensor::calibrateEC); } +// --- env command (feature 005 HIL verification path) --------------------- + +/// `env`: one read() through the locked sensor; thin wrapper, no logic. +/// Failure output is the binding contract format `ERROR ` with a +/// hint distinguishing error 1 (sensor not found — check wiring/address) +/// from error 2 (read failed — sensor present but communication or data +/// broke) — SC-006. +int env_cmd(int argc, char **argv) +{ + (void)argv; + if (argc != 1) { + printf("ERR usage: env\n"); + return 1; + } + if (s_env == nullptr) { + printf("ERR environmental sensor not available\n"); + return 1; + } + if (!s_env->read()) { + const int error = s_env->getLastError(); + const char *hint = (error == 1) ? "sensor not found" + : (error == 2) ? "read failed" + : "unknown error"; + printf("ERROR %d (%s)\n", error, hint); + return 1; + } + printf("OK temperature=%.1f C humidity=%.1f %%RH pressure=%.1f hPa\n", + static_cast(s_env->getTemperature()), + static_cast(s_env->getHumidity()), + static_cast(s_env->getPressure())); + return 0; +} + } // namespace void diag_console_register_pumps(IWaterPump& plant, IWaterPump& reservoir) @@ -616,6 +661,11 @@ void diag_console_register_soil(ISoilSensor& sensor, IModbusClient& client) s_modbus = &client; } +void diag_console_register_env(IEnvironmentalSensor& sensor) +{ + s_env = &sensor; +} + esp_err_t diag_console_start(void) { esp_console_repl_t *repl = nullptr; @@ -749,5 +799,19 @@ esp_err_t diag_console_start(void) return err; } + const esp_console_cmd_t cmd_env = { + .command = "env", + .help = "env — one environmental sensor read (T/RH/P or error code)", + .hint = nullptr, + .func = &env_cmd, + .argtable = nullptr, + .func_w_context = nullptr, + .context = nullptr, + }; + err = esp_console_cmd_register(&cmd_env); + 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 e0d3bda..899be62 100644 --- a/firmware/main/diag_console.h +++ b/firmware/main/diag_console.h @@ -14,6 +14,7 @@ #include "esp_err.h" #include "interfaces/IConfigStore.h" #include "interfaces/IDataStorage.h" +#include "interfaces/IEnvironmentalSensor.h" #include "interfaces/IModbusClient.h" #include "interfaces/ISoilSensor.h" #include "interfaces/IWaterPump.h" @@ -49,6 +50,17 @@ void diag_console_register_storage(IConfigStore& config, */ void diag_console_register_soil(ISoilSensor& sensor, IModbusClient& client); +/** + * @brief Register the environmental sensor the `env` command operates on + * (HIL verification path for feature 005). + * + * Pass the LockedEnvironmentalSensor decorator, never the raw sensor — the + * console handler runs on the REPL task, concurrently with the 5 s sensor + * task. Must be called before diag_console_start(); plain pointer + * registration. + */ +void diag_console_register_env(IEnvironmentalSensor& sensor); + /** * @brief Start the UART REPL (prompt "ws>") and register the commands. * diff --git a/firmware/main/sensor_task.cpp b/firmware/main/sensor_task.cpp new file mode 100644 index 0000000..ebe69ff --- /dev/null +++ b/firmware/main/sensor_task.cpp @@ -0,0 +1,101 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file sensor_task.cpp + * @brief 5 s environmental sensor poll task (feature 005, research.md R7). + * + * Parity parameters from the legacy controller task: 4096 B stack, + * priority 1, fixed 5000 ms cadence via vTaskDelayUntil (drift-free). + * Logging discipline (contracts/interfaces.md): INFO with the three values + * on success, WARN once on the valid→invalid transition and once on + * recovery, repeated failures at a bounded cadence — every + * kFailureLogInterval-th consecutive failure (12 × 5 s ≈ once a minute) to + * avoid log flood. The task never exits and never reboots on failures; + * recovery is the sensor driver's lazy re-init, driven by this cadence. + */ + +#include "sensor_task.h" + +#include "esp_log.h" +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" + +static const char *TAG = "sensor_task"; + +namespace { + +constexpr uint32_t kPeriodMs = 5000; ///< parity poll cadence (R7) +constexpr uint32_t kStackBytes = 4096; ///< parity stack size (R7) +constexpr UBaseType_t kPriority = 1; ///< parity priority (R7) + +/// Repeated-failure log cadence: every Nth consecutive failure (~1/min). +constexpr uint32_t kFailureLogInterval = 12; + +[[noreturn]] void sensor_task(void *arg) +{ + IEnvironmentalSensor &sensor = + *static_cast(arg); + + // Start in the "valid" state so the FIRST failure (e.g. booting with + // no sensor attached) logs the transition warning exactly once. + bool wasValid = true; + uint32_t consecutiveFailures = 0; + + 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)); + + if (sensor.read()) { + consecutiveFailures = 0; + if (!wasValid) { + ESP_LOGW(TAG, "environmental sensor recovered"); + wasValid = true; + } + // Periodic reading, consistent with the legacy 5 s status print. + ESP_LOGI(TAG, + "temperature=%.1f C humidity=%.1f %%RH pressure=%.1f hPa", + static_cast(sensor.getTemperature()), + static_cast(sensor.getHumidity()), + static_cast(sensor.getPressure())); + } else { + ++consecutiveFailures; + const int error = sensor.getLastError(); + if (wasValid) { + // Valid → invalid transition: WARN exactly once. + ESP_LOGW(TAG, + "environmental reading invalid (error %d) — " + "keeping last-good values, retrying every %lu ms", + error, static_cast(kPeriodMs)); + wasValid = false; + } else if (consecutiveFailures % kFailureLogInterval == 0) { + // Bounded repeat cadence while the failure persists. + ESP_LOGW(TAG, + "environmental sensor still failing (error %d, " + "%lu consecutive failures)", + error, + static_cast(consecutiveFailures)); + } + } + } +} + +} // namespace + +void sensor_task_start(IEnvironmentalSensor& sensor) +{ + // Task starts even when the sensor failed init: the driver's lazy + // re-initialization turns later polls into the recovery path (US2). + const BaseType_t created = + xTaskCreate(sensor_task, "sensor_task", kStackBytes, &sensor, + kPriority, nullptr); + if (created != pdPASS) { + // Not a safety function: log and continue without periodic + // readings (the console `env` command still works). + ESP_LOGE(TAG, "failed to create sensor task"); + return; + } + ESP_LOGI(TAG, "sensor task started (%lu ms cadence)", + static_cast(kPeriodMs)); +} diff --git a/firmware/main/sensor_task.h b/firmware/main/sensor_task.h new file mode 100644 index 0000000..c20774e --- /dev/null +++ b/firmware/main/sensor_task.h @@ -0,0 +1,35 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file sensor_task.h + * @brief Periodic environmental sensor poller (app wiring, feature 005). + * + * App-level FreeRTOS task, not a component: PR-11's watering controller is + * expected to absorb this cadence (research.md R7). Task/behavior contract: + * specs/005-bme280-i2c/contracts/interfaces.md ("Sensor task"). + */ + +#ifndef WATERINGSYSTEM_MAIN_SENSOR_TASK_H +#define WATERINGSYSTEM_MAIN_SENSOR_TASK_H + +#include "interfaces/IEnvironmentalSensor.h" + +/** + * @brief Start the 5 s environmental sensor poll task. + * + * Pass the LockedEnvironmentalSensor decorator, never the raw sensor — the + * task reads concurrently with the console REPL's `env` command (and + * PR-09/PR-11 consumers later). The task starts even when the sensor + * failed initialization (lazy re-init recovers later — parity), never + * exits and never reboots on failures; publishing IS the locked sensor + * itself (last-good values + getLastError()). + * + * Call once, after diag console registration in app_main. A task-creation + * failure is logged and swallowed — the poller is not a safety function. + * + * @param sensor Polled every 5 s; must outlive the task (i.e. forever — + * pass a function-local static from app_main). + */ +void sensor_task_start(IEnvironmentalSensor& sensor); + +#endif /* WATERINGSYSTEM_MAIN_SENSOR_TASK_H */ diff --git a/firmware/test_apps/host/main/CMakeLists.txt b/firmware/test_apps/host/main/CMakeLists.txt index 841ba31..533b8c8 100644 --- a/firmware/test_apps/host/main/CMakeLists.txt +++ b/firmware/test_apps/host/main/CMakeLists.txt @@ -4,5 +4,6 @@ idf_component_register( "test_config_store.cpp" "test_data_storage.cpp" "test_soil_sensor.cpp" + "test_bme280.cpp" REQUIRES unity actuators interfaces storage nvs_flash sensors ) diff --git a/firmware/test_apps/host/main/test_bme280.cpp b/firmware/test_apps/host/main/test_bme280.cpp new file mode 100644 index 0000000..61e47d3 --- /dev/null +++ b/firmware/test_apps/host/main/test_bme280.cpp @@ -0,0 +1,259 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file test_bme280.cpp + * @brief Host tests for the Bme280Sensor driver logic (linux target). + * + * Tests the REAL probe/calibration/compensation logic (Bme280Sensor) via + * MockI2cBus (scripted register maps + call recording). Registered via + * run_bme280_tests() from the shared Unity runner (test_main.cpp); the + * process exit code equals the failure count and is the CI gate. + * + * Coverage maps to tasks.md T006 (this file starts with the US1 happy + * path: initialization incl. the exact parity config-byte sequence, a + * reference-vector read, getters-before-first-read); T014/T017/T018/T020 + * extend this suite with the error paths, address-variant handling, the + * full Bosch vector set and the MockEnvironmentalSensor consistency tests. + */ + +#include + +#include "unity.h" + +#include "sensors/Bme280Sensor.h" +#include "sensors/testing/MockI2cBus.h" + +namespace { + +constexpr uint8_t kAddrPrimary = 0x76; +constexpr uint8_t kAddrSecondary = 0x77; ///< bench rig module strapping + +constexpr uint8_t kRegChipId = 0xD0; +constexpr uint8_t kRegCtrlHum = 0xF2; +constexpr uint8_t kRegCtrlMeas = 0xF4; +constexpr uint8_t kRegConfig = 0xF5; +constexpr uint8_t kRegData = 0xF7; + +// --------------------------------------------------------------------------- +// Reference vector (research R8). Calibration set: the BMP280 datasheet +// worked example (section "Calculation example": dig_T1..T3, dig_P1..P9; +// adc_T = 519888, adc_P = 415148 → 25.08 °C, 100653 Pa) extended with a +// typical humidity calibration (dig_H1=75, H2=362, H3=0, H4=315, H5=50, +// H6=30; adc_H = 32768). Expected outputs derived by running the Bosch +// datasheet reference routines (int32 T, int64 P, int32 H) over these +// inputs: +// t_fine = 128422 +// T int32 = 2508 → 25.08 °C (matches the datasheet) +// P Q24.8 = 25767233 → 100653.25 Pa = 1006.5325 hPa (datasheet +// double-precision result: 100653.27 Pa) +// H Q22.10 = 71319 → 69.64746 %RH +// --------------------------------------------------------------------------- + +/// calib00–25 (0x88–0xA1), little-endian: T1=27504 T2=26435 T3=-1000 +/// P1=36477 P2=-10685 P3=3024 P4=2855 P5=140 P6=-7 P7=15500 P8=-14600 +/// P9=6000, pad (0xA0), H1=75. +const std::vector kCalibBlock1 = { + 0x70, 0x6B, 0x43, 0x67, 0x18, 0xFC, // dig_T1..T3 + 0x7D, 0x8E, 0x43, 0xD6, 0xD0, 0x0B, 0x27, 0x0B, // dig_P1..P4 + 0x8C, 0x00, 0xF9, 0xFF, 0x8C, 0x3C, 0xF8, 0xC6, // dig_P5..P8 + 0x70, 0x17, // dig_P9 + 0x00, // 0xA0 (unused pad) + 0x4B, // dig_H1 = 75 +}; + +/// calib26–32 (0xE1–0xE7): H2=362 (0x016A LE), H3=0, H4=315 (0xE4=0x13, +/// 0xE5[3:0]=0xB), H5=50 (0xE5[7:4]=0x2, 0xE6=0x03), H6=30. +const std::vector kCalibBlock2 = { + 0x6A, 0x01, 0x00, 0x13, 0x2B, 0x03, 0x1E, +}; + +/// Data registers 0xF7–0xFE: adc_P=415148 (0x655AC), adc_T=519888 +/// (0x7EED0), adc_H=32768 (0x8000) — 20-bit P/T use xlsb bits [7:4]. +const std::vector kDataBlock = { + 0x65, 0x5A, 0xC0, 0x7E, 0xED, 0x00, 0x80, 0x00, +}; + +// Expected compensated floats (integer reference outputs ÷100, ÷256÷100, +// ÷1024 — see the derivation comment above). +constexpr float kExpectedTemperature = 25.08f; +constexpr float kExpectedPressure = 1006.5325f; +constexpr float kExpectedHumidity = 69.64746f; + +/// Script a complete, healthy BME280 at @p address: chip-ID, both +/// calibration blocks and one measurement in the data registers. +void scriptBme280(MockI2cBus& mock, uint8_t address) +{ + mock.addDevice(address); + mock.setRegister(address, kRegChipId, 0x60); + mock.setRegisters(address, 0x88, kCalibBlock1); + mock.setRegisters(address, 0xE1, kCalibBlock2); + mock.setRegisters(address, kRegData, kDataBlock); +} + +/// Fresh mock + sensor per test with the device at @p address; +/// initialization is part of the fixture and the recorded call log is +/// cleared so each test asserts only on its own transactions. +struct Fixture { + MockI2cBus mock; + Bme280Sensor sensor{mock}; + + explicit Fixture(uint8_t address = kAddrSecondary) + { + scriptBme280(mock, address); + TEST_ASSERT_TRUE(sensor.initialize()); + mock.calls.clear(); + } +}; + +} // namespace + +// -------------------------------------------------------------------------- +// initialize() finds the device (bench parity: strapped to 0x77) and +// writes the EXACT parity sampling profile, ctrl_hum BEFORE ctrl_meas +// (datasheet: ctrl_hum takes effect only on a ctrl_meas write), then +// config: (0xF2,0x01) → (0xF4,0x57) → (0xF5,0x90) +// -------------------------------------------------------------------------- +static void test_initialize_writes_parity_config_bytes_in_order(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrSecondary); + + TEST_ASSERT_TRUE(sensor.initialize()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + + // Exactly three writes, in the contract order, with the parity bytes. + std::vector writes; + for (const MockI2cBus::Call& call : mock.calls) { + if (call.type == MockI2cBus::Call::Type::Write) { + writes.push_back(call); + } + } + TEST_ASSERT_EQUAL(3, writes.size()); + TEST_ASSERT_EQUAL_UINT8(kRegCtrlHum, writes[0].reg); + TEST_ASSERT_EQUAL_UINT8(0x01, writes[0].value); // osrs_h ×1 + TEST_ASSERT_EQUAL_UINT8(kRegCtrlMeas, writes[1].reg); + TEST_ASSERT_EQUAL_UINT8(0x57, writes[1].value); // T×2, P×16, NORMAL + TEST_ASSERT_EQUAL_UINT8(kRegConfig, writes[2].reg); + TEST_ASSERT_EQUAL_UINT8(0x90, writes[2].value); // 500 ms, IIR ×16 + for (const MockI2cBus::Call& write : writes) { + TEST_ASSERT_EQUAL_UINT8(kAddrSecondary, write.address); + TEST_ASSERT_TRUE(write.succeeded); + } +} + +// -------------------------------------------------------------------------- +// Probing order: 0x76 is tried first (data-model.md); with the device at +// 0x76 the secondary address is never probed +// -------------------------------------------------------------------------- +static void test_initialize_probes_primary_address_first(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrPrimary); + + TEST_ASSERT_TRUE(sensor.initialize()); + + TEST_ASSERT_FALSE(mock.calls.empty()); + TEST_ASSERT_EQUAL(static_cast(MockI2cBus::Call::Type::Probe), + static_cast(mock.calls.front().type)); + TEST_ASSERT_EQUAL_UINT8(kAddrPrimary, mock.calls.front().address); + for (const MockI2cBus::Call& call : mock.calls) { + TEST_ASSERT_EQUAL_UINT8(kAddrPrimary, call.address); + } +} + +// -------------------------------------------------------------------------- +// initialize() is idempotent: a second call is a no-op on the bus +// -------------------------------------------------------------------------- +static void test_initialize_is_idempotent(void) +{ + Fixture f; + + TEST_ASSERT_TRUE(f.sensor.initialize()); + TEST_ASSERT_EQUAL(0, f.mock.calls.size()); +} + +// -------------------------------------------------------------------------- +// read() reproduces the reference vector: 25.08 °C / 69.647 %RH / +// 1006.53 hPa (Bosch integer paths ÷100, ÷1024, ÷256÷100) +// -------------------------------------------------------------------------- +static void test_read_produces_reference_values(void) +{ + Fixture f; + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + f.sensor.getTemperature()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedHumidity, + f.sensor.getHumidity()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedPressure, + f.sensor.getPressure()); +} + +// -------------------------------------------------------------------------- +// One read() = exactly ONE bus transaction: an 8-byte burst at 0xF7 +// (atomic snapshot within the chip's register shadowing) +// -------------------------------------------------------------------------- +static void test_read_is_one_eight_byte_burst(void) +{ + Fixture f; + + TEST_ASSERT_TRUE(f.sensor.read()); + + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + const MockI2cBus::Call& call = f.mock.calls.front(); + TEST_ASSERT_EQUAL(static_cast(MockI2cBus::Call::Type::Read), + static_cast(call.type)); + TEST_ASSERT_EQUAL_UINT8(kAddrSecondary, call.address); + TEST_ASSERT_EQUAL_UINT8(kRegData, call.reg); + TEST_ASSERT_EQUAL(8, call.len); + TEST_ASSERT_TRUE(call.succeeded); +} + +// -------------------------------------------------------------------------- +// Before the FIRST successful read() the getters return the documented +// meaningless placeholders (0.0) — consumers gate on read(), never on +// value plausibility (interface contract) +// -------------------------------------------------------------------------- +static void test_getters_before_first_read_are_placeholders(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrSecondary); + + TEST_ASSERT_TRUE(sensor.initialize()); + + TEST_ASSERT_EQUAL_FLOAT(0.0f, sensor.getTemperature()); + TEST_ASSERT_EQUAL_FLOAT(0.0f, sensor.getHumidity()); + TEST_ASSERT_EQUAL_FLOAT(0.0f, sensor.getPressure()); +} + +// -------------------------------------------------------------------------- +// Lazy initialization (parity): read() on a never-initialized sensor +// initializes first, then reads — one call delivers a valid reading +// -------------------------------------------------------------------------- +static void test_read_initializes_lazily(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrSecondary); + + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + sensor.getTemperature()); +} + +void run_bme280_tests(void) +{ + RUN_TEST(test_initialize_writes_parity_config_bytes_in_order); + RUN_TEST(test_initialize_probes_primary_address_first); + RUN_TEST(test_initialize_is_idempotent); + RUN_TEST(test_read_produces_reference_values); + RUN_TEST(test_read_is_one_eight_byte_burst); + RUN_TEST(test_getters_before_first_read_are_placeholders); + RUN_TEST(test_read_initializes_lazily); +} diff --git a/firmware/test_apps/host/main/test_main.cpp b/firmware/test_apps/host/main/test_main.cpp index dca0839..e076a04 100644 --- a/firmware/test_apps/host/main/test_main.cpp +++ b/firmware/test_apps/host/main/test_main.cpp @@ -17,6 +17,7 @@ void run_water_pump_tests(void); void run_config_store_tests(void); void run_data_storage_tests(void); void run_soil_sensor_tests(void); +void run_bme280_tests(void); // Unity requires setUp/tearDown definitions (shared by all suites). extern "C" void setUp(void) {} @@ -29,5 +30,6 @@ extern "C" void app_main(void) run_config_store_tests(); run_data_storage_tests(); run_soil_sensor_tests(); + run_bme280_tests(); std::exit(UNITY_END()); } diff --git a/specs/005-bme280-i2c/tasks.md b/specs/005-bme280-i2c/tasks.md index 1138721..e94bc38 100644 --- a/specs/005-bme280-i2c/tasks.md +++ b/specs/005-bme280-i2c/tasks.md @@ -17,9 +17,9 @@ surface and host-test harness this feature extends arrive with it. ## Phase 2: Foundational (blocking prerequisites for all user stories) -- [ ] T002 [P] Create `IEnvironmentalSensor` interface in `firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h` — standalone pure C++ (no IDF includes), soil-sensor conventions: initialize/read/isAvailable/getLastError + getTemperature (°C)/getHumidity (%RH)/getPressure (hPa), last-good-value validity contract in doc comments per `contracts/interfaces.md` -- [ ] T003 [P] Create `II2cBus` interface in `firmware/components/interfaces/include/interfaces/II2cBus.h` — probe(address7)/readRegisters(address7, startReg, buf, len)/writeRegister(address7, reg, value), repeated-start semantics documented, no IDF includes, per `contracts/interfaces.md` -- [ ] T004 Create `MockI2cBus` in `firmware/components/sensors/include/sensors/testing/MockI2cBus.h` — header-only scriptable register map (chip-ID, calibration block, data registers), probe results per address, failure injection (NACK on address, error on register read/write, corrupt data), call/write recording for config-byte assertions +- [x] T002 [P] Create `IEnvironmentalSensor` interface in `firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h` — standalone pure C++ (no IDF includes), soil-sensor conventions: initialize/read/isAvailable/getLastError + getTemperature (°C)/getHumidity (%RH)/getPressure (hPa), last-good-value validity contract in doc comments per `contracts/interfaces.md` +- [x] T003 [P] Create `II2cBus` interface in `firmware/components/interfaces/include/interfaces/II2cBus.h` — probe(address7)/readRegisters(address7, startReg, buf, len)/writeRegister(address7, reg, value), repeated-start semantics documented, no IDF includes, per `contracts/interfaces.md` +- [x] T004 Create `MockI2cBus` in `firmware/components/sensors/include/sensors/testing/MockI2cBus.h` — header-only scriptable register map (chip-ID, calibration block, data registers), probe results per address, failure injection (NACK on address, error on register read/write, corrupt data), call/write recording for config-byte assertions ## Phase 3: User Story 1 — Environmental readings on the bench rig (P1) 🎯 MVP @@ -27,14 +27,14 @@ surface and host-test harness this feature extends arrive with it. **Independent Test**: flash rig, watch 5 s log cadence, run `env`, compare against Arduino unit (±0.5 °C, ±3 %RH, ±1 hPa). -- [ ] T005 [US1] Implement `Bme280Sensor` (pure logic) in `firmware/components/sensors/include/sensors/Bme280Sensor.h` + `firmware/components/sensors/src/Bme280Sensor.cpp`: constructor takes `II2cBus&`; initialize() = probe 0x76→0x77, chip-ID check (0xD0 == 0x60), calibration readout/parse (dig_T/P/H incl. split-nibble H4/H5), sampling profile writes ctrl_hum(0xF2)=0x01 → ctrl_meas(0xF4)=0x57 → config(0xF5)=0x90 (data-model.md — verify byte encodings against Bosch datasheet field tables); read() = burst 0xF7–0xFE, compensate T→P→H via t_fine (Bosch int32/int64 reference algorithms), convert to °C/%RH/hPa (P: ÷256 ÷100), NaN → error 2; error codes 0/1/2 per data-model.md -- [ ] T006 [P] [US1] Host tests for init + happy-path read in `firmware/test_apps/host/main/test_bme280.cpp` (new suite `run_bme280_tests()`): scripted MockI2cBus with full register map → initialize() succeeds, exact config bytes written in order (ctrl_hum before ctrl_meas), read() produces expected values; getters-before-first-read documented behavior -- [ ] T007 [P] [US1] Implement `EspI2cBus` in `firmware/components/sensors/include/sensors/EspI2cBus.h` + `firmware/components/sensors/src/EspI2cBus.cpp` (target-only): owns `i2c_master_bus_handle_t` from `BOARD_PIN_I2C_SDA/SCL`, port auto, internal pull-ups on; per-address device handles created on first use at 100 kHz; probe→`i2c_master_probe`, readRegisters→`i2c_master_transmit_receive`, writeRegister→`i2c_master_transmit`; finite timeouts; errors returned as false, logged at debug; PRIV_REQUIRES `esp_driver_i2c` -- [ ] T008 [P] [US1] Implement `LockedEnvironmentalSensor` in `firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h` — header-only FreeRTOS-mutex decorator, same pattern as `LockedSoilSensor` (per-call serialization; document the read-then-getters pattern limitation + PR-11 snapshot bookkeeping) -- [ ] T009 [US1] Update `firmware/components/sensors/CMakeLists.txt`: add `Bme280Sensor.cpp` to all-target SRCS, `EspI2cBus.cpp` + `esp_driver_i2c` dep inside the existing `if(NOT ${IDF_TARGET} STREQUAL "linux")` guard -- [ ] T010 [US1] Implement sensor task in `firmware/main/sensor_task.cpp` + `firmware/main/sensor_task.h`: `sensor_task_start(IEnvironmentalSensor&)` creates FreeRTOS task (4096 B stack, priority 1, `vTaskDelayUntil` 5000 ms); each cycle read() + INFO log of values on success; WARN once on valid→invalid transition and on recovery, repeated failures at bounded cadence (every Nth); task starts even when sensor absent, never exits (contracts/interfaces.md) -- [ ] T011 [US1] Add `env` console command: `diag_console_register_env(IEnvironmentalSensor&)` in `firmware/main/diag_console.h` + handler in `firmware/main/diag_console.cpp` — thin wrapper, one read(), success prints the three values with units, failure prints `ERROR ` + hint distinguishing code 1 (not found) from code 2 (read failed) per contracts/interfaces.md -- [ ] T012 [US1] Wire it all in `firmware/main/app_main.cpp`: function-local static `EspI2cBus` → `Bme280Sensor` → `LockedEnvironmentalSensor`; `diag_console_register_env(...)` before `diag_console_start()`; `sensor_task_start(...)` after console registration; init failure = log-and-continue (non-safety subsystem), everything after `pumps_force_off()` — follow the PR-04 wiring pattern +- [x] T005 [US1] Implement `Bme280Sensor` (pure logic) in `firmware/components/sensors/include/sensors/Bme280Sensor.h` + `firmware/components/sensors/src/Bme280Sensor.cpp`: constructor takes `II2cBus&`; initialize() = probe 0x76→0x77, chip-ID check (0xD0 == 0x60), calibration readout/parse (dig_T/P/H incl. split-nibble H4/H5), sampling profile writes ctrl_hum(0xF2)=0x01 → ctrl_meas(0xF4)=0x57 → config(0xF5)=0x90 (data-model.md — verify byte encodings against Bosch datasheet field tables); read() = burst 0xF7–0xFE, compensate T→P→H via t_fine (Bosch int32/int64 reference algorithms), convert to °C/%RH/hPa (P: ÷256 ÷100), NaN → error 2; error codes 0/1/2 per data-model.md +- [x] T006 [P] [US1] Host tests for init + happy-path read in `firmware/test_apps/host/main/test_bme280.cpp` (new suite `run_bme280_tests()`): scripted MockI2cBus with full register map → initialize() succeeds, exact config bytes written in order (ctrl_hum before ctrl_meas), read() produces expected values; getters-before-first-read documented behavior +- [x] T007 [P] [US1] Implement `EspI2cBus` in `firmware/components/sensors/include/sensors/EspI2cBus.h` + `firmware/components/sensors/src/EspI2cBus.cpp` (target-only): owns `i2c_master_bus_handle_t` from `BOARD_PIN_I2C_SDA/SCL`, port auto, internal pull-ups on; per-address device handles created on first use at 100 kHz; probe→`i2c_master_probe`, readRegisters→`i2c_master_transmit_receive`, writeRegister→`i2c_master_transmit`; finite timeouts; errors returned as false, logged at debug; PRIV_REQUIRES `esp_driver_i2c` +- [x] T008 [P] [US1] Implement `LockedEnvironmentalSensor` in `firmware/components/sensors/include/sensors/LockedEnvironmentalSensor.h` — header-only FreeRTOS-mutex decorator, same pattern as `LockedSoilSensor` (per-call serialization; document the read-then-getters pattern limitation + PR-11 snapshot bookkeeping) +- [x] T009 [US1] Update `firmware/components/sensors/CMakeLists.txt`: add `Bme280Sensor.cpp` to all-target SRCS, `EspI2cBus.cpp` + `esp_driver_i2c` dep inside the existing `if(NOT ${IDF_TARGET} STREQUAL "linux")` guard +- [x] T010 [US1] Implement sensor task in `firmware/main/sensor_task.cpp` + `firmware/main/sensor_task.h`: `sensor_task_start(IEnvironmentalSensor&)` creates FreeRTOS task (4096 B stack, priority 1, `vTaskDelayUntil` 5000 ms); each cycle read() + INFO log of values on success; WARN once on valid→invalid transition and on recovery, repeated failures at bounded cadence (every Nth); task starts even when sensor absent, never exits (contracts/interfaces.md) +- [x] T011 [US1] Add `env` console command: `diag_console_register_env(IEnvironmentalSensor&)` in `firmware/main/diag_console.h` + handler in `firmware/main/diag_console.cpp` — thin wrapper, one read(), success prints the three values with units, failure prints `ERROR ` + hint distinguishing code 1 (not found) from code 2 (read failed) per contracts/interfaces.md +- [x] T012 [US1] Wire it all in `firmware/main/app_main.cpp`: function-local static `EspI2cBus` → `Bme280Sensor` → `LockedEnvironmentalSensor`; `diag_console_register_env(...)` before `diag_console_start()`; `sensor_task_start(...)` after console registration; init failure = log-and-continue (non-safety subsystem), everything after `pumps_force_off()` — follow the PR-04 wiring pattern **Checkpoint**: rig delivers periodic readings + `env` works → MVP demonstrable. @@ -66,7 +66,7 @@ surface and host-test harness this feature extends arrive with it. - [ ] T018 [US4] Add Bosch reference-vector tests in `firmware/test_apps/host/main/test_bme280.cpp`: fixed (calibration set, raw T/P/H) → expected compensated outputs from the Bosch datasheet reference implementation (int32 T, int64 P, int32 H), incl. the datasheet worked example, a negative-temperature vector and extreme-but-legal raws; each vector's derivation cited in a comment (research R8); float conversions (÷100, ÷256÷100, ÷1024) asserted - [ ] T019 [P] [US4] Create `MockEnvironmentalSensor` in `firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h` — scripted value/validity/error sequences WITH consistency helpers `scriptSuccessfulRead(t,h,p)` / `scriptFailedRead(error)` from the start (PR-04 lesson), for PR-11 consumer tests - [ ] T020 [P] [US4] Host tests for MockEnvironmentalSensor consistency in `firmware/test_apps/host/main/test_bme280.cpp`: scripted sequences observed exactly; helpers keep values/validity/error coherent -- [ ] T021 [US4] Register the suite: add `test_bme280.cpp` to SRCS in `firmware/test_apps/host/main/CMakeLists.txt`, declare + call `run_bme280_tests()` in `firmware/test_apps/host/main/test_main.cpp` between UNITY_BEGIN/UNITY_END +- [x] T021 [US4] Register the suite: add `test_bme280.cpp` to SRCS in `firmware/test_apps/host/main/CMakeLists.txt`, declare + call `run_bme280_tests()` in `firmware/test_apps/host/main/test_main.cpp` between UNITY_BEGIN/UNITY_END — NOTE: pulled forward into implementation mission 1 (with T002–T012) so the main session can run the T006 suite immediately ## Phase 7: Polish & Cross-Cutting From 75d3679b8106fa9044f6af9e569774b675ffeac3 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 16:53:37 +0200 Subject: [PATCH 3/5] feat(sensors): BME280 fault handling, address variants, Bosch vectors and docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit US2-US4 + polish: host-testable sensor-task log policy (SensorTaskLogPolicy, bounded WARN cadence), MockEnvironmentalSensor with consistency helpers, 23 new host tests (error paths incl. calibration-re-read on module swap, 0x76/0x77 address matrix, negative- temperature and extreme-raw Bosch reference vectors, log policy, mock coherence) - suite 109/0. firmware/CLAUDE.md section, parity-checklist §6 divergences, HIL checklist. Full verification: rev1+rev2 green from clean, dependencies.lock unchanged (T013-T025). Co-Authored-By: Claude Fable 5 --- docs/parity-checklist.md | 25 + firmware/CLAUDE.md | 78 +- .../include/interfaces/IEnvironmentalSensor.h | 11 +- .../include/sensors/SensorTaskLogPolicy.h | 89 +++ .../sensors/testing/MockEnvironmentalSensor.h | 139 ++++ .../components/sensors/src/Bme280Sensor.cpp | 5 + firmware/main/sensor_task.cpp | 69 +- firmware/test_apps/host/main/test_bme280.cpp | 687 +++++++++++++++++- specs/005-bme280-i2c/checklists/hil.md | 70 ++ specs/005-bme280-i2c/tasks.md | 26 +- 10 files changed, 1133 insertions(+), 66 deletions(-) create mode 100644 firmware/components/sensors/include/sensors/SensorTaskLogPolicy.h create mode 100644 firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h create mode 100644 specs/005-bme280-i2c/checklists/hil.md diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 1ba811d..b40f8cd 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -216,6 +216,31 @@ each item below is the new contract behavior, host-tested in PR-05 (reservoir board flag). The config store is extensible per-key so PR-05 can add them without a contract change. +### Deliberate divergences in the ESP-IDF port (feature 005, PR-03) + +Intentional behavior changes from the Arduino BME280 driver (section 5), not +parity targets; each is the new contract behavior, host-tested in +`firmware/test_apps/host/main/test_bme280.cpp`. + +- [ ] `[HOST]` **I2C address probing 0x76→0x77 with chip-identity check**: + legacy hard-codes address 0x77 and never verifies the chip ID; the port + probes both addresses, accepts only a device whose register 0xD0 reads 0x60 + (a BMP280/foreign device is logged distinctly and rejected), and re-probes + BOTH addresses on recovery after loss — a module swapped to the other + address while unplugged is picked up transparently (spec 005 US3/FR-004). +- [ ] `[HOST]` **Last-good getter values after a failed read**: legacy left + NaN in the getters after a failed read; the port keeps the previous good + values and consumers gate on the read() result / getLastError() — aligned + with the soil-sensor contract (spec 005 FR-001/FR-007). +- [ ] `[HOST]` **`isAvailable()` is a live chip-ID probe**: legacy caches + "available" forever after the first successful init and can report a dead + sensor as alive; the port performs a real chip-ID read on every call, so + the unplug/replug HIL criterion is observable (spec 005 FR-009). +- [ ] `[HOST]` **Synchronized cross-task access via `LockedEnvironmentalSensor`**: + legacy has two unsynchronized readers on the same I2C device (main loop + + web server); the port serializes every interface call through the mutex + decorator — same pattern as the other Locked* wrappers (spec 005 FR-010). + ## 7. WiFi / network / time - [ ] `[HIL]` STA connect at boot using saved credentials: STA mode, auto-reconnect off (handled manually), WiFi modem sleep disabled, clean disconnect first, **60 s** connect timeout with LED toggling every 500 ms (`src/main.cpp:46`, `168-212`) diff --git a/firmware/CLAUDE.md b/firmware/CLAUDE.md index 24e18b9..f8c0ce4 100644 --- a/firmware/CLAUDE.md +++ b/firmware/CLAUDE.md @@ -29,10 +29,13 @@ must stay green. ### Host tests (linux preview target) Logic is unit-tested natively, no ESP32 needed: pump enforcement, config -store, data storage and the soil sensor decode/validation/calibration -(`test_soil_sensor.cpp`, real `ModbusSoilSensor` over `MockModbusClient`). -The test executable's exit code equals the Unity failure count (CI gate, job -`host-test`): +store, data storage, the soil sensor decode/validation/calibration +(`test_soil_sensor.cpp`, real `ModbusSoilSensor` over `MockModbusClient`) +and the BME280 probe/calibration/compensation logic (`test_bme280.cpp`, +real `Bme280Sensor` over `MockI2cBus` — Bosch reference vectors, error +paths, address variants, sensor-task log policy, `MockEnvironmentalSensor` +consistency). The test executable's exit code equals the Unity failure +count (CI gate, job `host-test`): ```bash cd firmware/test_apps/host @@ -54,6 +57,7 @@ firmware/ ├── main/ │ ├── app_main.cpp # Entry point — pumps forced OFF first, always │ ├── diag_console.cpp/.h # esp_console UART REPL (prompt "ws>") +│ ├── sensor_task.cpp/.h # 5 s environmental poll task (feature 005) │ ├── Kconfig.projbuild # Board revision choice │ └── idf_component.yml # Pinned managed deps (esp-modbus, littlefs) ├── components/ @@ -62,17 +66,23 @@ firmware/ │ ├── interfaces/ # Header-only, NO IDF deps (host-includable) │ │ └── include/interfaces/ # IActuator, IWaterPump, ITimeProvider, │ │ # IConfigStore, IDataStorage, -│ │ # IModbusClient, ISoilSensor +│ │ # IModbusClient, ISoilSensor, +│ │ # IEnvironmentalSensor, II2cBus │ ├── actuators/ # Pump drivers │ │ ├── include/actuators/ # WaterPump (pure C++ logic), GpioWaterPump, │ │ │ # EspTimeProvider (esp32-only header), │ │ │ # testing/ (MockWaterPump, FakeTimeProvider) │ │ └── src/ # GpioWaterPump.cpp excluded on linux target -│ ├── sensors/ # RS485 Modbus soil sensor (feature 004) -│ │ ├── include/sensors/ # ModbusSoilSensor (pure C++ logic), -│ │ │ # EspModbusClient, LockedSoilSensor, -│ │ │ # testing/ (MockModbusClient, MockSoilSensor) -│ │ └── src/ # EspModbusClient.cpp + esp-modbus dep +│ ├── sensors/ # Soil sensor (feature 004) + BME280 (005) +│ │ ├── include/sensors/ # ModbusSoilSensor, Bme280Sensor (pure C++ +│ │ │ # logic), EspModbusClient, EspI2cBus, +│ │ │ # LockedSoilSensor, +│ │ │ # LockedEnvironmentalSensor, +│ │ │ # SensorTaskLogPolicy, testing/ +│ │ │ # (MockModbusClient, MockSoilSensor, +│ │ │ # MockI2cBus, MockEnvironmentalSensor) +│ │ └── src/ # EspModbusClient.cpp + esp-modbus dep and +│ │ # EspI2cBus.cpp + esp_driver_i2c dep │ │ # excluded on linux target │ └── storage/ # Config + data persistence (feature 003) │ ├── include/storage/ # NvsConfigStore, LittleFsDataStorage (POSIX, @@ -84,7 +94,8 @@ firmware/ └── test_apps/ └── host/ # Host test app (linux preview target, Unity): # pump + config store + data storage + - # soil sensor (test_soil_sensor.cpp) suites + # soil sensor (test_soil_sensor.cpp) + + # BME280 (test_bme280.cpp) suites ``` Future components (drivers, controllers, web server) are added as siblings @@ -135,6 +146,14 @@ rs485test # raw 1-register Modbus probe + statist soil_cal_moisture | soil_cal_ph | soil_cal_ec ``` +Feature 005 adds the environmental sensor command (HIL verification path, +one locked read; the failure hint distinguishes error 1 "sensor not found" +from error 2 "read failed" — SC-006): + +``` +env # one read(); T/RH/P with units or ERROR () +``` + ## Storage (config + data persistence) Feature 003 (PR-06). Two redesigned, host-includable interfaces in @@ -193,6 +212,43 @@ All pins and polarity/feature flags come from `board/board.h` `BOARD_HAS_INA226`, `BOARD_NAME`). Never hard-code GPIO numbers elsewhere. Board-conditional code uses `#if CONFIG_BOARD_REV2` / `#if BOARD_HAS_INA226`. +## BME280 environmental sensor (I2C) + +Feature 005 (PR-03). Same architecture split as the soil sensor, at the +`II2cBus` interface: `Bme280Sensor` is pure C++ and holds ALL policy — +0x76→0x77 address probing with chip-ID verification (0xD0 == 0x60, +rejects e.g. a BMP280), calibration readout/parsing (incl. the +split-nibble dig_H4/H5), the Bosch datasheet reference compensation +(int32 T / int64 P / int32 H via t_fine, transcribed exactly — do not +"clean up"), unit conversion (°C/%RH/hPa), error codes 0/1/2, lazy +re-init and uninitialize-on-bus-error recovery (re-probes BOTH +addresses). It is host-tested against `MockI2cBus`, including Bosch +reference vectors. `EspI2cBus` is the only hardware touchpoint (the new +`driver/i2c_master.h` API — never the legacy `driver/i2c.h` — 100 kHz, +pins from `board/board.h`) and is excluded from the linux build. + +**Shared bus (PR-05):** `app_main` owns the single `EspI2cBus` instance +(function-local static); PR-05's INA226 driver must receive the SAME +instance — never create a second bus on these pins. Bus-level transaction +safety comes from the i2c_master driver's bus lock; snapshot consistency +comes from `LockedEnvironmentalSensor`, the mandatory wrapper for all +cross-task access (sensor task + console REPL now; web PR-09, controller +PR-11). + +**Sampling profile is parity** (like-for-like HIL comparison against the +Arduino unit): NORMAL mode, oversampling T×2 / P×16 / H×1, IIR ×16, +standby 500 ms — ctrl_hum written before ctrl_meas, then config. + +The `sensor_task` (main/, 4096 B stack, priority 1, `vTaskDelayUntil` +5000 ms — parity parameters) polls the locked sensor, starts even when +the sensor is absent (lazy re-init recovers later) and never exits. Its +WARN/INFO/silence decisions live in the pure `SensorTaskLogPolicy` +(host-tested): WARN once on the valid→invalid transition and on recovery, +bounded repeats every 12th consecutive failure. Deliberate divergences +from the legacy driver (address probing, last-good getters, live +availability probe, locked access) are recorded in +`docs/parity-checklist.md` §6. + ## Partition layout (4MB flash) nvs (0x9000, 16K) | otadata (0xd000, 8K) | phy_init (0xf000, 4K) | diff --git a/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h b/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h index 18a686d..d9b49c7 100644 --- a/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h +++ b/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h @@ -79,15 +79,18 @@ class IEnvironmentalSensor { * @brief Probe sensor presence with a REAL chip-ID read (FR-009). * * Every call performs an actual bus transaction — never cached state - * (deliberate divergence from the legacy cached availability). Does not - * modify getLastError(). Recovery from earlier failures is implicit: a - * sensor that identifies itself again is available again. + * (deliberate divergence from the legacy cached availability). The + * probe itself does not modify getLastError(); when it triggers a lazy + * (re-)initialization, that initialize() owns its own error reporting — + * the ISoilSensor convention. Recovery from earlier failures is + * implicit: a sensor that identifies itself again is available again. */ virtual bool isAvailable() = 0; /** * @brief Error code of the most recent initialize()/read() (0 = OK; - * table in the class comment). isAvailable() never touches this code. + * table in the class comment). The isAvailable() probe never touches + * this code (its lazy-init path reports through initialize()). */ virtual int getLastError() = 0; diff --git a/firmware/components/sensors/include/sensors/SensorTaskLogPolicy.h b/firmware/components/sensors/include/sensors/SensorTaskLogPolicy.h new file mode 100644 index 0000000..3feb38e --- /dev/null +++ b/firmware/components/sensors/include/sensors/SensorTaskLogPolicy.h @@ -0,0 +1,89 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file SensorTaskLogPolicy.h + * @brief Pure log-decision policy of the environmental sensor task + * (header-only). + * + * Extracted from main/sensor_task.cpp so the logging discipline required by + * specs/005-bme280-i2c/contracts/interfaces.md ("Sensor task") is + * host-testable instead of review-verified: WARN once on the valid→invalid + * transition and once on recovery, repeated failures at a bounded cadence + * (every kFailureLogInterval-th consecutive failure), INFO readings on + * success. The policy decides WHAT to log; the task owns the ESP_LOG calls + * and the 5 s cadence. It lives in the sensors component (not main/) + * because the host test app builds components only; it stays free of + * IDF/FreeRTOS includes for the same reason. PR-11's controller may absorb + * it together with the task cadence. + * + * The policy has no terminal state — a poll loop driving it never runs out + * of decisions, matching the task contract (never exits, never reboots). + */ + +#ifndef WATERINGSYSTEM_SENSORS_SENSORTASKLOGPOLICY_H +#define WATERINGSYSTEM_SENSORS_SENSORTASKLOGPOLICY_H + +#include + +/** + * @brief Maps each poll's read() result to the log action the sensor task + * must take. + * + * Starts in the "valid" state so the FIRST failure (e.g. booting with no + * sensor attached) yields the transition warning exactly once. Feed every + * read() result to onReadResult(), in order. + */ +class SensorTaskLogPolicy { +public: + /// What the task logs for one poll result. + enum class Event { + Reading, ///< success, steady state: INFO reading only + Recovery, ///< success after failure(s): WARN recovery, then + ///< the INFO reading (values are fresh again) + FailureTransition, ///< first failure after valid: WARN exactly once + RepeatedFailure, ///< bounded repeat WARN while the failure persists + Silent, ///< failure within the bounded cadence: log nothing + }; + + /// Repeated-failure log cadence: every Nth consecutive failure + /// (12 × 5 s ≈ once a minute) to avoid log flood (research.md R7). + static constexpr uint32_t kFailureLogInterval = 12; + + /** + * @brief Record one poll result and decide the log action. + * + * Failure events fire on consecutive failures 1 (FailureTransition), + * then kFailureLogInterval, 2×kFailureLogInterval, ... (RepeatedFailure); + * everything in between is Silent. The first success after any failure + * run is Recovery, further successes are Reading. + */ + Event onReadResult(bool readOk) + { + if (readOk) { + consecutiveFailures_ = 0; + if (!wasValid_) { + wasValid_ = true; + return Event::Recovery; + } + return Event::Reading; + } + ++consecutiveFailures_; + if (wasValid_) { + wasValid_ = false; + return Event::FailureTransition; + } + if (consecutiveFailures_ % kFailureLogInterval == 0) { + return Event::RepeatedFailure; + } + return Event::Silent; + } + + /// Consecutive failures since the last success (for the repeat WARN). + uint32_t consecutiveFailures() const { return consecutiveFailures_; } + +private: + bool wasValid_ = true; + uint32_t consecutiveFailures_ = 0; +}; + +#endif /* WATERINGSYSTEM_SENSORS_SENSORTASKLOGPOLICY_H */ diff --git a/firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h b/firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h new file mode 100644 index 0000000..09f2c9e --- /dev/null +++ b/firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h @@ -0,0 +1,139 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file MockEnvironmentalSensor.h + * @brief Scriptable IEnvironmentalSensor test double (header-only). + * + * Serves the host tests of environmental-sensor CONSUMERS (watering + * controller PR-11, web server PR-09): script a sequence of read outcomes + * with the consistency helpers and assert on the call counters. The + * driver's own probe/calibration/compensation logic is tested against the + * REAL Bme280Sensor via MockI2cBus — never through this mock. Never + * compiled into target builds (only included from test code). No IDF + * includes. + * + * Consistency helpers from the start (PR-04 lesson — MockSoilSensor's + * separate result/error/value fields let tests script incoherent states): + * scriptSuccessfulRead()/scriptFailedRead() keep outcome, error code and + * getter values coherent per step, enforcing the interface's last-good + * contract (a failed read never touches the values). + */ + +#ifndef WATERINGSYSTEM_SENSORS_TESTING_MOCKENVIRONMENTALSENSOR_H +#define WATERINGSYSTEM_SENSORS_TESTING_MOCKENVIRONMENTALSENSOR_H + +#include +#include + +#include "interfaces/IEnvironmentalSensor.h" + +/** + * @brief IEnvironmentalSensor replaying a scripted read sequence, + * instrumented for tests. + * + * Each read() consumes the next scripted step; when the script runs out, + * the LAST step repeats forever (a steady sensor keeps delivering, a dead + * one keeps failing — matches the real driver's persistence). An entirely + * unscripted read() succeeds with error 0 and whatever values are current + * (the 0.0 placeholders before any successful step — same + * meaningless-before-first-read contract as the real driver). + * initialize()/isAvailable() results are plain scripted fields + * (MockSoilSensor style); per the interface contract neither touches the + * error code. + */ +class MockEnvironmentalSensor : public IEnvironmentalSensor { +public: + // -- Scripted results (non-read calls) ----------------------------------- + + bool initializeResult = true; ///< returned by initialize() + bool isAvailableResult = true; ///< returned by isAvailable() + + // -- Instrumentation ------------------------------------------------------ + + int initializeCalls = 0; + int readCalls = 0; + int isAvailableCalls = 0; + + // -- Consistency helpers (script one read() outcome each, FIFO) ---------- + + /// Script the next read() to succeed: returns true, error 0, and the + /// getters serve exactly these values afterwards. + void scriptSuccessfulRead(float temperature, float humidity, + float pressure) + { + script_.push_back({true, 0, temperature, humidity, pressure}); + } + + /// Script the next read() to fail with @p error (1 = not found, + /// 2 = read failed): returns false and leaves the getter values + /// untouched (last-good contract). + void scriptFailedRead(int error) + { + script_.push_back({false, error, 0.0f, 0.0f, 0.0f}); + } + + // -- IEnvironmentalSensor ------------------------------------------------- + + bool initialize() override + { + ++initializeCalls; + return initializeResult; + } + + bool read() override + { + ++readCalls; + if (script_.empty()) { + // Unscripted: succeed with the current values (placeholders + // before the first scripted success). + lastError_ = 0; + return true; + } + const Step& step = script_[next_]; + if (next_ + 1 < script_.size()) { + ++next_; // exhausted scripts repeat the last step forever + } + if (!step.ok) { + lastError_ = step.error; + return false; // values untouched — last-good contract + } + temperature_ = step.temperature; + humidity_ = step.humidity; + pressure_ = step.pressure; + lastError_ = 0; + return true; + } + + bool isAvailable() override + { + ++isAvailableCalls; + return isAvailableResult; + } + + int getLastError() override { return lastError_; } + + float getTemperature() override { return temperature_; } + float getHumidity() override { return humidity_; } + float getPressure() override { return pressure_; } + +private: + /// One scripted read() outcome; values are only meaningful when ok. + struct Step { + bool ok; + int error; + float temperature; + float humidity; + float pressure; + }; + + std::vector script_; + size_t next_ = 0; + int lastError_ = 0; + + // Last-good values (placeholders 0.0 before the first scripted success). + float temperature_ = 0.0f; + float humidity_ = 0.0f; + float pressure_ = 0.0f; +}; + +#endif /* WATERINGSYSTEM_SENSORS_TESTING_MOCKENVIRONMENTALSENSOR_H */ diff --git a/firmware/components/sensors/src/Bme280Sensor.cpp b/firmware/components/sensors/src/Bme280Sensor.cpp index cab6239..f23df9d 100644 --- a/firmware/components/sensors/src/Bme280Sensor.cpp +++ b/firmware/components/sensors/src/Bme280Sensor.cpp @@ -258,6 +258,11 @@ bool Bme280Sensor::read() bool Bme280Sensor::isAvailable() { // Lazy initialization doubles as the probe (parity, legacy :75-80). + // Note on the error contract: the availability PROBE below never + // touches lastError_, but this lazy path delegates to initialize(), + // which owns its own error reporting (1/2 on failure, 0 on success) — + // exactly the ModbusSoilSensor::isAvailable() convention the interface + // contract refers to. if (!initialized_) { return initialize(); } diff --git a/firmware/main/sensor_task.cpp b/firmware/main/sensor_task.cpp index ebe69ff..62f6ed2 100644 --- a/firmware/main/sensor_task.cpp +++ b/firmware/main/sensor_task.cpp @@ -9,8 +9,11 @@ * Logging discipline (contracts/interfaces.md): INFO with the three values * on success, WARN once on the valid→invalid transition and once on * recovery, repeated failures at a bounded cadence — every - * kFailureLogInterval-th consecutive failure (12 × 5 s ≈ once a minute) to - * avoid log flood. The task never exits and never reboots on failures; + * SensorTaskLogPolicy::kFailureLogInterval-th consecutive failure + * (12 × 5 s ≈ once a minute) to avoid log flood. The WHAT-to-log decision + * lives in the pure SensorTaskLogPolicy (sensors component) so it is + * host-tested, not review-verified; this task owns only the ESP_LOG calls + * and the cadence. The task never exits and never reboots on failures; * recovery is the sensor driver's lazy re-init, driven by this cadence. */ @@ -20,6 +23,8 @@ #include "freertos/FreeRTOS.h" #include "freertos/task.h" +#include "sensors/SensorTaskLogPolicy.h" + static const char *TAG = "sensor_task"; namespace { @@ -28,18 +33,14 @@ constexpr uint32_t kPeriodMs = 5000; ///< parity poll cadence (R7) constexpr uint32_t kStackBytes = 4096; ///< parity stack size (R7) constexpr UBaseType_t kPriority = 1; ///< parity priority (R7) -/// Repeated-failure log cadence: every Nth consecutive failure (~1/min). -constexpr uint32_t kFailureLogInterval = 12; - [[noreturn]] void sensor_task(void *arg) { IEnvironmentalSensor &sensor = *static_cast(arg); - // Start in the "valid" state so the FIRST failure (e.g. booting with - // no sensor attached) logs the transition warning exactly once. - bool wasValid = true; - uint32_t consecutiveFailures = 0; + // Host-tested log-decision policy: starts "valid" so the FIRST failure + // (e.g. booting with no sensor attached) WARNs exactly once. + SensorTaskLogPolicy logPolicy; TickType_t lastWake = xTaskGetTickCount(); while (true) { @@ -47,36 +48,38 @@ constexpr uint32_t kFailureLogInterval = 12; // conversion completes well within 5 s (research.md R9). vTaskDelayUntil(&lastWake, pdMS_TO_TICKS(kPeriodMs)); - if (sensor.read()) { - consecutiveFailures = 0; - if (!wasValid) { - ESP_LOGW(TAG, "environmental sensor recovered"); - wasValid = true; - } + const bool ok = sensor.read(); + switch (logPolicy.onReadResult(ok)) { + case SensorTaskLogPolicy::Event::Recovery: + ESP_LOGW(TAG, "environmental sensor recovered"); + [[fallthrough]]; // a recovered sensor also logs its reading + case SensorTaskLogPolicy::Event::Reading: // Periodic reading, consistent with the legacy 5 s status print. ESP_LOGI(TAG, "temperature=%.1f C humidity=%.1f %%RH pressure=%.1f hPa", static_cast(sensor.getTemperature()), static_cast(sensor.getHumidity()), static_cast(sensor.getPressure())); - } else { - ++consecutiveFailures; - const int error = sensor.getLastError(); - if (wasValid) { - // Valid → invalid transition: WARN exactly once. - ESP_LOGW(TAG, - "environmental reading invalid (error %d) — " - "keeping last-good values, retrying every %lu ms", - error, static_cast(kPeriodMs)); - wasValid = false; - } else if (consecutiveFailures % kFailureLogInterval == 0) { - // Bounded repeat cadence while the failure persists. - ESP_LOGW(TAG, - "environmental sensor still failing (error %d, " - "%lu consecutive failures)", - error, - static_cast(consecutiveFailures)); - } + break; + case SensorTaskLogPolicy::Event::FailureTransition: + // Valid → invalid transition: WARN exactly once. + ESP_LOGW(TAG, + "environmental reading invalid (error %d) — " + "keeping last-good values, retrying every %lu ms", + sensor.getLastError(), + static_cast(kPeriodMs)); + break; + case SensorTaskLogPolicy::Event::RepeatedFailure: + // Bounded repeat cadence while the failure persists. + ESP_LOGW(TAG, + "environmental sensor still failing (error %d, " + "%lu consecutive failures)", + sensor.getLastError(), + static_cast( + logPolicy.consecutiveFailures())); + break; + case SensorTaskLogPolicy::Event::Silent: + break; } } } diff --git a/firmware/test_apps/host/main/test_bme280.cpp b/firmware/test_apps/host/main/test_bme280.cpp index 61e47d3..fd4db91 100644 --- a/firmware/test_apps/host/main/test_bme280.cpp +++ b/firmware/test_apps/host/main/test_bme280.cpp @@ -9,18 +9,24 @@ * run_bme280_tests() from the shared Unity runner (test_main.cpp); the * process exit code equals the failure count and is the CI gate. * - * Coverage maps to tasks.md T006 (this file starts with the US1 happy - * path: initialization incl. the exact parity config-byte sequence, a - * reference-vector read, getters-before-first-read); T014/T017/T018/T020 - * extend this suite with the error paths, address-variant handling, the - * full Bosch vector set and the MockEnvironmentalSensor consistency tests. + * Coverage maps to tasks.md T006 (US1 happy path: initialization incl. the + * exact parity config-byte sequence, a reference-vector read, + * getters-before-first-read), T014 (US2 error paths: absent sensor, + * mid-read bus error with last-good retention, recovery incl. calibration + * re-read, sensorless boot), T015 (sensor-task log policy), T017 (US3 + * address variants and chip-ID rejection), T018 (Bosch reference vectors, + * integer outputs asserted exactly) and T020 (MockEnvironmentalSensor + * consistency). */ +#include #include #include "unity.h" #include "sensors/Bme280Sensor.h" +#include "sensors/SensorTaskLogPolicy.h" +#include "sensors/testing/MockEnvironmentalSensor.h" #include "sensors/testing/MockI2cBus.h" namespace { @@ -79,6 +85,67 @@ constexpr float kExpectedTemperature = 25.08f; constexpr float kExpectedPressure = 1006.5325f; constexpr float kExpectedHumidity = 69.64746f; +// --------------------------------------------------------------------------- +// The same reference calibration as a Calibration struct, for driving the +// public static compensate*() integer paths directly (T018 vectors). +// --------------------------------------------------------------------------- +constexpr Bme280Sensor::Calibration kRefCal = { + 27504, 26435, -1000, // dig_T1..T3 + 36477, -10685, 3024, 2855, 140, -7, 15500, -14600, 6000, // dig_P1..P9 + 75, 362, 0, 315, 50, 30, // dig_H1..H6 +}; + +// Integer reference outputs for the worked-example raws (adc_T=519888, +// adc_P=415148, adc_H=32768) — Bosch datasheet routines over kRefCal: +constexpr int32_t kRefTFine = 128422; +constexpr int32_t kRefTemperatureInt = 2508; // 0.01 °C +constexpr uint32_t kRefPressureQ248 = 25767233; // Q24.8 Pa +constexpr uint32_t kRefHumidityQ2210 = 71319; // Q22.10 %RH + +// --------------------------------------------------------------------------- +// Calibration set B — a DIFFERENT physical module for the recovery test +// (analyze finding I1: calibration must be re-read on re-init). Identical +// to the reference set except dig_T2 = 30000 (0x7530 LE at 0x8A/0x8B) +// instead of 26435. Expected outputs derived by running the Bosch +// datasheet reference routines over cal-B with the standard raw block: +// t_fine = 145791, T = 2847 (28.47 °C), P = 25901067 (1011.7604 hPa), +// H = 71558 (69.8809 %RH) +// --------------------------------------------------------------------------- +const std::vector kCalibBlock1B = { + 0x70, 0x6B, 0x30, 0x75, 0x18, 0xFC, // dig_T1, T2=30000, T3 + 0x7D, 0x8E, 0x43, 0xD6, 0xD0, 0x0B, 0x27, 0x0B, // dig_P1..P4 + 0x8C, 0x00, 0xF9, 0xFF, 0x8C, 0x3C, 0xF8, 0xC6, // dig_P5..P8 + 0x70, 0x17, // dig_P9 + 0x00, // 0xA0 (unused pad) + 0x4B, // dig_H1 = 75 +}; + +constexpr float kExpectedTemperatureB = 28.47f; // 2847 ÷ 100 +constexpr float kExpectedPressureB = 1011.76043f; // 25901067 ÷ 256 ÷ 100 +constexpr float kExpectedHumidityB = 69.880859f; // 71558 ÷ 1024 + +// --------------------------------------------------------------------------- +// Extra raw-data blocks for T014/T018 (encodings: P/T 20-bit msb,lsb, +// xlsb[7:4]; H 16-bit msb,lsb): +// +// Negative-temperature vector: adc_T=409600 (0x64000), adc_P=415148, +// adc_H=32768. Bosch reference over kRefCal: +// t_fine = -49208, T = -961 (-9.61 °C), P = 24415055 (953.7131 hPa), +// H = 68674 (67.0645 %RH) +// +// Extreme-but-legal vector: adc_T=0xFFFFF (20-bit max), adc_P=0 (20-bit +// min), adc_H=0xFFFF (16-bit max). Bosch reference over kRefCal: +// t_fine = 960246, T = 18755 (187.55 °C), P = 55536661 (2169.4008 hPa), +// H = 102400 (100.000 %RH — the reference algorithm's own upper clamp, +// 419430400 >> 12) +// --------------------------------------------------------------------------- +const std::vector kDataBlockNegativeTemp = { + 0x65, 0x5A, 0xC0, 0x64, 0x00, 0x00, 0x80, 0x00, +}; +const std::vector kDataBlockExtreme = { + 0x00, 0x00, 0x00, 0xFF, 0xFF, 0xF0, 0xFF, 0xFF, +}; + /// Script a complete, healthy BME280 at @p address: chip-ID, both /// calibration blocks and one measurement in the data registers. void scriptBme280(MockI2cBus& mock, uint8_t address) @@ -247,8 +314,590 @@ static void test_read_initializes_lazily(void) sensor.getTemperature()); } +// ========================================================================== +// US2 — error paths (T014) +// ========================================================================== + +// -------------------------------------------------------------------------- +// Absent sensor: probe NACKs on BOTH addresses → initialize() false, +// error 1 ("sensor not found") +// -------------------------------------------------------------------------- +static void test_initialize_absent_sensor_fails_error1(void) +{ + MockI2cBus mock; // no devices — every probe NACKs + Bme280Sensor sensor(mock); + + TEST_ASSERT_FALSE(sensor.initialize()); + TEST_ASSERT_EQUAL_INT(1, sensor.getLastError()); + + // Both candidate addresses were probed before giving up. + int probedPrimary = 0; + int probedSecondary = 0; + for (const MockI2cBus::Call& call : mock.calls) { + if (call.type == MockI2cBus::Call::Type::Probe) { + probedPrimary += call.address == kAddrPrimary; + probedSecondary += call.address == kAddrSecondary; + } + } + TEST_ASSERT_EQUAL(1, probedPrimary); + TEST_ASSERT_EQUAL(1, probedSecondary); +} + +// -------------------------------------------------------------------------- +// Mid-read bus error: read() false, error 2, and the last-good values stay +// untouched (validity contract — consumers gate on read(), values persist) +// -------------------------------------------------------------------------- +static void test_read_bus_error_keeps_last_good_and_sets_error2(void) +{ + Fixture f; + TEST_ASSERT_TRUE(f.sensor.read()); // last-good = reference values + + f.mock.queueReadOutcome(false); // inject a bus error on the data burst + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(2, f.sensor.getLastError()); + + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + f.sensor.getTemperature()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedHumidity, + f.sensor.getHumidity()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedPressure, + f.sensor.getPressure()); +} + +// -------------------------------------------------------------------------- +// Recovery after a bus error: the driver is uninitialized by the failure, +// so the NEXT read() re-probes BOTH addresses (FR-004) and succeeds +// -------------------------------------------------------------------------- +static void test_read_recovers_after_bus_error_with_reprobe(void) +{ + Fixture f; // device at 0x77 + f.mock.queueReadOutcome(false); + TEST_ASSERT_FALSE(f.sensor.read()); + f.mock.calls.clear(); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + + // The recovery read started with a full re-probe: 0x76 first (NACK), + // then 0x77 (found) — not a blind retry at the old address. + TEST_ASSERT_TRUE(f.mock.calls.size() >= 2); + TEST_ASSERT_EQUAL(static_cast(MockI2cBus::Call::Type::Probe), + static_cast(f.mock.calls[0].type)); + TEST_ASSERT_EQUAL_UINT8(kAddrPrimary, f.mock.calls[0].address); + TEST_ASSERT_EQUAL(static_cast(MockI2cBus::Call::Type::Probe), + static_cast(f.mock.calls[1].type)); + TEST_ASSERT_EQUAL_UINT8(kAddrSecondary, f.mock.calls[1].address); +} + +// -------------------------------------------------------------------------- +// Recovery re-reads the CALIBRATION (analyze finding I1): after a bus +// error, the re-attached module carries a different calibration set (a +// different physical sensor) — the recovered reading must reflect the NEW +// trimming parameters, proving re-init did not reuse the stale ones +// -------------------------------------------------------------------------- +static void test_recovery_rereads_calibration_of_replaced_module(void) +{ + Fixture f; // device at 0x77, reference calibration + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + f.sensor.getTemperature()); + + f.mock.queueReadOutcome(false); // sensor lost mid-read + TEST_ASSERT_FALSE(f.sensor.read()); + + // "Replug" a different module at the same address: same raws, cal-B. + f.mock.setRegisters(kAddrSecondary, 0x88, kCalibBlock1B); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperatureB, + f.sensor.getTemperature()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedHumidityB, + f.sensor.getHumidity()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedPressureB, + f.sensor.getPressure()); +} + +// -------------------------------------------------------------------------- +// Sensorless boot, sensor attached later: reads fail with error 1 while +// absent, then the lazy re-init picks the sensor up on a subsequent poll +// without any manual intervention (US2 scenario 3) +// -------------------------------------------------------------------------- +static void test_boot_sensorless_then_attach_recovers(void) +{ + MockI2cBus mock; // boot: nothing on the bus + Bme280Sensor sensor(mock); + + TEST_ASSERT_FALSE(sensor.read()); + TEST_ASSERT_EQUAL_INT(1, sensor.getLastError()); + + scriptBme280(mock, kAddrPrimary); // attach the module later + + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + sensor.getTemperature()); +} + +// -------------------------------------------------------------------------- +// isAvailable() on an initialized sensor is ONE real chip-ID read and does +// not touch the error code (read() owns the reading's error state) +// -------------------------------------------------------------------------- +static void test_is_available_reads_chip_id_and_leaves_error_untouched(void) +{ + Fixture f; + TEST_ASSERT_TRUE(f.sensor.read()); + f.mock.calls.clear(); + + TEST_ASSERT_TRUE(f.sensor.isAvailable()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + const MockI2cBus::Call& call = f.mock.calls.front(); + TEST_ASSERT_EQUAL(static_cast(MockI2cBus::Call::Type::Read), + static_cast(call.type)); + TEST_ASSERT_EQUAL_UINT8(kRegChipId, call.reg); + TEST_ASSERT_EQUAL(1, call.len); +} + +// -------------------------------------------------------------------------- +// isAvailable() detecting a loss: returns false WITHOUT touching the error +// code, and the next call (lazy re-init, sensor back) reports available +// again. (The lazy path delegates to initialize(), which owns its own +// error reporting — ModbusSoilSensor convention.) +// -------------------------------------------------------------------------- +static void test_is_available_false_on_loss_leaves_error_untouched(void) +{ + Fixture f; + TEST_ASSERT_TRUE(f.sensor.read()); // error 0 + + f.mock.queueReadOutcome(false); // chip-ID read fails: sensor lost + TEST_ASSERT_FALSE(f.sensor.isAvailable()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); // untouched + + // Device still scripted → the lazy re-probe finds it again. + TEST_ASSERT_TRUE(f.sensor.isAvailable()); +} + +// -------------------------------------------------------------------------- +// NaN guard (US2 scenario 4, FR-007): with the Bosch INTEGER compensation +// paths a NaN is unreachable from any raw register content — the guard is +// retained as the binding legacy-parity safety net. Verified here at the +// extreme of the legal raw range: the read stays finite and succeeds. +// (The NaN→error-2 branch itself cannot be triggered through the II2cBus +// seam; it would require a float path that does not exist.) +// -------------------------------------------------------------------------- +static void test_read_extreme_raws_never_produce_nan(void) +{ + Fixture f; + f.mock.setRegisters(kAddrSecondary, kRegData, kDataBlockExtreme); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + TEST_ASSERT_FALSE(std::isnan(f.sensor.getTemperature())); + TEST_ASSERT_FALSE(std::isnan(f.sensor.getHumidity())); + TEST_ASSERT_FALSE(std::isnan(f.sensor.getPressure())); +} + +// ========================================================================== +// US3 — address variants and chip identity (T017) +// ========================================================================== + +// -------------------------------------------------------------------------- +// Module strapped to 0x76: found, identified and read — every transaction +// stays on the primary address +// -------------------------------------------------------------------------- +static void test_device_at_primary_delivers_reading(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrPrimary); + + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + sensor.getTemperature()); + for (const MockI2cBus::Call& call : mock.calls) { + TEST_ASSERT_EQUAL_UINT8(kAddrPrimary, call.address); + } +} + +// -------------------------------------------------------------------------- +// Module strapped to 0x77 (bench rig / greenhouse variant): found after +// the 0x76 NACK and read normally +// -------------------------------------------------------------------------- +static void test_device_at_secondary_delivers_reading(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrSecondary); + + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + sensor.getTemperature()); +} + +// -------------------------------------------------------------------------- +// Wrong chip identity at 0x76 (e.g. a BMP280, ID 0x58) with a real BME280 +// at 0x77: the imposter is rejected and 0x77 is selected +// -------------------------------------------------------------------------- +static void test_wrong_chip_id_at_primary_selects_secondary(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + mock.addDevice(kAddrPrimary); + mock.setRegister(kAddrPrimary, kRegChipId, 0x58); // BMP280 — no RH path + scriptBme280(mock, kAddrSecondary); + + TEST_ASSERT_TRUE(sensor.initialize()); + TEST_ASSERT_TRUE(sensor.read()); + + // All data traffic goes to the verified device at 0x77. + for (const MockI2cBus::Call& call : mock.calls) { + if (call.type == MockI2cBus::Call::Type::Write || + call.reg == kRegData) { + TEST_ASSERT_EQUAL_UINT8(kAddrSecondary, call.address); + } + } +} + +// -------------------------------------------------------------------------- +// Foreign devices ACKing on BOTH addresses (address collision): every +// candidate fails the identity check → error 1, sensor unavailable rather +// than misread (US3 scenario 3 / edge case) +// -------------------------------------------------------------------------- +static void test_wrong_chip_id_everywhere_fails_error1(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + mock.addDevice(kAddrPrimary); + mock.setRegister(kAddrPrimary, kRegChipId, 0x58); + mock.addDevice(kAddrSecondary); + mock.setRegister(kAddrSecondary, kRegChipId, 0x61); // BME680 + + TEST_ASSERT_FALSE(sensor.initialize()); + TEST_ASSERT_EQUAL_INT(1, sensor.getLastError()); +} + +// -------------------------------------------------------------------------- +// Loss, then reappearance at the OTHER address (module swapped 0x77→0x76 +// while unpowered): the recovery re-probe covers both addresses, so the +// swap is transparent (spec edge case, FR-004) +// -------------------------------------------------------------------------- +static void test_loss_then_reappearance_at_other_address_recovers(void) +{ + Fixture f; // device at 0x77 + TEST_ASSERT_TRUE(f.sensor.read()); + + f.mock.removeDevice(kAddrSecondary); // unplug + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(2, f.sensor.getLastError()); + + scriptBme280(f.mock, kAddrPrimary); // replug, strapped to 0x76 now + f.mock.calls.clear(); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + // The data burst of the recovered reading happened at 0x76. + const MockI2cBus::Call& last = f.mock.calls.back(); + TEST_ASSERT_EQUAL_UINT8(kRegData, last.reg); + TEST_ASSERT_EQUAL_UINT8(kAddrPrimary, last.address); +} + +// ========================================================================== +// US4 — Bosch reference vectors (T018): integer outputs asserted EXACTLY, +// then the float conversions (÷100, ÷256÷100, ÷1024). Every expectation is +// derived by running the Bosch datasheet reference routines +// (BME280_compensate_T_int32 / _P_int64 / _H_int32) over kRefCal — see the +// per-vector derivation comments at the constants above. +// ========================================================================== + +// -------------------------------------------------------------------------- +// Datasheet worked example: adc_T=519888, adc_P=415148 → 25.08 °C, +// 100653.25 Pa (datasheet double result: 100653.27 Pa) + humidity extension +// -------------------------------------------------------------------------- +static void test_compensation_worked_example_integer_outputs(void) +{ + int32_t tFine = 0; + const int32_t t = Bme280Sensor::compensateTemperature(519888, kRefCal, + tFine); + TEST_ASSERT_EQUAL_INT32(kRefTFine, tFine); + TEST_ASSERT_EQUAL_INT32(kRefTemperatureInt, t); + + const uint32_t p = Bme280Sensor::compensatePressure(415148, kRefCal, + tFine); + TEST_ASSERT_EQUAL_UINT32(kRefPressureQ248, p); + + const uint32_t h = Bme280Sensor::compensateHumidity(32768, kRefCal, + tFine); + TEST_ASSERT_EQUAL_UINT32(kRefHumidityQ2210, h); + + // Float conversions (÷100, ÷256÷100, ÷1024). + TEST_ASSERT_EQUAL_FLOAT(25.08f, static_cast(t) / 100.0f); + TEST_ASSERT_EQUAL_FLOAT(1006.5325f, + static_cast(p) / 256.0f / 100.0f); + TEST_ASSERT_EQUAL_FLOAT(69.647461f, static_cast(h) / 1024.0f); +} + +// -------------------------------------------------------------------------- +// Negative-temperature vector: adc_T=409600 (0x64000) over kRefCal → +// t_fine=-49208, T=-961 (-9.61 °C); P/H fed with the negative t_fine +// (derivation: Bosch reference routines, constants comment above) +// -------------------------------------------------------------------------- +static void test_compensation_negative_temperature_vector(void) +{ + int32_t tFine = 0; + const int32_t t = Bme280Sensor::compensateTemperature(409600, kRefCal, + tFine); + TEST_ASSERT_EQUAL_INT32(-49208, tFine); + TEST_ASSERT_EQUAL_INT32(-961, t); + + const uint32_t p = Bme280Sensor::compensatePressure(415148, kRefCal, + tFine); + TEST_ASSERT_EQUAL_UINT32(24415055, p); + + const uint32_t h = Bme280Sensor::compensateHumidity(32768, kRefCal, + tFine); + TEST_ASSERT_EQUAL_UINT32(68674, h); + + TEST_ASSERT_EQUAL_FLOAT(-9.61f, static_cast(t) / 100.0f); + TEST_ASSERT_EQUAL_FLOAT(953.71309f, + static_cast(p) / 256.0f / 100.0f); + TEST_ASSERT_EQUAL_FLOAT(67.064453f, static_cast(h) / 1024.0f); +} + +// -------------------------------------------------------------------------- +// Extreme-but-legal raws: adc_T=0xFFFFF, adc_P=0, adc_H=0xFFFF over +// kRefCal → T=18755, P=55536661, H=102400 (= exactly 100 %RH, the +// reference algorithm's own upper clamp 419430400>>12; derivation in the +// constants comment above) +// -------------------------------------------------------------------------- +static void test_compensation_extreme_legal_raws_vector(void) +{ + int32_t tFine = 0; + const int32_t t = Bme280Sensor::compensateTemperature(0xFFFFF, kRefCal, + tFine); + TEST_ASSERT_EQUAL_INT32(960246, tFine); + TEST_ASSERT_EQUAL_INT32(18755, t); + + const uint32_t p = Bme280Sensor::compensatePressure(0, kRefCal, tFine); + TEST_ASSERT_EQUAL_UINT32(55536661, p); + + const uint32_t h = Bme280Sensor::compensateHumidity(0xFFFF, kRefCal, + tFine); + TEST_ASSERT_EQUAL_UINT32(102400, h); + + TEST_ASSERT_EQUAL_FLOAT(187.55f, static_cast(t) / 100.0f); + TEST_ASSERT_EQUAL_FLOAT(2169.4008f, + static_cast(p) / 256.0f / 100.0f); + TEST_ASSERT_EQUAL_FLOAT(100.0f, static_cast(h) / 1024.0f); +} + +// -------------------------------------------------------------------------- +// End-to-end read of the negative-temperature raw block: register +// assembly + compensation + float conversion through the full read() path +// -------------------------------------------------------------------------- +static void test_read_negative_temperature_block(void) +{ + Fixture f; + f.mock.setRegisters(kAddrSecondary, kRegData, kDataBlockNegativeTemp); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, -9.61f, f.sensor.getTemperature()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, 67.064453f, f.sensor.getHumidity()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, 953.71309f, f.sensor.getPressure()); +} + +// ========================================================================== +// Sensor-task log policy (T015, analyze finding U1): the WARN/INFO/silence +// decisions of main/sensor_task.cpp, host-tested deterministically +// ========================================================================== + +// -------------------------------------------------------------------------- +// Sensorless boot: the policy starts "valid", so failure #1 WARNs exactly +// once (FailureTransition); repeats fire at every 12th consecutive failure +// and everything in between is silent +// -------------------------------------------------------------------------- +static void test_log_policy_warns_once_then_bounded_repeats(void) +{ + SensorTaskLogPolicy policy; + + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::FailureTransition), + static_cast(policy.onReadResult(false))); // failure 1 + + for (uint32_t failure = 2; failure <= 25; ++failure) { + const SensorTaskLogPolicy::Event event = policy.onReadResult(false); + if (failure % SensorTaskLogPolicy::kFailureLogInterval == 0) { + TEST_ASSERT_EQUAL( + static_cast( + SensorTaskLogPolicy::Event::RepeatedFailure), + static_cast(event)); // failures 12 and 24 + TEST_ASSERT_EQUAL_UINT32(failure, policy.consecutiveFailures()); + } else { + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::Silent), + static_cast(event)); + } + } +} + +// -------------------------------------------------------------------------- +// Recovery: the first success after a failure run is a Recovery WARN, +// further successes are plain INFO readings, and a NEW failure run WARNs +// its transition again (the once-per-transition discipline) +// -------------------------------------------------------------------------- +static void test_log_policy_warns_on_recovery_then_reads(void) +{ + SensorTaskLogPolicy policy; + + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::Reading), + static_cast(policy.onReadResult(true))); // steady state + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::FailureTransition), + static_cast(policy.onReadResult(false))); + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::Recovery), + static_cast(policy.onReadResult(true))); + TEST_ASSERT_EQUAL_UINT32(0, policy.consecutiveFailures()); + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::Reading), + static_cast(policy.onReadResult(true))); + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::FailureTransition), + static_cast(policy.onReadResult(false))); +} + +// -------------------------------------------------------------------------- +// Hours-long outage (spec edge case): the log volume stays bounded — over +// 1000 consecutive failures exactly 1 transition WARN + 83 repeat WARNs — +// and the policy keeps producing decisions (no terminal state, no exit) +// -------------------------------------------------------------------------- +static void test_log_policy_long_outage_stays_bounded_and_never_stops(void) +{ + SensorTaskLogPolicy policy; + uint32_t transitions = 0; + uint32_t repeats = 0; + uint32_t silents = 0; + + for (int i = 0; i < 1000; ++i) { + switch (policy.onReadResult(false)) { + case SensorTaskLogPolicy::Event::FailureTransition: ++transitions; break; + case SensorTaskLogPolicy::Event::RepeatedFailure: ++repeats; break; + case SensorTaskLogPolicy::Event::Silent: ++silents; break; + default: TEST_FAIL_MESSAGE("success event from a failed read"); + } + } + TEST_ASSERT_EQUAL_UINT32(1, transitions); + TEST_ASSERT_EQUAL_UINT32(83, repeats); // floor(1000 / 12) + TEST_ASSERT_EQUAL_UINT32(916, silents); + + // Still alive after the outage: recovery is reported normally. + TEST_ASSERT_EQUAL( + static_cast(SensorTaskLogPolicy::Event::Recovery), + static_cast(policy.onReadResult(true))); +} + +// ========================================================================== +// MockEnvironmentalSensor consistency (T020) — the consumer-facing double +// for PR-09/PR-11 tests +// ========================================================================== + +// -------------------------------------------------------------------------- +// A scripted sequence is observed exactly: success → failure (last-good +// values retained, scripted error) → success (fresh values, error 0) +// -------------------------------------------------------------------------- +static void test_mock_env_scripted_sequence_observed_exactly(void) +{ + MockEnvironmentalSensor mock; + mock.scriptSuccessfulRead(21.5f, 40.0f, 1013.2f); + mock.scriptFailedRead(2); + mock.scriptSuccessfulRead(22.0f, 41.0f, 1012.8f); + + TEST_ASSERT_TRUE(mock.read()); + TEST_ASSERT_EQUAL_INT(0, mock.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(21.5f, mock.getTemperature()); + TEST_ASSERT_EQUAL_FLOAT(40.0f, mock.getHumidity()); + TEST_ASSERT_EQUAL_FLOAT(1013.2f, mock.getPressure()); + + TEST_ASSERT_FALSE(mock.read()); + TEST_ASSERT_EQUAL_INT(2, mock.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(21.5f, mock.getTemperature()); // last-good + TEST_ASSERT_EQUAL_FLOAT(40.0f, mock.getHumidity()); + TEST_ASSERT_EQUAL_FLOAT(1013.2f, mock.getPressure()); + + TEST_ASSERT_TRUE(mock.read()); + TEST_ASSERT_EQUAL_INT(0, mock.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(22.0f, mock.getTemperature()); + TEST_ASSERT_EQUAL_FLOAT(41.0f, mock.getHumidity()); + TEST_ASSERT_EQUAL_FLOAT(1012.8f, mock.getPressure()); + + TEST_ASSERT_EQUAL(3, mock.readCalls); +} + +// -------------------------------------------------------------------------- +// An exhausted script repeats its LAST step forever (steady sensor keeps +// delivering, dead sensor keeps failing) — documented convenience for +// consumer tests that poll more often than they script +// -------------------------------------------------------------------------- +static void test_mock_env_exhausted_script_repeats_last_step(void) +{ + MockEnvironmentalSensor good; + good.scriptSuccessfulRead(20.0f, 50.0f, 1000.0f); + TEST_ASSERT_TRUE(good.read()); + TEST_ASSERT_TRUE(good.read()); // repeats the success + TEST_ASSERT_EQUAL_FLOAT(20.0f, good.getTemperature()); + TEST_ASSERT_EQUAL_INT(0, good.getLastError()); + + MockEnvironmentalSensor dead; + dead.scriptSuccessfulRead(20.0f, 50.0f, 1000.0f); + dead.scriptFailedRead(1); + TEST_ASSERT_TRUE(dead.read()); + TEST_ASSERT_FALSE(dead.read()); + TEST_ASSERT_FALSE(dead.read()); // repeats the failure + TEST_ASSERT_EQUAL_INT(1, dead.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(20.0f, dead.getTemperature()); // last-good +} + +// -------------------------------------------------------------------------- +// Helpers keep values/validity/error coherent: a failed step carries its +// error and leaves values alone; a successful step always resets error 0. +// Unscripted reads succeed with the placeholder values (0.0), matching the +// real driver's before-first-read contract +// -------------------------------------------------------------------------- +static void test_mock_env_helpers_keep_state_coherent(void) +{ + MockEnvironmentalSensor unscripted; + TEST_ASSERT_TRUE(unscripted.read()); + TEST_ASSERT_EQUAL_INT(0, unscripted.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(0.0f, unscripted.getTemperature()); + + MockEnvironmentalSensor mock; + mock.scriptFailedRead(1); // e.g. sensorless boot + mock.scriptSuccessfulRead(18.0f, 55.0f, 990.0f); + TEST_ASSERT_FALSE(mock.read()); + TEST_ASSERT_EQUAL_INT(1, mock.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(0.0f, mock.getTemperature()); // still placeholder + TEST_ASSERT_TRUE(mock.read()); + TEST_ASSERT_EQUAL_INT(0, mock.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(18.0f, mock.getTemperature()); + + // initialize()/isAvailable() are scripted fields with call counters and + // never touch the error code (interface convention). + mock.initializeResult = false; + mock.isAvailableResult = false; + TEST_ASSERT_FALSE(mock.initialize()); + TEST_ASSERT_FALSE(mock.isAvailable()); + TEST_ASSERT_EQUAL_INT(0, mock.getLastError()); + TEST_ASSERT_EQUAL(1, mock.initializeCalls); + TEST_ASSERT_EQUAL(1, mock.isAvailableCalls); +} + void run_bme280_tests(void) { + // US1 (T006) RUN_TEST(test_initialize_writes_parity_config_bytes_in_order); RUN_TEST(test_initialize_probes_primary_address_first); RUN_TEST(test_initialize_is_idempotent); @@ -256,4 +905,32 @@ void run_bme280_tests(void) RUN_TEST(test_read_is_one_eight_byte_burst); RUN_TEST(test_getters_before_first_read_are_placeholders); RUN_TEST(test_read_initializes_lazily); + // US2 (T014) + RUN_TEST(test_initialize_absent_sensor_fails_error1); + RUN_TEST(test_read_bus_error_keeps_last_good_and_sets_error2); + RUN_TEST(test_read_recovers_after_bus_error_with_reprobe); + RUN_TEST(test_recovery_rereads_calibration_of_replaced_module); + RUN_TEST(test_boot_sensorless_then_attach_recovers); + RUN_TEST(test_is_available_reads_chip_id_and_leaves_error_untouched); + RUN_TEST(test_is_available_false_on_loss_leaves_error_untouched); + RUN_TEST(test_read_extreme_raws_never_produce_nan); + // US3 (T017) + RUN_TEST(test_device_at_primary_delivers_reading); + RUN_TEST(test_device_at_secondary_delivers_reading); + RUN_TEST(test_wrong_chip_id_at_primary_selects_secondary); + RUN_TEST(test_wrong_chip_id_everywhere_fails_error1); + RUN_TEST(test_loss_then_reappearance_at_other_address_recovers); + // US4 (T018) + RUN_TEST(test_compensation_worked_example_integer_outputs); + RUN_TEST(test_compensation_negative_temperature_vector); + RUN_TEST(test_compensation_extreme_legal_raws_vector); + RUN_TEST(test_read_negative_temperature_block); + // Sensor-task log policy (T015) + RUN_TEST(test_log_policy_warns_once_then_bounded_repeats); + RUN_TEST(test_log_policy_warns_on_recovery_then_reads); + RUN_TEST(test_log_policy_long_outage_stays_bounded_and_never_stops); + // MockEnvironmentalSensor (T020) + RUN_TEST(test_mock_env_scripted_sequence_observed_exactly); + RUN_TEST(test_mock_env_exhausted_script_repeats_last_step); + RUN_TEST(test_mock_env_helpers_keep_state_coherent); } diff --git a/specs/005-bme280-i2c/checklists/hil.md b/specs/005-bme280-i2c/checklists/hil.md new file mode 100644 index 0000000..ac283df --- /dev/null +++ b/specs/005-bme280-i2c/checklists/hil.md @@ -0,0 +1,70 @@ +# HIL Checklist: BME280 Environmental Sensor (005) — rev1 bench rig + +**Purpose**: hardware-in-the-loop verification of PR-03 at Checkpoint 3 (Paul, bench rig) +**Rig**: ESP32 devkit + BME280 breakout on I2C SDA=GPIO21 SCL=GPIO22 (rig module strapped to 0x77), Arduino unit nearby as reading reference +**Build**: rev1 target (`sdkconfig.board.rev1_devkit`), flash per `firmware/CLAUDE.md` +**Reference**: acceptance criteria `docs/prd/PR-03-bme280-i2c.md`; spec SC-003/004/005/006 +**Note**: section C doubles as an implicit regression check of the soil sensor sharing the rig — the RS485 subsystem must keep answering while the I2C sensor is unplugged (subsystem isolation). + +## A. Periodic readings & Arduino agreement (US1, SC-003) + +- [ ] A1. Boot with the BME280 connected; log shows + `temperature=… C humidity=… %RH pressure=… hPa` every 5 s (steady + cadence, first reading within ~10 s of boot) +- [ ] A2. Values are plausible for the environment (room: ~15–30 °C, + ~20–70 %RH, ~950–1050 hPa) +- [ ] A3. Compare against the Arduino unit in the same spot: agreement + within ±0.5 °C, ±3 %RH, ±1 hPa (identical parity sampling profile — + smoothness should also look alike, no extra jitter) +- [ ] A4. Let it run ≥10 min: cadence stays 5 s, no drift, no watchdog + resets, heap stable (no reboot lines in the log) + +## B. Console `env` command (US1/SC-006) + +- [ ] B1. `env` with the sensor connected → one immediate reading: + `OK temperature=… C humidity=… %RH pressure=… hPa` (documented + syntax works on the first attempt) +- [ ] B2. `env` with the sensor UNPLUGGED (after B4/C1 below or a + sensorless boot) → `ERROR 1 (sensor not found)` — distinguishable + from a read failure +- [ ] B3. If a mid-transaction failure can be provoked (wiggle SDA during + a poll): `ERROR 2 (read failed)` appears for the failed read — the + absent-vs-failed distinction is visible on the console (host tests + cover both codes deterministically; this bench item is + best-effort) +- [ ] B4. `env` immediately after replugging → OK reading again, no + restart needed + +## C. Unplug/replug & sensorless boot (US2, SC-004) + +- [ ] C1. Unplug the BME280 mid-run → next poll logs ONE + `environmental reading invalid (error …)` WARN; system keeps + running, no crash, no reboot +- [ ] C2. Leave it unplugged ≥2 min → repeat WARN appears at the bounded + cadence (~once a minute, every 12th failure), NOT every 5 s (no log + flood) +- [ ] C3. While unplugged: `soil` still answers normally (RS485 unaffected + by the I2C outage — implicit soil-sensor regression check) +- [ ] C4. Replug → within a poll or two: `environmental sensor recovered` + WARN followed by normal 5 s readings, no manual intervention +- [ ] C5. Boot WITHOUT the sensor → normal startup (console up, pumps OFF), + one invalid-reading WARN, no boot block; attach the sensor → + readings start on a subsequent poll (lazy re-init) + +## D. Address variant 0x76 (US3, SC-005) + +- [ ] D1. If a 0x76-strapped module (or a solderable ADDR pad) is on hand: + swap modules while unpowered, boot → sensor found at 0x76 with no + config/code change, readings flow. If NO 0x76 hardware is available: + mark host-covered — probe order, wrong-chip-ID rejection and + swapped-address recovery are all verified deterministically in + `test_bme280.cpp` (SC-005 allows this) + +## E. Regression guard + +- [ ] E1. Pumps still OFF at boot (watch outputs during A1/C5 boots) — + safety invariant untouched by this PR +- [ ] E2. Existing console commands still respond normally: `pump status`, + `soil`, `rs485test`, `config get`, `storage stats` + +**Sign-off**: date + result per item as PR comment (pattern from PR #7). diff --git a/specs/005-bme280-i2c/tasks.md b/specs/005-bme280-i2c/tasks.md index e94bc38..41d92a4 100644 --- a/specs/005-bme280-i2c/tasks.md +++ b/specs/005-bme280-i2c/tasks.md @@ -13,7 +13,7 @@ surface and host-test harness this feature extends arrive with it. ## Phase 1: Setup -- [ ] T001 Rebase/merge branch `005-bme280-i2c` onto origin/main AFTER PR #10 has merged; verify `firmware/components/sensors/`, `diag_console_register_soil` and `test_apps/host/main/test_soil_sensor.cpp` exist on the branch; verify both board targets and the host suite build green from clean checkout (baseline before any change) +- [x] T001 Rebase/merge branch `005-bme280-i2c` onto origin/main AFTER PR #10 has merged; verify `firmware/components/sensors/`, `diag_console_register_soil` and `test_apps/host/main/test_soil_sensor.cpp` exist on the branch; verify both board targets and the host suite build green from clean checkout (baseline before any change) ## Phase 2: Foundational (blocking prerequisites for all user stories) @@ -44,9 +44,9 @@ surface and host-test harness this feature extends arrive with it. **Independent Test**: unplug mid-run → invalid + warning, no crash; replug → recovery; boot sensorless → normal boot, later attach recovers. -- [ ] T013 [US2] Harden `Bme280Sensor` failure semantics in `firmware/components/sensors/src/Bme280Sensor.cpp`: bus error during read → error 2, getters keep last-good values, driver marked uninitialized (next call re-probes — recovery path); failed probe/chip-ID → error 1; lazy re-init from read() AND isAvailable() when uninitialized; isAvailable() = real chip-ID read, never cached, never touches lastError (contracts/interfaces.md, data-model.md state machine) -- [ ] T014 [P] [US2] Host tests for error paths in `firmware/test_apps/host/main/test_bme280.cpp`: absent sensor (probe NACK both addresses) → initialize false + error 1; mid-read bus error → read false + error 2 + last-good values intact + subsequent recovery re-probe succeeds; NaN-producing raw values → error 2; boot-sensorless then attach → lazy re-init delivers reading; isAvailable true/false against scripted bus, error code untouched -- [ ] T015 [US2] Verify sensor-task logging discipline against a scripted failing/recovering sensor: WARN on transitions, bounded repeat cadence, no task exit — host-test the pure log-decision helper if extracted, otherwise verify by targeted code review note in the task (logging policy per research R7) +- [x] T013 [US2] Harden `Bme280Sensor` failure semantics in `firmware/components/sensors/src/Bme280Sensor.cpp`: bus error during read → error 2, getters keep last-good values, driver marked uninitialized (next call re-probes — recovery path); failed probe/chip-ID → error 1; lazy re-init from read() AND isAvailable() when uninitialized; isAvailable() = real chip-ID read, never cached, never touches lastError (contracts/interfaces.md, data-model.md state machine) +- [x] T014 [P] [US2] Host tests for error paths in `firmware/test_apps/host/main/test_bme280.cpp`: absent sensor (probe NACK both addresses) → initialize false + error 1; mid-read bus error → read false + error 2 + last-good values intact + subsequent recovery re-probe succeeds; NaN-producing raw values → error 2; boot-sensorless then attach → lazy re-init delivers reading; isAvailable true/false against scripted bus, error code untouched +- [x] T015 [US2] Verify sensor-task logging discipline against a scripted failing/recovering sensor: WARN on transitions, bounded repeat cadence, no task exit — host-test the pure log-decision helper if extracted, otherwise verify by targeted code review note in the task (logging policy per research R7) ## Phase 5: User Story 3 — Works with both module address variants (P3) @@ -54,8 +54,8 @@ surface and host-test harness this feature extends arrive with it. **Independent Test**: scripted buses with the device at each address; wrong-chip-ID device rejected (host); module swap on rig if hardware available. -- [ ] T016 [US3] Verify/complete probing policy in `firmware/components/sensors/src/Bme280Sensor.cpp`: probe order 0x76 → 0x77, first ACK + correct chip-ID wins; ACK with wrong chip-ID logged distinctly and rejected (continues to next candidate, else error 1); recovery after loss re-probes BOTH addresses (covers swapped-address replug edge case) -- [ ] T017 [P] [US3] Host tests for address handling in `firmware/test_apps/host/main/test_bme280.cpp`: device at 0x76 found; device at 0x77 found; wrong-chip-ID at 0x76 with real device at 0x77 → 0x77 chosen; wrong-chip-ID everywhere → error 1; loss then reappearance at the OTHER address → recovered +- [x] T016 [US3] Verify/complete probing policy in `firmware/components/sensors/src/Bme280Sensor.cpp`: probe order 0x76 → 0x77, first ACK + correct chip-ID wins; ACK with wrong chip-ID logged distinctly and rejected (continues to next candidate, else error 1); recovery after loss re-probes BOTH addresses (covers swapped-address replug edge case) +- [x] T017 [P] [US3] Host tests for address handling in `firmware/test_apps/host/main/test_bme280.cpp`: device at 0x76 found; device at 0x77 found; wrong-chip-ID at 0x76 with real device at 0x77 → 0x77 chosen; wrong-chip-ID everywhere → error 1; loss then reappearance at the OTHER address → recovered ## Phase 6: User Story 4 — Sensor behavior testable without hardware (P4) @@ -63,17 +63,17 @@ surface and host-test harness this feature extends arrive with it. **Independent Test**: host suite passes on a hardware-less machine; CI job green. -- [ ] T018 [US4] Add Bosch reference-vector tests in `firmware/test_apps/host/main/test_bme280.cpp`: fixed (calibration set, raw T/P/H) → expected compensated outputs from the Bosch datasheet reference implementation (int32 T, int64 P, int32 H), incl. the datasheet worked example, a negative-temperature vector and extreme-but-legal raws; each vector's derivation cited in a comment (research R8); float conversions (÷100, ÷256÷100, ÷1024) asserted -- [ ] T019 [P] [US4] Create `MockEnvironmentalSensor` in `firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h` — scripted value/validity/error sequences WITH consistency helpers `scriptSuccessfulRead(t,h,p)` / `scriptFailedRead(error)` from the start (PR-04 lesson), for PR-11 consumer tests -- [ ] T020 [P] [US4] Host tests for MockEnvironmentalSensor consistency in `firmware/test_apps/host/main/test_bme280.cpp`: scripted sequences observed exactly; helpers keep values/validity/error coherent +- [x] T018 [US4] Add Bosch reference-vector tests in `firmware/test_apps/host/main/test_bme280.cpp`: fixed (calibration set, raw T/P/H) → expected compensated outputs from the Bosch datasheet reference implementation (int32 T, int64 P, int32 H), incl. the datasheet worked example, a negative-temperature vector and extreme-but-legal raws; each vector's derivation cited in a comment (research R8); float conversions (÷100, ÷256÷100, ÷1024) asserted +- [x] T019 [P] [US4] Create `MockEnvironmentalSensor` in `firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h` — scripted value/validity/error sequences WITH consistency helpers `scriptSuccessfulRead(t,h,p)` / `scriptFailedRead(error)` from the start (PR-04 lesson), for PR-11 consumer tests +- [x] T020 [P] [US4] Host tests for MockEnvironmentalSensor consistency in `firmware/test_apps/host/main/test_bme280.cpp`: scripted sequences observed exactly; helpers keep values/validity/error coherent - [x] T021 [US4] Register the suite: add `test_bme280.cpp` to SRCS in `firmware/test_apps/host/main/CMakeLists.txt`, declare + call `run_bme280_tests()` in `firmware/test_apps/host/main/test_main.cpp` between UNITY_BEGIN/UNITY_END — NOTE: pulled forward into implementation mission 1 (with T002–T012) so the main session can run the T006 suite immediately ## Phase 7: Polish & Cross-Cutting -- [ ] T022 [P] Update `firmware/CLAUDE.md`: directory tree (Bme280Sensor/EspI2cBus/Locked/testing files, sensor_task), console command list (`env`), a "BME280 environmental sensor" section (architecture split, shared-bus note for PR-05, parity sampling profile), host-test description -- [ ] T023 [P] Record deliberate divergences in `docs/parity-checklist.md` §6: address probing + chip-ID check, last-good getter values (legacy: NaN), live availability probe (legacy: cached), Locked-decorator synchronization (legacy: unsynchronized dual readers) — per contracts/interfaces.md list -- [ ] T024 [P] Write HIL checklist `specs/005-bme280-i2c/checklists/hil.md` following `specs/004-modbus-soil-sensor/checklists/hil.md` format: (A) periodic readings + Arduino agreement, (B) console `env` incl. absent-vs-failed distinction, (C) unplug/replug + boot-without-sensor, (D) 0x76 module swap (if hardware available, else mark host-covered), (E) regression guard (pumps OFF at boot, pump/soil console commands intact) -- [ ] T025 Full verification per `specs/005-bme280-i2c/quickstart.md`: host suite exit 0; rev1 AND rev2 build green from clean checkout (fullclean between overlays); `dependencies.lock` unchanged; capture outputs for the CP3 dossier +- [x] T022 [P] Update `firmware/CLAUDE.md`: directory tree (Bme280Sensor/EspI2cBus/Locked/testing files, sensor_task), console command list (`env`), a "BME280 environmental sensor" section (architecture split, shared-bus note for PR-05, parity sampling profile), host-test description +- [x] T023 [P] Record deliberate divergences in `docs/parity-checklist.md` §6: address probing + chip-ID check, last-good getter values (legacy: NaN), live availability probe (legacy: cached), Locked-decorator synchronization (legacy: unsynchronized dual readers) — per contracts/interfaces.md list +- [x] T024 [P] Write HIL checklist `specs/005-bme280-i2c/checklists/hil.md` following `specs/004-modbus-soil-sensor/checklists/hil.md` format: (A) periodic readings + Arduino agreement, (B) console `env` incl. absent-vs-failed distinction, (C) unplug/replug + boot-without-sensor, (D) 0x76 module swap (if hardware available, else mark host-covered), (E) regression guard (pumps OFF at boot, pump/soil console commands intact) +- [x] T025 Full verification per `specs/005-bme280-i2c/quickstart.md`: host suite exit 0; rev1 AND rev2 build green from clean checkout (fullclean between overlays); `dependencies.lock` unchanged; capture outputs for the CP3 dossier ## Dependencies & Execution Order From 4acbf0428b1b54ddffaff3271639d1b93fee206b Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 22:10:53 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(sensors):=20CP3=20review=20=E2=80=94=20?= =?UTF-8?q?log=20visibility,=20error-code=20truth,=20mock=20UB,=20test=20h?= =?UTF-8?q?ardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings A1-A5, B1-B9, C1-C4+C8: infrastructure I2C failures now ESP_LOGE/WARN instead of compiled-out debug (wedged bus distinguishable from absent device); bounded init-failure logging (sensorless boot no longer WARNs every 5 s); NaN pre-first-read placeholders (self-announcing for non-gating consumers); MockEnvironmentalSensor initialize() reports error codes like the real driver; MockI2cBus wrap-around UB fixed; EspI2cBus handle table mutex-guarded for PR-05 multi-task sharing; env command concurrent-state hint; error-code docs corrected (mid-init error 2, loss sequence 2-then-1); 10 new host tests (init-failure branches, chip-ID fallthrough, digP1=0 guard, nonzero H3 / negative H5-H6 vectors, clamps, Locked delegation, foreign chip-ID) — suite 119/0, rev1+rev2 green. Co-Authored-By: Claude Fable 5 --- .../include/interfaces/IEnvironmentalSensor.h | 40 ++- .../sensors/include/sensors/Bme280Sensor.h | 13 +- .../sensors/include/sensors/EspI2cBus.h | 22 +- .../sensors/testing/MockEnvironmentalSensor.h | 28 +- .../include/sensors/testing/MockI2cBus.h | 5 +- .../components/sensors/src/Bme280Sensor.cpp | 19 +- firmware/components/sensors/src/EspI2cBus.cpp | 56 +++- firmware/main/diag_console.cpp | 8 +- firmware/main/sensor_task.cpp | 10 +- firmware/test_apps/host/main/test_bme280.cpp | 297 +++++++++++++++++- specs/005-bme280-i2c/contracts/interfaces.md | 31 +- specs/005-bme280-i2c/data-model.md | 6 +- 12 files changed, 456 insertions(+), 79 deletions(-) diff --git a/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h b/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h index d9b49c7..3b7a4b0 100644 --- a/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h +++ b/firmware/components/interfaces/include/interfaces/IEnvironmentalSensor.h @@ -40,7 +40,9 @@ * 0 OK — last operation succeeded * 1 sensor not found — no device ACK on 0x76/0x77, or a responding * device failed the chip-identity check - * 2 read failed — bus/communication error during a data read, or the + * 2 read failed — bus/communication error during a data read, a + * mid-initialization bus failure after a device identified + * (calibration readout / sampling-profile write), or the * compensation produced NaN */ class IEnvironmentalSensor { @@ -53,10 +55,13 @@ class IEnvironmentalSensor { * Probes address 0x76 then 0x77, verifies the chip identity (register * 0xD0 == 0x60), reads the calibration data and writes the parity * sampling profile (ctrl_hum → ctrl_meas → config, data-model.md). - * Returns false with error 1 when no BME280 is found. Idempotent, and - * lazy-capable: read()/isAvailable() attempt initialization themselves - * when it has not happened yet — calling this first is recommended, - * not required (parity). + * Returns false with error 1 when no BME280 is found, and false with + * error 2 when a device identified but the calibration readout or + * sampling-profile write failed mid-initialization (the driver stays + * uninitialized, so the next attempt re-probes from scratch). + * Idempotent, and lazy-capable: read()/isAvailable() attempt + * initialization themselves when it has not happened yet — calling + * this first is recommended, not required (parity). */ virtual bool initialize() = 0; @@ -65,11 +70,14 @@ class IEnvironmentalSensor { * * Burst-reads the data registers 0xF7–0xFE in one bus transaction and * compensates T → P → H (t_fine ordering). Atomic: either all three - * getters are refreshed, or the call fails (error 1 on lost/absent - * sensor, error 2 on bus error or NaN) and the last-good values remain - * untouched. A bus error marks the driver uninitialized, so the next - * call re-probes both addresses (recovery, FR-004). Exactly one bus - * attempt, no retry — recovery comes from the caller's poll cadence. + * getters are refreshed, or the call fails (error 1 when the lazy + * (re-)initialization finds no sensor, error 2 on a bus error or NaN + * during the data read — a sensor unplugged mid-run reports error 2 on + * the failing read, then error 1 from the NEXT read's re-probe) and + * the last-good values remain untouched. A bus error marks the driver + * uninitialized, so the next call re-probes both addresses (recovery, + * FR-004). Exactly one bus attempt, no retry — recovery comes from the + * caller's poll cadence. * * @return true if a fully valid reading was taken. */ @@ -84,6 +92,10 @@ class IEnvironmentalSensor { * (re-)initialization, that initialize() owns its own error reporting — * the ISoilSensor convention. Recovery from earlier failures is * implicit: a sensor that identifies itself again is available again. + * + * Warning: after isAvailable() detects a loss, getLastError() may + * still be 0 (or stale) until the next read()/initialize() — consumers + * must not pair isAvailable() with getLastError(). */ virtual bool isAvailable() = 0; @@ -94,10 +106,10 @@ class IEnvironmentalSensor { */ virtual int getLastError() = 0; - // Values from the most recent successful read(). Before the FIRST - // successful read() they are meaningless placeholders, and after a - // failed read() they still hold the previous good reading — consumers - // gate on the read() result, never on the values. + // Values from the most recent successful read(). NaN until the first + // successful read() (self-announcing placeholders), and after a failed + // read() they still hold the previous good reading — consumers gate on + // the read() result, never on the values. /// Temperature in °C. virtual float getTemperature() = 0; diff --git a/firmware/components/sensors/include/sensors/Bme280Sensor.h b/firmware/components/sensors/include/sensors/Bme280Sensor.h index fb3c6fe..54bf648 100644 --- a/firmware/components/sensors/include/sensors/Bme280Sensor.h +++ b/firmware/components/sensors/include/sensors/Bme280Sensor.h @@ -29,6 +29,7 @@ #include #include +#include #include "interfaces/IEnvironmentalSensor.h" #include "interfaces/II2cBus.h" @@ -171,13 +172,19 @@ class Bme280Sensor : public IEnvironmentalSensor { II2cBus& bus_; uint8_t address_ = 0; ///< resolved device address (valid when initialized_) bool initialized_ = false; + /// WARN once per consecutive not-found run (lazy re-init retries every + /// poll); repeats are demoted to debug. Reset on successful init. + bool initFailureLogged_ = false; int lastError_ = 0; Calibration cal_{}; // Last-good reading (published only by a fully successful read()). - float temperature_ = 0.0f; - float humidity_ = 0.0f; - float pressure_ = 0.0f; + // NaN until the first successful read — self-announcing for consumers + // that forget to gate on read()/getLastError(); physically plausible + // 0.0 placeholders would silently pass for real readings. + float temperature_ = std::numeric_limits::quiet_NaN(); + float humidity_ = std::numeric_limits::quiet_NaN(); + float pressure_ = std::numeric_limits::quiet_NaN(); }; #endif /* WATERINGSYSTEM_SENSORS_BME280SENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/EspI2cBus.h b/firmware/components/sensors/include/sensors/EspI2cBus.h index f0f045c..4e04d34 100644 --- a/firmware/components/sensors/include/sensors/EspI2cBus.h +++ b/firmware/components/sensors/include/sensors/EspI2cBus.h @@ -20,9 +20,11 @@ * come from board/board.h inside the .cpp (BOARD_PIN_I2C_SDA/SCL). * * Concurrency: transaction-level safety across tasks comes from the - * i2c_master driver's per-transaction bus lock (research.md R3). Reading- - * snapshot consistency above this layer is LockedEnvironmentalSensor's - * job, not this class's. + * i2c_master driver's per-transaction bus lock (research.md R3). The + * device-handle bookkeeping (lazy bus creation + handle table) is + * internally synchronized with a private mutex, so concurrent first use + * from multiple tasks is safe. Reading-snapshot consistency above this + * layer is LockedEnvironmentalSensor's job, not this class's. */ #ifndef WATERINGSYSTEM_SENSORS_ESPI2CBUS_H @@ -30,6 +32,7 @@ #include #include +#include #include "interfaces/II2cBus.h" @@ -39,8 +42,10 @@ * The bus handle is created lazily on first use (no work in the * constructor — no error path there); per-address device handles are * created on first use at 100 kHz (FR-002) and cached. All transactions - * use finite timeouts; failures are returned as false and logged at debug - * level — error classification is the sensor driver's job. + * use finite timeouts; failures are returned as false and logged — + * transaction failures at debug level (expected NACKs) or warning level + * (timeouts/unexpected errors); one-time infrastructure failures at error + * level. Error classification is the sensor driver's job. */ class EspI2cBus : public II2cBus { public: @@ -75,11 +80,18 @@ class EspI2cBus : public II2cBus { /// Create the master bus on first use (board pins, port auto, /// internal pull-ups on). Returns false when creation fails. + /// Caller must hold mutex_. bool ensureBus(); /// Cached-or-created device handle for @p address7; nullptr on failure. + /// Takes mutex_ internally (handle-table bookkeeping). void* deviceHandle(uint8_t address7); + /// Guards busHandle_/devices_/deviceCount_ (lazy creation from + /// multiple tasks). Transactions run outside this lock — the + /// i2c_master driver's bus lock covers them. + std::mutex mutex_; + void* busHandle_ = nullptr; ///< opaque i2c_master_bus_handle_t Device devices_[kMaxDevices] = {}; size_t deviceCount_ = 0; diff --git a/firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h b/firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h index 09f2c9e..dc57545 100644 --- a/firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h +++ b/firmware/components/sensors/include/sensors/testing/MockEnvironmentalSensor.h @@ -23,6 +23,7 @@ #define WATERINGSYSTEM_SENSORS_TESTING_MOCKENVIRONMENTALSENSOR_H #include +#include #include #include "interfaces/IEnvironmentalSensor.h" @@ -35,11 +36,13 @@ * the LAST step repeats forever (a steady sensor keeps delivering, a dead * one keeps failing — matches the real driver's persistence). An entirely * unscripted read() succeeds with error 0 and whatever values are current - * (the 0.0 placeholders before any successful step — same - * meaningless-before-first-read contract as the real driver). + * (the NaN placeholders before any successful step — same + * NaN-before-first-read contract as the real driver). * initialize()/isAvailable() results are plain scripted fields - * (MockSoilSensor style); per the interface contract neither touches the - * error code. + * (MockSoilSensor style). Per the interface contract initialize() owns + * its own error reporting — a scripted failure sets error 1 ("not found", + * the mock's semantic for a false initializeResult), success sets 0 — + * while the isAvailable() probe never touches the error code. */ class MockEnvironmentalSensor : public IEnvironmentalSensor { public: @@ -77,6 +80,10 @@ class MockEnvironmentalSensor : public IEnvironmentalSensor { bool initialize() override { ++initializeCalls; + // initialize() owns its own error reporting (interface contract, + // matching the real driver): failure sets 1 ("not found" — the + // mock's semantic for a scripted failure), success sets 0. + lastError_ = initializeResult ? 0 : 1; return initializeResult; } @@ -84,8 +91,8 @@ class MockEnvironmentalSensor : public IEnvironmentalSensor { { ++readCalls; if (script_.empty()) { - // Unscripted: succeed with the current values (placeholders - // before the first scripted success). + // Unscripted: succeed with the current values (NaN + // placeholders before the first scripted success). lastError_ = 0; return true; } @@ -130,10 +137,11 @@ class MockEnvironmentalSensor : public IEnvironmentalSensor { size_t next_ = 0; int lastError_ = 0; - // Last-good values (placeholders 0.0 before the first scripted success). - float temperature_ = 0.0f; - float humidity_ = 0.0f; - float pressure_ = 0.0f; + // Last-good values (NaN placeholders before the first scripted + // success — same self-announcing contract as the real driver). + float temperature_ = std::numeric_limits::quiet_NaN(); + float humidity_ = std::numeric_limits::quiet_NaN(); + float pressure_ = std::numeric_limits::quiet_NaN(); }; #endif /* WATERINGSYSTEM_SENSORS_TESTING_MOCKENVIRONMENTALSENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/testing/MockI2cBus.h b/firmware/components/sensors/include/sensors/testing/MockI2cBus.h index ecf02de..26bcc6d 100644 --- a/firmware/components/sensors/include/sensors/testing/MockI2cBus.h +++ b/firmware/components/sensors/include/sensors/testing/MockI2cBus.h @@ -69,13 +69,14 @@ class MockI2cBus : public II2cBus { } /// Script consecutive register bytes starting at @p startReg (the device - /// is added implicitly if absent; the block must fit below 0x100). + /// is added implicitly if absent; indices wrap at 0xFF, matching + /// readRegisters()). void setRegisters(uint8_t address7, uint8_t startReg, const std::vector& values) { auto& map = devices_[address7]; for (size_t i = 0; i < values.size(); ++i) { - map[startReg + i] = values[i]; + map[static_cast(startReg + i)] = values[i]; } } diff --git a/firmware/components/sensors/src/Bme280Sensor.cpp b/firmware/components/sensors/src/Bme280Sensor.cpp index f23df9d..0ad0d8b 100644 --- a/firmware/components/sensors/src/Bme280Sensor.cpp +++ b/firmware/components/sensors/src/Bme280Sensor.cpp @@ -56,11 +56,21 @@ bool Bme280Sensor::initialize() } // No BME280 answers (or only wrong-identity devices answer): error 1, - // "sensor not found" (legacy error 1). + // "sensor not found" (legacy error 1). WARN only once per consecutive + // failure run — the lazy re-init retries every poll (5 s), and a + // permanently absent sensor must not defeat the sensor task's bounded + // log cadence with an unconditional WARN per attempt. Repeats go to + // debug; the flag resets on successful initialization below. if (!probeAndIdentify()) { lastError_ = 1; - ESP_LOGW(TAG, "initialize failed: no BME280 found (0x%02x/0x%02x)", - kPrimaryAddress, kSecondaryAddress); + if (!initFailureLogged_) { + initFailureLogged_ = true; + ESP_LOGW(TAG, "initialize failed: no BME280 found (0x%02x/0x%02x)", + kPrimaryAddress, kSecondaryAddress); + } else { + ESP_LOGD(TAG, "initialize failed: no BME280 found (0x%02x/0x%02x)", + kPrimaryAddress, kSecondaryAddress); + } return false; } @@ -86,6 +96,9 @@ bool Bme280Sensor::initialize() initialized_ = true; lastError_ = 0; + // Recovery re-arms the once-per-run not-found WARN (the INFO line + // below announces the recovery itself). + initFailureLogged_ = false; ESP_LOGI(TAG, "BME280 initialized at 0x%02x (NORMAL, T x2 / P x16 / " "H x1, IIR x16, standby 500 ms)", address_); return true; diff --git a/firmware/components/sensors/src/EspI2cBus.cpp b/firmware/components/sensors/src/EspI2cBus.cpp index 0201fa0..7b665d2 100644 --- a/firmware/components/sensors/src/EspI2cBus.cpp +++ b/firmware/components/sensors/src/EspI2cBus.cpp @@ -75,6 +75,12 @@ bool EspI2cBus::ensureBus() void* EspI2cBus::deviceHandle(uint8_t address7) { + // Guards the lazy bus creation and the handle table against concurrent + // first use from multiple tasks (sensor task + console REPL; INA226 in + // PR-05). Transactions themselves are NOT under this lock — the + // i2c_master driver's per-transaction bus lock covers those. + std::lock_guard lock(mutex_); + if (!ensureBus()) { return nullptr; } @@ -101,7 +107,9 @@ void* EspI2cBus::deviceHandle(uint8_t address7) const esp_err_t err = i2c_master_bus_add_device( static_cast(busHandle_), &dev_config, &dev); if (err != ESP_OK) { - ESP_LOGD(TAG, "i2c_master_bus_add_device(0x%02x) failed: %s", + // Infrastructure fault (never an expected runtime condition — + // same class as ensureBus()/table-full above). + ESP_LOGE(TAG, "i2c_master_bus_add_device(0x%02x) failed: %s", address7, esp_err_to_name(err)); return nullptr; } @@ -111,18 +119,30 @@ void* EspI2cBus::deviceHandle(uint8_t address7) bool EspI2cBus::probe(uint8_t address7) { - if (!ensureBus()) { - return false; + { + // ensureBus() mutates busHandle_ — same lock as deviceHandle(). + // Once created the handle never changes, so the transaction below + // can safely run outside the lock (driver bus lock covers it). + std::lock_guard lock(mutex_); + if (!ensureBus()) { + return false; + } } const esp_err_t err = i2c_master_probe( static_cast(busHandle_), address7, kTimeoutMs); - if (err != ESP_OK) { - // Expected for absent devices — debug level (classification is the - // sensor driver's job, contracts/interfaces.md). + if (err == ESP_ERR_NOT_FOUND) { + // NACK — expected for absent devices, debug level (classification + // is the sensor driver's job, contracts/interfaces.md). ESP_LOGD(TAG, "probe(0x%02x): %s", address7, esp_err_to_name(err)); return false; } + if (err != ESP_OK) { + // Timeout/bus fault — a wedged bus must be distinguishable from a + // device-absent NACK in the field logs. + ESP_LOGW(TAG, "probe(0x%02x): %s", address7, esp_err_to_name(err)); + return false; + } return true; } @@ -140,8 +160,17 @@ bool EspI2cBus::readRegisters(uint8_t address7, uint8_t startReg, const esp_err_t err = i2c_master_transmit_receive(dev, &startReg, 1, buf, len, kTimeoutMs); if (err != ESP_OK) { - ESP_LOGD(TAG, "readRegisters(0x%02x, 0x%02x, %u): %s", address7, - startReg, static_cast(len), esp_err_to_name(err)); + // NACK (device unplugged) stays at debug; a timeout/unexpected + // error means a wedged bus and must be visible at default level. + if (err == ESP_ERR_NOT_FOUND) { + ESP_LOGD(TAG, "readRegisters(0x%02x, 0x%02x, %u): %s", address7, + startReg, static_cast(len), + esp_err_to_name(err)); + } else { + ESP_LOGW(TAG, "readRegisters(0x%02x, 0x%02x, %u): %s", address7, + startReg, static_cast(len), + esp_err_to_name(err)); + } return false; } return true; @@ -158,8 +187,15 @@ bool EspI2cBus::writeRegister(uint8_t address7, uint8_t reg, uint8_t value) const esp_err_t err = i2c_master_transmit(dev, payload, sizeof(payload), kTimeoutMs); if (err != ESP_OK) { - ESP_LOGD(TAG, "writeRegister(0x%02x, 0x%02x, 0x%02x): %s", address7, - reg, value, esp_err_to_name(err)); + // Same classification as readRegisters(): expected NACK at debug, + // timeout/unexpected errors at warning. + if (err == ESP_ERR_NOT_FOUND) { + ESP_LOGD(TAG, "writeRegister(0x%02x, 0x%02x, 0x%02x): %s", + address7, reg, value, esp_err_to_name(err)); + } else { + ESP_LOGW(TAG, "writeRegister(0x%02x, 0x%02x, 0x%02x): %s", + address7, reg, value, esp_err_to_name(err)); + } return false; } return true; diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index e7b3f5c..24f566d 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -627,10 +627,16 @@ int env_cmd(int argc, char **argv) return 1; } if (!s_env->read()) { + // read() and getLastError() are separate locked calls; a sensor- + // task poll interleaving between them can succeed and reset the + // error to 0 (benign cross-lock race, TODO(PR-11) snapshot helper + // in LockedEnvironmentalSensor.h) — hint accordingly. const int error = s_env->getLastError(); const char *hint = (error == 1) ? "sensor not found" : (error == 2) ? "read failed" - : "unknown error"; + : (error == 0) + ? "state changed concurrently - retry" + : "unknown error"; printf("ERROR %d (%s)\n", error, hint); return 1; } diff --git a/firmware/main/sensor_task.cpp b/firmware/main/sensor_task.cpp index 62f6ed2..08b658c 100644 --- a/firmware/main/sensor_task.cpp +++ b/firmware/main/sensor_task.cpp @@ -62,10 +62,14 @@ constexpr UBaseType_t kPriority = 1; ///< parity priority (R7) static_cast(sensor.getPressure())); break; case SensorTaskLogPolicy::Event::FailureTransition: - // Valid → invalid transition: WARN exactly once. + // Valid → invalid transition: WARN exactly once. Note: + // read() and getLastError() are separate locked calls, so a + // console `env` read interleaving between them can change the + // error code — benign here (log-only); the consistent-snapshot + // helper is TODO(PR-11) (LockedEnvironmentalSensor.h). ESP_LOGW(TAG, - "environmental reading invalid (error %d) — " - "keeping last-good values, retrying every %lu ms", + "environmental reading invalid (error %d), " + "retrying every %lu ms", sensor.getLastError(), static_cast(kPeriodMs)); break; diff --git a/firmware/test_apps/host/main/test_bme280.cpp b/firmware/test_apps/host/main/test_bme280.cpp index fd4db91..4bb0e4d 100644 --- a/firmware/test_apps/host/main/test_bme280.cpp +++ b/firmware/test_apps/host/main/test_bme280.cpp @@ -16,7 +16,10 @@ * re-read, sensorless boot), T015 (sensor-task log policy), T017 (US3 * address variants and chip-ID rejection), T018 (Bosch reference vectors, * integer outputs asserted exactly) and T020 (MockEnvironmentalSensor - * consistency). + * consistency); review-fix hardening adds the mid-initialization error-2 + * paths, the continue-to-next-candidate probe path, extra Bosch vectors + * (dig_H3, negative H calibration, clamps, dig_P1=0 guard) and the + * LockedEnvironmentalSensor delegation check. */ #include @@ -25,6 +28,7 @@ #include "unity.h" #include "sensors/Bme280Sensor.h" +#include "sensors/LockedEnvironmentalSensor.h" #include "sensors/SensorTaskLogPolicy.h" #include "sensors/testing/MockEnvironmentalSensor.h" #include "sensors/testing/MockI2cBus.h" @@ -146,6 +150,39 @@ const std::vector kDataBlockExtreme = { 0x00, 0x00, 0x00, 0xFF, 0xFF, 0xF0, 0xFF, 0xFF, }; +// --------------------------------------------------------------------------- +// Negative-humidity-calibration vector (review fix C2): calib26–32 bytes +// with the sign bits set in 0xE4/0xE6/0xE7, pinning readCalibration's +// int8_t sign-extension casts. Parses to H2=362, H3=0, +// H4 = int8(0xFF)*16 | (0x25 & 0xF) = -16 | 5 = -11, +// H5 = int8(0xFF)*16 | (0x25 >> 4) = -16 | 2 = -14, +// H6 = int8(0xE2) = -30. +// adc_H = 11264 (0x2C00) is chosen so the result stays inside the Bosch +// 0–100 %RH clamps. Expected output derived by running the Bosch int32 +// reference routine over (H1=75, H2=362, H3=0, H4=-11, H5=-14, H6=-30, +// adc_H=11264, t_fine=128422): +// v = 128422 - 76800 = 51622 +// a = ((11264<<14) - (-11<<20) - (-14*51622) + 16384) >> 15 +// = (184549376 + 11534336 + 722708 + 16384) >> 15 = 6006 +// b = ((((((51622*(-30))>>10) * 32768) >> 10) + 2097152)*362 + 8192) >> 14 +// = 45266 +// v = 6006*45266 = 271867596; minus the H1 correction 2520393 +// = 269347203; >> 12 = 65758 → 64.216797 %RH +// A sign break in H5 or H6 changes the result grossly (H5 unsigned drives +// it to the 0 clamp, H6 unsigned past the 100 %RH clamp). H4's own sign +// extension is inherently unobservable through the int32 path — the <<20 +// keeps only the 12 value bits — but its parse runs through the same cast +// path end-to-end here. T/P calibration is kCalibBlock1, so temperature +// and pressure keep their reference expectations. +// --------------------------------------------------------------------------- +const std::vector kCalibBlock2Negative = { + 0x6A, 0x01, 0x00, 0xFF, 0x25, 0xFF, 0xE2, +}; +const std::vector kDataBlockSmallHumidity = { + 0x65, 0x5A, 0xC0, 0x7E, 0xED, 0x00, 0x2C, 0x00, +}; +constexpr float kExpectedHumidityNegativeCal = 64.216797f; // 65758 ÷ 1024 + /// Script a complete, healthy BME280 at @p address: chip-ID, both /// calibration blocks and one measurement in the data registers. void scriptBme280(MockI2cBus& mock, uint8_t address) @@ -282,8 +319,10 @@ static void test_read_is_one_eight_byte_burst(void) // -------------------------------------------------------------------------- // Before the FIRST successful read() the getters return the documented -// meaningless placeholders (0.0) — consumers gate on read(), never on -// value plausibility (interface contract) +// quiet-NaN placeholders — self-announcing for consumers that forget to +// gate on read() (a plausible 0.0 would silently pass for a real reading); +// consumers gate on read(), never on value plausibility (interface +// contract) // -------------------------------------------------------------------------- static void test_getters_before_first_read_are_placeholders(void) { @@ -293,9 +332,9 @@ static void test_getters_before_first_read_are_placeholders(void) TEST_ASSERT_TRUE(sensor.initialize()); - TEST_ASSERT_EQUAL_FLOAT(0.0f, sensor.getTemperature()); - TEST_ASSERT_EQUAL_FLOAT(0.0f, sensor.getHumidity()); - TEST_ASSERT_EQUAL_FLOAT(0.0f, sensor.getPressure()); + TEST_ASSERT_TRUE(std::isnan(sensor.getTemperature())); + TEST_ASSERT_TRUE(std::isnan(sensor.getHumidity())); + TEST_ASSERT_TRUE(std::isnan(sensor.getPressure())); } // -------------------------------------------------------------------------- @@ -343,6 +382,60 @@ static void test_initialize_absent_sensor_fails_error1(void) TEST_ASSERT_EQUAL(1, probedSecondary); } +// -------------------------------------------------------------------------- +// Mid-initialization bus error AFTER a device identified (review fix A5): +// the calibration readout fails → initialize() false, error 2 (a +// communication failure, not "not found"), the driver stays uninitialized +// and the next read() re-probes from scratch and recovers +// -------------------------------------------------------------------------- +static void test_initialize_calibration_read_error_sets_error2_then_recovers(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrSecondary); + + // Read outcomes are FIFO per readRegisters() call on a present device: + // the chip-ID read succeeds, the calib00–25 burst then fails. + mock.queueReadOutcome(true); // chip-ID at 0x77 + mock.queueReadOutcome(false); // calibration burst → bus error + + TEST_ASSERT_FALSE(sensor.initialize()); + TEST_ASSERT_EQUAL_INT(2, sensor.getLastError()); + + // Uninitialized after the failure: the next read() re-runs the full + // init (outcome queue exhausted → bus healthy) and delivers a reading. + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + sensor.getTemperature()); +} + +// -------------------------------------------------------------------------- +// Mid-initialization bus error on the sampling-profile write (review fix +// A5): initialize() false, error 2, and the driver stays uninitialized — +// the next initialize() is a full re-probe, not an idempotent no-op +// -------------------------------------------------------------------------- +static void test_initialize_profile_write_error_sets_error2_stays_uninitialized(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrSecondary); + + mock.queueWriteOutcome(false); // ctrl_hum write → bus error + + TEST_ASSERT_FALSE(sensor.initialize()); + TEST_ASSERT_EQUAL_INT(2, sensor.getLastError()); + + // Not initialized: the retry starts with a probe (re-probe from + // scratch) and succeeds once the bus behaves. + mock.calls.clear(); + TEST_ASSERT_TRUE(sensor.initialize()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_FALSE(mock.calls.empty()); + TEST_ASSERT_EQUAL(static_cast(MockI2cBus::Call::Type::Probe), + static_cast(mock.calls.front().type)); +} + // -------------------------------------------------------------------------- // Mid-read bus error: read() false, error 2, and the last-good values stay // untouched (validity contract — consumers gate on read(), values persist) @@ -479,6 +572,21 @@ static void test_is_available_false_on_loss_leaves_error_untouched(void) TEST_ASSERT_TRUE(f.sensor.isAvailable()); } +// -------------------------------------------------------------------------- +// isAvailable() after init with a FOREIGN chip identity (review fix C3): +// the device still ACKs and the chip-ID read succeeds, but the identity is +// a BMP280's (0x58) — unavailable, error code untouched (the probe never +// touches it; only the NEXT read()/initialize() reports) +// -------------------------------------------------------------------------- +static void test_is_available_false_on_foreign_chip_id_after_init(void) +{ + Fixture f; // initialized at 0x77, lastError == 0 + + f.mock.setRegister(kAddrSecondary, kRegChipId, 0x58); // BMP280 now + TEST_ASSERT_FALSE(f.sensor.isAvailable()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); +} + // -------------------------------------------------------------------------- // NaN guard (US2 scenario 4, FR-007): with the Bosch INTEGER compensation // paths a NaN is unreachable from any raw register content — the guard is @@ -562,6 +670,29 @@ static void test_wrong_chip_id_at_primary_selects_secondary(void) } } +// -------------------------------------------------------------------------- +// Chip-ID read ERROR at 0x76 (device ACKs the probe but the identity read +// fails) with a healthy BME280 at 0x77: the probing loop continues to the +// next candidate instead of giving up (review fix B6) +// -------------------------------------------------------------------------- +static void test_chip_id_read_error_at_primary_continues_to_secondary(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + mock.addDevice(kAddrPrimary); // ACKs, chip-ID read will error + scriptBme280(mock, kAddrSecondary); // healthy module at 0x77 + mock.queueReadOutcome(false); // first read = chip-ID at 0x76 + + TEST_ASSERT_TRUE(sensor.initialize()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + + // The candidate at 0x76 was skipped; the reading comes from 0x77. + TEST_ASSERT_TRUE(sensor.read()); + const MockI2cBus::Call& last = mock.calls.back(); + TEST_ASSERT_EQUAL_UINT8(kRegData, last.reg); + TEST_ASSERT_EQUAL_UINT8(kAddrSecondary, last.address); +} + // -------------------------------------------------------------------------- // Foreign devices ACKing on BOTH addresses (address collision): every // candidate fails the identity check → error 1, sensor unavailable rather @@ -694,6 +825,56 @@ static void test_compensation_extreme_legal_raws_vector(void) TEST_ASSERT_EQUAL_FLOAT(100.0f, static_cast(h) / 1024.0f); } +// -------------------------------------------------------------------------- +// dig_P1 = 0 division-by-zero guard (review fix B7): the reference +// algorithm's divisor var1 becomes ((1<<47) + var1) * 0 >> 33 = 0, and the +// datasheet behavior is to return 0 instead of dividing +// -------------------------------------------------------------------------- +static void test_compensate_pressure_zero_digp1_returns_zero(void) +{ + Bme280Sensor::Calibration cal = kRefCal; + cal.digP1 = 0; + + TEST_ASSERT_EQUAL_UINT32( + 0u, Bme280Sensor::compensatePressure(415148, cal, kRefTFine)); +} + +// -------------------------------------------------------------------------- +// dig_H3 ≠ 0 humidity vector (review fix C1) — every other set has H3=0, +// leaving the ((v*H3)>>11) term invisible. kRefCal with dig_H3=50, +// adc_H=32768, t_fine=128422; expected output derived by hand-running the +// Bosch int32 reference routine: +// v = 128422 - 76800 = 51622 +// a = ((32768<<14) - (315<<20) - 50*51622 + 16384) >> 15 = 6225 +// inner = ((51622*50)>>11) + 32768 = 1260 + 32768 = 34028 (the H3 term; +// 32768 exactly when H3 = 0) +// b = ((((((51622*30)>>10)*34028)>>10) + 2097152)*362 + 8192) >> 14 +// = 47446 +// v = 6225*47446 - 2974879 (H1 correction) = 292376471 +// H = 292376471 >> 12 = 71380 → 69.707 %RH (71319 with H3 = 0) +// -------------------------------------------------------------------------- +static void test_compensation_humidity_nonzero_digh3_vector(void) +{ + Bme280Sensor::Calibration cal = kRefCal; + cal.digH3 = 50; + + TEST_ASSERT_EQUAL_UINT32( + 71380u, Bme280Sensor::compensateHumidity(32768, cal, kRefTFine)); +} + +// -------------------------------------------------------------------------- +// Humidity lower clamp (review fix C4): adc_H = 0 over kRefCal drives the +// intermediate negative (Bosch reference over kRefCal at t_fine=128422: +// a = (0 - 315<<20 - 50*51622 + 16384)>>15 = -10159, b = 47405, product +// minus H1 correction ≈ -4.9e8 < 0), so the reference algorithm's own +// lower clamp must yield exactly 0 +// -------------------------------------------------------------------------- +static void test_compensation_humidity_lower_clamp_at_zero_adch(void) +{ + TEST_ASSERT_EQUAL_UINT32( + 0u, Bme280Sensor::compensateHumidity(0, kRefCal, kRefTFine)); +} + // -------------------------------------------------------------------------- // End-to-end read of the negative-temperature raw block: register // assembly + compensation + float conversion through the full read() path @@ -709,6 +890,30 @@ static void test_read_negative_temperature_block(void) TEST_ASSERT_FLOAT_WITHIN(0.001f, 953.71309f, f.sensor.getPressure()); } +// -------------------------------------------------------------------------- +// End-to-end read with NEGATIVE dig_H4/H5/H6 (review fix C2): calibration +// parsing (int8_t sign-extension of the split-nibble/H6 top bytes) + +// compensation through the full read() path — derivation at +// kCalibBlock2Negative above. T/P calibration is unchanged, so they keep +// the reference expectations +// -------------------------------------------------------------------------- +static void test_read_negative_h4_h5_h6_calibration(void) +{ + MockI2cBus mock; + Bme280Sensor sensor(mock); + scriptBme280(mock, kAddrSecondary); + mock.setRegisters(kAddrSecondary, 0xE1, kCalibBlock2Negative); + mock.setRegisters(kAddrSecondary, kRegData, kDataBlockSmallHumidity); + + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedHumidityNegativeCal, + sensor.getHumidity()); + TEST_ASSERT_FLOAT_WITHIN(0.005f, kExpectedTemperature, + sensor.getTemperature()); + TEST_ASSERT_FLOAT_WITHIN(0.001f, kExpectedPressure, + sensor.getPressure()); +} + // ========================================================================== // Sensor-task log policy (T015, analyze finding U1): the WARN/INFO/silence // decisions of main/sensor_task.cpp, host-tested deterministically @@ -864,7 +1069,7 @@ static void test_mock_env_exhausted_script_repeats_last_step(void) // -------------------------------------------------------------------------- // Helpers keep values/validity/error coherent: a failed step carries its // error and leaves values alone; a successful step always resets error 0. -// Unscripted reads succeed with the placeholder values (0.0), matching the +// Unscripted reads succeed with the NaN placeholder values, matching the // real driver's before-first-read contract // -------------------------------------------------------------------------- static void test_mock_env_helpers_keep_state_coherent(void) @@ -872,27 +1077,79 @@ static void test_mock_env_helpers_keep_state_coherent(void) MockEnvironmentalSensor unscripted; TEST_ASSERT_TRUE(unscripted.read()); TEST_ASSERT_EQUAL_INT(0, unscripted.getLastError()); - TEST_ASSERT_EQUAL_FLOAT(0.0f, unscripted.getTemperature()); + TEST_ASSERT_TRUE(std::isnan(unscripted.getTemperature())); MockEnvironmentalSensor mock; mock.scriptFailedRead(1); // e.g. sensorless boot mock.scriptSuccessfulRead(18.0f, 55.0f, 990.0f); TEST_ASSERT_FALSE(mock.read()); TEST_ASSERT_EQUAL_INT(1, mock.getLastError()); - TEST_ASSERT_EQUAL_FLOAT(0.0f, mock.getTemperature()); // still placeholder + TEST_ASSERT_TRUE(std::isnan(mock.getTemperature())); // still placeholder TEST_ASSERT_TRUE(mock.read()); TEST_ASSERT_EQUAL_INT(0, mock.getLastError()); TEST_ASSERT_EQUAL_FLOAT(18.0f, mock.getTemperature()); +} + +// -------------------------------------------------------------------------- +// initialize()/isAvailable() error-code discipline (review fix A2, matching +// the real driver and the interface contract): initialize() owns its own +// error reporting — a scripted failure sets error 1 ("not found", the +// mock's semantic for a false initializeResult), success resets 0 — while +// the isAvailable() probe never touches the error code in either direction +// -------------------------------------------------------------------------- +static void test_mock_env_initialize_reports_error_available_is_neutral(void) +{ + MockEnvironmentalSensor mock; - // initialize()/isAvailable() are scripted fields with call counters and - // never touch the error code (interface convention). mock.initializeResult = false; - mock.isAvailableResult = false; TEST_ASSERT_FALSE(mock.initialize()); + TEST_ASSERT_EQUAL_INT(1, mock.getLastError()); + + mock.isAvailableResult = false; TEST_ASSERT_FALSE(mock.isAvailable()); + TEST_ASSERT_EQUAL_INT(1, mock.getLastError()); // untouched by the probe + + mock.initializeResult = true; + TEST_ASSERT_TRUE(mock.initialize()); TEST_ASSERT_EQUAL_INT(0, mock.getLastError()); - TEST_ASSERT_EQUAL(1, mock.initializeCalls); - TEST_ASSERT_EQUAL(1, mock.isAvailableCalls); + + mock.isAvailableResult = true; + TEST_ASSERT_TRUE(mock.isAvailable()); + TEST_ASSERT_EQUAL_INT(0, mock.getLastError()); // untouched by the probe + + TEST_ASSERT_EQUAL(2, mock.initializeCalls); + TEST_ASSERT_EQUAL(2, mock.isAvailableCalls); +} + +// -------------------------------------------------------------------------- +// LockedEnvironmentalSensor delegates the full interface unchanged (review +// fix B9; LockedWaterPump precedent): every method forwards to the wrapped +// sensor, with distinct per-getter values so a transposed forward fails +// -------------------------------------------------------------------------- +static void test_locked_env_wrapper_delegates_all_methods(void) +{ + MockEnvironmentalSensor inner; + LockedEnvironmentalSensor sensor(inner); + inner.scriptSuccessfulRead(21.5f, 40.0f, 1013.2f); + inner.scriptFailedRead(2); + + TEST_ASSERT_TRUE(sensor.initialize()); + TEST_ASSERT_TRUE(sensor.isAvailable()); + + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(21.5f, sensor.getTemperature()); + TEST_ASSERT_EQUAL_FLOAT(40.0f, sensor.getHumidity()); + TEST_ASSERT_EQUAL_FLOAT(1013.2f, sensor.getPressure()); + + TEST_ASSERT_FALSE(sensor.read()); + TEST_ASSERT_EQUAL_INT(2, sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(21.5f, sensor.getTemperature()); // last-good + + // Every call reached the wrapped sensor exactly once per invocation. + TEST_ASSERT_EQUAL(1, inner.initializeCalls); + TEST_ASSERT_EQUAL(1, inner.isAvailableCalls); + TEST_ASSERT_EQUAL(2, inner.readCalls); } void run_bme280_tests(void) @@ -907,30 +1164,40 @@ void run_bme280_tests(void) RUN_TEST(test_read_initializes_lazily); // US2 (T014) RUN_TEST(test_initialize_absent_sensor_fails_error1); + RUN_TEST(test_initialize_calibration_read_error_sets_error2_then_recovers); + RUN_TEST(test_initialize_profile_write_error_sets_error2_stays_uninitialized); RUN_TEST(test_read_bus_error_keeps_last_good_and_sets_error2); RUN_TEST(test_read_recovers_after_bus_error_with_reprobe); RUN_TEST(test_recovery_rereads_calibration_of_replaced_module); RUN_TEST(test_boot_sensorless_then_attach_recovers); RUN_TEST(test_is_available_reads_chip_id_and_leaves_error_untouched); RUN_TEST(test_is_available_false_on_loss_leaves_error_untouched); + RUN_TEST(test_is_available_false_on_foreign_chip_id_after_init); RUN_TEST(test_read_extreme_raws_never_produce_nan); // US3 (T017) RUN_TEST(test_device_at_primary_delivers_reading); RUN_TEST(test_device_at_secondary_delivers_reading); RUN_TEST(test_wrong_chip_id_at_primary_selects_secondary); + RUN_TEST(test_chip_id_read_error_at_primary_continues_to_secondary); RUN_TEST(test_wrong_chip_id_everywhere_fails_error1); RUN_TEST(test_loss_then_reappearance_at_other_address_recovers); // US4 (T018) RUN_TEST(test_compensation_worked_example_integer_outputs); RUN_TEST(test_compensation_negative_temperature_vector); RUN_TEST(test_compensation_extreme_legal_raws_vector); + RUN_TEST(test_compensate_pressure_zero_digp1_returns_zero); + RUN_TEST(test_compensation_humidity_nonzero_digh3_vector); + RUN_TEST(test_compensation_humidity_lower_clamp_at_zero_adch); RUN_TEST(test_read_negative_temperature_block); + RUN_TEST(test_read_negative_h4_h5_h6_calibration); // Sensor-task log policy (T015) RUN_TEST(test_log_policy_warns_once_then_bounded_repeats); RUN_TEST(test_log_policy_warns_on_recovery_then_reads); RUN_TEST(test_log_policy_long_outage_stays_bounded_and_never_stops); - // MockEnvironmentalSensor (T020) + // MockEnvironmentalSensor (T020) + LockedEnvironmentalSensor RUN_TEST(test_mock_env_scripted_sequence_observed_exactly); RUN_TEST(test_mock_env_exhausted_script_repeats_last_step); RUN_TEST(test_mock_env_helpers_keep_state_coherent); + RUN_TEST(test_mock_env_initialize_reports_error_available_is_neutral); + RUN_TEST(test_locked_env_wrapper_delegates_all_methods); } diff --git a/specs/005-bme280-i2c/contracts/interfaces.md b/specs/005-bme280-i2c/contracts/interfaces.md index f02e501..8f331a9 100644 --- a/specs/005-bme280-i2c/contracts/interfaces.md +++ b/specs/005-bme280-i2c/contracts/interfaces.md @@ -26,18 +26,25 @@ Contract: - **`initialize()`**: probes 0x76 then 0x77, verifies chip identity (0x60), reads calibration, writes the parity sampling profile (ctrl_hum → ctrl_meas → config, - data-model.md). Returns false with error 1 if no BME280 is found. Idempotent; - callers need not call it first — `read()`/`isAvailable()` initialize lazily - (parity). + data-model.md). Returns false with error 1 if no BME280 is found, and false with + error 2 on a mid-initialization bus failure after a device identified + (calibration readout / sampling-profile write) — the driver stays uninitialized + and the next attempt re-probes from scratch. Idempotent; callers need not call + it first — `read()`/`isAvailable()` initialize lazily (parity). - **`read()`**: burst-reads 0xF7–0xFE in one transaction, compensates T → P → H (t_fine ordering), converts to °C/%RH/hPa. Atomic: either all three getters are - refreshed, or the call fails (error 1 on lost/absent sensor, error 2 on bus - error or NaN) and the last-good values remain untouched. **Consumers gate on the + refreshed, or the call fails (error 1 when the lazy (re-)initialization finds no + sensor, error 2 on a bus error or NaN during the data read — an unplug mid-run + reports error 2 on the failing read, then error 1 from the next read's re-probe) + and the last-good values remain untouched. **Consumers gate on the read result, never on value plausibility.** Before the first successful read the - getters return meaningless placeholders. A bus error marks the driver - uninitialized → the next call re-probes both addresses (recovery, spec FR-004). -- **`isAvailable()`**: real chip-ID read every call — never cached state. Does not - modify `getLastError()`. + getters return quiet-NaN placeholders (self-announcing). A bus error marks the + driver uninitialized → the next call re-probes both addresses (recovery, spec + FR-004). +- **`isAvailable()`**: real chip-ID read every call — never cached state. The + probe itself never modifies `getLastError()`; a lazy (re-)initialization + triggered by `isAvailable()` owns its own error reporting (reports through + `initialize()`). - Exactly one bus attempt per operation; no automatic retry (recovery comes from the caller's poll cadence — same philosophy as the soil sensor). @@ -87,8 +94,10 @@ Implements `II2cBus` over `driver/i2c_master.h` (new API — never the legacy **100 kHz** standard mode per device (spec FR-002). - `probe()` maps to `i2c_master_probe`; `readRegisters` to `i2c_master_transmit_receive`; `writeRegister` to `i2c_master_transmit`. - Finite timeouts (no infinite waits); errors logged at debug level and returned - as false — classification is `Bme280Sensor`'s job. + Finite timeouts (no infinite waits); failures returned as false — transaction + failures logged at debug level (expected NACKs) or warning level + (timeouts/unexpected errors), one-time infrastructure failures at error level — + classification is `Bme280Sensor`'s job. - **Bus sharing (spec FR-003)**: the one `EspI2cBus` instance is constructed in `app_main` (function-local static, established pattern) and passed to every I2C driver — PR-05's INA226 receives the same instance. No second bus creation diff --git a/specs/005-bme280-i2c/data-model.md b/specs/005-bme280-i2c/data-model.md index 40d766b..2891b17 100644 --- a/specs/005-bme280-i2c/data-model.md +++ b/specs/005-bme280-i2c/data-model.md @@ -23,9 +23,11 @@ consumers must distinguish (SC-006: console separates "absent" from "read failed |------|---------|---------------|--------| | 0 | OK | last operation succeeded | legacy 0 | | 1 | Sensor not found | probe of 0x76 and 0x77 finds no device ACK, or a responding device fails the chip-identity check (logged distinctly) | legacy 1 ("sensor not found") | -| 2 | Read failed | bus/communication error during a data read, or compensation yields NaN | legacy 2 ("read failed / NaN") | +| 2 | Read failed | bus/communication error during a data read, a mid-initialization bus failure after a device identified (calibration readout / sampling-profile write), or compensation yields NaN | legacy 2 ("read failed / NaN") | -`isAvailable()` never modifies the error code (soil-sensor convention). +The `isAvailable()` probe itself never modifies the error code; a lazy +(re-)initialization triggered by `isAvailable()` owns its own error reporting +(reports through `initialize()` — soil-sensor convention). ## BME280 register map (used subset) From f85abd14dfbb923a040b002e474f8e4178c24d4f Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 22:19:31 +0200 Subject: [PATCH 5/5] fix(sensors): correct v6 transaction NACK code in I2C log classification Re-review finding: i2c_master_transmit(_receive) return ESP_ERR_INVALID_RESPONSE on a device NACK in IDF v6 (ESP_ERR_NOT_FOUND is probe-only), so the expected-NACK debug branch in readRegisters/ writeRegister was dead code and unplug NACKs logged at WARN. Classify ESP_ERR_INVALID_RESPONSE as expected-NACK; add HIL item C6 pinning the no-WARN-flood behavior on hardware (target-only path, not host-testable). Co-Authored-By: Claude Fable 5 --- firmware/components/sensors/src/EspI2cBus.cpp | 12 +++++++++--- specs/005-bme280-i2c/checklists/hil.md | 5 +++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/firmware/components/sensors/src/EspI2cBus.cpp b/firmware/components/sensors/src/EspI2cBus.cpp index 7b665d2..da4c685 100644 --- a/firmware/components/sensors/src/EspI2cBus.cpp +++ b/firmware/components/sensors/src/EspI2cBus.cpp @@ -162,7 +162,12 @@ bool EspI2cBus::readRegisters(uint8_t address7, uint8_t startReg, if (err != ESP_OK) { // NACK (device unplugged) stays at debug; a timeout/unexpected // error means a wedged bus and must be visible at default level. - if (err == ESP_ERR_NOT_FOUND) { + // On IDF v6 a device NACK during a transaction surfaces as + // ESP_ERR_INVALID_RESPONSE (i2c_master.h: "I2C master transmit + // receives NACK") — NOT ESP_ERR_NOT_FOUND, which only + // i2c_master_probe returns; the latter is kept in the debug set + // as harmless future-proofing. + if (err == ESP_ERR_INVALID_RESPONSE || err == ESP_ERR_NOT_FOUND) { ESP_LOGD(TAG, "readRegisters(0x%02x, 0x%02x, %u): %s", address7, startReg, static_cast(len), esp_err_to_name(err)); @@ -187,9 +192,10 @@ bool EspI2cBus::writeRegister(uint8_t address7, uint8_t reg, uint8_t value) const esp_err_t err = i2c_master_transmit(dev, payload, sizeof(payload), kTimeoutMs); if (err != ESP_OK) { - // Same classification as readRegisters(): expected NACK at debug, + // Same classification as readRegisters(): expected NACK + // (ESP_ERR_INVALID_RESPONSE on IDF v6 transactions) at debug, // timeout/unexpected errors at warning. - if (err == ESP_ERR_NOT_FOUND) { + if (err == ESP_ERR_INVALID_RESPONSE || err == ESP_ERR_NOT_FOUND) { ESP_LOGD(TAG, "writeRegister(0x%02x, 0x%02x, 0x%02x): %s", address7, reg, value, esp_err_to_name(err)); } else { diff --git a/specs/005-bme280-i2c/checklists/hil.md b/specs/005-bme280-i2c/checklists/hil.md index ac283df..dc0c90e 100644 --- a/specs/005-bme280-i2c/checklists/hil.md +++ b/specs/005-bme280-i2c/checklists/hil.md @@ -50,6 +50,11 @@ - [ ] C5. Boot WITHOUT the sensor → normal startup (console up, pumps OFF), one invalid-reading WARN, no boot block; attach the sensor → readings start on a subsequent poll (lazy re-init) +- [ ] C6. During C1/C2 (unplugged): the WARNs come from the driver/task + only (`bme280`, `sensor_task` tags) — no per-poll flood of + `esp_i2c_bus` WARNs (unplug-NACKs classify at debug level; only + timeouts/unexpected bus errors WARN — not host-testable, the bench + pins this classification) ## D. Address variant 0x76 (US3, SC-005)