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 sighthound integration #28824

Merged
merged 12 commits into from Jan 23, 2020
Merged

Add sighthound integration #28824

merged 12 commits into from Jan 23, 2020

Conversation

robmarkcole
Copy link
Contributor

@robmarkcole robmarkcole commented Nov 17, 2019

Description:

Adds integration for person detection with Sighthound cloud. The Sighthound Developer tier (free for non-commercial use) allows 5000 requests per month. If you need more requests per month you will need to sign up for a production account (i.e. Basic or Pro account).

This integration adds an image processing entity where the state of the entity is the number of people detected in an image. For each person detected, an image_processing.person_detected event is fired. The event data includes the entity_id of the image processing entity firing the event, and the bounding box around the detected person. Note that in order to prevent accidentally using up your requets to Sighthound, by default the component will not automatically scan images, but requires you to call the image_processing.scan service e.g. using an automation triggered by motion. Alternativley, periodic scanning can be enabled by configuring a scan_interval.

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#11203

Example entry for configuration.yaml (if applicable):

image_processing:
  - platform: sighthound
    api_key: some_key
    source:
      - entity_id: camera.my_cam

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • 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.

@robmarkcole
Copy link
Contributor Author

OK CI is failing because simplehound has python_requires=">=3.7", do I need to make this dependency backwards compatible to py 36?

@MartinHjelmare MartinHjelmare changed the title Adds sighthound Add sighthound Nov 17, 2019
@frenck
Copy link
Member

frenck commented Nov 17, 2019

We currently still support 3.6, so that requires an integration to work with Python 3.6 as well. However, we are in the process of moving away from 3.6, now Python 3.8 has become stable.

IMHO, there are 2 options. Either make it Python 3.6 compatible or hold off a bit until we've officially dropped Python 3.6.

@robmarkcole
Copy link
Contributor Author

Made python 3.6 compatible

@springstan springstan changed the title Add sighthound Add sighthound integration Jan 15, 2020
@robmarkcole
Copy link
Contributor Author

The commit history is a bit of a mess now, does this matter?

@MartinHjelmare
Copy link
Member

History looks ok. 👍


_LOGGER = logging.getLogger(__name__)

EVENT_PERSON_DETECTED = "image_processing.person_detected"
Copy link
Member

Choose a reason for hiding this comment

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

We should prefix the event with the integration name. Replace image_processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

DEV = "dev"
PROD = "prod"

SCAN_INTERVAL = timedelta(days=365) # NEVER SCAN.
Copy link
Member

Choose a reason for hiding this comment

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

Set should_poll property to False instead in the entity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

)


def setup_platform(hass, config, add_devices, discovery_info=None):
Copy link
Member

Choose a reason for hiding this comment

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

Please rename add_devices to add_entities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

entities = []
for camera in config[CONF_SOURCE]:
sighthound = SighthoundEntity(
config.get(CONF_API_KEY),
Copy link
Member

Choose a reason for hiding this comment

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

Please use dict[key] for required config keys and keys with default config schema values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


def __init__(self, api_key, account_type, camera_entity, name):
"""Init."""
super().__init__()
Copy link
Member

Choose a reason for hiding this comment

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

We can remove call to parent. It doesn't have an init method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

def __init__(self, api_key, account_type, camera_entity, name):
"""Init."""
super().__init__()
self._api = hound.cloud(api_key, account_type)
Copy link
Member

Choose a reason for hiding this comment

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

Does this have side effects? We don't want side effects in init method.

We should check that the api key is ok before creating the entity.

Probably create the api instance before creating the entity and pass the api to the entity.

Don't we want to create a single api instance and pass the same instance to all entities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the API doesn't have an endpoint to validate credentials, so the integration will raise an exception if invalid credentials only when processing is performed. One option is to perform processing of a test image to see if credentials are valid? Any other suggestions? In the meantime I will raise this with the API provider

Copy link
Member

Choose a reason for hiding this comment

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

Processing a mock image would be ok, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

if name:
self._name = name
else:
self._camera_name = split_entity_id(camera_entity)[1]
Copy link
Member

Choose a reason for hiding this comment

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

Camera name doesn't have to be an instance attribute since it's only used in this method. Just make it a local variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Dev automation moved this from Needs review to Review in progress Jan 19, 2020
@MartinHjelmare
Copy link
Member

Dev automation moved this from Review in progress to Reviewer approved Jan 22, 2020
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!

@robmarkcole
Copy link
Contributor Author

Are PR merged automatically?

@MartinHjelmare
Copy link
Member

No, it's done manually. Thanks for the ping. 👍

@MartinHjelmare MartinHjelmare merged commit c71ae09 into home-assistant:dev Jan 23, 2020
Dev automation moved this from Reviewer approved to Done Jan 23, 2020
@lock lock bot locked and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants