Enhance Interface ABC with common connection management#1803
Enhance Interface ABC with common connection management#1803
Conversation
Formalize attributes and methods that all Interface implementations already share: xknx/cemi_received_callback slots, connection_state_changed() method, and current_address property. Update _Tunnel and Routing to use these Interface methods instead of direct xknx access. Pure refactor — no behavior changes. Prepares Interface for custom transport implementations per feedback on XKNX#1802. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the KNX/IP Interface ABC to centralize shared connection-management behavior (xknx/callback storage, connection state updates, and current individual address handling) and updates existing implementations to use the new base-class helpers.
Changes:
- Extend
Interfacewith additional slots/type annotations plusconnection_state_changed()andcurrent_addressaccessors. - Update
_TunnelandRoutingto use the newInterfacehelpers instead of directxknxaccess. - Remove duplicate slot declarations from
tunnel/routingimplementations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xknx/io/interface.py |
Adds common connection-management helpers and base slots/type annotations. |
xknx/io/tunnel.py |
Uses Interface.connection_state_changed() and Interface.current_address; removes duplicated slots. |
xknx/io/routing.py |
Uses Interface.connection_state_changed() and Interface.current_address; removes duplicated slots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1803 +/- ##
==========================================
- Coverage 97.38% 97.38% -0.01%
==========================================
Files 160 160
Lines 10951 10965 +14
==========================================
+ Hits 10665 10678 +13
- Misses 286 287 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Both suggestions look great. We agree to:
We'll update the PR accordingly. Thanks for the clear feedback! |
- Annotate connection_type as ClassVar[XknxConnectionType] in Interface ABC and all subclasses (_Tunnel, UDPTunnel, TCPTunnel, SecureTunnel, Routing, RoutingSecure) - Remove connection_type parameter from Interface.connection_state_changed; method now uses self.connection_type internally - Auto-reset connection_type to NOT_CONNECTED on DISCONNECTED state in ConnectionManager, making the invariant explicit and removing reliance on callers passing the correct default Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hello Matthias, just want to check in with you if you have any more comments or want us to refine the PR in any way? Have a nice weekend! /Mats |
|
Hi! Only the one above about the property. Otherwise I think this is good to go. |
Co-authored-by: Matthias Alphart <farmio@alphart.net>
| # Use the individual address provided by the tunnelling server | ||
| self._src_address = connect.crd.individual_address or IndividualAddress(0) | ||
| self.xknx.current_address = self._src_address | ||
| self.current_address = self._src_address |
There was a problem hiding this comment.
this should use the new method now
| self.xknx.connection_manager.connection_state_changed( | ||
| XknxConnectionState.CONNECTING, self.connection_type | ||
| ) | ||
| self.current_address = self.individual_address |
There was a problem hiding this comment.
this should use the new method now
Summary
Following @farmio's suggestion in #1802, this PR enhances the
InterfaceABC to formalize what all concrete implementations already share:xknxandcemi_received_callbackto__slots__and type annotations onInterfaceconnection_state_changed()concrete method (wrapsxknx.connection_manager)current_addressproperty (wrapsxknx.current_address)_TunnelandRoutingto use these Interface methods instead of directxknxaccessMotivation
Every
Interfaceimplementation (_Tunnel,Routing) already storesxknxandcemi_received_callback, and uses the same patterns for connection state changes and address management. Moving these to the base class:InterfaceprovidesBreaking change
xknxandcemi_received_callbackare now declared inInterface.__slots__. Third-partyInterfacesubclasses that redeclare these names in their own__slots__will need to remove them to avoid aValueErrorconflict. This is a one-line fix per subclass.Changes
xknx/io/interface.pyconnection_state_changed(),current_addresspropertyxknx/io/tunnel.pyxknx/cemi_received_callbackslots, use Interface methodsxknx/io/routing.pyxknx/cemi_received_callbackslots, use Interface methodsTesting
Pure refactor — no behavior changes. All 2876 existing tests pass with zero regressions.