From a6c7989bf1c72c736abc3c29a1bb0d1b7ba3c6ac Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 10:29:44 +0200 Subject: [PATCH 01/11] docs(spec): PR-04 modbus soil sensor specification Spec for the esp-modbus driver replacing the legacy SP3485 client: 9-register parity read map, fail-safe validity semantics, dual-board RS485 direction handling, and rev2 hardware requirements FW-2 (RX pull-up against floating RO) and FW-4 (TX echo discard) from the 2026-07-02 design review. One open clarification (calibration scope) pending at Checkpoint 1. Co-Authored-By: Claude Fable 5 --- .specify/feature.json | 2 +- .../checklists/requirements.md | 43 +++ specs/004-modbus-soil-sensor/spec.md | 289 ++++++++++++++++++ 3 files changed, 333 insertions(+), 1 deletion(-) create mode 100644 specs/004-modbus-soil-sensor/checklists/requirements.md create mode 100644 specs/004-modbus-soil-sensor/spec.md diff --git a/.specify/feature.json b/.specify/feature.json index 229797a..9101c8f 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1,3 @@ { - "feature_directory": "specs/003-nvs-littlefs-storage" + "feature_directory": "specs/004-modbus-soil-sensor" } diff --git a/specs/004-modbus-soil-sensor/checklists/requirements.md b/specs/004-modbus-soil-sensor/checklists/requirements.md new file mode 100644 index 0000000..297b929 --- /dev/null +++ b/specs/004-modbus-soil-sensor/checklists/requirements.md @@ -0,0 +1,43 @@ +# Specification Quality Checklist: Modbus Soil Sensor over RS485 + +**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 + +- "Implementation details" caveat (same stance as specs 002/003): register addresses, + scaling factors, timeout values, GPIO numbers and the `BOARD_HAS_RS485_DE` flag are + **parity contract facts** (`docs/parity-checklist.md` §5) and hardware facts from the + rev2 design review — they define WHAT the system must do, not HOW. References to + esp-modbus/interface names appear only in the Input quote and in Assumptions + (explicitly non-binding "who implements it" note). +- One scope judgment call documented under Assumptions instead of a + [NEEDS CLARIFICATION] marker: calibration commands are included by parity-checklist + coverage even though the mini-PRD scope list omits them. Flag for confirmation at + Checkpoint 2. diff --git a/specs/004-modbus-soil-sensor/spec.md b/specs/004-modbus-soil-sensor/spec.md new file mode 100644 index 0000000..421ea6e --- /dev/null +++ b/specs/004-modbus-soil-sensor/spec.md @@ -0,0 +1,289 @@ +# Feature Specification: Modbus Soil Sensor over RS485 + +**Feature Branch**: `004-modbus-soil-sensor` + +**Created**: 2026-07-02 + +**Status**: Draft + +**Input**: User description: "Replace the hand-rolled SP3485 Modbus client with esp-modbus +(pinned 2.1.2) behind IModbusClient/ISoilSensor interfaces, supporting both rev1 (manual DE +via RTS on GPIO 25) and rev2 (THVD1426 auto-direction, no DE pin) RS485 hardware, per +docs/prd/PR-04-modbus-soil-sensor.md (authoritative mini-PRD; ground truth for behavior is +docs/parity-checklist.md §5) plus hardware-driven requirements FW-2 and FW-4 from the rev2 +design review 2026-07-02." + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Soil readings on the bench rig (Priority: P1) + +Paul flashes the rev1 rig, which has the real RS485 soil sensor wired to it. From the +serial console he triggers a soil reading and gets all seven used values (moisture, +temperature, EC, pH, N, P, K) with the same numbers the production Arduino unit shows +for the same sensor — including correct negative temperatures. A bus-level diagnostic +command shows raw transaction health for troubleshooting wiring. + +**Why this priority**: Soil moisture is the primary input to every watering decision +(PR-11); a driver that reads the real sensor correctly on the rig is the core +deliverable of this PR and the second hardware-in-the-loop milestone of phase 1. + +**Independent Test**: Flash the rig with the sensor attached, issue the soil diagnostic +command, compare each value against the Arduino unit's readings for the same probe. + +**Acceptance Scenarios**: + +1. **Given** the rig with the soil sensor connected, **When** the operator requests a + soil reading, **Then** all nine registers are fetched in one transaction and the + seven used values are reported with correct scaling (moisture ÷10 → %, temperature + ÷10 → °C signed, EC ×1 µS/cm, pH ÷10, N/P/K ×1 mg/kg). +2. **Given** a sensor reporting a below-zero temperature, **When** the value is read, + **Then** it is reported as the correct negative number (signed interpretation). +3. **Given** the rig, **When** the operator runs the bus diagnostic command, **Then** + the output shows transaction success/failure and enough detail to distinguish "no + response" from "bad response". + +--- + +### User Story 2 - Sensor faults yield invalid data, never wrong data (Priority: P2) + +A wire comes loose in the greenhouse (or the sensor dies). The system does not crash, +does not hang, and does not present stale or garbage numbers as truth: the reading is +flagged invalid with a logged error, and when the wire is reconnected the very next +read cycle recovers without a reboot. The downstream fail-safe logic (PR-11) can rely +on this validity signal to stop watering. + +**Why this priority**: Constitution principle I (Safety First) — invalid sensor data +must stop the pumps in automatic mode. That fail-safe is only as good as the validity +flag this driver produces. + +**Independent Test**: On the rig, disconnect the RS485 A/B pair mid-operation, observe +invalid readings + logged errors and no crash; reconnect and observe automatic recovery +on a subsequent read. + +**Acceptance Scenarios**: + +1. **Given** a connected sensor, **When** the A/B pair is disconnected and a read is + attempted, **Then** the read fails with a timeout error after the configured + response timeout (3000 ms default), the result is flagged invalid, and the failure + is logged — no crash, no watchdog reset. +2. **Given** a disconnected sensor, **When** the pair is reconnected, **Then** the next + read attempt succeeds without any manual intervention or restart. +3. **Given** a sensor response with an out-of-range value (moisture outside 0–100 %, + temperature outside −40–80 °C, pH outside 3–9), **When** the reading is validated, + **Then** the read fails with a distinct validation error and no partial values are + presented as valid. +4. **Given** a failing read, **When** the driver reports the failure, **Then** exactly + one bus attempt was made (no automatic retry — recovery comes from the caller's + read cadence, parity with the Arduino client). +5. **Given** the sensor answers with a Modbus exception response, **When** the reply is + parsed, **Then** it maps to a distinct error code (not a generic timeout). + +--- + +### User Story 3 - One driver, two RS485 hardware generations (Priority: P3) + +An AI developer (or Paul) builds the firmware for either board revision. On rev1 the +transceiver needs explicit transmit/receive direction control on a dedicated pin with +safe switching margins; on rev2 the transceiver switches direction automatically, has +no direction pin at all, and — because its receiver is always on — every transmitted +frame is heard back on the receive line. The same driver serves both: the board +abstraction decides direction handling, the rev2 build discards its own transmit echo +before parsing replies, and the receive line is kept from floating when the rev2 +switched sensor power domain is off. + +**Why this priority**: Dual-board support is the reason this driver is being rewritten +at all (the legacy client is rev1-only). The rev2-specific behaviors (echo, floating +RX) come straight from the 2026-07-02 hardware design review and must be encoded now so +rev2 bring-up (PR-14) is a validation exercise, not a rewrite. + +**Independent Test**: Build both board variants in CI; verify the rev1 binary +configures the direction pin and the rev2 binary does not; verify by host test that +echo bytes preceding a reply are discarded. + +**Acceptance Scenarios**: + +1. **Given** a rev1 build, **When** the driver initializes, **Then** direction control + uses the board-defined direction pin (GPIO 25) and transmitted frames complete + without truncation at 9600 baud (direction-switch margins hold). +2. **Given** a rev2 build, **When** the driver initializes, **Then** no direction pin + is configured or touched (the pin macro does not even exist — compile-time + guarantee). +3. **Given** a rev2 build, **When** a request frame is transmitted, **Then** the echo + of that frame received on the RX line is discarded and only the sensor's reply is + parsed. +4. **Given** a rev2 board with the sensor power domain switched off, **When** the + receive line would otherwise float, **Then** the internal pull-up on the RX pin + keeps the line idle-high so no garbage bytes accumulate. + +--- + +### User Story 4 - Sensor behavior testable without hardware (Priority: P4) + +An AI developer changes decode, validation or error-handling logic and runs the host +test suite in CI. Register decoding (including signed temperature), range validation, +invalid-on-timeout behavior and echo discarding are verified against mock +implementations — no devkit, no sensor, failures block the merge. + +**Why this priority**: Constitution principle II (Host-Testability). The mock soil +sensor created here is also the input PR-11's watering-controller tests build on. + +**Independent Test**: Run the host test suite on a machine with no hardware attached; +decode/validation/timeout tests pass deterministically. + +**Acceptance Scenarios**: + +1. **Given** a mock Modbus client returning a known 9-register payload, **When** the + sensor logic decodes it, **Then** every value matches the expected scaled result, + including a negative temperature case. +2. **Given** a mock client that times out, **When** a reading is requested, **Then** + the result is invalid with a timeout error and no stale values leak through. +3. **Given** a mock client returning out-of-range values, **When** the reading is + validated, **Then** the read fails with a validation error. +4. **Given** CI on a clean checkout, **When** the host tests run, **Then** they + complete without any hardware or device-specific environment. + +--- + +### Edge Cases + +- Spurious/noise bytes on the bus (e.g. from the rev2 sensor domain powering on or + timing-offset garbage) must not permanently poison subsequent transactions: the next + well-formed transaction succeeds. (The legacy client tolerates leading garbage by + scanning for the address+function pattern; equivalent resilience is required.) +- Echo handling when the reply arrives back-to-back with the echo: the boundary between + echoed request and real reply must not be misparsed. +- A reply shorter than expected (truncated frame) or with a bad CRC fails the read with + an error — never a partially-decoded "valid" reading. +- Response timeout while bytes are still trickling in: the timeout window is extended + while reception is in progress (parity), so a slow-but-arriving reply is not cut off. +- Registers 0x0007 (salinity) and 0x0008 (TDS) are read as part of the block but not + exposed as values (parity: read but unused). +- Concurrent read requests (e.g. console diagnostic while a periodic read is active in + a later PR): transactions must serialize on the bus, never interleave. +- Sensor availability probing must be a real bus read (parity), not a cached flag — + otherwise a dead sensor looks alive. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: Modbus client and soil sensor functionality MUST be exposed through + hardware-independent interfaces (ported `IModbusClient`/`ISoilSensor`, pure C++, no + hardware-SDK types) usable from host tests without any hardware headers. +- **FR-002**: The soil sensor driver MUST read device address 0x01 with function 0x03, + one transaction of 9 holding registers starting at 0x0000, over the board-defined + RS485 UART at 9600 baud 8N1. Pin source of truth is the board component (rev1: + TX 16 / RX 17 / DE 25 per `docs/parity-checklist.md`; `docs/hardware.md` is known + swapped — QUIRK 6). +- **FR-003**: Register decoding MUST apply the parity scaling map: 0x0000 moisture ÷10 + → % (moisture ≡ humidity for this sensor); 0x0001 temperature ÷10 → °C **signed**; + 0x0002 EC ×1 → µS/cm × calibration factor; 0x0003 pH ÷10 × calibration factor; + 0x0004–0x0006 N/P/K ×1 → mg/kg; 0x0007–0x0008 read but unused. +- **FR-004**: Range validation MUST fail the read (distinct error) when moisture is + outside 0–100 %, temperature outside −40–80 °C, or pH outside 3–9. EC and N/P/K + ranges are NOT enforced on read (parity with the Arduino validation). +- **FR-005**: Every reading MUST carry an explicit validity signal (valid data XOR + error code) that downstream fail-safe logic (PR-11) can consume; a failed read MUST + never present stale or partial values as valid. +- **FR-006**: The response timeout MUST default to 3000 ms and be configurable; the + timeout window MUST be extended while response bytes are arriving. Exactly **one** + bus attempt per call — the driver MUST NOT retry automatically (parity; + `docs/parity-checklist.md` §5). Any future retry mechanism would be a deliberate + behavior change, out of scope here. +- **FR-007**: Transmit/receive direction handling MUST be selected by the board + component's `BOARD_HAS_RS485_DE` flag: rev1 drives the board-defined direction pin + around each transmission such that frames complete without truncation at 9600 baud + (the legacy 50 µs assert/release margins are the reference behavior); rev2 configures + **no** direction pin, and rev2 builds MUST NOT reference a direction pin at all + (compile-time enforced by the existing board sanity checks). +- **FR-008**: On boards without a direction pin (rev2), the driver MUST discard the + echo of its own transmitted frame from the receive path before parsing the reply + (THVD1426 receiver is always enabled; FW-4, rev2 design review 2026-07-02). +- **FR-009**: The internal pull-up on the RS485 RX pin MUST be enabled so the line does + not float when the transceiver's receiver output is high-impedance (on rev2 the + transceiver shuts down with the switched sensor power domain; FW-2, rev2 design + review 2026-07-02). Enabling it unconditionally on both boards is acceptable — the + receiver output drives through it. +- **FR-010**: Malformed traffic MUST degrade gracefully: bad CRC, truncated frames, + Modbus exception responses (mapped to distinct error codes, parity: 100 + exception + code) and spurious leading bytes each fail only the current transaction; the next + well-formed transaction MUST succeed without reinitialization. +- **FR-011**: Sensor availability checks MUST perform an actual bus read (parity), not + return cached state. +- **FR-012**: Calibration MUST follow parity: per-quantity calibration factors for + moisture, pH and EC are computed locally from an operator-supplied reference value + and best-effort written to sensor registers 0x0100–0x0102 (function 0x06, echo + verified); a failed sensor write is non-fatal (the local factor still applies). +- **FR-013**: Serial diagnostics equivalent to the legacy `rs485test` and `soil` + commands MUST exist on the rig console: trigger a raw bus test transaction and a + full decoded soil reading, and expose the client's success/error statistics counters + (parity) for troubleshooting. +- **FR-014**: Mock implementations of the Modbus client and soil sensor MUST exist for + host tests, sufficient to deterministically test register decoding (incl. signed + temperature), range validation, invalid-on-timeout, exception mapping and echo + discarding in CI. +- **FR-015**: Both board targets MUST build green in CI — rev1 with the direction pin + configured, rev2 without. + +### Key Entities + +- **Soil reading**: the seven used values (moisture %, temperature °C, EC µS/cm, pH, + N/P/K mg/kg) plus validity/error state; produced atomically from one 9-register + transaction. +- **Modbus transaction**: one request/response exchange with a single device — address, + function, register window, outcome (success, timeout, CRC error, exception, + validation failure), contributing to success/error statistics. +- **RS485 board profile**: the board component's UART pins and direction-control flag + (`BOARD_HAS_RS485_DE`); the single source of truth the driver configures itself from. +- **Calibration factor**: per-quantity multiplier (moisture, pH, EC) derived from an + operator reference value; applied locally on read, best-effort mirrored to the + sensor. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Both board variants build green in CI from a clean checkout; the rev2 + binary provably contains no direction-pin handling (compile-time check). +- **SC-002**: On the rig, a soil reading returns all seven used values matching the + Arduino unit's readings for the same sensor (same probe, same soil) on every + attempt of the HIL checklist, including the signed-temperature spot check. +- **SC-003**: Disconnecting the A/B pair produces flagged-invalid readings and logged + errors with zero crashes or watchdog resets; reconnection recovers automatically on + a subsequent read with no operator action (HIL checklist). +- **SC-004**: Host test suite covering decode, validation, timeout and echo handling + runs in CI with zero hardware dependencies and passes deterministically (no flaky + reruns). +- **SC-005**: Operator can run the bus diagnostic and soil reading commands on the + first attempt using documented commands, and the output is sufficient to distinguish + wiring faults from sensor faults (HIL checklist). + +## Assumptions + +- **Calibration is in scope by parity, not by the mini-PRD's scope list**: + `docs/prd/PR-04-modbus-soil-sensor.md` does not name the calibration commands, but + `docs/parity-checklist.md` §5 lists them as `[HOST]` items under this sensor and no + other PR covers Modbus writes. Included here with exact legacy semantics (local + factor + best-effort sensor write). Calibration factors are held in memory in this + PR; persisting them via the configuration store is wired when configuration + consumers land (PR-09/PR-11), matching where the legacy firmware persists them. +- **Read cadence stays out of scope**: the legacy 5 s periodic read loop is + controller-level behavior (PR-11). This PR delivers on-demand reads (console + diagnostics + API for later PRs); no periodic task is added. +- **rev2 electrical validation is deferred**: THVD1426 behavior at 9600 baud on real + rev2 hardware (echo timing, auto-direction margins) is validated at PR-14 bring-up. + This PR covers rev2 at build level and echo/pull-up logic at host-test level, per + the mini-PRD's out-of-scope note. +- **The switched sensor power domain (`SENS_PWR_EN`) is not managed here**: rev2 power + gating of the sensor rail belongs to the board/power layer (rev2 board profile, + PR-14). This driver only guarantees it behaves correctly when the domain is off + (pull-up, FR-009) and after it turns on. +- **esp-modbus internals may satisfy some requirements natively** (e.g. direction + control via RTS, echo suppression in half-duplex mode): where the pinned component + already provides a required behavior, verifying that behavior counts as + implementing the requirement — the observable contract above is what is binding, + not who implements it. +- The legacy client's tolerant response parsing (scanning the first bytes for the + address+function pattern) is a means, not an end: the binding requirement is + garbage-resilience per FR-010, achieved by whatever mechanism the new client stack + provides. From 4fc71bc9db8e23959bc94e4a340a89b64c74b994 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 10:34:52 +0200 Subject: [PATCH 02/11] =?UTF-8?q?docs(spec):=20record=20Checkpoint=201=20c?= =?UTF-8?q?larification=20=E2=80=94=20calibration=20in=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- .../checklists/requirements.md | 7 +++--- specs/004-modbus-soil-sensor/spec.md | 24 +++++++++++++------ 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/specs/004-modbus-soil-sensor/checklists/requirements.md b/specs/004-modbus-soil-sensor/checklists/requirements.md index 297b929..12138fa 100644 --- a/specs/004-modbus-soil-sensor/checklists/requirements.md +++ b/specs/004-modbus-soil-sensor/checklists/requirements.md @@ -37,7 +37,6 @@ rev2 design review — they define WHAT the system must do, not HOW. References to esp-modbus/interface names appear only in the Input quote and in Assumptions (explicitly non-binding "who implements it" note). -- One scope judgment call documented under Assumptions instead of a - [NEEDS CLARIFICATION] marker: calibration commands are included by parity-checklist - coverage even though the mini-PRD scope list omits them. Flag for confirmation at - Checkpoint 2. +- Calibration scope was raised as the single clarification question at Checkpoint 1 + and confirmed by Paul 2026-07-02 (include, exact legacy semantics) — recorded in + the spec's Clarifications section. diff --git a/specs/004-modbus-soil-sensor/spec.md b/specs/004-modbus-soil-sensor/spec.md index 421ea6e..e93133f 100644 --- a/specs/004-modbus-soil-sensor/spec.md +++ b/specs/004-modbus-soil-sensor/spec.md @@ -13,6 +13,17 @@ docs/prd/PR-04-modbus-soil-sensor.md (authoritative mini-PRD; ground truth for b docs/parity-checklist.md §5) plus hardware-driven requirements FW-2 and FW-4 from the rev2 design review 2026-07-02." +## Clarifications + +### Session 2026-07-02 + +- Q: Are the legacy calibration commands (moisture/pH/EC factor from reference value, + best-effort write to sensor registers 0x0100–0x0102 via function 0x06) in scope for + PR-04, given the mini-PRD scope list omits them but `docs/parity-checklist.md` §5 + lists them under this sensor? → A: Include with exact legacy semantics (option A); + factors held in memory this PR, store persistence wired when configuration + consumers land (PR-09/PR-11). Confirmed by Paul at Checkpoint 1. + ## User Scenarios & Testing *(mandatory)* ### User Story 1 - Soil readings on the bench rig (Priority: P1) @@ -260,13 +271,12 @@ decode/validation/timeout tests pass deterministically. ## Assumptions -- **Calibration is in scope by parity, not by the mini-PRD's scope list**: - `docs/prd/PR-04-modbus-soil-sensor.md` does not name the calibration commands, but - `docs/parity-checklist.md` §5 lists them as `[HOST]` items under this sensor and no - other PR covers Modbus writes. Included here with exact legacy semantics (local - factor + best-effort sensor write). Calibration factors are held in memory in this - PR; persisting them via the configuration store is wired when configuration - consumers land (PR-09/PR-11), matching where the legacy firmware persists them. +- **Calibration is in scope** (confirmed at Checkpoint 1, see Clarifications): + included with exact legacy semantics (local factor + best-effort sensor write) since + `docs/parity-checklist.md` §5 lists the calibration commands as `[HOST]` items under + this sensor and no other PR covers Modbus writes. Calibration factors are held in + memory in this PR; persisting them via the configuration store is wired when + configuration consumers land (PR-09/PR-11). - **Read cadence stays out of scope**: the legacy 5 s periodic read loop is controller-level behavior (PR-11). This PR delivers on-demand reads (console diagnostics + API for later PRs); no periodic task is added. From b34b36cedb26e21fca1f42a75640be9544f2d23c Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 10:41:14 +0200 Subject: [PATCH 03/11] docs(plan): PR-04 implementation plan, research, data model, contracts, quickstart Co-Authored-By: Claude Fable 5 --- .../contracts/interfaces.md | 72 +++++++ specs/004-modbus-soil-sensor/data-model.md | 69 +++++++ specs/004-modbus-soil-sensor/plan.md | 166 ++++++++++++++++ specs/004-modbus-soil-sensor/quickstart.md | 51 +++++ specs/004-modbus-soil-sensor/research.md | 185 ++++++++++++++++++ 5 files changed, 543 insertions(+) create mode 100644 specs/004-modbus-soil-sensor/contracts/interfaces.md create mode 100644 specs/004-modbus-soil-sensor/data-model.md create mode 100644 specs/004-modbus-soil-sensor/plan.md create mode 100644 specs/004-modbus-soil-sensor/quickstart.md create mode 100644 specs/004-modbus-soil-sensor/research.md diff --git a/specs/004-modbus-soil-sensor/contracts/interfaces.md b/specs/004-modbus-soil-sensor/contracts/interfaces.md new file mode 100644 index 0000000..e90b7cb --- /dev/null +++ b/specs/004-modbus-soil-sensor/contracts/interfaces.md @@ -0,0 +1,72 @@ +# Interface Contracts: Modbus Soil Sensor (004) + +**Date**: 2026-07-02. These are the host-includable pure C++ contracts (constitution +II). Ported from the legacy headers (`include/communication/IModbusClient.h`, +`include/sensors/ISoilSensor.h`, read-only reference) with the same method surface; +Arduino-era base-class baggage is trimmed to what the new codebase defines. + +## interfaces/IModbusClient.h + +```cpp +class IModbusClient { +public: + virtual ~IModbusClient() = default; + virtual bool initialize() = 0; + virtual bool readHoldingRegisters(uint8_t deviceAddress, uint16_t startRegister, + uint16_t count, uint16_t* buffer) = 0; + virtual bool writeSingleRegister(uint8_t deviceAddress, uint16_t registerAddress, + uint16_t value) = 0; + virtual int getLastError() = 0; // 0 = OK; see data-model error table + virtual void setTimeout(uint32_t timeoutMs) = 0; + virtual void getStatistics(uint32_t* successCount, uint32_t* errorCount) = 0; +}; +``` + +Contract notes: +- `readHoldingRegisters`/`writeSingleRegister` perform exactly ONE bus attempt; + false ⇒ `getLastError()` is set. No internal retry (parity). +- `writeSingleRegister` verifies the slave's echo response (parity). +- Statistics: every call increments exactly one of success/error. + +## interfaces/ISoilSensor.h + +```cpp +class ISoilSensor { +public: + virtual ~ISoilSensor() = default; + virtual bool initialize() = 0; + virtual bool read() = 0; // one 9-register transaction + virtual bool isAvailable() = 0; // real 1-register bus probe (parity) + virtual int getLastError() = 0; + // Values from the most recent successful read(): + virtual float getMoisture() = 0; // % + virtual float getTemperature() = 0; // °C, signed + virtual float getHumidity() = 0; // ≡ getMoisture() (parity) + virtual float getPH() = 0; + virtual float getEC() = 0; // µS/cm + virtual float getNitrogen() = 0; // mg/kg + virtual float getPhosphorus() = 0; // mg/kg + virtual float getPotassium() = 0; // mg/kg + // Calibration (CP1 decision A — legacy semantics): + virtual bool calibrateMoisture(float referenceValue) = 0; + virtual bool calibratePH(float referenceValue) = 0; + virtual bool calibrateEC(float referenceValue) = 0; +}; +``` + +Contract notes: +- `read()` false ⇒ data invalid; last-good getter values MUST NOT be mistaken for + fresh (PR-11 gates on the read result/error, per spec FR-005). +- Legacy `setValidRange`/`isWithinValidRange` are NOT ported: ranges are the fixed + parity constants (moisture 0–100, temp −40–80, pH 3–9); no caller in the legacy + firmware ever changed them at runtime. Recorded as a deliberate trim. +- `LockedSoilSensor` decorator provides per-call mutual exclusion (REPL vs main + loop), same pattern/caveats as `LockedWaterPump`/`LockedConfigStore`. + +## Console contract (HIL surface, FR-013) + +| Command | Behavior | +|---|---| +| `soil` | One `read()`; prints all 7 values or error code + name | +| `rs485test` | Raw 1-register probe; prints outcome + success/error counters | +| `soil_cal_moisture ` / `soil_cal_ph ` / `soil_cal_ec ` | Runs calibration; prints new factor and whether the sensor-register write succeeded | diff --git a/specs/004-modbus-soil-sensor/data-model.md b/specs/004-modbus-soil-sensor/data-model.md new file mode 100644 index 0000000..67fbcca --- /dev/null +++ b/specs/004-modbus-soil-sensor/data-model.md @@ -0,0 +1,69 @@ +# Data Model: Modbus Soil Sensor over RS485 (004) + +**Date**: 2026-07-02 | **Spec**: [spec.md](spec.md) | **Research**: [research.md](research.md) + +## SoilReading (value object) + +Produced atomically from one 9-register transaction; either fully valid or an error. + +| Field | Type | Source | Validation (read-fails-if) | +|---|---|---|---| +| moisture | float % | reg 0x0000 ÷10, × moisture factor | outside 0–100 | +| temperature | float °C | reg 0x0001 as **int16_t** ÷10 | outside −40–80 | +| ec | float µS/cm | reg 0x0002 ×1, × EC factor | not enforced (parity) | +| ph | float | reg 0x0003 ÷10, × pH factor | outside 3–9 | +| nitrogen | float mg/kg | reg 0x0004 ×1 | not enforced (parity) | +| phosphorus | float mg/kg | reg 0x0005 ×1 | not enforced (parity) | +| potassium | float mg/kg | reg 0x0006 ×1 | not enforced (parity) | +| (salinity) | — | reg 0x0007 read, not exposed | — | +| (tds) | — | reg 0x0008 read, not exposed | — | + +Humidity ≡ moisture for this sensor (`getHumidity()` returns moisture, parity). + +**Invariant**: a failed read (bus error, timeout, exception, out-of-range) leaves +the last-good values untouched but flags the sensor invalid; getters after a failed +read must not be presented as fresh (validity flag + error code carry the truth — +consumers in PR-11 gate on validity, never on value plausibility). + +## Error codes (ISoilSensor / IModbusClient contract) + +| Code | Meaning | Parity anchor | +|---|---|---| +| 0 | OK | — | +| 1 | Not initialized | legacy | +| 2 | Bus/communication error (CRC, framing, truncated) | legacy | +| 3 | Timeout (no response within 3000 ms) | legacy | +| 5 | Range validation failed | legacy error 5 | +| 100+n | Modbus slave exception n (when surfaced by esp-modbus; else single generic exception code, documented divergence) | legacy 100+n | + +## CalibrationFactor (per quantity: moisture, pH, EC) + +| Field | Type | Notes | +|---|---|---| +| factor | float, default 1.0 | `reference / rawReading` at calibration time | +| sensor register | 0x0100 / 0x0101 / 0x0102 | best-effort write ×100 via fn 0x06; write failure non-fatal | + +Lifecycle: set by `calibrate*(reference)`; applied on every subsequent read; RAM +only in this PR (persistence → PR-09/PR-11). + +## TransactionStatistics + +`successCount`, `errorCount` (uint32), one increment per `IModbusClient` call +outcome; exposed via `getStatistics()` and the `rs485test` console dump. + +## RS485 board profile (existing, consumed not defined) + +From `firmware/components/board/board.h`: `BOARD_PIN_RS485_TX/RX`, +`BOARD_HAS_RS485_DE` (+ `BOARD_PIN_RS485_DE` iff 1). UART port number is a +`sensors`-component Kconfig/board fact (UART2, parity). + +## State transitions (sensor availability) + +``` +UNINITIALIZED --initialize() ok--> READY +READY --read ok--> READY (valid data, successCount++) +READY --read fail--> READY (invalid flag + error code, errorCount++) [no retry] +any --isAvailable()--> performs real 1-register bus read (parity), no cached state +``` + +No permanent failure state: recovery is implicit in the next read (US2). diff --git a/specs/004-modbus-soil-sensor/plan.md b/specs/004-modbus-soil-sensor/plan.md new file mode 100644 index 0000000..5f3a5bd --- /dev/null +++ b/specs/004-modbus-soil-sensor/plan.md @@ -0,0 +1,166 @@ +# Implementation Plan: Modbus Soil Sensor over RS485 + +**Branch**: `004-modbus-soil-sensor` | **Date**: 2026-07-02 | **Spec**: [spec.md](spec.md) + +**Input**: Feature specification from `specs/004-modbus-soil-sensor/spec.md` + +## Summary + +Replace the legacy hand-rolled SP3485 Modbus client with esp-modbus 2.1.2 behind +ported `IModbusClient`/`ISoilSensor` interfaces. Decode/validation/calibration logic +lives in a pure-C++ `ModbusSoilSensor` (host-tested); the only hardware-touching +class is `EspModbusClient`, which configures the board-selected UART in RS485 +half-duplex mode — rev1 drives DE via hardware RTS (GPIO 25), rev2 configures no +direction pin, gets its TX echo suppressed by the half-duplex receive gating (FW-4) +and its RX pin pulled up against a floating transceiver output (FW-2). Console +gains `soil`, `rs485test` and calibration commands for HIL. Design decisions and +their rationale: [research.md](research.md). + +## 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**: `espressif/esp-modbus==2.1.2` (new pin, added to the +`sensors` component's `idf_component.yml`; `dependencies.lock` updated +deliberately), ESP-IDF UART/GPIO drivers, esp_console (existing REPL) + +**Storage**: N/A (calibration factors RAM-only this PR; persistence → PR-09/PR-11) + +**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` + +**Project Type**: ESP-IDF component set within existing `firmware/` project + +**Performance Goals**: 9600 baud 8N1 bus; one 9-register transaction well under the +5 s legacy read cadence; no busy-waiting in the driver + +**Constraints**: parity contract `docs/parity-checklist.md` §5 (register map, +scaling, ranges, 3000 ms timeout, NO retry, real-read availability probe, +statistics); rev2 hardware requirements FW-2/FW-4 (spec FR-008/FR-009); pumps/ +safety layer untouched by this PR + +**Scale/Scope**: 1 slave device (addr 0x01), 9 holding registers + 3 calibration +registers; ~2 new interfaces, 1 new component, 3–5 console commands + +## 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. The driver's contribution to + safety is the validity/error contract (FR-005) that PR-11's fail-safe consumes; + invalid-on-timeout and range-validation failure are host-tested here. +- **II. Host-Testability**: PASS — all decode/validation/calibration logic in pure + `ModbusSoilSensor` behind `IModbusClient`; only `EspModbusClient` touches IDF + APIs and contains no business logic; mocks provided; host suite extended in CI. + Echo suppression sits below the interface boundary (UART hardware mode) — argued + in research.md R3, HIL-verified rather than host-tested. +- **III. Reproducible Builds**: PASS — esp-modbus exactly `==2.1.2`, + `dependencies.lock` committed, both board targets built in CI from clean checkout. +- **IV. Frozen Legacy**: PASS — legacy files are read-only porting reference; no + modification. +- **V. Checkpoint-Gated Workflow**: PASS — CP1 held (calibration scope, answered A); + this plan stops at CP2; implementation via implementer subagent; review + CP3 + before commit/PR. +- **VI. English Outward**: PASS — all artifacts/code/commits in English. +- **Additional constraints**: board differences only via `board` component flags + (single `#if BOARD_HAS_RS485_DE` site in `EspModbusClient`); `ESP_LOG*` with + component tag; include guards `WATERINGSYSTEM_*_H`; no partition changes. + +## Project Structure + +### Documentation (this feature) + +```text +specs/004-modbus-soil-sensor/ +├── spec.md +├── plan.md # This file +├── research.md # Phase 0 — 9 resolved decisions (R1–R9) +├── data-model.md # Phase 1 — SoilReading, error codes, calibration, stats +├── contracts/ +│ └── interfaces.md # Phase 1 — IModbusClient, ISoilSensor, console contract +├── 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) + +```text +firmware/ +├── components/ +│ ├── interfaces/include/interfaces/ +│ │ ├── IModbusClient.h # NEW — ported, pure C++ +│ │ └── ISoilSensor.h # NEW — ported, trimmed (see contracts) +│ ├── sensors/ # NEW component (PR-02 actuators pattern) +│ │ ├── CMakeLists.txt # excludes EspModbusClient on linux target +│ │ ├── idf_component.yml # espressif/esp-modbus==2.1.2 +│ │ ├── include/sensors/ +│ │ │ ├── ModbusSoilSensor.h # pure logic: decode/validate/calibrate +│ │ │ ├── LockedSoilSensor.h # mutex decorator (REPL vs main loop) +│ │ │ ├── EspModbusClient.h # esp-modbus + UART/RS485 + pull-up +│ │ │ └── testing/ +│ │ │ ├── MockModbusClient.h # scripted responses/timeouts/exceptions +│ │ │ └── MockSoilSensor.h # for PR-11 consumers +│ │ └── src/ +│ │ ├── ModbusSoilSensor.cpp +│ │ └── EspModbusClient.cpp # target-only +│ └── board/include/board/board.h # unchanged (already has RS485 pins/flag) +├── main/ +│ ├── app_main.cpp # wire EspModbusClient + LockedSoilSensor +│ └── diag_console.cpp # + soil, rs485test, soil_cal_* commands +└── test_apps/host/main/ + ├── test_soil_sensor.cpp # NEW — decode/validate/timeout/calibration + └── CMakeLists.txt # register new test file +``` + +**Structure Decision**: one new `sensors` component following the `actuators` +component layout (pure base + hardware class + `Locked*` decorator + `testing/` +mocks); interface headers join the existing `interfaces` component. `EspModbusClient` +is CMake-excluded from the linux/host target the same way `storage` excludes +esp_littlefs (research R7). + +## Key Design Decisions (from research.md) + +| # | Decision | Spec FR | +|---|---|---| +| R1 | esp-modbus 2.x handle API, `mbc_master_send_request` raw requests — no CID dictionary | FR-002 | +| R2 | `UART_MODE_RS485_HALF_DUPLEX` on both boards; RTS = DE pin on rev1, no RTS on rev2; HIL verifies TXS0108E margins (fallback: manual GPIO DE) | FR-007 | +| R3 | rev2 echo suppressed by half-duplex RX gating; no app-level scrubber; PR-14 HIL verifies | FR-008 | +| R4 | Unconditional internal pull-up on RX pin in client init | FR-009 | +| R5 | `response_tout_ms = 3000`; esp-modbus event-driven receive ≈ legacy "extend while arriving"; strictly no retry | FR-006 | +| R6 | esp_err_t → legacy-shaped error codes; 100+n exception range when surfaced, else documented divergence | FR-010 | +| R7 | Component layout (above); decode/validation pure & host-tested | FR-001/014 | +| R8 | Calibration legacy-exact (CP1 answer A), RAM-only factors | FR-012 | +| R9 | Console: `soil`, `rs485test`, `soil_cal_*` via LockedSoilSensor | FR-013 | + +## Risks & Open Items (carried to tasks/HIL) + +1. **RTS timing vs TXS0108E margins (R2)** — HIL item on the rig; documented + fallback path that stays behind `IModbusClient`. +2. **esp-modbus runtime timeout setter availability (R5)** — implementer verifies + against pinned 2.1.2; degrade `setTimeout` to init-time-only with doc note. +3. **Exception-code granularity (R6)** — possible parity divergence (100+n → single + exception code); record in PR description like PR-06's divergences if hit. +4. **Kconfig timeout clamp (R5)** — assert `CONFIG_FMB_MASTER_TIMEOUT_MS_RESPOND` + allows 3000 in both board sdkconfigs; CI-visible if violated. +5. **rev2 echo behavior unverifiable until PR-14** — accepted per spec assumption; + T3.5 resync is the fallback layer. + +## Agent context update + +Skipped deliberately: root `CLAUDE.md` carries no `` markers in this +repo and unprompted root-CLAUDE.md edits are blocked by policy; +`.specify/feature.json` (already pointing at `specs/004-modbus-soil-sensor`) is what +downstream spec-kit commands use. + +## Complexity Tracking + +No constitution violations — table intentionally empty. diff --git a/specs/004-modbus-soil-sensor/quickstart.md b/specs/004-modbus-soil-sensor/quickstart.md new file mode 100644 index 0000000..88ff0d9 --- /dev/null +++ b/specs/004-modbus-soil-sensor/quickstart.md @@ -0,0 +1,51 @@ +# Quickstart Validation: Modbus Soil Sensor (004) + +Prerequisites: docker (espressif/idf:v6.0.1). OneDrive tree cannot be mounted by +Docker Desktop — rsync to /tmp first (PR-06 lesson): + +```bash +rsync -a --delete --exclude build --exclude managed_components --exclude sdkconfig \ + "$PWD/firmware/" /tmp/ws004-firmware/ +``` + +## 1. Host tests (CI-equivalent) — [CI] acceptance criteria + +```bash +docker run --rm -v /tmp/ws004-firmware:/project -w /project/test_apps/host \ + espressif/idf:v6.0.1 bash -c "idf.py --preview set-target linux && idf.py build \ + && ./build/host_tests.elf" +``` + +Expected: exit code 0; suite includes soil-sensor cases: decode of a known +9-register payload (incl. negative temperature), range-validation failures +(moisture/temp/pH), invalid-on-timeout via MockModbusClient, exception-code +mapping, calibration factor computation + best-effort write, statistics counters. + +## 2. Both board targets build — [CI] acceptance criterion + +```bash +# rev1 (repeat with BOARD_REV2 for rev2; fullclean + rm sdkconfig between boards) +docker run --rm -v /tmp/ws004-firmware:/project -w /project espressif/idf:v6.0.1 \ + bash -c "idf.py fullclean; rm -f sdkconfig; \ + echo CONFIG_BOARD_REV1_DEVKIT=y >> sdkconfig.defaults.local && idf.py build" +``` + +Expected: both builds green; rev2 build contains no `BOARD_PIN_RS485_DE` reference +(compile-time guaranteed). + +## 3. HIL on the rev1 bench rig — Checkpoint 3 checklist (Paul) + +Rig: devkit + RS485 5 Click + soil sensor at 9600 8N1 (TX 16 / RX 17 / DE 25). + +| # | Step | Expected | +|---|---|---| +| 1 | Flash, open console, run `soil` | All 7 values printed; plausible vs Arduino unit on same probe (SC-002) | +| 2 | Compare each value with the Arduino unit | Match within one scaling step; temperature sign correct | +| 3 | `rs485test` | Success outcome, counters increment | +| 4 | Disconnect A/B, run `soil` | Error after ~3 s (timeout), flagged invalid, logged; no crash/watchdog (SC-003) | +| 5 | Reconnect A/B, run `soil` | Next read succeeds, no reboot needed | +| 6 | `soil_cal_ph ` with known buffer (optional) | Factor updated; write result reported; subsequent `soil` reflects factor | +| 7 | Scope/logic-analyzer on DE (optional) | Frames untruncated at 9600 baud (R2 risk check, parity §5 HIL item) | + +rev2-specific items (echo suppression, RX pull-up effect with SENS_PWR_EN off) are +validated at PR-14 bring-up per spec assumption. diff --git a/specs/004-modbus-soil-sensor/research.md b/specs/004-modbus-soil-sensor/research.md new file mode 100644 index 0000000..9de5d4d --- /dev/null +++ b/specs/004-modbus-soil-sensor/research.md @@ -0,0 +1,185 @@ +# Research: Modbus Soil Sensor over RS485 (004) + +**Date**: 2026-07-02 | **Spec**: [spec.md](spec.md) + +All NEEDS CLARIFICATION items from Technical Context are resolved here. Sources: +esp-modbus docs (context7, `/espressif/esp-modbus`), legacy client +(`src/communication/SP3485ModbusClient.cpp`, read-only reference), parity contract +(`docs/parity-checklist.md` §5), rev2 design review 2026-07-02 (FW-2/FW-4). + +## R1: esp-modbus API generation — 2.x object API + +**Decision**: Use the esp-modbus 2.x handle-based API: `mbc_master_create_serial()` +with `mb_communication_info_t.ser_opts` (port, `MB_RTU`, 9600 baud, +`UART_DATA_8_BITS`, `UART_STOP_BITS_1`, `MB_PARITY_NONE`, +`response_tout_ms = 3000`), then `mbc_master_send_request()` with +`mb_param_request_t` for raw register access. No CID/data-dictionary layer. + +**Rationale**: Version pin 2.1.2 is mandated by the PRD. The request-based API maps +1:1 onto the ported `IModbusClient` (`readHoldingRegisters` → command 0x03, +`writeSingleRegister` → command 0x06) without inventing a parameter dictionary the +system doesn't need — the soil sensor is one device with one fixed register window. +`mbc_master_send_request` returns `ESP_ERR_TIMEOUT` on no-response, distinct codes +for invalid responses, satisfying FR-005/FR-010 error discrimination. + +**Alternatives considered**: +- *CID data dictionary (`mbc_master_get_parameter`)*: adds a static descriptor table + and float conversion machinery for no benefit — decode/scaling must live in + host-testable `ModbusSoilSensor` logic anyway (parity scaling is bespoke). +- *Raw UART port of the legacy client*: rejected by the PRD (esp-modbus is the point + of this PR); would re-own CRC/framing/timing code that esp-modbus already provides. + +## R2: RS485 direction control — UART half-duplex mode on both boards + +**Decision**: Configure the Modbus UART in `UART_MODE_RS485_HALF_DUPLEX` on **both** +boards, differing only in the RTS pin wiring: +- rev1 (`BOARD_HAS_RS485_DE == 1`): `uart_set_pin(..., rts = BOARD_PIN_RS485_DE)` — + the UART peripheral drives DE via RTS automatically around each frame. +- rev2 (`BOARD_HAS_RS485_DE == 0`): `uart_set_pin(..., rts = UART_PIN_NO_CHANGE)` — + no direction pin exists; the THVD1426 auto-directs. + +Call order per the pinned esp-modbus serial-master example: create → `uart_set_pin` +→ start → `uart_set_mode(UART_MODE_RS485_HALF_DUPLEX)`. + +**Rationale**: Satisfies FR-007 with zero application-level `#ifdef` logic beyond the +pin selection (constitution: board differences only via the board component). The +`#if BOARD_HAS_RS485_DE` guard lives in one place in `EspModbusClient`; rev2 builds +never reference `BOARD_PIN_RS485_DE` (it is undefined there — compile-time enforced +by the existing board.h sanity checks). + +**Risk (HIL-verified)**: hardware-timed RTS switches within microseconds of frame +start/end, whereas the legacy driver gave the TXS0108E-shifted DE a 50 µs (2× +applied = 100 µs) assert margin and 50 µs release margin. At 9600 baud one bit is +104 µs, and the TXS0108E propagates in nanoseconds, so margins should hold — but +this is exactly the parity-checklist §5 open HIL item ("frames still complete +without truncation"). If HIL shows truncation on rev1, fallback is manual DE via a +GPIO toggled around `uart_wait_tx_done()` (legacy-equivalent timing), still behind +`IModbusClient`. + +**Alternatives considered**: manual GPIO DE control as primary (rejected: reproduces +the code esp-modbus/UART hardware already provides and adds a race-prone task-timing +dependency); `UART_MODE_RS485_APP_CTRL` (rejected: lowest-level option, no need). + +## R3: rev2 TX echo (FW-4) — suppressed by half-duplex mode, verified at HIL + +**Decision**: Rely on `UART_MODE_RS485_HALF_DUPLEX` receive gating for echo removal: +in this mode the ESP32 UART ignores the receive path while the transmitter is +active, so the echo the THVD1426's always-on receiver (RE̅ grounded) feeds back +during transmission never reaches the driver. No application-level echo scrubber is +written up front. A rev2 HIL checklist item (deferred to PR-14 with the rest of +rev2 electrical validation, per spec assumption) verifies no echo bytes leak; the +esp-modbus RTU state machine's T3.5 frame resynchronization is the second line of +defense against residual tail bytes. + +**Rationale**: The echo is physically simultaneous with transmission (transceiver +loopback, not a store-and-forward reflection), so TX-gated receive removal is +complete by construction. Writing a speculative echo-scrubber above esp-modbus would +be dead code below our own test boundary — and FR-008's observable contract is +"only the sensor's reply is parsed", not "an echo scrubber exists". + +**Spec note (FR-014)**: this resolves "echo discarding" at a layer beneath +host-testable code; host tests therefore cover decode/validation/timeout/exception +paths against the mock client, and echo correctness is a HIL concern (PR-14). The +spec's FR-014 echo-discard mention is satisfied by testing that the sensor logic +parses exactly one well-formed reply per transaction from the mock — the +host-observable equivalent. + +## R4: RX pull-up (FW-2) — unconditional `gpio_pullup_en` on the RX pin + +**Decision**: After UART pin setup, enable the internal pull-up on +`BOARD_PIN_RS485_RX` (IO17) unconditionally on both boards, in `EspModbusClient` +initialization. + +**Rationale**: FR-009. On rev2 the THVD1426 SHDN̅ tracks `SENS_PWR_EN`; RO goes hi-Z +when the sensor domain is off and IO17 would otherwise float into the UART RX, +producing garbage bytes/noise. The transceiver's RO output drives through a weak +pull-up when active, so leaving it enabled permanently is harmless on both boards +(rev1's ADM3485 RO likewise drives push-pull). Doing it in the client (not the board +component) keeps the board component a pure pin/flag table, per PR-02's design. + +## R5: Timeout semantics — `response_tout_ms = 3000`, parity equivalence documented + +**Decision**: Set `ser_opts.response_tout_ms = 3000` at create time and route +`IModbusClient::setTimeout()` to the esp-modbus runtime timeout setter if 2.1.2 +exposes one; if it does not, `setTimeout` is honored at initialization only and +documented as such (implementer verifies against the pinned component; the console +never changes the timeout at runtime today). Verify that Kconfig +`CONFIG_FMB_MASTER_TIMEOUT_MS_RESPOND`'s upper bound does not clamp 3000 in +`sdkconfig.defaults`. + +**Rationale**: FR-006 requires a 3000 ms default (parity). The legacy +"timeout extended while bytes arrive" behavior is provided equivalently by +esp-modbus's event-driven receive path (inter-character T1.5 / frame T3.5 timers +mean a trickling frame is not cut off mid-reception); the observable contract — +slow-but-arriving replies survive, absent replies fail at ~3000 ms — holds. Exactly +one bus attempt per call: esp-modbus master performs no application-level retry for +`send_request`, matching the no-retry parity rule; the implementer MUST NOT add +retry loops. + +## R6: Error-code mapping — esp_err_t → legacy-shaped error codes + +**Decision**: `EspModbusClient` maps esp-modbus results onto the `IModbusClient` +error contract: 0 = OK, distinct codes for timeout (`ESP_ERR_TIMEOUT`), invalid +response/CRC (`ESP_ERR_INVALID_RESP`-class), and Modbus slave exceptions. Slave +exceptions map to the legacy 100+n range when the underlying API surfaces the +exception code; if 2.1.2 reports exceptions only as a generic invalid-response +error, they map to a single documented "slave exception/invalid response" code. + +**Rationale**: FR-010's binding requirement is *distinct from timeout* (fail-safe +diagnostics need to distinguish "sensor absent" from "sensor confused"). The legacy +100+exception granularity is preserved when the API allows; otherwise the coarser +mapping is recorded as a parity divergence in the PR (same mechanism as PR-06's +documented divergences). Statistics counters (FR-013) are counted in +`EspModbusClient` exactly like the legacy client: one success or one error per call. + +## R7: Component layout & decode placement — new `sensors` component, pure logic base + +**Decision**: Follow the PR-02 actuators pattern: +- `firmware/components/interfaces/include/interfaces/`: `IModbusClient.h`, + `ISoilSensor.h` (ported, pure C++, host-includable; drop the legacy `ISensor` + Arduino heritage in favor of the minimal base the new codebase needs). +- New `firmware/components/sensors/`: `ModbusSoilSensor` (pure logic: register + decode incl. signed temperature, scaling, range validation, calibration factors, + availability probe — depends only on `IModbusClient`), `LockedSoilSensor` + (mutex decorator, REPL + future controller access), `testing/MockModbusClient.h` + + `testing/MockSoilSensor.h`. +- `EspModbusClient` (the only file touching esp-modbus/UART/GPIO APIs) in the same + `sensors` component but excluded from the linux/host build via the same + CMake target-guard used by `storage` for esp_littlefs. +- esp-modbus dependency pinned `espressif/esp-modbus==2.1.2` in the component's + `idf_component.yml`; `dependencies.lock` updated deliberately (constitution III). + +**Rationale**: Constitution II — everything above `IModbusClient` is host-testable +by construction; the hardware-touching client contains no business logic. The +decode/validation/calibration logic is exactly what PR-11's fail-safe tests consume. + +**Alternatives considered**: separate `modbus` component for the client (rejected +for now: one consumer; can be split when the level sensors/INA226 PR needs shared +bus infrastructure — noted for PR-05). + +## R8: Calibration semantics (CP1 answer A) — legacy-exact + +**Decision**: Port legacy behavior verbatim: `calibrateMoisture/PH/EC(reference)` +compute `factor = reference / rawReading` from a fresh read, apply the factor +locally to subsequent reads (EC and pH per parity map; moisture factor exists in +legacy and is ported as-is), and best-effort write the factor (×100, per legacy +encoding) to sensor registers 0x0100/0x0101/0x0102 via function 0x06 with echo +verification — a failed write logs a warning and returns success for the local +part (non-fatal, parity). Factors live in memory; persistence is wired when +configuration consumers land (PR-09/PR-11). + +**Rationale**: CP1 decision (option A), parity checklist §5. Implementer ports the +exact legacy formula from `src/sensors/ModbusSoilSensor.cpp:207-322` (read-only +reference) rather than re-deriving it. + +## R9: Console diagnostics — extend `diag_console.cpp` + +**Decision**: Add `soil` (full decoded reading + validity/error), `rs485test` (raw +single-transaction bus probe + statistics counters dump) to the existing esp-console +REPL in `firmware/main/diag_console.cpp`, following the command style PR-02/PR-06 +established. Calibration console commands (`soil_cal_moisture ` etc.) included +since calibration is in scope. + +**Rationale**: FR-013; same HIL surface Paul already uses on the rig. Access to the +shared sensor goes through `LockedSoilSensor` (REPL vs future main-loop, the PR-02 +race lesson). From a693c0f87acd738cf24d4da67897bd587f924461 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 10:43:07 +0200 Subject: [PATCH 04/11] =?UTF-8?q?docs(tasks):=20PR-04=20task=20breakdown?= =?UTF-8?q?=20=E2=80=94=2029=20tasks=20across=207=20phases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- specs/004-modbus-soil-sensor/tasks.md | 216 ++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 specs/004-modbus-soil-sensor/tasks.md diff --git a/specs/004-modbus-soil-sensor/tasks.md b/specs/004-modbus-soil-sensor/tasks.md new file mode 100644 index 0000000..c80e0e4 --- /dev/null +++ b/specs/004-modbus-soil-sensor/tasks.md @@ -0,0 +1,216 @@ +# Tasks: Modbus Soil Sensor over RS485 + +**Input**: Design documents from `specs/004-modbus-soil-sensor/` +**Prerequisites**: plan.md, research.md (R1–R9), data-model.md, contracts/interfaces.md, quickstart.md + +**Tests**: included — the spec's [CI] acceptance criteria explicitly require host +tests (FR-014); test tasks precede/accompany implementation per constitution II. + +**Process rules (PR-06 lessons)**: implementer agent missions are WRITE-ONLY — no +docker builds inside agent missions. Every task marked **[VERIFY-MAIN]** is executed +by the orchestrator in the main session via Bash (docker + rsync-to-/tmp pattern, +see quickstart.md). Commit after each phase at minimum; tasks are sized to survive +agent death. + +**Organization**: tasks grouped by user story (US1 readings, US2 fault handling, +US3 dual-board, US4 host-testability) so each story is independently verifiable. + +## Phase 1: Setup + +- [ ] T001 Create `sensors` component skeleton: `firmware/components/sensors/CMakeLists.txt` + (REQUIRES `interfaces`, `board`; register `src/ModbusSoilSensor.cpp` always and + `src/EspModbusClient.cpp` only when `IDF_TARGET != linux`, same guard style as + `firmware/components/storage/CMakeLists.txt`) and + `firmware/components/sensors/idf_component.yml` pinning `espressif/esp-modbus: "==2.1.2"` +- [ ] T002 [VERIFY-MAIN] Confirm `CONFIG_FMB_MASTER_TIMEOUT_MS_RESPOND` bounds allow + 3000 ms in esp-modbus 2.1.2 Kconfig (inspect managed component after a + dependency fetch) and add any required override to `firmware/sdkconfig.defaults` + (plan risk 4) + +## Phase 2: Foundational (blocking prerequisites for all stories) + +- [ ] T003 [P] Port `IModbusClient` to + `firmware/components/interfaces/include/interfaces/IModbusClient.h` per + contracts/interfaces.md (pure C++, guard `WATERINGSYSTEM_INTERFACES_IMODBUSCLIENT_H`, + SPDX header, contract notes as doc comments: one attempt/no retry, write echo + verification, statistics semantics) +- [ ] T004 [P] Port `ISoilSensor` to + `firmware/components/interfaces/include/interfaces/ISoilSensor.h` per + contracts/interfaces.md (trimmed surface — no setValidRange/isWithinValidRange; + document the trim and the read()/validity contract) +- [ ] T005 [P] Create `MockModbusClient` in + `firmware/components/sensors/include/sensors/testing/MockModbusClient.h`: + scriptable register payloads per (address, startRegister, count), forced + timeout/CRC/exception errors, call recording (incl. writeSingleRegister log), + statistics counters — style of `actuators/testing` and `storage/testing` mocks + +**Checkpoint**: interfaces + mock exist — all story phases can start. + +## Phase 3: User Story 1 — Soil readings on the bench rig (P1) 🎯 MVP + +**Goal**: real sensor read on the rig: all 9 registers in one transaction, parity +scaling incl. signed temperature, console `soil`/`rs485test` output. + +**Independent test**: quickstart.md §1 decode tests green + §3 HIL steps 1–3. + +- [ ] T006 [P] [US1] Write host tests for register decode in + `firmware/test_apps/host/main/test_soil_sensor.cpp`: known 9-register payload + decodes to expected moisture/temp/EC/pH/N/P/K (data-model scaling table), + negative temperature case (0xFF38 → −20.0 °C), humidity ≡ moisture, + salinity/TDS not exposed; register in + `firmware/test_apps/host/main/CMakeLists.txt` +- [ ] T007 [US1] Implement `ModbusSoilSensor` (pure logic) in + `firmware/components/sensors/include/sensors/ModbusSoilSensor.h` + + `firmware/components/sensors/src/ModbusSoilSensor.cpp`: initialize/read + (one readHoldingRegisters(0x01, 0x0000, 9) call), decode per data-model.md, + getters from last successful read, real-bus-read isAvailable() (1 register, + parity), getLastError; port reference `src/sensors/ModbusSoilSensor.cpp` + (READ-ONLY legacy) +- [ ] T008 [US1] Implement `EspModbusClient` core in + `firmware/components/sensors/include/sensors/EspModbusClient.h` + + `firmware/components/sensors/src/EspModbusClient.cpp`: R1 create/start sequence + (`mbc_master_create_serial`, ser_opts 9600 8N1 `response_tout_ms=3000`, UART + port + pins from `board/board.h`), `uart_set_mode(UART_MODE_RS485_HALF_DUPLEX)`, + RTS = `BOARD_PIN_RS485_DE` under `#if BOARD_HAS_RS485_DE` else no RTS pin (R2), + readHoldingRegisters/writeSingleRegister via `mbc_master_send_request` + (commands 0x03/0x06), esp_err_t → error-code mapping per data-model.md (R6), + statistics counters +- [ ] T009 [US1] Add `LockedSoilSensor` decorator in + `firmware/components/sensors/include/sensors/LockedSoilSensor.h` (per-call + mutex, pattern of `actuators/LockedWaterPump.h`; document per-call-not- + cross-call atomicity like `storage/Locked*`) +- [ ] T010 [US1] Wire sensor into app in `firmware/main/app_main.cpp`: + construct `EspModbusClient` + `ModbusSoilSensor` + `LockedSoilSensor` after + storage init; boot log line with client init result; no periodic read task + (out of scope, PR-11) +- [ ] T011 [US1] Add console commands `soil` and `rs485test` in + `firmware/main/diag_console.cpp` per contracts/interfaces.md console contract + (values or error code+name; raw probe + statistics dump), going through + `LockedSoilSensor`/client +- [ ] T012 [VERIFY-MAIN] [US1] Host suite green (quickstart §1) — decode tests pass, + exit code 0 +- [ ] T013 [VERIFY-MAIN] [US1] rev1 target builds green (quickstart §2, rev1 half) + +**Checkpoint**: MVP flashable — HIL steps 1–3 executable by Paul at CP3. + +## Phase 4: User Story 2 — Faults yield invalid data, never wrong data (P2) + +**Goal**: timeout/validation/exception paths flag invalid + log, one attempt only, +auto-recovery on next read. + +**Independent test**: host tests for all fault paths green; HIL steps 4–5. + +- [ ] T014 [P] [US2] Extend `firmware/test_apps/host/main/test_soil_sensor.cpp` with + fault-path tests: timeout → read() false + timeout error + getters not + presented as fresh; out-of-range moisture/temp/pH → validation error 5; + Modbus exception → distinct 100+n (or documented generic) code; exactly ONE + client call per read() on failure (no retry — assert via mock call recording); + recovery: failing mock then working mock → next read() true; statistics + increment correctness +- [ ] T015 [US2] Implement fault handling in + `firmware/components/sensors/src/ModbusSoilSensor.cpp`: range validation per + data-model table (fail read, error 5, EC/NPK unenforced per parity), validity + flag semantics, ESP_LOG error on every failed read (tag `soil_sensor`), + no-retry invariant, lazy availability recovery (no permanent-failure state) +- [ ] T016 [US2] Verify/complete error discrimination in + `firmware/components/sensors/src/EspModbusClient.cpp`: timeout vs invalid- + response vs exception mapping (R6); if 2.1.2 hides the slave exception code, + map to the single documented exception code and note the parity divergence in + a code comment + PR notes +- [ ] T017 [VERIFY-MAIN] [US2] Host suite green including all fault-path tests + +**Checkpoint**: fail-safe contract (FR-005/FR-006/FR-010) host-proven. + +## Phase 5: User Story 3 — One driver, two RS485 generations (P3) + +**Goal**: rev2 build without DE pin, echo handled, RX pull-up active; rev1 +unaffected. + +**Independent test**: both targets build (quickstart §2); rev2 binary has no DE +reference; pull-up call present. + +- [ ] T018 [US3] Add FW-2 RX pull-up in + `firmware/components/sensors/src/EspModbusClient.cpp`: after pin setup, + `gpio_pullup_en(BOARD_PIN_RS485_RX)` unconditionally with comment referencing + FW-2/THVD1426 SHDN̅–SENS_PWR_EN coupling (R4) +- [ ] T019 [US3] Document FW-4 echo strategy in + `firmware/components/sensors/src/EspModbusClient.cpp` (comment at + uart_set_mode site): half-duplex RX gating suppresses THVD1426 echo on rev2, + T3.5 resync as fallback, PR-14 HIL verifies (R3); confirm no rev1-only + assumptions in the shared path +- [ ] T020 [VERIFY-MAIN] [US3] Both board targets build green from clean config + (quickstart §2) and `strings`/grep of rev2 build objects show no + `BOARD_PIN_RS485_DE` usage compiled in (board.h sanity would have failed the + build — record the check output) + +**Checkpoint**: FR-007/008/009/015 satisfied at build+code level; electrical proof +deferred to PR-14 per spec assumption. + +## Phase 6: User Story 4 — Testable without hardware + calibration (P4) + +**Goal**: complete host-test surface incl. calibration (CP1 answer A) and the mock +soil sensor PR-11 will consume; CI runs it all. + +**Independent test**: full host suite green in CI on linux target. + +- [ ] T021 [P] [US4] Extend `firmware/test_apps/host/main/test_soil_sensor.cpp` with + calibration tests: factor = reference/raw from fresh read; factor applied to + subsequent reads (EC, pH, moisture); best-effort write to 0x0100/0x0101/0x0102 + fn 0x06 with legacy ×100 encoding (assert via mock write log); write failure + → calibrate returns success for local part + factor still applied (parity + non-fatal) +- [ ] T022 [US4] Implement calibration in + `firmware/components/sensors/src/ModbusSoilSensor.cpp` + + `ModbusSoilSensor.h`: `calibrateMoisture/PH/EC` porting the exact legacy + formula from `src/sensors/ModbusSoilSensor.cpp:207-322` (READ-ONLY reference); + factors RAM-only (R8) +- [ ] T023 [P] [US4] Create `MockSoilSensor` in + `firmware/components/sensors/include/sensors/testing/MockSoilSensor.h` + (settable values/validity/errors — the PR-11 consumer fixture) +- [ ] T024 [US4] Add calibration console commands `soil_cal_moisture`, + `soil_cal_ph`, `soil_cal_ec` in `firmware/main/diag_console.cpp` per console + contract (factor + write-result reporting) +- [ ] T025 [VERIFY-MAIN] [US4] Full host suite green (all soil tests + existing 50 + tests, exit 0); confirm CI workflow needs no change beyond the new files + (esp-idf-ci-action already runs `test_apps/host` with `target: linux`) + +**Checkpoint**: all [CI] acceptance criteria met. + +## Phase 7: Polish & cross-cutting + +- [ ] T026 [P] Write HIL checklist to + `specs/004-modbus-soil-sensor/checklists/hil.md` from quickstart §3 (steps, + expected outcomes, sign-off boxes; note rev2 items deferred to PR-14) for + Paul at Checkpoint 3 +- [ ] T027 [P] Update `firmware/CLAUDE.md`: `sensors` component summary, esp-modbus + pin, console command additions, host-test file list +- [ ] T028 Record parity divergences (if any hit during implementation: exception- + code granularity R6, setTimeout runtime support R5, RTS-timing fallback R2) + in code comments + `specs/004-modbus-soil-sensor/plan.md` Risks section + updates +- [ ] T029 [VERIFY-MAIN] Final clean-checkout verification: rsync fresh copy, + both board builds + host suite from scratch (quickstart §1+§2), commit + `dependencies.lock` change deliberately (constitution III) + +## Dependencies & execution order + +- Phase 1 → Phase 2 → Phase 3 (US1) → Phase 4 (US2) → Phase 5 (US3) → Phase 6 + (US4) → Phase 7. Stories after US1 are logically independent but share + `ModbusSoilSensor.cpp`/`EspModbusClient.cpp`, so run sequentially (single + implementer at a time per file; [P] marks the safe parallel writes). +- T002 can run any time before T012. T005 blocks T006/T014/T021. T008 blocks + T010–T013. Console tasks (T011, T024) depend on wiring (T010). + +## Parallel opportunities + +- Phase 2: T003, T004, T005 (three different files). +- T006 (tests) alongside T007/T008 (different files). +- Phase 6: T021 ∥ T023; Phase 7: T026 ∥ T027. + +## Implementation strategy + +MVP = Phase 1–3 (US1): flashable rig build with `soil`/`rs485test` — already +HIL-demonstrable. Then US2 (safety contract), US3 (rev2 deltas), US4 +(calibration + full CI surface), polish. Implementer missions per phase (write-only, +one commit per phase minimum); orchestrator runs every [VERIFY-MAIN] task in the +main session between missions; fixer agent handles review findings at CP3. From 23b2db8f4db04066f335fc3619a4473d01497c62 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 10:51:00 +0200 Subject: [PATCH 05/11] docs(spec): apply CP2 analyze remediations I1/I2/C1/C2 Co-Authored-By: Claude Fable 5 --- specs/004-modbus-soil-sensor/plan.md | 2 +- specs/004-modbus-soil-sensor/spec.md | 7 +++++-- specs/004-modbus-soil-sensor/tasks.md | 9 +++++++-- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/specs/004-modbus-soil-sensor/plan.md b/specs/004-modbus-soil-sensor/plan.md index 5f3a5bd..c25553e 100644 --- a/specs/004-modbus-soil-sensor/plan.md +++ b/specs/004-modbus-soil-sensor/plan.md @@ -112,7 +112,7 @@ firmware/ │ │ └── src/ │ │ ├── ModbusSoilSensor.cpp │ │ └── EspModbusClient.cpp # target-only -│ └── board/include/board/board.h # unchanged (already has RS485 pins/flag) +│ └── board/include/board/board.h # + BOARD_RS485_UART_PORT in both profiles (analyze I1) ├── main/ │ ├── app_main.cpp # wire EspModbusClient + LockedSoilSensor │ └── diag_console.cpp # + soil, rs485test, soil_cal_* commands diff --git a/specs/004-modbus-soil-sensor/spec.md b/specs/004-modbus-soil-sensor/spec.md index e93133f..ef954e2 100644 --- a/specs/004-modbus-soil-sensor/spec.md +++ b/specs/004-modbus-soil-sensor/spec.md @@ -231,8 +231,11 @@ decode/validation/timeout tests pass deterministically. (parity) for troubleshooting. - **FR-014**: Mock implementations of the Modbus client and soil sensor MUST exist for host tests, sufficient to deterministically test register decoding (incl. signed - temperature), range validation, invalid-on-timeout, exception mapping and echo - discarding in CI. + temperature), range validation, invalid-on-timeout, exception mapping and + single-reply parsing (exactly one well-formed reply parsed per transaction — the + host-observable equivalent of rev2 echo discarding, which is resolved at the + hardware receive-gating layer per plan decision R3 and verified electrically at + PR-14) in CI. - **FR-015**: Both board targets MUST build green in CI — rev1 with the direction pin configured, rev2 without. diff --git a/specs/004-modbus-soil-sensor/tasks.md b/specs/004-modbus-soil-sensor/tasks.md index c80e0e4..0a2b935 100644 --- a/specs/004-modbus-soil-sensor/tasks.md +++ b/specs/004-modbus-soil-sensor/tasks.md @@ -70,7 +70,10 @@ scaling incl. signed temperature, console `soil`/`rs485test` output. `firmware/components/sensors/include/sensors/EspModbusClient.h` + `firmware/components/sensors/src/EspModbusClient.cpp`: R1 create/start sequence (`mbc_master_create_serial`, ser_opts 9600 8N1 `response_tout_ms=3000`, UART - port + pins from `board/board.h`), `uart_set_mode(UART_MODE_RS485_HALF_DUPLEX)`, + port + pins from `board/board.h` — add `BOARD_RS485_UART_PORT` (2, parity + UART2) to BOTH board profiles in + `firmware/components/board/include/board/board.h`, analyze finding I1), + `uart_set_mode(UART_MODE_RS485_HALF_DUPLEX)`, RTS = `BOARD_PIN_RS485_DE` under `#if BOARD_HAS_RS485_DE` else no RTS pin (R2), readHoldingRegisters/writeSingleRegister via `mbc_master_send_request` (commands 0x03/0x06), esp_err_t → error-code mapping per data-model.md (R6), @@ -106,7 +109,9 @@ auto-recovery on next read. Modbus exception → distinct 100+n (or documented generic) code; exactly ONE client call per read() on failure (no retry — assert via mock call recording); recovery: failing mock then working mock → next read() true; statistics - increment correctness + increment correctness; isAvailable() performs a real 1-register bus read + (assert via mock call recording — FR-011, analyze C1); setTimeout() reaches + the client (mock passthrough assert — FR-006, analyze C2) - [ ] T015 [US2] Implement fault handling in `firmware/components/sensors/src/ModbusSoilSensor.cpp`: range validation per data-model table (fail read, error 5, EC/NPK unenforced per parity), validity From ac34c53ef22f6c64817c3771e2df7b88508ab427 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 10:57:12 +0200 Subject: [PATCH 06/11] feat(firmware): sensors component skeleton, IModbusClient/ISoilSensor interfaces, mock client T001+T003-T005: component CMake with linux-target exclusion of the hardware client, esp-modbus ==2.1.2 rule-gated to non-linux targets, ported pure-C++ interfaces with parity contract notes, scriptable MockModbusClient with call recording for the host test suite. Co-Authored-By: Claude Fable 5 --- .../include/interfaces/IModbusClient.h | 105 +++++++++ .../include/interfaces/ISoilSensor.h | 125 ++++++++++ firmware/components/sensors/CMakeLists.txt | 28 +++ firmware/components/sensors/idf_component.yml | 12 + .../sensors/testing/MockModbusClient.h | 217 ++++++++++++++++++ .../sensors/src/EspModbusClient.cpp | 6 + .../sensors/src/ModbusSoilSensor.cpp | 5 + 7 files changed, 498 insertions(+) create mode 100644 firmware/components/interfaces/include/interfaces/IModbusClient.h create mode 100644 firmware/components/interfaces/include/interfaces/ISoilSensor.h create mode 100644 firmware/components/sensors/CMakeLists.txt create mode 100644 firmware/components/sensors/idf_component.yml create mode 100644 firmware/components/sensors/include/sensors/testing/MockModbusClient.h create mode 100644 firmware/components/sensors/src/EspModbusClient.cpp create mode 100644 firmware/components/sensors/src/ModbusSoilSensor.cpp diff --git a/firmware/components/interfaces/include/interfaces/IModbusClient.h b/firmware/components/interfaces/include/interfaces/IModbusClient.h new file mode 100644 index 0000000..1bc6991 --- /dev/null +++ b/firmware/components/interfaces/include/interfaces/IModbusClient.h @@ -0,0 +1,105 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file IModbusClient.h + * @brief Modbus RTU master interface for the RS485 sensor bus. + * + * Ported from the frozen Arduino firmware (include/communication/ + * IModbusClient.h) with the same method surface; Arduino-era base-class + * baggage is trimmed. Normative contract: + * specs/004-modbus-soil-sensor/contracts/interfaces.md. + * + * Error codes reported by getLastError() + * (specs/004-modbus-soil-sensor/data-model.md): + * + * 0 OK + * 1 not initialized + * 2 bus/communication error (CRC, framing, truncated response) + * 3 timeout (no response within the configured timeout) + * 5 range validation failed — set by the sensor layer on top of this + * interface, never by IModbusClient implementations themselves + * 100+n Modbus slave exception n + * + * Part of the header-only `interfaces` component: no IDF includes allowed. + */ + +#ifndef WATERINGSYSTEM_INTERFACES_IMODBUSCLIENT_H +#define WATERINGSYSTEM_INTERFACES_IMODBUSCLIENT_H + +#include + +/** + * @brief Modbus RTU master: single-attempt register reads/writes. + */ +class IModbusClient { +public: + virtual ~IModbusClient() = default; + + /** + * @brief Bring up the bus (UART/transceiver). Must precede any transfer. + * + * @return true on success; false leaves the client unusable (subsequent + * transfers fail with error 1). + */ + virtual bool initialize() = 0; + + /** + * @brief Read holding registers (Modbus function 0x03). + * + * Performs exactly ONE bus attempt — no internal retry (parity, + * docs/parity-checklist.md §5); recovery comes from the caller's read + * cadence. On failure returns false and getLastError() carries the code. + * + * @param deviceAddress Modbus slave address. + * @param startRegister First holding register address. + * @param count Number of consecutive registers to read. + * @param buffer Caller-owned array of at least `count` elements; filled + * only on success. + * @return true if all requested registers were read. + */ + virtual bool readHoldingRegisters(uint8_t deviceAddress, uint16_t startRegister, + uint16_t count, uint16_t* buffer) = 0; + + /** + * @brief Write a single holding register (Modbus function 0x06). + * + * Exactly one bus attempt, no retry (parity). Success requires the + * slave's echo response to be received and verified — a sent-but-not- + * acknowledged write reports false. + * + * @param deviceAddress Modbus slave address. + * @param registerAddress Holding register address. + * @param value Value to write. + * @return true if the write was echoed back by the slave. + */ + virtual bool writeSingleRegister(uint8_t deviceAddress, uint16_t registerAddress, + uint16_t value) = 0; + + /** + * @brief Error code of the most recent operation (0 = OK; table above). + */ + virtual int getLastError() = 0; + + /** + * @brief Set the response timeout for subsequent transfers. + * + * Parity default is 3000 ms (docs/parity-checklist.md §5). + * + * @param timeoutMs Timeout in milliseconds. + */ + virtual void setTimeout(uint32_t timeoutMs) = 0; + + /** + * @brief Cumulative transaction statistics. + * + * Every readHoldingRegisters()/writeSingleRegister() call increments + * exactly one of the two counters; reading the statistics increments + * neither. + * + * @param successCount Out: number of successful transfers. + * @param errorCount Out: number of failed transfers. + */ + virtual void getStatistics(uint32_t* successCount, uint32_t* errorCount) = 0; +}; + +#endif /* WATERINGSYSTEM_INTERFACES_IMODBUSCLIENT_H */ diff --git a/firmware/components/interfaces/include/interfaces/ISoilSensor.h b/firmware/components/interfaces/include/interfaces/ISoilSensor.h new file mode 100644 index 0000000..47e7e05 --- /dev/null +++ b/firmware/components/interfaces/include/interfaces/ISoilSensor.h @@ -0,0 +1,125 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file ISoilSensor.h + * @brief Interface for the RS485 Modbus soil sensor (7 quantities). + * + * Ported from the frozen Arduino firmware (include/sensors/ISoilSensor.h) + * with one deliberate trim: legacy setValidRange/isWithinValidRange are NOT + * ported — validation ranges are the fixed parity constants (moisture + * 0–100 %, temperature −40–80 °C, pH 3–9) and no caller in the legacy + * firmware ever changed them at runtime. Normative contract: + * specs/004-modbus-soil-sensor/contracts/interfaces.md; register map, + * scaling and error codes: specs/004-modbus-soil-sensor/data-model.md. + * + * Validity contract (FR-005): 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 (main loop + console REPL) wrap them in the LockedSoilSensor + * decorator, same pattern as LockedWaterPump/LockedConfigStore. + * + * Part of the header-only `interfaces` component: no IDF includes allowed. + */ + +#ifndef WATERINGSYSTEM_INTERFACES_ISOILSENSOR_H +#define WATERINGSYSTEM_INTERFACES_ISOILSENSOR_H + +/** + * @brief Soil sensor: atomic multi-quantity reads with parity validation. + */ +class ISoilSensor { +public: + virtual ~ISoilSensor() = default; + + /** + * @brief Prepare the sensor (brings up the underlying Modbus client if + * needed). Must precede read()/isAvailable(). + */ + virtual bool initialize() = 0; + + /** + * @brief Read all quantities in ONE 9-register transaction + * (registers 0x0000–0x0008), single bus attempt, no retry. + * + * The reading is atomic: either every value is decoded, scaled and + * range-validated and the getters are refreshed, or the call fails and + * the last-good getter values remain untouched. false means the data is + * INVALID (bus error, timeout, exception or range validation failure — + * see getLastError()); consumers gate on this result, never on how + * plausible the getter values look. + * + * @return true if a fully valid reading was taken. + */ + virtual bool read() = 0; + + /** + * @brief Probe sensor presence with a REAL 1-register bus read (parity). + * + * Every call performs an actual bus transaction — never a cached or + * derived state. Recovery from earlier failures is implicit: a sensor + * that responds again is available again. + */ + virtual bool isAvailable() = 0; + + /** + * @brief Error code of the most recent operation (0 = OK). + * + * Same table as IModbusClient plus code 5 (range validation failed); + * see specs/004-modbus-soil-sensor/data-model.md. + */ + virtual int getLastError() = 0; + + // Values from the most recent successful read(). After a failed read() + // they still hold the previous good reading — check the read() result + // before treating them as fresh. + + /// Soil moisture in percent (0–100). + virtual float getMoisture() = 0; + + /// Soil temperature in °C, signed (−40–80). + virtual float getTemperature() = 0; + + /** + * @brief Humidity in percent — identical to getMoisture() (parity). + * + * The sensor reports a single moisture/humidity quantity in register + * 0x0000; the legacy firmware exposed it under both names. + */ + virtual float getHumidity() = 0; + + /// Soil pH (validated 3–9). + virtual float getPH() = 0; + + /// Electrical conductivity in µS/cm (not range-enforced, parity). + virtual float getEC() = 0; + + /// Nitrogen content in mg/kg (not range-enforced, parity). + virtual float getNitrogen() = 0; + + /// Phosphorus content in mg/kg (not range-enforced, parity). + virtual float getPhosphorus() = 0; + + /// Potassium content in mg/kg (not range-enforced, parity). + virtual float getPotassium() = 0; + + // Calibration (CP1 decision A — legacy semantics). Each call computes a + // local correction factor (reference / current raw reading), applies it + // to every subsequent read, and best-effort writes the factor to the + // sensor's calibration register (0x0100/0x0101/0x0102) — a failed + // sensor-register write is NON-FATAL and does not fail the call. + // Factors are RAM-only in this PR (persistence arrives in PR-09/PR-11). + + /// Calibrate moisture against a reference value in percent. + virtual bool calibrateMoisture(float referenceValue) = 0; + + /// Calibrate pH against a reference value. + virtual bool calibratePH(float referenceValue) = 0; + + /// Calibrate EC against a reference value in µS/cm. + virtual bool calibrateEC(float referenceValue) = 0; +}; + +#endif /* WATERINGSYSTEM_INTERFACES_ISOILSENSOR_H */ diff --git a/firmware/components/sensors/CMakeLists.txt b/firmware/components/sensors/CMakeLists.txt new file mode 100644 index 0000000..224f082 --- /dev/null +++ b/firmware/components/sensors/CMakeLists.txt @@ -0,0 +1,28 @@ +# sensors — Modbus soil sensor driver layer. +# +# 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). +if(${IDF_TARGET} STREQUAL "linux") + idf_component_register( + SRCS "src/ModbusSoilSensor.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). + idf_component_register( + SRCS "src/ModbusSoilSensor.cpp" "src/EspModbusClient.cpp" + INCLUDE_DIRS "include" + REQUIRES interfaces board + PRIV_REQUIRES espressif__esp-modbus esp_driver_uart esp_driver_gpio + ) +endif() diff --git a/firmware/components/sensors/idf_component.yml b/firmware/components/sensors/idf_component.yml new file mode 100644 index 0000000..1c15502 --- /dev/null +++ b/firmware/components/sensors/idf_component.yml @@ -0,0 +1,12 @@ +# Managed dependencies of the sensors component, pinned exactly for +# reproducible builds (constitution III). +# +# esp-modbus is target-only: EspModbusClient.cpp is excluded from the linux +# host build (see CMakeLists.txt), so the dependency is rule-gated to keep +# esp-modbus out of the host-test dependency graph. +dependencies: + idf: ">=6.0.0" + espressif/esp-modbus: + version: "==2.1.2" + rules: + - if: "target != linux" diff --git a/firmware/components/sensors/include/sensors/testing/MockModbusClient.h b/firmware/components/sensors/include/sensors/testing/MockModbusClient.h new file mode 100644 index 0000000..90e6ab4 --- /dev/null +++ b/firmware/components/sensors/include/sensors/testing/MockModbusClient.h @@ -0,0 +1,217 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file MockModbusClient.h + * @brief Scriptable IModbusClient test double (header-only). + * + * Serves the ModbusSoilSensor host tests: script register payloads per + * (address, startRegister, count), queue per-call outcomes (success, + * timeout, bus error, slave exception) for fail-then-recover scenarios, + * and assert on the recorded call log (no-retry invariant, real-read + * availability probe, calibration register writes) and on the + * one-increment-per-call statistics. Never compiled into target builds + * (only included from test code). No IDF includes. + */ + +#ifndef WATERINGSYSTEM_SENSORS_TESTING_MOCKMODBUSCLIENT_H +#define WATERINGSYSTEM_SENSORS_TESTING_MOCKMODBUSCLIENT_H + +#include +#include +#include + +#include "interfaces/IModbusClient.h" + +/** + * @brief IModbusClient over scripted payloads, instrumented for tests. + * + * Outcome selection per read/write call: the front of `outcomeQueue` is + * consumed if non-empty, otherwise `defaultOutcome` applies (error-code + * convention: kOk = success). Reads that succeed return the payload + * scripted for their exact (address, startRegister, count) key, else the + * default payload, else all-zero registers. + */ +class MockModbusClient : public IModbusClient { +public: + // Error codes (data-model.md table) as readable test constants. + static constexpr int kOk = 0; + static constexpr int kErrNotInitialized = 1; + static constexpr int kErrBus = 2; ///< CRC/framing/truncated + static constexpr int kErrTimeout = 3; + static constexpr int kErrExceptionBase = 100; ///< 100 + n for slave exception n + + /// One recorded bus call (in `calls`, chronological). + struct Call { + enum class Type { Read, Write }; + Type type; + uint8_t deviceAddress; + uint16_t startRegister; ///< registerAddress for writes + uint16_t count; ///< register count (reads); 1 for writes + uint16_t value; ///< written value (writes); 0 for reads + bool succeeded; ///< outcome reported to the caller + }; + + // Instrumentation (public, MockConfigStore style). + std::vector calls; ///< every read/write, in call order + std::vector timeoutCalls; ///< every setTimeout() argument + int initializeCalls = 0; + bool initializeResult = true; ///< false: initialize() fails + int defaultOutcome = kOk; ///< used when outcomeQueue is empty + + // -- Scripting -------------------------------------------------------- + + /** + * @brief Script the payload returned for reads matching exactly + * (deviceAddress, startRegister, count == values.size()). + * Re-scripting the same key replaces the previous payload. + */ + void setRegisters(uint8_t deviceAddress, uint16_t startRegister, + std::vector values) + { + for (auto& entry : scripted_) { + if (entry.deviceAddress == deviceAddress && + entry.startRegister == startRegister && + entry.values.size() == values.size()) { + entry.values = std::move(values); + return; + } + } + scripted_.push_back({deviceAddress, startRegister, std::move(values)}); + } + + /** + * @brief Fallback payload for successful reads with no exact-key script; + * the first `count` values are returned (must hold at least `count`, + * shorter defaults fall through to all-zero registers). + */ + void setDefaultRegisters(std::vector values) + { + defaultRegisters_ = std::move(values); + } + + /** + * @brief Queue the outcome for the NEXT read/write call (FIFO). + * + * kOk = success; kErrTimeout, kErrBus, kErrExceptionBase + n, ... force + * that failure. Queue e.g. {kErrTimeout, kOk} for a fail-then-recover + * scenario. When the queue is empty, defaultOutcome applies. + */ + void queueOutcome(int errorCode) { outcomeQueue_.push_back(errorCode); } + + // -- IModbusClient ----------------------------------------------------- + + bool initialize() override + { + ++initializeCalls; + initialized_ = initializeResult; + return initialized_; + } + + bool readHoldingRegisters(uint8_t deviceAddress, uint16_t startRegister, + uint16_t count, uint16_t* buffer) override + { + Call call{Call::Type::Read, deviceAddress, startRegister, count, 0, false}; + if (!initialized_) { + return finish(call, kErrNotInitialized); + } + const int outcome = nextOutcome(); + if (outcome != kOk) { + return finish(call, outcome); + } + fillPayload(deviceAddress, startRegister, count, buffer); + return finish(call, kOk); + } + + bool writeSingleRegister(uint8_t deviceAddress, uint16_t registerAddress, + uint16_t value) override + { + Call call{Call::Type::Write, deviceAddress, registerAddress, 1, value, false}; + if (!initialized_) { + return finish(call, kErrNotInitialized); + } + return finish(call, nextOutcome()); + } + + int getLastError() override { return lastError_; } + + void setTimeout(uint32_t timeoutMs) override + { + timeoutCalls.push_back(timeoutMs); + } + + void getStatistics(uint32_t* successCount, uint32_t* errorCount) override + { + if (successCount != nullptr) { + *successCount = successCount_; + } + if (errorCount != nullptr) { + *errorCount = errorCount_; + } + } + +private: + struct ScriptedRead { + uint8_t deviceAddress; + uint16_t startRegister; + std::vector values; + }; + + int nextOutcome() + { + if (outcomeQueue_.empty()) { + return defaultOutcome; + } + const int outcome = outcomeQueue_.front(); + outcomeQueue_.erase(outcomeQueue_.begin()); + return outcome; + } + + void fillPayload(uint8_t deviceAddress, uint16_t startRegister, + uint16_t count, uint16_t* buffer) const + { + for (const auto& entry : scripted_) { + if (entry.deviceAddress == deviceAddress && + entry.startRegister == startRegister && + entry.values.size() == count) { + for (uint16_t i = 0; i < count; ++i) { + buffer[i] = entry.values[i]; + } + return; + } + } + if (defaultRegisters_.has_value() && defaultRegisters_->size() >= count) { + for (uint16_t i = 0; i < count; ++i) { + buffer[i] = (*defaultRegisters_)[i]; + } + return; + } + for (uint16_t i = 0; i < count; ++i) { + buffer[i] = 0; + } + } + + /// Record the call, apply the one-increment-per-call statistics + /// contract, set lastError, and return the call result. + bool finish(Call& call, int errorCode) + { + call.succeeded = (errorCode == kOk); + calls.push_back(call); + lastError_ = errorCode; + if (call.succeeded) { + ++successCount_; + } else { + ++errorCount_; + } + return call.succeeded; + } + + bool initialized_ = false; + int lastError_ = kOk; + uint32_t successCount_ = 0; + uint32_t errorCount_ = 0; + std::vector outcomeQueue_; + std::vector scripted_; + std::optional> defaultRegisters_; +}; + +#endif /* WATERINGSYSTEM_SENSORS_TESTING_MOCKMODBUSCLIENT_H */ diff --git a/firmware/components/sensors/src/EspModbusClient.cpp b/firmware/components/sensors/src/EspModbusClient.cpp new file mode 100644 index 0000000..125cedc --- /dev/null +++ b/firmware/components/sensors/src/EspModbusClient.cpp @@ -0,0 +1,6 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Placeholder translation unit — EspModbusClient implementation arrives in +// phase 3 (specs/004-modbus-soil-sensor/tasks.md, User Story 1). Target-only: +// excluded from the linux host build in CMakeLists.txt. diff --git a/firmware/components/sensors/src/ModbusSoilSensor.cpp b/firmware/components/sensors/src/ModbusSoilSensor.cpp new file mode 100644 index 0000000..bd5c6e5 --- /dev/null +++ b/firmware/components/sensors/src/ModbusSoilSensor.cpp @@ -0,0 +1,5 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Placeholder translation unit — ModbusSoilSensor implementation arrives in +// phase 3 (specs/004-modbus-soil-sensor/tasks.md, User Story 1). From 9e76c6bfe317dfda9a0514ad8c14addf14ac43b8 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 11:18:13 +0200 Subject: [PATCH 07/11] =?UTF-8?q?feat(firmware):=20Modbus=20soil=20sensor?= =?UTF-8?q?=20MVP=20=E2=80=94=20driver,=20esp-modbus=20client,=20console?= =?UTF-8?q?=20diagnostics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T006-T013 (US1): pure-logic ModbusSoilSensor ported from legacy with confirmed parity quirks (moisture factor not applied on read; pH/EC validated on factored values; availability probes register 0x0000), EspModbusClient on esp-modbus 2.1.2 with RS485 half-duplex on both boards (RTS=DE on rev1, none on rev2), FW-2 RX pull-up, FW-4 echo note, LockedSoilSensor decorator, app wiring, soil/rs485test console commands, BOARD_RS485_UART_PORT in both board profiles, four decode host tests. Verified: host suite 54/0 on linux target; rev1 board build green; FMB timeout Kconfig range 150-30000 accommodates 3000 ms (T002); dependencies.lock manifest hash updated. Co-Authored-By: Claude Fable 5 --- .../components/board/include/board/board.h | 7 + .../sensors/include/sensors/EspModbusClient.h | 92 ++++++ .../include/sensors/LockedSoilSensor.h | 151 +++++++++ .../include/sensors/ModbusSoilSensor.h | 142 +++++++++ .../sensors/src/EspModbusClient.cpp | 248 ++++++++++++++- .../sensors/src/ModbusSoilSensor.cpp | 286 +++++++++++++++++- firmware/dependencies.lock | 2 +- firmware/main/CMakeLists.txt | 2 +- firmware/main/app_main.cpp | 25 ++ firmware/main/diag_console.cpp | 126 ++++++++ firmware/main/diag_console.h | 14 + firmware/test_apps/host/main/CMakeLists.txt | 3 +- firmware/test_apps/host/main/test_main.cpp | 2 + .../test_apps/host/main/test_soil_sensor.cpp | 127 ++++++++ specs/004-modbus-soil-sensor/data-model.md | 2 +- specs/004-modbus-soil-sensor/tasks.md | 26 +- 16 files changed, 1231 insertions(+), 24 deletions(-) create mode 100644 firmware/components/sensors/include/sensors/EspModbusClient.h create mode 100644 firmware/components/sensors/include/sensors/LockedSoilSensor.h create mode 100644 firmware/components/sensors/include/sensors/ModbusSoilSensor.h create mode 100644 firmware/test_apps/host/main/test_soil_sensor.cpp diff --git a/firmware/components/board/include/board/board.h b/firmware/components/board/include/board/board.h index 087750c..d60bcd4 100644 --- a/firmware/components/board/include/board/board.h +++ b/firmware/components/board/include/board/board.h @@ -39,6 +39,9 @@ #define BOARD_PIN_RS485_RX 17 #define BOARD_HAS_RS485_DE 1 #define BOARD_PIN_RS485_DE 25 +/* Modbus RTU runs on UART2 at 9600 baud 8N1 (parity: legacy Serial2, + * docs/parity-checklist.md §5). */ +#define BOARD_RS485_UART_PORT 2 /* Pumps (MOSFET gates, active high) */ #define BOARD_PIN_MAIN_PUMP 26 @@ -83,6 +86,10 @@ * is 0: any reference that is not guarded by #if BOARD_HAS_RS485_DE becomes * a compile error instead of undefined behavior (e.g. 1ULL << -1) or a * silently dropped ESP_ERR_INVALID_ARG at runtime. */ +/* Modbus RTU runs on UART2 at 9600 baud 8N1 (parity: legacy Serial2, + * docs/parity-checklist.md §5). The UART number is a parity fact, not part + * of the provisional rev2 pin map — no TODO(SYNC1). */ +#define BOARD_RS485_UART_PORT 2 /* Pumps (MOSFET gates, active high) */ #define BOARD_PIN_MAIN_PUMP 26 // TODO(SYNC1): final rev2 pin map frozen at hardware sync 1 diff --git a/firmware/components/sensors/include/sensors/EspModbusClient.h b/firmware/components/sensors/include/sensors/EspModbusClient.h new file mode 100644 index 0000000..1b5d4bf --- /dev/null +++ b/firmware/components/sensors/include/sensors/EspModbusClient.h @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file EspModbusClient.h + * @brief IModbusClient over esp-modbus 2.1.2 (Modbus RTU master, RS485). + * + * ESP32-ONLY: excluded from the linux-target build together with the + * esp-modbus/driver dependencies (see this component's CMakeLists.txt). + * This is the sensors component's single hardware touchpoint — all + * business logic lives above IModbusClient (research.md R7). + * + * PRIV rule: esp-modbus and driver headers appear only in the .cpp, never + * here — the esp-modbus master handle is held as an opaque pointer. + * + * UART port and pins come from board/board.h inside the .cpp; the + * `#if BOARD_HAS_RS485_DE` direction-control difference between rev1 + * (RTS-driven DE) and rev2 (THVD1426 auto-direction) lives in exactly one + * place there (research.md R2). + * + * Concurrency: unsynchronized, like every base implementation in this + * codebase. In this PR the client is only reached from the console REPL + * task (directly by `rs485test` and through LockedSoilSensor by `soil`); + * a second consumer task would need a locking wrapper. + */ + +#ifndef WATERINGSYSTEM_SENSORS_ESPMODBUSCLIENT_H +#define WATERINGSYSTEM_SENSORS_ESPMODBUSCLIENT_H + +#include + +#include "interfaces/IModbusClient.h" + +/** + * @brief Modbus RTU master on the board's RS485 UART (9600 8N1, parity). + */ +class EspModbusClient : public IModbusClient { +public: + /// Parity response timeout (docs/parity-checklist.md §5). + static constexpr uint32_t kDefaultTimeoutMs = 3000; + + EspModbusClient() = default; + + /// Tears down the esp-modbus master stack (mbc_master_delete). + ~EspModbusClient() override; + + EspModbusClient(const EspModbusClient&) = delete; + EspModbusClient& operator=(const EspModbusClient&) = delete; + + /** + * @brief Bring up the esp-modbus master on the RS485 UART. + * + * create → uart_set_pin (RTS = DE pin iff BOARD_HAS_RS485_DE) → start → + * uart_set_mode(RS485 half-duplex) → RX pull-up (FW-2). Idempotent; + * on failure every partially-created resource is torn down again. + */ + bool initialize() override; + + bool readHoldingRegisters(uint8_t deviceAddress, uint16_t startRegister, + uint16_t count, uint16_t* buffer) override; + + bool writeSingleRegister(uint8_t deviceAddress, uint16_t registerAddress, + uint16_t value) override; + + int getLastError() override; + + /** + * @brief Set the response timeout for subsequent transfers. + * + * esp-modbus 2.1.2 exposes no documented runtime timeout setter, so the + * value is applied at initialize() time only (research.md R5, risk + * documented): calls after initialize() are stored but take effect only + * on a future re-initialization. No caller changes the timeout at + * runtime today. + */ + void setTimeout(uint32_t timeoutMs) override; + + void getStatistics(uint32_t* successCount, uint32_t* errorCount) override; + +private: + /// One bus transaction via mbc_master_send_request + bookkeeping. + bool sendRequest(uint8_t deviceAddress, uint8_t command, + uint16_t registerAddress, uint16_t count, void* data); + + void* mbHandle_ = nullptr; ///< opaque esp-modbus master handle + bool initialized_ = false; + int lastError_ = 0; + uint32_t timeoutMs_ = kDefaultTimeoutMs; + uint32_t successCount_ = 0; + uint32_t errorCount_ = 0; +}; + +#endif /* WATERINGSYSTEM_SENSORS_ESPMODBUSCLIENT_H */ diff --git a/firmware/components/sensors/include/sensors/LockedSoilSensor.h b/firmware/components/sensors/include/sensors/LockedSoilSensor.h new file mode 100644 index 0000000..55662f5 --- /dev/null +++ b/firmware/components/sensors/include/sensors/LockedSoilSensor.h @@ -0,0 +1,151 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file LockedSoilSensor.h + * @brief Mutex-serializing ISoilSensor decorator (header-only). + * + * WHY THIS EXISTS: the soil sensor will be reached from more than one + * FreeRTOS task — the diag console REPL task issues soil/calibration + * commands while application tasks (watering controller in PR-11) run + * periodic reads. ModbusSoilSensor itself is deliberately unsynchronized + * pure C++ (host-testable), so its plain float members would race: read() + * publishes eight values member-by-member, and a concurrent getter could + * observe a half-published reading (fresh moisture with stale pH). This + * decorator wraps an ISoilSensor and takes a mutex around every interface + * call, serializing all access (pattern of actuators/LockedWaterPump.h and + * storage/LockedConfigStore.h). + * + * USAGE RULE: once a sensor is wrapped, the underlying sensor must ONLY be + * accessed through the wrapper — every call site (boot wiring, console + * registration, controllers, ...) goes through the LockedSoilSensor, never + * through the wrapped object directly. + * + * SCOPE: this decorator provides PER-CALL atomicity only, not cross-call. + * A read-then-get sequence spanning two calls (read() then getMoisture()) + * is NOT protected against an interleaving read() from another task in + * between — another task may refresh (or invalidate) the values first. + * Such sequences need higher-level coordination (a caller-held lock or + * single-owner task). + * + * Pure C++ ( is available via pthread on ESP-IDF and on the linux + * preview target), so the decorator is host-testable. + */ + +#ifndef WATERINGSYSTEM_SENSORS_LOCKEDSOILSENSOR_H +#define WATERINGSYSTEM_SENSORS_LOCKEDSOILSENSOR_H + +#include + +#include "interfaces/ISoilSensor.h" + +/** + * @brief ISoilSensor 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 LockedSoilSensor : public ISoilSensor { +public: + /// Wrap @p sensor; the wrapped sensor must outlive this object. + explicit LockedSoilSensor(ISoilSensor& sensor) : sensor_(sensor) {} + + LockedSoilSensor(const LockedSoilSensor&) = delete; + LockedSoilSensor& operator=(const LockedSoilSensor&) = 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 getMoisture() override + { + std::lock_guard lock(mutex_); + return sensor_.getMoisture(); + } + + float getTemperature() override + { + std::lock_guard lock(mutex_); + return sensor_.getTemperature(); + } + + float getHumidity() override + { + std::lock_guard lock(mutex_); + return sensor_.getHumidity(); + } + + float getPH() override + { + std::lock_guard lock(mutex_); + return sensor_.getPH(); + } + + float getEC() override + { + std::lock_guard lock(mutex_); + return sensor_.getEC(); + } + + float getNitrogen() override + { + std::lock_guard lock(mutex_); + return sensor_.getNitrogen(); + } + + float getPhosphorus() override + { + std::lock_guard lock(mutex_); + return sensor_.getPhosphorus(); + } + + float getPotassium() override + { + std::lock_guard lock(mutex_); + return sensor_.getPotassium(); + } + + bool calibrateMoisture(float referenceValue) override + { + std::lock_guard lock(mutex_); + return sensor_.calibrateMoisture(referenceValue); + } + + bool calibratePH(float referenceValue) override + { + std::lock_guard lock(mutex_); + return sensor_.calibratePH(referenceValue); + } + + bool calibrateEC(float referenceValue) override + { + std::lock_guard lock(mutex_); + return sensor_.calibrateEC(referenceValue); + } + +private: + ISoilSensor& sensor_; + mutable std::mutex mutex_; +}; + +#endif /* WATERINGSYSTEM_SENSORS_LOCKEDSOILSENSOR_H */ diff --git a/firmware/components/sensors/include/sensors/ModbusSoilSensor.h b/firmware/components/sensors/include/sensors/ModbusSoilSensor.h new file mode 100644 index 0000000..60729e9 --- /dev/null +++ b/firmware/components/sensors/include/sensors/ModbusSoilSensor.h @@ -0,0 +1,142 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file ModbusSoilSensor.h + * @brief Pure C++ soil sensor logic over an injected IModbusClient. + * + * Ported from the frozen Arduino firmware (src/sensors/ModbusSoilSensor.cpp, + * read-only reference): register map, decode/scaling (incl. signed + * temperature), fixed parity validation ranges, real-bus-read availability + * probe and the legacy calibration semantics. Register map, scaling and + * error codes are normative in specs/004-modbus-soil-sensor/data-model.md. + * + * This class contains NO hardware access — every bus transaction goes + * through the injected IModbusClient, so the decode/validation/calibration + * logic is compiled and unit-tested on the IDF linux preview target against + * MockModbusClient (constitution II). + * + * Concurrency: unsynchronized by design (host-testable); cross-task + * consumers (console REPL + future main-loop controller) wrap it in + * LockedSoilSensor and access it only through the wrapper. + */ + +#ifndef WATERINGSYSTEM_SENSORS_MODBUSSOILSENSOR_H +#define WATERINGSYSTEM_SENSORS_MODBUSSOILSENSOR_H + +#include + +#include "interfaces/IModbusClient.h" +#include "interfaces/ISoilSensor.h" + +/** + * @brief ISoilSensor over Modbus RTU holding registers (data-model.md). + * + * One read() = one 9-register transaction (0x0000–0x0008), decoded, scaled + * and range-validated atomically: on any failure the last-good getter + * values remain untouched and getLastError() carries the cause. + */ +class ModbusSoilSensor : public ISoilSensor { +public: + /// Parity slave address (docs/parity-checklist.md §5). + static constexpr uint8_t kDefaultDeviceAddress = 0x01; + + /** + * @brief Construct the sensor over an injected Modbus client. + * + * @param client Modbus master used for every transaction; must outlive + * this object (same injection style as WaterPump's + * ITimeProvider). + * @param deviceAddress Modbus slave address of the sensor. + */ + explicit ModbusSoilSensor(IModbusClient& client, + uint8_t deviceAddress = kDefaultDeviceAddress); + + ~ModbusSoilSensor() override = default; + + ModbusSoilSensor(const ModbusSoilSensor&) = delete; + ModbusSoilSensor& operator=(const ModbusSoilSensor&) = delete; + + // ISoilSensor + bool initialize() override; + bool read() override; + bool isAvailable() override; + int getLastError() override; + + float getMoisture() override; + float getTemperature() override; + float getHumidity() override; + float getPH() override; + float getEC() override; + float getNitrogen() override; + float getPhosphorus() override; + float getPotassium() override; + + // Calibration (legacy-exact port, research.md R8); host tests for these + // paths land in phase 6 (tasks.md T021). + bool calibrateMoisture(float referenceValue) override; + bool calibratePH(float referenceValue) override; + bool calibrateEC(float referenceValue) override; + +private: + // Register map (data-model.md; legacy include/sensors/ModbusSoilSensor.h). + static constexpr uint16_t kRegHumidity = 0x0000; ///< 0.1 % (= moisture) + static constexpr uint16_t kRegTemperature = 0x0001; ///< 0.1 °C, signed + static constexpr uint16_t kRegEc = 0x0002; ///< 1 µS/cm + static constexpr uint16_t kRegPh = 0x0003; ///< 0.1 pH + static constexpr uint16_t kRegNitrogen = 0x0004; ///< 1 mg/kg + static constexpr uint16_t kRegPhosphorus = 0x0005; ///< 1 mg/kg + static constexpr uint16_t kRegPotassium = 0x0006; ///< 1 mg/kg + // 0x0007 (salinity) and 0x0008 (TDS) are read but not exposed (parity). + + /// Calibration factor registers (best-effort writes, factor ×100). + static constexpr uint16_t kRegMoistureCalib = 0x0100; + static constexpr uint16_t kRegPhCalib = 0x0101; + static constexpr uint16_t kRegEcCalib = 0x0102; + + /// One transaction covers registers 0x0000–0x0008. + static constexpr uint16_t kReadRegisterCount = 9; + + // Fixed parity validation ranges (the legacy setValidRange defaults; + // runtime range changes were trimmed — see interfaces/ISoilSensor.h). + static constexpr float kMoistureMin = 0.0f; + static constexpr float kMoistureMax = 100.0f; + static constexpr float kTemperatureMin = -40.0f; + static constexpr float kTemperatureMax = 80.0f; + static constexpr float kPhMin = 3.0f; + static constexpr float kPhMax = 9.0f; + + /** + * @brief Shared legacy calibration flow (legacy :207-322, three + * near-identical bodies folded into one). + * + * Reads one raw register, computes factor = reference / (raw / rawScale), + * stores it into @p factor and best-effort writes factor ×100 to + * @p calibRegister (a failed write is NON-FATAL: logged, lastError set, + * call still succeeds — parity). + */ + bool calibrate(uint16_t rawRegister, float rawScale, + uint16_t calibRegister, float& factor, + float referenceValue, const char* quantity); + + IModbusClient& client_; + uint8_t deviceAddress_; + bool initialized_ = false; + int lastError_ = 0; + + // Last-good reading (published only by a fully successful read()). + float moisture_ = 0.0f; + float temperature_ = 0.0f; + float humidity_ = 0.0f; + float ph_ = 0.0f; + float ec_ = 0.0f; + float nitrogen_ = 0.0f; + float phosphorus_ = 0.0f; + float potassium_ = 0.0f; + + // Calibration factors (RAM-only in this PR; persistence PR-09/PR-11). + float moistureCalibrationFactor_ = 1.0f; + float phCalibrationFactor_ = 1.0f; + float ecCalibrationFactor_ = 1.0f; +}; + +#endif /* WATERINGSYSTEM_SENSORS_MODBUSSOILSENSOR_H */ diff --git a/firmware/components/sensors/src/EspModbusClient.cpp b/firmware/components/sensors/src/EspModbusClient.cpp index 125cedc..8c455cf 100644 --- a/firmware/components/sensors/src/EspModbusClient.cpp +++ b/firmware/components/sensors/src/EspModbusClient.cpp @@ -1,6 +1,246 @@ // SPDX-FileCopyrightText: 2026 Cryptotomte // SPDX-License-Identifier: AGPL-3.0-or-later -// -// Placeholder translation unit — EspModbusClient implementation arrives in -// phase 3 (specs/004-modbus-soil-sensor/tasks.md, User Story 1). Target-only: -// excluded from the linux host build in CMakeLists.txt. +/** + * @file EspModbusClient.cpp + * @brief esp-modbus 2.1.2 Modbus RTU master implementation. + * + * Target-only translation unit (excluded from the linux host build). The + * only file in the sensors component that touches esp-modbus, UART and + * GPIO APIs. Setup sequence and RS485 half-duplex rationale: research.md + * R1/R2/R3/R4. + */ + +#include "sensors/EspModbusClient.h" + +#include "board/board.h" +#include "driver/gpio.h" +#include "driver/uart.h" +#include "esp_log.h" + +// esp-modbus umbrella header (managed component, pinned ==2.1.2). +#include "mbcontroller.h" + +static const char *TAG = "esp_modbus_client"; + +namespace { + +/** + * @brief Map an esp-modbus/esp_err_t result onto the IModbusClient error + * table (data-model.md, research.md R6). + * + * Kept as ONE helper so the mapping can be refined in a single place. + * + * parity divergence R6: esp-modbus 2.1.2's mbc_master_send_request does not + * surface the Modbus slave exception code to the caller — exceptions and + * other invalid responses come back as generic esp_err_t failures. The + * legacy 100+n exception granularity is therefore collapsed onto error 2 + * (bus/communication error) here. The binding FR-010 requirement — distinct + * from timeout — still holds (timeout maps to 3). + */ +int map_esp_err(esp_err_t err) +{ + switch (err) { + case ESP_OK: + return 0; + case ESP_ERR_TIMEOUT: + return 3; // No response within the configured timeout. + case ESP_ERR_INVALID_STATE: + return 1; // Stack not initialized/started. + default: + return 2; // CRC/framing/invalid response/slave exception class. + } +} + +} // namespace + +EspModbusClient::~EspModbusClient() +{ + if (mbHandle_ != nullptr) { + const esp_err_t err = mbc_master_delete(mbHandle_); + if (err != ESP_OK) { + ESP_LOGE(TAG, "mbc_master_delete failed: %s", + esp_err_to_name(err)); + } + mbHandle_ = nullptr; + } +} + +bool EspModbusClient::initialize() +{ + if (initialized_) { + return true; + } + + // R1: esp-modbus 2.x serial master, Modbus RTU 9600 8N1, parity + // response timeout (3000 ms default, member set via setTimeout()). + mb_communication_info_t comm_info = {}; + comm_info.ser_opts.port = static_cast(BOARD_RS485_UART_PORT); + comm_info.ser_opts.mode = MB_RTU; + comm_info.ser_opts.baudrate = 9600; + comm_info.ser_opts.data_bits = UART_DATA_8_BITS; + comm_info.ser_opts.stop_bits = UART_STOP_BITS_1; + comm_info.ser_opts.parity = MB_PARITY_NONE; + comm_info.ser_opts.uid = 0; // master + comm_info.ser_opts.response_tout_ms = timeoutMs_; + + esp_err_t err = mbc_master_create_serial(&comm_info, &mbHandle_); + if (err != ESP_OK || mbHandle_ == nullptr) { + ESP_LOGE(TAG, "mbc_master_create_serial failed: %s", + esp_err_to_name(err)); + mbHandle_ = nullptr; + lastError_ = 1; + return false; + } + + // R2: UART pins from board.h. rev1 drives the transceiver DE via the + // UART RTS line (hardware-timed around each frame); rev2 has no DE pin + // (THVD1426 auto-direction) so RTS stays unrouted. This #if is the one + // place the board direction-control difference exists. + err = uart_set_pin(static_cast(BOARD_RS485_UART_PORT), + BOARD_PIN_RS485_TX, BOARD_PIN_RS485_RX, +#if BOARD_HAS_RS485_DE + BOARD_PIN_RS485_DE, +#else + UART_PIN_NO_CHANGE, +#endif + UART_PIN_NO_CHANGE); + if (err != ESP_OK) { + ESP_LOGE(TAG, "uart_set_pin failed: %s", esp_err_to_name(err)); + mbc_master_delete(mbHandle_); + mbHandle_ = nullptr; + lastError_ = 1; + return false; + } + + err = mbc_master_start(mbHandle_); + if (err != ESP_OK) { + ESP_LOGE(TAG, "mbc_master_start failed: %s", esp_err_to_name(err)); + mbc_master_delete(mbHandle_); + mbHandle_ = nullptr; + lastError_ = 1; + return false; + } + + // R2/R3: RS485 half-duplex mode on BOTH boards. Besides the automatic + // RTS/DE framing on rev1, the TX-gated receive path suppresses the rev2 + // THVD1426 TX echo (RE̅ grounded, receiver always on): echo bytes are + // physically simultaneous with transmission and never reach the driver. + // The esp-modbus RTU T3.5 frame resynchronization is the fallback for + // residual tail bytes. Electrically verified on rev2 at PR-14. + err = uart_set_mode(static_cast(BOARD_RS485_UART_PORT), + UART_MODE_RS485_HALF_DUPLEX); + if (err != ESP_OK) { + ESP_LOGE(TAG, "uart_set_mode(RS485 half-duplex) failed: %s", + esp_err_to_name(err)); + mbc_master_delete(mbHandle_); + mbHandle_ = nullptr; + lastError_ = 1; + return false; + } + + // R4 (FW-2): internal pull-up on the RX pin, unconditionally on both + // boards. On rev2 the THVD1426 SHDN̅ tracks SENS_PWR_EN — RO goes hi-Z + // when the sensor power domain is off and the RX GPIO would otherwise + // float into the UART, producing garbage bytes. Harmless on rev1 + // (ADM3485 RO drives push-pull). + err = gpio_pullup_en(static_cast(BOARD_PIN_RS485_RX)); + if (err != ESP_OK) { + ESP_LOGE(TAG, "gpio_pullup_en(RX) failed: %s", esp_err_to_name(err)); + mbc_master_delete(mbHandle_); + mbHandle_ = nullptr; + lastError_ = 1; + return false; + } + + ESP_LOGI(TAG, + "Modbus RTU master up: UART%d 9600 8N1, timeout %lu ms, " + "RS485 half-duplex", + BOARD_RS485_UART_PORT, static_cast(timeoutMs_)); + initialized_ = true; + lastError_ = 0; + return true; +} + +bool EspModbusClient::sendRequest(uint8_t deviceAddress, uint8_t command, + uint16_t registerAddress, uint16_t count, + void* data) +{ + // Exactly one bus attempt per call, exactly one counter increment + // (interface contract; no retry — parity). + if (!initialized_) { + lastError_ = 1; + ++errorCount_; + return false; + } + + mb_param_request_t request = {}; + request.slave_addr = deviceAddress; + request.command = command; + request.reg_start = registerAddress; + request.reg_size = count; + + const esp_err_t err = mbc_master_send_request(mbHandle_, &request, data); + lastError_ = map_esp_err(err); + if (err != ESP_OK) { + ++errorCount_; + ESP_LOGW(TAG, + "request failed: cmd=0x%02x addr=%u reg=0x%04x n=%u: %s " + "(error %d)", + static_cast(command), + static_cast(deviceAddress), + static_cast(registerAddress), + static_cast(count), esp_err_to_name(err), + lastError_); + return false; + } + ++successCount_; + return true; +} + +bool EspModbusClient::readHoldingRegisters(uint8_t deviceAddress, + uint16_t startRegister, + uint16_t count, uint16_t* buffer) +{ + // Modbus function 0x03: esp-modbus fills the caller buffer with the + // register values only on success. + return sendRequest(deviceAddress, 0x03, startRegister, count, buffer); +} + +bool EspModbusClient::writeSingleRegister(uint8_t deviceAddress, + uint16_t registerAddress, + uint16_t value) +{ + // Modbus function 0x06: the esp-modbus master verifies the slave's + // echo response internally — ESP_OK means the write was acknowledged. + uint16_t writeValue = value; + return sendRequest(deviceAddress, 0x06, registerAddress, 1, &writeValue); +} + +int EspModbusClient::getLastError() +{ + return lastError_; +} + +void EspModbusClient::setTimeout(uint32_t timeoutMs) +{ + timeoutMs_ = timeoutMs; + if (initialized_) { + // R5: no documented runtime timeout setter in esp-modbus 2.1.2 — + // the stored value takes effect on a future re-initialization only. + ESP_LOGW(TAG, + "setTimeout(%lu) after initialize(): applied at next " + "initialization only", + static_cast(timeoutMs)); + } +} + +void EspModbusClient::getStatistics(uint32_t* successCount, + uint32_t* errorCount) +{ + if (successCount != nullptr) { + *successCount = successCount_; + } + if (errorCount != nullptr) { + *errorCount = errorCount_; + } +} diff --git a/firmware/components/sensors/src/ModbusSoilSensor.cpp b/firmware/components/sensors/src/ModbusSoilSensor.cpp index bd5c6e5..44aac19 100644 --- a/firmware/components/sensors/src/ModbusSoilSensor.cpp +++ b/firmware/components/sensors/src/ModbusSoilSensor.cpp @@ -1,5 +1,285 @@ // SPDX-FileCopyrightText: 2026 Cryptotomte // SPDX-License-Identifier: AGPL-3.0-or-later -// -// Placeholder translation unit — ModbusSoilSensor implementation arrives in -// phase 3 (specs/004-modbus-soil-sensor/tasks.md, User Story 1). +/** + * @file ModbusSoilSensor.cpp + * @brief Soil sensor decode/validation/calibration 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 actuators/WaterPump.cpp). + * + * Port reference: src/sensors/ModbusSoilSensor.cpp (frozen Arduino + * firmware, read-only). Legacy behaviors confirmed and ported exactly: + * + * - The moisture calibration factor is NOT applied in read(): legacy + * computes/stores/writes it in calibrateMoisture() but read() publishes + * humidity = raw / 10 with the comment "No calibration factor for + * humidity yet", and moisture = humidity. Ported as-is (research.md R8 + * "moisture factor exists in legacy and is ported as-is"; the data-model + * table's "× moisture factor" describes the factor's existence, not a + * legacy read-path multiplication). + * - pH and EC factors ARE applied in read(), and validation runs on the + * FACTORED values (legacy :103-128 validates after multiplying). + * - Validation covers moisture, temperature and pH only; EC/N/P/K are not + * range-enforced in read() (parity — legacy checks exactly moisture, + * temperature, humidity, ph; the humidity check is dropped here because + * humidity ≡ moisture makes it redundant). + * - Lazy initialization: read()/isAvailable()/calibrate*() attempt + * initialize() when not yet initialized (legacy :75-81, :134-137). + * - The availability/initialization probe reads ONE register at 0x0000 + * (legacy REG_HUMIDITY, :65 and :140) — not 0x0001. + * + * Error codes follow the new normative table (data-model.md) instead of the + * legacy ad-hoc codes 4/6..14: client failures propagate the client's error + * (2/3/100+n), range validation failure is 5 (parity), and the legacy + * "raw value too low for calibration" (7/10/13) maps onto 5 as well. + */ + +#include "sensors/ModbusSoilSensor.h" + +#include "esp_log.h" + +static const char *TAG = "soilsensor"; + +ModbusSoilSensor::ModbusSoilSensor(IModbusClient& client, uint8_t deviceAddress) + : client_(client), + deviceAddress_(deviceAddress) +{ +} + +bool ModbusSoilSensor::initialize() +{ + // Idempotent, like the legacy sensor (:49-51). + if (initialized_) { + return true; + } + + if (!client_.initialize()) { + // Legacy code 2 ("Modbus client initialization failed") — coincides + // with the normative table's bus/communication error. + lastError_ = 2; + ESP_LOGE(TAG, "initialize failed: Modbus client init error"); + return false; + } + + // Verify the sensor answers: one real register read (legacy :63-68; + // legacy hardcoded error 3, here the client's actual error code — a + // silent sensor reports 3 = timeout either way). + uint16_t testRegister = 0; + if (!client_.readHoldingRegisters(deviceAddress_, kRegHumidity, 1, + &testRegister)) { + lastError_ = client_.getLastError(); + ESP_LOGW(TAG, "initialize failed: sensor probe error %d", lastError_); + return false; + } + + initialized_ = true; + lastError_ = 0; + return true; +} + +bool ModbusSoilSensor::read() +{ + // Lazy initialization (legacy :77-81). + if (!initialized_ && !initialize()) { + return false; + } + + // One 9-register transaction, single bus attempt (FR-004/parity). + uint16_t registerValues[kReadRegisterCount] = {}; + if (!client_.readHoldingRegisters(deviceAddress_, kRegHumidity, + kReadRegisterCount, registerValues)) { + // Client failure: propagate the client's error code (legacy used + // the ad-hoc code 4) and leave the last-good values untouched. + lastError_ = client_.getLastError(); + ESP_LOGW(TAG, "read failed: bus error %d", lastError_); + return false; + } + + // Decode + scale into locals first — the members are published only + // after validation passes (all-or-nothing, FR-005). + // + // 0x0000 humidity/moisture 0.1 % — NO moisture calibration factor in + // the read path (legacy parity, see file header). + const float humidity = static_cast(registerValues[0]) / 10.0f; + const float moisture = humidity; + // 0x0001 temperature 0.1 °C, SIGNED 16-bit (legacy :98-99). + const float temperature = + static_cast(static_cast(registerValues[1])) / 10.0f; + // 0x0002 EC 1 µS/cm, calibration factor applied (legacy :102-103). + const float ec = + static_cast(registerValues[2]) * ecCalibrationFactor_; + // 0x0003 pH 0.1, calibration factor applied (legacy :106-107). + const float ph = + (static_cast(registerValues[3]) / 10.0f) * phCalibrationFactor_; + // 0x0004–0x0006 N/P/K 1 mg/kg, unscaled. + const float nitrogen = static_cast(registerValues[4]); + const float phosphorus = static_cast(registerValues[5]); + const float potassium = static_cast(registerValues[6]); + // 0x0007 salinity and 0x0008 TDS are read but not exposed (parity). + + // Validate AFTER decode/scaling, on the factored values (legacy + // :121-128 order). Failure publishes nothing. + if (moisture < kMoistureMin || moisture > kMoistureMax || + temperature < kTemperatureMin || temperature > kTemperatureMax || + ph < kPhMin || ph > kPhMax) { + lastError_ = 5; // Range validation failed (parity error 5). + ESP_LOGW(TAG, + "read failed: range validation (moisture=%.1f temp=%.1f " + "ph=%.1f)", + static_cast(moisture), + static_cast(temperature), static_cast(ph)); + return false; + } + + // Publish atomically w.r.t. this object: plain members are fine — + // cross-task exclusion is LockedSoilSensor's job. + humidity_ = humidity; + moisture_ = moisture; + temperature_ = temperature; + ec_ = ec; + ph_ = ph; + nitrogen_ = nitrogen; + phosphorus_ = phosphorus; + potassium_ = potassium; + + lastError_ = 0; + return true; +} + +bool ModbusSoilSensor::isAvailable() +{ + // Lazy initialization doubles as the probe (legacy :136-138). + if (!initialized_) { + return initialize(); + } + + // REAL 1-register bus read on every call, never cached (parity, + // legacy :140 — register 0x0000). Legacy passed nullptr as the buffer; + // the new IModbusClient contract requires a caller-owned buffer. + uint16_t testRegister = 0; + return client_.readHoldingRegisters(deviceAddress_, kRegHumidity, 1, + &testRegister); +} + +int ModbusSoilSensor::getLastError() +{ + return lastError_; +} + +float ModbusSoilSensor::getMoisture() +{ + return moisture_; +} + +float ModbusSoilSensor::getTemperature() +{ + return temperature_; +} + +float ModbusSoilSensor::getHumidity() +{ + // Humidity ≡ moisture for this sensor (register 0x0000, parity). + return humidity_; +} + +float ModbusSoilSensor::getPH() +{ + return ph_; +} + +float ModbusSoilSensor::getEC() +{ + return ec_; +} + +float ModbusSoilSensor::getNitrogen() +{ + return nitrogen_; +} + +float ModbusSoilSensor::getPhosphorus() +{ + return phosphorus_; +} + +float ModbusSoilSensor::getPotassium() +{ + return potassium_; +} + +// Calibration — faithful port of legacy :207-322 (three identical flows +// folded into one helper). Host tests land in phase 6 (tasks.md T021). +bool ModbusSoilSensor::calibrate(uint16_t rawRegister, float rawScale, + uint16_t calibRegister, float& factor, + float referenceValue, const char* quantity) +{ + // Lazy initialization (legacy :209-213). + if (!initialized_ && !initialize()) { + return false; + } + + // Fresh raw reading of the single quantity register (legacy errors + // 6/9/12 — here the client's actual error code). + uint16_t rawRegisterValue = 0; + if (!client_.readHoldingRegisters(deviceAddress_, rawRegister, 1, + &rawRegisterValue)) { + lastError_ = client_.getLastError(); + ESP_LOGW(TAG, "calibrate %s failed: raw read error %d", quantity, + lastError_); + return false; + } + + const float currentRawValue = + static_cast(rawRegisterValue) / rawScale; + + // Avoid division by zero (legacy :226-229; legacy codes 7/10/13 map + // onto the normative range-validation code 5). + if (currentRawValue < 0.01f) { + lastError_ = 5; + ESP_LOGW(TAG, "calibrate %s failed: raw value too low (%.3f)", + quantity, static_cast(currentRawValue)); + return false; + } + + factor = referenceValue / currentRawValue; + + // Best-effort factor write (×100) to the sensor's calibration register + // — NON-FATAL on failure: the factor is still used locally (parity, + // legacy :234-241; legacy codes 8/11/14 map onto the client's error). + const uint16_t calibFactorRegValue = + static_cast(factor * 100.0f); + if (!client_.writeSingleRegister(deviceAddress_, calibRegister, + calibFactorRegValue)) { + lastError_ = client_.getLastError(); + ESP_LOGW(TAG, + "calibrate %s: sensor register write failed (error %d), " + "factor %.3f kept locally", + quantity, lastError_, static_cast(factor)); + } else { + lastError_ = 0; + } + + ESP_LOGI(TAG, "calibrate %s: factor=%.3f", quantity, + static_cast(factor)); + return true; +} + +bool ModbusSoilSensor::calibrateMoisture(float referenceValue) +{ + return calibrate(kRegHumidity, 10.0f, kRegMoistureCalib, + moistureCalibrationFactor_, referenceValue, "moisture"); +} + +bool ModbusSoilSensor::calibratePH(float referenceValue) +{ + return calibrate(kRegPh, 10.0f, kRegPhCalib, phCalibrationFactor_, + referenceValue, "ph"); +} + +bool ModbusSoilSensor::calibrateEC(float referenceValue) +{ + // EC raw value is unscaled (legacy :300-301) — rawScale 1. + return calibrate(kRegEc, 1.0f, kRegEcCalib, ecCalibrationFactor_, + referenceValue, "ec"); +} diff --git a/firmware/dependencies.lock b/firmware/dependencies.lock index d9bed83..01808f4 100644 --- a/firmware/dependencies.lock +++ b/firmware/dependencies.lock @@ -27,6 +27,6 @@ direct_dependencies: - espressif/esp-modbus - idf - joltwallet/littlefs -manifest_hash: e580dfb7518fc7d4915da3d29c857a79126d5584a4d47fdbbb6abf4a64e8b96e +manifest_hash: a75946f2281bc7a0ba1812f2e56a4b4bed5255f5aeae0f1f46c455ecd24902e7 target: esp32 version: 3.0.0 diff --git a/firmware/main/CMakeLists.txt b/firmware/main/CMakeLists.txt index 6aac901..97ff861 100644 --- a/firmware/main/CMakeLists.txt +++ b/firmware/main/CMakeLists.txt @@ -2,7 +2,7 @@ idf_component_register( SRCS "app_main.cpp" "diag_console.cpp" PRIV_REQUIRES board esp_driver_gpio esp_app_format actuators interfaces console esp_timer - storage nvs_flash + storage nvs_flash sensors ) # Build-time littlefs image of the committed seed directory diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index 687addc..f86ace3 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -30,6 +30,9 @@ #include "actuators/EspTimeProvider.h" #include "actuators/GpioWaterPump.h" #include "actuators/LockedWaterPump.h" +#include "sensors/EspModbusClient.h" +#include "sensors/LockedSoilSensor.h" +#include "sensors/ModbusSoilSensor.h" #include "storage/LittleFsDataStorage.h" #include "storage/LockedConfigStore.h" #include "storage/LockedDataStorage.h" @@ -170,9 +173,31 @@ extern "C" void app_main(void) static_cast(stats.usedBytes / 1024), static_cast(stats.totalBytes / 1024)); + // RS485 Modbus soil sensor (feature 004). Not safety-critical: a failed + // client init is logged and the system keeps running — the sensor layer + // reports invalid data and recovers on later attempts (US2 semantics). + // Function-local statics after pumps_force_off() (boot fail-safe rule), + // sensor wrapped in the mutex-serializing decorator: accessed from the + // console REPL task now and the main-loop controller in PR-11, so EVERY + // sensor access goes through the wrapper. No periodic read task in this + // PR (PR-11) — reads happen on console command only. + static EspModbusClient modbus_client; + static ModbusSoilSensor soil_sensor_raw(modbus_client); + static LockedSoilSensor soil_sensor(soil_sensor_raw); + + if (modbus_client.initialize()) { + ESP_LOGI(TAG, "RS485 Modbus client up (UART%d)", + BOARD_RS485_UART_PORT); + } else { + ESP_LOGE(TAG, "RS485 Modbus client init failed (error %d) — " + "soil sensor unavailable until recovery", + modbus_client.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); esp_err_t err = diag_console_start(); if (err != ESP_OK) { // Console is a diagnostic aid, not a safety function: log and keep diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index c1f3c39..4d4c66d 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -27,6 +27,13 @@ * storage event # category = u8 (1..255) * storage events [n] # newest-first, default 10 * + * Soil sensor commands (HIL verification path for feature 004; console + * contract in specs/004-modbus-soil-sensor/contracts/interfaces.md — + * calibration commands arrive in a later phase of that feature): + * + * soil # one read(); 7 values or error + * rs485test # raw 1-register probe + statistics + * * 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 @@ -47,6 +54,8 @@ #include "interfaces/IConfigStore.h" #include "interfaces/IDataStorage.h" +#include "interfaces/IModbusClient.h" +#include "interfaces/ISoilSensor.h" #include "interfaces/IWaterPump.h" namespace { @@ -71,6 +80,11 @@ PumpSlot s_slots[2] = { IConfigStore *s_config = nullptr; IDataStorage *s_storage = nullptr; +// Soil sensor + Modbus client (set from app_main; the sensor is expected +// to be the LockedSoilSensor decorator). Same trivial-initialization rule. +ISoilSensor *s_soil = nullptr; +IModbusClient *s_modbus = nullptr; + const char *stop_reason_str(StopReason reason) { switch (reason) { @@ -430,6 +444,84 @@ int storage_cmd(int argc, char **argv) return print_storage_usage(); } +// --- soil / rs485test commands (feature 004 HIL verification path) ------ + +/// Error-code names per the data-model.md error table (feature 004). +const char *soil_error_str(int error) +{ + switch (error) { + case 0: + return "ok"; + case 1: + return "not_initialized"; + case 2: + return "bus_error"; + case 3: + return "timeout"; + case 5: + return "range_validation"; + default: + return error >= 100 ? "slave_exception" : "unknown"; + } +} + +/// `soil`: one read() through the locked sensor; all 7 values or the error. +int soil_cmd(int argc, char **argv) +{ + (void)argc; + (void)argv; + if (s_soil == nullptr) { + printf("ERR soil sensor not available\n"); + return 1; + } + if (!s_soil->read()) { + const int error = s_soil->getLastError(); + printf("ERR read failed: error %d (%s)\n", error, + soil_error_str(error)); + return 1; + } + printf("OK moisture=%.1f %% temp=%.1f C ec=%.0f uS/cm ph=%.1f " + "n=%.0f mg/kg p=%.0f mg/kg k=%.0f mg/kg\n", + static_cast(s_soil->getMoisture()), + static_cast(s_soil->getTemperature()), + static_cast(s_soil->getEC()), + static_cast(s_soil->getPH()), + static_cast(s_soil->getNitrogen()), + static_cast(s_soil->getPhosphorus()), + static_cast(s_soil->getPotassium())); + return 0; +} + +/// `rs485test`: one raw 1-register probe (slave 0x01, register 0x0000 — +/// the parity availability probe) + cumulative transaction statistics. +int rs485test_cmd(int argc, char **argv) +{ + (void)argc; + (void)argv; + if (s_modbus == nullptr) { + printf("ERR modbus client not available\n"); + return 1; + } + uint16_t value = 0; + const bool ok = s_modbus->readHoldingRegisters(0x01, 0x0000, 1, &value); + uint32_t successCount = 0; + uint32_t errorCount = 0; + s_modbus->getStatistics(&successCount, &errorCount); + if (ok) { + printf("OK reg 0x0000 = %u, stats: success=%lu error=%lu\n", + static_cast(value), + static_cast(successCount), + static_cast(errorCount)); + return 0; + } + const int error = s_modbus->getLastError(); + printf("ERR probe failed: error %d (%s), stats: success=%lu error=%lu\n", + error, soil_error_str(error), + static_cast(successCount), + static_cast(errorCount)); + return 1; +} + } // namespace void diag_console_register_pumps(IWaterPump& plant, IWaterPump& reservoir) @@ -444,6 +536,12 @@ void diag_console_register_storage(IConfigStore& config, IDataStorage& storage) s_storage = &storage; } +void diag_console_register_soil(ISoilSensor& sensor, IModbusClient& client) +{ + s_soil = &sensor; + s_modbus = &client; +} + esp_err_t diag_console_start(void) { esp_console_repl_t *repl = nullptr; @@ -504,5 +602,33 @@ esp_err_t diag_console_start(void) return err; } + const esp_console_cmd_t cmd_soil = { + .command = "soil", + .help = "soil — one soil sensor read (7 values or error code)", + .hint = nullptr, + .func = &soil_cmd, + .argtable = nullptr, + .func_w_context = nullptr, + .context = nullptr, + }; + err = esp_console_cmd_register(&cmd_soil); + if (err != ESP_OK) { + return err; + } + + const esp_console_cmd_t cmd_rs485test = { + .command = "rs485test", + .help = "rs485test — raw 1-register Modbus probe + statistics", + .hint = nullptr, + .func = &rs485test_cmd, + .argtable = nullptr, + .func_w_context = nullptr, + .context = nullptr, + }; + err = esp_console_cmd_register(&cmd_rs485test); + 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 fe6343a..e0d3bda 100644 --- a/firmware/main/diag_console.h +++ b/firmware/main/diag_console.h @@ -14,6 +14,8 @@ #include "esp_err.h" #include "interfaces/IConfigStore.h" #include "interfaces/IDataStorage.h" +#include "interfaces/IModbusClient.h" +#include "interfaces/ISoilSensor.h" #include "interfaces/IWaterPump.h" /** @@ -35,6 +37,18 @@ void diag_console_register_pumps(IWaterPump& plant, IWaterPump& reservoir); void diag_console_register_storage(IConfigStore& config, IDataStorage& storage); +/** + * @brief Register the soil sensor + Modbus client the `soil`/`rs485test` + * commands operate on (HIL verification path for feature 004). + * + * Pass the LockedSoilSensor decorator, never the raw sensor — the console + * handlers run on the REPL task, concurrently with the main-loop reader + * arriving in PR-11. The client may be passed raw: in this PR it is only + * reached from the REPL task (directly and via the locked sensor). Must be + * called before diag_console_start(); plain pointer registration. + */ +void diag_console_register_soil(ISoilSensor& sensor, IModbusClient& client); + /** * @brief Start the UART REPL (prompt "ws>") and register the commands. * diff --git a/firmware/test_apps/host/main/CMakeLists.txt b/firmware/test_apps/host/main/CMakeLists.txt index 7785cc2..841ba31 100644 --- a/firmware/test_apps/host/main/CMakeLists.txt +++ b/firmware/test_apps/host/main/CMakeLists.txt @@ -3,5 +3,6 @@ idf_component_register( "test_water_pump.cpp" "test_config_store.cpp" "test_data_storage.cpp" - REQUIRES unity actuators interfaces storage nvs_flash + "test_soil_sensor.cpp" + REQUIRES unity actuators interfaces storage nvs_flash sensors ) diff --git a/firmware/test_apps/host/main/test_main.cpp b/firmware/test_apps/host/main/test_main.cpp index cfef728..dca0839 100644 --- a/firmware/test_apps/host/main/test_main.cpp +++ b/firmware/test_apps/host/main/test_main.cpp @@ -16,6 +16,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); // Unity requires setUp/tearDown definitions (shared by all suites). extern "C" void setUp(void) {} @@ -27,5 +28,6 @@ extern "C" void app_main(void) run_water_pump_tests(); run_config_store_tests(); run_data_storage_tests(); + run_soil_sensor_tests(); std::exit(UNITY_END()); } diff --git a/firmware/test_apps/host/main/test_soil_sensor.cpp b/firmware/test_apps/host/main/test_soil_sensor.cpp new file mode 100644 index 0000000..b6f978e --- /dev/null +++ b/firmware/test_apps/host/main/test_soil_sensor.cpp @@ -0,0 +1,127 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file test_soil_sensor.cpp + * @brief Host tests for the ModbusSoilSensor register decode (linux target). + * + * Tests the REAL decode/scaling logic (ModbusSoilSensor) via + * MockModbusClient (scripted register payloads + call recording). + * Registered via run_soil_sensor_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 / quickstart.md §1: the data-model.md + * scaling table incl. the signed-temperature decode, humidity ≡ moisture, + * and the one-transaction-per-read() invariant. Fault paths (timeout, + * validation, exceptions, recovery) are phase 4 (T014); calibration is + * phase 6 (T021). + */ + +#include + +#include "unity.h" + +#include "sensors/ModbusSoilSensor.h" +#include "sensors/testing/MockModbusClient.h" + +namespace { + +constexpr uint8_t kAddr = 0x01; +constexpr uint16_t kStartReg = 0x0000; +constexpr uint16_t kRegCount = 9; + +/// Fresh mock + sensor per test; initialization (client init + 1-register +/// probe) is part of the fixture, and the recorded call log is cleared so +/// each test asserts only on its own transactions. +struct Fixture { + MockModbusClient mock; + ModbusSoilSensor sensor{mock, kAddr}; + + explicit Fixture(std::vector payload) + { + mock.setRegisters(kAddr, kStartReg, std::move(payload)); + TEST_ASSERT_TRUE(sensor.initialize()); + mock.calls.clear(); + } +}; + +} // namespace + +// -------------------------------------------------------------------------- +// Known 9-register payload decodes per the data-model scaling table, +// including a negative temperature (0xFF38 = -200 raw = -20.0 °C) +// -------------------------------------------------------------------------- +static void test_decode_known_payload_negative_temperature(void) +{ + // regs 0x0000..0x0008: humidity/moisture, temp, EC, pH, N, P, K, + // salinity, TDS. + Fixture f({550, 0xFF38, 1200, 68, 45, 30, 120, 7, 9}); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); // 550 / 10 + TEST_ASSERT_EQUAL_FLOAT(-20.0f, f.sensor.getTemperature()); // int16 -200 / 10 + TEST_ASSERT_EQUAL_FLOAT(1200.0f, f.sensor.getEC()); // unscaled + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); // 68 / 10 + TEST_ASSERT_EQUAL_FLOAT(45.0f, f.sensor.getNitrogen()); + TEST_ASSERT_EQUAL_FLOAT(30.0f, f.sensor.getPhosphorus()); + TEST_ASSERT_EQUAL_FLOAT(120.0f, f.sensor.getPotassium()); + + // Salinity (reg 0x0007 = 7) and TDS (reg 0x0008 = 9) are read but not + // exposed: ISoilSensor has no getter for them, so there is nothing + // further to assert beyond the 7 values above (deliberate parity trim). +} + +// -------------------------------------------------------------------------- +// Positive signed temperature decodes the same way (235 raw = 23.5 °C) +// -------------------------------------------------------------------------- +static void test_decode_positive_temperature(void) +{ + Fixture f({420, 235, 800, 65, 12, 8, 40, 3, 4}); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(23.5f, f.sensor.getTemperature()); + TEST_ASSERT_EQUAL_FLOAT(42.0f, f.sensor.getMoisture()); + TEST_ASSERT_EQUAL_FLOAT(6.5f, f.sensor.getPH()); +} + +// -------------------------------------------------------------------------- +// Humidity ≡ moisture (parity: single quantity in register 0x0000) +// -------------------------------------------------------------------------- +static void test_humidity_equals_moisture(void) +{ + Fixture f({550, 0xFF38, 1200, 68, 45, 30, 120, 7, 9}); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(f.sensor.getMoisture(), f.sensor.getHumidity()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getHumidity()); +} + +// -------------------------------------------------------------------------- +// One read() = exactly ONE bus transaction: all 9 registers in one +// readHoldingRegisters(0x01, 0x0000, 9) call (FR-004, no retry) +// -------------------------------------------------------------------------- +static void test_read_is_one_nine_register_transaction(void) +{ + Fixture f({550, 235, 1200, 68, 45, 30, 120, 7, 9}); + + TEST_ASSERT_TRUE(f.sensor.read()); + + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + const MockModbusClient::Call &call = f.mock.calls.front(); + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Read), + static_cast(call.type)); + TEST_ASSERT_EQUAL_UINT8(kAddr, call.deviceAddress); + TEST_ASSERT_EQUAL_UINT16(kStartReg, call.startRegister); + TEST_ASSERT_EQUAL_UINT16(kRegCount, call.count); + TEST_ASSERT_TRUE(call.succeeded); +} + +void run_soil_sensor_tests(void) +{ + RUN_TEST(test_decode_known_payload_negative_temperature); + RUN_TEST(test_decode_positive_temperature); + RUN_TEST(test_humidity_equals_moisture); + RUN_TEST(test_read_is_one_nine_register_transaction); +} diff --git a/specs/004-modbus-soil-sensor/data-model.md b/specs/004-modbus-soil-sensor/data-model.md index 67fbcca..eacbf36 100644 --- a/specs/004-modbus-soil-sensor/data-model.md +++ b/specs/004-modbus-soil-sensor/data-model.md @@ -8,7 +8,7 @@ Produced atomically from one 9-register transaction; either fully valid or an er | Field | Type | Source | Validation (read-fails-if) | |---|---|---|---| -| moisture | float % | reg 0x0000 ÷10, × moisture factor | outside 0–100 | +| moisture | float % | reg 0x0000 ÷10 (moisture factor NOT applied on read — legacy parity; `calibrateMoisture` stores/writes the factor but the read path never uses it) | outside 0–100 | | temperature | float °C | reg 0x0001 as **int16_t** ÷10 | outside −40–80 | | ec | float µS/cm | reg 0x0002 ×1, × EC factor | not enforced (parity) | | ph | float | reg 0x0003 ÷10, × pH factor | outside 3–9 | diff --git a/specs/004-modbus-soil-sensor/tasks.md b/specs/004-modbus-soil-sensor/tasks.md index 0a2b935..5e1838d 100644 --- a/specs/004-modbus-soil-sensor/tasks.md +++ b/specs/004-modbus-soil-sensor/tasks.md @@ -17,28 +17,28 @@ US3 dual-board, US4 host-testability) so each story is independently verifiable. ## Phase 1: Setup -- [ ] T001 Create `sensors` component skeleton: `firmware/components/sensors/CMakeLists.txt` +- [x] T001 Create `sensors` component skeleton: `firmware/components/sensors/CMakeLists.txt` (REQUIRES `interfaces`, `board`; register `src/ModbusSoilSensor.cpp` always and `src/EspModbusClient.cpp` only when `IDF_TARGET != linux`, same guard style as `firmware/components/storage/CMakeLists.txt`) and `firmware/components/sensors/idf_component.yml` pinning `espressif/esp-modbus: "==2.1.2"` -- [ ] T002 [VERIFY-MAIN] Confirm `CONFIG_FMB_MASTER_TIMEOUT_MS_RESPOND` bounds allow +- [x] T002 [VERIFY-MAIN] Confirm `CONFIG_FMB_MASTER_TIMEOUT_MS_RESPOND` bounds allow 3000 ms in esp-modbus 2.1.2 Kconfig (inspect managed component after a dependency fetch) and add any required override to `firmware/sdkconfig.defaults` (plan risk 4) ## Phase 2: Foundational (blocking prerequisites for all stories) -- [ ] T003 [P] Port `IModbusClient` to +- [x] T003 [P] Port `IModbusClient` to `firmware/components/interfaces/include/interfaces/IModbusClient.h` per contracts/interfaces.md (pure C++, guard `WATERINGSYSTEM_INTERFACES_IMODBUSCLIENT_H`, SPDX header, contract notes as doc comments: one attempt/no retry, write echo verification, statistics semantics) -- [ ] T004 [P] Port `ISoilSensor` to +- [x] T004 [P] Port `ISoilSensor` to `firmware/components/interfaces/include/interfaces/ISoilSensor.h` per contracts/interfaces.md (trimmed surface — no setValidRange/isWithinValidRange; document the trim and the read()/validity contract) -- [ ] T005 [P] Create `MockModbusClient` in +- [x] T005 [P] Create `MockModbusClient` in `firmware/components/sensors/include/sensors/testing/MockModbusClient.h`: scriptable register payloads per (address, startRegister, count), forced timeout/CRC/exception errors, call recording (incl. writeSingleRegister log), @@ -53,20 +53,20 @@ scaling incl. signed temperature, console `soil`/`rs485test` output. **Independent test**: quickstart.md §1 decode tests green + §3 HIL steps 1–3. -- [ ] T006 [P] [US1] Write host tests for register decode in +- [x] T006 [P] [US1] Write host tests for register decode in `firmware/test_apps/host/main/test_soil_sensor.cpp`: known 9-register payload decodes to expected moisture/temp/EC/pH/N/P/K (data-model scaling table), negative temperature case (0xFF38 → −20.0 °C), humidity ≡ moisture, salinity/TDS not exposed; register in `firmware/test_apps/host/main/CMakeLists.txt` -- [ ] T007 [US1] Implement `ModbusSoilSensor` (pure logic) in +- [x] T007 [US1] Implement `ModbusSoilSensor` (pure logic) in `firmware/components/sensors/include/sensors/ModbusSoilSensor.h` + `firmware/components/sensors/src/ModbusSoilSensor.cpp`: initialize/read (one readHoldingRegisters(0x01, 0x0000, 9) call), decode per data-model.md, getters from last successful read, real-bus-read isAvailable() (1 register, parity), getLastError; port reference `src/sensors/ModbusSoilSensor.cpp` (READ-ONLY legacy) -- [ ] T008 [US1] Implement `EspModbusClient` core in +- [x] T008 [US1] Implement `EspModbusClient` core in `firmware/components/sensors/include/sensors/EspModbusClient.h` + `firmware/components/sensors/src/EspModbusClient.cpp`: R1 create/start sequence (`mbc_master_create_serial`, ser_opts 9600 8N1 `response_tout_ms=3000`, UART @@ -78,21 +78,21 @@ scaling incl. signed temperature, console `soil`/`rs485test` output. readHoldingRegisters/writeSingleRegister via `mbc_master_send_request` (commands 0x03/0x06), esp_err_t → error-code mapping per data-model.md (R6), statistics counters -- [ ] T009 [US1] Add `LockedSoilSensor` decorator in +- [x] T009 [US1] Add `LockedSoilSensor` decorator in `firmware/components/sensors/include/sensors/LockedSoilSensor.h` (per-call mutex, pattern of `actuators/LockedWaterPump.h`; document per-call-not- cross-call atomicity like `storage/Locked*`) -- [ ] T010 [US1] Wire sensor into app in `firmware/main/app_main.cpp`: +- [x] T010 [US1] Wire sensor into app in `firmware/main/app_main.cpp`: construct `EspModbusClient` + `ModbusSoilSensor` + `LockedSoilSensor` after storage init; boot log line with client init result; no periodic read task (out of scope, PR-11) -- [ ] T011 [US1] Add console commands `soil` and `rs485test` in +- [x] T011 [US1] Add console commands `soil` and `rs485test` in `firmware/main/diag_console.cpp` per contracts/interfaces.md console contract (values or error code+name; raw probe + statistics dump), going through `LockedSoilSensor`/client -- [ ] T012 [VERIFY-MAIN] [US1] Host suite green (quickstart §1) — decode tests pass, +- [x] T012 [VERIFY-MAIN] [US1] Host suite green (quickstart §1) — decode tests pass, exit code 0 -- [ ] T013 [VERIFY-MAIN] [US1] rev1 target builds green (quickstart §2, rev1 half) +- [x] T013 [VERIFY-MAIN] [US1] rev1 target builds green (quickstart §2, rev1 half) **Checkpoint**: MVP flashable — HIL steps 1–3 executable by Paul at CP3. From fe20d55aa73ad9be87fde6963f3629d7fea7e697 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 11:25:12 +0200 Subject: [PATCH 08/11] =?UTF-8?q?test(firmware):=20soil=20sensor=20fault-p?= =?UTF-8?q?ath=20host=20tests=20=E2=80=94=20invalid=20data=20never=20wrong?= =?UTF-8?q?=20data?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T014-T017 (US2): 12 tests covering timeout/range/exception rejection with all-or-nothing publish, no-retry invariant, recovery without reinit, real-bus availability probing, lazy-init failure paths, statistics and setTimeout contracts. One logging gap fixed in isAvailable(). Host suite 66/0 on linux target. Co-Authored-By: Claude Fable 5 --- .../sensors/src/ModbusSoilSensor.cpp | 14 +- .../test_apps/host/main/test_soil_sensor.cpp | 302 +++++++++++++++++- specs/004-modbus-soil-sensor/tasks.md | 8 +- 3 files changed, 311 insertions(+), 13 deletions(-) diff --git a/firmware/components/sensors/src/ModbusSoilSensor.cpp b/firmware/components/sensors/src/ModbusSoilSensor.cpp index 44aac19..54d7856 100644 --- a/firmware/components/sensors/src/ModbusSoilSensor.cpp +++ b/firmware/components/sensors/src/ModbusSoilSensor.cpp @@ -80,7 +80,8 @@ bool ModbusSoilSensor::initialize() bool ModbusSoilSensor::read() { - // Lazy initialization (legacy :77-81). + // Lazy initialization (legacy :77-81). A failure here is already + // logged inside initialize() (both exits) and lastError_ is set there. if (!initialized_ && !initialize()) { return false; } @@ -158,8 +159,15 @@ bool ModbusSoilSensor::isAvailable() // legacy :140 — register 0x0000). Legacy passed nullptr as the buffer; // the new IModbusClient contract requires a caller-owned buffer. uint16_t testRegister = 0; - return client_.readHoldingRegisters(deviceAddress_, kRegHumidity, 1, - &testRegister); + const bool available = client_.readHoldingRegisters( + deviceAddress_, kRegHumidity, 1, &testRegister); + if (!available) { + // Probe failure is not latched (next call probes again) and does + // not touch lastError_ — read() owns the reading's error state. + ESP_LOGW(TAG, "availability probe failed: error %d", + client_.getLastError()); + } + return available; } int ModbusSoilSensor::getLastError() diff --git a/firmware/test_apps/host/main/test_soil_sensor.cpp b/firmware/test_apps/host/main/test_soil_sensor.cpp index b6f978e..878de3e 100644 --- a/firmware/test_apps/host/main/test_soil_sensor.cpp +++ b/firmware/test_apps/host/main/test_soil_sensor.cpp @@ -10,11 +10,13 @@ * (test_main.cpp); the process exit code equals the failure count and is * the CI gate. * - * Coverage maps to tasks.md T006 / quickstart.md §1: the data-model.md - * scaling table incl. the signed-temperature decode, humidity ≡ moisture, - * and the one-transaction-per-read() invariant. Fault paths (timeout, - * validation, exceptions, recovery) are phase 4 (T014); calibration is - * phase 6 (T021). + * Coverage maps to tasks.md T006 + T014 / quickstart.md §1: the + * data-model.md scaling table incl. the signed-temperature decode, + * humidity ≡ moisture, the one-transaction-per-read() invariant, and the + * US2 fault paths — timeout, range validation (error 5, all-or-nothing + * publish), exception propagation, no-retry, implicit recovery, lazy + * (re)initialization, real-bus availability probe, statistics and the + * setTimeout client contract. Calibration is phase 6 (T021). */ #include @@ -30,9 +32,18 @@ constexpr uint8_t kAddr = 0x01; constexpr uint16_t kStartReg = 0x0000; constexpr uint16_t kRegCount = 9; +/// Baseline valid payload: moisture 55.0 %, temperature 23.5 °C, +/// EC 1200 µS/cm, pH 6.8, N 45, P 30, K 120 (+ salinity/TDS, unexposed). +std::vector goodPayload() +{ + return {550, 235, 1200, 68, 45, 30, 120, 7, 9}; +} + /// Fresh mock + sensor per test; initialization (client init + 1-register /// probe) is part of the fixture, and the recorded call log is cleared so -/// each test asserts only on its own transactions. +/// each test asserts only on its own transactions. NOTE: the fixture's +/// probe read still counts in the mock STATISTICS (success 1 / error 0 +/// baseline) — only the call log is cleared. struct Fixture { MockModbusClient mock; ModbusSoilSensor sensor{mock, kAddr}; @@ -118,10 +129,289 @@ static void test_read_is_one_nine_register_transaction(void) TEST_ASSERT_TRUE(call.succeeded); } +// ========================================================================== +// Fault paths (T014, US2): timeout / validation / exception / no-retry / +// recovery / statistics / availability probe / setTimeout contract +// ========================================================================== + +// -------------------------------------------------------------------------- +// Timeout: read() fails with error 3 and the last-good values remain +// untouched — getters after a failed read are stale, not fresh (FR-005) +// -------------------------------------------------------------------------- +static void test_timeout_fails_read_and_keeps_last_good_values(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + + f.mock.queueOutcome(MockModbusClient::kErrTimeout); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(MockModbusClient::kErrTimeout, + f.sensor.getLastError()); + + // Last-good reading still served (validity is carried by the read() + // result + error code, never by the values). + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); + TEST_ASSERT_EQUAL_FLOAT(23.5f, f.sensor.getTemperature()); + TEST_ASSERT_EQUAL_FLOAT(1200.0f, f.sensor.getEC()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); +} + +// -------------------------------------------------------------------------- +// Range validation: moisture > 100 % (raw 1500 = 150.0 %) → error 5, +// nothing published +// -------------------------------------------------------------------------- +static void test_out_of_range_moisture_rejected_error5(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + + f.mock.setRegisters(kAddr, kStartReg, {1500, 235, 800, 68, 1, 2, 3, 0, 0}); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getHumidity()); +} + +// -------------------------------------------------------------------------- +// Range validation: temperature < -40 °C (raw 0xFE0C = int16 -500 = +// -50.0 °C) → error 5, nothing published +// -------------------------------------------------------------------------- +static void test_out_of_range_temperature_rejected_error5(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + + f.mock.setRegisters(kAddr, kStartReg, + {550, 0xFE0C, 800, 68, 1, 2, 3, 0, 0}); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL_FLOAT(23.5f, f.sensor.getTemperature()); +} + +// -------------------------------------------------------------------------- +// Range validation: pH > 9 (raw 95 = 9.5) → error 5, and the publish is +// ALL-OR-NOTHING: in-range fields of the same payload (EC, N/P/K) are +// rejected together with the offending one (FR-005) +// -------------------------------------------------------------------------- +static void test_out_of_range_ph_rejected_all_or_nothing(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + + // pH 9.5 out of range; EC 999 and N/P/K 77/88/99 are individually fine. + f.mock.setRegisters(kAddr, kStartReg, + {550, 235, 999, 95, 77, 88, 99, 0, 0}); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); + TEST_ASSERT_EQUAL_FLOAT(1200.0f, f.sensor.getEC()); // not 999 + TEST_ASSERT_EQUAL_FLOAT(45.0f, f.sensor.getNitrogen()); // not 77 + TEST_ASSERT_EQUAL_FLOAT(30.0f, f.sensor.getPhosphorus()); + TEST_ASSERT_EQUAL_FLOAT(120.0f, f.sensor.getPotassium()); +} + +// -------------------------------------------------------------------------- +// Modbus slave exception: the sensor layer propagates the client's error +// code VERBATIM (here 100+2 = 102). The real EspModbusClient coarsens +// exceptions to code 2 (esp-modbus 2.1.2 hides the exception number — +// documented parity divergence R6); this test pins the sensor-layer +// propagation contract, not the client mapping. +// -------------------------------------------------------------------------- +static void test_modbus_exception_propagates_verbatim(void) +{ + Fixture f(goodPayload()); + + f.mock.queueOutcome(MockModbusClient::kErrExceptionBase + 2); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(102, f.sensor.getLastError()); +} + +// -------------------------------------------------------------------------- +// No retry: a failed read() is exactly ONE bus attempt (FR-004) — the +// fixture cleared the init probe, so the call log holds only this read +// -------------------------------------------------------------------------- +static void test_failed_read_is_single_bus_attempt(void) +{ + Fixture f(goodPayload()); + + f.mock.queueOutcome(MockModbusClient::kErrTimeout); + TEST_ASSERT_FALSE(f.sensor.read()); + + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + const MockModbusClient::Call &call = f.mock.calls.front(); + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Read), + static_cast(call.type)); + TEST_ASSERT_EQUAL_UINT16(kRegCount, call.count); + TEST_ASSERT_FALSE(call.succeeded); +} + +// -------------------------------------------------------------------------- +// Implicit recovery: fail (timeout) then succeed — the next read() returns +// fresh values with NO re-initialization (no extra probe transaction, no +// second client initialize; there is no permanent-failure latch) +// -------------------------------------------------------------------------- +static void test_read_recovers_after_failure_without_reinit(void) +{ + Fixture f(goodPayload()); + + f.mock.queueOutcome(MockModbusClient::kErrTimeout); + TEST_ASSERT_FALSE(f.sensor.read()); + + TEST_ASSERT_TRUE(f.sensor.read()); // defaultOutcome kOk + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); + TEST_ASSERT_EQUAL_FLOAT(23.5f, f.sensor.getTemperature()); + + // Exactly the two 9-register reads — no 1-register probe in between. + TEST_ASSERT_EQUAL(2, f.mock.calls.size()); + TEST_ASSERT_EQUAL_UINT16(kRegCount, f.mock.calls[0].count); + TEST_ASSERT_EQUAL_UINT16(kRegCount, f.mock.calls[1].count); + TEST_ASSERT_EQUAL_INT(1, f.mock.initializeCalls); +} + +// -------------------------------------------------------------------------- +// Statistics: exactly one counter increments per client call, on both +// outcomes (IModbusClient contract; fixture probe = baseline 1 success) +// -------------------------------------------------------------------------- +static void test_statistics_count_one_per_call(void) +{ + Fixture f(goodPayload()); + + uint32_t successCount = 0; + uint32_t errorCount = 0; + f.mock.getStatistics(&successCount, &errorCount); + TEST_ASSERT_EQUAL_UINT32(1, successCount); // fixture init probe + TEST_ASSERT_EQUAL_UINT32(0, errorCount); + + TEST_ASSERT_TRUE(f.sensor.read()); + f.mock.getStatistics(&successCount, &errorCount); + TEST_ASSERT_EQUAL_UINT32(2, successCount); + TEST_ASSERT_EQUAL_UINT32(0, errorCount); + + f.mock.queueOutcome(MockModbusClient::kErrTimeout); + TEST_ASSERT_FALSE(f.sensor.read()); + f.mock.getStatistics(&successCount, &errorCount); + TEST_ASSERT_EQUAL_UINT32(2, successCount); + TEST_ASSERT_EQUAL_UINT32(1, errorCount); +} + +// -------------------------------------------------------------------------- +// isAvailable(): a REAL 1-register bus read of 0x0000 on EVERY call — +// never cached (FR-011); result reflects the live outcome, and a sensor +// that answers again is available again +// -------------------------------------------------------------------------- +static void test_is_available_performs_real_probe_every_call(void) +{ + Fixture f(goodPayload()); + + TEST_ASSERT_TRUE(f.sensor.isAvailable()); + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + const MockModbusClient::Call &probe = f.mock.calls.front(); + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Read), + static_cast(probe.type)); + TEST_ASSERT_EQUAL_UINT8(kAddr, probe.deviceAddress); + TEST_ASSERT_EQUAL_UINT16(0x0000, probe.startRegister); + TEST_ASSERT_EQUAL_UINT16(1, probe.count); + + // Second call hits the bus again (not a cached true) and reports the + // live failure... + f.mock.queueOutcome(MockModbusClient::kErrTimeout); + TEST_ASSERT_FALSE(f.sensor.isAvailable()); + TEST_ASSERT_EQUAL(2, f.mock.calls.size()); + + // ...and the next call recovers implicitly (no failure latch). + TEST_ASSERT_TRUE(f.sensor.isAvailable()); + TEST_ASSERT_EQUAL(3, f.mock.calls.size()); +} + +// -------------------------------------------------------------------------- +// setTimeout(): reaches the client and is recorded verbatim (FR-006 at the +// IModbusClient contract level — ModbusSoilSensor itself never sets the +// timeout; the parity 3000 ms default lives in the client) +// -------------------------------------------------------------------------- +static void test_set_timeout_reaches_client(void) +{ + MockModbusClient mock; + mock.setTimeout(1234); + + TEST_ASSERT_EQUAL(1, mock.timeoutCalls.size()); + TEST_ASSERT_EQUAL_UINT32(1234, mock.timeoutCalls.front()); +} + +// -------------------------------------------------------------------------- +// Lazy initialization: a failed client initialize() → read() fails with +// error 2 and NO bus transaction; once the client comes up, the next +// read() initializes (probe) and delivers fresh values +// -------------------------------------------------------------------------- +static void test_lazy_init_client_failure_then_recovery(void) +{ + MockModbusClient mock; + mock.setRegisters(kAddr, kStartReg, goodPayload()); + mock.initializeResult = false; + ModbusSoilSensor sensor(mock, kAddr); + + TEST_ASSERT_FALSE(sensor.read()); + TEST_ASSERT_EQUAL_INT(2, sensor.getLastError()); // client init failed + TEST_ASSERT_EQUAL(0, mock.calls.size()); // never reached the bus + + mock.initializeResult = true; + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_INT(0, sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, sensor.getMoisture()); + + // Recovery read = 1-register init probe + the 9-register data read. + TEST_ASSERT_EQUAL(2, mock.calls.size()); + TEST_ASSERT_EQUAL_UINT16(1, mock.calls[0].count); + TEST_ASSERT_EQUAL_UINT16(kRegCount, mock.calls[1].count); +} + +// -------------------------------------------------------------------------- +// Lazy initialization: a failed init PROBE keeps the sensor uninitialized +// (read() aborts after the probe — no data transaction) and the next +// read() re-runs the full initialization before reading +// -------------------------------------------------------------------------- +static void test_lazy_init_probe_failure_then_recovery(void) +{ + MockModbusClient mock; + mock.setRegisters(kAddr, kStartReg, goodPayload()); + ModbusSoilSensor sensor(mock, kAddr); + + mock.queueOutcome(MockModbusClient::kErrTimeout); // hits the init probe + TEST_ASSERT_FALSE(sensor.read()); + TEST_ASSERT_EQUAL_INT(MockModbusClient::kErrTimeout, + sensor.getLastError()); + TEST_ASSERT_EQUAL(1, mock.calls.size()); // probe only, no 9-reg read + TEST_ASSERT_EQUAL_UINT16(1, mock.calls[0].count); + + TEST_ASSERT_TRUE(sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, sensor.getMoisture()); + + // Second read re-initialized: probe (1 reg) + data read (9 regs). + TEST_ASSERT_EQUAL(3, mock.calls.size()); + TEST_ASSERT_EQUAL_UINT16(1, mock.calls[1].count); + TEST_ASSERT_EQUAL_UINT16(kRegCount, mock.calls[2].count); + TEST_ASSERT_EQUAL_INT(2, mock.initializeCalls); +} + void run_soil_sensor_tests(void) { RUN_TEST(test_decode_known_payload_negative_temperature); RUN_TEST(test_decode_positive_temperature); RUN_TEST(test_humidity_equals_moisture); RUN_TEST(test_read_is_one_nine_register_transaction); + RUN_TEST(test_timeout_fails_read_and_keeps_last_good_values); + RUN_TEST(test_out_of_range_moisture_rejected_error5); + RUN_TEST(test_out_of_range_temperature_rejected_error5); + RUN_TEST(test_out_of_range_ph_rejected_all_or_nothing); + RUN_TEST(test_modbus_exception_propagates_verbatim); + RUN_TEST(test_failed_read_is_single_bus_attempt); + RUN_TEST(test_read_recovers_after_failure_without_reinit); + RUN_TEST(test_statistics_count_one_per_call); + RUN_TEST(test_is_available_performs_real_probe_every_call); + RUN_TEST(test_set_timeout_reaches_client); + RUN_TEST(test_lazy_init_client_failure_then_recovery); + RUN_TEST(test_lazy_init_probe_failure_then_recovery); } diff --git a/specs/004-modbus-soil-sensor/tasks.md b/specs/004-modbus-soil-sensor/tasks.md index 5e1838d..f6cafb7 100644 --- a/specs/004-modbus-soil-sensor/tasks.md +++ b/specs/004-modbus-soil-sensor/tasks.md @@ -103,7 +103,7 @@ auto-recovery on next read. **Independent test**: host tests for all fault paths green; HIL steps 4–5. -- [ ] T014 [P] [US2] Extend `firmware/test_apps/host/main/test_soil_sensor.cpp` with +- [x] T014 [P] [US2] Extend `firmware/test_apps/host/main/test_soil_sensor.cpp` with fault-path tests: timeout → read() false + timeout error + getters not presented as fresh; out-of-range moisture/temp/pH → validation error 5; Modbus exception → distinct 100+n (or documented generic) code; exactly ONE @@ -112,17 +112,17 @@ auto-recovery on next read. increment correctness; isAvailable() performs a real 1-register bus read (assert via mock call recording — FR-011, analyze C1); setTimeout() reaches the client (mock passthrough assert — FR-006, analyze C2) -- [ ] T015 [US2] Implement fault handling in +- [x] T015 [US2] Implement fault handling in `firmware/components/sensors/src/ModbusSoilSensor.cpp`: range validation per data-model table (fail read, error 5, EC/NPK unenforced per parity), validity flag semantics, ESP_LOG error on every failed read (tag `soil_sensor`), no-retry invariant, lazy availability recovery (no permanent-failure state) -- [ ] T016 [US2] Verify/complete error discrimination in +- [x] T016 [US2] Verify/complete error discrimination in `firmware/components/sensors/src/EspModbusClient.cpp`: timeout vs invalid- response vs exception mapping (R6); if 2.1.2 hides the slave exception code, map to the single documented exception code and note the parity divergence in a code comment + PR notes -- [ ] T017 [VERIFY-MAIN] [US2] Host suite green including all fault-path tests +- [x] T017 [VERIFY-MAIN] [US2] Host suite green including all fault-path tests **Checkpoint**: fail-safe contract (FR-005/FR-006/FR-010) host-proven. From e522bc3952731ce629d78880fc59908e4ebfc074 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 11:34:40 +0200 Subject: [PATCH 09/11] feat(firmware): soil calibration tests, MockSoilSensor, console calibration commands T018-T027: FW-2/FW-4 confirmations (written in phase 3, verified), rev2 board build green with compile-time proof of no DE-pin reference, 6 calibration host tests (factor computation, x100 register write, moisture-factor-not-applied parity, non-fatal write failure, validation on factored values), MockSoilSensor fixture for PR-11, soil_cal_* console commands, HIL checklist, firmware/CLAUDE.md sensors section, plan risk outcomes recorded (R5/R6 divergences documented, R2 open for HIL). Host suite 72/0. Co-Authored-By: Claude Fable 5 --- firmware/CLAUDE.md | 42 +++- .../include/sensors/testing/MockSoilSensor.h | 115 ++++++++++ firmware/main/diag_console.cpp | 108 ++++++++- .../test_apps/host/main/test_soil_sensor.cpp | 206 +++++++++++++++++- .../004-modbus-soil-sensor/checklists/hil.md | 62 ++++++ specs/004-modbus-soil-sensor/plan.md | 34 ++- specs/004-modbus-soil-sensor/tasks.md | 20 +- 7 files changed, 558 insertions(+), 29 deletions(-) create mode 100644 firmware/components/sensors/include/sensors/testing/MockSoilSensor.h create mode 100644 specs/004-modbus-soil-sensor/checklists/hil.md diff --git a/firmware/CLAUDE.md b/firmware/CLAUDE.md index e074f3e..82ccdcb 100644 --- a/firmware/CLAUDE.md +++ b/firmware/CLAUDE.md @@ -28,8 +28,10 @@ must stay green. ### Host tests (linux preview target) -Pump enforcement logic is unit-tested natively, no ESP32 needed. The test -executable's exit code equals the Unity failure count (CI gate, job +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`): ```bash @@ -59,12 +61,19 @@ firmware/ │ │ └── include/board/board.h │ ├── interfaces/ # Header-only, NO IDF deps (host-includable) │ │ └── include/interfaces/ # IActuator, IWaterPump, ITimeProvider, -│ │ # IConfigStore, IDataStorage +│ │ # IConfigStore, IDataStorage, +│ │ # IModbusClient, ISoilSensor │ ├── 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 +│ │ # excluded on linux target │ └── storage/ # Config + data persistence (feature 003) │ ├── include/storage/ # NvsConfigStore, LittleFsDataStorage (POSIX, │ │ # host-runnable), StorageMount (esp32-only), @@ -74,7 +83,8 @@ firmware/ │ # on linux target (esp_littlefs has no port) └── test_apps/ └── host/ # Host test app (linux preview target, Unity): - # pump + config store + data storage suites + # pump + config store + data storage + + # soil sensor (test_soil_sensor.cpp) suites ``` Future components (drivers, controllers, web server) are added as siblings @@ -115,6 +125,16 @@ config get | set | wifi | wifi-clear | factory- storage stats | log | query [t0 t1] | event | events [n] ``` +Feature 004 adds the soil sensor commands (HIL verification path, same +thin-wrapper rule; a failed calibration-register write is reported as +non-fatal — legacy parity): + +``` +soil # one read(); 7 values or error code +rs485test # raw 1-register Modbus probe + statistics +soil_cal_moisture | soil_cal_ph | soil_cal_ec +``` + ## Storage (config + data persistence) Feature 003 (PR-06). Two redesigned, host-includable interfaces in @@ -146,7 +166,19 @@ data is migrated; on-disk formats diverge from legacy by design — see The diagnostic console (`ws>`) exposes `config` and `storage` subcommands for the HIL verification path (see below). -## Board abstraction +## Soil sensor (RS485 Modbus) + +Feature 004 (PR-08). `components/sensors/` splits the driver at the +`IModbusClient` interface: `ModbusSoilSensor` is pure C++ +(decode/scaling/validation/calibration, host-tested against +`MockModbusClient`), `EspModbusClient` is the only hardware touchpoint +(esp-modbus RTU master, UART RS485 half-duplex on both boards, RX pull-up +for the rev2 SHDN̅/hi-Z case) and is excluded from the linux build. The +**esp-modbus dependency is pinned `==2.1.2`** in the component's +`idf_component.yml` and rule-gated to `target != linux`, keeping it out of +the host-test dependency graph. Cross-task access goes through +`LockedSoilSensor` (console REPL now, main-loop reader in PR-11) — same +pattern as the other Locked* wrappers. Two board revisions exist, selected via Kconfig (`main/Kconfig.projbuild`): diff --git a/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h b/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h new file mode 100644 index 0000000..5389959 --- /dev/null +++ b/firmware/components/sensors/include/sensors/testing/MockSoilSensor.h @@ -0,0 +1,115 @@ +// SPDX-FileCopyrightText: 2026 Cryptotomte +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @file MockSoilSensor.h + * @brief Scriptable ISoilSensor test double (header-only). + * + * Serves the host tests of soil-sensor CONSUMERS (watering controller / + * sensor manager, PR-11): set the seven quantity values and the + * initialize()/read()/isAvailable() results, script the error code, and + * assert on the call counters. The sensor's own decode/validation logic is + * tested against the REAL ModbusSoilSensor via MockModbusClient — never + * through this mock. Never compiled into target builds (only included from + * test code). No IDF includes. + */ + +#ifndef WATERINGSYSTEM_SENSORS_TESTING_MOCKSOILSENSOR_H +#define WATERINGSYSTEM_SENSORS_TESTING_MOCKSOILSENSOR_H + +#include + +#include "interfaces/ISoilSensor.h" + +/** + * @brief ISoilSensor returning scripted values, instrumented for tests. + * + * All state is public (MockModbusClient/MockConfigStore style): assign the + * result fields and quantity values before driving the consumer, then + * assert on the counters/recorded arguments. The getters serve the scripted + * values unconditionally — like the real sensor after a failed read() they + * keep returning the last values, so consumers must gate on the read() + * result / lastError, never on value plausibility (FR-005). + */ +class MockSoilSensor : public ISoilSensor { +public: + // -- Scripted results --------------------------------------------------- + + bool initializeResult = true; ///< returned by initialize() + bool readResult = true; ///< returned by read() + bool isAvailableResult = true; ///< returned by isAvailable() + bool calibrateResult = true; ///< returned by all three calibrate*() + int lastError = 0; ///< returned by getLastError() + + // -- Scripted quantity values (served by the getters) -------------------- + + float moisture = 0.0f; + float temperature = 0.0f; + float humidity = 0.0f; ///< the real sensor keeps this ≡ moisture (parity) + float ph = 0.0f; + float ec = 0.0f; + float nitrogen = 0.0f; + float phosphorus = 0.0f; + float potassium = 0.0f; + + // -- Instrumentation ----------------------------------------------------- + + int initializeCalls = 0; + int readCalls = 0; + int isAvailableCalls = 0; + /// Every calibrate*() reference-value argument, in call order (the + /// vector size doubles as the per-quantity call counter). + std::vector calibrateMoistureCalls; + std::vector calibratePhCalls; + std::vector calibrateEcCalls; + + // -- ISoilSensor --------------------------------------------------------- + + bool initialize() override + { + ++initializeCalls; + return initializeResult; + } + + bool read() override + { + ++readCalls; + return readResult; + } + + bool isAvailable() override + { + ++isAvailableCalls; + return isAvailableResult; + } + + int getLastError() override { return lastError; } + + float getMoisture() override { return moisture; } + float getTemperature() override { return temperature; } + float getHumidity() override { return humidity; } + float getPH() override { return ph; } + float getEC() override { return ec; } + float getNitrogen() override { return nitrogen; } + float getPhosphorus() override { return phosphorus; } + float getPotassium() override { return potassium; } + + bool calibrateMoisture(float referenceValue) override + { + calibrateMoistureCalls.push_back(referenceValue); + return calibrateResult; + } + + bool calibratePH(float referenceValue) override + { + calibratePhCalls.push_back(referenceValue); + return calibrateResult; + } + + bool calibrateEC(float referenceValue) override + { + calibrateEcCalls.push_back(referenceValue); + return calibrateResult; + } +}; + +#endif /* WATERINGSYSTEM_SENSORS_TESTING_MOCKSOILSENSOR_H */ diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index 4d4c66d..4941eca 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -28,11 +28,13 @@ * storage events [n] # newest-first, default 10 * * Soil sensor commands (HIL verification path for feature 004; console - * contract in specs/004-modbus-soil-sensor/contracts/interfaces.md — - * calibration commands arrive in a later phase of that feature): + * contract in specs/004-modbus-soil-sensor/contracts/interfaces.md): * * soil # one read(); 7 values or error * rs485test # raw 1-register probe + statistics + * soil_cal_moisture # calibrate against a reference + * soil_cal_ph # value; a failed calibration- + * soil_cal_ec # register write is NON-FATAL * * Handler exit codes follow the esp_console convention: 0 on OK, 1 on ERR. * @@ -522,6 +524,63 @@ int rs485test_cmd(int argc, char **argv) return 1; } +/// Shared `soil_cal_*` flow: one calibrate*() call through the locked +/// sensor. ModbusSoilSensor exposes no factor getter (factors are private, +/// RAM-only in this PR), so the output reports success/failure plus the +/// legacy write-result semantics: a true return with a non-zero last error +/// means the factor is applied locally but the best-effort write to the +/// sensor's calibration register failed (NON-FATAL, parity). +int cmd_soil_calibrate(int argc, char **argv, const char *name, + bool (ISoilSensor::*calibrate)(float)) +{ + if (s_soil == nullptr) { + printf("ERR soil sensor not available\n"); + return 1; + } + if (argc != 2) { + printf("ERR usage: %s \n", name); + return 1; + } + float reference = 0.0f; + if (!parse_float(argv[1], reference)) { + printf("ERR reference-value: not a number\n"); + return 1; + } + if (!(s_soil->*calibrate)(reference)) { + const int error = s_soil->getLastError(); + printf("ERR calibration failed: error %d (%s)\n", error, + soil_error_str(error)); + return 1; + } + const int error = s_soil->getLastError(); + if (error != 0) { + printf("OK calibration applied (sensor register write failed, " + "non-fatal: error %d (%s))\n", + error, soil_error_str(error)); + } else { + printf("OK calibration applied\n"); + } + return 0; +} + +int soil_cal_moisture_cmd(int argc, char **argv) +{ + return cmd_soil_calibrate(argc, argv, "soil_cal_moisture", + &ISoilSensor::calibrateMoisture); +} + +int soil_cal_ph_cmd(int argc, char **argv) +{ + return cmd_soil_calibrate(argc, argv, "soil_cal_ph", + &ISoilSensor::calibratePH); +} + +int soil_cal_ec_cmd(int argc, char **argv) +{ + return cmd_soil_calibrate(argc, argv, "soil_cal_ec", + &ISoilSensor::calibrateEC); +} + } // namespace void diag_console_register_pumps(IWaterPump& plant, IWaterPump& reservoir) @@ -630,5 +689,50 @@ esp_err_t diag_console_start(void) return err; } + const esp_console_cmd_t cmd_soil_cal_moisture = { + .command = "soil_cal_moisture", + .help = "soil_cal_moisture — calibrate moisture " + "against a reference in %", + .hint = nullptr, + .func = &soil_cal_moisture_cmd, + .argtable = nullptr, + .func_w_context = nullptr, + .context = nullptr, + }; + err = esp_console_cmd_register(&cmd_soil_cal_moisture); + if (err != ESP_OK) { + return err; + } + + const esp_console_cmd_t cmd_soil_cal_ph = { + .command = "soil_cal_ph", + .help = "soil_cal_ph — calibrate pH against a " + "reference", + .hint = nullptr, + .func = &soil_cal_ph_cmd, + .argtable = nullptr, + .func_w_context = nullptr, + .context = nullptr, + }; + err = esp_console_cmd_register(&cmd_soil_cal_ph); + if (err != ESP_OK) { + return err; + } + + const esp_console_cmd_t cmd_soil_cal_ec = { + .command = "soil_cal_ec", + .help = "soil_cal_ec — calibrate EC against a " + "reference in uS/cm", + .hint = nullptr, + .func = &soil_cal_ec_cmd, + .argtable = nullptr, + .func_w_context = nullptr, + .context = nullptr, + }; + err = esp_console_cmd_register(&cmd_soil_cal_ec); + if (err != ESP_OK) { + return err; + } + return esp_console_start_repl(repl); } diff --git a/firmware/test_apps/host/main/test_soil_sensor.cpp b/firmware/test_apps/host/main/test_soil_sensor.cpp index 878de3e..92159f8 100644 --- a/firmware/test_apps/host/main/test_soil_sensor.cpp +++ b/firmware/test_apps/host/main/test_soil_sensor.cpp @@ -16,7 +16,12 @@ * US2 fault paths — timeout, range validation (error 5, all-or-nothing * publish), exception propagation, no-retry, implicit recovery, lazy * (re)initialization, real-bus availability probe, statistics and the - * setTimeout client contract. Calibration is phase 6 (T021). + * setTimeout client contract. T021 adds the calibration flows: factor + * computation from a fresh 1-register raw read, the ×100 best-effort + * calibration-register write (NON-FATAL on failure), read-path application + * for pH/EC (the moisture factor is stored/written but NOT applied — legacy + * parity), the raw-too-low guard (error 5) and range validation running on + * the FACTORED values. */ #include @@ -32,6 +37,15 @@ constexpr uint8_t kAddr = 0x01; constexpr uint16_t kStartReg = 0x0000; constexpr uint16_t kRegCount = 9; +// Calibration raw-read registers (data-model.md; 1-register reads — a +// distinct mock script key from the 9-register data payload above) and the +// calibration factor registers written with factor ×100. +constexpr uint16_t kRegEc = 0x0002; +constexpr uint16_t kRegPh = 0x0003; +constexpr uint16_t kRegMoistureCalib = 0x0100; +constexpr uint16_t kRegPhCalib = 0x0101; +constexpr uint16_t kRegEcCalib = 0x0102; + /// Baseline valid payload: moisture 55.0 %, temperature 23.5 °C, /// EC 1200 µS/cm, pH 6.8, N 45, P 30, K 120 (+ salinity/TDS, unexposed). std::vector goodPayload() @@ -396,6 +410,190 @@ static void test_lazy_init_probe_failure_then_recovery(void) TEST_ASSERT_EQUAL_INT(2, mock.initializeCalls); } +// ========================================================================== +// Calibration (T021, US3): factor computation + ×100 register write, +// read-path application (pH/EC yes, moisture no — parity), non-fatal write +// failure, raw-too-low guard, validation on factored values +// ========================================================================== + +// -------------------------------------------------------------------------- +// calibratePH(): fresh 1-register raw read of 0x0003, factor = +// reference / (raw / 10), factor ×100 written to 0x0101, and the factor IS +// applied to every subsequent read() +// -------------------------------------------------------------------------- +static void test_calibrate_ph_factor_write_and_read_effect(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); // pre-calibration + f.mock.calls.clear(); + + // Raw pH register scripted as 68 → 6.8; reference 7.0 → factor 7.0/6.8. + f.mock.setRegisters(kAddr, kRegPh, {68}); + TEST_ASSERT_TRUE(f.sensor.calibratePH(7.0f)); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + + // Exactly one 1-register raw read + one factor write (×100 truncated: + // 7.0/6.8 = 1.0294… → 102), already initialized so no extra probe. + TEST_ASSERT_EQUAL(2, f.mock.calls.size()); + const MockModbusClient::Call &raw = f.mock.calls[0]; + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Read), + static_cast(raw.type)); + TEST_ASSERT_EQUAL_UINT16(kRegPh, raw.startRegister); + TEST_ASSERT_EQUAL_UINT16(1, raw.count); + const MockModbusClient::Call &write = f.mock.calls[1]; + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Write), + static_cast(write.type)); + TEST_ASSERT_EQUAL_UINT8(kAddr, write.deviceAddress); + TEST_ASSERT_EQUAL_UINT16(kRegPhCalib, write.startRegister); + TEST_ASSERT_EQUAL_UINT16(1, write.count); + TEST_ASSERT_EQUAL_UINT16(102, write.value); + TEST_ASSERT_TRUE(write.succeeded); + + // Read-path effect: raw 68 → 6.8 × factor = 7.0. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(7.0f, f.sensor.getPH()); +} + +// -------------------------------------------------------------------------- +// calibrateEC(): same flow with unscaled raw (0x0002, rawScale 1), factor +// ×100 written to 0x0102, factor applied on subsequent reads +// -------------------------------------------------------------------------- +static void test_calibrate_ec_factor_write_and_read_effect(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(1200.0f, f.sensor.getEC()); // pre-calibration + f.mock.calls.clear(); + + // Raw EC register scripted as 1000 (unscaled); reference 1500 → + // factor 1.5, written as 150. + f.mock.setRegisters(kAddr, kRegEc, {1000}); + TEST_ASSERT_TRUE(f.sensor.calibrateEC(1500.0f)); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL(2, f.mock.calls.size()); + const MockModbusClient::Call &write = f.mock.calls[1]; + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Write), + static_cast(write.type)); + TEST_ASSERT_EQUAL_UINT16(kRegEcCalib, write.startRegister); + TEST_ASSERT_EQUAL_UINT16(150, write.value); + + // Read-path effect: 1200 × 1.5 = 1800. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(1800.0f, f.sensor.getEC()); +} + +// -------------------------------------------------------------------------- +// calibrateMoisture(): the factor is computed and written to 0x0100 (×100) +// but NOT applied in read() — legacy parity: read() publishes raw / 10 with +// "No calibration factor for humidity yet" (see ModbusSoilSensor.cpp header) +// -------------------------------------------------------------------------- +static void test_calibrate_moisture_factor_written_but_not_applied(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); + f.mock.calls.clear(); + + // Raw moisture register scripted as 500 → 50.0 %; reference 55.0 → + // factor 1.1, written as 110. + f.mock.setRegisters(kAddr, kStartReg, {500}); // 1-register key + TEST_ASSERT_TRUE(f.sensor.calibrateMoisture(55.0f)); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL(2, f.mock.calls.size()); + const MockModbusClient::Call &write = f.mock.calls[1]; + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Write), + static_cast(write.type)); + TEST_ASSERT_EQUAL_UINT16(kRegMoistureCalib, write.startRegister); + TEST_ASSERT_EQUAL_UINT16(110, write.value); + + // NO read-path effect (parity): raw 550 still publishes 55.0, not 60.5. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getHumidity()); +} + +// -------------------------------------------------------------------------- +// Failed calibration-register WRITE is NON-FATAL (parity): calibrate*() +// still returns true, getLastError() carries the write error, and the +// factor is applied locally on the next read() +// -------------------------------------------------------------------------- +static void test_calibrate_write_failure_nonfatal_factor_applied(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + f.mock.calls.clear(); + + f.mock.setRegisters(kAddr, kRegPh, {68}); + f.mock.queueOutcome(MockModbusClient::kOk); // raw read succeeds + f.mock.queueOutcome(MockModbusClient::kErrBus); // factor write fails + TEST_ASSERT_TRUE(f.sensor.calibratePH(7.0f)); // still succeeds + TEST_ASSERT_EQUAL_INT(MockModbusClient::kErrBus, f.sensor.getLastError()); + + // The write WAS attempted (and failed) — best-effort, single attempt. + TEST_ASSERT_EQUAL(2, f.mock.calls.size()); + const MockModbusClient::Call &write = f.mock.calls[1]; + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Write), + static_cast(write.type)); + TEST_ASSERT_EQUAL_UINT16(kRegPhCalib, write.startRegister); + TEST_ASSERT_FALSE(write.succeeded); + + // Factor kept locally despite the failed sensor-register write. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(7.0f, f.sensor.getPH()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); +} + +// -------------------------------------------------------------------------- +// Raw value < 0.01 → calibration fails with error 5 (division-by-zero +// guard, legacy codes 7/10/13 mapped onto 5), NO factor write, and the +// previous factor (1.0) stays in effect +// -------------------------------------------------------------------------- +static void test_calibrate_raw_too_low_error5_no_write(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + f.mock.calls.clear(); + + f.mock.setRegisters(kAddr, kRegPh, {0}); // raw 0.0 < 0.01 + TEST_ASSERT_FALSE(f.sensor.calibratePH(7.0f)); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + + // Only the 1-register raw read reached the bus — no write recorded. + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Read), + static_cast(f.mock.calls.front().type)); + + // Factor unchanged: the next read still publishes the unfactored 6.8. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); +} + +// -------------------------------------------------------------------------- +// Validation runs on the FACTORED values (legacy validates after +// multiplying): a factor that pushes pH out of the 3–9 parity range makes +// the next read() fail with error 5 and publish nothing +// -------------------------------------------------------------------------- +static void test_calibration_factor_subject_to_range_validation(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); + + // Raw pH 30 → 3.0; reference 9.0 → factor 3.0 (write value 300). + f.mock.setRegisters(kAddr, kRegPh, {30}); + TEST_ASSERT_TRUE(f.sensor.calibratePH(9.0f)); + + // Data payload pH 6.8 × 3.0 = 20.4 > 9 → range validation rejects the + // whole reading; the last-good (pre-calibration) values survive. + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); + TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); +} + void run_soil_sensor_tests(void) { RUN_TEST(test_decode_known_payload_negative_temperature); @@ -414,4 +612,10 @@ void run_soil_sensor_tests(void) RUN_TEST(test_set_timeout_reaches_client); RUN_TEST(test_lazy_init_client_failure_then_recovery); RUN_TEST(test_lazy_init_probe_failure_then_recovery); + RUN_TEST(test_calibrate_ph_factor_write_and_read_effect); + RUN_TEST(test_calibrate_ec_factor_write_and_read_effect); + RUN_TEST(test_calibrate_moisture_factor_written_but_not_applied); + RUN_TEST(test_calibrate_write_failure_nonfatal_factor_applied); + RUN_TEST(test_calibrate_raw_too_low_error5_no_write); + RUN_TEST(test_calibration_factor_subject_to_range_validation); } diff --git a/specs/004-modbus-soil-sensor/checklists/hil.md b/specs/004-modbus-soil-sensor/checklists/hil.md new file mode 100644 index 0000000..88e361c --- /dev/null +++ b/specs/004-modbus-soil-sensor/checklists/hil.md @@ -0,0 +1,62 @@ +# HIL Checklist: Modbus Soil Sensor (004) — rev1 bench rig + +**Purpose**: hardware-in-the-loop verification of PR-04 at Checkpoint 3 (Paul, bench rig) +**Rig**: ESP32 devkit + RS485 5 Click (ADM3485, manual DE) + NPK soil sensor, 9600 8N1, TX=GPIO16 RX=GPIO17 DE=GPIO25 +**Build**: rev1 target (`sdkconfig.board.rev1_devkit`), flash per `firmware/CLAUDE.md` +**Reference**: acceptance criteria `docs/prd/PR-04-modbus-soil-sensor.md`; spec SC-002/003/005 + +## A. Basic readings (US1, SC-002) + +- [ ] A1. Boot with sensor connected; console `soil` → all 7 values printed + (moisture %, temperature °C, EC µS/cm, pH, N/P/K mg/kg), no error +- [ ] A2. Values match the Arduino unit's readings for the same probe/soil + (within one scaling step, i.e. ±0.1 for moisture/temp/pH, ±1 for EC/NPK) +- [ ] A3. Temperature sign check: chill the probe tip (or verify a plausible + positive value and confirm sign handling was host-tested) — reading is + signed-correct, no 6553.x-style wraparound +- [ ] A4. `rs485test` → success outcome, success counter increments across + repeated runs + +## B. Fault injection & recovery (US2, SC-003) + +- [ ] B1. Disconnect the RS485 A/B pair; `soil` → error after ~3 s (timeout, + error 3), no crash, no watchdog reset, error logged on console +- [ ] B2. Repeat `soil` twice while disconnected → same failure each time, + error counter increments each attempt (no retry storms, no lockup) +- [ ] B3. Reconnect A/B; next `soil` → succeeds without reboot or manual + reinit (SC-003 automatic recovery) +- [ ] B4. Power-cycle the SENSOR only (if rig allows): `soil` fails while + sensor boots, recovers on a later read + +## C. RS485 frame integrity (plan risk 1 — hardware RTS vs TXS0108E margins) + +- [ ] C1. With scope or logic analyzer on DE (GPIO 25) and A/B (optional but + recommended, parity §5 open HIL item): DE asserts before the first start + bit and releases after the last stop bit; request frames are untruncated + at 9600 baud. If truncation appears: STOP, report — fallback is manual + GPIO DE control (research R2), goes back through fixer +- [ ] C2. 20× `soil` in a row → 20 successes (no intermittent framing losses; + check counters via `rs485test`) + +## D. Calibration commands (US4/FR-012, optional bench check) + +- [ ] D1. `soil_cal_ph ` with a known buffer (or plausible dummy + reference) → `OK calibration applied` (or the documented non-fatal + write-failure variant); subsequent `soil` reflects the factored pH +- [ ] D2. `soil_cal_moisture ` → OK, but subsequent `soil` moisture + UNCHANGED (parity: moisture factor stored, never applied on read — + expected behavior, not a bug) + +## E. Regression guard + +- [ ] E1. Pumps still OFF at boot (watch outputs during A1 boot) — safety + invariant untouched by this PR +- [ ] E2. Existing console commands (pump/storage) still respond normally + +## Deferred to PR-14 (rev2 bring-up — NOT this checklist) + +- rev2 THVD1426 echo suppression at 9600 baud (FW-4, research R3) +- rev2 RX pull-up effect with SENS_PWR_EN off (FW-2) +- rev2 auto-direction frame integrity + +**Sign-off**: date + result per item as PR comment (pattern from PR #7). diff --git a/specs/004-modbus-soil-sensor/plan.md b/specs/004-modbus-soil-sensor/plan.md index c25553e..1d4c74c 100644 --- a/specs/004-modbus-soil-sensor/plan.md +++ b/specs/004-modbus-soil-sensor/plan.md @@ -141,18 +141,30 @@ esp_littlefs (research R7). | R8 | Calibration legacy-exact (CP1 answer A), RAM-only factors | FR-012 | | R9 | Console: `soil`, `rs485test`, `soil_cal_*` via LockedSoilSensor | FR-013 | -## Risks & Open Items (carried to tasks/HIL) - -1. **RTS timing vs TXS0108E margins (R2)** — HIL item on the rig; documented - fallback path that stays behind `IModbusClient`. -2. **esp-modbus runtime timeout setter availability (R5)** — implementer verifies - against pinned 2.1.2; degrade `setTimeout` to init-time-only with doc note. -3. **Exception-code granularity (R6)** — possible parity divergence (100+n → single - exception code); record in PR description like PR-06's divergences if hit. -4. **Kconfig timeout clamp (R5)** — assert `CONFIG_FMB_MASTER_TIMEOUT_MS_RESPOND` - allows 3000 in both board sdkconfigs; CI-visible if violated. +## Risks & Open Items (outcomes recorded during implementation, T028) + +1. **RTS timing vs TXS0108E margins (R2)** — OPEN, HIL item C1/C2 in + `checklists/hil.md`; documented fallback (manual GPIO DE) stays behind + `IModbusClient`. +2. **esp-modbus runtime timeout setter (R5)** — CONFIRMED DIVERGENCE: 2.1.2 takes + the timeout via `ser_opts.response_tout_ms` at create time; `setTimeout()` + applies at initialize() only (documented in code; no runtime caller exists). +3. **Exception-code granularity (R6)** — CONFIRMED DIVERGENCE: 2.1.2 + `mbc_master_send_request` does not surface the slave exception number; all + non-timeout failures map to bus error 2 (distinct-from-timeout holds, which is + the safety-bearing part of FR-010). The sensor layer propagates client codes + verbatim, so 100+n granularity returns automatically if a future client + surfaces it. Noted for the PR description. +4. **Kconfig timeout clamp (R5)** — RESOLVED: `FMB_MASTER_TIMEOUT_MS_RESPOND` + range is 150–30000 ms (default 10000); 3000 fits, no override needed (T002). 5. **rev2 echo behavior unverifiable until PR-14** — accepted per spec assumption; - T3.5 resync is the fallback layer. + T3.5 resync is the fallback layer. rev2 target build verified green with + `CONFIG_BOARD_REV2=y` and no DE-pin reference (compile-time proof, T020). + +Additional parity fact confirmed during the port (also corrected in +data-model.md): the legacy read path never applies the moisture calibration +factor (`calibrateMoisture` stores/writes it only); pH/EC are validated on +factored values. Both ported as-is. ## Agent context update diff --git a/specs/004-modbus-soil-sensor/tasks.md b/specs/004-modbus-soil-sensor/tasks.md index f6cafb7..b74d4d4 100644 --- a/specs/004-modbus-soil-sensor/tasks.md +++ b/specs/004-modbus-soil-sensor/tasks.md @@ -134,16 +134,16 @@ unaffected. **Independent test**: both targets build (quickstart §2); rev2 binary has no DE reference; pull-up call present. -- [ ] T018 [US3] Add FW-2 RX pull-up in +- [x] T018 [US3] Add FW-2 RX pull-up in `firmware/components/sensors/src/EspModbusClient.cpp`: after pin setup, `gpio_pullup_en(BOARD_PIN_RS485_RX)` unconditionally with comment referencing FW-2/THVD1426 SHDN̅–SENS_PWR_EN coupling (R4) -- [ ] T019 [US3] Document FW-4 echo strategy in +- [x] T019 [US3] Document FW-4 echo strategy in `firmware/components/sensors/src/EspModbusClient.cpp` (comment at uart_set_mode site): half-duplex RX gating suppresses THVD1426 echo on rev2, T3.5 resync as fallback, PR-14 HIL verifies (R3); confirm no rev1-only assumptions in the shared path -- [ ] T020 [VERIFY-MAIN] [US3] Both board targets build green from clean config +- [x] T020 [VERIFY-MAIN] [US3] Both board targets build green from clean config (quickstart §2) and `strings`/grep of rev2 build objects show no `BOARD_PIN_RS485_DE` usage compiled in (board.h sanity would have failed the build — record the check output) @@ -158,24 +158,24 @@ soil sensor PR-11 will consume; CI runs it all. **Independent test**: full host suite green in CI on linux target. -- [ ] T021 [P] [US4] Extend `firmware/test_apps/host/main/test_soil_sensor.cpp` with +- [x] T021 [P] [US4] Extend `firmware/test_apps/host/main/test_soil_sensor.cpp` with calibration tests: factor = reference/raw from fresh read; factor applied to subsequent reads (EC, pH, moisture); best-effort write to 0x0100/0x0101/0x0102 fn 0x06 with legacy ×100 encoding (assert via mock write log); write failure → calibrate returns success for local part + factor still applied (parity non-fatal) -- [ ] T022 [US4] Implement calibration in +- [x] T022 [US4] Implement calibration in `firmware/components/sensors/src/ModbusSoilSensor.cpp` + `ModbusSoilSensor.h`: `calibrateMoisture/PH/EC` porting the exact legacy formula from `src/sensors/ModbusSoilSensor.cpp:207-322` (READ-ONLY reference); factors RAM-only (R8) -- [ ] T023 [P] [US4] Create `MockSoilSensor` in +- [x] T023 [P] [US4] Create `MockSoilSensor` in `firmware/components/sensors/include/sensors/testing/MockSoilSensor.h` (settable values/validity/errors — the PR-11 consumer fixture) -- [ ] T024 [US4] Add calibration console commands `soil_cal_moisture`, +- [x] T024 [US4] Add calibration console commands `soil_cal_moisture`, `soil_cal_ph`, `soil_cal_ec` in `firmware/main/diag_console.cpp` per console contract (factor + write-result reporting) -- [ ] T025 [VERIFY-MAIN] [US4] Full host suite green (all soil tests + existing 50 +- [x] T025 [VERIFY-MAIN] [US4] Full host suite green (all soil tests + existing 50 tests, exit 0); confirm CI workflow needs no change beyond the new files (esp-idf-ci-action already runs `test_apps/host` with `target: linux`) @@ -183,11 +183,11 @@ soil sensor PR-11 will consume; CI runs it all. ## Phase 7: Polish & cross-cutting -- [ ] T026 [P] Write HIL checklist to +- [x] T026 [P] Write HIL checklist to `specs/004-modbus-soil-sensor/checklists/hil.md` from quickstart §3 (steps, expected outcomes, sign-off boxes; note rev2 items deferred to PR-14) for Paul at Checkpoint 3 -- [ ] T027 [P] Update `firmware/CLAUDE.md`: `sensors` component summary, esp-modbus +- [x] T027 [P] Update `firmware/CLAUDE.md`: `sensors` component summary, esp-modbus pin, console command additions, host-test file list - [ ] T028 Record parity divergences (if any hit during implementation: exception- code granularity R6, setTimeout runtime support R5, RTS-timing fallback R2) From c73321167bd79baaaf76a9616265e49f1ce52914 Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 11:36:36 +0200 Subject: [PATCH 10/11] =?UTF-8?q?docs(spec):=20all=2029=20tasks=20complete?= =?UTF-8?q?=20=E2=80=94=20final=20clean=20verification=20green?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- specs/004-modbus-soil-sensor/tasks.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/004-modbus-soil-sensor/tasks.md b/specs/004-modbus-soil-sensor/tasks.md index b74d4d4..72b5840 100644 --- a/specs/004-modbus-soil-sensor/tasks.md +++ b/specs/004-modbus-soil-sensor/tasks.md @@ -189,11 +189,11 @@ soil sensor PR-11 will consume; CI runs it all. Paul at Checkpoint 3 - [x] T027 [P] Update `firmware/CLAUDE.md`: `sensors` component summary, esp-modbus pin, console command additions, host-test file list -- [ ] T028 Record parity divergences (if any hit during implementation: exception- +- [x] T028 Record parity divergences (if any hit during implementation: exception- code granularity R6, setTimeout runtime support R5, RTS-timing fallback R2) in code comments + `specs/004-modbus-soil-sensor/plan.md` Risks section updates -- [ ] T029 [VERIFY-MAIN] Final clean-checkout verification: rsync fresh copy, +- [x] T029 [VERIFY-MAIN] Final clean-checkout verification: rsync fresh copy, both board builds + host suite from scratch (quickstart §1+§2), commit `dependencies.lock` change deliberately (constitution III) From 36b512bbe73b8b826da2de19cff166b0eb8064df Mon Sep 17 00:00:00 2001 From: Cryptotomte Date: Thu, 2 Jul 2026 14:04:19 +0200 Subject: [PATCH 11/11] =?UTF-8?q?fix(sensors):=20CP3=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20calibration=20input=20hygiene,=20write-echo=20diver?= =?UTF-8?q?gence,=20doc=20accuracy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A1: reject non-finite/non-positive calibration references and factors exceeding the uint16 register encoding (UB before; NaN factors defeated range validation and poisoned readings as valid); console parse_float rejects non-finite input. A2+B1: 7 new host tests (untested validation branches temp>80/pH<3, boundary inclusivity, EC/NPK non-enforcement parity, calibration raw-read failure, invalid-reference and factor- overflow guards) + isAvailable lastError assert. A3: write-echo claim corrected — esp-modbus 2.1.2 validates FC06 framing only, no echoed- value comparison; documented as parity divergence (plan risk 6). B2: initialize() teardown paths log mbc_master_delete failures. B3-B6: interface doc scoping (100+n collapse, setTimeout timing, lazy init, getLastError scope), comment-rot fixes, console argc checks + TODO(PR-11) on rs485test concurrency, spec/contract sync (single-reply parsing wording, no-factor console output). Verified: host suite 79/0 on linux target; rev1 + rev2 builds green. Co-Authored-By: Claude Fable 5 --- firmware/CLAUDE.md | 4 +- .../include/interfaces/IModbusClient.h | 23 ++- .../include/interfaces/ISoilSensor.h | 28 ++- .../sensors/include/sensors/EspModbusClient.h | 8 +- .../include/sensors/ModbusSoilSensor.h | 7 +- .../sensors/src/EspModbusClient.cpp | 43 +++-- .../sensors/src/ModbusSoilSensor.cpp | 30 +++- firmware/main/app_main.cpp | 5 +- firmware/main/diag_console.cpp | 27 ++- firmware/main/idf_component.yml | 5 +- .../test_apps/host/main/test_soil_sensor.cpp | 168 ++++++++++++++++++ .../contracts/interfaces.md | 8 +- specs/004-modbus-soil-sensor/plan.md | 7 + specs/004-modbus-soil-sensor/spec.md | 20 ++- 14 files changed, 323 insertions(+), 60 deletions(-) diff --git a/firmware/CLAUDE.md b/firmware/CLAUDE.md index 82ccdcb..24e18b9 100644 --- a/firmware/CLAUDE.md +++ b/firmware/CLAUDE.md @@ -221,7 +221,9 @@ must fit in 1.5MB per slot. logging. Check IDF errors with `ESP_ERROR_CHECK` or explicit handling. - **Include guards:** `WATERINGSYSTEM_PATH_FILE_H` (e.g. `WATERINGSYSTEM_BOARD_BOARD_H`). -- **Managed dependencies** are pinned exactly in `main/idf_component.yml`; +- **Managed dependencies** are pinned exactly in `main/idf_component.yml` + AND `components/sensors/idf_component.yml` (esp-modbus is pinned `==2.1.2` + in both — bump the two in lockstep or the resolver conflicts); `dependencies.lock` is committed, `managed_components/` is not. ## Testing strategy diff --git a/firmware/components/interfaces/include/interfaces/IModbusClient.h b/firmware/components/interfaces/include/interfaces/IModbusClient.h index 1bc6991..0923cba 100644 --- a/firmware/components/interfaces/include/interfaces/IModbusClient.h +++ b/firmware/components/interfaces/include/interfaces/IModbusClient.h @@ -14,11 +14,16 @@ * * 0 OK * 1 not initialized - * 2 bus/communication error (CRC, framing, truncated response) + * 2 bus/communication error (CRC, framing, truncated response; also + * covers slave exceptions collapsed by implementations that cannot + * surface the exception number — see 100+n) * 3 timeout (no response within the configured timeout) * 5 range validation failed — set by the sensor layer on top of this * interface, never by IModbusClient implementations themselves - * 100+n Modbus slave exception n + * 100+n Modbus slave exception n (when the implementation can surface it + * — EspModbusClient collapses slave exceptions onto code 2, + * research.md R6; only test mocks emit 100+n today; consumers must + * not branch on 100+n) * * Part of the header-only `interfaces` component: no IDF includes allowed. */ @@ -63,14 +68,18 @@ class IModbusClient { /** * @brief Write a single holding register (Modbus function 0x06). * - * Exactly one bus attempt, no retry (parity). Success requires the - * slave's echo response to be received and verified — a sent-but-not- - * acknowledged write reports false. + * Exactly one bus attempt, no retry (parity). Success means the + * addressed slave returned a well-formed FC06 response (address, + * function and CRC validated) — a sent-but-not-acknowledged write + * reports false. Implementations are NOT required to compare the echoed + * register/value byte-for-byte against the request (the legacy client + * did; see the parity-divergence note in EspModbusClient.cpp). * * @param deviceAddress Modbus slave address. * @param registerAddress Holding register address. * @param value Value to write. - * @return true if the write was echoed back by the slave. + * @return true if the slave acknowledged the write with a well-formed + * response. */ virtual bool writeSingleRegister(uint8_t deviceAddress, uint16_t registerAddress, uint16_t value) = 0; @@ -84,6 +93,8 @@ class IModbusClient { * @brief Set the response timeout for subsequent transfers. * * Parity default is 3000 ms (docs/parity-checklist.md §5). + * Implementations may apply the value at initialize() time only + * (EspModbusClient does — research.md R5). * * @param timeoutMs Timeout in milliseconds. */ diff --git a/firmware/components/interfaces/include/interfaces/ISoilSensor.h b/firmware/components/interfaces/include/interfaces/ISoilSensor.h index 47e7e05..3c0f1aa 100644 --- a/firmware/components/interfaces/include/interfaces/ISoilSensor.h +++ b/firmware/components/interfaces/include/interfaces/ISoilSensor.h @@ -36,7 +36,9 @@ class ISoilSensor { /** * @brief Prepare the sensor (brings up the underlying Modbus client if - * needed). Must precede read()/isAvailable(). + * needed). Implementations may initialize lazily — read()/isAvailable()/ + * calibrate*() attempt initialization themselves when it has not + * happened yet — so calling this first is recommended, not required. */ virtual bool initialize() = 0; @@ -65,16 +67,23 @@ class ISoilSensor { virtual bool isAvailable() = 0; /** - * @brief Error code of the most recent operation (0 = OK). + * @brief Error code of the most recent read()/initialize()/calibrate*() + * (0 = OK). * - * Same table as IModbusClient plus code 5 (range validation failed); - * see specs/004-modbus-soil-sensor/data-model.md. + * isAvailable() never touches this code. Note that a calibrate*() + * returning true may still leave a nonzero code from the non-fatal + * calibration-register write (see the calibration block below). + * + * Same table as IModbusClient; code 5 (range validation failed) is + * produced by this layer, never by the client. See + * specs/004-modbus-soil-sensor/data-model.md. */ virtual int getLastError() = 0; - // Values from the most recent successful read(). After a failed read() - // they still hold the previous good reading — check the read() result - // before treating them as fresh. + // 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. /// Soil moisture in percent (0–100). virtual float getMoisture() = 0; @@ -109,8 +118,9 @@ class ISoilSensor { // local correction factor (reference / current raw reading), applies it // to every subsequent read, and best-effort writes the factor to the // sensor's calibration register (0x0100/0x0101/0x0102) — a failed - // sensor-register write is NON-FATAL and does not fail the call. - // Factors are RAM-only in this PR (persistence arrives in PR-09/PR-11). + // sensor-register write is NON-FATAL and does not fail the call: the + // call returns true while getLastError() reports the write error. + // Factors are RAM-only for now; persistence wired in PR-09/PR-11. /// Calibrate moisture against a reference value in percent. virtual bool calibrateMoisture(float referenceValue) = 0; diff --git a/firmware/components/sensors/include/sensors/EspModbusClient.h b/firmware/components/sensors/include/sensors/EspModbusClient.h index 1b5d4bf..f05d358 100644 --- a/firmware/components/sensors/include/sensors/EspModbusClient.h +++ b/firmware/components/sensors/include/sensors/EspModbusClient.h @@ -18,9 +18,11 @@ * place there (research.md R2). * * Concurrency: unsynchronized, like every base implementation in this - * codebase. In this PR the client is only reached from the console REPL - * task (directly by `rs485test` and through LockedSoilSensor by `soil`); - * a second consumer task would need a locking wrapper. + * codebase. As of feature 004 the client is only reached from the console + * REPL task (directly by `rs485test` and through LockedSoilSensor by + * `soil`); a second consumer task would need a locking wrapper. PR-11's + * main-loop reader adds exactly that second task — `rs485test`'s raw access + * must be routed through a locked client wrapper (or the sensor) then. */ #ifndef WATERINGSYSTEM_SENSORS_ESPMODBUSCLIENT_H diff --git a/firmware/components/sensors/include/sensors/ModbusSoilSensor.h b/firmware/components/sensors/include/sensors/ModbusSoilSensor.h index 60729e9..0b2f279 100644 --- a/firmware/components/sensors/include/sensors/ModbusSoilSensor.h +++ b/firmware/components/sensors/include/sensors/ModbusSoilSensor.h @@ -71,8 +71,8 @@ class ModbusSoilSensor : public ISoilSensor { float getPhosphorus() override; float getPotassium() override; - // Calibration (legacy-exact port, research.md R8); host tests for these - // paths land in phase 6 (tasks.md T021). + // Calibration (legacy-exact port, research.md R8); host-tested in + // test_soil_sensor.cpp (calibration suite). bool calibrateMoisture(float referenceValue) override; bool calibratePH(float referenceValue) override; bool calibrateEC(float referenceValue) override; @@ -133,7 +133,8 @@ class ModbusSoilSensor : public ISoilSensor { float phosphorus_ = 0.0f; float potassium_ = 0.0f; - // Calibration factors (RAM-only in this PR; persistence PR-09/PR-11). + // Calibration factors (RAM-only for now; persistence wired in + // PR-09/PR-11). float moistureCalibrationFactor_ = 1.0f; float phCalibrationFactor_ = 1.0f; float ecCalibrationFactor_ = 1.0f; diff --git a/firmware/components/sensors/src/EspModbusClient.cpp b/firmware/components/sensors/src/EspModbusClient.cpp index 8c455cf..d62d675 100644 --- a/firmware/components/sensors/src/EspModbusClient.cpp +++ b/firmware/components/sensors/src/EspModbusClient.cpp @@ -51,17 +51,25 @@ int map_esp_err(esp_err_t err) } } +/** + * @brief mbc_master_delete with a logged (never discarded) failure; nulls + * the handle. Shared by the initialize() failure paths and the destructor. + */ +void delete_master_logged(void*& handle) +{ + const esp_err_t err = mbc_master_delete(handle); + if (err != ESP_OK) { + ESP_LOGE(TAG, "mbc_master_delete failed: %s", esp_err_to_name(err)); + } + handle = nullptr; +} + } // namespace EspModbusClient::~EspModbusClient() { if (mbHandle_ != nullptr) { - const esp_err_t err = mbc_master_delete(mbHandle_); - if (err != ESP_OK) { - ESP_LOGE(TAG, "mbc_master_delete failed: %s", - esp_err_to_name(err)); - } - mbHandle_ = nullptr; + delete_master_logged(mbHandle_); } } @@ -106,8 +114,7 @@ bool EspModbusClient::initialize() UART_PIN_NO_CHANGE); if (err != ESP_OK) { ESP_LOGE(TAG, "uart_set_pin failed: %s", esp_err_to_name(err)); - mbc_master_delete(mbHandle_); - mbHandle_ = nullptr; + delete_master_logged(mbHandle_); lastError_ = 1; return false; } @@ -115,8 +122,7 @@ bool EspModbusClient::initialize() err = mbc_master_start(mbHandle_); if (err != ESP_OK) { ESP_LOGE(TAG, "mbc_master_start failed: %s", esp_err_to_name(err)); - mbc_master_delete(mbHandle_); - mbHandle_ = nullptr; + delete_master_logged(mbHandle_); lastError_ = 1; return false; } @@ -126,14 +132,13 @@ bool EspModbusClient::initialize() // THVD1426 TX echo (RE̅ grounded, receiver always on): echo bytes are // physically simultaneous with transmission and never reach the driver. // The esp-modbus RTU T3.5 frame resynchronization is the fallback for - // residual tail bytes. Electrically verified on rev2 at PR-14. + // residual tail bytes. To be electrically verified on rev2 (HIL, PR-14). err = uart_set_mode(static_cast(BOARD_RS485_UART_PORT), UART_MODE_RS485_HALF_DUPLEX); if (err != ESP_OK) { ESP_LOGE(TAG, "uart_set_mode(RS485 half-duplex) failed: %s", esp_err_to_name(err)); - mbc_master_delete(mbHandle_); - mbHandle_ = nullptr; + delete_master_logged(mbHandle_); lastError_ = 1; return false; } @@ -146,8 +151,7 @@ bool EspModbusClient::initialize() err = gpio_pullup_en(static_cast(BOARD_PIN_RS485_RX)); if (err != ESP_OK) { ESP_LOGE(TAG, "gpio_pullup_en(RX) failed: %s", esp_err_to_name(err)); - mbc_master_delete(mbHandle_); - mbHandle_ = nullptr; + delete_master_logged(mbHandle_); lastError_ = 1; return false; } @@ -210,8 +214,13 @@ bool EspModbusClient::writeSingleRegister(uint8_t deviceAddress, uint16_t registerAddress, uint16_t value) { - // Modbus function 0x06: the esp-modbus master verifies the slave's - // echo response internally — ESP_OK means the write was acknowledged. + // parity divergence (write-echo): esp-modbus 2.1.2 validates the FC06 + // response framing (length/address/function/CRC) but performs NO + // comparison of the echoed register/value against the request. The + // legacy client verified the full 8-byte echo + // (src/communication/SP3485ModbusClient.cpp:266-355, + // docs/parity-checklist.md §5). Same mechanism as R6 above: the gap is + // inside mbc_master_send_request, documented rather than patched. uint16_t writeValue = value; return sendRequest(deviceAddress, 0x06, registerAddress, 1, &writeValue); } diff --git a/firmware/components/sensors/src/ModbusSoilSensor.cpp b/firmware/components/sensors/src/ModbusSoilSensor.cpp index 54d7856..da02969 100644 --- a/firmware/components/sensors/src/ModbusSoilSensor.cpp +++ b/firmware/components/sensors/src/ModbusSoilSensor.cpp @@ -37,6 +37,8 @@ #include "sensors/ModbusSoilSensor.h" +#include + #include "esp_log.h" static const char *TAG = "soilsensor"; @@ -217,11 +219,22 @@ float ModbusSoilSensor::getPotassium() } // Calibration — faithful port of legacy :207-322 (three identical flows -// folded into one helper). Host tests land in phase 6 (tasks.md T021). +// folded into one helper). Host-tested in test_soil_sensor.cpp (calibration +// suite). bool ModbusSoilSensor::calibrate(uint16_t rawRegister, float rawScale, uint16_t calibRegister, float& factor, float referenceValue, const char* quantity) { + // Input hygiene, not parity: legacy accepted any float here. A NaN/inf + // or non-positive reference would poison the factor (and NaN silently + // passes every < comparison below), so reject it up front. + if (!std::isfinite(referenceValue) || referenceValue <= 0.0f) { + lastError_ = 5; + ESP_LOGW(TAG, "calibrate %s failed: invalid reference %.3f", + quantity, static_cast(referenceValue)); + return false; + } + // Lazy initialization (legacy :209-213). if (!initialized_ && !initialize()) { return false; @@ -250,7 +263,20 @@ bool ModbusSoilSensor::calibrate(uint16_t rawRegister, float rawScale, return false; } - factor = referenceValue / currentRawValue; + const float newFactor = referenceValue / currentRawValue; + + // Input hygiene, not parity: legacy static_cast this product straight to + // uint16_t (UB for values outside 0..65535). A factor that large is + // physically absurd, so reject it BEFORE it is stored or encoded (the + // reference guard above makes negative factors unreachable). + if (newFactor * 100.0f > 65535.0f) { + lastError_ = 5; + ESP_LOGW(TAG, "calibrate %s failed: factor %.3f out of encodable " + "range", quantity, static_cast(newFactor)); + return false; + } + + factor = newFactor; // Best-effort factor write (×100) to the sensor's calibration register // — NON-FATAL on failure: the factor is still used locally (parity, diff --git a/firmware/main/app_main.cpp b/firmware/main/app_main.cpp index f86ace3..58e6f9c 100644 --- a/firmware/main/app_main.cpp +++ b/firmware/main/app_main.cpp @@ -179,8 +179,9 @@ extern "C" void app_main(void) // Function-local statics after pumps_force_off() (boot fail-safe rule), // sensor wrapped in the mutex-serializing decorator: accessed from the // console REPL task now and the main-loop controller in PR-11, so EVERY - // sensor access goes through the wrapper. No periodic read task in this - // PR (PR-11) — reads happen on console command only. + // sensor access goes through the wrapper. No periodic read task in + // feature 004 (arrives with PR-11) — reads happen on console command + // only. static EspModbusClient modbus_client; static ModbusSoilSensor soil_sensor_raw(modbus_client); static LockedSoilSensor soil_sensor(soil_sensor_raw); diff --git a/firmware/main/diag_console.cpp b/firmware/main/diag_console.cpp index 4941eca..1a826b7 100644 --- a/firmware/main/diag_console.cpp +++ b/firmware/main/diag_console.cpp @@ -28,7 +28,9 @@ * storage events [n] # newest-first, default 10 * * Soil sensor commands (HIL verification path for feature 004; console - * contract in specs/004-modbus-soil-sensor/contracts/interfaces.md): + * contract in specs/004-modbus-soil-sensor/contracts/interfaces.md — the + * calibration commands print no factor value: no factor getter exists, + * see cmd_soil_calibrate): * * soil # one read(); 7 values or error * rs485test # raw 1-register probe + statistics @@ -44,6 +46,7 @@ #include "diag_console.h" +#include #include #include #include @@ -214,12 +217,13 @@ bool parse_u32(const char *arg, uint32_t &out) return true; } -/// Strict float parse; false on garbage (range checks are the store's job). +/// Strict float parse; false on garbage or non-finite input — nan/inf never +/// reach a float consumer (finite range checks are the consumer's job). bool parse_float(const char *arg, float &out) { char *end = nullptr; out = strtof(arg, &end); - return end != arg && *end == '\0'; + return end != arg && *end == '\0' && std::isfinite(out); } void print_config(const IConfigStore &config) @@ -470,8 +474,11 @@ const char *soil_error_str(int error) /// `soil`: one read() through the locked sensor; all 7 values or the error. int soil_cmd(int argc, char **argv) { - (void)argc; (void)argv; + if (argc != 1) { + printf("ERR usage: soil\n"); + return 1; + } if (s_soil == nullptr) { printf("ERR soil sensor not available\n"); return 1; @@ -496,10 +503,18 @@ int soil_cmd(int argc, char **argv) /// `rs485test`: one raw 1-register probe (slave 0x01, register 0x0000 — /// the parity availability probe) + cumulative transaction statistics. +/// +/// TODO(PR-11): this drives the raw, unsynchronized EspModbusClient BELOW +/// LockedSoilSensor's mutex; once PR-11 adds the main-loop reader this +/// becomes a cross-task race — route it through a locked client wrapper +/// (or through the sensor) then. int rs485test_cmd(int argc, char **argv) { - (void)argc; (void)argv; + if (argc != 1) { + printf("ERR usage: rs485test\n"); + return 1; + } if (s_modbus == nullptr) { printf("ERR modbus client not available\n"); return 1; @@ -526,7 +541,7 @@ int rs485test_cmd(int argc, char **argv) /// Shared `soil_cal_*` flow: one calibrate*() call through the locked /// sensor. ModbusSoilSensor exposes no factor getter (factors are private, -/// RAM-only in this PR), so the output reports success/failure plus the +/// RAM-only for now), so the output reports success/failure plus the /// legacy write-result semantics: a true return with a non-zero last error /// means the factor is applied locally but the best-effort write to the /// sensor's calibration register failed (NON-FATAL, parity). diff --git a/firmware/main/idf_component.yml b/firmware/main/idf_component.yml index dd74fdf..88b3fe0 100644 --- a/firmware/main/idf_component.yml +++ b/firmware/main/idf_component.yml @@ -1,6 +1,7 @@ # Managed dependencies (IDF Component Registry), pinned for reproducible builds. -# Not yet used by app_main — pinned here so dependency resolution is validated -# in CI from phase 0 onwards. +# Used by app_main via components/sensors since feature 004. Keep the esp-modbus +# version in lockstep with components/sensors/idf_component.yml (both pin +# ==2.1.2; bump together or the resolver conflicts). dependencies: idf: ">=6.0.0" espressif/esp-modbus: "==2.1.2" diff --git a/firmware/test_apps/host/main/test_soil_sensor.cpp b/firmware/test_apps/host/main/test_soil_sensor.cpp index 92159f8..af1ca8b 100644 --- a/firmware/test_apps/host/main/test_soil_sensor.cpp +++ b/firmware/test_apps/host/main/test_soil_sensor.cpp @@ -24,6 +24,7 @@ * the FACTORED values. */ +#include #include #include "unity.h" @@ -227,6 +228,82 @@ static void test_out_of_range_ph_rejected_all_or_nothing(void) TEST_ASSERT_EQUAL_FLOAT(120.0f, f.sensor.getPotassium()); } +// -------------------------------------------------------------------------- +// Range validation: temperature > 80 °C (raw 850 = 85.0 °C) → error 5, +// nothing published (upper bound of the parity temperature range) +// -------------------------------------------------------------------------- +static void test_out_of_range_high_temperature_rejected_error5(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + + f.mock.setRegisters(kAddr, kStartReg, {550, 850, 800, 68, 1, 2, 3, 0, 0}); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL_FLOAT(23.5f, f.sensor.getTemperature()); +} + +// -------------------------------------------------------------------------- +// Range validation: pH < 3 (raw 25 = 2.5) → error 5, nothing published +// (lower bound of the parity pH range) +// -------------------------------------------------------------------------- +static void test_out_of_range_low_ph_rejected_error5(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + + f.mock.setRegisters(kAddr, kStartReg, {550, 235, 800, 25, 1, 2, 3, 0, 0}); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); +} + +// -------------------------------------------------------------------------- +// Boundary inclusivity: the parity range limits themselves are VALID — +// moisture 100.0 % (raw 1000), temperature -40.0 °C (raw 0xFE70 = -400) +// and pH 3.0 (raw 30) in one payload; pH 9.0 (raw 90) too. One step past +// the limit (moisture 100.1 %, raw 1001) is invalid with error 5. +// -------------------------------------------------------------------------- +static void test_range_boundaries_are_inclusive(void) +{ + Fixture f({1000, 0xFE70, 800, 30, 1, 2, 3, 0, 0}); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(100.0f, f.sensor.getMoisture()); + TEST_ASSERT_EQUAL_FLOAT(-40.0f, f.sensor.getTemperature()); + TEST_ASSERT_EQUAL_FLOAT(3.0f, f.sensor.getPH()); + + // Upper pH bound (9.0) is valid too. + f.mock.setRegisters(kAddr, kStartReg, {1000, 0xFE70, 800, 90, 1, 2, 3, 0, 0}); + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(9.0f, f.sensor.getPH()); + + // One step past the moisture limit: 100.1 % → error 5. + f.mock.setRegisters(kAddr, kStartReg, {1001, 0xFE70, 800, 90, 1, 2, 3, 0, 0}); + TEST_ASSERT_FALSE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(100.0f, f.sensor.getMoisture()); +} + +// -------------------------------------------------------------------------- +// EC/N/P/K are NOT range-enforced on read (parity, FR-004 second sentence): +// extreme values — up to the uint16 maximum — are published as valid +// -------------------------------------------------------------------------- +static void test_ec_npk_not_range_enforced(void) +{ + Fixture f({550, 235, 65535, 68, 65535, 65535, 65535, 0, 0}); + + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + TEST_ASSERT_EQUAL_FLOAT(65535.0f, f.sensor.getEC()); + TEST_ASSERT_EQUAL_FLOAT(65535.0f, f.sensor.getNitrogen()); + TEST_ASSERT_EQUAL_FLOAT(65535.0f, f.sensor.getPhosphorus()); + TEST_ASSERT_EQUAL_FLOAT(65535.0f, f.sensor.getPotassium()); +} + // -------------------------------------------------------------------------- // Modbus slave exception: the sensor layer propagates the client's error // code VERBATIM (here 100+2 = 102). The real EspModbusClient coarsens @@ -336,6 +413,11 @@ static void test_is_available_performs_real_probe_every_call(void) TEST_ASSERT_FALSE(f.sensor.isAvailable()); TEST_ASSERT_EQUAL(2, f.mock.calls.size()); + // ...without clobbering getLastError(): the probe result is carried by + // the return value only — the fixture's error state (0 after the init) + // is UNCHANGED by the failed probe (read() owns the reading's error). + TEST_ASSERT_EQUAL_INT(0, f.sensor.getLastError()); + // ...and the next call recovers implicitly (no failure latch). TEST_ASSERT_TRUE(f.sensor.isAvailable()); TEST_ASSERT_EQUAL(3, f.mock.calls.size()); @@ -594,6 +676,85 @@ static void test_calibration_factor_subject_to_range_validation(void) TEST_ASSERT_EQUAL_FLOAT(55.0f, f.sensor.getMoisture()); } +// -------------------------------------------------------------------------- +// Failed calibration raw READ is fatal: calibrate*() fails with the +// client's error verbatim, exactly one bus attempt (the raw read), no +// factor write, and the next read() still publishes UNfactored values +// -------------------------------------------------------------------------- +static void test_calibrate_raw_read_failure_error_propagated_no_write(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + f.mock.calls.clear(); + + f.mock.queueOutcome(MockModbusClient::kErrTimeout); // hits the raw read + TEST_ASSERT_FALSE(f.sensor.calibratePH(7.0f)); + TEST_ASSERT_EQUAL_INT(MockModbusClient::kErrTimeout, + f.sensor.getLastError()); + + // Exactly the one failed raw read — no factor write attempted. + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Read), + static_cast(f.mock.calls.front().type)); + TEST_ASSERT_FALSE(f.mock.calls.front().succeeded); + + // Factor unchanged: the next read still publishes the unfactored 6.8. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); +} + +// -------------------------------------------------------------------------- +// Input-hygiene guard (not parity): a non-finite or non-positive reference +// is rejected up front with error 5 — no bus transaction, no factor write, +// factor unchanged +// -------------------------------------------------------------------------- +static void test_calibrate_rejects_invalid_reference(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + f.mock.calls.clear(); + + TEST_ASSERT_FALSE(f.sensor.calibratePH(NAN)); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + TEST_ASSERT_EQUAL(0, f.mock.calls.size()); // guard fires before the bus + + TEST_ASSERT_FALSE(f.sensor.calibratePH(-1.0f)); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + TEST_ASSERT_EQUAL(0, f.mock.calls.size()); + + // Factor unchanged: the next read still publishes the unfactored 6.8. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); +} + +// -------------------------------------------------------------------------- +// Input-hygiene guard (not parity): a factor whose ×100 register encoding +// would exceed uint16 (float→uint16 overflow was UB in the legacy cast) is +// rejected with error 5 BEFORE being stored — no write, factor unchanged. +// Raw pH 1 → 0.1 (passes the raw-too-low guard); reference 7000 → factor +// 70000 → ×100 = 7,000,000 > 65535. +// -------------------------------------------------------------------------- +static void test_calibrate_factor_overflow_rejected_error5(void) +{ + Fixture f(goodPayload()); + TEST_ASSERT_TRUE(f.sensor.read()); + f.mock.calls.clear(); + + f.mock.setRegisters(kAddr, kRegPh, {1}); + TEST_ASSERT_FALSE(f.sensor.calibratePH(7000.0f)); + TEST_ASSERT_EQUAL_INT(5, f.sensor.getLastError()); + + // Only the raw read reached the bus — the overflowing factor was never + // written. + TEST_ASSERT_EQUAL(1, f.mock.calls.size()); + TEST_ASSERT_EQUAL(static_cast(MockModbusClient::Call::Type::Read), + static_cast(f.mock.calls.front().type)); + + // Factor NOT applied: the next read still publishes the unfactored 6.8. + TEST_ASSERT_TRUE(f.sensor.read()); + TEST_ASSERT_EQUAL_FLOAT(6.8f, f.sensor.getPH()); +} + void run_soil_sensor_tests(void) { RUN_TEST(test_decode_known_payload_negative_temperature); @@ -604,6 +765,10 @@ void run_soil_sensor_tests(void) RUN_TEST(test_out_of_range_moisture_rejected_error5); RUN_TEST(test_out_of_range_temperature_rejected_error5); RUN_TEST(test_out_of_range_ph_rejected_all_or_nothing); + RUN_TEST(test_out_of_range_high_temperature_rejected_error5); + RUN_TEST(test_out_of_range_low_ph_rejected_error5); + RUN_TEST(test_range_boundaries_are_inclusive); + RUN_TEST(test_ec_npk_not_range_enforced); RUN_TEST(test_modbus_exception_propagates_verbatim); RUN_TEST(test_failed_read_is_single_bus_attempt); RUN_TEST(test_read_recovers_after_failure_without_reinit); @@ -618,4 +783,7 @@ void run_soil_sensor_tests(void) RUN_TEST(test_calibrate_write_failure_nonfatal_factor_applied); RUN_TEST(test_calibrate_raw_too_low_error5_no_write); RUN_TEST(test_calibration_factor_subject_to_range_validation); + RUN_TEST(test_calibrate_raw_read_failure_error_propagated_no_write); + RUN_TEST(test_calibrate_rejects_invalid_reference); + RUN_TEST(test_calibrate_factor_overflow_rejected_error5); } diff --git a/specs/004-modbus-soil-sensor/contracts/interfaces.md b/specs/004-modbus-soil-sensor/contracts/interfaces.md index e90b7cb..2d3e1f4 100644 --- a/specs/004-modbus-soil-sensor/contracts/interfaces.md +++ b/specs/004-modbus-soil-sensor/contracts/interfaces.md @@ -25,7 +25,11 @@ public: Contract notes: - `readHoldingRegisters`/`writeSingleRegister` perform exactly ONE bus attempt; false ⇒ `getLastError()` is set. No internal retry (parity). -- `writeSingleRegister` verifies the slave's echo response (parity). +- `writeSingleRegister` success means the addressed slave returned a well-formed + FC06 response (address/function/CRC validated); implementations are NOT required + to compare the echoed register/value byte-for-byte against the request (the + legacy client did — documented parity divergence, see EspModbusClient.cpp and + plan.md Risks item 6). - Statistics: every call increments exactly one of success/error. ## interfaces/ISoilSensor.h @@ -69,4 +73,4 @@ Contract notes: |---|---| | `soil` | One `read()`; prints all 7 values or error code + name | | `rs485test` | Raw 1-register probe; prints outcome + success/error counters | -| `soil_cal_moisture ` / `soil_cal_ph ` / `soil_cal_ec ` | Runs calibration; prints new factor and whether the sensor-register write succeeded | +| `soil_cal_moisture ` / `soil_cal_ph ` / `soil_cal_ec ` | Runs calibration; prints success/error plus whether the non-fatal sensor-register write succeeded (no factor value — factors are private, deliberate) | diff --git a/specs/004-modbus-soil-sensor/plan.md b/specs/004-modbus-soil-sensor/plan.md index 1d4c74c..b6c579f 100644 --- a/specs/004-modbus-soil-sensor/plan.md +++ b/specs/004-modbus-soil-sensor/plan.md @@ -160,6 +160,13 @@ esp_littlefs (research R7). 5. **rev2 echo behavior unverifiable until PR-14** — accepted per spec assumption; T3.5 resync is the fallback layer. rev2 target build verified green with `CONFIG_BOARD_REV2=y` and no DE-pin reference (compile-time proof, T020). +6. **Write-echo verification (FC06)** — CONFIRMED DIVERGENCE (same mechanism as + R6): 2.1.2 `mbc_master_send_request` validates the FC06 response framing + (length/address/function/CRC) but performs no comparison of the echoed + register/value against the request; the legacy client verified the full + 8-byte echo (`src/communication/SP3485ModbusClient.cpp:266-355`, + `docs/parity-checklist.md` §5). Documented in EspModbusClient.cpp and the + interface contract (writeSingleRegister doc softened accordingly). Additional parity fact confirmed during the port (also corrected in data-model.md): the legacy read path never applies the moisture calibration diff --git a/specs/004-modbus-soil-sensor/spec.md b/specs/004-modbus-soil-sensor/spec.md index ef954e2..4103b62 100644 --- a/specs/004-modbus-soil-sensor/spec.md +++ b/specs/004-modbus-soil-sensor/spec.md @@ -109,7 +109,9 @@ rev2 bring-up (PR-14) is a validation exercise, not a rewrite. **Independent Test**: Build both board variants in CI; verify the rev1 binary configures the direction pin and the rev2 binary does not; verify by host test that -echo bytes preceding a reply are discarded. +exactly one well-formed reply is parsed per transaction (single-reply parsing via +the mock — echo suppression itself is hardware receive-gating, verified +electrically at PR-14 per plan decision R3/FR-014). **Acceptance Scenarios**: @@ -120,8 +122,11 @@ echo bytes preceding a reply are discarded. is configured or touched (the pin macro does not even exist — compile-time guarantee). 3. **Given** a rev2 build, **When** a request frame is transmitted, **Then** the echo - of that frame received on the RX line is discarded and only the sensor's reply is - parsed. + of that frame never reaches the reply parser and only the sensor's reply is + parsed — echo suppression is hardware receive-gating (RS485 half-duplex TX + gating, plan decision R3), verified electrically at PR-14; host tests verify + single-reply parsing (exactly one well-formed reply per transaction) via the + mock (FR-014). 4. **Given** a rev2 board with the sensor power domain switched off, **When** the receive line would otherwise float, **Then** the internal pull-up on the RX pin keeps the line idle-high so no garbage bytes accumulate. @@ -132,7 +137,7 @@ echo bytes preceding a reply are discarded. An AI developer changes decode, validation or error-handling logic and runs the host test suite in CI. Register decoding (including signed temperature), range validation, -invalid-on-timeout behavior and echo discarding are verified against mock +invalid-on-timeout behavior and single-reply parsing are verified against mock implementations — no devkit, no sensor, failures block the merge. **Why this priority**: Constitution principle II (Host-Testability). The mock soil @@ -265,9 +270,10 @@ decode/validation/timeout tests pass deterministically. - **SC-003**: Disconnecting the A/B pair produces flagged-invalid readings and logged errors with zero crashes or watchdog resets; reconnection recovers automatically on a subsequent read with no operator action (HIL checklist). -- **SC-004**: Host test suite covering decode, validation, timeout and echo handling - runs in CI with zero hardware dependencies and passes deterministically (no flaky - reruns). +- **SC-004**: Host test suite covering decode, validation, timeout and single-reply + parsing (echo suppression itself is hardware receive-gating, verified + electrically at PR-14 per plan decision R3/FR-014) runs in CI with zero hardware + dependencies and passes deterministically (no flaky reruns). - **SC-005**: Operator can run the bus diagnostic and soil reading commands on the first attempt using documented commands, and the output is sufficient to distinguish wiring faults from sensor faults (HIL checklist).