vinteo / hass-opensprinkler

OpenSprinkler Integration for Home Assistant
MIT License
204 stars 40 forks source link

Add rain delay service #269

Closed curlydingo closed 8 months ago

curlydingo commented 8 months ago

Caveat: It's my first time, please be gentle.

Adds a rain delay service to the integration per #251. Modelled largely off the existing service to set the water level.

Tested in a HA dev container against my local opensprinkler instance and works well, BUT I had to do some extra filtering of the EntityPlatforms in order to be able to call the service at all, which the rest of the code doesn't do. At the moment, in my dev environment none of those other services can be called. The specific change is in the second commit.

At first glance it seems like a change from HA has caused a need for this but I can't seem to find it. Grateful for any help that can be offered, but until we figure this out I'm not sure it should be merged in its current state.

Second caveat: The DE and SK translations are literally just an online translator's output of EN; not sure how you'd like these to be done properly?

curlydingo commented 8 months ago

FYI my dev environment is a Dev Container in VSCode based on HA/Core/dev. No other integrations installed.

vinteo commented 8 months ago

I think if the translations are missing it should just default to EN? so just omit them for now

EdLeckert commented 8 months ago

services.yaml

The standard unit_of_measurement for hours is "h", per .../homeassistant/const.py

# Time units ... TIME_HOURS = "h"

EdLeckert commented 8 months ago

@curlydingo, can you better explain what you're trying to accomplish with the filtering code that Vincent points out above. The selector in services.yaml limits the user's choice of entities to sensor types already. Now, some of those entities will work, and some won't, but they're all sensors in any case.

It appears that the reason the code is throwing an error is that async_get_platforms returns a list[EntityPlatform], whereas your filtering logic tries to pass sensor_platform.entities, a dict[str, Entity], to the service call. This actually works:

        await hass.helpers.service.entity_service_call(
            [sensor_platform], SERVICE_SET_RAIN_DELAY, call
        )

However, I'm not convinced we need this filtering.

In my test environment, Set water level and Set rain delay both work with the change above. But they also work with the pattern used by Set water level.

Incidentally, this issue of having to guess which entities will work with services that are really device services is something I've been meaning to look into, so don't feel you have to solve it today.

curlydingo commented 8 months ago

HI @EdLeckert, @vinteo, thanks for looking in to this.

TLDR; The code that @EdLeckert gave in his response does not work for me - it fails with the stack trace below. So something must be different about our setups. Am I on a too-new version of HA's devcontainer somehow?

Firstly, I should highlight that I didn't really want any extra filtering code in the first place, but without it the service call fails. It seems that _get_permissible_entity_candidates, which HA calls at some stage during the service call, requires a dict[str, Entities] in order to do its checks. Stack trace here:

_get_permissible_entity_candidates (\workspaces\core\homeassistant\helpers\service.py:781) entity_service_call (\workspaces\core\homeassistant\helpers\service.py:828) _async_send_set_rain_delay_command (\workspaces\core\config\custom_components\opensprinkler__init__.py:148) _execute_service (\workspaces\core\homeassistant\core.py:2221) async_call (\workspaces\core\homeassistant\core.py:2184) run (\workspaces\core\homeassistant\runner.py:188) main (\workspaces\core\homeassistant__main__.py:209)

(hass:8) At line 781 of service.py there is a call to entities.get, which fails if 'entities' is a list rather than a dict. I get the same exception for services I haven't changed, including setting the water level, so I'm fairly confused. The code in the pull request is basically just what I came up with to choose an appropriate dict[str, Entities] from the list that async_get_platforms provides, but I don't think any of us like it! (At least until we can figure out why it's necessary.)
curlydingo commented 8 months ago

Found it, I think.

Given the upstream change, should I add similar filtering to the other service calls? Happy to take advice on how to do that - the way I've written it seems a bit non-pythony (it's not my strongest language).

Alternatively, we implement a solution that only provides a single entity to the service call, since as @EdLeckert mentions it doesn't seem like we really care about the entity in these cases anyway - they're really services for the device.

EdLeckert commented 8 months ago

I just upgraded my production system to HA 2024.1.2 and Set water level works fine.

curlydingo commented 8 months ago

Yes, the commit I linked doesn't seem to be in 2024.1.2. I've set my dev environment to that release (instead of core's dev branch) and it works fine without the extra filtering. I've removed it from this PR since the issue doesn't need to be solved yet.

sonarcloud[bot] commented 8 months ago

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

vinteo commented 8 months ago

Thanks @curlydingo