From 25d1c5bc2be446ff9bec7f495a4c6536e3c992a3 Mon Sep 17 00:00:00 2001 From: Claas Date: Sat, 22 Aug 2026 20:06:02 +0200 Subject: [PATCH] Read the fan speed on boot instead of assuming the fans are off The fans keep spinning while the controller resets, but the controller assumed nothing about them: current_set_point started as None and last_fan_state at SetPoint::ZERO. The LEDs and the Home Assistant state were wrong until someone issued a command, and a Home Assistant "on" restored a set point that was never in effect. Reading holding registers (function code 0x03) is now implemented. ReadHoldingRegister asks for exactly one register, which keeps the response a fixed length, and Client::read_holding_register returns its contents. What both functions share is factored out of the write path rather than copied: - send_request drives the line, writes the frame and hands the line back - read_header reads the device address and function code, turns an exception frame into Error::Exception, and returns once the header is the answer that was asked for, so each transaction only reads its own body - A read is rejected if it announces a byte count other than the single register asked for. That is checked before the checksum, because a different length means the wrong bytes were just read and the checksum would fail without naming the actual problem - Exception now covers both functions: 0x03 (response too long) exists only for reads, and WriteRefused became AccessRefused because 0x04 also means a register that cannot be read (specification sections 1.3.1 and 1.3.3) - send_3 is now write_holding_register, to pair with read_holding_register fan_control_routine reads the set point before it waits for anything and sends it into the display Watch, so the LEDs, Home Assistant and the button all start from what the fans are actually doing. read_set_point retries with the write path's backoff, which is now shared, and gives up early if a set point is requested in the meantime: a command is worth more than the state being read, and an unreachable fan costs a timeout per attempt. current_set_point stays an Option rather than becoming a plain SetPoint. A fan that does not answer leaves its state genuinely unknown, and both places that use it already treat not knowing as its own case; filling it with a guess would make them silently wrong instead. last_fan_state no longer starts at zero and no longer records the set points Home Assistant asks for. mqtt_brain_routine watches the confirmed display state of both fans instead, so it is seeded from the boot read and also picks up the speeds set with the button, and it remembers only non-zero speeds since zero is what "on" restores from. It now records what a fan accepted rather than what was asked of it, so a failed write no longer leaves behind a speed that was never in effect. The display Watch went from two receivers to three for this, behind a DISPLAY_STATE_RECEIVERS alias so the count lives in one place. display_routine also reports whether the fans are on or off on the first update after boot. It only published that on a change, and the first update is not a change, so the speed reached Home Assistant without the state it needs to render it. The 250 ms alternating LED pattern documentation.md describes as "while the initial fan speed data is getting read from the fan" needed no work: it is what led_routine already plays before it has an LedState. It just describes reality now instead of blinking until the first command. Untested on hardware. The read path has never run: worth checking that the fans answer 0x03 at all, what they report for a fan that is off, and whether the value comes back with the four least significant bits the fan ignores zeroed or as they were written. Note this removes an accident that kept the fan ping-pong in TODO.md item 4 from happening on a cold boot. current_set_point could not be set before a successful write, and now it can, so a bus that dies right after boot reaches that loop too. Recorded there. Co-Authored-By: Claude Opus 5 --- fan-controller/TODO.md | 100 +++++-- fan-controller/src/main.rs | 222 ++++++++++++--- fan-controller/src/modbus/client.rs | 266 +++++++++++++----- fan-controller/src/modbus/function/code.rs | 2 + fan-controller/src/modbus/function/mod.rs | 2 + .../modbus/function/read_holding_register.rs | 47 ++++ 6 files changed, 494 insertions(+), 145 deletions(-) create mode 100644 fan-controller/src/modbus/function/read_holding_register.rs diff --git a/fan-controller/TODO.md b/fan-controller/TODO.md index bd26c1c..44bd369 100644 --- a/fan-controller/TODO.md +++ b/fan-controller/TODO.md @@ -48,7 +48,7 @@ Untested on hardware. The exception, checksum, and incomplete-frame paths have n enabled into the window where the fan may already be answering. Both want a look at the log on the first flash. -Still open in the same area: `src/modbus/client.rs:187`, why the flush must be blocking to avoid +Still open in the same area: `src/modbus/client.rs:329`, why the flush must be blocking to avoid `WouldBlock`. **2. The MQTT client never reconnects** — done, `src/task.rs` @@ -92,33 +92,75 @@ Follow-up in the same area: nothing is re-announced after reconnecting. Home Ass discovered entities, so the device does not disappear, but its state is whatever it was before the drop until the next fan change publishes a new one. Re-publishing the discovery payload and the current display state on every successful connect needs a signal from the session out to -`display_routine`, which is easier once P1 item 3 has removed the `Option` from the display state. +`display_routine`. P1 item 3 made that worth doing: the display state is now seeded from the fans on +boot, so there is a real state to re-announce rather than a `None`. Messages that `talk` or `handle_publish_send` had picked up but not yet written are also lost when the session is cancelled; for state updates where only the latest value matters that is acceptable. ### P1 — state is wrong after every reset -**3. Read the fan speed on boot** — `README.md`, `src/main.rs:649`, `src/main.rs:561` - -After a reset the fans keep spinning at whatever they were set to, but the controller assumes -nothing: `current_set_point` starts as `None` and `last_fan_state` starts at `SetPoint::ZERO`. So -the LEDs and the Home Assistant state are wrong until someone issues a command, and a Home -Assistant "on" restores a set point that was never actually in effect. - -This needs a read-holding-register function; `src/modbus/function/` currently only implements -`WriteHoldingRegister`. Landing it also removes the `Option` from `current_set_point` and is a -prerequisite for the README item about the button picking up state changed through Home Assistant. - -**4. Fans ping-pong forever when the bus is down** — `src/main.rs:713` +**3. Read the fan speed on boot** — done, `src/modbus/`, `src/main.rs` + +After a reset the fans keep spinning at whatever they were set to, but the controller assumed +nothing: `current_set_point` started as `None` and `last_fan_state` at `SetPoint::ZERO`. The LEDs +and the Home Assistant state were wrong until someone issued a command, and a Home Assistant "on" +restored a set point that was never actually in effect. + +Reading holding registers (function code `0x03`) is now implemented. `ReadHoldingRegister` asks for +exactly one register, which keeps the response a fixed length, and `Client::read_holding_register` +returns its contents. The parts both functions share are factored out of the write path rather than +copied: `send_request` drives the line and writes the frame, and `read_header` reads the address and +function code, turns an exception frame into `Error::Exception`, and returns once the header is the +answer that was asked for, so each transaction only has to read its own body. A read is rejected if +it announces a byte count other than the one register asked for, which is checked before the +checksum because a different length means the wrong bytes were just read. `Exception` covers both +functions now: `0x03` (response too long) only exists for reads, and `WriteRefused` became +`AccessRefused` because `0x04` also means a register that cannot be read. `send_3` was renamed to +`write_holding_register` to pair with it. + +`fan_control_routine` reads the set point before it waits for anything, and sends what it gets into +the display `Watch`, so the LEDs, Home Assistant and the button all start from what the fans are +actually doing. `read_set_point` retries `MAX_ATTEMPTS` times sharing the write path's backoff, and +gives up early if a set point is requested in the meantime — a command is worth more than the state +being read, and an unreachable fan costs a timeout per attempt. + +`current_set_point` stays an `Option` rather than becoming a plain `SetPoint` as this item +originally suggested. A fan that does not answer leaves its state genuinely unknown, and every place +that uses it — the redundant-write check and the "push the other fan back" path — already treats not +knowing as its own case. Filling it with a guess would make those two silently wrong instead. + +`last_fan_state` no longer starts at zero and no longer records the set points that Home Assistant +asks for. `mqtt_brain_routine` now watches the confirmed display state of both fans, so it is seeded +from the boot read and picks up the speeds set with the button as well, and it only remembers +non-zero speeds since zero is the state "on" restores from. It records what a fan actually accepted +rather than what was asked of it, so a failed write no longer leaves a speed behind that was never +in effect. The display `Watch` went from two receivers to three for this, through a +`DISPLAY_STATE_RECEIVERS` alias so the count lives in one place. + +`display_routine` also tells Home Assistant whether the fans are on or off on the first update after +boot. It only published that on a *change*, and the first update is not a change, so the speed +arrived without the on/off state that Home Assistant needs to render it. + +The 250 ms alternating LED pattern that `documentation.md` describes as "while the initial fan speed +data is getting read from the fan" needed no work: it is what `led_routine` already plays before it +has an `LedState`. It just describes reality now instead of blinking until the first command. + +Untested on hardware. The read path has never run: worth checking on the first flash that the fans +answer `0x03` at all, what they report for a fan that is off, and whether the value comes back with +the four least significant bits the fan ignores zeroed or as they were written. + +**4. Fans ping-pong forever when the bus is down** — `src/main.rs:849` After exhausting `MAX_ATTEMPTS`, a fan signals the *other* fan back to its own last known good set point. If the bus itself is down, that fan fails too and signals back, and neither ever stops. It is a slow churn rather than a spin — roughly four attempts at a 5 s timeout plus backoff per -round — but it never terminates and keeps cycling the Modbus mutex. Note it cannot happen on a cold -boot: `current_set_point` is `None` until the first success, so `Option::inspect` does nothing. -The fix options are already written in place at `src/main.rs:714` and `src/main.rs:715` — a -once-only retry strategy carried on the signal, or a counter that detects the ping-pong. +round — but it never terminates and keeps cycling the Modbus mutex. It used to be impossible on a +cold boot, because `current_set_point` stayed `None` until the first successful write and +`Option::inspect` then does nothing. P1 item 3 removed that accident: the boot read can fill +`current_set_point` before any write has succeeded, so a bus that dies right after boot now reaches +this too. The fix options are already written in place at `src/main.rs:850` and `src/main.rs:851` — +a once-only retry strategy carried on the signal, or a counter that detects the ping-pong. ### Cheap win worth slotting in anywhere @@ -136,15 +178,13 @@ change that turns the only meaningful unit tests in the firmware back on. | Where | Item | |---|---| -| `src/main.rs:561` | `last_fan_state` should come from state loaded from the fan, not a hardcoded `ZERO` | -| `src/main.rs:649` | Load the initial fan speed over Modbus so `current_set_point` stops being an `Option` | -| `src/main.rs:589` | Make `is_synchronization_on` configurable through a switch (hardcoded `true`) | -| `src/main.rs:711` | Skip pushing the other fan's speed when a setting allows the fans to run out of sync | -| `src/main.rs:713` | Fix the endless loop when both fans fail and keep signalling each other back | -| `src/main.rs:714` | Option A: a retry strategy on the signal, set to once | -| `src/main.rs:715` | Option B: a counter that detects the loop | -| `src/main.rs:655` | Consider updating the display state even when the new set point equals the current one | -| `src/main.rs:258`, `:271`, `:295`, `:308` | Handle backpressure when the MQTT out channel is full | +| `src/main.rs:633` | Make `is_synchronization_on` configurable through a switch (hardcoded `true`) | +| `src/main.rs:847` | Skip pushing the other fan's speed when a setting allows the fans to run out of sync | +| `src/main.rs:849` | Fix the endless loop when both fans fail and keep signalling each other back | +| `src/main.rs:850` | Option A: a retry strategy on the signal, set to once | +| `src/main.rs:851` | Option B: a counter that detects the loop | +| `src/main.rs:794` | Consider updating the display state even when the new set point equals the current one | +| `src/main.rs:262`, `:275`, `:300`, `:313` | Handle backpressure when the MQTT out channel is full | The four backpressure sites are the same code twice per fan (state update, then speed update) and currently log an error and drop the publish, so Home Assistant silently misses the update. Since @@ -188,10 +228,10 @@ the rest are protocol conformance polish against a broker you control. | Where | Item | |---|---| -| `src/modbus/client.rs:187` | Understand why the flush must be blocking to avoid `WouldBlock` | +| `src/modbus/client.rs:329` | Understand why the flush must be blocking to avoid `WouldBlock` | -Response validation and the short-read hazard are done; see P0 item 1. Reading holding registers is -still unimplemented, which is what blocks P1 item 3. +Response validation and the short-read hazard are done; see P0 item 1, and reading holding +registers is done; see P1 item 3. ### Configuration — `src/configuration.rs` diff --git a/fan-controller/src/main.rs b/fan-controller/src/main.rs index 4302b69..667a245 100644 --- a/fan-controller/src/main.rs +++ b/fan-controller/src/main.rs @@ -103,10 +103,7 @@ async fn gain_control( #[embassy_executor::task] async fn input_routine( pin: PIN_18, - mut display_state: ( - watch::Receiver<'static, CriticalSectionRawMutex, SetPoint, 2>, - watch::Receiver<'static, CriticalSectionRawMutex, SetPoint, 2>, - ), + mut display_state: (DisplayStateReceiver, DisplayStateReceiver), fan_state: ( &'static Signal, &'static Signal, @@ -149,6 +146,15 @@ async fn input_routine( type ModbusMutex = Mutex>; type ModbusOnceLock = OnceLock; +/// How many routines watch a fan's confirmed set point: the displays, the button, and the MQTT +/// brain that restores the last running speed when Home Assistant turns the fans back on +const DISPLAY_STATE_RECEIVERS: usize = 3; +type DisplayStateWatch = Watch; +type DisplayStateSender = + watch::Sender<'static, CriticalSectionRawMutex, SetPoint, DISPLAY_STATE_RECEIVERS>; +type DisplayStateReceiver = + watch::Receiver<'static, CriticalSectionRawMutex, SetPoint, DISPLAY_STATE_RECEIVERS>; + /// This routine takes the latest fan state updates and updates all parts of the device that display a state. /// This includes at the time of writing Home Assistant through MQTT and two status LEDs on the device. /// Displays fan status with 2 LEDs: @@ -158,10 +164,7 @@ type ModbusOnceLock = OnceLock; /// On On -> Fan on high setting #[embassy_executor::task] async fn display_routine( - mut display_state: ( - watch::Receiver<'static, CriticalSectionRawMutex, SetPoint, 2>, - watch::Receiver<'static, CriticalSectionRawMutex, SetPoint, 2>, - ), + mut display_state: (DisplayStateReceiver, DisplayStateReceiver), led_state: &'static Signal, mqtt_out: channel::Sender<'static, CriticalSectionRawMutex, OutgoingPublish, CHANNEL_SIZE>, ) { @@ -243,12 +246,13 @@ async fn display_routine( info!("[Display] Update after debounce"); if let Some(update) = display_update_state.0 { - // Turn on the fan on home assistant if it was off before + // Tell home assistant the fan turned on or off. The first update after boot is not a + // change but still has to be reported, because home assistant has no idea yet // We already checked above if the new state is not the same as the current state - if let Some(command) = current_display_state - .0 - .and_then(|current| SetStateCommandValue::from_change(current, update)) - { + if let Some(command) = current_display_state.0.map_or_else( + || SetStateCommandValue::from_first(update), + |current| SetStateCommandValue::from_change(current, update), + ) { // Update setting before is on state for smoother transition in homeassistant UI let publish = OutgoingPublish::UpdateState { fan: Fan::One, @@ -281,12 +285,13 @@ async fn display_routine( } if let Some(update) = display_update_state.1 { - // Turn on the fan on home assistant if it was off before + // Tell home assistant the fan turned on or off. The first update after boot is not a + // change but still has to be reported, because home assistant has no idea yet // We already checked above if the new state is not the same as the current state - if let Some(command) = current_display_state - .1 - .and_then(|current| SetStateCommandValue::from_change(current, update)) - { + if let Some(command) = current_display_state.1.map_or_else( + || SetStateCommandValue::from_first(update), + |current| SetStateCommandValue::from_change(current, update), + ) { let publish = OutgoingPublish::UpdateState { fan: Fan::Two, payload: command, @@ -354,7 +359,23 @@ enum SetStateCommandValue { Off, } +impl From for SetStateCommandValue { + fn from(speed: SetPoint) -> Self { + if speed == SetPoint::ZERO { + SetStateCommandValue::Off + } else { + SetStateCommandValue::On + } + } +} + impl SetStateCommandValue { + /// The state to report for a set point when there is no previous one to compare against, which + /// is the first update after boot: not a change, but the state the fans were already in + fn from_first(speed: SetPoint) -> Option { + Some(Self::from(speed)) + } + fn from_change(old_speed: SetPoint, new_speed: SetPoint) -> Option { if old_speed == SetPoint::ZERO && new_speed != SetPoint::ZERO { Some(SetStateCommandValue::On) @@ -556,13 +577,36 @@ async fn mqtt_brain_routine( >, fan_one_state: &'static Signal, fan_two_state: &'static Signal, + mut display_state: (DisplayStateReceiver, DisplayStateReceiver), ) { - // Remembering the last fan state for when the home assistant turns the device off and then on again - //TODO use state loaded from fan + // Remembering the last speed the fans were running at for when Home Assistant turns the device + // off and then on again. It comes from the confirmed state rather than from the commands that + // arrive here, so it starts at the speed read back from the fans on boot and also picks up the + // speeds set with the button. Zero is not remembered: it is the state that "on" restores from let mut last_fan_state = (SetPoint::ZERO, SetPoint::ZERO); loop { info!("[MQTT Brain] Waiting for new publish"); - let message = receiver_in.receive().await; + let message = match select3( + receiver_in.receive(), + display_state.0.changed(), + display_state.1.changed(), + ) + .await + { + Either3::First(message) => message, + Either3::Second(set_point) => { + if set_point != SetPoint::ZERO { + last_fan_state.0 = set_point; + } + continue; + } + Either3::Third(set_point) => { + if set_point != SetPoint::ZERO { + last_fan_state.1 = set_point; + } + continue; + } + }; info!("[MQTT Brain] Received publish"); let publish = match message { @@ -595,14 +639,12 @@ async fn mqtt_brain_routine( command: FanCommand::SetSpeed { set_point }, } => match target { Fan::One => { - last_fan_state.0 = set_point; fan_one_state.signal(set_point); if is_synchronization_on { fan_two_state.signal(set_point); } } Fan::Two => { - last_fan_state.1 = set_point; fan_two_state.signal(set_point); if is_synchronization_on { fan_one_state.signal(set_point); @@ -626,6 +668,95 @@ async fn mqtt_brain_routine( } } +/// How often a modbus transaction is attempted before the fan counts as unreachable +const MAX_ATTEMPTS: u8 = 3; + +/// Waits before the next attempt of a modbus transaction: the attempt number squared, times +/// 100 ms. [`MAX_ATTEMPTS`] keeps the attempt number small enough for this to stay well inside a +/// `u64` and inside a second or two +async fn back_off(attempt: u8) { + Timer::after_millis(u64::from(attempt).pow(2) * 100).await; +} + +/// Reads the set point a fan is currently running at. +/// +/// Returns `None` when the fan stays silent, answers with a value outside the set point range, or +/// a set point is requested before the read succeeds. That leaves the state unknown, which +/// everything displaying or restoring a fan state already treats as its own case, so guessing here +/// would be worse than admitting it. +async fn read_set_point( + modbus_mutex: &'static ModbusMutex, + fan_address: modbus::device::Address, + requested_set_point: &'static Signal, + fan_identifier: &str, +) -> Option { + let function = modbus::function::ReadHoldingRegister::new( + fan_address, + fan::holding_registers::REFERENCE_SET_POINT, + ); + + for attempt in 1..=MAX_ATTEMPTS { + info!( + "{} Reading the current set point from the fan", + fan_identifier + ); + let result = modbus_mutex + .lock() + .await + .read_holding_register(&function) + .await; + + match result { + // The fan ignores the four least significant bits of a set point, so the value that + // comes back can be slightly below the one that was written. That is close enough for + // everything this state is used for and rounding it back up would invent precision + Ok(value) => match SetPoint::new(value) { + Ok(set_point) => { + info!( + "{} Fan is running at set point {}", + fan_identifier, *set_point + ); + return Some(set_point); + } + Err(_error) => { + error!( + "{} Fan reports set point {} which is above the maximum of {}", + fan_identifier, + value, + fan::set_point::MAX + ); + return None; + } + }, + Err(error) => error!( + "{} Failed to read the current set point on attempt {}: {:?}", + fan_identifier, attempt, error + ), + } + + // A fan that does not answer takes a timeout per attempt, which is long enough that a + // command can arrive in the meantime. That command is a state worth more than the one + // being read, so stop asking and let it be applied + if requested_set_point.signaled() { + info!( + "{} A set point was requested while reading the current one. Applying that instead", + fan_identifier + ); + return None; + } + + if attempt < MAX_ATTEMPTS { + back_off(attempt).await; + } + } + + error!( + "{} Giving up reading the current set point after {} attempts. The fan state stays unknown until it is set", + fan_identifier, MAX_ATTEMPTS + ); + None +} + /// Receives the fan state updates and sends them to modbus as modbus messages /// After a successful response, this sends an update to the fan display logic unit #[embassy_executor::task(pool_size = 2)] @@ -634,7 +765,7 @@ async fn fan_control_routine( current_fan_speed: &'static Signal, other_fan_speed: &'static Signal, modbus: &'static ModbusOnceLock, - display_state: watch::Sender<'static, CriticalSectionRawMutex, SetPoint, 2>, + display_state: DisplayStateSender, ) { let fan_identifier = match *fan_address { 2 => "[Fan 1]", @@ -646,8 +777,16 @@ async fn fan_control_routine( let modbus_mutex = modbus.get().await; info!("{} MODBUS initialized", fan_identifier); - //TODO load initial fan speed through modbus from fan and make current_speed non optional - let mut current_set_point: Option = None; + // The fans keep running while the controller resets, so whatever they are set to now is the + // state to start from. Reading it back is what lets the LEDs, Home Assistant and the button + // describe what is actually happening instead of assuming the fans are off. + // Stays `None` when the fan cannot be reached, which is honest about not knowing rather than + // guessing at a speed + let mut current_set_point = + read_set_point(modbus_mutex, fan_address, current_fan_speed, fan_identifier).await; + if let Some(set_point) = current_set_point { + display_state.send(set_point); + } 'signal_loop: loop { info!("{} Waiting for fan state update", fan_identifier); let mut set_point = current_fan_speed.wait().await; @@ -676,9 +815,8 @@ async fn fan_control_routine( ); info!("{} Sending fan state update through modbus", fan_identifier); - const MAX_ATTEMPTS: u8 = 3; let mut attempt = 1; - while let Err(error) = modbus.send_3(&function).await + while let Err(error) = modbus.write_holding_register(&function).await && attempt <= MAX_ATTEMPTS { // Release lock so other tasks get a chance to access modbus for sending messages to devices @@ -694,9 +832,7 @@ async fn fan_control_routine( continue 'signal_loop; } - // Exponential backoff - // Safe power of 2 because maximum value is 3 (900ms max) - Timer::after_millis(u64::from(attempt).pow(2) * 100).await; + back_off(attempt).await; info!("{} Waiting for lock on modbus for retry", fan_identifier); modbus = modbus_mutex.lock().await; info!("{} Acquired lock on modbus for retry", fan_identifier); @@ -977,16 +1113,16 @@ async fn main(spawner: Spawner) { // The display state is updated after the fan state has been successfully applied // and is used to update any component that displays the fan state like the LEDs or Home Assistant through MQTT - static FAN_ONE_DISPLAY_STATE: Watch = Watch::new(); - static FAN_TWO_DISPLAY_STATE: Watch = Watch::new(); + static FAN_ONE_DISPLAY_STATE: DisplayStateWatch = Watch::new(); + static FAN_TWO_DISPLAY_STATE: DisplayStateWatch = Watch::new(); let display_receivers = ( FAN_ONE_DISPLAY_STATE .receiver() - .expect("Expected receiver to be configured to allow 2 receivers"), + .expect("Expected the watch to be configured for DISPLAY_STATE_RECEIVERS receivers"), FAN_TWO_DISPLAY_STATE .receiver() - .expect("Expected receiver to be configured to allow 2 receivers"), + .expect("Expected the watch to be configured for DISPLAY_STATE_RECEIVERS receivers"), ); let sender_out = OUT.sender(); @@ -1001,10 +1137,10 @@ async fn main(spawner: Spawner) { let button_receivers = ( FAN_ONE_DISPLAY_STATE .receiver() - .expect("Expected receiver to be configured to allow 2 receivers"), + .expect("Expected the watch to be configured for DISPLAY_STATE_RECEIVERS receivers"), FAN_TWO_DISPLAY_STATE .receiver() - .expect("Expected receiver to be configured to allow 2 receivers"), + .expect("Expected the watch to be configured for DISPLAY_STATE_RECEIVERS receivers"), ); unwrap!(spawner.spawn(input_routine( pin_18, @@ -1012,11 +1148,21 @@ async fn main(spawner: Spawner) { (&FAN_ONE_STATE, &FAN_TWO_STATE) ))); + let brain_receivers = ( + FAN_ONE_DISPLAY_STATE + .receiver() + .expect("Expected the watch to be configured for DISPLAY_STATE_RECEIVERS receivers"), + FAN_TWO_DISPLAY_STATE + .receiver() + .expect("Expected the watch to be configured for DISPLAY_STATE_RECEIVERS receivers"), + ); + let receiver_in = IN.receiver(); unwrap!(spawner.spawn(mqtt_brain_routine( receiver_in, &FAN_ONE_STATE, - &FAN_TWO_STATE + &FAN_TWO_STATE, + brain_receivers ))); let display_fan_one_sender = FAN_ONE_DISPLAY_STATE.sender(); diff --git a/fan-controller/src/modbus/client.rs b/fan-controller/src/modbus/client.rs index 91ade2c..ae2e35d 100644 --- a/fan-controller/src/modbus/client.rs +++ b/fan-controller/src/modbus/client.rs @@ -10,7 +10,7 @@ use embedded_io_async::{Read, ReadExactError, Write}; use crate::{ configuration, - modbus::function::{WriteHoldingRegister, code}, + modbus::function::{ReadHoldingRegister, WriteHoldingRegister, code}, }; /// Which part of the response was being read when something went wrong. The parts are read one @@ -23,6 +23,8 @@ pub(crate) enum Part { Exception, /// The register, the value, and the checksum echoed back after a successful write Echo, + /// The byte count, the register contents, and the checksum that follow a successful read + Data, } /// The device answered, but not with the acknowledgement that was asked for @@ -38,19 +40,25 @@ pub(crate) enum InvalidResponse { Checksum(Part), /// The frame does not echo the request. Holds the whole frame that arrived, which can be /// compared against the request that was logged when it was sent - Echo([u8; RESPONSE_LENGTH]), + Echo([u8; WRITE_RESPONSE_LENGTH]), + /// A read answered with a different number of data bytes than the one register that was asked + /// for. Holds the byte count it announced + ByteCount(u8), } -/// The exception codes the fan documents for write single register. -/// See MODBUS Parameter RadiCal im Spiralgehäuse V1.00, section 1.3.3 +/// The exception codes the fan documents for the two functions this client uses. +/// See MODBUS Parameter RadiCal im Spiralgehäuse V1.00, sections 1.3.1 and 1.3.3 #[derive(Debug, Clone, Copy, defmt::Format)] pub(crate) enum Exception { /// The register address is outside the D000 ... D614 range the fan accepts RegisterOutOfRange, - /// The register could not be written, either because the electronics are defective or because - /// this password level has no write permission for it - WriteRefused, - /// A code the specification does not list for this function + /// Reading only: the answer would exceed the 80 byte maximum telegram length, which means + /// more than 37 or zero registers were asked for + ResponseTooLong, + /// The register could not be read or written, because the electronics are defective or, + /// for a write, because this password level has no write permission for it + AccessRefused, + /// A code the specification does not list for these functions Unknown(u8), } @@ -58,7 +66,8 @@ impl From for Exception { fn from(code: u8) -> Self { match code { 0x02 => Self::RegisterOutOfRange, - 0x04 => Self::WriteRefused, + 0x03 => Self::ResponseTooLong, + 0x04 => Self::AccessRefused, other => Self::Unknown(other), } } @@ -74,7 +83,7 @@ pub(crate) enum Error { ResponseTimeout(Part), /// The UART failed while reading this part of the response ResponseUart(Part), - /// The fan answered with a modbus exception instead of performing the write + /// The fan answered with a modbus exception instead of performing the request Exception(Exception), InvalidResponse(InvalidResponse), } @@ -102,7 +111,14 @@ const HEADER_LENGTH: usize = 2; const IGNORED_SET_POINT_BITS: u16 = 0x000F; /// A successful response to a write holding register request echoes the request back -const RESPONSE_LENGTH: usize = 8; +const WRITE_RESPONSE_LENGTH: usize = 8; + +/// A successful response to a read holding register request: the header, the byte count, the +/// contents of the one register that was asked for, and the checksum +const READ_RESPONSE_LENGTH: usize = 7; + +/// How many data bytes a read of the single register asked for has to announce +const READ_BYTE_COUNT: u8 = 2; /// An exception response replaces the register and value of the echo with a single exception code const EXCEPTION_RESPONSE_LENGTH: usize = 5; @@ -111,6 +127,15 @@ const EXCEPTION_RESPONSE_LENGTH: usize = 5; /// Modbus separates frames by 3.5 characters of silence which is about 2 ms at 19200 baud 8E1 const DISCARD_TIMEOUT: Duration = Duration::from_millis(5); +/// Which of the two fans a device address belongs to, for the log +fn fan_identifier(device_address: u8) -> &'static str { + match device_address { + 2 => "[Fan 1]", + 3 => "[Fan 2]", + _other => "Unknown (oops)", + } +} + /// Modbus transmits the checksum low byte first, unlike the rest of the frame fn is_checksum_valid(frame: &[u8]) -> bool { let (data, checksum) = frame.split_at(frame.len() - 2); @@ -144,36 +169,153 @@ impl<'a, UART: uart::Instance, PIN: Pin> Client<'a, UART, PIN> { } } - pub(crate) async fn send_3(&mut self, message: &WriteHoldingRegister) -> Result<(), Error> { - // For debugging - let fan_identifier = match *message.device_address() { - 2 => "[Fan 1]", - 3 => "[Fan 2]", - _other => "Unknown (oops)", - }; + /// Writes a fan's set point and waits for the fan to acknowledge it + pub(crate) async fn write_holding_register( + &mut self, + message: &WriteHoldingRegister, + ) -> Result<(), Error> { + let fan_identifier = fan_identifier(*message.device_address()); + + let result = self.transact_write(message, fan_identifier).await; + self.clear_line_after(&result, fan_identifier).await; + + result + } + + /// Reads back what a fan currently holds in one of its registers + pub(crate) async fn read_holding_register( + &mut self, + message: &ReadHoldingRegister, + ) -> Result { + let fan_identifier = fan_identifier(*message.device_address()); - let result = self.transact(message, fan_identifier).await; + let result = self.transact_read(message, fan_identifier).await; + self.clear_line_after(&result, fan_identifier).await; - // A failed transaction can leave part of a frame in the receive buffer. Dropping it keeps - // the next transaction from reading those leftovers as its own response. Both fans share - // this UART, so leftovers from one would otherwise be read as an answer from the other. + result + } + + /// A failed transaction can leave part of a frame in the receive buffer. Dropping it keeps the + /// next transaction from reading those leftovers as its own response. Both fans share this + /// UART, so leftovers from one would otherwise be read as an answer from the other. + async fn clear_line_after(&mut self, result: &Result, fan_identifier: &str) { if result.is_err() { self.discard_incoming(fan_identifier).await; } - - result } - async fn transact( + async fn transact_write( &mut self, message: &WriteHoldingRegister, fan_identifier: &str, ) -> Result<(), Error> { + let request = message.as_ref(); + self.send_request(request, fan_identifier).await?; + + // The response is either an echo of the request or a shorter exception frame, so the + // address and function code are read first to find out which one is arriving. Reading + // exactly as many bytes as the frame holds leaves nothing behind for the next transaction. + let mut response = [0u8; WRITE_RESPONSE_LENGTH]; + self.read_header(&mut response, request, fan_identifier) + .await?; + + self.read_exact(&mut response[HEADER_LENGTH..], Part::Echo) + .await?; + + if !is_checksum_valid(&response) { + warn!( + "{} Response failed checksum: {:?}", + fan_identifier, response + ); + return Err(Error::InvalidResponse(InvalidResponse::Checksum( + Part::Echo, + ))); + } + + // The echo repeats the register and the value that were written. The register has to match + // exactly, but the fan ignores the four least significant bits of a set point, and the + // specification does not say whether it echoes back the bits it received or the value it + // stored. Masking those bits on both sides accepts either without accepting a real + // mismatch, and the checksum above still catches a corrupted frame + let echoed_register = u16::from_be_bytes([response[2], response[3]]); + let requested_register = u16::from_be_bytes([request[2], request[3]]); + let echoed_value = u16::from_be_bytes([response[4], response[5]]); + let requested_value = u16::from_be_bytes([request[4], request[5]]); + + if echoed_register != requested_register + || echoed_value & !IGNORED_SET_POINT_BITS != requested_value & !IGNORED_SET_POINT_BITS + { + warn!( + "{} Response {:?} does not echo the request {:?}", + fan_identifier, response, request + ); + return Err(Error::InvalidResponse(InvalidResponse::Echo(response))); + } + + info!( + "{} Fan acknowledged the write: {:?}", + fan_identifier, response + ); + + Ok(()) + } + + async fn transact_read( + &mut self, + message: &ReadHoldingRegister, + fan_identifier: &str, + ) -> Result { + let request = message.as_ref(); + self.send_request(request, fan_identifier).await?; + + // Unlike the write, the answer does not repeat the request: it carries a byte count and + // the register contents. Only one register was asked for, so its length is known in + // advance and the byte count is a check rather than something to act on. + let mut response = [0u8; READ_RESPONSE_LENGTH]; + self.read_header(&mut response, request, fan_identifier) + .await?; + + self.read_exact(&mut response[HEADER_LENGTH..], Part::Data) + .await?; + + // Checked before the checksum: a different byte count means the frame is a different + // length than the one that was just read, so the checksum would fail for a reason that + // does not name the actual problem + if response[2] != READ_BYTE_COUNT { + warn!( + "{} Response announced {:?} data bytes instead of {:?}: {:?}", + fan_identifier, response[2], READ_BYTE_COUNT, response + ); + return Err(Error::InvalidResponse(InvalidResponse::ByteCount( + response[2], + ))); + } + + if !is_checksum_valid(&response) { + warn!( + "{} Response failed checksum: {:?}", + fan_identifier, response + ); + return Err(Error::InvalidResponse(InvalidResponse::Checksum( + Part::Data, + ))); + } + + let value = u16::from_be_bytes([response[3], response[4]]); + info!( + "{} Fan answered the read with {:?}: {:?}", + fan_identifier, value, response + ); + + Ok(value) + } + + /// Drives the line, writes the request, and hands the line back to the fan + async fn send_request(&mut self, request: &[u8], fan_identifier: &str) -> Result<(), Error> { // Write then read // Set pin setting DE (driver enable) to on (high) on the MAX845 to send data self.driver_enable.set_high(); - let request = message.as_ref(); info!("{} Sending message to fan: {:?}", fan_identifier, request); // As ref because &[u8; 8] is not the same as &[u8] with_timeout(configuration::FAN_TIMEOUT, self.uart.write_all(request)) @@ -201,11 +343,20 @@ impl<'a, UART: uart::Instance, PIN: Pin> Client<'a, UART, PIN> { // Close sending data to enable receiving data self.driver_enable.set_low(); - // Read - // The response is either an echo of the request or a shorter exception frame, so the - // address and function code are read first to find out which one is arriving. Reading - // exactly as many bytes as the frame holds leaves nothing behind for the next transaction. - let mut response = [0u8; RESPONSE_LENGTH]; + Ok(()) + } + + /// Reads the device address and function code every response starts with, and the rest of the + /// exception frame when the fan answered with one. Returns once the header is the answer that + /// was asked for, so the caller can read the rest of its own frame into the same buffer. + /// + /// The buffer has to hold at least [`EXCEPTION_RESPONSE_LENGTH`] bytes + async fn read_header( + &mut self, + response: &mut [u8], + request: &[u8], + fan_identifier: &str, + ) -> Result<(), Error> { info!("{} Waiting for response from fan", fan_identifier); self.read_exact(&mut response[..HEADER_LENGTH], Part::Header) .await?; @@ -220,7 +371,8 @@ impl<'a, UART: uart::Instance, PIN: Pin> Client<'a, UART, PIN> { ))); } - if response[1] == code::WRITE_SINGLE_REGISTER | code::EXCEPTION_MASK { + let function_code = request[1]; + if response[1] == function_code | code::EXCEPTION_MASK { self.read_exact( &mut response[HEADER_LENGTH..EXCEPTION_RESPONSE_LENGTH], Part::Exception, @@ -240,62 +392,22 @@ impl<'a, UART: uart::Instance, PIN: Pin> Client<'a, UART, PIN> { let exception = Exception::from(response[2]); error!( - "{} Fan rejected the write with modbus exception {:?}", - fan_identifier, exception + "{} Fan rejected function code {:?} with modbus exception {:?}", + fan_identifier, function_code, exception ); return Err(Error::Exception(exception)); } - if response[1] != code::WRITE_SINGLE_REGISTER { + if response[1] != function_code { warn!( "{} Response used function code {:?} instead of {:?}", - fan_identifier, - response[1], - code::WRITE_SINGLE_REGISTER + fan_identifier, response[1], function_code ); return Err(Error::InvalidResponse(InvalidResponse::FunctionCode( response[1], ))); } - self.read_exact(&mut response[HEADER_LENGTH..], Part::Echo) - .await?; - - if !is_checksum_valid(&response) { - warn!( - "{} Response failed checksum: {:?}", - fan_identifier, response - ); - return Err(Error::InvalidResponse(InvalidResponse::Checksum( - Part::Echo, - ))); - } - - // The echo repeats the register and the value that were written. The register has to match - // exactly, but the fan ignores the four least significant bits of a set point, and the - // specification does not say whether it echoes back the bits it received or the value it - // stored. Masking those bits on both sides accepts either without accepting a real - // mismatch, and the checksum above still catches a corrupted frame - let echoed_register = u16::from_be_bytes([response[2], response[3]]); - let requested_register = u16::from_be_bytes([request[2], request[3]]); - let echoed_value = u16::from_be_bytes([response[4], response[5]]); - let requested_value = u16::from_be_bytes([request[4], request[5]]); - - if echoed_register != requested_register - || echoed_value & !IGNORED_SET_POINT_BITS != requested_value & !IGNORED_SET_POINT_BITS - { - warn!( - "{} Response {:?} does not echo the request {:?}", - fan_identifier, response, request - ); - return Err(Error::InvalidResponse(InvalidResponse::Echo(response))); - } - - info!( - "{} Fan acknowledged the write: {:?}", - fan_identifier, response - ); - Ok(()) } @@ -316,7 +428,7 @@ impl<'a, UART: uart::Instance, PIN: Pin> Client<'a, UART, PIN> { /// Reads until the line has been silent for [`DISCARD_TIMEOUT`] to drop a partial or /// unexpected frame before the next transaction starts async fn discard_incoming(&mut self, fan_identifier: &str) { - let mut discarded = [0u8; RESPONSE_LENGTH]; + let mut discarded = [0u8; WRITE_RESPONSE_LENGTH]; while let Ok(result) = with_timeout(DISCARD_TIMEOUT, self.uart.read(&mut discarded)).await { match result { Ok(0) => break, diff --git a/fan-controller/src/modbus/function/code.rs b/fan-controller/src/modbus/function/code.rs index b5b2c41..6ff4c8b 100644 --- a/fan-controller/src/modbus/function/code.rs +++ b/fan-controller/src/modbus/function/code.rs @@ -1,3 +1,5 @@ +pub const READ_HOLDING_REGISTERS: u8 = 0x03; + pub const WRITE_SINGLE_REGISTER: u8 = 0x06; /// A device reports an error by responding with the function code of the request and this bit set diff --git a/fan-controller/src/modbus/function/mod.rs b/fan-controller/src/modbus/function/mod.rs index ac46344..23eadf8 100644 --- a/fan-controller/src/modbus/function/mod.rs +++ b/fan-controller/src/modbus/function/mod.rs @@ -1,4 +1,6 @@ pub(super) mod code; +pub(crate) mod read_holding_register; pub(crate) mod write_holding_register; +pub(crate) use read_holding_register::ReadHoldingRegister; pub(crate) use write_holding_register::WriteHoldingRegister; diff --git a/fan-controller/src/modbus/function/read_holding_register.rs b/fan-controller/src/modbus/function/read_holding_register.rs new file mode 100644 index 0000000..e925c6c --- /dev/null +++ b/fan-controller/src/modbus/function/read_holding_register.rs @@ -0,0 +1,47 @@ +use crate::modbus; + +/// Reads a single holding register. Modbus can read a range in one request, but the fan controller +/// only ever wants one register at a time and asking for exactly one keeps the response a fixed +/// length. See MODBUS Parameter RadiCal im Spiralgehäuse V1.00, section 1.3.1 +pub(crate) struct ReadHoldingRegister([u8; 8]); + +impl ReadHoldingRegister { + /// How many registers to read, which the request carries as a count rather than a range + const COUNT: u16 = 1; + + pub(crate) fn new( + device_address: modbus::device::Address, + register_address: modbus::register::Address, + ) -> Self { + let register_address = register_address.to_be_bytes(); + let count = Self::COUNT.to_be_bytes(); + let mut data = [ + *device_address, + modbus::function::code::READ_HOLDING_REGISTERS, + register_address[0], + register_address[1], + count[0], + count[1], + // CRC set in next step + 0, + 0, + ]; + + let checksum = modbus::CRC.checksum(&data[..6]).to_be_bytes(); + + // They come out reversed (or is us using to_be_bytes reversed?) + data[6] = checksum[1]; + data[7] = checksum[0]; + Self(data) + } + + pub(crate) fn device_address(&self) -> modbus::device::Address { + self.0[0].into() + } +} + +impl AsRef<[u8]> for ReadHoldingRegister { + fn as_ref(&self) -> &[u8] { + &self.0 + } +} -- 2.51.2