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

Make rest sensor and binary sensor more efficient #14484

Merged
merged 8 commits into from
Sep 21, 2018

Conversation

exxamalte
Copy link
Contributor

@exxamalte exxamalte commented May 16, 2018

Description:

I am proposing the following small changes to the rest sensor and rest binary sensor:

  • Currently, if the initial rest call fails when setting up a rest binary sensor that binary sensor will not be created. I propose to create the binary sensor anyway and keep trying fetching the rest resource to update the status. The component is already capable of handling the case where the rest resource is unavailable.
  • Currently, both - rest sensor and rest binary sensor - fetch the rest resource twice during the setup. Once manually, and then again through the add_devices call. I propose to initially only make one call to fetch the rest resource. For the binary sensor I propose to initially only make one call to fetch the rest resource - the sensor state can be determined based on that.
  • I also fixed up several test assertions that aren't currently working as expected. The tests pass but the assertions aren't working as intended.

Related issue (if applicable): n/a

Pull request in home-assistant.github.io with documentation (if applicable): n/a

Example entry for configuration.yaml (if applicable):

n/a

Checklist:

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


if rest.data is None:
_LOGGER.error("Unable to fetch REST data from %s", resource)
return False
Copy link
Member

Choose a reason for hiding this comment

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

We should raise PlatformNotReady. Keep in mind that the platform can't recover if the credentials or header entries are wrong. Then the users will end-up with non-functional platforms in their setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review - I see what you mean.
I am hoping to find a way to distinguish between recoverable and unrecoverable errors. Once the sensor is correctly set up (correct URL, correct username/password, etc.), it would be nice if the code could handle intermittent errors. At present, if a third-party service used in a rest binary sensor is unavailable just at the time when HA is restarted, that binary sensor is not created.
BTW: A rest sensor is still created in the same situation. And a rest switch is only not created if the HTTP response code is >= 400 or if specific exceptions occur; this may be a role model for sensor and binary sensor?
I'll look into PlatformNotReady.

Copy link
Contributor Author

@exxamalte exxamalte May 17, 2018

Choose a reason for hiding this comment

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

Alright, I wasn't aware that when raising a PlatformNotReady HA automatically retries after 30 seconds (with backoff). That definitely helps with intermittent errors during the setup of the sensors.

@exxamalte
Copy link
Contributor Author

exxamalte commented May 17, 2018

@fabaff: As suggested I changed the setup of the sensors to raise PlatformNotReady if the rest call does not return data. However this approach requires to fetch the rest resource within the setup method. To keep avoiding fetching that same resource again immediately after the setup I introduced throttling access to the RestData#update method to 10 seconds.

@syssi
Copy link
Member

syssi commented May 18, 2018

@exxamalte
Copy link
Contributor Author

exxamalte commented May 18, 2018

@syssi: That approach would work for the rest binary sensor. However, if the rest sensor isn't updated immediately it remains in an unknown state(i.e. self._state remains STATE_UNKNOWN).
Furthermore, the reason that the above would work for the rest binary sensor is because it doesn't cache its state but calculates it every time is_on is called. Most sensors I have seen update their state in their update method and then store that value.

@exxamalte
Copy link
Contributor Author

exxamalte commented Jun 8, 2018

Are there any more suggestions on how to improve this PR or can it be merged please?

@syssi
Copy link
Member

syssi commented Jun 8, 2018

The problem of throttling the update method is you cannot use a SCAN_INTERVAL < 10 seconds anymore. :-(

@exxamalte
Copy link
Contributor Author

Yes, that is correct.
So, if update intervals <10 seconds are important, then how about the following compromise: I remove the throttling of the update and remove the immediate update of the rest binary sensor. This means that the rest sensor will still update twice during the initial setup.
In a next step (future PR) I will look at how to refactor the two sensors (and I found a few others that make use of the RestData class) in a way that makes their behaviour more consistent and avoids the two updates during setup.

@exxamalte exxamalte force-pushed the rest-binarysensor-enhancements branch from 157d2b1 to 5fa8175 Compare June 10, 2018 05:12
@exxamalte
Copy link
Contributor Author

@syssi: Is this now ready to be merged?

@exxamalte
Copy link
Contributor Author

Is there anything left to do for this PR, or can this be merged now?

@ghost ghost assigned balloob Sep 21, 2018
@balloob balloob merged commit d5813cf into home-assistant:dev Sep 21, 2018
@ghost ghost removed the in progress label Sep 21, 2018
@balloob balloob mentioned this pull request Sep 28, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
@exxamalte exxamalte deleted the rest-binarysensor-enhancements branch February 17, 2020 10:29
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.

5 participants