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

WebDAV camera #22607

Closed
wants to merge 15 commits into from
Closed

WebDAV camera #22607

wants to merge 15 commits into from

Conversation

jkeljo
Copy link
Contributor

@jkeljo jkeljo commented Apr 1, 2019

Breaking Change:

None

Description:

As suggested by @dgomes in #19758, here's a camera backed by image files on
a WebDAV share. The entity exposes a stream and individual images, iterating
through the images in sorted order by filename at a configurable rate.

Related issue (if applicable): N/A

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: webdav
    name: file_server
    host: 'https://server.name'
    image_interval: '1:00'

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.

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

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 or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

As suggested by @dgomes in home-assistant#19758, here's a camera backed by image files on
a WebDAV share. The entity exposes a stream and individual images, iterating
through the images in sorted order by filename at a configurable rate.
Copy link
Contributor

@dgomes dgomes left a comment

Choose a reason for hiding this comment

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

Maybe instead of the wait_and_advance() you could use async_track_time_change from homeassistant.helpers.event

@awarecan
Copy link
Contributor

awarecan commented Apr 4, 2019

It is harsh time for new integration as we are doing refactoring, we are now requesting all components has to have a manifest file.

Please rebase you branch against current dev branch, and create a manifest.json file under your component's folder.

You may need to do more rebase in the next few days as we are still in the progress of refactoring.

@jkeljo
Copy link
Contributor Author

jkeljo commented Apr 7, 2019

@dgomes, thanks for the pointer! I see async_track_time_interval in that same file, which seems to be more what I want.

@awarecan, no worries, I know how it goes. I can roll with it. :-)

@codecov

This comment has been minimized.

@dgomes
Copy link
Contributor

dgomes commented Apr 18, 2019

Can you please rebase ? There several changes in the core that must be tested with this component before any merge.

@MartinHjelmare
Copy link
Member

@jkeljo are you planning to continue here?

@jkeljo
Copy link
Contributor Author

jkeljo commented Jul 14, 2019 via email

@MartinHjelmare
Copy link
Member

@jkeljo please close, since then you can open this PR again yourself, when you have time, and we can keep the comment history in one place.

@balloob balloob closed this Jul 15, 2019
@balloob balloob reopened this Jul 15, 2019
@balloob
Copy link
Member

balloob commented Jul 15, 2019

oops, waiting for @jkeljo to close, so he can also re-open it, compared to when I close it.

@jkeljo jkeljo closed this Jul 15, 2019
@jkeljo jkeljo reopened this Aug 17, 2019
@codecov

This comment has been minimized.

2 similar comments
@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

I think I was thinking in another language, since this was not valid Python. Result was that the thread that maintained the SocketIO connection to the table would die early on, so no status updates were actually flowing from the table.
@MartinHjelmare MartinHjelmare added this to Needs review in Dev Sep 23, 2019
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.

Be wary of mixing sync and async calls in the same context. Coroutines should be awaited in async context and blocking (sync) I/O is not allowed in async context.

This component models a WebDAV share full of images as a camera.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/camera.webdav/
Copy link
Member

Choose a reason for hiding this comment

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

We don't add the url to the docstring anymore. Just keep the very first line.

"""

import logging

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this blank line. I recommend the isort tool to automatically sort and group imports.


import asyncio
from datetime import timedelta
from aiohttp import ClientError
Copy link
Member

Choose a reason for hiding this comment

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

Please add a blank line between standard library and 3rd party import groups.

"webdav_root": path,
}
)
session = async_get_clientsession(hass)
Copy link
Member

Choose a reason for hiding this comment

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

This should be called in async context. Currently we're in sync context.

SCAN_INTERVAL = timedelta(minutes=15)


def setup_platform(hass, config, add_entities, 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.

Change def setup_platform to async def async_setup_platform.

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_entities to async_add_entities.

update_before_add=True,
)
except WebDavException as exception:
_LOGGER.warning("Failed to connect to %s: %s", client.get_url(""), exception)
Copy link
Member

Choose a reason for hiding this comment

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

Does client.get_url make I/O?

while images_checked < len(self._files):
self._image_number += 1
images_checked += 1
content_type = self._client.get_property(
Copy link
Member

Choose a reason for hiding this comment

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

This makes I/O so needs to be scheduled on the executor thread pool. Use await self.hass.async_add_executor_job(...).

"""Return True if the camera is playing through files in the share."""
return self._stop_advancing is not None

def turn_on(self):
Copy link
Member

Choose a reason for hiding this comment

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

This should be a coroutine and rename it to async_turn_on.

self.hass, self._advance, self._image_interval
)

def turn_off(self):
Copy link
Member

Choose a reason for hiding this comment

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

As above.

async def async_added_to_hass(self):
"""Set up periodic image advancement."""
self._image_lock = asyncio.Lock()
self.turn_on()
Copy link
Member

Choose a reason for hiding this comment

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

await self.async_turn_on()

Dev automation moved this from Needs review to Review in progress Nov 1, 2019
@balloob
Copy link
Member

balloob commented Nov 26, 2019

This PR seems to have gone stale. Closing it.

@balloob balloob closed this Nov 26, 2019
Dev automation moved this from Review in progress to Cancelled Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

9 participants