Xiaomi Cameras - multiple models by vaidyasr · Pull Request #14244 · home-assistant/core

vaidyasr · 2018-05-02T15:56:20Z

Description:

Adds support for Xiaomi Cameras running the alternate firmwares.

Related issue (if applicable): N/A

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5290

Example entry for `configuration.yaml` (if applicable):

camera:
  - platform: xiaomi
    name: Hall Camera
    host: '192.168.1.100'
    model: 'xiaofang'
    username: root
    password: my_password_123
    ffmpeg_arguments: '-vf scale=800:450'

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

Documentation added/updated in home-assistant.github.io

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

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

houndci-bot · 2018-05-02T15:56:29Z

+
+        return 'ftp://{0}:{1}@{2}:{3}{4}/{5}/{6}/{7}'.format(
+            self.user, self.passwd, self.host, self.port, self.path,
+            first_dir,latest_dir, videos[-2])


missing whitespace after ','

houndci-bot · 2018-05-02T15:56:29Z

+        return DEFAULT_BRAND
+
+    def get_latest_video_url(self):
+        """Retrieve the latest video file from the customized Xiaofang FTP server."""


line too long (85 > 79 characters)

dgomes · 2018-05-02T21:24:57Z

I saw your previous PR (#14238), and I'm wandering: how different is this camera from the Yi ?

The code is almost identical, since they are from the same vendor can we have a single platform that covers both cameras with a configuration option indicating the model ?

vaidyasr · 2018-05-03T04:53:30Z

The folder structure is different than yi model. The folder structure to retrieve in yi model is 2018Y05M01D01H/41M00S.mp4 where as in xiaofang it is 20180501/01/41.mp4. And the latest file writing by the camera is not recognizable by the ffmpeg, so the program has to read last but one file, which is 1 minute lag of live video.

dgomes · 2018-05-03T08:44:41Z

I think those differences could fit in the same platform and we would avoid the duplication of much of the 90% of the code...

vaidyasr · 2018-05-03T09:07:14Z

I agree, will try to modify the yi.py code and test it on my end before publishing.

dgomes · 2018-05-03T09:09:44Z

Please create a new camera platform "xiaomi" that merges both.

But leave "Yi" alone for now, so we don't break users install right away.

vaidyasr · 2018-05-03T09:56:43Z

I don't get it, this pull is a new platform, how to do it?

dgomes · 2018-05-03T10:00:18Z

$ git mv xiaofang.py xiaomi.py and edit the platform to encompass both Yi and Xiaofang cameras

Added Xiaomi Camera to accommodate multiple models like Yi, Xiaofang, etc.

dgomes

Add a test and please make sure the test covers both models.

dgomes · 2018-05-03T12:05:24Z

-            _LOGGER.error('Unable to find path: %s', first_dir)
-            _LOGGER.debug(exc)
-            return False
+        if self._model == 'xiaofang':


Define constants at the beginning of the file for all the fixed strings

you mean to store 'xiaofang' and 'yi' as constants?

dgomes · 2018-05-03T12:05:43Z

+                ftp.cwd(first_dir)
+            except error_perm as exc:
+                _LOGGER.error('Unable to find path: %s', first_dir)
+                _LOGGER.debug(exc)


Don't double log

I think both logging are different, 1st one is level 1 and next is level 2.

yet both will log the same issue, just leave one of them

but 1st will report if 20180503 directory exists and 2nd will report if 15 directory exists, there may be cases where the 2nd level will not be there when the camera option was set to only record when there is an motion detection.

they will both appear when log level is debug, and since they are in the same except clause they are reporting the same issue.

I do have both models and tested both and worked fine.

Have a look at existing tests:

https://github.com/home-assistant/home-assistant/tree/dev/tests/components/camera

create a new file for your platform

Do I need to create a test?. I don't see it was created for yi already. And it is just a few modification over yi which was already released.

Yi camera had no options.

I would mock the FTP server, and validade the existence of the image file in a different path depending on the configured model.

This camera has same options like Yi camera, and am not sure how to do testing, as I am not an expert in python. The code of this one is same as Yi Camera, except one directory above.

dgomes · 2018-05-03T12:36:54Z

+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Required(CONF_NAME): cv.string,
+    vol.Required(CONF_HOST): cv.string,
+    vol.Required(CONF_MODEL): cv.string,


Need to validate the model

dgomes · 2018-05-03T12:40:55Z

+        self._manager = hass.data[DATA_FFMPEG]
+        self._name = config.get(CONF_NAME)
+        self.host = config.get(CONF_HOST)
+        self._model = config.get(CONF_MODEL)


Required configuration parameters should be retrieved with config[CONF_PARAMETER]

Could you please let me know how to do it?

config[CONF_MODEL] and alike for name and host

only for required options and options with default value

dgomes · 2018-05-03T15:48:57Z

+                ftp.cwd(first_dir)
+            except error_perm as exc:
+                _LOGGER.error('Unable to find path: %s', first_dir)
+                _LOGGER.debug(exc)


You should also implement:

@property def model(self): """Return the camera model.""" return None

vaidyasr

Fixed the code per review

dgomes · 2018-05-03T21:58:12Z

+            ftp.login(self.user, self.passwd)
+        except error_perm as exc:
+            _LOGGER.error('There was an error while logging into the camera')
+            _LOGGER.debug(exc)


Still the double logging issue... just merge the two

Even though double logging is already there in some of the official released codes, am fixing it

vaidyasr · 2018-05-04T08:49:34Z

I don't understand the failure of continuous-integration. The version of attrs library conflict it seems, can someone confirm.

dgomes · 2018-05-04T09:01:35Z

There is currently a PR #14281 related to the issue, wait until it gets merged and then rebase.

vaidyasr · 2018-05-07T10:55:35Z

I have seen the PR has been merged, how do I rebase? please help.

dgomes · 2018-05-07T11:08:46Z

There is no need to rebase (CI is already passing)

What is currently missing is a test so coverage doesn't decrease.

vaidyasr

Changes implemented.

MartinHjelmare · 2018-05-29T08:37:50Z

+_LOGGER = logging.getLogger(__name__)
+
+DEFAULT_BRAND = 'Xiaomi Home Camera'
+DEFAULT_PASSWORD = ''


This isn't used.

MartinHjelmare · 2018-05-29T08:38:19Z

+                               async_add_devices,
+                               discovery_info=None):
+    """Set up a Xiaomi Camera."""
+    _LOGGER.debug('Received configuration: %s', config)


Don't log the config.

MartinHjelmare · 2018-05-29T08:42:15Z

+        try:
+            ftp.login(self.user, self.passwd)
+        except error_perm as exc:
+            _LOGGER.error('There was an error while logging: %s', exc)


Write at least: "... while logging in:".

Or rephrase to eg: "Camera login failed:".

MartinHjelmare · 2018-05-29T08:43:55Z

+                return False
+
+        if self._model == MODEL_XIAOFANG:
+            dirs = [d for d in ftp.nlst() if '.' not in d]


This is the same for both models. Can it be moved up?

Changes made per comment

houndci-bot · 2018-06-01T07:57:00Z

+
+
+        dirs = [d for d in ftp.nlst() if '.' not in d]
+          if not dirs:


IndentationError: unexpected indent
indentation is not a multiple of four
unexpected indentation

houndci-bot · 2018-06-01T07:57:00Z

+            return False
+
+
+        dirs = [d for d in ftp.nlst() if '.' not in d]


too many blank lines (2)

houndci-bot · 2018-06-01T07:57:01Z

+            _LOGGER.error('Unable to find path: %s - %s', self.path, exc)
+            return False
+
+


blank line contains whitespace

disrupted · 2018-06-18T01:07:32Z

@vaidyasr is this supposed to work with the https://github.com/samtap/fang-hacks cfw? do I need to configure anything there since it looks like this component works with mjpeg whereas fang-hacks by default opens a rtsp stream?

vaidyasr · 2018-06-19T01:33:27Z

