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 Trafikverket train component #23470

Merged
merged 14 commits into from Jun 24, 2019
Merged
Changes from 11 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -612,6 +612,7 @@ omit =
homeassistant/components/trackr/device_tracker.py
homeassistant/components/tradfri/*
homeassistant/components/tradfri/light.py
homeassistant/components/trafikverket_train/sensor.py
homeassistant/components/trafikverket_weatherstation/sensor.py
homeassistant/components/transmission/*
homeassistant/components/travisci/sensor.py
@@ -230,6 +230,7 @@ homeassistant/components/toon/* @frenck
homeassistant/components/tplink/* @rytilahti
homeassistant/components/traccar/* @ludeeus
homeassistant/components/tradfri/* @ggravlingen
homeassistant/components/trafikverket_train/* @endor-force
homeassistant/components/tts/* @robbiet480
homeassistant/components/twilio_call/* @robbiet480
homeassistant/components/twilio_sms/* @robbiet480
@@ -0,0 +1 @@
"""The trafikverket_train component."""
@@ -0,0 +1,12 @@
{
"domain": "trafikverket_train",
"name": "Trafikverket train information",
"documentation": "https://www.home-assistant.io/components/trafikverket_train",
"requirements": [
"pytrafikverket==0.1.5.9"
],
"dependencies": [],
"codeowners": [
"@endor-force"
]
}
@@ -0,0 +1,185 @@
"""Train information for departures and delays, provided by Trafikverket."""

from datetime import date, datetime, timedelta
import logging

from pytrafikverket import TrafikverketTrain
import voluptuous as vol

from homeassistant.components.sensor import PLATFORM_SCHEMA
from homeassistant.const import CONF_API_KEY, CONF_NAME, CONF_WEEKDAY, WEEKDAYS
from homeassistant.helpers.aiohttp_client import async_get_clientsession
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity import Entity

_LOGGER = logging.getLogger(__name__)

CONF_TRAINS = "trains"
CONF_FROM = "from"
CONF_TO = "to"
CONF_TIME = "time"

ATTR_CANCELED = "canceled"
ATTR_DELAY_TIME = "number_of_minutes_delayed"
ATTR_PLANNED_TIME = "planned_time"
ATTR_ESTIMATED_TIME = "estimated_time"
ATTR_ACTUAL_TIME = "actual_time"
ATTR_OTHER_INFORMATION = "other_information"
ATTR_DEVIATIONS = "deviations"

ICON = "mdi:train"
SCAN_INTERVAL = timedelta(minutes=5)

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_API_KEY): cv.string,
vol.Required(CONF_TRAINS): [{
vol.Required(CONF_NAME): cv.string,
vol.Required(CONF_TO): cv.string,
vol.Required(CONF_FROM): cv.string,
vol.Optional(CONF_TIME): cv.time,
vol.Optional(CONF_WEEKDAY, default=WEEKDAYS):
vol.All(cv.ensure_list, [vol.In(WEEKDAYS)])}]
})


async def async_setup_platform(
hass, config, async_add_entities, discovery_info=None):
"""Set up the departure sensor."""
httpsession = async_get_clientsession(hass)
train_api = TrafikverketTrain(httpsession, config.get(CONF_API_KEY))
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@fabaff

fabaff May 2, 2019

Member

We should check that the credentials are valid and make the setup fail otherwise. If the API key is not valid then the users end up with a non-functional integration.

This comment has been minimized.

Copy link
@endor-force

endor-force May 2, 2019

Author Contributor

@fabaff Do you mean an internal check that we have a value (maybe matching a certain format) or full validation with at test login to the API?

This comment has been minimized.

Copy link
@endor-force

endor-force May 2, 2019

Author Contributor

Tested with an invalid key with the latest code pushed and it looks like below now..

2019-05-02 13:10:48 ERROR (MainThread) [homeassistant.components.trafikverket_train.sensor] Problem identifying station Lund C or Kävlinge. Error: Source: Authentication, message: Invalid authentication
2019-05-02 13:10:48 ERROR (MainThread) [homeassistant.components.sensor] Error while setting up platform trafikverket_train
Traceback (most recent call last):
  File "/mnt/c/download/git/home-assistant/homeassistant/helpers/entity_platform.py", line 126, in _async_setup_platform
    SLOW_SETUP_MAX_WAIT, loop=hass.loop)
  File "/usr/lib/python3.6/asyncio/tasks.py", line 358, in wait_for
    return fut.result()
  File "/mnt/c/download/git/home-assistant/homeassistant/components/trafikverket_train/sensor.py", line 65, in async_setup_platform
    from_station,
UnboundLocalError: local variable 'from_station' referenced before assignment

This comment has been minimized.

Copy link
@endor-force

endor-force May 3, 2019

Author Contributor

Improved error handling now. It will show proper error in the log but it will cancel and return instead of trying to add the sensor as it did above..

sensors = []
station_cache = {}
for train in config.get(CONF_TRAINS):
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

Use dict[key] for required config keys and keys with default config schema values.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Yep, i will fix. Should i use dict[key] for all occations where i have dict.get[key] or are there any cases where dict.get should be preferred?

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

Only use dict.get if we don't know if the key will be present in the dict or not, ie we need a default value.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Pushed, could not change for train.get(CONF_TIME) since train[CONF_TIME] would translate to train['time']

line 78, in async_setup_platform
    train[CONF_TIME])
KeyError: 'time'

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

Yeah, CONF_TIME is an optional config item, without default value.

try:
if train.get(CONF_FROM) in station_cache:
from_station = station_cache.get(train.get(CONF_FROM))
else:
from_station = await train_api.async_get_train_station(
train.get(CONF_FROM))
station_cache[train.get(CONF_FROM)] = from_station

if train.get(CONF_TO) in station_cache:
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

Looks like this block is a repetition of the block above besides replacing CONF_FROM with CONF_TO.

I suggest making another for loop that iterates over those keys.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Having only CONF_TO and CONF_FROM would not be a problem to make a for loop instead, but we also assign the output to from_station and to_station variables and i tried with a dictionary.

This works also, but it's not very easy to read? ;)

    for train in config[CONF_TRAINS]:
        try:
            trainstops = {"from_station":[train.get(CONF_FROM), None],
                          "to_station":[train.get(CONF_TO), None]}

            for station in trainstops:
                _LOGGER.error(trainstops[station][0])
                if trainstops[station][0] in station_cache:
                    trainstops[station][1] = station_cache.get(trainstops[station][0])
                else:
                    trainstops[station][1] = await train_api.async_get_train_station(trainstops[station][0])
                    station_cache[trainstops[station][0]] = trainstops[station][1]

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

We can use station_cache[train[CONF_FROM]] and station_cache[train[CONF_TO]] instead of storing in variables.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Haha yes! 🎉

            trainstops = {"from_station": train[CONF_FROM],
                          "to_station": train[CONF_TO]}

            for station in trainstops:
                if trainstops[station] not in station_cache:
                    station_cache[trainstops[station]] = await train_api.async_get_train_station(trainstops[station])

....

        sensor = TrainSensor(train_api,
                             train[CONF_NAME],
                             station_cache[train[CONF_FROM]],
                             station_cache[train[CONF_TO]],
                             train[CONF_WEEKDAY],
                             train.get(CONF_TIME))
        sensors.append(sensor)

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Pushed, but changed the dict trainstops to an array.

to_station = station_cache.get(train.get(CONF_TO))
else:
to_station = await train_api.async_get_train_station(
train.get(CONF_TO))
station_cache[train.get(CONF_TO)] = to_station

except ValueError as station_error:
station_error = str(station_error)
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

We don't need to copy to string. The exception argument has a __str__ magic method so we can use it as a logging message formatting argument directly.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 21, 2019

Author Contributor

Hmm, the reason i forced it to a string was that the lookup of text in the exception message fail otherwise.

Maybe there are other possibilities?

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 21, 2019

Author Contributor

This one works for looking up content in error message:
if "Invalid authentication" in station_error.args[0]:

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 22, 2019

Author Contributor

Pushed change

if "Invalid authentication" in station_error:
_LOGGER.error("Unable to set up up component: %s",
station_error)
return
_LOGGER.error("Problem when trying station %s to %s. Error: %s ",
train.get(CONF_FROM), train.get(CONF_TO),
station_error)
continue

sensor = TrainSensor(train_api,
train.get(CONF_NAME),
from_station,
to_station,
train.get(CONF_WEEKDAY),
train.get(CONF_TIME))
sensors.append(sensor)

async_add_entities(sensors, update_before_add=True)


def next_weekday(fromdate, weekday):
"""Return the date of the next time a specific weekday happen."""
days_ahead = weekday - fromdate.weekday()
if days_ahead <= 0:
days_ahead += 7
return fromdate + timedelta(days_ahead)


def next_departuredate(departure):
"""Calculate the next departuredate from an array input of short days."""
today_date = date.today()
today_weekday = date.weekday(today_date)
if WEEKDAYS[today_weekday] in departure:
return today_date
for day in departure:
next_departure = WEEKDAYS.index(day)
if next_departure > today_weekday:
return next_weekday(today_date, next_departure)
return next_weekday(today_date, WEEKDAYS.index(departure[0]))


class TrainSensor(Entity):
"""Contains data about a train depature."""

def __init__(self, train_api, name,
from_station, to_station, weekday, time):
"""Initialize the sensor."""
self._train_api = train_api
self._name = name
self._from_station = from_station
self._to_station = to_station
self._weekday = weekday
self._time = time
self._state = None
self._departure_state = None
self._delay_in_minutes = None

async def async_update(self):
"""Retrieve latest state."""
if self._time is not None:
departure_day = next_departuredate(self._weekday)
when = datetime.combine(departure_day, self._time)
try:
self._state = await \
self._train_api.async_get_train_stop(
self._from_station, self._to_station, when)
except ValueError as output_error:
_LOGGER.error("Departure %s encountered a problem: %s",
when, output_error)
else:
when = datetime.now()
self._state = await \
self._train_api.async_get_next_train_stop(
self._from_station, self._to_station, when)
self._departure_state = self._state.get_state().name
self._delay_in_minutes = self._state.get_delay_time()

@property
def device_state_attributes(self):
"""Return the state attributes."""
if self._state is None:
return None
state = self._state
other_information = None
if state.other_information is not None:
other_information = ", ".join(state.other_information)
deviations = None
if state.deviations is not None:
deviations = ", ".join(state.deviations)
if self._delay_in_minutes is not None:
self._delay_in_minutes = \
self._delay_in_minutes.total_seconds() / 60
return {ATTR_CANCELED: state.canceled,
ATTR_DELAY_TIME: self._delay_in_minutes,
ATTR_PLANNED_TIME: state.advertised_time_at_location,
ATTR_ESTIMATED_TIME: state.estimated_time_at_location,
ATTR_ACTUAL_TIME: state.time_at_location,
ATTR_OTHER_INFORMATION: other_information,
ATTR_DEVIATIONS: deviations}

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

@property
def icon(self):
"""Return the icon for the frontend."""
return ICON

@property
def state(self):
"""Return the departure state."""
if self._state is not None:
return self._departure_state
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member

What type of data are we returning here? Is it a datetime string?

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

States like this: on_time, delayed, canceled
Times are in attributes.
image

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

You could consider changing state to planned time of departure and use device class timestamp. It will then show as a relative time until departure in the frontend.

What's the most useful state info out of all the attributes?

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Yes, that is a really neat feature.. I would say both values are equally important. The current setup will highlight if everything is ok or not,

Having time as state would need some additional logic to show the state since planned time is the timetable time and would not be valid to show as state if the train is delayed or canceled.

If the train is delayed and have an estimated time, this value will be in a separate attribute.
image

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

Yeah, timestamp state will be a bit more complex. I wanted to mention it at least.

This comment has been minimized.

Copy link
@balloob

balloob Jun 20, 2019

Member

Timestamp is I think the way to go. A sensor that will always show the timestamp of the next time the train leaves.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Hmm, yes, it is appealing at first. But given the fact how trains work in south of sweden there is most likely a delay on daily basis and then the planned time is no longer valid but the estimated time is the one to aim for.
However, it is only an estimate and the train can arrive earlier in my experience, even though they often arrive later ;)

So if we were to show time in state, if there is a delay and an estimated time we should then update and set the state to the estimated time instead. It might look better and make more sense in the ui showing the relative time to departure.,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 20, 2019

Member

Sweden 2.0. 😆

This comment has been minimized.

Copy link
@balloob

balloob Jun 20, 2019

Member

If you set the time to a timestamp, users in the UI can choose how to show the time as they want. Time until it happens, just the time etc. It gives users the most flexibility.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 22, 2019

Author Contributor

Looks neat in the UI with the time.
I have added the device class as timestamp and moved the previous state to departure_state attribute.

@property
 def state(self):
     """Return the departure state."""
     state = self._state
     if state is not None:
         if state.time_at_location is not None:
             return state.time_at_location
         elif state.estimated_time_at_location is not None:
             return state.estimated_time_at_location
         return state.advertised_time_at_location
     return None

image

And if the train is delayed it will show the estimated time..

image

However, if the departure is cancelled it will still show the planned time, not sure what the best way would be to show as state in that case.. Maybe that is up to the user, based on the attribute canceled and departure_state..

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 22, 2019

Member

Very nice! Yes, the user should decide what to do with the cancel attribute. We should keep the main state consistent in terms of type. It should always be a timepoint now.

return None
@@ -1440,6 +1440,7 @@ pytrackr==0.0.5
# homeassistant.components.tradfri
pytradfri[async]==6.0.1

# homeassistant.components.trafikverket_train
# homeassistant.components.trafikverket_weatherstation
pytrafikverket==0.1.5.9

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.