Skip to content

Conversation

@sfo2001
Copy link

@sfo2001 sfo2001 commented Jun 23, 2025

Problem

Discovery fails silently on Windows systems due to ProactorEventLoop not properly handling the "<broadcast>" address, resulting in UDP packets not reaching the network interface. This particularly affects Windows systems with complex network configurations (multiple adapters, VirtualBox, WSL, etc.).

Fixes #61

Root Cause

The hardcoded "<broadcast>" string in discovery.py:39 works on Linux SelectorEventLoop but fails on Windows ProactorEventLoop, causing silent discovery failures where no bridges are found despite being present on the network.

Solution

  • Replace hardcoded "<broadcast>" with programmatic broadcast address detection using netifaces
  • Add robust fallback to 255.255.255.255 when interface detection fails
  • Improve error handling around protobuf parsing
  • Add comprehensive documentation and logging

Testing

Before (Windows)

Discovered bridges:

After (Windows)

Discovered bridges:
<Bridge 192.168.xxx.yy, UID=000000000022xxxxxxxxxxxxxxxxxxxx>

Cross-platform compatibility

  • Windows: Now works with ProactorEventLoop
  • Linux: Continues to work (tested on both platforms)
  • Fallback: Graceful degradation when network detection fails

Changes

  • Add netifaces dependency for network interface detection
  • Enhance broadcast address resolution logic
  • Improve error handling and logging
  • Modernize type hints to Python 3.10+ standards
  • Add comprehensive function documentation

Breaking Changes

None - maintains full backward compatibility.

The discovery process failed due to the default Python broadcast address not working in all cases, causing UDP packets to not leave the interface and resulting in no units being found in the local network. This fix ensures the correct broadcast address is used.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances bridge discovery functionality by replacing hardcoded broadcast addresses with programmatic detection using the netifaces library, and adds comprehensive error handling and documentation.

Key changes:

  • Adds netifaces dependency to dynamically determine network broadcast addresses
  • Updates type hints to use modern Python 3.10+ union syntax (str | None instead of str = None)
  • Adds try-except blocks for robust error handling during broadcast address detection and response parsing
  • Adds comprehensive docstring to discover_bridges() function

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
pyproject.toml Adds netifaces ^0.11.0 dependency for network interface detection
aiocomfoconnect/discovery.py Implements dynamic broadcast address detection, adds error handling for discovery response parsing, modernizes type hints, and adds comprehensive function documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +41 to +43
default_iface = gws['default'][netifaces.AF_INET][1]
addrs = netifaces.ifaddresses(default_iface)
broadcast_addr = addrs[netifaces.AF_INET][0].get('broadcast', '255.255.255.255')
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

This code assumes the presence of keys in nested dictionaries without checking their existence, which can cause KeyError exceptions not caught by the generic Exception handler. Specifically, gws['default'], gws['default'][netifaces.AF_INET], and addrs[netifaces.AF_INET] could raise KeyError if the network doesn't have a default IPv4 gateway or IPv4 addresses configured. While the broad exception handler will catch this, it would be more explicit and maintainable to use .get() methods or check for key existence.

Suggested change
default_iface = gws['default'][netifaces.AF_INET][1]
addrs = netifaces.ifaddresses(default_iface)
broadcast_addr = addrs[netifaces.AF_INET][0].get('broadcast', '255.255.255.255')
default = gws.get('default')
default_inet = default.get(netifaces.AF_INET) if default else None
default_iface = default_inet[1] if default_inet and len(default_inet) > 1 else None
if default_iface:
addrs = netifaces.ifaddresses(default_iface)
inet_addrs = addrs.get(netifaces.AF_INET)
if inet_addrs and len(inet_addrs) > 0:
broadcast_addr = inet_addrs[0].get('broadcast', '255.255.255.255')
else:
broadcast_addr = '255.255.255.255'
else:
broadcast_addr = '255.255.255.255'

Copilot uses AI. Check for mistakes.
List[Bridge]: A list of discovered Bridge objects.
Raises:
Any exceptions raised by the underlying asyncio transport or protocol.
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The 'Raises' section is vague and unhelpful. It should document specific exceptions that callers might need to handle. Based on the code, this function doesn't raise exceptions from the protocol (they're caught internally), but could raise exceptions from loop.create_datagram_endpoint(). Consider either removing this section or documenting specific exceptions like OSError for network issues.

Suggested change
Any exceptions raised by the underlying asyncio transport or protocol.
OSError: If a network error occurs while creating the datagram endpoint.

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +46
broadcast_addr = addrs[netifaces.AF_INET][0].get('broadcast', '255.255.255.255')
except Exception as e:
_LOGGER.warning("Could not determine broadcast address, using 255.255.255.255: %s", e)
broadcast_addr = '255.255.255.255'
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The code indexes into addrs[netifaces.AF_INET][0] assuming the first address entry contains the broadcast address, but some network configurations may have multiple IPv4 addresses on an interface. While the fallback to '255.255.255.255' handles missing 'broadcast' keys, the indexing with [0] could still raise IndexError if the AF_INET list is empty. This is an edge case but worth addressing for robustness.

Suggested change
broadcast_addr = addrs[netifaces.AF_INET][0].get('broadcast', '255.255.255.255')
except Exception as e:
_LOGGER.warning("Could not determine broadcast address, using 255.255.255.255: %s", e)
broadcast_addr = '255.255.255.255'
inet_addrs = addrs.get(netifaces.AF_INET, [])
if inet_addrs and 'broadcast' in inet_addrs[0]:
broadcast_addr = inet_addrs[0]['broadcast']
else:
broadcast_addr = '255.255.255.255'

Copilot uses AI. Check for mistakes.
@michaelarnauts
Copy link
Owner

I wonder if I can just use 255.255.255.255 instead of <broadcast>?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discovery fails on Windows - <broadcast> address not working with ProactorEventLoop

2 participants