Skip to content

Require ESPHome ASH auto-ACKing when using a serial proxy - #766

Merged
puddly merged 11 commits into
zigpy:devfrom
puddly:puddly/fast-acks
Oct 22, 2026
Merged

puddly merged 11 commits into
zigpy:devfrom
puddly:puddly/fast-acks

Conversation

@puddly

@puddly puddly commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

ESPHome serial proxying now supports ezsp_proxy_tap, allowing bellows to opt into on-device ASH auto-ACKing and mitigate network latency issues. This is a requirement for serial proxies.

Depends on puddly/serialx#138.

Copilot AI balanced review requested due to automatic review settings October 13, 2026 17:41
@codecov

codecov Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.58%. Comparing base (aea9248) to head (17e79aa).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #766   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          64       64           
  Lines        4331     4350   +19     
=======================================
+ Hits         4313     4332   +19     
  Misses         18       18           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

🟡 Changes recommended

The dependency bound lacks mandatory tap enforcement, and connection timing and URL mode overrides can leave ASH frames unacknowledged.

2 open findings
What changed in this PR

Requires ESPHome serial proxies to use on-device ASH ACK handling while Bellows retains NAK handling.

Changes:

  • Requests ESPHome protocol-tap mode.
  • Adds configurable ASH ACK suppression and tests.
  • Adds the serialx dependency.
File Description
bellows/​uart.py Enables ESPHome protocol mode and ACK suppression.
bellows/​ash.py Adds optional standalone ACK suppression.
tests/​test_uart.py Tests transport-mode selection.
tests/​test_ash.py Tests ACK suppression while preserving NAKs.
pyproject.toml Adds serialx dependency.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pyproject.toml Outdated
Comment thread bellows/uart.py
@puddly
puddly merged commit 4271531 into zigpy:dev Oct 22, 2026
12 checks passed

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Post-merge review (merged as 4271531, not in a release yet — latest is 1.1.0). The ASH side is clean: _send_ack() covers both ACK sites, NAKs still go out, and both earlier Copilot threads are handled at the head (serialx 1.13.0 contains puddly/serialx#138, and suppress_acks is fixed at construction). Tests pass locally (714 passed, Python 3.14, serialx 1.15.0).

Summary of findings

  • Blocking (inline on bellows/uart.py): on esphome-hass://, the URL HA actually stores, serialx drops mode="protocol". bellows decides to stop ACKing from the URL scheme alone, so it does anyway, and the NCP gets no standalone ACKs. A ?mode=raw query on esphome:// hits the same mismatch. Suggested fix: decide suppression in AshProtocol.connection_made from the port's confirmed tap_mode, and have HA's ESPHome handler declare mode.
  • Should be fixed too (inline on bellows/ash.py): if ESPHome drops a chunk the tap already ACKed, ASH can't recover it. bellows discards the following frames, and every DATA frame it sends carries an ackNum the NCP rejects, so both directions stall until a reset. A blind resync isn't a safe fix; a timeout on the reject condition is.

Nits / notes

  • Requesting protocol is now mandatory: ESPHome devices below API 1.17, or a proxy without ezsp_proxy_tap, now fail to open with UnsupportedSetting instead of running self-ACKed. That's intended per the title, but worth a release note so users on older ESPHome firmware know why ZHA stopped starting.
  • pyproject.toml: "serialx>=1.13.0" has no trailing comma, unlike the rest of the list.
  • _connect calls is_esphome_url(path) three times; one local is enough. (This goes away anyway if suppression moves into connection_made.)
  • The comment above the AshProtocol(...) line ("serialx opens an ESPHome port in the mode we ask for or raises") is the assumption the first finding breaks. Worth rewording with the fix.

Comment thread bellows/uart.py
Comment thread bellows/ash.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Breaking change (mainly for PRs) new feature Adds new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants