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

HA: configurable IA, Multicast group and port #372

Merged
merged 2 commits into from Sep 3, 2020

Conversation

farmio
Copy link
Member

@farmio farmio commented Sep 2, 2020

I'm still not happy with having to set local_ip manually for routing connections, but I haven't found a way to configure it so that

knx:
  routing:
  expose:
    ....

or similar would be possible.

Should we change the configuration to something like that:

knx:
  connection_type: routing # or "tunneling"
  local_ip: 123 # optional - would fit for routing and tunnelling
# or
knx:
  connection_type: tunneling
  tunneling:
    host: 10.1.1.1
    port: 1234
# or
knx:
  connection_type: tunneling
  tunneling_host: 10.1.1.1
  tunneling_port: 1234

?

@coveralls
Copy link

coveralls commented Sep 2, 2020

Coverage Status

Coverage remained the same at 92.136% when pulling 65ff504 on farmio:HA-connection-parameters into b59a8b8 on XKNX:master.

Copy link
Member

@marvin-w marvin-w left a comment

Choose a reason for hiding this comment

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

Changes look good. I like the last option most.

knx:
  connection_type: tunneling
  tunneling_host: 10.1.1.1
  tunneling_port: 1234

One thing:

In the tunneling connection schema you could as well define a default port:

gateway_port = self.config[DOMAIN][CONF_XKNX_TUNNELING].get(CONF_PORT)
        local_ip = self.config[DOMAIN][CONF_XKNX_TUNNELING].get(
            ConnectionSchema.CONF_XKNX_LOCAL_IP
        )
        if gateway_port is None:
            gateway_port = DEFAULT_MCAST_PORT

Just so that it's nice and tidy.

@marvin-w
Copy link
Member

marvin-w commented Sep 3, 2020

Looks good now, thanks! I'd suggest opening another PR for changing the connection params.

@marvin-w marvin-w merged commit 74989ac into XKNX:master Sep 3, 2020
@farmio farmio deleted the HA-connection-parameters branch September 4, 2020 05:17
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.

None yet

3 participants