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.

Sign up for GitHub

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

Jump to bottom

Bom weather current component #3370

Merged
merged 13 commits into from
Sep 21, 2016
Merged

Bom weather current component #3370

merged 13 commits into from
Sep 21, 2016

Conversation

tinglis1
Copy link
Contributor

@tinglis1 tinglis1 commented Sep 13, 2016 •
edited

Description:
The Bureau of Meteorology (BOM) Australia is the most accurate source of weather information in Australia.

The bom_weather_current platform allows you to get the current weather conditions from the Bureau of Meteorology (BOM) Australia.

  • Each sensor will be given the device_id of "bom [optionalname] friendlyname units"
  • Get the station id for your local BOM station from BOM > State > Observations > Latest Observations > Choose the station > read the url
  • The url will look like "http://www.bom.gov.au/products/IDS60801/IDS60801.94675.shtml". This is for Adelaide.
  • The url is read as: "http://www.bom.gov.au/products/[zone_id]/[zone_id].[wmo_id].shtml"
  • A name is optional but if multiple bom weather stations are used a name will be required.
  • The sensor will update every minute 35 minutes after last data timestamp. This allows for the 30 minute observation cycle and the approximate 5 minute update delay in publishing the data.

Related issue (if applicable): fixes #
none known.

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

Example entry for configuration.yaml (if applicable):

sensor:
  platform: bom_weather_current
  name: "optional name"
  zone_id: IDS60801
  wmo_id: 94675
  monitored_conditions:
    - wmo
    - name
    - history_product
    - local_date_time
    - local_date_time_full
    - aifstime_utc
    - lat
    - lon
    - apparent_t
    - cloud
    - cloud_base_m
    - cloud_oktas
    - cloud_type_id
    - cloud_type
    - delta_t
    - gust_kmh
    - gust_kt
    - air_temp
    - dewpt
    - press
    - press_qnh
    - press_msl
    - press_tend
    - rain_trace
    - rel_hum
    - sea_state
    - swell_dir_worded
    - swell_height
    - swell_period
    - vis_km
    - weather
    - wind_dir
    - wind_spd_kmh
    - wind_spd_kt

Checklist:

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

If code communicates with devices, web services, or a:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • 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.

@tinglis1 tinglis1 mentioned this pull request Sep 13, 2016
Merged
@balloob
Copy link
Member

balloob commented Sep 14, 2016

Looks good. Can merge when linting issues addressed 👍

fabaff
fabaff reviewed Sep 19, 2016
View reviewed changes
homeassistant/components/sensor/bom_weather_current.py
Support for bom.gov.au current condition weather service.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/sensor.bom_weather_current
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 an additional / at the end of the URL.

fabaff
fabaff reviewed Sep 19, 2016
View reviewed changes
homeassistant/components/sensor/bom_weather_current.py
import logging
import requests
import voluptuous as vol
import datetime
Copy link
Member

Choose a reason for hiding this comment

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

Can you please update the ordering of the imports according PEP8?

fabaff
fabaff reviewed Sep 19, 2016
View reviewed changes
homeassistant/components/sensor/bom_weather_current.py
CONF_ZONE_ID = 'zone_id'
CONF_WMO_ID = 'wmo_id'

MIN_TIME_BETWEEN_UPDATES = datetime.timedelta(seconds=60)
Copy link
Member

Choose a reason for hiding this comment

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

Why running every minute when you have to wait at least 35 minutes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bom data updates at the hour and half hour. It then takes about 4 to 7 minutes for the information to propagate to the accessible api.

The sensor runs every minute until it gets the data. Then using the bom time-stamp of the last data it has received it waits 35 min before hitting the bom api again every minute until it gets new data.

The aim of this is to not hit the bom api for the 35min that there is no new data. If this works as intended there should only be a few api data requests every half hour period.

If i just make the delay 30min then if you start HA at 10min before the hour then you will always receive your data 20min late.

Copy link
Member

Choose a reason for hiding this comment

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

We are talking about weather data. Usually it doesn't matter if there is a delay. It's your call and it looks that you spent some thoughs on this.

fabaff
fabaff reviewed Sep 19, 2016
View reviewed changes
homeassistant/components/sensor/bom_weather_current.py
'cloud_type_id': ['Cloud Type ID', None],
'cloud_type': ['Cloud Type', None],
'delta_t': ['Delta Temp C', TEMP_CELSIUS],
'gust_kmh': ['Wind Gust kmh', 'kmh'],
Copy link
Member

Choose a reason for hiding this comment

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

km/h to be in alignment with the other platforms.

fabaff
fabaff reviewed Sep 19, 2016
View reviewed changes
homeassistant/components/sensor/bom_weather_current.py
'gust_kt': ['Wind Gust kt', 'kt'],
'air_temp': ['Air Temp C', TEMP_CELSIUS],
'dewpt': ['Dew Point C', TEMP_CELSIUS],
'press': ['Pressure mb', 'mb'],
Copy link
Member

Choose a reason for hiding this comment

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

Other weather platforms use the unit mbar.

fabaff
fabaff reviewed Sep 19, 2016
View reviewed changes
homeassistant/components/sensor/bom_weather_current.py
'vis_km': ['Visability km', 'km'],
'weather': ['Weather', None],
'wind_dir': ['Wind Direction', None],
'wind_spd_kmh': ['Wind Speed kmh', 'kmh'],
Copy link
Member

Choose a reason for hiding this comment

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

km/h dito

fabaff
fabaff reviewed Sep 19, 2016
View reviewed changes
homeassistant/components/sensor/bom_weather_current.py
def _build_url(self):
url = _RESOURCE.format(self._zone_id, self._zone_id, self._wmo_id)
_LOGGER.info("BOM url {}".format(url))
return url
Copy link
Member

Choose a reason for hiding this comment

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

Using a method to build an URL seems to be a bit overkill as you only using this once. Just an observation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so too. This process of building the url was borrowed from another component as I have tried to used what others are doing in components to keep the style in line with the project .

@fabaff
Copy link
Member

fabaff commented Sep 19, 2016

I would also suggest to rename the platform to bom. All other weather sensor platforms aren't post-fixed with _weather and the documentation can state that there are not forecasts supported.

@tinglis1
Copy link
Contributor Author

I will implement those changes when I get a chance. I will unlikely get to this for a week or so though.

Also I haven't setup my dev platform with tox yet, hence why i haven't done all the dev checks yet.
I currently run HA inside a docker but may have to look at using a VM for the dev instance.

@tinglis1
Copy link
Contributor Author

Made those suggested changes and fixed the linting issues.

Not sure how to fix the failed coverage/coveralls check though.

fabaff
fabaff reviewed Sep 21, 2016
View reviewed changes
.coveragerc
@@ -209,6 +209,7 @@ omit =
homeassistant/components/scene/hunterdouglas_powerview.py
homeassistant/components/sensor/arest.py
homeassistant/components/sensor/bitcoin.py
homeassistant/components/sensor/bom_weather_current.py
Copy link
Member

Choose a reason for hiding this comment

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

Needs an update as well then coveralls will be fine.

@fabaff
Copy link
Member

fabaff commented Sep 21, 2016

🐦

@fabaff fabaff merged commit f1caf3f into home-assistant:dev Sep 21, 2016
@tinglis1 tinglis1 deleted the bom_weather_current branch September 21, 2016 21:58
@tinglis1 tinglis1 mentioned this pull request Sep 25, 2016
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fabaff fabaff fabaff left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tinglis1 @balloob @fabaff