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

Method calls to async_update(), improved error handling

  • Loading branch information...
endor-force committed May 3, 2019
commit 803b562ae63685d8012c8114e498a9c7f99c01c2
@@ -57,9 +57,11 @@ def async_setup_platform(
to_station = yield from train_api.async_get_train_station(
train.get(CONF_TO))
except ValueError as station_error:
_LOGGER.error("Problem identifying station %s or %s. Error: %s ",
_LOGGER.error("Problem when trying station %s to %s. Error: %s ",
train.get(CONF_FROM), train.get(CONF_TO),
station_error)
return
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@balloob

balloob Jun 19, 2019

Member

By returning, other valid sensors are not added either.

This comment has been minimized.

Copy link
@endor-force

endor-force Jun 20, 2019

Author Contributor

Added check to see if the error is due to invalid authentication (api_key) then it is no point trying to add more.
Otherwise it would continue and move next in the loop.


sensor = TrainSensor(train_api,
train.get(CONF_NAME),
from_station,
@@ -71,11 +73,6 @@ def async_setup_platform(
async_add_devices(sensors, update_before_add=True)
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jun 19, 2019

Member
Suggested change
async_add_devices(sensors, update_before_add=True)
async_add_entities(sensors, update_before_add=True)


def dayname(day):
"""Return the short name with the weekday number as input."""
return WEEKDAYS[day]


def next_weekday(fromdate, weekday):
"""Return the date of the next time a specific weekday happen."""
days_ahead = weekday - fromdate.weekday()
@@ -88,7 +85,7 @@ def next_departuredate(departure):
"""Calculate the next departuredate from an array input of short days."""
today_weekday = date.weekday(date.today())
today_date = date.today()
if dayname(today_weekday) in departure:
if WEEKDAYS[today_weekday] in departure:
return date.today()
for day in departure:
next_departure = WEEKDAYS.index(day)
@@ -110,6 +107,8 @@ def __init__(self, train_api, name,
self._weekday = weekday
self._time = time
self._state = None
self._departure_state = None
self._delay_in_minutes = None

@asyncio.coroutine
This conversation was marked as resolved by endor-force

This comment has been minimized.

Copy link
@balloob

balloob Jun 19, 2019

Member

We've migrated to async/await syntax. Please drop this decorator and prefix async methods with async like async def async_setup_platform. Replace any yield from with await.

def async_update(self):
@@ -129,6 +128,8 @@ def async_update(self):
self._state = yield from \
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):
@@ -142,11 +143,11 @@ def device_state_attributes(self):
deviations = None
if state.deviations is not None:
deviations = ", ".join(state.deviations)
delay_in_minutes = state.get_delay_time()
if delay_in_minutes is not None:
delay_in_minutes = delay_in_minutes.total_seconds() / 60
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: delay_in_minutes,
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,
@@ -166,6 +167,6 @@ def icon(self):
@property
def state(self):
"""Return the departure state."""
if self._state is None:
return None
return self._state.get_state().name
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
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.