Simplisafe motion event capture and outdoor camera support #109024
Conversation
home-assistant
bot
added
cla-signed
integration: simplisafe
new-feature
new-platform
labels
|
Hey there @bachya, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Looks like you accidentally pulled in other changes. Please rebase this branch off of the latest |
$ git rebase --continue
dropping e1795b2eddbd99d86702bdd872b1d88d902454c6 update simplisafe-python version to 2024.01.0 for motion capture support -- patch contents already upstream
dropping 8f23b9ff691c7ab339899e94396c11dbd1170b44 sync with proper EVENT_DISARMED -- patch contents already upstream
dropping 94c45b00504e8d3f478a855048fed5f5d6641a43 add a base class for cameras that support motion, and an OutdoorCamera that uses this base class. -- patch contents already upstream
dropping ea767df7ccd7c32243e52b641915a0ad336c8804 update simplisafe-python version, coverage section -- patch contents already upstream
Successfully rebased and updated refs/heads/simplisafe-outdoor-camera-support.$ git status
On branch simplisafe-outdoor-camera-support
Your branch and 'origin/simplisafe-outdoor-camera-support' have diverged,
and have 324 and 21 different commits each, respectively.
(use "git pull" to merge the remote branch into yours)
nothing to commit, working tree cleanSo, should I pull, and then commit? I am pretty sure this is what I did before. |
You should force-push this branch after the rebase—that will ensure the unrelated commits are removed. |
|
Done: $ git push origin simplisafe-outdoor-camera-support --force
Enumerating objects: 52, done.
Counting objects: 100% (52/52), done.
Delta compression using up to 12 threads
Compressing objects: 100% (43/43), done.
Writing objects: 100% (43/43), 6.69 KiB | 6.69 MiB/s, done.
Total 43 (delta 36), reused 0 (delta 0)
remote: Resolving deltas: 100% (36/36), completed with 9 local objects.
To github.com:peebles/core.git
+ 0398b13bff...469eabc8a2 simplisafe-outdoor-camera-support -> simplisafe-outdoor-camera-support (forced update) |
|
I get a CI failure for "docs-missing". How do I resolve this? I have a PR (home-assistant/home-assistant.io#31110) for the documentation that is open. Does that just need to be approved? |
|
Do I need to do anything to wrap this up? |
|
Your PR is in draft, which means it's not ready for review—that's why you haven't gotten interaction. If you're wanting a review, remove it from draft status and I can take a look. |
| async def async_media_request(self, url: str) -> bytes | None: | ||
| """Fetch a media file.""" | ||
| content = await self._api.async_media(url) | ||
| if content is None: |
There was a problem hiding this comment.
Is this necessary? Can we just return content?
There was a problem hiding this comment.
Because if I just "return content" mypy says: error: Returning Any from function declared to return "bytes | None" [no-any-return]
There was a problem hiding this comment.
And in simplisafe-python (_api) the function is declared:
async def async_media(self, url: str) -> bytes | None:
So I don't understand why mypy is freaking out.
|
|
||
| for camera in system.cameras.values(): | ||
| if camera.camera_type == CameraTypes.OUTDOOR_CAMERA: | ||
| cameras.append( |
There was a problem hiding this comment.
This can be simplified via a comprehension:
cameras = [
SimpliSafeOutdoorCamera(hass, simplisafe, system, camera)
for camera in system.cameras.values()
if camera.camera_type == CameraTypes.OUTDOOR_CAMERA
]| return self._device.name | ||
|
|
||
| @callback | ||
| def async_unload(self) -> None: |
There was a problem hiding this comment.
Where does this get called?
There was a problem hiding this comment.
Does HA call this at some point? I believe I found this sort of thing in other components, so I adopted it here too. If HA does not call it, then I'll remove it ... because I certainly don't call it.
| self, width: int | None = None, height: int | None = None | ||
| ) -> bytes | None: | ||
| """Return bytes of camera image.""" | ||
| if self._attr_cached_image is not None: |
There was a problem hiding this comment.
Is this ever reset? Are we always returning a cached image once it's been set?
There was a problem hiding this comment.
It gets set to None in async_update_from_websocket_event() when a new motion event arrives.
| return None | ||
|
|
||
| self._attr_cached_image = await self._simplisafe.async_media_request( | ||
| self._attr_image_url.replace("{&width}", "&width=" + str(width)) |
There was a problem hiding this comment.
This seems odd. Why not just construct the entire URL when you need it?
There was a problem hiding this comment.
This is what the urls look like from SS. I do not know the desired width (from a user's automation) until now.
| schema=SERVICE_OC_IMAGE_SCHEMA, | ||
| ) | ||
|
|
||
| self._hass.services.async_register( |
There was a problem hiding this comment.
Loop through these service registration calls (since they're almost the same).
There was a problem hiding this comment.
Only DOMAIN is common. The other 3 (of 4) are unique. Seems to me the loop would be complicated.
| return | ||
|
|
||
| width = call.data.get(ATTR_WIDTH) | ||
| filename: Template = call.data.get(ATTR_FILENAME) # type: ignore[assignment] |
There was a problem hiding this comment.
Why are we disabling mypy here?
There was a problem hiding this comment.
Because if I don't I get: error: Incompatible types in assignment (expression has type "Any | None", variable has type "Template") [assignment]
| if self._attr_clip_url is None: | ||
| return | ||
|
|
||
| filename: Template = call.data.get(ATTR_FILENAME) # type: ignore[assignment] |
There was a problem hiding this comment.
Why are we disabling mypy here?
There was a problem hiding this comment.
Because I get: error: Incompatible types in assignment (expression has type "Any | None", variable has type "Template") [assignment]
| def _register_services(self) -> None: | ||
| """Register the Outdoor Camera hass service calls.""" | ||
|
|
||
| def _write_image(to_file: str, image_data: bytes) -> None: |
There was a problem hiding this comment.
All these nested functions are challenging to follow. What about just including them in the entity?
There was a problem hiding this comment.
Because they need the reference to self, and wouldn't get it when called from HA as services? I copied this pattern from other components, thinking this is the way to do it.
|
I've been following this PR and the associated conversations in the other thread closely, so thank you in advance @peebles and @bachya for working on adding this functionality. I'm noticing the code still refers to outdoor cameras, will this not work with indoor ones? I have one and would be happy to test the code at some point if helpful. |
|
I am getting this error when trying to commit: usage: pylint [options] and I see /workspaces/core/pylint/plugins/hass_enforce_type_hints.py:# be checked by pylint when --ignore-missing-annotations is False |
* Add more sensors * Fix coverage * Dont do this rename yet * Fix case * Update snapshot * Add icons * Remove unused icons * Update snapshot * Remove last_value logic from TimeSensor * Apply suggestions from code review Co-authored-by: G Johansson <[email protected]> * Update constant case * Remove useless test * Add refresh test back * Add assertion to post coordinator refresh --------- Co-authored-by: G Johansson <[email protected]>
Co-authored-by: jan iversen <[email protected]>
cdce8p
removed request for
ludeeus,
pvizeli,
davet2001,
Ernst79,
Kane610,
edenhaus,
cdce8p,
mib1185,
uvjustin,
chemelli74 and
gjohansson-ST
jbouwh
removed request for
fabaff,
bdraco,
robbiet480,
frenck,
marvin-w,
dgomes,
ctalkington,
scop,
Jc2k,
bieniu,
elupus,
OverloadUT,
StevenLooman,
michaelarnauts and
Danielhiversen
|
Please open a new PR with a fresh branch. This branch has unrelated commits. |
Proposed change
This PR works in conjunction with version 2024.01.0 of simplisafe-python and adds HA support for capturing motion event media files from Simplisafe cameras that support it. In particular, support for the Simplisafe outdoor camera is included.
Type of change
Additional information
Originally inspired by bachya/simplisafe-python#85.
A new
SimplisafeMotionCamerahas been added; a base class that handles motion events from Simplisafe by saving media file urls and providing services for fetching and saving those media files. A newSimplisafeOutdoorCamerahas been added that leverages this base class.Changelog for simplisafe-python library: https://github.com/bachya/simplisafe-python/pull/708/files
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..coveragerc.To help with the load of incoming pull requests: