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)); + } +}