From 3a62a30a7612f1d7b42a79720b51264a21a97471 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 03:59:22 +0000 Subject: [PATCH 01/29] Re-resolve held sends on every timer pass --- .../ziggurat-driver/src/zigbee_stack/sends.rs | 44 ++++++------- crates/ziggurat-zigbee/src/sends/mod.rs | 63 ++++++++++--------- crates/ziggurat-zigbee/src/sends/unrouted.rs | 37 ++++++----- 3 files changed, 77 insertions(+), 67 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/sends.rs b/crates/ziggurat-driver/src/zigbee_stack/sends.rs index 636bf4d..cfb783f 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/sends.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/sends.rs @@ -18,7 +18,6 @@ use ziggurat_zigbee::sends::{Action, Broadcast as BroadcastRecord, Unicast as Un use crate::runtime::Runtime; -use super::routing::Status as RouteStatus; use super::{ Confirm, CoreGuard, DeliveryError, Frame, Nib, NwkSecurityMode, RouteDirective, Schedule, SendActions, SendHandle, SendId, SendMode, SendSlot, Transmit, TxPolicy, ZigbeeCore, @@ -178,15 +177,14 @@ impl ZigbeeStack { .map(Ieee802154Address::Eui64) } - /// The next hop of an unrouted frame once its route is known, and the address it - /// polls under if it is a sleepy child. The route can have vanished in the race, - /// which fails the send. - fn resolve_routed_unicast( + /// Resolve the route of an unrouted frame again: its next hop and the address it + /// polls under if it is a sleepy child. `None` means no route yet. + fn resolve_unrouted_unicast( &self, nib: &Nib, frame: &mut Frame, now: CoreInstant, - ) -> Result<(Nwk, Option), DeliveryError> { + ) -> Option<(Nwk, Option)> { let Frame::Nwk { nwk_frame, .. } = frame else { unreachable!("only NWK unicasts await a route") }; @@ -201,9 +199,10 @@ impl ZigbeeStack { ); match resolved { - NextHop::Resolved(next_hop) => Ok((next_hop, Self::poll_address(nib, next_hop))), - NextHop::NeedDiscovery | NextHop::Discard => { - Err(DeliveryError::RouteInactiveAfterDiscovery) + NextHop::Resolved(next_hop) => Some((next_hop, Self::poll_address(nib, next_hop))), + NextHop::NeedDiscovery => None, + NextHop::Discard => { + unreachable!("a frame is only held for a route if it allows discovery") } } } @@ -299,7 +298,7 @@ impl ZigbeeStack { } /// One timer pass: settle the broadcasts whose passive-ack quorum was heard, route - /// the sends whose route became active, restart or fail the discoveries that + /// the held sends that have become routable, restart or fail the discoveries that /// lapsed, then run the table's timed transitions. Returns when the next pass is /// due: the table's next deadline, or that of a route discovery unrouted sends wait /// on, which lives in the routing table. @@ -318,18 +317,19 @@ impl ZigbeeStack { } for destination in core.sends.unrouted_destinations() { - if core.nib.routing.route_status(destination) == Some(RouteStatus::Active) { - actions.extend(core.sends.route_unicasts( - destination, - now, - &self.tunables, - |send| self.resolve_routed_unicast(&core.nib, send, now), - )); - } else if core - .nib - .routing - .discovery_deadline(destination, now) - .is_none() + actions.extend( + core.sends + .route_unicasts(destination, now, &self.tunables, |send| { + self.resolve_unrouted_unicast(&core.nib, send, now) + }), + ); + + if core.sends.has_unrouted(destination) + && core + .nib + .routing + .discovery_deadline(destination, now) + .is_none() { if core.sends.retry_discovery(destination) { actions.extend(self.start_route_discovery(core, destination)); diff --git a/crates/ziggurat-zigbee/src/sends/mod.rs b/crates/ziggurat-zigbee/src/sends/mod.rs index 9527595..d00dcf5 100644 --- a/crates/ziggurat-zigbee/src/sends/mod.rs +++ b/crates/ziggurat-zigbee/src/sends/mod.rs @@ -440,44 +440,47 @@ impl> Sends { actions } - /// A route to `destination` is known: move every unrouted send to it to the unicasts, - /// keeping the deadline it was admitted with. `resolve` finds each frame's next hop - /// (rewriting the frame for a source route) and the address the device polls under - /// if the frame must wait for its poll, or fails the send. + /// Move the unrouted sends to `destination` that `resolve` finds a next hop for. pub fn route_unicasts( &mut self, destination: Nwk, now: Instant, tunables: &Tunables, - mut resolve: impl FnMut(&mut F) -> Result<(Nwk, Option), E>, + mut resolve: impl FnMut(&mut F) -> Option<(Nwk, Option)>, ) -> Actions { // Fail the sends past their deadline first, so routing never revives one - let mut actions = self.unrouted_unicasts.tick(now); - - for (id, mut unrouted) in self.unrouted_unicasts.route(destination) { - match resolve(&mut unrouted.frame) { - Ok((next_hop, poll_address)) => self.unicasts.enqueue( - id, - Unicast { - frame: unrouted.frame, - priority: unrouted.priority, - waiter: unrouted.waiter, - confirm: unrouted.confirm, - next_hop, - poll_address, - }, - now, - Some(unrouted.deadline), - None, - tunables, - ), - Err(err) => actions.push(Action::Settled { - waiter: unrouted.waiter, - result: Err(err), + let actions = self.unrouted_unicasts.tick(now); + let mut routed = false; + + for id in self.unrouted_unicasts.sends_to(destination) { + // `resolve` may rewrite the frame for a source route. Without a next hop + // the send stays held. + let Some((next_hop, poll_address)) = resolve(self.unrouted_unicasts.frame_mut(id)) + else { + continue; + }; + + let unrouted = self.unrouted_unicasts.take(id); + self.unicasts.enqueue( + id, + Unicast { frame: unrouted.frame, - failed_hop: None, - }), - } + priority: unrouted.priority, + waiter: unrouted.waiter, + confirm: unrouted.confirm, + next_hop, + poll_address, + }, + now, + Some(unrouted.deadline), + None, + tunables, + ); + routed = true; + } + + if routed { + self.unrouted_unicasts.routed(destination); } actions diff --git a/crates/ziggurat-zigbee/src/sends/unrouted.rs b/crates/ziggurat-zigbee/src/sends/unrouted.rs index 65eb637..69c29a6 100644 --- a/crates/ziggurat-zigbee/src/sends/unrouted.rs +++ b/crates/ziggurat-zigbee/src/sends/unrouted.rs @@ -129,25 +129,32 @@ impl> UnroutedUnicasts { .replace(request) } - /// Take every send to `destination`: its route is known. - pub(super) fn route(&mut self, destination: Nwk) -> Vec<(SendId, Record)> { - self.discoveries.remove(&destination); - self.take(destination) - } - - fn take(&mut self, destination: Nwk) -> Vec<(SendId, Record)> { - let ids: Vec = self - .records + /// The id of every send to `destination`, oldest first. + pub(super) fn sends_to(&self, destination: Nwk) -> Vec { + self.records .iter() .filter(|(_, send)| send.destination == destination) .map(|(id, _)| *id) - .collect(); - - ids.into_iter() - .map(|id| (id, self.records.remove(&id).unwrap())) .collect() } + pub(super) fn frame_mut(&mut self, id: SendId) -> &mut F { + &mut self.records.get_mut(&id).unwrap().frame + } + + /// Take a held send: it has a route, or it failed. + pub(super) fn take(&mut self, id: SendId) -> Record { + self.records.remove(&id).unwrap() + } + + /// A send to `destination` found a route. Its discovery is over once no send waits + /// on it any more. + pub(super) fn routed(&mut self, destination: Nwk) { + if self.sends_to(destination).is_empty() { + self.discoveries.remove(&destination); + } + } + fn settled(send: Record, failure: SendFailure) -> Action { Action::Settled { waiter: send.waiter, @@ -184,9 +191,9 @@ impl> UnroutedUnicasts { .and_then(|discovery| discovery.request); let actions = self - .take(destination) + .sends_to(destination) .into_iter() - .map(|(_, send)| Self::settled(send, SendFailure::RouteDiscoveryTimeout)) + .map(|id| Self::settled(self.take(id), SendFailure::RouteDiscoveryTimeout)) .collect(); (request, actions) From 004f8c1a96f50943fcb9e8f9165cb037f88483a1 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 03:59:33 +0000 Subject: [PATCH 02/29] Wake the send timer when a route record changes --- crates/ziggurat-driver/src/zigbee_stack/nwk.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs index 36c5503..6e9320f 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs @@ -258,6 +258,9 @@ impl ZigbeeStack { destination: source, relays: cmd.relays.clone(), }); + + // Sends held for a route to the device can now be source routed + self.send_timer_wake.notify_one(); } } NwkCommand::RouteRequest(cmd) => { From f29f2ad3d05aeec97ea53e36e85c729b4e1b915e Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 03:59:39 +0000 Subject: [PATCH 03/29] Remove the unused `RouteInactiveAfterDiscovery` send status --- crates/ziggurat-driver/src/zigbee_stack.rs | 2 -- crates/ziggurat-protocol/src/bridge.rs | 1 - crates/ziggurat-protocol/src/wire.rs | 1 - 3 files changed, 4 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack.rs b/crates/ziggurat-driver/src/zigbee_stack.rs index 584d417..969be98 100644 --- a/crates/ziggurat-driver/src/zigbee_stack.rs +++ b/crates/ziggurat-driver/src/zigbee_stack.rs @@ -98,8 +98,6 @@ pub enum EnqueueError { pub enum DeliveryError { #[error("route discovery timed out")] RouteDiscoveryTimeout, - #[error("route not active after discovery completed")] - RouteInactiveAfterDiscovery, #[error("next hop {next_hop:?} did not ACK")] NwkNoAck { next_hop: Ieee802154Address }, #[error("transmit rejected due to CCA failure")] diff --git a/crates/ziggurat-protocol/src/bridge.rs b/crates/ziggurat-protocol/src/bridge.rs index aeb863e..f24060c 100644 --- a/crates/ziggurat-protocol/src/bridge.rs +++ b/crates/ziggurat-protocol/src/bridge.rs @@ -420,7 +420,6 @@ pub(crate) const fn send_status(result: &Result<(), DeliveryError>) -> SendStatu Ok(()) => SendStatus::Success, Err(err) => match err { DeliveryError::RouteDiscoveryTimeout => SendStatus::RouteDiscoveryTimeout, - DeliveryError::RouteInactiveAfterDiscovery => SendStatus::RouteInactiveAfterDiscovery, DeliveryError::NwkNoAck { .. } => SendStatus::NwkNoAck, DeliveryError::CcaFailure => SendStatus::CcaFailure, DeliveryError::TransmitFailed(_) => SendStatus::TransmitFailed, diff --git a/crates/ziggurat-protocol/src/wire.rs b/crates/ziggurat-protocol/src/wire.rs index c44869b..3e51ed5 100644 --- a/crates/ziggurat-protocol/src/wire.rs +++ b/crates/ziggurat-protocol/src/wire.rs @@ -713,7 +713,6 @@ pub enum SendStatus { Success = 0, RouteDiscoveryTimeout = 1, RouteDiscoveryNoEntry = 2, - RouteInactiveAfterDiscovery = 3, NwkNoAck = 4, CcaFailure = 5, TransmitFailed = 6, From b2cb431143a31f4752331fdfe4f3a547c06570fb Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 04:12:33 +0000 Subject: [PATCH 04/29] Move the timer pass's routing decisions into the send tables --- .../ziggurat-driver/src/zigbee_stack/sends.rs | 53 ++++------- crates/ziggurat-zigbee/src/sends/mod.rs | 95 +++++++++++++------ 2 files changed, 87 insertions(+), 61 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/sends.rs b/crates/ziggurat-driver/src/zigbee_stack/sends.rs index cfb783f..02fa5f1 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/sends.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/sends.rs @@ -297,16 +297,14 @@ impl ZigbeeStack { } } - /// One timer pass: settle the broadcasts whose passive-ack quorum was heard, route - /// the held sends that have become routable, restart or fail the discoveries that - /// lapsed, then run the table's timed transitions. Returns when the next pass is - /// due: the table's next deadline, or that of a route discovery unrouted sends wait - /// on, which lives in the routing table. + /// One timer pass: settle the broadcasts whose passive-ack quorum was heard, run + /// the send tables' pass against the routing and neighbor tables, then start the + /// route discoveries it asks for. Returns when the next pass is due. fn drive_sends(&self) -> Option { let now = self.core_now(); let mut guard = self.core(); - // A plain `&mut ZigbeeCore`, so the closure below borrows `nib` beside `sends` + // A plain `&mut ZigbeeCore`, so the closures below borrow `nib` beside `sends` let core = &mut *guard; let mut actions = SendActions::new(); @@ -316,38 +314,25 @@ impl ZigbeeStack { } } - for destination in core.sends.unrouted_destinations() { - actions.extend( - core.sends - .route_unicasts(destination, now, &self.tunables, |send| { - self.resolve_unrouted_unicast(&core.nib, send, now) - }), - ); - - if core.sends.has_unrouted(destination) - && core - .nib - .routing - .discovery_deadline(destination, now) - .is_none() - { - if core.sends.retry_discovery(destination) { - actions.extend(self.start_route_discovery(core, destination)); - } else { - core.nib.routing.mark_discovery_failed(destination); - actions.extend(core.sends.fail_unrouted(destination)); - } - } + let tick = core.sends.tick( + now, + &self.tunables, + |frame| self.resolve_unrouted_unicast(&core.nib, frame, now), + |destination| core.nib.routing.discovery_deadline(destination, now), + ); + actions.extend(tick.actions); + + for destination in tick.restart_discovery { + actions.extend(self.start_route_discovery(core, destination)); } - actions.extend(core.sends.tick(now)); + for destination in tick.failed_discovery { + core.nib.routing.mark_discovery_failed(destination); + } - let discoveries = core + let next_wake = core .sends - .unrouted_destinations() - .into_iter() - .filter_map(|destination| core.nib.routing.discovery_deadline(destination, now)); - let next_wake = core.sends.next_wake().into_iter().chain(discoveries).min(); + .next_wake(|destination| core.nib.routing.discovery_deadline(destination, now)); // This pass is the timer task's own: waking the timer here would make it spin self.run_send_actions(guard, actions); diff --git a/crates/ziggurat-zigbee/src/sends/mod.rs b/crates/ziggurat-zigbee/src/sends/mod.rs index d00dcf5..d554c5c 100644 --- a/crates/ziggurat-zigbee/src/sends/mod.rs +++ b/crates/ziggurat-zigbee/src/sends/mod.rs @@ -185,6 +185,17 @@ pub enum Action { }, } +/// What a timer pass leaves for the driver. +#[derive(Debug)] +pub struct Tick { + pub actions: Actions, + /// Destinations whose route discovery lapsed with a restart left. The driver starts + /// a new discovery for each. + pub restart_discovery: Vec, + /// Destinations whose route discovery is over without a route. Their sends failed. + pub failed_discovery: Vec, +} + /// What the driver must do after a mutation, in order. Every mutating verb returns one, /// so a mutation whose consequences are dropped is a compile-time warning. #[must_use = "actions resolve waiters; dropping them loses sends"] @@ -400,22 +411,11 @@ impl> Sends { self.unicasts.ack_received(ack) } - /// Every destination with unrouted sends. - pub fn unrouted_destinations(&self) -> Vec { - self.unrouted_unicasts.destinations() - } - /// Whether any unrouted send waits on `destination`'s route discovery. pub fn has_unrouted(&self, destination: Nwk) -> bool { self.unrouted_unicasts.has_unrouted(destination) } - /// `destination`'s discovery lapsed: consume a restart of it, if one is left and - /// an unrouted send still waits on it. The caller restarts the discovery. - pub fn retry_discovery(&mut self, destination: Nwk) -> bool { - self.unrouted_unicasts.retry_discovery(destination) - } - /// The driver broadcast `request`, the route request of `destination`'s discovery. /// Copies of the request it replaces still to come are not sent. pub fn route_request_sent(&mut self, destination: Nwk, request: SendId) -> Actions { @@ -440,22 +440,20 @@ impl> Sends { actions } - /// Move the unrouted sends to `destination` that `resolve` finds a next hop for. - pub fn route_unicasts( + /// Move the unrouted sends to `destination` that `next_hop` finds a next hop for. + fn route_unicasts( &mut self, destination: Nwk, now: Instant, tunables: &Tunables, - mut resolve: impl FnMut(&mut F) -> Option<(Nwk, Option)>, - ) -> Actions { - // Fail the sends past their deadline first, so routing never revives one - let actions = self.unrouted_unicasts.tick(now); + next_hop: &mut impl FnMut(&mut F) -> Option<(Nwk, Option)>, + ) { let mut routed = false; for id in self.unrouted_unicasts.sends_to(destination) { - // `resolve` may rewrite the frame for a source route. Without a next hop + // `next_hop` may rewrite the frame for a source route. Without a next hop // the send stays held. - let Some((next_hop, poll_address)) = resolve(self.unrouted_unicasts.frame_mut(id)) + let Some((next_hop, poll_address)) = next_hop(self.unrouted_unicasts.frame_mut(id)) else { continue; }; @@ -482,8 +480,6 @@ impl> Sends { if routed { self.unrouted_unicasts.routed(destination); } - - actions } /// The key of every broadcast awaiting its passive-ack quorum. @@ -540,16 +536,60 @@ impl> Sends { Some(actions) } - /// The timer pass: re-queue what is due and fail what is past its deadline. - pub fn tick(&mut self, now: Instant) -> Actions { - let mut actions = self.unicasts.tick(now); - actions.extend(self.unrouted_unicasts.tick(now)); + /// The timer pass over the three tables. + pub fn tick( + &mut self, + now: Instant, + tunables: &Tunables, + // A frame's next hop, and the address its device polls under if the frame must + // wait for its poll + mut next_hop: impl FnMut(&mut F) -> Option<(Nwk, Option)>, + // When the live route discovery toward a destination lapses + discovery_deadline: impl Fn(Nwk) -> Option, + ) -> Tick { + // Fail the sends past their deadline first, so routing never revives one + let mut actions = self.unrouted_unicasts.tick(now); + let mut restart_discovery = Vec::new(); + let mut failed_discovery = Vec::new(); + + for destination in self.unrouted_unicasts.destinations() { + self.route_unicasts(destination, now, tunables, &mut next_hop); + + if !self.unrouted_unicasts.has_unrouted(destination) + || discovery_deadline(destination).is_some() + { + continue; + } + + if self.unrouted_unicasts.retry_discovery(destination) { + restart_discovery.push(destination); + } else { + actions.extend(self.fail_unrouted(destination)); + failed_discovery.push(destination); + } + } + + actions.extend(self.unicasts.tick(now)); actions.extend(self.broadcasts.tick(now)); - actions + + Tick { + actions, + restart_discovery, + failed_discovery, + } } /// When the timer must next run, or `None` when nothing is waiting on time. - pub fn next_wake(&self) -> Option { + pub fn next_wake( + &self, + discovery_deadline: impl Fn(Nwk) -> Option, + ) -> Option { + let discoveries = self + .unrouted_unicasts + .destinations() + .into_iter() + .filter_map(discovery_deadline); + [ self.unicasts.next_wake(), self.unrouted_unicasts.next_wake(), @@ -557,6 +597,7 @@ impl> Sends { ] .into_iter() .flatten() + .chain(discoveries) .min() } From c0aeb17c3bf6ba83a56be0051436e191665c2111 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:03:12 +0000 Subject: [PATCH 05/29] Fail held sends again when a device joins --- .../ziggurat-driver/src/zigbee_stack/nwk.rs | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs index 6e9320f..3a4001d 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs @@ -26,9 +26,10 @@ use ziggurat_zigbee::nwk::next_hop::{self, NextHop}; use ziggurat_zigbee::sends::{Unicast as UnicastRecord, UnroutedUnicast}; use super::{ - AddrConflictSource, Broadcast, Confirm, DeliveryError, EnqueueError, Frame, MAX_DEPTH, Nib, - NwkSecurityMode, PROTOCOL_VERSION, RouteDirective, Schedule, SendActions, SendHandle, SendId, - SendMode, SendSlot, TxPolicy, TxPriority, Unicast, ZigbeeCore, ZigbeeNotification, ZigbeeStack, + AddrConflictSource, Broadcast, Confirm, CoreGuard, DeliveryError, EnqueueError, Frame, + MAX_DEPTH, Nib, NwkSecurityMode, PROTOCOL_VERSION, RouteDirective, Schedule, SendActions, + SendHandle, SendId, SendMode, SendSlot, TxPolicy, TxPriority, Unicast, ZigbeeCore, + ZigbeeNotification, ZigbeeStack, }; impl ZigbeeStack { @@ -45,17 +46,17 @@ impl ZigbeeStack { return; } - // The device can be reached again, so sends held for it are kept. The route - // request they wait on went out before it joined. With that discovery gone, the - // timer pass starts a new one. - core.nib.routing.forget(self.state.network_address, nwk); - self.commit_sends(core, SendActions::new()); + self.forget_nwk_state_in(core, nwk); } - /// The device at `nwk` left, so what we know about the address belongs to a session - /// that is over. Held sends fail: nothing will answer a route request for it. + /// The device at `nwk` left or joined again, so what we know about the address + /// belongs to its previous session. Held sends fail: they wait on a route request + /// the device never heard, and would block a new one until it expires. pub(super) fn forget_nwk_state(&self, nwk: Nwk) { - let mut core = self.core(); + self.forget_nwk_state_in(self.core(), nwk); + } + + fn forget_nwk_state_in(&self, mut core: CoreGuard<'_>, nwk: Nwk) { core.nib.routing.forget(self.state.network_address, nwk); let actions = core.sends.fail_unrouted(nwk); self.commit_sends(core, actions); From 6685951c6affc3ebe753385621cc8481b9c69a94 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:22:56 +0000 Subject: [PATCH 06/29] Keep a route found after the sends waiting on its discovery are gone --- crates/ziggurat-zigbee/src/sends/mod.rs | 11 +++------ crates/ziggurat-zigbee/src/sends/unrouted.rs | 24 ++++++++------------ 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/crates/ziggurat-zigbee/src/sends/mod.rs b/crates/ziggurat-zigbee/src/sends/mod.rs index d554c5c..1cc7252 100644 --- a/crates/ziggurat-zigbee/src/sends/mod.rs +++ b/crates/ziggurat-zigbee/src/sends/mod.rs @@ -448,8 +448,6 @@ impl> Sends { tunables: &Tunables, next_hop: &mut impl FnMut(&mut F) -> Option<(Nwk, Option)>, ) { - let mut routed = false; - for id in self.unrouted_unicasts.sends_to(destination) { // `next_hop` may rewrite the frame for a source route. Without a next hop // the send stays held. @@ -474,11 +472,6 @@ impl> Sends { None, tunables, ); - routed = true; - } - - if routed { - self.unrouted_unicasts.routed(destination); } } @@ -555,7 +548,9 @@ impl> Sends { for destination in self.unrouted_unicasts.destinations() { self.route_unicasts(destination, now, tunables, &mut next_hop); - if !self.unrouted_unicasts.has_unrouted(destination) + // A discovery nothing waits on is over, not failed: reporting it as failed + // would discard a route it found after its sends were gone + if self.unrouted_unicasts.end_idle_discovery(destination) || discovery_deadline(destination).is_some() { continue; diff --git a/crates/ziggurat-zigbee/src/sends/unrouted.rs b/crates/ziggurat-zigbee/src/sends/unrouted.rs index 69c29a6..7aa8b62 100644 --- a/crates/ziggurat-zigbee/src/sends/unrouted.rs +++ b/crates/ziggurat-zigbee/src/sends/unrouted.rs @@ -147,12 +147,15 @@ impl> UnroutedUnicasts { self.records.remove(&id).unwrap() } - /// A send to `destination` found a route. Its discovery is over once no send waits - /// on it any more. - pub(super) fn routed(&mut self, destination: Nwk) { - if self.sends_to(destination).is_empty() { - self.discoveries.remove(&destination); + /// End `destination`'s discovery if no send waits on it any more: its sends were + /// routed, cancelled or timed out. Returns whether it ended. + pub(super) fn end_idle_discovery(&mut self, destination: Nwk) -> bool { + if !self.sends_to(destination).is_empty() { + return false; } + + self.discoveries.remove(&destination); + true } fn settled(send: Record, failure: SendFailure) -> Action { @@ -164,17 +167,10 @@ impl> UnroutedUnicasts { } } - /// Consume a restart of `destination`'s discovery, if one is left and a send still - /// waits on it. + /// Consume a restart of `destination`'s discovery, if one is left. pub(super) fn retry_discovery(&mut self, destination: Nwk) -> bool { - let waiting = self - .records - .values() - .any(|send| send.destination == destination); - let discovery = self.discoveries.get_mut(&destination).unwrap(); - // A discovery nothing waits on any more is not restarted - if discovery.restarts_left == 0 || !waiting { + if discovery.restarts_left == 0 { return false; } From 353ac56291a612b0c27631964149ec00fc0a2c79 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:23:41 +0000 Subject: [PATCH 07/29] Wake held sends when a route record changes a relay's route --- .../ziggurat-driver/src/zigbee_stack/nwk.rs | 9 +++-- crates/ziggurat-zigbee/src/nwk/routing.rs | 38 +++++++++++++++---- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs index 3a4001d..bb8642e 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs @@ -248,19 +248,22 @@ impl ZigbeeStack { NwkCommand::RouteRecord(cmd) => { tracing::trace!("Route record command frame received: {cmd:?}"); let source = nwk_frame.nwk_header.source; - let changed = self + let change = self .core() .nib .routing .store_route_record(source, cmd.relays.clone()); - if changed { + if change.source { self.push_notification(ZigbeeNotification::RouteRecord { destination: source, relays: cmd.relays.clone(), }); + } - // Sends held for a route to the device can now be source routed + // Sends held for the device, or for a relay on its path, can now be + // source routed + if change.any { self.send_timer_wake.notify_one(); } } diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index f3b294f..bd1371b 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -102,6 +102,17 @@ impl TableEntry { } } +/// What storing a route record changed. +#[derive(Debug, Clone, Copy)] +pub struct RouteRecordChange { + /// The source route to the record's sender changed. Only these are worth the + /// client persisting. + pub source: bool, + /// Some stored source route changed: the sender's, or that of a relay on its path. + /// A send held for any of them may now have a route. + pub any: bool, +} + #[derive(Debug)] pub struct DiscoveryEntry { /// A sequence number for a route request command frame that is incremented each time @@ -308,22 +319,33 @@ impl Routing { self.route_table.insert(destination, entry); } - /// Returns whether the stored relay list for `source` changed, so the client only - /// persists a genuinely new source route (most route records are redundant). - pub fn store_route_record(&mut self, source: Nwk, relays: Vec) -> bool { + /// Store the route record `source` sent, and the routes to its relays that follow + /// from it. Most route records are redundant, so the result says what changed. + pub fn store_route_record(&mut self, source: Nwk, relays: Vec) -> RouteRecordChange { + let mut relay_changed = false; + // Spec 3.6.4.5.5: the new route also replaces any existing source routes to // the intermediary relays. The relays between relay `i` and us form its own // route; the last relay delivered the record to us directly. for i in 1..relays.len() { - self.route_record_table - .insert(relays[i - 1], relays[i..].to_vec()); + relay_changed |= self.replace_route_record(relays[i - 1], relays[i..].to_vec()); } if let Some(&last) = relays.last() { - self.route_record_table.insert(last, Vec::new()); + relay_changed |= self.replace_route_record(last, Vec::new()); } - let changed = self.route_record_table.get(&source) != Some(&relays); - self.route_record_table.insert(source, relays); + let source_changed = self.replace_route_record(source, relays); + + RouteRecordChange { + source: source_changed, + any: source_changed || relay_changed, + } + } + + /// Store one source route, returning whether it differs from the one it replaces. + fn replace_route_record(&mut self, destination: Nwk, relays: Vec) -> bool { + let changed = self.route_record_table.get(&destination) != Some(&relays); + self.route_record_table.insert(destination, relays); changed } From 55525a02dbb63e06e3c613572353a2cabf6722d7 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:26:24 +0000 Subject: [PATCH 08/29] Add unit tests for the send tables and routing --- crates/ziggurat-zigbee/src/nwk/routing.rs | 3 + .../ziggurat-zigbee/src/nwk/routing/tests.rs | 165 +++++++++ crates/ziggurat-zigbee/src/sends/mod.rs | 2 + crates/ziggurat-zigbee/src/sends/tests.rs | 328 ++++++++++++++++++ 4 files changed, 498 insertions(+) create mode 100644 crates/ziggurat-zigbee/src/nwk/routing/tests.rs create mode 100644 crates/ziggurat-zigbee/src/sends/tests.rs diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index bd1371b..d6cd5dd 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -8,6 +8,9 @@ use ziggurat_ieee_802154::types::Nwk; use crate::nwk::frame::BROADCAST_ALL_ROUTERS_AND_COORDINATOR; +#[cfg(test)] +mod tests; + pub type RouteRequestId = u8; const UNKNOWN_NEXT_HOP: Nwk = Nwk(0xFFFF); diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs new file mode 100644 index 0000000..3c16c47 --- /dev/null +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -0,0 +1,165 @@ +use alloc::vec; +use core::time::Duration; + +use ziggurat_ieee_802154::types::Nwk; + +use crate::Instant; +use crate::nwk::commands::NwkRouteRequestManyToOne; + +use super::*; + +const OWN: Nwk = Nwk(0x0000); +const DEVICE: Nwk = Nwk(0x1111); +const OTHER_DEVICE: Nwk = Nwk(0x2222); +const ROUTER: Nwk = Nwk(0x3333); +const NEIGHBOR: Nwk = Nwk(0x4444); + +const START: Instant = Instant::from_micros(0); +const ROUTE_DISCOVERY_TIME: Duration = Duration::from_secs(10); +const MAX_SOURCE_ROUTE: u8 = 11; + +/// A route request relayed to us by `NEIGHBOR`. +#[derive(Clone, Copy)] +struct RouteRequest { + originator: Nwk, + destination: Nwk, + path_cost: u8, +} + +/// Receive a route request. Returns whether it was accepted as new or as a better path. +fn receive(routing: &mut Routing, request: RouteRequest, now: Instant) -> bool { + routing + .accept_route_request( + request.originator, + 1, + request.destination, + NEIGHBOR, + request.path_cost, + NwkRouteRequestManyToOne::NotManyToOne, + now, + ROUTE_DISCOVERY_TIME, + ) + .accepted +} + +// Issue #88: a device that joins again never heard the route request we sent before it +// joined. Our discovery toward it must go, or it blocks a new one until it expires. +#[test] +fn test_forget_drops_our_discovery_toward_the_device() { + let mut routing = Routing::default(); + routing.begin_discovery(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME); + assert_eq!( + routing.discovery_deadline(DEVICE, START), + Some(START + ROUTE_DISCOVERY_TIME) + ); + assert_eq!( + routing.route_status(DEVICE), + Some(Status::DiscoveryUnderway) + ); + + routing.forget(OWN, DEVICE); + + assert_eq!(routing.discovery_deadline(DEVICE, START), None); + assert_eq!(routing.route_status(DEVICE), None); +} + +#[test] +fn test_forget_drops_the_route_and_the_route_record() { + let mut routing = Routing::default(); + routing.restore_route(DEVICE, NEIGHBOR, 3); + routing.store_route_record(DEVICE, vec![ROUTER]); + assert!(routing.route_to(DEVICE, MAX_SOURCE_ROUTE).is_some()); + + routing.forget(OWN, DEVICE); + + assert_eq!(routing.next_hop(DEVICE), None); + assert_eq!(routing.route_to(DEVICE, MAX_SOURCE_ROUTE), None); +} + +#[test] +fn test_forget_leaves_other_destinations_alone() { + let mut routing = Routing::default(); + routing.restore_route(OTHER_DEVICE, NEIGHBOR, 3); + routing.begin_discovery(OWN, ROUTER, START, ROUTE_DISCOVERY_TIME); + + routing.forget(OWN, DEVICE); + + assert_eq!(routing.next_hop(OTHER_DEVICE), Some(NEIGHBOR)); + assert_eq!( + routing.discovery_deadline(ROUTER, START), + Some(START + ROUTE_DISCOVERY_TIME) + ); +} + +// Route requests bypass the broadcast transaction table, so the discovery entry is the +// only duplicate check for a relayed request. Forgetting its originator must not make +// a late copy look new, or we rebroadcast it. +#[test] +fn test_forget_keeps_the_duplicate_check_for_a_request_the_device_originated() { + let mut routing = Routing::default(); + let request = RouteRequest { + originator: DEVICE, + destination: OTHER_DEVICE, + path_cost: 5, + }; + assert!(receive(&mut routing, request, START)); + + routing.forget(OWN, DEVICE); + + let late_copy = RouteRequest { + path_cost: 7, + ..request + }; + assert!(!receive(&mut routing, late_copy, START)); +} + +#[test] +fn test_forget_keeps_the_duplicate_check_for_a_request_toward_the_device() { + let mut routing = Routing::default(); + let request = RouteRequest { + originator: OTHER_DEVICE, + destination: DEVICE, + path_cost: 5, + }; + assert!(receive(&mut routing, request, START)); + + routing.forget(OWN, DEVICE); + + let late_copy = RouteRequest { + path_cost: 7, + ..request + }; + assert!(!receive(&mut routing, late_copy, START)); +} + +// A relayed request marks its destination `DiscoveryUnderway`. Only the expiry of the +// discovery entry retires that mark, so the entry must survive forgetting the +// originator. +#[test] +fn test_relayed_discovery_still_expires_after_its_originator_is_forgotten() { + let mut routing = Routing::default(); + let request = RouteRequest { + originator: DEVICE, + destination: OTHER_DEVICE, + path_cost: 5, + }; + assert!(receive(&mut routing, request, START)); + routing.note_relayed_discovery(OTHER_DEVICE); + assert_eq!( + routing.route_status(OTHER_DEVICE), + Some(Status::DiscoveryUnderway) + ); + + routing.forget(OWN, DEVICE); + + // Any later request runs the expiry + let unrelated = RouteRequest { + originator: ROUTER, + destination: NEIGHBOR, + path_cost: 5, + }; + let after_expiry = START + ROUTE_DISCOVERY_TIME + Duration::from_secs(1); + receive(&mut routing, unrelated, after_expiry); + + assert_eq!(routing.route_status(OTHER_DEVICE), None); +} diff --git a/crates/ziggurat-zigbee/src/sends/mod.rs b/crates/ziggurat-zigbee/src/sends/mod.rs index 1cc7252..5566520 100644 --- a/crates/ziggurat-zigbee/src/sends/mod.rs +++ b/crates/ziggurat-zigbee/src/sends/mod.rs @@ -26,6 +26,8 @@ //! deadline fails with the error its state implies. mod broadcasts; +#[cfg(test)] +mod tests; mod unicasts; mod unrouted; diff --git a/crates/ziggurat-zigbee/src/sends/tests.rs b/crates/ziggurat-zigbee/src/sends/tests.rs new file mode 100644 index 0000000..0e2fee7 --- /dev/null +++ b/crates/ziggurat-zigbee/src/sends/tests.rs @@ -0,0 +1,328 @@ +use alloc::vec::Vec; +use core::time::Duration; + +use ziggurat_ieee_802154::types::Nwk; + +use crate::Instant; +use crate::constants::Tunables; + +use super::*; + +type TestSends = Sends<&'static str, u8, SendFailure>; +type TestActions = Actions<&'static str, u8, SendFailure>; +type TestTick = Tick<&'static str, u8, SendFailure>; + +const DESTINATION: Nwk = Nwk(0x1234); +const NEXT_HOP: Nwk = Nwk(0x5678); + +// The waiters of the sends a test enqueues +const HELD_UNICAST: u8 = 1; +const ROUTE_REQUEST: u8 = 2; +const SECOND_ROUTE_REQUEST: u8 = 3; + +/// What the driver's routing table answers about `DESTINATION` during a timer pass. +struct DriverTables { + next_hop: Option, + discovery_live: bool, +} + +/// The send tables, with the tunables and the clock a test drives them with. +struct Fixture { + sends: TestSends, + tunables: Tunables, + now: Instant, +} + +impl Fixture { + fn new() -> Self { + Self { + sends: TestSends::default(), + tunables: Tunables::default(), + now: Instant::from_micros(0), + } + } + + fn advance(&mut self, duration: Duration) { + self.now = self.now + duration; + } + + /// Hold a unicast to `DESTINATION` until a route to it is known. + fn hold_unicast(&mut self) { + let (_, actions) = self.sends.enqueue_unrouted_unicast( + UnroutedUnicast { + frame: "held unicast", + priority: TxPriority::UserNormal, + waiter: HELD_UNICAST, + confirm: Confirm::OnHandoff, + destination: DESTINATION, + }, + self.now, + &self.tunables, + ); + assert!(settled(actions).is_empty()); + } + + /// Broadcast the route request of `DESTINATION`'s discovery, as the driver does: + /// four copies, the first due now. Returns what registering the request settles. + fn broadcast_route_request(&mut self, waiter: u8) -> Vec<(u8, Result<(), SendFailure>)> { + let (request, actions) = self.sends.enqueue_broadcast( + Broadcast { + frame: "route request", + priority: TxPriority::UserNormal, + waiter, + schedule: Schedule::FixedInterval { + interval: Duration::from_millis(254), + copies_remaining: 4, + next_attempt: self.now, + }, + }, + self.now, + &self.tunables, + ); + assert!(settled(actions).is_empty()); + + settled(self.sends.route_request_sent(DESTINATION, request)) + } + + /// Run a timer pass. + fn tick(&mut self, tables: DriverTables) -> TestTick { + let discovery_deadline = tables + .discovery_live + .then_some(self.now + Duration::from_secs(10)); + + self.sends.tick( + self.now, + &self.tunables, + |_| tables.next_hop.map(|next_hop| (next_hop, None)), + |_| discovery_deadline, + ) + } + + fn dequeue(&mut self) -> Option> { + self.sends.dequeue(self.now) + } + + /// Report a successful transmit. Returns what it settles. + fn transmitted( + &mut self, + in_flight: InFlight<&'static str>, + ) -> Vec<(u8, Result<(), SendFailure>)> { + settled( + self.sends + .transmitted(in_flight, Ok(()), self.now, Duration::ZERO, Duration::ZERO), + ) + } +} + +/// The waiter and verdict of every send the actions settle. +fn settled(actions: TestActions) -> Vec<(u8, Result<(), SendFailure>)> { + actions + .into_iter() + .filter_map(|action| match action { + Action::Settled { waiter, result, .. } => Some((waiter, result)), + Action::HandedOff(_) => None, + }) + .collect() +} + +#[test] +fn test_held_unicast_waits_while_its_discovery_is_live() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: true, + }); + + assert!(settled(tick.actions).is_empty()); + assert!(tick.restart_discovery.is_empty()); + assert!(tick.failed_discovery.is_empty()); + assert!(fixture.sends.has_unrouted(DESTINATION)); + assert!(fixture.dequeue().is_none()); +} + +// A held unicast is bound to its destination, not to one discovery: any pass that +// finds a next hop releases it, even while a discovery toward it is still live. +#[test] +fn test_held_unicast_is_routed_once_a_next_hop_is_found() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + + let tick = fixture.tick(DriverTables { + next_hop: Some(NEXT_HOP), + discovery_live: true, + }); + + assert!(settled(tick.actions).is_empty()); + assert!(tick.restart_discovery.is_empty()); + assert!(tick.failed_discovery.is_empty()); + assert!(!fixture.sends.has_unrouted(DESTINATION)); + + let in_flight = fixture.dequeue().unwrap(); + assert_eq!(in_flight.frame, "held unicast"); + assert!(matches!( + in_flight.transmit(), + Transmit::Unicast { next_hop, .. } if next_hop == NEXT_HOP + )); +} + +#[test] +fn test_lapsed_discovery_without_a_restart_fails_its_unicasts() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + + assert_eq!( + settled(tick.actions), + [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] + ); + assert!(tick.restart_discovery.is_empty()); + assert_eq!(tick.failed_discovery, [DESTINATION]); + assert!(!fixture.sends.has_unrouted(DESTINATION)); +} + +#[test] +fn test_lapsed_discovery_with_a_restart_left_is_restarted_once() { + let mut fixture = Fixture::new(); + fixture + .tunables + .set("pending_route_discovery_attempts", 2) + .unwrap(); + fixture.hold_unicast(); + + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + + assert!(settled(tick.actions).is_empty()); + assert_eq!(tick.restart_discovery, [DESTINATION]); + assert!(tick.failed_discovery.is_empty()); + assert!(fixture.sends.has_unrouted(DESTINATION)); + + // The restarted discovery lapses too, and no restart is left + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + + assert_eq!( + settled(tick.actions), + [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] + ); + assert!(tick.restart_discovery.is_empty()); + assert_eq!(tick.failed_discovery, [DESTINATION]); +} + +// Issue #88: a device that joins again never heard the route request sent before it +// joined. Failing the held unicasts must leave nothing behind that keeps the next send +// to the address from starting its own discovery. +#[test] +fn test_failing_held_unicasts_frees_the_destination_for_a_new_discovery() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + + let actions = fixture.sends.fail_unrouted(DESTINATION); + + assert_eq!( + settled(actions), + [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] + ); + assert!(!fixture.sends.has_unrouted(DESTINATION)); +} + +// An abandoned discovery must not keep broadcasting its route request. +#[test] +fn test_failing_held_unicasts_stops_their_route_request() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); + + // The first of the four copies goes out + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: true, + }); + assert!(settled(tick.actions).is_empty()); + let first_copy = fixture.dequeue().unwrap(); + assert!(fixture.transmitted(first_copy).is_empty()); + + let actions = fixture.sends.fail_unrouted(DESTINATION); + + assert_eq!( + settled(actions), + [ + (HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout)), + (ROUTE_REQUEST, Err(SendFailure::Cancelled)), + ] + ); + + // The other three copies are never queued + fixture.advance(Duration::from_secs(1)); + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + assert!(settled(tick.actions).is_empty()); + assert!(fixture.dequeue().is_none()); +} + +// A copy in the radio cannot be recalled. It completes, and no copy follows it. +#[test] +fn test_stopped_route_request_in_the_radio_sends_no_further_copy() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); + + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: true, + }); + assert!(settled(tick.actions).is_empty()); + let first_copy = fixture.dequeue().unwrap(); + + // Only the held unicast settles: the route request is on loan to the radio + let actions = fixture.sends.fail_unrouted(DESTINATION); + assert_eq!( + settled(actions), + [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] + ); + + assert!(fixture.transmitted(first_copy).is_empty()); + + // One copy reached the air, which completes a route request + fixture.advance(Duration::from_secs(1)); + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + assert_eq!(settled(tick.actions), [(ROUTE_REQUEST, Ok(()))]); + assert!(fixture.dequeue().is_none()); +} + +// A restarted discovery broadcasts a new route request. The one it replaces stops. +#[test] +fn test_new_route_request_stops_the_one_it_replaces() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); + + let stopped = fixture.broadcast_route_request(SECOND_ROUTE_REQUEST); + + assert_eq!(stopped, [(ROUTE_REQUEST, Err(SendFailure::Cancelled))]); + + // Only the new request has a copy to send + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: true, + }); + assert!(settled(tick.actions).is_empty()); + let only_copy = fixture.dequeue().unwrap(); + assert!(fixture.transmitted(only_copy).is_empty()); + assert!(fixture.dequeue().is_none()); +} From 0ada509b1181612bf501c070599e45df48bcdfd1 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:28:40 +0000 Subject: [PATCH 09/29] Test that an idle discovery ends quietly and that route records report what changed --- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 45 +++++++++++ crates/ziggurat-zigbee/src/sends/tests.rs | 79 ++++++++++++++++++- 2 files changed, 122 insertions(+), 2 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 3c16c47..14e1260 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -163,3 +163,48 @@ fn test_relayed_discovery_still_expires_after_its_originator_is_forgotten() { assert_eq!(routing.route_status(OTHER_DEVICE), None); } + +// Most route records repeat what is stored. Only a changed one is reported. +#[test] +fn test_route_record_reports_a_new_source_route_once() { + let mut routing = Routing::default(); + + let change = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); + assert!(change.source); + assert!(change.any); + + let change = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); + assert!(!change.source); + assert!(!change.any); +} + +#[test] +fn test_route_record_reports_a_changed_path() { + let mut routing = Routing::default(); + routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); + + let change = routing.store_route_record(DEVICE, vec![NEIGHBOR]); + + assert!(change.source); + assert!(change.any); +} + +// A record also carries the routes to its relays. An unchanged record restores a relay +// route that was dropped: the device's own route is the same, but a send held for the +// relay can be routed again. +#[test] +fn test_unchanged_route_record_reports_a_relay_route_it_restores() { + let mut routing = Routing::default(); + routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); + assert!(routing.remove_route_record(ROUTER)); + assert_eq!(routing.route_to(ROUTER, MAX_SOURCE_ROUTE), None); + + let change = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); + + assert!(!change.source); + assert!(change.any); + assert_eq!( + routing.route_to(ROUTER, MAX_SOURCE_ROUTE), + Some(Route::SourceRouted(vec![NEIGHBOR])) + ); +} diff --git a/crates/ziggurat-zigbee/src/sends/tests.rs b/crates/ziggurat-zigbee/src/sends/tests.rs index 0e2fee7..962e7a2 100644 --- a/crates/ziggurat-zigbee/src/sends/tests.rs +++ b/crates/ziggurat-zigbee/src/sends/tests.rs @@ -47,8 +47,8 @@ impl Fixture { } /// Hold a unicast to `DESTINATION` until a route to it is known. - fn hold_unicast(&mut self) { - let (_, actions) = self.sends.enqueue_unrouted_unicast( + fn hold_unicast(&mut self) -> SendId { + let (id, actions) = self.sends.enqueue_unrouted_unicast( UnroutedUnicast { frame: "held unicast", priority: TxPriority::UserNormal, @@ -60,6 +60,17 @@ impl Fixture { &self.tunables, ); assert!(settled(actions).is_empty()); + + id + } + + /// Cancel a held unicast. + fn cancel(&mut self, id: SendId) { + let actions = self.sends.cancel(id).unwrap(); + assert_eq!( + settled(actions), + [(HELD_UNICAST, Err(SendFailure::Cancelled))] + ); } /// Broadcast the route request of `DESTINATION`'s discovery, as the driver does: @@ -326,3 +337,67 @@ fn test_new_route_request_stops_the_one_it_replaces() { assert!(fixture.transmitted(only_copy).is_empty()); assert!(fixture.dequeue().is_none()); } + +// A discovery whose sends are all gone has nothing left to fail. Reporting it as failed +// makes the driver mark the route `DiscoveryFailed`, which discards a route the reply +// produced after the sends were gone. +#[test] +fn test_discovery_ends_quietly_after_its_unicast_is_cancelled() { + let mut fixture = Fixture::new(); + let id = fixture.hold_unicast(); + fixture.cancel(id); + + // The discovery lapses with nothing waiting on it + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + + assert!(settled(tick.actions).is_empty()); + assert!(tick.restart_discovery.is_empty()); + assert!(tick.failed_discovery.is_empty()); + assert!(!fixture.sends.has_unrouted(DESTINATION)); +} + +#[test] +fn test_discovery_ends_quietly_after_its_unicast_times_out() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + + // The unicast passes its deadline in the same pass that finds the discovery lapsed + fixture.advance(fixture.tunables.handoff_timeout()); + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + + assert_eq!( + settled(tick.actions), + [(HELD_UNICAST, Err(SendFailure::HandoffTimeout))] + ); + assert!(tick.restart_discovery.is_empty()); + assert!(tick.failed_discovery.is_empty()); + assert!(!fixture.sends.has_unrouted(DESTINATION)); +} + +// A restart is for a send that still waits. A discovery nothing waits on is not +// restarted, however many restarts it has left. +#[test] +fn test_discovery_nothing_waits_on_is_not_restarted() { + let mut fixture = Fixture::new(); + fixture + .tunables + .set("pending_route_discovery_attempts", 2) + .unwrap(); + let id = fixture.hold_unicast(); + fixture.cancel(id); + + let tick = fixture.tick(DriverTables { + next_hop: None, + discovery_live: false, + }); + + assert!(tick.restart_discovery.is_empty()); + assert!(tick.failed_discovery.is_empty()); + assert!(!fixture.sends.has_unrouted(DESTINATION)); +} From d547be5452a28c84b39a1d4297881c0c7bd6c6ed Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:33:56 +0000 Subject: [PATCH 10/29] Shorten the send table and route record tests --- crates/ziggurat-zigbee/src/nwk/routing.rs | 2 +- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 41 +-- crates/ziggurat-zigbee/src/sends/tests.rs | 342 ++++++++---------- 3 files changed, 169 insertions(+), 216 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index d6cd5dd..159e686 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -106,7 +106,7 @@ impl TableEntry { } /// What storing a route record changed. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct RouteRecordChange { /// The source route to the record's sender changed. Only these are worth the /// client persisting. diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 14e1260..393bd45 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -166,27 +166,22 @@ fn test_relayed_discovery_still_expires_after_its_originator_is_forgotten() { // Most route records repeat what is stored. Only a changed one is reported. #[test] -fn test_route_record_reports_a_new_source_route_once() { +fn test_route_record_reports_only_a_changed_source_route() { let mut routing = Routing::default(); + let changed = RouteRecordChange { + source: true, + any: true, + }; + let unchanged = RouteRecordChange { + source: false, + any: false, + }; - let change = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); - assert!(change.source); - assert!(change.any); - - let change = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); - assert!(!change.source); - assert!(!change.any); -} - -#[test] -fn test_route_record_reports_a_changed_path() { - let mut routing = Routing::default(); - routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); - - let change = routing.store_route_record(DEVICE, vec![NEIGHBOR]); + let first = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); + let repeat = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); + let new_path = routing.store_route_record(DEVICE, vec![NEIGHBOR]); - assert!(change.source); - assert!(change.any); + assert_eq!([first, repeat, new_path], [changed, unchanged, changed]); } // A record also carries the routes to its relays. An unchanged record restores a relay @@ -197,12 +192,16 @@ fn test_unchanged_route_record_reports_a_relay_route_it_restores() { let mut routing = Routing::default(); routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); assert!(routing.remove_route_record(ROUTER)); - assert_eq!(routing.route_to(ROUTER, MAX_SOURCE_ROUTE), None); let change = routing.store_route_record(DEVICE, vec![ROUTER, NEIGHBOR]); - assert!(!change.source); - assert!(change.any); + assert_eq!( + change, + RouteRecordChange { + source: false, + any: true, + } + ); assert_eq!( routing.route_to(ROUTER, MAX_SOURCE_ROUTE), Some(Route::SourceRouted(vec![NEIGHBOR])) diff --git a/crates/ziggurat-zigbee/src/sends/tests.rs b/crates/ziggurat-zigbee/src/sends/tests.rs index 962e7a2..c12b728 100644 --- a/crates/ziggurat-zigbee/src/sends/tests.rs +++ b/crates/ziggurat-zigbee/src/sends/tests.rs @@ -1,3 +1,4 @@ +use alloc::vec; use alloc::vec::Vec; use core::time::Duration; @@ -10,7 +11,9 @@ use super::*; type TestSends = Sends<&'static str, u8, SendFailure>; type TestActions = Actions<&'static str, u8, SendFailure>; -type TestTick = Tick<&'static str, u8, SendFailure>; + +/// A send's waiter and how it settled. +type Verdict = (u8, Result<(), SendFailure>); const DESTINATION: Nwk = Nwk(0x1234); const NEXT_HOP: Nwk = Nwk(0x5678); @@ -20,10 +23,55 @@ const HELD_UNICAST: u8 = 1; const ROUTE_REQUEST: u8 = 2; const SECOND_ROUTE_REQUEST: u8 = 3; +const HELD_TIMED_OUT: Verdict = (HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout)); +const REQUEST_CANCELLED: Verdict = (ROUTE_REQUEST, Err(SendFailure::Cancelled)); + /// What the driver's routing table answers about `DESTINATION` during a timer pass. -struct DriverTables { - next_hop: Option, - discovery_live: bool, +#[derive(Clone, Copy)] +enum Tables { + /// No route yet, and a discovery toward it is running. + Discovering, + /// No route, and no discovery is running: the last one lapsed. + Lapsed, + /// A route through `NEXT_HOP`. + Routed, +} + +/// What a timer pass did. The default is a pass that did nothing. +#[derive(Debug, Default, PartialEq)] +struct Pass { + settled: Vec, + /// The driver is asked to restart `DESTINATION`'s discovery. + restarts_discovery: bool, + /// `DESTINATION`'s discovery is reported as failed. + fails_discovery: bool, +} + +impl Pass { + /// A pass that settled these sends and did nothing else. + fn settling(settled: impl Into>) -> Self { + Self { + settled: settled.into(), + ..Self::default() + } + } + + /// A pass that asked for the discovery to be restarted. + fn restarting() -> Self { + Self { + restarts_discovery: true, + ..Self::default() + } + } + + /// A pass that gave up on the discovery and failed the held unicast. + fn failing() -> Self { + Self { + settled: vec![HELD_TIMED_OUT], + fails_discovery: true, + ..Self::default() + } + } } /// The send tables, with the tunables and the clock a test drives them with. @@ -42,6 +90,17 @@ impl Fixture { } } + /// Tables that allow a route discovery to be restarted once. + fn with_one_restart() -> Self { + let fixture = Self::new(); + fixture + .tunables + .set("pending_route_discovery_attempts", 2) + .unwrap(); + + fixture + } + fn advance(&mut self, duration: Duration) { self.now = self.now + duration; } @@ -64,18 +123,20 @@ impl Fixture { id } - /// Cancel a held unicast. - fn cancel(&mut self, id: SendId) { - let actions = self.sends.cancel(id).unwrap(); - assert_eq!( - settled(actions), - [(HELD_UNICAST, Err(SendFailure::Cancelled))] - ); + /// Whether the tables still track a discovery toward `DESTINATION`. + fn discovering(&self) -> bool { + self.sends.has_unrouted(DESTINATION) + } + + /// Fail every unicast held for `DESTINATION`, as the driver does when the device + /// leaves or joins again. + fn fail_held(&mut self) -> Vec { + settled(self.sends.fail_unrouted(DESTINATION)) } /// Broadcast the route request of `DESTINATION`'s discovery, as the driver does: /// four copies, the first due now. Returns what registering the request settles. - fn broadcast_route_request(&mut self, waiter: u8) -> Vec<(u8, Result<(), SendFailure>)> { + fn broadcast_route_request(&mut self, waiter: u8) -> Vec { let (request, actions) = self.sends.enqueue_broadcast( Broadcast { frame: "route request", @@ -96,17 +157,23 @@ impl Fixture { } /// Run a timer pass. - fn tick(&mut self, tables: DriverTables) -> TestTick { - let discovery_deadline = tables - .discovery_live - .then_some(self.now + Duration::from_secs(10)); + fn pass(&mut self, tables: Tables) -> Pass { + let next_hop = matches!(tables, Tables::Routed).then_some(NEXT_HOP); + let discovery_deadline = + (!matches!(tables, Tables::Lapsed)).then_some(self.now + Duration::from_secs(10)); - self.sends.tick( + let tick = self.sends.tick( self.now, &self.tunables, - |_| tables.next_hop.map(|next_hop| (next_hop, None)), + |_| next_hop.map(|next_hop| (next_hop, None)), |_| discovery_deadline, - ) + ); + + Pass { + settled: settled(tick.actions), + restarts_discovery: tick.restart_discovery == [DESTINATION], + fails_discovery: tick.failed_discovery == [DESTINATION], + } } fn dequeue(&mut self) -> Option> { @@ -114,19 +181,22 @@ impl Fixture { } /// Report a successful transmit. Returns what it settles. - fn transmitted( - &mut self, - in_flight: InFlight<&'static str>, - ) -> Vec<(u8, Result<(), SendFailure>)> { + fn transmitted(&mut self, in_flight: InFlight<&'static str>) -> Vec { settled( self.sends .transmitted(in_flight, Ok(()), self.now, Duration::ZERO, Duration::ZERO), ) } + + /// Transmit the next frame that is due. Returns what that settles. + fn transmit_next(&mut self) -> Vec { + let in_flight = self.dequeue().unwrap(); + self.transmitted(in_flight) + } } /// The waiter and verdict of every send the actions settle. -fn settled(actions: TestActions) -> Vec<(u8, Result<(), SendFailure>)> { +fn settled(actions: TestActions) -> Vec { actions .into_iter() .filter_map(|action| match action { @@ -141,15 +211,8 @@ fn test_held_unicast_waits_while_its_discovery_is_live() { let mut fixture = Fixture::new(); fixture.hold_unicast(); - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: true, - }); - - assert!(settled(tick.actions).is_empty()); - assert!(tick.restart_discovery.is_empty()); - assert!(tick.failed_discovery.is_empty()); - assert!(fixture.sends.has_unrouted(DESTINATION)); + assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); + assert!(fixture.discovering()); assert!(fixture.dequeue().is_none()); } @@ -160,15 +223,8 @@ fn test_held_unicast_is_routed_once_a_next_hop_is_found() { let mut fixture = Fixture::new(); fixture.hold_unicast(); - let tick = fixture.tick(DriverTables { - next_hop: Some(NEXT_HOP), - discovery_live: true, - }); - - assert!(settled(tick.actions).is_empty()); - assert!(tick.restart_discovery.is_empty()); - assert!(tick.failed_discovery.is_empty()); - assert!(!fixture.sends.has_unrouted(DESTINATION)); + assert_eq!(fixture.pass(Tables::Routed), Pass::default()); + assert!(!fixture.discovering()); let in_flight = fixture.dequeue().unwrap(); assert_eq!(in_flight.frame, "held unicast"); @@ -183,51 +239,49 @@ fn test_lapsed_discovery_without_a_restart_fails_its_unicasts() { let mut fixture = Fixture::new(); fixture.hold_unicast(); - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); - - assert_eq!( - settled(tick.actions), - [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] - ); - assert!(tick.restart_discovery.is_empty()); - assert_eq!(tick.failed_discovery, [DESTINATION]); - assert!(!fixture.sends.has_unrouted(DESTINATION)); + assert_eq!(fixture.pass(Tables::Lapsed), Pass::failing()); + assert!(!fixture.discovering()); } #[test] fn test_lapsed_discovery_with_a_restart_left_is_restarted_once() { - let mut fixture = Fixture::new(); - fixture - .tunables - .set("pending_route_discovery_attempts", 2) - .unwrap(); + let mut fixture = Fixture::with_one_restart(); fixture.hold_unicast(); - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); - - assert!(settled(tick.actions).is_empty()); - assert_eq!(tick.restart_discovery, [DESTINATION]); - assert!(tick.failed_discovery.is_empty()); - assert!(fixture.sends.has_unrouted(DESTINATION)); + assert_eq!(fixture.pass(Tables::Lapsed), Pass::restarting()); + assert!(fixture.discovering()); // The restarted discovery lapses too, and no restart is left - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); + assert_eq!(fixture.pass(Tables::Lapsed), Pass::failing()); +} + +// A discovery whose sends are all gone has nothing left to fail, or to restart for. +// Reporting it as failed makes the driver mark the route `DiscoveryFailed`, which +// discards a route the reply produced after the sends were gone. +#[test] +fn test_discovery_ends_quietly_after_its_unicast_is_cancelled() { + let mut fixture = Fixture::with_one_restart(); + let id = fixture.hold_unicast(); + + let cancelled = settled(fixture.sends.cancel(id).unwrap()); + assert_eq!(cancelled, [(HELD_UNICAST, Err(SendFailure::Cancelled))]); + + assert_eq!(fixture.pass(Tables::Lapsed), Pass::default()); + assert!(!fixture.discovering()); +} + +#[test] +fn test_discovery_ends_quietly_after_its_unicast_times_out() { + let mut fixture = Fixture::new(); + fixture.hold_unicast(); + // The unicast passes its deadline in the same pass that finds the discovery lapsed + fixture.advance(fixture.tunables.handoff_timeout()); assert_eq!( - settled(tick.actions), - [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] + fixture.pass(Tables::Lapsed), + Pass::settling([(HELD_UNICAST, Err(SendFailure::HandoffTimeout))]) ); - assert!(tick.restart_discovery.is_empty()); - assert_eq!(tick.failed_discovery, [DESTINATION]); + assert!(!fixture.discovering()); } // Issue #88: a device that joins again never heard the route request sent before it @@ -238,13 +292,8 @@ fn test_failing_held_unicasts_frees_the_destination_for_a_new_discovery() { let mut fixture = Fixture::new(); fixture.hold_unicast(); - let actions = fixture.sends.fail_unrouted(DESTINATION); - - assert_eq!( - settled(actions), - [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] - ); - assert!(!fixture.sends.has_unrouted(DESTINATION)); + assert_eq!(fixture.fail_held(), [HELD_TIMED_OUT]); + assert!(!fixture.discovering()); } // An abandoned discovery must not keep broadcasting its route request. @@ -255,31 +304,14 @@ fn test_failing_held_unicasts_stops_their_route_request() { assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); // The first of the four copies goes out - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: true, - }); - assert!(settled(tick.actions).is_empty()); - let first_copy = fixture.dequeue().unwrap(); - assert!(fixture.transmitted(first_copy).is_empty()); + assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); + assert!(fixture.transmit_next().is_empty()); - let actions = fixture.sends.fail_unrouted(DESTINATION); - - assert_eq!( - settled(actions), - [ - (HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout)), - (ROUTE_REQUEST, Err(SendFailure::Cancelled)), - ] - ); + assert_eq!(fixture.fail_held(), [HELD_TIMED_OUT, REQUEST_CANCELLED]); // The other three copies are never queued fixture.advance(Duration::from_secs(1)); - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); - assert!(settled(tick.actions).is_empty()); + assert_eq!(fixture.pass(Tables::Lapsed), Pass::default()); assert!(fixture.dequeue().is_none()); } @@ -290,29 +322,19 @@ fn test_stopped_route_request_in_the_radio_sends_no_further_copy() { fixture.hold_unicast(); assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: true, - }); - assert!(settled(tick.actions).is_empty()); + assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); let first_copy = fixture.dequeue().unwrap(); // Only the held unicast settles: the route request is on loan to the radio - let actions = fixture.sends.fail_unrouted(DESTINATION); - assert_eq!( - settled(actions), - [(HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout))] - ); - + assert_eq!(fixture.fail_held(), [HELD_TIMED_OUT]); assert!(fixture.transmitted(first_copy).is_empty()); // One copy reached the air, which completes a route request fixture.advance(Duration::from_secs(1)); - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); - assert_eq!(settled(tick.actions), [(ROUTE_REQUEST, Ok(()))]); + assert_eq!( + fixture.pass(Tables::Lapsed), + Pass::settling([(ROUTE_REQUEST, Ok(()))]) + ); assert!(fixture.dequeue().is_none()); } @@ -323,81 +345,13 @@ fn test_new_route_request_stops_the_one_it_replaces() { fixture.hold_unicast(); assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); - let stopped = fixture.broadcast_route_request(SECOND_ROUTE_REQUEST); - - assert_eq!(stopped, [(ROUTE_REQUEST, Err(SendFailure::Cancelled))]); - - // Only the new request has a copy to send - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: true, - }); - assert!(settled(tick.actions).is_empty()); - let only_copy = fixture.dequeue().unwrap(); - assert!(fixture.transmitted(only_copy).is_empty()); - assert!(fixture.dequeue().is_none()); -} - -// A discovery whose sends are all gone has nothing left to fail. Reporting it as failed -// makes the driver mark the route `DiscoveryFailed`, which discards a route the reply -// produced after the sends were gone. -#[test] -fn test_discovery_ends_quietly_after_its_unicast_is_cancelled() { - let mut fixture = Fixture::new(); - let id = fixture.hold_unicast(); - fixture.cancel(id); - - // The discovery lapses with nothing waiting on it - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); - - assert!(settled(tick.actions).is_empty()); - assert!(tick.restart_discovery.is_empty()); - assert!(tick.failed_discovery.is_empty()); - assert!(!fixture.sends.has_unrouted(DESTINATION)); -} - -#[test] -fn test_discovery_ends_quietly_after_its_unicast_times_out() { - let mut fixture = Fixture::new(); - fixture.hold_unicast(); - - // The unicast passes its deadline in the same pass that finds the discovery lapsed - fixture.advance(fixture.tunables.handoff_timeout()); - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); - assert_eq!( - settled(tick.actions), - [(HELD_UNICAST, Err(SendFailure::HandoffTimeout))] + fixture.broadcast_route_request(SECOND_ROUTE_REQUEST), + [REQUEST_CANCELLED] ); - assert!(tick.restart_discovery.is_empty()); - assert!(tick.failed_discovery.is_empty()); - assert!(!fixture.sends.has_unrouted(DESTINATION)); -} -// A restart is for a send that still waits. A discovery nothing waits on is not -// restarted, however many restarts it has left. -#[test] -fn test_discovery_nothing_waits_on_is_not_restarted() { - let mut fixture = Fixture::new(); - fixture - .tunables - .set("pending_route_discovery_attempts", 2) - .unwrap(); - let id = fixture.hold_unicast(); - fixture.cancel(id); - - let tick = fixture.tick(DriverTables { - next_hop: None, - discovery_live: false, - }); - - assert!(tick.restart_discovery.is_empty()); - assert!(tick.failed_discovery.is_empty()); - assert!(!fixture.sends.has_unrouted(DESTINATION)); + // Only the new request has a copy to send + assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); + assert!(fixture.transmit_next().is_empty()); + assert!(fixture.dequeue().is_none()); } From adaa9be450edc1962b18db7603185d530be3db38 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:46:51 +0000 Subject: [PATCH 11/29] Keep an idle discovery tracked until it lapses --- crates/ziggurat-zigbee/src/sends/mod.rs | 32 +++++++++++++++----- crates/ziggurat-zigbee/src/sends/tests.rs | 11 +++++-- crates/ziggurat-zigbee/src/sends/unrouted.rs | 17 ++++++----- 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/crates/ziggurat-zigbee/src/sends/mod.rs b/crates/ziggurat-zigbee/src/sends/mod.rs index 5566520..dda38f5 100644 --- a/crates/ziggurat-zigbee/src/sends/mod.rs +++ b/crates/ziggurat-zigbee/src/sends/mod.rs @@ -443,13 +443,16 @@ impl> Sends { } /// Move the unrouted sends to `destination` that `next_hop` finds a next hop for. + /// Returns whether any moved. fn route_unicasts( &mut self, destination: Nwk, now: Instant, tunables: &Tunables, next_hop: &mut impl FnMut(&mut F) -> Option<(Nwk, Option)>, - ) { + ) -> bool { + let mut routed = false; + for id in self.unrouted_unicasts.sends_to(destination) { // `next_hop` may rewrite the frame for a source route. Without a next hop // the send stays held. @@ -474,7 +477,10 @@ impl> Sends { None, tunables, ); + routed = true; } + + routed } /// The key of every broadcast awaiting its passive-ack quorum. @@ -548,13 +554,25 @@ impl> Sends { let mut failed_discovery = Vec::new(); for destination in self.unrouted_unicasts.destinations() { - self.route_unicasts(destination, now, tunables, &mut next_hop); + let routed = self.route_unicasts(destination, now, tunables, &mut next_hop); + let idle = self.unrouted_unicasts.is_idle(destination); + + // A route was found and nothing waits any more: the discovery did its job + if routed && idle { + self.unrouted_unicasts.end_discovery(destination); + continue; + } + + // The route request is still on the network, whether or not anything waits + // on it. It stays tracked so a send that arrives meanwhile rides it + if discovery_deadline(destination).is_some() { + continue; + } - // A discovery nothing waits on is over, not failed: reporting it as failed - // would discard a route it found after its sends were gone - if self.unrouted_unicasts.end_idle_discovery(destination) - || discovery_deadline(destination).is_some() - { + // A lapsed discovery nothing waits on is over, not failed: reporting it as + // failed would discard a route it found after its sends were gone + if idle { + self.unrouted_unicasts.end_discovery(destination); continue; } diff --git a/crates/ziggurat-zigbee/src/sends/tests.rs b/crates/ziggurat-zigbee/src/sends/tests.rs index c12b728..31a1e0a 100644 --- a/crates/ziggurat-zigbee/src/sends/tests.rs +++ b/crates/ziggurat-zigbee/src/sends/tests.rs @@ -255,9 +255,11 @@ fn test_lapsed_discovery_with_a_restart_left_is_restarted_once() { assert_eq!(fixture.pass(Tables::Lapsed), Pass::failing()); } -// A discovery whose sends are all gone has nothing left to fail, or to restart for. -// Reporting it as failed makes the driver mark the route `DiscoveryFailed`, which -// discards a route the reply produced after the sends were gone. +// The route request is on the network whether or not anything still waits on it. A +// discovery whose sends are all gone stays tracked until it lapses, so a send that +// arrives meanwhile does not start a second one. Then it has nothing to fail, or to +// restart for: reporting it as failed makes the driver mark the route +// `DiscoveryFailed`, which discards a route the reply produced. #[test] fn test_discovery_ends_quietly_after_its_unicast_is_cancelled() { let mut fixture = Fixture::with_one_restart(); @@ -266,6 +268,9 @@ fn test_discovery_ends_quietly_after_its_unicast_is_cancelled() { let cancelled = settled(fixture.sends.cancel(id).unwrap()); assert_eq!(cancelled, [(HELD_UNICAST, Err(SendFailure::Cancelled))]); + assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); + assert!(fixture.discovering()); + assert_eq!(fixture.pass(Tables::Lapsed), Pass::default()); assert!(!fixture.discovering()); } diff --git a/crates/ziggurat-zigbee/src/sends/unrouted.rs b/crates/ziggurat-zigbee/src/sends/unrouted.rs index 7aa8b62..519c336 100644 --- a/crates/ziggurat-zigbee/src/sends/unrouted.rs +++ b/crates/ziggurat-zigbee/src/sends/unrouted.rs @@ -147,15 +147,15 @@ impl> UnroutedUnicasts { self.records.remove(&id).unwrap() } - /// End `destination`'s discovery if no send waits on it any more: its sends were - /// routed, cancelled or timed out. Returns whether it ended. - pub(super) fn end_idle_discovery(&mut self, destination: Nwk) -> bool { - if !self.sends_to(destination).is_empty() { - return false; - } + /// Whether no send waits on `destination`'s discovery any more: its sends were + /// routed, cancelled or timed out. + pub(super) fn is_idle(&self, destination: Nwk) -> bool { + self.sends_to(destination).is_empty() + } + /// Stop tracking `destination`'s discovery. + pub(super) fn end_discovery(&mut self, destination: Nwk) { self.discoveries.remove(&destination); - true } fn settled(send: Record, failure: SendFailure) -> Action { @@ -206,7 +206,8 @@ impl> UnroutedUnicasts { } /// The destination stays awaited, so a new send to it does not start a second - /// discovery; it lapses on its own once nothing waits on it. + /// discovery: the route request is on the network whether or not anything still + /// waits on it. The discovery ends once it lapses. pub(super) fn cancel(&mut self, id: SendId) -> Actions { let send = self.records.remove(&id).unwrap(); let mut actions = Actions::new(); From 779270bcdc12933944b39141ae8b4f6700415a71 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:48:18 +0000 Subject: [PATCH 12/29] Check held sends against a model over every short sequence of operations --- crates/ziggurat-zigbee/src/sends/tests.rs | 160 +++++++++++++++++++++- 1 file changed, 159 insertions(+), 1 deletion(-) diff --git a/crates/ziggurat-zigbee/src/sends/tests.rs b/crates/ziggurat-zigbee/src/sends/tests.rs index 31a1e0a..bcccf4a 100644 --- a/crates/ziggurat-zigbee/src/sends/tests.rs +++ b/crates/ziggurat-zigbee/src/sends/tests.rs @@ -1,3 +1,4 @@ +use alloc::collections::VecDeque; use alloc::vec; use alloc::vec::Vec; use core::time::Duration; @@ -27,7 +28,7 @@ const HELD_TIMED_OUT: Verdict = (HELD_UNICAST, Err(SendFailure::RouteDiscoveryTi const REQUEST_CANCELLED: Verdict = (ROUTE_REQUEST, Err(SendFailure::Cancelled)); /// What the driver's routing table answers about `DESTINATION` during a timer pass. -#[derive(Clone, Copy)] +#[derive(Debug, Clone, Copy)] enum Tables { /// No route yet, and a discovery toward it is running. Discovering, @@ -360,3 +361,160 @@ fn test_new_route_request_stops_the_one_it_replaces() { assert!(fixture.transmit_next().is_empty()); assert!(fixture.dequeue().is_none()); } + +/// One operation on the held unicasts to `DESTINATION`. +#[derive(Debug, Clone, Copy)] +enum Step { + Hold, + /// Cancel the oldest held unicast. + Cancel, + /// Let every unicast held so far pass its deadline. + Expire, + /// Fail the held unicasts, as the driver does when the device leaves. + FailHeld, + Pass(Tables), +} + +const STEPS: [Step; 7] = [ + Step::Hold, + Step::Cancel, + Step::Expire, + Step::FailHeld, + Step::Pass(Tables::Discovering), + Step::Pass(Tables::Lapsed), + Step::Pass(Tables::Routed), +]; + +/// What the tables should hold for `DESTINATION`, as counts instead of records. +#[derive(Debug, Default)] +struct Model { + /// Held unicasts within their deadline. + waiting: usize, + /// Held unicasts past their deadline, which the next pass fails. + expired: usize, + discovering: bool, + restarts_left: u8, +} + +fn verdicts(count: usize, verdict: fn() -> Verdict) -> Vec { + (0..count).map(|_| verdict()).collect() +} + +/// Run `steps` against the tables and the model, comparing them after each one. +fn check_held_unicasts(steps: &[Step], restarts: u8) { + let mut fixture = Fixture::new(); + fixture + .tunables + .set("pending_route_discovery_attempts", u64::from(restarts) + 1) + .unwrap(); + + let mut model = Model::default(); + // The held unicasts, oldest first, so the expired ones lead + let mut held = VecDeque::new(); + + for step in steps { + match *step { + Step::Hold => { + held.push_back(fixture.hold_unicast()); + + if !model.discovering { + model.discovering = true; + model.restarts_left = restarts; + } + model.waiting += 1; + } + Step::Cancel => { + let Some(id) = held.pop_front() else { + continue; + }; + + let cancelled = settled(fixture.sends.cancel(id).unwrap()); + assert_eq!(cancelled, [(HELD_UNICAST, Err(SendFailure::Cancelled))]); + + if model.expired > 0 { + model.expired -= 1; + } else { + model.waiting -= 1; + } + } + Step::Expire => { + fixture.advance(fixture.tunables.handoff_timeout()); + + model.expired += model.waiting; + model.waiting = 0; + } + Step::FailHeld => { + let failed = verdicts(model.expired + model.waiting, || HELD_TIMED_OUT); + assert_eq!(fixture.fail_held(), failed, "{steps:?}"); + + held.clear(); + model = Model::default(); + } + Step::Pass(tables) => { + // Deadlines come first, so routing never revives a late send + let mut expected = Pass::settling(verdicts(model.expired, || { + (HELD_UNICAST, Err(SendFailure::HandoffTimeout)) + })); + held.drain(..model.expired); + model.expired = 0; + + let mut routed = 0; + match tables { + Tables::Routed if model.waiting > 0 => { + routed = model.waiting; + model.waiting = 0; + model.discovering = false; + } + // The route request is still on the network: the discovery stays + Tables::Routed | Tables::Discovering => {} + Tables::Lapsed if !model.discovering || model.waiting == 0 => { + model.discovering = false; + } + Tables::Lapsed if model.restarts_left > 0 => { + model.restarts_left -= 1; + expected.restarts_discovery = true; + } + Tables::Lapsed => { + expected + .settled + .extend(verdicts(model.waiting, || HELD_TIMED_OUT)); + expected.fails_discovery = true; + model.waiting = 0; + model.discovering = false; + } + } + if model.waiting == 0 { + held.clear(); + } + + assert_eq!(fixture.pass(tables), expected, "{steps:?}"); + + for _ in 0..routed { + assert_eq!(fixture.transmit_next(), [(HELD_UNICAST, Ok(()))]); + } + } + } + + assert_eq!(fixture.discovering(), model.discovering, "{steps:?}"); + } +} + +// Every sequence of six operations, with and without a discovery restart. The model +// states when a discovery is tracked: from the first held unicast until a route takes +// the last one, the discovery lapses with nothing waiting, or its unicasts fail. +#[test] +fn test_held_unicasts_match_the_model_over_every_short_sequence() { + const LENGTH: u32 = 6; + + for restarts in [0, 1] { + for mut sequence in 0..STEPS.len().pow(LENGTH) { + let mut steps = Vec::new(); + for _ in 0..LENGTH { + steps.push(STEPS[sequence % STEPS.len()]); + sequence /= STEPS.len(); + } + + check_held_unicasts(&steps, restarts); + } + } +} From 574d4618884a9ea1bdc544a37491627d687a6624 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:56:58 +0000 Subject: [PATCH 13/29] Let the routing table own route discovery for held sends --- .../ziggurat-driver/src/zigbee_stack/nwk.rs | 20 +- .../ziggurat-driver/src/zigbee_stack/route.rs | 16 +- .../ziggurat-driver/src/zigbee_stack/sends.rs | 41 ++- crates/ziggurat-zigbee/src/nwk/routing.rs | 106 +++++- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 265 ++++++++++++++ .../ziggurat-zigbee/src/sends/broadcasts.rs | 4 + crates/ziggurat-zigbee/src/sends/mod.rs | 121 ++----- crates/ziggurat-zigbee/src/sends/tests.rs | 334 ++---------------- crates/ziggurat-zigbee/src/sends/unrouted.rs | 126 ++----- 9 files changed, 508 insertions(+), 525 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs index bb8642e..528a7e9 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs @@ -426,8 +426,6 @@ impl ZigbeeStack { tracing::debug!("Holding frame until a route to {destination:?} is known"); let slot = Arc::new(SendSlot::default()); - // The first unrouted send to a destination starts its discovery - let discovering = core.sends.has_unrouted(destination); let (id, mut actions) = core.sends.enqueue_unrouted_unicast( UnroutedUnicast { frame: Frame::nwk(nwk_frame, security), @@ -439,8 +437,22 @@ impl ZigbeeStack { now, &self.tunables, ); - if !discovering { - actions.extend(self.start_route_discovery(&mut core, destination)); + + // The routing table starts a discovery unless one it started for this + // destination is still tracked + let route_request_identifier = core.nib.routing.want_route( + self.state.network_address, + destination, + now, + self.tunables.route_discovery_time(), + self.tunables.pending_route_discovery_attempts(), + ); + if let Some(route_request_identifier) = route_request_identifier { + actions.extend(self.broadcast_discovery_request( + &mut core, + destination, + route_request_identifier, + )); } (SendHandle::new(id, slot), actions) diff --git a/crates/ziggurat-driver/src/zigbee_stack/route.rs b/crates/ziggurat-driver/src/zigbee_stack/route.rs index 9904657..da42dc1 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/route.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/route.rs @@ -443,24 +443,16 @@ impl ZigbeeStack { } } - /// Begin or restart ad-hoc route discovery toward a destination: the routing entry - /// enters `DiscoveryUnderway` and a route request is broadcast. The send timer - /// watches the discovery on behalf of the unrouted sends to it; this only kicks it - /// off. Under a core lock the caller holds and commits. - pub(super) fn start_route_discovery( + /// Broadcast the route request of a discovery the routing table started or + /// restarted toward a destination. Under a core lock the caller holds and commits. + pub(super) fn broadcast_discovery_request( &self, core: &mut ZigbeeCore, destination: Nwk, + route_request_identifier: u8, ) -> SendActions { tracing::debug!("Sending route discovery for NWK {destination:?}"); - let route_request_identifier = core.nib.routing.begin_discovery( - self.state.network_address, - destination, - self.core_now(), - self.tunables.route_discovery_time(), - ); - // If we know the EUI64 corresponding to the NWK, use it let destination_eui64 = core.nib.address_map.eui64_for(destination); diff --git a/crates/ziggurat-driver/src/zigbee_stack/sends.rs b/crates/ziggurat-driver/src/zigbee_stack/sends.rs index 02fa5f1..7dd845b 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/sends.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/sends.rs @@ -298,8 +298,9 @@ impl ZigbeeStack { } /// One timer pass: settle the broadcasts whose passive-ack quorum was heard, run - /// the send tables' pass against the routing and neighbor tables, then start the - /// route discoveries it asks for. Returns when the next pass is due. + /// the send tables' pass against the routing and neighbor tables, then let the + /// routing table restart or give up on the discoveries that lapsed. Returns when + /// the next pass is due. fn drive_sends(&self) -> Option { let now = self.core_now(); @@ -314,25 +315,37 @@ impl ZigbeeStack { } } - let tick = core.sends.tick( + actions.extend(core.sends.tick(now, &self.tunables, |frame| { + self.resolve_unrouted_unicast(&core.nib, frame, now) + })); + + // A discovery is only restarted for a destination a held send still waits on + let discoveries = core.nib.routing.tick_discoveries( + self.state.network_address, now, - &self.tunables, - |frame| self.resolve_unrouted_unicast(&core.nib, frame, now), - |destination| core.nib.routing.discovery_deadline(destination, now), + self.tunables.route_discovery_time(), + |destination| core.sends.has_unrouted(destination), ); - actions.extend(tick.actions); - for destination in tick.restart_discovery { - actions.extend(self.start_route_discovery(core, destination)); + for (destination, route_request_identifier) in discoveries.restarted { + actions.extend(self.broadcast_discovery_request( + core, + destination, + route_request_identifier, + )); } - for destination in tick.failed_discovery { - core.nib.routing.mark_discovery_failed(destination); + for destination in discoveries.unreachable { + actions.extend(core.sends.fail_unrouted(destination)); } - let next_wake = core - .sends - .next_wake(|destination| core.nib.routing.discovery_deadline(destination, now)); + let next_wake = [ + core.sends.next_wake(), + core.nib.routing.next_discovery_lapse(now), + ] + .into_iter() + .flatten() + .min(); // This pass is the timer task's own: waking the timer here would make it spin self.run_send_actions(guard, actions); diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 159e686..c7f6da4 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -193,6 +193,19 @@ pub struct RouteReplyOutcome { pub update: Option, } +/// What a timer pass decided about the route discoveries we started. +#[derive(Debug, Default, PartialEq, Eq)] +#[must_use = "restarted discoveries need their route request broadcast, and frames \ + held for an unreachable destination must fail"] +pub struct DiscoveryTick { + /// Lapsed discoveries started again: the destination, and the identifier to put in + /// the new route request. + pub restarted: Vec<(Nwk, RouteRequestId)>, + /// Destinations given up on, with no restart left. Frames held for a route to them + /// fail. + pub unreachable: Vec, +} + /// The NWK routing layer: route table, route discovery table, and route record table, /// with the decision logic for route request/reply processing. #[derive(Debug, Default)] @@ -207,6 +220,12 @@ pub struct Routing { discovery_table: FlatMap<(Nwk, RouteRequestId), DiscoveryEntry>, route_record_table: FlatMap>, + /// The destinations we are discovering a route to for our own frames, each with + /// the number of times its discovery may still be restarted. An entry lasts from + /// the first frame that wants the route until a reply answers the discovery, or + /// until it lapses with nothing left to restart it for. + own_discoveries: FlatMap, + /// Implied from the spec: "notice that this 8-bit identifier is distinct from the /// 16-bit Routing Sequence Number. The former is used to discern route requests /// originating in a particular router; the latter is used to identify stale routing @@ -255,8 +274,84 @@ impl Routing { }) } - pub fn mark_discovery_failed(&mut self, destination: Nwk) { - if let Some(entry) = self.route_table.get_mut(&destination) { + /// One of our frames needs a route to `destination`. Returns the identifier to put + /// in the route request to broadcast, or `None` when a discovery we started is + /// still tracked. Its route request is on the network whatever became of the + /// frames that wanted it, so the new frame waits on that one. + pub fn want_route( + &mut self, + own_address: Nwk, + destination: Nwk, + now: Instant, + route_discovery_time: Duration, + attempts: u8, + ) -> Option { + if self.own_discoveries.contains_key(&destination) { + return None; + } + + // `attempts` counts discoveries; the first one is not a restart + self.own_discoveries + .insert(destination, attempts.saturating_sub(1)); + + Some(self.begin_discovery(own_address, destination, now, route_discovery_time)) + } + + /// Decide what becomes of each discovery we started that has lapsed unanswered. + /// `wanted` says whether a frame still waits for a route to a destination. + pub fn tick_discoveries( + &mut self, + own_address: Nwk, + now: Instant, + route_discovery_time: Duration, + wanted: impl Fn(Nwk) -> bool, + ) -> DiscoveryTick { + let mut tick = DiscoveryTick::default(); + + let lapsed: Vec = self + .own_discoveries + .keys() + .copied() + .filter(|destination| self.discovery_deadline(*destination, now).is_none()) + .collect(); + + for destination in lapsed { + // Nothing waits for the route any more: the discovery is over, not failed + if !wanted(destination) { + self.own_discoveries.remove(&destination); + continue; + } + + let restarts_left = self.own_discoveries.get_mut(&destination).unwrap(); + + if *restarts_left > 0 { + *restarts_left -= 1; + let request_id = + self.begin_discovery(own_address, destination, now, route_discovery_time); + tick.restarted.push((destination, request_id)); + } else { + self.own_discoveries.remove(&destination); + self.mark_discovery_failed(destination); + tick.unreachable.push(destination); + } + } + + tick + } + + /// When the next discovery we started lapses, or `None` when none is live. + pub fn next_discovery_lapse(&self, now: Instant) -> Option { + self.own_discoveries + .keys() + .filter_map(|destination| self.discovery_deadline(*destination, now)) + .min() + } + + /// A discovery ended without a route. A route found some other way stays. + fn mark_discovery_failed(&mut self, destination: Nwk) { + if let Some(entry) = self.route_table.get_mut(&destination) + && entry.status != Status::Active + { entry.status = Status::DiscoveryFailed; } } @@ -305,6 +400,7 @@ impl Routing { // unknown responder. That router gets no route through us for that discovery. self.route_table.remove(&nwk); self.route_record_table.remove(&nwk); + self.own_discoveries.remove(&nwk); // Only our own discoveries toward it are dropped. Discoveries we relay stay // until they expire: they are the only duplicate check for late copies of @@ -394,7 +490,7 @@ impl Routing { /// Prepare table state for a route discovery we originate: the routing entry enters /// `DiscoveryUnderway` and a discovery entry keyed by our own address is created. /// Returns the request identifier to put in the route request command. - pub fn begin_discovery( + fn begin_discovery( &mut self, own_address: Nwk, destination: Nwk, @@ -626,6 +722,10 @@ impl Routing { routing_entry.path_cost = updated_path_cost; discovery_entry.residual_cost = updated_path_cost; + // The discovery is answered. Better replies are still taken until its + // entry expires, but a frame that needs the route again starts a new one + self.own_discoveries.remove(&responder); + let update = (previous_next_hop != Some(sender)).then_some(RouteUpdate { destination: responder, next_hop: sender, diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 393bd45..102e018 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -1,4 +1,5 @@ use alloc::vec; +use alloc::vec::Vec; use core::time::Duration; use ziggurat_ieee_802154::types::Nwk; @@ -207,3 +208,267 @@ fn test_unchanged_route_record_reports_a_relay_route_it_restores() { Some(Route::SourceRouted(vec![NEIGHBOR])) ); } + +/// A frame of ours wants a route to `DEVICE`. Returns whether that starts a discovery. +fn want(routing: &mut Routing, now: Instant, attempts: u8) -> bool { + routing + .want_route(OWN, DEVICE, now, ROUTE_DISCOVERY_TIME, attempts) + .is_some() +} + +/// Run a timer pass, with a frame still waiting for the route or not. +fn tick(routing: &mut Routing, now: Instant, wanted: bool) -> DiscoveryTick { + routing.tick_discoveries(OWN, now, ROUTE_DISCOVERY_TIME, |_| wanted) +} + +// The route request is on the network whatever became of the frame that wanted it, so +// a second frame waits on the same discovery. +#[test] +fn test_wanting_a_route_starts_one_discovery() { + let mut routing = Routing::default(); + + assert!(want(&mut routing, START, 1)); + assert!(!want(&mut routing, START, 1)); + + assert_eq!( + routing.route_status(DEVICE), + Some(Status::DiscoveryUnderway) + ); + assert_eq!( + routing.next_discovery_lapse(START), + Some(START + ROUTE_DISCOVERY_TIME) + ); +} + +#[test] +fn test_lapsed_discovery_without_a_restart_makes_the_destination_unreachable() { + let mut routing = Routing::default(); + assert!(want(&mut routing, START, 1)); + + let lapsed = START + ROUTE_DISCOVERY_TIME; + assert_eq!(tick(&mut routing, lapsed, true).unreachable, vec![DEVICE]); + + assert_eq!(routing.route_status(DEVICE), Some(Status::DiscoveryFailed)); + // The next frame starts over + assert!(want(&mut routing, lapsed, 1)); +} + +#[test] +fn test_lapsed_discovery_is_restarted_while_a_frame_waits() { + let mut routing = Routing::default(); + assert!(want(&mut routing, START, 2)); + + let lapsed = START + ROUTE_DISCOVERY_TIME; + let restarted = tick(&mut routing, lapsed, true).restarted; + + assert_eq!(restarted.len(), 1); + assert_eq!(restarted[0].0, DEVICE); + assert_eq!( + routing.next_discovery_lapse(lapsed), + Some(lapsed + ROUTE_DISCOVERY_TIME) + ); + + // The restart was the last one + let lapsed_again = lapsed + ROUTE_DISCOVERY_TIME; + assert_eq!( + tick(&mut routing, lapsed_again, true).unreachable, + vec![DEVICE] + ); +} + +// A discovery nothing waits on is over, not failed: it is neither restarted nor +// reported, and a route found for it stays. +#[test] +fn test_lapsed_discovery_nothing_waits_on_ends_quietly() { + let mut routing = Routing::default(); + assert!(want(&mut routing, START, 2)); + + let lapsed = START + ROUTE_DISCOVERY_TIME; + assert_eq!(tick(&mut routing, lapsed, false), DiscoveryTick::default()); + + assert!(want(&mut routing, lapsed, 2)); +} + +#[test] +fn test_live_discovery_is_left_alone() { + let mut routing = Routing::default(); + assert!(want(&mut routing, START, 1)); + + let live = START + Duration::from_secs(1); + assert_eq!(tick(&mut routing, live, true), DiscoveryTick::default()); + assert_eq!(tick(&mut routing, live, false), DiscoveryTick::default()); + assert!(!want(&mut routing, live, 1)); +} + +// The reply answers the discovery. If the route is lost again, the next frame starts a +// new one instead of waiting on a request that was already answered. +#[test] +fn test_route_reply_ends_the_discovery() { + let mut routing = Routing::default(); + let request_id = routing + .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, 1) + .unwrap(); + + let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, 3); + assert_eq!(reply.disposition, RouteReplyDisposition::Established); + assert_eq!(routing.next_hop(DEVICE), Some(NEIGHBOR)); + + routing.invalidate_via(NEIGHBOR); + assert!(want(&mut routing, START, 1)); +} + +#[test] +fn test_forget_ends_our_discovery_toward_the_device() { + let mut routing = Routing::default(); + assert!(want(&mut routing, START, 1)); + + routing.forget(OWN, DEVICE); + + assert_eq!(routing.next_discovery_lapse(START), None); + assert!(want(&mut routing, START, 1)); +} + +/// One event in the life of our route discovery toward `DEVICE`. +#[derive(Debug, Clone, Copy)] +enum Step { + /// A frame is held for the route. + Hold, + /// The held frames are gone: cancelled, or past their deadline. + Release, + /// Time passes until every discovery has lapsed. + Lapse, + Tick, + /// The device answers the live discovery. + Reply, + /// The route to the device is lost. + Break, +} + +const STEPS: [Step; 6] = [ + Step::Hold, + Step::Release, + Step::Lapse, + Step::Tick, + Step::Reply, + Step::Break, +]; + +/// What `Routing` should hold of our discovery toward `DEVICE`. +#[derive(Debug, Default)] +struct Model { + /// A frame waits for the route. + wanted: bool, + /// A discovery we started is tracked: from the first frame that wants the route + /// until a reply answers it, or it lapses with nothing left to restart it for. + tracked: bool, + /// The tracked discovery has not lapsed. + live: bool, + restarts_left: u8, +} + +/// Run `steps` against `Routing` and the model, comparing them after each one. +fn check_own_discovery(steps: &[Step], attempts: u8) { + let mut routing = Routing::default(); + let mut model = Model::default(); + let mut now = START; + let mut request_id = None; + + for step in steps { + match *step { + // The driver only holds a frame when it has no route + Step::Hold if routing.next_hop(DEVICE).is_some() => {} + Step::Hold => { + let started = routing.want_route(OWN, DEVICE, now, ROUTE_DISCOVERY_TIME, attempts); + assert_eq!(started.is_some(), !model.tracked, "{steps:?}"); + + if let Some(started) = started { + request_id = Some(started); + model.tracked = true; + model.live = true; + model.restarts_left = attempts - 1; + } + model.wanted = true; + } + Step::Release => model.wanted = false, + Step::Lapse => { + now = now + ROUTE_DISCOVERY_TIME; + model.live = false; + } + Step::Tick => { + let tick = tick(&mut routing, now, model.wanted); + let mut restarted = false; + let mut unreachable = false; + + if model.tracked && !model.live { + if !model.wanted { + model.tracked = false; + } else if model.restarts_left > 0 { + model.restarts_left -= 1; + model.live = true; + restarted = true; + } else { + model.tracked = false; + // The driver fails the frames held for the destination + model.wanted = false; + unreachable = true; + } + } + + let destinations: Vec = tick.restarted.iter().map(|r| r.0).collect(); + assert_eq!( + destinations, + [DEVICE][..usize::from(restarted)], + "{steps:?}" + ); + assert_eq!( + tick.unreachable, + [DEVICE][..usize::from(unreachable)], + "{steps:?}" + ); + + if let Some(restart) = tick.restarted.first() { + request_id = Some(restart.1); + } + } + Step::Reply if model.tracked && model.live => { + let reply = + routing.accept_route_reply(OWN, OWN, request_id.unwrap(), DEVICE, NEIGHBOR, 3); + assert_eq!( + reply.disposition, + RouteReplyDisposition::Established, + "{steps:?}" + ); + + model.tracked = false; + // The held frames have a next hop and leave + model.wanted = false; + } + Step::Reply => {} + Step::Break => routing.invalidate_via(NEIGHBOR), + } + + assert_eq!( + routing.next_discovery_lapse(now).is_some(), + model.tracked && model.live, + "{steps:?}" + ); + } +} + +// Every sequence of seven events, with one and with two discovery attempts. +#[test] +fn test_own_discovery_matches_the_model_over_every_short_sequence() { + const LENGTH: u32 = 7; + + for attempts in [1, 2] { + for mut sequence in 0..STEPS.len().pow(LENGTH) { + let mut steps = Vec::new(); + for _ in 0..LENGTH { + steps.push(STEPS[sequence % STEPS.len()]); + sequence /= STEPS.len(); + } + + check_own_discovery(&steps, attempts); + } + } +} diff --git a/crates/ziggurat-zigbee/src/sends/broadcasts.rs b/crates/ziggurat-zigbee/src/sends/broadcasts.rs index 3945e59..c65a87c 100644 --- a/crates/ziggurat-zigbee/src/sends/broadcasts.rs +++ b/crates/ziggurat-zigbee/src/sends/broadcasts.rs @@ -295,6 +295,10 @@ impl> Broadcasts { } /// `None` if a copy is in the radio right now, or the send is unknown. + pub(super) fn contains(&self, id: SendId) -> bool { + self.records.contains_key(&id) + } + pub(super) fn cancel(&mut self, id: SendId) -> Option> { let record = self.records.get(&id)?; if record.copy == CopyState::Transmitting { diff --git a/crates/ziggurat-zigbee/src/sends/mod.rs b/crates/ziggurat-zigbee/src/sends/mod.rs index dda38f5..3c8630c 100644 --- a/crates/ziggurat-zigbee/src/sends/mod.rs +++ b/crates/ziggurat-zigbee/src/sends/mod.rs @@ -187,17 +187,6 @@ pub enum Action { }, } -/// What a timer pass leaves for the driver. -#[derive(Debug)] -pub struct Tick { - pub actions: Actions, - /// Destinations whose route discovery lapsed with a restart left. The driver starts - /// a new discovery for each. - pub restart_discovery: Vec, - /// Destinations whose route discovery is over without a route. Their sends failed. - pub failed_discovery: Vec, -} - /// What the driver must do after a mutation, in order. Every mutating verb returns one, /// so a mutation whose consequences are dropped is a compile-time warning. #[must_use = "actions resolve waiters; dropping them loses sends"] @@ -265,6 +254,10 @@ pub struct Sends { unicasts: Unicasts, unrouted_unicasts: UnroutedUnicasts, broadcasts: Broadcasts, + /// The broadcast carrying the latest route request toward a destination, while it + /// still has copies to send. Kept to stop those copies when the discovery is + /// replaced or abandoned. + route_requests: FlatMap, } impl Default for Sends { @@ -274,6 +267,7 @@ impl Default for Sends { unicasts: Unicasts::default(), unrouted_unicasts: UnroutedUnicasts::default(), broadcasts: Broadcasts::default(), + route_requests: FlatMap::new(), } } } @@ -312,10 +306,8 @@ impl> Sends { (id, Actions::new()) } - /// Enqueue a unicast with no route yet, held until its destination's discovery - /// ends. The caller starts that discovery if the destination was not already - /// awaited; the first unrouted send to a destination fixes how often it is - /// restarted. + /// Enqueue a unicast with no route yet, held until a timer pass finds it a next + /// hop. The caller tells the routing table a route is wanted. pub fn enqueue_unrouted_unicast( &mut self, send: UnroutedUnicast, @@ -413,47 +405,40 @@ impl> Sends { self.unicasts.ack_received(ack) } - /// Whether any unrouted send waits on `destination`'s route discovery. + /// Whether any unrouted send waits for a route to `destination`. pub fn has_unrouted(&self, destination: Nwk) -> bool { - self.unrouted_unicasts.has_unrouted(destination) + self.unrouted_unicasts.has_sends_to(destination) } - /// The driver broadcast `request`, the route request of `destination`'s discovery. - /// Copies of the request it replaces still to come are not sent. + /// The driver broadcast `request`, a route request toward `destination`. Copies of + /// the request it replaces still to come are not sent. pub fn route_request_sent(&mut self, destination: Nwk, request: SendId) -> Actions { - match self - .unrouted_unicasts - .route_request_sent(destination, request) - { + match self.route_requests.insert(destination, request) { Some(previous) => self.broadcasts.stop(previous), None => Actions::new(), } } - /// Fail every unrouted send to `destination`: its discovery is over without a - /// route. Copies of its route request still to come are not sent. + /// Fail every unrouted send to `destination`: no route to it will be found. Copies + /// of its route request still to come are not sent. pub fn fail_unrouted(&mut self, destination: Nwk) -> Actions { - let (request, mut actions) = self.unrouted_unicasts.fail(destination); + let mut actions = self.unrouted_unicasts.fail(destination); - if let Some(request) = request { + if let Some(request) = self.route_requests.remove(&destination) { actions.extend(self.broadcasts.stop(request)); } actions } - /// Move the unrouted sends to `destination` that `next_hop` finds a next hop for. - /// Returns whether any moved. + /// Move every unrouted send that `next_hop` finds a next hop for. fn route_unicasts( &mut self, - destination: Nwk, now: Instant, tunables: &Tunables, - next_hop: &mut impl FnMut(&mut F) -> Option<(Nwk, Option)>, - ) -> bool { - let mut routed = false; - - for id in self.unrouted_unicasts.sends_to(destination) { + mut next_hop: impl FnMut(&mut F) -> Option<(Nwk, Option)>, + ) { + for id in self.unrouted_unicasts.ids() { // `next_hop` may rewrite the frame for a source route. Without a next hop // the send stays held. let Some((next_hop, poll_address)) = next_hop(self.unrouted_unicasts.frame_mut(id)) @@ -477,10 +462,7 @@ impl> Sends { None, tunables, ); - routed = true; } - - routed } /// The key of every broadcast awaiting its passive-ack quorum. @@ -505,6 +487,7 @@ impl> Sends { let mut waiters = self.unicasts.abandon(); waiters.extend(self.unrouted_unicasts.abandon()); waiters.extend(self.broadcasts.abandon()); + self.route_requests.clear(); waiters } @@ -544,67 +527,24 @@ impl> Sends { tunables: &Tunables, // A frame's next hop, and the address its device polls under if the frame must // wait for its poll - mut next_hop: impl FnMut(&mut F) -> Option<(Nwk, Option)>, - // When the live route discovery toward a destination lapses - discovery_deadline: impl Fn(Nwk) -> Option, - ) -> Tick { + next_hop: impl FnMut(&mut F) -> Option<(Nwk, Option)>, + ) -> Actions { // Fail the sends past their deadline first, so routing never revives one let mut actions = self.unrouted_unicasts.tick(now); - let mut restart_discovery = Vec::new(); - let mut failed_discovery = Vec::new(); - - for destination in self.unrouted_unicasts.destinations() { - let routed = self.route_unicasts(destination, now, tunables, &mut next_hop); - let idle = self.unrouted_unicasts.is_idle(destination); - - // A route was found and nothing waits any more: the discovery did its job - if routed && idle { - self.unrouted_unicasts.end_discovery(destination); - continue; - } - - // The route request is still on the network, whether or not anything waits - // on it. It stays tracked so a send that arrives meanwhile rides it - if discovery_deadline(destination).is_some() { - continue; - } - - // A lapsed discovery nothing waits on is over, not failed: reporting it as - // failed would discard a route it found after its sends were gone - if idle { - self.unrouted_unicasts.end_discovery(destination); - continue; - } - - if self.unrouted_unicasts.retry_discovery(destination) { - restart_discovery.push(destination); - } else { - actions.extend(self.fail_unrouted(destination)); - failed_discovery.push(destination); - } - } + self.route_unicasts(now, tunables, next_hop); actions.extend(self.unicasts.tick(now)); actions.extend(self.broadcasts.tick(now)); - Tick { - actions, - restart_discovery, - failed_discovery, - } + // A route request with no copy left to send needs no stopping + self.route_requests + .retain(|_, request| self.broadcasts.contains(*request)); + + actions } /// When the timer must next run, or `None` when nothing is waiting on time. - pub fn next_wake( - &self, - discovery_deadline: impl Fn(Nwk) -> Option, - ) -> Option { - let discoveries = self - .unrouted_unicasts - .destinations() - .into_iter() - .filter_map(discovery_deadline); - + pub fn next_wake(&self) -> Option { [ self.unicasts.next_wake(), self.unrouted_unicasts.next_wake(), @@ -612,7 +552,6 @@ impl> Sends { ] .into_iter() .flatten() - .chain(discoveries) .min() } diff --git a/crates/ziggurat-zigbee/src/sends/tests.rs b/crates/ziggurat-zigbee/src/sends/tests.rs index bcccf4a..99f3e07 100644 --- a/crates/ziggurat-zigbee/src/sends/tests.rs +++ b/crates/ziggurat-zigbee/src/sends/tests.rs @@ -1,5 +1,3 @@ -use alloc::collections::VecDeque; -use alloc::vec; use alloc::vec::Vec; use core::time::Duration; @@ -27,54 +25,6 @@ const SECOND_ROUTE_REQUEST: u8 = 3; const HELD_TIMED_OUT: Verdict = (HELD_UNICAST, Err(SendFailure::RouteDiscoveryTimeout)); const REQUEST_CANCELLED: Verdict = (ROUTE_REQUEST, Err(SendFailure::Cancelled)); -/// What the driver's routing table answers about `DESTINATION` during a timer pass. -#[derive(Debug, Clone, Copy)] -enum Tables { - /// No route yet, and a discovery toward it is running. - Discovering, - /// No route, and no discovery is running: the last one lapsed. - Lapsed, - /// A route through `NEXT_HOP`. - Routed, -} - -/// What a timer pass did. The default is a pass that did nothing. -#[derive(Debug, Default, PartialEq)] -struct Pass { - settled: Vec, - /// The driver is asked to restart `DESTINATION`'s discovery. - restarts_discovery: bool, - /// `DESTINATION`'s discovery is reported as failed. - fails_discovery: bool, -} - -impl Pass { - /// A pass that settled these sends and did nothing else. - fn settling(settled: impl Into>) -> Self { - Self { - settled: settled.into(), - ..Self::default() - } - } - - /// A pass that asked for the discovery to be restarted. - fn restarting() -> Self { - Self { - restarts_discovery: true, - ..Self::default() - } - } - - /// A pass that gave up on the discovery and failed the held unicast. - fn failing() -> Self { - Self { - settled: vec![HELD_TIMED_OUT], - fails_discovery: true, - ..Self::default() - } - } -} - /// The send tables, with the tunables and the clock a test drives them with. struct Fixture { sends: TestSends, @@ -91,17 +41,6 @@ impl Fixture { } } - /// Tables that allow a route discovery to be restarted once. - fn with_one_restart() -> Self { - let fixture = Self::new(); - fixture - .tunables - .set("pending_route_discovery_attempts", 2) - .unwrap(); - - fixture - } - fn advance(&mut self, duration: Duration) { self.now = self.now + duration; } @@ -124,8 +63,8 @@ impl Fixture { id } - /// Whether the tables still track a discovery toward `DESTINATION`. - fn discovering(&self) -> bool { + /// Whether a unicast is still held for `DESTINATION`. + fn holding(&self) -> bool { self.sends.has_unrouted(DESTINATION) } @@ -157,24 +96,12 @@ impl Fixture { settled(self.sends.route_request_sent(DESTINATION, request)) } - /// Run a timer pass. - fn pass(&mut self, tables: Tables) -> Pass { - let next_hop = matches!(tables, Tables::Routed).then_some(NEXT_HOP); - let discovery_deadline = - (!matches!(tables, Tables::Lapsed)).then_some(self.now + Duration::from_secs(10)); - - let tick = self.sends.tick( - self.now, - &self.tunables, - |_| next_hop.map(|next_hop| (next_hop, None)), - |_| discovery_deadline, - ); - - Pass { - settled: settled(tick.actions), - restarts_discovery: tick.restart_discovery == [DESTINATION], - fails_discovery: tick.failed_discovery == [DESTINATION], - } + /// Run a timer pass in which the routing table offers this next hop toward + /// `DESTINATION`. Returns what the pass settles. + fn pass(&mut self, next_hop: Option) -> Vec { + settled(self.sends.tick(self.now, &self.tunables, |_| { + next_hop.map(|next_hop| (next_hop, None)) + })) } fn dequeue(&mut self) -> Option> { @@ -208,24 +135,24 @@ fn settled(actions: TestActions) -> Vec { } #[test] -fn test_held_unicast_waits_while_its_discovery_is_live() { +fn test_held_unicast_waits_without_a_next_hop() { let mut fixture = Fixture::new(); fixture.hold_unicast(); - assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); - assert!(fixture.discovering()); + assert!(fixture.pass(None).is_empty()); + assert!(fixture.holding()); assert!(fixture.dequeue().is_none()); } -// A held unicast is bound to its destination, not to one discovery: any pass that -// finds a next hop releases it, even while a discovery toward it is still live. +// A held unicast waits for a next hop, not for a discovery: any pass that finds one +// releases it. #[test] fn test_held_unicast_is_routed_once_a_next_hop_is_found() { let mut fixture = Fixture::new(); fixture.hold_unicast(); - assert_eq!(fixture.pass(Tables::Routed), Pass::default()); - assert!(!fixture.discovering()); + assert!(fixture.pass(Some(NEXT_HOP)).is_empty()); + assert!(!fixture.holding()); let in_flight = fixture.dequeue().unwrap(); assert_eq!(in_flight.frame, "held unicast"); @@ -236,70 +163,39 @@ fn test_held_unicast_is_routed_once_a_next_hop_is_found() { } #[test] -fn test_lapsed_discovery_without_a_restart_fails_its_unicasts() { +fn test_held_unicast_fails_at_its_deadline() { let mut fixture = Fixture::new(); fixture.hold_unicast(); - assert_eq!(fixture.pass(Tables::Lapsed), Pass::failing()); - assert!(!fixture.discovering()); -} - -#[test] -fn test_lapsed_discovery_with_a_restart_left_is_restarted_once() { - let mut fixture = Fixture::with_one_restart(); - fixture.hold_unicast(); - - assert_eq!(fixture.pass(Tables::Lapsed), Pass::restarting()); - assert!(fixture.discovering()); + fixture.advance(fixture.tunables.handoff_timeout()); - // The restarted discovery lapses too, and no restart is left - assert_eq!(fixture.pass(Tables::Lapsed), Pass::failing()); + // The deadline comes first: a next hop does not revive a late send + assert_eq!( + fixture.pass(Some(NEXT_HOP)), + [(HELD_UNICAST, Err(SendFailure::HandoffTimeout))] + ); + assert!(!fixture.holding()); } -// The route request is on the network whether or not anything still waits on it. A -// discovery whose sends are all gone stays tracked until it lapses, so a send that -// arrives meanwhile does not start a second one. Then it has nothing to fail, or to -// restart for: reporting it as failed makes the driver mark the route -// `DiscoveryFailed`, which discards a route the reply produced. #[test] -fn test_discovery_ends_quietly_after_its_unicast_is_cancelled() { - let mut fixture = Fixture::with_one_restart(); +fn test_held_unicast_can_be_cancelled() { + let mut fixture = Fixture::new(); let id = fixture.hold_unicast(); let cancelled = settled(fixture.sends.cancel(id).unwrap()); - assert_eq!(cancelled, [(HELD_UNICAST, Err(SendFailure::Cancelled))]); - assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); - assert!(fixture.discovering()); - - assert_eq!(fixture.pass(Tables::Lapsed), Pass::default()); - assert!(!fixture.discovering()); -} - -#[test] -fn test_discovery_ends_quietly_after_its_unicast_times_out() { - let mut fixture = Fixture::new(); - fixture.hold_unicast(); - - // The unicast passes its deadline in the same pass that finds the discovery lapsed - fixture.advance(fixture.tunables.handoff_timeout()); - assert_eq!( - fixture.pass(Tables::Lapsed), - Pass::settling([(HELD_UNICAST, Err(SendFailure::HandoffTimeout))]) - ); - assert!(!fixture.discovering()); + assert_eq!(cancelled, [(HELD_UNICAST, Err(SendFailure::Cancelled))]); + assert!(!fixture.holding()); } -// Issue #88: a device that joins again never heard the route request sent before it -// joined. Failing the held unicasts must leave nothing behind that keeps the next send -// to the address from starting its own discovery. +// The routing table gave up on the destination, or the device left or joined again. #[test] -fn test_failing_held_unicasts_frees_the_destination_for_a_new_discovery() { +fn test_failing_held_unicasts_leaves_nothing_held() { let mut fixture = Fixture::new(); fixture.hold_unicast(); assert_eq!(fixture.fail_held(), [HELD_TIMED_OUT]); - assert!(!fixture.discovering()); + assert!(!fixture.holding()); } // An abandoned discovery must not keep broadcasting its route request. @@ -310,14 +206,14 @@ fn test_failing_held_unicasts_stops_their_route_request() { assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); // The first of the four copies goes out - assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); + assert!(fixture.pass(None).is_empty()); assert!(fixture.transmit_next().is_empty()); assert_eq!(fixture.fail_held(), [HELD_TIMED_OUT, REQUEST_CANCELLED]); // The other three copies are never queued fixture.advance(Duration::from_secs(1)); - assert_eq!(fixture.pass(Tables::Lapsed), Pass::default()); + assert!(fixture.pass(None).is_empty()); assert!(fixture.dequeue().is_none()); } @@ -328,7 +224,7 @@ fn test_stopped_route_request_in_the_radio_sends_no_further_copy() { fixture.hold_unicast(); assert!(fixture.broadcast_route_request(ROUTE_REQUEST).is_empty()); - assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); + assert!(fixture.pass(None).is_empty()); let first_copy = fixture.dequeue().unwrap(); // Only the held unicast settles: the route request is on loan to the radio @@ -337,10 +233,7 @@ fn test_stopped_route_request_in_the_radio_sends_no_further_copy() { // One copy reached the air, which completes a route request fixture.advance(Duration::from_secs(1)); - assert_eq!( - fixture.pass(Tables::Lapsed), - Pass::settling([(ROUTE_REQUEST, Ok(()))]) - ); + assert_eq!(fixture.pass(None), [(ROUTE_REQUEST, Ok(()))]); assert!(fixture.dequeue().is_none()); } @@ -357,164 +250,7 @@ fn test_new_route_request_stops_the_one_it_replaces() { ); // Only the new request has a copy to send - assert_eq!(fixture.pass(Tables::Discovering), Pass::default()); + assert!(fixture.pass(None).is_empty()); assert!(fixture.transmit_next().is_empty()); assert!(fixture.dequeue().is_none()); } - -/// One operation on the held unicasts to `DESTINATION`. -#[derive(Debug, Clone, Copy)] -enum Step { - Hold, - /// Cancel the oldest held unicast. - Cancel, - /// Let every unicast held so far pass its deadline. - Expire, - /// Fail the held unicasts, as the driver does when the device leaves. - FailHeld, - Pass(Tables), -} - -const STEPS: [Step; 7] = [ - Step::Hold, - Step::Cancel, - Step::Expire, - Step::FailHeld, - Step::Pass(Tables::Discovering), - Step::Pass(Tables::Lapsed), - Step::Pass(Tables::Routed), -]; - -/// What the tables should hold for `DESTINATION`, as counts instead of records. -#[derive(Debug, Default)] -struct Model { - /// Held unicasts within their deadline. - waiting: usize, - /// Held unicasts past their deadline, which the next pass fails. - expired: usize, - discovering: bool, - restarts_left: u8, -} - -fn verdicts(count: usize, verdict: fn() -> Verdict) -> Vec { - (0..count).map(|_| verdict()).collect() -} - -/// Run `steps` against the tables and the model, comparing them after each one. -fn check_held_unicasts(steps: &[Step], restarts: u8) { - let mut fixture = Fixture::new(); - fixture - .tunables - .set("pending_route_discovery_attempts", u64::from(restarts) + 1) - .unwrap(); - - let mut model = Model::default(); - // The held unicasts, oldest first, so the expired ones lead - let mut held = VecDeque::new(); - - for step in steps { - match *step { - Step::Hold => { - held.push_back(fixture.hold_unicast()); - - if !model.discovering { - model.discovering = true; - model.restarts_left = restarts; - } - model.waiting += 1; - } - Step::Cancel => { - let Some(id) = held.pop_front() else { - continue; - }; - - let cancelled = settled(fixture.sends.cancel(id).unwrap()); - assert_eq!(cancelled, [(HELD_UNICAST, Err(SendFailure::Cancelled))]); - - if model.expired > 0 { - model.expired -= 1; - } else { - model.waiting -= 1; - } - } - Step::Expire => { - fixture.advance(fixture.tunables.handoff_timeout()); - - model.expired += model.waiting; - model.waiting = 0; - } - Step::FailHeld => { - let failed = verdicts(model.expired + model.waiting, || HELD_TIMED_OUT); - assert_eq!(fixture.fail_held(), failed, "{steps:?}"); - - held.clear(); - model = Model::default(); - } - Step::Pass(tables) => { - // Deadlines come first, so routing never revives a late send - let mut expected = Pass::settling(verdicts(model.expired, || { - (HELD_UNICAST, Err(SendFailure::HandoffTimeout)) - })); - held.drain(..model.expired); - model.expired = 0; - - let mut routed = 0; - match tables { - Tables::Routed if model.waiting > 0 => { - routed = model.waiting; - model.waiting = 0; - model.discovering = false; - } - // The route request is still on the network: the discovery stays - Tables::Routed | Tables::Discovering => {} - Tables::Lapsed if !model.discovering || model.waiting == 0 => { - model.discovering = false; - } - Tables::Lapsed if model.restarts_left > 0 => { - model.restarts_left -= 1; - expected.restarts_discovery = true; - } - Tables::Lapsed => { - expected - .settled - .extend(verdicts(model.waiting, || HELD_TIMED_OUT)); - expected.fails_discovery = true; - model.waiting = 0; - model.discovering = false; - } - } - if model.waiting == 0 { - held.clear(); - } - - assert_eq!(fixture.pass(tables), expected, "{steps:?}"); - - for _ in 0..routed { - assert_eq!(fixture.transmit_next(), [(HELD_UNICAST, Ok(()))]); - } - } - } - - assert_eq!(fixture.discovering(), model.discovering, "{steps:?}"); - } -} - -// Every sequence of six operations, with and without a discovery restart. The model -// states when a discovery is tracked: from the first held unicast until a route takes -// the last one, the discovery lapses with nothing waiting, or its unicasts fail. -#[test] -fn test_held_unicasts_match_the_model_over_every_short_sequence() { - const LENGTH: u32 = 6; - - for restarts in [0, 1] { - for mut sequence in 0..STEPS.len().pow(LENGTH) { - let mut steps = Vec::new(); - for _ in 0..LENGTH { - steps.push(STEPS[sequence % STEPS.len()]); - sequence /= STEPS.len(); - } - - check_held_unicasts(&steps, restarts); - } - } -} diff --git a/crates/ziggurat-zigbee/src/sends/unrouted.rs b/crates/ziggurat-zigbee/src/sends/unrouted.rs index 519c336..5052477 100644 --- a/crates/ziggurat-zigbee/src/sends/unrouted.rs +++ b/crates/ziggurat-zigbee/src/sends/unrouted.rs @@ -1,9 +1,7 @@ -//! Unicasts with no route yet, held until their destination's route discovery ends. +//! Unicasts with no route yet, held until one is known or their deadline passes. //! -//! Discovery runs once per destination and the whole bucket moves on together, so ten -//! frames to one device ride a single discovery. The discovery itself lives in the -//! routing table; this table counts the restarts it has left and remembers the -//! broadcast of its route request, to stop it if the discovery is abandoned. +//! This table knows nothing of route discovery. The routing table decides when one is +//! started, restarted or given up on; a held send only waits for a next hop. use alloc::vec::Vec; use core::marker::PhantomData; @@ -38,21 +36,9 @@ pub(super) struct Record { pub(super) deadline: Instant, } -/// What this table tracks of a destination's route discovery. -#[derive(Debug)] -struct Discovery { - /// Restarts left. The first discovery is the driver's, started at admission; these - /// are the ones after it. - restarts_left: u8, - /// The broadcast of the latest route request. - request: Option, -} - #[derive(Debug)] pub(super) struct UnroutedUnicasts { records: FlatMap>, - /// One per destination with unrouted sends. - discoveries: FlatMap, _error: PhantomData E>, } @@ -60,7 +46,6 @@ impl Default for UnroutedUnicasts { fn default() -> Self { Self { records: FlatMap::new(), - discoveries: FlatMap::new(), _error: PhantomData, } } @@ -71,12 +56,14 @@ impl> UnroutedUnicasts { self.records.contains_key(&id) } - pub(super) fn has_unrouted(&self, destination: Nwk) -> bool { - self.discoveries.contains_key(&destination) + /// Whether any send waits for a route to `destination`. + pub(super) fn has_sends_to(&self, destination: Nwk) -> bool { + self.records + .values() + .any(|send| send.destination == destination) } - /// Hold a send. It fails if still unrouted within the handoff timeout. The first - /// unrouted send to a destination fixes how often its discovery may be restarted. + /// Hold a send. It fails if still unrouted within the handoff timeout. pub(super) fn enqueue( &mut self, id: SendId, @@ -84,7 +71,6 @@ impl> UnroutedUnicasts { now: Instant, tunables: &Tunables, ) { - let destination = send.destination; self.records.insert( id, Record { @@ -92,72 +78,26 @@ impl> UnroutedUnicasts { priority: send.priority, waiter: send.waiter, confirm: send.confirm, - destination, + destination: send.destination, deadline: now + tunables.handoff_timeout(), }, ); - - if !self.discoveries.contains_key(&destination) { - self.discoveries.insert( - destination, - Discovery { - // The tunable counts discoveries; the first one is not a restart - restarts_left: tunables - .pending_route_discovery_attempts() - .saturating_sub(1), - request: None, - }, - ); - } - } - - pub(super) fn destinations(&self) -> Vec { - self.discoveries.keys().copied().collect() } - /// The driver broadcast a route request for `destination`'s discovery. Returns the - /// broadcast of the request it replaces, for the caller to stop. - pub(super) fn route_request_sent( - &mut self, - destination: Nwk, - request: SendId, - ) -> Option { - self.discoveries - .get_mut(&destination) - .unwrap() - .request - .replace(request) - } - - /// The id of every send to `destination`, oldest first. - pub(super) fn sends_to(&self, destination: Nwk) -> Vec { - self.records - .iter() - .filter(|(_, send)| send.destination == destination) - .map(|(id, _)| *id) - .collect() + /// The id of every held send, oldest first. + pub(super) fn ids(&self) -> Vec { + self.records.keys().copied().collect() } pub(super) fn frame_mut(&mut self, id: SendId) -> &mut F { &mut self.records.get_mut(&id).unwrap().frame } - /// Take a held send: it has a route, or it failed. + /// Take a held send: it has a route. pub(super) fn take(&mut self, id: SendId) -> Record { self.records.remove(&id).unwrap() } - /// Whether no send waits on `destination`'s discovery any more: its sends were - /// routed, cancelled or timed out. - pub(super) fn is_idle(&self, destination: Nwk) -> bool { - self.sends_to(destination).is_empty() - } - - /// Stop tracking `destination`'s discovery. - pub(super) fn end_discovery(&mut self, destination: Nwk) { - self.discoveries.remove(&destination); - } - fn settled(send: Record, failure: SendFailure) -> Action { Action::Settled { waiter: send.waiter, @@ -167,37 +107,22 @@ impl> UnroutedUnicasts { } } - /// Consume a restart of `destination`'s discovery, if one is left. - pub(super) fn retry_discovery(&mut self, destination: Nwk) -> bool { - let discovery = self.discoveries.get_mut(&destination).unwrap(); - if discovery.restarts_left == 0 { - return false; - } - - discovery.restarts_left -= 1; - true - } - - /// Fail every send to `destination`: its discovery is over without a route. Returns - /// the broadcast of its route request, for the caller to stop. - pub(super) fn fail(&mut self, destination: Nwk) -> (Option, Actions) { - let request = self - .discoveries - .remove(&destination) - .and_then(|discovery| discovery.request); - - let actions = self - .sends_to(destination) - .into_iter() - .map(|id| Self::settled(self.take(id), SendFailure::RouteDiscoveryTimeout)) + /// Fail every send to `destination`: no route to it will be found. + pub(super) fn fail(&mut self, destination: Nwk) -> Actions { + let ids: Vec = self + .records + .iter() + .filter(|(_, send)| send.destination == destination) + .map(|(id, _)| *id) .collect(); - (request, actions) + ids.into_iter() + .map(|id| Self::settled(self.take(id), SendFailure::RouteDiscoveryTimeout)) + .collect() } /// Drop every record and return its waiter: the stack is shutting down. pub(super) fn abandon(&mut self) -> Vec { - self.discoveries.clear(); let mut waiters = Vec::new(); while let Some((_, send)) = self.records.pop_first() { waiters.push(send.waiter); @@ -205,9 +130,6 @@ impl> UnroutedUnicasts { waiters } - /// The destination stays awaited, so a new send to it does not start a second - /// discovery: the route request is on the network whether or not anything still - /// waits on it. The discovery ends once it lapses. pub(super) fn cancel(&mut self, id: SendId) -> Actions { let send = self.records.remove(&id).unwrap(); let mut actions = Actions::new(); From 103c586f9625164be59158d088c4eb66af8574e5 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 21:08:51 +0000 Subject: [PATCH 14/29] Keep an answered discovery tracked until a pass sees no frame waiting --- crates/ziggurat-zigbee/src/nwk/routing.rs | 68 +++++++++++++------ .../ziggurat-zigbee/src/nwk/routing/tests.rs | 65 ++++++++++++++---- 2 files changed, 99 insertions(+), 34 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index c7f6da4..7c3c819 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -193,6 +193,17 @@ pub struct RouteReplyOutcome { pub update: Option, } +/// A route discovery we started for our own frames. +#[derive(Debug)] +struct OwnDiscovery { + /// How many times it may still be started again. + restarts_left: u8, + /// A reply established a route to the destination. The discovery is over, but it + /// stays tracked until a timer pass has seen that no frame still waits: a frame + /// the route did not release must not be left without a discovery. + answered: bool, +} + /// What a timer pass decided about the route discoveries we started. #[derive(Debug, Default, PartialEq, Eq)] #[must_use = "restarted discoveries need their route request broadcast, and frames \ @@ -220,11 +231,10 @@ pub struct Routing { discovery_table: FlatMap<(Nwk, RouteRequestId), DiscoveryEntry>, route_record_table: FlatMap>, - /// The destinations we are discovering a route to for our own frames, each with - /// the number of times its discovery may still be restarted. An entry lasts from - /// the first frame that wants the route until a reply answers the discovery, or - /// until it lapses with nothing left to restart it for. - own_discoveries: FlatMap, + /// The route discoveries we started for our own frames, by destination. One lasts + /// from the first frame that wants the route until a timer pass finds it answered + /// or lapsed with no frame left waiting, or gives up on it. + own_discoveries: FlatMap, /// Implied from the spec: "notice that this 8-bit identifier is distinct from the /// 16-bit Routing Sequence Number. The former is used to discern route requests @@ -290,15 +300,21 @@ impl Routing { return None; } - // `attempts` counts discoveries; the first one is not a restart - self.own_discoveries - .insert(destination, attempts.saturating_sub(1)); + self.own_discoveries.insert( + destination, + OwnDiscovery { + // `attempts` counts discoveries; the first one is not a restart + restarts_left: attempts.saturating_sub(1), + answered: false, + }, + ); Some(self.begin_discovery(own_address, destination, now, route_discovery_time)) } - /// Decide what becomes of each discovery we started that has lapsed unanswered. - /// `wanted` says whether a frame still waits for a route to a destination. + /// Decide what becomes of each discovery we started that is over: answered, or + /// lapsed unanswered. `wanted` says whether a frame still waits for a route to a + /// destination, after the frames the routing table can now route have left. pub fn tick_discoveries( &mut self, own_address: Nwk, @@ -308,24 +324,30 @@ impl Routing { ) -> DiscoveryTick { let mut tick = DiscoveryTick::default(); - let lapsed: Vec = self + let over: Vec = self .own_discoveries - .keys() - .copied() - .filter(|destination| self.discovery_deadline(*destination, now).is_none()) + .iter() + .filter(|(destination, discovery)| { + discovery.answered || self.discovery_deadline(**destination, now).is_none() + }) + .map(|(destination, _)| *destination) .collect(); - for destination in lapsed { - // Nothing waits for the route any more: the discovery is over, not failed + for destination in over { + // Nothing waits for the route any more: the discovery did its job, or + // nobody needs it. It is not failed if !wanted(destination) { self.own_discoveries.remove(&destination); continue; } - let restarts_left = self.own_discoveries.get_mut(&destination).unwrap(); + // A frame still waits. An answered discovery whose route was lost again is + // started over like a lapsed one, so the frame is never left without one + let discovery = self.own_discoveries.get_mut(&destination).unwrap(); - if *restarts_left > 0 { - *restarts_left -= 1; + if discovery.restarts_left > 0 { + discovery.restarts_left -= 1; + discovery.answered = false; let request_id = self.begin_discovery(own_address, destination, now, route_discovery_time); tick.restarted.push((destination, request_id)); @@ -722,9 +744,11 @@ impl Routing { routing_entry.path_cost = updated_path_cost; discovery_entry.residual_cost = updated_path_cost; - // The discovery is answered. Better replies are still taken until its - // entry expires, but a frame that needs the route again starts a new one - self.own_discoveries.remove(&responder); + // A route to the responder exists, whichever of our requests this reply + // answers. The next timer pass ends the discovery once no frame waits + if let Some(discovery) = self.own_discoveries.get_mut(&responder) { + discovery.answered = true; + } let update = (previous_next_hop != Some(sender)).then_some(RouteUpdate { destination: responder, diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 102e018..34305f1 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -300,23 +300,49 @@ fn test_live_discovery_is_left_alone() { assert!(!want(&mut routing, live, 1)); } -// The reply answers the discovery. If the route is lost again, the next frame starts a -// new one instead of waiting on a request that was already answered. -#[test] -fn test_route_reply_ends_the_discovery() { - let mut routing = Routing::default(); +/// Start a discovery toward `DEVICE` and have the device answer it. +fn answered_discovery(routing: &mut Routing, attempts: u8) { let request_id = routing - .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, 1) + .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, attempts) .unwrap(); let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, 3); assert_eq!(reply.disposition, RouteReplyDisposition::Established); assert_eq!(routing.next_hop(DEVICE), Some(NEIGHBOR)); +} + +// The reply answers the discovery, and the next pass ends it once the held frames have +// left. If the route is lost later, the next frame starts a new discovery instead of +// waiting on a request that was already answered. +#[test] +fn test_answered_discovery_ends_at_the_next_pass() { + let mut routing = Routing::default(); + answered_discovery(&mut routing, 1); + + assert_eq!(tick(&mut routing, START, false), DiscoveryTick::default()); routing.invalidate_via(NEIGHBOR); assert!(want(&mut routing, START, 1)); } +// The route can be lost between the reply and the pass that would have released the +// held frame. The frame must not be left waiting with no discovery behind it. +#[test] +fn test_answered_discovery_is_restarted_for_a_frame_its_route_did_not_release() { + let mut routing = Routing::default(); + answered_discovery(&mut routing, 2); + routing.invalidate_via(NEIGHBOR); + + assert_eq!(tick(&mut routing, START, true).restarted.len(), 1); + + // With no restart left the frame fails instead + let mut routing = Routing::default(); + answered_discovery(&mut routing, 1); + routing.invalidate_via(NEIGHBOR); + + assert_eq!(tick(&mut routing, START, true).unreachable, vec![DEVICE]); +} + #[test] fn test_forget_ends_our_discovery_toward_the_device() { let mut routing = Routing::default(); @@ -359,10 +385,12 @@ struct Model { /// A frame waits for the route. wanted: bool, /// A discovery we started is tracked: from the first frame that wants the route - /// until a reply answers it, or it lapses with nothing left to restart it for. + /// until a pass finds it over with no frame waiting, or gives up on it. tracked: bool, /// The tracked discovery has not lapsed. live: bool, + /// The device answered the tracked discovery. + answered: bool, restarts_left: u8, } @@ -385,6 +413,7 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { request_id = Some(started); model.tracked = true; model.live = true; + model.answered = false; model.restarts_left = attempts - 1; } model.wanted = true; @@ -395,16 +424,22 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { model.live = false; } Step::Tick => { + // The send tables go first: a held frame leaves once it has a next hop + if routing.next_hop(DEVICE).is_some() { + model.wanted = false; + } + let tick = tick(&mut routing, now, model.wanted); let mut restarted = false; let mut unreachable = false; - if model.tracked && !model.live { + if model.tracked && (model.answered || !model.live) { if !model.wanted { model.tracked = false; } else if model.restarts_left > 0 { model.restarts_left -= 1; model.live = true; + model.answered = false; restarted = true; } else { model.tracked = false; @@ -430,7 +465,7 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { request_id = Some(restart.1); } } - Step::Reply if model.tracked && model.live => { + Step::Reply if model.tracked && model.live && !model.answered => { let reply = routing.accept_route_reply(OWN, OWN, request_id.unwrap(), DEVICE, NEIGHBOR, 3); assert_eq!( @@ -439,9 +474,8 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { "{steps:?}" ); - model.tracked = false; - // The held frames have a next hop and leave - model.wanted = false; + // The held frames leave at the next pass, if the route is still there + model.answered = true; } Step::Reply => {} Step::Break => routing.invalidate_via(NEIGHBOR), @@ -452,6 +486,13 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { model.tracked && model.live, "{steps:?}" ); + + // A frame that waits always has something that will release or fail it: a + // route, or a discovery a later pass acts on + assert!( + !model.wanted || model.tracked || routing.next_hop(DEVICE).is_some(), + "a frame waits with no route and no discovery: {steps:?}" + ); } } From adc3174ca38ecf979e7699622e46cef97429bb28 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 23:09:27 +0000 Subject: [PATCH 15/29] Derive our route discoveries from the routing tables and keep only an attempt count --- .../ziggurat-driver/src/zigbee_stack/nwk.rs | 4 +- .../ziggurat-driver/src/zigbee_stack/sends.rs | 9 +- crates/ziggurat-zigbee/src/nwk/routing.rs | 156 +++++++-------- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 182 +++++++++++++----- 4 files changed, 216 insertions(+), 135 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs index 528a7e9..c07c70e 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs @@ -438,8 +438,8 @@ impl ZigbeeStack { &self.tunables, ); - // The routing table starts a discovery unless one it started for this - // destination is still tracked + // The routing table starts a discovery unless the send can wait on + // one: a discovery of ours still underway, or one the timer pass starts let route_request_identifier = core.nib.routing.want_route( self.state.network_address, destination, diff --git a/crates/ziggurat-driver/src/zigbee_stack/sends.rs b/crates/ziggurat-driver/src/zigbee_stack/sends.rs index 7dd845b..57fede6 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/sends.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/sends.rs @@ -299,8 +299,8 @@ impl ZigbeeStack { /// One timer pass: settle the broadcasts whose passive-ack quorum was heard, run /// the send tables' pass against the routing and neighbor tables, then let the - /// routing table restart or give up on the discoveries that lapsed. Returns when - /// the next pass is due. + /// routing table start a discovery for, or give up on, each destination a send + /// still waits on with none underway. Returns when the next pass is due. fn drive_sends(&self) -> Option { let now = self.core_now(); @@ -319,7 +319,6 @@ impl ZigbeeStack { self.resolve_unrouted_unicast(&core.nib, frame, now) })); - // A discovery is only restarted for a destination a held send still waits on let discoveries = core.nib.routing.tick_discoveries( self.state.network_address, now, @@ -341,7 +340,9 @@ impl ZigbeeStack { let next_wake = [ core.sends.next_wake(), - core.nib.routing.next_discovery_lapse(now), + core.nib + .routing + .next_discovery_lapse(self.state.network_address, now), ] .into_iter() .flatten() diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 7c3c819..49f8c94 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -19,6 +19,8 @@ const UNKNOWN_NEXT_HOP: Nwk = Nwk(0xFFFF); pub enum Status { Active = 0, DiscoveryUnderway = 1, + /// Never set: the spec defines the value but an unanswered discovery deletes its + /// entry (3.6.4.5.1.7). DiscoveryFailed = 2, Inactive = 3, } @@ -193,24 +195,13 @@ pub struct RouteReplyOutcome { pub update: Option, } -/// A route discovery we started for our own frames. -#[derive(Debug)] -struct OwnDiscovery { - /// How many times it may still be started again. - restarts_left: u8, - /// A reply established a route to the destination. The discovery is over, but it - /// stays tracked until a timer pass has seen that no frame still waits: a frame - /// the route did not release must not be left without a discovery. - answered: bool, -} - /// What a timer pass decided about the route discoveries we started. #[derive(Debug, Default, PartialEq, Eq)] #[must_use = "restarted discoveries need their route request broadcast, and frames \ held for an unreachable destination must fail"] pub struct DiscoveryTick { - /// Lapsed discoveries started again: the destination, and the identifier to put in - /// the new route request. + /// Discoveries started for frames that wait with none underway: the destination, + /// and the identifier to put in the new route request. pub restarted: Vec<(Nwk, RouteRequestId)>, /// Destinations given up on, with no restart left. Frames held for a route to them /// fail. @@ -231,10 +222,11 @@ pub struct Routing { discovery_table: FlatMap<(Nwk, RouteRequestId), DiscoveryEntry>, route_record_table: FlatMap>, - /// The route discoveries we started for our own frames, by destination. One lasts - /// from the first frame that wants the route until a timer pass finds it answered - /// or lapsed with no frame left waiting, or gives up on it. - own_discoveries: FlatMap, + /// The destinations our frames wait for a route to, with how many more discoveries + /// each may get. An entry lasts from the first frame that wants the route until a + /// timer pass finds no frame waiting, or gives up. The discoveries themselves are + /// only in the two spec tables above. + discoveries_left: FlatMap, /// Implied from the spec: "notice that this 8-bit identifier is distinct from the /// 16-bit Routing Sequence Number. The former is used to discern route requests @@ -284,10 +276,40 @@ impl Routing { }) } + /// When the last live discovery we started toward a destination lapses. + fn own_discovery_lapse( + &self, + own_address: Nwk, + destination: Nwk, + now: Instant, + ) -> Option { + self.discovery_table + .values() + .filter(|entry| { + entry.source_address == own_address + && entry.destination_address == destination + && entry.expiration_time > now + }) + .map(|entry| entry.expiration_time) + .max() + } + + /// Spec 3.6.4.3: a frame waits on a discovery that is underway only if we started + /// it. The status alone does not say so: relaying another router's route request + /// sets it too. A route that was found and lost again ends the wait, because the + /// status is no longer `DiscoveryUnderway`. + fn own_discovery_underway(&self, own_address: Nwk, destination: Nwk, now: Instant) -> bool { + self.route_status(destination) == Some(Status::DiscoveryUnderway) + && self + .own_discovery_lapse(own_address, destination, now) + .is_some() + } + /// One of our frames needs a route to `destination`. Returns the identifier to put - /// in the route request to broadcast, or `None` when a discovery we started is - /// still tracked. Its route request is on the network whatever became of the - /// frames that wanted it, so the new frame waits on that one. + /// in the route request to broadcast, or `None` when the frame waits on a discovery + /// that exists or that the next timer pass starts. A discovery we started that is + /// still underway counts as the first of the frame's `attempts`: its route request + /// is on the network whatever became of the frames that wanted it. pub fn want_route( &mut self, own_address: Nwk, @@ -296,25 +318,23 @@ impl Routing { route_discovery_time: Duration, attempts: u8, ) -> Option { - if self.own_discoveries.contains_key(&destination) { + if self.discoveries_left.contains_key(&destination) { return None; } - self.own_discoveries.insert( - destination, - OwnDiscovery { - // `attempts` counts discoveries; the first one is not a restart - restarts_left: attempts.saturating_sub(1), - answered: false, - }, - ); + self.discoveries_left + .insert(destination, attempts.saturating_sub(1)); + + if self.own_discovery_underway(own_address, destination, now) { + return None; + } Some(self.begin_discovery(own_address, destination, now, route_discovery_time)) } - /// Decide what becomes of each discovery we started that is over: answered, or - /// lapsed unanswered. `wanted` says whether a frame still waits for a route to a - /// destination, after the frames the routing table can now route have left. + /// Decide what becomes of each destination our frames wait for a route to. + /// `wanted` says whether a frame still waits, after the frames the routing table + /// can now route have left. pub fn tick_discoveries( &mut self, own_address: Nwk, @@ -324,36 +344,31 @@ impl Routing { ) -> DiscoveryTick { let mut tick = DiscoveryTick::default(); - let over: Vec = self - .own_discoveries - .iter() - .filter(|(destination, discovery)| { - discovery.answered || self.discovery_deadline(**destination, now).is_none() - }) - .map(|(destination, _)| *destination) - .collect(); + self.expire_discoveries(now); - for destination in over { - // Nothing waits for the route any more: the discovery did its job, or - // nobody needs it. It is not failed + let destinations: Vec = self.discoveries_left.keys().copied().collect(); + + for destination in destinations { if !wanted(destination) { - self.own_discoveries.remove(&destination); + self.discoveries_left.remove(&destination); + continue; + } + + if self.own_discovery_underway(own_address, destination, now) { continue; } - // A frame still waits. An answered discovery whose route was lost again is - // started over like a lapsed one, so the frame is never left without one - let discovery = self.own_discoveries.get_mut(&destination).unwrap(); + // A frame waits and no discovery of ours is underway: it lapsed, or its + // route was lost before the frame could use it + let discoveries_left = self.discoveries_left.get_mut(&destination).unwrap(); - if discovery.restarts_left > 0 { - discovery.restarts_left -= 1; - discovery.answered = false; + if *discoveries_left > 0 { + *discoveries_left -= 1; let request_id = self.begin_discovery(own_address, destination, now, route_discovery_time); tick.restarted.push((destination, request_id)); } else { - self.own_discoveries.remove(&destination); - self.mark_discovery_failed(destination); + self.discoveries_left.remove(&destination); tick.unreachable.push(destination); } } @@ -361,23 +376,15 @@ impl Routing { tick } - /// When the next discovery we started lapses, or `None` when none is live. - pub fn next_discovery_lapse(&self, now: Instant) -> Option { - self.own_discoveries + /// When the next discovery a frame of ours waits on lapses, or `None` when none + /// is live. + pub fn next_discovery_lapse(&self, own_address: Nwk, now: Instant) -> Option { + self.discoveries_left .keys() - .filter_map(|destination| self.discovery_deadline(*destination, now)) + .filter_map(|destination| self.own_discovery_lapse(own_address, *destination, now)) .min() } - /// A discovery ended without a route. A route found some other way stays. - fn mark_discovery_failed(&mut self, destination: Nwk) { - if let Some(entry) = self.route_table.get_mut(&destination) - && entry.status != Status::Active - { - entry.status = Status::DiscoveryFailed; - } - } - /// A frame was forwarded along the route: bump its usage counters. pub fn record_usage(&mut self, destination: Nwk) { if let Some(entry) = self.route_table.get_mut(&destination) { @@ -422,7 +429,7 @@ impl Routing { // unknown responder. That router gets no route through us for that discovery. self.route_table.remove(&nwk); self.route_record_table.remove(&nwk); - self.own_discoveries.remove(&nwk); + self.discoveries_left.remove(&nwk); // Only our own discoveries toward it are dropped. Discoveries we relay stay // until they expire: they are the only duplicate check for late copies of @@ -583,17 +590,6 @@ impl Routing { request_id } - /// The expiration time of the live discovery toward a destination, if any. - pub fn discovery_deadline(&self, destination: Nwk, now: Instant) -> Option { - self.discovery_table.values().find_map(|entry| { - if entry.expiration_time > now && entry.destination_address == destination { - Some(entry.expiration_time) - } else { - None - } - }) - } - /// Deduplicate a received route request and track the best path back to its /// originator. Returns false for duplicates with a strictly worse forward cost /// (our own requests, seeded at cost 0, still never echo back); an equal-cost @@ -744,12 +740,6 @@ impl Routing { routing_entry.path_cost = updated_path_cost; discovery_entry.residual_cost = updated_path_cost; - // A route to the responder exists, whichever of our requests this reply - // answers. The next timer pass ends the discovery once no frame waits - if let Some(discovery) = self.own_discoveries.get_mut(&responder) { - discovery.answered = true; - } - let update = (previous_next_hop != Some(sender)).then_some(RouteUpdate { destination: responder, next_hop: sender, diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 34305f1..8ed73c1 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -50,7 +50,7 @@ fn test_forget_drops_our_discovery_toward_the_device() { let mut routing = Routing::default(); routing.begin_discovery(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME); assert_eq!( - routing.discovery_deadline(DEVICE, START), + routing.own_discovery_lapse(OWN, DEVICE, START), Some(START + ROUTE_DISCOVERY_TIME) ); assert_eq!( @@ -60,7 +60,7 @@ fn test_forget_drops_our_discovery_toward_the_device() { routing.forget(OWN, DEVICE); - assert_eq!(routing.discovery_deadline(DEVICE, START), None); + assert_eq!(routing.own_discovery_lapse(OWN, DEVICE, START), None); assert_eq!(routing.route_status(DEVICE), None); } @@ -87,7 +87,7 @@ fn test_forget_leaves_other_destinations_alone() { assert_eq!(routing.next_hop(OTHER_DEVICE), Some(NEIGHBOR)); assert_eq!( - routing.discovery_deadline(ROUTER, START), + routing.own_discovery_lapse(OWN, ROUTER, START), Some(START + ROUTE_DISCOVERY_TIME) ); } @@ -221,8 +221,6 @@ fn tick(routing: &mut Routing, now: Instant, wanted: bool) -> DiscoveryTick { routing.tick_discoveries(OWN, now, ROUTE_DISCOVERY_TIME, |_| wanted) } -// The route request is on the network whatever became of the frame that wanted it, so -// a second frame waits on the same discovery. #[test] fn test_wanting_a_route_starts_one_discovery() { let mut routing = Routing::default(); @@ -235,11 +233,12 @@ fn test_wanting_a_route_starts_one_discovery() { Some(Status::DiscoveryUnderway) ); assert_eq!( - routing.next_discovery_lapse(START), + routing.next_discovery_lapse(OWN, START), Some(START + ROUTE_DISCOVERY_TIME) ); } +// Spec 3.6.4.5.1.7: an unanswered discovery deletes its routing entry. #[test] fn test_lapsed_discovery_without_a_restart_makes_the_destination_unreachable() { let mut routing = Routing::default(); @@ -248,7 +247,7 @@ fn test_lapsed_discovery_without_a_restart_makes_the_destination_unreachable() { let lapsed = START + ROUTE_DISCOVERY_TIME; assert_eq!(tick(&mut routing, lapsed, true).unreachable, vec![DEVICE]); - assert_eq!(routing.route_status(DEVICE), Some(Status::DiscoveryFailed)); + assert_eq!(routing.route_status(DEVICE), None); // The next frame starts over assert!(want(&mut routing, lapsed, 1)); } @@ -264,7 +263,7 @@ fn test_lapsed_discovery_is_restarted_while_a_frame_waits() { assert_eq!(restarted.len(), 1); assert_eq!(restarted[0].0, DEVICE); assert_eq!( - routing.next_discovery_lapse(lapsed), + routing.next_discovery_lapse(OWN, lapsed), Some(lapsed + ROUTE_DISCOVERY_TIME) ); @@ -277,7 +276,7 @@ fn test_lapsed_discovery_is_restarted_while_a_frame_waits() { } // A discovery nothing waits on is over, not failed: it is neither restarted nor -// reported, and a route found for it stays. +// reported. #[test] fn test_lapsed_discovery_nothing_waits_on_ends_quietly() { let mut routing = Routing::default(); @@ -297,7 +296,58 @@ fn test_live_discovery_is_left_alone() { let live = START + Duration::from_secs(1); assert_eq!(tick(&mut routing, live, true), DiscoveryTick::default()); assert_eq!(tick(&mut routing, live, false), DiscoveryTick::default()); +} + +// The route request is on the network whatever became of the frames that wanted it. +// A later frame waits on it, and it is that frame's one attempt. +#[test] +fn test_new_frame_waits_on_a_discovery_whose_frames_are_gone() { + let mut routing = Routing::default(); + assert!(want(&mut routing, START, 1)); + + let live = START + Duration::from_secs(1); + assert_eq!(tick(&mut routing, live, false), DiscoveryTick::default()); assert!(!want(&mut routing, live, 1)); + + let lapsed = START + ROUTE_DISCOVERY_TIME; + assert_eq!(tick(&mut routing, lapsed, true).unreachable, vec![DEVICE]); +} + +// Spec 3.6.4.3: relaying a route request also marks the destination's entry as +// `DiscoveryUnderway`. A frame of ours does not wait on another router's discovery. +#[test] +fn test_relayed_discovery_toward_the_destination_is_not_ours() { + let mut routing = Routing::default(); + let request = RouteRequest { + originator: ROUTER, + destination: DEVICE, + path_cost: 3, + }; + assert!(receive(&mut routing, request, START)); + routing.note_relayed_discovery(DEVICE); + + assert!(want(&mut routing, START, 1)); +} + +#[test] +fn test_relayed_discovery_does_not_keep_ours_alive() { + let mut routing = Routing::default(); + assert!(want(&mut routing, START, 1)); + + let request = RouteRequest { + originator: ROUTER, + destination: DEVICE, + path_cost: 3, + }; + assert!(receive( + &mut routing, + request, + START + Duration::from_secs(5) + )); + + let lapsed = START + ROUTE_DISCOVERY_TIME; + assert_eq!(routing.next_discovery_lapse(OWN, lapsed), None); + assert_eq!(tick(&mut routing, lapsed, true).unreachable, vec![DEVICE]); } /// Start a discovery toward `DEVICE` and have the device answer it. @@ -311,11 +361,11 @@ fn answered_discovery(routing: &mut Routing, attempts: u8) { assert_eq!(routing.next_hop(DEVICE), Some(NEIGHBOR)); } -// The reply answers the discovery, and the next pass ends it once the held frames have -// left. If the route is lost later, the next frame starts a new discovery instead of -// waiting on a request that was already answered. +// The reply makes the route active, so no discovery is underway any more. If the route +// is lost later, the next frame starts a new discovery instead of waiting on a request +// that was already answered. #[test] -fn test_answered_discovery_ends_at_the_next_pass() { +fn test_frame_after_an_answered_discovery_starts_a_new_one() { let mut routing = Routing::default(); answered_discovery(&mut routing, 1); @@ -350,7 +400,7 @@ fn test_forget_ends_our_discovery_toward_the_device() { routing.forget(OWN, DEVICE); - assert_eq!(routing.next_discovery_lapse(START), None); + assert_eq!(routing.next_discovery_lapse(OWN, START), None); assert!(want(&mut routing, START, 1)); } @@ -364,7 +414,7 @@ enum Step { /// Time passes until every discovery has lapsed. Lapse, Tick, - /// The device answers the live discovery. + /// The device answers the latest discovery. Reply, /// The route to the device is lost. Break, @@ -379,19 +429,41 @@ const STEPS: [Step; 6] = [ Step::Break, ]; -/// What `Routing` should hold of our discovery toward `DEVICE`. +/// What `Routing` should hold about `DEVICE`. #[derive(Debug, Default)] struct Model { /// A frame waits for the route. wanted: bool, - /// A discovery we started is tracked: from the first frame that wants the route - /// until a pass finds it over with no frame waiting, or gives up on it. - tracked: bool, - /// The tracked discovery has not lapsed. + /// How many more discoveries the waiting frames may get. Kept from the first frame + /// that wants the route until a pass finds no frame waiting, or gives up. + discoveries_left: Option, + /// The status of the routing entry, if there is one. + status: Option, + /// A discovery we started has not lapsed. live: bool, - /// The device answered the tracked discovery. - answered: bool, - restarts_left: u8, + /// The device answered the latest discovery. + replied: bool, +} + +impl Model { + fn underway(&self) -> bool { + self.live && self.status == Some(Status::DiscoveryUnderway) + } + + /// Lapsed discoveries are only cleaned up by a pass or by the next discovery. An + /// unanswered one takes its routing entry with it. + fn expire(&mut self) { + if !self.live && self.status == Some(Status::DiscoveryUnderway) { + self.status = None; + } + } + + fn begin_discovery(&mut self) { + self.expire(); + self.status = Some(Status::DiscoveryUnderway); + self.live = true; + self.replied = false; + } } /// Run `steps` against `Routing` and the model, comparing them after each one. @@ -407,14 +479,18 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { Step::Hold if routing.next_hop(DEVICE).is_some() => {} Step::Hold => { let started = routing.want_route(OWN, DEVICE, now, ROUTE_DISCOVERY_TIME, attempts); - assert_eq!(started.is_some(), !model.tracked, "{steps:?}"); + // Only the first frame to wait can start a discovery, and a discovery + // of ours that is still underway serves it + let starts = model.discoveries_left.is_none() && !model.underway(); + assert_eq!(started.is_some(), starts, "{steps:?}"); + + if model.discoveries_left.is_none() { + model.discoveries_left = Some(attempts - 1); + } if let Some(started) = started { request_id = Some(started); - model.tracked = true; - model.live = true; - model.answered = false; - model.restarts_left = attempts - 1; + model.begin_discovery(); } model.wanted = true; } @@ -433,20 +509,23 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { let mut restarted = false; let mut unreachable = false; - if model.tracked && (model.answered || !model.live) { - if !model.wanted { - model.tracked = false; - } else if model.restarts_left > 0 { - model.restarts_left -= 1; - model.live = true; - model.answered = false; - restarted = true; - } else { - model.tracked = false; + model.expire(); + + match model.discoveries_left { + None => {} + Some(_) if !model.wanted => model.discoveries_left = None, + Some(_) if model.underway() => {} + Some(0) => { + model.discoveries_left = None; // The driver fails the frames held for the destination model.wanted = false; unreachable = true; } + Some(left) => { + model.discoveries_left = Some(left - 1); + model.begin_discovery(); + restarted = true; + } } let destinations: Vec = tick.restarted.iter().map(|r| r.0).collect(); @@ -464,8 +543,11 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { if let Some(restart) = tick.restarted.first() { request_id = Some(restart.1); } + + // A pass leaves every waiting frame with a discovery underway + assert!(!model.wanted || model.underway(), "{steps:?}"); } - Step::Reply if model.tracked && model.live && !model.answered => { + Step::Reply if model.live && !model.replied => { let reply = routing.accept_route_reply(OWN, OWN, request_id.unwrap(), DEVICE, NEIGHBOR, 3); assert_eq!( @@ -475,23 +557,31 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { ); // The held frames leave at the next pass, if the route is still there - model.answered = true; + model.status = Some(Status::Active); + model.replied = true; } Step::Reply => {} - Step::Break => routing.invalidate_via(NEIGHBOR), + Step::Break => { + routing.invalidate_via(NEIGHBOR); + + if model.status == Some(Status::Active) { + model.status = Some(Status::Inactive); + } + } } + assert_eq!(routing.route_status(DEVICE), model.status, "{steps:?}"); assert_eq!( - routing.next_discovery_lapse(now).is_some(), - model.tracked && model.live, + routing.next_discovery_lapse(OWN, now).is_some(), + model.discoveries_left.is_some() && model.live, "{steps:?}" ); // A frame that waits always has something that will release or fail it: a - // route, or a discovery a later pass acts on + // route, or a count a later pass acts on assert!( - !model.wanted || model.tracked || routing.next_hop(DEVICE).is_some(), - "a frame waits with no route and no discovery: {steps:?}" + !model.wanted || model.discoveries_left.is_some() || routing.next_hop(DEVICE).is_some(), + "a frame waits with no route and nothing a pass acts on: {steps:?}" ); } } From 8be706bc800752617dc98e0964bbc4075c382c82 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Thu, 12 Nov 2026 23:09:36 +0000 Subject: [PATCH 16/29] Keep a routing entry whose discovery is underway when its route is removed --- crates/ziggurat-zigbee/src/nwk/routing.rs | 8 +++++++- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 49f8c94..8e7f53c 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -417,8 +417,14 @@ impl Routing { } } - /// Remove the route to a destination, returning whether an entry was removed. + /// Remove the route to a destination, returning whether an entry was removed. An + /// entry in `DiscoveryUnderway` holds no route and stays: without it the reply to + /// the discovery is dropped (spec 3.6.4.5.2). pub fn remove_route(&mut self, destination: Nwk) -> bool { + if self.route_status(destination) == Some(Status::DiscoveryUnderway) { + return false; + } + self.route_table.remove(&destination).is_some() } diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 8ed73c1..8f87d26 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -350,6 +350,24 @@ fn test_relayed_discovery_does_not_keep_ours_alive() { assert_eq!(tick(&mut routing, lapsed, true).unreachable, vec![DEVICE]); } +// A relay can report an earlier frame undeliverable after the next frame started a +// discovery. The report is about the old route. The discovery must still take its reply. +#[test] +fn test_removing_the_route_keeps_a_discovery_underway() { + let mut routing = Routing::default(); + routing.restore_route(DEVICE, NEIGHBOR, 3); + assert!(routing.remove_route(DEVICE)); + + let request_id = routing + .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, 1) + .unwrap(); + assert!(!routing.remove_route(DEVICE)); + + let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, 3); + assert_eq!(reply.disposition, RouteReplyDisposition::Established); + assert_eq!(routing.next_hop(DEVICE), Some(NEIGHBOR)); +} + /// Start a discovery toward `DEVICE` and have the device answer it. fn answered_discovery(routing: &mut Routing, attempts: u8) { let request_id = routing From 957de29b6164821883ac61c24144a1cede1bcb3e Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:09:18 +0000 Subject: [PATCH 17/29] Move tests to `tests.rs` --- crates/ziggurat-zigbee/src/nwk/routing.rs | 54 ------------------- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 37 +++++++++++++ 2 files changed, 37 insertions(+), 54 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 9e9e995..6a8a867 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -856,57 +856,3 @@ impl Routing { } } } - -#[cfg(test)] -mod test { - use super::*; - use alloc::vec; - - const DESTINATION: Nwk = Nwk(0x0391); - const RELAY: Nwk = Nwk(0xEF2F); - const MAX_SOURCE_ROUTE: u8 = 12; - - #[test] - fn test_forget_route_to_drops_a_direct_route_record() { - let mut routing = Routing::default(); - routing.store_route_record(DESTINATION, vec![]); - assert_eq!( - routing.route_to(DESTINATION, MAX_SOURCE_ROUTE), - Some(Route::NextHop(DESTINATION)) - ); - - assert!(routing.forget_route_to(DESTINATION)); - assert_eq!(routing.route_to(DESTINATION, MAX_SOURCE_ROUTE), None); - } - - #[test] - fn test_forget_route_to_drops_a_source_route_record() { - let mut routing = Routing::default(); - routing.store_route_record(DESTINATION, vec![RELAY]); - - assert!(routing.forget_route_to(DESTINATION)); - assert_eq!(routing.route_to(DESTINATION, MAX_SOURCE_ROUTE), None); - } - - #[test] - fn test_forget_route_to_drops_the_routing_entry() { - let mut routing = Routing::default(); - routing.restore_route(DESTINATION, RELAY, 1); - assert_eq!(routing.next_hop(DESTINATION), Some(RELAY)); - - assert!(!routing.forget_route_to(DESTINATION)); - assert_eq!(routing.next_hop(DESTINATION), None); - } - - #[test] - fn test_forget_route_to_keeps_other_destinations() { - let mut routing = Routing::default(); - routing.store_route_record(DESTINATION, vec![RELAY]); - - routing.forget_route_to(DESTINATION); - assert_eq!( - routing.route_to(RELAY, MAX_SOURCE_ROUTE), - Some(Route::NextHop(RELAY)) - ); - } -} diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 8f87d26..714c2c7 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -209,6 +209,42 @@ fn test_unchanged_route_record_reports_a_relay_route_it_restores() { ); } +// A route record wins over the routing table, so a reported failure must remove it +// too, whether it is direct or a source route. +#[test] +fn test_forget_route_to_drops_the_route_record_and_the_routing_entry() { + for relays in [vec![], vec![ROUTER]] { + let mut routing = Routing::default(); + routing.restore_route(DEVICE, NEIGHBOR, 3); + routing.store_route_record(DEVICE, relays); + + assert!(routing.forget_route_to(DEVICE)); + assert_eq!(routing.route_to(DEVICE, MAX_SOURCE_ROUTE), None); + } +} + +#[test] +fn test_forget_route_to_reports_only_a_removed_route_record() { + let mut routing = Routing::default(); + routing.restore_route(DEVICE, NEIGHBOR, 3); + + assert!(!routing.forget_route_to(DEVICE)); + assert_eq!(routing.next_hop(DEVICE), None); +} + +#[test] +fn test_forget_route_to_leaves_other_destinations_alone() { + let mut routing = Routing::default(); + routing.store_route_record(DEVICE, vec![ROUTER]); + + routing.forget_route_to(DEVICE); + + assert_eq!( + routing.route_to(ROUTER, MAX_SOURCE_ROUTE), + Some(Route::NextHop(ROUTER)) + ); +} + /// A frame of ours wants a route to `DEVICE`. Returns whether that starts a discovery. fn want(routing: &mut Routing, now: Instant, attempts: u8) -> bool { routing @@ -362,6 +398,7 @@ fn test_removing_the_route_keeps_a_discovery_underway() { .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, 1) .unwrap(); assert!(!routing.remove_route(DEVICE)); + assert!(!routing.forget_route_to(DEVICE)); let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, 3); assert_eq!(reply.disposition, RouteReplyDisposition::Established); From 9e963ac3b68c23e7a89360d59a3f62baa9bed8a4 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:09:34 +0000 Subject: [PATCH 18/29] Align with new naming --- crates/ziggurat-zigbee/src/nwk/routing.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 6a8a867..c96827a 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -488,16 +488,13 @@ impl Routing { self.route_record_table.remove(&destination).is_some() } - /// Spec 3.6.4.8.1: a router reported it could not deliver a frame we originated - /// toward `destination` (`LinkFailure`, `SourceRouteFailure` or a legacy - /// equivalent). Both the routing entry and the stored route record go: a record - /// that reached the failing router as a plain next hop, such as a direct record - /// (no relays) for a device that moved away, is as stale as a broken source route, - /// and only its removal lets the next transmission discover a fresh route. - /// Returns whether a route record was removed. + /// Spec 3.6.4.8.1: a router could not deliver a frame of ours toward `destination`. + /// The route record goes with the routing entry: it wins over the routing table, so + /// a stale one would keep the broken path in use. Returns whether a route record + /// was removed. pub fn forget_route_to(&mut self, destination: Nwk) -> bool { - self.route_table.remove(&destination); - self.route_record_table.remove(&destination).is_some() + self.remove_route(destination); + self.remove_route_record(destination) } /// Spec 3.6.4.3.2: relaying a source-routed frame proves the concentrator that From 9fdcd2be6b31f8923b2ddf11d61b1d3321257713 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:22:13 +0000 Subject: [PATCH 19/29] Do not wait on an answered discovery that a relayed route request marks underway again --- crates/ziggurat-zigbee/src/nwk/routing.rs | 13 ++++++---- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 24 +++++++++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index c96827a..2397b0f 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -296,13 +296,16 @@ impl Routing { /// Spec 3.6.4.3: a frame waits on a discovery that is underway only if we started /// it. The status alone does not say so: relaying another router's route request - /// sets it too. A route that was found and lost again ends the wait, because the - /// status is no longer `DiscoveryUnderway`. + /// sets it too, even after our own discovery was answered and its route lost. So + /// a discovery of ours must also be live and have no reply yet. fn own_discovery_underway(&self, own_address: Nwk, destination: Nwk, now: Instant) -> bool { self.route_status(destination) == Some(Status::DiscoveryUnderway) - && self - .own_discovery_lapse(own_address, destination, now) - .is_some() + && self.discovery_table.values().any(|entry| { + entry.source_address == own_address + && entry.destination_address == destination + && entry.expiration_time > now + && entry.residual_cost == u8::MAX + }) } /// One of our frames needs a route to `destination`. Returns the identifier to put diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 714c2c7..98b6981 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -430,6 +430,30 @@ fn test_frame_after_an_answered_discovery_starts_a_new_one() { assert!(want(&mut routing, START, 1)); } +// Relaying another router's route request marks the entry `DiscoveryUnderway` again. +// Our answered discovery is still in the table, but its request is spent. +#[test] +fn test_relayed_discovery_does_not_revive_our_answered_one() { + let mut routing = Routing::default(); + answered_discovery(&mut routing, 1); + assert_eq!(tick(&mut routing, START, false), DiscoveryTick::default()); + routing.invalidate_via(NEIGHBOR); + + let request = RouteRequest { + originator: ROUTER, + destination: DEVICE, + path_cost: 3, + }; + assert!(receive(&mut routing, request, START)); + routing.note_relayed_discovery(DEVICE); + assert_eq!( + routing.route_status(DEVICE), + Some(Status::DiscoveryUnderway) + ); + + assert!(want(&mut routing, START, 1)); +} + // The route can be lost between the reply and the pass that would have released the // held frame. The frame must not be left waiting with no discovery behind it. #[test] From 0c2e6da6da65789cbfa054d1606ed0576c7ff0ec Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:23:22 +0000 Subject: [PATCH 20/29] Add relayed route requests to the discovery model and check it against what a waiting frame needs --- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 87 ++++++++++++++----- 1 file changed, 67 insertions(+), 20 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 98b6981..e244001 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -497,15 +497,18 @@ enum Step { Reply, /// The route to the device is lost. Break, + /// We relay another router's route request toward the device. + Relay, } -const STEPS: [Step; 6] = [ +const STEPS: [Step; 7] = [ Step::Hold, Step::Release, Step::Lapse, Step::Tick, Step::Reply, Step::Break, + Step::Relay, ]; /// What `Routing` should hold about `DEVICE`. @@ -520,19 +523,27 @@ struct Model { status: Option, /// A discovery we started has not lapsed. live: bool, + /// A discovery we relay has not lapsed. + relayed_live: bool, /// The device answered the latest discovery. replied: bool, } impl Model { + /// A route request of ours is on the network and may still be answered. This is + /// what a waiting frame needs, whatever the routing entry says. + const fn request_pending(&self) -> bool { + self.live && !self.replied + } + fn underway(&self) -> bool { - self.live && self.status == Some(Status::DiscoveryUnderway) + self.request_pending() && self.status == Some(Status::DiscoveryUnderway) } - /// Lapsed discoveries are only cleaned up by a pass or by the next discovery. An - /// unanswered one takes its routing entry with it. + /// Lapsed discoveries are only cleaned up by a pass, a received route request or + /// the next discovery. The last one to go takes an unanswered routing entry with it. fn expire(&mut self) { - if !self.live && self.status == Some(Status::DiscoveryUnderway) { + if !self.live && !self.relayed_live && self.status == Some(Status::DiscoveryUnderway) { self.status = None; } } @@ -564,6 +575,14 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { let starts = model.discoveries_left.is_none() && !model.underway(); assert_eq!(started.is_some(), starts, "{steps:?}"); + // The first frame to wait is never left to a request that is spent + assert!( + started.is_some() + || model.discoveries_left.is_some() + || model.request_pending(), + "{steps:?}" + ); + if model.discoveries_left.is_none() { model.discoveries_left = Some(attempts - 1); } @@ -577,6 +596,7 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { Step::Lapse => { now = now + ROUTE_DISCOVERY_TIME; model.live = false; + model.relayed_live = false; } Step::Tick => { // The send tables go first: a held frame leaves once it has a next hop @@ -623,10 +643,15 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { request_id = Some(restart.1); } - // A pass leaves every waiting frame with a discovery underway - assert!(!model.wanted || model.underway(), "{steps:?}"); + // A pass leaves every waiting frame with a request that may still be + // answered, and with a wake for when it lapses + assert!(!model.wanted || model.request_pending(), "{steps:?}"); + assert!( + !model.wanted || routing.next_discovery_lapse(OWN, now).is_some(), + "{steps:?}" + ); } - Step::Reply if model.live && !model.replied => { + Step::Reply if model.request_pending() => { let reply = routing.accept_route_reply(OWN, OWN, request_id.unwrap(), DEVICE, NEIGHBOR, 3); assert_eq!( @@ -647,6 +672,21 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { model.status = Some(Status::Inactive); } } + Step::Relay => { + let request = RouteRequest { + originator: ROUTER, + destination: DEVICE, + path_cost: 3, + }; + assert!(receive(&mut routing, request, now), "{steps:?}"); + routing.note_relayed_discovery(DEVICE); + + model.expire(); + model.relayed_live = true; + if model.status != Some(Status::Active) { + model.status = Some(Status::DiscoveryUnderway); + } + } } assert_eq!(routing.route_status(DEVICE), model.status, "{steps:?}"); @@ -665,20 +705,27 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { } } -// Every sequence of seven events, with one and with two discovery attempts. -#[test] -fn test_own_discovery_matches_the_model_over_every_short_sequence() { +/// Check every sequence of seven events. +fn check_every_short_sequence(attempts: u8) { const LENGTH: u32 = 7; - for attempts in [1, 2] { - for mut sequence in 0..STEPS.len().pow(LENGTH) { - let mut steps = Vec::new(); - for _ in 0..LENGTH { - steps.push(STEPS[sequence % STEPS.len()]); - sequence /= STEPS.len(); - } - - check_own_discovery(&steps, attempts); + for mut sequence in 0..STEPS.len().pow(LENGTH) { + let mut steps = Vec::new(); + for _ in 0..LENGTH { + steps.push(STEPS[sequence % STEPS.len()]); + sequence /= STEPS.len(); } + + check_own_discovery(&steps, attempts); } } + +#[test] +fn test_own_discovery_matches_the_model_with_one_attempt() { + check_every_short_sequence(1); +} + +#[test] +fn test_own_discovery_matches_the_model_with_two_attempts() { + check_every_short_sequence(2); +} From 8375b7528d57e6dcc0f86bc1cf6da2d8d62c2710 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:23:31 +0000 Subject: [PATCH 21/29] Correct why held sends fail when a device leaves or joins again --- crates/ziggurat-driver/src/zigbee_stack/nwk.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs index c07c70e..934c508 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs @@ -51,7 +51,7 @@ impl ZigbeeStack { /// The device at `nwk` left or joined again, so what we know about the address /// belongs to its previous session. Held sends fail: they wait on a route request - /// the device never heard, and would block a new one until it expires. + /// the device never heard, and forgetting the address drops that discovery. pub(super) fn forget_nwk_state(&self, nwk: Nwk) { self.forget_nwk_state_in(self.core(), nwk); } From 1f7a552f65f02bee7dca9c9e981698aabec0462e Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:24:10 +0000 Subject: [PATCH 22/29] Release a send held by a forced route discovery once that discovery is over --- crates/ziggurat-driver/src/zigbee_stack/sends.rs | 11 ++++++++++- crates/ziggurat-zigbee/src/nwk/routing.rs | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/sends.rs b/crates/ziggurat-driver/src/zigbee_stack/sends.rs index 57fede6..fe977b7 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/sends.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/sends.rs @@ -189,12 +189,21 @@ impl ZigbeeStack { unreachable!("only NWK unicasts await a route") }; + // A forced discovery holds the frame only while it is underway. Once it is + // answered or has lapsed, the frame takes any route like every other frame + let mut policy = self.next_hop_policy(); + policy.force_route_discovery &= nib.routing.own_discovery_underway( + self.state.network_address, + nwk_frame.nwk_header.destination, + now, + ); + let resolved = next_hop::resolve( &nib.routing, &nib.neighbors, nwk_frame, &SendMode::Route(RouteDirective::StackDecides), - self.next_hop_policy(), + policy, now, ); diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 2397b0f..aac8f93 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -298,7 +298,7 @@ impl Routing { /// it. The status alone does not say so: relaying another router's route request /// sets it too, even after our own discovery was answered and its route lost. So /// a discovery of ours must also be live and have no reply yet. - fn own_discovery_underway(&self, own_address: Nwk, destination: Nwk, now: Instant) -> bool { + pub fn own_discovery_underway(&self, own_address: Nwk, destination: Nwk, now: Instant) -> bool { self.route_status(destination) == Some(Status::DiscoveryUnderway) && self.discovery_table.values().any(|entry| { entry.source_address == own_address From 1507ef49e5e14aec1a5f5afa836ee497ff3f8de6 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:24:12 +0000 Subject: [PATCH 23/29] Correct three comments the held send rework left behind --- crates/ziggurat-zigbee/src/nwk/routing.rs | 4 ++-- crates/ziggurat-zigbee/src/sends/broadcasts.rs | 2 +- crates/ziggurat-zigbee/src/sends/mod.rs | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index aac8f93..7dd6ac0 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -687,8 +687,8 @@ impl Routing { } } - /// Before relaying a route request: track that discovery toward the destination is - /// underway, so frames for it are held off until it completes. + /// Before relaying a route request: mark the destination's entry as + /// `DiscoveryUnderway`, so the reply we relay back finds it (spec 3.6.4.5.1.2). pub fn note_relayed_discovery(&mut self, destination: Nwk) { self.route_table .entry(destination) diff --git a/crates/ziggurat-zigbee/src/sends/broadcasts.rs b/crates/ziggurat-zigbee/src/sends/broadcasts.rs index c65a87c..7d4d33f 100644 --- a/crates/ziggurat-zigbee/src/sends/broadcasts.rs +++ b/crates/ziggurat-zigbee/src/sends/broadcasts.rs @@ -294,11 +294,11 @@ impl> Broadcasts { waiters } - /// `None` if a copy is in the radio right now, or the send is unknown. pub(super) fn contains(&self, id: SendId) -> bool { self.records.contains_key(&id) } + /// `None` if a copy is in the radio right now, or the send is unknown. pub(super) fn cancel(&mut self, id: SendId) -> Option> { let record = self.records.get(&id)?; if record.copy == CopyState::Transmitting { diff --git a/crates/ziggurat-zigbee/src/sends/mod.rs b/crates/ziggurat-zigbee/src/sends/mod.rs index 3c8630c..fab4888 100644 --- a/crates/ziggurat-zigbee/src/sends/mod.rs +++ b/crates/ziggurat-zigbee/src/sends/mod.rs @@ -7,9 +7,9 @@ //! - [`Unicasts`](unicasts::Unicasts): a frame with a known next hop, delivered directly //! (with retries) or to a sleepy child when it polls. //! - [`UnroutedUnicasts`](unrouted::UnroutedUnicasts): a unicast with no route yet, held -//! until its destination's route discovery ends. The driver runs the discovery against its -//! routing table and reports when the route is known or the discovery lapsed; the -//! send then moves to the unicasts or fails. +//! until a timer pass finds a next hop for it. The routing table decides when a route +//! discovery is started or given up on; the driver reports a destination it gave up +//! on, and the sends held for it fail. //! - [`Broadcasts`](broadcasts::Broadcasts): a frame sent once, or rebroadcast on a //! schedule until its passive-ack quorum is heard. //! From a489574b933974d9f081c10b3e07c1a82e3d183c Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:26:29 +0000 Subject: [PATCH 24/29] Add reported route failures and forgotten devices to the discovery model --- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index e244001..d00390e 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -499,9 +499,13 @@ enum Step { Break, /// We relay another router's route request toward the device. Relay, + /// A router reports that it could not deliver a frame of ours to the device. + ForgetRoute, + /// The device leaves or joins again. + Forget, } -const STEPS: [Step; 7] = [ +const STEPS: [Step; 9] = [ Step::Hold, Step::Release, Step::Lapse, @@ -509,6 +513,8 @@ const STEPS: [Step; 7] = [ Step::Reply, Step::Break, Step::Relay, + Step::ForgetRoute, + Step::Forget, ]; /// What `Routing` should hold about `DEVICE`. @@ -687,6 +693,24 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { model.status = Some(Status::DiscoveryUnderway); } } + Step::ForgetRoute => { + routing.forget_route_to(DEVICE); + + // An entry that waits for a reply holds no route + if model.status != Some(Status::DiscoveryUnderway) { + model.status = None; + } + } + Step::Forget => { + routing.forget(OWN, DEVICE); + + // Discoveries we relay stay. The driver fails the frames held for the + // device + model.status = None; + model.live = false; + model.discoveries_left = None; + model.wanted = false; + } } assert_eq!(routing.route_status(DEVICE), model.status, "{steps:?}"); From fcc56d31e8b79ad807108b9f8decf2f2c380373a Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:28:12 +0000 Subject: [PATCH 25/29] Drop the two discovery model steps that reach nothing the others do not --- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 28 ++----------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index d00390e..5a39702 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -495,26 +495,21 @@ enum Step { Tick, /// The device answers the latest discovery. Reply, - /// The route to the device is lost. - Break, /// We relay another router's route request toward the device. Relay, - /// A router reports that it could not deliver a frame of ours to the device. + /// A router reports that it could not deliver a frame of ours to the device: a + /// route we had is lost. ForgetRoute, - /// The device leaves or joins again. - Forget, } -const STEPS: [Step; 9] = [ +const STEPS: [Step; 7] = [ Step::Hold, Step::Release, Step::Lapse, Step::Tick, Step::Reply, - Step::Break, Step::Relay, Step::ForgetRoute, - Step::Forget, ]; /// What `Routing` should hold about `DEVICE`. @@ -671,13 +666,6 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { model.replied = true; } Step::Reply => {} - Step::Break => { - routing.invalidate_via(NEIGHBOR); - - if model.status == Some(Status::Active) { - model.status = Some(Status::Inactive); - } - } Step::Relay => { let request = RouteRequest { originator: ROUTER, @@ -701,16 +689,6 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { model.status = None; } } - Step::Forget => { - routing.forget(OWN, DEVICE); - - // Discoveries we relay stay. The driver fails the frames held for the - // device - model.status = None; - model.live = false; - model.discoveries_left = None; - model.wanted = false; - } } assert_eq!(routing.route_status(DEVICE), model.status, "{steps:?}"); From 16fdb6d9ff401b18d31bd3c564884648afbf7bfe Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 01:14:59 +0000 Subject: [PATCH 26/29] Record that a route reply was accepted apart from its path cost --- crates/ziggurat-zigbee/src/nwk/routing.rs | 28 +++++++++++-------- .../ziggurat-zigbee/src/nwk/routing/tests.rs | 18 ++++++++++++ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 7dd6ac0..4333faf 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -137,7 +137,8 @@ pub struct DiscoveryEntry { /// device. pub forward_cost: u8, /// The accumulated path cost from the current device to the destination device. - pub residual_cost: u8, + /// `None` until a route reply is accepted. + pub residual_cost: Option, /// A countdown timer indicating the number of milliseconds until route discovery /// expires. The initial value is `nwkcRouteDiscoveryTime`. pub expiration_time: Instant, @@ -304,7 +305,7 @@ impl Routing { entry.source_address == own_address && entry.destination_address == destination && entry.expiration_time > now - && entry.residual_cost == u8::MAX + && entry.residual_cost.is_none() }) } @@ -567,7 +568,7 @@ impl Routing { source_address: own_address, sender_address: UNKNOWN_NEXT_HOP, forward_cost: 0, - residual_cost: u8::MAX, + residual_cost: None, expiration_time: now + route_discovery_time, destination_address: destination, }); @@ -599,7 +600,7 @@ impl Routing { source_address: own_address, sender_address: UNKNOWN_NEXT_HOP, forward_cost: 0, - residual_cost: u8::MAX, + residual_cost: None, expiration_time: now + route_discovery_time, destination_address: BROADCAST_ALL_ROUTERS_AND_COORDINATOR, }, @@ -649,7 +650,7 @@ impl Routing { source_address: originator, sender_address: sender, forward_cost: updated_path_cost, - residual_cost: u8::MAX, + residual_cost: None, expiration_time: now + route_discovery_time, destination_address: destination, }, @@ -733,10 +734,12 @@ impl Routing { // If we are the originator, handling is simplified if originator == own_address { if routing_entry.status == Status::Active - && updated_path_cost >= discovery_entry.residual_cost + && discovery_entry + .residual_cost + .is_some_and(|residual_cost| updated_path_cost >= residual_cost) { tracing::debug!( - "Ignoring route reply for us with higher cost ({} > {})", + "Ignoring route reply for us with higher cost ({} > {:?})", updated_path_cost, discovery_entry.residual_cost ); @@ -756,7 +759,7 @@ impl Routing { routing_entry.status = Status::Active; routing_entry.next_hop_address = sender; routing_entry.path_cost = updated_path_cost; - discovery_entry.residual_cost = updated_path_cost; + discovery_entry.residual_cost = Some(updated_path_cost); let update = (previous_next_hop != Some(sender)).then_some(RouteUpdate { destination: responder, @@ -772,9 +775,12 @@ impl Routing { // Otherwise, we need to decide if we need to update our own routes and possibly // relay the frame - if updated_path_cost >= discovery_entry.residual_cost { + if discovery_entry + .residual_cost + .is_some_and(|residual_cost| updated_path_cost >= residual_cost) + { tracing::debug!( - "Ignoring unsolicited route reply with higher cost ({} > {})", + "Ignoring unsolicited route reply with higher cost ({} > {:?})", updated_path_cost, discovery_entry.residual_cost ); @@ -787,7 +793,7 @@ impl Routing { routing_entry.status = Status::Active; routing_entry.next_hop_address = sender; routing_entry.path_cost = updated_path_cost; - discovery_entry.residual_cost = updated_path_cost; + discovery_entry.residual_cost = Some(updated_path_cost); // The reply travels back along the path the request came from let next_hop = discovery_entry.sender_address; diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 5a39702..8fbe485 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -454,6 +454,24 @@ fn test_relayed_discovery_does_not_revive_our_answered_one() { assert!(want(&mut routing, START, 1)); } +// A reply can carry the highest path cost. It still answers the discovery. +#[test] +fn test_reply_with_the_highest_path_cost_answers_the_discovery() { + let mut routing = Routing::default(); + let request_id = routing + .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, 1) + .unwrap(); + + let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, u8::MAX); + assert_eq!(reply.disposition, RouteReplyDisposition::Established); + assert_eq!(tick(&mut routing, START, false), DiscoveryTick::default()); + + routing.invalidate_via(NEIGHBOR); + routing.note_relayed_discovery(DEVICE); + + assert!(want(&mut routing, START, 1)); +} + // The route can be lost between the reply and the pass that would have released the // held frame. The frame must not be left waiting with no discovery behind it. #[test] From f233c1fb0f53f5052049170d9467af0230d44211 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 01:16:12 +0000 Subject: [PATCH 27/29] Drop a route reply that comes after its discovery lapsed --- .../ziggurat-driver/src/zigbee_stack/route.rs | 2 ++ crates/ziggurat-zigbee/src/nwk/routing.rs | 5 +++ .../ziggurat-zigbee/src/nwk/routing/tests.rs | 36 ++++++++++++++++--- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/route.rs b/crates/ziggurat-driver/src/zigbee_stack/route.rs index 67dc506..4f41d44 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/route.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/route.rs @@ -35,6 +35,7 @@ impl ZigbeeStack { // responder seeds the first hop and each relay adds the link it forwards across. let updated_path_cost = route_reply_cmd.path_cost; + let now = self.core_now(); let outcome = self.core().nib.routing.accept_route_reply( self.state.network_address, route_reply_cmd.originator_nwk, @@ -42,6 +43,7 @@ impl ZigbeeStack { route_reply_cmd.responder_nwk, nwk_frame.nwk_header.source, updated_path_cost, + now, ); let (next_hop_nwk, path_cost) = match outcome.disposition { diff --git a/crates/ziggurat-zigbee/src/nwk/routing.rs b/crates/ziggurat-zigbee/src/nwk/routing.rs index 4333faf..da19c9e 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing.rs @@ -706,6 +706,7 @@ impl Routing { /// Process a received route reply against the discovery and route tables and /// decide whether to drop it, accept it for our own request, or relay it onward. /// `updated_path_cost` is the advertised cost plus the sender's outgoing link cost. + #[allow(clippy::too_many_arguments)] pub fn accept_route_reply( &mut self, own_address: Nwk, @@ -714,7 +715,11 @@ impl Routing { responder: Nwk, sender: Nwk, updated_path_cost: u8, + now: Instant, ) -> RouteReplyOutcome { + // A reply to a discovery that has lapsed finds no entry and is dropped + self.expire_discoveries(now); + let Some(discovery_entry) = self.discovery_table.get_mut(&(originator, request_id)) else { tracing::debug!("Route reply for unknown route discovery, ignoring"); return RouteReplyOutcome { diff --git a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs index 8fbe485..4e34080 100644 --- a/crates/ziggurat-zigbee/src/nwk/routing/tests.rs +++ b/crates/ziggurat-zigbee/src/nwk/routing/tests.rs @@ -400,18 +400,23 @@ fn test_removing_the_route_keeps_a_discovery_underway() { assert!(!routing.remove_route(DEVICE)); assert!(!routing.forget_route_to(DEVICE)); - let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, 3); + let reply = reply(&mut routing, request_id, START); assert_eq!(reply.disposition, RouteReplyDisposition::Established); assert_eq!(routing.next_hop(DEVICE), Some(NEIGHBOR)); } +/// `DEVICE` answers our route request through `NEIGHBOR`. +fn reply(routing: &mut Routing, request_id: RouteRequestId, now: Instant) -> RouteReplyOutcome { + routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, 3, now) +} + /// Start a discovery toward `DEVICE` and have the device answer it. fn answered_discovery(routing: &mut Routing, attempts: u8) { let request_id = routing .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, attempts) .unwrap(); - let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, 3); + let reply = reply(routing, request_id, START); assert_eq!(reply.disposition, RouteReplyDisposition::Established); assert_eq!(routing.next_hop(DEVICE), Some(NEIGHBOR)); } @@ -462,7 +467,7 @@ fn test_reply_with_the_highest_path_cost_answers_the_discovery() { .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, 1) .unwrap(); - let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, u8::MAX); + let reply = routing.accept_route_reply(OWN, OWN, request_id, DEVICE, NEIGHBOR, u8::MAX, START); assert_eq!(reply.disposition, RouteReplyDisposition::Established); assert_eq!(tick(&mut routing, START, false), DiscoveryTick::default()); @@ -501,6 +506,21 @@ fn test_forget_ends_our_discovery_toward_the_device() { assert!(want(&mut routing, START, 1)); } +// Nothing wakes the timer for a discovery no frame waits on, so its entry can outlive +// its expiry. A reply must not find it. +#[test] +fn test_reply_to_a_lapsed_discovery_is_dropped() { + let mut routing = Routing::default(); + let request_id = routing + .want_route(OWN, DEVICE, START, ROUTE_DISCOVERY_TIME, 1) + .unwrap(); + + let late = reply(&mut routing, request_id, START + ROUTE_DISCOVERY_TIME); + + assert_eq!(late.disposition, RouteReplyDisposition::Drop); + assert_eq!(routing.route_status(DEVICE), None); +} + /// One event in the life of our route discovery toward `DEVICE`. #[derive(Debug, Clone, Copy)] enum Step { @@ -671,8 +691,7 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { ); } Step::Reply if model.request_pending() => { - let reply = - routing.accept_route_reply(OWN, OWN, request_id.unwrap(), DEVICE, NEIGHBOR, 3); + let reply = reply(&mut routing, request_id.unwrap(), now); assert_eq!( reply.disposition, RouteReplyDisposition::Established, @@ -683,6 +702,13 @@ fn check_own_discovery(steps: &[Step], attempts: u8) { model.status = Some(Status::Active); model.replied = true; } + // A reply that comes after its discovery lapsed is too late + Step::Reply if !model.live && request_id.is_some() => { + let reply = reply(&mut routing, request_id.unwrap(), now); + assert_eq!(reply.disposition, RouteReplyDisposition::Drop, "{steps:?}"); + + model.expire(); + } Step::Reply => {} Step::Relay => { let request = RouteRequest { From c0b49fa0a0219026def63c209d2081d4e1aa1110 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 01:32:51 +0000 Subject: [PATCH 28/29] Forget an address's routing state and fail its held sends on an address conflict --- crates/ziggurat-driver/src/zigbee_stack/joining.rs | 7 ++++--- crates/ziggurat-driver/src/zigbee_stack/nwk.rs | 8 ++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/ziggurat-driver/src/zigbee_stack/joining.rs b/crates/ziggurat-driver/src/zigbee_stack/joining.rs index f35450e..16e8bec 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/joining.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/joining.rs @@ -296,11 +296,12 @@ impl ZigbeeStack { return; } - // Routers resolve their own conflicts after hearing the notification; our - // mapping for the address is ambiguous until the keeper re-announces + // Routers resolve their own conflicts after hearing the notification. Until + // one announces itself we cannot tell which device a frame to the address + // would reach, so sends held for it fail let mut core = self.core(); core.nib.address_map.forget_address(address); - core.nib.routing.remove_route(address); + self.forget_nwk_state_in(core, address); } /// The address-conflict report reactor: a single long-lived task owning the diff --git a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs index 934c508..48dddea 100644 --- a/crates/ziggurat-driver/src/zigbee_stack/nwk.rs +++ b/crates/ziggurat-driver/src/zigbee_stack/nwk.rs @@ -49,14 +49,14 @@ impl ZigbeeStack { self.forget_nwk_state_in(core, nwk); } - /// The device at `nwk` left or joined again, so what we know about the address - /// belongs to its previous session. Held sends fail: they wait on a route request - /// the device never heard, and forgetting the address drops that discovery. + /// The device at `nwk` left or joined again, or two devices claim the address, so + /// what we know about it no longer holds. Held sends fail: they wait on a route + /// request the device never heard, and forgetting the address drops that discovery. pub(super) fn forget_nwk_state(&self, nwk: Nwk) { self.forget_nwk_state_in(self.core(), nwk); } - fn forget_nwk_state_in(&self, mut core: CoreGuard<'_>, nwk: Nwk) { + pub(super) fn forget_nwk_state_in(&self, mut core: CoreGuard<'_>, nwk: Nwk) { core.nib.routing.forget(self.state.network_address, nwk); let actions = core.sends.fail_unrouted(nwk); self.commit_sends(core, actions); From 8c84e1bfc60f44070d8b0094f17320e91bb66d31 Mon Sep 17 00:00:00 2001 From: puddly <32534428+puddly@users.noreply.github.com> Date: Fri, 13 Nov 2026 01:54:57 +0000 Subject: [PATCH 29/29] Keep the wire values of send statuses that are no longer sent --- crates/ziggurat-protocol/src/wire.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/ziggurat-protocol/src/wire.rs b/crates/ziggurat-protocol/src/wire.rs index 3e51ed5..845cf67 100644 --- a/crates/ziggurat-protocol/src/wire.rs +++ b/crates/ziggurat-protocol/src/wire.rs @@ -713,10 +713,14 @@ pub enum SendStatus { Success = 0, RouteDiscoveryTimeout = 1, RouteDiscoveryNoEntry = 2, + /// No longer sent. Kept so a confirmation from older firmware still decodes. + RouteInactiveAfterDiscovery = 3, NwkNoAck = 4, CcaFailure = 5, TransmitFailed = 6, ApsAckTimeout = 7, + /// No longer sent. Kept so a confirmation from older firmware still decodes. + BroadcastQuorumNotReached = 8, IndirectExpired = 9, FrameBudgetExhausted = 10, Cancelled = 11,