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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split timer service for Sensibo #73684
Conversation
Hey there @andrey-git, mind taking a look at this pull request as it has been labeled with an integration ( |
@bdraco would be happy if you take a look at this one as it's related to the previous two. |
Not a breaking change since the other PR isn't released |
@gjohansson-ST Did you forget to commit the |
Somehow I managed to discard those changes. Thanks! |
Does it make sense to change the It could set a sensible default time when you turned it on, and they could continue use the timer service to set a custom time? |
That's not a bad idea as it would have state and you can operate it and leave only one custom service to set a different time than "standard". Do you think 30 minutes sounds reasonable? |
Personally I'd set it for an hour, but I'm not the user here so either is fine. |
It's moved to a switch now and it looks good I think. |
async def async_turn_on(self, **kwargs: Any) -> None: | ||
"""Turn the entity on.""" | ||
new_state = bool(self.device_data.ac_states["on"] is False) | ||
params = { | ||
"minutesFromNow": 60, | ||
"acState": {**self.device_data.ac_states, "on": new_state}, | ||
} | ||
result = await self.async_send_command("set_timer", params) | ||
|
||
if result["status"] == "success": | ||
return await self.coordinator.async_request_refresh() | ||
raise HomeAssistantError(f"Could not enable timer for device {self.name}") | ||
|
||
async def async_turn_off(self, **kwargs: Any) -> None: | ||
"""Turn the entity off.""" | ||
result = await self.async_send_command("del_timer") | ||
|
||
if result["status"] == "success": | ||
return await self.coordinator.async_request_refresh() | ||
raise HomeAssistantError(f"Could not disable timer for device {self.name}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the plan is to also do this for the pure_boost
it might make sense to define these in the entity description
These is a bit of state flip-flopping and the sensor going unavailable in testing. When I turn it on sometimes it stays on, but sometimes it flips back to off, and then flips back to on. I'm not sure if its caused by the coordinator refreshing before the api as I did not investigate further, but if it is something like
|
It's because I set the coordinator to request a refresh instead of instant refresh as in good practice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing looks good.
Thanks @gjohansson-ST
if result["status"] == "success": | ||
setattr(self.device_data, self.entity_description.remote_key, True) | ||
self.async_write_ha_state() | ||
return await self.coordinator.async_request_refresh() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method shouldn't return anything.
self.entity_description.command_on, params | ||
) | ||
|
||
if result["status"] == "success": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd invert the check and raise if true. Then we can outdent below.
Proposed change
Split the timer service for sensibo into two services
sensibo.enable_timer
andsensibo.disable_timer
Make it a bit more user friendly
Previous PR: #73072
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: