Skip to content

Rewrite of Trafikverket weather - Multiple sensor types supported#15935

Merged
MartinHjelmare merged 11 commits intohome-assistant:devfrom
endor-force:update_trafikverketsensor
Aug 27, 2018
Merged

Rewrite of Trafikverket weather - Multiple sensor types supported#15935
MartinHjelmare merged 11 commits intohome-assistant:devfrom
endor-force:update_trafikverketsensor

Conversation

@endor-force
Copy link
Copy Markdown
Contributor

@endor-force endor-force commented Aug 12, 2018

Description:

Rewritten with library pytrafikverket for retreiving data.

Breaking change, requires user to update configuration!
Instead of having multiple sensor configurations per station, add only one configuration per station and select the type of measurement data to subscribe to using monitored_conditions.
The configuration value type should no longer be used.

Supports more weather data from Trafikverket API.
(Text values returned from API is string in Swedish (wind_direction_text and precipitation).)

Related issue (if applicable): fixes N/A

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

Example entry for configuration.yaml (if applicable):

- sensor: 
   - platform: trafikverket_weatherstation
     name: Trafikverket Precipitation WeatherStation Upplid
     api_key: eXXcbXXXacXXXXc39XX3aXXX4aXX46XX
     station: Upplid
     monitored_conditions:
         - air_temp
         - road_temp
         - humidity
         - precipitation
         - wind_direction
         - wind_direction_text
         - wind_speed

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.
  • New files were added to .coveragerc. N/A

If the code does not interact with devices:

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

Enables users to see type of precipitation.
Value returned from API is a string in swedish.
@homeassistant homeassistant added cla-signed merging-to-master This PR is merging into the master branch and should probably change the branch to `dev`. platform: sensor.trafikverket_weatherstation small-pr PRs with less than 30 lines. labels Aug 12, 2018
@ghost ghost added the in progress label Aug 12, 2018
@endor-force endor-force changed the base branch from master to dev August 12, 2018 10:34
@endor-force
Copy link
Copy Markdown
Contributor Author

Moved from master to dev, tag "merging to master" can be cleared.

@MartinHjelmare MartinHjelmare removed the merging-to-master This PR is merging into the master branch and should probably change the branch to `dev`. label Aug 12, 2018
Correction of tox findings
add_devices([TrafikverketWeatherStation(
sensor_name, sensor_api, sensor_station, sensor_type)], True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

Travis tox failed due to problem with tox build process. 
Correcting in a comment to trigger retry in travis..

# air_vs_road contains "Air" or "Road" depending on user input.
self._state = final[air_vs_road]["Temp"]
# air_vs_road contains "Air", "Road" or "Precipitation" depending on user input as well as suffix key.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (110 > 79 characters)


# air_vs_road contains "Air" or "Road" depending on user input.
self._state = final[air_vs_road]["Temp"]
# air_vs_road contains "Air", "Road" or "Precipitation"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

trailing whitespace

<EQ name="Name" value='""" + self._station + """' />
</FILTER>
<INCLUDE>Measurement.""" + air_vs_road + """.Temp</INCLUDE>
<INCLUDE>Measurement.""" + air_vs_road + """.""" + keytype + """</INCLUDE>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We cannot accept any PR for this platform until all protocol specific code has been extracted into a third party lib which is then used.

fabaff
fabaff previously approved these changes Aug 14, 2018
Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Conflict

@fabaff fabaff dismissed their stale review August 14, 2018 10:11

Superseded

@endor-force
Copy link
Copy Markdown
Contributor Author

Sorry, rookie here, do you know any sensor that I can look at for reference on putting protocol code in separate library?

@fabaff
Copy link
Copy Markdown
Member

fabaff commented Aug 14, 2018

luftnetz, pi_hole, shodan, swiss_public_transportation, thinkingcleaner, vasttrafik and some others.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Aug 15, 2018

@endor-force
Copy link
Copy Markdown
Contributor Author

Sure, I will investigate the possibilities of using and extending pytrafikverket for this.

Extended pytrafikverket with weather sensor collction
Changed behaviour of sensor component to use pytrafikverket.
Added more sensors.

User need to change config to use new version.
 [] Documentation needs to be updated
"""Set up the Trafikverket sensor platform."""
from pytrafikverket.trafikverket_weather import TrafikverketWeather

sensor_name = config.get(CONF_NAME)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use dict[key] for required config keys.



def setup_platform(hass, config, add_entities, discovery_info=None):
async def async_setup_platform(hass, config, async_add_devices,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rename async_add_devices to async_add_entities.

self._name = SENSOR_TYPES[sensor_type][0]
self._type = sensor_type
self._state = None
self._state = STATE_UNKNOWN
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Init state to None.

# air_vs_road contains "Air" or "Road" depending on user input.
self._state = final[air_vs_road]["Temp"]
self._weather = await\
self._weather_api.async_get_weather(self._station)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Break the line after the parenthesis instead.

self._state = None
self._state = STATE_UNKNOWN
self._unit = SENSOR_TYPES[sensor_type][1]
self._code = None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not used.

Appreciate the feedback
@endor-force
Copy link
Copy Markdown
Contributor Author

Updated and smoke tested with some scenarios for errors.

[ ] Documentation needs to be updated since it is a change in how to configure
I will fix that within short

@MartinHjelmare
Copy link
Copy Markdown
Member

Please also add a paragraph in the PR description about the breaking change for the release notes.

Copy link
Copy Markdown
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.

Ping me when description is updated and docs done.

@endor-force endor-force changed the title Added precipitation type from API Rewrite of Trafikverket weather - Multiple sensor types added Aug 26, 2018
@endor-force endor-force changed the title Rewrite of Trafikverket weather - Multiple sensor types added Rewrite of Trafikverket weather - Multiple sensor types supported Aug 26, 2018
@endor-force
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare
I have updated existing PR for the doc and updated the description to accommodate for the changes.

@MartinHjelmare
Copy link
Copy Markdown
Member

Please write what has changed regarding configuration.

@endor-force
Copy link
Copy Markdown
Contributor Author

Done 👍

@MartinHjelmare MartinHjelmare merged commit a439690 into home-assistant:dev Aug 27, 2018
@ghost ghost removed the in progress label Aug 27, 2018
@endor-force endor-force deleted the update_trafikverketsensor branch August 27, 2018 10:32
@tubalainen
Copy link
Copy Markdown
Contributor

This is sooo great! Thanks guys! Cannot wait for the release! <3
Tack @MartinHjelmare !

@balloob balloob mentioned this pull request Sep 17, 2018
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants