diff --git a/fan-controller/TODO.md b/fan-controller/TODO.md index 44bd369..1cf49ce 100644 --- a/fan-controller/TODO.md +++ b/fan-controller/TODO.md @@ -8,11 +8,17 @@ 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. + --- ## Suggested priority -### P0 — the two items that make the device wrong or dead in the field +### P0 — the two items that made the device wrong or dead in the field **1. Validate the Modbus response** — done, `src/modbus/client.rs` @@ -97,7 +103,7 @@ 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 +### P1 — state was wrong after every reset **3. Read the fan speed on boot** — done, `src/modbus/`, `src/main.rs` @@ -149,18 +155,37 @@ Untested on hardware. The read path has never run: worth checking on the first f 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` +**4. Fans ping-pong forever when the bus is down** — done, `src/main.rs` + +After exhausting `MAX_ATTEMPTS`, a fan signalled the *other* fan back to its own last known good set +point, to keep the two from drifting apart and putting the house under or over pressure. If the bus +itself was down, that fan failed too and signalled back, and neither ever stopped. A slow churn +rather than a spin — roughly four attempts at a 5 s timeout plus backoff per round — but it never +terminated and kept cycling the Modbus mutex. + +The signal now carries where the set point came from, as `RequestedSetPoint::FromUser` or +`FromOtherFan`, and only a request from a user pushes the other fan back when it fails. That is +option A from the notes that used to sit here, a retry strategy on the signal set to once, except +the "once" falls out of what the value means rather than being counted. + +The reasoning is that a correction which fails is not the two fans disagreeing, it is the bus being +down, and there is nothing left for a second correction to fix: the pushing fan is at its own set +point and the fan being pushed already failed at that exact value. Answering it with another +correction is precisely what bounced the same value back and forth. So a correction is applied and +retried like anything else, but it never produces another correction, and every round of signalling +ends after at most two: one if a single fan failed, two if both were commanded at once and both +failed. + +That leaves the fans genuinely out of sync in one case — a fan that fails to apply a correction — +which is the honest outcome, because nothing else can be tried until the bus comes back. It is not +silent: the display state is only updated on a confirmed write, so the two LEDs blink the +out-of-sync pattern until a later command succeeds. -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. +Option B, a counter that detects the loop, is not needed on top of this. It would spot the same +situation later and without saying why it happened. -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. 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. +Untested on hardware, and hard to reach on purpose: it needs both fans to fail after the retries, +which means pulling the bus rather than anything Home Assistant can ask for. ### Cheap win worth slotting in anywhere @@ -178,13 +203,10 @@ change that turns the only meaningful unit tests in the firmware back on. | Where | Item | |---|---| -| `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 | +| `src/main.rs:665` | Make `is_synchronization_on` configurable through a switch (hardcoded `true`) | +| `src/main.rs:898` | Skip pushing the other fan's speed when a setting allows the fans to run out of sync | +| `src/main.rs:834` | Consider updating the display state even when the new set point equals the current one | +| `src/main.rs:294`, `:307`, `:332`, `:345` | 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 diff --git a/fan-controller/src/main.rs b/fan-controller/src/main.rs index 667a245..2e792eb 100644 --- a/fan-controller/src/main.rs +++ b/fan-controller/src/main.rs @@ -104,10 +104,7 @@ async fn gain_control( async fn input_routine( pin: PIN_18, mut display_state: (DisplayStateReceiver, DisplayStateReceiver), - fan_state: ( - &'static Signal, - &'static Signal, - ), + fan_state: (&'static SetPointSignal, &'static SetPointSignal), ) { // The button just rotates through fan settings. This is because we currently only have one button // Will probably use something more advanced in the future @@ -138,14 +135,49 @@ async fn input_routine( SetPoint::ZERO }; - fan_state.0.signal(next_set_point); - fan_state.1.signal(next_set_point); + fan_state + .0 + .signal(RequestedSetPoint::FromUser(next_set_point)); + fan_state + .1 + .signal(RequestedSetPoint::FromUser(next_set_point)); } } type ModbusMutex = Mutex>; type ModbusOnceLock = OnceLock; +/// A set point signalled to a [`fan_control_routine`], together with where it came from. The +/// origin is what decides whether a fan that cannot be reached drags the other one with it +#[derive(Clone, Copy, Format)] +enum RequestedSetPoint { + /// Home Assistant or the button asked for this speed + FromUser(SetPoint), + /// The other fan is pushing this one back to the speed they were last in sync at, because its + /// own write failed + FromOtherFan(SetPoint), +} + +impl RequestedSetPoint { + fn set_point(self) -> SetPoint { + match self { + Self::FromUser(set_point) | Self::FromOtherFan(set_point) => set_point, + } + } + + /// Whether failing to apply this should push the other fan back, to keep the two from drifting + /// apart and putting the house under or over pressure. + /// + /// Only a request from a user does. A correction that fails means the bus is down rather than + /// the two fans disagreeing, and answering it with another correction is exactly what made the + /// fans signal each other back and forth without ever stopping + fn should_correct_other_fan(self) -> bool { + matches!(self, Self::FromUser(_)) + } +} + +type SetPointSignal = Signal; + /// 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; @@ -575,8 +607,8 @@ async fn mqtt_brain_routine( Result, CHANNEL_SIZE, >, - fan_one_state: &'static Signal, - fan_two_state: &'static Signal, + fan_one_state: &'static SetPointSignal, + fan_two_state: &'static SetPointSignal, mut display_state: (DisplayStateReceiver, DisplayStateReceiver), ) { // Remembering the last speed the fans were running at for when Home Assistant turns the device @@ -639,15 +671,15 @@ async fn mqtt_brain_routine( command: FanCommand::SetSpeed { set_point }, } => match target { Fan::One => { - fan_one_state.signal(set_point); + fan_one_state.signal(RequestedSetPoint::FromUser(set_point)); if is_synchronization_on { - fan_two_state.signal(set_point); + fan_two_state.signal(RequestedSetPoint::FromUser(set_point)); } } Fan::Two => { - fan_two_state.signal(set_point); + fan_two_state.signal(RequestedSetPoint::FromUser(set_point)); if is_synchronization_on { - fan_one_state.signal(set_point); + fan_one_state.signal(RequestedSetPoint::FromUser(set_point)); } } }, @@ -656,12 +688,20 @@ async fn mqtt_brain_routine( command: FanCommand::SetState(new_state), } => match target { Fan::One => match new_state { - SetStateCommandValue::On => fan_one_state.signal(last_fan_state.0), - SetStateCommandValue::Off => fan_one_state.signal(SetPoint::ZERO), + SetStateCommandValue::On => { + fan_one_state.signal(RequestedSetPoint::FromUser(last_fan_state.0)) + } + SetStateCommandValue::Off => { + fan_one_state.signal(RequestedSetPoint::FromUser(SetPoint::ZERO)) + } }, Fan::Two => match new_state { - SetStateCommandValue::On => fan_two_state.signal(last_fan_state.1), - SetStateCommandValue::Off => fan_two_state.signal(SetPoint::ZERO), + SetStateCommandValue::On => { + fan_two_state.signal(RequestedSetPoint::FromUser(last_fan_state.1)) + } + SetStateCommandValue::Off => { + fan_two_state.signal(RequestedSetPoint::FromUser(SetPoint::ZERO)) + } }, }, } @@ -687,7 +727,7 @@ async fn back_off(attempt: u8) { async fn read_set_point( modbus_mutex: &'static ModbusMutex, fan_address: modbus::device::Address, - requested_set_point: &'static Signal, + requested_set_point: &'static SetPointSignal, fan_identifier: &str, ) -> Option { let function = modbus::function::ReadHoldingRegister::new( @@ -762,8 +802,8 @@ async fn read_set_point( #[embassy_executor::task(pool_size = 2)] async fn fan_control_routine( fan_address: modbus::device::Address, - current_fan_speed: &'static Signal, - other_fan_speed: &'static Signal, + current_fan_speed: &'static SetPointSignal, + other_fan_speed: &'static SetPointSignal, modbus: &'static ModbusOnceLock, display_state: DisplayStateSender, ) { @@ -789,8 +829,8 @@ async fn fan_control_routine( } 'signal_loop: loop { info!("{} Waiting for fan state update", fan_identifier); - let mut set_point = current_fan_speed.wait().await; - if current_set_point.is_some_and(|speed| speed == set_point) { + let mut request = current_fan_speed.wait().await; + if current_set_point.is_some_and(|speed| speed == request.set_point()) { //TODO consider to update fan display state nonetheless info!( "{} Fan state update received but has same state", @@ -799,14 +839,15 @@ async fn fan_control_routine( continue; } - info!("{} Received fan state", fan_identifier); + info!("{} Received fan state {:?}", fan_identifier, request); // Instruct modbus to send update info!("{} Attempting to acquire lock (again?)", fan_identifier); let mut modbus = modbus_mutex.lock().await; info!("{} Acquired lock on modbus (again?)", fan_identifier); // Check we have the latest state in case it was updated while waiting for the lock - set_point = current_fan_speed.try_take().unwrap_or(set_point); + request = current_fan_speed.try_take().unwrap_or(request); + let set_point = request.set_point(); let function = modbus::function::WriteHoldingRegister::new( fan_address, @@ -844,14 +885,21 @@ async fn fan_control_routine( fan_identifier, MAX_ATTEMPTS ); + if !request.should_correct_other_fan() { + // This was already the other fan pushing this one back. Pushing back a second time + // is what used to bounce the same correction between the two fans forever + info!( + "{} Leaving the other fan alone because this was its own correction", + fan_identifier + ); + continue; + } + //TODO don't try to update other fan speed if we have a setting to allow fans to run out of sync // Set other fan to current fan speed to avoid them getting out of sync and creating over or underpressure in the house - //TODO fix endless loop when both fan speed setting fails and they keep retrying and sending each other instructions to set back to previous speed. - //TODO Could additionally provide a retry strategy to the signal that is set to once to avoid endless loop - //TODO or provide a counter to detect the endless loop - // There is no Option::copied or Option::cloned for some reason in core - current_set_point.inspect(|speed| other_fan_speed.signal(*speed)); + current_set_point + .inspect(|speed| other_fan_speed.signal(RequestedSetPoint::FromOtherFan(*speed))); continue; } @@ -1131,8 +1179,8 @@ async fn main(spawner: Spawner) { info!("[Main] Sent out discocery"); unwrap!(spawner.spawn(display_routine(display_receivers, &LED_STATE, sender_out))); - static FAN_ONE_STATE: Signal = Signal::new(); - static FAN_TWO_STATE: Signal = Signal::new(); + static FAN_ONE_STATE: SetPointSignal = Signal::new(); + static FAN_TWO_STATE: SetPointSignal = Signal::new(); let button_receivers = ( FAN_ONE_DISPLAY_STATE