Irrigation sensor #728
Irrigation sensor #728
Conversation
| sensors = [ | ||
| WyzeSensor(sensor_service, sensor) | ||
| for sensor in await sensor_service.get_sensors() | ||
| ] |
There was a problem hiding this comment.
I think if we are re-enabling the binary sensor platform we need to disable all the other binary sensors that are here and only add the irrigation one. I think the issue was over-polling with all these sensors so that's why the whole platform was disabled.
There was a problem hiding this comment.
Yeah let's disable these other sensors here and on Line 81 so that users who have these devices don't see them suddenly show up.
| async def async_added_to_hass(self) -> None: | ||
| """Subscribe to updates and set up periodic updates.""" | ||
| # Set up periodic updates every minute for zone running status | ||
| self._unsub_periodic = async_track_time_change( | ||
| self.hass, | ||
| self._async_periodic_update, | ||
| second=0 # Update every minute at second 0 | ||
| ) | ||
| return await super().async_added_to_hass() |
There was a problem hiding this comment.
This is probably more of a question for @SecKatie, but I'm thinking instead of adding a specific time interval sensor, shouldn't we be adding this to the update manager like we do for all the other platforms? For continuity and that would keep the total update interval in check if lots of devices are being updated.
There was a problem hiding this comment.
Really we should be moving to the UpdateCoordinator pattern. For this change I am fine with this as is unless @FeatherKing wants to take a stab at building an UpdateCoordinator for us. My only other recommendation would be for us to make it a longer delay. Something like 2-5 minutes should be better.
There was a problem hiding this comment.
I'd have to look into it more, but I'm not sure an update coordinator would work well in our case. It's more designed for a single endpoint to retrieve one big data dump and then send it to all the devices that use it. In our case we hit so many endpoints with specific device info in each call that it might take a crazy number of updaters.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Wyze irrigation zone running binary sensors to track when individual zones are active. It extends the existing binary sensor platform to include irrigation device monitoring capabilities.
- Adds new
WyzeIrrigationZoneRunningbinary sensor class for monitoring irrigation zone status - Updates platform configuration to include binary_sensor in the supported platforms list
- Implements periodic polling every 60 seconds to check zone running status
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| custom_components/wyzeapi/binary_sensor.py | Adds irrigation service integration and WyzeIrrigationZoneRunning binary sensor class |
| custom_components/wyzeapi/init.py | Adds "binary_sensor" to the PLATFORMS list |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| self._unsub_periodic = async_track_time_change( | ||
| self.hass, | ||
| self._async_periodic_update, | ||
| second=0 # Update every minute at second 0 | ||
| ) |
There was a problem hiding this comment.
Polling every 60 seconds for all irrigation zones could create significant API load when multiple zones exist. Consider implementing a shared update mechanism or allowing configurable polling intervals to reduce API calls and potential rate limiting issues.
| except Exception as e: | ||
| _LOGGER.error("Failed to update zone running status for device %s zone %s: %s", | ||
| self._device.mac, self._zone_number, str(e)) | ||
| self._running = False |
There was a problem hiding this comment.
Catching generic Exception is too broad. Consider catching more specific exceptions related to API calls or network issues to avoid masking unexpected errors that should bubble up.
SecKatie
left a comment
SecKatie
left a comment
There was a problem hiding this comment.
Looks great overall. One little comment about the update method that you can choose to address or not.
| async def async_added_to_hass(self) -> None: | ||
| """Subscribe to updates and set up periodic updates.""" | ||
| # Set up periodic updates every minute for zone running status | ||
| self._unsub_periodic = async_track_time_change( | ||
| self.hass, | ||
| self._async_periodic_update, | ||
| second=0 # Update every minute at second 0 | ||
| ) | ||
| return await super().async_added_to_hass() |
There was a problem hiding this comment.
Really we should be moving to the UpdateCoordinator pattern. For this change I am fine with this as is unless @FeatherKing wants to take a stab at building an UpdateCoordinator for us. My only other recommendation would be for us to make it a longer delay. Something like 2-5 minutes should be better.
|
im still planning to implement this. i took a look at the updatecoordinator and that is probably more that i can commit to at the moment. this has been working fine at home for a month so ill increase the sensor duration and call it good for now |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
3 participants
related to SecKatie/wyzeapy#156
working locally
currently this is polling every 60 seconds. ive seen the wyze api do rate limiting but i dont know how fast you can hit them