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

Fritz keepalive #18155

Merged
merged 8 commits into from Nov 6, 2018

Conversation

Projects
None yet
6 participants
@akloeckner
Contributor

akloeckner commented Nov 3, 2018

Description:

Adds TCP keepalive support to fritzbox call monitor. This is needed, if the FRITZ!Box is queried through a firewall with NAT, since the firewall forgets about established connections after a while. TCP keepalive prevents this.

Related issue (if applicable): None

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7348

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: fritzbox_callmonitor
    name: FRITZ!Box
    #host: !secret fritz_host
    username: !secret fritz_username
    password: !secret fritz_password
    tcp_keepalive: on
    phonebook: 0
    prefixes:
    - '0221'

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

akloeckner added some commits Nov 3, 2018

Add keepalive support
- adds keepalive support
- adds debug messages
- corrects CONF_PHONEBOOK and CONF_PREFIXES constants

@akloeckner akloeckner referenced this pull request Nov 3, 2018

Closed

Add tcp_keepalive #7348

2 of 2 tasks complete
Show resolved Hide resolved homeassistant/const.py Outdated

akloeckner added some commits Nov 4, 2018

@pvizeli

This comment has been minimized.

Member

pvizeli commented Nov 4, 2018

Why we make that as an option? Do we have any pain if we do that default?

@balloob

This comment has been minimized.

Member

balloob commented Nov 4, 2018

I agree with Pascal. Absolutely not an option.

@akloeckner

This comment has been minimized.

Contributor

akloeckner commented Nov 4, 2018

As to the why: When searching the web for that functionality, I just saw someone recommending "If you are nice to your users, implement it as an option": https://www.tldp.org/HOWTO/html_single/TCP-Keepalive-HOWTO/#codeneeding . I figured, I'd be nice to our users and add the option with no harm.

As to the pain: There might be a good reason, why keepalive is disabled in Python sockets by default. A few disadvantages of keepalive are listed here: http://www.pcvr.nl/tcpip/tcp_keep.htm. Hoever, I guess the only relevant one for us would be "(1) they can cause perfectly good connections to be dropped during transient failures". In our case, we might have the home assistant sitting on an unreliable network. I don't know, what happens, if the connection to the fritzbox is constantly restarted becuase of the failing keepalives.

Summary: I find the option does no harm. But no hard feelings. If you all think different, I'll just make keepalive the default behavior. Please request a change then...

@balloob

This comment has been minimized.

Member

balloob commented Nov 5, 2018

please change it.

@akloeckner

This comment has been minimized.

Contributor

akloeckner commented Nov 5, 2018

Ok. Changed it. Could you please elaborate on the rationale? I'd like to avoid putting work in such things in the future and probably need to understand the design philosophy a bit better for that...

@balloob

balloob approved these changes Nov 5, 2018

@pvizeli pvizeli merged commit 3322fee into home-assistant:dev Nov 6, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 93.103%
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

@pvizeli

This comment has been minimized.

Member

pvizeli commented Nov 6, 2018

We want remove complexity and options. If you say that is a to heavy edge case, we can revert this PR and you need redesign your network. We going to be user friendly and not a piece of software they can be reconfigured until we are dummy.

@akloeckner

This comment has been minimized.

Contributor

akloeckner commented Nov 6, 2018

I see. So in summary and ideally and in that order:

  1. defaults should be suitable for all users
  2. if defaults are not enough, the software should somehow adapt itself
  3. if that is not manageable, an option becomes an option.

I'll keeep that in mind for future work. Thanks.

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Fritz keepalive (home-assistant#18155)
* Add keepalive support

- adds keepalive support
- adds debug messages
- corrects CONF_PHONEBOOK and CONF_PREFIXES constants

* Add keepalive config constant

* Fix default value

* More visual indentation

* move to platform

* Move to platform

* Make keepalive default and remove option

* Forgot a few lines

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

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