diff --git a/CLAUDE.md b/CLAUDE.md index bdc6c26..ef6ed98 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,6 +129,8 @@ would require extracting it into its own crate. - [fan-controller/documentation.md](fan-controller/documentation.md) — the LED status protocol (which blink pattern means which fan speed / out-of-sync state) and the Home Assistant onboarding sequence. Update it when LED behaviour changes. -- [fan-controller/README.md](fan-controller/README.md) — the working TODO list for the firmware. +- [fan-controller/TODO.md](fan-controller/TODO.md) — the working TODO list for the firmware: + every outstanding item with source line references and a suggested priority order. Keep it in + sync when adding or resolving a `//TODO` in `fan-controller/src/`. - [README.md](README.md) — probe firmware updates and where Home Assistant logs rejected discovery payloads. diff --git a/fan-controller/README.md b/fan-controller/README.md index aa11d00..77e3326 100644 --- a/fan-controller/README.md +++ b/fan-controller/README.md @@ -4,18 +4,6 @@ install [probe.rs](https://probe.rs) `cargo run` # TODO -- [ ] Read fan speed on boot in case fans were already running -- [x] Debounce button tap -- [ ] Retry if there was an error writing to one or two fans -- [ ] When retrying fails after a certain while, try reset the other fan to not create an underpressure or overpressure in the house. -- [ ] Confirm fan speed is set on homeassistant and retry otherwise (there might be network interference). Can implement MQTT QoS for that -- [x] Flash LEDs for one second after boot to indicate if they work -- [ ] Make Button press pick up state that was changed through Homeassistant and not keep its own fan state -- [ ] Read temperature sensors -- [ ] Switch to only using refactored send for modbus -- [ ] [Reboot](https://github.com/embassy-rs/embassy/blob/f8685560531fcecb2f4327a490ec4df4f2b190f6/examples/rp/src/bin/rtc.rs#L50) on error after retries or connection lost. Or try reconnect in background. -- [ ] Try out bundling all channels in a sort of event bus like actor model - -## Aspirational TODOs - -- [ ] Make fan configurable through a web interface if it is not configured and set up with WiFi yet +The working list lives in [TODO.md](TODO.md), which collects every outstanding item — the ones that +used to be here plus the `//TODO` comments in `src/` — with source line references and a suggested +priority order. diff --git a/fan-controller/TODO.md b/fan-controller/TODO.md new file mode 100644 index 0000000..0bb0c03 --- /dev/null +++ b/fan-controller/TODO.md @@ -0,0 +1,182 @@ +# TODO inventory + +Every outstanding item in the firmware, collected from [README.md](README.md), +[documentation.md](documentation.md), and `//TODO` comments in `src/`. +Paths and line numbers are relative to `fan-controller/` and were accurate as of commit `9b59619`. + +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. + +--- + +## Suggested priority + +### P0 — the two items that make the device wrong or dead in the field + +**1. Validate the Modbus response** — `src/modbus/client.rs:115`, `src/modbus/client.rs:104` + +`Client::send_3` ends in `Ok(())` unconditionally. It never inspects `response_buffer` and never +even checks `bytes_read`. Anything that is not a UART error or a timeout counts as success: a +Modbus exception response, a reply from the *other* fan, or line noise. + +That matters more than it looks, because the whole "confirmed set point" design rests on it. The +`Watch` senders are documented as publishing only after the fan acknowledged the write, and +`fan_control_routine` updates `current_set_point`, the display state, the LEDs, and the Home +Assistant state on the strength of that `Ok(())`. Right now that acknowledgement is not real. + +Two related defects live in the same function: + +- The `//TODO` at `src/modbus/client.rs:104` worries the read waits for the buffer to fill. + It is the opposite: `embedded_io_async::Read::read` returns as soon as at least one byte is + available, so a *short* read is the hazard — the rest of the frame stays in the RX buffer and the + next transaction consumes it as its own response. Both fans share the UART, so leftovers alias + across fans. +- A write-holding-register response echoes the request, so the expected length is known. `read_exact` + into a fixed 8-byte frame, then validate device address, function code (including the `0x80` + exception bit), register, value, and CRC — and drain the RX buffer on any error before returning. + +Do this first: the retry work, the "confirm in Home Assistant" item, and any out-of-sync detection +are only meaningful once a failed write actually reports as failed. + +**2. The MQTT client never reconnects** — `src/task.rs:682`, plus the reboot item in `README.md` + +`mqtt_routine` (`src/main.rs:520`) calls `mqtt_with_connect` exactly once, with no surrounding loop. +Inside, `join5` completes as soon as `listen` or `keep_alive` returns, and both return on a read +error or timeout. When that happens the task finishes and is never respawned, so after any broker +or Wi-Fi drop the controller is offline until it is power cycled. + +The failure is quiet, which makes it worse: the button routine is independent and keeps working, and +no LED pattern signals that Home Assistant has been disconnected. + +The initial TCP connect already retries forever (`src/task.rs:609`), so only post-connect loss is +affected. Wrap the session in a reconnect loop with backoff, or take the watchdog-reboot approach +already sketched in `README.md`. `src/task.rs:682` is the related cleanup — cancel the sibling tasks +on connection loss rather than relying on `join5` unwinding. + +### 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` + +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. + +### Cheap win worth slotting in anywhere + +**Make `SetPoint` host-testable** — `src/fan/set_point.rs:67` + +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. + +--- + +## Full inventory + +### Fan state and control logic — `src/main.rs` + +| 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 | + +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 +these are display updates where only the latest value matters, dropping the *oldest* or moving to a +latest-wins primitive fits better than blocking. Worth factoring into one helper while you are there. + +### MQTT client — `src/task.rs` + +| Where | Item | +|---|---| +| `src/task.rs:682` | Cancel all tasks when the client loses connection — see P0 item 2 | +| `src/task.rs:45` | Rework subscribe acknowledgement into a channel that sends the packet identifier | +| `src/task.rs:71` | `wait_for_acknowledgement` can hang if called concurrently: one waker plus a `try_lock`. Use `embassy-sync::waitqueue` and/or a blocking mutex | +| `src/task.rs:632` | Replace the static "global" acknowledgement state once wakers and polling are settled | +| `src/task.rs:300` | Free packet-identifier management | +| `src/task.rs:220` | Actually close the TCP connection on `Disconnect` | +| `src/task.rs:620` | Real error handling instead of `defmt::unwrap!` on `Connect::try_from` | +| `src/task.rs:516` | Support IPv6 in broker DNS resolution (currently `DnsQueryType::A` only) | + +The acknowledgement cluster (`:45`, `:71`, `:632`, `:300`) is one refactor, not four. The concurrency +hazard is real but is only exercised at startup with a fixed subscription set today; it becomes +urgent if subscriptions ever become dynamic. + +### MQTT packet encode / decode — `src/mqtt/` + +| Where | Item | +|---|---| +| `src/mqtt/packet/publish.rs:158` | Validate there is enough space left in the buffer | +| `src/mqtt/packet/publish.rs:146` | Validate the topic name contains no MQTT wildcard characters | +| `src/mqtt/packet/publish.rs:66`, `src/mqtt/mod.rs:54` | Set flags | +| `src/mqtt/packet/connect.rs:40` | Check the fixed header can even be written | +| `src/mqtt/packet/subscribe.rs:127` | Support packet identifiers greater than `u8::MAX` | +| `src/mqtt/packet/subscribe_acknowledgement.rs:18` | Convert to the decode trait | +| `src/mqtt/packet/subscribe_acknowledgement.rs:27` | Stop ignoring properties | +| `src/mqtt/packet/subscribe_acknowledgement.rs:28` | Check the topics are actually acknowledged | +| `src/mqtt/packet/connect_acknowledgement.rs:187` | Decode properties | +| `src/mqtt/packet/disconnect.rs:103` | Decode more fields when needed | + +Of these only `publish.rs:158` has a memory-safety flavour and is worth checking for a panic path; +the rest are protocol conformance polish against a broker you control. + +### Modbus — `src/modbus/client.rs` + +| Where | Item | +|---|---| +| `src/modbus/client.rs:115` | Validate the response from the fan and read the correct number of bytes — see P0 item 1 | +| `src/modbus/client.rs:104` | The short-read hazard described in P0 item 1 | +| `src/modbus/client.rs:79` | Understand why the flush must be blocking to avoid `WouldBlock` | + +### Configuration — `src/configuration.rs` + +`src/configuration.rs:23`, `:27`, `:31`, `:34` all say the same thing: make the Wi-Fi SSID, Wi-Fi +password, broker address, and broker port configurable at runtime instead of baked in by +`build.rs`. This is the same underlying work as the aspirational web-configuration item in +`README.md`, and it is the largest single change on this list — there is no runtime configuration +or persistent storage in the firmware at all today. + +### Testing and documentation + +| 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` + +Not already covered above: + +- Retry if there was an error writing to one or two fans +- When retrying fails after a while, reset the other fan to avoid under- or overpressure in the house +- Confirm the fan speed is set in Home Assistant and retry otherwise; MQTT QoS can implement this +- Make the button press pick up state changed through Home Assistant instead of keeping its own state +- Read temperature sensors +- Switch to only using the refactored `send` for Modbus +- Try bundling all channels into an event-bus / actor-model shape +- Aspirational: a web interface for configuring the fan when Wi-Fi is not set up yet + +Already done: debounce the button tap; flash the LEDs for a second after boot.