Skip to content
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

Add PTZ support to Foscam camera component #27238

Merged
merged 10 commits into from
Oct 7, 2019
Merged

Add PTZ support to Foscam camera component #27238

merged 10 commits into from
Oct 7, 2019

Conversation

skgsergio
Copy link
Contributor

@skgsergio skgsergio commented Oct 5, 2019

Description

This PR extends the current foscam component to support PTZ through a service (like the onvif component).

I also took this opportunity to update the module for using the async API.

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#10625

Demo

Kapture 2019-10-06 at 13 31 29

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @skgsergio,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all the side effects out of the entity init method.

homeassistant/components/camera/services.yaml Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
 - Move service to foscam domain
 - Use `dict[key]` for required schema keys or with defaults
 - Fix sync operations in async context
 - Remove excessive logging
@skgsergio
Copy link
Contributor Author

Addresed all issues and re-tested. Not sure the procedure here, should I squash?

homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
homeassistant/components/foscam/camera.py Outdated Show resolved Hide resolved
@skgsergio
Copy link
Contributor Author

Moved al initialization to async_setup_platform now __init__ doesn't contain logic.
Moved constants to const.
I saw the motion detection logic wasn't ok so I fixed it too in this commit.
Addressed the rest of the comments.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@MartinHjelmare
Copy link
Member

Can be merged when build passes.

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please exclude homeassistant/components/foscam/const.py from our test coverage by adding it to .coveragerc.

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@frenck
Copy link
Member

frenck commented Oct 7, 2019

Thank you for your contribution thus far! 🎖 One final thing: Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

@skgsergio
Copy link
Contributor Author

Done 👍 and thank everyone for the project and the awesome help with the PR (next time I'll have the guidelines more clear 😂)

@frenck
Copy link
Member

frenck commented Oct 7, 2019

Well, the opposite is the case: Thank you sir! 🎉

@frenck frenck merged commit f6b8cff into home-assistant:dev Oct 7, 2019
@skgsergio skgsergio deleted the add-ptz-to-foscam branch October 7, 2019 12:37
@lock lock bot locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants