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

Camera component for BOM integration #22816

Merged
merged 12 commits into from Apr 9, 2019

Conversation

Projects
None yet
7 participants
@maddenp
Copy link
Contributor

commented Apr 7, 2019

Description:

This PR adds a camera component that delivers animated GIF loops of weather-radar imagery from the Australian Bureau of Meteorology to the existing bom integration.

This replaces #21183, which I fouled up with a bad merge from master instead of dev.

Example entry for configuration.yaml:

camera:
  - platform: bom
    location: Sydney

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. (The code passes individual flake8, pylint, and pydocstyle tests as described in Testing outside of Tox.
  • 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:

  • The manifest file has all fields filled out correctly (example).
  • New dependencies have been added to requirements in the manifest (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.

maddenp added some commits Apr 5, 2019

@maddenp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

@MartinHjelmare I've addressed all your requests from #21183, except for the one about using vol.Inclusive. The required configuration constraints are:

  1. id and location are mutually exclusive, but one or the other must be specified.
  2. If (and only if) id is specified, delta and frames are required; otherwise they are optional.
  3. name and filename are always optional.

I was able to use strictly vol.Exclusive and vol.Inclusive to enforce the constraints, but only with a lot of unwanted duplication, and with misleading messages from the validator when there are configuration errors. I'm happy to try further if you can suggest an explicit solution, but the current code enforces the requirements correctly with useful error messages. (I was able to use vol.Exclusive to good effect, so thanks for that suggestion.)

@codecov

This comment has been minimized.

Copy link

commented Apr 7, 2019

Codecov Report

Merging #22816 into dev will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22816      +/-   ##
==========================================
+ Coverage   93.82%   93.83%   +<.01%     
==========================================
  Files         448      448              
  Lines       36529    36524       -5     
==========================================
- Hits        34275    34271       -4     
+ Misses       2254     2253       -1
Impacted Files Coverage Δ
homeassistant/components/hassio/ingress.py 71.05% <0%> (-0.26%) ⬇️
homeassistant/components/stream/worker.py 93.68% <0%> (+0.06%) ⬆️
homeassistant/components/hassio/http.py 88.23% <0%> (+0.17%) ⬆️
homeassistant/components/axis/camera.py 91.66% <0%> (+1.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c17b2f...68cd487. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Apr 7, 2019

Codecov Report

Merging #22816 into dev will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22816      +/-   ##
==========================================
+ Coverage   93.82%   93.83%   +<.01%     
==========================================
  Files         448      448              
  Lines       36529    36524       -5     
==========================================
- Hits        34275    34272       -3     
+ Misses       2254     2252       -2
Impacted Files Coverage Δ
homeassistant/components/hassio/ingress.py 71.05% <0%> (-0.26%) ⬇️
homeassistant/components/stream/worker.py 93.68% <0%> (+0.06%) ⬆️
homeassistant/components/hassio/http.py 88.23% <0%> (+0.17%) ⬆️
homeassistant/components/uk_transport/sensor.py 94.16% <0%> (+0.72%) ⬆️
homeassistant/components/axis/camera.py 91.66% <0%> (+1.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c17b2f...acfd0bf. Read the comment docs.

@frenck

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

IMHO, this should be part of the bom integration, which is already present.

Creating a new integration and suffixing it with camera will just defeat the great migration we have recently done.

@maddenp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

That seems sensible, though just a few days ago, in my previous PR, @MartinHjelmare specifically wrote

Move, git mv, the camera platform under a new bomradarcam component package. That's our new way of structuring modules.
homeassistant/components/bomradarcam/__init__.py
homeassistant/components/bomradarcam/camera.py

So please let me know if you're specifically requesting that change before this PR can be merged, or just airing an idea. If it's the former, can you please be more specific about how the code (or maybe it's mostly the files) should be restructured to make that happen? I have very limited HA experience and would be nervous modifying an existing, working component.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

We don't put standalone platforms under entity components, like camera, anymore. That's why I asked to move the platform from under the camera component.

If sounds good to put this platform under the bom component.

homeassistant/components/bom/camera.py
@maddenp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

I've moved it -- looks like a cleaner solution. I'll go update the docs now.

@MartinHjelmare
Copy link
Member

left a comment

Looks good!

Can be merged when the PR description is updated after the module move, the docs PR is linked in the PR description and the build passes.

@maddenp maddenp changed the title Camera platform for animated Australian BOM radar imagery Camera component for BOM integration Apr 7, 2019

@maddenp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Fixed PR titles and descriptions (both here and in home-assistant.io), updated docs, and linked to the docs PR in the description here.

@cgarwood cgarwood merged commit 88694c9 into home-assistant:dev Apr 9, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 8c17b2f...acfd0bf
Details
codecov/project 93.83% (target 90%)
Details

@ghost ghost removed the in progress label Apr 9, 2019

@DavidFW1960

This comment has been minimized.

Copy link

commented Apr 18, 2019

  1. id and location are mutually exclusive, but one or the other must be specified.
  2. If (and only if) id is specified, delta and frames are required; otherwise they are optional.
  3. name and filename are always optional.

Just a note: I am using a radar that is not a 'standard' one so used the ID - but IF the ID has a leading 0.. like 034 for example, you need to enclose it in quotes. '034' otherwise it won't be read or work (black box)

@maddenp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@DavidFW1960 Thanks for pointing this out. The documentation PR hadn't been merged yet, so I updated the example using id/delta/frames to use a quoted id string, which had previously been a missed opportunity to make it clear that the id value needs to be (or, at least, parse to) a string.

I believe the YAML literal 034 is being parsed as an octal integer to decimal 28. But I'm surprised that the configuration validation does not complain, as id is declared to be a string. I must have done something wrong in the validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.