Add Switch entity for One Time Charge #108
Conversation
|
After testing around with PyViCare I can confirm that if you enable or disable the feature and then read the state almost simultaneously it can lead to inaccuracies. However, I have observed that a waiting time of one to two seconds is enough to get the expected response. Therefore, I would suggest that it first internally sets the value to the desired value, provided that no error was issued and then after one to two seconds waiting time it updates using the api |
|
@PaulProjects thanks for the feedback. I am testing the changes on my local setup, will push those changes (plus some logging refinement) later today! Edit, could you kindly test again @PaulProjects ? |
|
hey @lukx first of all thanks for the contribution. I kinda missed this PR and have started some minor work on the onetime charge as well. In general I think your PR is good but please have a look at my changes to the button entity: I check for a PyViCareNotSupportedFeatureError before creating the button to keep it from showing up for users who don't have the functionality since that's very irritating. Could we do the same for the switch entity. Other than that we should probably create this PR in the Home Assistant repository first. |
I guess we need to see how the HA Core devs feel about "assuming state". I personally think the switch is the way to go and we should get rid of the button in a separate PR in an upcoming release |
… deactivate-onetimecharge
|
Hi @lukx |
|
Thanks gor updating the PR. |
| async def async_update(self): | ||
| """update internal state""" | ||
| now = datetime.datetime.utcnow() | ||
| """we have identified that the API does not directly sync the represented state, therefore we want to keep |
There was a problem hiding this comment.
we might change this behavior in the future. I think we should trigger an update after sending a command on the API. But for now (since we don't have that mechanism yet) let's keep this
| """Return true if device is on.""" | ||
| return self._state | ||
|
|
||
| async def async_update(self): |
There was a problem hiding this comment.
I don't think we should make this method async. 99% of the work in this method needs to be in an executor job anyway.
Also we could more easily retrieve the initial state (which we have in PyVicare's Cache anyway during startup) by replacing self._state = None in line 123 with update()
| self._device_config = device_config | ||
| self._api = api | ||
| self._ignore_update_until = datetime.datetime.utcnow() | ||
| self._state = None |
There was a problem hiding this comment.
Please replace self._state = None in with update() like in the other integrations. This way we can also write tests which assert the state.
See comment below
There was a problem hiding this comment.
@lukx Can you please have a look at this change request? - I'm using your custom repo at the moment to control the heating of water when the solar panels are producing too much, but seems I can't log-in anymore using your repo. (401: Token missing. Could be unrelated) - But I still think it'd be better to get this integrated in the official repo :)
I've sent you a PR with the change, have a look and just approve :)
Thanks!
Implemented and tested:
Note: Enabling/Disabling currently does trigger a scheduled update to the API. However it looks like the return value of the API still shows the old state, making it a bit clumpsy: The switch gets disabled again, and then enabled after the next Poll.
I have not yet found out how to fix this beautifully, but wanted to share the progress.
Missing also a statement by @oischinger whether we should mark the "button" entity as deprecated somehow?
fixes #96