Skip to content

SMA: Optional import in schema & backoff fix#18099

Merged
kellerza merged 2 commits intohome-assistant:devfrom
kellerza:jksma
Nov 4, 2018
Merged

SMA: Optional import in schema & backoff fix#18099
kellerza merged 2 commits intohome-assistant:devfrom
kellerza:jksma

Conversation

@kellerza
Copy link
Copy Markdown
Member

@kellerza 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

@ghost ghost assigned kellerza Nov 1, 2018
@ghost ghost added in progress and removed platform: sensor.sma small-pr PRs with less than 30 lines. labels Nov 1, 2018
backoff = 10
if not values:
try:
backoff = [1, 1, 1, 6, 6, 6, 30, 30][backoff_step]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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)

"""Set up SMA WebConnect sensor."""
import pysma

# Check config again during load - dependancy available
Copy link
Copy Markdown
Contributor

@tjorim tjorim Nov 4, 2018

Choose a reason for hiding this comment

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

Let's try this 'suggested change' feature 😉

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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
@ghost ghost removed the in progress label Nov 4, 2018
@kellerza kellerza deleted the jksma branch November 4, 2018 17:09
@kellerza kellerza mentioned this pull request Nov 4, 2018
7 tasks
kellerza added a commit to kellerza/ha-core that referenced this pull request Nov 6, 2018
@balloob balloob mentioned this pull request Nov 9, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants