Add config flow to Amcrest #80453

flacjacket · 2022-10-17T03:12:59Z

Proposed change

Setup the Amcrest integration with config flow to be able to configure new Amcrest camera entities. Include import logic to update and deprecate existing yaml configurations.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Update amcrest docs to use config flow home-assistant.io#24593

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

Setup the Amcrest integration with config flow to be able to configure new Amcrest camera entities. Include import logic to update and deprecate existing yaml configurations.

iMicknl

I had a quick look and some minor remarks to get you started.

homeassistant/components/amcrest/__init__.py

homeassistant/components/amcrest/binary_sensor.py

homeassistant/components/amcrest/config_flow.py

Co-authored-by: Mick Vleeshouwer <[email protected]>

homeassistant/components/amcrest/translations/en.json

.coveragerc

tests/components/amcrest/test_config_flow.py

homeassistant/components/amcrest/config_flow.py

home-assistant · 2023-03-22T15:43:57Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

github-actions · 2023-07-28T02:17:46Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

…onfig_flow

stale

bdraco · 2023-09-09T14:16:53Z

somewhat related. There is currently a pretty significant performance issue with the lib: tchellomello/python-amcrest#231

bdraco · 2023-09-09T14:19:42Z

homeassistant/components/amcrest/binary_sensor.py

-        key=_AUDIO_DETECTED_POLLED_KEY,
-        name=_AUDIO_DETECTED_NAME,
+        key=AUDIO_DETECTED_POLLED_KEY,
+        name=AUDIO_DETECTED_NAME + " Polled",


Please use f-strings

bdraco · 2023-09-09T14:19:46Z

homeassistant/components/amcrest/binary_sensor.py

-        key=_CROSSLINE_DETECTED_POLLED_KEY,
-        name=_CROSSLINE_DETECTED_NAME,
+        key=CROSSLINE_DETECTED_POLLED_KEY,
+        name=CROSSLINE_DETECTED_NAME + " Polled",


Please use f-strings

bdraco · 2023-09-09T14:19:50Z

homeassistant/components/amcrest/binary_sensor.py

-        key=_MOTION_DETECTED_POLLED_KEY,
-        name=_MOTION_DETECTED_NAME,
+        key=MOTION_DETECTED_POLLED_KEY,
+        name=MOTION_DETECTED_NAME + " Polled",


Please use f-strings

bdraco · 2023-09-09T14:20:41Z

homeassistant/components/amcrest/binary_sensor.py

        device_class=BinarySensorDeviceClass.SOUND,
-        event_codes=_AUDIO_DETECTED_EVENT_CODES,
+        event_codes=AUDIO_DETECTED_EVENT_CODES,
        should_poll=True,


Why are there polling and non-polling sensors?

Not for this PR, but can the polling ones be disabled by default?

bdraco · 2023-09-09T14:21:44Z

homeassistant/components/amcrest/camera.py

@@ -181,6 +176,7 @@ def __init__(self, name: str, device: AmcrestDevice, ffmpeg: FFmpegManager) -> N
        self._channel = device.channel
        self._token = self._auth = device.authentication
        self._control_light = device.control_light
+        self._attr_unique_id = device.serial_number


Can there be multiple channels?

bdraco · 2023-09-09T14:22:33Z

homeassistant/components/amcrest/sensor.py

+    sensors = config_entry.options.get(CONF_SENSORS, [])
+    if sensors:
+        name = config_entry.data[CONF_NAME]
+        device = hass.data[DOMAIN][DEVICES][config_entry.entry_id]


please type device

bdraco · 2023-09-09T14:22:58Z

homeassistant/components/amcrest/camera.py

-    device = hass.data[DATA_AMCREST][DEVICES][name]
+    """Set up the camera entity."""
+    name = config_entry.data[CONF_NAME]
+    device = hass.data[DOMAIN][DEVICES][config_entry.entry_id]


please type device

bdraco · 2023-09-09T14:24:14Z

homeassistant/components/amcrest/__init__.py

