Navigation Menu

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

Add RMV public transport sensor #15814

Merged
merged 27 commits into from Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c3f61e0
Add new public transport sensor for RMV (Rhein-Main area).
cgtobi Aug 2, 2018
b9b59aa
Add required module.
cgtobi Aug 2, 2018
41c34ea
Fix naming problem.
cgtobi Aug 2, 2018
f9e88b6
Add unit test.
cgtobi Aug 3, 2018
221167e
Update dependency version to 0.0.5.
cgtobi Aug 3, 2018
504049b
Add new requirements.
cgtobi Aug 3, 2018
db76dd0
Fix variable name.
cgtobi Aug 3, 2018
2d9c122
Fix issues pointed out in review.
cgtobi Aug 5, 2018
485ae89
Remove unnecessary code.
cgtobi Aug 5, 2018
6fcc48d
Fix linter error.
cgtobi Aug 5, 2018
741a711
Fix config value validation.
cgtobi Aug 6, 2018
1403293
Replace minutes as state by departure timestamp. (see ##14983)
cgtobi Aug 6, 2018
f67f22e
More work on the timestamp. (see ##14983)
cgtobi Aug 7, 2018
74117c3
Revert timestamp work until #14983 gets merged.
cgtobi Aug 7, 2018
2accb78
Simplify product validation.
cgtobi Aug 7, 2018
661bad1
Remove redundant code.
cgtobi Aug 7, 2018
26f1227
Address code change requests.
cgtobi Aug 9, 2018
b714364
Address more code change requests.
cgtobi Aug 9, 2018
8be8b07
Address even more code change requests.
cgtobi Aug 9, 2018
d21cec3
Simplify destination check.
cgtobi Aug 9, 2018
4341b8a
Fix linter problem.
cgtobi Aug 9, 2018
72c3b7e
Bump dependency version to 0.0.7.
cgtobi Aug 9, 2018
9f29d30
Name variable more explicit.
cgtobi Aug 10, 2018
844c439
Only query once a minute.
cgtobi Aug 10, 2018
cacfd46
Update test case.
cgtobi Aug 10, 2018
a9949a4
Fix config validation.
cgtobi Aug 10, 2018
1642b47
Remove unneeded import.
cgtobi Aug 10, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
210 changes: 210 additions & 0 deletions homeassistant/components/sensor/rmvtransport.py
@@ -0,0 +1,210 @@
"""
Support for real-time departure information for Rhein-Main public transport.

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

Choose a reason for hiding this comment

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

The url is wrong. It should match the module name.

"""
import logging
from datetime import timedelta

Choose a reason for hiding this comment

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

'datetime.timedelta' imported but unused


import voluptuous as vol

import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity import Entity
from homeassistant.components.sensor import PLATFORM_SCHEMA
from homeassistant.const import (
CONF_NAME, ATTR_ATTRIBUTION, STATE_UNKNOWN
)

REQUIREMENTS = ['PyRMVtransport==0.0.6']

_LOGGER = logging.getLogger(__name__)

CONF_NEXT_DEPARTURE = 'nextdeparture'
Copy link
Member

Choose a reason for hiding this comment

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

CONF_NEXT_DEPARTURE = 'next_departure'


CONF_STATION = 'station'
CONF_DESTINATIONS = 'destinations'
CONF_DIRECTIONS = 'directions'
CONF_LINES = 'lines'
CONF_PRODUCTS = 'products'
Copy link
Contributor

Choose a reason for hiding this comment

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

products is weird for me, maybe vehicle?

Copy link
Contributor Author

@cgtobi cgtobi Aug 5, 2018

Choose a reason for hiding this comment

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

product is the wording taken from the providers public API and it is their product so it made sense to me to keep the wording the same. Maybe I should add some comments.

CONF_TIMEOFFSET = 'timeoffset'
Copy link
Contributor

Choose a reason for hiding this comment

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

timeoffset is not very clear for me, but I am not good at word choice as well.

Copy link
Contributor Author

@cgtobi cgtobi Aug 5, 2018

Choose a reason for hiding this comment

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

Is it not explicit enough or is the documentation lacking detailed information?

CONF_MAXJOURNEYS = 'max'
Copy link
Contributor

Choose a reason for hiding this comment

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

max what? I cannot find it in your document PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct. I added it to the documentation.


DEFAULT_NAME = 'RMV Journey'

VALID_PRODUCTS = ['U-Bahn', 'Tram', 'Bus', 'S', 'RB', 'RE', 'EC', 'IC', 'ICE']

ICONS = {
'U-Bahn': 'mdi:subway',
'Tram': 'mdi:tram',
'Bus': 'mdi:bus',
'S': 'mdi:train',
'RB': 'mdi:train',
'RE': 'mdi:train',
'EC': 'mdi:train',
'IC': 'mdi:train',
'ICE': 'mdi:train',
'SEV': 'mdi:checkbox-blank-circle-outline',
None: 'mdi:clock'
}
ATTRIBUTION = "Data provided by opendata.rmv.de"

SCAN_INTERVAL = timedelta(seconds=30)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to update more frequently than once a minute? The unit of measurement is 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.

You're probably right. Every minute should be sufficient. 30 seconds was simply to make sure not to query just before the values actually get updated and therefore miss "the bus". Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, you decide.


PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_NEXT_DEPARTURE): [{
vol.Required(CONF_STATION): cv.string,
vol.Optional(CONF_DESTINATIONS, default=['']): cv.ensure_list_csv,
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't use list csv. Just ensure list. List csv is only to allow non breaking change when migrating config schemas. Default to empty list instead.

vol.Optional(CONF_DIRECTIONS, default=['']): cv.ensure_list_csv,
vol.Optional(CONF_LINES, default=['']): cv.ensure_list_csv,
vol.Optional(CONF_PRODUCTS, default=VALID_PRODUCTS):
vol.All(cv.ensure_list, [vol.In(VALID_PRODUCTS)]),
vol.Optional(CONF_TIMEOFFSET, default=0): cv.positive_int,
vol.Optional(CONF_MAXJOURNEYS, default=5): cv.positive_int,
Copy link
Member

Choose a reason for hiding this comment

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

What do we call this in other sensor commuting platforms?

Copy link
Contributor Author

@cgtobi cgtobi Aug 9, 2018

Choose a reason for hiding this comment

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

products/CONF_PRODUCTS: MVG calls it products, which is the original name of the argument the actual HABAS backend. It is the only I found which has a similar setting.
UK transport calls it mode, which switches between bus or train.

max/CONF_MAXJOURNEYS: MVG calls it number, but I found that a bit unspecific. It is the only I found which has a similar setting.

timeoffset/CONF_TIMEOFFSET: MVG calls it timeoffset as well, Public Transit (GTFS) calls it offset and Västtrafik Public Transport calls it delay. A bit of a mix.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. max isn't so specific either I think. But I don't have a better suggestion at the moment.

It would be good to come up with a descriptive name and then have all platforms use the same term. But it can wait to a different PR.

But please separate words by underscore in variable and constant names and strings. So change to CONF_TIME_OFFSET = 'time_offset' and CONF_MAX_JOURNEYS.

vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string}]
})


