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

Geo Location component #15953

Merged
merged 33 commits into from Aug 30, 2018

Conversation

Projects
None yet
7 participants
@exxamalte
Contributor

exxamalte commented Aug 13, 2018

Description:

This introduces a new component for any geo-location related things. These are typically short-lived events in the real world like for example bushfires, earthquakes or severe weather events.
Since each external event has longitude/latitude associated, it will automatically be displayed on the map.
The entity class contained in the component can be extended by platforms if additional information is supplied by external sources. The entity's state is the distance of the event to the HA instance.
The demo platform generates 5 random events in the vicinity of the HA instance. Every minute one event will be replaced by a random new one.

Future enhancements (not included in this PR):

  • GeoRSS platform with several enhancements over the existing geo_rss_events sensor.
  • The use of zones could be used to automatically trigger automations, but this would require support for wildcards in the entity id configuration of the zone trigger, for example entity_id: 'geo_location.nsw_fire_service_*'
  • It may be nice to display an icon instead of the abbreviated title of each external event on the map.
  • If the external event is defined by a polygon (not just a single point) it may be nice to show that polygon on the map. Example: bushfire affecting a large area of land.
  • Deprecate and remove the GeoRSS sensor when all its use-cases are covered by this new platform.

Related issue (if applicable): home-assistant/architecture#42

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

Example entry for configuration.yaml (if applicable):

geo_location:
  - platform: demo

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:

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.
  • Tests have been added to verify that the new code works.

exxamalte added some commits Jul 26, 2018

ability to define the desired state attribute and corresponding unit …
…of measurement; sort devices in group by configured state; find centroid for map if event is defined by polygon; updated tests
@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 13, 2018

Python 3.7 build fails because of a change in behaviour which is not yet covered in the feedparser library: kurtmckee/feedparser#131

@awarecan

This comment has been minimized.

Contributor

awarecan commented Aug 13, 2018

Please remove GeoRSS platform from this PR, and add a demo platform to explain how this component works.

GeoRSS platform should be in a separate PR.

@frenck frenck added the docs-missing label Aug 13, 2018

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 14, 2018

@awarecan: Adding a demo platform is a good idea. I'll do that now, and see how I can move the GeoRSS platform into a separate PR then.

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 16, 2018

New demo platform added. Will move the GeoRSS platform into a separate branch out of this PR next.

exxamalte added some commits Aug 20, 2018

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Aug 22, 2018

Seems something about the test could be flaky:
https://travis-ci.org/home-assistant/home-assistant/jobs/419285626#L1077

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 22, 2018

Yes, I saw the test result. This test works locally just fine.
In the travis log I can see that the _update method was called because it actually does remove one entity and adds a new one:

demo.py                     79 DEBUG    Removing <Entity Burn off: 45.6>
demo.py                     86 DEBUG    Adding <Entity Blizzard: 42.8>

Tracking the time interval is initiated in the init method, i.e. before the demo entities are generated. Is there a chance that the travis server is somewhat slower so that the test "misses" the tracked time somehow?

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 23, 2018

This time the test went through fine; not sure if that is just a coincidence or if the use of that helper method had any influence on that.

@exxamalte exxamalte referenced this pull request Aug 27, 2018

Merged

New geo location component with demo platform #6094

2 of 2 tasks complete
"""Return the state attributes of this external event."""
data = {}
if self.distance is not None:
data[ATTR_DISTANCE] = self.distance

This comment has been minimized.

@fabaff

fabaff Aug 28, 2018

Member

The distance is already available as state if one wants to use it for automations.

This comment has been minimized.

@exxamalte

exxamalte Aug 28, 2018

Contributor

The reason why there is a separate distance property is because the distance is probably a useful default state for most use-cases in this component. However, depending on the nature of the external event, other attributes may be more important and used as the state of an entity. Example: If the external event is an earthquake, the magnitude becomes much more relevant.

In a separate branch I have already built a GeoRSS platform where the user can choose in the configuration what state the entities should have.

This comment has been minimized.

@exxamalte

exxamalte Aug 28, 2018

Contributor

Or, are you saying that I should remove the distance from the state_attributes because it is already available as state, and if a platform decides to expose something other than the distance as state then this platform could add the distance as an attribute?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

It's not up to the platform to decide what the state property should return. The component decides this.

This comment has been minimized.

@exxamalte

exxamalte Aug 28, 2018

Contributor

Alright, and how does that relate to for example the sensor component. There are a lot of platforms in that component that return plenty of different things - temperature, humidity, luminance, number of Fedex deliveries, ...?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

The sensor component doesn't have a specific state definition. All other components have a specific state definition.

if self.latitude is not None:
data[ATTR_LATITUDE] = self.latitude
if self.longitude is not None:
data[ATTR_LONGITUDE] = self.longitude

This comment has been minimized.

