Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion homeassistant/components/sensor/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from homeassistant.helpers.config_validation import PLATFORM_SCHEMA # noqa
from homeassistant.const import (
DEVICE_CLASS_BATTERY, DEVICE_CLASS_HUMIDITY, DEVICE_CLASS_ILLUMINANCE,
DEVICE_CLASS_TEMPERATURE)
DEVICE_CLASS_TEMPERATURE, DEVICE_CLASS_TIMESTAMP)

_LOGGER = logging.getLogger(__name__)

Expand All @@ -28,6 +28,7 @@
DEVICE_CLASS_HUMIDITY, # % of humidity in the air
DEVICE_CLASS_ILLUMINANCE, # current light level (lx/lm)
DEVICE_CLASS_TEMPERATURE, # temperature (C/F)
DEVICE_CLASS_TIMESTAMP, # ISO formatted timestamp
]

DEVICE_CLASSES_SCHEMA = vol.All(vol.Lower, vol.In(DEVICE_CLASSES))
Expand Down
222 changes: 222 additions & 0 deletions homeassistant/components/sensor/rmvtransport.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
"""
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
Copy Markdown
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

Copy link
Copy Markdown

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, DEVICE_CLASS_TIMESTAMP, STATE_UNKNOWN
)

REQUIREMENTS = ['PyRMVtransport==0.0.6']

_LOGGER = logging.getLogger(__name__)

CONF_NEXT_DEPARTURE = 'nextdeparture'

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.

CONF_NEXT_DEPARTURE = 'next_departure'


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

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

@cgtobi cgtobi Aug 5, 2018

Copy link
Copy Markdown
Contributor Author

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

@cgtobi cgtobi Aug 5, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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 = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think that is right.

I used following code to test

>>> import voluptuous as vol
>>> import homeassistant.helpers.config_validation as cv
>>> LIST = ['a','b','c']
>>> schema = vol.Schema(vol.All(cv.ensure_list, [vol.In(LIST)]))
>>> schema('a')
['a']
>>> schema(['a'])
['a']
>>> schema(['a','b'])
['a', 'b']
>>> schema(['a','b','d'])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jason/ha/home-assistant/venv/lib/python3.6/site-packages/voluptuous/schema_builder.py", line 267, in __call__
    return self._compiled([], data)
  File "/home/jason/ha/home-assistant/venv/lib/python3.6/site-packages/voluptuous/validators.py", line 204, in _run
    return self._exec(self._compiled, value, path)
  File "/home/jason/ha/home-assistant/venv/lib/python3.6/site-packages/voluptuous/validators.py", line 286, in _exec
    raise e if self.msg is None else AllInvalid(self.msg, path=path)
  File "/home/jason/ha/home-assistant/venv/lib/python3.6/site-packages/voluptuous/validators.py", line 284, in _exec
    v = func(path, v)
  File "/home/jason/ha/home-assistant/venv/lib/python3.6/site-packages/voluptuous/schema_builder.py", line 641, in validate_sequence
    raise er.MultipleInvalid(errors)
voluptuous.error.MultipleInvalid: value is not allowed @ data[2]

If config validation script has issue, you may need look at that script

@cgtobi cgtobi Aug 6, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It must be a dictionary not a list. As far as I can see it is working as expected.

vol.Optional(CONF_PRODUCTS, default=DEFAULT_PRODUCT):
    vol.All(cv.ensure_list, [vol.In(VALID_PRODUCTS)]),

which uses

VALID_PRODUCTS = {'U-Bahn': ['U-Bahn'], 'Tram': ['Tram'], 'Bus': ['Bus'], 'S': ['S'], 'RB': ['RB'], 'RE': ['RE'], 'EC': ['EC'], 'IC': ['IC'], 'ICE': ['ICE']}
DEFAULT_PRODUCT = list(VALID_PRODUCTS.keys())

'U-Bahn': ['U-Bahn'],
'Tram': ['Tram'],
'Bus': ['Bus'],
'S': ['S'],
'RB': ['RB'],
'RE': ['RE'],
'EC': ['EC'],
'IC': ['IC'],
'ICE': ['ICE']
}
DEFAULT_PRODUCT = list(VALID_PRODUCTS.keys())

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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=DEFAULT_PRODUCT):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Better validate each product user configured is in the default product list

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I fixed that. Thanks for the hint.

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
Copy Markdown
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?

@cgtobi cgtobi Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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 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
Copy Markdown
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
Copy Markdown
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('departure_time', None)
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'),
'departure_time':
self.data.departures[0].get('departure_time'),
'product': self.data.departures[0].get('product'),
}
except IndexError:
pass

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 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 device_class(self):
"""Return the device class of the sensor."""
return DEVICE_CLASS_TIMESTAMP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do not do that. You should first implement it as an existing device class, wait until #14983 merged, then open anther PR to change your sensor's device class. Otherwise I have to put your PR on hold until the dependency satisfied.

I don't know when #14983 will merge.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair enough.

# 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('departure_time', None)
self._icon = ICONS[self.data.departures[0].get('product', None)]
return

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 needed return.

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

@cgtobi cgtobi Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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']:
_nextdep[k] = journey.get(k, '')
_nextdep['line'] = journey.get('number', '')
_deps.append(_nextdep)
if len(_deps) > self._maxjourneys:
break
self.departures = _deps
1 change: 1 addition & 0 deletions homeassistant/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@
DEVICE_CLASS_HUMIDITY = 'humidity'
DEVICE_CLASS_ILLUMINANCE = 'illuminance'
DEVICE_CLASS_TEMPERATURE = 'temperature'
DEVICE_CLASS_TIMESTAMP = 'timestamp'

# #### STATES ####
STATE_ON = 'on'
Expand Down
3 changes: 3 additions & 0 deletions requirements_all.txt
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
'pymonoprice',
'pynx584',
'pyqwikswitch',
'PyRMVtransport',
'python-forecastio',
'python-nest',
'pytradfri\[async\]',
Expand Down
Loading