def setup_platform(hass, config, add_entities, discovery_info=None):
"""Set up the RMV departure sensor."""
sensors = []
for nextdeparture in config.get(CONF_NEXT_DEPARTURE):
Copy link
Member

Choose a reason for hiding this comment

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

for next_departure in ...

sensors.append(
RMVDepartureSensor(
nextdeparture.get(CONF_STATION),
Copy link
Member

Choose a reason for hiding this comment

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

Don't use dict.get(key) for required config keys. Use dict[key].

nextdeparture.get(CONF_DESTINATIONS),
nextdeparture.get(CONF_DIRECTIONS),
nextdeparture.get(CONF_LINES),
nextdeparture.get(CONF_PRODUCTS),
nextdeparture.get(CONF_TIMEOFFSET),
nextdeparture.get(CONF_MAXJOURNEYS),
nextdeparture.get(CONF_NAME)))
add_entities(sensors, True)


class RMVDepartureSensor(Entity):
"""Implementation of an RMV departure sensor."""

def __init__(self, station, destinations, directions,
lines, products, timeoffset, maxjourneys, name):
"""Initialize the sensor."""
self._station = station
self._name = name
self.data = RMVDepartureData(station, destinations, directions,
lines, products, timeoffset, maxjourneys)
self._state = STATE_UNKNOWN
Copy link
Member

Choose a reason for hiding this comment

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

Init state as None and let that represent unknown state.

self._icon = ICONS[None]

@property
def name(self):
"""Return the name of the sensor."""
return self._name

@property
def available(self):
"""Return True if entity is available."""
return self._state != STATE_UNKNOWN
Copy link
Member

Choose a reason for hiding this comment

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

Don't use STATE_UNKNOWN, use None.

return self._state is not None

Is there no use to differ between unknown state and unavailable?

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'll check that.


@property
def state(self):
"""Return the next departure time."""
self._state = self.data.departures[0].get('minutes', None)
Copy link
Member

Choose a reason for hiding this comment

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

Remove this. It's already present in update.

return self._state

@property
def state_attributes(self):
"""Return the state attributes."""
result = {}
try:
result = {
'next_departures': [val for val in self.data.departures[1:]],
'direction': self.data.departures[0].get('direction'),
'line': self.data.departures[0].get('line'),
'minutes': self.data.departures[0].get('minutes'),
'departure_time':
self.data.departures[0].get('departure_time'),
'product': self.data.departures[0].get('product'),
}
except IndexError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

We could return empty dict here instead of defining it at the top.

return result

@property
def icon(self):
"""Icon to use in the frontend, if any."""
return self._icon

@property
def unit_of_measurement(self):
"""Return the unit this state is expressed in."""
return "min"

def update(self):
"""Get the latest data and update the state."""
self.data.update()
if not self.data.departures:
self._state = None
self._icon = ICONS[None]
return
if self._name == DEFAULT_NAME:
self._name = self.data.station
self._station = self.data.station
self._state = self.data.departures[0].get('minutes')
self._state = self.data.departures[0].get('departure_time')
Copy link
Member

Choose a reason for hiding this comment

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

Why overwrite state here? We just set it at the line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted. I already fixed that in my local branch. I simply forgot to remove the second call.

self._icon = ICONS[self.data.departures[0].get('product')]
return
Copy link
Member

Choose a reason for hiding this comment

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

Not needed return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So true. :-)



class RMVDepartureData:
"""Pull data from the opendata.rmv.de web page."""

def __init__(self, station_id, destinations, directions,
lines, products, timeoffset, maxjourneys):
"""Initialize the sensor."""
import RMVtransport
self.station = None
self._station_id = station_id
self._destinations = destinations
self._directions = directions
self._lines = lines
self._products = products
self._timeoffset = timeoffset
self._maxjourneys = maxjourneys
self.rmv = RMVtransport.RMVtransport()
self.departures = []

def update(self):
"""Update the connection data."""
try:
_data = self.rmv.get_departures(self._station_id,
products=self._products,
maxJourneys=50)
except ValueError:
self.departures = {}
Copy link
Member

Choose a reason for hiding this comment

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

We set this as a list in init. We probably shouldn't change the type.

Copy link
Contributor Author

@cgtobi cgtobi Aug 9, 2018

Choose a reason for hiding this comment

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

Well spotted. Not sure when that happened. ;)

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 rather initialise as a dict.

Copy link
Member

Choose a reason for hiding this comment

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

But we use self.departures as a list and it's updated to be a list during update.

_LOGGER.warning("Returned data not understood")
return
self.station = _data.get('station', None)
Copy link
Member

Choose a reason for hiding this comment

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

None is the default value returned when the key is missing in the dict when using dict.get.

_deps = []
for journey in _data['journeys']:
# find the first departure meeting the criteria
_nextdep = {ATTR_ATTRIBUTION: ATTRIBUTION}
if '' not in self._destinations[:1]:
Copy link
Member

Choose a reason for hiding this comment

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

Just do:

if self._destinations:

dest_found = False
for dest in self._destinations:
if dest in journey['stops']:
dest_found = True
_nextdep['destination'] = dest
if not dest_found:
continue
elif ('' not in self._lines[:1] and
Copy link
Member

Choose a reason for hiding this comment

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

elif self._lines and journey['number'] not in self._lines:

journey['number'] not in self._lines):
continue
elif journey['minutes'] < self._timeoffset:
continue
for k in ['direction', 'departure_time', 'product', 'minutes']:
Copy link
Member

Choose a reason for hiding this comment

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

for attr in ...

Copy link
Member

Choose a reason for hiding this comment

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

k doesn't explain what the variable is. Please use a descriptive variable name, eg attr.

_nextdep[k] = journey.get(k, '')
_nextdep['line'] = journey.get('number', '')
_deps.append(_nextdep)
if len(_deps) > self._maxjourneys:
break
self.departures = _deps
3 changes: 3 additions & 0 deletions requirements_all.txt
Expand Up @@ -47,6 +47,9 @@ PyMVGLive==1.1.4
# homeassistant.components.arduino
PyMata==2.14

# homeassistant.components.sensor.rmvtransport
PyRMVtransport==0.0.6

# homeassistant.components.xiaomi_aqara
PyXiaomiGateway==0.9.5

Expand Down
3 changes: 3 additions & 0 deletions requirements_test_all.txt
Expand Up @@ -24,6 +24,9 @@ HAP-python==2.2.2
# homeassistant.components.notify.html5
PyJWT==1.6.0

# homeassistant.components.sensor.rmvtransport
PyRMVtransport==0.0.6

# homeassistant.components.sonos
SoCo==0.14

Expand Down
1 change: 1 addition & 0 deletions script/gen_requirements_all.py
Expand Up @@ -77,6 +77,7 @@
'pymonoprice',
'pynx584',
'pyqwikswitch',
'PyRMVtransport',
'python-forecastio',
'python-nest',
'pytradfri\[async\]',
Expand Down