@fabaff

fabaff Aug 28, 2018

Member

The attributes should be rounded when they end up on the frontend. Instead of 15 decimal places for the coordinates let's limit it to 5 or so.

This comment has been minimized.

@exxamalte

exxamalte Aug 28, 2018

Contributor

OK. I agree - 5 decimal places leaves us with an accuracy of about 1 metre. That should be sufficient.

exxamalte added some commits Aug 28, 2018

"Earthquake", "Tsunami"]
def setup_platform(hass, config, add_devices, discovery_info=None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

Rename add_devices to add_entities. We've renamed that function.

This comment has been minimized.

@exxamalte

exxamalte Aug 28, 2018

Contributor

OK.

assert state_first_entry.attributes['unit_of_measurement'] == \
DEFAULT_UNIT_OF_MEASUREMENT
# Update (replaces 1 device).
fire_time_changed(self.hass, dt_util.utcnow() +

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

I think we should patch homeassistant.util.dt.utcnow before we fire the time changed event, to make sure that time is correct. I think the lack of that was the reason for the flaky test fail. See test_platform_not_ready in tests/helpers/test_entity_component.py for example.

This comment has been minimized.

@exxamalte

exxamalte Aug 28, 2018

Contributor

That sounds like a good idea as it should give us more control over the test conditions.

renamed add_devices to add_entities; tweaked test to gain more contro…
…l over the timed update in the demo platform
assert state_first_entry.attributes['unit_of_measurement'] == \
DEFAULT_UNIT_OF_MEASUREMENT
# Update (replaces 1 device).
fire_time_changed(self.hass, dt_util.utcnow() +

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 29, 2018

Member

Use the cached time value here utcnow.

This comment has been minimized.

@exxamalte

exxamalte Aug 29, 2018

Contributor

OK

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 29, 2018

Stay tuned, I think the test fails because the order of entries retrieved via hass.states.entity_ids(geo_location.DOMAIN) does not necessarily relate to the order in which the entities have been created.
Maybe I need to check all elements of the before-array and after-array to ensure that one entry must be different.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Aug 29, 2018

Yes, you're correct. States are stored in a dictionary which isn't ordered in Python 3.5, by default. So when calling for the entity ids list, the order isn't necessarily as the order of entity creation.

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 29, 2018

Thanks for the explanation. I'm using Python 3.6 locally which may explain why it always worked here. I modified the test and just compare the whole list of states now.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Aug 30, 2018

For completeness, we should add a test that adds an entity that returns None for distance, latitude and longitude, to cover those cases as well.

@MartinHjelmare

Great!

@MartinHjelmare MartinHjelmare merged commit f20a331 into home-assistant:dev Aug 30, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 93.765%
Details

@wafflebot wafflebot bot removed the in progress label Aug 30, 2018

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Aug 30, 2018

Thanks, I'll get on with the GeoRSS platform now, and try to keep it as minimal as possible in the first instance.

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Geo Location component (home-assistant#15953)
* initial working version of a geo location component and georss platform

* ensure that custom attributes don't override built-in ones

* bugfixes and tests

* fixing tests because of introduction of new component using same fixture

* improving test cases

* removing potentially unavailable attribute from debug message output

* completing test suite

* cleaning up debug messages; sorting entries in group view by distance

* ability to define the desired state attribute and corresponding unit of measurement; sort devices in group by configured state; find centroid for map if event is defined by polygon; updated tests

* sort entries in group; code clean-ups

* fixing indentation

* added requirements of new component and platform

* fixed various lint issues

* fixed more lint issues

* introducing demo geo location platform; refactored geo location component and geo rss platform to fit

* removing geo rss events platform; added unit tests for geo location platform and demo platform

* reverting change in debug message for feedreader to avoid confusion with new geo location component

* updated requirements after removing georss platform

* removed unused imports

* fixing a lint issue and a test case

* simplifying component code; moving code into demo platform; fixing tests

* removed grouping from demo platform; small refactorings

* automating the entity id generation (the use of an entity namespace achieves the same thing)

* undoing changes made for the georss platform

* simplified test cases

* small tweaks to test case

* rounding all state attribute values

* fixing lint; removing distance from state attributes

* fixed test

* renamed add_devices to add_entities; tweaked test to gain more control over the timed update in the demo platform

* reusing utcnow variable instead of patched method

* fixed test by avoiding to make assumptions about order of list of entity ids

* adding test for the geo location event class

@exxamalte exxamalte referenced this pull request Sep 13, 2018

Merged

GeoJSON platform #16610

7 of 7 tasks complete

@balloob balloob referenced this pull request Sep 17, 2018

Merged

0.78.0 #16666

@exxamalte exxamalte referenced this pull request Sep 23, 2018

Merged

NSW Rural Fire Service platform #16802

7 of 7 tasks complete

@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.