Add acknowledgements for proxy subscribe and configuration requests - #1865
Add acknowledgements for proxy subscribe and configuration requests#1865kbx81 wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1865 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 4288 4339 +51
=========================================
+ Hits 4288 4339 +51 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WalkthroughThe protocol now defines serial and Z-Wave proxy acknowledgements. ChangesProxy acknowledgement support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIClient
participant ESPHomeDevice
APIClient->>ESPHomeDevice: Send proxy request
alt API 1.16 or newer
ESPHomeDevice-->>APIClient: Return matching acknowledgement
APIClient-->>APIClient: Return response model
else API below 1.16
APIClient-->>APIClient: Return None after sending
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
aioesphomeapi/client.py (1)
831-837: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPlace private helpers after public API methods.
These underscore-prefixed helpers are placed between public API methods. Move them to the class private-helper section after the public API methods.
As per coding guidelines: “Organize methods with public API at the top and private helpers (_underscore_prefixed) at the bottom.”
Also applies to: 852-857, 903-907
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aioesphomeapi/client.py` around lines 831 - 837, Move the private helper methods _await_serial_proxy_response and the other underscore-prefixed methods identified in this area below all public API methods in the client class, keeping their implementations unchanged and preserving the existing private-helper section organization.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@aioesphomeapi/client.py`:
- Around line 831-837: Move the private helper methods
_await_serial_proxy_response and the other underscore-prefixed methods
identified in this area below all public API methods in the client class,
keeping their implementations unchanged and preserving the existing
private-helper section organization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e61ee06-3a1a-44dd-87b5-988e755164fb
📒 Files selected for processing (6)
aioesphomeapi/api.protoaioesphomeapi/api_pb2.pyaioesphomeapi/client.pyaioesphomeapi/core.pyaioesphomeapi/model.pytests/test_client.py
What does this implement/fix?
Client side of the proxy request acknowledgements introduced in esphome/esphome#18312 (API 1.16). Devices now answer serial proxy subscribe/unsubscribe/configure/modem-pin requests and Z-Wave proxy subscribe/unsubscribe with a status, including a denial reason (
PORT_IN_USE/IN_USE) when another client holds the subscription.api.protosynced with the firmware repo; bindings regenerated via the docker builder. NewZWaveProxyRequestResponse(id 151) withZWaveProxyStatus;SerialProxyStatusgainsPORT_IN_USEandINVALID_ARGUMENT;SerialProxyRequestTypegains the response-onlyCONFIGUREandSET_MODEM_PINSdiscriminators.serial_proxy_subscribe_await_response/serial_proxy_unsubscribe_await_responsemethods now work (the firmware previously never sent the response they await, so they always timed out).serial_proxy_configure_await_response,serial_proxy_set_modem_pins_await_response,zwave_proxy_subscribe,zwave_proxy_unsubscribe,zwave_proxy_subscribe_await_response,zwave_proxy_unsubscribe_await_response.*_await_responsemethods gate on the device API version: below 1.16 the request is sent fire-and-forget andNoneis returned, so callers never hang against older firmware.serial_proxy_flushis deliberately not gated — its acknowledgement predates 1.16.Verified against real hardware (ESP32-S3 with W5500 ethernet) in three directions: this client against API 1.16 firmware (all acknowledgement and denial paths), this client against pre-1.16 firmware (fallback returns
Noneimmediately, flush still round-trips), and the released 45.10.0 client against API 1.16 firmware (unsolicited acks are ignored cleanly, unknown id 151 skipped at debug level).Types of changes
Related issue or feature (if applicable):
Pull request in esphome (if applicable):
Checklist:
tests/folder).