+    control_light: bool = entry.options.get(CONF_CONTROL_LIGHT, DEFAULT_CONTROL_LIGHT)
+
+    hass.data.setdefault(DOMAIN, {DEVICES: {}, CAMERAS: []})
+    hass.data[DOMAIN][DEVICES][entry.entry_id] = AmcrestDevice(


Usually we store data by

hass.data[DOMAIN][entry.entry_id]

What you have is ok but I'd change DEVICES to ENTRIES since it looks like config entry data

bdraco · 2023-09-09T14:25:20Z

homeassistant/components/amcrest/__init__.py

+            hass,
+            DOMAIN,
+            "deprecated_yaml",
+            breaks_in_ha_version=None,


6 versions from now

https://github.com/home-assistant/architecture/blob/master/adr/0021-YAML-integration-configuration-deprecation-policy.md#context

bdraco · 2023-09-09T14:25:42Z

homeassistant/components/amcrest/binary_sensor.py

+MOTION_DETECTED_KEY = "motion_detected"
+MOTION_DETECTED_POLLED_KEY = "motion_detected_polled"
+MOTION_DETECTED_NAME = "Motion Detected"
+MOTION_DETECTED_EVENT_CODE = "VideoMotion"


Do all these need to be renamed? Can we leave the _ to make this merge smaller?

bdraco · 2023-09-09T14:27:55Z

homeassistant/components/amcrest/config_flow.py

+        vol.Optional(CONF_BINARY_SENSORS, default=[]): cv.multi_select(
+            {sensor.key: sensor.name for sensor in BINARY_SENSORS}
+        ),
+        vol.Optional(CONF_SENSORS, default=[]): cv.multi_select(
+            {sensor.key: sensor.name for sensor in SENSORS}
+        ),
+        vol.Optional(CONF_CONTROL_LIGHT, default=DEFAULT_CONTROL_LIGHT): bool,


We should not allow them to select entities in the options flow. We should use the standard functionality of HA to enable/disable entities.

Instead set entity_registery_enabled_default=True using the old yaml so when they migrate, entities are there but disabled.

github-actions · 2023-12-08T10:05:15Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

bdraco · 2023-12-08T20:56:15Z

@flacjacket would you like to continue this PR?

flacjacket · 2023-12-14T02:35:32Z

@bdraco Thanks for the ping on this, I'll try to get it back up-to-date and get the fixes in from the previous review this weekend

github-actions · 2024-02-12T16:06:29Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

home-assistant bot added cla-signed config-flow This integration migrates to the UI by adding a config flow deprecation Indicates a breaking change to happen in the future Hacktoberfest has-tests integration: amcrest new-feature by-code-owner Quality Scale: No score labels Oct 17, 2022

flacjacket force-pushed the config_flow branch from 4b31cb6 to 8c65bde Compare October 17, 2022 03:17

flacjacket mentioned this pull request Oct 17, 2022

Update amcrest docs to use config flow home-assistant/home-assistant.io#24593

Closed

8 tasks

flacjacket force-pushed the config_flow branch from 8c65bde to d1f65b5 Compare October 17, 2022 03:36

Add config flow for amcrest

5aacfb4

Setup the Amcrest integration with config flow to be able to configure new Amcrest camera entities. Include import logic to update and deprecate existing yaml configurations.

flacjacket force-pushed the config_flow branch from d1f65b5 to 5aacfb4 Compare October 17, 2022 03:48

iMicknl reviewed Oct 17, 2022

View reviewed changes

homeassistant/components/amcrest/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/amcrest/binary_sensor.py Show resolved Hide resolved

homeassistant/components/amcrest/config_flow.py Outdated Show resolved Hide resolved

Allow for recovery from errors to connect to camera

c0d930f

flacjacket requested a review from iMicknl October 17, 2022 19:10

Merge branch 'dev' into config_flow

ed95382

flacjacket force-pushed the config_flow branch from e7007f8 to ed95382 Compare October 24, 2022 14:52

frenck added the hacktoberfest-accepted label Oct 30, 2022

flacjacket and others added 3 commits November 3, 2022 11:31

Merge branch 'dev' into config_flow

eb63651

Update homeassistant/components/amcrest/config_flow.py

d79b099

Co-authored-by: Mick Vleeshouwer <[email protected]>

Merge branch 'dev' into config_flow

16909ac

peter-funktionIT mentioned this pull request Jan 3, 2023

Add binary sensor for CrossRegion to Amcrest #85011

Closed

19 tasks

epenet previously requested changes Mar 22, 2023

View reviewed changes

home-assistant bot marked this pull request as draft March 22, 2023 15:43

epenet changed the title ~~Add config flow setup for Amcrest component~~ Add config flow to Amcrest Apr 7, 2023

github-actions bot added the stale label Jul 28, 2023

flacjacket added 2 commits July 30, 2023 22:33

Merge branch 'dev' into config_flow

9279f42

Merge remote-tracking branch 'refs/remotes/github/config_flow' into c…

bc65e5e

…onfig_flow

github-actions bot removed the stale label Jul 31, 2023

flacjacket added 2 commits September 9, 2023 08:34

Merge branch 'dev' into config_flow

4cf6ef0

Fixes based on review comments

ea19d57

bdraco reviewed Sep 9, 2023

View reviewed changes

github-actions bot added the stale label Dec 8, 2023

github-actions bot removed the stale label Dec 8, 2023

github-actions bot added the stale label Feb 12, 2024

github-actions bot closed this Feb 19, 2024

home-assistant bot removed the Hacktoberfest label Feb 19, 2024

github-actions bot locked and limited conversation to collaborators Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config flow to Amcrest #80453

Add config flow to Amcrest #80453

flacjacket commented Oct 17, 2022 •

edited

Loading

iMicknl left a comment

home-assistant bot commented Mar 22, 2023

github-actions bot commented Jul 28, 2023

bdraco commented Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

bdraco Sep 9, 2023

github-actions bot commented Dec 8, 2023

bdraco commented Dec 8, 2023

flacjacket commented Dec 14, 2023

github-actions bot commented Feb 12, 2024

Add config flow to Amcrest #80453

Add config flow to Amcrest #80453

Conversation

flacjacket commented Oct 17, 2022 • edited Loading

Proposed change

Type of change

Additional information

Checklist

iMicknl left a comment

Choose a reason for hiding this comment

home-assistant bot commented Mar 22, 2023

github-actions bot commented Jul 28, 2023

bdraco commented Sep 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 8, 2023

bdraco commented Dec 8, 2023

flacjacket commented Dec 14, 2023

github-actions bot commented Feb 12, 2024

flacjacket commented Oct 17, 2022 •

edited

Loading