Skip to content

Account for handoff time when waiting for APS ACKs - #67

Open
puddly wants to merge 2 commits into
devfrom
puddly/aps-ack-timer-retransmission
Open

puddly wants to merge 2 commits into
devfrom
puddly/aps-ack-timer-retransmission

Conversation

@puddly

@puddly puddly commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

See #64 for more info.

@zigpy-review-bot zigpy-review-bot added the bugfix This PR fixes a bug label Oct 17, 2026

@zigpy-review-bot zigpy-review-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings. This is the "arm the deadline at handoff" part of #64 (item 2, and the misleading APS_ACK_TIMEOUT half of item 3), and it reads correct to me.

What I checked at c4a088b:

  • The pending ack is still registered before enqueue, so a fast ack is caught. handle_aps_ack removes the entry on match and resolve_aps_ack_handoff then finds nothing to arm, which is harmless because the slot's write-once rule back-fills handed_off.
  • Every handoff path goes through resolve_outcome. That includes route-discovery failure, cancellation and own sleepy children (indirect completion carries the ApsAck outcome until the poll), so a failed or cancelled send removes its entry and the host gets the real cause instead of a timeout.
  • outcome_cancelled covers ApsAck, and a cancelled entry with no deadline is still expired by expire_aps_acks, so the reactor still reaps it.
  • aps_ack_wake is notified when a deadline is armed, which is the only time it can change.
  • cargo test --workspace passes and cargo clippy -p ziggurat-driver is clean.

Non-blocking:

  • No test covers the new behaviour (deadline not armed while queued or awaiting route discovery, armed on success, entry dropped on handoff failure). The driver crate has no unit tests for this path today, so I understand if it's left for a harness; worth a follow-up if one exists.
  • An unarmed entry now depends on the queue/discovery/indirect-expiry paths always resolving the handoff. I found no path that doesn't, but this is a new coupling, and a doc note on PendingApsAck::deadline that None means "still awaiting handoff" would help.
  • Retransmission (#64 item 1) and the sleepy-behind-router hold time (item 4) remain open, so the PR body's "See #64" is fine but shouldn't close it.

@zigpy-review-bot zigpy-review-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings at 7ad76d9. This is the follow-up to my earlier review at c4a088b: deadline is now non-optional, armed at registration with aps_ack_handoff_timeout and reset to wait at handoff.

What I checked:

  • The new aps_ack_handoff_timeout (30s) is comfortably above the worst legitimate handoff wait: route_discovery_time (10s) plus transaction_persistence_time (7.68s) is about 17.7s, so a healthy send never hits it.
  • expire_aps_acks classifies correctly: cancelled first, then !is_handed_off() gives HandoffTimeout, otherwise ApsAckTimeout. A handoff failure removes the entry before this runs, so only a handoff that never resolved can reach the HandoffTimeout branch.
  • resolve_aps_ack_handoff resets the deadline before resolving HandOff and notifies aps_ack_wake, so the timeout reactor re-sleeps against the shorter deadline.
  • The new HandoffTimeout = 13 is mapped in send_status and appended after RadioError = 12, so existing wire values are unchanged.
  • cargo test --workspace passes and cargo clippy -p ziggurat-driver -p ziggurat-protocol is clean.

Non-blocking:

  • The new variant is a wire-visible status, so the host side (zigpy-ziggurat) needs to learn status 13, or it will see an unknown value.
  • My earlier note about a test for the new behaviour still applies: nothing exercises the armed-at-handoff / dropped-on-failure / HandoffTimeout paths.

@puddly
puddly marked this pull request as ready for review October 23, 2026 02:16
Copilot AI balanced review requested due to automatic review settings October 23, 2026 02:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Queued-send invalidation remains unresolved, and concurrent handoff, cancellation, and timeout ordering needs targeted validation before approval.

1 open issue
What changed in this PR

This PR separates queueing and handoff time from APS acknowledgment waiting, addressing the early-timeout behavior described in #64.

Changes:

  • Starts the APS ACK wait after successful handoff while registering pending acknowledgments before enqueueing.
  • Adds a configurable 30-second handoff watchdog and a corresponding delivery error and wire status.
  • Routes APS handoff outcomes through dedicated deadline and cleanup handling.
File Description
crates/​ziggurat-zigbee/​src/​constants.rs Adds the handoff timeout tunable.
crates/​ziggurat-protocol/​src/​wire.rs Adds the handoff timeout wire status.
crates/​ziggurat-protocol/​src/​bridge.rs Maps the new delivery error to its wire status.
crates/​ziggurat-driver/​src/​zigbee_stack/​send_handle.rs Exposes whether handoff has resolved.
crates/​ziggurat-driver/​src/​zigbee_stack/​nwk.rs Handles APS handoff outcomes and cancellation checks.
crates/​ziggurat-driver/​src/​zigbee_stack/​aps.rs Separates handoff and ACK deadlines and handles expiry.
crates/​ziggurat-driver/​src/​zigbee_stack.rs Defines the new error, outcome, and pending-ACK timing fields.

Review effort: Balanced

entry.ack_data.src,
entry.ack_data.counter
);
Err(DeliveryError::HandoffTimeout)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This PR fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants