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

SMA: Optional import in schema & backoff fix #18099

Merged
merged 2 commits into from Nov 4, 2018

Conversation

Projects
None yet
6 participants
@kellerza
Member

kellerza commented Nov 1, 2018

Description:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
Show resolved Hide resolved homeassistant/components/sensor/sma.py Outdated
backoff = 10
if not values:
try:
backoff = [1, 1, 1, 6, 6, 6, 30, 30][backoff_step]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 1, 2018

Member

This looks ok. I would probably instead use a start value that is multiplied and cap it with min and max values. The value would represent the time until next try. Eg:
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/netgear_lte.py#L110

But maybe we need a different approach here? You know the integration best.

This comment has been minimized.

@kellerza

kellerza Nov 2, 2018

Member

Let’s keep this. I will change to 1,1,1,6,30 and end with the 60

Reasoning
The current implementation ensures multiples of SCAN_INTERVAL, which feels consistent for reporting. Can likely do time based delay, but it will need to take S_I into account.

The library is fairly resilient and that why I have the many 1xS_I to start. If something goes wrong (a) might be network based or (b) not sure about the exact timeout when the user will be available(might have logged in through browser etc)

@@ -73,6 +76,9 @@ def _check_sensor_schema(conf):
"""Set up SMA WebConnect sensor."""
import pysma

# Check config again during load - dependancy available

This comment has been minimized.

@tjorim

tjorim Nov 4, 2018

Contributor

Let's try this 'suggested change' feature 😉

Suggested change Beta
# Check config again during load - dependancy available
# Check config again during load - dependency available

This comment has been minimized.

@kellerza

kellerza Nov 4, 2018

Member

Thanks @tjorim ! I fixed this on Friday, but could not push due to issues on github. I pushed my fixes now

This comment has been minimized.

@kellerza

kellerza Nov 4, 2018

Member

This suggested change feature looks quite cool though!

@amelchio amelchio added this to the 0.82 milestone Nov 4, 2018

@kellerza kellerza merged commit 44556a8 into home-assistant:dev Nov 4, 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.002%) to 93.065%
Details

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

@kellerza kellerza deleted the kellerza:jksma branch Nov 4, 2018

@kellerza kellerza referenced this pull request Nov 4, 2018

Merged

Update SMA sensor #17988

7 of 7 tasks complete

balloob added a commit that referenced this pull request Nov 5, 2018

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

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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