diff --git a/CHANGELOG.md b/CHANGELOG.md index c39b985..ee5da48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `rustyfarian-esp-idf-mqtt`: non-blocking `MqttHandle::try_publish`, `try_publish_retained`, and `try_publish_with` with `TryPublishError` for time-critical loops - `wifi-pure`: `WifiPowerSave` enum (`None`, `MinModem`, `MaxModem`) and `WiFiConfig::with_power_save()` builder method - `rustyfarian-esp-idf-wifi`: applies configured power save mode via `esp_wifi_set_ps()` after Wi-Fi start +- `rustyfarian-esp-idf-espnow`: `EspIdfEspNow::init_with_radio()` starts and owns the Wi-Fi radio for ESP-NOW-only devices (ADR 008) +- `rustyfarian-esp-idf-espnow`: `EspIdfEspNow::default_interface()` returns the correct `WifiInterface` based on init mode +- `espnow-pure`: `PeerConfig::with_ap_interface()` builder method for ESP-NOW-only devices ## [0.1.0] - 2026-03-16 diff --git a/crates/espnow-pure/src/lib.rs b/crates/espnow-pure/src/lib.rs index a1ec089..50e6943 100644 --- a/crates/espnow-pure/src/lib.rs +++ b/crates/espnow-pure/src/lib.rs @@ -132,6 +132,14 @@ impl PeerConfig { interface: WifiInterface::Sta, } } + + /// Sets the Wi-Fi interface to [`WifiInterface::Ap`]. + /// + /// Use this for peers on ESP-NOW-only devices that have no STA connection. + pub fn with_ap_interface(mut self) -> Self { + self.interface = WifiInterface::Ap; + self + } } // ─── EspNowDriver trait ────────────────────────────────────────────────────── @@ -220,12 +228,11 @@ mod tests { #[test] fn peer_config_with_ap_interface() { let mac = [0x01, 0x02, 0x03, 0x04, 0x05, 0x06]; - let config = PeerConfig { - interface: WifiInterface::Ap, - ..PeerConfig::new(mac) - }; + let config = PeerConfig::new(mac).with_ap_interface(); assert_eq!(config.interface, WifiInterface::Ap); assert_eq!(config.mac, mac); + assert_eq!(config.channel, 0); + assert!(!config.encrypt); } #[test] diff --git a/crates/rustyfarian-esp-idf-espnow/Cargo.toml b/crates/rustyfarian-esp-idf-espnow/Cargo.toml index bc2f94e..56809aa 100644 --- a/crates/rustyfarian-esp-idf-espnow/Cargo.toml +++ b/crates/rustyfarian-esp-idf-espnow/Cargo.toml @@ -16,4 +16,5 @@ embuild.workspace = true espnow-pure.workspace = true anyhow.workspace = true esp-idf-svc.workspace = true +esp-idf-hal.workspace = true log.workspace = true diff --git a/crates/rustyfarian-esp-idf-espnow/src/lib.rs b/crates/rustyfarian-esp-idf-espnow/src/lib.rs index 4513000..e3bafd6 100644 --- a/crates/rustyfarian-esp-idf-espnow/src/lib.rs +++ b/crates/rustyfarian-esp-idf-espnow/src/lib.rs @@ -16,6 +16,8 @@ use std::sync::mpsc::{sync_channel, Receiver, SyncSender}; use anyhow::Context as _; use esp_idf_svc::espnow::{EspNow, PeerInfo}; +use esp_idf_svc::hal::modem::Modem; +use esp_idf_svc::wifi::EspWifi; pub use espnow_pure::{ EspNowDriver, EspNowEvent, MacAddress, PeerConfig, WifiInterface, BROADCAST_MAC, DEFAULT_RX_CHANNEL_CAPACITY, MAX_DATA_LEN, @@ -25,22 +27,75 @@ pub use espnow_pure::{ /// /// Wraps [`EspNow<'static>`] and bridges the C receive callback into a /// [`std::sync::mpsc::sync_channel`] for non-blocking polling. +/// +/// # Radio management +/// +/// - [`init()`](EspIdfEspNow::init) — caller owns the Wi-Fi radio; +/// the radio must already be started before calling this. +/// - [`init_with_radio()`](EspIdfEspNow::init_with_radio) — the driver +/// starts and owns the radio internally. Use this for ESP-NOW-only devices +/// that do not connect to a Wi-Fi AP. pub struct EspIdfEspNow { esp_now: EspNow<'static>, rx: Receiver, + _wifi: Option>, } impl EspIdfEspNow { /// Initialise ESP-NOW with the default receive-queue capacity of /// [`DEFAULT_RX_CHANNEL_CAPACITY`] frames. + /// + /// The Wi-Fi radio must already be started by the caller. + /// For ESP-NOW-only devices, use [`init_with_radio()`](Self::init_with_radio) instead. pub fn init() -> anyhow::Result { - Self::init_with_capacity(DEFAULT_RX_CHANNEL_CAPACITY) + Self::init_inner(DEFAULT_RX_CHANNEL_CAPACITY, None) } /// Initialise ESP-NOW with a custom receive-queue capacity. /// + /// The Wi-Fi radio must already be started by the caller. /// Frames received while the queue is full are dropped with a warning log. pub fn init_with_capacity(capacity: usize) -> anyhow::Result { + Self::init_inner(capacity, None) + } + + /// Initialise ESP-NOW and start the Wi-Fi radio internally. + /// + /// Use this for devices that need ESP-NOW without connecting to a Wi-Fi AP. + /// The radio is kept alive for the lifetime of the returned driver. + /// + /// Peers should use [`WifiInterface::Ap`] (or call + /// [`PeerConfig::with_ap_interface()`]) since there is no STA connection. + /// See [`default_interface()`](Self::default_interface). + pub fn init_with_radio( + modem: Modem<'static>, + sys_loop: esp_idf_svc::eventloop::EspSystemEventLoop, + nvs: Option, + ) -> anyhow::Result { + let mut wifi = EspWifi::new(modem, sys_loop, nvs) + .context("failed to create EspWifi for ESP-NOW radio")?; + wifi.start() + .context("failed to start Wi-Fi radio for ESP-NOW")?; + log::info!("Wi-Fi radio started for ESP-NOW (no AP connection)"); + + Self::init_inner(DEFAULT_RX_CHANNEL_CAPACITY, Some(wifi)) + } + + /// Returns the recommended [`WifiInterface`] for peer configuration. + /// + /// - [`WifiInterface::Ap`] when the driver owns the radio + /// (created via [`init_with_radio()`](Self::init_with_radio) — no STA connection) + /// - [`WifiInterface::Sta`] when the caller manages Wi-Fi + /// (created via [`init()`](Self::init) — STA is assumed) + pub fn default_interface(&self) -> WifiInterface { + if self._wifi.is_some() { + WifiInterface::Ap + } else { + WifiInterface::Sta + } + } + + fn init_inner(capacity: usize, wifi: Option>) -> anyhow::Result { let esp_now = EspNow::take().context("failed to acquire EspNow singleton")?; let (tx, rx): (SyncSender, Receiver) = sync_channel(capacity); @@ -58,7 +113,11 @@ impl EspIdfEspNow { }) .context("failed to register ESP-NOW receive callback")?; - Ok(Self { esp_now, rx }) + Ok(Self { + esp_now, + rx, + _wifi: wifi, + }) } } diff --git a/docs/adr/008-espnow-radio-only-init.md b/docs/adr/008-espnow-radio-only-init.md new file mode 100644 index 0000000..0fb540b --- /dev/null +++ b/docs/adr/008-espnow-radio-only-init.md @@ -0,0 +1,89 @@ +# ADR 008: ESP-NOW Radio-Only Initialisation + +## Status + +Accepted + +## Context + +Devices that use ESP-NOW without connecting to a Wi-Fi access point (e.g. LED matrix, nunchuck controller) must manually initialise the Wi-Fi radio before calling `EspIdfEspNow::init()`. +Every consumer writes the same boilerplate: + +```rust +let mut wifi = EspWifi::new(peripherals.modem, sys_loop, Some(nvs))?; +wifi.start()?; +let espnow = EspIdfEspNow::init()?; +``` + +Additionally, ESP-NOW-only devices must manually override `PeerConfig::interface` from the default `WifiInterface::Sta` to `WifiInterface::Ap`, because there is no STA connection. +This is a known ESP-NOW quirk that catches every new consumer and wastes debugging time. + +Evidence from downstream firmware: +- `firmware-rgb-matrix/src/main.rs` — raw `EspWifi::new()` + `.start()` +- `firmware-rgb-nunchuck/src/main.rs` — same pattern + manual `brain_peer.interface = WifiInterface::Ap` + +### Design decisions under evaluation + +**Where to store the radio** + +`init_with_radio()` must keep the `EspWifi` instance alive — if dropped, the radio shuts down and ESP-NOW stops working. +Two options: a separate wrapper struct, or an `Option>` field on the existing `EspIdfEspNow`. +A separate struct would duplicate the `EspNowDriver` trait implementation. + +**WifiInterface auto-detection** + +Consumers who use `init_with_radio()` (no STA connection) always need `WifiInterface::Ap` on their peers. +The driver knows whether it owns the radio, so it can expose a `default_interface()` method to guide callers. + +**PeerConfig ergonomics** + +`PeerConfig` in `espnow-pure` defaults `interface` to `Sta`. +A builder method for the AP case would eliminate the field-level override that every ESP-NOW-only device needs. + +## Decision + +Add `init_with_radio()` to `EspIdfEspNow` and a `with_ap_interface()` builder to `PeerConfig`. +Leave the existing `init()` unchanged for devices that manage Wi-Fi themselves. + +### Changes to `rustyfarian-esp-idf-espnow` + +- Add `_wifi: Option>` field to `EspIdfEspNow`. + Existing `init()` sets it to `None`; `init_with_radio()` stores `Some(wifi)`. + +- Add `init_with_radio(modem, sys_loop, nvs)` method that: + 1. Creates `EspWifi::new(modem, sys_loop, nvs)` and calls `.start()` + 2. Stores the `EspWifi` instance to keep the radio alive + 3. Delegates to the existing `init_with_capacity()` logic for ESP-NOW setup + +- Add `default_interface()` method returning `WifiInterface::Ap` when the driver owns the radio, `WifiInterface::Sta` otherwise. + +- Add `esp-idf-hal` workspace dependency to `Cargo.toml` (for `Modem` type). + +### Changes to `espnow-pure` + +- Add `PeerConfig::with_ap_interface()` builder method — sets `interface` to `WifiInterface::Ap`. + +### What stays unchanged + +- `EspNowDriver` trait — radio init is a platform concern, not a driver operation. +- `WifiInterface` enum — no new variants needed. +- Existing `init()` / `init_with_capacity()` signatures — fully backwards compatible. + +## Consequences + +### Positive + +- **Eliminates boilerplate** — ESP-NOW-only firmware drops ~8 lines of manual radio init per crate +- **Eliminates the `WifiInterface::Ap` gotcha** — `default_interface()` + `with_ap_interface()` make the correct choice obvious +- **Backwards compatible** — existing `init()` callers are unaffected +- **Radio lifetime is safe** — `EspWifi` stored in the struct prevents accidental drop + +### Negative + +- **`EspIdfEspNow` grows an `Option` field** — minor size increase; `None` path has no overhead +- **New dependency** — `esp-idf-hal` added to `rustyfarian-esp-idf-espnow` (already a transitive dep via `esp-idf-svc`) + +## References + +- [ADR 007 — ESP-NOW Abstraction Layer](007-espnow-abstraction.md) +- Feature request: `review-queue/espnow-radio-only-init.md`