-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
Add support for "Indoor Only" mode on Sure Petcare pets/flaps #158073
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
base: dev
Are you sure you want to change the base?
Conversation
|
Hey there @benleb, @Danielhiversen, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
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.
Pull request overview
This PR adds support for the "Indoor Only" mode feature to the Sure Petcare integration, allowing users to configure pet flaps so that pets can enter but not exit. The implementation creates switch entities for each pet-flap combination, calls the appropriate API endpoint, and attaches switches to pet devices with entities disabled by default to reduce clutter.
Key Changes:
- New switch platform with
SurePetcareIndoorModeSwitchentity implementation - Added profile mode constants (PROFILE_INDOOR=3, PROFILE_OUTDOOR=2)
- Integration test coverage for entity creation and unique ID verification
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/surepetcare/__init__.py |
Added Platform.SWITCH to the PLATFORMS list to enable switch platform |
homeassistant/components/surepetcare/const.py |
Added PROFILE_INDOOR and PROFILE_OUTDOOR constants for pet access mode values |
homeassistant/components/surepetcare/switch.py |
New switch platform implementation with indoor mode control via API |
homeassistant/components/surepetcare/strings.json |
Added entity translation for indoor mode switch naming |
tests/components/surepetcare/__init__.py |
Extended mock data with tag profiles for cat and pet flaps |
tests/components/surepetcare/test_switch.py |
Added basic tests for switch entity creation and unique IDs |
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
a411cb3 to
80c9dd1
Compare
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
3423f43 to
9a057e7
Compare
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.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
9a057e7 to
9cf4b27
Compare
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.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
9cf4b27 to
b60b2df
Compare
|
@Danielhiversen / @benleb after various Copilot reviews, this is finally ready for review and would really appreciate it if you could find the time to take a look :) |
| # pet profile modes | ||
| PROFILE_INDOOR = 3 # indoor mode - pet can enter but cannot exit | ||
| PROFILE_OUTDOOR = 2 # outdoor/normal mode - pet can enter and exit |
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.
- are there more profiles?
- How does the app call this?
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.
The best API docs that exist for this vendor is from their Swagger docs for their Beta env. There is a schema for the profile (SpecialProfiles) however that just gives an array of ints with no description as to what these are ([ 0, 1, 2, 3, 4, 5, 6 ]]).
When viewing the requests made on their web app I cannot see any usage other than 2 (outdoor) or 3 (indoor) when you are marking a pet to be "inside only" for a given flap
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.
Hmm, do we have a way to discover other modes? Because if we find more we should make it a select entity instead
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 suspect that the remaining profiles are related to their other products (devices) such as their smart feeder and water fountain. They appear to use "tags" on each "device" to control certain features, one of which is the indoor only mode for their cat flap as implemented here.
If I can find some batteries for our feeder I can help confirm that. Though going through their app again for everything catflap related, it's only ever "2" or "3"
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.
What if you use their API to set it to something different?
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.
So long as you pass in an int from 0 to 6, you can successfully set that profile on the device. However, none of the other values have any impact on the Indoor Only mode. I have reached out to their support to see if they are able to provide any more info here, though I do suspect these other profile values are for other devices (not flaps)
| tags_by_id = { | ||
| tag.get("id"): tag for tag in surepy_entity.raw_data().get("tags", []) | ||
| } |
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.
Future improvement for the library, we should add this in the model in an easy way, as we're now looping over the tags for every door
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 agree with you here, see my comment around the general state of the library below
| await self.coordinator.surepy.sac.call( | ||
| method="PUT", | ||
| resource=f"{BASE_RESOURCE}/device/{self._flap.id}/tag/{self._pet.tag_id}", | ||
| json={"profile": profile}, | ||
| ) |
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.
We should move this to the library. in HA we have the rule that all device or service specific code should exist in the library. I would consider the method, endpoint and content to be service specific and thus it should belong in the library
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 agree, that is the best place for it. The reason I've implemented it this way is that the surepy library appears to no longer be maintained by the author, with the last release (v0.9.0) from 2 years ago and no real signs that development will continue. By doing it this way, we at least can get the functionality into Home Assistant whilst we work out how best to handle this situation going forwards.
I would like to breath some life into this integration (and therefore the standalone Python library as well), and that may involve forking benleb's repository to carry on his great work. If that looks like the road to go down, then I feel that would be better as a separate PR as it would require a change to manifest requirements.
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.
That would happen before this PR then, as I think this is too service specific. I sent benleb a message on Discord, sometimes he pops up. Do keep in mind that we don't want to change libraries often, so whenever we change it we should do our due diligence and do our best to keep the current library.
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.
Totally! Great lets see if we get a response, as it would be best to have this in the surepy lib
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.
@joostlek I have raised benleb/surepy#232 for the surepy library which implements the needed methods. If you're in contact with @benleb then I'd appreciate if you could let him know about that.
Once that gets reviewed, merged, and a new surepy release made, then updating this PR to use these methods would be trivial
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.
In the meantime, consider adding yourself as a codeowner and send me a message on Discord, when I was looking into this PR I saw some thing we could improve in the integration, so that could be a nice way to help in the meantime :)
| await self.coordinator.async_request_refresh() | ||
| mode = "indoor" if profile == PROFILE_INDOOR else "outdoor" | ||
| raise HomeAssistantError( | ||
| f"Failed to set {self._pet.name} {mode} mode on {self._flap.name}" | ||
| ) from err | ||
|
|
||
| # Update state immediately after successful API call | ||
| self._profile_id = profile | ||
| self._attr_is_on = profile == PROFILE_INDOOR | ||
| self.async_write_ha_state() |
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.
Why do we only refresh when it failed?
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.
My thinking was that if the API request is successful, then there is no need to make another API request to the data that we already know. A refresh will happen on the next polling interval (every 3 minutes for this integration).
On the failure state, I feel a refresh is required because we don't know what happened to the request, so fetching the latest data right away would bring it back in sync.
Happy to take your advice here though
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.
Did you ever get it to error out?
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.
Not in a real example, only via the tests. However I do see an issue as the surepy library doesn't raise any exception when a HTTP 5xx occurs for example. See https://github.com/benleb/surepy/blob/d4af3694b4201d05afc06ff46e61e586b7cc2fed/surepy/client.py#L244-L246
From what I can see, None would get returned. Looks like other calls would treat None as "something went wrong" and then throw SurePetcareError e.g. https://github.com/benleb/surepy/blob/d4af3694b4201d05afc06ff46e61e586b7cc2fed/surepy/client.py#L381
I can update this to throw SurePetcareError if the response to .call is None - just as the other methods do, if you agree? This feels like an area that could do with some attention in the actual surepy lib to address more cleanly
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.
we shouldn't throw exceptions other than ServiceValidationError (when the user has a skill issue) and HomeAssistantError (aka, we had a reason that it failed)
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 mean within the try block its self, if the response is None then throw SurePetcareError - that then gets caught by the existing except SurePetcareError which will ultimately raise a HomeAssistantError
This is what existing methods do as linked to above
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've made the above change to align with how the other SureAPIClient methods handle this
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
`surepy` library doesn't raise an exception on 5xx errors for example, but instead will return `None`. Other methods on the `SureAPIClient` treat `None` as an error, so we should do the same here.
Proposed change
This PR allows users to configure "Indoor Only" mode for their (cat) flaps, a feature which is part of the official Sure Petcare application and currently missing from this integration. When a pet is configured as "Indoor Only" they are unable to exit through the (cat) flap, however they can always enter again (should they escape through a window, for example).
This has been implemented by using a switch entity for each pet-flap combination, and calls the appropriate API endpoint using the surepy library. Each switch is attached to flap devices for logical grouping, and is disabled by default to reduce entity clutter.
Type of change
Additional information
Checklist
ruff format 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.To help with the load of incoming pull requests: