From d4cc8188b5eb17253e14863aba069410ee01fdab Mon Sep 17 00:00:00 2001 From: Claas Date: Sat, 22 Aug 2026 20:29:41 +0200 Subject: [PATCH] Move SetPoint into its own crate so its tests run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #[cfg(test)] module in fan-controller/src/fan/set_point.rs was compiled by nothing: cargo test targets thumbv6m-none-eabi, which has no test harness, and the host target fails because cortex-m uses ARM inline assembly. It had already rotted once and was being kept correct by hand against a scratch copy. SetPoint now lives in the set_point crate, no_std with defmt behind a feature so the same type works on the device and on the host. fan/mod.rs re-exports it, so every fan::set_point::… path in the firmware reads the same as before. Its tests run with `cd set_point && cargo test`. The two that were rotting are back, and two more cover FromStr: every speed Home Assistant asks for arrives as the text of an MQTT payload and goes through it, and nothing tested it before, including what happens for payloads that are not set points at all. to_string takes self by value now. SetPoint is Copy, the call sites are unchanged, and it silences the one clippy warning that came along with the move rather than leaving it in a new crate. CLAUDE.md records the crate and the pattern it establishes for testing anything else in the firmware. It also no longer claims `bacon test -- ` works in fan-controller: that job comes from the stock bacon template and the target has no test harness, so it never could. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 33 ++++--- Cargo.lock | 9 ++ Cargo.toml | 1 + fan-controller/Cargo.toml | 1 + fan-controller/TODO.md | 29 +++--- fan-controller/src/fan/mod.rs | 4 +- fan-controller/src/fan/set_point.rs | 92 ------------------- set_point/Cargo.toml | 11 +++ set_point/src/lib.rs | 132 ++++++++++++++++++++++++++++ 9 files changed, 199 insertions(+), 113 deletions(-) delete mode 100644 fan-controller/src/fan/set_point.rs create mode 100644 set_point/Cargo.toml create mode 100644 set_point/src/lib.rs diff --git a/CLAUDE.md b/CLAUDE.md index ef6ed98..044d641 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,8 +35,9 @@ cd home_assistant_discovery && cargo test -- serialize_custom_example ``` `home_assistant_discovery` uses `insta` snapshots (`src/snapshots/`); accept changes with -`cargo insta review`. `fan-controller` has a `bacon.toml`; `bacon` defaults to `cargo check`, `c` -is bound to `clippy-all`, and `bacon test -- ` runs a single test. +`cargo insta review`. `fan-controller` has a `bacon.toml`; `bacon` defaults to `cargo check` and `c` +is bound to `clippy-all`. Its `test` job comes from the stock template and cannot work — see +Testing reality below. ## Build-time configuration (fan-controller) @@ -62,6 +63,7 @@ is bound to `clippy-all`, and `bacon test -- ` runs a single test. | `fan-controller` | `thumbv6m-none-eabi` | The firmware. Everything below supports it. | | `mqtt` | `no_std` | Protocol-level MQTT types shared between firmware and build script. Feature-gated `defmt` / `serde` so the same types work on device and on host. | | `topic` | `no_std` | The single source of truth for Home Assistant MQTT topic strings, composed at compile time with `const_format`. Used by both the firmware and `build.rs`. | +| `set_point` | `no_std` | The `SetPoint` newtype and its bounds, parsing and formatting. Its own crate purely so it can be tested on the host; re-exported by the firmware as `crate::fan::set_point`. Feature-gated `defmt`. | | `home_assistant_discovery` | host | Serde model of the Home Assistant MQTT discovery payload. Build-dependency only. `components` is a `BTreeMap` so the generated payload is byte-stable across builds. | | `debug-listener` | host | Reads the RS-485/Modbus line off a USB serial adapter to inspect fan traffic. The port path is hardcoded in `src/main.rs`. | @@ -105,7 +107,8 @@ be encoded straight into the TCP buffer without intermediate allocation — ther ## Domain constants -- Set points are 0..=64_000 (`fan::set_point::MAX`), wrapped in the `SetPoint` newtype. +- Set points are 0..=64_000 (`set_point::MAX`, re-exported as `fan::set_point::MAX`), wrapped in + the `SetPoint` newtype. - User-facing speeds are deliberately *not* the full range — `fan::user_setting::{LOW, MEDIUM, HIGH}` are tuned to the house and cap at 50 % to reduce wear. Home Assistant is told `speed_range_max: 32_000` to match. @@ -114,15 +117,25 @@ be encoded straight into the TCP buffer without intermediate allocation — ther ## Testing reality -`home_assistant_discovery` is the only crate whose tests actually run — `cd home_assistant_discovery -&& cargo test`. - `fan-controller` cannot be tested by any normal means, so don't waste time trying: `cargo test` targets `thumbv6m-none-eabi`, which has no test harness, and `cargo test --target -aarch64-apple-darwin` fails because `cortex-m` uses ARM inline assembly. The `#[cfg(test)]` module -in `src/fan/set_point.rs` is therefore never compiled. It is kept correct by hand (verified against -a scratch copy of the module), but it will silently rot again — making `SetPoint` host-testable -would require extracting it into its own crate. +aarch64-apple-darwin` fails because `cortex-m` uses ARM inline assembly. A `#[cfg(test)]` module +anywhere in `fan-controller/src/` is compiled by nothing and will rot unnoticed — `set_point` used +to be one and did. + +Tests therefore only exist in the crates that build for the host: + +```bash +cd set_point && cargo test +``` + +```bash +cd home_assistant_discovery && cargo test +``` + +That is also the way to make firmware logic testable at all: move it into its own `no_std` crate +and re-export it, the way `fan/mod.rs` re-exports `set_point`. Worth doing for anything with rules +of its own; not worth it for code that only exists to drive a peripheral. ## Reference documents diff --git a/Cargo.lock b/Cargo.lock index 542fae3..3dee55c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1027,6 +1027,7 @@ dependencies = [ "rand", "reqwless", "serde_json", + "set_point", "static_cell", "thiserror 2.0.12", "topic", @@ -2130,6 +2131,14 @@ dependencies = [ "winapi", ] +[[package]] +name = "set_point" +version = "0.1.0" +dependencies = [ + "defmt 1.0.1", + "heapless 0.8.0", +] + [[package]] name = "sha2" version = "0.10.8" diff --git a/Cargo.toml b/Cargo.toml index 2945688..99b12fc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,6 +4,7 @@ members = [ "fan-controller", "home_assistant_discovery", "mqtt", + "set_point", "topic", ] resolver = "3" diff --git a/fan-controller/Cargo.toml b/fan-controller/Cargo.toml index 824ee58..71dca6f 100644 --- a/fan-controller/Cargo.toml +++ b/fan-controller/Cargo.toml @@ -46,6 +46,7 @@ panic-probe = "0.3.2" portable-atomic = { version = "1.7", features = ["critical-section"] } rand = { version = "0.8.5", default-features = false } reqwless = { version = "0.12.1", features = ["defmt"] } +set_point = { version = "0.1.0", path = "../set_point", features = ["defmt"] } static_cell = "2" topic = { version = "0.1.0", path = "../topic" } diff --git a/fan-controller/TODO.md b/fan-controller/TODO.md index 1cf49ce..d1df449 100644 --- a/fan-controller/TODO.md +++ b/fan-controller/TODO.md @@ -8,11 +8,12 @@ treat them as a starting point rather than an exact address. The first section is a suggested order of work with the reasoning; the sections after it are the full inventory grouped by area, so nothing gets lost. -All four ranked items are done. They are kept here rather than deleted because each one records -what was actually wrong, what was decided, and what has never run on hardware — the four write-ups -are the closest thing this firmware has to a changelog with reasons. Nothing that follows is ranked; -pick from the inventory. The one thing worth doing before anything else is flashing the device and -watching the log, because every one of the four is untested on hardware. +Everything in the priority section is done: the four ranked items and the cheap win. They are kept +here rather than deleted because each one records what was actually wrong, what was decided, and +what has never run on hardware — the write-ups are the closest thing this firmware has to a +changelog with reasons. Nothing that follows is ranked; pick from the inventory. The one thing +worth doing before anything else is flashing the device and watching the log, because all four of +the ranked items are untested on hardware. --- @@ -189,11 +190,20 @@ which means pulling the bus rather than anything Home Assistant can ask for. ### Cheap win worth slotting in anywhere -**Make `SetPoint` host-testable** — `src/fan/set_point.rs:67` +**Make `SetPoint` host-testable** — done, `set_point/` -The `#[cfg(test)]` module never compiles, and `CLAUDE.md` records that it has already rotted once -and is kept correct by hand. Extracting `SetPoint` into its own crate is a small, self-contained -change that turns the only meaningful unit tests in the firmware back on. +The `#[cfg(test)]` module in `src/fan/set_point.rs` never compiled, and it had already rotted once +and was being kept correct by hand. `SetPoint` now lives in its own `no_std` crate at +`set_point/`, with `defmt` behind a feature so the same type works on the device and on the host, +and `fan/mod.rs` re-exports it so every `fan::set_point::…` path still reads the same. + +Its tests run: `cd set_point && cargo test`. The two that were rotting are back, and two more cover +`FromStr`, which every speed Home Assistant asks for goes through and which nothing tested before — +including the payloads that are not set points at all. + +This is the pattern for testing anything else in the firmware: move the logic into its own crate +and re-export it. Recorded in `CLAUDE.md`, which also no longer claims `bacon test` works in +`fan-controller`, because that job is from the stock template and the target has no test harness. --- @@ -267,7 +277,6 @@ or persistent storage in the firmware at all today. | Where | Item | |---|---| -| `src/fan/set_point.rs:67` | Move `SetPoint` into a host-testable crate so its tests actually run | | `documentation.md:53` | Write the Wiring section: Pico W, debug probe, MAX485, status LEDs, button | ### Remaining items from `README.md` diff --git a/fan-controller/src/fan/mod.rs b/fan-controller/src/fan/mod.rs index 35034c2..637ef88 100644 --- a/fan-controller/src/fan/mod.rs +++ b/fan-controller/src/fan/mod.rs @@ -1,7 +1,9 @@ //! ebm-pabst [RadiCal centrifugal fans in scroll housings for residential ventilation](https://www.ebmpapst.com/us/en/campaigns/product-campaigns/centrifugal-fans/radical-with-scroll-housing.html) //! specific configuration and constants -pub(crate) mod set_point; +/// Its own crate so it can be tested on the host, re-exported here because it is part of what a +/// fan is. See the crate documentation for why it cannot live in this one +pub(crate) use ::set_point; use embassy_rp::uart::{self, DataBits, Parity, StopBits}; diff --git a/fan-controller/src/fan/set_point.rs b/fan-controller/src/fan/set_point.rs deleted file mode 100644 index 03d0d5c..0000000 --- a/fan-controller/src/fan/set_point.rs +++ /dev/null @@ -1,92 +0,0 @@ -use core::{ops::Deref, str::FromStr}; - -use defmt::Format; - -pub(crate) const MAX: u16 = 64_000; - -/// Describes the desired speed of the fan from 0 to [`MAX_SET_POINT`] -#[derive(Debug, Format, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -pub(crate) struct SetPoint(u16); - -#[derive(Debug, Format, PartialEq, Eq)] -pub(crate) struct SetPointOutOfBoundsError; - -impl SetPoint { - pub(crate) const ZERO: Self = match Self::new(0) { - Ok(setting) => setting, - Err(_error) => panic!("Invalid value. This should not be reachable."), - }; - - pub(crate) const fn new(set_point: u16) -> Result { - if set_point > MAX { - return Err(SetPointOutOfBoundsError); - } - - Ok(Self(set_point)) - } - - /// This should always succeed - pub(crate) fn to_string(&self) -> heapless::String<5> { - heapless::String::<5>::try_from(self.0) - .expect("The maximum value for a u16 is 65535 which is a 5 digit number and should should be represented as a string with 5 characters and thus fit into a string with a capacity of 5") - } -} - -impl Deref for SetPoint { - type Target = u16; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -pub(crate) enum ParseSetPointError { - ParseInt, - SettingOutOfBounds(SetPointOutOfBoundsError), -} - -impl From for ParseSetPointError { - fn from(_error: core::num::ParseIntError) -> Self { - ParseSetPointError::ParseInt - } -} - -impl FromStr for SetPoint { - type Err = ParseSetPointError; - - fn from_str(s: &str) -> Result { - let set_point = s.parse()?; - Self::new(set_point).map_err(ParseSetPointError::SettingOutOfBounds) - } -} - -#[cfg(test)] -mod tests { - //! These tests cannot run in this crate: it only builds for `thumbv6m-none-eabi`, which has no - //! test harness, and it cannot build for the host because `cortex-m` uses ARM inline assembly. - //TODO move `SetPoint` into a host-testable crate so these actually run - extern crate std; - - use super::*; - - /// These are important hardcoded values I want to make sure are not changed accidentally - #[test] - fn setting_does_not_exceed_max_set_point() { - core::assert_eq!(MAX, 64_000); - core::assert_eq!(SetPoint::new(64_000), Ok(SetPoint(64_000))); - core::assert_eq!(SetPoint::new(64_000 + 1), Err(SetPointOutOfBoundsError)); - core::assert_eq!(SetPoint::new(u16::MAX), Err(SetPointOutOfBoundsError)); - } - - #[test] - fn fits_into_string() -> Result<(), SetPointOutOfBoundsError> { - let set_point = SetPoint::new(12_345)?; - core::assert_eq!(set_point.to_string().len(), 5); - - let set_point = SetPoint::new(MAX)?; - core::assert_eq!(*set_point, 64_000); - core::assert_eq!(set_point.to_string(), "64000"); - - Ok(()) - } -} diff --git a/set_point/Cargo.toml b/set_point/Cargo.toml new file mode 100644 index 0000000..2432b42 --- /dev/null +++ b/set_point/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "set_point" +version = "0.1.0" +edition = "2024" + +[dependencies] +defmt = { workspace = true, optional = true } +heapless = "0.8.0" + +[features] +defmt = ["dep:defmt"] diff --git a/set_point/src/lib.rs b/set_point/src/lib.rs new file mode 100644 index 0000000..366aec6 --- /dev/null +++ b/set_point/src/lib.rs @@ -0,0 +1,132 @@ +//! The desired speed of an ebm-papst RadiCal fan. +//! +//! This lives in its own crate so it can be tested on the host. `fan-controller` only builds for +//! `thumbv6m-none-eabi`, which has no test harness, and it cannot build for the host because +//! `cortex-m` uses ARM inline assembly, so anything left in there is compiled by nothing and rots +//! unnoticed. + +#![no_std] + +use core::{ops::Deref, str::FromStr}; + +/// The highest value the fan accepts. In speed control it means the maximum revolutions per +/// minute the fan is configured for, and zero means standstill. +/// See MODBUS Parameter RadiCal im Spiralgehäuse V1.00, section 2.3 +pub const MAX: u16 = 64_000; + +/// Describes the desired speed of the fan from 0 to [`MAX`] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub struct SetPoint(u16); + +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +#[derive(Debug, PartialEq, Eq)] +pub struct SetPointOutOfBoundsError; + +impl SetPoint { + pub const ZERO: Self = match Self::new(0) { + Ok(setting) => setting, + Err(_error) => panic!("Invalid value. This should not be reachable."), + }; + + pub const fn new(set_point: u16) -> Result { + if set_point > MAX { + return Err(SetPointOutOfBoundsError); + } + + Ok(Self(set_point)) + } + + /// This should always succeed + pub fn to_string(self) -> heapless::String<5> { + heapless::String::<5>::try_from(self.0) + .expect("The maximum value for a u16 is 65535 which is a 5 digit number and should should be represented as a string with 5 characters and thus fit into a string with a capacity of 5") + } +} + +impl Deref for SetPoint { + type Target = u16; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +#[derive(Debug, PartialEq, Eq)] +pub enum ParseSetPointError { + ParseInt, + SettingOutOfBounds(SetPointOutOfBoundsError), +} + +impl From for ParseSetPointError { + fn from(_error: core::num::ParseIntError) -> Self { + ParseSetPointError::ParseInt + } +} + +impl FromStr for SetPoint { + type Err = ParseSetPointError; + + fn from_str(s: &str) -> Result { + let set_point = s.parse()?; + Self::new(set_point).map_err(ParseSetPointError::SettingOutOfBounds) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// These are important hardcoded values I want to make sure are not changed accidentally + #[test] + fn setting_does_not_exceed_max_set_point() { + assert_eq!(MAX, 64_000); + assert_eq!(SetPoint::new(64_000), Ok(SetPoint(64_000))); + assert_eq!(SetPoint::new(64_000 + 1), Err(SetPointOutOfBoundsError)); + assert_eq!(SetPoint::new(u16::MAX), Err(SetPointOutOfBoundsError)); + } + + #[test] + fn fits_into_string() -> Result<(), SetPointOutOfBoundsError> { + let set_point = SetPoint::new(12_345)?; + assert_eq!(set_point.to_string().len(), 5); + + let set_point = SetPoint::new(MAX)?; + assert_eq!(*set_point, 64_000); + assert_eq!(set_point.to_string(), "64000"); + + Ok(()) + } + + /// Every speed Home Assistant asks for arrives as the text of a MQTT payload and comes through + /// here, including whatever a hand written payload contains + #[test] + fn parses_from_a_payload() { + assert_eq!("0".parse(), Ok(SetPoint::ZERO)); + assert_eq!("19393".parse(), Ok(SetPoint::new(19_393).unwrap())); + assert_eq!("64000".parse::(), Ok(SetPoint::new(MAX).unwrap())); + } + + #[test] + fn rejects_a_payload_that_is_not_a_set_point() { + assert_eq!( + "64001".parse::(), + Err(ParseSetPointError::SettingOutOfBounds( + SetPointOutOfBoundsError + )) + ); + // Above u16 rather than above the maximum set point, so it does not even parse as a number + assert_eq!( + "65536".parse::(), + Err(ParseSetPointError::ParseInt) + ); + assert_eq!("".parse::(), Err(ParseSetPointError::ParseInt)); + assert_eq!( + "high".parse::(), + Err(ParseSetPointError::ParseInt) + ); + assert_eq!("-1".parse::(), Err(ParseSetPointError::ParseInt)); + assert_eq!("1.5".parse::(), Err(ParseSetPointError::ParseInt)); + } +} -- 2.51.2