feat: add support for setting sACN interface#107
feat: add support for setting sACN interface#107Breina merged 5 commits intoBreina:platform-refactor2from
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configuring which network interface the sACN server should bind to for sending and receiving data. This is useful for Home Assistant installations with multiple network interfaces where DMX/sACN traffic should be isolated to a specific network.
Key Changes
- Added
interface_ipconfiguration option to specify the network interface by IP address - Updated socket binding logic to use the specified interface for both sending and receiving multicast traffic
- Added documentation explaining when and how to use the new configuration option
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sacn_server.py | Added test assertions for the new interface_ip configuration field |
| docs/sacn-communication.md | Added documentation section explaining the interface_ip setting and when to use it |
| custom_components/dmx/setup/config_processor.py | Added CONF_INTERFACE_IP constant and validation schema for the new configuration option |
| custom_components/dmx/server/sacn_server.py | Implemented interface binding logic in both SacnServer and SacnReceiver classes |
| custom_components/dmx/protocols/sacn/protocol.py | Integrated interface_ip configuration into the sACN protocol setup |
| custom_components/dmx/protocols/base.py | Refactored to use constants instead of string literals (unrelated cleanup) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.socket.setsockopt(socket.IPPROTO_IP, socket.IP_MULTICAST_IF, packed_interface) | ||
| self.running = True | ||
| log.info(f"sACN server started on {self.config.interface_ip}:{SACN_PORT}") | ||
| else: | ||
| self.running = True |
There was a problem hiding this comment.
The logic flow here is redundant. The self.running = True assignment is duplicated in both branches of the conditional. Consider moving it outside the conditional block to avoid duplication and improve maintainability. Additionally, the multicast interface configuration should be set regardless of whether an interface_ip is provided, as the socket binding already handles the interface selection.
| self.socket.setsockopt(socket.IPPROTO_IP, socket.IP_MULTICAST_IF, packed_interface) | |
| self.running = True | |
| log.info(f"sACN server started on {self.config.interface_ip}:{SACN_PORT}") | |
| else: | |
| self.running = True | |
| else: | |
| # Use INADDR_ANY to allow the OS to choose the default multicast interface | |
| packed_interface = struct.pack("=I", socket.INADDR_ANY) | |
| self.socket.setsockopt(socket.IPPROTO_IP, socket.IP_MULTICAST_IF, packed_interface) | |
| self.running = True | |
| if self.config.interface_ip: | |
| log.info(f"sACN server started on {self.config.interface_ip}:{SACN_PORT}") | |
| else: |
There was a problem hiding this comment.
Hmm. This may be a bug that previously existed. 🤔
Do we want to bind and automatically default to registering the multicast groups on all interfaces, or only on the one interface? Right now the listener binds to all interfaces but only registers with IGMP on one the chosen by os interface. This is what happens in the refactor tree, even before this pull request.
| log.info(f"sACN server started on {self.config.interface_ip}:{SACN_PORT}") | ||
| else: | ||
| self.running = True | ||
| log.info(f"sACN server started on {SACN_PORT}") |
There was a problem hiding this comment.
The log message is misleading. The server binds to a random port (port 0 means the OS assigns a random available port), but the log message displays SACN_PORT (5568). The socket sends TO port 5568 on multicast addresses, but listens on a random port. The log message should either show the actual bound port by reading it from the socket, or clarify that it's sending to port 5568 rather than listening on it. Consider using self.socket.getsockname()[1] to get the actual bound port.
There was a problem hiding this comment.
Might be worth updating the log. But I'm unsure if sockname works cross-platform (HA in docker, in a VM, bare-metal, ..).
|
I have a commit to fix most of this copilot review (which I've also disabled for future pushes, I think) but haven't pushed it yet and am currently away from my PC. Can push later, I think. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Alright thanks a lot! I'll test this as soon as I can, and then investigate #108 with it as we go. Note that I've renamed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.running = True | ||
| log.info(f"sACN server started on port {SACN_PORT}") | ||
| if self.config.interface_ip: | ||
| packed_interface = socket.inet_aton(self.config.interface_ip) |
There was a problem hiding this comment.
The socket.inet_aton() call can raise OSError if the IP address is invalid. While there is a general exception handler, this specific error should be caught with a more descriptive error message to help users identify configuration issues, especially since the validation may not catch all edge cases.
| packed_interface = socket.inet_aton(self.config.interface_ip) | |
| try: | |
| packed_interface = socket.inet_aton(self.config.interface_ip) | |
| except OSError as exc: | |
| log.error( | |
| "Invalid interface_ip '%s' configured for sACN server. " | |
| "Please check the DMX integration configuration.", | |
| self.config.interface_ip, | |
| ) | |
| raise ValueError( | |
| f"Invalid interface_ip configured for sACN server: {self.config.interface_ip}" | |
| ) from exc |
| self.socket.bind((self.config.interface_ip or "", 0)) | ||
|
|
||
| self.running = True | ||
| log.info(f"sACN server started on port {SACN_PORT}") | ||
| if self.config.interface_ip: | ||
| packed_interface = socket.inet_aton(self.config.interface_ip) | ||
| self.socket.setsockopt(socket.IPPROTO_IP, socket.IP_MULTICAST_IF, packed_interface) | ||
| self.running = True | ||
| log.info(f"sACN server started on {self.config.interface_ip}:{SACN_PORT}") | ||
| else: | ||
| self.running = True | ||
| log.info(f"sACN server started on {SACN_PORT}") |
There was a problem hiding this comment.
The actual behavior of binding to a specific interface IP and setting IP_MULTICAST_IF socket option lacks test coverage. While configuration tests exist, there should be tests verifying that the socket is correctly configured with the interface IP when specified.
| if self.interface_ip: | ||
| mreq = struct.pack("4s4s", socket.inet_aton(multicast_addr), socket.inet_aton(self.interface_ip)) | ||
| else: | ||
| mreq = struct.pack("4sl", socket.inet_aton(multicast_addr), socket.INADDR_ANY) |
There was a problem hiding this comment.
Similar to the sender, the socket.inet_aton() call in the receiver can raise OSError if the IP address is invalid. This should have more specific error handling to provide clear feedback to users about invalid interface IP configuration.
| if self.interface_ip: | |
| mreq = struct.pack("4s4s", socket.inet_aton(multicast_addr), socket.inet_aton(self.interface_ip)) | |
| else: | |
| mreq = struct.pack("4sl", socket.inet_aton(multicast_addr), socket.INADDR_ANY) | |
| try: | |
| multicast_packed = socket.inet_aton(multicast_addr) | |
| except OSError as err: | |
| log.error( | |
| "Invalid multicast address '%s' while trying to %s multicast group for universe %d: %s", | |
| multicast_addr, | |
| action.lower(), | |
| universe_id, | |
| err, | |
| ) | |
| return | |
| if self.interface_ip: | |
| try: | |
| interface_packed = socket.inet_aton(self.interface_ip) | |
| except OSError as err: | |
| log.error( | |
| "Invalid interface IP '%s' while trying to %s multicast group %s for universe %d: %s", | |
| self.interface_ip, | |
| action.lower(), | |
| multicast_addr, | |
| universe_id, | |
| err, | |
| ) | |
| return | |
| mreq = struct.pack("4s4s", multicast_packed, interface_packed) | |
| else: | |
| mreq = struct.pack("4sl", multicast_packed, socket.INADDR_ANY) |
| vol.Optional(CONF_PRIORITY, default=CONF_PRIORITY_DEFAULT): vol.All( | ||
| vol.Coerce(int), vol.Range(min=0, max=200) | ||
| ), | ||
| vol.Optional(CONF_INTERFACE_IP): cv.string, |
There was a problem hiding this comment.
The interface_ip configuration should use cv.ipv4 instead of cv.string to properly validate that the provided value is a valid IPv4 address. This prevents configuration errors from invalid IP addresses.
| vol.Optional(CONF_INTERFACE_IP): cv.string, | |
| vol.Optional(CONF_INTERFACE_IP): cv.ipv4, |
There was a problem hiding this comment.
Well, if true, that's more useful. Should probably unionize it with ipv6 and that should be sufficient.
There was a problem hiding this comment.
This doesn't actually exist, as far as I can tell.
|
Tested and looks OK! Thank you very much! |
Partially closes #84.
Not fully tested since my sACN network is partially broken in a different and unique way.
I'm also bikeshedding the configuration name. Not sure if
bind_ipmakes more sense. Intentionally adding bind since I want to be able to support interface names too. For my usecase, I have a static IP and sometimes that static IP moves interfaces.Some further testing is required as I think I've discovered a bug with the receiving server. https://stackoverflow.com/questions/46066244/what-is-the-purpose-and-result-of-using-inaddr-any. It binds to all but only registers itself with one interface.