Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto detection of local ip #184

Merged
merged 11 commits into from
Apr 7, 2019
Merged

Conversation

farmio
Copy link
Member

@farmio farmio commented Feb 23, 2019

Make local_ip optional for tunneling and routing ConnectionConfig.

Tunnelling

  • Function: An interface on the same subnet as the configured gateway will be selected. If none was found, an interface on the same subnet as the default gateway will be selected.
  • Intention: Environments where multicast doesn't work or ip address of host changes.

Routing

  • Function: If no local_ip is configured KNXIPInterface is started like Automatic with a GatewayScanFilter searching only routing devices. If the filter is set only routing connections will be made.
  • Intention: Multiple gateways on the bus; routing shall be preferred to reserve tunneling connections for other applications (eg. ETS).

@coveralls
Copy link

coveralls commented Feb 23, 2019

Coverage Status

Coverage increased (+0.3%) to 89.494% when pulling f48d936 on farmio:tunneling-auto-local-ip into 44121cf on XKNX:master.

Copy link
Collaborator

@Julius2342 Julius2342 left a comment

Choose a reason for hiding this comment

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

It looks good :) I have some stylistic comments, but i am happy to discuss if you think I'm wrong ..

xknx/core/config.py Outdated Show resolved Hide resolved
xknx/io/knxip_interface.py Show resolved Hide resolved
xknx/io/knxip_interface.py Outdated Show resolved Hide resolved
xknx/io/knxip_interface.py Outdated Show resolved Hide resolved
xknx/io/knxip_interface.py Outdated Show resolved Hide resolved
xknx/core/config.py Outdated Show resolved Hide resolved
xknx/io/knxip_interface.py Outdated Show resolved Hide resolved
self.xknx.logger.debug(
"No interface on same subnet as gateway found. Falling back to default gateway.")
default_gateway = _find_default_gateway()
local_ip = _scan_interfaces(default_gateway)
Copy link
Member Author

Choose a reason for hiding this comment

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

Shall we try: except: and raise here?

self._parse_connection_prefs(ConnectionType.AUTOMATIC, prefs)
elif conn.startswith("tunneling"):
elif conn == "tunneling":
self._parse_connection_prefs(ConnectionType.TUNNELING, prefs)
Copy link
Member Author

Choose a reason for hiding this comment

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

Shall we raise if tunneling: is used without gateway_ip:? Or maybe just logger.warning and fall back to ConnectionType.AUTOMATIC ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should raise ...

@farmio farmio changed the title Auto detection of local ip [WIP] Auto detection of local ip Mar 2, 2019
scan_filter.tunnelling = True
elif connection_type == ConnectionType.ROUTING:
scan_filter.routing = True
self.scan_filter = scan_filter
Copy link
Member Author

Choose a reason for hiding this comment

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

GatewayScanFilter now accounting ConnectionType on initialization. I was thinking it wouldn't make sense to scan for routers if a tunneling connection shall be made (and vice versa).

@farmio farmio changed the title [WIP] Auto detection of local ip Auto detection of local ip Mar 16, 2019
@farmio farmio requested a review from Julius2342 March 16, 2019 15:22
@Julius2342 Julius2342 merged commit 40888c9 into XKNX:master Apr 7, 2019
@farmio farmio deleted the tunneling-auto-local-ip branch April 8, 2019 04:47
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.

3 participants