This component works with saved mp4 streams and not the live rtsp stream. For live rtsp stream, you may need to use the ffmpeg camera component.

balloob · 2018-06-25T15:35:20Z

I didn't realize that this was for alternative firmware. In that case, the integration should be called after the alternative firmware, not the product.

vaidyasr added 2 commits May 2, 2018 21:18

Added support for Xiaofang Camera

89b2dd9

Added entry for Xiaofang 1080p camera

7599e77

homeassistant added new-platform cla-signed platform: camera.xiaofang labels May 2, 2018

houndci-bot reviewed May 2, 2018

View reviewed changes

Code fix

7e6fcec

homeassistant added the cla-signed label May 2, 2018

vaidyasr mentioned this pull request May 3, 2018

Added Documentation for Xiaomi Cameras home-assistant/home-assistant.io#5290

Merged

2 tasks

Minor comment fix

9319778

vaidyasr added 2 commits May 3, 2018 17:16

Updated coveragerc for Xiaomi cameras

31c6f67

Added Xiaomi Camera

564daa0

Added Xiaomi Camera to accommodate multiple models like Yi, Xiaofang, etc.

vaidyasr changed the title ~~Xiaofang 1080p Camera~~ Xiaomi Cameras - multiple models May 3, 2018

dgomes requested changes May 3, 2018

View reviewed changes

Minor code fix

bdab2e0

homeassistant added the cla-signed label May 3, 2018

dgomes reviewed May 3, 2018

View reviewed changes

Minor code fix

3c40007

dgomes reviewed May 3, 2018

View reviewed changes

Added model property

79567f4

vaidyasr commented May 3, 2018

View reviewed changes

dgomes reviewed May 3, 2018

View reviewed changes

Update package_constraints.txt

2fccf01

Minor fix

aba4dfd

Update xiaomi.py

bc00d3f

vaidyasr commented May 8, 2018

View reviewed changes

MartinHjelmare reviewed May 29, 2018

View reviewed changes

Update xiaomi.py

d4be965

Changes made per comment

houndci-bot reviewed Jun 1, 2018

View reviewed changes

Update xiaomi.py

fa91354

dgomes added the almost-done label Jun 7, 2018

dgomes mentioned this pull request Jun 13, 2018

Make Yi platform async #14944

Merged

5 tasks

Don't update on add.

bd22fe1

homeassistant added the cla-signed label Jun 15, 2018

ghost assigned balloob Jun 15, 2018

ghost added in progress and removed almost-done labels Jun 15, 2018

balloob approved these changes Jun 15, 2018

View reviewed changes

balloob merged commit c917470 into home-assistant:dev Jun 15, 2018

ghost removed the in progress label Jun 15, 2018

home-assistant locked and limited conversation to collaborators Jun 19, 2018

balloob added platform: camera.xiaomi and removed platform: camera.xiaofang labels Jun 22, 2018

ghost added integration: xiaomi and removed platform: camera.xiaomi labels Mar 21, 2019

		_LOGGER.error('Unable to find path: %s - %s', self.path, exc)
		return False

Uh oh!

Conversation

vaidyasr commented May 2, 2018 • edited by fabaff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaidyasr commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgomes commented May 3, 2018

Uh oh!

vaidyasr commented May 3, 2018

Uh oh!

dgomes commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaidyasr commented May 3, 2018

Uh oh!

dgomes commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgomes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaidyasr commented May 2, 2018 •

edited by fabaff

Loading

Example entry for `configuration.yaml` (if applicable):

dgomes commented May 2, 2018 •

edited

Loading

vaidyasr commented May 3, 2018 •

edited

Loading

dgomes commented May 3, 2018 •

edited

Loading

dgomes commented May 3, 2018 •

edited

Loading

dgomes left a comment •

edited

Loading

dgomes May 3, 2018 •

edited

Loading

dgomes May 3, 2018 •

edited

Loading

dgomes May 3, 2018 •

edited

Loading

vaidyasr left a comment •

edited

Loading